[PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Tejun Heo]

sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
reduce confusion.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/sata_inic162x.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: work/drivers/ata/sata_inic162x.c
===================================================================
--- work.orig/drivers/ata/sata_inic162x.c
+++ work/drivers/ata/sata_inic162x.c
@@ -664,8 +664,12 @@ static int inic_init_one(struct pci_dev 
 	void __iomem * const *iomap;
 	int i, rc;
 
-	if (!printed_version++)
+	if (!printed_version++) {
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
+		printk(KERN_WARNING "WARNING: sata_inic162x doesn't support "
+		       "LBA48 yet. Devices larger than\n         "
+		       "2^28 - 1 sectors (~127GiB) won't work.\n");
+	}
 
 	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, NR_PORTS);

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Greg Freemyer]

On 6/28/07, Tejun Heo <htejun@gmail.com> wrote:
> sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
> reduce confusion.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/sata_inic162x.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> Index: work/drivers/ata/sata_inic162x.c
> ===================================================================
> --- work.orig/drivers/ata/sata_inic162x.c
> +++ work/drivers/ata/sata_inic162x.c
> @@ -664,8 +664,12 @@ static int inic_init_one(struct pci_dev
>         void __iomem * const *iomap;
>         int i, rc;
>
> -       if (!printed_version++)
> +       if (!printed_version++) {
>                 dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
> +               printk(KERN_WARNING "WARNING: sata_inic162x doesn't support "
> +                      "LBA48 yet. Devices larger than\n         "
> +                      "2^28 - 1 sectors (~127GiB) won't work.\n");
> +       }
>
>         /* alloc host */
>         host = ata_host_alloc_pinfo(&pdev->dev, ppi, NR_PORTS);
> -

Does it simply fail?  Or does it corrupt?

In my Windows experience, if you try to write data past ~128GiB and
you don't have LBA48 support you get a wraparound effect that causes
corruption of the data below ~128GiB.  I've seen it happen several
times under Win2K in particular.

Greg
-- 
Greg Freemyer
The Norcross Group
Forensics for the 21st Century

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Tejun Heo]

Greg Freemyer wrote:
> Does it simply fail?  Or does it corrupt?
> 
> In my Windows experience, if you try to write data past ~128GiB and
> you don't have LBA48 support you get a wraparound effect that causes
> corruption of the data below ~128GiB.  I've seen it happen several
> times under Win2K in particular.

It will probably wrap and corrupt data.  The driver is already marked
HIGHLY EXPERIMENTAL.  Do you think we need bigger hammer?

-- 
tejun

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Alan Cox]

On Fri, 29 Jun 2007 01:27:29 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Greg Freemyer wrote:
> > Does it simply fail?  Or does it corrupt?
> > 
> > In my Windows experience, if you try to write data past ~128GiB and
> > you don't have LBA48 support you get a wraparound effect that causes
> > corruption of the data below ~128GiB.  I've seen it happen several
> > times under Win2K in particular.
> 
> It will probably wrap and corrupt data.  The driver is already marked
> HIGHLY EXPERIMENTAL.  Do you think we need bigger hammer

Probably if the driver has set the no lba48 flag and the drive is > 128GB
we need to clip the reported size of the volume and/or print a message
and skip it

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Greg Freemyer]

On 6/28/07, Tejun Heo <htejun@gmail.com> wrote:
> Greg Freemyer wrote:
> > Does it simply fail?  Or does it corrupt?
> >
> > In my Windows experience, if you try to write data past ~128GiB and
> > you don't have LBA48 support you get a wraparound effect that causes
> > corruption of the data below ~128GiB.  I've seen it happen several
> > times under Win2K in particular.
>
> It will probably wrap and corrupt data.  The driver is already marked
> HIGHLY EXPERIMENTAL.  Do you think we need bigger hammer?
>
> --
> tejun
>
At a minimum the error msg should be much stronger than "won't work".
Something like "will be horribly corrupted and all your valuable data
will be lost."

Even better from my perspective would be to simply cause a > 128GiB
drive to be ignored and totally unaccessible.  Obviously it should
have a message to that effect.

ie. Assume you have a partition that spans the 128 GiB barrier.  If
you allow access to the first half of the partition and not the last,
you have another major corruption possibility even though you don't
have any wrap-around affects.

Greg
-- 
Greg Freemyer
The Norcross Group
Forensics for the 21st Century

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Mark Lord]

Tejun Heo wrote:
> sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
> reduce confusion.

Why not whine only when an affected device is actually present?

Cheers from OLS.

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Jeff Garzik]

Mark Lord wrote:
> Tejun Heo wrote:
>> sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
>> reduce confusion.
> 
> Why not whine only when an affected device is actually present?

That's sorta that I think...

	Jeff

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Tejun Heo]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
>>> reduce confusion.
>>
>> Why not whine only when an affected device is actually present?
> 
> That's sorta that I think...

That was me being lazy.  I'll just ban > LBA28 disks on the controller.

-- 
tejun

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>> Tejun Heo wrote:
>>>> sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
>>>> reduce confusion.
>>> Why not whine only when an affected device is actually present?
>> That's sorta that I think...
> 
> That was me being lazy.  I'll just ban > LBA28 disks on the controller.

Sounds better to me...  I certainly prefer that to clipping-to-1xxGB, 
especially given the shaky state of the overall driver.

	Jeff

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Mark Lord]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Jeff Garzik wrote:
>>> Mark Lord wrote:
>>>> Tejun Heo wrote:
>>>>> sata_inic162x can't do LBA48 properly yet.  Whine loudly about it to
>>>>> reduce confusion.
>>>> Why not whine only when an affected device is actually present?
>>> That's sorta that I think...
>>
>> That was me being lazy.  I'll just ban > LBA28 disks on the controller.
> 
> Sounds better to me...  I certainly prefer that to clipping-to-1xxGB, 
> especially given the shaky state of the overall driver.

I wonder if PIO works for LBA48 on that chipset (very, *very* likely).

Maybe just fall back to PIO for an LBA48 drive.
Or even better, fall back to PIO only for sectors beyond 128GB.

???

Or wait for a more stable driver..

Re: [PATCH 2.6.22-rc6] sata_inic162x: add big fat warning about broken LBA48 support

[Tejun Heo]

Mark Lord wrote:
> I wonder if PIO works for LBA48 on that chipset (very, *very* likely).

HOB register access doesn't work (even get native max address is broken).

> Maybe just fall back to PIO for an LBA48 drive.
> Or even better, fall back to PIO only for sectors beyond 128GB.
> 
> ???
> 
> Or wait for a more stable driver..

I don't really think the driver is useful in its current state.  Maybe
we should just mark it BROKEN.  I don't think I'm gonna work on it
without further information and initio is almost impossible to contact.

-- 
tejun