Re: [PATCH] spi: Add HiSilicon SPI controller driver support

[Mark Brown <broonie@kernel.org>]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]