| Message ID | 1431884112-28236-1-git-send-email-hauke@hauke-m.de |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: > This moves the linux,part-probe device tree parsing code from > physmap_of.c to mtdpart.c. Now all drivers can use this feature by just > providing a reference to they device tree node in struct s/they/their/ or s/they/the/ ? > mtd_part_parser_data. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > drivers/mtd/maps/physmap_of.c | 40 +--------------------------------------- > drivers/mtd/mtdpart.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+), 39 deletions(-) > > diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c > index 774b32f..fd3750f 100644 > --- a/drivers/mtd/maps/physmap_of.c > +++ b/drivers/mtd/maps/physmap_of.c > @@ -112,45 +112,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev, > static const char * const part_probe_types_def[] = { > "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL }; > > -static const char * const *of_get_probes(struct device_node *dp) > -{ > - const char *cp; > - int cplen; > - unsigned int l; > - unsigned int count; > - const char **res; > - > - cp = of_get_property(dp, "linux,part-probe", &cplen); > - if (cp == NULL) > - return part_probe_types_def; > - > - count = 0; > - for (l = 0; l != cplen; l++) > - if (cp[l] == 0) > - count++; > - > - res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL); > - count = 0; > - while (cplen > 0) { > - res[count] = cp; > - l = strlen(cp) + 1; > - cp += l; > - cplen -= l; > - count++; > - } > - return res; > -} > - > -static void of_free_probes(const char * const *probes) > -{ > - if (probes != part_probe_types_def) > - kfree(probes); > -} Huh. I didn't even notice this code existed. Like you, I've wondered if we could have a better way to suggest the partition parser types without out specifying them in each driver. That seems like the wrong level of abstraction. Ideally, we'd just be able to have a standard set that satisfies all users, like how all loaded block device partitioners are all checked for each block device (check_part[] in block/partitions/check.c). The only problem I can think of would be if some parsers have subtleties such that they can't safely be applied to all MTDs, even with careful prioritization. But if that doesn't work, then I suppose a device-tree property like this could work. > - > static const struct of_device_id of_flash_match[]; > static int of_flash_probe(struct platform_device *dev) > { > - const char * const *part_probe_types; > const struct of_device_id *match; > struct device_node *dp = dev->dev.of_node; > struct resource res; > @@ -310,10 +274,8 @@ static int of_flash_probe(struct platform_device *dev) > goto err_out; > > ppdata.of_node = dp; > - part_probe_types = of_get_probes(dp); > - mtd_device_parse_register(info->cmtd, part_probe_types, &ppdata, > + mtd_device_parse_register(info->cmtd, part_probe_types_def, &ppdata, > NULL, 0); > - of_free_probes(part_probe_types); > > kfree(mtd_list); > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index cafdb88..b3059d6 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -29,6 +29,7 @@ > #include <linux/kmod.h> > #include <linux/mtd/mtd.h> > #include <linux/mtd/partitions.h> > +#include <linux/of.h> > #include <linux/err.h> > #include <linux/kconfig.h> > > @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) > } > EXPORT_SYMBOL_GPL(deregister_mtd_parser); > > +static const char * const *of_get_probes(struct device_node *dp) Maybe a quick comment at the top to note that if non-NULL, its return value must be kfree()'d by the caller? Could be helpful if we want to move the code elsewhere. (I'm almost tempted to suggest moving this to drivers/of/of_mtd.c, in fact.) > +{ > + const char *cp; > + int cplen; > + unsigned int l; > + unsigned int count; > + const char **res; > + > + cp = of_get_property(dp, "linux,part-probe", &cplen); I see this is not property is not documented in the first place. If we're going to make it generically useful, we definitely need to document it, including what the supported partition probe names are. I'm also kind of ambivalent on this point, but I might as well ask: does the 'linux,...' naming make sense? We're not really supposed to be putting pure software information in the device tree, let alone info that's specific to Linux. I see how the partition parser could be (and should be, really) considered OS-agnostic, though, and might deserve a spot in the DT, at least if it's not safe to run arbitrary partition parsers on all MTDs. > + if (cp == NULL) > + return NULL; > + > + count = 0; > + for (l = 0; l != cplen; l++) > + if (cp[l] == 0) > + count++; > + > + res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL); Might as well fix the spacing here while you're moving the code around. > + count = 0; > + while (cplen > 0) { > + res[count] = cp; > + l = strlen(cp) + 1; > + cp += l; > + cplen -= l; > + count++; > + } > + return res; > +} > + > /* > * Do not forget to update 'parse_mtd_partitions()' kerneldoc comment if you > * are changing this array! > @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > { > struct mtd_part_parser *parser; > int ret = 0; > + const char *const *types_of = NULL; > + > + if (data && data->of_node) { > + types_of = of_get_probes(data->of_node); > + if (types_of != NULL) > + types = types_of; > + } I'm not sure I really like the semantics here. This means that the device tree can override all Linux defaults. So we can't, for instance, have the cmdline parser override all other selections (e.g., for debugging), or make sure that 'ofpart' is available for all MTDs. > > if (!types) > types = default_mtd_part_types; In fact, I don't really like this existing hunk either ^^^. It has the same effect, of killing things like cmdlinepart, which should be available on all MTDs. > @@ -772,6 +809,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > break; > } > } > + kfree(types_of); > return ret; > } > Brian
Hi, On Tue, May 19, 2015 at 12:49 AM, Brian Norris <computersforpeace@gmail.com> wrote: > On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: >> This moves the linux,part-probe device tree parsing code from >> physmap_of.c to mtdpart.c. Now all drivers can use this feature by just >> providing a reference to they device tree node in struct > > s/they/their/ > or > s/they/the/ > ? > >> mtd_part_parser_data. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> drivers/mtd/maps/physmap_of.c | 40 +--------------------------------------- (snip) >> drivers/mtd/mtdpart.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 39 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> index cafdb88..b3059d6 100644 >> --- a/drivers/mtd/mtdpart.c >> +++ b/drivers/mtd/mtdpart.c >> @@ -29,6 +29,7 @@ >> #include <linux/kmod.h> >> #include <linux/mtd/mtd.h> >> #include <linux/mtd/partitions.h> >> +#include <linux/of.h> >> #include <linux/err.h> >> #include <linux/kconfig.h> >> >> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) >> } >> EXPORT_SYMBOL_GPL(deregister_mtd_parser); >> >> +static const char * const *of_get_probes(struct device_node *dp) > > Maybe a quick comment at the top to note that if non-NULL, its return > value must be kfree()'d by the caller? Could be helpful if we want to > move the code elsewhere. (I'm almost tempted to suggest moving this to > drivers/of/of_mtd.c, in fact.) > >> +{ >> + const char *cp; >> + int cplen; >> + unsigned int l; >> + unsigned int count; >> + const char **res; >> + >> + cp = of_get_property(dp, "linux,part-probe", &cplen); > > I see this is not property is not documented in the first place. If > we're going to make it generically useful, we definitely need to > document it, including what the supported partition probe names are. > > I'm also kind of ambivalent on this point, but I might as well ask: does > the 'linux,...' naming make sense? We're not really supposed to be > putting pure software information in the device tree, let alone info > that's specific to Linux. I see how the partition parser could be (and > should be, really) considered OS-agnostic, though, and might deserve a > spot in the DT, at least if it's not safe to run arbitrary partition > parsers on all MTDs. As long as we use the direct parser names, we should keep the linux prefix. Actually a common issue is that many firmware partition tags aren't capable of describing the full flash layout, so partition parsers have to "cheat" and make a lot of assumptions, e.g. the location and the size of the bootloader. So wo would need to have a mix of static partitions (bootloader, bootloader config, calibration data) and dynamic ones (kernel, rootfs, overlay). So ideally we would pass the static partitions as device tree nodes, and let the partition parsers then be applied to partitions instead of the full mtd device. This would also work as a safeguard against mis-guessing important partitions like calibration data which aren't recoverable in case they get overwritten. So my first Idea would be having something like this: mtdevice@foo { ... bootloader: partition@0 { reg = <0 0x10000>; device_type = "flash-partition"; read-only; }; firmware: partition@10000 { reg = <0x1000 0x7e0000>; device_type = "flash-partition"; compatible = "brcm,trx"; }; calibration: partition@7f0000 { reg = < >; device_type = "flash-partition"; read-only; }; }; Then the ofpart parsing could be integrated into the core, and partition parsers get run according to their compatibles on the individual partitions. Of course this also might be total overengineering and completely the wrong place for devicetree as it's essentially configuration data, but since the now described partitions are now more or less "static", IMHO this would be a-okay. >> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, >> { >> struct mtd_part_parser *parser; >> int ret = 0; >> + const char *const *types_of = NULL; >> + >> + if (data && data->of_node) { >> + types_of = of_get_probes(data->of_node); >> + if (types_of != NULL) >> + types = types_of; >> + } > > I'm not sure I really like the semantics here. This means that the > device tree can override all Linux defaults. So we can't, for instance, > have the cmdline parser override all other selections (e.g., for > debugging), or make sure that 'ofpart' is available for all MTDs. Letting the device tree override cmdline parsers is actually a valid use case for OpenWrt, where the bootloader passes a different/incompatible partition layout through mtdparts than what is actually used by the firmware on flash. If we want to make it overrideable for debug purposes adding a commandline parameter like mtdparser="<mtdname>:<parser1[,parser2,...]>" might be a better option. >> >> if (!types) >> types = default_mtd_part_types; > > In fact, I don't really like this existing hunk either ^^^. It has the > same effect, of killing things like cmdlinepart, which should be > available on all MTDs. default_mtd_part_types contains cmdline part, so it should be available unless they pass their own parsers. Regards Jonas
On 05/19/2015 12:49 AM, Brian Norris wrote: > On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: >> This moves the linux,part-probe device tree parsing code from >> physmap_of.c to mtdpart.c. Now all drivers can use this feature by just >> providing a reference to they device tree node in struct > > s/they/their/ > or > s/they/the/ > ? > >> mtd_part_parser_data. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> drivers/mtd/maps/physmap_of.c | 40 +--------------------------------------- >> drivers/mtd/mtdpart.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 39 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c >> index 774b32f..fd3750f 100644 >> --- a/drivers/mtd/maps/physmap_of.c >> +++ b/drivers/mtd/maps/physmap_of.c >> @@ -112,45 +112,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev, >> static const char * const part_probe_types_def[] = { >> "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL }; >> >> -static const char * const *of_get_probes(struct device_node *dp) >> -{ >> - const char *cp; >> - int cplen; >> - unsigned int l; >> - unsigned int count; >> - const char **res; >> - >> - cp = of_get_property(dp, "linux,part-probe", &cplen); >> - if (cp == NULL) >> - return part_probe_types_def; >> - >> - count = 0; >> - for (l = 0; l != cplen; l++) >> - if (cp[l] == 0) >> - count++; >> - >> - res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL); >> - count = 0; >> - while (cplen > 0) { >> - res[count] = cp; >> - l = strlen(cp) + 1; >> - cp += l; >> - cplen -= l; >> - count++; >> - } >> - return res; >> -} >> - >> -static void of_free_probes(const char * const *probes) >> -{ >> - if (probes != part_probe_types_def) >> - kfree(probes); >> -} > > Huh. I didn't even notice this code existed. Like you, I've wondered if > we could have a better way to suggest the partition parser types without > out specifying them in each driver. That seems like the wrong level of > abstraction. > > Ideally, we'd just be able to have a standard set that satisfies all > users, like how all loaded block device partitioners are all checked for > each block device (check_part[] in block/partitions/check.c). The only > problem I can think of would be if some parsers have subtleties such > that they can't safely be applied to all MTDs, even with careful > prioritization. I do not think all the parsers are or will be completely safe when they are trying to parse other data. Some random block could get mixed up with some vendor header format and then you get wrong partition layout. > But if that doesn't work, then I suppose a device-tree property like > this could work. Currently some flash drivers have the parser order hard coded and I do not think that is a good idea, if we have device tree. >> - >> static const struct of_device_id of_flash_match[]; >> static int of_flash_probe(struct platform_device *dev) >> { >> - const char * const *part_probe_types; >> const struct of_device_id *match; >> struct device_node *dp = dev->dev.of_node; >> struct resource res; >> @@ -310,10 +274,8 @@ static int of_flash_probe(struct platform_device *dev) >> goto err_out; >> >> ppdata.of_node = dp; >> - part_probe_types = of_get_probes(dp); >> - mtd_device_parse_register(info->cmtd, part_probe_types, &ppdata, >> + mtd_device_parse_register(info->cmtd, part_probe_types_def, &ppdata, >> NULL, 0); >> - of_free_probes(part_probe_types); >> >> kfree(mtd_list); >> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> index cafdb88..b3059d6 100644 >> --- a/drivers/mtd/mtdpart.c >> +++ b/drivers/mtd/mtdpart.c >> @@ -29,6 +29,7 @@ >> #include <linux/kmod.h> >> #include <linux/mtd/mtd.h> >> #include <linux/mtd/partitions.h> >> +#include <linux/of.h> >> #include <linux/err.h> >> #include <linux/kconfig.h> >> >> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) >> } >> EXPORT_SYMBOL_GPL(deregister_mtd_parser); >> >> +static const char * const *of_get_probes(struct device_node *dp) > > Maybe a quick comment at the top to note that if non-NULL, its return > value must be kfree()'d by the caller? Could be helpful if we want to > move the code elsewhere. (I'm almost tempted to suggest moving this to > drivers/of/of_mtd.c, in fact.) Yes I will add a comment. >> +{ >> + const char *cp; >> + int cplen; >> + unsigned int l; >> + unsigned int count; >> + const char **res; >> + >> + cp = of_get_property(dp, "linux,part-probe", &cplen); > > I see this is not property is not documented in the first place. If > we're going to make it generically useful, we definitely need to > document it, including what the supported partition probe names are. Sorry I did not check, I just amused that it is documented because it was already there, I will add. > I'm also kind of ambivalent on this point, but I might as well ask: does > the 'linux,...' naming make sense? We're not really supposed to be > putting pure software information in the device tree, let alone info > that's specific to Linux. I see how the partition parser could be (and > should be, really) considered OS-agnostic, though, and might deserve a > spot in the DT, at least if it's not safe to run arbitrary partition > parsers on all MTDs. > >> + if (cp == NULL) >> + return NULL; >> + >> + count = 0; >> + for (l = 0; l != cplen; l++) >> + if (cp[l] == 0) >> + count++; >> + >> + res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL); > > Might as well fix the spacing here while you're moving the code around. Will fix that. >> + count = 0; >> + while (cplen > 0) { >> + res[count] = cp; >> + l = strlen(cp) + 1; >> + cp += l; >> + cplen -= l; >> + count++; >> + } >> + return res; >> +} >> + >> /* >> * Do not forget to update 'parse_mtd_partitions()' kerneldoc comment if you >> * are changing this array! >> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, >> { >> struct mtd_part_parser *parser; >> int ret = 0; >> + const char *const *types_of = NULL; >> + >> + if (data && data->of_node) { >> + types_of = of_get_probes(data->of_node); >> + if (types_of != NULL) >> + types = types_of; >> + } > > I'm not sure I really like the semantics here. This means that the > device tree can override all Linux defaults. So we can't, for instance, > have the cmdline parser override all other selections (e.g., for > debugging), or make sure that 'ofpart' is available for all MTDs. parse_mtd_partitions() often gets called by the driver with a hard coded list and I wanted to make it possible to overwrite this list through device tree. > >> >> if (!types) >> types = default_mtd_part_types; > > In fact, I don't really like this existing hunk either ^^^. It has the > same effect, of killing things like cmdlinepart, which should be > available on all MTDs. I do not get this. cmdlinepart is one of the default parsers, if you haven't specified any you will get it. It could be that you do not want the cmdlinepart parser, so I think it should be possible to deactivate it, like though device tree when you do not specify it in the list of parsers. > >> @@ -772,6 +809,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, >> break; >> } >> } >> + kfree(types_of); >> return ret; >> } >> > > Brian >
On 05/19/2015 10:25 AM, Jonas Gorski wrote: > Hi, > > On Tue, May 19, 2015 at 12:49 AM, Brian Norris > <computersforpeace@gmail.com> wrote: >> On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: >>> This moves the linux,part-probe device tree parsing code from >>> physmap_of.c to mtdpart.c. Now all drivers can use this feature by just >>> providing a reference to they device tree node in struct >> >> s/they/their/ >> or >> s/they/the/ >> ? >> >>> mtd_part_parser_data. >>> >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>> --- >>> drivers/mtd/maps/physmap_of.c | 40 +--------------------------------------- > (snip) >>> drivers/mtd/mtdpart.c | 38 ++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 39 insertions(+), 39 deletions(-) >>> >>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >>> index cafdb88..b3059d6 100644 >>> --- a/drivers/mtd/mtdpart.c >>> +++ b/drivers/mtd/mtdpart.c >>> @@ -29,6 +29,7 @@ >>> #include <linux/kmod.h> >>> #include <linux/mtd/mtd.h> >>> #include <linux/mtd/partitions.h> >>> +#include <linux/of.h> >>> #include <linux/err.h> >>> #include <linux/kconfig.h> >>> >>> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) >>> } >>> EXPORT_SYMBOL_GPL(deregister_mtd_parser); >>> >>> +static const char * const *of_get_probes(struct device_node *dp) >> >> Maybe a quick comment at the top to note that if non-NULL, its return >> value must be kfree()'d by the caller? Could be helpful if we want to >> move the code elsewhere. (I'm almost tempted to suggest moving this to >> drivers/of/of_mtd.c, in fact.) >> >>> +{ >>> + const char *cp; >>> + int cplen; >>> + unsigned int l; >>> + unsigned int count; >>> + const char **res; >>> + >>> + cp = of_get_property(dp, "linux,part-probe", &cplen); >> >> I see this is not property is not documented in the first place. If >> we're going to make it generically useful, we definitely need to >> document it, including what the supported partition probe names are. >> >> I'm also kind of ambivalent on this point, but I might as well ask: does >> the 'linux,...' naming make sense? We're not really supposed to be >> putting pure software information in the device tree, let alone info >> that's specific to Linux. I see how the partition parser could be (and >> should be, really) considered OS-agnostic, though, and might deserve a >> spot in the DT, at least if it's not safe to run arbitrary partition >> parsers on all MTDs. > > As long as we use the direct parser names, we should keep the linux prefix. > > Actually a common issue is that many firmware partition tags aren't > capable of describing the full flash layout, so partition parsers have > to "cheat" and make a lot of assumptions, e.g. the location and the > size of the bootloader. So wo would need to have a mix of static > partitions (bootloader, bootloader config, calibration data) and > dynamic ones (kernel, rootfs, overlay). So ideally we would pass the > static partitions as device tree nodes, and let the partition parsers > then be applied to partitions instead of the full mtd device. This > would also work as a safeguard against mis-guessing important > partitions like calibration data which aren't recoverable in case they > get overwritten. > > So my first Idea would be having something like this: > > mtdevice@foo { > ... > bootloader: partition@0 { > reg = <0 0x10000>; > device_type = "flash-partition"; > read-only; > }; > > firmware: partition@10000 { > reg = <0x1000 0x7e0000>; > device_type = "flash-partition"; > compatible = "brcm,trx"; > }; > > calibration: partition@7f0000 { > reg = < >; > device_type = "flash-partition"; > read-only; > }; > }; > > Then the ofpart parsing could be integrated into the core, and > partition parsers get run according to their compatibles on the > individual partitions. > > Of course this also might be total overengineering and completely the > wrong place for devicetree as it's essentially configuration data, but > since the now described partitions are now more or less "static", IMHO > this would be a-okay. This sounds like the problem Rafał wanted to solve in his patch "mtd: add support for typed parsers splitting partitions" This would also solve my problem and would make the bcm47xx part parser a lot simpler and more secure for targets using device tree. I still think we should add a alternative for the people that do not want to rewrite their partition parser. Specifying the list of available partition parser in the flash driver is a bad idea because it is the wrong place. I am in favor of both concepts, because there is already one driver reading the partition parser list from device tree and it is implemented pretty easy. Jonas Approach sounds nice because it really solves two more problems. Jonas approach would work on bcm47xx and bcm53xx targets, because there the header is located after the boot loader and does not contain all partitions. This could also be used to divide the partition containing the kernel and the read only root fs in OpenWrt. Are there any more users which could use this extended approach? >>> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, >>> { >>> struct mtd_part_parser *parser; >>> int ret = 0; >>> + const char *const *types_of = NULL; >>> + >>> + if (data && data->of_node) { >>> + types_of = of_get_probes(data->of_node); >>> + if (types_of != NULL) >>> + types = types_of; >>> + } >> >> I'm not sure I really like the semantics here. This means that the >> device tree can override all Linux defaults. So we can't, for instance, >> have the cmdline parser override all other selections (e.g., for >> debugging), or make sure that 'ofpart' is available for all MTDs. > > Letting the device tree override cmdline parsers is actually a valid > use case for OpenWrt, where the bootloader passes a > different/incompatible partition layout through mtdparts than what is > actually used by the firmware on flash. > > If we want to make it overrideable for debug purposes adding a > commandline parameter like > mtdparser="<mtdname>:<parser1[,parser2,...]>" might be a better > option. > >>> >>> if (!types) >>> types = default_mtd_part_types; >> >> In fact, I don't really like this existing hunk either ^^^. It has the >> same effect, of killing things like cmdlinepart, which should be >> available on all MTDs. > > default_mtd_part_types contains cmdline part, so it should be > available unless they pass their own parsers. > > > Regards > Jonas >
On 19 May 2015 at 23:59, Hauke Mehrtens <hauke@hauke-m.de> wrote: >> So my first Idea would be having something like this: >> >> mtdevice@foo { >> ... >> bootloader: partition@0 { >> reg = <0 0x10000>; >> device_type = "flash-partition"; >> read-only; >> }; >> >> firmware: partition@10000 { >> reg = <0x1000 0x7e0000>; >> device_type = "flash-partition"; >> compatible = "brcm,trx"; >> }; >> >> calibration: partition@7f0000 { >> reg = < >; >> device_type = "flash-partition"; >> read-only; >> }; >> }; >> >> Then the ofpart parsing could be integrated into the core, and >> partition parsers get run according to their compatibles on the >> individual partitions. >> >> Of course this also might be total overengineering and completely the >> wrong place for devicetree as it's essentially configuration data, but >> since the now described partitions are now more or less "static", IMHO >> this would be a-okay. > > This sounds like the problem Rafał wanted to solve in his patch "mtd: > add support for typed parsers splitting partitions" > > This would also solve my problem and would make the bcm47xx part parser > a lot simpler and more secure for targets using device tree. > I still think we should add a alternative for the people that do not > want to rewrite their partition parser. Specifying the list of available > partition parser in the flash driver is a bad idea because it is the > wrong place. This is what I hoped for all the time. To get both patches accepted (linux,part-probe & typed parsers). That would give us a really nice way to handle partitioning on every device. > I am in favor of both concepts, because there is already one driver > reading the partition parser list from device tree and it is implemented > pretty easy. Jonas Approach sounds nice because it really solves two > more problems. > Jonas approach would work on bcm47xx and bcm53xx targets, because there > the header is located after the boot loader and does not contain all > partitions. This could also be used to divide the partition containing > the kernel and the read only root fs in OpenWrt. Are there any more > users which could use this extended approach? I got a bit lost with above. The patch adding typed parsers I sent will allow us to write both: TRX parser (splitter) and SquashFS parser (splitter). This is all we need in OpenWrt and I my patch is highly based on OpenWrt version obviously too.
On Tue, May 19, 2015 at 10:25:58AM +0200, Jonas Gorski wrote: > On Tue, May 19, 2015 at 12:49 AM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: > >> --- a/drivers/mtd/mtdpart.c > >> +++ b/drivers/mtd/mtdpart.c ... > >> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) > >> +static const char * const *of_get_probes(struct device_node *dp) > >> +{ ... > >> + cp = of_get_property(dp, "linux,part-probe", &cplen); > > > > I see this is not property is not documented in the first place. If > > we're going to make it generically useful, we definitely need to > > document it, including what the supported partition probe names are. > > > > I'm also kind of ambivalent on this point, but I might as well ask: does > > the 'linux,...' naming make sense? We're not really supposed to be > > putting pure software information in the device tree, let alone info > > that's specific to Linux. I see how the partition parser could be (and > > should be, really) considered OS-agnostic, though, and might deserve a > > spot in the DT, at least if it's not safe to run arbitrary partition > > parsers on all MTDs. > > As long as we use the direct parser names, we should keep the linux prefix. > > Actually a common issue is that many firmware partition tags aren't > capable of describing the full flash layout, so partition parsers have > to "cheat" and make a lot of assumptions, e.g. the location and the > size of the bootloader. So wo would need to have a mix of static > partitions (bootloader, bootloader config, calibration data) and > dynamic ones (kernel, rootfs, overlay). OK, so this much seems to suggest that we can't really determine a set of parsers that apply to all MTDs, and we have to stick with some kind of platform information for choosing a set of custom parsers. > So ideally we would pass the > static partitions as device tree nodes, and let the partition parsers > then be applied to partitions instead of the full mtd device. This > would also work as a safeguard against mis-guessing important > partitions like calibration data which aren't recoverable in case they > get overwritten. > > So my first Idea would be having something like this: [...] Not commenting on this portion yet. I'll read the others' comments later, but I think that's veering a little off the track of the original problem. > >> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > >> { > >> struct mtd_part_parser *parser; > >> int ret = 0; > >> + const char *const *types_of = NULL; > >> + > >> + if (data && data->of_node) { > >> + types_of = of_get_probes(data->of_node); > >> + if (types_of != NULL) > >> + types = types_of; > >> + } > > > > I'm not sure I really like the semantics here. This means that the > > device tree can override all Linux defaults. So we can't, for instance, > > have the cmdline parser override all other selections (e.g., for > > debugging), or make sure that 'ofpart' is available for all MTDs. > > Letting the device tree override cmdline parsers is actually a valid > use case for OpenWrt, where the bootloader passes a > different/incompatible partition layout through mtdparts than what is > actually used by the firmware on flash. That seems like a sucky firmware, and not an argument for changing how Linux works. Can't you override their cmdline somehow? > If we want to make it overrideable for debug purposes adding a > commandline parameter like > mtdparser="<mtdname>:<parser1[,parser2,...]>" might be a better > option. > > >> > >> if (!types) > >> types = default_mtd_part_types; > > > > In fact, I don't really like this existing hunk either ^^^. It has the > > same effect, of killing things like cmdlinepart, which should be > > available on all MTDs. > > default_mtd_part_types contains cmdline part, so it should be > available unless they pass their own parsers. Right, but (1) I don't like each driver deciding which parsers are available; platform setup should be independent of the flash/controller driver, and (2) the cmdline partition parser and ofpart parsers should always be available for use on all drivers; to break that is a bug, IMO. I think Hauke brought up a similar point to (1). Brian
Let's get this discussion back on track ;-) On 20.05.2015 01:09, Brian Norris wrote: > On Tue, May 19, 2015 at 10:25:58AM +0200, Jonas Gorski wrote: >> On Tue, May 19, 2015 at 12:49 AM, Brian Norris >> <computersforpeace@gmail.com> wrote: >>> On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote: >>>> --- a/drivers/mtd/mtdpart.c >>>> +++ b/drivers/mtd/mtdpart.c > ... >>>> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) >>>> +static const char * const *of_get_probes(struct device_node *dp) >>>> +{ > ... >>>> + cp = of_get_property(dp, "linux,part-probe", &cplen); >>> >>> I see this is not property is not documented in the first place. If >>> we're going to make it generically useful, we definitely need to >>> document it, including what the supported partition probe names are. >>> >>> I'm also kind of ambivalent on this point, but I might as well ask: does >>> the 'linux,...' naming make sense? We're not really supposed to be >>> putting pure software information in the device tree, let alone info >>> that's specific to Linux. I see how the partition parser could be (and >>> should be, really) considered OS-agnostic, though, and might deserve a >>> spot in the DT, at least if it's not safe to run arbitrary partition >>> parsers on all MTDs. >> >> As long as we use the direct parser names, we should keep the linux prefix. >> >> Actually a common issue is that many firmware partition tags aren't >> capable of describing the full flash layout, so partition parsers have >> to "cheat" and make a lot of assumptions, e.g. the location and the >> size of the bootloader. So wo would need to have a mix of static >> partitions (bootloader, bootloader config, calibration data) and >> dynamic ones (kernel, rootfs, overlay). > > OK, so this much seems to suggest that we can't really determine a set of > parsers that apply to all MTDs, and we have to stick with some kind of > platform information for choosing a set of custom parsers. We even want per-device information, because device manufacturers might "port" image tags to other architectures, e.g. Linksys used the trx partition table originally from bcm47xx also for ar71xx devices, wich normally uses u-boot uImage (+ cmdlineparts). >> So ideally we would pass the >> static partitions as device tree nodes, and let the partition parsers >> then be applied to partitions instead of the full mtd device. This >> would also work as a safeguard against mis-guessing important >> partitions like calibration data which aren't recoverable in case they >> get overwritten. >> >> So my first Idea would be having something like this: > [...] > > Not commenting on this portion yet. I'll read the others' comments > later, but I think that's veering a little off the track of the original > problem. > >>>> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, >>>> { >>>> struct mtd_part_parser *parser; >>>> int ret = 0; >>>> + const char *const *types_of = NULL; >>>> + >>>> + if (data && data->of_node) { >>>> + types_of = of_get_probes(data->of_node); >>>> + if (types_of != NULL) >>>> + types = types_of; >>>> + } >>> >>> I'm not sure I really like the semantics here. This means that the >>> device tree can override all Linux defaults. So we can't, for instance, >>> have the cmdline parser override all other selections (e.g., for >>> debugging), or make sure that 'ofpart' is available for all MTDs. >> >> Letting the device tree override cmdline parsers is actually a valid >> use case for OpenWrt, where the bootloader passes a >> different/incompatible partition layout through mtdparts than what is >> actually used by the firmware on flash. > > That seems like a sucky firmware, and not an argument for changing how > Linux works. Can't you override their cmdline somehow? Sometimes yes, sometimes no. If the bootloader doesn't touch the dtb, there is no issue, but e.g. ipq806x bootloaders will overwrite any pre- existing bootargs in the chosen node with the bootloader one. At OpenWrt we do have hacks like "bootargs-replace" and "bootargs-extend", but this doesn't feel very clean either. And sometimes you even want a mix of them, e.g. on devices with support for multiple images you want the information from which image it booted (which is usually exposed through bootargs), but still use your own proprietary layout. Or the bootloader passes the ethernet mac addresses through the commandline, etc. >> If we want to make it overrideable for debug purposes adding a >> commandline parameter like >> mtdparser="<mtdname>:<parser1[,parser2,...]>" might be a better >> option. >> >>>> >>>> if (!types) >>>> types = default_mtd_part_types; >>> >>> In fact, I don't really like this existing hunk either ^^^. It has the >>> same effect, of killing things like cmdlinepart, which should be >>> available on all MTDs. >> >> default_mtd_part_types contains cmdline part, so it should be >> available unless they pass their own parsers. > > Right, but > (1) I don't like each driver deciding which parsers are available; > platform setup should be independent of the flash/controller driver, and > (2) the cmdline partition parser and ofpart parsers should always be > available for use on all drivers; to break that is a bug, IMO. > > I think Hauke brought up a similar point to (1). Right. For (1), I think adding a way for specifying parsers to use through commandline for non-OF and a OF-way would be the way to get away from a driver centric assignment. (2) is a lot harder, because we also have to think about precedence. If there are both cmdline and ofpart parititions, which one should trump? And what about the platform specific parser(s)? Also I noticed a thing which I don't like about the current "linux,part-probe" (or my proposed mtdparsers commandline flag) at all: we define parser names, but we should define partition table names, which are well defined in a document somewhere. So maybe for the of-part, maybe we could make it "similar" to the stdout thing: aliases { sflash0 = &sflash0; sflash1 = &sflash1; nand = &nand; }; chosen { ... mtd-partition-table-type = "nand:fixed-layout", "sflash0:commandline", "sflash1:bcm47xx-trx"; }; Which makes it also clear that this is configuration data. The commandline version could be then mtdtabletypes="<mtdname>:<table-type>[;...]" And then translate the table type names to their parser names (maybe parsers might also support more than one partition table type). This would make a "clean" separation from the linux-internal parser names. Jonas
diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c index 774b32f..fd3750f 100644 --- a/drivers/mtd/maps/physmap_of.c +++ b/drivers/mtd/maps/physmap_of.c @@ -112,45 +112,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev, static const char * const part_probe_types_def[] = { "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL }; -static const char * const *of_get_probes(struct device_node *dp) -{ - const char *cp; - int cplen; - unsigned int l; - unsigned int count; - const char **res; - - cp = of_get_property(dp, "linux,part-probe", &cplen); - if (cp == NULL) - return part_probe_types_def; - - count = 0; - for (l = 0; l != cplen; l++) - if (cp[l] == 0) - count++; - - res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL); - count = 0; - while (cplen > 0) { - res[count] = cp; - l = strlen(cp) + 1; - cp += l; - cplen -= l; - count++; - } - return res; -} - -static void of_free_probes(const char * const *probes) -{ - if (probes != part_probe_types_def) - kfree(probes); -} - static const struct of_device_id of_flash_match[]; static int of_flash_probe(struct platform_device *dev) { - const char * const *part_probe_types; const struct of_device_id *match; struct device_node *dp = dev->dev.of_node; struct resource res; @@ -310,10 +274,8 @@ static int of_flash_probe(struct platform_device *dev) goto err_out; ppdata.of_node = dp; - part_probe_types = of_get_probes(dp); - mtd_device_parse_register(info->cmtd, part_probe_types, &ppdata, + mtd_device_parse_register(info->cmtd, part_probe_types_def, &ppdata, NULL, 0); - of_free_probes(part_probe_types); kfree(mtd_list); diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index cafdb88..b3059d6 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -29,6 +29,7 @@ #include <linux/kmod.h> #include <linux/mtd/mtd.h> #include <linux/mtd/partitions.h> +#include <linux/of.h> #include <linux/err.h> #include <linux/kconfig.h> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p) } EXPORT_SYMBOL_GPL(deregister_mtd_parser); +static const char * const *of_get_probes(struct device_node *dp) +{ + const char *cp; + int cplen; + unsigned int l; + unsigned int count; + const char **res; + + cp = of_get_property(dp, "linux,part-probe", &cplen); + if (cp == NULL) + return NULL; + + count = 0; + for (l = 0; l != cplen; l++) + if (cp[l] == 0) + count++; + + res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL); + count = 0; + while (cplen > 0) { + res[count] = cp; + l = strlen(cp) + 1; + cp += l; + cplen -= l; + count++; + } + return res; +} + /* * Do not forget to update 'parse_mtd_partitions()' kerneldoc comment if you * are changing this array! @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, { struct mtd_part_parser *parser; int ret = 0; + const char *const *types_of = NULL; + + if (data && data->of_node) { + types_of = of_get_probes(data->of_node); + if (types_of != NULL) + types = types_of; + } if (!types) types = default_mtd_part_types; @@ -772,6 +809,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, break; } } + kfree(types_of); return ret; }
This moves the linux,part-probe device tree parsing code from physmap_of.c to mtdpart.c. Now all drivers can use this feature by just providing a reference to they device tree node in struct mtd_part_parser_data. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- drivers/mtd/maps/physmap_of.c | 40 +--------------------------------------- drivers/mtd/mtdpart.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 39 deletions(-)