u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] efi_loader: provide media ID
@ 2022-09-15 20:02 Heinrich Schuchardt
  2022-09-15 20:02 ` [PATCH 1/2] dm: blk: assign media ID to block devices Heinrich Schuchardt
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-15 20:02 UTC (permalink / raw)
  To: Simon Glass, Ilias Apalodimas
  Cc: AKASHI Takahiro, Masahisa Kojima, u-boot, Heinrich Schuchardt

The medium a device like 'mmc 0' or 'usb 0' points to may change over
time. Hence device type and number are not sufficient to identify the
inserted medium. The same is true for the device path generated for
such a device. This is why the EFI_BLOCK_IO_PROTOCOL provides a field
MediaId.

Whenever a removable medium is changed or a new block device with a
previously used device path is created we should provide a different
MediaID.

This series adds a field media_id to the block device descriptor and fills
it after probing. The value of the field is then copied to the
EFI_BLOCK_IO_PROTOCOL.

With future patches we can refine this in sub-systems like USB, MMC, SCSI
to indicate media changes

Heinrich Schuchardt (2):
  dm: blk: assign media ID to block devices
  efi_loader: fill media_id from block device descriptor

 drivers/block/blk-uclass.c | 16 +++++++++++++++-
 include/blk.h              | 11 +++++++++++
 lib/efi_loader/efi_disk.c  |  6 +-----
 3 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.37.2


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] dm: blk: assign media ID to block devices
  2022-09-15 20:02 [PATCH 0/2] efi_loader: provide media ID Heinrich Schuchardt
@ 2022-09-15 20:02 ` Heinrich Schuchardt
  2022-09-16  1:30   ` Simon Glass
  2022-09-15 20:02 ` [PATCH 2/2] efi_loader: fill media_id from block device descriptor Heinrich Schuchardt
  2022-09-16  0:58 ` [PATCH 0/2] efi_loader: provide media ID AKASHI Takahiro
  2 siblings, 1 reply; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-15 20:02 UTC (permalink / raw)
  To: Simon Glass, Ilias Apalodimas
  Cc: AKASHI Takahiro, Masahisa Kojima, u-boot, Heinrich Schuchardt

Currently block devices are only identified by uclass_id and device number.
When dealing with removable media this is not enough to uniquely identify
the medium.

E.g. after host unbind, host bind we can have the same device number but a
different backing file.

The EFI specification uses a 32bit number media ID to identify media. Add a
matching field to the block device descriptor.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/block/blk-uclass.c | 16 +++++++++++++++-
 include/blk.h              | 11 +++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 21c5209bb6..c6f6be81c3 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -51,6 +51,11 @@ static enum uclass_id if_type_uclass_id[IF_TYPE_COUNT] = {
 	[IF_TYPE_PVBLOCK]	= UCLASS_PVBLOCK,
 };
 
+/*
+ * Global counter for media IDs
+ */
+static u32 media_id_counter;
+
 static enum if_type if_typename_to_iftype(const char *if_typename)
 {
 	int i;
@@ -741,11 +746,20 @@ int blk_unbind_all(int if_type)
 	return 0;
 }
 
+void blk_assign_media_id(struct blk_desc *desc)
+{
+	desc->media_id = ++media_id_counter;
+}
+
 static int blk_post_probe(struct udevice *dev)
 {
+	struct blk_desc *desc = dev_get_uclass_plat(dev);
+
+	if (!desc->media_id)
+		blk_assign_media_id(desc);
+
 	if (CONFIG_IS_ENABLED(PARTITIONS) &&
 	    IS_ENABLED(CONFIG_HAVE_BLOCK_DEVICE)) {
-		struct blk_desc *desc = dev_get_uclass_plat(dev);
 
 		part_init(desc);
 
diff --git a/include/blk.h b/include/blk.h
index 9503369db8..c176f8374e 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -77,6 +77,7 @@ struct blk_desc {
 	unsigned char	hwpart;		/* HW partition, e.g. for eMMC */
 	unsigned char	type;		/* device type */
 	unsigned char	removable;	/* removable device */
+	u32		media_id;	/* ID to identify media */
 #ifdef CONFIG_LBA48
 	/* device can use 48bit addr (ATA/ATAPI v7) */
 	unsigned char	lba48;
@@ -808,4 +809,14 @@ int blk_find_next(enum blk_flag_t flags, struct udevice **devp);
  */
 int blk_count_devices(enum blk_flag_t flag);
 
+/**
+ * blk_assign_media_id() - assign a media ID to the block device
+ *
+ * The field media_id of the block device descriptor is filled from a global
+ * counter.
+ *
+ * @desc:	block device descriptor
+ */
+void blk_assign_media_id(struct blk_desc *desc);
+
 #endif
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] efi_loader: fill media_id from block device descriptor
  2022-09-15 20:02 [PATCH 0/2] efi_loader: provide media ID Heinrich Schuchardt
  2022-09-15 20:02 ` [PATCH 1/2] dm: blk: assign media ID to block devices Heinrich Schuchardt
