linux-kernel - Re: Crashes in -next due to 'phy: add support for a reset-gpio specification'

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] [day] [month] [year] [list]

Message-ID: <20160523052517.GE23704@pengutronix.de>
Date:	Mon, 23 May 2016 07:25:17 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	"linux-next@...r.kernel.org" <linux-next@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-gpio@...r.kernel.org
Subject: Re: Crashes in -next due to 'phy: add support for a reset-gpio
 specification'

Hello Guenter,

On Sun, May 22, 2016 at 06:06:24PM -0700, Guenter Roeck wrote:
> On 05/22/2016 11:21 AM, Uwe Kleine-König wrote:
> >On Sun, May 22, 2016 at 08:41:23AM -0700, Guenter Roeck wrote:
> >>I am not exactly in favor of forcing GPIOLIB to be enabled for every
> >>system with Ethernet support, just because a few of them may require
> >>a gpio based phy reset. The same is true, really, for every other
> >>driver using _optional gpiolib functions.
> >>
> >>Personally, I think that the _optional functions in gpiolib _should_
> >>return no error if gpiolib is not configured. After all, those
> >>gpio pins _are_ supposed to be optional. gpiolib should be enabled
> >>for affected configurations, ie on systems with gpio support which
> >>do need the optional gpio pins. Forcing gpiolib to be enabled even
> >>in systems with no gpio support seems to be a bit heavy-handed.
> >>It just bloats the kernel on such systems with no added benefit.
> >
> >That's wrong. The usage of gpio_get_optional and friends means that the
> >gpio is optional for the *driver*, that is there are devices that make
> >use of said gpio and others don't. For the devices where the gpio is
> >specified its usage is not optional. So it must not be ignored e.g. by
> >GPIOLIB=n configurations.
> 
> Hi Uwe,
> 
> I understand what you are saying, I just have a different perspective.
> From your point of view, you consider it unacceptable if optional GPIO
> pins are made unavailable by changing the configuration to GPIOLIB=n.
> Ok, but that position has number of drawbacks.
> 
> Playing the system this way only works if the optional GPIO pins
> are the only GPIO pins in use by the system. In almost all cases, this
> will not be the case. Disabling GPIOLIB in systems really needing it
> is simply not a realistic option.
> 
> As such, disabling GPIOLIB to "disable" optional GPIO pins is in most cases
> quite heavy-handed. Whoever wants to "disable" those optional pins can simply
> disable them by removing the respective lines from the devicetree file,
> or from the ACPI DSDT. Much easier to do, with less side effects.

Yeah, I agree. I don't see where the idea comes from to disable GPIOLIB
to make a driver work and it seems from your reply that is was mine.
Note I didn't want to imply this. Also I expect that returning NULL in
gpiod_get_optional breaks a few setups and I consider this worse enough
to accept that the driver failes for many devices that would work just
fine with NULL. The reason is that it's easier to debug as the code
fails at a place that is related to the GPIO in question and not at some
obscure point later in time.

> So, in summary, the current approach of mandating that GPIOLIB=y to be able
> to use drivers with optional GPIO pins only has the effect of unnecessarily
> increasing the code size for platforms with no GPIO support. Nothing else.
> It doesn't prevent users from "disabling" optional GPIO pins. There are other
> means to do that, from manipulating the devicetree file to manipulating
> the DSDT to disabling ACPI (yes, that works too).

I don't know about ACPI, so I cannot comment here.

> >enable that part of GPIOLIB such that the _optional variants do the
> >following with GPIOLIB=n:
> >
> >	if a GPIO is specified:
> >		return -ENOSYS
> >	else:
> >		return NULL
> >
> 
> Sure. I thought about it, and had a brief look into the gpiolib code. It would
> require a substantial part of the gpiolib code to be enabled, and as mentioned
> it would still be inconsistent as it really only applies to devicetree and
> lookup table based configurations, but not to ACPI.
> 
> The potential gain of ensuring that GPIOLIB is not accidentally disabled
> just doesn't seem to be worth the effort and the cost, at least not to me.
> Any platform which needs GPIOLIB should just have and keep it enabled,
> and I don't really see it as such a big deal to expect users to keep that
> in mind.
> 
> On the other side, I do dislike the notion of enforcing GPIOLIB=y just because
> some driver(s) used by a platform use optional gpio pins.
> 
> I understand that the current approach is what it is. I just don't
> agree with it, and I don't think it is particularly useful or
> beneficial. But it isn't really worth arguing about either, so let's
> just agree to disagree.

That's fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |