[PATCH] EDD: Check for correct EDD 3.0 length

[PATCH] EDD: Check for correct EDD 3.0 length

[Hannes Reinecke]

The device_path_info_length for EDD 3.0 is 36, not 44.
Cf http://mbldr.sourceforge.net/specsedd30.pdf.

This is a regression introduced by commit
0c61227094b3ddaca2f847ee287c4a2e3762b5a2

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: H. Peter Anvin <hpa@linux.intel.com>

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index e229576..09a77d5 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -545,8 +545,8 @@ edd_has_edd30(struct edd_device *edev)
 	}
 
 
-	/* We support only T13 spec */
-	if (info->params.device_path_info_length != 44)
+	/* EDD 3.0 specifies this to be 36 */
+	if (info->params.device_path_info_length != 36)
 		return 0;
 
 	for (i = 30; i < info->params.device_path_info_length + 30; i++)

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

Next time you resent an email say why you are doing it (wrong lkml
address in this case).

On Tue, May 15, 2012 at 01:04:49PM +0200, Hannes Reinecke wrote:
> The device_path_info_length for EDD 3.0 is 36, not 44.
> Cf http://mbldr.sourceforge.net/specsedd30.pdf.
> 
That's the wrong spec.

> This is a regression introduced by commit
> 0c61227094b3ddaca2f847ee287c4a2e3762b5a2
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> 
> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> index e229576..09a77d5 100644
> --- a/drivers/firmware/edd.c
> +++ b/drivers/firmware/edd.c
> @@ -545,8 +545,8 @@ edd_has_edd30(struct edd_device *edev)
>  	}
>  
>  
> -	/* We support only T13 spec */
> -	if (info->params.device_path_info_length != 44)
Here is the spec that code supports is spelled out, but you just replace
the comment with pointer to the spec that the code does not support.

> +	/* EDD 3.0 specifies this to be 36 */
> +	if (info->params.device_path_info_length != 36)
>  		return 0;
>  
>  	for (i = 30; i < info->params.device_path_info_length + 30; i++)

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Hannes Reinecke]

On 05/15/2012 01:12 PM, Gleb Natapov wrote:
> Next time you resent an email say why you are doing it (wrong lkml
> address in this case).
> 
> On Tue, May 15, 2012 at 01:04:49PM +0200, Hannes Reinecke wrote:
>> The device_path_info_length for EDD 3.0 is 36, not 44.
>> Cf http://mbldr.sourceforge.net/specsedd30.pdf.
>>
> That's the wrong spec.
> 
Ah-ha.
Why?

>> This is a regression introduced by commit
>> 0c61227094b3ddaca2f847ee287c4a2e3762b5a2
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Cc: Gleb Natapov <gleb@redhat.com>
>> Cc: H. Peter Anvin <hpa@linux.intel.com>
>>
>> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
>> index e229576..09a77d5 100644
>> --- a/drivers/firmware/edd.c
>> +++ b/drivers/firmware/edd.c
>> @@ -545,8 +545,8 @@ edd_has_edd30(struct edd_device *edev)
>>  	}
>>  
>>  
>> -	/* We support only T13 spec */
>> -	if (info->params.device_path_info_length != 44)
> Here is the spec that code supports is spelled out, but you just replace
> the comment with pointer to the spec that the code does not support.
> 
Hmm? How so?

I have this EDD info:

# hexdump -C raw_data
00000000  1e 00 03 00 14 04 00 00  ff 00 00 00 3f 00 00 00
|............?...|
00000010  00 00 00 01 00 00 00 00  00 02 ff ff ff ff dd be
|................|
00000020  24 00 00 00 50 43 49 00  46 49 42 52 45 00 00 00
|$...PCI.FIBRE...|
00000030  07 00 00 00 00 00 00 00  50 01 43 80 01 3c 94 48
|........P.C..<.H|
00000040  01 c8 00 00 00 00 00 00  00 00                    |..........|

which used to be perfectly valid, contains an 'interface_type'
string, and a device path. All perfectly okay.
And, as mentioned, used to be displayed ok prior to the mentioned
commit.

