| Message ID | 20231121163004.21232-4-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qca_spi: collection of major fixes | expand |
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? > 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@gmx.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) Cheers, Paolo
Hi Paolo, 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@gmx.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. Best regards > Cheers, > > Paolo >
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@gmx.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;
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; }
After calling ethtool -g it was not possible to adjust the TX ring size again. 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@gmx.net> --- drivers/net/ethernet/qualcomm/qca_debug.c | 2 -- 1 file changed, 2 deletions(-) -- 2.34.1