[PATCH 0/4 net] qca_spi: collection of major fixes

This series contains a collection of major fixes for the qca_spi driver,
which has been recently discovered.

Stefan Wahren (4):
  qca_spi: Fix SPI thread creation
  qca_spi: Fix SPI IRQ handling
  qca_spi: Fix ethtool -G iface tx behavior
  qca_spi: Fix reset behavior

 drivers/net/ethernet/qualcomm/qca_debug.c |  2 -
 drivers/net/ethernet/qualcomm/qca_spi.c   | 63 +++++++++++++++--------
 2 files changed, 41 insertions(+), 24 deletions(-)

--
2.34.1

The functions qcaspi_netdev_open/close are responsible of request &
free of the SPI interrupt, which wasn't the best choice. Currently
it's possible to trigger a double free of the interrupt by calling
qcaspi_netdev_close() after qcaspi_netdev_open() has failed.
So let us split IRQ allocation & enabling, so we can take advantage
of a device managed IRQ and also fix the issue.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/qualcomm/qca_spi.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index b11a998b2456..b2573eea8a7a 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c

@@ -674,7 +674,6 @@ static int
 qcaspi_netdev_open(struct net_device *dev)
 {
 	struct qcaspi *qca = netdev_priv(dev);
-	int ret = 0;

 	if (!qca)
 		return -EINVAL;

@@ -684,13 +683,7 @@ qcaspi_netdev_open(struct net_device *dev)
 	qca->sync = QCASPI_SYNC_UNKNOWN;
 	qcafrm_fsm_init_spi(&qca->frm_handle);

-	ret = request_irq(qca->spi_dev->irq, qcaspi_intr_handler, 0,
-			  dev->name, qca);
-	if (ret) {
-		netdev_err(dev, "%s: unable to get IRQ %d (irqval=%d).\n",
-			   QCASPI_DRV_NAME, qca->spi_dev->irq, ret);
-		return ret;
-	}
+	enable_irq(qca->spi_dev->irq);

 	/* SPI thread takes care of TX queue */
 	kthread_unpark(qca->spi_thread);

@@ -707,7 +700,7 @@ qcaspi_netdev_close(struct net_device *dev)
 	netif_stop_queue(dev);

 	qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0, wr_verify);
-	free_irq(qca->spi_dev->irq, qca);
+	disable_irq(qca->spi_dev->irq);

 	kthread_park(qca->spi_thread);
 	qcaspi_flush_tx_ring(qca);

@@ -977,6 +970,15 @@ qca_spi_probe(struct spi_device *spi)

 	spi_set_drvdata(spi, qcaspi_devs);

+	ret = devm_request_irq(&spi->dev, spi->irq, qcaspi_intr_handler,
+			       IRQF_NO_AUTOEN, qca->net_dev->name, qca);
+	if (ret) {
+		dev_err(&spi->dev, "Unable to get IRQ %d (irqval=%d).\n",
+			spi->irq, ret);
+		free_netdev(qcaspi_devs);
+		return ret;
+	}
+
 	ret = of_get_ethdev_address(spi->dev.of_node, qca->net_dev);
 	if (ret) {
 		eth_hw_addr_random(qca->net_dev);
--

2.34.1

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(-)

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;
 }

--

2.34.1

The qca_spi driver create/stop the SPI kernel thread in case
of netdev_open/close. This is a big issue because it allows
userspace to prevent from restarting the SPI thread after
ring parameter changes (e.g. signals which stop the thread).
This could be done by terminating a script which changes
the ring parameter in a loop.

So fix this by moving create/stop of the SPI kernel into
the init/uninit ops. The open/close ops could be realized just
by 'park/unpark' the SPI kernel thread.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/qualcomm/qca_spi.c | 35 ++++++++++++++++---------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index bec723028e96..b11a998b2456 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c

