| Message ID | 20181106183609.207702-2-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simplify mediatek clk driver probes | expand |
On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote: > > We have a handful of clk drivers that have a collection of slightly > variant device support keyed off of the compatible string. In each of > these drivers, we demux the variant and then call the "real" probe > function based on whatever is stored in the match data for that > compatible string. Let's generalize this function so that it can be > re-used as the platform_driver probe function directly. This looks really hacky to me. It sounds kind of general, but really only works if we have match data that's a single function and we lose any type checking on the function. What about things other than platform devices? Rob
Quoting Rob Herring (2018-11-06 12:44:52) > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > We have a handful of clk drivers that have a collection of slightly > > variant device support keyed off of the compatible string. In each of > > these drivers, we demux the variant and then call the "real" probe > > function based on whatever is stored in the match data for that > > compatible string. Let's generalize this function so that it can be > > re-used as the platform_driver probe function directly. > > This looks really hacky to me. It sounds kind of general, but really > only works if we have match data that's a single function and we lose > any type checking on the function. I don't know what this means. Are you saying that we lose the ability to type check the function pointer stored in the data member? I don't have a good answer for this besides it's not any worse than the status quo for the mediatek drivers. One alternative is to add a DT only array to the platform_driver struct that has the platform driver probe function type and matches the index in the of_device_id table. Then we can add some logic into platform_drv_probe() to look for this table being set and find the probe function to call. Then we still have match data for anything that wants that (maybe it could be passed in to the probe function) at the cost of having another array. I don't have a use-case for this right now so I'm not sure this is a great idea. struct of_platform_driver_probe_func { int (*probe)(struct platform_device *pdev); }; struct of_platform_driver_probe_func mtk_probes[] = { mtk_probe1, mtk_probe2, mtk_probe3, }; struct platform_driver mtk_driver = { .of_probes = &mtk_probes; .driver = { .name = "mtk-foo"; .of_match_table = mtk_match_table, }, }; > What about things other than > platform devices? > I suppose those would need similar functions that take the right struct type and match the driver probe function signature. To make the above more generic we could try to figure out a way to put the 'of_probes' array inside struct device_driver. That would require dropping platform_device specifically from the probe functions and having those take a plain 'struct device' that those probe functions turn into the appropriate structure with to_platform_device() or to_i2c_client()? So the example would become struct of_driver_probe_func { int (*probe)(struct device *dev); }; struct of_driver_probe_func mtk_probes[] = { mtk_probe1, mtk_probe2, mtk_probe3, }; struct platform_driver mtk_driver = { .driver = { .name = "mtk-foo"; .of_match_table = mtk_match_table, .of_probes = &mtk_probes; }, }; And the probe functions might need to container_of() the device pointer to get the struct they know they need. The probe function could also be added to of_device_id and then we would have to look and see if that pointer is populated when the device is matched in generic device code.
On 06/11/2018 19:36, Stephen Boyd wrote: > We have a handful of clk drivers that have a collection of slightly > variant device support keyed off of the compatible string. In each of > these drivers, we demux the variant and then call the "real" probe > function based on whatever is stored in the match data for that > compatible string. Let's generalize this function so that it can be > re-used as the platform_driver probe function directly. > > Cc: Ryder Lee <ryder.lee@mediatek.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/of/device.c | 31 +++++++++++++++++++++++++++++++ > include/linux/of_device.h | 1 + > 2 files changed, 32 insertions(+) > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index 0f27fad9fe94..8381f33ed2d8 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -195,6 +195,37 @@ const void *of_device_get_match_data(const struct device *dev) > } > EXPORT_SYMBOL(of_device_get_match_data); > > +/** > + * platform_driver_probe_by_of_match_data - Probe a platform device using match data > + * @pdev: platform device to probe > + * > + * For use by device drivers that multiplex their probe function through DT > + * match data. Drivers can use this function to call their platform > + * device probe directly without having to implement a wrapper function. > + * > + * static const struct of_device_id probe_funcs[] = { > + * { .compatible = "compat,foo", .data = foo_probe }, > + * {} > + * }; > + * > + * struct platform_driver foo_driver = { > + * .probe = platform_driver_probe_by_of_match_data, > + * .driver = { > + * of_match_table = probe_funcs, > + * }, > + * }; > + * > + */ > +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) > +{ > + int (*probe_func)(struct platform_device *pdev); > + > + probe_func = of_device_get_match_data(&pdev->dev); Shouldn't we check if probe_func is not NULL? > + > + return probe_func(pdev); > +} > +EXPORT_SYMBOL_GPL(platform_driver_probe_by_of_match_data); > + > static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len) > { > const char *compat; > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > index 8d31e39dd564..4de84691d1c6 100644 > --- a/include/linux/of_device.h > +++ b/include/linux/of_device.h > @@ -33,6 +33,7 @@ extern int of_device_add(struct platform_device *pdev); > extern int of_device_register(struct platform_device *ofdev); > extern void of_device_unregister(struct platform_device *ofdev); > > +extern int platform_driver_probe_by_of_match_data(struct platform_device *pdev); > extern const void *of_device_get_match_data(const struct device *dev); > > extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len); >
Quoting Matthias Brugger (2018-11-08 00:29:46) > On 06/11/2018 19:36, Stephen Boyd wrote: > > +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) > > +{ > > + int (*probe_func)(struct platform_device *pdev); > > + > > + probe_func = of_device_get_match_data(&pdev->dev); > > Shouldn't we check if probe_func is not NULL? Is the oops from the NULL pointer deref insufficient?
Hi Stephen, On Wed, Nov 7, 2018 at 7:37 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Rob Herring (2018-11-06 12:44:52) > > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > We have a handful of clk drivers that have a collection of slightly > > > variant device support keyed off of the compatible string. In each of > > > these drivers, we demux the variant and then call the "real" probe > > > function based on whatever is stored in the match data for that > > > compatible string. Let's generalize this function so that it can be > > > re-used as the platform_driver probe function directly. > > > > This looks really hacky to me. It sounds kind of general, but really > > only works if we have match data that's a single function and we lose > > any type checking on the function. > > I don't know what this means. Are you saying that we lose the ability to > type check the function pointer stored in the data member? I don't have > a good answer for this besides it's not any worse than the status quo > for the mediatek drivers. The .data field has always been void *, and is used for storing different things, depending on the driver. It's already up to the driver writer to get that right. > One alternative is to add a DT only array to the platform_driver struct > that has the platform driver probe function type and matches the index > in the of_device_id table. Then we can add some logic into > platform_drv_probe() to look for this table being set and find the probe > function to call. Then we still have match data for anything that wants > that (maybe it could be passed in to the probe function) at the cost of > having another array. I don't have a use-case for this right now so I'm > not sure this is a great idea. > > struct of_platform_driver_probe_func { > int (*probe)(struct platform_device *pdev); > }; > > struct of_platform_driver_probe_func mtk_probes[] = { > mtk_probe1, > mtk_probe2, > mtk_probe3, > }; > > struct platform_driver mtk_driver = { > .of_probes = &mtk_probes; > .driver = { > .name = "mtk-foo"; > .of_match_table = mtk_match_table, > }, > }; This looks worse to me: people tend to be very good at keeping multiple arrays in sync :-( Gr{oetje,eeting}s, Geert
On 08/11/2018 18:58, Stephen Boyd wrote: > Quoting Matthias Brugger (2018-11-08 00:29:46) >> On 06/11/2018 19:36, Stephen Boyd wrote: >>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) >>> +{ >>> + int (*probe_func)(struct platform_device *pdev); >>> + >>> + probe_func = of_device_get_match_data(&pdev->dev); >> >> Shouldn't we check if probe_func is not NULL? > > Is the oops from the NULL pointer deref insufficient? > Do you think we should crash the machine if someone uses the call wrongly? Or should we provide the possibility to error out on the caller side? Regards, Matthias
Hi Matthias, On Fri, Nov 9, 2018 at 11:29 AM Matthias Brugger <matthias.bgg@gmail.com> wrote: > On 08/11/2018 18:58, Stephen Boyd wrote: > > Quoting Matthias Brugger (2018-11-08 00:29:46) > >> On 06/11/2018 19:36, Stephen Boyd wrote: > >>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) > >>> +{ > >>> + int (*probe_func)(struct platform_device *pdev); > >>> + > >>> + probe_func = of_device_get_match_data(&pdev->dev); > >> > >> Shouldn't we check if probe_func is not NULL? > > > > Is the oops from the NULL pointer deref insufficient? > > Do you think we should crash the machine if someone uses the call wrongly? Or > should we provide the possibility to error out on the caller side? I believe that would be a bug in the driver, to be discovered ASAP. So yes, please do crash ;-) Gr{oetje,eeting}s, Geert
On Fri, Nov 9, 2018 at 4:36 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Matthias, > > On Fri, Nov 9, 2018 at 11:29 AM Matthias Brugger <matthias.bgg@gmail.com> wrote: > > On 08/11/2018 18:58, Stephen Boyd wrote: > > > Quoting Matthias Brugger (2018-11-08 00:29:46) > > >> On 06/11/2018 19:36, Stephen Boyd wrote: > > >>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) > > >>> +{ > > >>> + int (*probe_func)(struct platform_device *pdev); > > >>> + > > >>> + probe_func = of_device_get_match_data(&pdev->dev); > > >> > > >> Shouldn't we check if probe_func is not NULL? > > > > > > Is the oops from the NULL pointer deref insufficient? > > > > Do you think we should crash the machine if someone uses the call wrongly? Or > > should we provide the possibility to error out on the caller side? > > I believe that would be a bug in the driver, to be discovered ASAP. > So yes, please do crash ;-) Depends on the driver. If it's not needed to get a console out, then it should just WARN. Otherwise, you've got to go enable earlycon to see the crash. Of course, someone could just as easily stick data in here instead of a function ptr and we'll be back to a crash. Rob
On 11/9/18 2:36 AM, Geert Uytterhoeven wrote: > Hi Matthias, > > On Fri, Nov 9, 2018 at 11:29 AM Matthias Brugger <matthias.bgg@gmail.com> wrote: >> On 08/11/2018 18:58, Stephen Boyd wrote: >>> Quoting Matthias Brugger (2018-11-08 00:29:46) >>>> On 06/11/2018 19:36, Stephen Boyd wrote: >>>>> +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) >>>>> +{ >>>>> + int (*probe_func)(struct platform_device *pdev); >>>>> + >>>>> + probe_func = of_device_get_match_data(&pdev->dev); >>>> >>>> Shouldn't we check if probe_func is not NULL? >>> >>> Is the oops from the NULL pointer deref insufficient? >> >> Do you think we should crash the machine if someone uses the call wrongly? Or >> should we provide the possibility to error out on the caller side? > > I believe that would be a bug in the driver, to be discovered ASAP. > So yes, please do crash ;-) This is one of Linus' pet peeves. He does not think crashing the machine is the proper choice (as a general statement). -Frank > > Gr{oetje,eeting}s, > > Geert >
Quoting Geert Uytterhoeven (2018-11-09 01:56:01) > On Wed, Nov 7, 2018 at 7:37 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Rob Herring (2018-11-06 12:44:52) > > > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote: > > int (*probe)(struct platform_device *pdev); > > }; > > > > struct of_platform_driver_probe_func mtk_probes[] = { > > mtk_probe1, > > mtk_probe2, > > mtk_probe3, > > }; > > > > struct platform_driver mtk_driver = { > > .of_probes = &mtk_probes; > > .driver = { > > .name = "mtk-foo"; > > .of_match_table = mtk_match_table, > > }, > > }; > > This looks worse to me: people tend to be very good at keeping multiple > arrays in sync :-( To be _not_ very good? Agreed, and so specifying the probe function as another member in struct of_device_id seems to be the way to go.
Hi Stephen, On Fri, Nov 9, 2018 at 5:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Geert Uytterhoeven (2018-11-09 01:56:01) > > On Wed, Nov 7, 2018 at 7:37 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > Quoting Rob Herring (2018-11-06 12:44:52) > > > > On Tue, Nov 6, 2018 at 12:36 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > int (*probe)(struct platform_device *pdev); > > > }; > > > > > > struct of_platform_driver_probe_func mtk_probes[] = { > > > mtk_probe1, > > > mtk_probe2, > > > mtk_probe3, > > > }; > > > > > > struct platform_driver mtk_driver = { > > > .of_probes = &mtk_probes; > > > .driver = { > > > .name = "mtk-foo"; > > > .of_match_table = mtk_match_table, > > > }, > > > }; > > > > This looks worse to me: people tend to be very good at keeping multiple > > arrays in sync :-( > > To be _not_ very good? Agreed, and so specifying the probe function as > another member in struct of_device_id seems to be the way to go. Correct, sometimes sarcasm doesn't arrive well at the other end of the electronic tunnel... Gr{oetje,eeting}s, Geert
Quoting Stephen Boyd (2018-11-07 10:37:31) > appropriate structure with to_platform_device() or to_i2c_client()? > > So the example would become > > struct of_driver_probe_func { > int (*probe)(struct device *dev); > }; > > struct of_driver_probe_func mtk_probes[] = { > mtk_probe1, > mtk_probe2, > mtk_probe3, > }; > > struct platform_driver mtk_driver = { > .driver = { > .name = "mtk-foo"; > .of_match_table = mtk_match_table, > .of_probes = &mtk_probes; > }, > }; > > And the probe functions might need to container_of() the device pointer > to get the struct they know they need. The probe function could also be > added to of_device_id and then we would have to look and see if that > pointer is populated when the device is matched in generic device code. > I guess I'll go down the path of extending the of_device_id structure?
On Thu, Nov 29, 2018 at 6:28 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Stephen Boyd (2018-11-07 10:37:31) > > appropriate structure with to_platform_device() or to_i2c_client()? > > > > So the example would become > > > > struct of_driver_probe_func { > > int (*probe)(struct device *dev); > > }; > > > > struct of_driver_probe_func mtk_probes[] = { > > mtk_probe1, > > mtk_probe2, > > mtk_probe3, > > }; > > > > struct platform_driver mtk_driver = { > > .driver = { > > .name = "mtk-foo"; > > .of_match_table = mtk_match_table, > > .of_probes = &mtk_probes; > > }, > > }; > > > > And the probe functions might need to container_of() the device pointer > > to get the struct they know they need. The probe function could also be > > added to of_device_id and then we would have to look and see if that > > pointer is populated when the device is matched in generic device code. > > > > I guess I'll go down the path of extending the of_device_id structure? Unfortunately, I don't think you can change of_device_id as it's part of the kernel ABI. Rob
Quoting Rob Herring (2018-11-29 17:01:54) > On Thu, Nov 29, 2018 at 6:28 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Stephen Boyd (2018-11-07 10:37:31) > > > appropriate structure with to_platform_device() or to_i2c_client()? > > > > > > So the example would become > > > > > > struct of_driver_probe_func { > > > int (*probe)(struct device *dev); > > > }; > > > > > > struct of_driver_probe_func mtk_probes[] = { > > > mtk_probe1, > > > mtk_probe2, > > > mtk_probe3, > > > }; > > > > > > struct platform_driver mtk_driver = { > > > .driver = { > > > .name = "mtk-foo"; > > > .of_match_table = mtk_match_table, > > > .of_probes = &mtk_probes; > > > }, > > > }; > > > > > > And the probe functions might need to container_of() the device pointer > > > to get the struct they know they need. The probe function could also be > > > added to of_device_id and then we would have to look and see if that > > > pointer is populated when the device is matched in generic device code. > > > > > > > I guess I'll go down the path of extending the of_device_id structure? > > Unfortunately, I don't think you can change of_device_id as it's part > of the kernel ABI. Ok. Then I'll go down the path of making it a parallel array?
diff --git a/drivers/of/device.c b/drivers/of/device.c index 0f27fad9fe94..8381f33ed2d8 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -195,6 +195,37 @@ const void *of_device_get_match_data(const struct device *dev) } EXPORT_SYMBOL(of_device_get_match_data); +/** + * platform_driver_probe_by_of_match_data - Probe a platform device using match data + * @pdev: platform device to probe + * + * For use by device drivers that multiplex their probe function through DT + * match data. Drivers can use this function to call their platform + * device probe directly without having to implement a wrapper function. + * + * static const struct of_device_id probe_funcs[] = { + * { .compatible = "compat,foo", .data = foo_probe }, + * {} + * }; + * + * struct platform_driver foo_driver = { + * .probe = platform_driver_probe_by_of_match_data, + * .driver = { + * of_match_table = probe_funcs, + * }, + * }; + * + */ +int platform_driver_probe_by_of_match_data(struct platform_device *pdev) +{ + int (*probe_func)(struct platform_device *pdev); + + probe_func = of_device_get_match_data(&pdev->dev); + + return probe_func(pdev); +} +EXPORT_SYMBOL_GPL(platform_driver_probe_by_of_match_data); + static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len) { const char *compat; diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 8d31e39dd564..4de84691d1c6 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -33,6 +33,7 @@ extern int of_device_add(struct platform_device *pdev); extern int of_device_register(struct platform_device *ofdev); extern void of_device_unregister(struct platform_device *ofdev); +extern int platform_driver_probe_by_of_match_data(struct platform_device *pdev); extern const void *of_device_get_match_data(const struct device *dev); extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len);
We have a handful of clk drivers that have a collection of slightly variant device support keyed off of the compatible string. In each of these drivers, we demux the variant and then call the "real" probe function based on whatever is stored in the match data for that compatible string. Let's generalize this function so that it can be re-used as the platform_driver probe function directly. Cc: Ryder Lee <ryder.lee@mediatek.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/of/device.c | 31 +++++++++++++++++++++++++++++++ include/linux/of_device.h | 1 + 2 files changed, 32 insertions(+)