Re: [PATCH net-next v4 1/7] net: dsa: motorcomm: Hoist port_to_priv helper into chip.h

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The helper promoted into the driver-wide header,
  `yt921x_port_to_priv(pp)` in drivers/net/dsa/motorcomm/chip.h:992-993,…

--

net: dsa: motorcomm: Hoist port_to_priv helper into chip.h

The duplicated container_of_const() expression used to get from a
struct yt921x_port back to the enclosing struct yt921x_priv is moved out
of leds.c into chip.h as yt921x_port_to_priv(), and yt921x_poll_mib() in
chip.c is converted to use it instead of its open-coded pointer
arithmetic.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 8b0ac5fc8c8b6..38e7879a9f1ae 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -608,9 +608,8 @@ static void yt921x_poll_mib(struct work_struct *work)
>  {
>  	struct yt921x_port *pp = container_of_const(work, struct yt921x_port,
>  						    mib_read.work);
> -	struct yt921x_priv *priv = (void *)(pp - pp->index) -
> -				   offsetof(struct yt921x_priv, ports);
>  	unsigned long delay = YT921X_STATS_INTERVAL_JIFFIES;
> +	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
>  	int port = pp->index;
>  	int res;
>  
> diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
> index 83cd454955ddb..9cd1010eb9700 100644
> --- a/drivers/net/dsa/motorcomm/chip.h
> +++ b/drivers/net/dsa/motorcomm/chip.h
> @@ -989,4 +989,7 @@ struct yt921x_priv {
>  	struct yt921x_acl_blk *acl_blks[YT921X_ACL_BLK_NUM];
>  };
>  
> +#define yt921x_port_to_priv(pp) \
> +	container_of_const((pp), struct yt921x_priv, ports[(pp)->index])
> +

[Severity: Low]
This isn't a bug today, but now that this helper lives in chip.h and is
visible to every file that includes it, is the double expansion of the
macro argument worth avoiding?

The argument pp appears twice: once as the container_of_const() base
pointer, and again inside the offsetof() member designator:

  container_of_const((pp), struct yt921x_priv, ports[(pp)->index])

container_of_const() only caches the first occurrence in its internal
__mptr, so the second (pp)->index is a separate textual evaluation of the
caller's expression. A caller passing something with side effects or a
function call, say:

  priv = yt921x_port_to_priv(&other->ports[i++]);
  priv = yt921x_port_to_priv(get_port(ds, i));

would compile cleanly but compute the container address from a different
port than the base pointer.

Would a static inline be preferable here?

  static inline struct yt921x_priv *yt921x_port_to_priv(struct yt921x_port *pp)

That gets single evaluation and type checking. All six users
(yt921x_poll_mib() in chip.c, and yt921x_cled_brightness_set_blocking(),
yt921x_cled_blink_set(), yt921x_cled_hw_control_get_device(),
yt921x_cled_hw_control_is_supported(), yt921x_cled_hw_control_get() and
yt921x_cled_hw_control_set() in leds.c) assign the result to a plain
struct yt921x_priv *, so the const-preserving behaviour of
container_of_const() does not appear to be needed.

Failing that, could a short comment be added stating that the argument
must be free of side effects?

>  #endif

[ ... ]

The macro is still present verbatim at the end of the series, so nothing
later in the series changes this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921203040.1203911-1-mmyangfl%40gmail.com