@ 2022-09-15 20:02 ` Heinrich Schuchardt
  2022-09-23  7:07   ` Ilias Apalodimas
  2022-09-16  0:58 ` [PATCH 0/2] efi_loader: provide media ID AKASHI Takahiro
  2 siblings, 1 reply; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-15 20:02 UTC (permalink / raw)
  To: Simon Glass, Ilias Apalodimas
  Cc: AKASHI Takahiro, Masahisa Kojima, u-boot, Heinrich Schuchardt

Fill the media ID in the block IO protocol from the block device descriptor
of the driver model.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/efi_loader/efi_disk.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 73745ccaa0..35790aa86d 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -476,11 +476,7 @@ static efi_status_t efi_disk_add_dev(
 	/* Fill in EFI IO Media info (for read/write callbacks) */
 	diskobj->media.removable_media = desc->removable;
 	diskobj->media.media_present = 1;
-	/*
-	 * MediaID is just an arbitrary counter.
-	 * We have to change it if the medium is removed or changed.
-	 */
-	diskobj->media.media_id = 1;
+	diskobj->media.media_id = desc->media_id;
 	diskobj->media.block_size = desc->blksz;
 	diskobj->media.io_align = desc->blksz;
 	if (part)
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-15 20:02 [PATCH 0/2] efi_loader: provide media ID Heinrich Schuchardt
  2022-09-15 20:02 ` [PATCH 1/2] dm: blk: assign media ID to block devices Heinrich Schuchardt
  2022-09-15 20:02 ` [PATCH 2/2] efi_loader: fill media_id from block device descriptor Heinrich Schuchardt
@ 2022-09-16  0:58 ` AKASHI Takahiro
  2022-09-26  6:06   ` Heinrich Schuchardt
  2 siblings, 1 reply; 17+ messages in thread
From: AKASHI Takahiro @ 2022-09-16  0:58 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Simon Glass, Ilias Apalodimas, Masahisa Kojima, u-boot

On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
> The medium a device like 'mmc 0' or 'usb 0' points to may change over
> time. Hence device type and number are not sufficient to identify the
> inserted medium. The same is true for the device path generated for
> such a device.

Well, it depends on how a device path is generated in U-Boot's UEFI
implementation. I believe that a device path represents an "unique path"
to a given device however this device is enumerated.
In this sense, the current dp_fill()/efi_dp_from_part() is not a right
implementation as it relies on device numbers.
Furthermore, a generated device path here is different from one generated
by EDK2 (even if both software are run on the same board).

This is an issue that I used to tackle in
https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
although I have since had no progress.

> This is why the EFI_BLOCK_IO_PROTOCOL provides a field
> MediaId.
> 
> Whenever a removable medium is changed or a new block device with a
> previously used device path is created we should provide a different
> MediaID.
> 
> This series adds a field media_id to the block device descriptor and fills
> it after probing. The value of the field is then copied to the
> EFI_BLOCK_IO_PROTOCOL.

I'm afraid that your patch doesn't always work as you expect.
When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
all the existing devices and associated blk_desc structures are once freed
and even if nothing is changed, i.e. a device is neither removed nor added,
the exact same structures will be re-created.
With your patch applied, however, a new (and different) "media_id" will be
assigned to an existing device. UEFI User may be notified of "media change".
(To be honest, this is quite unlikely because the current UEFI implementation
doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)

-Takahiro Akashi

> With future patches we can refine this in sub-systems like USB, MMC, SCSI
> to indicate media changes
> 
> Heinrich Schuchardt (2):
>   dm: blk: assign media ID to block devices
>   efi_loader: fill media_id from block device descriptor
> 
>  drivers/block/blk-uclass.c | 16 +++++++++++++++-
>  include/blk.h              | 11 +++++++++++
>  lib/efi_loader/efi_disk.c  |  6 +-----
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> -- 
> 2.37.2
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] dm: blk: assign media ID to block devices
  2022-09-15 20:02 ` [PATCH 1/2] dm: blk: assign media ID to block devices Heinrich Schuchardt
@ 2022-09-16  1:30   ` Simon Glass
  2022-09-16  6:41     ` Heinrich Schuchardt
  0 siblings, 1 reply; 17+ messages in thread
From: Simon Glass @ 2022-09-16  1:30 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Ilias Apalodimas, AKASHI Takahiro, Masahisa Kojima, U-Boot Mailing List

Hi Heinrich,

On Thu, 15 Sept 2022 at 14:02, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Currently block devices are only identified by uclass_id and device number.
> When dealing with removable media this is not enough to uniquely identify
> the medium.
>
> E.g. after host unbind, host bind we can have the same device number but a
> different backing file.
>
> The EFI specification uses a 32bit number media ID to identify media. Add a
> matching field to the block device descriptor.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/block/blk-uclass.c | 16 +++++++++++++++-
>  include/blk.h              | 11 +++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)

Shouldn't this be handled by connecting the  EFI data to its udevice.
I think Takahiro has been looking at this?

NAK to any EFI fields in blk_desc, in any case

Regards,
Simon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] dm: blk: assign media ID to block devices
  2022-09-16  1:30   ` Simon Glass
@ 2022-09-16  6:41     ` Heinrich Schuchardt
  2022-09-16 20:29       ` Simon Glass
  0 siblings, 1 reply; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-16  6:41 UTC (permalink / raw)
  To: Simon Glass
  Cc: Ilias Apalodimas, AKASHI Takahiro, Masahisa Kojima, U-Boot Mailing List



On 9/16/22 03:30, Simon Glass wrote:
> Hi Heinrich,
> 
> On Thu, 15 Sept 2022 at 14:02, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Currently block devices are only identified by uclass_id and device number.
>> When dealing with removable media this is not enough to uniquely identify
>> the medium.
>>
>> E.g. after host unbind, host bind we can have the same device number but a
>> different backing file.
>>
>> The EFI specification uses a 32bit number media ID to identify media. Add a
>> matching field to the block device descriptor.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   drivers/block/blk-uclass.c | 16 +++++++++++++++-
>>   include/blk.h              | 11 +++++++++++
>>   2 files changed, 26 insertions(+), 1 deletion(-)
> 
> Shouldn't this be handled by connecting the  EFI data to its udevice.
> I think Takahiro has been looking at this?
> 
> NAK to any EFI fields in blk_desc, in any case

The information that a medium has changed can only come from the DM 
layer. If for instance the SD-card is swapped, this is indicated by the 
card detector switch. USB also has a signal for media changes.

What are your ideas how the DM layer shall indicate media changes?

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] dm: blk: assign media ID to block devices
  2022-09-16  6:41     ` Heinrich Schuchardt
@ 2022-09-16 20:29       ` Simon Glass
  0 siblings, 0 replies; 17+ messages in thread
From: Simon Glass @ 2022-09-16 20:29 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Ilias Apalodimas, AKASHI Takahiro, Masahisa Kojima, U-Boot Mailing List

Hi Heinrich,

On Fri, 16 Sept 2022 at 00:41, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 9/16/22 03:30, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Thu, 15 Sept 2022 at 14:02, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Currently block devices are only identified by uclass_id and device number.
> >> When dealing with removable media this is not enough to uniquely identify
> >> the medium.
> >>
> >> E.g. after host unbind, host bind we can have the same device number but a
> >> different backing file.
> >>
> >> The EFI specification uses a 32bit number media ID to identify media. Add a
> >> matching field to the block device descriptor.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   drivers/block/blk-uclass.c | 16 +++++++++++++++-
> >>   include/blk.h              | 11 +++++++++++
> >>   2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > Shouldn't this be handled by connecting the  EFI data to its udevice.
> > I think Takahiro has been looking at this?
> >
> > NAK to any EFI fields in blk_desc, in any case
>
> The information that a medium has changed can only come from the DM
> layer. If for instance the SD-card is swapped, this is indicated by the
> card detector switch. USB also has a signal for media changes.
>
> What are your ideas how the DM layer shall indicate media changes?

It should send an event, e.g. EVT_MEDIA and subscribers can then do
what is needed.

Regards,
Simon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] efi_loader: fill media_id from block device descriptor
  2022-09-15 20:02 ` [PATCH 2/2] efi_loader: fill media_id from block device descriptor Heinrich Schuchardt
