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", ®);
> > > + 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.
...
