Re: [PATCH net-next 02/12] net: dsa: qca8K: Move queuing for request frame into the core

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: qca8K: Move queuing for request frame into the core

This patch introduces dsa_inband_request() in the DSA core which
reinitialises the completion, transmits the skb via dev_queue_xmit(),
and waits for a reply. The qca8k driver is converted to use this new
helper in place of the open-coded dev_queue_xmit() plus
dsa_inband_wait_for_completion() sequence.

[Low]
The subject line uses qca8K with a capital K, which is inconsistent
with qca8k as used everywhere else in the driver and in the rest of
the series.

The commit body refers to functions that do not exist:

    Combine the queuing of the request and waiting for the completion into
    one core helper. Add the function dsa_rmu_request() to perform this.

    Access to statistics is not a strict request/reply, so the
    dsa_rmu_wait_for_completion needs to be kept.

    It is also no possible to combine dsa_rmu_request() and
    dsa_rmu_wait_for_completion() since we need to avoid the race of
    sending the request, receiving a reply, and the completion has not
    been reinitialised because the schedule at decided to do other things.

The introduced symbol is dsa_inband_request(), and the preserved
symbol is dsa_inband_wait_for_completion(); should the message refer
to those names so that git log --grep for dsa_inband_request finds
this commit?

There are also a couple of small grammar issues in the same paragraph
("It is also no possible" and "the schedule at decided to do other
things").

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index fe6fb69b6f92..4da6094552f1 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c

[ ... ]

> @@ -757,8 +747,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dev_queue_xmit(clear_skb);
> -
>  	dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
>  				       QCA8K_ETHERNET_TIMEOUT);

[High]
The other five hunks replace dev_queue_xmit() plus
dsa_inband_wait_for_completion() with a single dsa_inband_request()
call. This hunk deletes dev_queue_xmit(clear_skb) but leaves the bare
dsa_inband_wait_for_completion() in place, so no packet is ever sent
for the clear step.

A few consequences follow from that:

Does this leak clear_skb on every path through qca8k_phy_eth_command()
that reaches the exit: label? The skb is allocated by
qca8k_alloc_mdio_header() and, after this change, nothing ever hands
it to dev_queue_xmit() or frees it.

Does every PHY read/write through this driver now stall for
QCA8K_ETHERNET_TIMEOUT (200ms) at the exit: label, since the
completion is reinitialised but no request is ever queued for the
switch to reply to?

Does this also skip resetting the switch's MDIO master, which was the
purpose of the clear_skb step?

The next patch in the series, 68baa3a653c1 ("net: dsa: qca8k:
dsa_inband_request: More normal return values"), replaces the
dsa_inband_wait_for_completion() call with

    dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
                       QCA8K_ETHERNET_TIMEOUT);

Would it be worth squashing or reordering so that bisection landing
on this commit does not see hangs and skb leaks on every management
frame PHY access?

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 9b0c109b3058..6b5aeb99ec3b 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1357,6 +1357,8 @@ struct dsa_inband {
>  
>  void dsa_inband_init(struct dsa_inband *inband);
>  void dsa_inband_complete(struct dsa_inband *inband);
> +int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
> +		       int timeout_ms);
>  int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);

[ ... ]