So why do you claim we can't display it anymore?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 01:20:41PM +0200, Hannes Reinecke wrote:
> On 05/15/2012 01:12 PM, Gleb Natapov wrote:
> > Next time you resent an email say why you are doing it (wrong lkml
> > address in this case).
> > 
> > On Tue, May 15, 2012 at 01:04:49PM +0200, Hannes Reinecke wrote:
> >> The device_path_info_length for EDD 3.0 is 36, not 44.
> >> Cf http://mbldr.sourceforge.net/specsedd30.pdf.
> >>
> > That's the wrong spec.
> > 
> Ah-ha.
> Why?
www.t13.org/documents/UploadedDocuments/docs2008/e08134r0-BIOS_Enhanced_Disk_Drive_Services_4.0.doc

> 
> >> This is a regression introduced by commit
> >> 0c61227094b3ddaca2f847ee287c4a2e3762b5a2
> >>
> >> Signed-off-by: Hannes Reinecke <hare@suse.de>
> >> Cc: Gleb Natapov <gleb@redhat.com>
> >> Cc: H. Peter Anvin <hpa@linux.intel.com>
> >>
> >> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> >> index e229576..09a77d5 100644
> >> --- a/drivers/firmware/edd.c
> >> +++ b/drivers/firmware/edd.c
> >> @@ -545,8 +545,8 @@ edd_has_edd30(struct edd_device *edev)
> >>  	}
> >>  
> >>  
> >> -	/* We support only T13 spec */
> >> -	if (info->params.device_path_info_length != 44)
> > Here is the spec that code supports is spelled out, but you just replace
> > the comment with pointer to the spec that the code does not support.
> > 
> Hmm? How so?
> 
> I have this EDD info:
> 
> # hexdump -C raw_data
> 00000000  1e 00 03 00 14 04 00 00  ff 00 00 00 3f 00 00 00
> |............?...|
> 00000010  00 00 00 01 00 00 00 00  00 02 ff ff ff ff dd be
> |................|
> 00000020  24 00 00 00 50 43 49 00  46 49 42 52 45 00 00 00
> |$...PCI.FIBRE...|
> 00000030  07 00 00 00 00 00 00 00  50 01 43 80 01 3c 94 48
> |........P.C..<.H|
> 00000040  01 c8 00 00 00 00 00 00  00 00                    |..........|
> 
> which used to be perfectly valid, contains an 'interface_type'
> string, and a device path. All perfectly okay.
> And, as mentioned, used to be displayed ok prior to the mentioned
> commit.
> 
It is valid according to one spec but invalid according to another.

> So why do you claim we can't display it anymore?
> 
Before commit 0c61227094b3ddaca2f847ee287c4a2e3762b5a2 the code didn't
calculate checksum correctly. The check always succeed, so when bios
provided information according to T13 EDD4.0 spec interface_type contained
garbage. After the commit checksum is calculated correctly, but according
to T13 EDD4.0 spec, so for BIOSes that supply info according to another
spec check fails. Since T13 EDD4.0 spec support modern interfaces (RAID,
SATA, SAS) which another spec omits, and for interfaces they both support
T13 EDD4.0 actually supply enough information to link edd entry to actual
device (another spec does not), I do not see support for other spec to
be important, but you are welcome to write support for it if you need
it. The only way I see to check what spec edd info corresponds to is to
calculate checksum according to both specs and see which one succeeds.

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 02:59:17PM +0300, Gleb Natapov wrote:
> > So why do you claim we can't display it anymore?
> > 
> Before commit 0c61227094b3ddaca2f847ee287c4a2e3762b5a2 the code didn't
> calculate checksum correctly. The check always succeed, so when bios
> provided information according to T13 EDD4.0 spec interface_type contained
Correction. If BIOS provided information _not_ according to T13 EDD4.0 spec
interface_type contained garbage since the reset of the code assumes the
EDD4.0 information.

Looked like that:
# cat /sys/firmware/edd/int13_dev80/interface 
SCSI            id: 0  lun: 1224979098644774912

> garbage. After the commit checksum is calculated correctly, but according
> to T13 EDD4.0 spec, so for BIOSes that supply info according to another
> spec check fails. Since T13 EDD4.0 spec support modern interfaces (RAID,
> SATA, SAS) which another spec omits, and for interfaces they both support
> T13 EDD4.0 actually supply enough information to link edd entry to actual
> device (another spec does not), I do not see support for other spec to
> be important, but you are welcome to write support for it if you need
> it. The only way I see to check what spec edd info corresponds to is to
> calculate checksum according to both specs and see which one succeeds.
> 
> --
> 			Gleb.
> --
> 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/

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> I do not see support for other spec to be important, but you are welcome
> to write support for it if you need it. The only way I see to check what
> spec edd info corresponds to is to calculate checksum according to both
> specs and see which one succeeds.

