Hi,
On Wed, Aug 26, 2015 at 05:15:51PM -0300, Ezequiel Garcia wrote:
<snip>
> >> be prepared to handle it any time, coming from any sources (not only
> >> your device). And CONFIG_DEBUG_SHIRQ does exactly that, in order to
> >> make sure all the drivers passing IRQF_SHARED comply with that rule.
> >
> > you need to be sure of that with non-shared IRQs anyway.
>
> Not entirely. If your IRQ is not shared, then you usually have a register
> to enable or unmask your peripheral interrupts. So the driver is in control
> of when it will get interrupts.
>
> If the IRQ is shared, this won't do. This is what I mean by "shared IRQs
> must be prepared to receive an interrupt any time", in the sense that
> the driver has no way of preventing IRQs (because they may be
> coming from any source).
right, the problem is much less likely on non-shared lines but the fine
that a line is shared or not is a function of HW integration, not the
e.g. USB Controller, so that knowledge really doesn't fit the driver in
a sense.
We might as well get rid of IRQF_SHARED and assume all lines are
shareable.
> In the same sense, shared IRQs handlers need to double-check
> the IRQ is coming to the current device by checking some IRQ
> status register to see if there's pending work.
you should the status register even on non-shared IRQs to catch spurious
right ?
> > Also, an IRQ
> > which isn't shared in SoC A, might become shared in SoC B which uses the
> > same IP.
> >
> >> So you either avoid using devm_request_irq, or you prepare your handler
> >> accordingly to be ready to handle an interrupt _any time_.
> >
> > the handler is ready to handle at any time, what isn't correct is the
> > fact that clocks get gated before IRQ is freed.
> >
> > There should be no such special case as "if your handler is shared,
> > don't use devm_request_*irq()" because if we just disable PM_RUNTIME, it
> > works as expected anyway.
> >
>
> Yeah, I meant to say: if you use devm_request_irq with IRQF_SHARED
> then you _must_ be prepared to get an IRQ *after* your remove() has
> been called.
>
> Let's consider this snippet from tw68:
>
> static irqreturn_t tw68_irq(int irq, void *dev_id)
> {
> struct tw68_dev *dev = dev_id;
> u32 status, orig;
> int loop;
>
> status = orig = tw_readl(TW68_INTSTAT) & dev->pci_irqmask;
Now try to read that register when your clock is gated. That's the
problem I'm talking about. Everything about the handler is functioning
correctly; however clocks are gated in ->remove() and free_irq() is
only called *AFTER* ->remove() has returned.
> [etc]
> }
>
> The IRQ handler accesses the device struct and then
> reads through PCI. So if you use devm_request_irq
> you need to make sure the device struct is still allocated
> after remove(), and the PCI read won't stall or crash.
dude, that's not the problem I'm talking about. I still have my
private_data around, what I don't have is:
_ _
__ _ ___| | ___ ___| | __
/ _` | / __| |/ _ \ / __| |/ /
| (_| | | (__| | (_) | (__| <
\__,_| \___|_|\___/ \___|_|\_\
> Interestingly, tw68 uses devm_request_irq with IRQF_SHARED :-)
>
> Still, I don't think that's a good idea, since it relies on
> the IRQ being freed *before* the device struct.
that's not an issue at all. If you're using devm_request_irq() you're
likely using devm_kzalloc() for the device struct anyway. Also, you
called devm_kzalloc() before devm_request_irq() so IRQ *will* be freed
before your private data; there's nothing wrong there.
--
balbi
Attachment:
signature.asc
Description: Digital signature