@ 2022-09-23  7:07   ` Ilias Apalodimas
  2022-09-25 14:15     ` Simon Glass
  0 siblings, 1 reply; 17+ messages in thread
From: Ilias Apalodimas @ 2022-09-23  7:07 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Simon Glass, AKASHI Takahiro, Masahisa Kojima, u-boot

On Thu, 15 Sept 2022 at 23:02, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Fill the media ID in the block IO protocol from the block device descriptor
> of the driver model.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_loader/efi_disk.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 73745ccaa0..35790aa86d 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -476,11 +476,7 @@ static efi_status_t efi_disk_add_dev(
>         /* Fill in EFI IO Media info (for read/write callbacks) */
>         diskobj->media.removable_media = desc->removable;
>         diskobj->media.media_present = 1;
> -       /*
> -        * MediaID is just an arbitrary counter.
> -        * We have to change it if the medium is removed or changed.
> -        */
> -       diskobj->media.media_id = 1;
> +       diskobj->media.media_id = desc->media_id;
>         diskobj->media.block_size = desc->blksz;
>         diskobj->media.io_align = desc->blksz;
>         if (part)
> --
> 2.37.2
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] efi_loader: fill media_id from block device descriptor
  2022-09-23  7:07   ` Ilias Apalodimas
@ 2022-09-25 14:15     ` Simon Glass
  2022-09-26  0:05       ` AKASHI Takahiro
  0 siblings, 1 reply; 17+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
  To: Ilias Apalodimas
  Cc: Heinrich Schuchardt, AKASHI Takahiro, Masahisa Kojima,
	U-Boot Mailing List

Hi,

On Fri, 23 Sept 2022 at 01:08, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 15 Sept 2022 at 23:02, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > Fill the media ID in the block IO protocol from the block device descriptor
> > of the driver model.
> >
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> >  lib/efi_loader/efi_disk.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index 73745ccaa0..35790aa86d 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -476,11 +476,7 @@ static efi_status_t efi_disk_add_dev(
> >         /* Fill in EFI IO Media info (for read/write callbacks) */
> >         diskobj->media.removable_media = desc->removable;
> >         diskobj->media.media_present = 1;
> > -       /*
> > -        * MediaID is just an arbitrary counter.
> > -        * We have to change it if the medium is removed or changed.
> > -        */
> > -       diskobj->media.media_id = 1;
> > +       diskobj->media.media_id = desc->media_id;
> >         diskobj->media.block_size = desc->blksz;
> >         diskobj->media.io_align = desc->blksz;
> >         if (part)
> > --
> > 2.37.2
> >
>
> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Still a NAK from me as we should not add this to the desc thing.

Ilias, we talked about getting the next step going for integrating EFI
properly with driver model, which should sort this out.

Regards,
SImon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] efi_loader: fill media_id from block device descriptor
  2022-09-25 14:15     ` Simon Glass
@ 2022-09-26  0:05       ` AKASHI Takahiro
  0 siblings, 0 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2022-09-26  0:05 UTC (permalink / raw)
  To: Simon Glass
  Cc: Ilias Apalodimas, Heinrich Schuchardt, Masahisa Kojima,
	U-Boot Mailing List

On Sun, Sep 25, 2022 at 08:15:36AM -0600, Simon Glass wrote:
> Hi,
> 
> On Fri, 23 Sept 2022 at 01:08, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Thu, 15 Sept 2022 at 23:02, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > > Fill the media ID in the block IO protocol from the block device descriptor
> > > of the driver model.
> > >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > >  lib/efi_loader/efi_disk.c | 6 +-----
> > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > index 73745ccaa0..35790aa86d 100644
> > > --- a/lib/efi_loader/efi_disk.c
> > > +++ b/lib/efi_loader/efi_disk.c
> > > @@ -476,11 +476,7 @@ static efi_status_t efi_disk_add_dev(
> > >         /* Fill in EFI IO Media info (for read/write callbacks) */
> > >         diskobj->media.removable_media = desc->removable;
> > >         diskobj->media.media_present = 1;
> > > -       /*
> > > -        * MediaID is just an arbitrary counter.
> > > -        * We have to change it if the medium is removed or changed.
> > > -        */
> > > -       diskobj->media.media_id = 1;
> > > +       diskobj->media.media_id = desc->media_id;
> > >         diskobj->media.block_size = desc->blksz;
> > >         diskobj->media.io_align = desc->blksz;
> > >         if (part)
> > > --
> > > 2.37.2
> > >
> >
> > Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Still a NAK from me as we should not add this to the desc thing.

In addition, I have already raised my concern about Heinrich's approach:
https://lists.denx.de/pipermail/u-boot/2022-September/494764.html

-Takahiro Akashi


> Ilias, we talked about getting the next step going for integrating EFI
> properly with driver model, which should sort this out.
> 
> Regards,
> SImon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-16  0:58 ` [PATCH 0/2] efi_loader: provide media ID AKASHI Takahiro
@ 2022-09-26  6:06   ` Heinrich Schuchardt
  2022-09-27  1:51     ` AKASHI Takahiro
  0 siblings, 1 reply; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-26  6:06 UTC (permalink / raw)
  To: AKASHI Takahiro; +Cc: Simon Glass, Ilias Apalodimas, Masahisa Kojima, u-boot



On 9/16/22 02:58, AKASHI Takahiro wrote:
> On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
>> The medium a device like 'mmc 0' or 'usb 0' points to may change over
>> time. Hence device type and number are not sufficient to identify the
>> inserted medium. The same is true for the device path generated for
>> such a device.
> 
> Well, it depends on how a device path is generated in U-Boot's UEFI
> implementation. I believe that a device path represents an "unique path"
> to a given device however this device is enumerated.
> In this sense, the current dp_fill()/efi_dp_from_part() is not a right
> implementation as it relies on device numbers.
> Furthermore, a generated device path here is different from one generated
> by EDK2 (even if both software are run on the same board).
> 
> This is an issue that I used to tackle in
> https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
> although I have since had no progress.
> 
>> This is why the EFI_BLOCK_IO_PROTOCOL provides a field
>> MediaId.
>>
>> Whenever a removable medium is changed or a new block device with a
>> previously used device path is created we should provide a different
>> MediaID.
>>
>> This series adds a field media_id to the block device descriptor and fills
>> it after probing. The value of the field is then copied to the
>> EFI_BLOCK_IO_PROTOCOL.
> 
> I'm afraid that your patch doesn't always work as you expect.
> When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
> all the existing devices and associated blk_desc structures are once freed
> and even if nothing is changed, i.e. a device is neither removed nor added,
> the exact same structures will be re-created.
> With your patch applied, however, a new (and different) "media_id" will be
> assigned to an existing device. UEFI User may be notified of "media change".
> (To be honest, this is quite unlikely because the current UEFI implementation
> doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)

