[U-Boot-Users] Bug in fat_register_device() ?

[U-Boot-Users] Bug in fat_register_device() ?

[hs at denx.de]

Hello,

I have on a CF Card a MBR with "FAT" on offset 0x36 (sektor 0). The actual
U-Boot Code assumes know, that this is a PBR, but it is a MBR ... So I prefer
First to check, if it is a valid MBR, and if not, check if it is maybe a PBR.
Like the following Patch:

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index a823b5a..d0e99ec 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -83,23 +83,25 @@ fat_register_device(block_dev_desc_t *dev_desc, int part_no)
 		/* no signature found */
 		return -1;
 	}
-	if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET],"FAT",3)) {
-		/* ok, we assume we are on a PBR only */
-		cur_part = 1;
-		part_offset=0;
-	}
-	else {
 #if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) || \
     (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
+	{
 		disk_partition_t info;
 		if(!get_partition_info(dev_desc, part_no, &info)) {
 			part_offset = info.start;
 			cur_part = part_no;
 		}
 		else {
-			printf ("** Partition %d not valid on device %d **\n",part_no,dev_desc->dev);
-			return -1;
+			if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET],"FAT",3)) {
+				/* ok, we assume we are on a PBR only */
+				cur_part = 1;
+				part_offset=0;
+			} else {
+				printf ("** Partition %d not valid on device %d **\n",part_no,dev_desc->dev);
+				return -1;
+			}
 		}
+	}
 #else
 		/* FIXME we need to determine the start block of the
 		 * partition where the DOS FS resides. This can be done
@@ -109,7 +111,6 @@ fat_register_device(block_dev_desc_t *dev_desc, int part_no)
 		part_offset=32;
 		cur_part = 1;
 #endif
-	}
 	return 0;
 }

Some comments? Other ways to distinguish between a MBR or PBR?

Best regards
Heiko Schocher
--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot-Users] Bug in fat_register_device() ?

[Wolfgang Denk]

Dear Heiko,

in message <1180970321.46642d51d24a1@webmail.mnet-online.de> you wrote:
> 
>  #if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) || \
>      (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
> +	{

Incorrect brace style.

>  		disk_partition_t info;
>  		if(!get_partition_info(dev_desc, part_no, &info)) {
>  			part_offset = info.start;
>  			cur_part = part_no;
>  		}
>  		else {

Ditto (not by you, but lease clean up anyway).

>  		}
> +	}
>  #else
>  		/* FIXME we need to determine the start block of the
--------^
>  		 * partition where the DOS FS resides. This can be done
--------^
> @@ -109,7 +111,6 @@ fat_register_device(block_dev_desc_t *dev_desc, int part_no)
>  		part_offset=32;
--------^
>  		cur_part = 1;
--------^

Seems you should fix indentation in these lines, and everything
inbetween.


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
Es ist offensichtlich, dass das menschliche Gehirn wie  ein  Computer
funktioniert.  Da  es  keine  dummen Computer gibt, gibt es also auch
keine dummen Menschen. Nur ein paar Leute, die unter DOS laufen.
                                                       -- <unbekannt>

[U-Boot-Users] Bug in fat_register_device() ?

[hs at denx.de]

Hello Wolfgang,

Wolfgang Denk <wd@denx.de> wrote:
> >  #if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) ||
> \
> >      (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
> > +	{
> 
> Incorrect brace style.
> 
> >  		disk_partition_t info;
> >  		if(!get_partition_info(dev_desc, part_no, &info)) {
> >  			part_offset = info.start;
> >  			cur_part = part_no;
> >  		}
> >  		else {
> 
> Ditto (not by you, but lease clean up anyway).
> 
> >  		}
> > +	}
> >  #else
> >  		/* FIXME we need to determine the start block of the
> --------^
> >  		 * partition where the DOS FS resides. This can be done
> --------^
> > @@ -109,7 +111,6 @@ fat_register_device(block_dev_desc_t *dev_desc, int
> part_no)
> >  		part_offset=32;
> --------^
> >  		cur_part = 1;
> --------^
> 
> Seems you should fix indentation in these lines, and everything
> inbetween.

Sorry, didnt had a look at this (was happy to find this issue ;-)
Here the new version of the patch:

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index a823b5a..2eaa42b 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -69,10 +69,14 @@ int
 fat_register_device(block_dev_desc_t *dev_desc, int part_no)
 {
 	unsigned char buffer[SECTOR_SIZE];
+#if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) || \
+    (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
+	disk_partition_t info;
+#endif
 
 	if (!dev_desc->block_read)
 		return -1;
-	cur_dev=dev_desc;
+	cur_dev = dev_desc;
 	/* check if we have a MBR (on floppies we have only a PBR) */
 	if (dev_desc->block_read (dev_desc->dev, 0, 1, (ulong *) buffer) != 1) {
 		printf ("** Can't read from device %d **\n", dev_desc->dev);
@@ -83,33 +87,31 @@ fat_register_device(block_dev_desc_t *dev_desc, int part_no)
 		/* no signature found */
 		return -1;
 	}
-	if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET],"FAT",3)) {
-		/* ok, we assume we are on a PBR only */
-		cur_part = 1;
-		part_offset=0;
-	}
-	else {
 #if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) || \
     (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
-		disk_partition_t info;
-		if(!get_partition_info(dev_desc, part_no, &info)) {
-			part_offset = info.start;
-			cur_part = part_no;
-		}
-		else {
-			printf ("** Partition %d not valid on device %d **\n",part_no,dev_desc->dev);
+	if(!get_partition_info (dev_desc, part_no, &info)) {
+		part_offset = info.start;
+		cur_part = part_no;
+	} else {
+		if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET], "FAT", 3)) {
+			/* ok, we assume we are on a PBR only */
+			cur_part = 1;
+			part_offset = 0;
+		} else {
+			printf ("** Partition %d not valid on device %d **\n",
+				part_no, dev_desc->dev);
 			return -1;
 		}
+	}
 #else
-		/* FIXME we need to determine the start block of the
-		 * partition where the DOS FS resides. This can be done
-		 * by using the get_partition_info routine. For this
-		 * purpose the libpart must be included.
-		 */
-		part_offset=32;
-		cur_part = 1;
+	/* FIXME we need to determine the start block of the
+	 * partition where the DOS FS resides. This can be done
+	 * by using the get_partition_info routine. For this
+	 * purpose the libpart must be included.
+	 */
+	part_offset = 32;
+	cur_part = 1;
 #endif
-	}
 	return 0;
 }

thanks
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot-Users] Bug in fat_register_device() ?

[Stefan Roese]

Hi Heiko,

On Tuesday 05 June 2007, hs at denx.de wrote:
> +	if(!get_partition_info (dev_desc, part_no, &info)) {

Space after the "if" please.

> +		part_offset = info.start;
> +		cur_part = part_no;
> +	} else {
> +		if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET], "FAT", 3)) {

Here too.

Viele Gr??e,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot-Users] Bug in fat_register_device() ?

[hs at denx.de]

Hello Stefan,

Stefan Roese wrote:
> > +	if(!get_partition_info (dev_desc, part_no, &info)) {
> 
> Space after the "if" please.
> 
> > +		part_offset = info.start;
> > +		cur_part = part_no;
> > +	} else {
> > +		if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET], "FAT", 3)) {
> 
> Here too.

Sorry, here the 3. version of the Patch:

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index a823b5a..d24b4d1 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -69,10 +69,14 @@ int
 fat_register_device(block_dev_desc_t *dev_desc, int part_no)
 {
 	unsigned char buffer[SECTOR_SIZE];
+#if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) || \
+    (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
+	disk_partition_t info;
+#endif
 
 	if (!dev_desc->block_read)
 		return -1;
-	cur_dev=dev_desc;
+	cur_dev = dev_desc;
 	/* check if we have a MBR (on floppies we have only a PBR) */
 	if (dev_desc->block_read (dev_desc->dev, 0, 1, (ulong *) buffer) != 1) {
 		printf ("** Can't read from device %d **\n", dev_desc->dev);
@@ -83,33 +87,31 @@ fat_register_device(block_dev_desc_t *dev_desc, int part_no)
 		/* no signature found */
 		return -1;
 	}
-	if(!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET],"FAT",3)) {
-		/* ok, we assume we are on a PBR only */
-		cur_part = 1;
-		part_offset=0;
-	}
-	else {
 #if (CONFIG_COMMANDS & CFG_CMD_IDE) || (CONFIG_COMMANDS & CFG_CMD_SCSI) || \
     (CONFIG_COMMANDS & CFG_CMD_USB) || defined(CONFIG_SYSTEMACE)
-		disk_partition_t info;
-		if(!get_partition_info(dev_desc, part_no, &info)) {
-			part_offset = info.start;
-			cur_part = part_no;
-		}
-		else {
-			printf ("** Partition %d not valid on device %d **\n",part_no,dev_desc->dev);
+	if (!get_partition_info (dev_desc, part_no, &info)) {
+		part_offset = info.start;
+		cur_part = part_no;
+	} else {
+		if (!strncmp((char *)&buffer[DOS_FS_TYPE_OFFSET], "FAT", 3)) {
+			/* ok, we assume we are on a PBR only */
+			cur_part = 1;
+			part_offset = 0;
+		} else {
+			printf ("** Partition %d not valid on device %d **\n",
+				part_no, dev_desc->dev);
 			return -1;
 		}
+	}
 #else
-		/* FIXME we need to determine the start block of the
-		 * partition where the DOS FS resides. This can be done
-		 * by using the get_partition_info routine. For this
-		 * purpose the libpart must be included.
-		 */
-		part_offset=32;
-		cur_part = 1;
+	/* FIXME we need to determine the start block of the
+	 * partition where the DOS FS resides. This can be done
+	 * by using the get_partition_info routine. For this
+	 * purpose the libpart must be included.
+	 */
+	part_offset = 32;
+	cur_part = 1;
 #endif
-	}
 	return 0;
 }
 
thanks
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany