Re: [PATCH net-next 9/9] net: dsa: microchip: split ksz_connect_tag_protocol()

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says: I also note that there is an AI review of patch 7/9.
However, I believe that is a false positive.

---
net: dsa: microchip: split ksz_connect_tag_protocol()

The commit splits the common ksz_connect_tag_protocol() into switch-specific
operations to ensure each switch only accepts the tag protocol it supports.

> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 9bac95bb079ff..ac2c63fe0588c 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
[ ... ]
> @@ -1624,6 +1625,20 @@ static enum dsa_tag_protocol ksz9477_get_tag_protocol(struct dsa_switch *ds,
>  	return DSA_TAG_PROTO_KSZ9477;
>  }
>  
> +static int ksz9477_connect_tag_protocol(struct dsa_switch *ds,
> +					enum dsa_tag_protocol proto)
> +{
> +	struct ksz_tagger_data *tagger_data;
> +
> +	if (proto != DSA_TAG_PROTO_KSZ9893 && proto != DSA_TAG_PROTO_KSZ9477)
> +		return -EPROTONOSUPPORT;

Does this fully enforce the restriction mentioned in the commit message?

The commit message states "each switch will only accept to connect the tag
protocol it supports". While ksz88xx_connect_tag_protocol() correctly checks
the chip family, ksz9477_connect_tag_protocol() accepts either
DSA_TAG_PROTO_KSZ9893 or DSA_TAG_PROTO_KSZ9477 without checking the specific
chip ID.

For comparison, ksz9477_get_tag_protocol() returns DSA_TAG_PROTO_KSZ9893 only
for specific chips (KSZ8563, KSZ9893, KSZ9563).

Should this function check dev->chip_id to prevent a chip from being
configured to use the wrong protocol?

> +
> +	tagger_data = ksz_tagger_data(ds);
> +	tagger_data->xmit_work_fn = ksz_port_deferred_xmit;
> +
> +	return 0;
> +}