This behavior matches what EDK II does if you remove a device and create 
a new device.

If a device is removed and recreated anything could have happened in 
between like complete repartitioning. We cannot assume that any cached 
state is valid anymore even if GUIDs are the same.

So it is correct to change the media ID in this case.

Commands like scsi rescan are needed because we don't monitor media 
changes in the DM drivers yet. Simon's suggestion to use provide an 
event for media changes looks like the right approach to me.

Best regards

Heinrich

> 
> -Takahiro Akashi
> 
>> With future patches we can refine this in sub-systems like USB, MMC, SCSI
>> to indicate media changes
>>
>> Heinrich Schuchardt (2):
>>    dm: blk: assign media ID to block devices
>>    efi_loader: fill media_id from block device descriptor
>>
>>   drivers/block/blk-uclass.c | 16 +++++++++++++++-
>>   include/blk.h              | 11 +++++++++++
>>   lib/efi_loader/efi_disk.c  |  6 +-----
>>   3 files changed, 27 insertions(+), 6 deletions(-)
>>
>> -- 
>> 2.37.2
>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-26  6:06   ` Heinrich Schuchardt
@ 2022-09-27  1:51     ` AKASHI Takahiro
  2022-09-27  6:53       ` Heinrich Schuchardt
  0 siblings, 1 reply; 17+ messages in thread
From: AKASHI Takahiro @ 2022-09-27  1:51 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Simon Glass, Ilias Apalodimas, Masahisa Kojima, u-boot

On Mon, Sep 26, 2022 at 08:06:52AM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 9/16/22 02:58, AKASHI Takahiro wrote:
> > On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
> > > The medium a device like 'mmc 0' or 'usb 0' points to may change over
> > > time. Hence device type and number are not sufficient to identify the
> > > inserted medium. The same is true for the device path generated for
> > > such a device.
> > 
> > Well, it depends on how a device path is generated in U-Boot's UEFI
> > implementation. I believe that a device path represents an "unique path"
> > to a given device however this device is enumerated.
> > In this sense, the current dp_fill()/efi_dp_from_part() is not a right
> > implementation as it relies on device numbers.
> > Furthermore, a generated device path here is different from one generated
> > by EDK2 (even if both software are run on the same board).
> > 
> > This is an issue that I used to tackle in
> > https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
> > although I have since had no progress.
> > 
> > > This is why the EFI_BLOCK_IO_PROTOCOL provides a field
> > > MediaId.
> > > 
> > > Whenever a removable medium is changed or a new block device with a
> > > previously used device path is created we should provide a different
> > > MediaID.
> > > 
> > > This series adds a field media_id to the block device descriptor and fills
> > > it after probing. The value of the field is then copied to the
> > > EFI_BLOCK_IO_PROTOCOL.
> > 
> > I'm afraid that your patch doesn't always work as you expect.
> > When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
> > all the existing devices and associated blk_desc structures are once freed
> > and even if nothing is changed, i.e. a device is neither removed nor added,
> > the exact same structures will be re-created.
> > With your patch applied, however, a new (and different) "media_id" will be
> > assigned to an existing device. UEFI User may be notified of "media change".
> > (To be honest, this is quite unlikely because the current UEFI implementation
> > doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)
> 
> This behavior matches what EDK II does if you remove a device and create a
> new device.

I don't think that EDK2 has "scsi rescan" or others, which users can invoke
at any time. Moreover, I believe that EDK2 code (drivers) checks whether a device
is really changed or not before updating a MediaId.

> If a device is removed and recreated anything could have happened in between
> like complete repartitioning. We cannot assume that any cached state is
> valid anymore even if GUIDs are the same.

I'm not sure if you fully understand my point.
My assumption is the case where a device is NOT removed around "scsi rescan"
(or usb stop/start) and stays online. In this case,
1. access to, say, "scsi 0:1", via UEFI BLOCK_IO succeeds
2. "scsi rescan"
3. access to the same device, "scsi 0:1", via UEFI BLOCK_IO 
currently (3) succeeds, but with your patch, it may potentially fail because
of media_id altered.

I admit that it will not happen under the current UEFI implementation because
non of UEFI applications will survive across command lines and none of information,
including media_id or handle, can be carried over from (1) to (3).
But unconditionally incrementing an internally-held media_id, as in your patch,
is a wrong behavior.

-Takahiro Akashi

> 
> So it is correct to change the media ID in this case.

> Commands like scsi rescan are needed because we don't monitor media changes
> in the DM drivers yet. Simon's suggestion to use provide an event for media
> changes looks like the right approach to me.
> 
> Best regards
> 
> Heinrich
> 
> > 
> > -Takahiro Akashi
> > 
> > > With future patches we can refine this in sub-systems like USB, MMC, SCSI
> > > to indicate media changes
> > > 
> > > Heinrich Schuchardt (2):
> > >    dm: blk: assign media ID to block devices
> > >    efi_loader: fill media_id from block device descriptor
> > > 
> > >   drivers/block/blk-uclass.c | 16 +++++++++++++++-
> > >   include/blk.h              | 11 +++++++++++
> > >   lib/efi_loader/efi_disk.c  |  6 +-----
> > >   3 files changed, 27 insertions(+), 6 deletions(-)
> > > 
> > > -- 
> > > 2.37.2
> > > 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-27  1:51     ` AKASHI Takahiro
@ 2022-09-27  6:53       ` Heinrich Schuchardt
  2022-09-28  1:54         ` Simon Glass
  0 siblings, 1 reply; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-27  6:53 UTC (permalink / raw)
  To: AKASHI Takahiro; +Cc: Simon Glass, Ilias Apalodimas, Masahisa Kojima, u-boot



