Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <1a4a0b4c013b254d92f8c1d7c4e88e145a5be4a3.camel@redhat.com>
Subject: Re: [PATCH 3/4 net] qca_spi: Fix ethtool -G iface tx behavior
From:   Paolo Abeni <pabeni@redhat.com>
To:     Stefan Wahren <wahrenst@gmx.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>
Cc:     Lino Sanfilippo <LinoSanfilippo@gmx.de>,
        Florian Fainelli <f.fainelli@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Fri, 24 Nov 2023 16:49:04 +0100
In-Reply-To: <0d1cf97c-abbe-4a7a-a634-312caa882fad@gmx.net>
References: <20231121163004.21232-1-wahrenst@gmx.net>
         <20231121163004.21232-4-wahrenst@gmx.net>
         <ea0087881f20dc154ca08a5b748b853246e2b86f.camel@redhat.com>
         <0d1cf97c-abbe-4a7a-a634-312caa882fad@gmx.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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 si=
ze
> > > again.
> > Could you please report the exact command sequence that will fail?
> ethtool -g eth1
> ethtool -G eth1 tx 8
> >=20
> >=20
> > > 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 adjusta=
ble
> > > 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@gmx.net>
> > > ---
> > >   drivers/net/ethernet/qualcomm/qca_debug.c | 2 --
> > >   1 file changed, 2 deletions(-)
> > >=20
> > > 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, stru=
ct ethtool_ringparam *ring,
> > >   {
> > >   	struct qcaspi *qca =3D netdev_priv(dev);
> > >=20
> > > -	ring->rx_max_pending =3D 4;
> > >   	ring->tx_max_pending =3D TX_RING_MAX_LEN;
> > > -	ring->rx_pending =3D 4;
> > >   	ring->tx_pending =3D 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)
>=20
> Sorry, i didn't get. The whole point is that there is no RX ring at all,
> just a TX ring.=C2=A0
> 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/ethern=
et/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 eth=
tool_ringparam *ring,
        const struct net_device_ops *ops =3D dev->netdev_ops;
        struct qcaspi *qca =3D netdev_priv(dev);
=20
-       if ((ring->rx_pending) ||
+       if ((ring->rx_pending !=3D 4) ||
            (ring->rx_mini_pending) ||
            (ring->rx_jumbo_pending))
                return -EINVAL;