Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[Linus Walleij]

On Fri, Oct 25, 2013 at 12:24 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> On Fri, 2013-10-25 at 11:34 +0200, Linus Walleij wrote:

> Minnow doesn't currently have a CONFIG_ upstream which can be used to
> force this selection.

Well it wouldn't help with randconfig or allyesconfig anyway.

>> Is there some default y we can add so this gets compiled for this
>> entire board, or shall I set:
>>
>> depends on PCH_GBE
>> default y
>>
>> On the GPIO_LYNXPOINT or GPIO_PCH driver?
>
> Why would GPIO_PCH depend on PCH_GBE? The inverse is certainly true for
> MinnowBoard.

OK this was the wrong idea.

> I had some MinnowBoard Kconfig bits in my early drivers, but they
> included things like CONFIG_MINNOWBOARD which is not upstream (rightly
> so) but it means I don't have a way to conditionally make PCH_GBE
> dependent on GPIO_PCH and GPIOLIB.
>
> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
> know if David Miller would be amenable to that.

Well we should probably just stick a dependency to GPIOLIB in there.

- It #includes <linux/gpio.h>
- It uses gpiolib functions to do something vital

It was just happy that dummy versions were slotted in until now.

I'll post the following:
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig b/drivers/net/etherne
index a588ffde9700..f2528262aba7 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
@@ -4,7 +4,7 @@

 config PCH_GBE
        tristate "OKI SEMICONDUCTOR IOH(ML7223/ML7831) GbE"
-       depends on PCI && (X86 || COMPILE_TEST)
+       depends on PCI && GPIOLIB && (X86 || COMPILE_TEST)
        select MII
        select PTP_1588_CLOCK_PCH
        ---help---

Yours,
Linus Walleij

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[Linus Walleij]

On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:

>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
>> know if David Miller would be amenable to that.
>
> Well we should probably just stick a dependency to GPIOLIB in there.
>
> - It #includes <linux/gpio.h>
> - It uses gpiolib functions to do something vital
>
> It was just happy that dummy versions were slotted in until now.

...or maybe I'm just confused now?

Should we just add a static inline stub of devm_gpio_request_one()?

Yours,
Linus Walleij

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[David Cohen]

Hi Linus,

On 10/25/2013 03:49 AM, Linus Walleij wrote:
> On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>
>>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
>>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
>>> know if David Miller would be amenable to that.
>>
>> Well we should probably just stick a dependency to GPIOLIB in there.
>>
>> - It #includes <linux/gpio.h>
>> - It uses gpiolib functions to do something vital
>>
>> It was just happy that dummy versions were slotted in until now.
>
> ...or maybe I'm just confused now?
>
> Should we just add a static inline stub of devm_gpio_request_one()?

I am not familiar with the HW. But checking the code, platform 
initialization should fail with a dummy devm_gpio_request_one()
implementation. IMO it makes more sense to depend on GPIOLIB.

Br, David Cohen

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[David Cohen]

On 10/25/2013 02:21 PM, David Cohen wrote:
> Hi Linus,
>
> On 10/25/2013 03:49 AM, Linus Walleij wrote:
>> On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
>> <linus.walleij@linaro.org> wrote:
>>
>>>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
>>>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
>>>> know if David Miller would be amenable to that.
>>>
>>> Well we should probably just stick a dependency to GPIOLIB in there.
>>>
>>> - It #includes <linux/gpio.h>
>>> - It uses gpiolib functions to do something vital
>>>
>>> It was just happy that dummy versions were slotted in until now.
>>
>> ...or maybe I'm just confused now?
>>
>> Should we just add a static inline stub of devm_gpio_request_one()?
>
> I am not familiar with the HW. But checking the code, platform
> initialization should fail with a dummy devm_gpio_request_one()
> implementation. IMO it makes more sense to depend on GPIOLIB.

Actually, forget about it. Despite driver_data->platform_init() may
return error, probe() never checks for it. I think the driver must be
fixed, but in meanwhile a static inline stub seems reasonable.

>
> Br, David Cohen

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[Darren Hart]