On 9/27/22 03:51, AKASHI Takahiro wrote:
> On Mon, Sep 26, 2022 at 08:06:52AM +0200, Heinrich Schuchardt wrote:
>>
>>
>> On 9/16/22 02:58, AKASHI Takahiro wrote:
>>> On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
>>>> The medium a device like 'mmc 0' or 'usb 0' points to may change over
>>>> time. Hence device type and number are not sufficient to identify the
>>>> inserted medium. The same is true for the device path generated for
>>>> such a device.
>>>
>>> Well, it depends on how a device path is generated in U-Boot's UEFI
>>> implementation. I believe that a device path represents an "unique path"
>>> to a given device however this device is enumerated.
>>> In this sense, the current dp_fill()/efi_dp_from_part() is not a right
>>> implementation as it relies on device numbers.
>>> Furthermore, a generated device path here is different from one generated
>>> by EDK2 (even if both software are run on the same board).
>>>
>>> This is an issue that I used to tackle in
>>> https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
>>> although I have since had no progress.
>>>
>>>> This is why the EFI_BLOCK_IO_PROTOCOL provides a field
>>>> MediaId.
>>>>
>>>> Whenever a removable medium is changed or a new block device with a
>>>> previously used device path is created we should provide a different
>>>> MediaID.
>>>>
>>>> This series adds a field media_id to the block device descriptor and fills
>>>> it after probing. The value of the field is then copied to the
>>>> EFI_BLOCK_IO_PROTOCOL.
>>>
>>> I'm afraid that your patch doesn't always work as you expect.
>>> When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
>>> all the existing devices and associated blk_desc structures are once freed
>>> and even if nothing is changed, i.e. a device is neither removed nor added,
>>> the exact same structures will be re-created.
>>> With your patch applied, however, a new (and different) "media_id" will be
>>> assigned to an existing device. UEFI User may be notified of "media change".
>>> (To be honest, this is quite unlikely because the current UEFI implementation
>>> doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)
>>
>> This behavior matches what EDK II does if you remove a device and create a
>> new device.
> 
> I don't think that EDK2 has "scsi rescan" or others, which users can invoke
> at any time. Moreover, I believe that EDK2 code (drivers) checks whether a device
> is really changed or not before updating a MediaId.
> 
>> If a device is removed and recreated anything could have happened in between
>> like complete repartitioning. We cannot assume that any cached state is
>> valid anymore even if GUIDs are the same.
> 
> I'm not sure if you fully understand my point.
> My assumption is the case where a device is NOT removed around "scsi rescan"
> (or usb stop/start) and stays online. In this case,
> 1. access to, say, "scsi 0:1", via UEFI BLOCK_IO succeeds
> 2. "scsi rescan"
> 3. access to the same device, "scsi 0:1", via UEFI BLOCK_IO
> currently (3) succeeds, but with your patch, it may potentially fail because
> of media_id altered.
> 
> I admit that it will not happen under the current UEFI implementation because
> non of UEFI applications will survive across command lines and none of information,
> including media_id or handle, can be carried over from (1) to (3).
> But unconditionally incrementing an internally-held media_id, as in your patch,
> is a wrong behavior.

The patch issues a new media ID if a new device is probed which only 
happens to have the same device number if another device of that number 
was removed before.

Commands like 'usb scan' don't necessarily issue the same numbers to the 
same device as before the command if a new device has been attached in 
the meanwhile.

Assuming that a new device contains the same medium as an old one 
because by chance it has the same device number is definitively unsafe.

If a device is probed, we have to assume that it contains a new medium.

Best regards

Heinrich

> 
> -Takahiro Akashi
> 
>>
>> So it is correct to change the media ID in this case.
> 
>> Commands like scsi rescan are needed because we don't monitor media changes
>> in the DM drivers yet. Simon's suggestion to use provide an event for media
>> changes looks like the right approach to me.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> With future patches we can refine this in sub-systems like USB, MMC, SCSI
>>>> to indicate media changes
>>>>
>>>> Heinrich Schuchardt (2):
>>>>     dm: blk: assign media ID to block devices
>>>>     efi_loader: fill media_id from block device descriptor
>>>>
>>>>    drivers/block/blk-uclass.c | 16 +++++++++++++++-
>>>>    include/blk.h              | 11 +++++++++++
>>>>    lib/efi_loader/efi_disk.c  |  6 +-----
>>>>    3 files changed, 27 insertions(+), 6 deletions(-)
>>>>
>>>> -- 
>>>> 2.37.2
>>>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-27  6:53       ` Heinrich Schuchardt
@ 2022-09-28  1:54         ` Simon Glass
  2022-09-28  6:57           ` Heinrich Schuchardt
  0 siblings, 1 reply; 17+ messages in thread
From: Simon Glass @ 2022-09-28  1:54 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: AKASHI Takahiro, Ilias Apalodimas, Masahisa Kojima, U-Boot Mailing List

Hi,

On Tue, 27 Sept 2022 at 00:53, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 9/27/22 03:51, AKASHI Takahiro wrote:
> > On Mon, Sep 26, 2022 at 08:06:52AM +0200, Heinrich Schuchardt wrote:
> >>
> >>
> >> On 9/16/22 02:58, AKASHI Takahiro wrote:
> >>> On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
> >>>> The medium a device like 'mmc 0' or 'usb 0' points to may change over
> >>>> time. Hence device type and number are not sufficient to identify the
> >>>> inserted medium. The same is true for the device path generated for
> >>>> such a device.
> >>>
> >>> Well, it depends on how a device path is generated in U-Boot's UEFI
> >>> implementation. I believe that a device path represents an "unique path"
> >>> to a given device however this device is enumerated.
> >>> In this sense, the current dp_fill()/efi_dp_from_part() is not a right
> >>> implementation as it relies on device numbers.
> >>> Furthermore, a generated device path here is different from one generated
> >>> by EDK2 (even if both software are run on the same board).
> >>>
> >>> This is an issue that I used to tackle in
> >>> https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
> >>> although I have since had no progress.
> >>>
> >>>> This is why the EFI_BLOCK_IO_PROTOCOL provides a field
> >>>> MediaId.
> >>>>
> >>>> Whenever a removable medium is changed or a new block device with a
> >>>> previously used device path is created we should provide a different
> >>>> MediaID.
> >>>>
> >>>> This series adds a field media_id to the block device descriptor and fills
> >>>> it after probing. The value of the field is then copied to the
> >>>> EFI_BLOCK_IO_PROTOCOL.
> >>>
> >>> I'm afraid that your patch doesn't always work as you expect.
> >>> When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
> >>> all the existing devices and associated blk_desc structures are once freed
> >>> and even if nothing is changed, i.e. a device is neither removed nor added,
> >>> the exact same structures will be re-created.
> >>> With your patch applied, however, a new (and different) "media_id" will be
> >>> assigned to an existing device. UEFI User may be notified of "media change".
> >>> (To be honest, this is quite unlikely because the current UEFI implementation
> >>> doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)
> >>
> >> This behavior matches what EDK II does if you remove a device and create a
> >> new device.
> >
> > I don't think that EDK2 has "scsi rescan" or others, which users can invoke
> > at any time. Moreover, I believe that EDK2 code (drivers) checks whether a device
> > is really changed or not before updating a MediaId.
> >
> >> If a device is removed and recreated anything could have happened in between
> >> like complete repartitioning. We cannot assume that any cached state is
> >> valid anymore even if GUIDs are the same.
> >
> > I'm not sure if you fully understand my point.
> > My assumption is the case where a device is NOT removed around "scsi rescan"
> > (or usb stop/start) and stays online. In this case,
> > 1. access to, say, "scsi 0:1", via UEFI BLOCK_IO succeeds
> > 2. "scsi rescan"
> > 3. access to the same device, "scsi 0:1", via UEFI BLOCK_IO
> > currently (3) succeeds, but with your patch, it may potentially fail because
> > of media_id altered.
> >
> > I admit that it will not happen under the current UEFI implementation because
> > non of UEFI applications will survive across command lines and none of information,
> > including media_id or handle, can be carried over from (1) to (3).
> > But unconditionally incrementing an internally-held media_id, as in your patch,
> > is a wrong behavior.
>
> The patch issues a new media ID if a new device is probed which only
> happens to have the same device number if another device of that number
> was removed before.
>
> Commands like 'usb scan' don't necessarily issue the same numbers to the
> same device as before the command if a new device has been attached in
> the meanwhile.
>
> Assuming that a new device contains the same medium as an old one
> because by chance it has the same device number is definitively unsafe.
>
> If a device is probed, we have to assume that it contains a new medium.

Sorry if I repeat myself, but this sort of thing should be handled in
the driver model code. Can we get some more progress on integrating
the EFI layer better?

Regards,
Simon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-28  1:54         ` Simon Glass
@ 2022-09-28  6:57           ` Heinrich Schuchardt
  2022-09-28  7:24             ` AKASHI Takahiro
  0 siblings, 1 reply; 17+ messages in thread
