| Message ID | 20220919221853.4095491-5-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | DSA: Move parts of inband signalling into the DSA | expand |
On Tue, Sep 20, 2022 at 12:18:48AM +0200, Andrew Lunn wrote: > wait_for_completion_timeout() has unusual return values. It can > return negative error conditions. If it times out, it returns 0, and > on success it returns the number of remaining jiffies for the timeout. The one that also returns negative errors is wait_for_completion_interruptible() (and its variants). In my experience the interruptible version is also a huge foot gun, since user space can kill the process waiting for the RMU response, and the RMU response can still come afterwards, while no one is waiting for it. The noninterruptible wait that we use here really returns an unsigned long, so no negatives.
On Tue, Sep 20, 2022 at 12:18:48AM +0200, Andrew Lunn wrote: > wait_for_completion_timeout() has unusual return values. It can > @@ -591,8 +588,8 @@ 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; > > - dsa_inband_wait_for_completion(&mgmt_eth_data->inband, > - QCA8K_ETHERNET_TIMEOUT); > + ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb, > + QCA8K_ETHERNET_TIMEOUT); Ansuel commented in Message-ID 12edaefc-89a2-f231-156e-5dbe198ae6f6@gmail.com that not checking the error code here was deliberate, and that when Mattias did check the error code, things broke. > > mutex_unlock(&mgmt_eth_data->mutex); >
On Mon, Sep 19, 2022 at 11:02:14PM +0000, Vladimir Oltean wrote: > On Tue, Sep 20, 2022 at 12:18:48AM +0200, Andrew Lunn wrote: > > wait_for_completion_timeout() has unusual return values. It can > > return negative error conditions. If it times out, it returns 0, and > > on success it returns the number of remaining jiffies for the timeout. > > The one that also returns negative errors is wait_for_completion_interruptible() > (and its variants). In my experience the interruptible version is also > a huge foot gun, since user space can kill the process waiting for the > RMU response, and the RMU response can still come afterwards, while no > one is waiting for it. The noninterruptible wait that we use here > really returns an unsigned long, so no negatives. The driver needs to handle the reply coming later independent of ^C handling, etc. The qca8k has a timeout of 5ms. I don't know if that is actually enough, if 1G of traffic is being passed over the interface, and the TX queue is full, and the request frame does not get put at the head of the queue. And if there is 1G of traffic also being received from the switch, how long are the queues for the reply? Does the switch put the reply at the head of the queue? This is one thing i want to play with sometime soon, heavily load the CPU link and see how well the RMU interface to mv88e6xxx works, are the timeouts big enough? Do frames get dropped and are retires needed? Do we need to play with the QoS bits of the skb to make Linux put the RMU packets at the head of the queue etc. I would also like to have another look at the code and make sure it is sane for exactly this case. Andrew
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c index 9c44a09590a6..9481a248273a 100644 --- a/drivers/net/dsa/qca/qca8k-8xxx.c +++ b/drivers/net/dsa/qca/qca8k-8xxx.c @@ -264,8 +264,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len) mutex_unlock(&mgmt_eth_data->mutex); - if (ret <= 0) - return -ETIMEDOUT; + if (ret) + return ret; if (!ack) return -EINVAL; @@ -308,8 +308,8 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len) mutex_unlock(&mgmt_eth_data->mutex); - if (ret <= 0) - return -ETIMEDOUT; + if (ret) + return ret; if (!ack) return -EINVAL; @@ -450,8 +450,8 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data, ack = mgmt_eth_data->ack; - if (ret <= 0) - return -ETIMEDOUT; + if (ret) + return ret; if (!ack) return -EINVAL; @@ -538,8 +538,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy, ack = mgmt_eth_data->ack; - if (ret <= 0) { - ret = -ETIMEDOUT; + if (ret) { kfree_skb(read_skb); goto exit; } @@ -571,10 +570,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy, ack = mgmt_eth_data->ack; - if (ret <= 0) { - ret = -ETIMEDOUT; + if (ret) goto exit; - } if (!ack) { ret = -EINVAL; @@ -591,8 +588,8 @@ 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; - dsa_inband_wait_for_completion(&mgmt_eth_data->inband, - QCA8K_ETHERNET_TIMEOUT); + ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb, + QCA8K_ETHERNET_TIMEOUT); mutex_unlock(&mgmt_eth_data->mutex); diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 8de0c3124abf..68576f1c5b02 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -547,12 +547,18 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb, int timeout_ms) { unsigned long jiffies = msecs_to_jiffies(timeout_ms); + int ret; reinit_completion(&inband->completion); dev_queue_xmit(skb); - return wait_for_completion_timeout(&inband->completion, jiffies); + ret = wait_for_completion_timeout(&inband->completion, jiffies); + if (ret < 0) + return ret; + if (ret == 0) + return -ETIMEDOUT; + return 0; } EXPORT_SYMBOL_GPL(dsa_inband_request);
wait_for_completion_timeout() has unusual return values. It can return negative error conditions. If it times out, it returns 0, and on success it returns the number of remaining jiffies for the timeout. For the use case here, the remaining time is not needed. All that is really interesting is, it succeeded and returns 0, or there was an error or a timeout. Massage the return value to fit this, and modify the callers to the more usual pattern of ret < 0 is an error. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/dsa/qca/qca8k-8xxx.c | 23 ++++++++++------------- net/dsa/dsa.c | 8 +++++++- 2 files changed, 17 insertions(+), 14 deletions(-)