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

[Wolfgang Denk]

Dear Josh Wu,

In message <53995100.9080307 at 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?

> 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?

> 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.

> >
> >> 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.

> >
> >> +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.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Everyting looks interesting until you do it. Then you find it's  just
another job.                     - Terry Pratchett, _Moving Pictures_