On Sat, 2013-10-26 at 21:33 +0100, Darren Hart wrote:
> On Fri, 2013-10-25 at 14:25 -0700, David Cohen wrote:
> > On 10/25/2013 02:21 PM, David Cohen wrote:
> > > Hi Linus,
> > >
> > > On 10/25/2013 03:49 AM, Linus Walleij wrote:
> > >> On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
> > >> <linus.walleij@linaro.org> wrote:
> > >>
> > >>>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
> > >>>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
> > >>>> know if David Miller would be amenable to that.
> > >>>
> > >>> Well we should probably just stick a dependency to GPIOLIB in there.
> > >>>
> > >>> - It #includes <linux/gpio.h>
> > >>> - It uses gpiolib functions to do something vital
> > >>>
> > >>> It was just happy that dummy versions were slotted in until now.
> > >>
> > >> ...or maybe I'm just confused now?
> > >>
> > >> Should we just add a static inline stub of devm_gpio_request_one()?
> > >
> > > I am not familiar with the HW. But checking the code, platform
> > > initialization should fail with a dummy devm_gpio_request_one()
> > > implementation. IMO it makes more sense to depend on GPIOLIB.
> > 
> > Actually, forget about it. Despite driver_data->platform_init() may
> > return error, probe() never checks for it. I think the driver must be
> > fixed, but in meanwhile a static inline stub seems reasonable.
> > 
> 
> Ah, that's a problem, and one I created :/ I'm traveling a bit through
> Europe atm for the conferences. I will try and have a look on the planes
> and add a check.

