Re: [PATCH 3/4 net] qca_spi: Fix ethtool -G iface tx behavior

[Paolo Abeni]

On Fri, 2023-11-24 at 15:17 +0100, Stefan Wahren wrote:
> Am 23.11.23 um 12:51 schrieb Paolo Abeni:
> > On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:
> > > After calling ethtool -g it was not possible to adjust the TX ring size
> > > again.
> > Could you please report the exact command sequence that will fail?
> ethtool -g eth1
> ethtool -G eth1 tx 8
> > 
> > 
> > > The reason for this is that the readonly setting rx_pending get
> > > initialized and after that the range check in qcaspi_set_ringparam()
> > > fails regardless of the provided parameter. Since there is no adjustable
> > > RX ring at all, drop it from qcaspi_get_ringparam().
> > > Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
> > > Signed-off-by: Stefan Wahren <wahrenst@xxxxxxx>
> > > ---
> > >   drivers/net/ethernet/qualcomm/qca_debug.c | 2 --
> > >   1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/qualcomm/qca_debug.c b/drivers/net/ethernet/qualcomm/qca_debug.c
> > > index 6f2fa2a42770..613eb688cba2 100644
> > > --- a/drivers/net/ethernet/qualcomm/qca_debug.c
> > > +++ b/drivers/net/ethernet/qualcomm/qca_debug.c
> > > @@ -252,9 +252,7 @@ qcaspi_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring,
> > >   {
> > >   	struct qcaspi *qca = netdev_priv(dev);
> > > 
> > > -	ring->rx_max_pending = 4;
> > >   	ring->tx_max_pending = TX_RING_MAX_LEN;
> > > -	ring->rx_pending = 4;
> > >   	ring->tx_pending = qca->txr.count;
> > >   }
> > I think it's preferable update qcaspi_set_ringparam() to complete
> > successfully when the provided arguments don't change the rx_pending
> > default (4)
> 
> Sorry, i didn't get. The whole point is that there is no RX ring at all,
> just a TX ring. 
> During the time of writing this driver, i was under the
> assumption that the driver needs to provide a rx_pending in
> qcaspi_get_ringparam even this is no RX ring. The number 4 represent the
> maximum of 4 packets which can be received at once. But it's not a ring.

Even if the H/W in charge of receiving and storing the incoming packet
is not exactly a ring but some fixed-size structure, I think it would
be better to avoid changing the exposed defaults given they are not
actually changed by this patch and they represent the current status
IMHO quite accurately.

The change I suggested is something alike the following (note that you
could possibly define a macro with a helpful name instead of the raw
number '4')

Cheers,

Paolo
---
diff --git a/drivers/net/ethernet/qualcomm/qca_debug.c b/drivers/net/ethernet/qualcomm/qca_debug.c
index 6f2fa2a42770..05c5450bff79 100644
--- a/drivers/net/ethernet/qualcomm/qca_debug.c
+++ b/drivers/net/ethernet/qualcomm/qca_debug.c
@@ -266,7 +266,7 @@ qcaspi_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring,
        const struct net_device_ops *ops = dev->netdev_ops;
        struct qcaspi *qca = netdev_priv(dev);
 
-       if ((ring->rx_pending) ||
+       if ((ring->rx_pending != 4) ||
            (ring->rx_mini_pending) ||
            (ring->rx_jumbo_pending))
                return -EINVAL;