| Message ID | 20220922175821.4184622-9-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | DSA: Move parts of inband signalling into the DSA | expand |
On Thu, Sep 22, 2022 at 07:58:19PM +0200, Andrew Lunn wrote: > The code which decodes the frame and signals the complete can detect > error within the reply, such as fields have unexpected values. Pass an > error code between the completer and the function waiting on the > complete. This simplifies the error handling, since all errors are > combined into one place. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > v2: > > Remove EPROTO if the sequence numbers don't match, drop the reply > --- > @@ -1439,6 +1400,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk > const struct qca8k_mib_desc *mib; > struct mib_ethhdr *mib_ethhdr; > int i, mib_len, offset = 0; > + int err = 0; > u64 *data; > u8 port; > > @@ -1449,8 +1411,10 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk > * parse only the requested one. > */ > port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr)); > - if (port != mib_eth_data->req_port) > + if (port != mib_eth_data->req_port) { > + err = -EPROTO; > goto exit; > + } > > data = mib_eth_data->data; > > @@ -1479,7 +1443,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk > exit: > /* Complete on receiving all the mib packet */ > if (refcount_dec_and_test(&mib_eth_data->port_parsed)) > - dsa_inband_complete(&mib_eth_data->inband); > + dsa_inband_complete(&mib_eth_data->inband, err); > } My understanding of the autocast function (I could be wrong) is that it's essentially one request with 10 (or how many ports there are) responses. At least this is what the code appears to handle. I don't think you can say "if port isn't the requested port => set err to -EPROTO". Because this will make dsa_inband_wait_for_completion() see as a return code -EPROTO whenever the requested port was anything except for the last port for which the switch had something to autocast. Which is probably the last port.
> My understanding of the autocast function (I could be wrong) is that > it's essentially one request with 10 (or how many ports there are) > responses. At least this is what the code appears to handle. The autocast packet handling does not fit the model. I already excluded it from parts of the patchset. I might need to exclude it from more. It is something i need to understand more. I did find a leaked data sheet for the qca8337, but i've not had time to read it yet. Either the model needs to change a bit, or we don't convert this part of the code to use shared functions, or maybe we can do a different implementation in the driver for statistics access. Andrew
On Sat, Sep 24, 2022 at 04:32:15PM +0200, Andrew Lunn wrote: > > My understanding of the autocast function (I could be wrong) is that > > it's essentially one request with 10 (or how many ports there are) > > responses. At least this is what the code appears to handle. > > The autocast packet handling does not fit the model. I already > excluded it from parts of the patchset. I might need to exclude it > from more. It is something i need to understand more. I did find a > leaked data sheet for the qca8337, but i've not had time to read it > yet. > > Either the model needs to change a bit, or we don't convert this part > of the code to use shared functions, or maybe we can do a different > implementation in the driver for statistics access. I was thinking, as a complement to your series, maybe we could make the response processing also generic (so instead of the tagger calling a driver-provided handler, let it call a dsa_inband_response() instead). This would look through the list of queued ds->requests, and have a (*match_cb)() which returns an action, be it "packet doesn't match this request", "packet matches, please remove request from list", or "packet matches, but still keep request in list". In addition, the queued request will also have a (*cb)(), which is the action to execute on match from a response. The idea is that if we bother to provide a generic implementation within DSA, at least we could try to make its core async, and just offer sychronous wrappers if this is what drivers wish to use (like a generic cb() which calls complete()). This would also come hand in hand with the requests being allocated on demand, which I think would simplify a bit the notion that an unexpected response might trigger a match with an unsolicited request.
> > My understanding of the autocast function (I could be wrong) is that > > it's essentially one request with 10 (or how many ports there are) > > responses. At least this is what the code appears to handle. > > The autocast packet handling does not fit the model. I already > excluded it from parts of the patchset. I might need to exclude it > from more. It is something i need to understand more. I did find a > leaked data sheet for the qca8337, but i've not had time to read it > yet. > > Either the model needs to change a bit, or we don't convert this part > of the code to use shared functions, or maybe we can do a different > implementation in the driver for statistics access. > Honestly autocast mib seems a very different feature than inband. And I can totally see other switch works in a similar way. It's a different feature than using inband to access mib data. For now I would focus on inband and think about this later.
On Sat, Sep 24, 2022 at 02:42:50PM +0000, Vladimir Oltean wrote: > On Sat, Sep 24, 2022 at 04:32:15PM +0200, Andrew Lunn wrote: > > > My understanding of the autocast function (I could be wrong) is that > > > it's essentially one request with 10 (or how many ports there are) > > > responses. At least this is what the code appears to handle. > > > > The autocast packet handling does not fit the model. I already > > excluded it from parts of the patchset. I might need to exclude it > > from more. It is something i need to understand more. I did find a > > leaked data sheet for the qca8337, but i've not had time to read it > > yet. > > > > Either the model needs to change a bit, or we don't convert this part > > of the code to use shared functions, or maybe we can do a different > > implementation in the driver for statistics access. > > I was thinking, as a complement to your series, maybe we could make the > response processing also generic (so instead of the tagger calling a > driver-provided handler, let it call a dsa_inband_response() instead). I agree i've mostly looked at the dsa_inband_request side. Not having any hardware means i've been trying to keep the patches simple, obviously, and bug free. It seems i've failed at this last part. I hope we can iterate the dsa_inband_response() afterwards. My patchset is a good size on its own, so maybe we should get that merged first, before starting on dsa_inband_response(). I would also like to get some basic mv88e6xxx code merged, so it gives me a test system. > This would look through the list of queued ds->requests, and have a > (*match_cb)() which returns an action, be it "packet doesn't match this > request", "packet matches, please remove request from list", or "packet > matches, but still keep request in list". In addition, the queued > request will also have a (*cb)(), which is the action to execute on > match from a response. The idea is that if we bother to provide a > generic implementation within DSA, at least we could try to make its > core async, and just offer sychronous wrappers if this is what drivers > wish to use (like a generic cb() which calls complete()). The autocast is just different. What i don't know is, does any other switch have anything similar? I guess every switch with inband is going to have Request/reply, so i think we should concentrate on that. Another thing we need to decide is, do we want to allow multiple in flight request/reply. If we say No, all need is to be able to determine is if a reply is late reply we should discard, or the reply to the current request. That is much simpler than a generic match the reply to a list of requests. For the moment, i would prefer KISS, lets get something working for two devices. We can make it more complex later. Andrew
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c index 1127fa138960..096b43f89869 100644 --- a/drivers/net/dsa/qca/qca8k-8xxx.c +++ b/drivers/net/dsa/qca/qca8k-8xxx.c @@ -138,6 +138,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb) struct qca8k_priv *priv = ds->priv; struct qca_mgmt_ethhdr *mgmt_ethhdr; u8 len, cmd; + int err = 0; mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb); mgmt_eth_data = &priv->mgmt_eth_data; @@ -160,7 +161,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb) QCA_HDR_MGMT_DATA2_LEN); } - dsa_inband_complete(&mgmt_eth_data->inband); + dsa_inband_complete(&mgmt_eth_data->inband, err); } static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val, @@ -229,7 +230,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len) { struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data; struct sk_buff *skb; - int err; int ret; skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, @@ -256,24 +256,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len) if (len > QCA_HDR_MGMT_DATA1_LEN) memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN); - err = mgmt_eth_data->err; - mutex_unlock(&mgmt_eth_data->mutex); - if (ret) - return ret; - - if (err) - return err; - - return 0; + return ret; } static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len) { struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data; struct sk_buff *skb; - int err; int ret; skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val, @@ -296,17 +287,9 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len) qca8k_mdio_header_fill_seq_num, QCA8K_ETHERNET_TIMEOUT); - err = mgmt_eth_data->err; - mutex_unlock(&mgmt_eth_data->mutex); - if (ret) - return ret; - - if (err) - return err; - - return 0; + return ret; } static int @@ -429,21 +412,15 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data, struct sk_buff *read_skb, u32 *val) { struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL); - int err; int ret; ret = dsa_inband_request(&mgmt_eth_data->inband, skb, qca8k_mdio_header_fill_seq_num, QCA8K_ETHERNET_TIMEOUT); - err = mgmt_eth_data->err; - if (ret) return ret; - if (err) - return err; - *val = mgmt_eth_data->data[0]; return 0; @@ -458,7 +435,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy, u32 write_val, clear_val = 0, val; struct net_device *mgmt_master; int ret, ret1; - int err; if (regnum >= QCA8K_MDIO_MASTER_MAX_REG) return -EINVAL; @@ -520,19 +496,11 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy, qca8k_mdio_header_fill_seq_num, QCA8K_ETHERNET_TIMEOUT); - err = mgmt_eth_data->err; - if (ret) { kfree_skb(read_skb); goto exit; } - if (err) { - ret = err; - kfree_skb(read_skb); - goto exit; - } - ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1, !(val & QCA8K_MDIO_MASTER_BUSY), 0, QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false, @@ -548,16 +516,9 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy, qca8k_mdio_header_fill_seq_num, QCA8K_ETHERNET_TIMEOUT); - err = mgmt_eth_data->err; - if (ret) goto exit; - if (err) { - ret = err; - goto exit; - } - ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK; } else { kfree_skb(read_skb); @@ -1439,6 +1400,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk const struct qca8k_mib_desc *mib; struct mib_ethhdr *mib_ethhdr; int i, mib_len, offset = 0; + int err = 0; u64 *data; u8 port; @@ -1449,8 +1411,10 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk * parse only the requested one. */ port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr)); - if (port != mib_eth_data->req_port) + if (port != mib_eth_data->req_port) { + err = -EPROTO; goto exit; + } data = mib_eth_data->data; @@ -1479,7 +1443,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk exit: /* Complete on receiving all the mib packet */ if (refcount_dec_and_test(&mib_eth_data->port_parsed)) - dsa_inband_complete(&mib_eth_data->inband); + dsa_inband_complete(&mib_eth_data->inband, err); } static int diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h index 7b928c160c0e..ce27a732dba0 100644 --- a/drivers/net/dsa/qca/qca8k.h +++ b/drivers/net/dsa/qca/qca8k.h @@ -348,7 +348,6 @@ enum { struct qca8k_mgmt_eth_data { struct dsa_inband inband; struct mutex mutex; /* Enforce one mdio read/write at time */ - int err; u32 data[4]; }; diff --git a/include/net/dsa.h b/include/net/dsa.h index e8cbd67279ea..497d0eb85cf1 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -1311,10 +1311,11 @@ struct dsa_inband { struct completion completion; u32 seqno; u32 seqno_mask; + int err; }; void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask); -void dsa_inband_complete(struct dsa_inband *inband); +void dsa_inband_complete(struct dsa_inband *inband, int err); int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb, void (*insert_seqno)(struct sk_buff *skb, u32 seqno), int timeout_ms); diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index cddf62916932..a426459c74c5 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -526,8 +526,9 @@ void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask) } EXPORT_SYMBOL_GPL(dsa_inband_init); -void dsa_inband_complete(struct dsa_inband *inband) +void dsa_inband_complete(struct dsa_inband *inband, int err) { + inband->err = err; complete(&inband->completion); } EXPORT_SYMBOL_GPL(dsa_inband_complete); @@ -552,6 +553,8 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb, unsigned long jiffies = msecs_to_jiffies(timeout_ms); int ret; + inband->err = 0; + if (insert_seqno) { inband->seqno++; insert_seqno(skb, inband->seqno & inband->seqno_mask); @@ -564,7 +567,8 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb, ret = wait_for_completion_timeout(&inband->completion, jiffies); if (ret == 0) return -ETIMEDOUT; - return 0; + + return inband->err; } EXPORT_SYMBOL_GPL(dsa_inband_request);
The code which decodes the frame and signals the complete can detect error within the reply, such as fields have unexpected values. Pass an error code between the completer and the function waiting on the complete. This simplifies the error handling, since all errors are combined into one place. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- v2: Remove EPROTO if the sequence numbers don't match, drop the reply --- drivers/net/dsa/qca/qca8k-8xxx.c | 54 ++++++-------------------------- drivers/net/dsa/qca/qca8k.h | 1 - include/net/dsa.h | 3 +- net/dsa/dsa.c | 8 +++-- 4 files changed, 17 insertions(+), 49 deletions(-)