No it doesn't work like that. This is a regression for existing working
systems. It needs to be reverted or fixed. If it was a new feature you'd
have a point - but it isn't. You've broken stuff, undo the breakage.

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 02:49:45PM +0100, Alan Cox wrote:
> > I do not see support for other spec to be important, but you are welcome
> > to write support for it if you need it. The only way I see to check what
> > spec edd info corresponds to is to calculate checksum according to both
> > specs and see which one succeeds.
> 
> No it doesn't work like that. This is a regression for existing working
> systems. It needs to be reverted or fixed. If it was a new feature you'd
> have a point - but it isn't. You've broken stuff, undo the breakage.
> 
Code never supported anything but EDD4.0 spec. I checked history git.
Code erroneously tried to interpret EDD3.0 info according to EDD4.0 spec
providing garbage as a result.

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

On Tue, 15 May 2012 17:12:14 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Tue, May 15, 2012 at 02:49:45PM +0100, Alan Cox wrote:
> > > I do not see support for other spec to be important, but you are welcome
> > > to write support for it if you need it. The only way I see to check what
> > > spec edd info corresponds to is to calculate checksum according to both
> > > specs and see which one succeeds.
> > 
> > No it doesn't work like that. This is a regression for existing working
> > systems. It needs to be reverted or fixed. If it was a new feature you'd
> > have a point - but it isn't. You've broken stuff, undo the breakage.
> > 
> Code never supported anything but EDD4.0 spec. I checked history git.
> Code erroneously tried to interpret EDD3.0 info according to EDD4.0 spec
> providing garbage as a result.

Providing some valid data and info, which has now disappeared.

So I still think this is a regression. It wants fixing properly and that
patch should be reverted until it has been done right.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 03:18:49PM +0100, Alan Cox wrote:
> On Tue, 15 May 2012 17:12:14 +0300
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Tue, May 15, 2012 at 02:49:45PM +0100, Alan Cox wrote:
> > > > I do not see support for other spec to be important, but you are welcome
> > > > to write support for it if you need it. The only way I see to check what
> > > > spec edd info corresponds to is to calculate checksum according to both
> > > > specs and see which one succeeds.
> > > 
> > > No it doesn't work like that. This is a regression for existing working
> > > systems. It needs to be reverted or fixed. If it was a new feature you'd
> > > have a point - but it isn't. You've broken stuff, undo the breakage.
> > > 
> > Code never supported anything but EDD4.0 spec. I checked history git.
> > Code erroneously tried to interpret EDD3.0 info according to EDD4.0 spec
> > providing garbage as a result.
> 
> Providing some valid data and info, which has now disappeared.
> 
False. See my other mail:

# cat /sys/firmware/edd/int13_dev80/interface
SCSI            id: 0  lun: 1224979098644774912

This is what I got on my system. How is it useful?

> So I still think this is a regression. It wants fixing properly and that
> patch should be reverted until it has been done right.

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

On Tue, 15 May 2012 17:21:29 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Tue, May 15, 2012 at 03:18:49PM +0100, Alan Cox wrote:
> > On Tue, 15 May 2012 17:12:14 +0300
> > Gleb Natapov <gleb@redhat.com> wrote:
> > 
> > > On Tue, May 15, 2012 at 02:49:45PM +0100, Alan Cox wrote:
> > > > > I do not see support for other spec to be important, but you are welcome
> > > > > to write support for it if you need it. The only way I see to check what
> > > > > spec edd info corresponds to is to calculate checksum according to both
> > > > > specs and see which one succeeds.
> > > > 
> > > > No it doesn't work like that. This is a regression for existing working
> > > > systems. It needs to be reverted or fixed. If it was a new feature you'd
> > > > have a point - but it isn't. You've broken stuff, undo the breakage.
> > > > 
> > > Code never supported anything but EDD4.0 spec. I checked history git.
> > > Code erroneously tried to interpret EDD3.0 info according to EDD4.0 spec
> > > providing garbage as a result.
> > 
> > Providing some valid data and info, which has now disappeared.
> > 
> False. See my other mail:
> 
> # cat /sys/firmware/edd/int13_dev80/interface
> SCSI            id: 0  lun: 1224979098644774912
> 
> This is what I got on my system. How is it useful?

