| Message ID | 20220915200242.18358-1-heinrich.schuchardt@canonical.com |
|---|---|
| Headers | show |
| Series | efi_loader: provide media ID | expand |
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 >
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 >>
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 > > >
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 >>>>
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
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
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
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