Hi, Leon KUKOVEC wrote:

Hi Wolfgang,

On Mon, 2004-03-15 at 14:32, Wolfgang Denk wrote:

...

In message 4055AA27.3010805@staubli.com you wrote:

...

The attached patch fixes a bug in the ide identification string copy.

I may be missing something - but why do we copy these strings in the first place? Who is using them? (and where?)

The block_dev_desc_t structure defines vendor[40], product[20] and revision[8] - same as it is defined in hd_driveid_t.

ident_cpy's description says that it will copy the string and terminate it, but it can't in the case where all 8 bytes of revision are occupied - since the buffers are equal size.

My hard disk has a 8 bytes revision string. In this case, u-boot crashes in init_part because the block_read field of the block_dev_desc_t structure has been partially erased by the ending null of the revision string. This crash didn't occured with the revison 1.5 of include/part.h because the field removable was beetween fields revision and block_read.

Therefore Pierre's patch is correct, but instead of hardcoded values 8, 40, 20 I would rather see sizeof (iop->fw_rev) etc.

The attached patch replaces my previous with this modification.

I noticed the same thing today when I was testing the PCMCIA patch that I'm about to send for PXA.

Best regards

diff -aur u-boot/common/cmd_ide.c u-boot-fix/common/cmd_ide.c --- u-boot/common/cmd_ide.c 2004-03-14 23:25:44.000000000 +0100 +++ u-boot-fix/common/cmd_ide.c 2004-03-16 08:55:24.000000000 +0100 @@ -1098,9 +1098,9 @@

input_swap_data (device, iobuf, ATA_SECTORWORDS);

- ident_cpy (dev_desc->revision, iop->fw_rev, sizeof(dev_desc->revision)); - ident_cpy (dev_desc->vendor, iop->model, sizeof(dev_desc->vendor)); - ident_cpy (dev_desc->product, iop->serial_no, sizeof(dev_desc->product)); + ident_cpy (dev_desc->revision, iop->fw_rev, sizeof (iop->fw_rev)); + ident_cpy (dev_desc->vendor, iop->model, sizeof (iop->model)); + ident_cpy (dev_desc->product, iop->serial_no, sizeof (iop->serial_no)); #ifdef __LITTLE_ENDIAN /* * firmware revision and model number have Big Endian Byte diff -aur u-boot/include/part.h u-boot-fix/include/part.h --- u-boot/include/part.h 2004-03-14 23:25:50.000000000 +0100 +++ u-boot-fix/include/part.h 2004-03-15 14:03:58.000000000 +0100 @@ -37,9 +37,9 @@ #endif unsigned long lba; /* number of blocks */ unsigned long blksz; /* block size */ - unsigned char vendor[40]; /* IDE model, SCSI Vendor */ - unsigned char product[20]; /* IDE Serial no, SCSI product */ - unsigned char revision[8]; /* firmware revision */ + unsigned char vendor[41]; /* IDE model, SCSI Vendor */ + unsigned char product[21]; /* IDE Serial no, SCSI product */ + unsigned char revision[9]; /* firmware revision */ unsigned long (*block_read)(int dev, unsigned long start, unsigned long blkcnt,

Hi again Wolfgang,

The IDE ident problem overflow problem isn't solved in the CVS. If a string has trailing whitespaces, they're not always suppressed. It depends on the character following the end of the source string.

The attached patch fixes that.

CHANGELOG: * Patch by Pierre Aubert 18 Mar 2004 - Fix string cleaning in IDE identification

--- u-boot/common/cmd_ide.c 2004-03-17 02:13:08.000000000 +0100 +++ u-boot-paub/common/cmd_ide.c 2004-03-18 17:25:11.000000000 +0100 @@ -1417,7 +1417,7 @@ unsigned char *end, *last;

last = dst; - end = src + len; + end = src + len - 1;

/* reserve space for '\0' */ if (len < 2)