Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Lukasz, In message <20120727153345.008fde41@amdc308.digital.local> you wrote: > > So I suppose, that you are proposing something like this (pseudo code): Yes, very close. > int mmc_block_write() { > sprintf(cmd_buf, "mmc write 0x%x %x %x") > run_command(cmd_buf, 0); > } > > int mmc_file_write(enum fs_type) { > switch (fs_type) > case FAT: > sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx") > break; > case EXT4: > sprintf(cmd_buf, "ext4write mmc %d:%d 0x%x %s %lx") > break; > run_command(cmd_buf); > } > int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len) > { I suggest just use int dfu_write_medium(struct dfu_entity *dfu, void *buf, long *len) here; you can then so something like switch (dfu->dev_type) { case MMC: block_write = mmc_block_write; file_write = mmc_file_write; break; #ifdef NAND_SUPPORT_AVAILABLE case NAND: block_write = nand_block_write; file_write = nand_block_write; break; #endif ... and use block_write() resp. file_write() in the rest. So the code is really trivial to extend for other storage devices and file systems. I feel we are very close, thanks! Let's see if Pavel adds some comments about the best API to chose to be as compatible with possible with the upcoming block device layer, and then we can go for it. 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: w...@denx.de Houston, Tranquillity Base here. The Eagle has landed. -- Neil Armstrong ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Marek Vasut, In message <201207271515.36085.ma...@denx.de> you wrote: > > > sprintf(cmd_buf, "%s %s %d:%d 0x%x %s %lx", > > command, device, ... > > > > and set command as needed to "fatread" or "ext2load" or whatever, and > > device can be set to "mmc" or "usb" or ... > > Given that they have slightly different syntax, this is crazy too. Draw the line between clever and crazy, genius and idiot :-) > Well ... after this discussion, I feel like I'll just go dig me a grave again. Why? I think the discussion has been beneficial - we isolated a poor piece of code, discussed how it should be improved, and now ry to find an intermediate solution that adds not too much work for Lukasz while still keeing the existing code clean and allowing for easy adaption to the new DM interfaces - three worthwile goals at once. 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: w...@denx.de Sorry, but my karma just ran over your dogma. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Wolfgang Denk, > Dear Lukasz Majewski, > > In message <1341416922-13792-5-git-send-email-l.majew...@samsung.com> > you wrote: > > Support for MMC storage devices to work with DFU framework. > > Sorry for jumping in late. > > > > + switch (dfu->layout) { > > + case RAW_ADDR: > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned > > int) buf, > > + dfu->data.mmc.lba_start, > > dfu->data.mmc.lba_size); > > + break; > > + case FAT: > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > + (unsigned int) buf, dfu->name, *len); > > + break; > > + default: > > + printf("%s: Wrong layout!\n", __func__); > > + } > > In case of error, you should always print what the unexpected data > was. The end user who receives an "Wrong layout!" error is probably > pretty much surpised and doesn't know what he did wrong. If you > print instead: "EXT2 layout not supported (yet)" he would know exactly > what to change to make it work. Ok, this can be easily corrected. > > > There has been some dicussion already if using this sprintf() / > run_command() approach is good or not, or how it should be fixed. > > It appears, all this discussion forgot to take into account that > patch 3/7 dfu: DFU backend implementation promised to add "platform > and storage independent operation of DFU." Here we are breaking this > promise. > > And by adding special functions to the FAT file system code thing sget > just worse, as now not only the DFU code needs to be extended when we > want to use this with any other file system type, but suddenly this > also bleeds into all supported file system code. > > > I am aware that the current implementation suffers from the fact that > we don't have a unified access to file systems - each comes with it's > own set of commands and more or less (in)compatible command line and > function call interfaces. This will hopefully improve in the context > of the device model rework, but we don't want to block you until then. > But considering that this is going to change in a foreseeable future, > it also makes littel sense to invest big efforts into generic code > that covers all potential future uses. > > Here is my poposal: > > Let's introduce a thin shim layer to abstract the file system > interface from the accesses you need here. As far as I can see here, > you need exactly 4 functions: > > - block_read() > - block_write() > - file_read() > - file_write() > > These names could be function pointers to implement the device I/O in > a both device and file system agnostic way; you can implement > specific versions of the functions like mmc_block_read(), > usb_block_read(), ... etc. and use the settings of dfu_device_type > and dfu_layout to set the pointers to these functions. > > For this shim layer I would actually prefer the original approach > (shown above) to use sprintf() to build commands based on the existing > command interface - this is the minimal intrusive way to implement > this for now. With this approach, no modifications to file system > and/or device driver code are needed, and we still maintain full > flexibility here in the DFU code. Yes, error checking may not be > perfect, and we my not win a price for elegance of design either, but > we should get mostly clean, working code quickly. > So I suppose, that you are proposing something like this (pseudo code): int mmc_block_write() { sprintf(cmd_buf, "mmc write 0x%x %x %x") run_command(cmd_buf, 0); } int mmc_file_write(enum fs_type) { switch (fs_type) case FAT: sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx") break; case EXT4: sprintf(cmd_buf, "ext4write mmc %d:%d 0x%x %s %lx") break; run_command(cmd_buf); } int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len) { ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE); memset(cmd_buf, '\0', sizeof(cmd_buf)); switch (dfu->layout) { case RAW_ADDR: mmc_block_write() break; case FAT: case EXT3: case EXT4: mmc_file_write(dfu->layout); break; default: printf("%s: Wrong layout %s!\n", __func__, layout_table[dfu->layout]); } return 0; } > > The advantage I see for this code is that we have separated all > this device interface stuff from both the generic DFU code and from > the device and file system code as well. As soon as we have a better > implementation below, all we need to adjust (or potentially even > remove) is this shim layer. > -- Best regards, Lukasz Majewski Samsung Poland R&D Center | Linux Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http:
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Wolfgang Denk, > Dear Marek Vasut, > > In message <201207271443.45189.ma...@denx.de> you wrote: > > > For this shim layer I would actually prefer the original approach > > > (shown above) to use sprintf() to build commands based on the existing > > > command interface > > > > We discussed it for a while ... figure out the missing typechecking and > > abuse of the command line interface is not a way to go. > > I agree that this is not the preferred way for a permanent solution, > but for now I cxonsider it acceptable, as it allows to easily deal > with the inconsistent API of the misc file system handlers. For > example instead of > > sprintf(cmd_buf, "fatread mmc %d:%d 0x%x %s %lx", ... > > we can even do something like > > sprintf(cmd_buf, "%s %s %d:%d 0x%x %s %lx", > command, device, ... > > and set command as needed to "fatread" or "ext2load" or whatever, and > device can be set to "mmc" or "usb" or ... Given that they have slightly different syntax, this is crazy too. > > > this is the minimal intrusive way to implement > > > this for now. With this approach, no modifications to file system > > > and/or device driver code are needed, and we still maintain full > > > flexibility here in the DFU code. Yes, error checking may not be > > > perfect, and we my not win a price for elegance of design either, but > > > we should get mostly clean, working code quickly. > > > > But now that we started going in the typechecking way, I'd prefer to go > > all the way. And once DFU extends properly towards other supported > > filesystems/devices, the rest of the interface can get cleaned along the > > way. > > You can implement the type checking code when you hav ea stable, well > define API for storage devices and file systems. For now we don't > have that, and it makes ZERO sense to add special code just for that > just to throw it away in a few weeks. Ad API -- adding Pavel to CC. Hope his GMail works better than mine (mine doesn't, crashed again, damned google stuff). Pavel, can you provide us with how the API will look maybe? > > Shim layer is cool -- abusing command line is not cool ;-) > > Whether this is abuse or clever use remains to be defined, and this > is probably largely a matter of taste. In any case, this will be a > small piece of code that is of clearly transient nature, to be > replaced or removed as soon as we can do better. > > Best regards, > > Wolfgang Denk Well ... after this discussion, I feel like I'll just go dig me a grave again. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Marek Vasut, In message <201207271443.45189.ma...@denx.de> you wrote: > > > For this shim layer I would actually prefer the original approach > > (shown above) to use sprintf() to build commands based on the existing > > command interface > > We discussed it for a while ... figure out the missing typechecking and abuse > of > the command line interface is not a way to go. I agree that this is not the preferred way for a permanent solution, but for now I cxonsider it acceptable, as it allows to easily deal with the inconsistent API of the misc file system handlers. For example instead of sprintf(cmd_buf, "fatread mmc %d:%d 0x%x %s %lx", ... we can even do something like sprintf(cmd_buf, "%s %s %d:%d 0x%x %s %lx", command, device, ... and set command as needed to "fatread" or "ext2load" or whatever, and device can be set to "mmc" or "usb" or ... > > this is the minimal intrusive way to implement > > this for now. With this approach, no modifications to file system > > and/or device driver code are needed, and we still maintain full > > flexibility here in the DFU code. Yes, error checking may not be > > perfect, and we my not win a price for elegance of design either, but > > we should get mostly clean, working code quickly. > > But now that we started going in the typechecking way, I'd prefer to go all > the > way. And once DFU extends properly towards other supported > filesystems/devices, > the rest of the interface can get cleaned along the way. You can implement the type checking code when you hav ea stable, well define API for storage devices and file systems. For now we don't have that, and it makes ZERO sense to add special code just for that just to throw it away in a few weeks. > Shim layer is cool -- abusing command line is not cool ;-) Whether this is abuse or clever use remains to be defined, and this is probably largely a matter of taste. In any case, this will be a small piece of code that is of clearly transient nature, to be replaced or removed as soon as we can do better. 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: w...@denx.de The nice thing about standards is that there are so many to choose from. - Andrew S. Tanenbaum ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Wolfgang Denk, > Dear Lukasz Majewski, > > In message <1341416922-13792-5-git-send-email-l.majew...@samsung.com> you wrote: > > Support for MMC storage devices to work with DFU framework. > > Sorry for jumping in late. > > > + switch (dfu->layout) { > > + case RAW_ADDR: > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf, > > + dfu->data.mmc.lba_start, dfu->data.mmc.lba_size); > > + break; > > + case FAT: > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > + (unsigned int) buf, dfu->name, *len); > > + break; > > + default: > > + printf("%s: Wrong layout!\n", __func__); > > + } > > In case of error, you should always print what the unexpected data > was. The end user who receives an "Wrong layout!" error is probably > pretty much surpised and doesn't know what he did wrong. If you > print instead: "EXT2 layout not supported (yet)" he would know exactly > what to change to make it work. > > > There has been some dicussion already if using this sprintf() / > run_command() approach is good or not, or how it should be fixed. > > It appears, all this discussion forgot to take into account that > patch 3/7 dfu: DFU backend implementation promised to add "platform > and storage independent operation of DFU." Here we are breaking this > promise. > > And by adding special functions to the FAT file system code thing sget > just worse, as now not only the DFU code needs to be extended when we > want to use this with any other file system type, but suddenly this > also bleeds into all supported file system code. > > > I am aware that the current implementation suffers from the fact that > we don't have a unified access to file systems - each comes with it's > own set of commands and more or less (in)compatible command line and > function call interfaces. This will hopefully improve in the context > of the device model rework, but we don't want to block you until then. > But considering that this is going to change in a foreseeable future, > it also makes littel sense to invest big efforts into generic code > that covers all potential future uses. > > Here is my poposal: > > Let's introduce a thin shim layer to abstract the file system > interface from the accesses you need here. As far as I can see here, > you need exactly 4 functions: > > - block_read() > - block_write() > - file_read() > - file_write() > > These names could be function pointers to implement the device I/O in > a both device and file system agnostic way; you can implement > specific versions of the functions like mmc_block_read(), > usb_block_read(), ... etc. and use the settings of dfu_device_type > and dfu_layout to set the pointers to these functions. > > For this shim layer I would actually prefer the original approach > (shown above) to use sprintf() to build commands based on the existing > command interface We discussed it for a while ... figure out the missing typechecking and abuse of the command line interface is not a way to go. > this is the minimal intrusive way to implement > this for now. With this approach, no modifications to file system > and/or device driver code are needed, and we still maintain full > flexibility here in the DFU code. Yes, error checking may not be > perfect, and we my not win a price for elegance of design either, but > we should get mostly clean, working code quickly. But now that we started going in the typechecking way, I'd prefer to go all the way. And once DFU extends properly towards other supported filesystems/devices, the rest of the interface can get cleaned along the way. > The advantage I see for this code is that we have separated all > this device interface stuff from both the generic DFU code and from > the device and file system code as well. As soon as we have a better > implementation below, all we need to adjust (or potentially even > remove) is this shim layer. Shim layer is cool -- abusing command line is not cool ;-) > Best regards, > > Wolfgang Denk ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Lukasz Majewski, In message <1341416922-13792-5-git-send-email-l.majew...@samsung.com> you wrote: > Support for MMC storage devices to work with DFU framework. Sorry for jumping in late. > + switch (dfu->layout) { > + case RAW_ADDR: > + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf, > + dfu->data.mmc.lba_start, dfu->data.mmc.lba_size); > + break; > + case FAT: > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", > + dfu->data.mmc.dev, dfu->data.mmc.part, > + (unsigned int) buf, dfu->name, *len); > + break; > + default: > + printf("%s: Wrong layout!\n", __func__); > + } In case of error, you should always print what the unexpected data was. The end user who receives an "Wrong layout!" error is probably pretty much surpised and doesn't know what he did wrong. If you print instead: "EXT2 layout not supported (yet)" he would know exactly what to change to make it work. There has been some dicussion already if using this sprintf() / run_command() approach is good or not, or how it should be fixed. It appears, all this discussion forgot to take into account that patch 3/7 dfu: DFU backend implementation promised to add "platform and storage independent operation of DFU." Here we are breaking this promise. And by adding special functions to the FAT file system code thing sget just worse, as now not only the DFU code needs to be extended when we want to use this with any other file system type, but suddenly this also bleeds into all supported file system code. I am aware that the current implementation suffers from the fact that we don't have a unified access to file systems - each comes with it's own set of commands and more or less (in)compatible command line and function call interfaces. This will hopefully improve in the context of the device model rework, but we don't want to block you until then. But considering that this is going to change in a foreseeable future, it also makes littel sense to invest big efforts into generic code that covers all potential future uses. Here is my poposal: Let's introduce a thin shim layer to abstract the file system interface from the accesses you need here. As far as I can see here, you need exactly 4 functions: - block_read() - block_write() - file_read() - file_write() These names could be function pointers to implement the device I/O in a both device and file system agnostic way; you can implement specific versions of the functions like mmc_block_read(), usb_block_read(), ... etc. and use the settings of dfu_device_type and dfu_layout to set the pointers to these functions. For this shim layer I would actually prefer the original approach (shown above) to use sprintf() to build commands based on the existing command interface - this is the minimal intrusive way to implement this for now. With this approach, no modifications to file system and/or device driver code are needed, and we still maintain full flexibility here in the DFU code. Yes, error checking may not be perfect, and we my not win a price for elegance of design either, but we should get mostly clean, working code quickly. The advantage I see for this code is that we have separated all this device interface stuff from both the generic DFU code and from the device and file system code as well. As soon as we have a better implementation below, all we need to adjust (or potentially even remove) is this shim layer. 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: w...@denx.de To the systems programmer, users and applications serve only to provide a test load. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
>> > Generally it is in my opinion a good way to go. >> > >> > However, why we aren't first writing sanity checks for passed arguments? >> >> Simply because I didn't want to ask you to do a lot more unrelated work >> >> :) If you want to split and check the mmc (and fatwrite) argueuments >> >> and then make the DFU series depend on that, by all means please do so! > > Would be cool indeed. > >> > We are adding one more level of abstraction, but don't think of the main >> > problem (checking values of passed arguments)? >> > >> > Anyway we shall wait for Marek's opinion. >> >> Yes, a good idea as well. > > My opinion is that if you'll do the sanity checks, that'd be good. We're right > before .07 release anyway, so the patches will hit the next merge window. Are > you up for doing a lot of unrelated work to make this proper? I agree with the general sense that adding sanity checking would be good. Just because I was too lazy to add them, doesn't mean I was right. ;) Andy ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Tom Rini, > On Thu, Jul 12, 2012 at 02:39:27PM +0200, Lukasz Majewski wrote: > > On Wed, 11 Jul 2012 04:54:31 -0700 > > > > Tom Rini wrote: > > > On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote: > > > > Hi Tom, > > > > > > > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote: > > > > > > Support for MMC storage devices to work with DFU framework. > > > > > > > > > > > > Signed-off-by: Lukasz Majewski > > > > > > Signed-off-by: Kyungmin Park > > > > > > Cc: Marek Vasut > > > > > > > > > > [snip] > > > > > > > > > > > + case RAW_ADDR: > > > > > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", > > > > > > (unsigned int) buf, > > > > > > + dfu->data.mmc.lba_start, > > > > > > dfu->data.mmc.lba_size); > > > > > > + break; > > > > > > + case FAT: > > > > > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s > > > > > > %lx", > > > > > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > > > > > + (unsigned int) buf, dfu->name, *len); > > > > > > + break; > > > > > > + default: > > > > > > + printf("%s: Wrong layout!\n", __func__); > > > > > > + } > > > > > > + > > > > > > + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > > > > > > + run_command(cmd_buf, 0); > > > > > > > > > > If we try and take the long-view here, that fatwrite/mmc write > > > > > don't perform a lot of sanity checking on input isn't good. Lots > > > > > of commands I believe don't, but we can start somewhere. > > > > > > > > Yes, indeed they don't. But I think, that it is a deeper problem. > > > > > > > > When one looks into the cmd_mmc.c, the code is not checking the > > > > correctness of passed data. It performs strncmp, then simple_strtoul > > > > and with this parameter calls mmc->block_dev.block_read(). > > > > > > > > But I'm a bit concern if adding function: > > > > > > > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) > > > > to > > > > > > > > do_mmcops(argc, argv) { > > > > > > > > int i = simple_strtol(argv[]); > > > > return do_mmcops_check(i); > > > > > > > > } > > > > > > Well, what I was suggesting would be: > > > > > > do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. } > > > do_mmcops_from_cmd(argc, argv) { > > > > > > ... convert user input today, maybe try and sanity check input > > > > > > tomorrow .. > > > } > > > > > > And then dfu calls do_mmcops_real(lba_start, ...). A further clean-up > > > would be to make the interface the command uses to perform checking of > > > the arguments passed. Does this make sense? > > > > Generally it is in my opinion a good way to go. > > > > However, why we aren't first writing sanity checks for passed arguments? > > Simply because I didn't want to ask you to do a lot more unrelated work > > :) If you want to split and check the mmc (and fatwrite) argueuments > > and then make the DFU series depend on that, by all means please do so! Would be cool indeed. > > We are adding one more level of abstraction, but don't think of the main > > problem (checking values of passed arguments)? > > > > Anyway we shall wait for Marek's opinion. > > Yes, a good idea as well. My opinion is that if you'll do the sanity checks, that'd be good. We're right before .07 release anyway, so the patches will hit the next merge window. Are you up for doing a lot of unrelated work to make this proper? Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
On Thu, Jul 12, 2012 at 02:39:27PM +0200, Lukasz Majewski wrote: > On Wed, 11 Jul 2012 04:54:31 -0700 > Tom Rini wrote: > > > On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote: > > > Hi Tom, > > > > > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote: > > > > > Support for MMC storage devices to work with DFU framework. > > > > > > > > > > Signed-off-by: Lukasz Majewski > > > > > Signed-off-by: Kyungmin Park > > > > > Cc: Marek Vasut > > > > [snip] > > > > > + case RAW_ADDR: > > > > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", > > > > > (unsigned int) buf, > > > > > + dfu->data.mmc.lba_start, > > > > > dfu->data.mmc.lba_size); > > > > > + break; > > > > > + case FAT: > > > > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s > > > > > %lx", > > > > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > > > > + (unsigned int) buf, dfu->name, *len); > > > > > + break; > > > > > + default: > > > > > + printf("%s: Wrong layout!\n", __func__); > > > > > + } > > > > > + > > > > > + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > > > > > + run_command(cmd_buf, 0); > > > > > > > > If we try and take the long-view here, that fatwrite/mmc write > > > > don't perform a lot of sanity checking on input isn't good. Lots > > > > of commands I believe don't, but we can start somewhere. > > > Yes, indeed they don't. But I think, that it is a deeper problem. > > > > > > When one looks into the cmd_mmc.c, the code is not checking the > > > correctness of passed data. It performs strncmp, then simple_strtoul > > > and with this parameter calls mmc->block_dev.block_read(). > > > > > > But I'm a bit concern if adding function: > > > > > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) > > > to > > > > > > do_mmcops(argc, argv) { > > > int i = simple_strtol(argv[]); > > > return do_mmcops_check(i); > > > } > > > > Well, what I was suggesting would be: > > > > do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. } > > do_mmcops_from_cmd(argc, argv) { > > ... convert user input today, maybe try and sanity check input > > tomorrow .. > > } > > > > And then dfu calls do_mmcops_real(lba_start, ...). A further clean-up > > would be to make the interface the command uses to perform checking of > > the arguments passed. Does this make sense? > > > > Generally it is in my opinion a good way to go. > > However, why we aren't first writing sanity checks for passed arguments? Simply because I didn't want to ask you to do a lot more unrelated work :) If you want to split and check the mmc (and fatwrite) argueuments and then make the DFU series depend on that, by all means please do so! > We are adding one more level of abstraction, but don't think of the main > problem (checking values of passed arguments)? > > Anyway we shall wait for Marek's opinion. Yes, a good idea as well. -- Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
On Wed, 11 Jul 2012 04:54:31 -0700 Tom Rini wrote: > On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote: > > Hi Tom, > > > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote: > > > > Support for MMC storage devices to work with DFU framework. > > > > > > > > Signed-off-by: Lukasz Majewski > > > > Signed-off-by: Kyungmin Park > > > > Cc: Marek Vasut > > > [snip] > > > > + case RAW_ADDR: > > > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", > > > > (unsigned int) buf, > > > > + dfu->data.mmc.lba_start, > > > > dfu->data.mmc.lba_size); > > > > + break; > > > > + case FAT: > > > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s > > > > %lx", > > > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > > > + (unsigned int) buf, dfu->name, *len); > > > > + break; > > > > + default: > > > > + printf("%s: Wrong layout!\n", __func__); > > > > + } > > > > + > > > > + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > > > > + run_command(cmd_buf, 0); > > > > > > If we try and take the long-view here, that fatwrite/mmc write > > > don't perform a lot of sanity checking on input isn't good. Lots > > > of commands I believe don't, but we can start somewhere. > > Yes, indeed they don't. But I think, that it is a deeper problem. > > > > When one looks into the cmd_mmc.c, the code is not checking the > > correctness of passed data. It performs strncmp, then simple_strtoul > > and with this parameter calls mmc->block_dev.block_read(). > > > > But I'm a bit concern if adding function: > > > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) > > to > > > > do_mmcops(argc, argv) { > > int i = simple_strtol(argv[]); > > return do_mmcops_check(i); > > } > > Well, what I was suggesting would be: > > do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. } > do_mmcops_from_cmd(argc, argv) { > ... convert user input today, maybe try and sanity check input > tomorrow .. > } > > And then dfu calls do_mmcops_real(lba_start, ...). A further clean-up > would be to make the interface the command uses to perform checking of > the arguments passed. Does this make sense? > Generally it is in my opinion a good way to go. However, why we aren't first writing sanity checks for passed arguments? We are adding one more level of abstraction, but don't think of the main problem (checking values of passed arguments)? Anyway we shall wait for Marek's opinion. -- Best regards, Lukasz Majewski Samsung Poland R&D Center | Linux Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote: > Hi Tom, > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote: > > > Support for MMC storage devices to work with DFU framework. > > > > > > Signed-off-by: Lukasz Majewski > > > Signed-off-by: Kyungmin Park > > > Cc: Marek Vasut > > [snip] > > > + case RAW_ADDR: > > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned > > > int) buf, > > > + dfu->data.mmc.lba_start, > > > dfu->data.mmc.lba_size); > > > + break; > > > + case FAT: > > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", > > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > > + (unsigned int) buf, dfu->name, *len); > > > + break; > > > + default: > > > + printf("%s: Wrong layout!\n", __func__); > > > + } > > > + > > > + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > > > + run_command(cmd_buf, 0); > > > > If we try and take the long-view here, that fatwrite/mmc write don't > > perform a lot of sanity checking on input isn't good. Lots of > > commands I believe don't, but we can start somewhere. > Yes, indeed they don't. But I think, that it is a deeper problem. > > When one looks into the cmd_mmc.c, the code is not checking the > correctness of passed data. It performs strncmp, then simple_strtoul > and with this parameter calls mmc->block_dev.block_read(). > > But I'm a bit concern if adding function: > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) to > > do_mmcops(argc, argv) { > int i = simple_strtol(argv[]); > return do_mmcops_check(i); > } Well, what I was suggesting would be: do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. } do_mmcops_from_cmd(argc, argv) { ... convert user input today, maybe try and sanity check input tomorrow .. } And then dfu calls do_mmcops_real(lba_start, ...). A further clean-up would be to make the interface the command uses to perform checking of the arguments passed. Does this make sense? -- Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Hi Tom, > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote: > > Support for MMC storage devices to work with DFU framework. > > > > Signed-off-by: Lukasz Majewski > > Signed-off-by: Kyungmin Park > > Cc: Marek Vasut > [snip] > > + case RAW_ADDR: > > + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned > > int) buf, > > + dfu->data.mmc.lba_start, > > dfu->data.mmc.lba_size); > > + break; > > + case FAT: > > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", > > + dfu->data.mmc.dev, dfu->data.mmc.part, > > + (unsigned int) buf, dfu->name, *len); > > + break; > > + default: > > + printf("%s: Wrong layout!\n", __func__); > > + } > > + > > + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > > + run_command(cmd_buf, 0); > > If we try and take the long-view here, that fatwrite/mmc write don't > perform a lot of sanity checking on input isn't good. Lots of > commands I believe don't, but we can start somewhere. Yes, indeed they don't. But I think, that it is a deeper problem. When one looks into the cmd_mmc.c, the code is not checking the correctness of passed data. It performs strncmp, then simple_strtoul and with this parameter calls mmc->block_dev.block_read(). But I'm a bit concern if adding function: do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) to do_mmcops(argc, argv) { int i = simple_strtol(argv[]); return do_mmcops_check(i); } will help with preventing errors. As I've written previously, data from prompt is passed in a form of text, which is converted to the well defined type anyway (with e.g. simple_strtoul - returns unsigned long, strict_strtoul - returns int, simple_strtol - returns long, simple_strtoull - returns unsigned long long). Using those functions correctly would ensure correct types passed to e.g. mmc->block_dev.block_read(). When one create the do_mmcops_check() function, the compiler would check if its arguments are correct (i.e. the i in the above example is int). What is the difference between checking at compile time the output of simple_strtoul? The real problem in my opinion is the lack of checking if arguments passed as text to the do_mmcops are correct or not. This is not done for MMC and I doubt, if adding compile time type checking (in a form of a separate function) would solve/alleviate the problem. > So, lets do > what Marek was suggesting of making common/cmd_mmc.c and > common/cmd_fat.c call a sub-function that takes compile-time > typecheckable inputs, and call that here. That opens things up for > later making the user commands perform better checking and so forth. I'd like to point to the problem with passing and then parsing parameters as text. User typed parameters aren't checked. Please correct me if I misunderstood the problem or the proposed solution. -- Best regards, Lukasz Majewski Samsung Poland R&D Center | Linux Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote: > Support for MMC storage devices to work with DFU framework. > > Signed-off-by: Lukasz Majewski > Signed-off-by: Kyungmin Park > Cc: Marek Vasut [snip] > + case RAW_ADDR: > + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf, > + dfu->data.mmc.lba_start, dfu->data.mmc.lba_size); > + break; > + case FAT: > + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", > + dfu->data.mmc.dev, dfu->data.mmc.part, > + (unsigned int) buf, dfu->name, *len); > + break; > + default: > + printf("%s: Wrong layout!\n", __func__); > + } > + > + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); > + run_command(cmd_buf, 0); If we try and take the long-view here, that fatwrite/mmc write don't perform a lot of sanity checking on input isn't good. Lots of commands I believe don't, but we can start somewhere. So, lets do what Marek was suggesting of making common/cmd_mmc.c and common/cmd_fat.c call a sub-function that takes compile-time typecheckable inputs, and call that here. That opens things up for later making the user commands perform better checking and so forth. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Dear Lukasz Majewski, > Support for MMC storage devices to work with DFU framework. > > Signed-off-by: Lukasz Majewski > Signed-off-by: Kyungmin Park > Cc: Marek Vasut > This one opens some questions. [...] > +int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s) > +{ > + char *st = NULL; > + int n = 0; > + > + dfu->dev_type = MMC; > + st = dfu_extract_token(&s, &n); > + > + if (!strncmp(st, "mmc", n)) { > + dfu->layout = RAW_ADDR; > + > + dfu->data.mmc.lba_start = simple_strtoul(s, &s, 16); > + dfu->data.mmc.lba_size = simple_strtoul(++s, &s, 16); > + dfu->data.mmc.lba_blk_size = get_mmc_blk_size(dfu->dev_num); > + ^ one line too much. > + } else if (!strncmp(st, "fat", n)) { > + dfu->layout = FAT; > + > + dfu->data.mmc.dev = simple_strtoul(s, &s, 10); > + dfu->data.mmc.part = simple_strtoul(++s, &s, 10); > + > + } else { > + printf("%s: Wrong memory layout!\n", __func__); > + } > + > + dfu->read_medium = dfu_read_medium_mmc; > + dfu->write_medium = dfu_write_medium_mmc; > + > + return 0; > +} Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation
Support for MMC storage devices to work with DFU framework. Signed-off-by: Lukasz Majewski Signed-off-by: Kyungmin Park Cc: Marek Vasut --- Changes for v2: - None --- drivers/dfu/Makefile |1 + drivers/dfu/dfu_mmc.c | 126 + 2 files changed, 127 insertions(+), 0 deletions(-) create mode 100644 drivers/dfu/dfu_mmc.c diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile index 7736485..7b717bc 100644 --- a/drivers/dfu/Makefile +++ b/drivers/dfu/Makefile @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB= $(obj)libdfu.o COBJS-$(CONFIG_DFU_FUNCTION) += dfu.o +COBJS-$(CONFIG_DFU_MMC) += dfu_mmc.o SRCS:= $(COBJS-y:.o=.c) OBJS := $(addprefix $(obj),$(COBJS-y)) diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c new file mode 100644 index 000..3151fbc --- /dev/null +++ b/drivers/dfu/dfu_mmc.c @@ -0,0 +1,126 @@ +/* + * dfu.c -- DFU back-end routines + * + * Copyright (C) 2012 Samsung Electronics + * authors: Andrzej Pietrasiewicz + * Lukasz Majewski + * + * 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 +#include +#include + +int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len) +{ + ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE); + + memset(cmd_buf, '\0', sizeof(cmd_buf)); + + switch (dfu->layout) { + case RAW_ADDR: + sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf, + dfu->data.mmc.lba_start, dfu->data.mmc.lba_size); + break; + case FAT: + sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx", + dfu->data.mmc.dev, dfu->data.mmc.part, + (unsigned int) buf, dfu->name, *len); + break; + default: + printf("%s: Wrong layout!\n", __func__); + } + + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); + run_command(cmd_buf, 0); + + return 0; +} + +int dfu_read_medium_mmc(struct dfu_entity *dfu, void *buf, long *len) +{ + ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE); + char *str_env = NULL; + int ret = 0; + + memset(cmd_buf, '\0', sizeof(cmd_buf)); + + switch (dfu->layout) { + case RAW_ADDR: + sprintf(cmd_buf, "mmc read 0x%x %x %x", (unsigned int) buf, + dfu->data.mmc.lba_start, dfu->data.mmc.lba_size); + + *len = dfu->data.mmc.lba_blk_size * dfu->data.mmc.lba_size; + break; + case FAT: + sprintf(cmd_buf, "fatload mmc %d:%d 0x%x %s", + dfu->data.mmc.dev, dfu->data.mmc.part, + (unsigned int) buf, dfu->name); + break; + default: + printf("%s: Wrong layout!\n", __func__); + } + + debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf); + + ret = run_command(cmd_buf, 0); + if (ret) { + puts("dfu: Read error!\n"); + return ret; + } + + if (dfu->layout != RAW_ADDR) { + str_env = getenv("filesize"); + if (str_env == NULL) { + puts("dfu: Wrong file size!\n"); + return -1; + } + + *len = simple_strtoul(str_env, NULL, 16); + } + return ret; +} + +int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s) +{ + char *st = NULL; + int n = 0; + + dfu->dev_type = MMC; + st = dfu_extract_token(&s, &n); + + if (!strncmp(st, "mmc", n)) { + dfu->layout = RAW_ADDR; + + dfu->data.mmc.lba_start = simple_strtoul(s, &s, 16); + dfu->data.mmc.lba_size = simple_strtoul(++s, &s, 16); + dfu->data.mmc.lba_blk_size = get_mmc_blk_size(dfu->dev_num); + + } else if (!strncmp(st, "fat", n)) { + dfu->layout = FAT; + + dfu->data.mmc.dev = simple_strtoul(s, &s, 10); + dfu->data.mmc.part = simple_strtoul(++s, &s, 10); + + } else { + printf("%s: Wrong memory layout!\n", __func__); + } + + dfu->read_medium = dfu_read_medium_mmc; + dfu->write_medium = dfu_write_medium_mmc; + + return 0; +} -- 1.7.2.3 _