Hi Pavel,

On Sunday, September 23, 2012 5:28:31 PM, Pavel Herrmann wrote:

add some CCs

On Wednesday 19 September 2012 16:34:02 Pavel Herrmann wrote:

...

Put block device interaction code into separate file from filesystem logic. This makes it easier to change block device API, and is similar to what other filesystems do. Cleanup some logic inconsistencies as well.

Sounds good. See my comments below.

This might need some light rework to make it applicable after http://patchwork.ozlabs.org/patch/184793/, which should be applied soon.

...

Signed-off-by: Pavel Herrmann morpheus.ibis@gmail.com

fs/fat/Makefile | 4 +- fs/fat/dev.c | 191 +++++++++++++++++++++++++++++++++++++++++++++++++++++ fs/fat/fat.c | 175 ++++++------------------------------------------ fs/fat/fat_write.c | 21 +++--- include/fat.h | 7 ++ 5 files changed, 228 insertions(+), 170 deletions(-) create mode 100644 fs/fat/dev.c

diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..5d10b24 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -24,8 +24,8 @@ include $(TOPDIR)/config.mk LIB = $(obj)libfat.o

AOBJS = -COBJS-$(CONFIG_CMD_FAT) := fat.o -COBJS-$(CONFIG_FAT_WRITE):= fat_write.o +COBJS-$(CONFIG_CMD_FAT) := fat.o dev.o +COBJS-$(CONFIG_FAT_WRITE) := fat_write.o dev.o

ifndef CONFIG_SPL_BUILD COBJS-$(CONFIG_CMD_FAT) += file.o diff --git a/fs/fat/dev.c b/fs/fat/dev.c new file mode 100644 index 0000000..d5ff0c5 --- /dev/null +++ b/fs/fat/dev.c @@ -0,0 +1,191 @@ +/*

• • (C) Copyright 2012
• • Pavel Herrmann morpheus.ibis@gmail.com

Since 99% of the code here comes from fat.c and fat_write.c, the copyright information from these files should probably be added here too.

...

• • See file CREDITS for list of people who contributed to this
• • project.
• • This program is free software; you can redistribute it and/or
• • modify it under the terms of the GNU General Public License as
• • published by the Free Software Foundation; either version 2 of
• • the License, or (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public

License

• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+#include <common.h> +#include <fat.h> +#include <part.h> +#include <errno.h>

+#define DOS_BOOT_MAGIC_OFFSET 0x1fe +#define DOS_FS_TYPE_OFFSET 0x36 +#define DOS_FS32_TYPE_OFFSET 0x52

+static block_dev_desc_t *cur_dev; +static unsigned int cur_part_nr; +static disk_partition_t cur_part_info;

+int fat_disk_read(__u32 block, __u32 nr_blocks, void *buf) +{

• if (!cur_dev || !cur_dev->block_read)

• return -1;
  

• return cur_dev->block_read(cur_dev->dev,

• cur_part_info.start + block, nr_blocks, buf);
  

+}

+int fat_disk_write(__u32 block, __u32 nr_blocks, void *buf) +{

• if (!cur_dev || !cur_dev->block_read)

• return -1;
  

• return cur_dev->block_read(cur_dev->dev,

• cur_part_info.start + block, nr_blocks, buf);
  

+}

block_write...

Please, split this patch into: - moved code + renaming, - behavior changes (one patch per behavior change). That will make things clearer and easier to review.

...

+int fat_get_blksz(void)

cur_dev->blksz is ulong, so long might be better here, even if it does not seem strictly necessary.

...

+{

• if (cur_dev == NULL)

• return -EINVAL;
  

• return cur_dev->blksz;

Or cur_part_info.blksize? Your calls to this function replace both usages, so please make sure that this won't cause any trouble.

...

+}

+int fat_get_partsize(void)

long?

...

+{

• if (cur_dev == NULL)

• return -EINVAL;
  

This test looks unrelated to the line below and changes the behavior where this function is called. Please clarify.

...

• return cur_part_info.size;

+}

+int fat_register_device(block_dev_desc_t *dev_desc, int part_no) +{

• ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
• /* First close any currently found FAT filesystem */
• cur_dev = NULL;

