| Message ID | 1461099853-15011-2-git-send-email-afenkart@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Andreas-Fenkart/mmc-omap_hsmmc-devm_pinctrl_get-returns-ERR_PTR-upon-error/20160420-050634
base: https://git.linaro.org/people/ulf.hansson/mmc next
config: mips-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips
All warnings (new ones prefixed by >>):
warning: (BCM47XX && ARCH_REQUIRE_GPIOLIB && PINCTRL_AT91 && PINCTRL_AT91PIO4 && PINCTRL_MESON && PINCTRL_CHERRYVIEW && PINCTRL_INTEL && PINCTRL_TB10X && PINCTRL_SH_PFC_GPIO && PINCTRL_STM32 && PINCTRL_MTK && MFD_TC6393XB) selects GPIOLIB which has unmet direct dependencies (ARCH_WANT_OPTIONAL_GPIOLIB || ARCH_REQUIRE_GPIOLIB)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tuesday 19 April 2016 23:04:13 Andreas Fenkart wrote: > Only the dummy implementation of devm_pinctrl_get returns NULL. > The real implementation returns ERR_PTR. By enforcing pinselect > in Kconfig we can simplify the test to check only for ERR_PTR. > detected/triggered by static code checker. This is not how the interface is meant to work: The dummy devm_pinctrl_get() intentionally returns NULL because IS_ERR() treats that as valid. All other pinctrl functions subsequently ignore that NULL pointer, so the second half of the patch is ok without the first half. Arnd > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Andreas Fenkart <afenkart@gmail.com> > --- > drivers/mmc/host/Kconfig | 1 + > drivers/mmc/host/omap_hsmmc.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 1526b8a..b469755 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -366,6 +366,7 @@ config MMC_OMAP > config MMC_OMAP_HS > tristate "TI OMAP High Speed Multimedia Card Interface support" > depends on HAS_DMA > + select PINCTRL > depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || COMPILE_TEST > help > This selects the TI OMAP High Speed Multimedia card Interface. > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 62e421a..15ebc3b 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -1842,8 +1842,8 @@ static int omap_hsmmc_configure_wake_irq(struct omap_hsmmc_host *host) > */ > if (host->pdata->controller_flags & OMAP_HSMMC_SWAKEUP_MISSING) { > struct pinctrl *p = devm_pinctrl_get(host->dev); > - if (!p) { > - ret = -ENODEV; > + if (IS_ERR(p)) { > + ret = PTR_ERR(p); > goto err_free_irq; > } > if (IS_ERR(pinctrl_lookup_state(p, PINCTRL_STATE_DEFAULT))) { > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Andreas-Fenkart/mmc-omap_hsmmc-devm_pinctrl_get-returns-ERR_PTR-upon-error/20160420-050634
base: https://git.linaro.org/people/ulf.hansson/mmc next
config: tile-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile
All warnings (new ones prefixed by >>):
warning: (ARCH_REQUIRE_GPIOLIB && PINCTRL_AT91 && PINCTRL_AT91PIO4 && PINCTRL_MESON && PINCTRL_CHERRYVIEW && PINCTRL_INTEL && PINCTRL_TB10X && PINCTRL_SH_PFC_GPIO && PINCTRL_STM32 && PINCTRL_MTK && MFD_TC6393XB) selects GPIOLIB which has unmet direct dependencies (ARCH_WANT_OPTIONAL_GPIOLIB || ARCH_REQUIRE_GPIOLIB)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi,
[auto build test WARNING on ulf.hansson-mmc/next]
[also build test WARNING on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Andreas-Fenkart/mmc-omap_hsmmc-devm_pinctrl_get-returns-ERR_PTR-upon-error/20160420-050634
base: https://git.linaro.org/people/ulf.hansson/mmc next
config: blackfin-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=blackfin
All warnings (new ones prefixed by >>):
warning: (MMC_OMAP_HS) selects PINCTRL which has unmet direct dependencies (BF54x || BF60x)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Apr 19, 2016 at 03:12:27PM -0700, Arnd Bergmann wrote: > On Tuesday 19 April 2016 23:04:13 Andreas Fenkart wrote: > > Only the dummy implementation of devm_pinctrl_get returns NULL. > > The real implementation returns ERR_PTR. By enforcing pinselect > > in Kconfig we can simplify the test to check only for ERR_PTR. > > detected/triggered by static code checker. > > This is not how the interface is meant to work: > > The dummy devm_pinctrl_get() intentionally returns NULL because IS_ERR() > treats that as valid. All other pinctrl functions subsequently ignore > that NULL pointer, so the second half of the patch is ok without the > first half. > We had that discussion. The static checker warning triggered the discussion but the real reason for this patch is that the hardware will not work without PINCTRL. So instead of compiling an unusable kernel we added the select. I forget, but I think one of ARCH_OMAP2PLUS or ARCH_KEYSTONE selects PINCTRL already and the other doesn't. That was part of the discussion as well. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-04-20 10:11 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>: > On Tue, Apr 19, 2016 at 03:12:27PM -0700, Arnd Bergmann wrote: >> On Tuesday 19 April 2016 23:04:13 Andreas Fenkart wrote: >> > Only the dummy implementation of devm_pinctrl_get returns NULL. >> > The real implementation returns ERR_PTR. By enforcing pinselect >> > in Kconfig we can simplify the test to check only for ERR_PTR. >> > detected/triggered by static code checker. >> >> This is not how the interface is meant to work: >> >> The dummy devm_pinctrl_get() intentionally returns NULL because IS_ERR() >> treats that as valid. All other pinctrl functions subsequently ignore >> that NULL pointer, so the second half of the patch is ok without the >> first half. >> > > We had that discussion. The static checker warning triggered the > discussion but the real reason for this patch is that the > hardware will not work without PINCTRL. So instead of compiling an > unusable kernel we added the select. beaglebone needs the workaround, most chips don't. The code discussed, only gives some feedback to users about an incorrect configuration. I can drop that part or replace it with a dev_info line, pointing to "Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt" > I forget, but I think one of ARCH_OMAP2PLUS or ARCH_KEYSTONE selects > PINCTRL already and the other doesn't. That was part of the discussion > as well. ARCH_KEYSTONE doesn't have pinctrl, ARCH_OMAP2PLUS does /Andi -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Andreas Fenkart <afenkart@gmail.com> [160420 04:39]: > 2016-04-20 10:11 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>: > > On Tue, Apr 19, 2016 at 03:12:27PM -0700, Arnd Bergmann wrote: > >> On Tuesday 19 April 2016 23:04:13 Andreas Fenkart wrote: > >> > Only the dummy implementation of devm_pinctrl_get returns NULL. > >> > The real implementation returns ERR_PTR. By enforcing pinselect > >> > in Kconfig we can simplify the test to check only for ERR_PTR. > >> > detected/triggered by static code checker. > >> > >> This is not how the interface is meant to work: > >> > >> The dummy devm_pinctrl_get() intentionally returns NULL because IS_ERR() > >> treats that as valid. All other pinctrl functions subsequently ignore > >> that NULL pointer, so the second half of the patch is ok without the > >> first half. > >> > > > > We had that discussion. The static checker warning triggered the > > discussion but the real reason for this patch is that the > > hardware will not work without PINCTRL. So instead of compiling an > > unusable kernel we added the select. > > beaglebone needs the workaround, most chips don't. The code discussed, > only gives some feedback to users about an incorrect configuration. > I can drop that part or replace it with a dev_info line, pointing to > "Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt" Yes pinctrl is optional for sure. And leaving pinctrl out does not produce an unusable kernel for some boards that do all the pin muxing in the bootloader. Like Andreas says, only some boards need dynamic pin muxing for the wake-up events. The dev_info change sounds like papering over the issue.. I'm fine with that, but isn't the real problem devm_pinctrl_get prototype? > > I forget, but I think one of ARCH_OMAP2PLUS or ARCH_KEYSTONE selects > > PINCTRL already and the other doesn't. That was part of the discussion > > as well. > > ARCH_KEYSTONE doesn't have pinctrl, ARCH_OMAP2PLUS does Yes. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 20 April 2016 08:13:37 Tony Lindgren wrote: > * Andreas Fenkart <afenkart@gmail.com> [160420 04:39]: > > 2016-04-20 10:11 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>: > > > On Tue, Apr 19, 2016 at 03:12:27PM -0700, Arnd Bergmann wrote: > > >> On Tuesday 19 April 2016 23:04:13 Andreas Fenkart wrote: > > >> > Only the dummy implementation of devm_pinctrl_get returns NULL. > > >> > The real implementation returns ERR_PTR. By enforcing pinselect > > >> > in Kconfig we can simplify the test to check only for ERR_PTR. > > >> > detected/triggered by static code checker. > > >> > > >> This is not how the interface is meant to work: > > >> > > >> The dummy devm_pinctrl_get() intentionally returns NULL because IS_ERR() > > >> treats that as valid. All other pinctrl functions subsequently ignore > > >> that NULL pointer, so the second half of the patch is ok without the > > >> first half. > > >> > > > > > > We had that discussion. The static checker warning triggered the > > > discussion but the real reason for this patch is that the > > > hardware will not work without PINCTRL. So instead of compiling an > > > unusable kernel we added the select. Ok, but if that is the case, that the changelog text above should be rewritten to explain it (but as Tony explains it was probably a misinterpretation). > > beaglebone needs the workaround, most chips don't. The code discussed, > > only gives some feedback to users about an incorrect configuration. > > I can drop that part or replace it with a dev_info line, pointing to > > "Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt" > > Yes pinctrl is optional for sure. And leaving pinctrl out does not > produce an unusable kernel for some boards that do all the pin muxing > in the bootloader. Like Andreas says, only some boards need dynamic > pin muxing for the wake-up events. > > The dev_info change sounds like papering over the issue.. I'm fine > with that, but isn't the real problem devm_pinctrl_get prototype? > > I don't see anything wrong with the devm_pinctrl_get prototype other than how it frequently confuses driver writers. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 21, 2016 at 12:28:05AM +0200, Arnd Bergmann wrote: > I don't see anything wrong with the devm_pinctrl_get prototype other > than how it frequently confuses driver writers. Yeah. It basically works. There are a bunch of similar APIs where NULL vs ERR_PTR is used the same way. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 1526b8a..b469755 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -366,6 +366,7 @@ config MMC_OMAP config MMC_OMAP_HS tristate "TI OMAP High Speed Multimedia Card Interface support" depends on HAS_DMA + select PINCTRL depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || COMPILE_TEST help This selects the TI OMAP High Speed Multimedia card Interface. diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 62e421a..15ebc3b 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1842,8 +1842,8 @@ static int omap_hsmmc_configure_wake_irq(struct omap_hsmmc_host *host) */ if (host->pdata->controller_flags & OMAP_HSMMC_SWAKEUP_MISSING) { struct pinctrl *p = devm_pinctrl_get(host->dev); - if (!p) { - ret = -ENODEV; + if (IS_ERR(p)) { + ret = PTR_ERR(p); goto err_free_irq; } if (IS_ERR(pinctrl_lookup_state(p, PINCTRL_STATE_DEFAULT))) {
Only the dummy implementation of devm_pinctrl_get returns NULL. The real implementation returns ERR_PTR. By enforcing pinselect in Kconfig we can simplify the test to check only for ERR_PTR. detected/triggered by static code checker. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Andreas Fenkart <afenkart@gmail.com> --- drivers/mmc/host/Kconfig | 1 + drivers/mmc/host/omap_hsmmc.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)