Message-ID: <YymzN6O4lHQIGZdH@lunn.ch>
Date:   Tue, 20 Sep 2022 14:33:59 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "mattias.forsblad@...il.com" <mattias.forsblad@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Christian Marangi <ansuelsmth@...il.com>
Subject: Re: [PATCH rfc v0 8/9] net: dsa: qca8k: Pass response buffer via
 dsa_rmu_request

On Tue, Sep 20, 2022 at 12:27:56AM +0000, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 12:18:52AM +0200, Andrew Lunn wrote:
> > Make the calling of operations on the switch more like a request
> > response API by passing the address of the response buffer, rather
> > than making use of global state.
> > 
> > To avoid race conditions with the completion timeout, and late
> > arriving responses, protect the resp members via a mutex.
> 
> Cannot be a mutex; the context of qca8k_rw_reg_ack_handler(), caller of
> dsa_inband_complete(), is NET_RX softirq and that is not sleepable.

Thanks. I will make it a spinlock.

> >  static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > @@ -230,6 +230,7 @@ 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;
> > +	u32 data[4];
> >  	int ret;
> >  
> >  	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> > @@ -249,12 +250,13 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  	skb->dev = priv->mgmt_master;
> >  
> >  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> > -			      qca8k_mdio_header_fill_seq_num,
> > -			      QCA8K_ETHERNET_TIMEOUT);
> 
> Argument list should have been properly aligned when this patch set introduced it.
> 
> > +				 qca8k_mdio_header_fill_seq_num,
> > +				 &data, sizeof(data),
> > +				 QCA8K_ETHERNET_TIMEOUT);
> 
> Kind of feeling the need for an error check right here, instead of
> proceeding to look at the buffer.

Yes, i can add an error check. data is however safe to access, even if
it is uninitilized. It is on this functions stack and known to be big
enough.

> 
> >  
> > -	*val = mgmt_eth_data->data[0];
> > +	*val = data[0];
> >  	if (len > QCA_HDR_MGMT_DATA1_LEN)
> > -		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> > +		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
> 
> This is pretty hard to digest, but it looks like it could work.
> So this can run concurrently with qca8k_rw_reg_ack_handler(), but since
> the end of dsa_inband_request() sets inband->resp to NULL, then even if
> the response will come later, it won't touch the driver-provided on-stack
> buffer, since the DSA completion structure lost the reference to it.

Yes, that is what i want the mutex for, soon to become a spinlock.

> 
> How do we deal with the response being processed so late by the handler
> that it overlaps with the dsa_inband_request() call of the next seqid?
> We open up to another window of opportunity for the handler to have a
> valid buffer and length to which it can copy stuff. Does it matter,
> since the seqid of the response will be smaller than that of the request?

That is what i need to look at. So long as the sequence number is
incremented first, then the completion reinitialized, i think we are
safe.

> Is reordering on multi-CPU, multi-queue masters handled in any way? This
> will be a problem regardless of QoS - currently we assume that all
> management frames are treated the same by the DSA master. But it has no
> insight into the DSA header format, so why would it? It could be doing
> RSS and even find some entropy in our seqid junk data. It's a bit late
> to think through right now.

There is a big mutex serializing all inband operations. There should
never be two or more valid operations in flight.

The way the sequence numbers work, i think in theory you could have
multiple operations in flight and the hardware would do the right
thing, and you could get a little bit more performance. But you really
need to worry about packets getting reordered. I've no numbers yet,
but i think the performance gains from MDIO to single in flight inband
is sufficient we don't need to worry about squeezing the last bit of
performance out.

> >  struct qca8k_mib_eth_data {
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 1a920f89b667..dad9e31d36ce 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -1285,12 +1285,17 @@ struct dsa_inband {
> >  	u32 seqno;
> >  	u32 seqno_mask;
> >  	int err;
> > +	struct mutex resp_lock; /* Protects resp* members */
> > +	void *resp;
> > +	unsigned int resp_len;
> 
> Would be good to be a bit more verbose about what "protecting" means
> here (just offering a consistent view of the buffer pointer and of its
> length from DSA's perspective).

Yes, i can expand the comment.

> > -void dsa_inband_complete(struct dsa_inband *inband, int err)
> > +void dsa_inband_complete(struct dsa_inband *inband,
> > +			 void *resp, unsigned int resp_len,
> > +			 int err)
> >  {
> >  	inband->err = err;
> > +
> > +	mutex_lock(&inband->resp_lock);
> > +	resp_len = min(inband->resp_len, resp_len);
> 
> No warning for truncation caused by resp_len > inband->resp_len?
> It seems like a valid error. At least I tried to test Mattias' patch
> set, and this is one of the problems that really happened.

I need to go back and look at Mattias patches, but he made the comment
that you don't always know the size of the response. It can be family
dependent. I think the 6390 adds additional statistics, so its
response it going to be bigger than other devices. We don't currently
handle those additional statistics. I made the same comment as you
did, maybe return -ENOBUF or something if it truncates. QCA8k is
simpler in this respect, it can have a maximum of 12 bytes of optional
data, but given that the frame needs to be padded to 64 bytes, you
know you will always have those bytes, even if they are full of junk.

This is one of those areas where i'm not sure there is a correct
answer. Do more checking here, and force some complexity into the
caller? Truncate, but the caller has no idea a truncate has happened?

	Andrew

Powered by blists - more mailing lists