| Message ID | 20230703060845.311114-1-masahisa.kojima@linaro.org |
|---|---|
| State | Accepted |
| Commit | 06fc19ca4de943827f5aa026f7aa9c3a05411677 |
| Headers | show |
| Series | [v2] efi_driver: fix duplicate efiblk#0 issue | expand |
On 7/3/23 08:08, Masahisa Kojima wrote: > The devnum value of the blk_desc structure starts from 0, > current efi_bl_create_block_device() function creates > two "efiblk#0" devices for the cases that blk_find_max_devnum() > returns -ENODEV and blk_find_max_devnum() returns 0(one device > found in this case). > > This commit uses blk_next_free_devnum() instead of blk_find_max_devnum(). > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > Changes in v2: > - uses blk_next_free_devnum() instead of blk_find_max_devnum() > > lib/efi_driver/efi_block_device.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > index add00eeebb..e3abd90275 100644 > --- a/lib/efi_driver/efi_block_device.c > +++ b/lib/efi_driver/efi_block_device.c > @@ -124,10 +124,8 @@ efi_bl_create_block_device(efi_handle_t handle, void *interface) > struct efi_block_io *io = interface; > struct efi_blk_plat *plat; > > - devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); > - if (devnum == -ENODEV) > - devnum = 0; > - else if (devnum < 0) > + devnum = blk_next_free_devnum(UCLASS_EFI_LOADER); > + if (devnum < 0) > return EFI_OUT_OF_RESOURCES; Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */
Hi Masahisa, On Mon, 3 Jul 2023 at 07:09, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > The devnum value of the blk_desc structure starts from 0, > current efi_bl_create_block_device() function creates > two "efiblk#0" devices for the cases that blk_find_max_devnum() > returns -ENODEV and blk_find_max_devnum() returns 0(one device > found in this case). > > This commit uses blk_next_free_devnum() instead of blk_find_max_devnum(). > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > Changes in v2: > - uses blk_next_free_devnum() instead of blk_find_max_devnum() > > lib/efi_driver/efi_block_device.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > index add00eeebb..e3abd90275 100644 > --- a/lib/efi_driver/efi_block_device.c > +++ b/lib/efi_driver/efi_block_device.c > @@ -124,10 +124,8 @@ efi_bl_create_block_device(efi_handle_t handle, void *interface) > struct efi_block_io *io = interface; > struct efi_blk_plat *plat; > > - devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); > - if (devnum == -ENODEV) > - devnum = 0; > - else if (devnum < 0) > + devnum = blk_next_free_devnum(UCLASS_EFI_LOADER); This really should be an internal function but I see it was exported as part of the virtio work. How come the EFI and DM block devices are getting out of sync? Anyway this function is munging around in the internals of the device and should be fixed before it causes more problems. For now, I suggest following what most other drivers so which is to call blk_create_devicef() passing a devnum of -1. > + if (devnum < 0) > return EFI_OUT_OF_RESOURCES; > > name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */ > -- > 2.34.1 > Regards, Simon
On 03.07.23 15:30, Simon Glass wrote: > Hi Masahisa, > > On Mon, 3 Jul 2023 at 07:09, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: >> >> The devnum value of the blk_desc structure starts from 0, >> current efi_bl_create_block_device() function creates >> two "efiblk#0" devices for the cases that blk_find_max_devnum() >> returns -ENODEV and blk_find_max_devnum() returns 0(one device >> found in this case). >> >> This commit uses blk_next_free_devnum() instead of blk_find_max_devnum(). >> Fixes: 05ef48a2484b ("efi_driver: EFI block driver") >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >> --- >> Changes in v2: >> - uses blk_next_free_devnum() instead of blk_find_max_devnum() >> >> lib/efi_driver/efi_block_device.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c >> index add00eeebb..e3abd90275 100644 >> --- a/lib/efi_driver/efi_block_device.c >> +++ b/lib/efi_driver/efi_block_device.c >> @@ -124,10 +124,8 @@ efi_bl_create_block_device(efi_handle_t handle, void *interface) >> struct efi_block_io *io = interface; >> struct efi_blk_plat *plat; >> >> - devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); Simon, this line was last changed by your patch e33a5c6be55e ("blk: Switch over to using uclass IDs") >> - if (devnum == -ENODEV) >> - devnum = 0; >> - else if (devnum < 0) >> + devnum = blk_next_free_devnum(UCLASS_EFI_LOADER); > > This really should be an internal function but I see it was exported > as part of the virtio work. > > How come the EFI and DM block devices are getting out of sync? They never were in sync: The bug dates back to Jan 2018: 05ef48a2484b ("efi_driver: EFI block driver") Best regards Heinrich > > Anyway this function is munging around in the internals of the device > and should be fixed before it causes more problems. > > For now, I suggest following what most other drivers so which is to > call blk_create_devicef() passing a devnum of -1. > >> + if (devnum < 0) >> return EFI_OUT_OF_RESOURCES; >> >> name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */ >> -- >> 2.34.1 >> > > Regards, > Simon
Hi Heinrich, On Tue, 4 Jul 2023 at 00:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 03.07.23 15:30, Simon Glass wrote: > > Hi Masahisa, > > > > On Mon, 3 Jul 2023 at 07:09, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > >> > >> The devnum value of the blk_desc structure starts from 0, > >> current efi_bl_create_block_device() function creates > >> two "efiblk#0" devices for the cases that blk_find_max_devnum() > >> returns -ENODEV and blk_find_max_devnum() returns 0(one device > >> found in this case). > >> > >> This commit uses blk_next_free_devnum() instead of blk_find_max_devnum(). > >> > > Fixes: 05ef48a2484b ("efi_driver: EFI block driver") > > >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >> --- > >> Changes in v2: > >> - uses blk_next_free_devnum() instead of blk_find_max_devnum() > >> > >> lib/efi_driver/efi_block_device.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > >> index add00eeebb..e3abd90275 100644 > >> --- a/lib/efi_driver/efi_block_device.c > >> +++ b/lib/efi_driver/efi_block_device.c > >> @@ -124,10 +124,8 @@ efi_bl_create_block_device(efi_handle_t handle, void *interface) > >> struct efi_block_io *io = interface; > >> struct efi_blk_plat *plat; > >> > >> - devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); > > Simon, this line was last changed by your patch > e33a5c6be55e ("blk: Switch over to using uclass IDs") > > >> - if (devnum == -ENODEV) > >> - devnum = 0; > >> - else if (devnum < 0) > >> + devnum = blk_next_free_devnum(UCLASS_EFI_LOADER); > > > > This really should be an internal function but I see it was exported > > as part of the virtio work. > > > > How come the EFI and DM block devices are getting out of sync? > > They never were in sync: > > The bug dates back to Jan 2018: > 05ef48a2484b ("efi_driver: EFI block driver") OK I see. Well it looks like the original code was a bit off, as per my comment. It should be fixed. > > Best regards > > Heinrich > > > > > Anyway this function is munging around in the internals of the device > > and should be fixed before it causes more problems. > > > > For now, I suggest following what most other drivers so which is to > > call blk_create_devicef() passing a devnum of -1. > > > >> + if (devnum < 0) > >> return EFI_OUT_OF_RESOURCES; > >> > >> name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */ > >> -- > >> 2.34.1 Regards, Simon
diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c index add00eeebb..e3abd90275 100644 --- a/lib/efi_driver/efi_block_device.c +++ b/lib/efi_driver/efi_block_device.c @@ -124,10 +124,8 @@ efi_bl_create_block_device(efi_handle_t handle, void *interface) struct efi_block_io *io = interface; struct efi_blk_plat *plat; - devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); - if (devnum == -ENODEV) - devnum = 0; - else if (devnum < 0) + devnum = blk_next_free_devnum(UCLASS_EFI_LOADER); + if (devnum < 0) return EFI_OUT_OF_RESOURCES; name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */
The devnum value of the blk_desc structure starts from 0, current efi_bl_create_block_device() function creates two "efiblk#0" devices for the cases that blk_find_max_devnum() returns -ENODEV and blk_find_max_devnum() returns 0(one device found in this case). This commit uses blk_next_free_devnum() instead of blk_find_max_devnum(). Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- Changes in v2: - uses blk_next_free_devnum() instead of blk_find_max_devnum() lib/efi_driver/efi_block_device.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)