Sample case of .. 1

Have you checked how the various distribution grub installers and the
like respond to the various changes or having bits of their edd data
vanish mysteriously ?

It's been this way for a long time, there is no rush to fix it. So we
should fix it properly not break stuff.

Actual systems in the field are using both 3.0 and 4.0 so we need to deal
with that as part of fixing this. That is the reality of the situation.

1.1 I suspect we can not care about.

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 03:36:14PM +0100, Alan Cox wrote:
> On Tue, 15 May 2012 17:21:29 +0300
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Tue, May 15, 2012 at 03:18:49PM +0100, Alan Cox wrote:
> > > On Tue, 15 May 2012 17:12:14 +0300
> > > Gleb Natapov <gleb@redhat.com> wrote:
> > > 
> > > > On Tue, May 15, 2012 at 02:49:45PM +0100, Alan Cox wrote:
> > > > > > I do not see support for other spec to be important, but you are welcome
> > > > > > to write support for it if you need it. The only way I see to check what
> > > > > > spec edd info corresponds to is to calculate checksum according to both
> > > > > > specs and see which one succeeds.
> > > > > 
> > > > > No it doesn't work like that. This is a regression for existing working
> > > > > systems. It needs to be reverted or fixed. If it was a new feature you'd
> > > > > have a point - but it isn't. You've broken stuff, undo the breakage.
> > > > > 
> > > > Code never supported anything but EDD4.0 spec. I checked history git.
> > > > Code erroneously tried to interpret EDD3.0 info according to EDD4.0 spec
> > > > providing garbage as a result.
> > > 
> > > Providing some valid data and info, which has now disappeared.
> > > 
> > False. See my other mail:
> > 
> > # cat /sys/firmware/edd/int13_dev80/interface
> > SCSI            id: 0  lun: 1224979098644774912
> > 
> > This is what I got on my system. How is it useful?
> 
> Sample case of .. 1
> 
Not one. Any system with SCSI disk. Revert the patch and check on yours.

> Have you checked how the various distribution grub installers and the
> like respond to the various changes or having bits of their edd data
> vanish mysteriously ?
> 
That's how I entered this mess in the first place. Fedora/RHEL installer
didn't check edd info at all (no wonder, it was useless). It used mbr
signature to match HDD to int13. For vitalization this approach does not
work since usually disk images are unformatted, so I wanted to use EDD
info. That is when I found this bug and that phoenix EDD3 does not
provide enough info to match even basic things like ATA or SCSI.

> It's been this way for a long time, there is no rush to fix it. So we
> should fix it properly not break stuff.
> 
> Actual systems in the field are using both 3.0 and 4.0 so we need to deal
> with that as part of fixing this. That is the reality of the situation.
Again let me correct myself. This is not about 3.0 versus 4.0. This is
about Phoenix vs T13 spec. The code was written to support only T13.

> 
> 1.1 I suspect we can not care about.
> 
> Alan

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> > Sample case of .. 1
> > 
> Not one. Any system with SCSI disk. Revert the patch and check on yours.

I don't have a SCSI disk. In fact most users don't have a SCSI disk 8)

> > Actual systems in the field are using both 3.0 and 4.0 so we need to deal
> > with that as part of fixing this. That is the reality of the situation.
> Again let me correct myself. This is not about 3.0 versus 4.0. This is
> about Phoenix vs T13 spec. The code was written to support only T13.

Well actually the code was written to mishandle both in different ways.
You propose to change it to handle one correctly and completely fail on
the other. That isn't what needs to happen.

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 03:54:50PM +0100, Alan Cox wrote:
> > > Sample case of .. 1
> > > 
> > Not one. Any system with SCSI disk. Revert the patch and check on yours.
> 
> I don't have a SCSI disk. In fact most users don't have a SCSI disk 8)
> 
Most of them have SATA which is not even defined by Phoenix spec 8)

> > > Actual systems in the field are using both 3.0 and 4.0 so we need to deal
> > > with that as part of fixing this. That is the reality of the situation.
> > Again let me correct myself. This is not about 3.0 versus 4.0. This is
> > about Phoenix vs T13 spec. The code was written to support only T13.
> 
> Well actually the code was written to mishandle both in different ways.
This is simply not true. The code defines device path structure
according to T13 spec and clearly states in the header which spec it was
written to support. May be this is not clear from the thread but the code
still provides every other information except device path from Phoenix
EDD. You can still see pci device from /sys/firmware/edd/int13_80.

