[net-next] bnx2x: handle spurious interrupts

In scenarios in which a previous driver was removed without proper cleanup
(e.g., kdump), it is possible for the chip to generate an interrupt without
any apparent reason once interrupts are enabled.

This patch causes the driver to ignore any interrupt which arrives in an
inappropriate time, i.e., when the driver has not yet properly configured
its interrupt routines.

Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
Hi Dave,

This was previously sent as part of a patch series - which you rejected due
to this patch. As the merge window opened the following day, I did not
argue on behalf of this patch (but I intend to do so now).

Your issue with the patch was that we've encountered a NULL pointer dereference
once said interrupt was generated, as bnx2x requests the IRQ from the kernel
prior to the initialization of all structs required for its handling.

However, my claim is that the 2 issues are orthogonal -
Perhaps this area in the bnx2x needs to be changed (even though no interrupts
should be generated until all structs are initialized), but the behaviour
added in current patch would still be unchanged.

Since the interrupt is unintentional, the proper driver behaviour should still
be to ignore it, as ACKing the HW for it might be harmful.
Since the driver must request the IRQ prior to enabling HW generation of
interrupts, it would still need to ignore every interrupt arriving during that
interval.

Thanks,
Yuval
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |   11 +++++++++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    3 +++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c |    3 ++-
 4 files changed, 17 insertions(+), 1 deletions(-)

From: "Yuval Mintz" <yuvalmin@broadcom.com>
Date: Wed, 27 Mar 2013 20:19:39 +0200

> Since the driver must request the IRQ prior to enabling HW generation of
> interrupts, it would still need to ignore every interrupt arriving during that
> interval.

This is the real issue.

request_irq() must not be invoked until you are ready to handle
interrupts, and this means initializing any and all software
datastructures that might be accessed in the interrupt handler.

If this requirement is met, seeing NULL pointers is not possible.

I still reject this patch, please fix the fundamental issue instead,
thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > Since the driver must request the IRQ prior to enabling HW generation of
> > interrupts, it would still need to ignore every interrupt arriving during that
> > interval.
> 
> This is the real issue.
> 
> request_irq() must not be invoked until you are ready to handle
> interrupts, and this means initializing any and all software
> datastructures that might be accessed in the interrupt handler.
> 
> If this requirement is met, seeing NULL pointers is not possible.

I agree we won't be seeing NULL pointers, but that's only the manifestation of the
problem, not the problem itself - if we would have handled those spurious 
interrupts in the same manner we do regular ones, we might leave HW in an 
unsteady state.

The real issue (well, both are real but this is the one THIS patch tries
solving, while the one you describe is the one I've named `orthogonal') is that 
there exists a scenario in which the HW generates an interrupt although it's 
configured not to do so, and the bnx2x driver shouldn't ACK but rather discard it. 

> I still reject this patch, please fix the fundamental issue instead,
> thanks.

This is of course your prerogative.
We will work on solving both issues.

Thanks,
Yuval

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: "Yuval Mintz" <yuvalmin@broadcom.com>
Date: Wed, 27 Mar 2013 21:08:17 +0000

> The real issue (well, both are real but this is the one THIS patch
> tries solving, while the one you describe is the one I've named
> `orthogonal') is that there exists a scenario in which the HW
> generates an interrupt although it's configured not to do so, and
> the bnx2x driver shouldn't ACK but rather discard it.

Then I'd rather you test for this situation in a cheap and explicit
manner instead of "pointer is NULL, datastructures not setup yet,
must be spurious."

Actually, in drivers, the handling of spurious interrupts is probably
the domain in which I see the most poorly thought out and implemented
patches.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html