All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] regulator: convert to use gpio_desc internally
@ 2014-06-29 16:55 Russell King
  2014-06-30 10:06 ` Mark Brown
  2014-06-30 16:31 ` Linus Walleij
  0 siblings, 2 replies; 11+ messages in thread
From: Russell King @ 2014-06-29 16:55 UTC (permalink / raw)
  To: linux-arm-kernel

Convert the regulator GPIO handling to use a gpio descriptor rather than
numbers.  This allows us to revise the interfaces to permit all GPIOs
to be used with the regulator core.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
This is an initial step towards allowing the first GPIO (specified in
DT on Cubox-i/HB as <&gpio1 0 0> to work with the regulator code.

 drivers/regulator/core.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4c1f999041dd..03ce33387a5d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -24,6 +24,7 @@
 #include <linux/suspend.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
@@ -77,7 +78,7 @@ struct regulator_map {
  */
 struct regulator_enable_gpio {
 	struct list_head list;
-	int gpio;
+	struct gpio_desc *gpiod;
 	u32 enable_count;	/* a number of enabled shared GPIO */
 	u32 request_count;	/* a number of requested shared GPIO */
 	unsigned int ena_gpio_invert:1;
@@ -1660,10 +1661,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 				const struct regulator_config *config)
 {
 	struct regulator_enable_gpio *pin;
+	struct gpio_desc *gpiod;
 	int ret;
 
+	gpiod = gpio_to_desc(config->ena_gpio);
+
 	list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
-		if (pin->gpio == config->ena_gpio) {
+		if (pin->gpiod == gpiod) {
 			rdev_dbg(rdev, "GPIO %d is already used\n",
 				config->ena_gpio);
 			goto update_ena_gpio_to_rdev;
@@ -1682,7 +1686,7 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 		return -ENOMEM;
 	}
 
-	pin->gpio = config->ena_gpio;
+	pin->gpiod = gpiod;
 	pin->ena_gpio_invert = config->ena_gpio_invert;
 	list_add(&pin->list, &regulator_ena_gpio_list);
 
@@ -1701,10 +1705,10 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
 
 	/* Free the GPIO only in case of no use */
 	list_for_each_entry_safe(pin, n, &regulator_ena_gpio_list, list) {
-		if (pin->gpio == rdev->ena_pin->gpio) {
+		if (pin->gpiod == rdev->ena_pin->gpiod) {
 			if (pin->request_count <= 1) {
 				pin->request_count = 0;
-				gpio_free(pin->gpio);
+				gpiod_put(pin->gpiod);
 				list_del(&pin->list);
 				kfree(pin);
 			} else {
@@ -1732,8 +1736,8 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 	if (enable) {
 		/* Enable GPIO at initial use */
 		if (pin->enable_count == 0)
-			gpio_set_value_cansleep(pin->gpio,
-						!pin->ena_gpio_invert);
+			gpiod_set_value_cansleep(pin->gpiod,
+						 !pin->ena_gpio_invert);
 
 		pin->enable_count++;
 	} else {
@@ -1744,8 +1748,8 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 
 		/* Disable GPIO if not used */
 		if (pin->enable_count <= 1) {
-			gpio_set_value_cansleep(pin->gpio,
-						pin->ena_gpio_invert);
+			gpiod_set_value_cansleep(pin->gpiod,
+						 pin->ena_gpio_invert);
 			pin->enable_count = 0;
 		}
 	}
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-06-29 16:55 [PATCH] regulator: convert to use gpio_desc internally Russell King
@ 2014-06-30 10:06 ` Mark Brown
  2014-06-30 16:31 ` Linus Walleij
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Brown @ 2014-06-30 10:06 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Jun 29, 2014 at 05:55:54PM +0100, Russell King wrote:
> Convert the regulator GPIO handling to use a gpio descriptor rather than
> numbers.  This allows us to revise the interfaces to permit all GPIOs
> to be used with the regulator core.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140630/8bbd4166/attachment-0001.sig>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-06-29 16:55 [PATCH] regulator: convert to use gpio_desc internally Russell King
  2014-06-30 10:06 ` Mark Brown
@ 2014-06-30 16:31 ` Linus Walleij
  2014-06-30 16:39   ` Russell King - ARM Linux
  1 sibling, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2014-06-30 16:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Jun 29, 2014 at 6:55 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:

> Convert the regulator GPIO handling to use a gpio descriptor rather than
> numbers.  This allows us to revise the interfaces to permit all GPIOs
> to be used with the regulator core.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> This is an initial step towards allowing the first GPIO (specified in
> DT on Cubox-i/HB as <&gpio1 0 0> to work with the regulator code.
>
>  drivers/regulator/core.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 4c1f999041dd..03ce33387a5d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -24,6 +24,7 @@
>  #include <linux/suspend.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>

^ This include should not be needed anymore after using
<linux/gpio/consumer.h> I presume?

> +#include <linux/gpio/consumer.h>
>  #include <linux/of.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/of_regulator.h>

(...)

> @@ -1660,10 +1661,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
>                                 const struct regulator_config *config)
>  {
>         struct regulator_enable_gpio *pin;
> +       struct gpio_desc *gpiod;
>         int ret;
>
> +       gpiod = gpio_to_desc(config->ena_gpio);

So this point is where we will optionally first look for a descriptor
from the device
itself, like devm_gpiod_get(&rdev->dev, "enable"); or so. (For a later
patch.) That
should work smoothly with DT I think (but haven't tried, admittedly).

> @@ -1701,10 +1705,10 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
>
>         /* Free the GPIO only in case of no use */
>         list_for_each_entry_safe(pin, n, &regulator_ena_gpio_list, list) {
> -               if (pin->gpio == rdev->ena_pin->gpio) {
> +               if (pin->gpiod == rdev->ena_pin->gpiod) {
>                         if (pin->request_count <= 1) {
>                                 pin->request_count = 0;
> -                               gpio_free(pin->gpio);
> +                               gpiod_put(pin->gpiod);

gpio_to_desc() doesn't automatically gpiod_get() the GPIOs, it just grabs
the descriptor off the global list, so just skip this. I know this is not
elegant, it's a way of dealing with backward compatibility.

When we complement the code using devm_gpiod_get() later we get
automatic grabage collection and can thus handle both cases seamlessly.

The rest of the patch looks good.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-06-30 16:31 ` Linus Walleij
@ 2014-06-30 16:39   ` Russell King - ARM Linux
  2014-06-30 16:49     ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux @ 2014-06-30 16:39 UTC (permalink / raw)
  To: linux-arm-kernel

Linus,

Bear in mind that Mark has already committed this patch - rather too
hastily I think.  I've no idea why he didn't wait for your review
before doing so, that would have been the appropriate thing to have
done.

On Mon, Jun 30, 2014 at 06:31:08PM +0200, Linus Walleij wrote:
> On Sun, Jun 29, 2014 at 6:55 PM, Russell King
> <rmk+kernel@arm.linux.org.uk> wrote:
> 
> > Convert the regulator GPIO handling to use a gpio descriptor rather than
> > numbers.  This allows us to revise the interfaces to permit all GPIOs
> > to be used with the regulator core.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> > This is an initial step towards allowing the first GPIO (specified in
> > DT on Cubox-i/HB as <&gpio1 0 0> to work with the regulator code.
> >
> >  drivers/regulator/core.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 4c1f999041dd..03ce33387a5d 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/suspend.h>
> >  #include <linux/delay.h>
> >  #include <linux/gpio.h>
> 
> ^ This include should not be needed anymore after using
> <linux/gpio/consumer.h> I presume?

No, because we still need to use the legacy API to request the GPIO.

> 
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/of.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/of_regulator.h>
> 
> (...)
> 
> > @@ -1660,10 +1661,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> >                                 const struct regulator_config *config)
> >  {
> >         struct regulator_enable_gpio *pin;
> > +       struct gpio_desc *gpiod;
> >         int ret;
> >
> > +       gpiod = gpio_to_desc(config->ena_gpio);
> 
> So this point is where we will optionally first look for a descriptor
> from the device
> itself, like devm_gpiod_get(&rdev->dev, "enable"); or so. (For a later
> patch.) That
> should work smoothly with DT I think (but haven't tried, admittedly).

This is where the problem comes.  devm_gpiod_get() can only be used once
per GPIO, so there's no sharing possible when using this interface.
Since we need to share GPIOs here, we can't use any GPIO interface which
ensures exclusivity.

Consider the case where you have two regulators wired up to a single
control GPIO.  DT would specify the same GPIO in two regulator
descriptions.  The GPIO interfaces will allow the GPIO to be "got"
for the first regulator to be probed, and fail the second one with
-EBUSY.

That's what this code is trying to avoid.

> > @@ -1701,10 +1705,10 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
> >
> >         /* Free the GPIO only in case of no use */
> >         list_for_each_entry_safe(pin, n, &regulator_ena_gpio_list, list) {
> > -               if (pin->gpio == rdev->ena_pin->gpio) {
> > +               if (pin->gpiod == rdev->ena_pin->gpiod) {
> >                         if (pin->request_count <= 1) {
> >                                 pin->request_count = 0;
> > -                               gpio_free(pin->gpio);
> > +                               gpiod_put(pin->gpiod);
> 
> gpio_to_desc() doesn't automatically gpiod_get() the GPIOs, it just grabs
> the descriptor off the global list, so just skip this. I know this is not
> elegant, it's a way of dealing with backward compatibility.

Correct, and that's why it's being used.  It's being used to get a
"cookie" for the GPIO which we can then match against the GPIOs we
already know about.  If we find that it's a new cookie, we use the
legacy gpio_request() API to mark it in use.

This is why I mentioned in the commit message that this is a step
towards solving this properly - the core regulator code needs to deal
with gpio_desc's internally and use the new API, but we need to solve
how to get the gpio_desc from OF in a manner which permits sharing.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-06-30 16:39   ` Russell King - ARM Linux
@ 2014-06-30 16:49     ` Linus Walleij
  2014-07-01  0:59       ` Alexandre Courbot
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2014-06-30 16:49 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jun 30, 2014 at 6:39 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Bear in mind that Mark has already committed this patch - rather too
> hastily I think.  I've no idea why he didn't wait for your review
> before doing so, that would have been the appropriate thing to have
> done.

Well, he's usually just as quick at pulling them out when there
is trouble so let's hope that happens.

>> >  #include <linux/gpio.h>
>>
>> ^ This include should not be needed anymore after using
>> <linux/gpio/consumer.h> I presume?
>
> No, because we still need to use the legacy API to request the GPIO.

Oh :-/

I have a hard time seeing that mixture here due to the nature
of patch, sorry.

>> > @@ -1660,10 +1661,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
>> >                                 const struct regulator_config *config)
>> >  {
>> >         struct regulator_enable_gpio *pin;
>> > +       struct gpio_desc *gpiod;
>> >         int ret;
>> >
>> > +       gpiod = gpio_to_desc(config->ena_gpio);
>>
>> So this point is where we will optionally first look for a descriptor
>> from the device
>> itself, like devm_gpiod_get(&rdev->dev, "enable"); or so. (For a later
>> patch.) That
>> should work smoothly with DT I think (but haven't tried, admittedly).
>
> This is where the problem comes.  devm_gpiod_get() can only be used once
> per GPIO, so there's no sharing possible when using this interface.
> Since we need to share GPIOs here, we can't use any GPIO interface which
> ensures exclusivity.
>
> Consider the case where you have two regulators wired up to a single
> control GPIO.  DT would specify the same GPIO in two regulator
> descriptions.  The GPIO interfaces will allow the GPIO to be "got"
> for the first regulator to be probed, and fail the second one with
> -EBUSY.
>
> That's what this code is trying to avoid.

OK that's a snag.

Alexandre: do you have a good idea about how we solve the
multiple consumer problem for GPIO descriptors?

I can think of things like tagging lines as allowing multiple consumers
in the gpiochip (in the gpiochip node, in the DT case) or simply just
allowing this for any GPIO, or just allow it if obtained using a certain
variant gpiod_multiple_consumer_get() call.

Ideas?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-06-30 16:49     ` Linus Walleij
@ 2014-07-01  0:59       ` Alexandre Courbot
  2014-07-01 11:10         ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2014-07-01  0:59 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jul 1, 2014 at 1:49 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jun 30, 2014 at 6:39 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>
>> Bear in mind that Mark has already committed this patch - rather too
>> hastily I think.  I've no idea why he didn't wait for your review
>> before doing so, that would have been the appropriate thing to have
>> done.
>
> Well, he's usually just as quick at pulling them out when there
> is trouble so let's hope that happens.
>
>>> >  #include <linux/gpio.h>
>>>
>>> ^ This include should not be needed anymore after using
>>> <linux/gpio/consumer.h> I presume?
>>
>> No, because we still need to use the legacy API to request the GPIO.
>
> Oh :-/
>
> I have a hard time seeing that mixture here due to the nature
> of patch, sorry.
>
>>> > @@ -1660,10 +1661,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
>>> >                                 const struct regulator_config *config)
>>> >  {
>>> >         struct regulator_enable_gpio *pin;
>>> > +       struct gpio_desc *gpiod;
>>> >         int ret;
>>> >
>>> > +       gpiod = gpio_to_desc(config->ena_gpio);
>>>
>>> So this point is where we will optionally first look for a descriptor
>>> from the device
>>> itself, like devm_gpiod_get(&rdev->dev, "enable"); or so. (For a later
>>> patch.) That
>>> should work smoothly with DT I think (but haven't tried, admittedly).
>>
>> This is where the problem comes.  devm_gpiod_get() can only be used once
>> per GPIO, so there's no sharing possible when using this interface.
>> Since we need to share GPIOs here, we can't use any GPIO interface which
>> ensures exclusivity.
>>
>> Consider the case where you have two regulators wired up to a single
>> control GPIO.  DT would specify the same GPIO in two regulator
>> descriptions.  The GPIO interfaces will allow the GPIO to be "got"
>> for the first regulator to be probed, and fail the second one with
>> -EBUSY.
>>
>> That's what this code is trying to avoid.
>
> OK that's a snag.
>
> Alexandre: do you have a good idea about how we solve the
> multiple consumer problem for GPIO descriptors?
>
> I can think of things like tagging lines as allowing multiple consumers
> in the gpiochip (in the gpiochip node, in the DT case) or simply just
> allowing this for any GPIO, or just allow it if obtained using a certain
> variant gpiod_multiple_consumer_get() call.

Ah right, we cannot obtain a GPIO descriptor to compare to until we
call gpiod_get() ourselves.

We could compare on the (dev, func, index) triplet instead of the GPIO
number, but that looks a little bit overkill (and won't work if we mix
integers and descriptors).

Requiring DT tagging to allow sharing would break DT compatibility I'm afraid.

Maybe it is simply time to revisit our policy regarding shared GPIOs,
at least for the gpiod interface. For the integer representation we
had a truly legitimate reason for not allowing sharing, since anyone
could forge a GPIO number, request it, and mess up with the system.

The gpiod interface does not allow that, and GPIOs are now assigned by
a trusted authority (the board file, DT, or ACPI firmware designer).
If that authority thinks that, for any reason, a GPIO ought to be
shared by two devices because the driver knows how to handle this, or
because it is guaranteed that the two functions will never be used at
the same time, who are we to forbid this? We had other requests to
allow limited GPIO sharing in the past, and this should also satisfy
them.

However, there is at least one loophole since one can currently forge
a GPIO number, call gpio_to_desc() on it, and hijack any GPIO they
want. All the same, one can directly call gpio_get_value() and spoof
on any currently-requested GPIO value. This is not a new problem - but
the current issue might give us a chance to address it.

How about we add a SHAREABLE flag to the descriptors that indicates
whether a GPIO can be shared. I can then see one of the following two
policies being applied:

1) GPIOs are marked shareable individually by their first user, and
regulator would do this for its enable_gpio. I'm not sure I am fond of
this one as it requires explicit driver support.
2) GPIOs are shareable by default, and remain that way as long as they
are obtained using the gpiod interface exclusively. A call to
gpio_to_desc() or gpio_request_one() would mark the GPIO as
non-shareable until it is released by the integer interface (and would
fail if the GPIO is already requested by gpiod). Any integer interface
call on a gpiod-requested GPIO would now simply fail with -EPERM.

I am not sure whether 2) covers all the requirements to allow gpiod to
be "secure", but if it does, then I suggest we give it a shot
(although at least a few drivers will certainly need fixing, but such
is life). This would add a very welcome layer of security and
guarantee gpiod users that nobody can spoof or mess otherwise with
their GPIOs unless the board or firmware designer allows it.

Oh and as a side-effect it should also fix the present issue.

Alex.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-07-01  0:59       ` Alexandre Courbot
@ 2014-07-01 11:10         ` Mark Brown
  2014-07-01 12:27           ` Alexandre Courbot
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2014-07-01 11:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jul 01, 2014 at 09:59:01AM +0900, Alexandre Courbot wrote:

> We could compare on the (dev, func, index) triplet instead of the GPIO
> number, but that looks a little bit overkill (and won't work if we mix
> integers and descriptors).

This only works if requests always come from the same triplet - if we're
requesting in the client context that doesn't work.

> How about we add a SHAREABLE flag to the descriptors that indicates
> whether a GPIO can be shared. I can then see one of the following two
> policies being applied:

> 2) GPIOs are shareable by default, and remain that way as long as they
> are obtained using the gpiod interface exclusively. A call to
> gpio_to_desc() or gpio_request_one() would mark the GPIO as
> non-shareable until it is released by the integer interface (and would
> fail if the GPIO is already requested by gpiod). Any integer interface
> call on a gpiod-requested GPIO would now simply fail with -EPERM.

This seems sensible to me.

> I am not sure whether 2) covers all the requirements to allow gpiod to
> be "secure", but if it does, then I suggest we give it a shot
> (although at least a few drivers will certainly need fixing, but such
> is life). This would add a very welcome layer of security and
> guarantee gpiod users that nobody can spoof or mess otherwise with
> their GPIOs unless the board or firmware designer allows it.

I don't see security as a big deal - so long as normal, well written
code gets the benefit of the access protection I think we're doing
enough.  If something jumpst through hoops to bypass it then it's buggy
but fundamentally we have to trust the kernel to at least some extent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140701/f0588b31/attachment.sig>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-07-01 11:10         ` Mark Brown
@ 2014-07-01 12:27           ` Alexandre Courbot
  2014-07-03 22:07             ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2014-07-01 12:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jul 1, 2014 at 8:10 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 01, 2014 at 09:59:01AM +0900, Alexandre Courbot wrote:
>
>> We could compare on the (dev, func, index) triplet instead of the GPIO
>> number, but that looks a little bit overkill (and won't work if we mix
>> integers and descriptors).
>
> This only works if requests always come from the same triplet - if we're
> requesting in the client context that doesn't work.

Indeed.

>
>> How about we add a SHAREABLE flag to the descriptors that indicates
>> whether a GPIO can be shared. I can then see one of the following two
>> policies being applied:
>
>> 2) GPIOs are shareable by default, and remain that way as long as they
>> are obtained using the gpiod interface exclusively. A call to
>> gpio_to_desc() or gpio_request_one() would mark the GPIO as
>> non-shareable until it is released by the integer interface (and would
>> fail if the GPIO is already requested by gpiod). Any integer interface
>> call on a gpiod-requested GPIO would now simply fail with -EPERM.
>
> This seems sensible to me.
>
>> I am not sure whether 2) covers all the requirements to allow gpiod to
>> be "secure", but if it does, then I suggest we give it a shot
>> (although at least a few drivers will certainly need fixing, but such
>> is life). This would add a very welcome layer of security and
>> guarantee gpiod users that nobody can spoof or mess otherwise with
>> their GPIOs unless the board or firmware designer allows it.
>
> I don't see security as a big deal - so long as normal, well written
> code gets the benefit of the access protection I think we're doing
> enough.  If something jumpst through hoops to bypass it then it's buggy
> but fundamentally we have to trust the kernel to at least some extent.

The security consideration apart, making GPIOs obtained through gpiod
shareable seems to be a good way to solve the issue we are having
here.

Linus, if you agree with the idea I can try to give it a shot sometime
this week.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-07-01 12:27           ` Alexandre Courbot
@ 2014-07-03 22:07             ` Linus Walleij
  2014-07-04  5:52               ` Alexandre Courbot
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2014-07-03 22:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jul 1, 2014 at 2:27 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Jul 1, 2014 at 8:10 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Jul 01, 2014 at 09:59:01AM +0900, Alexandre Courbot wrote:

>> I don't see security as a big deal - so long as normal, well written
>> code gets the benefit of the access protection I think we're doing
>> enough.  If something jumpst through hoops to bypass it then it's buggy
>> but fundamentally we have to trust the kernel to at least some extent.
>
> The security consideration apart, making GPIOs obtained through gpiod
> shareable seems to be a good way to solve the issue we are having
> here.
>
> Linus, if you agree with the idea I can try to give it a shot sometime
> this week.

I'm game for this approach. I hope this can get Russell's original
problem out of the way.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-07-03 22:07             ` Linus Walleij
@ 2014-07-04  5:52               ` Alexandre Courbot
  2014-07-04 19:15                 ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2014-07-04  5:52 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jul 4, 2014 at 7:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jul 1, 2014 at 2:27 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Jul 1, 2014 at 8:10 PM, Mark Brown <broonie@kernel.org> wrote:
>>> On Tue, Jul 01, 2014 at 09:59:01AM +0900, Alexandre Courbot wrote:
>
>>> I don't see security as a big deal - so long as normal, well written
>>> code gets the benefit of the access protection I think we're doing
>>> enough.  If something jumpst through hoops to bypass it then it's buggy
>>> but fundamentally we have to trust the kernel to at least some extent.
>>
>> The security consideration apart, making GPIOs obtained through gpiod
>> shareable seems to be a good way to solve the issue we are having
>> here.
>>
>> Linus, if you agree with the idea I can try to give it a shot sometime
>> this week.
>
> I'm game for this approach. I hope this can get Russell's original
> problem out of the way.

Great, I will give it a shot then. Could you let me know what you
think about my patch series that moves the different GPIO APIs into
their own files, so I know which base I should work on? IMHO if we are
to implement different behaviors for the integer and descriptor
interfaces, it makes all the more sense to have them in different
source files.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] regulator: convert to use gpio_desc internally
  2014-07-04  5:52               ` Alexandre Courbot
@ 2014-07-04 19:15                 ` Linus Walleij
  0 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2014-07-04 19:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Jul 4, 2014 at 7:52 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> Could you let me know what you
> think about my patch series that moves the different GPIO APIs into
> their own files, so I know which base I should work on?

I will get to it. Apart from being a bit snowed over I'm also technically
on vacation :-D

But I enjoy doing GPIO so...

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-07-04 19:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-29 16:55 [PATCH] regulator: convert to use gpio_desc internally Russell King
2014-06-30 10:06 ` Mark Brown
2014-06-30 16:31 ` Linus Walleij
2014-06-30 16:39   ` Russell King - ARM Linux
2014-06-30 16:49     ` Linus Walleij
2014-07-01  0:59       ` Alexandre Courbot
2014-07-01 11:10         ` Mark Brown
2014-07-01 12:27           ` Alexandre Courbot
2014-07-03 22:07             ` Linus Walleij
2014-07-04  5:52               ` Alexandre Courbot
2014-07-04 19:15                 ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.