> You propose to change it to handle one correctly and completely fail on
> the other. That isn't what needs to happen.
> 
Not completely fail, only suppress information the code does not know
how to parse!

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> T13 EDD4.0 actually supply enough information to link edd entry to actual
> device (another spec does not), I do not see support for other spec to
> be important, but you are welcome to write support for it if you need
> it. The only way I see to check what spec edd info corresponds to is to
> calculate checksum according to both specs and see which one succeeds.

I also think you are confused on this: EDD 3.0 provides enough
information to link most types of hardware to the device. It provides the
PCI or Legacy path to the interface and the device LUN, or for 1394 etc
the WWID which can be used to cross match. It doesn't address port
multipliers.

EDD 1.1 provides sufficient information for old systems too via the FDPT
as they only have to deal with MFM/IDE/ATA controllers at the legacy
mappings and these are described by the FDPT.

Prior to that it gets a bit more interesting and you have to go poking in
BIOS magic and have the required spellbooks to hand. However right back
to the original 386 era PCs the data is available.

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 03:00:15PM +0100, Alan Cox wrote:
> > T13 EDD4.0 actually supply enough information to link edd entry to actual
> > device (another spec does not), I do not see support for other spec to
> > be important, but you are welcome to write support for it if you need
> > it. The only way I see to check what spec edd info corresponds to is to
> > calculate checksum according to both specs and see which one succeeds.
> 
> I also think you are confused on this: EDD 3.0 provides enough
> information to link most types of hardware to the device. It provides the
> PCI or Legacy path to the interface and the device LUN, or for 1394 etc
> the WWID which can be used to cross match. It doesn't address port
> multipliers.
> 
It is easy to be confused since there are two EDD 3 specs. First is from
Phoenix BIOS (linked at the first mail of the thread) and it does not have
enough info even for ATA. You can't tell primary ATA controller from secondary.
SCSI specify only LUN, not SCSI ID and so on. Second is from T3
(http://www.t13.org/documents/UploadedDocuments/docs2004/d1572r3-EDD3.pdf)
and fixes all that but specify length of device path to be 44 bytes.

So let me correct myself. The code works for EDD3 spec from T13 and that
is what it claims in the file header.

> EDD 1.1 provides sufficient information for old systems too via the FDPT
> as they only have to deal with MFM/IDE/ATA controllers at the legacy
> mappings and these are described by the FDPT.
> 
> Prior to that it gets a bit more interesting and you have to go poking in
> BIOS magic and have the required spellbooks to hand. However right back
> to the original 386 era PCs the data is available.
> 
> Alan

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> It is easy to be confused since there are two EDD 3 specs. First is from
> Phoenix BIOS (linked at the first mail of the thread) and it does not have
> enough info even for ATA. You can't tell primary ATA controller from secondary.

Yes you can - it's defined by the PCI specification for compatibility
mappings on hardware of that era. Once you move to newer systems with ATA
native PCI controllers then you need the 3.0 spec not the Phoenix spec.

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 03:52:06PM +0100, Alan Cox wrote:
> > It is easy to be confused since there are two EDD 3 specs. First is from
> > Phoenix BIOS (linked at the first mail of the thread) and it does not have
> > enough info even for ATA. You can't tell primary ATA controller from secondary.
> 
> Yes you can - it's defined by the PCI specification for compatibility
How? ATA device path has only Master/Slave. Interface path has only
bus/slot/function. So given all that in int13_80 how do I know if this
is primary or secondary controller?

T13 added has Channel number in Interface path.

> mappings on hardware of that era. Once you move to newer systems with ATA
> native PCI controllers then you need the 3.0 spec not the Phoenix spec.
> 
> Alan

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

On Tue, 15 May 2012 17:53:16 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Tue, May 15, 2012 at 03:52:06PM +0100, Alan Cox wrote:
> > > It is easy to be confused since there are two EDD 3 specs. First is from
> > > Phoenix BIOS (linked at the first mail of the thread) and it does not have
> > > enough info even for ATA. You can't tell primary ATA controller from secondary.
> > 
> > Yes you can - it's defined by the PCI specification for compatibility
> How? ATA device path has only Master/Slave. Interface path has only
> bus/slot/function. So given all that in int13_80 how do I know if this
> is primary or secondary controller?

For compatibility mode by the I/O port address.

For native mode most controllers pretend to be two devfns for exactly
this reason, and report one on each. Some non-MMIO devices don't and you
can tell them apart by looking at the addresses given in DPTE as well.

Simples.. 

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 04:22:26PM +0100, Alan Cox wrote:
> On Tue, 15 May 2012 17:53:16 +0300
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Tue, May 15, 2012 at 03:52:06PM +0100, Alan Cox wrote:
> > > > It is easy to be confused since there are two EDD 3 specs. First is from
> > > > Phoenix BIOS (linked at the first mail of the thread) and it does not have
> > > > enough info even for ATA. You can't tell primary ATA controller from secondary.
> > > 
> > > Yes you can - it's defined by the PCI specification for compatibility
> > How? ATA device path has only Master/Slave. Interface path has only
> > bus/slot/function. So given all that in int13_80 how do I know if this
> > is primary or secondary controller?
> 
> For compatibility mode by the I/O port address.
> 
Hmm, will work if standard I/O addresses are used for master/slave, but
I do not see this information be exposed via /sys by EDD. So how can I
do it from OS installer with current kernel or kernel with reverted
patch?

> For native mode most controllers pretend to be two devfns for exactly
> this reason, and report one on each. Some non-MMIO devices don't and you
> can tell them apart by looking at the addresses given in DPTE as well.
> 
> Simples.. 
> 
> Alan

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> Hmm, will work if standard I/O addresses are used for master/slave, but
> I do not see this information be exposed via /sys by EDD. So how can I
> do it from OS installer with current kernel or kernel with reverted
> patch?

I don't think we provide the needed info for this (or EDD 2.1 which I've
just discovered actually shipped in some plug in boards). Not sure 2.1 is
around online anywhere.