+#if (defined(CONFIG_CMD_IDE) || \

• defined(CONFIG_CMD_SATA) || \
• defined(CONFIG_CMD_SCSI) || \
• defined(CONFIG_CMD_USB) || \
• defined(CONFIG_MMC) || \
• defined(CONFIG_SYSTEMACE))
• /* Read the partition table, if present */
• if (!get_partition_info(dev_desc, part_no, &cur_part_info)) {

• cur_dev = dev_desc;
  

• cur_part_nr = part_no;
  

• }

+#endif

• /* Otherwise it might be a superfloppy (whole-disk FAT

filesystem) */

• if (!cur_dev) {

• if (part_no != 1) {
  

• 	printf("** Partition %d not valid on device %d **\n",
  

• 	part_no, dev_desc->dev);
  

• 	return -1;
  

• }
  

• cur_dev = dev_desc;
  

• cur_part_nr = 1;
  

• cur_part_info.start = 0;
  

• cur_part_info.size = dev_desc->lba;
  

• cur_part_info.blksz = dev_desc->blksz;
  

• memset(cur_part_info.name, 0, sizeof(cur_part_info.name));
  

• memset(cur_part_info.type, 0, sizeof(cur_part_info.type));
  

• }
• /* Make sure it has a valid FAT header */
• if (fat_disk_read(0, 1, buffer) != 1) {

• cur_dev = NULL;
  

• return -1;
  

• }
• /* Check if it's actually a DOS volume */
• if (memcmp(buffer + DOS_BOOT_MAGIC_OFFSET, "\x55\xAA", 2)) {

• cur_dev = NULL;
  

• return -1;
  

• }
• /* Check for FAT12/FAT16/FAT32 filesystem */
• if (!memcmp(buffer + DOS_FS_TYPE_OFFSET, "FAT", 3))

• return 0;
  

• if (!memcmp(buffer + DOS_FS32_TYPE_OFFSET, "FAT32", 5))

• return 0;
  

• cur_dev = NULL;
• return -1;

+}

+int file_fat_detectfs(void) +{

• boot_sector bs;
• volume_info volinfo;
• int fatsize;
• char vol_label[12];
• if (cur_dev == NULL) {

• printf("No current device\n");
  

• return 1;
  

• }

+#if defined(CONFIG_CMD_IDE) || \

• defined(CONFIG_CMD_SATA) || \
• defined(CONFIG_CMD_SCSI) || \
• defined(CONFIG_CMD_USB) || \
• defined(CONFIG_MMC)
• printf("Interface: ");
• switch (cur_dev->if_type) {
• case IF_TYPE_IDE:

• printf("IDE");
  

• break;
  

• case IF_TYPE_SATA:

• printf("SATA");
  

• break;
  

• case IF_TYPE_SCSI:

• printf("SCSI");
  

• break;
  

• case IF_TYPE_ATAPI:

• printf("ATAPI");
  

• break;
  

• case IF_TYPE_USB:

• printf("USB");
  

• break;
  

• case IF_TYPE_DOC:

• printf("DOC");
  

• break;
  

• case IF_TYPE_MMC:

• printf("MMC");
  

• break;
  

• default:

• printf("Unknown");
  

• }
• printf("\n Device %d: ", cur_dev->dev);
• dev_print(cur_dev);

+#endif

• if (fat_read_bootsectandvi(&bs, &volinfo, &fatsize)) {

• printf("\nNo valid FAT fs found\n");
  

• return 1;
  

• }
• memcpy(vol_label, volinfo.volume_label, 11);
• vol_label[11] = '\0';
• volinfo.fs_type[5] = '\0';
• printf("Partition %d: Filesystem: %s "%s"\n", cur_part_nr,

• volinfo.fs_type, vol_label);
  

• return 0;

+}

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 19f6a8c..a13c62b 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -45,84 +45,6 @@ static void downcase(char *str) } }

