* [PATCH] floppy: request only the ports we actually use
@ 2009-02-05 17:48 Bjorn Helgaas
2009-02-05 17:55 ` Bjorn Helgaas
2009-02-13 8:55 ` [PATCH] floppy: request and release only the ports we actually use Philippe De Muyter
0 siblings, 2 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2009-02-05 17:48 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Philippe De Muyter, Robert Hancock, Adam M Belay,
Len Brown
The floppy driver requests an I/O port it doesn't need, and
sometimes this causes a conflict with a motherboard device
reported by PNPBIOS.
This patch makes the floppy driver request only the ports it
actually uses.
The current floppy driver uses only these ports:
0x3f2 (FD_DOR)
0x3f4 (FD_STATUS)
0x3f5 (FD_DATA)
0x3f7 (FD_DCR/FD_DIR)
but it requests 0x3f2-0x3f5 and 0x3f7, which includes the
unused port 0x3f3.
Some BIOSes report 0x3f3 as a motherboard resource. The PNP system
driver reserves that, which causes a conflict when the floppy driver
requests 0x3f2-0x3f5 later.
Philippe reported that this conflict broke the floppy driver between
2.6.11 and 2.6.22. His PNPBIOS reports these devices:
$ cat 00:07/id 00:07/resources # motherboard device
PNP0c02
state = active
io 0x80-0x80
io 0x10-0x1f
io 0x22-0x3f
io 0x44-0x5f
io 0x90-0x9f
io 0xa2-0xbf
io 0x3f0-0x3f1
io 0x3f3-0x3f3
$ cat 00:03/id 00:03/resources # floppy device
PNP0700
state = active
io 0x3f4-0x3f5
io 0x3f2-0x3f2
Reference:
http://lkml.org/lkml/2009/1/31/162
Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Reported-by: Philippe De Muyter <phdm@macqel.be>
Tested-by: Philippe De Muyter <phdm@macqel.be>
CC: Adam M Belay <abelay@mit.edu>
CC: Robert Hancock <hancockrwd@gmail.com>
---
drivers/block/floppy.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index cf29cc4..44f711b 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4399,15 +4399,20 @@ static int floppy_grab_irq_and_dma(void)
for (fdc = 0; fdc < N_FDC; fdc++) {
if (FDCS->address != -1) {
- if (!request_region(FDCS->address + 2, 4, "floppy")) {
+ if (!request_region(FDCS->address + 2, 1, "floppy")) {
DPRINT("Floppy io-port 0x%04lx in use\n",
FDCS->address + 2);
goto cleanup1;
}
+ if (!request_region(FDCS->address + 4, 2, "floppy")) {
+ DPRINT("Floppy io-port 0x%04lx in use\n",
+ FDCS->address + 4);
+ goto cleanup2;
+ }
if (!request_region(FDCS->address + 7, 1, "floppy DIR")) {
DPRINT("Floppy io-port 0x%04lx in use\n",
FDCS->address + 7);
- goto cleanup2;
+ goto cleanup3;
}
/* address + 6 is reserved, and may be taken by IDE.
* Unfortunately, Adaptec doesn't know this :-(, */
@@ -4432,13 +4437,16 @@ static int floppy_grab_irq_and_dma(void)
fdc = 0;
irqdma_allocated = 1;
return 0;
+cleanup3:
+ release_region(FDCS->address + 4, 2);
cleanup2:
- release_region(FDCS->address + 2, 4);
+ release_region(FDCS->address + 2, 1);
cleanup1:
fd_free_irq();
fd_free_dma();
while (--fdc >= 0) {
- release_region(FDCS->address + 2, 4);
+ release_region(FDCS->address + 2, 1);
+ release_region(FDCS->address + 4, 2);
release_region(FDCS->address + 7, 1);
}
spin_lock_irqsave(&floppy_usage_lock, flags);
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] floppy: request only the ports we actually use 2009-02-05 17:48 [PATCH] floppy: request only the ports we actually use Bjorn Helgaas @ 2009-02-05 17:55 ` Bjorn Helgaas 2009-02-05 22:29 ` Andrew Morton 2009-02-13 8:55 ` [PATCH] floppy: request and release only the ports we actually use Philippe De Muyter 1 sibling, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2009-02-05 17:55 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Philippe De Muyter, Robert Hancock, Adam M Belay, Len Brown On Thursday 05 February 2009 10:48:59 am Bjorn Helgaas wrote: > The floppy driver requests an I/O port it doesn't need, and > sometimes this causes a conflict with a motherboard device > reported by PNPBIOS. > > This patch makes the floppy driver request only the ports it > actually uses. > ... > Philippe reported that this conflict broke the floppy driver between > 2.6.11 and 2.6.22. ... While this is old and BIOS-specific, it is a regression, so we might consider it for 2.6.29. Bjorn ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: request only the ports we actually use 2009-02-05 17:55 ` Bjorn Helgaas @ 2009-02-05 22:29 ` Andrew Morton 2009-02-05 23:43 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Andrew Morton @ 2009-02-05 22:29 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel, phdm, hancockrwd, abelay, lenb, stable On Thu, 5 Feb 2009 10:55:33 -0700 Bjorn Helgaas <bjorn.helgaas@hp.com> wrote: > On Thursday 05 February 2009 10:48:59 am Bjorn Helgaas wrote: > > The floppy driver requests an I/O port it doesn't need, and > > sometimes this causes a conflict with a motherboard device > > reported by PNPBIOS. > > > > This patch makes the floppy driver request only the ports it > > actually uses. > > ... > > Philippe reported that this conflict broke the floppy driver between > > 2.6.11 and 2.6.22. ... > > While this is old and BIOS-specific, it is a regression, so we might > consider it for 2.6.29. > And earlier, surely? I tagged it Cc: <stable@kernel.org> [2.6.25.x, 2.6.26.x, 2.6.27.x, 2.6.28.x] (I'm not sure if 2.6.25.x is still being maintained by Greg & Chris, but other parties are maintaining 2.6.25-based kernels, and such tagging might help them). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: request only the ports we actually use 2009-02-05 22:29 ` Andrew Morton @ 2009-02-05 23:43 ` Bjorn Helgaas 2009-02-06 8:55 ` [PATCH] floppy: release only the ports we actually requested Philippe De Muyter 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2009-02-05 23:43 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, phdm, hancockrwd, abelay, lenb, stable On Thursday 05 February 2009 03:29:48 pm Andrew Morton wrote: > On Thu, 5 Feb 2009 10:55:33 -0700 > Bjorn Helgaas <bjorn.helgaas@hp.com> wrote: > > > On Thursday 05 February 2009 10:48:59 am Bjorn Helgaas wrote: > > > The floppy driver requests an I/O port it doesn't need, and > > > sometimes this causes a conflict with a motherboard device > > > reported by PNPBIOS. > > > > > > This patch makes the floppy driver request only the ports it > > > actually uses. > > > ... > > > Philippe reported that this conflict broke the floppy driver between > > > 2.6.11 and 2.6.22. ... > > > > While this is old and BIOS-specific, it is a regression, so we might > > consider it for 2.6.29. > > And earlier, surely? Sure, I'm OK with that. I haven't heard any complaints, so maybe we want it in -mm for a while before putting it in stable? > I tagged it > > Cc: <stable@kernel.org> [2.6.25.x, 2.6.26.x, 2.6.27.x, 2.6.28.x] > > (I'm not sure if 2.6.25.x is still being maintained by Greg & Chris, > but other parties are maintaining 2.6.25-based kernels, and such > tagging might help them). ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] floppy: release only the ports we actually requested 2009-02-05 23:43 ` Bjorn Helgaas @ 2009-02-06 8:55 ` Philippe De Muyter 2009-02-06 16:20 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Philippe De Muyter @ 2009-02-06 8:55 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, linux-kernel, hancockrwd, abelay, lenb, stable Bjorn, Andrew, -- With the last floppy patch, the floppy driver requests only the ports that it really uses, but the code contains yet places where it releases those unrequested ports. I don't know if it is harmfull, but I think it is cleaner that the parameters of the release_region calls match the request_region ones. Signed-off-by: Philippe De Muyter <phdm@macqel.be> --- a/drivers/block/floppy.c 2009-02-06 08:56:33.000000000 +0100 +++ b/drivers/block/floppy.c 2009-02-06 09:05:55.000000000 +0100 @@ -4274,7 +4274,8 @@ static int __init floppy_init(void) FDCS->rawcmd = 2; if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); + release_region(FDCS->address + 2, 1); + release_region(FDCS->address + 4, 2); release_region(FDCS->address + 7, 1); FDCS->address = -1; FDCS->version = FDC_NONE; @@ -4284,7 +4285,8 @@ static int __init floppy_init(void) FDCS->version = get_fdc_version(); if (FDCS->version == FDC_NONE) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); + release_region(FDCS->address + 2, 1); + release_region(FDCS->address + 4, 2); release_region(FDCS->address + 7, 1); FDCS->address = -1; continue; @@ -4510,7 +4512,8 @@ static void floppy_release_irq_and_dma(v old_fdc = fdc; for (fdc = 0; fdc < N_FDC; fdc++) if (FDCS->address != -1) { - release_region(FDCS->address + 2, 4); + release_region(FDCS->address + 2, 1); + release_region(FDCS->address + 4, 2); release_region(FDCS->address + 7, 1); } fdc = old_fdc; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: release only the ports we actually requested 2009-02-06 8:55 ` [PATCH] floppy: release only the ports we actually requested Philippe De Muyter @ 2009-02-06 16:20 ` Bjorn Helgaas 2009-02-06 17:57 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2009-02-06 16:20 UTC (permalink / raw) To: Philippe De Muyter Cc: Andrew Morton, linux-kernel, hancockrwd, abelay, lenb, stable On Friday 06 February 2009 01:55:01 am Philippe De Muyter wrote: > Bjorn, Andrew, > > -- > > With the last floppy patch, the floppy driver requests only the ports that > it really uses, but the code contains yet places where it releases those > unrequested ports. I don't know if it is harmfull, but I think it is cleaner > that the parameters of the release_region calls match the request_region ones. Doh! Boy, do I feel stupid. I doubt this is in any git trees yet, so I'll combine the two patches and send a cleaner one. Bjorn > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > > --- a/drivers/block/floppy.c 2009-02-06 08:56:33.000000000 +0100 > +++ b/drivers/block/floppy.c 2009-02-06 09:05:55.000000000 +0100 > @@ -4274,7 +4274,8 @@ static int __init floppy_init(void) > FDCS->rawcmd = 2; > if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > - release_region(FDCS->address + 2, 4); > + release_region(FDCS->address + 2, 1); > + release_region(FDCS->address + 4, 2); > release_region(FDCS->address + 7, 1); > FDCS->address = -1; > FDCS->version = FDC_NONE; > @@ -4284,7 +4285,8 @@ static int __init floppy_init(void) > FDCS->version = get_fdc_version(); > if (FDCS->version == FDC_NONE) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > - release_region(FDCS->address + 2, 4); > + release_region(FDCS->address + 2, 1); > + release_region(FDCS->address + 4, 2); > release_region(FDCS->address + 7, 1); > FDCS->address = -1; > continue; > @@ -4510,7 +4512,8 @@ static void floppy_release_irq_and_dma(v > old_fdc = fdc; > for (fdc = 0; fdc < N_FDC; fdc++) > if (FDCS->address != -1) { > - release_region(FDCS->address + 2, 4); > + release_region(FDCS->address + 2, 1); > + release_region(FDCS->address + 4, 2); > release_region(FDCS->address + 7, 1); > } > fdc = old_fdc; > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: release only the ports we actually requested 2009-02-06 16:20 ` Bjorn Helgaas @ 2009-02-06 17:57 ` Bjorn Helgaas 2009-02-09 9:15 ` Philippe De Muyter 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2009-02-06 17:57 UTC (permalink / raw) To: Philippe De Muyter Cc: Andrew Morton, linux-kernel, hancockrwd, abelay, lenb, stable On Friday 06 February 2009 09:20:13 am Bjorn Helgaas wrote: > On Friday 06 February 2009 01:55:01 am Philippe De Muyter wrote: > > Bjorn, Andrew, > > > > -- > > > > With the last floppy patch, the floppy driver requests only the ports that > > it really uses, but the code contains yet places where it releases those > > unrequested ports. I don't know if it is harmfull, but I think it is cleaner > > that the parameters of the release_region calls match the request_region ones. > > Doh! Boy, do I feel stupid. I doubt this is in any git trees yet, > so I'll combine the two patches and send a cleaner one. Philippe, can you give this a once-over? I can compile it, but I don't have a machine with a floppy drive handy to test it. Since you pointed out all the places I forgot to change, I went ahead and factored out the request/release stuff so they're all in one place now. Bjorn diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index cf29cc4..c2b3db4 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -558,6 +558,8 @@ static void process_fd_request(void); static void recalibrate_floppy(void); static void floppy_shutdown(unsigned long); +static int floppy_request_regions(int); +static void floppy_release_regions(int); static int floppy_grab_irq_and_dma(void); static void floppy_release_irq_and_dma(void); @@ -4274,8 +4276,7 @@ static int __init floppy_init(void) FDCS->rawcmd = 2; if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); FDCS->address = -1; FDCS->version = FDC_NONE; continue; @@ -4284,8 +4285,7 @@ static int __init floppy_init(void) FDCS->version = get_fdc_version(); if (FDCS->version == FDC_NONE) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); FDCS->address = -1; continue; } @@ -4358,6 +4358,38 @@ out_put_disk: static DEFINE_SPINLOCK(floppy_usage_lock); +static int floppy_request_regions(int fdc) +{ + if (!request_region(FDCS->address + 2, 1, "floppy")) { + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + 2); + return -EBUSY; + } + if (!request_region(FDCS->address + 4, 2, "floppy")) { + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + 4); + goto cleanup1; + } + if (!request_region(FDCS->address + 7, 1, "floppy DIR")) { + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + 7); + goto cleanup2; + } + /* address + 6 is reserved, and may be taken by IDE. + * Unfortunately, Adaptec doesn't know this :-(, */ + return 0; + +cleanup2: + release_region(FDCS->address + 4, 2); +cleanup1: + release_region(FDCS->address + 2, 1); + return -EBUSY; +} + +static void floppy_release_regions(int fdc) +{ + release_region(FDCS->address + 2, 1); + release_region(FDCS->address + 4, 2); + release_region(FDCS->address + 7, 1); +} + static int floppy_grab_irq_and_dma(void) { unsigned long flags; @@ -4399,18 +4431,8 @@ static int floppy_grab_irq_and_dma(void) for (fdc = 0; fdc < N_FDC; fdc++) { if (FDCS->address != -1) { - if (!request_region(FDCS->address + 2, 4, "floppy")) { - DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + 2); - goto cleanup1; - } - if (!request_region(FDCS->address + 7, 1, "floppy DIR")) { - DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + 7); - goto cleanup2; - } - /* address + 6 is reserved, and may be taken by IDE. - * Unfortunately, Adaptec doesn't know this :-(, */ + if (floppy_request_regions(fdc)) + goto cleanup; } } for (fdc = 0; fdc < N_FDC; fdc++) { @@ -4432,15 +4454,11 @@ static int floppy_grab_irq_and_dma(void) fdc = 0; irqdma_allocated = 1; return 0; -cleanup2: - release_region(FDCS->address + 2, 4); -cleanup1: +cleanup: fd_free_irq(); fd_free_dma(); - while (--fdc >= 0) { - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); - } + while (--fdc >= 0) + floppy_release_regions(fdc); spin_lock_irqsave(&floppy_usage_lock, flags); usage_count--; spin_unlock_irqrestore(&floppy_usage_lock, flags); @@ -4502,8 +4520,7 @@ static void floppy_release_irq_and_dma(void) old_fdc = fdc; for (fdc = 0; fdc < N_FDC; fdc++) if (FDCS->address != -1) { - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); } fdc = old_fdc; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: release only the ports we actually requested 2009-02-06 17:57 ` Bjorn Helgaas @ 2009-02-09 9:15 ` Philippe De Muyter 2009-02-10 17:16 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Philippe De Muyter @ 2009-02-09 9:15 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, linux-kernel, hancockrwd, abelay, lenb, stable Bjorn, On Fri, Feb 06, 2009 at 10:57:06AM -0700, Bjorn Helgaas wrote: > On Friday 06 February 2009 09:20:13 am Bjorn Helgaas wrote: > > On Friday 06 February 2009 01:55:01 am Philippe De Muyter wrote: > > > Bjorn, Andrew, > > > > > > -- > > > > > > With the last floppy patch, the floppy driver requests only the ports that > > > it really uses, but the code contains yet places where it releases those > > > unrequested ports. I don't know if it is harmfull, but I think it is cleaner > > > that the parameters of the release_region calls match the request_region ones. > > > > Doh! Boy, do I feel stupid. I doubt this is in any git trees yet, > > so I'll combine the two patches and send a cleaner one. > > Philippe, can you give this a once-over? I can compile it, but I don't > have a machine with a floppy drive handy to test it. Since you pointed > out all the places I forgot to change, I went ahead and factored out > the request/release stuff so they're all in one place now. As expected, your code works (tested linked in kernel and as a module), but I took your idea a little bit further, avoiding to keep 3 places with the io regions list, avoiding gotos and producing a slightly smaller binary. Of course, it is also compiled and tested both linked in kernel and in module mode. Is it OK for you ? Philippe diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -558,6 +558,8 @@ static void recalibrate_floppy(void); static void recalibrate_floppy(void); static void floppy_shutdown(unsigned long); +static int floppy_request_regions(int); +static void floppy_release_regions(int); static int floppy_grab_irq_and_dma(void); static void floppy_release_irq_and_dma(void); @@ -4274,8 +4276,7 @@ static int __init floppy_init(void) FDCS->rawcmd = 2; if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); FDCS->address = -1; FDCS->version = FDC_NONE; continue; @@ -4284,8 +4285,7 @@ static int __init floppy_init(void) FDCS->version = get_fdc_version(); if (FDCS->version == FDC_NONE) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); FDCS->address = -1; continue; } @@ -4358,6 +4358,47 @@ out_put_disk: static DEFINE_SPINLOCK(floppy_usage_lock); +static struct io_region { + int offset; + int size; +} io_regions[] = { + { 2, 1 }, + /* address + 3 is sometimes reserved by pnp bios for motherboard */ + { 4, 2 }, + /* address + 6 is reserved, and may be taken by IDE. + * Unfortunately, Adaptec doesn't know this :-(, */ + { 7, 1 }, +}; + +static void floppy_release_allocated_regions(int fdc, struct io_region *p) +{ + while (p != io_regions) { + p--; + release_region(FDCS->address + p->offset, p->size); + } +} + +#define ARRAY_END(X) (&((X)[ARRAY_SIZE(X)])) + +static int floppy_request_regions(int fdc) +{ + struct io_region *p; + + for (p = io_regions; p < ARRAY_END(io_regions); p++) { + if (!request_region(FDCS->address + p->offset, p->size, "floppy")) { + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + p->offset); + floppy_release_allocated_regions(fdc, p); + return -EBUSY; + } + } + return 0; +} + +static void floppy_release_regions(int fdc) +{ + floppy_release_allocated_regions(fdc, ARRAY_END(io_regions)); +} + static int floppy_grab_irq_and_dma(void) { unsigned long flags; @@ -4399,18 +4440,8 @@ static int floppy_grab_irq_and_dma(void) for (fdc = 0; fdc < N_FDC; fdc++) { if (FDCS->address != -1) { - if (!request_region(FDCS->address + 2, 4, "floppy")) { - DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + 2); - goto cleanup1; - } - if (!request_region(FDCS->address + 7, 1, "floppy DIR")) { - DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + 7); - goto cleanup2; - } - /* address + 6 is reserved, and may be taken by IDE. - * Unfortunately, Adaptec doesn't know this :-(, */ + if (floppy_request_regions(fdc)) + goto cleanup; } } for (fdc = 0; fdc < N_FDC; fdc++) { @@ -4432,15 +4463,11 @@ static int floppy_grab_irq_and_dma(void) fdc = 0; irqdma_allocated = 1; return 0; -cleanup2: - release_region(FDCS->address + 2, 4); -cleanup1: +cleanup: fd_free_irq(); fd_free_dma(); - while (--fdc >= 0) { - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); - } + while (--fdc >= 0) + floppy_release_regions(fdc); spin_lock_irqsave(&floppy_usage_lock, flags); usage_count--; spin_unlock_irqrestore(&floppy_usage_lock, flags); @@ -4502,8 +4529,7 @@ static void floppy_release_irq_and_dma(v old_fdc = fdc; for (fdc = 0; fdc < N_FDC; fdc++) if (FDCS->address != -1) { - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); } fdc = old_fdc; } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: release only the ports we actually requested 2009-02-09 9:15 ` Philippe De Muyter @ 2009-02-10 17:16 ` Bjorn Helgaas 0 siblings, 0 replies; 11+ messages in thread From: Bjorn Helgaas @ 2009-02-10 17:16 UTC (permalink / raw) To: Philippe De Muyter Cc: Andrew Morton, linux-kernel, hancockrwd, abelay, lenb, stable On Monday 09 February 2009 02:15:27 am Philippe De Muyter wrote: > Bjorn, > > On Fri, Feb 06, 2009 at 10:57:06AM -0700, Bjorn Helgaas wrote: > > On Friday 06 February 2009 09:20:13 am Bjorn Helgaas wrote: > > > On Friday 06 February 2009 01:55:01 am Philippe De Muyter wrote: > > > > Bjorn, Andrew, > > > > > > > > -- > > > > > > > > With the last floppy patch, the floppy driver requests only the ports that > > > > it really uses, but the code contains yet places where it releases those > > > > unrequested ports. I don't know if it is harmfull, but I think it is cleaner > > > > that the parameters of the release_region calls match the request_region ones. > > > > > > Doh! Boy, do I feel stupid. I doubt this is in any git trees yet, > > > so I'll combine the two patches and send a cleaner one. > > > > Philippe, can you give this a once-over? I can compile it, but I don't > > have a machine with a floppy drive handy to test it. Since you pointed > > out all the places I forgot to change, I went ahead and factored out > > the request/release stuff so they're all in one place now. > > As expected, your code works (tested linked in kernel and as a module), > but I took your idea a little bit further, avoiding to keep 3 places with > the io regions list, avoiding gotos and producing a slightly smaller binary. > Of course, it is also compiled and tested both linked in kernel and in module > mode. > > Is it OK for you ? Looks great to me. Feel free to add a suitable changelog and my Reviewed-by. Thanks for all your work on this! Bjorn > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > --- a/drivers/block/floppy.c > +++ b/drivers/block/floppy.c > @@ -558,6 +558,8 @@ static void recalibrate_floppy(void); > static void recalibrate_floppy(void); > static void floppy_shutdown(unsigned long); > > +static int floppy_request_regions(int); > +static void floppy_release_regions(int); > static int floppy_grab_irq_and_dma(void); > static void floppy_release_irq_and_dma(void); > > @@ -4274,8 +4276,7 @@ static int __init floppy_init(void) > FDCS->rawcmd = 2; > if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > + floppy_release_regions(fdc); > FDCS->address = -1; > FDCS->version = FDC_NONE; > continue; > @@ -4284,8 +4285,7 @@ static int __init floppy_init(void) > FDCS->version = get_fdc_version(); > if (FDCS->version == FDC_NONE) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > + floppy_release_regions(fdc); > FDCS->address = -1; > continue; > } > @@ -4358,6 +4358,47 @@ out_put_disk: > > static DEFINE_SPINLOCK(floppy_usage_lock); > > +static struct io_region { > + int offset; > + int size; > +} io_regions[] = { > + { 2, 1 }, > + /* address + 3 is sometimes reserved by pnp bios for motherboard */ > + { 4, 2 }, > + /* address + 6 is reserved, and may be taken by IDE. > + * Unfortunately, Adaptec doesn't know this :-(, */ > + { 7, 1 }, > +}; > + > +static void floppy_release_allocated_regions(int fdc, struct io_region *p) > +{ > + while (p != io_regions) { > + p--; > + release_region(FDCS->address + p->offset, p->size); > + } > +} > + > +#define ARRAY_END(X) (&((X)[ARRAY_SIZE(X)])) > + > +static int floppy_request_regions(int fdc) > +{ > + struct io_region *p; > + > + for (p = io_regions; p < ARRAY_END(io_regions); p++) { > + if (!request_region(FDCS->address + p->offset, p->size, "floppy")) { > + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + p->offset); > + floppy_release_allocated_regions(fdc, p); > + return -EBUSY; > + } > + } > + return 0; > +} > + > +static void floppy_release_regions(int fdc) > +{ > + floppy_release_allocated_regions(fdc, ARRAY_END(io_regions)); > +} > + > static int floppy_grab_irq_and_dma(void) > { > unsigned long flags; > @@ -4399,18 +4440,8 @@ static int floppy_grab_irq_and_dma(void) > > for (fdc = 0; fdc < N_FDC; fdc++) { > if (FDCS->address != -1) { > - if (!request_region(FDCS->address + 2, 4, "floppy")) { > - DPRINT("Floppy io-port 0x%04lx in use\n", > - FDCS->address + 2); > - goto cleanup1; > - } > - if (!request_region(FDCS->address + 7, 1, "floppy DIR")) { > - DPRINT("Floppy io-port 0x%04lx in use\n", > - FDCS->address + 7); > - goto cleanup2; > - } > - /* address + 6 is reserved, and may be taken by IDE. > - * Unfortunately, Adaptec doesn't know this :-(, */ > + if (floppy_request_regions(fdc)) > + goto cleanup; > } > } > for (fdc = 0; fdc < N_FDC; fdc++) { > @@ -4432,15 +4463,11 @@ static int floppy_grab_irq_and_dma(void) > fdc = 0; > irqdma_allocated = 1; > return 0; > -cleanup2: > - release_region(FDCS->address + 2, 4); > -cleanup1: > +cleanup: > fd_free_irq(); > fd_free_dma(); > - while (--fdc >= 0) { > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > - } > + while (--fdc >= 0) > + floppy_release_regions(fdc); > spin_lock_irqsave(&floppy_usage_lock, flags); > usage_count--; > spin_unlock_irqrestore(&floppy_usage_lock, flags); > @@ -4502,8 +4529,7 @@ static void floppy_release_irq_and_dma(v > old_fdc = fdc; > for (fdc = 0; fdc < N_FDC; fdc++) > if (FDCS->address != -1) { > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > + floppy_release_regions(fdc); > } > fdc = old_fdc; > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] floppy: request and release only the ports we actually use 2009-02-05 17:48 [PATCH] floppy: request only the ports we actually use Bjorn Helgaas 2009-02-05 17:55 ` Bjorn Helgaas @ 2009-02-13 8:55 ` Philippe De Muyter 2009-02-13 16:37 ` Bjorn Helgaas 1 sibling, 1 reply; 11+ messages in thread From: Philippe De Muyter @ 2009-02-13 8:55 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, linux-kernel, Robert Hancock, Adam M Belay, Len Brown Andrew, this is a combined patch replacing the following ones currently in the -mm tree : floppy-request-only-the-ports-we-actually-use.patch floppy-request-only-the-ports-we-actually-use-fix.patch Bjorn, Since you did the hard job of collecting the used io-ports and writing the changelog and part of this patch, I kept your Signed-off-by : Philippe -- The floppy driver requests an I/O port it doesn't need, and sometimes this causes a conflict with a motherboard device reported by PNPBIOS. This patch makes the floppy driver request and release only the ports it actually uses. It also factors out the request/release stuff and the io-ports list so they're all in one place now. The current floppy driver uses only these ports: 0x3f2 (FD_DOR) 0x3f4 (FD_STATUS) 0x3f5 (FD_DATA) 0x3f7 (FD_DCR/FD_DIR) but it requests 0x3f2-0x3f5 and 0x3f7, which includes the unused port 0x3f3. Some BIOSes report 0x3f3 as a motherboard resource. The PNP system driver reserves that, which causes a conflict when the floppy driver requests 0x3f2-0x3f5 later. Philippe reported that this conflict broke the floppy driver between 2.6.11 and 2.6.22. His PNPBIOS reports these devices: $ cat 00:07/id 00:07/resources # motherboard device PNP0c02 state = active io 0x80-0x80 io 0x10-0x1f io 0x22-0x3f io 0x44-0x5f io 0x90-0x9f io 0xa2-0xbf io 0x3f0-0x3f1 io 0x3f3-0x3f3 $ cat 00:03/id 00:03/resources # floppy device PNP0700 state = active io 0x3f4-0x3f5 io 0x3f2-0x3f2 Reference: http://lkml.org/lkml/2009/1/31/162 Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com> Signed-off-by: Philippe De Muyter <phdm@macqel.be> Reported-by: Philippe De Muyter <phdm@macqel.be> Tested-by: Philippe De Muyter <phdm@macqel.be> CC: Adam M Belay <abelay@mit.edu> CC: Robert Hancock <hancockrwd@gmail.com> --- diff -r 94166a3a38bd drivers/block/floppy.c --- a/drivers/block/floppy.c Sat Jan 31 15:56:23 2009 -0800 +++ b/drivers/block/floppy.c Fri Feb 13 09:24:51 2009 +0100 @@ -558,6 +558,8 @@ static void recalibrate_floppy(void); static void recalibrate_floppy(void); static void floppy_shutdown(unsigned long); +static int floppy_request_regions(int); +static void floppy_release_regions(int); static int floppy_grab_irq_and_dma(void); static void floppy_release_irq_and_dma(void); @@ -4274,8 +4276,7 @@ static int __init floppy_init(void) FDCS->rawcmd = 2; if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); FDCS->address = -1; FDCS->version = FDC_NONE; continue; @@ -4284,8 +4285,7 @@ static int __init floppy_init(void) FDCS->version = get_fdc_version(); if (FDCS->version == FDC_NONE) { /* free ioports reserved by floppy_grab_irq_and_dma() */ - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); + floppy_release_regions(fdc); FDCS->address = -1; continue; } @@ -4358,6 +4358,47 @@ out_put_disk: static DEFINE_SPINLOCK(floppy_usage_lock); +static const struct io_region { + int offset; + int size; +} io_regions[] = { + { 2, 1 }, + /* address + 3 is sometimes reserved by pnp bios for motherboard */ + { 4, 2 }, + /* address + 6 is reserved, and may be taken by IDE. + * Unfortunately, Adaptec doesn't know this :-(, */ + { 7, 1 }, +}; + +static void floppy_release_allocated_regions(int fdc, const struct io_region *p) +{ + while (p != io_regions) { + p--; + release_region(FDCS->address + p->offset, p->size); + } +} + +#define ARRAY_END(X) (&((X)[ARRAY_SIZE(X)])) + +static int floppy_request_regions(int fdc) +{ + const struct io_region *p; + + for (p = io_regions; p < ARRAY_END(io_regions); p++) { + if (!request_region(FDCS->address + p->offset, p->size, "floppy")) { + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + p->offset); + floppy_release_allocated_regions(fdc, p); + return -EBUSY; + } + } + return 0; +} + +static void floppy_release_regions(int fdc) +{ + floppy_release_allocated_regions(fdc, ARRAY_END(io_regions)); +} + static int floppy_grab_irq_and_dma(void) { unsigned long flags; @@ -4399,18 +4440,8 @@ static int floppy_grab_irq_and_dma(void) for (fdc = 0; fdc < N_FDC; fdc++) { if (FDCS->address != -1) { - if (!request_region(FDCS->address + 2, 4, "floppy")) { - DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + 2); - goto cleanup1; - } - if (!request_region(FDCS->address + 7, 1, "floppy DIR")) { - DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + 7); - goto cleanup2; - } - /* address + 6 is reserved, and may be taken by IDE. - * Unfortunately, Adaptec doesn't know this :-(, */ + if (floppy_request_regions(fdc)) + goto cleanup; } } for (fdc = 0; fdc < N_FDC; fdc++) { @@ -4432,15 +4463,11 @@ static int floppy_grab_irq_and_dma(void) fdc = 0; irqdma_allocated = 1; return 0; -cleanup2: - release_region(FDCS->address + 2, 4); -cleanup1: +cleanup: fd_free_irq(); fd_free_dma(); - while (--fdc >= 0) { - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); - } + while (--fdc >= 0) + floppy_release_regions(fdc); spin_lock_irqsave(&floppy_usage_lock, flags); usage_count--; spin_unlock_irqrestore(&floppy_usage_lock, flags); @@ -4501,10 +4528,8 @@ static void floppy_release_irq_and_dma(v #endif old_fdc = fdc; for (fdc = 0; fdc < N_FDC; fdc++) - if (FDCS->address != -1) { - release_region(FDCS->address + 2, 4); - release_region(FDCS->address + 7, 1); - } + if (FDCS->address != -1) + floppy_release_regions(fdc); fdc = old_fdc; } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] floppy: request and release only the ports we actually use 2009-02-13 8:55 ` [PATCH] floppy: request and release only the ports we actually use Philippe De Muyter @ 2009-02-13 16:37 ` Bjorn Helgaas 0 siblings, 0 replies; 11+ messages in thread From: Bjorn Helgaas @ 2009-02-13 16:37 UTC (permalink / raw) To: Philippe De Muyter Cc: Andrew Morton, linux-kernel, Robert Hancock, Adam M Belay, Len Brown On Friday 13 February 2009 01:55:37 am Philippe De Muyter wrote: > Andrew, > > this is a combined patch replacing the following ones currently in > the -mm tree : > floppy-request-only-the-ports-we-actually-use.patch > floppy-request-only-the-ports-we-actually-use-fix.patch > > Bjorn, > > Since you did the hard job of collecting the used io-ports and writing > the changelog and part of this patch, I kept your Signed-off-by : Great! Thanks for putting this together. This looks like the right thing to me. Bjorn > -- > > The floppy driver requests an I/O port it doesn't need, and > sometimes this causes a conflict with a motherboard device > reported by PNPBIOS. > > This patch makes the floppy driver request and release > only the ports it actually uses. It also factors out > the request/release stuff and the io-ports list so > they're all in one place now. > > The current floppy driver uses only these ports: > 0x3f2 (FD_DOR) > 0x3f4 (FD_STATUS) > 0x3f5 (FD_DATA) > 0x3f7 (FD_DCR/FD_DIR) > but it requests 0x3f2-0x3f5 and 0x3f7, which includes the > unused port 0x3f3. > > Some BIOSes report 0x3f3 as a motherboard resource. The PNP system > driver reserves that, which causes a conflict when the floppy driver > requests 0x3f2-0x3f5 later. > > Philippe reported that this conflict broke the floppy driver between > 2.6.11 and 2.6.22. His PNPBIOS reports these devices: > > $ cat 00:07/id 00:07/resources # motherboard device > PNP0c02 > state = active > io 0x80-0x80 > io 0x10-0x1f > io 0x22-0x3f > io 0x44-0x5f > io 0x90-0x9f > io 0xa2-0xbf > io 0x3f0-0x3f1 > io 0x3f3-0x3f3 > > $ cat 00:03/id 00:03/resources # floppy device > PNP0700 > state = active > io 0x3f4-0x3f5 > io 0x3f2-0x3f2 > > Reference: > http://lkml.org/lkml/2009/1/31/162 > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com> > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > Reported-by: Philippe De Muyter <phdm@macqel.be> > Tested-by: Philippe De Muyter <phdm@macqel.be> > CC: Adam M Belay <abelay@mit.edu> > CC: Robert Hancock <hancockrwd@gmail.com> > --- > diff -r 94166a3a38bd drivers/block/floppy.c > --- a/drivers/block/floppy.c Sat Jan 31 15:56:23 2009 -0800 > +++ b/drivers/block/floppy.c Fri Feb 13 09:24:51 2009 +0100 > @@ -558,6 +558,8 @@ static void recalibrate_floppy(void); > static void recalibrate_floppy(void); > static void floppy_shutdown(unsigned long); > > +static int floppy_request_regions(int); > +static void floppy_release_regions(int); > static int floppy_grab_irq_and_dma(void); > static void floppy_release_irq_and_dma(void); > > @@ -4274,8 +4276,7 @@ static int __init floppy_init(void) > FDCS->rawcmd = 2; > if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > + floppy_release_regions(fdc); > FDCS->address = -1; > FDCS->version = FDC_NONE; > continue; > @@ -4284,8 +4285,7 @@ static int __init floppy_init(void) > FDCS->version = get_fdc_version(); > if (FDCS->version == FDC_NONE) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > + floppy_release_regions(fdc); > FDCS->address = -1; > continue; > } > @@ -4358,6 +4358,47 @@ out_put_disk: > > static DEFINE_SPINLOCK(floppy_usage_lock); > > +static const struct io_region { > + int offset; > + int size; > +} io_regions[] = { > + { 2, 1 }, > + /* address + 3 is sometimes reserved by pnp bios for motherboard */ > + { 4, 2 }, > + /* address + 6 is reserved, and may be taken by IDE. > + * Unfortunately, Adaptec doesn't know this :-(, */ > + { 7, 1 }, > +}; > + > +static void floppy_release_allocated_regions(int fdc, const struct io_region *p) > +{ > + while (p != io_regions) { > + p--; > + release_region(FDCS->address + p->offset, p->size); > + } > +} > + > +#define ARRAY_END(X) (&((X)[ARRAY_SIZE(X)])) > + > +static int floppy_request_regions(int fdc) > +{ > + const struct io_region *p; > + > + for (p = io_regions; p < ARRAY_END(io_regions); p++) { > + if (!request_region(FDCS->address + p->offset, p->size, "floppy")) { > + DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + p->offset); > + floppy_release_allocated_regions(fdc, p); > + return -EBUSY; > + } > + } > + return 0; > +} > + > +static void floppy_release_regions(int fdc) > +{ > + floppy_release_allocated_regions(fdc, ARRAY_END(io_regions)); > +} > + > static int floppy_grab_irq_and_dma(void) > { > unsigned long flags; > @@ -4399,18 +4440,8 @@ static int floppy_grab_irq_and_dma(void) > > for (fdc = 0; fdc < N_FDC; fdc++) { > if (FDCS->address != -1) { > - if (!request_region(FDCS->address + 2, 4, "floppy")) { > - DPRINT("Floppy io-port 0x%04lx in use\n", > - FDCS->address + 2); > - goto cleanup1; > - } > - if (!request_region(FDCS->address + 7, 1, "floppy DIR")) { > - DPRINT("Floppy io-port 0x%04lx in use\n", > - FDCS->address + 7); > - goto cleanup2; > - } > - /* address + 6 is reserved, and may be taken by IDE. > - * Unfortunately, Adaptec doesn't know this :-(, */ > + if (floppy_request_regions(fdc)) > + goto cleanup; > } > } > for (fdc = 0; fdc < N_FDC; fdc++) { > @@ -4432,15 +4463,11 @@ static int floppy_grab_irq_and_dma(void) > fdc = 0; > irqdma_allocated = 1; > return 0; > -cleanup2: > - release_region(FDCS->address + 2, 4); > -cleanup1: > +cleanup: > fd_free_irq(); > fd_free_dma(); > - while (--fdc >= 0) { > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > - } > + while (--fdc >= 0) > + floppy_release_regions(fdc); > spin_lock_irqsave(&floppy_usage_lock, flags); > usage_count--; > spin_unlock_irqrestore(&floppy_usage_lock, flags); > @@ -4501,10 +4528,8 @@ static void floppy_release_irq_and_dma(v > #endif > old_fdc = fdc; > for (fdc = 0; fdc < N_FDC; fdc++) > - if (FDCS->address != -1) { > - release_region(FDCS->address + 2, 4); > - release_region(FDCS->address + 7, 1); > - } > + if (FDCS->address != -1) > + floppy_release_regions(fdc); > fdc = old_fdc; > } > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-02-13 16:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-05 17:48 [PATCH] floppy: request only the ports we actually use Bjorn Helgaas 2009-02-05 17:55 ` Bjorn Helgaas 2009-02-05 22:29 ` Andrew Morton 2009-02-05 23:43 ` Bjorn Helgaas 2009-02-06 8:55 ` [PATCH] floppy: release only the ports we actually requested Philippe De Muyter 2009-02-06 16:20 ` Bjorn Helgaas 2009-02-06 17:57 ` Bjorn Helgaas 2009-02-09 9:15 ` Philippe De Muyter 2009-02-10 17:16 ` Bjorn Helgaas 2009-02-13 8:55 ` [PATCH] floppy: request and release only the ports we actually use Philippe De Muyter 2009-02-13 16:37 ` Bjorn Helgaas
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).