Re: [EXTERNAL] Re: [PATCH v8 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Simon Horman]

Hi Anwar,

On Wed, Jul 12, 2023 at 05:55:57PM +0530, Anwar, Md Danish wrote:
> Hi Simon
> On 7/11/2023 11:15 PM, Simon Horman wrote:
> > On Mon, Jul 10, 2023 at 11:05:50AM +0530, MD Danish Anwar wrote:
> > > From: Roger Quadros <rogerq@xxxxxx>

...

> > > +static void icssg_miig_queues_init(struct prueth *prueth, int slice)
> > > +{
> > > +	struct regmap *miig_rt = prueth->miig_rt;
> > > +	void __iomem *smem = prueth->shram.va;
> > > +	u8 pd[ICSSG_SPECIAL_PD_SIZE];
> > > +	int queue = 0, i, j;
> > > +	u32 *pdword;
> > > +
> > > +	/* reset hwqueues */
> > > +	if (slice)
> > > +		queue = ICSSG_NUM_TX_QUEUES;
> > > +
> > > +	for (i = 0; i < ICSSG_NUM_TX_QUEUES; i++) {
> > > +		regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue);
> > > +		queue++;
> > > +	}
> > > +
> > > +	queue = slice ? RECYCLE_Q_SLICE1 : RECYCLE_Q_SLICE0;
> > > +	regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue);
> > > +
> > > +	for (i = 0; i < ICSSG_NUM_OTHER_QUEUES; i++) {
> > > +		regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET,
> > > +			     hwq_map[slice][i].queue);
> > > +	}
> > > +
> > > +	/* initialize packet descriptors in SMEM */
> > > +	/* push pakcet descriptors to hwqueues */
> > > +
> > > +	pdword = (u32 *)pd;
> > > +	for (j = 0; j < ICSSG_NUM_OTHER_QUEUES; j++) {
> > > +		const struct map *mp;
> > > +		int pd_size, num_pds;
> > > +		u32 pdaddr;
> > > +
> > > +		mp = &hwq_map[slice][j];
> > > +		if (mp->special) {
> > > +			pd_size = ICSSG_SPECIAL_PD_SIZE;
> > > +			num_pds = ICSSG_NUM_SPECIAL_PDS;
> > > +		} else	{
> > > +			pd_size = ICSSG_NORMAL_PD_SIZE;
> > > +			num_pds = ICSSG_NUM_NORMAL_PDS;
> > > +		}
> > > +
> > > +		for (i = 0; i < num_pds; i++) {
> > > +			memset(pd, 0, pd_size);
> > > +
> > > +			pdword[0] &= cpu_to_le32(ICSSG_FLAG_MASK);
> > > +			pdword[0] |= cpu_to_le32(mp->flags);
> > 
> > Sparse warns that the endieness of pdword is not le32.
> 
> I will fix this.

Thanks.

> > There are also other sparse warnings added by this patch.
> > Please look over them.
> 
> There is one more warning for "expected restricted __le16 [usertype]
> rx_base_flow got restricted __le32 [usertype]". I will fix this as well.

I haven't looked carefully through these.
But for the record, this is what Sparse tells me:

  .../icssg_config.c:91:18: warning: symbol 'hwq_map' was not declared. Should it be static?
  .../icssg_config.c:189:35: warning: invalid assignment: &=
  .../icssg_config.c:189:35:    left side has type unsigned int
  .../icssg_config.c:189:35:    right side has type restricted __le32
  .../icssg_config.c:190:35: warning: invalid assignment: |=
  .../icssg_config.c:190:35:    left side has type unsigned int
  .../icssg_config.c:190:35:    right side has type restricted __le32
  .../icssg_config.c:225:11: warning: incorrect type in assignment (different address spaces)
  .../icssg_config.c:225:11:    expected struct icssg_r30_cmd *p
  .../icssg_config.c:225:11:    got void [noderef] __iomem *
  .../icssg_config.c:228:42: warning: incorrect type in argument 2 (different address spaces)
  .../icssg_config.c:228:42:    expected void volatile [noderef] __iomem *addr
  .../icssg_config.c:228:42:    got unsigned int *
  .../icssg_config.c:237:11: warning: incorrect type in assignment (different address spaces)
  .../icssg_config.c:237:11:    expected struct icssg_r30_cmd const *p
  .../icssg_config.c:237:11:    got void [noderef] __iomem *
  .../icssg_config.c:240:36: warning: incorrect type in argument 1 (different address spaces)
  .../icssg_config.c:240:36:    expected void const volatile [noderef] __iomem *addr
  .../icssg_config.c:240:36:    got unsigned int const *
  .../icssg_config.c:270:19: warning: incorrect type in assignment (different address spaces)
  .../icssg_config.c:270:19:    expected struct icssg_buffer_pool_cfg *bpool_cfg
  .../icssg_config.c:270:19:    got void [noderef] __iomem *
  .../icssg_config.c:289:17: warning: incorrect type in assignment (different address spaces)
  .../icssg_config.c:289:17:    expected struct icssg_rxq_ctx *rxq_ctx
  .../icssg_config.c:289:17:    got void [noderef] __iomem *
  .../icssg_config.c:297:17: warning: incorrect type in assignment (different address spaces)
  .../icssg_config.c:297:17:    expected struct icssg_rxq_ctx *rxq_ctx
  .../icssg_config.c:297:17:    got void [noderef] __iomem *
  .../icssg_config.c:325:38: warning: incorrect type in initializer (different address spaces)
  .../icssg_config.c:325:38:    expected void *config
  .../icssg_config.c:325:38:    got void [noderef] __iomem *
  .../icssg_config.c:332:19: warning: incorrect type in argument 1 (different address spaces)
  .../icssg_config.c:332:19:    expected void volatile [noderef] __iomem *
  .../icssg_config.c:332:19:    got void *config
  .../icssg_config.c:361:32: warning: incorrect type in assignment (different base types)
  .../icssg_config.c:361:32:    expected restricted __le16 [usertype] rx_base_flow
  .../icssg_config.c:361:32:    got restricted __le32 [usertype]
  .../icssg_config.c:406:11: warning: incorrect type in assignment (different address spaces)
  .../icssg_config.c:406:11:    expected struct icssg_r30_cmd *p
  .../icssg_config.c:406:11:    got void [noderef] __iomem *
  .../icssg_config.c:417:61: warning: incorrect type in argument 2 (different address spaces)
  .../icssg_config.c:417:61:    expected void volatile [noderef] __iomem *addr
  .../icssg_config.c:417:61:    got unsigned int *
  .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces)
  .../icssg_prueth.c:1665:9:    expected void const *
  .../icssg_prueth.c:1665:9:    got void [noderef] __iomem *va
  .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces)
  .../icssg_prueth.c:1665:9:    expected void const *
  .../icssg_prueth.c:1665:9:    got void [noderef] __iomem *va
  .../icssg_prueth.c:1665:9: warning: incorrect type in argument 1 (different address spaces)
  .../icssg_prueth.c:1665:9:    expected void *
  .../icssg_prueth.c:1665:9:    got void [noderef] __iomem *va

