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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 28 Oct 2013 00:04:37 +0000
From:	Darren Hart <dvhart@...ux.intel.com>
To:	David Cohen <david.a.cohen@...ux.intel.com>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	"David S. Miller" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Fengguang Wu <fengguang.wu@...el.com>,
	Alexandre Courbot <acourbot@...dia.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to
 `devm_gpio_request_one'

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@...aro.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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html