diff mbox

[U-Boot,v0,13/20] efi_loader: use proper device-paths for partitions

Message ID 201708061837.v76IbIwY021924@glazunov.sibelius.xs4all.nl
State Deferred
Delegated to: Alexander Graf
Headers show

Commit Message

Mark Kettenis Aug. 6, 2017, 6:37 p.m. UTC
> Date: Sun, 6 Aug 2017 20:21:45 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> 
> > Mind sending me or pastebin'ing your u-boot .config?  There are some
> > different device-path construction depending on legacy vs
> > CONFIG_DM+CONFIG_BLK (the legacy case *looks* right to me, and is used
> > by vexpress_ca15_tc2.. so I think it should work..)
> 
> See below.  The Banana Pi (and all other sunxi boards) indeed uses the
> legacy code path.  And I think there is a bug in the legacy codepath
> where it encodes the partition in the "file" path component.

If I fix the code to not insert the partition number there, I can boot
from SD card and SATA again.

Comments

Rob Clark Aug. 6, 2017, 6:47 p.m. UTC | #1
On Sun, Aug 6, 2017 at 2:37 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Sun, 6 Aug 2017 20:21:45 +0200 (CEST)
>> From: Mark Kettenis <mark.kettenis@xs4all.nl>
>>
>> > Mind sending me or pastebin'ing your u-boot .config?  There are some
>> > different device-path construction depending on legacy vs
>> > CONFIG_DM+CONFIG_BLK (the legacy case *looks* right to me, and is used
>> > by vexpress_ca15_tc2.. so I think it should work..)
>>
>> See below.  The Banana Pi (and all other sunxi boards) indeed uses the
>> legacy code path.  And I think there is a bug in the legacy codepath
>> where it encodes the partition in the "file" path component.
>
> If I fix the code to not insert the partition number there, I can boot
> from SD card and SATA again.
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index b5acf73f98..8ba0db2d7a 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -305,8 +305,8 @@ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part)
>         struct efi_device_path_file_path *fp;
>         char devname[32] = { 0 }; /* fp->str is u16[32] long */
>
> -       snprintf(devname, sizeof(devname), "%d.%d.%d", desc->if_type,
> -                desc->devnum, part);
> +       snprintf(devname, sizeof(devname), "%d.%d", desc->if_type,
> +                desc->devnum);
>
>         memcpy(buf, &ROOT, sizeof(ROOT));
>         buf += sizeof(ROOT);


Hmm, that is probably not a good idea, since now the disk object along
w/ partition objects will have same devicepath.  (One change from
before is now we have diskobjs for the disk (part=0) and child
diskobjs for each partition.. fwiw in UEFI terminology a
efi_device_path_hard_drive_path is actually a partition.. for maximum
confusion)

Probably that we have a file-path node w/ child hard-drive objects is
confusing your previous workaround.. let me see if I can come up with
something better.

BR,
-R
Rob Clark Aug. 6, 2017, 6:53 p.m. UTC | #2
On Sun, Aug 6, 2017 at 2:47 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Sun, Aug 6, 2017 at 2:37 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>> Date: Sun, 6 Aug 2017 20:21:45 +0200 (CEST)
>>> From: Mark Kettenis <mark.kettenis@xs4all.nl>
>>>
>>> > Mind sending me or pastebin'ing your u-boot .config?  There are some
>>> > different device-path construction depending on legacy vs
>>> > CONFIG_DM+CONFIG_BLK (the legacy case *looks* right to me, and is used
>>> > by vexpress_ca15_tc2.. so I think it should work..)
>>>
>>> See below.  The Banana Pi (and all other sunxi boards) indeed uses the
>>> legacy code path.  And I think there is a bug in the legacy codepath
>>> where it encodes the partition in the "file" path component.
>>
>> If I fix the code to not insert the partition number there, I can boot
>> from SD card and SATA again.
>>
>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>> index b5acf73f98..8ba0db2d7a 100644
>> --- a/lib/efi_loader/efi_device_path.c
>> +++ b/lib/efi_loader/efi_device_path.c
>> @@ -305,8 +305,8 @@ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part)
>>         struct efi_device_path_file_path *fp;
>>         char devname[32] = { 0 }; /* fp->str is u16[32] long */
>>
>> -       snprintf(devname, sizeof(devname), "%d.%d.%d", desc->if_type,
>> -                desc->devnum, part);
>> +       snprintf(devname, sizeof(devname), "%d.%d", desc->if_type,
>> +                desc->devnum);
>>
>>         memcpy(buf, &ROOT, sizeof(ROOT));
>>         buf += sizeof(ROOT);
>
>
> Hmm, that is probably not a good idea, since now the disk object along
> w/ partition objects will have same devicepath.  (One change from
> before is now we have diskobjs for the disk (part=0) and child
> diskobjs for each partition.. fwiw in UEFI terminology a
> efi_device_path_hard_drive_path is actually a partition.. for maximum
> confusion)

Oh, scratch that.. you are right, part shouldn't be in the string..
since it results in the children having different parent device paths.

(It is still probably a bit funny to file-paths for parent of the
partition objects.. I'll try to come up with something a bit better.)

BR,
-R
diff mbox

Patch

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index b5acf73f98..8ba0db2d7a 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -305,8 +305,8 @@  static void *dp_part_fill(void *buf, struct blk_desc *desc, int part)
 	struct efi_device_path_file_path *fp;
 	char devname[32] = { 0 }; /* fp->str is u16[32] long */
 
-	snprintf(devname, sizeof(devname), "%d.%d.%d", desc->if_type,
-		 desc->devnum, part);
+	snprintf(devname, sizeof(devname), "%d.%d", desc->if_type,
+		 desc->devnum);
 
 	memcpy(buf, &ROOT, sizeof(ROOT));
 	buf += sizeof(ROOT);