> There is one more sparse warning "warning: symbol 'icssg_ethtool_ops' was
> not declared. Should it be static?". This should be ignored as no need to
> change 'icssg_ethtool_ops' to static as this is decalred in icssg_ethtool.c
> and used in icssg_prueth.c

I think the preferred approach there would be to declare the symbol
in a header file that is available to both .c files.

...

> > > +	prueth->dev = dev;
> > > +	eth_ports_node = of_get_child_by_name(np, "ethernet-ports");
> > > +	if (!eth_ports_node)
> > > +		return -ENOENT;
> > > +
> > > +	for_each_child_of_node(eth_ports_node, eth_node) {
> > > +		u32 reg;
> > > +
> > > +		if (strcmp(eth_node->name, "port"))
> > > +			continue;
> > > +		ret = of_property_read_u32(eth_node, "reg", &reg);
> > > +		if (ret < 0) {
> > > +			dev_err(dev, "%pOF error reading port_id %d\n",
> > > +				eth_node, ret);
> > > +		}
> > > +
> > > +		of_node_get(eth_node);
> > > +
> > > +		if (reg == 0) {
> > > +			eth0_node = eth_node;
> > > +			if (!of_device_is_available(eth0_node)) {
> > > +				of_node_put(eth0_node);
> > > +				eth0_node = NULL;
> > > +			}
> > > +		} else if (reg == 1) {
> > > +			eth1_node = eth_node;
> > > +			if (!of_device_is_available(eth1_node)) {
> > > +				of_node_put(eth1_node);
> > > +				eth1_node = NULL;
> > > +			}
> > > +		} else {
> > > +			dev_err(dev, "port reg should be 0 or 1\n");
> > 
> > Should this be treated as an error and either return or goto an
> > unwind path?
> > 
> 
> I don't think we should error out or return to any goto label here. Here we
> are checking 'reg' property in all available ports. If reg=0, we assign the
> node to eth0_node. If reg=1, we assign the node to eth1_node. If the reg is
> neither 0 nor 1, we will just keep looking through other available ports,
> instead of returning error. We will eventually look through all available
> nodes.
> 
> Once we come out of the for loop, we should at least have one node with reg
> property being either 0 or 1. If no node had reg as 0 or 1, both eth0_node
> and eth1_node will be NULL, then we will error out with -ENODEV error by
> below if check.
> 
> if (!eth0_node && !eth1_node) {
> 	dev_err(dev, "neither port0 nor port1 node available\n");
> 	return -ENODEV;
> }

Thanks, that makes sense to me.

> > > +		}
> > > +	}
> > > +
> > > +	of_node_put(eth_ports_node);
> > > +
> > > +	/* At least one node must be present and available else we fail */
> > > +	if (!eth0_node && !eth1_node) {
> > 
> > Smatch warns that eth0_node and eth1_node may be uninitialised here.
> > 
> 
> Sure, I will initialise eth0_node and eth1_node as NULL.

Thanks.

...