Ah, now I remember. The situation is this. If there is a cable plugged
into the jack, the PHY will not go to sleep. If there isn't, there is a
good chance it will go to sleep before the driver initializes. If it is
asleep, the scan for the PHY will fail. If it isn't, the scan will work
correctly and the device will be properly setup. The code currently
displays a dev error:

	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
				    "minnow_phy_reset");
	if (ret) {
		dev_err(&pdev->dev,
			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);

But deliberately does not about the probe because there is a chance
things will work just fine. If they do not, the PHY detection code will
print display errors about a failure to communicate over RGMII, and the
device probe will fail from there.

This still seems like the correct approach to me. Does anyone disagree?

(we still need to sort out the GPIOLIB and GPIO_SCH dependency though of
course)

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[David Cohen]

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

On 10/26/2013 02:15 PM, Darren Hart wrote:
> On Sat, 2013-10-26 at 21:33 +0100, Darren Hart wrote:
>> On Fri, 2013-10-25 at 14:25 -0700, David Cohen wrote:
>>> On 10/25/2013 02:21 PM, David Cohen wrote:
>>>> Hi Linus,
>>>>
>>>> On 10/25/2013 03:49 AM, Linus Walleij wrote:
>>>>> On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
>>>>> <linus.walleij@linaro.org> wrote:
>>>>>
>>>>>>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
>>>>>>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
>>>>>>> know if David Miller would be amenable to that.
>>>>>>
>>>>>> Well we should probably just stick a dependency to GPIOLIB in there.
>>>>>>
>>>>>> - It #includes <linux/gpio.h>
>>>>>> - It uses gpiolib functions to do something vital
>>>>>>
>>>>>> It was just happy that dummy versions were slotted in until now.
>>>>>
>>>>> ...or maybe I'm just confused now?
>>>>>
>>>>> Should we just add a static inline stub of devm_gpio_request_one()?
>>>>
>>>> I am not familiar with the HW. But checking the code, platform
>>>> initialization should fail with a dummy devm_gpio_request_one()
>>>> implementation. IMO it makes more sense to depend on GPIOLIB.
>>>
>>> Actually, forget about it. Despite driver_data->platform_init() may
>>> return error, probe() never checks for it. I think the driver must be
>>> fixed, but in meanwhile a static inline stub seems reasonable.
>>>
>>
>> Ah, that's a problem, and one I created :/ I'm traveling a bit through
>> Europe atm for the conferences. I will try and have a look on the planes
>> and add a check.
> 
> Ah, now I remember. The situation is this. If there is a cable plugged
> into the jack, the PHY will not go to sleep. If there isn't, there is a
> good chance it will go to sleep before the driver initializes. If it is
> asleep, the scan for the PHY will fail. If it isn't, the scan will work
> correctly and the device will be properly setup. The code currently
> displays a dev error:
> 
> 	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> 				    "minnow_phy_reset");
> 	if (ret) {
> 		dev_err(&pdev->dev,
> 			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
> 
> But deliberately does not about the probe because there is a chance
> things will work just fine. If they do not, the PHY detection code will
> print display errors about a failure to communicate over RGMII, and the
> device probe will fail from there.
> 
> This still seems like the correct approach to me. Does anyone disagree?

Considering this scenario it seems to be correct. But if
devm_gpio_request_one() may fail for "unfriendly" reasons too, then
it's incomplete.

> 
> (we still need to sort out the GPIOLIB and GPIO_SCH dependency though of
> course)

Maybe if GPIOLIB has the static inline stubs returning -ENODEV we could
use a patch similar to the one attached here.

Br, David

[-- Attachment #2: pch_gbe_main.patch --]
[-- Type: text/x-patch, Size: 1276 bytes --]

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 5a0f04c..4702876 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -2637,8 +2637,11 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	adapter->hw.back = adapter;
 	adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
 	adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
-	if (adapter->pdata && adapter->pdata->platform_init)
-		adapter->pdata->platform_init(pdev);
+	if (adapter->pdata && adapter->pdata->platform_init) {
+		ret = adapter->pdata->platform_init(pdev);
+		if (ret)
+			goto err_free_netdev;
+	}
 
 	adapter->ptp_pdev = pci_get_bus_and_slot(adapter->pdev->bus->number,
 					       PCI_DEVFN(12, 4));
@@ -2740,9 +2743,9 @@ static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
 	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
 				    "minnow_phy_reset");
 	if (ret) {
-		dev_err(&pdev->dev,
-			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
-		return ret;
+		dev_warn(&pdev->dev,
+			 "ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+		return ret == -ENODEV ? ret : 0;
 	}
 
 	gpio_set_value(gpio, 0);

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[Darren Hart]

On Sat, 2013-10-26 at 22:31 -0700, David Cohen wrote:
> On 10/26/2013 02:15 PM, Darren Hart wrote:
> > On Sat, 2013-10-26 at 21:33 +0100, Darren Hart wrote:
> >> On Fri, 2013-10-25 at 14:25 -0700, David Cohen wrote:
> >>> On 10/25/2013 02:21 PM, David Cohen wrote:
> >>>> Hi Linus,
> >>>>
> >>>> On 10/25/2013 03:49 AM, Linus Walleij wrote:
> >>>>> On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
> >>>>> <linus.walleij@linaro.org> wrote:
> >>>>>
> >>>>>>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
> >>>>>>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
> >>>>>>> know if David Miller would be amenable to that.
> >>>>>>
> >>>>>> Well we should probably just stick a dependency to GPIOLIB in there.
> >>>>>>
> >>>>>> - It #includes <linux/gpio.h>
> >>>>>> - It uses gpiolib functions to do something vital
> >>>>>>
> >>>>>> It was just happy that dummy versions were slotted in until now.
> >>>>>
> >>>>> ...or maybe I'm just confused now?
> >>>>>
> >>>>> Should we just add a static inline stub of devm_gpio_request_one()?
> >>>>
> >>>> I am not familiar with the HW. But checking the code, platform
> >>>> initialization should fail with a dummy devm_gpio_request_one()
> >>>> implementation. IMO it makes more sense to depend on GPIOLIB.
> >>>
> >>> Actually, forget about it. Despite driver_data->platform_init() may
> >>> return error, probe() never checks for it. I think the driver must be
> >>> fixed, but in meanwhile a static inline stub seems reasonable.
> >>>
> >>
> >> Ah, that's a problem, and one I created :/ I'm traveling a bit through
> >> Europe atm for the conferences. I will try and have a look on the planes
> >> and add a check.
> > 
> > Ah, now I remember. The situation is this. If there is a cable plugged
> > into the jack, the PHY will not go to sleep. If there isn't, there is a
> > good chance it will go to sleep before the driver initializes. If it is
> > asleep, the scan for the PHY will fail. If it isn't, the scan will work
> > correctly and the device will be properly setup. The code currently
> > displays a dev error:
> > 
> > 	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> > 				    "minnow_phy_reset");
> > 	if (ret) {
> > 		dev_err(&pdev->dev,
> > 			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
> > 
> > But deliberately does not about the probe because there is a chance
> > things will work just fine. If they do not, the PHY detection code will
> > print display errors about a failure to communicate over RGMII, and the
> > device probe will fail from there.
> > 
> > This still seems like the correct approach to me. Does anyone disagree?
> 
> Considering this scenario it seems to be correct. But if
> devm_gpio_request_one() may fail for "unfriendly" reasons too, then
> it's incomplete.
> 
> > 
> > (we still need to sort out the GPIOLIB and GPIO_SCH dependency though of
> > course)
> 
> Maybe if GPIOLIB has the static inline stubs returning -ENODEV we could
> use a patch similar to the one attached here.
> 

I think you may have inverted your logic on the return?

+		dev_warn(&pdev->dev,
+			 "ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+		return ret == -ENODEV ? ret : 0;

Did you mean:

+		/* 
		 * Things may still work if the GPIO driver wasn't
		 * compiled in
		 */
+		return ret == -ENODEV ? 0 : ret;

The concern here of course would be someone added another GPIO
controller over i2c over the expansion connector or something similar
and did not enable the GPIO_SCH driver, then it could conceivably grab
the wrong GPIO pin.... or would those never map to GPIO 13?



-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[David Cohen]

>>> 	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
>>> 				    "minnow_phy_reset");
>>> 	if (ret) {
>>> 		dev_err(&pdev->dev,
>>> 			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
>>>
>>> But deliberately does not about the probe because there is a chance
>>> things will work just fine. If they do not, the PHY detection code will
>>> print display errors about a failure to communicate over RGMII, and the
>>> device probe will fail from there.
>>>
>>> This still seems like the correct approach to me. Does anyone disagree?
>>
>> Considering this scenario it seems to be correct. But if
>> devm_gpio_request_one() may fail for "unfriendly" reasons too, then
>> it's incomplete.
>>
>>>
>>> (we still need to sort out the GPIOLIB and GPIO_SCH dependency though of
>>> course)
>>
>> Maybe if GPIOLIB has the static inline stubs returning -ENODEV we could
>> use a patch similar to the one attached here.
>>
>
> I think you may have inverted your logic on the return?
>
> +		dev_warn(&pdev->dev,
> +			 "ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
> +		return ret == -ENODEV ? ret : 0;
>
> Did you mean:
>
> +		/*
> 		 * Things may still work if the GPIO driver wasn't
> 		 * compiled in
> 		 */

Actually I did mean what I sent, but I misunderstood the above case
where it may work without GPIO driver.
But anyway, "things *may* still work" IMO may create instability issue
by allowing an uncertain situation to go through. If PCH_GBE depends on
GPIOLIB in MinnowBoard case it should fail right away with a clear
message why.

If pch_gbe driver can detect what devm_gpio_request_one() returns from
the static inline stub then it can create a condition to gracefully
fail. So whoever is interested in MinnowBoard will spend less time
trying to identify this problem.

Br, David Cohen

> +		return ret == -ENODEV ? 0 : ret;
>
> The concern here of course would be someone added another GPIO
> controller over i2c over the expansion connector or something similar
> and did not enable the GPIO_SCH driver, then it could conceivably grab
> the wrong GPIO pin.... or would those never map to GPIO 13?
>

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[Linus Walleij]

On Sat, Oct 26, 2013 at 10:31 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:

> Maybe if GPIOLIB has the static inline stubs returning -ENODEV we could
> use a patch similar to the one attached here.

I've added stubs returning -EINVAL just like all siblings there, if you
like to have this changed, send a patch changing it to -ENODEV
and this patch on top as 2/2 please.

Yours,
Linus Walleij

Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'

[David Cohen]

On 10/30/2013 11:21 AM, Linus Walleij wrote:
> On Sat, Oct 26, 2013 at 10:31 PM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
> 
>> Maybe if GPIOLIB has the static inline stubs returning -ENODEV we could
>> use a patch similar to the one attached here.
> 
> I've added stubs returning -EINVAL just like all siblings there, if you
> like to have this changed, send a patch changing it to -ENODEV
> and this patch on top as 2/2 please.

Thanks.

Darren, do you have any objections WRT failing pch_gbe on Minnowboard
due to lack of GPIO?

BR, David