[PATCH] can: mcp251xfd: mcp251xfd_open(): request IRQ as shared

[PATCH] can: mcp251xfd: mcp251xfd_open(): request IRQ as shared

[Marc Kleine-Budde]

The driver's IRQ handler supports shared IRQs, so request a shared IRQ
handler.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 90b06052549d..2b1e57552e1c 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -2527,8 +2527,8 @@ static int mcp251xfd_open(struct net_device *ndev)
 	can_rx_offload_enable(&priv->offload);
 
 	err = request_threaded_irq(spi->irq, NULL, mcp251xfd_irq,
-				   IRQF_ONESHOT, dev_name(&spi->dev),
-				   priv);
+				   IRQF_SHARED | IRQF_ONESHOT,
+				   dev_name(&spi->dev), priv);
 	if (err)
 		goto out_can_rx_offload_disable;
 
-- 
2.30.2

Re: [PATCH] can: mcp251xfd: mcp251xfd_open(): request IRQ as shared

[Manivannan Sadhasivam]

On Sat, Jul 24, 2021 at 10:52:13PM +0200, Marc Kleine-Budde wrote:
> The driver's IRQ handler supports shared IRQs, so request a shared IRQ
> handler.
> 

I don't see any issue with the idea but I'd like to understand the requirement
for it. Usually the IRQ lines are shared when multiple devices use them
physically. For instance, a MFD device using a single GPIO for all of its
functions. But I don't see any sort of requirement like that here.

Making the IRQ lines shared will only induce latency IMO.

Thanks,
Mani

> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index 90b06052549d..2b1e57552e1c 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -2527,8 +2527,8 @@ static int mcp251xfd_open(struct net_device *ndev)
>  	can_rx_offload_enable(&priv->offload);
>  
>  	err = request_threaded_irq(spi->irq, NULL, mcp251xfd_irq,
> -				   IRQF_ONESHOT, dev_name(&spi->dev),
> -				   priv);
> +				   IRQF_SHARED | IRQF_ONESHOT,
> +				   dev_name(&spi->dev), priv);
>  	if (err)
>  		goto out_can_rx_offload_disable;
>  
> -- 
> 2.30.2
> 
> 

Re: [PATCH] can: mcp251xfd: mcp251xfd_open(): request IRQ as shared

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On 27.07.2021 12:47:17, Manivannan Sadhasivam wrote:
> On Sat, Jul 24, 2021 at 10:52:13PM +0200, Marc Kleine-Budde wrote:
> > The driver's IRQ handler supports shared IRQs, so request a shared IRQ
> > handler.
> 
> I don't see any issue with the idea but I'd like to understand the
> requirement for it.

Hardware designers might come up with strange ideas, so better be
prepared for this. :)

But seriously, there's a group of people trying to bring the mcp251xfd
driver to work on ACPI based systems. Having a shared mcp251xfd IRQ
handler will (hopefully) help them during debugging.

I've written the IRQ handler to properly only return IRQ_HANDLED if
there really was an interrupt, this means it should be capable running
as a shared IRQ handler. I've tested it and it works. So let the driver
request a shared IRQ handler.

> Usually the IRQ lines are shared when multiple devices use them
> physically. For instance, a MFD device using a single GPIO for all of its
> functions. But I don't see any sort of requirement like that here.

Indeed there is really no good reason to do so, but it works. For
testing I've connected 2 mcp2518fd to the same IRQ line and the kernel
runs both IRQ handlers an interrupt is triggered. If course this brings
the overhead of an additional SPI transfer per IRQ, but it works.

> Making the IRQ lines shared will only induce latency IMO.

ACK - you will not get better performance compared to separate IRQ lines
:) But if you don't use shared interrupts this change doesn't make the
driver or system any slower or induce latency.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: mcp251xfd: mcp251xfd_open(): request IRQ as shared

[Manivannan Sadhasivam]

On Tue, Jul 27, 2021 at 09:46:17AM +0200, Marc Kleine-Budde wrote:
> On 27.07.2021 12:47:17, Manivannan Sadhasivam wrote:
> > On Sat, Jul 24, 2021 at 10:52:13PM +0200, Marc Kleine-Budde wrote:
> > > The driver's IRQ handler supports shared IRQs, so request a shared IRQ
> > > handler.
> > 
> > I don't see any issue with the idea but I'd like to understand the
> > requirement for it.
> 
> Hardware designers might come up with strange ideas, so better be
> prepared for this. :)
> 
> But seriously, there's a group of people trying to bring the mcp251xfd
> driver to work on ACPI based systems. Having a shared mcp251xfd IRQ
> handler will (hopefully) help them during debugging.
> 

Oh, that's very interesting! ACPI on a ECU :)

> I've written the IRQ handler to properly only return IRQ_HANDLED if
> there really was an interrupt, this means it should be capable running
> as a shared IRQ handler. I've tested it and it works. So let the driver
> request a shared IRQ handler.
> 

I had one more look at it and looks fine to me. The driver authors tend to
return IRQ_HANDLED even during error cases (especially for devices sitting on
buses like i2c, spi) but I hate it. Why would anyone want to enable the
interrupt for the device if it can't be communicated in the ISR?

But you did return IRQ_NONE for error cases, so fine.

> > Usually the IRQ lines are shared when multiple devices use them
> > physically. For instance, a MFD device using a single GPIO for all of its
> > functions. But I don't see any sort of requirement like that here.
> 
> Indeed there is really no good reason to do so, but it works. For
> testing I've connected 2 mcp2518fd to the same IRQ line and the kernel
> runs both IRQ handlers an interrupt is triggered. If course this brings
> the overhead of an additional SPI transfer per IRQ, but it works.
> 
> > Making the IRQ lines shared will only induce latency IMO.
> 
> ACK - you will not get better performance compared to separate IRQ lines
> :) But if you don't use shared interrupts this change doesn't make the
> driver or system any slower or induce latency.
> 

Ack. Feel free to add,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> regards,
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |