* [PATCH 1/1] block: fix blk_get_devnum_by_typename()
@ 2022-08-02 9:49 Heinrich Schuchardt
2022-08-02 12:41 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2022-08-02 9:49 UTC (permalink / raw)
To: Simon Glass; +Cc: AKASHI Takahiro, u-boot, Heinrich Schuchardt
Both the 'host' and the 'efiloader' block devices use the same parent
uclass root. Thus the parent uclass is not an indicator the interface type.
Currently the following fails:
setenv efi_selftest block device
bootefi selftest
part list efiloader 0
Struct blk_desc contains the interface type. So we can check it directly
without caring about the parent uclass.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
drivers/block/blk-uclass.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 21c5209bb6..779cda7834 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -122,15 +122,11 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)
debug("%s: if_type=%d, devnum=%d: %s, %d, %d\n", __func__,
if_type, devnum, dev->name, desc->if_type, desc->devnum);
- if (desc->devnum != devnum)
- continue;
- /* Find out the parent device uclass */
- if (device_get_uclass_id(dev->parent) != uclass_id) {
- debug("%s: parent uclass %d, this dev %d\n", __func__,
- device_get_uclass_id(dev->parent), uclass_id);
+ if (desc->if_type != if_type)
+ continue;
+ if (desc->devnum != devnum)
continue;
- }
if (device_probe(dev))
return NULL;
--
2.36.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: fix blk_get_devnum_by_typename()
2022-08-02 9:49 [PATCH 1/1] block: fix blk_get_devnum_by_typename() Heinrich Schuchardt
@ 2022-08-02 12:41 ` Simon Glass
2022-08-02 16:22 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2022-08-02 12:41 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: AKASHI Takahiro, U-Boot Mailing List
Hi Heinrich,
On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Both the 'host' and the 'efiloader' block devices use the same parent
> uclass root. Thus the parent uclass is not an indicator the interface type.
>
> Currently the following fails:
>
> setenv efi_selftest block device
> bootefi selftest
> part list efiloader 0
>
> Struct blk_desc contains the interface type. So we can check it directly
> without caring about the parent uclass.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> drivers/block/blk-uclass.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
We've had this discussion before, but this patch will make it
difficult to migrate away from IF_TYPE.
Instead we should fix EFI. Having the root as a parent of a block
device seems wrong to me. What is the actual device that provides the
block device?
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 21c5209bb6..779cda7834 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -122,15 +122,11 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)
>
> debug("%s: if_type=%d, devnum=%d: %s, %d, %d\n", __func__,
> if_type, devnum, dev->name, desc->if_type, desc->devnum);
> - if (desc->devnum != devnum)
> - continue;
>
> - /* Find out the parent device uclass */
> - if (device_get_uclass_id(dev->parent) != uclass_id) {
> - debug("%s: parent uclass %d, this dev %d\n", __func__,
> - device_get_uclass_id(dev->parent), uclass_id);
> + if (desc->if_type != if_type)
> + continue;
> + if (desc->devnum != devnum)
> continue;
> - }
>
> if (device_probe(dev))
> return NULL;
> --
> 2.36.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: fix blk_get_devnum_by_typename()
2022-08-02 12:41 ` Simon Glass
@ 2022-08-02 16:22 ` Heinrich Schuchardt
2022-08-03 18:14 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2022-08-02 16:22 UTC (permalink / raw)
To: Simon Glass; +Cc: AKASHI Takahiro, U-Boot Mailing List
On 8/2/22 14:41, Simon Glass wrote:
> Hi Heinrich,
>
> On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Both the 'host' and the 'efiloader' block devices use the same parent
>> uclass root. Thus the parent uclass is not an indicator the interface type.
>>
>> Currently the following fails:
>>
>> setenv efi_selftest block device
>> bootefi selftest
>> part list efiloader 0
>>
>> Struct blk_desc contains the interface type. So we can check it directly
>> without caring about the parent uclass.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> drivers/block/blk-uclass.c | 10 +++-------
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> We've had this discussion before, but this patch will make it
Yes, you blocked the obvious solution.
> difficult to migrate away from IF_TYPE.
My patch does not have any impact on the migration as function
blk_get_devnum_by_typename() will simply vanish together with IF_TYPE.
Migrating away from IF_TYPE could follow the following path if you
wanted to keep struct blk_desc:
Just replace devnum by the udevice in struct blk_desc and add the GUI
representation of the device type (e.g. "mmc") as field to struct blk_ops.
The field devnum only made sense in the world of legacy drivers.
By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers?
A better solution would be to completely do away with struct blk_desc
and instead always use the udevice.
>
> Instead we should fix EFI. Having the root as a parent of a block
> device seems wrong to me. What is the actual device that provides the
> block device?
There is no actual parent device. In
lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM
disk. This is the same situation as with the sandbox host device where
you have chosen root as the dummy parent for good reason.
In
"[1/1] drivers: add memory disk support"
https://patchwork.ozlabs.org/project/uboot/patch/20220419211641.316935-1-heinrich.schuchardt@canonical.com/
I have proposed a further block device type that has no actual parent.
The idea of using a parent device to match a block device was always a
dead end. Let's bury it now.
Best regards
Heinrich
>
>>
>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>> index 21c5209bb6..779cda7834 100644
>> --- a/drivers/block/blk-uclass.c
>> +++ b/drivers/block/blk-uclass.c
>> @@ -122,15 +122,11 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)
>>
>> debug("%s: if_type=%d, devnum=%d: %s, %d, %d\n", __func__,
>> if_type, devnum, dev->name, desc->if_type, desc->devnum);
>> - if (desc->devnum != devnum)
>> - continue;
>>
>> - /* Find out the parent device uclass */
>> - if (device_get_uclass_id(dev->parent) != uclass_id) {
>> - debug("%s: parent uclass %d, this dev %d\n", __func__,
>> - device_get_uclass_id(dev->parent), uclass_id);
>> + if (desc->if_type != if_type)
>> + continue;
>> + if (desc->devnum != devnum)
>> continue;
>> - }
>>
>> if (device_probe(dev))
>> return NULL;
>> --
>> 2.36.1
>>
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: fix blk_get_devnum_by_typename()
2022-08-02 16:22 ` Heinrich Schuchardt
@ 2022-08-03 18:14 ` Simon Glass
2022-08-04 6:06 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2022-08-03 18:14 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: AKASHI Takahiro, U-Boot Mailing List
Hi Heinrich,
On Tue, 2 Aug 2022 at 10:22, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 8/2/22 14:41, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Both the 'host' and the 'efiloader' block devices use the same parent
> >> uclass root. Thus the parent uclass is not an indicator the interface type.
> >>
> >> Currently the following fails:
> >>
> >> setenv efi_selftest block device
> >> bootefi selftest
> >> part list efiloader 0
> >>
> >> Struct blk_desc contains the interface type. So we can check it directly
> >> without caring about the parent uclass.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >> drivers/block/blk-uclass.c | 10 +++-------
> >> 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > We've had this discussion before, but this patch will make it
>
> Yes, you blocked the obvious solution.
Yes, I explained the problem with that at the time.
>
> > difficult to migrate away from IF_TYPE.
>
> My patch does not have any impact on the migration as function
> blk_get_devnum_by_typename() will simply vanish together with IF_TYPE.
>
> Migrating away from IF_TYPE could follow the following path if you
> wanted to keep struct blk_desc:
>
> Just replace devnum by the udevice in struct blk_desc and add the GUI
> representation of the device type (e.g. "mmc") as field to struct blk_ops.
>
> The field devnum only made sense in the world of legacy drivers.
> By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers?
>
> A better solution would be to completely do away with struct blk_desc
> and instead always use the udevice.
>
> >
> > Instead we should fix EFI. Having the root as a parent of a block
> > device seems wrong to me. What is the actual device that provides the
> > block device?
>
> There is no actual parent device. In
> lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM
> disk. This is the same situation as with the sandbox host device where
> you have chosen root as the dummy parent for good reason.
Is it a RAM disk on disk, or an in-memory one?
>
> In
> "[1/1] drivers: add memory disk support"
> https://patchwork.ozlabs.org/project/uboot/patch/20220419211641.316935-1-heinrich.schuchardt@canonical.com/
> I have proposed a further block device type that has no actual parent.
>
> The idea of using a parent device to match a block device was always a
> dead end. Let's bury it now.
Possibly, but it is how we can drop the IF_TYPE stuff. Let me spend a
bit of time looking at this and see what can be done.
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] block: fix blk_get_devnum_by_typename()
2022-08-03 18:14 ` Simon Glass
@ 2022-08-04 6:06 ` Heinrich Schuchardt
2022-08-07 15:48 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2022-08-04 6:06 UTC (permalink / raw)
To: Simon Glass; +Cc: AKASHI Takahiro, U-Boot Mailing List
On 8/3/22 20:14, Simon Glass wrote:
> Hi Heinrich,
>
> On Tue, 2 Aug 2022 at 10:22, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 8/2/22 14:41, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Both the 'host' and the 'efiloader' block devices use the same parent
>>>> uclass root. Thus the parent uclass is not an indicator the interface type.
>>>>
>>>> Currently the following fails:
>>>>
>>>> setenv efi_selftest block device
>>>> bootefi selftest
>>>> part list efiloader 0
>>>>
>>>> Struct blk_desc contains the interface type. So we can check it directly
>>>> without caring about the parent uclass.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> drivers/block/blk-uclass.c | 10 +++-------
>>>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> We've had this discussion before, but this patch will make it
>>
>> Yes, you blocked the obvious solution.
>
> Yes, I explained the problem with that at the time.
>
>>
>>> difficult to migrate away from IF_TYPE.
>>
>> My patch does not have any impact on the migration as function
>> blk_get_devnum_by_typename() will simply vanish together with IF_TYPE.
>>
>> Migrating away from IF_TYPE could follow the following path if you
>> wanted to keep struct blk_desc:
>>
>> Just replace devnum by the udevice in struct blk_desc and add the GUI
>> representation of the device type (e.g. "mmc") as field to struct blk_ops.
>>
>> The field devnum only made sense in the world of legacy drivers.
>> By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers?
>>
>> A better solution would be to completely do away with struct blk_desc
>> and instead always use the udevice.
>>
>>>
>>> Instead we should fix EFI. Having the root as a parent of a block
>>> device seems wrong to me. What is the actual device that provides the
>>> block device?
>>
>> There is no actual parent device. In
>> lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM
>> disk. This is the same situation as with the sandbox host device where
>> you have chosen root as the dummy parent for good reason.
>
> Is it a RAM disk on disk, or an in-memory one?
With the patch it is just an memory area embedded the U-Boot binary. But
in future you might also use it to declare a memory area in the rest of
RAM as backing a RAM disk.
>
>>
>> In
>> "[1/1] drivers: add memory disk support"
>> https://patchwork.ozlabs.org/project/uboot/patch/20220419211641.316935-1-heinrich.schuchardt@canonical.com/
>> I have proposed a further block device type that has no actual parent.
>>
>> The idea of using a parent device to match a block device was always a
>> dead end. Let's bury it now.
>
> Possibly, but it is how we can drop the IF_TYPE stuff. Let me spend a
> bit of time looking at this and see what can be done.
To me the device driver of the actual device would be the most natural
indicator of the device type. Looking forward to your suggestion.
Best regards
Heinrich
>
> [..]
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] block: fix blk_get_devnum_by_typename()
2022-08-04 6:06 ` Heinrich Schuchardt
@ 2022-08-07 15:48 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2022-08-07 15:48 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: AKASHI Takahiro, U-Boot Mailing List
Hi Heinrich,
On Thu, 4 Aug 2022 at 00:06, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 8/3/22 20:14, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 2 Aug 2022 at 10:22, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 8/2/22 14:41, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> Both the 'host' and the 'efiloader' block devices use the same parent
> >>>> uclass root. Thus the parent uclass is not an indicator the interface type.
> >>>>
> >>>> Currently the following fails:
> >>>>
> >>>> setenv efi_selftest block device
> >>>> bootefi selftest
> >>>> part list efiloader 0
> >>>>
> >>>> Struct blk_desc contains the interface type. So we can check it directly
> >>>> without caring about the parent uclass.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> ---
> >>>> drivers/block/blk-uclass.c | 10 +++-------
> >>>> 1 file changed, 3 insertions(+), 7 deletions(-)
> >>>
> >>> We've had this discussion before, but this patch will make it
> >>
> >> Yes, you blocked the obvious solution.
> >
> > Yes, I explained the problem with that at the time.
> >
> >>
> >>> difficult to migrate away from IF_TYPE.
> >>
> >> My patch does not have any impact on the migration as function
> >> blk_get_devnum_by_typename() will simply vanish together with IF_TYPE.
> >>
> >> Migrating away from IF_TYPE could follow the following path if you
> >> wanted to keep struct blk_desc:
> >>
> >> Just replace devnum by the udevice in struct blk_desc and add the GUI
> >> representation of the device type (e.g. "mmc") as field to struct blk_ops.
> >>
> >> The field devnum only made sense in the world of legacy drivers.
> >> By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers?
> >>
> >> A better solution would be to completely do away with struct blk_desc
> >> and instead always use the udevice.
> >>
> >>>
> >>> Instead we should fix EFI. Having the root as a parent of a block
> >>> device seems wrong to me. What is the actual device that provides the
> >>> block device?
> >>
> >> There is no actual parent device. In
> >> lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM
> >> disk. This is the same situation as with the sandbox host device where
> >> you have chosen root as the dummy parent for good reason.
> >
> > Is it a RAM disk on disk, or an in-memory one?
>
> With the patch it is just an memory area embedded the U-Boot binary. But
> in future you might also use it to declare a memory area in the rest of
> RAM as backing a RAM disk.
I sent a series to clean this up.
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-07 15:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-02 9:49 [PATCH 1/1] block: fix blk_get_devnum_by_typename() Heinrich Schuchardt
2022-08-02 12:41 ` Simon Glass
2022-08-02 16:22 ` Heinrich Schuchardt
2022-08-03 18:14 ` Simon Glass
2022-08-04 6:06 ` Heinrich Schuchardt
2022-08-07 15:48 ` 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).