Re: [net-next Patch v6 5/6] octeontx2-pf: Add support for HTB offload

[Simon Horman <simon.horman@corigine.com>]

On Thu, Apr 06, 2023 at 03:51:02PM +0530, Hariprasad Kelam wrote:
> From: Naveen Mamindlapalli <naveenm@marvell.com>
> 
> This patch registers callbacks to support HTB offload.
> 
> Below are features supported,
> 
> - supports traffic shaping on the given class by honoring rate and ceil
> configuration.
> 
> - supports traffic scheduling,  which prioritizes different types of
> traffic based on strict priority values.
> 
> - supports the creation of leaf to inner classes such that parent node
> rate limits apply to all child nodes.

...

> +static int otx2_qos_leaf_alloc_queue(struct otx2_nic *pfvf, u16 classid,
> +				     u32 parent_classid, u64 rate, u64 ceil,
> +				     u64 prio, struct netlink_ext_ack *extack)
> +{
> +	struct otx2_qos_cfg *old_cfg, *new_cfg;
> +	struct otx2_qos_node *node, *parent;
> +	int qid, ret, err;
> +
> +	netdev_dbg(pfvf->netdev,
> +		   "TC_HTB_LEAF_ALLOC_QUEUE: classid=0x%x parent_classid=0x%x rate=%lld ceil=%lld prio=%lld\n",
> +		   classid, parent_classid, rate, ceil, prio);
> +
> +	if (prio > OTX2_QOS_MAX_PRIO) {
> +		NL_SET_ERR_MSG_MOD(extack, "Valid priority range 0 to 7");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}

out dereferences parent, but it is not set until a few lines below.

reported by gcc-12 with W=1 EXTRA_CFLAGS=-Wmaybe-uninitialized as:

drivers/net/ethernet/marvell/octeontx2/nic/qos.c: In function 'otx2_qos_leaf_alloc_queue':
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1178:31: error: 'parent' may be used uninitialized [-Werror=maybe-uninitialized]
 1178 |         clear_bit(prio, parent->prio_bmap);
      |                         ~~~~~~^~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1076:38: note: 'parent' was declared here
 1076 |         struct otx2_qos_node *node, *parent;
      |         

And by clang-16:

drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1083:6: error: variable 'parent' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
        if (prio > OTX2_QOS_MAX_PRIO) {
            ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1178:18: note: uninitialized use occurs here
        clear_bit(prio, parent->prio_bmap);
                        ^~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1083:2: note: remove the 'if' if its condition is always false
        if (prio > OTX2_QOS_MAX_PRIO) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1076:37: note: initialize the variable 'parent' to silence this warning
        struct otx2_qos_node *node, *parent;
                                           ^
                                            = NULL

> +
> +	/* get parent node */
> +	parent = otx2_sw_node_find(pfvf, parent_classid);
> +	if (!parent) {
> +		NL_SET_ERR_MSG_MOD(extack, "parent node not found");
> +		ret = -ENOENT;
> +		goto out;
> +	}

...

> +	return pfvf->hw.tx_queues + qid;
> +
> +free_node:
> +	otx2_qos_sw_node_delete(pfvf, node);
> +free_old_cfg:
> +	kfree(old_cfg);
> +out:
> +	clear_bit(prio, parent->prio_bmap);
> +	return ret;
> +}
> +
> +static int otx2_qos_leaf_to_inner(struct otx2_nic *pfvf, u16 classid,
> +				  u16 child_classid, u64 rate, u64 ceil, u64 prio,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct otx2_qos_cfg *old_cfg, *new_cfg;
> +	struct otx2_qos_node *node, *child;
> +	int ret, err;
> +	u16 qid;
> +
> +	netdev_dbg(pfvf->netdev,
> +		   "TC_HTB_LEAF_TO_INNER classid %04x, child %04x, rate %llu, ceil %llu\n",
> +		   classid, child_classid, rate, ceil);
> +
> +	if (prio > OTX2_QOS_MAX_PRIO) {
> +		NL_SET_ERR_MSG_MOD(extack, "Valid priority range 0 to 7");
> +		ret = -EOPNOTSUPP;
> +		goto out;

Likewise, out dereferences node, but it is not set until a few lines below.

reported by gcc-12 with W=1 EXTRA_CFLAGS=-Wmaybe-uninitialized as:

drivers/net/ethernet/marvell/octeontx2/nic/qos.c: In function 'otx2_qos_leaf_to_inner':
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1288:29: error: 'node' may be used uninitialized [-Werror=maybe-uninitialized]
 1288 |         clear_bit(prio, node->prio_bmap);
      |                         ~~~~^~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1187:31: note: 'node' was declared here
 1187 |         struct otx2_qos_node *node, *child;
      |                               ^~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c: In function 'otx2_qos_leaf_alloc_queue':
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1178:31: error: 'parent' may be used uninitialized [-Werror=maybe-uninitialized]
 1178 |         clear_bit(prio, parent->prio_bmap);
      |                         ~~~~~~^~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1076:38: note: 'parent' was declared here
 1076 |         struct otx2_qos_node *node, *parent;
      |         

And by clang-16 as:

drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1195:6: error: variable 'node' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
        if (prio > OTX2_QOS_MAX_PRIO) {
            ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1288:18: note: uninitialized use occurs here
        clear_bit(prio, node->prio_bmap);
                        ^~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1195:2: note: remove the 'if' if its condition is always false
        if (prio > OTX2_QOS_MAX_PRIO) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/octeontx2/nic/qos.c:1187:28: note: initialize the variable 'node' to silence this warning
        struct otx2_qos_node *node, *child;
                                  ^
                                   = NULL

> +	}
> +
> +	/* find node related to classid */
> +	node = otx2_sw_node_find(pfvf, classid);
> +	if (!node) {
> +		NL_SET_ERR_MSG_MOD(extack, "HTB node not found");
> +		ret = -ENOENT;
> +		goto out;
> +	}

...

> +	return 0;
> +
> +free_node:
> +	otx2_qos_sw_node_delete(pfvf, child);
> +free_old_cfg:
> +	kfree(old_cfg);
> +out:
> +	clear_bit(prio, node->prio_bmap);
> +	return ret;
> +}

...