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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a4a0b4c013b254d92f8c1d7c4e88e145a5be4a3.camel@redhat.com>
Date:   Fri, 24 Nov 2023 16:49:04 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Stefan Wahren <wahrenst@....net>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     Lino Sanfilippo <LinoSanfilippo@....de>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4 net] qca_spi: Fix ethtool -G iface tx behavior

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@....net>
> > > ---
> > >   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;