Re: [PATCH v6 1/2] Ethernet driver for the WIZnet W5300 chip

[Mark Brown]

On Wed, Apr 04, 2012 at 12:18:12PM +0600, Mike Sinkovsky wrote:
> 03.04.2012 19:36, Mark Brown ÐÐÐÐÑÐÐ:

> >...you use devm_request_threaded_irq() here and rely on it for cleanup.
> >Are you sure there's no possibility of the interrupt firing after you
> >start to tear down the device?

> >By using a specifically threaded IRQ you're also adding a performance
> >overhead for no good reason if you can call netdev_carrier_*() from IRQ
> >context and the GPIO is capable of generating a hard IRQ.  If you use
> >request_any_context_irq() instead then the driver will get a hard IRQ if
> >that's supported.

> There isn't devm* variant of request_any_context_irq(), and using
> plain version looks inconsistent with other resources handling.
> Anyway, this is not performance critical procedure, and latency
> around 100 millisecond is acceptable. Some our boards even don't
> have this gpio at all, and nothing bad happens, just userspace
> doesn't know is carrier on or off.

None of this addresses the primary concern which is that because you're
not (as far as I can tell) ensuring that the interrupt won't fire the
driver might crash if the interrupt fires in between the resources it
needs to handle the interrupt being deallocated and the interrupt being
unregistered.  Managed interrupts are relatively tricky to use because
of this issue, with most things like memory it doesn't matter exactly
when it's deallocated but interrupts can potentially trigger actions
themselves.

If you can use it and devm_request_any_context_irq() doesn't exist it'd
be better to add it.

Attachment: signature.asc
Description: Digital signature