Thats a separate problem though.

My big concern is that we are sure it disappearing won't break anything.
It's been noticed at SuSE so it's obviously not entiely invisible.

I'd love to see EDD 1.1 and 2.1 support in the kernel as I could then
merge pata_hdd which drives original MFM/RLL drives. However I don't
believe its worth the effort unlike doing 3.0 right.

Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 04:41:43PM +0100, Alan Cox wrote:
> > Hmm, will work if standard I/O addresses are used for master/slave, but
> > I do not see this information be exposed via /sys by EDD. So how can I
> > do it from OS installer with current kernel or kernel with reverted
> > patch?
> 
> I don't think we provide the needed info for this (or EDD 2.1 which I've
> just discovered actually shipped in some plug in boards). Not sure 2.1 is
> around online anywhere.
> 
> Thats a separate problem though.
> 
It is not. You are saying that the code we have now supported Phoenix
spec and now it is not. This is not the case though.

> My big concern is that we are sure it disappearing won't break anything.
> It's been noticed at SuSE so it's obviously not entiely invisible.
So lets ask Hannes what problem he tries to fix by the patch.

> 
> I'd love to see EDD 1.1 and 2.1 support in the kernel as I could then
> merge pata_hdd which drives original MFM/RLL drives. However I don't
> believe its worth the effort unlike doing 3.0 right.
> 
It is OK to add Phoenix support to the code. To do that code needs to
figure what spec BIOS uses and parse the data differently, not just blindly
misinterpret binary buffer.

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> Phoenix BIOS (linked at the first mail of the thread) and it does not have
> enough info even for ATA.

<History lecture mode on>

In the beginning was the CMOS, and the CMOS was good.

Then people started adding extra drives and screwing around with mappings.

For this period there are a set of rituals (and I use the word
intentionally) for finding the geometry data and configuration mappings
which depend on which of the major religions your BIOS belonged to.

The entire thing got out of hand and so EDD was born

The base Phoenix spec is 1.1 not 3.0 and was published in 1995. It
assumes legacy mappings. It does have enough information for
compatibility mode ATA, which is all there was in the BIOS at the time.
Closely related to all this is the emergency of the Compaq/Phoenix/Intel
BIOS Boot Specification v1.01 (1996).

The follow on Phoenix spec is 3.0 (rev 0.8) and was published in
1998. That one addresses IEEE 1394 and some other bits. It does support
PCI bus device paths and lun identification to some extent but isn't
adequate for all complex topologies but is for most.

