Date:   Thu, 18 Aug 2022 13:48:26 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
cc:     m.chetan.kumar@...el.com, Netdev <netdev@...r.kernel.org>,
        kuba@...nel.org, davem@...emloft.net, johannes@...solutions.net,
        ryazanov.s.a@...il.com, loic.poulain@...aro.org,
        krishna.c.sudi@...el.com, linuxwwan@...el.com,
        Haijun Liu <haijun.liu@...iatek.com>,
        Madhusmita Sahu <madhusmita.sahu@...el.com>,
        Ricardo Martinez <ricardo.martinez@...ux.intel.com>,
        Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>
Subject: Re: [PATCH net-next 2/5] net: wwan: t7xx: Infrastructure for early
 port configuration

On Wed, 17 Aug 2022, Kumar, M Chetan wrote:

> 
> On 8/17/2022 5:40 PM, Ilpo Järvinen wrote:
> > On Tue, 16 Aug 2022, m.chetan.kumar@...el.com wrote:
> > 
> > > From: Haijun Liu <haijun.liu@...iatek.com>
> > > 
> > > To support cases such as FW update or Core dump, the t7xx device
> > > is capable of signaling the host that a special port needs
> > > to be created before the handshake phase.
> > > 
> > > This patch adds the infrastructure required to create the
> > > early ports which also requires a different configuration of
> > > CLDMA queues.
> > > 
> > > Signed-off-by: Haijun Liu <haijun.liu@...iatek.com>
> > > Co-developed-by: Madhusmita Sahu <madhusmita.sahu@...el.com>
> > > Signed-off-by: Madhusmita Sahu <madhusmita.sahu@...el.com>
> > > Signed-off-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
> > > Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>
> > > Signed-off-by: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
> > > ---
> > 
> > > diff --git a/drivers/net/wwan/t7xx/t7xx_port.h
> > > b/drivers/net/wwan/t7xx/t7xx_port.h
> > > index 4a29bd04cbe2..6a96ee6d9449 100644
> > > --- a/drivers/net/wwan/t7xx/t7xx_port.h
> > > +++ b/drivers/net/wwan/t7xx/t7xx_port.h
> > > @@ -100,6 +100,7 @@ struct t7xx_port_conf {
> > >   	struct port_ops		*ops;
> > >   	char			*name;
> > >   	enum wwan_port_type	port_type;
> > > +	bool			is_early_port;
> > >   };
> > >     struct t7xx_port {
> > > @@ -130,9 +131,11 @@ struct t7xx_port {
> > >   	struct task_struct		*thread;
> > >   };
> > >   +int t7xx_get_port_mtu(struct t7xx_port *port);
> > >   struct sk_buff *t7xx_port_alloc_skb(int payload);
> > >   struct sk_buff *t7xx_ctrl_alloc_skb(int payload);
> > >   int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb);
> > > +int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff *skb);
> > >   int t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb,
> > > unsigned int pkt_header,
> > >   		       unsigned int ex_msg);
> > >   int t7xx_port_send_ctl_skb(struct t7xx_port *port, struct sk_buff *skb,
> > > unsigned int msg,
> > > diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> > > b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> > > index 62305d59da90..7582777cf94d 100644
> > > --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> > > +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> > > @@ -88,6 +88,20 @@ static const struct t7xx_port_conf t7xx_md_port_conf[]
> > > = {
> > >   	},
> > >   };
> > >   +static struct t7xx_port_conf t7xx_early_port_conf[] = {
> > > +	{
> > > +		.tx_ch = 0xffff,
> > > +		.rx_ch = 0xffff,
> > > +		.txq_index = 1,
> > > +		.rxq_index = 1,
> > > +		.txq_exp_index = 1,
> > > +		.rxq_exp_index = 1,
> > > +		.path_id = CLDMA_ID_AP,
> > > +		.is_early_port = true,
> > > +		.name = "ttyDUMP",
> > > +	},
> > > +};
> > > +
> > >   static struct t7xx_port *t7xx_proxy_get_port_by_ch(struct port_proxy
> > > *port_prox, enum port_ch ch)
> > >   {
> > >   	const struct t7xx_port_conf *port_conf;
> > > @@ -202,7 +216,17 @@ int t7xx_port_enqueue_skb(struct t7xx_port *port,
> > > struct sk_buff *skb)
> > >   	return 0;
> > >   }
> > >   -static int t7xx_port_send_raw_skb(struct t7xx_port *port, struct
> > > sk_buff *skb)
> > > +int t7xx_get_port_mtu(struct t7xx_port *port)
> > > +{
> > > +	enum cldma_id path_id = port->port_conf->path_id;
> > > +	int tx_qno = t7xx_port_get_queue_no(port);
> > > +	struct cldma_ctrl *md_ctrl;
> > > +
> > > +	md_ctrl = port->t7xx_dev->md->md_ctrl[path_id];
> > > +	return md_ctrl->tx_ring[tx_qno].pkt_size;
> > > +}
> > > +
> > > +int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff *skb)
> > 
> > Why you removed static from this function here (+add the prototype into a
> > header), I cannot see anything in this patch. Perhaps those changes belong
> > to patch 4?
> 
> Prototype is added in header file. Patch4 is using this func.

