| Message ID | 20240124223211.4687-2-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qca_spi: collection of improvements | expand |
On Wed, 24 Jan 2024 23:31:58 +0100 Stefan Wahren wrote: > The qca_spi driver create/stop the SPI kernel thread in case > of netdev_open/close. This isn't optimal because there is no > need for such an expensive operation. > > So improve this by moving create/stop of the SPI kernel into > the init/uninit ops. The open/close ops could just > 'park/unpark' the SPI kernel thread. What's the concern? I don't think that creating a thread is all expensive. And we shouldn't have a thread sitting around when the interface isn't use. I mean - if you ask me what's better a small chance that the creation will fail at open or having a parked and unused thread when device is down - I'd pick the former.. But I may well be missing the point. > @@ -825,6 +813,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; > @@ -848,6 +837,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); I'm 90% sure this leaks resources on failure, too. Rest of the series LGTM!
Hi Jakub, Am 26.01.24 um 03:36 schrieb Jakub Kicinski: > On Wed, 24 Jan 2024 23:31:58 +0100 Stefan Wahren wrote: >> The qca_spi driver create/stop the SPI kernel thread in case >> of netdev_open/close. This isn't optimal because there is no >> need for such an expensive operation. >> >> So improve this by moving create/stop of the SPI kernel into >> the init/uninit ops. The open/close ops could just >> 'park/unpark' the SPI kernel thread. > What's the concern? I don't think that creating a thread is all > expensive. And we shouldn't have a thread sitting around when > the interface isn't use. I mean - if you ask me what's better > a small chance that the creation will fail at open or having > a parked and unused thread when device is down - I'd pick > the former.. But I may well be missing the point. there is actually another reason for this change which is not mentioned in the commit message. The pointer spi_thread can have 3 states: - valid thread - error pointer - NULL So the second motivation was to eliminate the possibility of an error pointer by using a local variable. So we only have to care about NULL. I will change this in V4. > >> @@ -825,6 +813,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; >> @@ -848,6 +837,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); > I'm 90% sure this leaks resources on failure, too. Oops Best regards > > Rest of the series LGTM!
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c index 5f3c11fb3fa2..fc272ca7bdca 100644 --- a/drivers/net/ethernet/qualcomm/qca_spi.c +++ b/drivers/net/ethernet/qualcomm/qca_spi.c @@ -697,25 +697,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; } @@ -725,15 +717,11 @@ qcaspi_netdev_close(struct net_device *dev) { struct qcaspi *qca = netdev_priv(dev); - netif_stop_queue(dev); + kthread_park(qca->spi_thread); 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; - qcaspi_flush_tx_ring(qca); - return 0; } @@ -825,6 +813,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; @@ -848,6 +837,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; } @@ -856,6 +854,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);
The qca_spi driver create/stop the SPI kernel thread in case of netdev_open/close. This isn't optimal because there is no need for such an expensive operation. So improve this by moving create/stop of the SPI kernel into the init/uninit ops. The open/close ops could just 'park/unpark' the SPI kernel thread. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/net/ethernet/qualcomm/qca_spi.c | 33 ++++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) -- 2.34.1