2003-08-26 22:33:30

by Jan Niehusmann

[permalink] [raw]
Subject: Promise IDE patches

Hi!

Two weeks ago, I tried two patches against 2.4.21 regarding LBA48
support. One limits the size of a drive to 137GB if LBA48 is not
available. Without this patch, severe data corruption is possible.

http://gondor.com/linux/patch-limit48-2.4.21

The other one is making LBA48 support work with pdc 20265 controllers.

http://gondor.com/linux/patch-pdc-lba48-2.4.22

I think they should be candidates for inclusion in 2.4.23, as well as
a fix for hdparm -I (and other commands going directly to the drive) on
(some?) promise controllers:

http://marc.theaimsgroup.com/?l=linux-kernel&m=104250818527780&w=2

Jan


Subject: Re: Promise IDE patches


Thanks Jan.

Marcelo can you apply these patches?

On Wednesday 27 of August 2003 00:31, Jan Niehusmann wrote:
> Hi!
>
> Two weeks ago, I tried two patches against 2.4.21 regarding LBA48
> support. One limits the size of a drive to 137GB if LBA48 is not
> available. Without this patch, severe data corruption is possible.
>
> http://gondor.com/linux/patch-limit48-2.4.21
>
> The other one is making LBA48 support work with pdc 20265 controllers.
>
> http://gondor.com/linux/patch-pdc-lba48-2.4.22
>
> I think they should be candidates for inclusion in 2.4.23, as well as
> a fix for hdparm -I (and other commands going directly to the drive) on
> (some?) promise controllers:
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=104250818527780&w=2
>
> Jan

2003-08-27 01:26:15

by Erik Andersen

[permalink] [raw]
Subject: Re: Promise IDE patches

> Thanks Jan.
>
> Marcelo can you apply these patches?
>
> On Wednesday 27 of August 2003 00:31, Jan Niehusmann wrote:
> > Hi!
> >
> > Two weeks ago, I tried two patches against 2.4.21 regarding
> > LBA48
> > support. One limits the size of a drive to 137GB if LBA48 is
> > not
> > available. Without this patch, severe data corruption is
> > possible.
> >
> > http://gondor.com/linux/patch-limit48-2.4.21

My recent IDE patch also fixes this problem....

http://marc.theaimsgroup.com/?l=linux-kernel&m=106159060721871&w=2

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

2003-08-27 14:59:24

by Alan

[permalink] [raw]
Subject: Re: Promise IDE patches

On Maw, 2003-08-26 at 23:54, Bartlomiej Zolnierkiewicz wrote:
> Thanks Jan.
>
> Marcelo can you apply these patches?

The last one looks wrong to me. At the least it needs to check
which pdc202xx chip first

2003-08-27 15:00:40

by Alan

[permalink] [raw]
Subject: Re: Promise IDE patches

On Maw, 2003-08-26 at 23:54, Bartlomiej Zolnierkiewicz wrote:
> Thanks Jan.
>
> Marcelo can you apply these patches?

The first one of these is wrong too. It'll stop some people from
being able to access existing file systems. This has to be fixed
properly (in both 2.4 and 2.6) to distinguish between
"Can PIO LBA48" "Can DMA LBA48" and "no LBA48". The latter being
basically non existant.

If you fail to do this then existing PIO LBA48 setups will just die
on boot.

2003-08-27 15:12:39

by Jan Niehusmann

[permalink] [raw]
Subject: Re: Promise IDE patches

On Wed, Aug 27, 2003 at 03:59:52PM +0100, Alan Cox wrote:
> If you fail to do this then existing PIO LBA48 setups will just die
> on boot.

But 'die on boot' is much better than 'silently currupt data', don't you
think?
Still, a proper fix would work in all cases...

What do you think about a check in __ide_do_rw_disk? calling
lba_28_rw_disk or chs_rw_disk when the block address doesn't fit in
28bit is surely wrong and should return an error.

This will lead to read or write errors if LBA48 doesn't work, but
it will prevent silent data corruption.

Jan

Subject: Re: Promise IDE patches

On Wednesday 27 of August 2003 16:58, Alan Cox wrote:
> On Maw, 2003-08-26 at 23:54, Bartlomiej Zolnierkiewicz wrote:
> > Thanks Jan.
> >
> > Marcelo can you apply these patches?
>
> The last one looks wrong to me. At the least it needs to check
> which pdc202xx chip first

Yep, you are right.
[ I guess that PIO forced is 20246 specific, but we don't know. ]

--bartlomiej

2003-08-27 15:21:14

by Jan Niehusmann

[permalink] [raw]
Subject: Re: Promise IDE patches

On Wed, Aug 27, 2003 at 05:12:27PM +0200, Jan Niehusmann wrote:
> What do you think about a check in __ide_do_rw_disk? calling
> lba_28_rw_disk or chs_rw_disk when the block address doesn't fit in
> 28bit is surely wrong and should return an error.

Sorry - I didn't notice that this straight forward distinction is only
possible in the CONFIG_IDE_TASKFILE_IO case.

Jan

Subject: Re: Promise IDE patches

On Wednesday 27 of August 2003 16:59, Alan Cox wrote:
> On Maw, 2003-08-26 at 23:54, Bartlomiej Zolnierkiewicz wrote:
> > Thanks Jan.
> >
> > Marcelo can you apply these patches?
>
> The first one of these is wrong too. It'll stop some people from
> being able to access existing file systems. This has to be fixed
> properly (in both 2.4 and 2.6) to distinguish between
> "Can PIO LBA48" "Can DMA LBA48" and "no LBA48". The latter being
> basically non existant.
>
> If you fail to do this then existing PIO LBA48 setups will just die
> on boot.

Alan, its not true. Please check with ide-disk.c.
If hwif->addressing is 1 then probe_lba_addressing() wont set
drive->addressing to 1 and therefore you wont get LBA48
(both PIO and DMA) because of drive->addressing check
insided __ide_do_rw_disk().

Its not complete fix but it doesnt break anything and stops fs corruption.

--bartlomiej

2003-08-27 15:46:16

by Alan

[permalink] [raw]
Subject: Re: Promise IDE patches

On Mer, 2003-08-27 at 16:12, Jan Niehusmann wrote:
> On Wed, Aug 27, 2003 at 03:59:52PM +0100, Alan Cox wrote:
> > If you fail to do this then existing PIO LBA48 setups will just die
> > on boot.
>
> But 'die on boot' is much better than 'silently currupt data', don't you
> think?
> Still, a proper fix would work in all cases...

You'd prevent people from using valid working file systems in ways
that don't corrupt.

I think the logic we need is something like

if drive_is_lba48 && controller_doesnt_do_lba48
clip_capacity

as a variant of the current change. That covers the "no way" case. For
the others we need to check capacity requires lba48 && !dma_lba48
somewhere, probably ide_dma_check. That would make us drop out of DMA
for unsafe setups in a way that doesnt cause crashes or suprises and
still let everyone access the data properly.