From: Heinrich Schuchardt @ 2022-09-28  6:57 UTC (permalink / raw)
  To: Simon Glass
  Cc: AKASHI Takahiro, Ilias Apalodimas, Masahisa Kojima, U-Boot Mailing List



On 9/28/22 03:54, Simon Glass wrote:
> Hi,
> 
> On Tue, 27 Sept 2022 at 00:53, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 9/27/22 03:51, AKASHI Takahiro wrote:
>>> On Mon, Sep 26, 2022 at 08:06:52AM +0200, Heinrich Schuchardt wrote:
>>>>
>>>>
>>>> On 9/16/22 02:58, AKASHI Takahiro wrote:
>>>>> On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
>>>>>> The medium a device like 'mmc 0' or 'usb 0' points to may change over
>>>>>> time. Hence device type and number are not sufficient to identify the
>>>>>> inserted medium. The same is true for the device path generated for
>>>>>> such a device.
>>>>>
>>>>> Well, it depends on how a device path is generated in U-Boot's UEFI
>>>>> implementation. I believe that a device path represents an "unique path"
>>>>> to a given device however this device is enumerated.
>>>>> In this sense, the current dp_fill()/efi_dp_from_part() is not a right
>>>>> implementation as it relies on device numbers.
>>>>> Furthermore, a generated device path here is different from one generated
>>>>> by EDK2 (even if both software are run on the same board).
>>>>>
>>>>> This is an issue that I used to tackle in
>>>>> https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
>>>>> although I have since had no progress.
>>>>>
>>>>>> This is why the EFI_BLOCK_IO_PROTOCOL provides a field
>>>>>> MediaId.
>>>>>>
>>>>>> Whenever a removable medium is changed or a new block device with a
>>>>>> previously used device path is created we should provide a different
>>>>>> MediaID.
>>>>>>
>>>>>> This series adds a field media_id to the block device descriptor and fills
>>>>>> it after probing. The value of the field is then copied to the
>>>>>> EFI_BLOCK_IO_PROTOCOL.
>>>>>
>>>>> I'm afraid that your patch doesn't always work as you expect.
>>>>> When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
>>>>> all the existing devices and associated blk_desc structures are once freed
>>>>> and even if nothing is changed, i.e. a device is neither removed nor added,
>>>>> the exact same structures will be re-created.
>>>>> With your patch applied, however, a new (and different) "media_id" will be
>>>>> assigned to an existing device. UEFI User may be notified of "media change".
>>>>> (To be honest, this is quite unlikely because the current UEFI implementation
>>>>> doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)
>>>>
>>>> This behavior matches what EDK II does if you remove a device and create a
>>>> new device.
>>>
>>> I don't think that EDK2 has "scsi rescan" or others, which users can invoke
>>> at any time. Moreover, I believe that EDK2 code (drivers) checks whether a device
>>> is really changed or not before updating a MediaId.
>>>
>>>> If a device is removed and recreated anything could have happened in between
>>>> like complete repartitioning. We cannot assume that any cached state is
>>>> valid anymore even if GUIDs are the same.
>>>
>>> I'm not sure if you fully understand my point.
>>> My assumption is the case where a device is NOT removed around "scsi rescan"
>>> (or usb stop/start) and stays online. In this case,
>>> 1. access to, say, "scsi 0:1", via UEFI BLOCK_IO succeeds
>>> 2. "scsi rescan"
>>> 3. access to the same device, "scsi 0:1", via UEFI BLOCK_IO
>>> currently (3) succeeds, but with your patch, it may potentially fail because
>>> of media_id altered.
>>>
>>> I admit that it will not happen under the current UEFI implementation because
>>> non of UEFI applications will survive across command lines and none of information,
>>> including media_id or handle, can be carried over from (1) to (3).
>>> But unconditionally incrementing an internally-held media_id, as in your patch,
>>> is a wrong behavior.
>>
>> The patch issues a new media ID if a new device is probed which only
>> happens to have the same device number if another device of that number
>> was removed before.
>>
>> Commands like 'usb scan' don't necessarily issue the same numbers to the
>> same device as before the command if a new device has been attached in
>> the meanwhile.
>>
>> Assuming that a new device contains the same medium as an old one
>> because by chance it has the same device number is definitively unsafe.
>>
>> If a device is probed, we have to assume that it contains a new medium.
> 
> Sorry if I repeat myself, but this sort of thing should be handled in
> the driver model code. Can we get some more progress on integrating
> the EFI layer better?

