[PATCH] libata: add warning about possible HPA compatibility issue

[PATCH] libata: add warning about possible HPA compatibility issue

[Bartlomiej Zolnierkiewicz]

Document "ignore_hpa" module parameter and add a warning about
possible HPA compatibility issue.

This should fix bug #13365:

	http://bugzilla.kernel.org/show_bug.cgi?id=13365

To: Jeff Garzik <jgarzik@pobox.com>
Reported-by: Jacob <fun2program8@yahoo.com>
Reported-by: Christopher Hogan <chris.c.hogan@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Robert Hancock <hancockrwd@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
If people are fine with the patch I can push it together with
ide fixes for 2.6.30-rc8.

 Documentation/kernel-parameters.txt |    6 ++++++
 drivers/ata/libata-core.c           |   15 +++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

Index: b/Documentation/kernel-parameters.txt
===================================================================
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1101,6 +1101,12 @@ and is between 256 and 4096 characters. 
 			If there are multiple matching configurations changing
 			the same attribute, the last one is used.
 
+	libata.ignore_hpa=
+			[LIBATA] Ignore HPA limit
+			Format: { "0" | "1" }
+			0 -- keep BIOS limits (default)
+			1 -- ignore limits, using full disk
+
 	lmb=debug	[KNL] Enable lmb debug messages.
 
 	load_ramdisk=	[RAM] List of ramdisks to load from floppy
Index: b/drivers/ata/libata-core.c
===================================================================
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1472,6 +1472,16 @@ static int ata_set_max_sectors(struct at
 	return 0;
 }
 
+static inline void ata_hpa_warn(unsigned long long first_sector,
+				unsigned long long last_sector)
+{
+	printk(KERN_WARNING "*** possible compatibility issue:\n"
+		"*** sectors %llu-%llu may be inaccessible\n"
+		"*** if Host Protected Area is used for filesystem data\n"
+		"*** (please refer to \"ignore_hpa\" module parameter)\n",
+		first_sector, last_sector);
+}
+
 /**
  *	ata_hpa_resize		-	Resize a device with an HPA set
  *	@dev: Device to resize
@@ -1521,12 +1531,13 @@ static int ata_hpa_resize(struct ata_dev
 		if (!print_info || native_sectors == sectors)
 			return 0;
 
-		if (native_sectors > sectors)
+		if (native_sectors > sectors) {
 			ata_dev_printk(dev, KERN_INFO,
 				"HPA detected: current %llu, native %llu\n",
 				(unsigned long long)sectors,
 				(unsigned long long)native_sectors);
-		else if (native_sectors < sectors)
+			ata_hpa_warn(sectors, native_sectors - 1);
+		} else if (native_sectors < sectors)
 			ata_dev_printk(dev, KERN_WARNING,
 				"native sectors (%llu) is smaller than "
 				"sectors (%llu)\n",

Re: [PATCH] libata: add warning about possible HPA compatibility issue

[Alan Cox]

> +static inline void ata_hpa_warn(unsigned long long first_sector,
> +				unsigned long long last_sector)
> +{
> +	printk(KERN_WARNING "*** possible compatibility issue:\n"
> +		"*** sectors %llu-%llu may be inaccessible\n"
> +		"*** if Host Protected Area is used for filesystem data\n"
> +		"*** (please refer to \"ignore_hpa\" module parameter)\n",
> +		first_sector, last_sector);
> +}

NAK - this is extremely misleading and will just randomly terrify users,
who may then set the paramater and corrupt things like BIOS save areas.

Re: [PATCH] libata: add warning about possible HPA compatibility issue

[Bartlomiej Zolnierkiewicz]

On Tuesday 26 May 2009 00:20:14 Alan Cox wrote:
> > +static inline void ata_hpa_warn(unsigned long long first_sector,
> > +				unsigned long long last_sector)
> > +{
> > +	printk(KERN_WARNING "*** possible compatibility issue:\n"
> > +		"*** sectors %llu-%llu may be inaccessible\n"
> > +		"*** if Host Protected Area is used for filesystem data\n"
> > +		"*** (please refer to \"ignore_hpa\" module parameter)\n",
> > +		first_sector, last_sector);
> > +}
> 
> NAK - this is extremely misleading and will just randomly terrify users,
> who may then set the paramater and corrupt things like BIOS save areas.

Please propose better description.

Re: [PATCH] libata: add warning about possible HPA compatibility issue

[Alan Cox]

> > NAK - this is extremely misleading and will just randomly terrify users,
> > who may then set the paramater and corrupt things like BIOS save areas.
> 
> Please propose better description.

As was discussed before

Tejun has a patch in the queue which exposes the HPA and real sizes to
the OS

Libata can support revalidating a disc

Once the HPA/Real size data is there the tools can be tuned for this and
a sysfs node added to switch hpa on/off per disc. At that point this can
be done by the user space intelligently.

The fact this is a user parameter is itself a problem.

Re: [PATCH] libata: add warning about possible HPA compatibility issue

[Bartlomiej Zolnierkiewicz]

On Tuesday 26 May 2009 00:38:14 Alan Cox wrote:
> > > NAK - this is extremely misleading and will just randomly terrify users,
> > > who may then set the paramater and corrupt things like BIOS save areas.
> > 
> > Please propose better description.
> 
> As was discussed before
> 
> Tejun has a patch in the queue which exposes the HPA and real sizes to
> the OS

In queue for 2.6.30?

> Libata can support revalidating a disc
> 
> Once the HPA/Real size data is there the tools can be tuned for this and
> a sysfs node added to switch hpa on/off per disc. At that point this can
> be done by the user space intelligently.

Unless you happen to have partition with tools in HPA area etc.

Also care to post link to the existing user-space tools with added HPA
support and documentation how to use them so I can start pointing people
reporting problems in this direction?

> The fact this is a user parameter is itself a problem.

Yes.  It is a problem but we can't reverse the history.

I fully agree w.r.t to proper handling of HPA but we need "something" in
the meantime because it is *real* issue resulting in *real* users filling
*real* bug-reports (which unfortunately always get assigned to me first
so I'm needlessly wasting my time on them).

IOW Please stop downplaying bug #13365 which this patch tries to alleviate.

I'm adding Andrew to cc:...

Re: [PATCH] libata: add warning about possible HPA compatibility issue

[Alan Cox]

> > Tejun has a patch in the queue which exposes the HPA and real sizes to
> > the OS
> 
> In queue for 2.6.30?

2.6.30 merge window is closed, as you well kow

> I fully agree w.r.t to proper handling of HPA but we need "something" in
> the meantime because it is *real* issue resulting in *real* users filling
> *real* bug-reports (which unfortunately always get assigned to me first
> so I'm needlessly wasting my time on them).

The merge window is closed. This is a 2.6.31 discussion and randomly
adding bogus printks to the kernel won't help because

- Users have no idea what they mean
- Distros boot with "quiet" so users don't see them anyway

It would be good to get Tejun's patch upstream and also runtime HPA
switching.

> IOW Please stop downplaying bug #13365 which this patch tries to alleviate.

I'm still not convinced 13365 is an HPA problem entirely - but there are
certainly other HPA confusions here and there.

> I'm adding Andrew to cc:...

Hello Andrew ;)

Alan