Thus, put those two changes (proto + static removal) into patch 4.

> > >   {
> > >   	enum cldma_id path_id = port->port_conf->path_id;
> > >   	struct cldma_ctrl *md_ctrl;
> > > @@ -317,6 +341,26 @@ static void t7xx_proxy_setup_ch_mapping(struct
> > > port_proxy *port_prox)
> > >   	}
> > >   }
> > >   +static int t7xx_port_proxy_recv_skb_from_queue(struct t7xx_pci_dev
> > > *t7xx_dev,
> > > +					       struct cldma_queue *queue,
> > > struct sk_buff *skb)
> > > +{
> > > +	struct port_proxy *port_prox = t7xx_dev->md->port_prox;
> > > +	const struct t7xx_port_conf *port_conf;
> > > +	struct t7xx_port *port;
> > > +	int ret;
> > > +
> > > +	port = port_prox->ports;
> > > +	port_conf = port->port_conf;
> > > +
> > > +	ret = port_conf->ops->recv_skb(port, skb);
> > > +	if (ret < 0 && ret != -ENOBUFS) {
> > > +		dev_err(port->dev, "drop on RX ch %d, %d\n", port_conf->rx_ch,
> > > ret);
> > > +		dev_kfree_skb_any(skb);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   static struct t7xx_port *t7xx_port_proxy_find_port(struct t7xx_pci_dev
> > > *t7xx_dev,
> > >   						   struct cldma_queue *queue,
> > > u16 channel)
> > >   {
> > > @@ -338,6 +382,22 @@ static struct t7xx_port
> > > *t7xx_port_proxy_find_port(struct t7xx_pci_dev *t7xx_dev
> > >   	return NULL;
> > >   }
> > >   +struct t7xx_port *t7xx_port_proxy_get_port_by_name(struct port_proxy
> > > *port_prox, char *port_name)
> > > +{
> > > +	const struct t7xx_port_conf *port_conf;
> > > +	struct t7xx_port *port;
> > > +	int i;
> > > +
> > > +	for_each_proxy_port(i, port, port_prox) {
> > > +		port_conf = port->port_conf;
> > > +
> > > +		if (!strncmp(port_conf->name, port_name,
> > > strlen(port_conf->name)))
> > > +			return port;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > >   /**
> > >    * t7xx_port_proxy_recv_skb() - Dispatch received skb.
> > >    * @queue: CLDMA queue.
> > > @@ -358,6 +418,9 @@ static int t7xx_port_proxy_recv_skb(struct cldma_queue
> > > *queue, struct sk_buff *s
> > >   	u16 seq_num, channel;
> > >   	int ret;
> > >   +	if (queue->q_type == CLDMA_DEDICATED_Q)
> > > +		return t7xx_port_proxy_recv_skb_from_queue(t7xx_dev, queue,
> > > skb);
> > > +
> > 
> > So ->recv_skb() is per cldma but now you'd actually want to have a
> > different one per queue?
> 
> dump and download port uses different configuration (packet size is
> different, received data doesn't contain header portion) so using q_type
> to distinguish rx flow.

If you want to distinguish something by q_type and already have that func 
ptr for recv_skb() anyway, why not move the recv_skb() ptr to cldma_queue 
so you don't need to add any this kind of additional conditions to just 
call another kind of handler function?

> > >   	ret = port_conf->ops->recv_skb(port, skb);
> > >   	/* Error indicates to try again later */
> > > @@ -439,26 +503,58 @@ static void t7xx_proxy_init_all_ports(struct
> > > t7xx_modem *md)
> > >   	t7xx_proxy_setup_ch_mapping(port_prox);
> > >   }
> > >   +void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id
> > > cfg_id)
> > > +{
> > > +	struct port_proxy *port_prox = md->port_prox;
> > > +	const struct t7xx_port_conf *port_conf;
> > > +	struct device *dev = port_prox->dev;
> > > +	unsigned int port_count;
> > > +	struct t7xx_port *port;
> > > +	int i;
> > > +
> > > +	if (port_prox->cfg_id == cfg_id)
> > > +		return;
> > > +
> > > +	if (port_prox->cfg_id != PORT_CFG_ID_INVALID) {
> > 
> > This seems to be always true.
> 
> In initialization flow it would be false.
> Depending on boot stage, right port config would be chosen and
> cfg_id reflects the chosen config.

Oh, I think I misunderstood the code here quite badly, so it depends on 
the initial value being 0?

But now I realize there's also the preceeding check that returns. ...So 
doesn't that then mean port_prox->cfg_id can only be PORT_CFG_ID_INVALID 
at this point or am I still missing something (making the whole block 
unnecessary)?

> > > +		for_each_proxy_port(i, port, port_prox)
> > > +			port->port_conf->ops->uninit(port);

This would be t7xx_port_proxy_uninit() I think.

> > > +		dev_err(dev, "invalid device status\n");
> > > +		ret = -EINVAL;
> > > +		goto finish_command;
> > > +	}
> > > +
> > >   	ctl->curr_state = FSM_STATE_PRE_START;
> > >   	t7xx_md_event_notify(md, FSM_PRE_START);
> > >   -	ret = read_poll_timeout(ioread32, dev_status,
> > > -				(dev_status & MISC_STAGE_MASK) == LINUX_STAGE,
> > > 20000, 2000000,
> > > -				false, IREG_BASE(md->t7xx_dev) +
> > > T7XX_PCIE_MISC_DEV_STATUS);
> > > -	if (ret) {
> > > -		struct device *dev = &md->t7xx_dev->pdev->dev;
> > > +	device_stage = FIELD_GET(MISC_STAGE_MASK, dev_status);
> > > +	if (dev_status == ctl->prev_dev_status) {
> > 
> > Maybe the local variables from need dev_ or device_ prefixes. They just
> > makes them harder to read.
> 
> Ok. will consider it.

I had quite bad English there, just to be sure we understood it anyway,
I meant I don't think those prefixes benefit anything and could be 
removed.

> > > +		if (ctl->device_stage_check_cnt++ >= DEVICE_STAGE_POLL_COUNT)
> > > {
> > > +			dev_err(dev, "Timeout at device stage 0x%x\n",
> > > device_stage);
> > > +			ctl->device_stage_check_cnt = 0;
> > > +			ret = -ETIMEDOUT;
> > > +		} else {
> > > +			msleep(DEVICE_STAGE_POLL_INTERVAL_MS);
> > > +			ret = t7xx_fsm_append_cmd(ctl, FSM_CMD_START, 0);
> > 
> > I'm somewhat skeptical about this. Could this contain a race that results
> > in skipping over a stage?
> 
> Ideally it should not skip the stage. The device_stage would reflect the
> actual device boot stage i.e. BROM -> PL -> LK -> Linux.
> 
> Just in case, when the next device stage has not changed it poll's for a
> certain interval and returns ETIMEDOUT value to fsm cmd waiter.

What I tried to say is this:

To get to this else branch, the stage has not advanced from previous one 
(and poll count is not yet exhausted). It does sleep + FSM_CMD_START. Is 
there something that guarantees that the stage doesn't advance during 
sleep (note how stage is not reread at this point) such that FSM_CMD_START 
is used when already in the next stage (that could advance stage again, 
thus "skipping" a stage)?

Perhaps this doesn't matter.

> > > +		}
> > >   -		fsm_finish_command(ctl, cmd, -ETIMEDOUT);
> > > -		dev_err(dev, "Invalid device status 0x%lx\n", dev_status &
> > > MISC_STAGE_MASK);
> > > -		return;
> > > +		goto finish_command;
> > > +	}
> > > +
> > > +	switch (device_stage) {
> > > +	case INIT_STAGE:
> > > +	case PRE_BROM_STAGE:
> > > +	case POST_BROM_STAGE:
> > > +		ret = t7xx_fsm_append_cmd(ctl, FSM_CMD_START, 0);
> > > +		break;
> > > +
> > > +	case LK_STAGE:
> > > +		dev_info(dev, "LK_STAGE Entered");
> > > +		t7xx_lk_stage_event_handling(ctl, dev_status);
> > > +		break;
> > > +
> > > +	case LINUX_STAGE:
> > > +		t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
> > > +		t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
> > > +		t7xx_port_proxy_set_cfg(md, PORT_CFG_ID_NORMAL);
> > > +		ret = fsm_routine_starting(ctl);
> > > +		break;
> > > +
> > > +	default:
> > > +		break;
> > >   	}
> > >   -	t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
> > > -	t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
> > > -	fsm_finish_command(ctl, cmd, fsm_routine_starting(ctl));
> > > +finish_command:
> > > +	ctl->prev_dev_status = dev_status;
> > > +	fsm_finish_command(ctl, cmd, ret);
> > >   }
> > >     static int fsm_main_thread(void *data)
> > 
> > 
> 
> 

-- 
 i.

Powered by blists - more mailing lists