The last mails where about *whether* the media ID should be bumped after 
a block device has been created and not about where we will implement it.

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-28  6:57           ` Heinrich Schuchardt
@ 2022-09-28  7:24             ` AKASHI Takahiro
  2022-09-28 16:27               ` Simon Glass
  0 siblings, 1 reply; 17+ messages in thread
From: AKASHI Takahiro @ 2022-09-28  7:24 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Simon Glass, Ilias Apalodimas, Masahisa Kojima, U-Boot Mailing List

On Wed, Sep 28, 2022 at 08:57:43AM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 9/28/22 03:54, Simon Glass wrote:
> > Hi,
> > 
> > On Tue, 27 Sept 2022 at 00:53, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > > 
> > > 
> > > 
> > > On 9/27/22 03:51, AKASHI Takahiro wrote:
> > > > On Mon, Sep 26, 2022 at 08:06:52AM +0200, Heinrich Schuchardt wrote:
> > > > > 
> > > > > 
> > > > > On 9/16/22 02:58, AKASHI Takahiro wrote:
> > > > > > On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
> > > > > > > The medium a device like 'mmc 0' or 'usb 0' points to may change over
> > > > > > > time. Hence device type and number are not sufficient to identify the
> > > > > > > inserted medium. The same is true for the device path generated for
> > > > > > > such a device.
> > > > > > 
> > > > > > Well, it depends on how a device path is generated in U-Boot's UEFI
> > > > > > implementation. I believe that a device path represents an "unique path"
> > > > > > to a given device however this device is enumerated.
> > > > > > In this sense, the current dp_fill()/efi_dp_from_part() is not a right
> > > > > > implementation as it relies on device numbers.
> > > > > > Furthermore, a generated device path here is different from one generated
> > > > > > by EDK2 (even if both software are run on the same board).
> > > > > > 
> > > > > > This is an issue that I used to tackle in
> > > > > > https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
> > > > > > although I have since had no progress.
> > > > > > 
> > > > > > > This is why the EFI_BLOCK_IO_PROTOCOL provides a field
> > > > > > > MediaId.
> > > > > > > 
> > > > > > > Whenever a removable medium is changed or a new block device with a
> > > > > > > previously used device path is created we should provide a different
> > > > > > > MediaID.
> > > > > > > 
> > > > > > > This series adds a field media_id to the block device descriptor and fills
> > > > > > > it after probing. The value of the field is then copied to the
> > > > > > > EFI_BLOCK_IO_PROTOCOL.
> > > > > > 
> > > > > > I'm afraid that your patch doesn't always work as you expect.
> > > > > > When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
> > > > > > all the existing devices and associated blk_desc structures are once freed
> > > > > > and even if nothing is changed, i.e. a device is neither removed nor added,
> > > > > > the exact same structures will be re-created.
> > > > > > With your patch applied, however, a new (and different) "media_id" will be
> > > > > > assigned to an existing device. UEFI User may be notified of "media change".
> > > > > > (To be honest, this is quite unlikely because the current UEFI implementation
> > > > > > doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)
> > > > > 
> > > > > This behavior matches what EDK II does if you remove a device and create a
> > > > > new device.
> > > > 
> > > > I don't think that EDK2 has "scsi rescan" or others, which users can invoke
> > > > at any time. Moreover, I believe that EDK2 code (drivers) checks whether a device
> > > > is really changed or not before updating a MediaId.
> > > > 
> > > > > If a device is removed and recreated anything could have happened in between
> > > > > like complete repartitioning. We cannot assume that any cached state is
> > > > > valid anymore even if GUIDs are the same.
> > > > 
> > > > I'm not sure if you fully understand my point.
> > > > My assumption is the case where a device is NOT removed around "scsi rescan"
> > > > (or usb stop/start) and stays online. In this case,
> > > > 1. access to, say, "scsi 0:1", via UEFI BLOCK_IO succeeds
> > > > 2. "scsi rescan"
> > > > 3. access to the same device, "scsi 0:1", via UEFI BLOCK_IO
> > > > currently (3) succeeds, but with your patch, it may potentially fail because
> > > > of media_id altered.
> > > > 
> > > > I admit that it will not happen under the current UEFI implementation because
> > > > non of UEFI applications will survive across command lines and none of information,
> > > > including media_id or handle, can be carried over from (1) to (3).
> > > > But unconditionally incrementing an internally-held media_id, as in your patch,
> > > > is a wrong behavior.
> > > 
> > > The patch issues a new media ID if a new device is probed which only
> > > happens to have the same device number if another device of that number
> > > was removed before.
> > > 
> > > Commands like 'usb scan' don't necessarily issue the same numbers to the
> > > same device as before the command if a new device has been attached in
> > > the meanwhile.
> > > 
> > > Assuming that a new device contains the same medium as an old one
> > > because by chance it has the same device number is definitively unsafe.
> > > 
> > > If a device is probed, we have to assume that it contains a new medium.
> > 
> > Sorry if I repeat myself, but this sort of thing should be handled in
> > the driver model code. Can we get some more progress on integrating
> > the EFI layer better?
> 
> The last mails where about *whether* the media ID should be bumped after a
> block device has been created and not about where we will implement it.

Indeed. I don't care "where" for now, but "how" or "whether".

The most essential issue is that none of U-Boot block device drivers has
ability of detecting media insertion or removal immediately
(due to the lack of interrupt support).
This is even not related to DM or not.

-Takahiro Akashi

> Best regards
> 
> Heinrich

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] efi_loader: provide media ID
  2022-09-28  7:24             ` AKASHI Takahiro
