On Monday 22 June 2009 08:43:13 Frans Pop wrote:
> On Monday 22 June 2009, David Miller wrote:
> > Some things other than the commit in question have changed in the area
> > of interrupt and chip initialization and I'll try to go through
> > the commits to get some clues.
>
> Great.
>
> JFTR, here's the full output of your debug print. All 32 prints are
> identical and the "nobody cared" is printed immediately after.
> No really new info I guess...
Frans, here is a draft fix (sorry for not being able to finish it yesterday
so we could potentially close the issue within 24h of the initial report).
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: fix handling of unexpected IRQs vs request_irq()
Add ide_host_enable_irqs() helper and use it in ide_host_register()
before registering ports. Then remove no longer needed IRQ unmasking
from in init_irq().
This should fix the problem with "screaming" shared IRQ on the first
port (after request_irq() call while we have the unexpected IRQ pending
on the second port) which was uncovered by my rework of the serialized
interfaces support.
Cc: David Miller <[email protected]>
Reported-by: Frans Pop <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
David, if it doesn't work I think that the next thing to look into is
the change of nIEN setting itself (some devices are just buggy and may
generate spurious IRQs on such operation).
Alternatively you can fix the issue quickly be extending the coverage of
the recently added IDE_PFLAG_PROBING port flag and testing for the flag
inside ide_intr().
drivers/ide/ide-probe.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
Index: b/drivers/ide/ide-probe.c
===================================================================
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -818,6 +818,24 @@ static int ide_port_setup_devices(ide_hw
return j;
}
+static void ide_host_enable_irqs(struct ide_host *host)
+{
+ ide_hwif_t *hwif;
+ int i;
+
+ ide_host_for_each_port(i, hwif, host) {
+ if (hwif == NULL)
+ continue;
+
+ /* clear any pending IRQs */
+ hwif->tp_ops->read_status(hwif);
+
+ /* unmask IRQs */
+ if (hwif->io_ports.ctl_addr)
+ hwif->tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
+ }
+}
+
/*
* This routine sets up the IRQ for an IDE interface.
*/
@@ -831,9 +849,6 @@ static int init_irq (ide_hwif_t *hwif)
if (irq_handler == NULL)
irq_handler = ide_intr;
- if (io_ports->ctl_addr)
- hwif->tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
-
if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
goto out_up;
@@ -1404,6 +1419,8 @@ int ide_host_register(struct ide_host *h
ide_port_tune_devices(hwif);
}
+ ide_host_enable_irqs(host);
+
ide_host_for_each_port(i, hwif, host) {
if (hwif == NULL)
continue;
On Monday 22 June 2009, you wrote:
> From: Bartlomiej Zolnierkiewicz <[email protected]>
> Subject: [PATCH] ide: fix handling of unexpected IRQs vs request_irq()
>
> Add ide_host_enable_irqs() helper and use it in ide_host_register()
> before registering ports. Then remove no longer needed IRQ unmasking
> from in init_irq().
>
> This should fix the problem with "screaming" shared IRQ on the first
> port (after request_irq() call while we have the unexpected IRQ pending
> on the second port) which was uncovered by my rework of the serialized
> interfaces support.
Thanks Bart. This does solve the "nobody cared" problem.
Tested-by: Frans Pop <[email protected]>
I also tested it without David's initial patch (i.e. *with*
IDE_HFLAG_SERIALIZE in host-flags) and that seems to work fine too:
ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14 (serialized)
ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (serialized)
No idea whether serialized is to be preferred or not. Guess that's David's
call now.
I do still get the "bad DMA info in identify block" error for the CD
drive, so that's still a regression relative to 2.6.26:
hdd: host max PIO5 wanted PIO255(auto-tune) selected PIO4
-hdd: MWDMA2 mode selected
+hdd: bad DMA info in identify block
+hdd: host max PIO5 wanted PIO255(auto-tune) selected PIO4
Cheers,
FJP
On Monday 22 June 2009 16:04:15 Frans Pop wrote:
> On Monday 22 June 2009, you wrote:
> > From: Bartlomiej Zolnierkiewicz <[email protected]>
> > Subject: [PATCH] ide: fix handling of unexpected IRQs vs request_irq()
> >
> > Add ide_host_enable_irqs() helper and use it in ide_host_register()
> > before registering ports. Then remove no longer needed IRQ unmasking
> > from in init_irq().
> >
> > This should fix the problem with "screaming" shared IRQ on the first
> > port (after request_irq() call while we have the unexpected IRQ pending
> > on the second port) which was uncovered by my rework of the serialized
> > interfaces support.
>
> Thanks Bart. This does solve the "nobody cared" problem.
> Tested-by: Frans Pop <[email protected]>
>
> I also tested it without David's initial patch (i.e. *with*
> IDE_HFLAG_SERIALIZE in host-flags) and that seems to work fine too:
> ide0 at 0x1fe02c00000-0x1fe02c00007,0x1fe02c0000a on irq 14 (serialized)
> ide1 at 0x1fe02c00010-0x1fe02c00017,0x1fe02c0001a on irq 14 (serialized)
Great, thanks for testing it (and once again sorry for the trouble).
> No idea whether serialized is to be preferred or not. Guess that's David's
> call now.
Since you have verified that serialization is not needed we should get rid
of it while we are at it (it negatively affects performance of simultaneous
operations on both ports of the controller).
> I do still get the "bad DMA info in identify block" error for the CD
> drive, so that's still a regression relative to 2.6.26:
> hdd: host max PIO5 wanted PIO255(auto-tune) selected PIO4
> -hdd: MWDMA2 mode selected
> +hdd: bad DMA info in identify block
> +hdd: host max PIO5 wanted PIO255(auto-tune) selected PIO4
I begin to wonder whether this problem could be the one responsible for
generating the spurious IRQ that we are seeing on the second port (I think
that this is _very_ likely)..
I promised to look into it but I still need a identify block content to
tell more (you can add #define DEBUG to the ide-probe.c so we will get id
before and after changing of transfer mode settings):
---
drivers/ide/ide-probe.c | 2 ++
1 file changed, 2 insertions(+)
Index: b/drivers/ide/ide-probe.c
===================================================================
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -39,6 +39,8 @@
#include <asm/uaccess.h>
#include <asm/io.h>
+#define DEBUG
+
/**
* generic_id - add a generic drive id
* @drive: drive to make an ID block for
On Monday 22 June 2009, Bartlomiej Zolnierkiewicz wrote:
> I promised to look into it but I still need a identify block content to
> tell more (you can add #define DEBUG to the ide-probe.c so we will get
> id before and after changing of transfer mode settings):
probing for hdd: present=0, media=32, probetype=ATA
probing for hdd: present=0, media=32, probetype=ATAPI
hdd: dumping identify data
c085 0000 0000 0000 0000 0000 0000 0000
0000 0000 2020 2020 2020 2020 2020 2020
2020 2020 2020 2020 0000 0000 0000 3841
2045 2020 2020 4443 522d 4d4f 3520 5836
412f 484b 2020 2020 2020 2020 2020 2020
2020 2020 2020 2020 2020 2020 2020 0000
0000 000f 0000 0003 0002 0200 0000 0000
0000 0000 0000 0000 0000 0000 0701 0701
0300 7800 7800 5802 7800 0000 0000 0000
0000 0000 0000 3030 3030 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0700 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
On Monday 22 June 2009 17:16:04 Frans Pop wrote:
> On Monday 22 June 2009, Bartlomiej Zolnierkiewicz wrote:
> > I promised to look into it but I still need a identify block content to
> > tell more (you can add #define DEBUG to the ide-probe.c so we will get
> > id before and after changing of transfer mode settings):
>
> probing for hdd: present=0, media=32, probetype=ATA
> probing for hdd: present=0, media=32, probetype=ATAPI
> hdd: dumping identify data
> c085 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 2020 2020 2020 2020 2020 2020
> 2020 2020 2020 2020 0000 0000 0000 3841
> 2045 2020 2020 4443 522d 4d4f 3520 5836
> 412f 484b 2020 2020 2020 2020 2020 2020
> 2020 2020 2020 2020 2020 2020 2020 0000
> 0000 000f 0000 0003 0002 0200 0000 0000
> 0000 0000 0000 0000 0000 0000 0701 0701
Thanks. Please notice 0701 0701 words above -- it means that this
device reports both SWDMA0 and MWDMA0 enabled at once (which results
in IDE layer failing DMA tuning).
The patch below should fix it and it would be quite interesting to try
it on vanilla kernel to see if it helps with unexpected IRQ problem.
However this still doesn't explain the regression fully -- we had
ide_id_dma_bug() checks since Dec 2007 (and equivalent ide_dma_verbose()
ones since almost forever) while 2.6.26 (which works fine) is much
younger than that. I suspect that there are some other kernel changes
coming into the picture (Power Management?). Would it be possible
to try 2.6.2[78] and/or bisect this problem further?
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: relax DMA info validity checking
There are some broken devices that report multiple DMA xfer modes
enabled at once (ATA spec doesn't allow it) but otherwise work fine
with DMA so just delete ide_id_dma_bug().
Cc: David Miller <[email protected]>
Reported-by: Frans Pop <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
drivers/ide/ide-dma.c | 21 ---------------------
drivers/ide/ide-iops.c | 3 ---
include/linux/ide.h | 2 --
3 files changed, 26 deletions(-)
Index: b/drivers/ide/ide-dma.c
===================================================================
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -361,9 +361,6 @@ static int ide_tune_dma(ide_drive_t *dri
if (__ide_dma_bad_drive(drive))
return 0;
- if (ide_id_dma_bug(drive))
- return 0;
-
if (hwif->host_flags & IDE_HFLAG_TRUST_BIOS_FOR_DMA)
return config_drive_for_dma(drive);
@@ -394,24 +391,6 @@ static int ide_dma_check(ide_drive_t *dr
return -1;
}
-int ide_id_dma_bug(ide_drive_t *drive)
-{
- u16 *id = drive->id;
-
- if (id[ATA_ID_FIELD_VALID] & 4) {
- if ((id[ATA_ID_UDMA_MODES] >> 8) &&
- (id[ATA_ID_MWDMA_MODES] >> 8))
- goto err_out;
- } else if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
- (id[ATA_ID_SWDMA_MODES] >> 8))
- goto err_out;
-
- return 0;
-err_out:
- printk(KERN_ERR "%s: bad DMA info in identify block\n", drive->name);
- return 1;
-}
-
int ide_set_dma(ide_drive_t *drive)
{
int rc;
Index: b/drivers/ide/ide-iops.c
===================================================================
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -329,9 +329,6 @@ int ide_driveid_update(ide_drive_t *driv
kfree(id);
- if ((drive->dev_flags & IDE_DFLAG_USING_DMA) && ide_id_dma_bug(drive))
- ide_dma_off(drive);
-
return 1;
out_err:
if (rc == 2)
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1361,7 +1361,6 @@ int ide_in_drive_list(u16 *, const struc
#ifdef CONFIG_BLK_DEV_IDEDMA
int ide_dma_good_drive(ide_drive_t *);
int __ide_dma_bad_drive(ide_drive_t *);
-int ide_id_dma_bug(ide_drive_t *);
u8 ide_find_dma_mode(ide_drive_t *, u8);
@@ -1402,7 +1401,6 @@ void ide_dma_lost_irq(ide_drive_t *);
ide_startstop_t ide_dma_timeout_retry(ide_drive_t *, int);
#else
-static inline int ide_id_dma_bug(ide_drive_t *drive) { return 0; }
static inline u8 ide_find_dma_mode(ide_drive_t *drive, u8 speed) { return 0; }
static inline u8 ide_max_dma_mode(ide_drive_t *drive) { return 0; }
static inline void ide_dma_off_quietly(ide_drive_t *drive) { ; }
On Monday 22 June 2009, Bartlomiej Zolnierkiewicz wrote:
> On Monday 22 June 2009 17:16:04 Frans Pop wrote:
> Thanks. Please notice 0701 0701 words above -- it means that this
> device reports both SWDMA0 and MWDMA0 enabled at once (which results
> in IDE layer failing DMA tuning).
>
> The patch below should fix it
Yes, this gives back MWDMA2 for hdd.
> and it would be quite interesting to try
> it on vanilla kernel to see if it helps with unexpected IRQ problem.
Will do later.
> However this still doesn't explain the regression fully -- we had
> ide_id_dma_bug() checks since Dec 2007 (and equivalent
> ide_dma_verbose() ones since almost forever) while 2.6.26 (which works
> fine) is much younger than that. I suspect that there are some other
> kernel changes coming into the picture (Power Management?). Would it
> be possible to try 2.6.2[78] and/or bisect this problem further?
I suspect commit 8d64fcd9 "ide: identify data word 53 bit 1 doesn't cover
words 62 and 63 (take 3)":
@@ -396,15 +393,14 @@ int ide_id_dma_bug(ide_drive_t *drive)
if (id[ATA_ID_FIELD_VALID] & 4) {
if ((id[ATA_ID_UDMA_MODES] >> 8) &&
(id[ATA_ID_MWDMA_MODES] >> 8))
goto err_out;
- } else if (id[ATA_ID_FIELD_VALID] & 2) {
- if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
- (id[ATA_ID_SWDMA_MODES] >> 8))
- goto err_out;
- }
+ } else if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
+ (id[ATA_ID_SWDMA_MODES] >> 8))
+ goto err_out;
The logs I posted were from 2.6.30. I also tried 2.6.29 and that did *not*
yet have the DMA problem. The commit above is from the 2.6.30 development
cycle, so that fits. I expect you can verify it from the identify data.
> From: Bartlomiej Zolnierkiewicz <[email protected]>
> Subject: [PATCH] ide: relax DMA info validity checking
>
> There are some broken devices that report multiple DMA xfer modes
> enabled at once (ATA spec doesn't allow it) but otherwise work fine
> with DMA so just delete ide_id_dma_bug().
The question is maybe: are there other devices that currently have dma
disabled because of the (old) code and would stop working with
ide_id_dma_bug() completely removed? The conservative thing to do I guess
would be to reverse 8d64fcd9.
There is one thing I should mention here. I have been seeing the following
error with this CD drive:
ide-cd: hdd: weird block size 2352
ide-cd: hdd: default to 2kb block size
This was present with 2.6.26 and also now with 2.6.31; not sure about
older kernels. I initially saw it with a self-burned Debian installation
CD. I also now see it with an audio CD. It does not seem to affect
reading the disks: installations go fine and the audio CD plays without
any problems.
Any risk this may be related to something we've been discussing so far, or
is this a separate issue?
On Monday 22 June 2009 21:01:37 Frans Pop wrote:
> On Monday 22 June 2009, Bartlomiej Zolnierkiewicz wrote:
> > On Monday 22 June 2009 17:16:04 Frans Pop wrote:
> > Thanks. Please notice 0701 0701 words above -- it means that this
> > device reports both SWDMA0 and MWDMA0 enabled at once (which results
> > in IDE layer failing DMA tuning).
> >
> > The patch below should fix it
>
> Yes, this gives back MWDMA2 for hdd.
Cool.
> > and it would be quite interesting to try
> > it on vanilla kernel to see if it helps with unexpected IRQ problem.
>
> Will do later.
>
> > However this still doesn't explain the regression fully -- we had
> > ide_id_dma_bug() checks since Dec 2007 (and equivalent
> > ide_dma_verbose() ones since almost forever) while 2.6.26 (which works
> > fine) is much younger than that. I suspect that there are some other
> > kernel changes coming into the picture (Power Management?). Would it
> > be possible to try 2.6.2[78] and/or bisect this problem further?
>
> I suspect commit 8d64fcd9 "ide: identify data word 53 bit 1 doesn't cover
> words 62 and 63 (take 3)":
> @@ -396,15 +393,14 @@ int ide_id_dma_bug(ide_drive_t *drive)
>
> if (id[ATA_ID_FIELD_VALID] & 4) {
> if ((id[ATA_ID_UDMA_MODES] >> 8) &&
> (id[ATA_ID_MWDMA_MODES] >> 8))
> goto err_out;
> - } else if (id[ATA_ID_FIELD_VALID] & 2) {
> - if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
> - (id[ATA_ID_SWDMA_MODES] >> 8))
> - goto err_out;
> - }
> + } else if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
> + (id[ATA_ID_SWDMA_MODES] >> 8))
> + goto err_out;
>
>
> The logs I posted were from 2.6.30. I also tried 2.6.29 and that did *not*
This breaks my beautiful theory about the root cause of unexpected IRQs.. ;(
> yet have the DMA problem. The commit above is from the 2.6.30 development
> cycle, so that fits. I expect you can verify it from the identify data.
I had the same idea initially, unfortunately bit 1 is set for word 53 so this
must be something else...
> > From: Bartlomiej Zolnierkiewicz <[email protected]>
> > Subject: [PATCH] ide: relax DMA info validity checking
> >
> > There are some broken devices that report multiple DMA xfer modes
> > enabled at once (ATA spec doesn't allow it) but otherwise work fine
> > with DMA so just delete ide_id_dma_bug().
>
> The question is maybe: are there other devices that currently have dma
> disabled because of the (old) code and would stop working with
> ide_id_dma_bug() completely removed? The conservative thing to do I guess
> would be to reverse 8d64fcd9.
This is quite unlikely given that libata has never had such checks..
> There is one thing I should mention here. I have been seeing the following
> error with this CD drive:
> ide-cd: hdd: weird block size 2352
> ide-cd: hdd: default to 2kb block size
>
> This was present with 2.6.26 and also now with 2.6.31; not sure about
> older kernels. I initially saw it with a self-burned Debian installation
> CD. I also now see it with an audio CD. It does not seem to affect
> reading the disks: installations go fine and the audio CD plays without
> any problems.
>
> Any risk this may be related to something we've been discussing so far, or
> is this a separate issue?
This is just a harmless warning coming from enabling of the workaround for
weird ATAPI devices (the one you have in this sparc machine seems to score
really high on the weirdness scale ;) introduced by commit e8e7b9e.
On Monday 22 June 2009, Bartlomiej Zolnierkiewicz wrote:
> On Monday 22 June 2009 21:01:37 Frans Pop wrote:
> > There is one thing I should mention here. I have been seeing the
> > following error with this CD drive:
> > ide-cd: hdd: weird block size 2352
> > ide-cd: hdd: default to 2kb block size
>
> This is just a harmless warning coming from enabling of the workaround
> for weird ATAPI devices (the one you have in this sparc machine seems
> to score really high on the weirdness scale ;) introduced by commit
> e8e7b9e.
In that case I'd like to propose the following patch. Currently the error
can get printed much to frequently when there's a disc in the drive.
Example:
Jun 13 18:06:28 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:06:28 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:06:32 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:06:42 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:07:02 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:07:02 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:07:05 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:07:05 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:07:09 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:07:09 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:07:14 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:07:14 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:07:35 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:07:35 gimli kernel: ide-cd: hdd: default to 2kb block size
Jun 13 18:07:51 gimli kernel: ide-cd: hdd: weird block size 2352
Jun 13 18:07:51 gimli kernel: ide-cd: hdd: default to 2kb block size
I was not using the CD at all here. I suspect HAL's stupid polling to be
the culprit as I first saw it after upgrading X.Org packages to a version
which depends on HAL. I since disabled polling for the device, but I
still feel that warning once should be sufficient as IIUC the value is
device dependent and not medium dependent.
With the patch it only gets printed once, when the driver is initialized.
Cheers,
FJP
---
From: Frans Pop <[email protected]>
Subject: ide-cd: Improve "weird block size" error message
Currently the error gets repeated too frequently, for example
each time HAL polls the device when a disc is present. Avoid that
by using printk_once instead of printk.
Also join the error and corrective action messages into a single line.
Signed-off-by: Frans Pop <[email protected]>
Cc: Bartlomiej Zolnierkiewicz <[email protected]>
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 4a19686..7ec6996 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -886,10 +886,9 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
case 4096:
break;
default:
- printk(KERN_ERR PFX "%s: weird block size %u\n",
+ printk_once(KERN_ERR PFX "%s: weird block size %u; "
+ "setting default block size to 2048\n",
drive->name, blocklen);
- printk(KERN_ERR PFX "%s: default to 2kb block size\n",
- drive->name);
blocklen = 2048;
break;
}
On Tue, Jun 23, 2009 at 09:51:23AM +0200, Frans Pop wrote:
[..]
> ---
> From: Frans Pop <[email protected]>
> Subject: ide-cd: Improve "weird block size" error message
>
> Currently the error gets repeated too frequently, for example
> each time HAL polls the device when a disc is present. Avoid that
> by using printk_once instead of printk.
> Also join the error and corrective action messages into a single line.
>
> Signed-off-by: Frans Pop <[email protected]>
> Cc: Bartlomiej Zolnierkiewicz <[email protected]>
>
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 4a19686..7ec6996 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -886,10 +886,9 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
> case 4096:
> break;
> default:
> - printk(KERN_ERR PFX "%s: weird block size %u\n",
> + printk_once(KERN_ERR PFX "%s: weird block size %u; "
> + "setting default block size to 2048\n",
> drive->name, blocklen);
> - printk(KERN_ERR PFX "%s: default to 2kb block size\n",
> - drive->name);
Please leave the weird block size in the printk since it sometimes might
give insights on what is going on.
Thanks.
--
Regards/Gruss,
Boris.
On Tue, Jun 23, 2009 at 09:57:33AM +0200, Borislav Petkov wrote:
> On Tue, Jun 23, 2009 at 09:51:23AM +0200, Frans Pop wrote:
>
> [..]
>
> > ---
> > From: Frans Pop <[email protected]>
> > Subject: ide-cd: Improve "weird block size" error message
> >
> > Currently the error gets repeated too frequently, for example
> > each time HAL polls the device when a disc is present. Avoid that
> > by using printk_once instead of printk.
> > Also join the error and corrective action messages into a single line.
> >
> > Signed-off-by: Frans Pop <[email protected]>
> > Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> >
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index 4a19686..7ec6996 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -886,10 +886,9 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
> > case 4096:
> > break;
> > default:
> > - printk(KERN_ERR PFX "%s: weird block size %u\n",
> > + printk_once(KERN_ERR PFX "%s: weird block size %u; "
> > + "setting default block size to 2048\n",
> > drive->name, blocklen);
> > - printk(KERN_ERR PFX "%s: default to 2kb block size\n",
> > - drive->name);
>
Ah, nevermind! Hadn't had a coffee yet, sorry :).
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
On Tuesday 23 June 2009, Borislav Petkov wrote:
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index 4a19686..7ec6996 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -886,10 +886,9 @@ static int cdrom_read_capacity(ide_drive_t
> > *drive, unsigned long *capacity, case 4096:
> > break;
> > default:
> > - printk(KERN_ERR PFX "%s: weird block size %u\n",
> > + printk_once(KERN_ERR PFX "%s: weird block size %u; "
^^^^^^^
> > + "setting default block size to 2048\n",
> > drive->name, blocklen);
> > - printk(KERN_ERR PFX "%s: default to 2kb block size\n",
> > - drive->name);
>
> Please leave the weird block size in the printk since it sometimes
> might give insights on what is going on.
I did :-)
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Mon, 22 Jun 2009 23:35:06 +0200
> On Monday 22 June 2009 21:01:37 Frans Pop wrote:
>> yet have the DMA problem. The commit above is from the 2.6.30 development
>> cycle, so that fits. I expect you can verify it from the identify data.
>
> I had the same idea initially, unfortunately bit 1 is set for word 53 so this
> must be something else...
So this change should have had zero effect for Frans's case.
I double checked everything with test programs going over his
ID dump and it all checks out.
We might need to bisect this one. But Frans, just for the record
could you simply test reverting just that hunk? Thanks!
>> @@ -396,15 +393,14 @@ int ide_id_dma_bug(ide_drive_t *drive)
>>
>> if (id[ATA_ID_FIELD_VALID] & 4) {
>> if ((id[ATA_ID_UDMA_MODES] >> 8) &&
>> (id[ATA_ID_MWDMA_MODES] >> 8))
>> goto err_out;
>> - } else if (id[ATA_ID_FIELD_VALID] & 2) {
>> - if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
>> - (id[ATA_ID_SWDMA_MODES] >> 8))
>> - goto err_out;
>> - }
>> + } else if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
>> + (id[ATA_ID_SWDMA_MODES] >> 8))
>> + goto err_out;
From: Frans Pop <[email protected]>
Date: Mon, 22 Jun 2009 16:04:15 +0200
> On Monday 22 June 2009, you wrote:
>> From: Bartlomiej Zolnierkiewicz <[email protected]>
>> Subject: [PATCH] ide: fix handling of unexpected IRQs vs request_irq()
>>
>> Add ide_host_enable_irqs() helper and use it in ide_host_register()
>> before registering ports. Then remove no longer needed IRQ unmasking
>> from in init_irq().
>>
>> This should fix the problem with "screaming" shared IRQ on the first
>> port (after request_irq() call while we have the unexpected IRQ pending
>> on the second port) which was uncovered by my rework of the serialized
>> interfaces support.
>
> Thanks Bart. This does solve the "nobody cared" problem.
> Tested-by: Frans Pop <[email protected]>
I've applied this patch to my tree, thanks everyone!
From: Frans Pop <[email protected]>
Date: Mon, 22 Jun 2009 21:01:37 +0200
> The question is maybe: are there other devices that currently have dma
> disabled because of the (old) code and would stop working with
> ide_id_dma_bug() completely removed? The conservative thing to do I guess
> would be to reverse 8d64fcd9.
For this specific situation I would likely avoid a revert at least
with how things currently stand.
Right now we are not yet certain what introduced the problem.
We are also not certain what might break by reverting this commit,
either. There are portions of that commit other than the one
changing the logic of ide_id_dma_bug().
That's a "fix" based upon two large unknowns, which is not wise I
think :)
From: Frans Pop <[email protected]>
Date: Tue, 23 Jun 2009 09:51:23 +0200
> In that case I'd like to propose the following patch. Currently the error
> can get printed much to frequently when there's a disc in the drive.
> Example:
>
> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: weird block size 2352
> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: default to 2kb block size
> Jun 13 18:06:32 gimli kernel: ide-cd: hdd: weird block size 2352
> Jun 13 18:06:42 gimli kernel: ide-cd: hdd: default to 2kb block size
Thinking about this a bit. Let's look at what problem this is
trying to avoid, as per the commit message:
--------------------
ide-cd: fix oops when using growisofs
cdrom_read_capacity() will blindly return the capacity from the device
without sanity-checking it. This later causes code in fs/buffer.c to
oops.
Fix this by checking that the device is telling us sensible things.
--------------------
Well, for the values Frans's CDROM is giving, this OOPS would not
take place and the weird sector value is completely harmless.
Since SECTOR_BITS is 9:
(2352 >> 9) == (2048 >> 9) == 4
There is simply no benefit from this warning in this situation.
Therefore, any objections to something like this?
ide-cd: Don't warn on bogus block size unless it actually matters.
Frans Pop reported that his CDROM drive reports a blocksize of 2352,
and this causes new warnings due to commit
e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
growisofs").
What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
is something the block layer won't choke on.
And for Frans case "2352 >> SECTOR_BITS" is equal to
"2048 >> SECTOR_BITS", and thats "4".
So warning in this case gives no real benefit.
Reported-by: Frans Pop <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 4a19686..a9a1bfb 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -876,9 +876,12 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
return stat;
/*
- * Sanity check the given block size
+ * Sanity check the given block size, in so far as making
+ * sure the sectors_per_frame we give to the caller won't
+ * end up being bogus.
*/
blocklen = be32_to_cpu(capbuf.blocklen);
+ blocklen = (blocklen >> SECTOR_BITS) << SECTOR_BITS;
switch (blocklen) {
case 512:
case 1024:
On Tuesday 23 June 2009, David Miller wrote:
> Therefore, any objections to something like this?
>
> ide-cd: Don't warn on bogus block size unless it actually matters.
>
> Frans Pop reported that his CDROM drive reports a blocksize of 2352,
> and this causes new warnings due to commit
> e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
> growisofs").
>
> What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
> is something the block layer won't choke on.
>
> And for Frans case "2352 >> SECTOR_BITS" is equal to
> "2048 >> SECTOR_BITS", and thats "4".
So basically there's garbage in unused bits?
> So warning in this case gives no real benefit.
Fine by me. I'll be glad to be rid of that error :-)
I'll give your patch a try with my next build.
But I think my patch still makes sense for those cases where the original
error _does_ exist. Unless your patch fixes those as well, but we can't
know that for sure, can we?
From: Frans Pop <[email protected]>
Date: Tue, 23 Jun 2009 13:13:53 +0200
> But I think my patch still makes sense for those cases where the original
> error _does_ exist. Unless your patch fixes those as well, but we can't
> know that for sure, can we?
That's a very good point, so yes it makes sense to add your
change as well.
I'm convinced there exists some case where my patch doesn't "fix"
things. Jens only would have submitted that patch if there were some
OOPS that he had diagnosed to precisely this problem.
On Tuesday 23 June 2009, you wrote:
> We might need to bisect this one. But Frans, just for the record
> could you simply test reverting just that hunk? Thanks!
I'm way ahead of you :-)
Instead of a bisect [1] I decided to first see if some printks in both .26
and .31 would show anything useful.
With 2.6.31 and code included below I get:
hda: ST34342A, ATA DISK drive
FJP: id_dma_bug 0x7: &4: 0x0-0x4 no error
hda: MWDMA2 mode selected
hdc: Maxtor 6E040L0, ATA DISK drive
hdd: CD-ROM 56X/AKH, ATAPI CD/DVD-ROM drive
hdc: host max PIO5 wanted PIO255(auto-tune) selected PIO4
FJP: ID_FIELD_VALID: 0x7 (true)
FJP: id_dma_bug 0x7: &4: 0x0-0x4 no error
hdc: MWDMA2 mode selected
hdd: host max PIO5 wanted PIO255(auto-tune) selected PIO4
FJP: ID_FIELD_VALID: 0x2 (true)
FJP: id_dma_bug 0x2: &2: 0x1-0x1 bad modes <-------------
hdd: bad DMA info in identify block
Note that this included a complete revert of 8d64fcd9 (with minor conflict
resolved).
Here's the same output with 2.6.26.3 with equivalent debug statements:
hda: ST34342A, ATA DISK drive
FJP: id_dma_bug 0x7: &4: 0x0-0x0 no error
hda: MWDMA2 mode selected
hdc: Maxtor 6E040L0, ATA DISK drive
hdd: CD-ROM 56X/AKH, ATAPI CD/DVD-ROM drive
FJP: id_dma_bug 0x7: &4: 0x0-0x0 no error
hdc: MWDMA2 mode selected
FJP: id_dma_bug 0x2: &2: 0x0-0x0 no error <-------------
hdd: MWDMA2 mode selected
So it seems to me that in 2.6.26 something was broken in the way these ID
fields were handled, at least in this check. This is now fixed (possibly
by the changes around 5b90e990..48fb2688) and *that* causes the
regression. Note that the hard disks are also affected.
If I'm correct I guess that supports Bart's patch to just remove the whole
thing. But did this only affect the id_dma_bug check or also the use of
these fields elsewhere?
Cheers,
FJP
[1] I don't have a crossbuild environment for sparc, so a bisect would be
painful with 300MHz; I at least have plenty memory luckily.
int ide_id_dma_bug(ide_drive_t *drive)
{
u16 *id = drive->id;
printk("FJP: id_dma_bug 0x%x:", id[ATA_ID_FIELD_VALID]);
if (id[ATA_ID_FIELD_VALID] & 4) {
printk(" &4: 0x%x-0x%x", (id[ATA_ID_UDMA_MODES] >> 8),
(id[ATA_ID_MWDMA_MODES] >> 8));
if ((id[ATA_ID_UDMA_MODES] >> 8) &&
(id[ATA_ID_MWDMA_MODES] >> 8)) {
printk(" bad modes");
goto err_out;
}
} else if (id[ATA_ID_FIELD_VALID] & 2) {
printk(" &2: 0x%x-0x%x", (id[ATA_ID_MWDMA_MODES] >> 8),
(id[ATA_ID_SWDMA_MODES] >> 8));
if ((id[ATA_ID_MWDMA_MODES] >> 8) &&
(id[ATA_ID_SWDMA_MODES] >> 8)) {
printk(" bad modes");
goto err_out;
}
}
printk(" no error\n");
return 0;
err_out:
printk("\n");
printk(KERN_ERR "%s: bad DMA info in identify block\n",
drive->name);
return 1;
}
On Tuesday 23 June 2009 16:58:54 Frans Pop wrote:
> On Tuesday 23 June 2009, you wrote:
> > We might need to bisect this one. But Frans, just for the record
> > could you simply test reverting just that hunk? Thanks!
>
> I'm way ahead of you :-)
>
> Instead of a bisect [1] I decided to first see if some printks in both .26
> and .31 would show anything useful.
>
> With 2.6.31 and code included below I get:
> hda: ST34342A, ATA DISK drive
> FJP: id_dma_bug 0x7: &4: 0x0-0x4 no error
> hda: MWDMA2 mode selected
> hdc: Maxtor 6E040L0, ATA DISK drive
> hdd: CD-ROM 56X/AKH, ATAPI CD/DVD-ROM drive
> hdc: host max PIO5 wanted PIO255(auto-tune) selected PIO4
> FJP: ID_FIELD_VALID: 0x7 (true)
> FJP: id_dma_bug 0x7: &4: 0x0-0x4 no error
> hdc: MWDMA2 mode selected
> hdd: host max PIO5 wanted PIO255(auto-tune) selected PIO4
> FJP: ID_FIELD_VALID: 0x2 (true)
> FJP: id_dma_bug 0x2: &2: 0x1-0x1 bad modes <-------------
> hdd: bad DMA info in identify block
>
> Note that this included a complete revert of 8d64fcd9 (with minor conflict
> resolved).
>
> Here's the same output with 2.6.26.3 with equivalent debug statements:
> hda: ST34342A, ATA DISK drive
> FJP: id_dma_bug 0x7: &4: 0x0-0x0 no error
> hda: MWDMA2 mode selected
> hdc: Maxtor 6E040L0, ATA DISK drive
> hdd: CD-ROM 56X/AKH, ATAPI CD/DVD-ROM drive
> FJP: id_dma_bug 0x7: &4: 0x0-0x0 no error
> hdc: MWDMA2 mode selected
> FJP: id_dma_bug 0x2: &2: 0x0-0x0 no error <-------------
> hdd: MWDMA2 mode selected
>
> So it seems to me that in 2.6.26 something was broken in the way these ID
> fields were handled, at least in this check. This is now fixed (possibly
Great debugging work, thanks!
> by the changes around 5b90e990..48fb2688) and *that* causes the
> regression. Note that the hard disks are also affected.
I see it now -- in commit c419993 ("ide-iops: only clear DMA words
on setting DMA mode") we fixed a bug in ide_config_drive_speed()..
[ It was a real bug resulting in incorrect data being passed to
the user-space through HDIO_GET_IDENTITY ioctl ('hdparm -i'). ]
On Tuesday 23 June 2009, David Miller wrote:
> ide-cd: Don't warn on bogus block size unless it actually matters.
>
> Frans Pop reported that his CDROM drive reports a blocksize of 2352,
> and this causes new warnings due to commit
> e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
> growisofs").
>
> What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
> is something the block layer won't choke on.
>
> And for Frans case "2352 >> SECTOR_BITS" is equal to
> "2048 >> SECTOR_BITS", and thats "4".
Pedantic correction: Frans' case
> So warning in this case gives no real benefit.
>
> Reported-by: Frans Pop <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
As expected, it's gone. Nice.
Tested-by: Frans Pop <[email protected]>
From: Frans Pop <[email protected]>
Date: Tue, 23 Jun 2009 23:30:18 +0200
> On Tuesday 23 June 2009, David Miller wrote:
>> ide-cd: Don't warn on bogus block size unless it actually matters.
>>
>> Frans Pop reported that his CDROM drive reports a blocksize of 2352,
>> and this causes new warnings due to commit
>> e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
>> growisofs").
>>
>> What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
>> is something the block layer won't choke on.
>>
>> And for Frans case "2352 >> SECTOR_BITS" is equal to
>> "2048 >> SECTOR_BITS", and thats "4".
>
> Pedantic correction: Frans' case
Corrected, thanks.
>> So warning in this case gives no real benefit.
>>
>> Reported-by: Frans Pop <[email protected]>
>> Signed-off-by: David S. Miller <[email protected]>
>
> As expected, it's gone. Nice.
>
> Tested-by: Frans Pop <[email protected]>
Applied, thanks for testing!
From: Borislav Petkov <[email protected]>
Date: Tue, 23 Jun 2009 10:02:23 +0200
> On Tue, Jun 23, 2009 at 09:57:33AM +0200, Borislav Petkov wrote:
>> On Tue, Jun 23, 2009 at 09:51:23AM +0200, Frans Pop wrote:
>>
>> [..]
>>
>> > ---
>> > From: Frans Pop <[email protected]>
>> > Subject: ide-cd: Improve "weird block size" error message
>> >
>> > Currently the error gets repeated too frequently, for example
>> > each time HAL polls the device when a disc is present. Avoid that
>> > by using printk_once instead of printk.
>> > Also join the error and corrective action messages into a single line.
>> >
>> > Signed-off-by: Frans Pop <[email protected]>
>> > Cc: Bartlomiej Zolnierkiewicz <[email protected]>
...
> Acked-by: Borislav Petkov <[email protected]>
Applied, thanks!
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 23 Jun 2009 18:13:08 +0200
> On Tuesday 23 June 2009 16:58:54 Frans Pop wrote:
>> So it seems to me that in 2.6.26 something was broken in the way these ID
>> fields were handled, at least in this check. This is now fixed (possibly
>
> Great debugging work, thanks!
Indeed, thanks for all of that work Frans!
>> by the changes around 5b90e990..48fb2688) and *that* causes the
>> regression. Note that the hard disks are also affected.
>
> I see it now -- in commit c419993 ("ide-iops: only clear DMA words
> on setting DMA mode") we fixed a bug in ide_config_drive_speed()..
>
> [ It was a real bug resulting in incorrect data being passed to
> the user-space through HDIO_GET_IDENTITY ioctl ('hdparm -i'). ]
Given all of this I also agree that we should apply Bart's patch to
remove the debugging check altogether, and I will thus do that right
now.
On Tuesday 2009-06-23 12:59, David Miller wrote:
>> In that case I'd like to propose the following patch. Currently the error
>> can get printed much to frequently when there's a disc in the drive.
>> Example:
>>
>> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: weird block size 2352
>> Jun 13 18:06:28 gimli kernel: ide-cd: hdd: default to 2kb block size
>> Jun 13 18:06:32 gimli kernel: ide-cd: hdd: weird block size 2352
>> Jun 13 18:06:42 gimli kernel: ide-cd: hdd: default to 2kb block size
>
>Thinking about this a bit. Let's look at what problem this is
>trying to avoid, as per the commit message:
>
>--------------------
> ide-cd: fix oops when using growisofs
>
> cdrom_read_capacity() will blindly return the capacity from the device
> without sanity-checking it. This later causes code in fs/buffer.c to
> oops.
>
> Fix this by checking that the device is telling us sensible things.
>--------------------
>
>Well, for the values Frans's CDROM is giving, this OOPS would not
>take place and the weird sector value is completely harmless.
>
>Since SECTOR_BITS is 9:
>
>(2352 >> 9) == (2048 >> 9) == 4
>
>There is simply no benefit from this warning in this situation.
But 2352 is the block size for CDDA (yes, I reckon that is not quite
relevant for the block layer), so it's not like there would be
garbage bits in the lower 9 bits.
>
>Therefore, any objections to something like this?
>
>ide-cd: Don't warn on bogus block size unless it actually matters.
>
>Frans Pop reported that his CDROM drive reports a blocksize of 2352,
>and this causes new warnings due to commit
>e8e7b9eb11c34ee18bde8b7011af41938d1ad667 ("ide-cd: fix oops when using
>growisofs").
>
>What we're trying to do is make sure that "blocklen >> SECTOR_BITS"
>is something the block layer won't choke on.
>
>And for Frans case "2352 >> SECTOR_BITS" is equal to
>"2048 >> SECTOR_BITS", and thats "4".