| Message ID | 20180516104417.7664-1-arnout@mind.be |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | UBI handler: allow mtd device to be specified by name | expand |
Hi Arnout, On 16/05/2018 12:44, Arnout Vandecappelle (Essensium/Mind) wrote: > It is often more convenient to specify the partition by name instead > of by number. This is especially true for boards with multiple chips, > where the numbering may change after an update of the kernel. > > If get_mtd_from_device fails, fall back to get_mtd_from_name. In other > words, if mtd partition mtd5 is named "mtd0" (which would be a stupid > thing to do), "mtd0" will still resolve to /dev/mtd0. > > Note that currently, the flash handlers already have this feature. > However, they use the entry 'mtdname' instead of 'device' when it's > a name. We could do the same for the UBI handler, but that would > require an update to the partition parsing as well. I am just missing this point. ? The parser just searches for volume names. The MTD are scanned once without getting their name, but looking for the volume. > Also there is no > real reason to do this: the approach used in this patch is perfectly > backward compatible, easier to understand, and works in all cases > except when a partition is stupidly named "mtdX". > I understand this but the explanation will just stay in git history. To add this, we need to document it (doc/source/handlers.rst). This becomes an implicit rule inside SWUpdate - fine, if it is clearly explained. And maybe you can take a look at the UBI chapter and extend what is already missing... > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > handlers/ubivol_handler.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c > index 0c6fcbf..61a1577 100644 > --- a/handlers/ubivol_handler.c > +++ b/handlers/ubivol_handler.c > @@ -159,6 +159,10 @@ static int adjust_volume(struct img_type *cfg, > * Other MTD are not touched > */ > mtdnum = get_mtd_from_device(cfg->device); > + if (mtdnum < 0) { > + /* Allow device to be specified by name OR number */ > + mtdnum = get_mtd_from_name(cfg->device); > + } > if (mtdnum < 0 || !mtd_dev_present(flash->libmtd, mtdnum)) { > ERROR("%s does not exist: partitioning not possible", > cfg->device); > Best regards, Stefano Babic
On 16-05-18 12:55, Stefano Babic wrote: > Hi Arnout, > > On 16/05/2018 12:44, Arnout Vandecappelle (Essensium/Mind) wrote: >> It is often more convenient to specify the partition by name instead >> of by number. This is especially true for boards with multiple chips, >> where the numbering may change after an update of the kernel. >> >> If get_mtd_from_device fails, fall back to get_mtd_from_name. In other >> words, if mtd partition mtd5 is named "mtd0" (which would be a stupid >> thing to do), "mtd0" will still resolve to /dev/mtd0. >> >> Note that currently, the flash handlers already have this feature. >> However, they use the entry 'mtdname' instead of 'device' when it's >> a name. We could do the same for the UBI handler, but that would >> require an update to the partition parsing as well. > > I am just missing this point. ? The parser just searches for volume > names. The MTD are scanned once without getting their name, but looking > for the volume. parse_partitions() does if (!strlen(partition->volname) || !strlen(partition->device)) { ERROR("Partition incompleted in description file"); free_image(partition); return -1; } but a sw-description entry 'mtdname="foo"' would be stored in partition->path, not in partition->device. So this condition would have to be changed to be able to use the mtdname= construct. > >> Also there is no >> real reason to do this: the approach used in this patch is perfectly >> backward compatible, easier to understand, and works in all cases >> except when a partition is stupidly named "mtdX". >> > > I understand this but the explanation will just stay in git history. To > add this, we need to document it (doc/source/handlers.rst). This becomes > an implicit rule inside SWUpdate - fine, if it is clearly explained. Absolutely. Is a follow-up patch OK or do you want a v2 that includes it? > And maybe you can take a look at the UBI chapter and extend what is > already missing... Can we wait a little with that? I may have more UBI-related patches which would affect the manual. One thing I have lined up is a 'genubiswu' Python script that generates a sw-description and .swu file from a list of files. It's fairly OK but probably not flexible enough to be used as a primary tool (generating the sw-description is a bit iffy). I'm not sure how to contribute this. I can just mail it, or put it in a github gist and refer to it, or include it in the documentation (but it's 85 lines, a bit long...), or add it to the examples directory, or ...? Regards, Arnout [snip]
Hi Arnout, On 16/05/2018 17:15, Arnout Vandecappelle wrote: > > > On 16-05-18 12:55, Stefano Babic wrote: >> Hi Arnout, >> >> On 16/05/2018 12:44, Arnout Vandecappelle (Essensium/Mind) wrote: >>> It is often more convenient to specify the partition by name instead >>> of by number. This is especially true for boards with multiple chips, >>> where the numbering may change after an update of the kernel. >>> >>> If get_mtd_from_device fails, fall back to get_mtd_from_name. In other >>> words, if mtd partition mtd5 is named "mtd0" (which would be a stupid >>> thing to do), "mtd0" will still resolve to /dev/mtd0. >>> >>> Note that currently, the flash handlers already have this feature. >>> However, they use the entry 'mtdname' instead of 'device' when it's >>> a name. We could do the same for the UBI handler, but that would >>> require an update to the partition parsing as well. >> >> I am just missing this point. ? The parser just searches for volume >> names. The MTD are scanned once without getting their name, but looking >> for the volume. > > parse_partitions() does > > if (!strlen(partition->volname) || !strlen(partition->device)) { > ERROR("Partition incompleted in description file"); > free_image(partition); > return -1; > } > > but a sw-description entry 'mtdname="foo"' would be stored in partition->path, > not in partition->device. So this condition would have to be changed to be able > to use the mtdname= construct. ok - but I usually set a partition like: partitions: ( /* UBI Volumes */ { name = "kernel1"; device = "mtd0"; size = 8507392; /* in bytes */ }, }, ............... And device will go into partition->device, even if we have a "mtdname" attribute. > > >> >>> Also there is no >>> real reason to do this: the approach used in this patch is perfectly >>> backward compatible, easier to understand, and works in all cases >>> except when a partition is stupidly named "mtdX". >>> >> >> I understand this but the explanation will just stay in git history. To >> add this, we need to document it (doc/source/handlers.rst). This becomes >> an implicit rule inside SWUpdate - fine, if it is clearly explained. > > Absolutely. Is a follow-up patch OK or do you want a v2 that includes it? I prefer to have a V2, even if you plan follow up patches for something else. > > >> And maybe you can take a look at the UBI chapter and extend what is >> already missing... > > Can we wait a little with that? Sure. > I may have more UBI-related patches which would > affect the manual. Nice. > > One thing I have lined up is a 'genubiswu' Python script that generates a > sw-description and .swu file from a list of files. Is it a self-containing tool or is it planned to be used in Yocto, that is in meta-swupdate ? > It's fairly OK but probably > not flexible enough to be used as a primary tool (generating the sw-description > is a bit iffy). I'm not sure how to contribute this. I can just mail it, or put > it in a github gist and refer to it, or include it in the documentation (but > it's 85 lines, a bit long...), or add it to the examples directory, or ...? I think it is ok to push into examples directory. Best regards, Stefano Babic
Hi Stefano, Coming back to this old patch - I got distracted :-) On 16/05/2018 18:52, Stefano Babic wrote: > Hi Arnout, > > > On 16/05/2018 17:15, Arnout Vandecappelle wrote: >> >> >> On 16-05-18 12:55, Stefano Babic wrote: >>> Hi Arnout, >>> >>> On 16/05/2018 12:44, Arnout Vandecappelle (Essensium/Mind) wrote: >>>> It is often more convenient to specify the partition by name instead >>>> of by number. This is especially true for boards with multiple chips, >>>> where the numbering may change after an update of the kernel. >>>> >>>> If get_mtd_from_device fails, fall back to get_mtd_from_name. In other >>>> words, if mtd partition mtd5 is named "mtd0" (which would be a stupid >>>> thing to do), "mtd0" will still resolve to /dev/mtd0. >>>> >>>> Note that currently, the flash handlers already have this feature. >>>> However, they use the entry 'mtdname' instead of 'device' when it's >>>> a name. We could do the same for the UBI handler, but that would >>>> require an update to the partition parsing as well. >>> >>> I am just missing this point. ? The parser just searches for volume >>> names. The MTD are scanned once without getting their name, but looking >>> for the volume. So, the point is: it's not OK for me to specify the partition as mtdX in my sw-description, because depending on the kernel that is already installed on the device, the mtd numbering will be different. That's because the two flashes (NAND and NOR) are enumerated in a different order. What I would like to have is to use: partitions = ( { name = "dtb"; size = 35211; device = "ubi"; } ); I could also live with 'mtdname = "ubi";' instead of 'device', but then you have this problem: >> parse_partitions() does >> >> if (!strlen(partition->volname) || !strlen(partition->device)) { >> ERROR("Partition incompleted in description file"); >> free_image(partition); >> return -1; >> } >> >> but a sw-description entry 'mtdname="foo"' would be stored in partition->path, >> not in partition->device. So this condition would have to be changed to be able >> to use the mtdname= construct. > > ok - but I usually set a partition like: > > > partitions: ( /* UBI Volumes */ > { > name = "kernel1"; > device = "mtd0"; > size = 8507392; /* in bytes */ > }, > }, > ............... > > And device will go into partition->device, even if we have a "mtdname" > attribute. So the point is that I don't know what the device is. >>>> Also there is no >>>> real reason to do this: the approach used in this patch is perfectly >>>> backward compatible, easier to understand, and works in all cases >>>> except when a partition is stupidly named "mtdX". >>>> >>> >>> I understand this but the explanation will just stay in git history. To >>> add this, we need to document it (doc/source/handlers.rst). This becomes >>> an implicit rule inside SWUpdate - fine, if it is clearly explained. >> >> Absolutely. Is a follow-up patch OK or do you want a v2 that includes it? > > I prefer to have a V2, even if you plan follow up patches for something > else. I forgot what I was supposed to do in the v2... Improve the commit log and update documentation, is that it? >>> And maybe you can take a look at the UBI chapter and extend what is >>> already missing... >> >> Can we wait a little with that? > > Sure. > >> I may have more UBI-related patches which would >> affect the manual. > > Nice. > >> >> One thing I have lined up is a 'genubiswu' Python script that generates a >> sw-description and .swu file from a list of files. > > Is it a self-containing tool or is it planned to be used in Yocto, that > is in meta-swupdate ? I use Buildroot, not Yocot, so it's not for meta-swupdate :-) Regards, Arnout > >> It's fairly OK but probably >> not flexible enough to be used as a primary tool (generating the sw-description >> is a bit iffy). I'm not sure how to contribute this. I can just mail it, or put >> it in a github gist and refer to it, or include it in the documentation (but >> it's 85 lines, a bit long...), or add it to the examples directory, or ...? > > I think it is ok to push into examples directory. > > Best regards, > Stefano Babic >
Hi Arnout, On 16/08/2018 14:36, Arnout Vandecappelle wrote: > Hi Stefano, > > Coming back to this old patch - I got distracted :-) > Me too... > On 16/05/2018 18:52, Stefano Babic wrote: >> Hi Arnout, >> >> >> On 16/05/2018 17:15, Arnout Vandecappelle wrote: >>> >>> >>> On 16-05-18 12:55, Stefano Babic wrote: >>>> Hi Arnout, >>>> >>>> On 16/05/2018 12:44, Arnout Vandecappelle (Essensium/Mind) wrote: >>>>> It is often more convenient to specify the partition by name instead >>>>> of by number. This is especially true for boards with multiple chips, >>>>> where the numbering may change after an update of the kernel. >>>>> >>>>> If get_mtd_from_device fails, fall back to get_mtd_from_name. In other >>>>> words, if mtd partition mtd5 is named "mtd0" (which would be a stupid >>>>> thing to do), "mtd0" will still resolve to /dev/mtd0. >>>>> >>>>> Note that currently, the flash handlers already have this feature. >>>>> However, they use the entry 'mtdname' instead of 'device' when it's >>>>> a name. We could do the same for the UBI handler, but that would >>>>> require an update to the partition parsing as well. >>>> >>>> I am just missing this point. ? The parser just searches for volume >>>> names. The MTD are scanned once without getting their name, but looking >>>> for the volume. > > So, the point is: it's not OK for me to specify the partition as mtdX in my > sw-description, because depending on the kernel that is already installed on the > device, the mtd numbering will be different. That's because the two flashes > (NAND and NOR) are enumerated in a different order. > > What I would like to have is to use: > > partitions = > ( > { > name = "dtb"; > size = 35211; > device = "ubi"; > } > ); > > > I could also live with 'mtdname = "ubi";' instead of 'device', but then you > have this problem: I think device = "ubi" is ok, it is clear that it is a name (with the strange exception we have talked at the beginning of the thread). > >>> parse_partitions() does >>> >>> if (!strlen(partition->volname) || !strlen(partition->device)) { >>> ERROR("Partition incompleted in description file"); >>> free_image(partition); >>> return -1; >>> } >>> >>> but a sw-description entry 'mtdname="foo"' would be stored in partition->path, >>> not in partition->device. So this condition would have to be changed to be able >>> to use the mtdname= construct. >> >> ok - but I usually set a partition like: >> >> >> partitions: ( /* UBI Volumes */ >> { >> name = "kernel1"; >> device = "mtd0"; >> size = 8507392; /* in bytes */ >> }, >> }, >> ............... >> >> And device will go into partition->device, even if we have a "mtdname" >> attribute. > > So the point is that I don't know what the device is. > > >>>>> Also there is no >>>>> real reason to do this: the approach used in this patch is perfectly >>>>> backward compatible, easier to understand, and works in all cases >>>>> except when a partition is stupidly named "mtdX". >>>>> >>>> >>>> I understand this but the explanation will just stay in git history. To >>>> add this, we need to document it (doc/source/handlers.rst). This becomes >>>> an implicit rule inside SWUpdate - fine, if it is clearly explained. >>> >>> Absolutely. Is a follow-up patch OK or do you want a v2 that includes it? >> >> I prefer to have a V2, even if you plan follow up patches for something >> else. > > I forgot what I was supposed to do in the v2... Improve the commit log and > update documentation, is that it? That's it - I set V1 patch in patchwork as "Changes Requested" and I will consider V2. > > >>>> And maybe you can take a look at the UBI chapter and extend what is >>>> already missing... >>> >>> Can we wait a little with that? >> >> Sure. >> >>> I may have more UBI-related patches which would >>> affect the manual. >> >> Nice. >> >>> >>> One thing I have lined up is a 'genubiswu' Python script that generates a >>> sw-description and .swu file from a list of files. >> >> Is it a self-containing tool or is it planned to be used in Yocto, that >> is in meta-swupdate ? > > I use Buildroot, not Yocot, so it's not for meta-swupdate :-) > It does not matter - it could be put in the "tools" directory or into the examples, as a way to generate the SWU. I guess it is independent from Yocto or Buildroot. Regards, Stefano Babic
diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c index 0c6fcbf..61a1577 100644 --- a/handlers/ubivol_handler.c +++ b/handlers/ubivol_handler.c @@ -159,6 +159,10 @@ static int adjust_volume(struct img_type *cfg, * Other MTD are not touched */ mtdnum = get_mtd_from_device(cfg->device); + if (mtdnum < 0) { + /* Allow device to be specified by name OR number */ + mtdnum = get_mtd_from_name(cfg->device); + } if (mtdnum < 0 || !mtd_dev_present(flash->libmtd, mtdnum)) { ERROR("%s does not exist: partitioning not possible", cfg->device);
It is often more convenient to specify the partition by name instead of by number. This is especially true for boards with multiple chips, where the numbering may change after an update of the kernel. If get_mtd_from_device fails, fall back to get_mtd_from_name. In other words, if mtd partition mtd5 is named "mtd0" (which would be a stupid thing to do), "mtd0" will still resolve to /dev/mtd0. Note that currently, the flash handlers already have this feature. However, they use the entry 'mtdname' instead of 'device' when it's a name. We could do the same for the UBI handler, but that would require an update to the partition parsing as well. Also there is no real reason to do this: the approach used in this patch is perfectly backward compatible, easier to understand, and works in all cases except when a partition is stupidly named "mtdX". Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> --- handlers/ubivol_handler.c | 4 ++++ 1 file changed, 4 insertions(+)