-static block_dev_desc_t *cur_dev; -static unsigned int cur_part_nr; -static disk_partition_t cur_part_info;

-#define DOS_BOOT_MAGIC_OFFSET 0x1fe -#define DOS_FS_TYPE_OFFSET 0x36 -#define DOS_FS32_TYPE_OFFSET 0x52

-static int disk_read(__u32 block, __u32 nr_blocks, void *buf) -{

• if (!cur_dev || !cur_dev->block_read)

• return -1;
  

• return cur_dev->block_read(cur_dev->dev,

• 	cur_part_info.start + block, nr_blocks, buf);
  

-}

-int fat_register_device(block_dev_desc_t * dev_desc, int part_no) -{

• ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
• /* First close any currently found FAT filesystem */
• cur_dev = NULL;

-#if (defined(CONFIG_CMD_IDE) || \

• defined(CONFIG_CMD_SATA) || \
  

• defined(CONFIG_CMD_SCSI) || \
  

• defined(CONFIG_CMD_USB) || \
  

• defined(CONFIG_MMC) || \
  

• defined(CONFIG_SYSTEMACE) )
  

• /* Read the partition table, if present */
• if (!get_partition_info(dev_desc, part_no, &cur_part_info)) {

• cur_dev = dev_desc;
  

• cur_part_nr = part_no;
  

• }

-#endif

• /* Otherwise it might be a superfloppy (whole-disk FAT

filesystem) */

• if (!cur_dev) {

• if (part_no != 1) {
  

• 	printf("** Partition %d not valid on device %d **\n",
  

• 			part_no, dev_desc->dev);
  

• 	return -1;
  

• }
  

• cur_dev = dev_desc;
  

• cur_part_nr = 1;
  

• cur_part_info.start = 0;
  

• cur_part_info.size = dev_desc->lba;
  

• cur_part_info.blksz = dev_desc->blksz;
  

• memset(cur_part_info.name, 0, sizeof(cur_part_info.name));
  

• memset(cur_part_info.type, 0, sizeof(cur_part_info.type));
  

• }
• /* Make sure it has a valid FAT header */
• if (disk_read(0, 1, buffer) != 1) {

• cur_dev = NULL;
  

• return -1;
  

• }
• /* Check if it's actually a DOS volume */
• if (memcmp(buffer + DOS_BOOT_MAGIC_OFFSET, "\x55\xAA", 2)) {

• cur_dev = NULL;
  

• return -1;
  

• }
• /* Check for FAT12/FAT16/FAT32 filesystem */
• if (!memcmp(buffer + DOS_FS_TYPE_OFFSET, "FAT", 3))

• return 0;
  

• if (!memcmp(buffer + DOS_FS32_TYPE_OFFSET, "FAT32", 5))

• return 0;
  

• cur_dev = NULL;
• return -1;

-}

/*

• Get the first occurence of a directory delimiter ('/' or '')

in a string. * Return index into string if found, -1 otherwise. @@ -212,7 +134,7 @@ static __u32 get_fatent(fsdata *mydata, __u32 entry)

startblock += mydata->fat_sect;	/* Offset from start of disk */

• if (disk_read(startblock, getsize, bufptr) < 0) {
  

• if (fat_disk_read(startblock, getsize, bufptr) < 0) {
  debug("Error reading FAT blocks\n");
  return ret;
  

  }

@@ -290,7 +212,7 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) printf("FAT: Misaligned buffer address (%p)\n", buffer);

