Re: [dpdk-dev] [PATCH v2 2/4] net/mrvl: add mrvl net pmd driver

[Tomasz Duszynski <tdu@semihalf.com>]

On Fri, Sep 29, 2017 at 08:38:00AM -0700, Stephen Hemminger wrote:
> On Thu, 28 Sep 2017 12:22:36 +0200
> Tomasz Duszynski <tdu@semihalf.com> wrote:
>
> > +
> > +struct mrvl_rxq;
> > +struct mrvl_txq;
>
> These forward decl should not be nececessary
ACK
> > +static inline int
> > +mrvl_get_bpool_size(int	pp2_id, int pool_id)
>  No tab here please
Good catch, thanks.
>
> Why does this need to be inlined?  Is it in critical path?
>
>
Right, in rx handler precisely.
> > +{
> > +	int i;
> > +	int size = 0;
> > +
> > +	for (i = mrvl_lcore_first; i <= mrvl_lcore_last; i++)
> > +		size += mrvl_port_bpool_size[pp2_id][pool_id][i];
> > +
> > +	return size;
> > +}
> > +
>
> Also, I prefer that the following restrictions from the kernel be
> also applied to DPDK code.
>
> ### [dpdk-dev] [PATCH v2 2/4] net/mrvl: add mrvl net pmd driver
>
> CHECK:LINE_SPACING: Please don't use multiple blank lines
> #452: FILE: drivers/net/mrvl/mrvl_ethdev.c:180:
> +
> +
>
> WARNING:LINE_SPACING: Missing a blank line after declarations
> #457: FILE: drivers/net/mrvl/mrvl_ethdev.c:185:
> +	int n = sizeof(*bitmap) * 8 - __builtin_clz(*bitmap);
> +	if (n >= max)
These were not triggered by the DPDK checkpatch wrapper. On the other
hand using kernel checkpatch will show them. That makes we wonder which
tool should actually be used? Anyway, will fix that in v3.
>
> CHECK:LINE_SPACING: Please don't use multiple blank lines
> #562: FILE: drivers/net/mrvl/mrvl_ethdev.c:290:
> +
> +
>
> WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'priv->ppio_params.inqs_params.tcs_params[i].inqs_params'
> #880: FILE: drivers/net/mrvl/mrvl_ethdev.c:608:
> +			rte_free(priv->ppio_params.inqs_params.
> +				tcs_params[i].inqs_params);
>
> WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'priv->ppio_params.inqs_params.tcs_params[i].inqs_params'
> #882: FILE: drivers/net/mrvl/mrvl_ethdev.c:610:
> +			priv->ppio_params.inqs_params.
> +				tcs_params[i].inqs_params = NULL;
>
> WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'priv->ppio_params.inqs_params.tcs_params[tc].inqs_params[inq].size'
> #1330: FILE: drivers/net/mrvl/mrvl_ethdev.c:1058:
> +	qinfo->nb_desc = priv->ppio_params.inqs_params.
> +			 tcs_params[tc].inqs_params[inq].size;
>
> WARNING:SPLIT_STRING: quoted string split across lines
> #1476: FILE: drivers/net/mrvl/mrvl_ethdev.c:1204:
> +		RTE_LOG(ERR, PMD, "Mbuf size must be increased to %u bytes"
> +				  " to hold up to %u bytes of data.\n",
Personally I would go with kernel checkpatch, but DPDK checker complained
about overly long lines thus such comments were split across lines.
>
> WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'priv->ppio_params.inqs_params.tcs_params[priv->rxq_map[rxq->queue_id].tc'
> #1500: FILE: drivers/net/mrvl/mrvl_ethdev.c:1228:
> +	priv->ppio_params.inqs_params.
> +		tcs_params[priv->rxq_map[rxq->queue_id].tc].
>
> WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'q->priv->ppio_params.inqs_params.tcs_params[q->priv->rxq_map[q->queue_id].tc'
> #1532: FILE: drivers/net/mrvl/mrvl_ethdev.c:1260:
> +	num = q->priv->ppio_params.inqs_params.
> +			tcs_params[q->priv->rxq_map[q->queue_id].tc].
>
> WARNING:SPLIT_STRING: quoted string split across lines
> #1902: FILE: drivers/net/mrvl/mrvl_ethdev.c:1630:
> +			RTE_LOG(DEBUG, PMD, "\nport-%d:%d: bpool %d oversize -"
> +				" remove %d buffers (pool size: %d -> %d)\n",
>
> WARNING:SPLIT_STRING: quoted string split across lines
> #2094: FILE: drivers/net/mrvl/mrvl_ethdev.c:1822:
> +			"No room in shadow queue for %d packets!!!"
> +			"%d packets will be sent.\n",
>
> CHECK:LINE_SPACING: Please don't use multiple blank lines
> #2294: FILE: drivers/net/mrvl/mrvl_ethdev.c:2022:
> +
> +
>
> CHECK:LINE_SPACING: Please don't use multiple blank lines
> #2595: FILE: drivers/net/mrvl/mrvl_ethdev.h:40:
> +
> +
>
> WARNING:LINE_SPACING: Missing a blank line after declarations
> #3253: FILE: drivers/net/mrvl/mrvl_qos.c:577:
> +			uint8_t idx = port_cfg->tc[tc].inq[i];
> +			priv->rxq_map[idx].tc = tc;
>

--
- Tomasz Duszyński