@@ -580,6 +580,11 @@ qcaspi_spi_thread(void *data)
 	netdev_info(qca->net_dev, "SPI thread created\n");
 	while (!kthread_should_stop()) {
 		set_current_state(TASK_INTERRUPTIBLE);
+		if (kthread_should_park()) {
+			kthread_parkme();
+			continue;
+		}
+
 		if ((qca->intr_req == qca->intr_svc) &&
 		    !qca->txr.skb[qca->txr.head])
 			schedule();

@@ -679,25 +684,17 @@ qcaspi_netdev_open(struct net_device *dev)
 	qca->sync = QCASPI_SYNC_UNKNOWN;
 	qcafrm_fsm_init_spi(&qca->frm_handle);

-	qca->spi_thread = kthread_run((void *)qcaspi_spi_thread,
-				      qca, "%s", dev->name);
-
-	if (IS_ERR(qca->spi_thread)) {
-		netdev_err(dev, "%s: unable to start kernel thread.\n",
-			   QCASPI_DRV_NAME);
-		return PTR_ERR(qca->spi_thread);
-	}
-
 	ret = request_irq(qca->spi_dev->irq, qcaspi_intr_handler, 0,
 			  dev->name, qca);
 	if (ret) {
 		netdev_err(dev, "%s: unable to get IRQ %d (irqval=%d).\n",
 			   QCASPI_DRV_NAME, qca->spi_dev->irq, ret);
-		kthread_stop(qca->spi_thread);
 		return ret;
 	}

 	/* SPI thread takes care of TX queue */
+	kthread_unpark(qca->spi_thread);
+	wake_up_process(qca->spi_thread);

 	return 0;
 }

@@ -712,8 +709,7 @@ qcaspi_netdev_close(struct net_device *dev)
 	qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0, wr_verify);
 	free_irq(qca->spi_dev->irq, qca);

-	kthread_stop(qca->spi_thread);
-	qca->spi_thread = NULL;
+	kthread_park(qca->spi_thread);
 	qcaspi_flush_tx_ring(qca);

 	return 0;

@@ -807,6 +803,7 @@ static int
 qcaspi_netdev_init(struct net_device *dev)
 {
 	struct qcaspi *qca = netdev_priv(dev);
+	struct task_struct *thread;

 	dev->mtu = QCAFRM_MAX_MTU;
 	dev->type = ARPHRD_ETHER;

@@ -830,6 +827,15 @@ qcaspi_netdev_init(struct net_device *dev)
 		return -ENOBUFS;
 	}

+	thread = kthread_create(qcaspi_spi_thread, qca, "%s", dev->name);
+	if (IS_ERR(thread)) {
+		netdev_err(dev, "%s: unable to start kernel thread.\n",
+			   QCASPI_DRV_NAME);
+		return PTR_ERR(thread);
+	}
+
+	qca->spi_thread = thread;
+
 	return 0;
 }

