| Message ID | 20231214150944.55808-3-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qca_spi: collection of improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Thu, Dec 14, 2023 at 04:09:34PM +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 because > allocation problems are discovered not during probe. So let us split > IRQ allocation & enabling, so we can take advantage of a device > managed IRQ. Could you replace the kernel thread with a threaded interrupt handler? Andrew
Hi Andrew, Am 17.12.23 um 19:14 schrieb Andrew Lunn: > On Thu, Dec 14, 2023 at 04:09:34PM +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 because >> allocation problems are discovered not during probe. So let us split >> IRQ allocation & enabling, so we can take advantage of a device >> managed IRQ. > Could you replace the kernel thread with a threaded interrupt handler? the kernel thread is responsible for receiving, transmitting and reset handling (there is no GPIO reset in this driver) which must be synchronized along the same SPI interface. The interrupt just signalize a chip reset or a received packet is available. Could you please elaborate this request more in detail: What is the problem with the kernel thread? Why should i use the threaded interrupt as a replacement instead of e.g. workqueue? Please don't get me wrong, but i need to convince my employer for such a big rewrite. Regards > > Andrew
On Sun, Dec 17, 2023 at 08:17:56PM +0100, Stefan Wahren wrote: > Hi Andrew, > > Am 17.12.23 um 19:14 schrieb Andrew Lunn: > > On Thu, Dec 14, 2023 at 04:09:34PM +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 because > > > allocation problems are discovered not during probe. So let us split > > > IRQ allocation & enabling, so we can take advantage of a device > > > managed IRQ. > > Could you replace the kernel thread with a threaded interrupt handler? > the kernel thread is responsible for receiving, transmitting and reset > handling (there is no GPIO reset in this driver) which must be > synchronized along the same SPI interface. The interrupt just signalize > a chip reset or a received packet is available. > > Could you please elaborate this request more in detail: > What is the problem with the kernel thread? > Why should i use the threaded interrupt as a replacement instead of e.g. > workqueue? > > Please don't get me wrong, but i need to convince my employer for such a > big rewrite. I don't know this driver, which is why i asked the question. Its just a suggestion. Maybe it makes no sense. But there have been other SPI based Ethernet drivers which have been simplified by using threaded interrupts rather than a kernel thread or a work queue, since the interrupt core does all the thread management, and in particular the creating and destroying of the thread which drivers often get wrong. Andrew
Hi Andrew, Am 17.12.23 um 23:48 schrieb Andrew Lunn: > On Sun, Dec 17, 2023 at 08:17:56PM +0100, Stefan Wahren wrote: >> Hi Andrew, >> >> Am 17.12.23 um 19:14 schrieb Andrew Lunn: >>> On Thu, Dec 14, 2023 at 04:09:34PM +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 because >>>> allocation problems are discovered not during probe. So let us split >>>> IRQ allocation & enabling, so we can take advantage of a device >>>> managed IRQ. >>> Could you replace the kernel thread with a threaded interrupt handler? >> the kernel thread is responsible for receiving, transmitting and reset >> handling (there is no GPIO reset in this driver) which must be >> synchronized along the same SPI interface. The interrupt just signalize >> a chip reset or a received packet is available. >> >> Could you please elaborate this request more in detail: >> What is the problem with the kernel thread? >> Why should i use the threaded interrupt as a replacement instead of e.g. >> workqueue? >> >> Please don't get me wrong, but i need to convince my employer for such a >> big rewrite. > I don't know this driver, which is why i asked the question. Its just > a suggestion. Maybe it makes no sense. But there have been other SPI > based Ethernet drivers which have been simplified by using threaded > interrupts rather than a kernel thread or a work queue, since the > interrupt core does all the thread management, and in particular the > creating and destroying of the thread which drivers often get wrong. thanks for the explanation. I guess you refer to enc28j60 and i had a look at commit 995585ecdf42 ("net: enc28j60: Use threaded interrupt instead of workqueue"). The fact that the qca_spi driver has a kernel thread which is kind of persistent (see patch 1 of this series) and the request to change this confused me. So your suggestion is more about the interrupt handling and not about the kernel thread which handles the SPI communication. Yes the usage of threaded IRQ makes sense especially this would allow to support level triggered interrupts. But i think this complex change should be a separate series. > Andrew
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c index a9f01fb3455a..88f2468ba676 100644 --- a/drivers/net/ethernet/qualcomm/qca_spi.c +++ b/drivers/net/ethernet/qualcomm/qca_spi.c @@ -669,7 +669,6 @@ static int qcaspi_netdev_open(struct net_device *dev) { struct qcaspi *qca = netdev_priv(dev); - int ret = 0; if (!qca) return -EINVAL; @@ -679,13 +678,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); @@ -702,7 +695,7 @@ qcaspi_netdev_close(struct net_device *dev) kthread_park(qca->spi_thread); qcaspi_write_register(qca, SPI_REG_INTR_ENABLE, 0, wr_verify); - free_irq(qca->spi_dev->irq, qca); + disable_irq(qca->spi_dev->irq); return 0; } @@ -969,6 +962,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);
The functions qcaspi_netdev_open/close are responsible of request & free of the SPI interrupt, which wasn't the best choice because allocation problems are discovered not during probe. So let us split IRQ allocation & enabling, so we can take advantage of a device managed IRQ. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/net/ethernet/qualcomm/qca_spi.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) -- 2.34.1