* [PATCH] net: eepro testing positive EBUSY return by request_irq()? @ 2010-12-31 14:47 roel kluin 2010-12-31 15:27 ` Ben Hutchings 0 siblings, 1 reply; 5+ messages in thread From: roel kluin @ 2010-12-31 14:47 UTC (permalink / raw) To: davem, netdev, Andrew Morton, LKML Fix -EBUSY test for request_irq(). Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- drivers/net/eepro.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I just found this in the code, no bug was observed. Is this patch needed? the test for an -EBUSY return by request_irq() occurs much more frequently in kernel code. diff --git a/drivers/net/eepro.c b/drivers/net/eepro.c index 7c82631..47cfecb 100644 --- a/drivers/net/eepro.c +++ b/drivers/net/eepro.c @@ -920,7 +920,7 @@ static int eepro_grab_irq(struct net_device *dev) eepro_sw2bank0(ioaddr); /* Switch back to Bank 0 */ - if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != EBUSY) { + if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != -EBUSY) { unsigned long irq_mask; /* Twinkle the interrupt, and check if it's seen */ irq_mask = probe_irq_on(); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: eepro testing positive EBUSY return by request_irq()? 2010-12-31 14:47 [PATCH] net: eepro testing positive EBUSY return by request_irq()? roel kluin @ 2010-12-31 15:27 ` Ben Hutchings 2011-01-02 14:52 ` roel kluin 0 siblings, 1 reply; 5+ messages in thread From: Ben Hutchings @ 2010-12-31 15:27 UTC (permalink / raw) To: roel kluin; +Cc: davem, netdev, Andrew Morton, LKML [-- Attachment #1: Type: text/plain, Size: 1400 bytes --] On Fri, 2010-12-31 at 15:47 +0100, roel kluin wrote: > Fix -EBUSY test for request_irq(). > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > drivers/net/eepro.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > I just found this in the code, no bug was observed. Is this patch needed? the test > for an -EBUSY return by request_irq() occurs much more frequently in kernel code. > > diff --git a/drivers/net/eepro.c b/drivers/net/eepro.c > index 7c82631..47cfecb 100644 > --- a/drivers/net/eepro.c > +++ b/drivers/net/eepro.c > @@ -920,7 +920,7 @@ static int eepro_grab_irq(struct net_device *dev) > > eepro_sw2bank0(ioaddr); /* Switch back to Bank 0 */ > > - if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != EBUSY) { > + if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != -EBUSY) { > unsigned long irq_mask; > /* Twinkle the interrupt, and check if it's seen */ > irq_mask = probe_irq_on(); This condition is completely bogus - request_irq() with a NULL handler now returns -EINVAL before even checking whether the IRQ is in use. The code should be fixed along the lines of what I did for 3c503 in commit b0cf4dfb7cd21556efd9a6a67edcba0840b4d98d. The e2100 and hp net drivers have the same bug. Ben. -- Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: eepro testing positive EBUSY return by request_irq()? 2010-12-31 15:27 ` Ben Hutchings @ 2011-01-02 14:52 ` roel kluin 2011-01-02 19:51 ` Ben Hutchings 2011-01-03 19:37 ` David Miller 0 siblings, 2 replies; 5+ messages in thread From: roel kluin @ 2011-01-02 14:52 UTC (permalink / raw) To: Ben Hutchings; +Cc: davem, netdev, Andrew Morton, LKML >> - if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != EBUSY) { >> + if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != -EBUSY) { > This condition is completely bogus - request_irq() with a NULL handler > now returns -EINVAL before even checking whether the IRQ is in use. The > code should be fixed along the lines of what I did for 3c503 in commit > b0cf4dfb7cd21556efd9a6a67edcba0840b4d98d. > > The e2100 and hp net drivers have the same bug. > > Ben. I've made an attempt to fix the mentioned issues, but I'm not sure it is complete. The commit made some other changes as well which didn't seem appropriate here to me. And I didn't compile or run test this. As a side note, in the function changed by the commit you mentioned, el2_open(), it appears that in current git, boolean `seen' cannot ever become true, can it? So it loops forever until the request_irq() returns an error other than -EBUSY. Not sure how it should be fixed though. ------------->8-------------------8<---------------- Handle errors returned by request_irq() appropriately Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- drivers/net/e2100.c | 14 +++++++++----- drivers/net/eepro.c | 26 +++++++++++++++----------- drivers/net/hp.c | 25 ++++++++++++++----------- 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/drivers/net/e2100.c b/drivers/net/e2100.c index 06e72fb..115fa16 100644 --- a/drivers/net/e2100.c +++ b/drivers/net/e2100.c @@ -217,11 +217,15 @@ static int __init e21_probe1(struct net_device *dev, int ioaddr) if (dev->irq < 2) { int irqlist[] = {15, 11, 10, 12, 5, 9, 3, 4}; - for (i = 0; i < ARRAY_SIZE(irqlist); i++) - if (request_irq (irqlist[i], NULL, 0, "bogus", NULL) != -EBUSY) { - dev->irq = irqlist[i]; - break; - } + for (i = 0; i < ARRAY_SIZE(irqlist); i++) { + retval = request_irq (irqlist[i], NULL, 0, "bogus", NULL); + if (retval != -EBUSY) + continue; + if (retval < 0) + goto out; + dev->irq = irqlist[i]; + break; + } if (i >= ARRAY_SIZE(irqlist)) { printk(" unable to get IRQ %d.\n", dev->irq); retval = -EAGAIN; diff --git a/drivers/net/eepro.c b/drivers/net/eepro.c index 7c82631..24ff522 100644 --- a/drivers/net/eepro.c +++ b/drivers/net/eepro.c @@ -897,6 +897,7 @@ static int eepro_grab_irq(struct net_device *dev) { int irqlist[] = { 3, 4, 5, 7, 9, 10, 11, 12, 0 }; int *irqp = irqlist, temp_reg, ioaddr = dev->base_addr; + int retval; eepro_sw2bank1(ioaddr); /* be CAREFUL, BANK 1 now */ @@ -919,21 +920,24 @@ static int eepro_grab_irq(struct net_device *dev) outb((temp_reg & 0xf8) | irqrmap[*irqp], ioaddr + INT_NO_REG); eepro_sw2bank0(ioaddr); /* Switch back to Bank 0 */ + retval = request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev); + if (retval == -EBUSY) + continue; + if (retval < 0) + return retval; - if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != EBUSY) { - unsigned long irq_mask; - /* Twinkle the interrupt, and check if it's seen */ - irq_mask = probe_irq_on(); + unsigned long irq_mask; + /* Twinkle the interrupt, and check if it's seen */ + irq_mask = probe_irq_on(); - eepro_diag(ioaddr); /* RESET the 82595 */ - mdelay(20); + eepro_diag(ioaddr); /* RESET the 82595 */ + mdelay(20); - if (*irqp == probe_irq_off(irq_mask)) /* It's a good IRQ line */ - break; + if (*irqp == probe_irq_off(irq_mask)) /* It's a good IRQ line */ + break; - /* clear all interrupts */ - eepro_clear_int(ioaddr); - } + /* clear all interrupts */ + eepro_clear_int(ioaddr); } while (*++irqp); eepro_sw2bank1(ioaddr); /* Switch back to Bank 1 */ diff --git a/drivers/net/hp.c b/drivers/net/hp.c index d15d2f2..aeb68a0 100644 --- a/drivers/net/hp.c +++ b/drivers/net/hp.c @@ -167,17 +167,20 @@ static int __init hp_probe1(struct net_device *dev, int ioaddr) int *irqp = wordmode ? irq_16list : irq_8list; do { int irq = *irqp; - if (request_irq (irq, NULL, 0, "bogus", NULL) != -EBUSY) { - unsigned long cookie = probe_irq_on(); - /* Twinkle the interrupt, and check if it's seen. */ - outb_p(irqmap[irq] | HP_RUN, ioaddr + HP_CONFIGURE); - outb_p( 0x00 | HP_RUN, ioaddr + HP_CONFIGURE); - if (irq == probe_irq_off(cookie) /* It's a good IRQ line! */ - && request_irq (irq, eip_interrupt, 0, DRV_NAME, dev) == 0) { - printk(" selecting IRQ %d.\n", irq); - dev->irq = *irqp; - break; - } + retval = request_irq (irq, NULL, 0, "bogus", NULL); + if (retval == -EBUSY) + continue; + if (retval < 0) + goto out; + unsigned long cookie = probe_irq_on(); + /* Twinkle the interrupt, and check if it's seen. */ + outb_p(irqmap[irq] | HP_RUN, ioaddr + HP_CONFIGURE); + outb_p( 0x00 | HP_RUN, ioaddr + HP_CONFIGURE); + if (irq == probe_irq_off(cookie) /* It's a good IRQ line! */ + && request_irq (irq, eip_interrupt, 0, DRV_NAME, dev) == 0) { + printk(" selecting IRQ %d.\n", irq); + dev->irq = *irqp; + break; } } while (*++irqp); if (*irqp == 0) { ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: eepro testing positive EBUSY return by request_irq()? 2011-01-02 14:52 ` roel kluin @ 2011-01-02 19:51 ` Ben Hutchings 2011-01-03 19:37 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: Ben Hutchings @ 2011-01-02 19:51 UTC (permalink / raw) To: roel kluin; +Cc: davem, netdev, Andrew Morton, LKML [-- Attachment #1: Type: text/plain, Size: 2115 bytes --] On Sun, 2011-01-02 at 15:52 +0100, roel kluin wrote: > >> - if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != EBUSY) { > >> + if (request_irq (*irqp, NULL, IRQF_SHARED, "bogus", dev) != -EBUSY) { > > > This condition is completely bogus - request_irq() with a NULL handler > > now returns -EINVAL before even checking whether the IRQ is in use. The > > code should be fixed along the lines of what I did for 3c503 in commit > > b0cf4dfb7cd21556efd9a6a67edcba0840b4d98d. > > > > The e2100 and hp net drivers have the same bug. > > > > Ben. > > I've made an attempt to fix the mentioned issues, but I'm not sure it is complete. > The commit made some other changes as well which didn't seem appropriate here to me. They are absolutely necessary. In the bad old days before plug-and-play, some expansion cards would have jumpers to configure the IRQ number and no software mechanism to set or query it. Users were expected to specify the IRQ to the driver explicitly. probe_irq_{on,off}() allow drivers to avoid this by triggering an IRQ and checking which was received. That assumes that there are no IRQ conflicts, and it only works reliably in a quiescent system. These drivers have been abusing request_irq(), probe_irq_{on,off}() to do resource allocation for hardware that *does* have a software mechanism to set the IRQ number. This has become less reliable as driver loading has been made more dynamic, and is now broken completely due to the change in request_irq(). I believe the correct way to do this allocation is what I did in 3c503. Possibly it's worth adding a generic function. > And I didn't compile or run test this. I compiled but didn't have hardware to test. > As a side note, in the function changed by the commit you mentioned, el2_open(), it > appears that in current git, boolean `seen' cannot ever become true, can it? [...] The IRQ handler data is set to &seen and el2_probe_interrupt() uses that pointer to set it. Ben. -- Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: eepro testing positive EBUSY return by request_irq()? 2011-01-02 14:52 ` roel kluin 2011-01-02 19:51 ` Ben Hutchings @ 2011-01-03 19:37 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2011-01-03 19:37 UTC (permalink / raw) To: roel.kluin; +Cc: ben, netdev, akpm, linux-kernel From: roel kluin <roel.kluin@gmail.com> Date: Sun, 02 Jan 2011 15:52:23 +0100 > + for (i = 0; i < ARRAY_SIZE(irqlist); i++) { > + retval = request_irq (irqlist[i], NULL, 0, "bogus", NULL); > + if (retval != -EBUSY) > + continue; > + if (retval < 0) > + goto out; > + dev->irq = irqlist[i]; > + break; This series of tests don't make much sense. If we get to the "retval < 0" check, retval must be -EBUSY. So at best it's superfluous, at worst it's confusing. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-01-03 19:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-31 14:47 [PATCH] net: eepro testing positive EBUSY return by request_irq()? roel kluin 2010-12-31 15:27 ` Ben Hutchings 2011-01-02 14:52 ` roel kluin 2011-01-02 19:51 ` Ben Hutchings 2011-01-03 19:37 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).