| Message ID | 1359589584-19846-1-git-send-email-lkundrak@v3.sk |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
Dear Lubomir Rintel, In message <1359589584-19846-1-git-send-email-lkundrak@v3.sk> you wrote: > In certain cases, memory device is present as flat file or block device (via > mmc or mtdblock layer). Do not attempt MTD operations against it. What exactly would be such cases? > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c > index 37b60b8..0d8052d 100644 > --- a/tools/env/fw_env.c > +++ b/tools/env/fw_env.c > @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, > ioctl (fd, MEMUNLOCK, &erase); > > /* Dataflash does not need an explicit erase cycle */ > - if (mtd_type != MTD_DATAFLASH) > + if (mtd_type && mtd_type != MTD_DATAFLASH) This change appears to be redundant. If mtd_type is null, then this is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it? > - rc = ioctl (fd, MEMGETINFO, &mtdinfo); > + rc = fstat (fd, &st); > if (rc < 0) { > - perror ("Cannot get MTD information"); > + perror ("Cannot access MTD device"); I don't understand this. You talk about a MTD device here, but expect that MEMGETINFO will not work on it? Please explain in which exact circumstances such a situation wouldhappen. > if (mtdinfo.type != MTD_NORFLASH && > mtdinfo.type != MTD_NANDFLASH && > - mtdinfo.type != MTD_DATAFLASH) { > + mtdinfo.type != MTD_DATAFLASH && > + mtdinfo.type) { Again, this last line appears to be redundant. > } > > DEVTYPE(dev_current) = mtdinfo.type; > - > rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE, Please don't make such unrelated white space changes in this commit. > diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config > index 8e21d5a..c086512 100644 > --- a/tools/env/fw_env.config > +++ b/tools/env/fw_env.config > @@ -17,3 +17,6 @@ > > # NAND example > #/dev/mtd0 0x4000 0x4000 0x20000 2 > + > +# Block device example > +#/dev/mmcblk0 0xc0000 0x20000 I don't see why one would use that. Please elucidate. Please also make sure to run your patch through checkpatch - it catches a number of "space prohibited between function name and open parenthesis" warnings and tells you that your Signed-off-by: line is missing. Best regards, Wolfgang Denk
On Thu, 2013-01-31 at 07:48 +0100, Wolfgang Denk wrote: > Dear Lubomir Rintel, > > In message <1359589584-19846-1-git-send-email-lkundrak@v3.sk> you wrote: > > In certain cases, memory device is present as flat file or block device (via > > mmc or mtdblock layer). Do not attempt MTD operations against it. > > What exactly would be such cases? Mine is a Kobo Mini e-book reader, which is basically Freescale MX50 with the only storage there being an MMC/SD card (removable from a slot if disassembled), which contains uboot and its environment as well as partition table, root and storage volume. Apart from wiring a serial console, running fw_* tools seems to be about the only way to modify kernel command line on that device. Also, I can imagine that it would be useful to prepare a flat file image on a different computer and copy it to the image afterwards. > Please also make sure to run your patch through checkpatch - it > catches a number of "space prohibited between function name and open > parenthesis" warnings I tried to stick with the coding style already present in the file. No problem though, I'll follow up with an updated patch later. > and tells you that your Signed-off-by: line is > missing. Noted, will fix. Thank you! Lubo
Dear Lubomir Rintel, In message <1359630144.16475.6.camel@hobbes> you wrote: > > Mine is a Kobo Mini e-book reader, which is basically Freescale MX50 > with the only storage there being an MMC/SD card (removable from a slot > if disassembled), which contains uboot and its environment as well as > partition table, root and storage volume. OK - but you do not comment on my remarks about the actual code? Best regards, Wolfgang Denk
On Thu, 2013-01-31 at 15:10 +0100, Wolfgang Denk wrote: > Dear Lubomir Rintel, > > In message <1359630144.16475.6.camel@hobbes> you wrote: > > > > Mine is a Kobo Mini e-book reader, which is basically Freescale MX50 > > with the only storage there being an MMC/SD card (removable from a slot > > if disassembled), which contains uboot and its environment as well as > > partition table, root and storage volume. > > OK - but you do not comment on my remarks about the actual code? On Thu, 2013-01-31 at 07:48 +0100, Wolfgang Denk wrote: > > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c > > index 37b60b8..0d8052d 100644 > > --- a/tools/env/fw_env.c > > +++ b/tools/env/fw_env.c > > @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, > > ioctl (fd, MEMUNLOCK, &erase); > > > > /* Dataflash does not need an explicit erase cycle */ > > - if (mtd_type != MTD_DATAFLASH) > > + if (mtd_type && mtd_type != MTD_DATAFLASH) > > This change appears to be redundant. If mtd_type is null, then this > is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it? No. We don't want the erase ioctl to be called for non-MTD devices and files (where mtd_type is null). > > - rc = ioctl (fd, MEMGETINFO, &mtdinfo); > > + rc = fstat (fd, &st); > > if (rc < 0) { > > - perror ("Cannot get MTD information"); > > + perror ("Cannot access MTD device"); > > I don't understand this. You talk about a MTD device here, but expect > that MEMGETINFO will not work on it? Please explain in which exact > circumstances such a situation wouldhappen. The error message (mention of MTD at that point) is incorrect. fstat() is called to determine whether the device is a character device (such as MTD devices -- MEMGETINFO call will follow) or not (it might be a blockdevice or flat file). > > if (mtdinfo.type != MTD_NORFLASH && > > mtdinfo.type != MTD_NANDFLASH && > > - mtdinfo.type != MTD_DATAFLASH) { > > + mtdinfo.type != MTD_DATAFLASH && > > + mtdinfo.type) { > > Again, this last line appears to be redundant. The same response again -- if the type is nul, then the device is not a flash device at all. > > > } > > > > DEVTYPE(dev_current) = mtdinfo.type; > > - > > rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE, > > Please don't make such unrelated white space changes in this commit. That was a mistake. Have a nice day! -- Lubo
Dear Lubomir Rintel, In message <1360191866.3594.10.camel@unicorn> you wrote: > > > > - if (mtd_type != MTD_DATAFLASH) > > > + if (mtd_type && mtd_type != MTD_DATAFLASH) > > > > This change appears to be redundant. If mtd_type is null, then this > > is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it? > > No. We don't want the erase ioctl to be called for non-MTD devices and > files (where mtd_type is null). I see. But you are misusing mtd_type. You should define something like MTD_NO_FLASH or so, and use that. > > > - perror ("Cannot get MTD information"); > > > + perror ("Cannot access MTD device"); > > > > I don't understand this. You talk about a MTD device here, but expect > > that MEMGETINFO will not work on it? Please explain in which exact > > circumstances such a situation wouldhappen. > > The error message (mention of MTD at that point) is incorrect. fstat() So it needs to be fixed. > > > if (mtdinfo.type != MTD_NORFLASH && > > > mtdinfo.type != MTD_NANDFLASH && > > > - mtdinfo.type != MTD_DATAFLASH) { > > > + mtdinfo.type != MTD_DATAFLASH && > > > + mtdinfo.type) { > > > > Again, this last line appears to be redundant. > > The same response again -- if the type is nul, then the device is not a > flash device at all. See above. Please make the code sonsistent and define a proper name for this type. Best regards, Wolfgang Denk
On Thu, 2013-02-07 at 00:21 +0100, Wolfgang Denk wrote: > Dear Lubomir Rintel, > > In message <1360191866.3594.10.camel@unicorn> you wrote: > > > > > > - if (mtd_type != MTD_DATAFLASH) > > > > + if (mtd_type && mtd_type != MTD_DATAFLASH) > > > > > > This change appears to be redundant. If mtd_type is null, then this > > > is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it? > > > > No. We don't want the erase ioctl to be called for non-MTD devices and > > files (where mtd_type is null). > > I see. But you are misusing mtd_type. > > You should define something like MTD_NO_FLASH or so, and use that. I found it a bit more abusive to use a MTD_* macro in mtd_type variable for something what is not actually a type of a MTD device, specially when a change in MTD ABI would be needed. Maybe passing a NULL mtd_info_user pointer to flash_{read,write}_buf() instead of zero type (which happens to be MTD_ABSENT, and, as you pointed out, a misuse) would make more sense for a non-MTD file? > > > > > - perror ("Cannot get MTD information"); > > > > + perror ("Cannot access MTD device"); > > > > > > I don't understand this. You talk about a MTD device here, but expect > > > that MEMGETINFO will not work on it? Please explain in which exact > > > circumstances such a situation wouldhappen. > > > > The error message (mention of MTD at that point) is incorrect. fstat() > > So it needs to be fixed. Hopefully fixed in a follow-up patch. I'll follow up with another one (this time with proper changlog and versioning, sorry for that) once an acceptable solution to the issue above is agreed upon. Thanks, -- Lubo
Dear Lubomir, In message <1360244552.29426.9.camel@hobbes> you wrote: > > > You should define something like MTD_NO_FLASH or so, and use that. > > I found it a bit more abusive to use a MTD_* macro in mtd_type variable > for something what is not actually a type of a MTD device, specially > when a change in MTD ABI would be needed. But that's exactly what you are doing here, just in a way that it is not even visible. By assigning a mtd_type value of 0, you are setting it to MTD_ABSENT - but you don't write MTD_ABSENT. This is even more dangerous. > Maybe passing a NULL mtd_info_user pointer to flash_{read,write}_buf() > instead of zero type (which happens to be MTD_ABSENT, and, as you > pointed out, a misuse) would make more sense for a non-MTD file? I don't have any real problems with using MTD_ABSENT - it even makes kind of sense. Best regards, Wolfgang Denk
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 37b60b8..0d8052d 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, ioctl (fd, MEMUNLOCK, &erase); /* Dataflash does not need an explicit erase cycle */ - if (mtd_type != MTD_DATAFLASH) + if (mtd_type && mtd_type != MTD_DATAFLASH) if (ioctl (fd, MEMERASE, &erase) != 0) { fprintf (stderr, "MTD erase error on %s: %s\n", DEVNAME (dev), @@ -949,23 +949,34 @@ static int flash_write (int fd_current, int fd_target, int dev_target) static int flash_read (int fd) { struct mtd_info_user mtdinfo; + struct stat st; int rc; - rc = ioctl (fd, MEMGETINFO, &mtdinfo); + rc = fstat (fd, &st); if (rc < 0) { - perror ("Cannot get MTD information"); + perror ("Cannot access MTD device"); return -1; } + if (S_ISCHR (st.st_mode)) { + rc = ioctl (fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + perror ("Cannot get MTD information"); + return -1; + } + } else { + memset (&mtdinfo, 0, sizeof (mtdinfo)); + } + if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH && - mtdinfo.type != MTD_DATAFLASH) { + mtdinfo.type != MTD_DATAFLASH && + mtdinfo.type) { fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); return -1; } DEVTYPE(dev_current) = mtdinfo.type; - rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE, DEVOFFSET (dev_current), mtdinfo.type); diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config index 8e21d5a..c086512 100644 --- a/tools/env/fw_env.config +++ b/tools/env/fw_env.config @@ -17,3 +17,6 @@ # NAND example #/dev/mtd0 0x4000 0x4000 0x20000 2 + +# Block device example +#/dev/mmcblk0 0xc0000 0x20000