| Message ID | 20170331093013.29123-1-zajec5@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
On Fri, 31 Mar 2017 11:30:12 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Handling (creating) partitions for flash devices requires using a proper > driver that will read partition table (out of somewhere). We can't > simply try all existing drivers one by one, so MTD subsystem allows > drivers to specify a list of applicable part probes. > > So far physmap_of was the only driver with support for linux,part-probe > DT property. Other ones had list or probes hardcoded which wasn't making > them really flexible. It prevented using common flash drivers on > platforms that required some specific partition table access. > > This commit moves support for mentioned DT property to the MTD core > file. Thanks to calling it on device parse registration (as suggested by > Boris) all drivers gain support for it for free. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/mtd/maps/physmap_of.c | 36 +----------------------------------- > drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++ Maybe you should split the patch: 1/ add core infrastructure 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c BTW, not sure the intermediate "mtd: physmap_of: use OF helpers for reading strings" patch is really useful, since you move to the common infrastructure here. By following my suggestion you get rid of the dependency you have between this series and patch "mtd: physmap_of: use OF helpers for reading strings". > 2 files changed, 35 insertions(+), 35 deletions(-) > > diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c > index 62fa6836f218..49dbb7235848 100644 > --- a/drivers/mtd/maps/physmap_of.c > +++ b/drivers/mtd/maps/physmap_of.c > @@ -114,37 +114,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 **res; > - int count; > - > - count = of_property_count_strings(dp, "linux,part-probe"); > - if (count < 0) > - return part_probe_types_def; > - > - res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL); > - if (!res) > - return NULL; > - > - count = of_property_read_string_array(dp, "linux,part-probe", res, > - count); > - if (count < 0) > - return NULL; > - > - 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,14 +282,8 @@ static int of_flash_probe(struct platform_device *dev) > > info->cmtd->dev.parent = &dev->dev; > mtd_set_of_node(info->cmtd, dp); > - part_probe_types = of_get_probes(dp); > - if (!part_probe_types) { > - err = -ENOMEM; > - goto err_out; > - } > - mtd_device_parse_register(info->cmtd, part_probe_types, NULL, > + mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL, > NULL, 0); > - of_free_probes(part_probe_types); > > kfree(mtd_list); > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 66a9dedd1062..a7ab0a24ba1e 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd) > } > } > > +static const char * const *mtd_of_get_probes(struct device_node *np) > +{ > + const char **res; > + int count; > + > + count = of_property_count_strings(np, "linux,part-probe"); > + if (count < 0) > + return NULL; > + > + res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL); > + if (!res) > + return NULL; > + > + count = of_property_read_string_array(np, "linux,part-probe", res, > + count); > + if (count < 0) > + return NULL; > + > + return res; > +} > + > +static inline void mtd_of_free_probes(const char * const *probes) > +{ > + kfree(probes); > +} > + > /** > * mtd_device_parse_register - parse partitions and register an MTD device. > * > @@ -698,11 +724,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > const struct mtd_partition *parts, > int nr_parts) > { > + const char * const *part_probe_types; > struct mtd_partitions parsed; > int ret; > > mtd_set_dev_defaults(mtd); > > + part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd)); > + if (!part_probe_types) > + part_probe_types = types; > + > memset(&parsed, 0, sizeof(parsed)); > > ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); > @@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > memset(&parsed, 0, sizeof(parsed)); > } > > + if (part_probe_types != types) > + mtd_of_free_probes(part_probe_types); > + > ret = mtd_add_device_partitions(mtd, &parsed); > if (ret) > goto out;
On 03/31/2017 11:56 AM, Boris Brezillon wrote: > On Fri, 31 Mar 2017 11:30:12 +0200 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> Handling (creating) partitions for flash devices requires using a proper >> driver that will read partition table (out of somewhere). We can't >> simply try all existing drivers one by one, so MTD subsystem allows >> drivers to specify a list of applicable part probes. >> >> So far physmap_of was the only driver with support for linux,part-probe >> DT property. Other ones had list or probes hardcoded which wasn't making >> them really flexible. It prevented using common flash drivers on >> platforms that required some specific partition table access. >> >> This commit moves support for mentioned DT property to the MTD core >> file. Thanks to calling it on device parse registration (as suggested by >> Boris) all drivers gain support for it for free. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> drivers/mtd/maps/physmap_of.c | 36 +----------------------------------- >> drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++ > > Maybe you should split the patch: > 1/ add core infrastructure > 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c What's the gain of that? Is this patch too complex to review properly? Will that be useful for anyone to have it split? For me it only adds an intermediate code duplication. > BTW, not sure the intermediate "mtd: physmap_of: use OF helpers > for reading strings" patch is really useful, since you move to the > common infrastructure here. > By following my suggestion you get rid of the dependency you have > between this series and patch "mtd: physmap_of: use OF helpers for > reading strings". I learned (the very hard way) MTD people can be really nitpicking so I'm sending as simple patches as I can. I see it as the only way for someone from OpenWrt/LEDE project to get patch through your review. It's like with this patch: even a simple code move can be questioned. Please drop this patchset, I'll resend it after/if I manage to get [PATCH] mtd: physmap_of: use OF helpers for reading strings accepted.
On Fri, 31 Mar 2017 12:12:56 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > On 03/31/2017 11:56 AM, Boris Brezillon wrote: > > On Fri, 31 Mar 2017 11:30:12 +0200 > > Rafał Miłecki <zajec5@gmail.com> wrote: > > > >> From: Rafał Miłecki <rafal@milecki.pl> > >> > >> Handling (creating) partitions for flash devices requires using a proper > >> driver that will read partition table (out of somewhere). We can't > >> simply try all existing drivers one by one, so MTD subsystem allows > >> drivers to specify a list of applicable part probes. > >> > >> So far physmap_of was the only driver with support for linux,part-probe > >> DT property. Other ones had list or probes hardcoded which wasn't making > >> them really flexible. It prevented using common flash drivers on > >> platforms that required some specific partition table access. > >> > >> This commit moves support for mentioned DT property to the MTD core > >> file. Thanks to calling it on device parse registration (as suggested by > >> Boris) all drivers gain support for it for free. > >> > >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > >> --- > >> drivers/mtd/maps/physmap_of.c | 36 +----------------------------------- > >> drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++ > > > > Maybe you should split the patch: > > 1/ add core infrastructure > > 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c > > What's the gain of that? Is this patch too complex to review properly? Will that > be useful for anyone to have it split? For me it only adds an intermediate code > duplication. The gain is that we get rid of the dependency we have on patch "mtd: physmap_of: use OF helpers for reading strings" which is not even mentioned in your mails. > > > > BTW, not sure the intermediate "mtd: physmap_of: use OF helpers > > for reading strings" patch is really useful, since you move to the > > common infrastructure here. > > By following my suggestion you get rid of the dependency you have > > between this series and patch "mtd: physmap_of: use OF helpers for > > reading strings". > > I learned (the very hard way) MTD people can be really nitpicking so I'm > sending as simple patches as I can. I see it as the only way for someone from > OpenWrt/LEDE project to get patch through your review. And I learned the hard way that OpenWRT/LEDE developers tend to not listen to our advices and keep arguing on things that have been proven to be existing because of bad decision they made at some point in the project life. So I think we're even :-P. > > It's like with this patch: even a simple code move can be questioned. Please > drop this patchset, I'll resend it after/if I manage to get > [PATCH] mtd: physmap_of: use OF helpers for reading strings > accepted. But really, what's the point of this patch? It's just a cleanup. You're not fixing a bug or changing the behavior, and your real objective is to get support for the linux,part-probe in the core, which will then allow us to drop the open-coded version you have in physmap_of.c. I don't think it deserves an intermediate patch, unless your real objective is patchcount.
On 03/31/2017 12:31 PM, Boris Brezillon wrote: > On Fri, 31 Mar 2017 12:12:56 +0200 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> On 03/31/2017 11:56 AM, Boris Brezillon wrote: >>> On Fri, 31 Mar 2017 11:30:12 +0200 >>> Rafał Miłecki <zajec5@gmail.com> wrote: >>> >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> Handling (creating) partitions for flash devices requires using a proper >>>> driver that will read partition table (out of somewhere). We can't >>>> simply try all existing drivers one by one, so MTD subsystem allows >>>> drivers to specify a list of applicable part probes. >>>> >>>> So far physmap_of was the only driver with support for linux,part-probe >>>> DT property. Other ones had list or probes hardcoded which wasn't making >>>> them really flexible. It prevented using common flash drivers on >>>> platforms that required some specific partition table access. >>>> >>>> This commit moves support for mentioned DT property to the MTD core >>>> file. Thanks to calling it on device parse registration (as suggested by >>>> Boris) all drivers gain support for it for free. >>>> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>> --- >>>> drivers/mtd/maps/physmap_of.c | 36 +----------------------------------- >>>> drivers/mtd/mtdcore.c | 34 ++++++++++++++++++++++++++++++++++ >>> >>> Maybe you should split the patch: >>> 1/ add core infrastructure >>> 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c >> >> What's the gain of that? Is this patch too complex to review properly? Will that >> be useful for anyone to have it split? For me it only adds an intermediate code >> duplication. > > The gain is that we get rid of the dependency we have on patch "mtd: > physmap_of: use OF helpers for reading strings" which is not even > mentioned in your mails. This is definitely my mistake, initially I pushed a proper comment below tear line but have overwritten it later. Sorry! >>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers >>> for reading strings" patch is really useful, since you move to the >>> common infrastructure here. >>> By following my suggestion you get rid of the dependency you have >>> between this series and patch "mtd: physmap_of: use OF helpers for >>> reading strings". >> >> I learned (the very hard way) MTD people can be really nitpicking so I'm >> sending as simple patches as I can. I see it as the only way for someone from >> OpenWrt/LEDE project to get patch through your review. > > And I learned the hard way that OpenWRT/LEDE developers tend to not > listen to our advices and keep arguing on things that have been proven > to be existing because of bad decision they made at some point in the > project life. So I think we're even :-P. I wish you could sometimes forget what you've learned and review/discuss things without all that negative approach I keep seeing. >> It's like with this patch: even a simple code move can be questioned. Please >> drop this patchset, I'll resend it after/if I manage to get >> [PATCH] mtd: physmap_of: use OF helpers for reading strings >> accepted. > > But really, what's the point of this patch? It's just a cleanup. You're > not fixing a bug or changing the behavior, and your real objective is > to get support for the linux,part-probe in the core, which will then > allow us to drop the open-coded version you have in physmap_of.c. > > I don't think it deserves an intermediate patch, unless your real > objective is patchcount. OK, I'm going to trust that and see how easily I get can patch your way. I'll resend combined version soon.
Rafal, On Fri, 31 Mar 2017 12:46:38 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > >>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers > >>> for reading strings" patch is really useful, since you move to the > >>> common infrastructure here. > >>> By following my suggestion you get rid of the dependency you have > >>> between this series and patch "mtd: physmap_of: use OF helpers for > >>> reading strings". > >> > >> I learned (the very hard way) MTD people can be really nitpicking so I'm > >> sending as simple patches as I can. I see it as the only way for someone from > >> OpenWrt/LEDE project to get patch through your review. > > > > And I learned the hard way that OpenWRT/LEDE developers tend to not > > listen to our advices and keep arguing on things that have been proven > > to be existing because of bad decision they made at some point in the > > project life. So I think we're even :-P. > > I wish you could sometimes forget what you've learned and review/discuss things > without all that negative approach I keep seeing. I try to stay objective, and if you look back at my review, you'll see that I actually agree with most of your changes. So if one person is taking it personally it's you, not me. Now, regarding other contributions, like support for the TRX format, I keep thinking that it's badly designed and should not be supported in mainline. I clearly expressed my opinion, and I also said I wouldn't block the patches if other MTD maintainers were okay to take them (which is already a good thing, don't you think?). But don't expect me to say "Youhou, let's merge this awesome feature!". More generally, if you look back at all the contributions OpenWRT/LEDE devs made, all uncontroversial features were merged rather quickly. For the other ones, each time we tried to come up with alternative solutions, but if you don't want to follow these suggestions (or at least try them before saying it's impossible), then I think there's nothing we can do on our side. > > > >> It's like with this patch: even a simple code move can be questioned. Please > >> drop this patchset, I'll resend it after/if I manage to get > >> [PATCH] mtd: physmap_of: use OF helpers for reading strings > >> accepted. > > > > But really, what's the point of this patch? It's just a cleanup. You're > > not fixing a bug or changing the behavior, and your real objective is > > to get support for the linux,part-probe in the core, which will then > > allow us to drop the open-coded version you have in physmap_of.c. > > > > I don't think it deserves an intermediate patch, unless your real > > objective is patchcount. > > OK, I'm going to trust that and see how easily I get can patch your way. I'll > resend combined version soon. Let's wait for a DT review, since this is probably the main blocking aspect. Regards, Boris
On 03/31/2017 01:41 PM, Boris Brezillon wrote: > Rafal, > > On Fri, 31 Mar 2017 12:46:38 +0200 > Rafał Miłecki <zajec5@gmail.com> wrote: > >>>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers >>>>> for reading strings" patch is really useful, since you move to the >>>>> common infrastructure here. >>>>> By following my suggestion you get rid of the dependency you have >>>>> between this series and patch "mtd: physmap_of: use OF helpers for >>>>> reading strings". >>>> >>>> I learned (the very hard way) MTD people can be really nitpicking so I'm >>>> sending as simple patches as I can. I see it as the only way for someone from >>>> OpenWrt/LEDE project to get patch through your review. >>> >>> And I learned the hard way that OpenWRT/LEDE developers tend to not >>> listen to our advices and keep arguing on things that have been proven >>> to be existing because of bad decision they made at some point in the >>> project life. So I think we're even :-P. >> >> I wish you could sometimes forget what you've learned and review/discuss things >> without all that negative approach I keep seeing. > > I try to stay objective, and if you look back at my review, you'll see > that I actually agree with most of your changes. So if one person is > taking it personally it's you, not me. > > Now, regarding other contributions, like support for the TRX format, I > keep thinking that it's badly designed and should not be supported in > mainline. I clearly expressed my opinion, and I also said I wouldn't > block the patches if other MTD maintainers were okay to take them (which > is already a good thing, don't you think?). But don't expect me to say > "Youhou, let's merge this awesome feature!". > > More generally, if you look back at all the contributions OpenWRT/LEDE > devs made, all uncontroversial features were merged rather quickly. For > the other ones, each time we tried to come up with alternative > solutions, but if you don't want to follow these suggestions (or at > least try them before saying it's impossible), then I think there's > nothing we can do on our side. Sounds fair from you, thanks. Please note I'm actually following your suggestions, just recently I sent RFC init for initramfs which should handle some of OpenWrt/LEDE hacks in user space as you told us to do this. https://patchwork.ozlabs.org/patch/744093/ >>>> It's like with this patch: even a simple code move can be questioned. Please >>>> drop this patchset, I'll resend it after/if I manage to get >>>> [PATCH] mtd: physmap_of: use OF helpers for reading strings >>>> accepted. >>> >>> But really, what's the point of this patch? It's just a cleanup. You're >>> not fixing a bug or changing the behavior, and your real objective is >>> to get support for the linux,part-probe in the core, which will then >>> allow us to drop the open-coded version you have in physmap_of.c. >>> >>> I don't think it deserves an intermediate patch, unless your real >>> objective is patchcount. >> >> OK, I'm going to trust that and see how easily I get can patch your way. I'll >> resend combined version soon. > > Let's wait for a DT review, since this is probably the main blocking > aspect. OK.
On Fri, 31 Mar 2017 14:23:11 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > On 03/31/2017 01:41 PM, Boris Brezillon wrote: > > Rafal, > > > > On Fri, 31 Mar 2017 12:46:38 +0200 > > Rafał Miłecki <zajec5@gmail.com> wrote: > > > >>>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers > >>>>> for reading strings" patch is really useful, since you move to the > >>>>> common infrastructure here. > >>>>> By following my suggestion you get rid of the dependency you have > >>>>> between this series and patch "mtd: physmap_of: use OF helpers for > >>>>> reading strings". > >>>> > >>>> I learned (the very hard way) MTD people can be really nitpicking so I'm > >>>> sending as simple patches as I can. I see it as the only way for someone from > >>>> OpenWrt/LEDE project to get patch through your review. > >>> > >>> And I learned the hard way that OpenWRT/LEDE developers tend to not > >>> listen to our advices and keep arguing on things that have been proven > >>> to be existing because of bad decision they made at some point in the > >>> project life. So I think we're even :-P. > >> > >> I wish you could sometimes forget what you've learned and review/discuss things > >> without all that negative approach I keep seeing. > > > > I try to stay objective, and if you look back at my review, you'll see > > that I actually agree with most of your changes. So if one person is > > taking it personally it's you, not me. > > > > Now, regarding other contributions, like support for the TRX format, I > > keep thinking that it's badly designed and should not be supported in > > mainline. I clearly expressed my opinion, and I also said I wouldn't > > block the patches if other MTD maintainers were okay to take them (which > > is already a good thing, don't you think?). But don't expect me to say > > "Youhou, let's merge this awesome feature!". > > > > More generally, if you look back at all the contributions OpenWRT/LEDE > > devs made, all uncontroversial features were merged rather quickly. For > > the other ones, each time we tried to come up with alternative > > solutions, but if you don't want to follow these suggestions (or at > > least try them before saying it's impossible), then I think there's > > nothing we can do on our side. > > Sounds fair from you, thanks. Please note I'm actually following your > suggestions, just recently I sent RFC init for initramfs which should handle > some of OpenWrt/LEDE hacks in user space as you told us to do this. > > https://patchwork.ozlabs.org/patch/744093/ Yep, I followed the discussion you had with Richard on IRC. That's good news.
diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c index 62fa6836f218..49dbb7235848 100644 --- a/drivers/mtd/maps/physmap_of.c +++ b/drivers/mtd/maps/physmap_of.c @@ -114,37 +114,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 **res; - int count; - - count = of_property_count_strings(dp, "linux,part-probe"); - if (count < 0) - return part_probe_types_def; - - res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL); - if (!res) - return NULL; - - count = of_property_read_string_array(dp, "linux,part-probe", res, - count); - if (count < 0) - return NULL; - - 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,14 +282,8 @@ static int of_flash_probe(struct platform_device *dev) info->cmtd->dev.parent = &dev->dev; mtd_set_of_node(info->cmtd, dp); - part_probe_types = of_get_probes(dp); - if (!part_probe_types) { - err = -ENOMEM; - goto err_out; - } - mtd_device_parse_register(info->cmtd, part_probe_types, NULL, + mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL, NULL, 0); - of_free_probes(part_probe_types); kfree(mtd_list); diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 66a9dedd1062..a7ab0a24ba1e 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd) } } +static const char * const *mtd_of_get_probes(struct device_node *np) +{ + const char **res; + int count; + + count = of_property_count_strings(np, "linux,part-probe"); + if (count < 0) + return NULL; + + res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL); + if (!res) + return NULL; + + count = of_property_read_string_array(np, "linux,part-probe", res, + count); + if (count < 0) + return NULL; + + return res; +} + +static inline void mtd_of_free_probes(const char * const *probes) +{ + kfree(probes); +} + /** * mtd_device_parse_register - parse partitions and register an MTD device. * @@ -698,11 +724,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, const struct mtd_partition *parts, int nr_parts) { + const char * const *part_probe_types; struct mtd_partitions parsed; int ret; mtd_set_dev_defaults(mtd); + part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd)); + if (!part_probe_types) + part_probe_types = types; + memset(&parsed, 0, sizeof(parsed)); ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); @@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, memset(&parsed, 0, sizeof(parsed)); } + if (part_probe_types != types) + mtd_of_free_probes(part_probe_types); + ret = mtd_add_device_partitions(mtd, &parsed); if (ret) goto out;