linux-kernel - Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)

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]

Message-ID: <191168.5104.qm@web180308.mail.gq1.yahoo.com>
Date:	Tue, 22 Jun 2010 21:37:47 -0700 (PDT)
From:	David Brownell <david-b@...bell.net>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Ryan Mallon <ryan@...ewatersys.com>
Cc:	David Brownell <dbrownell@...rs.sourceforge.net>, gregkh@...e.de,
	linux kernel <linux-kernel@...r.kernel.org>,
	ext-jani.1.nikula@...ia.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCH] Rework gpio cansleep (was Re: gpiolib and sleeping gpios)

--- On Tue, 6/22/10, Ryan Mallon <ryan@...ewatersys.com> wrote:

> gpiolib

Again, you're talking about "gpiolib" when
you seem to mean the GPIO framework itself
(for which gpiolib is only an implementation
option)...

> framework could be simplified for cansleep gpios.
> 
> 'Can sleep' for a gpio has two different meanings depending
> on context

NO; for the GPIO itself it's only ever had one
meaning, regardless of context.

You're trying to conflate the GPIO and one
of the contexts in which it's used.  That's
the problem you seem to be struggling with.

Please stop conflating/confusing
those two disparate concepts...

I hope you don't have such a hard time with
the distinction in other contexts.  Like,
the fact that some calls can't be made while
holding spinlocks.  That notion is everywhere
in Linux.

> example, if a driver calls gpio_get_value(gpio) from an
> interupt handler
> then the gpio must not be a sleeping gpio.

In a threaded IRQ handler it's OK to use
the get_value_cansleep() option..

> 
> This patch introduces a new flag, FLAG_CANSLEEP, internal
> to gpiolib

NAK; Superfluous; the gpio_chip already has
that information recorded.

> new request function, gpio_request_cansleep, requests a
> gpio which may
> only be used from sleep possible contexts

Also superfluous.

 The existing
> gpio_request
> function requests a gpio, but does not allow it to be used
> from a
> context where sleep is not possible.

Changing semantics of existing calls is a big
mess, and should be avoided even if it seems
appropriate.

Since the request is just reserving a resource
that's already been identified (and which has
known characteristics, like whether the GPIO
value must be accessed only from sleeping
contexts), this call would also be superfluous.

If you want to ensure the GPIO is a cansleep()
one, just check that before reserving it.  There
is no need for new calls to support that model;
it works today.

(NAK...)

> The benefits I see to this approach are:
>  ...
>  - The API is simplified by combining gpio_(set/get)_value
> and
> gpio_(set/get)_value_cansleep

You have a strange definition of "simplified"...

Recognize that you're also proposing to remove
an API characteristic which much simplifies
code review:  you can look at calls and see
that because they're the cansleep() version,
they are unsafe in IRQ context ...

That is, you're making code (and patch)
reviews much harder and more error-prone.
This isn't good, and doesn't simplify any
process I can think of...

So, NAK on these proposed changes.

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/