[PATCH v3 0/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR

[PATCH v3 0/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR

[Andreas Fenkart]

These are prevous submits:
v1: http://www.spinics.net/lists/linux-mmc/msg35861.html
v2: http://www.spinics.net/lists/linux-mmc/msg35869.html

Both patches tried to check for NULL and ERR_PTR, which makes the check verbose,
or loses the error code in ERR_PTR.  This patch tries to make hard dependency on
pinctrl.

The driver itself does not require pinctrl to work. The pinctrl_pm_select_*
calls in runtime suspend/resume can be dummies. The code section that needs
pinctrl functions, is used to give users a heads up, that requred pinctrl states
are missing. These are needed for the swakeup workaround. But technically the pm
layer selects them.

So pinctrl is needed for user feedback and only on those chips requiring the
SWAKEUP workaround. Most chips don't require that workaround, forcing pinctrl
for those might/might not be invasive.

Select pinctrl in the drivers Kconfig section only selects only the core layer,
which does nothing without an actual driver. Depending on pinctrl will break
keystone, that doesn't have pinctrl selected.

I chose 'select pinctrl' in Kconfig, since keystone and others will not
notice the change. 

v3:
- use select pinctrl in Kconf

v2:
- addressed issues from review
- race-condition issue discussed in patch description

Andreas Fenkart (1):
  mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

 drivers/mmc/host/Kconfig      | 1 +
 drivers/mmc/host/omap_hsmmc.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.8.0.rc3

[PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Andreas Fenkart]

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(-)

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))) {
-- 
2.8.0.rc3

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 42067 bytes --]

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Arnd Bergmann]

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))) {
> 

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Dan Carpenter]

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

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Andreas Fenkart]

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

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Tony Lindgren]

* 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

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Arnd Bergmann]

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

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[Dan Carpenter]

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

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44602 bytes --]

Re: [PATCH v3 1/1] mmc: omap_hsmmc: devm_pinctrl_get returns ERR_PTR upon error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 40446 bytes --]