Re: [v10,net-next,1/2] net/sched: mqprio: pass qdisc handle to offload drivers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Concern that the new `u32 handle` member of `struct
  tc_mqprio_qopt_offload` (include/net/pkt_sched.h:168) is added with no…
- [Low] The new `handle` member of `struct tc_mqprio_qopt_offload` is
  populated by only one of the two in-tree producers of that structure.

--

net/sched: mqprio: pass qdisc handle to offload drivers

This adds a u32 handle member to struct tc_mqprio_qopt_offload and fills it
from sch->handle in both mqprio_enable_offload() and mqprio_disable_offload()
so that offload drivers can tell mqprio setup, teardown and qdisc replace
apart.  The second patch in the series consumes the field in the octeontx2
driver.

> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> index 90d3e7943b19a..9f8694ecd4363 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -165,6 +165,7 @@ struct tc_mqprio_caps {
>  struct tc_mqprio_qopt_offload {
>  	/* struct tc_mqprio_qopt must always be the first element */
>  	struct tc_mqprio_qopt qopt;
> +	u32 handle;

[Severity: Low]
This isn't a bug, but could the new member get a short comment describing
its contract, next to the existing "struct tc_mqprio_qopt must always be
the first element" note?

The struct is driver facing and shared, and nothing here states that
handle carries the owning mqprio qdisc handle, that it is never
TC_H_UNSPEC when the command comes from sch_mqprio, or what a value of 0
means for a consumer.

>  	struct netlink_ext_ack *extack;
>  	u16 mode;
>  	u16 shaper;
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index 6ced7008ef5c8..adc1393f84eaa 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -39,6 +39,7 @@ static int mqprio_enable_offload(struct Qdisc *sch,
>  	struct net_device *dev = qdisc_dev(sch);
>  	struct tc_mqprio_qopt_offload mqprio = {
>  		.qopt = *qopt,
> +		.handle = sch->handle,
>  		.extack = extack,
>  	};
>  	int err, i;

[Severity: Low]
Should sch_taprio also fill this field?  struct tc_mqprio_qopt_offload is
embedded in struct tc_taprio_qopt_offload, and taprio_enable_offload()
populates the embedded copy member by member:

net/sched/sch_taprio.c:taprio_enable_offload() {
	...
	offload->cmd = TAPRIO_CMD_REPLACE;
	offload->extack = extack;
	mqprio_qopt_reconstruct(dev, &offload->mqprio.qopt);
	offload->mqprio.extack = extack;
	taprio_sched_to_offload(dev, sched, offload, &caps);
	mqprio_fp_to_offload(q->fp, &offload->mqprio);
	...
}

The object comes from the kzalloc in taprio_offload_alloc(), so
offload->mqprio.handle stays 0 (TC_H_UNSPEC), which is a value the
sch_mqprio path never produces.

Several drivers feed that embedded sub-struct into the same handler they
use for TC_SETUP_QDISC_MQPRIO:

  felix_vsc9959.c    vsc9959_qos_port_tas_set() -> ocelot_port_mqprio()
  enetc_qos.c        enetc_setup_tc_taprio()    -> enetc_setup_tc_mqprio()
  am65-cpsw-qos.c    am65_cpsw_setup_taprio()   -> am65_cpsw_setup_mqprio()

am65_cpsw_setup_mqprio() memcpy-caches the whole struct, so it would cache
handle == 0 on the taprio path and a real handle on the mqprio path for the
same hardware state.

No current consumer is affected: the only reader added by this series is
octeontx2, and otx2_setup_tc() handles TC_QUERY_CAPS, TC_SETUP_BLOCK,
TC_SETUP_QDISC_HTB, TC_SETUP_QDISC_MQPRIO and TC_SETUP_ROOT_QDISC only, so
it never sees a taprio-produced struct.

Would it be better to set offload->mqprio.handle from sch->handle in
taprio_enable_offload() as well, or to document in pkt_sched.h that the
field is only meaningful for TC_SETUP_QDISC_MQPRIO issued by sch_mqprio,
before drivers start keying committed state off it?

> @@ -79,7 +80,9 @@ static int mqprio_enable_offload(struct Qdisc *sch,
>  
>  static void mqprio_disable_offload(struct Qdisc *sch)
>  {
> -	struct tc_mqprio_qopt_offload mqprio = { { 0 } };
> +	struct tc_mqprio_qopt_offload mqprio = {
> +		.handle = sch->handle,
> +	};
>  	struct mqprio_sched *priv = qdisc_priv(sch);
>  	struct net_device *dev = qdisc_dev(sch);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831131014.2639581-1-rkannoth%40marvell.com