I discovered that hdparm -X <mode> /dev/hda can lock up IDE
interface if there is some activity.
I walked up from via driver's handler for it:
via_set_speed(): directly programs IDE controller timings.
via_set_drive -> via_set_speed
via82cxxx_tune_drive -> via_set_drive
via82cxxx_ide_dma_check -> via_set_drive
init_hwif_via82cxxx:
hwif->tuneproc = &via82cxxx_tune_drive;
hwif->speedproc = &via_set_drive;
...
hwif->ide_dma_check = &via82cxxx_ide_dma_check;
I think via_set_drive is eventually gets called
by hdparm -X, not other two functions.
ide_set_xfer_rate -> HWIF(drive)->speedproc
pre_reset -> check_dma_crc -> ide_set_xfer_rate
set_xfer_rate -> ide_set_xfer_rate
void ide_add_generic_settings(drive):
...
ide_add_setting(drive, "current_speed", SETTING_RW,
-1, -1, TYPE_BYTE, 0, 70, 1, 1, &drive->current_speed,
set_xfer_rate);
^^^^^^^^^^^^^
Seems like there is no locking protecting mode change.
--
vda
On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> interface if there is some activity.
Known bug and is on TODO but fixing it ain't easy.
Thanks for a report anyway.
Regards,
Bartlomiej
On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > interface if there is some activity.
>
> Known bug and is on TODO but fixing it ain't easy.
> Thanks for a report anyway.
Wouldn't it be possible to do the stuff that needs serializing from the
end_request() part and get automatic synchronization with normal
requests?
--
Jens Axboe
On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > > I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > > interface if there is some activity.
> >
> > Known bug and is on TODO but fixing it ain't easy.
> > Thanks for a report anyway.
>
> Wouldn't it be possible to do the stuff that needs serializing from the
> end_request() part and get automatic synchronization with normal
> requests?
That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
we need to synchronize both channels (whereas we don't need to serialize
normal operations).
Regards,
Bartlomiej
On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> > On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> > > On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > > > I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > > > interface if there is some activity.
> > >
> > > Known bug and is on TODO but fixing it ain't easy.
> > > Thanks for a report anyway.
> >
> > Wouldn't it be possible to do the stuff that needs serializing from the
> > end_request() part and get automatic synchronization with normal
> > requests?
>
> That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
> we need to synchronize both channels (whereas we don't need to serialize
> normal operations).
Ugh yes, that gets nasty... Good luck with that :)
--
Jens Axboe
Bartlomiej Zolnierkiewicz wrote:
> On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
>
>>On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
>>
>>>On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
>>>
>>>>I discovered that hdparm -X <mode> /dev/hda can lock up IDE
>>>>interface if there is some activity.
>>>
>>>Known bug and is on TODO but fixing it ain't easy.
>>>Thanks for a report anyway.
>>
>>Wouldn't it be possible to do the stuff that needs serializing from the
>>end_request() part and get automatic synchronization with normal
>>requests?
>
>
> That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
> we need to synchronize both channels (whereas we don't need to serialize
> normal operations).
blk_stop_queue() on all queues attached to the hardware?
You need to synchronize anyway for the rare hardware that reports itself
as "simplex" -- one DMA engine for both channels.
Jeff
On Thursday 11 March 2004 19:59, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> >>On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> >>>On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> >>>>I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> >>>>interface if there is some activity.
> >>>
> >>>Known bug and is on TODO but fixing it ain't easy.
> >>>Thanks for a report anyway.
> >>
> >>Wouldn't it be possible to do the stuff that needs serializing from the
> >>end_request() part and get automatic synchronization with normal
> >>requests?
> >
> > That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
> > we need to synchronize both channels (whereas we don't need to serialize
> > normal operations).
>
> blk_stop_queue() on all queues attached to the hardware?
>
> You need to synchronize anyway for the rare hardware that reports itself
> as "simplex" -- one DMA engine for both channels.
This patch survived cyclic switching of all DMA modes from mwdma0 to udma4
and continuous write load of type
while(1) { dd if=/dev/zero of=file bs=1M count=<RAM size>; sync; }
for five minutes.
It does not handle crippled/simplex chipset, it is a TODO.
Things commented by C++ style comments (//) aren't meant to stay.
I think original code was a bit buggy:
set_transfer() returned 0 for modes < XFER_SW_DMA_0, ie,
for all PIO modes. Later,
if (set_transfer(drive, &tfargs)) {
xfer_rate = args[1];
if (ide_ata66_check(drive, &tfargs))
goto abort;
}
err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3], argbuf);
if (!err && xfer_rate) {
ide_set_xfer_rate(drive, xfer_rate);
ide_driveid_update(drive);
}
This will never execute ide_set_xfer_rate() for PIO modes
--
vda
On Friday 12 of March 2004 01:21, Denis Vlasenko wrote:
> On Thursday 11 March 2004 19:59, Jeff Garzik wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > > On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> > >>On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> > >>>On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > >>>>I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > >>>>interface if there is some activity.
> > >>>
> > >>>Known bug and is on TODO but fixing it ain't easy.
> > >>>Thanks for a report anyway.
> > >>
> > >>Wouldn't it be possible to do the stuff that needs serializing from the
> > >>end_request() part and get automatic synchronization with normal
> > >>requests?
> > >
> > > That's the way to do it (REQ_SPECIAL) but unfortunately on some
> > > chipsets we need to synchronize both channels (whereas we don't need to
> > > serialize normal operations).
> >
> > blk_stop_queue() on all queues attached to the hardware?
> >
> > You need to synchronize anyway for the rare hardware that reports itself
> > as "simplex" -- one DMA engine for both channels.
>
> This patch survived cyclic switching of all DMA modes from mwdma0 to udma4
> and continuous write load of type
> while(1) { dd if=/dev/zero of=file bs=1M count=<RAM size>; sync; }
> for five minutes.
>
> It does not handle crippled/simplex chipset, it is a TODO.
>
> Things commented by C++ style comments (//) aren't meant to stay.
>
> I think original code was a bit buggy:
> set_transfer() returned 0 for modes < XFER_SW_DMA_0, ie,
> for all PIO modes. Later,
> if (set_transfer(drive, &tfargs)) {
> xfer_rate = args[1];
> if (ide_ata66_check(drive, &tfargs))
> goto abort;
> }
> err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3],
> argbuf); if (!err && xfer_rate) {
> ide_set_xfer_rate(drive, xfer_rate);
> ide_driveid_update(drive);
> }
> This will never execute ide_set_xfer_rate() for PIO modes
It will also never execute ide_set_xfer_rate() for DMA on PIO only drive.
I must check if "no PIO" thing is a bug or a design decision.
Thanks,
Bartlomiej
> > This patch survived cyclic switching of all DMA modes from mwdma0 to
> > udma4 and continuous write load of type
> > while(1) { dd if=/dev/zero of=file bs=1M count=<RAM size>; sync; }
> > for five minutes.
> >
> > It does not handle crippled/simplex chipset, it is a TODO.
> >
> > Things commented by C++ style comments (//) aren't meant to stay.
> >
> > I think original code was a bit buggy:
> > set_transfer() returned 0 for modes < XFER_SW_DMA_0, ie,
> > for all PIO modes. Later,
> > if (set_transfer(drive, &tfargs)) {
> > xfer_rate = args[1];
> > if (ide_ata66_check(drive, &tfargs))
> > goto abort;
> > }
> > err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3],
> > argbuf); if (!err && xfer_rate) {
> > ide_set_xfer_rate(drive, xfer_rate);
> > ide_driveid_update(drive);
> > }
> > This will never execute ide_set_xfer_rate() for PIO modes
>
> It will also never execute ide_set_xfer_rate() for DMA on PIO only drive.
> I must check if "no PIO" thing is a bug or a design decision.
I think IDE driver will never ask for DMA on PIO only drive
Bartlomiej, do you have comments on my patch? So far, I myself spotted
some minor problems:
I forgot to remove now-extraneous if() from ide.c:
static int set_xfer_rate (ide_drive_t *drive, int arg)
{
int err = ide_wait_cmd(drive,
WIN_SETFEATURES, (u8) arg,
SETFEATURES_XFER, 0, NULL);
if (!err && arg) {
ide_set_xfer_rate(drive, (u8) arg);
ide_driveid_update(drive);
}
return err;
}
And second, in ide_do_drive_cmd(), mdelay(2000)
after WIN_SETFEATURES is a bit rude.
--
vda
> I forgot to remove now-extraneous if() from ide.c:
>
> static int set_xfer_rate (ide_drive_t *drive, int arg)
> {
> int err = ide_wait_cmd(drive,
> WIN_SETFEATURES, (u8) arg,
> SETFEATURES_XFER, 0, NULL);
>
> if (!err && arg) {
> ide_set_xfer_rate(drive, (u8) arg);
> ide_driveid_update(drive);
> }
> return err;
> }
>
> And second, in ide_do_drive_cmd(), mdelay(2000)
> after WIN_SETFEATURES is a bit rude.
...and wrongly placed. I just realized that ide_driveid_update(drive)
actually talks with the drive!
New patch is attached.
--
vda
On Friday 12 of March 2004 10:39, Denis Vlasenko wrote:
> > I forgot to remove now-extraneous if() from ide.c:
> >
> > static int set_xfer_rate (ide_drive_t *drive, int arg)
> > {
> > int err = ide_wait_cmd(drive,
> > WIN_SETFEATURES, (u8) arg,
> > SETFEATURES_XFER, 0, NULL);
> >
> > if (!err && arg) {
> > ide_set_xfer_rate(drive, (u8) arg);
> > ide_driveid_update(drive);
> > }
> > return err;
> > }
> >
> > And second, in ide_do_drive_cmd(), mdelay(2000)
> > after WIN_SETFEATURES is a bit rude.
>
> ...and wrongly placed. I just realized that ide_driveid_update(drive)
> actually talks with the drive!
>
> New patch is attached.
diff -urN linux-2.6.4.orig/drivers/ide/ide-io.c linux-2.6.4/drivers/ide/ide-io.c
--- linux-2.6.4.orig/drivers/ide/ide-io.c Fri Mar 12 11:15:00 2004
+++ linux-2.6.4/drivers/ide/ide-io.c Fri Mar 12 11:33:30 2004
@@ -1387,10 +1387,25 @@
err = 0;
if (must_wait) {
+ int xfer_rate = -1;
+ /* Are we going to do hdparm -X n ? */
HDIO_DRIVE_CMD is an ordinary ioctl, not some hdparm specific thing.
+ if(rq->buffer
+ && rq->buffer[0] == (char)WIN_SETFEATURES
+ && rq->buffer[2] == (char)SETFEATURES_XFER
+ ) {
+ xfer_rate = rq->buffer[1]; /* -X n */
+ }
+
wait_for_completion(&wait);
if (rq->errors)
err = -EIO;
+ if(!err && xfer_rate != -1) {
+ ide_delay_50ms(); /* be gentle */
Why?
+ /* ask chipset to change DMA/PIO timings */
+ ide_set_xfer_rate(drive, xfer_rate);
+ ide_driveid_update(drive);
+ }
blk_put_request(rq);
}
diff -urN linux-2.6.4.orig/drivers/ide/ide-iops.c linux-2.6.4/drivers/ide/ide-iops.c
--- linux-2.6.4.orig/drivers/ide/ide-iops.c Fri Mar 12 11:07:07 2004
+++ linux-2.6.4/drivers/ide/ide-iops.c Fri Mar 12 11:26:55 2004
@@ -660,52 +660,34 @@
EXPORT_SYMBOL(eighty_ninty_three);
-int ide_ata66_check (ide_drive_t *drive, ide_task_t *args)
+/*
+ * Is drive/channel capable of handling this?
+ * Currently checks only for ioctl(HDIO_DRIVE_CMD, SETFEATURES_XFER)
+ * (hdparm -X n)
+ */
+int unsupported_by_drive (ide_drive_t *drive, ide_task_t *args)
{
- if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
- (args->tfRegister[IDE_SECTOR_OFFSET] > XFER_UDMA_2) &&
- (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER)) {
+ if (args->tfRegister[IDE_COMMAND_OFFSET] != WIN_SETFEATURES) return 0;
+ if (args->tfRegister[IDE_FEATURE_OFFSET] != SETFEATURES_XFER) return 0;
+ if (args->tfRegister[IDE_SECTOR_OFFSET] <= XFER_UDMA_2) return 0;
+
#ifndef CONFIG_IDEDMA_IVB
- if ((drive->id->hw_config & 0x6000) == 0) {
+ if ( (drive->id->hw_config & 0x6000) == 0) {
#else /* !CONFIG_IDEDMA_IVB */
- if (((drive->id->hw_config & 0x2000) == 0) ||
- ((drive->id->hw_config & 0x4000) == 0)) {
+ if ( ((drive->id->hw_config & 0x2000) == 0) ||
+ ((drive->id->hw_config & 0x4000) == 0) ) {
#endif /* CONFIG_IDEDMA_IVB */
- printk("%s: Speed warnings UDMA 3/4/5 is not "
- "functional.\n", drive->name);
- return 1;
- }
- if (!HWIF(drive)->udma_four) {
- printk("%s: Speed warnings UDMA 3/4/5 is not "
- "functional.\n",
- HWIF(drive)->name);
- return 1;
- }
+ printk("%s is not capable of UDMA 3/4/5\n", drive->name);
+ return 1;
}
- return 0;
-}
-
-EXPORT_SYMBOL(ide_ata66_check);
-/*
- * Backside of HDIO_DRIVE_CMD call of SETFEATURES_XFER.
- * 1 : Safe to update drive->id DMA registers.
- * 0 : OOPs not allowed.
- */
-int set_transfer (ide_drive_t *drive, ide_task_t *args)
-{
- if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
- (args->tfRegister[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0) &&
- (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) &&
- (drive->id->dma_ultra ||
- drive->id->dma_mword ||
- drive->id->dma_1word))
+ if (!HWIF(drive)->udma_four) {
+ printk("%s is not capable of UDMA 3/4/5\n", HWIF(drive)->name);
return 1;
-
+ }
return 0;
}
-EXPORT_SYMBOL(set_transfer);
+EXPORT_SYMBOL(unsupported_by_drive);
This ide_ata66_check() -> unsupported_by_driver()
change is totally unnecessary.
Please also leave set_transfer() in place,
"no PIO" issue can be addressed later.
Regards,
Bartlomiej