The T13 draft D3186 was published in 2000 and leads on to the EDD 4.0
proposal of 2008 which generally is what people consider EDD 3.0

So I think you have your specifications confused too.

I would suggest you read and compare the documents. In particular please
read Phoenix EDD 3.0 and pay attention to the sections 5.8-5.8.3 which
cover the device path and explain how EDD3.0 describes the actual device
to BIOS mapping. In particular the DPT interface path identifies the PCI
bus/devfn and the DPT device path identifiers if the unit is master or
slave.

In the absence of the device/interface path being entirely unique (or
indeed present at all) you use the DPTE words 0-3 to identify the mapping
for any SFF style ATA interface. This is sufficient to identify all non
MMIO configurations. The DPTE in 3.0 doesn't allow for pure MMIO
controllers. You just walk the address back to a device mapping and to
the controller/port in question.

</>



Alan

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Alan Cox]

> The T13 draft D3186 was published in 2000 and leads on to the EDD 4.0
> proposal of 2008 which generally is what people consider EDD 3.0

Typo - I mean 4.0 of course

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Tue, May 15, 2012 at 04:14:39PM +0100, Alan Cox wrote:
> > Phoenix BIOS (linked at the first mail of the thread) and it does not have
> > enough info even for ATA.
> 
> <History lecture mode on>
> 
> In the beginning was the CMOS, and the CMOS was good.
> 
> Then people started adding extra drives and screwing around with mappings.
> 
> For this period there are a set of rituals (and I use the word
> intentionally) for finding the geometry data and configuration mappings
> which depend on which of the major religions your BIOS belonged to.
> 
> The entire thing got out of hand and so EDD was born
> 
> The base Phoenix spec is 1.1 not 3.0 and was published in 1995. It
> assumes legacy mappings. It does have enough information for
> compatibility mode ATA, which is all there was in the BIOS at the time.
> Closely related to all this is the emergency of the Compaq/Phoenix/Intel
> BIOS Boot Specification v1.01 (1996).
> 
> The follow on Phoenix spec is 3.0 (rev 0.8) and was published in
> 1998. That one addresses IEEE 1394 and some other bits. It does support
> PCI bus device paths and lun identification to some extent but isn't
> adequate for all complex topologies but is for most.
> 
> The T13 draft D3186 was published in 2000 and leads on to the EDD 4.0
> proposal of 2008 which generally is what people consider EDD 3.0
> 
> So I think you have your specifications confused too.
> 
There is Phoenix spec 3.0 and there are T13 EDD3.0/4.0. They are
incompatible and you can't tell which one specific BIOS actually use.
Everything is correct till now?

> I would suggest you read and compare the documents. In particular please
I have them both open right now.

> read Phoenix EDD 3.0 and pay attention to the sections 5.8-5.8.3 which
> cover the device path and explain how EDD3.0 describes the actual device
> to BIOS mapping. In particular the DPT interface path identifies the PCI
> bus/devfn and the DPT device path identifiers if the unit is master or
> slave.
> 
Those sections is what I am looking at. I skipped DPTE part probably
because I didn't see Linux actually using it and I agree that you can use
I/O port to figure master/slave. It does not help much since the
information is not exposed to userspace (because code support different
spec where this is not needed) and because Phonix spec does not support
many other modern interfaces. The spec even specifies USB as TBD :) SATA
is missing completely.

> In the absence of the device/interface path being entirely unique (or
> indeed present at all) you use the DPTE words 0-3 to identify the mapping
> for any SFF style ATA interface. This is sufficient to identify all non
> MMIO configurations. The DPTE in 3.0 doesn't allow for pure MMIO
> controllers. You just walk the address back to a device mapping and to
> the controller/port in question.
> 
> </>
> 
> 
> 
> Alan

--
			Gleb.

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[H. Peter Anvin]

On 05/15/2012 08:46 AM, Gleb Natapov wrote:
> There is Phoenix spec 3.0 and there are T13 EDD3.0/4.0. They are
> incompatible and you can't tell which one specific BIOS actually use.
> Everything is correct till now?

You can by the data length.

But yes, there is Phoenix 3.0 (1998), T13 3.0 (2004) and T13 4.0 (2010),
although the latter seems to be stalled in final draft; according to the
T13 logs it went through final ballot resolution but doesn't seem to
have actually been published, unless it got merged into another document
(which is possible).

	-hpa

