[EXTERNAL] Re: [EXTERNAL] Re: [PATCH v8 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver
Md Danish Anwar
a0501179 at ti.com
Thu Jul 13 04:41:12 PDT 2023
Hi Simon,
On 13/07/23 2:20 pm, Simon Horman wrote:
> 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 at ti.com>
>
> ...
>
>>>> +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:
>
I am working on fixing all these sparse warning. I will send next revision
after fixing these warning.
> .../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.
>
Sure. I will keep the declaration in a icssg_prueth.h.
> ...
>
>>>> + 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.
>
> ...
I will fix all the sparse and smatch warning and send next revision.
--
Thanks and Regards,
Danish.
More information about the linux-arm-kernel
mailing list