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

04.04.2012 16:01, Mark Brown написал:

On Wed, Apr 04, 2012 at 12:18:12PM +0600, Mike Sinkovsky wrote:

quoted

03.04.2012 19:36, Mark Brown написал:

quoted

quoted

...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?

quoted

quoted

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.

quoted

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.

Ok, now I understand - better don't use devm* functions for interrupts, use plain request* in probe() and free_irq() in remove().

Will post v8.


-- 
Mike