@@ -838,6 +844,11 @@ qcaspi_netdev_uninit(struct net_device *dev)
 {
 	struct qcaspi *qca = netdev_priv(dev);

+	if (qca->spi_thread) {
+		kthread_stop(qca->spi_thread);
+		qca->spi_thread = NULL;
+	}
+
 	kfree(qca->rx_buffer);
 	qca->buffer_size = 0;
 	dev_kfree_skb(qca->rx_skb);
--

2.34.1

In case of a reset triggered by the QCA7000 itself, the behavior of the
qca_spi driver was not quite correct:
- in case of a pending RX frame decoding the drop counter must be
  incremented and decoding state machine reseted
- also the reset counter must always be incremented regardless of sync
  state

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/qualcomm/qca_spi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index b2573eea8a7a..844d255f5d55 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c

@@ -613,11 +613,17 @@ qcaspi_spi_thread(void *data)
 			if (intr_cause & SPI_INT_CPU_ON) {
 				qcaspi_qca7k_sync(qca, QCASPI_EVENT_CPUON);

+				/* Frame decoding in progress */
+				if (qca->frm_handle.state != qca->frm_handle.init)
+					qca->net_dev->stats.rx_dropped++;
+
+				qcafrm_fsm_init_spi(&qca->frm_handle);
+				qca->stats.device_reset++;
+
 				/* not synced. */
 				if (qca->sync != QCASPI_SYNC_READY)
 					continue;

-				qca->stats.device_reset++;
 				netif_wake_queue(qca->net_dev);
 				netif_carrier_on(qca->net_dev);
 			}
--

2.34.1

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

The above looks racy: after 'request_irq()' the interrupt handler can
raise an irq before the thread being unparked.

Additionally I think you can drop the 'if (qca->spi_thread)' in
qcaspi_intr_handler()

Cheers,

Paolo

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

The functions qcaspi_netdev_open/close are responsible of request &
free of the SPI interrupt, which wasn't the best choice. Currently
it's possible to trigger a double free of the interrupt by calling
qcaspi_netdev_close() after qcaspi_netdev_open() has failed.
So let us split IRQ allocation & enabling, so we can take advantage
of a device managed IRQ and also fix the issue.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

The change makes sense, but the changelog is confusing. 

qcaspi_netdev_close() and qcaspi_netdev_open() are invoked only via
ndo_open and ndo_close(), right? So qcaspi_netdev_close() will never be
invoked qcaspi_netdev_open(), failure - that is when IFF_UP is not set.

Cheers,

Paolo

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().

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:26 schrieb Paolo Abeni:

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

quoted

The qca_spi driver create/stop the SPI kernel thread in case
of netdev_open/close. This is a big issue because it allows
userspace to prevent from restarting the SPI thread after
ring parameter changes (e.g. signals which stop the thread).
This could be done by terminating a script which changes
the ring parameter in a loop.

So fix this by moving create/stop of the SPI kernel into
the init/uninit ops. The open/close ops could be realized just
by 'park/unpark' the SPI kernel thread.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
  drivers/net/ethernet/qualcomm/qca_spi.c | 35 ++++++++++++++++---------
  1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index bec723028e96..b11a998b2456 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c

@@ -580,6 +580,11 @@ qcaspi_spi_thread(void *data)
  	netdev_info(qca->net_dev, "SPI thread created\n");
  	while (!kthread_should_stop()) {
  		set_current_state(TASK_INTERRUPTIBLE);
+		if (kthread_should_park()) {
+			kthread_parkme();
+			continue;
+		}
+
  		if ((qca->intr_req == qca->intr_svc) &&
  		    !qca->txr.skb[qca->txr.head])
  			schedule();

@@ -679,25 +684,17 @@ qcaspi_netdev_open(struct net_device *dev)
  	qca->sync = QCASPI_SYNC_UNKNOWN;
  	qcafrm_fsm_init_spi(&qca->frm_handle);

-	qca->spi_thread = kthread_run((void *)qcaspi_spi_thread,
-				      qca, "%s", dev->name);
-
-	if (IS_ERR(qca->spi_thread)) {
-		netdev_err(dev, "%s: unable to start kernel thread.\n",
-			   QCASPI_DRV_NAME);
-		return PTR_ERR(qca->spi_thread);
-	}
-
  	ret = request_irq(qca->spi_dev->irq, qcaspi_intr_handler, 0,
  			  dev->name, qca);
  	if (ret) {
  		netdev_err(dev, "%s: unable to get IRQ %d (irqval=%d).\n",
  			   QCASPI_DRV_NAME, qca->spi_dev->irq, ret);
-		kthread_stop(qca->spi_thread);
  		return ret;
  	}

  	/* SPI thread takes care of TX queue */
+	kthread_unpark(qca->spi_thread);
+	wake_up_process(qca->spi_thread);

The above looks racy: after 'request_irq()' the interrupt handler can
raise an irq before the thread being unparked.

yes fixing the whole resource allocation issue requires patch 1 and 2
applied, which should avoid the race. But i didn't want to combine both
patches to keep it applicable for stable. My thought was that 2 smaller
patches are more acceptable than a big one.

Should i squash them?

My concern is about the amount of affected devices. The QCA7000 is used
mostly in EV charging stations and EVs. I don't how many of them use
this driver.

Additionally I think you can drop the 'if (qca->spi_thread)' in
qcaspi_intr_handler()

Thanks i will check that for the next version.

Cheers,

Paolo

Hi Paolo,

Am 23.11.23 um 12:37 schrieb Paolo Abeni:

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

quoted

The functions qcaspi_netdev_open/close are responsible of request &
free of the SPI interrupt, which wasn't the best choice. Currently
it's possible to trigger a double free of the interrupt by calling
qcaspi_netdev_close() after qcaspi_netdev_open() has failed.
So let us split IRQ allocation & enabling, so we can take advantage
of a device managed IRQ and also fix the issue.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

The change makes sense, but the changelog is confusing.

qcaspi_netdev_close() and qcaspi_netdev_open() are invoked only via
ndo_open and ndo_close(), right? So qcaspi_netdev_close() will never be
invoked qcaspi_netdev_open(), failure - that is when IFF_UP is not set.

sorry, i missed to mention an important part. This issue is partly
connected to patch 3.
Please look at qcaspi_set_ringparam() which also call ndo_close() and
ndo_open(). If you only apply patch 3 you could trigger this issue by
running the following script, interrupt via Strg+C and start again:

#!/bin/bash

while [ true ]; do
   ethtool -G eth1 tx 8
   ethtool -g eth1
   ethtool -G eth1 tx 10
done


[   75.713471] qcaspi spi1.0 eth1: SPI thread exit
[   75.721814] qcaspi spi1.0 eth1: SPI thread created
[   76.795239] qcaspi spi1.0 eth1: SPI thread exit
[   76.815801] qcaspi spi1.0 eth1: SPI thread created
[   77.915872] qcaspi spi1.0 eth1: SPI thread exit
[   77.933982] qcaspi spi1.0 eth1: SPI thread created
[   79.036024] qcaspi spi1.0 eth1: SPI thread exit
[   79.055595] qcaspi spi1.0 eth1: SPI thread created
[   80.076223] qcaspi spi1.0 eth1: SPI thread exit
[   80.097305] qcaspi spi1.0 eth1: SPI thread created
[   81.196471] qcaspi spi1.0 eth1: SPI thread exit
[   81.217351] qcaspi spi1.0 eth1: SPI thread created
[   82.316592] qcaspi spi1.0 eth1: SPI thread exit
[   82.336963] qcaspi spi1.0 eth1: SPI thread created
[   83.436864] qcaspi spi1.0 eth1: SPI thread exit
[   83.461252] qcaspi spi1.0 eth1: SPI thread created
[   84.556950] qcaspi spi1.0 eth1: SPI thread exit
[   84.575897] qcaspi spi1.0 eth1: SPI thread created
[   85.677105] qcaspi spi1.0 eth1: SPI thread exit
[   85.695061] qcaspi spi1.0 eth1: SPI thread created
[   86.717215] qcaspi spi1.0 eth1: SPI thread exit
[   86.739535] qcaspi spi1.0 eth1: SPI thread created
[   87.837355] qcaspi spi1.0 eth1: SPI thread exit
<-- Strg + C
[   87.841072] qcaspi spi1.0 eth1: qcaspi: unable to start kernel thread.
root@tarragon:/srv# ./test_ring_fast.sh
------------[ cut here ]------------
WARNING: CPU: 0 PID: 724 at kernel/irq/manage.c:1887 free_irq+0x23c/0x288
Trying to free already-free IRQ 73
CPU: 0 PID: 724 Comm: ethtool Not tainted
6.1.49-chargebyte-00029-g8c38d497af8a-dirty #108
Hardware name: Freescale i.MX6 Ultralite (Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x24/0x2c
  dump_stack_lvl from __warn+0x74/0xbc
  __warn from warn_slowpath_fmt+0xc8/0x120
  warn_slowpath_fmt from free_irq+0x23c/0x288
  free_irq from qcaspi_netdev_close+0x38/0x5c
  qcaspi_netdev_close from qcaspi_set_ringparam+0x48/0x90
  qcaspi_set_ringparam from ethnl_set_rings+0x2dc/0x320
  ethnl_set_rings from genl_rcv_msg+0x2c4/0x344
  genl_rcv_msg from netlink_rcv_skb+0x98/0xfc
  netlink_rcv_skb from genl_rcv+0x20/0x34
  genl_rcv from netlink_unicast+0x114/0x1a4
  netlink_unicast from netlink_sendmsg+0x314/0x340
  netlink_sendmsg from sock_sendmsg_nosec+0x14/0x24
  sock_sendmsg_nosec from __sys_sendto+0xc4/0xf8
  __sys_sendto from ret_fast_syscall+0x0/0x54
Exception stack(0xe115dfa8 to 0xe115dff0)
dfa0:                   b6ed24dc 0000000c 00000003 005c4238 0000002c
00000000
dfc0: b6ed24dc 0000000c b6f6a5a0 00000122 00472e04 005c41f0 00436b60
005c4190
dfe0: 00000122 bec50b68 b6e5f841 b6dd1ae6
---[ end trace 0000000000000000 ]---

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:

quoted

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

quoted

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:01 +0100, Stefan Wahren wrote:

Hi Paolo,

Am 23.11.23 um 12:37 schrieb Paolo Abeni:

quoted

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

quoted

The functions qcaspi_netdev_open/close are responsible of request &
free of the SPI interrupt, which wasn't the best choice. Currently
it's possible to trigger a double free of the interrupt by calling
qcaspi_netdev_close() after qcaspi_netdev_open() has failed.
So let us split IRQ allocation & enabling, so we can take advantage
of a device managed IRQ and also fix the issue.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

The change makes sense, but the changelog is confusing.

qcaspi_netdev_close() and qcaspi_netdev_open() are invoked only via
ndo_open and ndo_close(), right? So qcaspi_netdev_close() will never be
invoked qcaspi_netdev_open(), failure - that is when IFF_UP is not set.

sorry, i missed to mention an important part. This issue is partly
connected to patch 3.
Please look at qcaspi_set_ringparam() which also call ndo_close() and
ndo_open(). 

Ah, I see it now. IMHO root cause of the problem is there. The ethtool
op should not flip the device state. 

A more narrow fix would be to park/unpark the thread inside
set_ringparam() - instead of the whole patch 1 && 2 I suspect.

IMHO the changes in this still make sense - a refactor for net-next.

Cheers,

Paolo

On Fri, 2023-11-24 at 15:17 +0100, Stefan Wahren wrote:

Am 23.11.23 um 12:51 schrieb Paolo Abeni:

quoted

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

quoted

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

quoted

quoted

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;

Hi Paolo,

Am 24.11.23 um 16:33 schrieb Paolo Abeni:

On Fri, 2023-11-24 at 15:01 +0100, Stefan Wahren wrote:

quoted

Hi Paolo,

Am 23.11.23 um 12:37 schrieb Paolo Abeni:

quoted

On Tue, 2023-11-21 at 17:30 +0100, Stefan Wahren wrote:

quoted

The functions qcaspi_netdev_open/close are responsible of request &
free of the SPI interrupt, which wasn't the best choice. Currently
it's possible to trigger a double free of the interrupt by calling
qcaspi_netdev_close() after qcaspi_netdev_open() has failed.
So let us split IRQ allocation & enabling, so we can take advantage
of a device managed IRQ and also fix the issue.

Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

The change makes sense, but the changelog is confusing.

qcaspi_netdev_close() and qcaspi_netdev_open() are invoked only via
ndo_open and ndo_close(), right? So qcaspi_netdev_close() will never be
invoked qcaspi_netdev_open(), failure - that is when IFF_UP is not set.

sorry, i missed to mention an important part. This issue is partly
connected to patch 3.
Please look at qcaspi_set_ringparam() which also call ndo_close() and
ndo_open().

Ah, I see it now. IMHO root cause of the problem is there. The ethtool
op should not flip the device state.

A more narrow fix would be to park/unpark the thread inside
set_ringparam() - instead of the whole patch 1 && 2 I suspect.

before i send a complete new version of this series, could you please
have a look at this replacement for patch 1 & 2:

qca_debug: Prevent crash on TX ring changes

The qca_spi driver stop and restart the SPI kernel thread
(via ndo_stop & ndo_open) in case of TX ring changes. This is
a big issue because it allows userspace to prevent restart of
the SPI kernel thread (via signals). A subsequent change of
TX ring wrongly assume a valid spi_thread pointer which result
in a crash.

So prevent this by stopping the network queue and temporary park
the SPI thread. Because this could happen during transmission
we also need to call qcaspi_flush_tx_ring().

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 | 17 ++++++++++++-----
  drivers/net/ethernet/qualcomm/qca_spi.c   |  7 ++++++-
  drivers/net/ethernet/qualcomm/qca_spi.h   |  2 ++
  3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/qca_debug.c

b/drivers/net/ethernet/qualcomm/qca_debug.c
index f62c39544e08..478ab3ce949d 100644

--- a/drivers/net/ethernet/qualcomm/qca_debug.c
+++ b/drivers/net/ethernet/qualcomm/qca_debug.c

@@ -263,22 +263,29 @@ qcaspi_set_ringparam(struct net_device *dev,

struct ethtool_ringparam *ring,
               struct kernel_ethtool_ringparam *kernel_ring,
               struct netlink_ext_ack *extack)
  {
-    const struct net_device_ops *ops = dev->netdev_ops;
      struct qcaspi *qca = netdev_priv(dev);
+    bool queue_active = !netif_queue_stopped(dev);

      if ((ring->rx_pending) ||
          (ring->rx_mini_pending) ||
          (ring->rx_jumbo_pending))
          return -EINVAL;

-    if (netif_running(dev))
-        ops->ndo_stop(dev);
+    if (queue_active)
+        netif_stop_queue(dev);

+    if (qca->spi_thread)
+        kthread_park(qca->spi_thread);
+
+    qcaspi_flush_tx_ring(qca);
      qca->txr.count = max_t(u32, ring->tx_pending, TX_RING_MIN_LEN);
      qca->txr.count = min_t(u16, qca->txr.count, TX_RING_MAX_LEN);

-    if (netif_running(dev))
-        ops->ndo_open(dev);
+    if (qca->spi_thread)
+        kthread_unpark(qca->spi_thread);
+
+    if (queue_active)
+        netif_wake_queue(dev);

      return 0;
  }

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c

b/drivers/net/ethernet/qualcomm/qca_spi.c
index d0578530dfbc..2ebe9834a1d3 100644

--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c

@@ -468,7 +468,7 @@ qcaspi_tx_ring_has_space(struct tx_ring *txr)
   *   call from the qcaspi_spi_thread.
   */

-static void
+void
  qcaspi_flush_tx_ring(struct qcaspi *qca)
  {
      int i;

@@ -581,6 +581,11 @@ qcaspi_spi_thread(void *data)
      netdev_info(qca->net_dev, "SPI thread created\n");
      while (!kthread_should_stop()) {
          set_current_state(TASK_INTERRUPTIBLE);
+        if (kthread_should_park()) {
+            kthread_parkme();
+            continue;
+        }
+
          if ((qca->intr_req == qca->intr_svc) &&
              !qca->txr.skb[qca->txr.head])
              schedule();

diff --git a/drivers/net/ethernet/qualcomm/qca_spi.h

b/drivers/net/ethernet/qualcomm/qca_spi.h
index 3067356106f0..95d7306e58e9 100644

--- a/drivers/net/ethernet/qualcomm/qca_spi.h
+++ b/drivers/net/ethernet/qualcomm/qca_spi.h

@@ -107,4 +107,6 @@ struct qcaspi {
      u16 burst_len;
  };

+void qcaspi_flush_tx_ring(struct qcaspi *qca);
+
  #endif /* _QCA_SPI_H */
--

2.34.1

IMHO the changes in this still make sense - a refactor for net-next.

Cheers,

Paolo