On Sun, Mar 07, 2021 at 03:43:13PM +0100, Lukas Wunner wrote:
> On Mon, Mar 01, 2021 at 01:54:05PM +0000, Mark Brown wrote:
> > On Mon, Mar 01, 2021 at 07:56:11PM +0800, Jay Fang wrote:
> > > + ret = devm_request_irq(dev, hs->irq, hisi_spi_irq, IRQF_SHARED,
> > > + dev_name(dev), master);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to get IRQ=%d, ret=%d\n", hs->irq, ret);
> > > + return ret;
> > > + }
> > This will free the IRQ *after* the controller is unregistered, it's
> > better to manually free the interrupt
> Transfers may still be ongoing until spi_unregister_controller() returns.
> (It's called from devres_release_all() in this case.) Since the IRQ is
> presumably necessary to handle those transfers, freeing the IRQ after
> unregistering is actually correct. So the code looks fine in principle.
Well, it should just time out which is annoying.
> However, because the IRQ is requested with IRQF_SHARED, the handler may
> be invoked at any time, even after the controller has been unregistered.
> It is therefore necessary to quiesce the SPI controller's interrupt on
> unregistering and it is also necessary to check in the IRQ handler whether
> an interrupt is actually pending (and bail out if not).
It's also important and even more of a concern that even if there is a
valid interrupt the handler doesn't try to use structures that might
have been deallocated before the handler uses it as this controller
does, that will segfault which is more serious.
Attachment:
signature.asc
Description: PGP signature
