[U-Boot] [PATCH 15/44] dm: blk: Add a legacy block interface

[Simon Glass]

Hi Stephen,

On 12 April 2016 at 14:28, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/09/2016 08:45 PM, Simon Glass wrote:
>>
>> There is quite a bit of duplicated common code related to block devices
>> in the IDE and SCSI implementations.
>>
>> Create some helper functions that can be used to reduce the duplication.
>> These rely on a linker list of interface-type drivers
>
>
> It'd be useful to know if this gets thrown away later in the series if
> everything is converted to DM. That would affect how thoroughly one reviews
> it.
>
> At this point I'm slightly lost where the series is going, since it all
> seems to be adding a slew of stuff for legacy paths more than converting to
> DM block devices. Perhaps it'll become more clear as I go along.

It's a bit of a pain to review. If it's any consolation it was more of
a pain to write. The goal is to create a 'legacy' driver which can be
used for non-DM code. It is just too painful to try to have the old
code and DM code co-exist without a common API.

>
>> diff --git a/drivers/block/blk_legacy.c b/drivers/block/blk_legacy.c
>
>
>> +#ifdef HAVE_BLOCK_DEVICE
>> +int blk_list_part(enum if_type if_type)
>> +{
>> +       struct blk_driver *drv = blk_driver_lookup_type(if_type);
>> +       struct blk_desc *desc;
>> +       int devnum, ok;
>> +
>> +       if (!drv)
>> +               return -ENOSYS;
>
>
> Nit: If drv can be NULL, I'd prefer to see the assignment right before the
> line that tests it rather than in the declaration. That makes it clearer to
> someone that they can't insert a line into the variable declarations that
> uses drv. That's something I've seen happen, and it's hard to spot the issue
> in a patch if the context isn't long enough to see this not-yet-happened
> test later in the code.

OK.

>
>> +       for (ok = 0, devnum = 0; devnum < drv->max_devs; ++devnum) {
>> +               if (get_desc(drv, devnum, &desc))
>> +                       continue;
>> +               if (desc->part_type != PART_TYPE_UNKNOWN) {
>> +                       ++ok;
>> +                       if (devnum)
>> +                               putc('\n');
>> +                       part_print(desc);
>
>
> Does this function support dumping the partition list for multiple devices
> in one invocation? If so, that seems odd; is there any command to do that?
> If not, I would expect a break here, and I'm not sure what if (devnum)
> putc() is about. Also, shouldn't the putc happen if a previous iteration of
> the for loop did print something, not based on the index in the list, since
> presumably it's possible that entries 0..2 are PART_TYPE_UNKNOWN and entry 3
> isn't?

Yes it supports that. I'll fix the newline handling. Actually I'm not
sure that it can happy, but let's assume it can.

>
>> +int blk_dselect_hwpart(struct blk_desc *desc, int hwpart)
>
>
> What does "dselect" mean as opposed to "select"?

There is a comment next to the main ones:

/*
 * These functions should take struct udevice instead of struct blk_desc,
 * but this is convenient for migration to driver model. Add a 'd' prefix
 * to the function operations, so that blk_read(), etc. can be reserved for
 * functions with the correct arguments.
 */

>
>> +struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int
>> devnum)
>
>
> Doesn't this get a desc not a devnum? It seems to be /by/ typename+devnum.

It had to find the desc, given the interface type devnum.

>
>> diff --git a/include/blk.h b/include/blk.h
>
>
>> +struct blk_driver {
>
>
>> +       /**
>> +        * select_hwpart() - Select a hardware partition
>> +        *
>> +        * Some devices (e.g. MMC) can support partitioning at the
>> hardware
>> +        * level. This is quite separate from the normal idea of
>> +        * software-based partitions. MMC hardware partitions must be
>> +        * explicitly selected. Once selected only the region of the
>> device
>> +        * covered by that partition is accessible.
>> +        *
>> +        * The MMC standard provides for two boot partitions (numbered 1
>> and 2),
>> +        * rpmb (3), and up to 4 addition general-purpose partitions
>> (4-7).
>
>
> There's also partition 0, the main user data partition, which is what is
> primarily used.

OK, will update comment.

>
>> +        *
>> +        * @desc:       Block device descriptor
>> +        * @hwpart:     Hardware partition number to select. 0 means the
>> raw
>> +        *              device, 1 is the first partition, 2 is the second,
>> etc.
>
>
> 0 isn't really "raw". At least to me, "raw" implies access to all data on
> the storage medium including partition tables, inter-partition gaps, and
> data across all partitions. However, for eMMC "0" means just another
> partition like any other partition, but this one just happens to be the one
> that's used most/typically.

OK will update comment. I want to get a bit more detail into the API
comments so that the meaning of these things is clear.

Regards,
Simon