@ 2022-09-28 16:27               ` Simon Glass
  0 siblings, 0 replies; 17+ messages in thread
From: Simon Glass @ 2022-09-28 16:27 UTC (permalink / raw)
  To: AKASHI Takahiro, Heinrich Schuchardt, Simon Glass,
	Ilias Apalodimas, Masahisa Kojima, U-Boot Mailing List

Hi,

On Wed, 28 Sept 2022 at 01:24, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Wed, Sep 28, 2022 at 08:57:43AM +0200, Heinrich Schuchardt wrote:
> >
> >
> > On 9/28/22 03:54, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 27 Sept 2022 at 00:53, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > > >
> > > >
> > > >
> > > > On 9/27/22 03:51, AKASHI Takahiro wrote:
> > > > > On Mon, Sep 26, 2022 at 08:06:52AM +0200, Heinrich Schuchardt wrote:
> > > > > >
> > > > > >
> > > > > > On 9/16/22 02:58, AKASHI Takahiro wrote:
> > > > > > > On Thu, Sep 15, 2022 at 10:02:40PM +0200, Heinrich Schuchardt wrote:
> > > > > > > > The medium a device like 'mmc 0' or 'usb 0' points to may change over
> > > > > > > > time. Hence device type and number are not sufficient to identify the
> > > > > > > > inserted medium. The same is true for the device path generated for
> > > > > > > > such a device.
> > > > > > >
> > > > > > > Well, it depends on how a device path is generated in U-Boot's UEFI
> > > > > > > implementation. I believe that a device path represents an "unique path"
> > > > > > > to a given device however this device is enumerated.
> > > > > > > In this sense, the current dp_fill()/efi_dp_from_part() is not a right
> > > > > > > implementation as it relies on device numbers.
> > > > > > > Furthermore, a generated device path here is different from one generated
> > > > > > > by EDK2 (even if both software are run on the same board).
> > > > > > >
> > > > > > > This is an issue that I used to tackle in
> > > > > > > https://lists.denx.de/pipermail/u-boot/2021-November/468216.html
> > > > > > > although I have since had no progress.
> > > > > > >
> > > > > > > > This is why the EFI_BLOCK_IO_PROTOCOL provides a field
> > > > > > > > MediaId.
> > > > > > > >
> > > > > > > > Whenever a removable medium is changed or a new block device with a
> > > > > > > > previously used device path is created we should provide a different
> > > > > > > > MediaID.
> > > > > > > >
> > > > > > > > This series adds a field media_id to the block device descriptor and fills
> > > > > > > > it after probing. The value of the field is then copied to the
> > > > > > > > EFI_BLOCK_IO_PROTOCOL.
> > > > > > >
> > > > > > > I'm afraid that your patch doesn't always work as you expect.
> > > > > > > When "scsi rescan" or "usb stop; usb start", for instance, is invoked,
> > > > > > > all the existing devices and associated blk_desc structures are once freed
> > > > > > > and even if nothing is changed, i.e. a device is neither removed nor added,
> > > > > > > the exact same structures will be re-created.
> > > > > > > With your patch applied, however, a new (and different) "media_id" will be
> > > > > > > assigned to an existing device. UEFI User may be notified of "media change".
> > > > > > > (To be honest, this is quite unlikely because the current UEFI implementation
> > > > > > > doesn't use BLOCK_IO_PROTOCOL internally, say, for file system access.)
> > > > > >
> > > > > > This behavior matches what EDK II does if you remove a device and create a
> > > > > > new device.
> > > > >
> > > > > I don't think that EDK2 has "scsi rescan" or others, which users can invoke
> > > > > at any time. Moreover, I believe that EDK2 code (drivers) checks whether a device
> > > > > is really changed or not before updating a MediaId.
> > > > >
> > > > > > If a device is removed and recreated anything could have happened in between
> > > > > > like complete repartitioning. We cannot assume that any cached state is
> > > > > > valid anymore even if GUIDs are the same.
> > > > >
> > > > > I'm not sure if you fully understand my point.
> > > > > My assumption is the case where a device is NOT removed around "scsi rescan"
> > > > > (or usb stop/start) and stays online. In this case,
> > > > > 1. access to, say, "scsi 0:1", via UEFI BLOCK_IO succeeds
> > > > > 2. "scsi rescan"
> > > > > 3. access to the same device, "scsi 0:1", via UEFI BLOCK_IO
> > > > > currently (3) succeeds, but with your patch, it may potentially fail because
> > > > > of media_id altered.
> > > > >
> > > > > I admit that it will not happen under the current UEFI implementation because
> > > > > non of UEFI applications will survive across command lines and none of information,
> > > > > including media_id or handle, can be carried over from (1) to (3).
> > > > > But unconditionally incrementing an internally-held media_id, as in your patch,
> > > > > is a wrong behavior.
> > > >
> > > > The patch issues a new media ID if a new device is probed which only
> > > > happens to have the same device number if another device of that number
> > > > was removed before.
> > > >
> > > > Commands like 'usb scan' don't necessarily issue the same numbers to the
> > > > same device as before the command if a new device has been attached in
> > > > the meanwhile.
> > > >
> > > > Assuming that a new device contains the same medium as an old one
> > > > because by chance it has the same device number is definitively unsafe.
> > > >
> > > > If a device is probed, we have to assume that it contains a new medium.
> > >
> > > Sorry if I repeat myself, but this sort of thing should be handled in
> > > the driver model code. Can we get some more progress on integrating
> > > the EFI layer better?
> >
> > The last mails where about *whether* the media ID should be bumped after a
> > block device has been created and not about where we will implement it.
>
> Indeed. I don't care "where" for now, but "how" or "whether".
>
> The most essential issue is that none of U-Boot block device drivers has
> ability of detecting media insertion or removal immediately
> (due to the lack of interrupt support).
> This is even not related to DM or not.

This could be implemented using the cyclic feature now present, or
perhaps using an IDLE event I am planning to introduce for VBE.

But another way is to have a command to indicate that the device has
been removed.

Regards,
Simon

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-09-28 16:27 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-15 20:02 [PATCH 0/2] efi_loader: provide media ID Heinrich Schuchardt
2022-09-15 20:02 ` [PATCH 1/2] dm: blk: assign media ID to block devices Heinrich Schuchardt
2022-09-16  1:30   ` Simon Glass
2022-09-16  6:41     ` Heinrich Schuchardt
2022-09-16 20:29       ` Simon Glass
2022-09-15 20:02 ` [PATCH 2/2] efi_loader: fill media_id from block device descriptor Heinrich Schuchardt
2022-09-23  7:07   ` Ilias Apalodimas
2022-09-25 14:15     ` Simon Glass
2022-09-26  0:05       ` AKASHI Takahiro
2022-09-16  0:58 ` [PATCH 0/2] efi_loader: provide media ID AKASHI Takahiro
2022-09-26  6:06   ` Heinrich Schuchardt
2022-09-27  1:51     ` AKASHI Takahiro
2022-09-27  6:53       ` Heinrich Schuchardt
2022-09-28  1:54         ` Simon Glass
2022-09-28  6:57           ` Heinrich Schuchardt
2022-09-28  7:24             ` AKASHI Takahiro
2022-09-28 16:27               ` Simon Glass

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).