[U-Boot] [PATCH] fs/fat: add a parameter: allow_whole_dev to fat_register_device()

[Josh Wu <josh.wu@atmel.com>]

Dear Wolfgang

On 6/12/2014 4:52 PM, Wolfgang Denk wrote:
> Dear Josh Wu,
>
> In message <53995100.9080307@atmel.com> you wrote:
>> In U-Boot when we access a partition of a device, we use 'ifname
>> dev:part' format.
>> For instance: 'mmc 0:1' means the MMC card's #1 partition of the device #0.
> Don;t we also support plain "ifname dev", i. e. without partition
> specification?

The problem is we only support "ifname dev" on command line mode or the 
filesystem call which calls get_device_and_partition().

For environment save/load and SPL load on FAT, which use 
fat_register_device() instead of calling get_device_and_partition(),
we need specify the partition number. It DOESN'T support "ifname dev" 
without partition number.
for example:
   1. to enable FAT env save/load, we need define: 
FAT_ENV_INTERFACE(=mmc), FAT_ENV_DEVICE(=0), FAT_ENV_PART(=1)
   2. to enable FAT SPL load, we need fine: 
CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION(=1)

>
>> But for a case if the mmc device has no partition table (MBR), it only
>> has one FAT partition.
> Um... No. Without a partition table, there are no partitins at all, so
> there can be no FAT partitions.  I guess you mean there is a FAT file
> system on the device?

yes, if we cannot find a partition table, then the first sector is 
assumed as FAT file system's first sector.

>
>> To support that case, we need to access by using 'mmc 0:0'.
> I feel that should be "mmc 0".
>
>> So the problem is: if we specify mmc 0:0 then I cannot access the mmc
>> device if it has a partition table.
> Well, if it _has_ a partition table, then you should select the
> correct partition number, and not use the (nonexitent) number 0.
>
>> And if we specify mmc 0:1 then I cannot access if it has no partition table.
> Correct.  Because no partition 1 exists.
>
>> For the fs layer this case is solved by use 'mmc 0', or 'mmc 0:auto' by
>> commit:
>> 10a37fd7a4 (disk: get_device_and_partition() "auto" partition and cleanup)
>>
>> But for env fat and SPL fat, we don't use the function in above commit
>> as we use a simpler function fat_register_device().
>> So this patch make this function works too.
> I may be wrong, but I think with your patch we do not implement the
> same behaviour as for get_device_and_partition() [see the commit
> message for the aforementioned commit how it is supposed to work].
>
> I think, we should implemnt consistent behaviour here.

I agree. I will read the get_device_and_partition() code to understand it.

>
>>>> But in FAT SPL and environment case, when we specify the partition number as
>>>> 1, it is reasonable to support the device has no partition table and only a
>>>> FAT partition.
>>> Why would the expectations in SPL be different from other use cases?
>> For example, when I use SPL binary in mmc card, I want it to load the
>> file: u-boot.img from the first partition.
>> I expect it should work even if the mmc device has no partition table.
> Please see the description in the commit message how it is supposed to
> work.  Note that it's not just the first partition, but actully the
> first _bootable_ partition which should get used.
>
>> But when I define CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION as 1.  it cannot
>> work when the mmc has no partition table.
>>
>> same thing happens for saving environment to a FAT file in MMC.
> This is even more problematic, as we do not have any definition where
> the environment would be located.  Is it the first partition?  Or the
> first bootable partition?  Or a specifically defined one?
>
> I think it is dangerous to just "make it work" without clearly
> specifying first what the expected behaviour should be.

I understand your concern. Thanks a lot for your comments.
After the discussing I get a further think of this problem. Let me 
summary here:

The problem I met is FAT env save/load and FAT spl load use 
fat_register_device() instead of get_device_and_partition().
So that means I must specify a partition number.

I think for usually user case, we don't want to specify the partition 
number. that means:
    1. if has partition table, please use partition #1.
    2. if no partition table, assume the whole device is a FAT file system.

if we agree with above, then the solution should be implement a way to 
support the case we don't specify a partition number.
    1. use get_device_and_partition().
    2. add a unspecify partition number support in fat_register_device()

I think #1 is the best way be cause we don't have to implement same 
things in two place. But I am doubt that the FAT spl can use it. I'll 
check this.

What do think of that?

>
>>>> +int fat_register_device(block_dev_desc_t *dev_desc, int part_no,
>>>> +			bool allow_whole_dev);
>>> Please make this an "int" type, and use 0 and 1.
>> Is there any special concern for that? like cause machine compatiable issue?
> Boolean values in C are 1 and 0.  Hiding these under other names (like
> "true" and "false") doesn't buy anything.

Okay. I just think use bool will be more readable. That also can make 
people less use an integer number, which in some case it's hard to 
understand it.

>
> Best regards,
>
> Wolfgang Denk
>

Best Regards,
Josh Wu