while (size >= mydata->sect_size) {

• 	ret = disk_read(startsect++, 1, tmpbuf);
  

• 	ret = fat_disk_read(startsect++, 1, tmpbuf);
  if (ret != 1) {
  	debug("Error reading data (got %d)\n", ret);
  	return -1;
  

@@ -302,7 +224,7 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) } } else { idx = size / mydata->sect_size;

• ret = disk_read(startsect, idx, buffer);
  

• ret = fat_disk_read(startsect, idx, buffer);
  

  if (ret != idx) { debug("Error reading data (got %d)\n", ret); return -1;

@@ -315,7 +237,7 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) if (size) { ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size);

• ret = disk_read(startsect, 1, tmpbuf);
  

• ret = fat_disk_read(startsect, 1, tmpbuf);
  

  if (ret != 1) { debug("Error reading data (got %d)\n", ret); return -1;

@@ -702,25 +624,28 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, /*

• Read boot sector and volume info from a FAT filesystem

*/ -static int -read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) +int +fat_read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) { __u8 *block; volume_info *vistart; int ret = 0;

• int blksz;

• if (cur_dev == NULL) {

• blksz = fat_get_blksz();
• if (blksz <= 0) {

• /*this means the device is NULL, or otherwise unavailable*/
  

  debug("Error: no device selected\n"); return -1; }

• block = memalign(ARCH_DMA_MINALIGN, cur_dev->blksz);

• block = memalign(ARCH_DMA_MINALIGN, blksz); if (block == NULL) { debug("Error: allocating block\n"); return -1; }

• if (disk_read(0, 1, block) < 0) {

• if (fat_disk_read(0, 1, block) < 0) { debug("Error: reading block\n"); goto fail; }

@@ -792,11 +717,14 @@ do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols) __u32 root_cluster = 0; int rootdir_size = 0; int j;

• int blksz;

• if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {

• if (fat_read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) { debug("Error: reading boot sector\n"); return -1; }

• /*this will always succeed, because it did in above call*/

• blksz = fat_get_blksz();

  if (mydata->fatsize == 32) { root_cluster = bs.root_cluster;

@@ -812,9 +740,9 @@ do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols)

mydata->sect_size = (bs.sector_size[1] << 8) + bs.sector_size[0]; mydata->clust_size = bs.cluster_size;

• if (mydata->sect_size != cur_part_info.blksz) {

• printf("Error: FAT sector size mismatch (fs=%hu, dev=%lu)\n",
  

• 		mydata->sect_size, cur_part_info.blksz);
  

• if (mydata->sect_size != blksz) {

cur_part_info vs. cur_dev OK?

...

• printf("Error: FAT sector size mismatch (fs=%hu, dev=%d)\n",
  

• 		mydata->sect_size, blksz);
  

  return -1; }

@@ -884,7 +812,7 @@ do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols) debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n", cursect, mydata->clust_size, DIRENTSPERBLOCK);

• 	if (disk_read(cursect,
  

• 	if (fat_disk_read(cursect,
  		(mydata->fatsize == 32) ?
  		(mydata->clust_size) :
  		PREFETCH_BLOCKS,
  

@@ -1124,69 +1052,6 @@ exit: return ret; }

-int file_fat_detectfs(void) -{

• boot_sector bs;
• volume_info volinfo;
• int fatsize;
• char vol_label[12];
• if (cur_dev == NULL) {

• printf("No current device\n");
  

• return 1;
  

• }

-#if defined(CONFIG_CMD_IDE) || \

• defined(CONFIG_CMD_SATA) || \
• defined(CONFIG_CMD_SCSI) || \
• defined(CONFIG_CMD_USB) || \
• defined(CONFIG_MMC)
• printf("Interface: ");
• switch (cur_dev->if_type) {
• case IF_TYPE_IDE:

• printf("IDE");
  

• break;
  

• case IF_TYPE_SATA:

• printf("SATA");
  

• break;
  

• case IF_TYPE_SCSI:

• printf("SCSI");
  

• break;
  

• case IF_TYPE_ATAPI:

• printf("ATAPI");
  

• break;
  

• case IF_TYPE_USB:

• printf("USB");
  

• break;
  

• case IF_TYPE_DOC:

• printf("DOC");
  

• break;
  

• case IF_TYPE_MMC:

• printf("MMC");
  

• break;
  

• default:

• printf("Unknown");
  

• }
• printf("\n Device %d: ", cur_dev->dev);
• dev_print(cur_dev);

-#endif

• if (read_bootsectandvi(&bs, &volinfo, &fatsize)) {

• printf("\nNo valid FAT fs found\n");
  

• return 1;
  

• }
• memcpy(vol_label, volinfo.volume_label, 11);
• vol_label[11] = '\0';
• volinfo.fs_type[5] = '\0';
• printf("Partition %d: Filesystem: %s "%s"\n", cur_part_nr,

• volinfo.fs_type, vol_label);
  

• return 0;

-}

int file_fat_ls(const char *dir) { return do_fat_read(dir, NULL, 0, LS_YES); diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index a6181e7..2e5bda9 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c

Please get rid of the '#include "fat.c"'. This is the perfect opportunity to do so.

...

@@ -43,17 +43,12 @@ static void uppercase(char *str, int len) static int total_sector; static int disk_write(__u32 block, __u32 nr_blocks, void *buf) {

• if (!cur_dev || !cur_dev->block_write)

• return -1;
  

• if (cur_part_info.start + block + nr_blocks >

• cur_part_info.start + total_sector) {
  

• if (block + nr_blocks > total_sector) { printf("error: overflow occurs\n"); return -1; }

• return cur_dev->block_write(cur_dev->dev,

• 	cur_part_info.start + block, nr_blocks,	buf);
  

• return fat_disk_write(block, nr_blocks, buf);

}

Why don't you move that to dev.c as well?

...

/* @@ -196,7 +191,7 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return -1; }

• if (disk_read(startblock, getsize, bufptr) < 0) {
  

• if (fat_disk_read(startblock, getsize, bufptr) < 0) {
  debug("Error reading FAT blocks\n");
  return ret;
  

  }

@@ -509,7 +504,7 @@ static int set_fatent_value(fsdata *mydata, __u32 entry, __u32 entry_value) return -1; }

• if (disk_read(startblock, getsize, bufptr) < 0) {
  

• if (fat_disk_read(startblock, getsize, bufptr) < 0) {
  debug("Error reading FAT blocks\n");
  return -1;
  

  }

@@ -793,7 +788,7 @@ static int check_overflow(fsdata *mydata, __u32 clustnum, unsigned long size) if (size % mydata->sect_size) sect_num++;

• if (startsect + sect_num > cur_part_info.start + total_sector)

• if (startsect + sect_num > total_sector)

Are you certain? It depends on if mydata->data_begin and mydata->rootdir_sect are disk- or partition-relative (I did not check).

Anyway, this is a change of behavior, so that should go into a separate patch.

...

return -1;

return 0; @@ -932,14 +927,14 @@ static int do_fat_write(const char *filename, void *buffer,

dir_curclust = 0;

• if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {

• if (fat_read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) { debug("error: reading boot sector\n"); return -1; }

  total_sector = bs.total_sect; if (total_sector == 0)

• total_sector = cur_part_info.size;
  

• total_sector = fat_get_partsize();
  

cur_dev was not tested here in the original code.

...

if (mydata->fatsize == 32) mydata->fatlength = bs.fat32_length; @@ -977,7 +972,7 @@ static int do_fat_write(const char *filename, void *buffer, return -1; }

• if (disk_read(cursect,

• if (fat_disk_read(cursect, (mydata->fatsize == 32) ? (mydata->clust_size) : PREFETCH_BLOCKS, do_fat_read_block) < 0) {

diff --git a/include/fat.h b/include/fat.h index f1b4a0d..98f77a5 100644 --- a/include/fat.h +++ b/include/fat.h @@ -213,4 +213,11 @@ const char *file_getfsname(int idx); int fat_register_device(block_dev_desc_t *dev_desc, int part_no);

int file_fat_write(const char *filename, void *buffer, unsigned long maxsize); + +/*internal functions for fs/blockdev API abstraction*/ +int fat_disk_read(__u32 block, __u32 nr_blocks, void *buf); +int fat_disk_write(__u32 block, __u32 nr_blocks, void *buf); +int fat_read_bootsectandvi(boot_sector *bs, volume_info *vi, int *fatsize); +int fat_get_blksz(void); +int fat_get_partsize(void); #endif /* _FAT_H_ */

Since these are FAT-private functions, shouldn't their declarations go to some private header file in the fs/fat/ folder?

Best regards, Benoît