[PATCH] EDD: Check for correct EDD 3.0 length

[Hannes Reinecke]

There are two competing EDD 3.0 specifications,
the original one from Phoenix
<http://mbldr.sourceforge.net/specsedd30.pdf>
and the T-13 approved one
<http://www.t13.org/documents/UploadedDocuments/docs2004/d1572r3-EDD3.pdf>

They differ in the length of the device_path field, which
is one quad word for the Phoenix spec and two quad words
for the T-13 spec. So we need to test for both lengths
and blank out the second quad word when a Phoenix
version is detected.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index e229576..beedf4c 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -192,6 +192,13 @@ edd_show_interface(struct edd_device *edev, char *buf)
 			p += scnprintf(p, left, " ");
 		}
 	}
+	/*
+	 * Phoenix EDD 3.0 reserves only one quad word for the
+	 * device path, so blank out the second quad word.
+	 */
+	if (info->params.device_path_info_length == 36)
+		info->params.device_path.unknown.reserved2 = 0;
+
 	if (!strncmp(info->params.interface_type, "ATAPI", 5)) {
 		p += scnprintf(p, left, "\tdevice: %u  lun: %u\n",
 			     info->params.device_path.atapi.device,
@@ -545,8 +552,12 @@ edd_has_edd30(struct edd_device *edev)
 	}
 
 
-	/* We support only T13 spec */
-	if (info->params.device_path_info_length != 44)
+	/*
+	 * Phoenix EDD 3.0 specifies this to be 36,
+	 * T-13 EDD 3.0 uses 44. So check for both.
+	 */
+	if (info->params.device_path_info_length != 36 &&
+	    info->params.device_path_info_length != 44)
 		return 0;
 
 	for (i = 30; i < info->params.device_path_info_length + 30; i++)

Re: [PATCH] EDD: Check for correct EDD 3.0 length

[Gleb Natapov]

On Wed, May 16, 2012 at 08:51:36AM +0200, Hannes Reinecke wrote:
> There are two competing EDD 3.0 specifications,
> the original one from Phoenix
> <http://mbldr.sourceforge.net/specsedd30.pdf>
> and the T-13 approved one
> <http://www.t13.org/documents/UploadedDocuments/docs2004/d1572r3-EDD3.pdf>
> 
> They differ in the length of the device_path field, which
> is one quad word for the Phoenix spec and two quad words
> for the T-13 spec. So we need to test for both lengths
> and blank out the second quad word when a Phoenix
> version is detected.
> 
Can you please clarify what practical problem are you seeing that
you are trying to fix with this patch?

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> 
> diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
> index e229576..beedf4c 100644
> --- a/drivers/firmware/edd.c
> +++ b/drivers/firmware/edd.c
> @@ -192,6 +192,13 @@ edd_show_interface(struct edd_device *edev, char *buf)
>  			p += scnprintf(p, left, " ");
>  		}
>  	}
> +	/*
> +	 * Phoenix EDD 3.0 reserves only one quad word for the
> +	 * device path, so blank out the second quad word.
> +	 */
> +	if (info->params.device_path_info_length == 36)
> +		info->params.device_path.unknown.reserved2 = 0;
> +
You overwrite checksum here. Hope it will not be checked after that point.
This will work for all interface types defined by Phoenix spec except
SCSI as far as I see. In Phoenix spec SCSI interface is one byte LUN,
in T13 it is changed to be: word SCSI ID, qword LUN.

>  	if (!strncmp(info->params.interface_type, "ATAPI", 5)) {
>  		p += scnprintf(p, left, "\tdevice: %u  lun: %u\n",
>  			     info->params.device_path.atapi.device,
> @@ -545,8 +552,12 @@ edd_has_edd30(struct edd_device *edev)
>  	}
>  
>  
> -	/* We support only T13 spec */
> -	if (info->params.device_path_info_length != 44)
> +	/*
> +	 * Phoenix EDD 3.0 specifies this to be 36,
> +	 * T-13 EDD 3.0 uses 44. So check for both.
> +	 */
> +	if (info->params.device_path_info_length != 36 &&
> +	    info->params.device_path_info_length != 44)
>  		return 0;
>  
>  	for (i = 30; i < info->params.device_path_info_length + 30; i++)

--
			Gleb.