Re: Fix 2.5.34+swsusp data corruption on IDE

From: Andre Hedrick (andre@linux-ide.org)
Date: Sat Sep 14 2002 - 15:40:18 EST


On Sat, 14 Sep 2002, Pavel Machek wrote:

> Hi!
>
> > I spoke with Jens, and he wants us to hold off until we can settle the
> > current issues. I have one concern about the model, and maybe you can
> > explain away the concern.
>
> Jens, where is the problem? This should have absolutely zero impact on
> "interesting" code, making only changes to initialization and
> suspend/resume...

I think we are looking to settle current problems before adding anything
else in the mix. It will go in somehow, okay.

> > Why are we not blocking read/write requests in the mainloop regardless?
> > If the request gets to the subdriver, ide-disk, has it not gotten to far
> > down the pipes?
>
> All processes capable of generating requests are safely stopped, so no
> request should get down to drivers. That's why I simply BUG_ON(), not
> block or anything more sophiscated. It should never ever happen.

You have to qualify it to R/W, and that has to be done high than where it
is now. What you can not block is the command to reset; regardless, if it
is soft or hard. This is how you wake a device, then you have to clean up
the mess resulting from the reset.

> > Specifically ls120's and zips.
> >
> > I understand you are address disk but suspend is more than disk in the
> > power management picture. Can you walk me through your process of sole
> > concern with platter media? Remember microdrvies are platters too, as are
> > flash drives, and memory drives.
>
> High levels are stopped, so there are no new requests coming.

Where?

> What is needed in idedisk_suspend is to make sure that no requests are
> "in flight". DMA scribbling over random memory is not good thing.
> idedisk_suspend then sends drive to standby to make sure writeback
> caches are flushed.

Well then you need to block the queue and the hold until the device goes
idle via check-power. Then Flush-Cache, and repeat the host idle check
until valid. Otherwise you have no way to insure all data is down.

Do you disagree with this point?

> as ls120s and zips... If they are DMA capable, they badly need
> support. If not, they should not kill rest of the system during
> suspend-to-disk, but still support would be nice.

hdc: ATAPI DVD-ROM DVD-RAM drive, 1024kB Cache, UDMA(33)

RAM is a r/w media.

 Model=MATSHITADVD-RAM LF-D210, FwRev=A106, SerialNo=
 Config={ Fixed Removeable DTR<=5Mbs DTR>10Mbs nonMagnetic }
 RawCHS=0/0/0, TrkSize=0, SectSize=0, ECCbytes=0
 BuffType=unknown, BuffSize=0kB, MaxMultSect=0
 (maybe): CurCHS=0/0/0, CurSects=0, LBA=yes, LBAsects=0
 IORDY=on/off, tPIO={min:180,w/IORDY:120}, tDMA={min:120,rec:120}
 PIO modes: pio0 pio1 pio2 pio3 pio4
 DMA modes: sdma0 sdma1 sdma2 mdma0 mdma1 mdma2 udma0 udma1 *udma2
 AdvancedPM=no
 Drive Supports : Reserved : ATA-4

hdg: 123264kB, 963/8/32 CHS, 533 kBps, 512 sector size, 720 rpm, DMA

 Model=LS-120 VER5 00 UHD Floppy, FwRev=F523M5A9, SerialNo=9727M9A01951
 Config={ Removeable nonMagnetic }
 RawCHS=963/8/32, TrkSize=49, SectSize=512, ECCbytes=0
 BuffType=unknown, BuffSize=0kB, MaxMultSect=0
 CurCHS=963/8/32, CurSects=3353542659, LBA=yes, LBAsects=3271557123
 IORDY=yes, tPIO={min:180,w/IORDY:180}, tDMA={min:150,rec:150}
 PIO modes: pio0 pio1 pio2 pio3
 DMA modes: sdma0 sdma1 mdma0 *mdma1
 AdvancedPM=no

Here are two types of media which use DMA and are read/write native.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

> > > +static int idedisk_suspend(struct device *dev, u32 state, u32 level)
> > > +{
> > > + ide_drive_t *drive = dev->driver_data;
> > > +
> > > + /* I hope that every freeze operations from the upper levels have
> > > + * already been done...
> > > + */
> > > +
> > > + BUG_ON(in_interrupt());
> > > +
> > > + if (level != SUSPEND_SAVE_STATE)
> > > + return 0;
> > > +
> > > + /* wait until all commands are finished */
> > > + /* FIXME: waiting for spinlocks should be done instead. */
> > > + while (HWGROUP(drive)->handler)
> > > + yield();
> > > +
> > > + /* set the drive to standby */
> > > + printk(KERN_INFO "suspending: %s ", drive->name);
> > > + if (drive->driver) {
> > > + if (drive->driver->standby)
> > > + drive->driver->standby(drive);
> > > + }
> > > + drive->blocked = 1;
> > > +
> > > + return 0;
>
> --
> Casualities in World Trade Center: ~3k dead inside the building,
> cryptography in U.S.A. and free speech in Czech Republic.
>

Andre Hedrick
LAD Storage Consulting Group

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Sun Sep 15 2002 - 22:00:36 EST