| Message ID | 20190902035842.2747-2-andrew@aj.id.au |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | mmc: sdhci-of-aspeed: Fixes for AST2600 eMMC support | expand |
On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote: > > Resolves the following build error reported by the 0-day bot: > > ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined! > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the > callsite to maintain build coverage for the rest of the driver. > > Reported-by: kbuild test robot <lkp@intel.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 13 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > index d5acb5afc50f..96ca494752c5 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = { > .remove = aspeed_sdhci_remove, > }; > > -static int aspeed_sdc_probe(struct platform_device *pdev) > - > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev) > { > +#if defined(CONFIG_OF_ADDRESS) This is going to be untested code forever, as no one will be running on a chip with this hardware present but OF_ADDRESS disabled. How about we make the driver depend on OF_ADDRESS instead? Cheers, Joel > struct device_node *parent, *child; > + > + parent = pdev->dev.of_node; > + > + for_each_available_child_of_node(parent, child) { > + struct platform_device *cpdev; > + > + cpdev = of_platform_device_create(child, NULL, &pdev->dev); > + if (!cpdev) { > + of_node_put(child); > + return -ENODEV; > + } > + } > +#endif > + > + return 0; > +} > + > +static int aspeed_sdc_probe(struct platform_device *pdev) > + > +{ > struct aspeed_sdc *sdc; > int ret; > > @@ -256,17 +276,9 @@ static int aspeed_sdc_probe(struct platform_device *pdev) > > dev_set_drvdata(&pdev->dev, sdc); > > - parent = pdev->dev.of_node; > - for_each_available_child_of_node(parent, child) { > - struct platform_device *cpdev; > - > - cpdev = of_platform_device_create(child, NULL, &pdev->dev); > - if (!cpdev) { > - of_node_put(child); > - ret = -ENODEV; > - goto err_clk; > - } > - } > + ret = aspeed_sdc_create_sdhcis(pdev); > + if (ret) > + goto err_clk; > > return 0; > > -- > 2.20.1 >
On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote: > On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > Resolves the following build error reported by the 0-day bot: > > > > ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined! > > > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the > > callsite to maintain build coverage for the rest of the driver. > > > > Reported-by: kbuild test robot <lkp@intel.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++---------- > > 1 file changed, 25 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > > index d5acb5afc50f..96ca494752c5 100644 > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = { > > .remove = aspeed_sdhci_remove, > > }; > > > > -static int aspeed_sdc_probe(struct platform_device *pdev) > > - > > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev) > > { > > +#if defined(CONFIG_OF_ADDRESS) > > This is going to be untested code forever, as no one will be running > on a chip with this hardware present but OF_ADDRESS disabled. > > How about we make the driver depend on OF_ADDRESS instead? Testing is split into two pieces here: compile-time and run-time. Clearly the run-time behaviour is going to be broken on configurations without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think that means we shouldn't allow it to be compiled in that case (e.g. CONFIG_COMPILE_TEST performs a similar role). With respect to compile-time it's possible to compile either path as demonstrated by the build failure report. Having said that there's no reason we couldn't do what you suggest, just it wasn't the existing solution pattern for the problem (there are several other drivers that suffered the same bug that were fixed in the style of this patch). Either way works, it's all somewhat academic. Your suggestion is more obvious in terms of correctness, but this patch is basically just code motion (the only addition is the `#if`/ `#endif` lines over what was already there if we disregard the function declaration/invocation). I'll change it if there are further complaints and a reason to do a v3. Andrew
On Mon, 2 Sep 2019 at 07:26, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote: > > On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > Resolves the following build error reported by the 0-day bot: > > > > > > ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined! > > > > > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the > > > callsite to maintain build coverage for the rest of the driver. > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > --- > > > drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++---------- > > > 1 file changed, 25 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > > > index d5acb5afc50f..96ca494752c5 100644 > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = { > > > .remove = aspeed_sdhci_remove, > > > }; > > > > > > -static int aspeed_sdc_probe(struct platform_device *pdev) > > > - > > > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev) > > > { > > > +#if defined(CONFIG_OF_ADDRESS) > > > > This is going to be untested code forever, as no one will be running > > on a chip with this hardware present but OF_ADDRESS disabled. > > > > How about we make the driver depend on OF_ADDRESS instead? > > Testing is split into two pieces here: compile-time and run-time. > Clearly the run-time behaviour is going to be broken on configurations > without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think > that means we shouldn't allow it to be compiled in that case > (e.g. CONFIG_COMPILE_TEST performs a similar role). > > With respect to compile-time it's possible to compile either path as > demonstrated by the build failure report. > > Having said that there's no reason we couldn't do what you suggest, > just it wasn't the existing solution pattern for the problem (there are > several other drivers that suffered the same bug that were fixed in the > style of this patch). Either way works, it's all somewhat academic. > Your suggestion is more obvious in terms of correctness, but this > patch is basically just code motion (the only addition is the `#if`/ > `#endif` lines over what was already there if we disregard the > function declaration/invocation). I'll change it if there are further > complaints and a reason to do a v3. I am in favor of Joel's suggestion as I don't really like having ifdefs bloating around in the driver (unless very good reasons). Please re-spin a v3. Another option is to implement stub function and to deal with error codes, but that sounds more like a long term thingy, if even applicable here. Kind regards Uffe
On Wed, 4 Sep 2019, at 00:18, Ulf Hansson wrote: > On Mon, 2 Sep 2019 at 07:26, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On Mon, 2 Sep 2019, at 13:42, Joel Stanley wrote: > > > On Mon, 2 Sep 2019 at 03:58, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > Resolves the following build error reported by the 0-day bot: > > > > > > > > ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined! > > > > > > > > SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the > > > > callsite to maintain build coverage for the rest of the driver. > > > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > > --- > > > > drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++---------- > > > > 1 file changed, 25 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > > > > index d5acb5afc50f..96ca494752c5 100644 > > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > > > @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = { > > > > .remove = aspeed_sdhci_remove, > > > > }; > > > > > > > > -static int aspeed_sdc_probe(struct platform_device *pdev) > > > > - > > > > +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev) > > > > { > > > > +#if defined(CONFIG_OF_ADDRESS) > > > > > > This is going to be untested code forever, as no one will be running > > > on a chip with this hardware present but OF_ADDRESS disabled. > > > > > > How about we make the driver depend on OF_ADDRESS instead? > > > > Testing is split into two pieces here: compile-time and run-time. > > Clearly the run-time behaviour is going to be broken on configurations > > without CONFIG_OF_ADDRESS (SPARC as mentioned), but I don't think > > that means we shouldn't allow it to be compiled in that case > > (e.g. CONFIG_COMPILE_TEST performs a similar role). > > > > With respect to compile-time it's possible to compile either path as > > demonstrated by the build failure report. > > > > Having said that there's no reason we couldn't do what you suggest, > > just it wasn't the existing solution pattern for the problem (there are > > several other drivers that suffered the same bug that were fixed in the > > style of this patch). Either way works, it's all somewhat academic. > > Your suggestion is more obvious in terms of correctness, but this > > patch is basically just code motion (the only addition is the `#if`/ > > `#endif` lines over what was already there if we disregard the > > function declaration/invocation). I'll change it if there are further > > complaints and a reason to do a v3. > > I am in favor of Joel's suggestion as I don't really like having > ifdefs bloating around in the driver (unless very good reasons). > Please re-spin a v3. > > Another option is to implement stub function and to deal with error > codes, but that sounds more like a long term thingy, if even > applicable here. No worries then, will post a respin shortly. Andrew
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c index d5acb5afc50f..96ca494752c5 100644 --- a/drivers/mmc/host/sdhci-of-aspeed.c +++ b/drivers/mmc/host/sdhci-of-aspeed.c @@ -224,10 +224,30 @@ static struct platform_driver aspeed_sdhci_driver = { .remove = aspeed_sdhci_remove, }; -static int aspeed_sdc_probe(struct platform_device *pdev) - +static int aspeed_sdc_create_sdhcis(struct platform_device *pdev) { +#if defined(CONFIG_OF_ADDRESS) struct device_node *parent, *child; + + parent = pdev->dev.of_node; + + for_each_available_child_of_node(parent, child) { + struct platform_device *cpdev; + + cpdev = of_platform_device_create(child, NULL, &pdev->dev); + if (!cpdev) { + of_node_put(child); + return -ENODEV; + } + } +#endif + + return 0; +} + +static int aspeed_sdc_probe(struct platform_device *pdev) + +{ struct aspeed_sdc *sdc; int ret; @@ -256,17 +276,9 @@ static int aspeed_sdc_probe(struct platform_device *pdev) dev_set_drvdata(&pdev->dev, sdc); - parent = pdev->dev.of_node; - for_each_available_child_of_node(parent, child) { - struct platform_device *cpdev; - - cpdev = of_platform_device_create(child, NULL, &pdev->dev); - if (!cpdev) { - of_node_put(child); - ret = -ENODEV; - goto err_clk; - } - } + ret = aspeed_sdc_create_sdhcis(pdev); + if (ret) + goto err_clk; return 0;
Resolves the following build error reported by the 0-day bot: ERROR: "of_platform_device_create" [drivers/mmc/host/sdhci-of-aspeed.ko] undefined! SPARC does not set CONFIG_OF_ADDRESS so the symbol is missing. Guard the callsite to maintain build coverage for the rest of the driver. Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/mmc/host/sdhci-of-aspeed.c | 38 ++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 13 deletions(-)