Skip to contentSkip to conversation

sata_inic162x: add big fat warning about broken LBA48 support

PATCH In review Tejun Heo 5 participants 11 messages Jun 28, 2007 – Jun 29, 2007 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 sata_inic162x: add big fat warning about broken LBA48 support

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

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/sata_inic162x.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/ata/sata_inic162x.c +5 -1
Index: 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);
#1 lore raw
Quoted 17 lines · Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/sata_inic162x.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: work/drivers/ata/sata_inic162x.c
===================================================================

drivers/ata/sata_inic162x.c +0 -1
@@ -664,8 +664,12 @@ static int inic_init_one(struct pci_dev
        void __iomem * const *iomap;
        int i, rc;
 
-       if (!printed_version++)

drivers/ata/sata_inic162x.c static int inic_init_one(struct pci_dev

Quoted 10 lines
drivers/ata/sata_inic162x.c +5 -1
+       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

Signature · 3 lines
Greg Freemyer
The Norcross Group
Forensics for the 21st Century
author
#2 lore raw
Quoted 6 lines · Greg Freemyer

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?

Signature · 1 line
tejun
#3 lore raw
Quoted 10 lines · On Fri, 29 Jun 2007 01:27:29 +0900 Tejun Heo <[email protected]> wrote:
Quoted 6 lines · Greg Freemyer

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

#4 lore raw
In reply to Tejun Heo
Quoted 8 lines · Tejun Heo
Quoted 6 lines · Greg Freemyer

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.

Quoted 6 lines

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

Signature · 3 lines
Greg Freemyer
The Norcross Group
Forensics for the 21st Century
#5 lore raw
In reply to Tejun Heo
Quoted 2 lines · Tejun Heo

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.

#6 lore raw
Quoted 5 lines · Mark Lord
Quoted 2 lines · Tejun Heo

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

author
#7 lore raw
Quoted 8 lines · Jeff Garzik
Quoted 5 lines · Mark Lord
Quoted 2 lines · Tejun Heo

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.

Signature · 1 line
tejun
#8 lore raw
Quoted 9 lines · Tejun Heo
Quoted 6 lines · Jeff Garzik
Quoted 4 lines · Mark Lord
Quoted 2 lines · Tejun Heo

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

#9 lore raw
Quoted 4 lines · Jeff Garzik
Quoted 3 lines · Tejun Heo
Quoted 2 lines · Jeff Garzik
Quoted 1 line · Mark Lord

Tejun Heo wrote:

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines

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..

author
#10 lore raw
Quoted 1 line · Mark Lord

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).

Quoted 6 lines

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.

Signature · 1 line
tejun