Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Davide Hug]

Hi,

I'm looking at a driver (hwmon/sht15) that does not load with message:

	gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output

This happens because the driver uses a GPIO line alternately (and not at the
same time) as IRQ and output. To do this irq_enable/disable are used to
prevent setting a GPIO with enabled IRQ as output. This seems the right
approach to me since in the patch "gpio: add API to be strict about GPIO IRQ
usage"[1] I read:

	The API is symmetric so that lines can also be flagged from
	.irq_enable() and unflagged from IRQ by .irq_disable().

But I didn't find any GPIO driver implementing this API in irq_enable/disable.

Since my platform uses omap_gpio which does not implement
irq_enable/disable at all, I added a default irq_enable/disable
implementation in gpio/gpiolib.c (in the patch below).

Is it the right approach to call gpiochip_lock_as_irq/gpiochip_unlock_as_irq
in irq_enable/irq_disable?

Thanks, Davide

[1]: https://github.com/torvalds/linux/commit/d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1



diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8b4d721d6d63..c1db0461f29d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1678,6 +1678,22 @@ static void gpiochip_irq_relres(struct irq_data *d)
 	module_put(chip->gpiodev->owner);
 }
 
+static void gpiochip_irq_disable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	gpiochip_unlock_as_irq(chip, data->hwirq);
+	data->chip->irq_mask(data);
+}
+
+static void gpiochip_irq_enable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	gpiochip_lock_as_irq(chip, data->hwirq);
+	data->chip->irq_unmask(data);
+}
+
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	return irq_find_mapping(chip->irqdomain, offset);
@@ -1714,6 +1730,8 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 	if (gpiochip->irqchip) {
 		gpiochip->irqchip->irq_request_resources = NULL;
 		gpiochip->irqchip->irq_release_resources = NULL;
+		gpiochip->irqchip->irq_disable = NULL;
+		gpiochip->irqchip->irq_enable  = NULL;
 		gpiochip->irqchip = NULL;
 	}
 
@@ -1814,6 +1832,11 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 		irqchip->irq_request_resources = gpiochip_irq_reqres;
 		irqchip->irq_release_resources = gpiochip_irq_relres;
 	}
+	if (!irqchip->irq_disable &&
+	    !irqchip->irq_enable) {
+		irqchip->irq_disable = gpiochip_irq_disable;
+		irqchip->irq_enable  = gpiochip_irq_enable;
+	}
 
 	/*
 	 * Prepare the mapping since the irqchip shall be orthogonal to

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Linus Walleij]

On Tue, Jul 11, 2017 at 7:55 PM, Davide Hug <d@videhug.ch> wrote:

> I'm looking at a driver (hwmon/sht15) that does not load with message:
>
>         gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output
>
> This happens because the driver uses a GPIO line alternately (and not at the
> same time) as IRQ and output. To do this irq_enable/disable are used to
> prevent setting a GPIO with enabled IRQ as output. This seems the right
> approach to me since in the patch "gpio: add API to be strict about GPIO IRQ
> usage"[1] I read:
>
>         The API is symmetric so that lines can also be flagged from
>         .irq_enable() and unflagged from IRQ by .irq_disable().
>
> But I didn't find any GPIO driver implementing this API in irq_enable/disable.
>
> Since my platform uses omap_gpio which does not implement
> irq_enable/disable at all, I added a default irq_enable/disable
> implementation in gpio/gpiolib.c (in the patch below).
>
> Is it the right approach to call gpiochip_lock_as_irq/gpiochip_unlock_as_irq

We already call this in gpiochip_irq_reqres() and gpiochip_irq_relres().
from the .irq_request_resources()/.irq_release_resources() callbacks.

Your patch makes us call the functions twice I think.

Why isn't gpiochip_irq_relres() called before you reuse the line
for something else than IRQ?

If there is a legitimate reason why .irq_disable() and .irq_enable()
are called but not .irq_request_resources() and .irq_release_resources()
I want to know why that is so.

TGLX implemented these functions in the first place exactly to
be used for gpiochip_lock_as_irq()/gpiochip_unlock_as_irq(), see:
commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5
"genirq: Provide irq_request/release_resources chip callbacks"

Then we switch in
commit 57ef04288abd27a717287a652d324f95cb77c3c6
"gpio: switch drivers to use new callback"

At the time we were calling this from .irq_startup() and .irq_shutdown()
and I am afraid that .irq_enable() and .irq_disable() will just recreate
the same problem we had in 2014.

Yours,
Linus Walleij

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Grygorii Strashko]

On 07/12/2017 06:35 AM, Linus Walleij wrote:
> On Tue, Jul 11, 2017 at 7:55 PM, Davide Hug <d@videhug.ch> wrote:
> 
>> I'm looking at a driver (hwmon/sht15) that does not load with message:
>>
>>          gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output
>>
>> This happens because the driver uses a GPIO line alternately (and not at the
>> same time) as IRQ and output. To do this irq_enable/disable are used to
>> prevent setting a GPIO with enabled IRQ as output. This seems the right
>> approach to me since in the patch "gpio: add API to be strict about GPIO IRQ
>> usage"[1] I read:
>>
>>          The API is symmetric so that lines can also be flagged from
>>          .irq_enable() and unflagged from IRQ by .irq_disable().
>>
>> But I didn't find any GPIO driver implementing this API in irq_enable/disable.
>>
>> Since my platform uses omap_gpio which does not implement
>> irq_enable/disable at all, I added a default irq_enable/disable
>> implementation in gpio/gpiolib.c (in the patch below).
>>
>> Is it the right approach to call gpiochip_lock_as_irq/gpiochip_unlock_as_irq

It might solve your problem, but this is not the way to move forward :(
Looks like, hwmon/sht15 is very specific driver ;)
and, honestly, I think it better to use polling instead of IRQs.


> 
> We already call this in gpiochip_irq_reqres() and gpiochip_irq_relres().
> from the .irq_request_resources()/.irq_release_resources() callbacks.
> 
> Your patch makes us call the functions twice I think.
> 
> Why isn't gpiochip_irq_relres() called before you reuse the line
> for something else than IRQ?
> 
> If there is a legitimate reason why .irq_disable() and .irq_enable()
> are called but not .irq_request_resources() and .irq_release_resources()
> I want to know why that is so.
> 
> TGLX implemented these functions in the first place exactly to
> be used for gpiochip_lock_as_irq()/gpiochip_unlock_as_irq(), see:
> commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5
> "genirq: Provide irq_request/release_resources chip callbacks"
> 
> Then we switch in
> commit 57ef04288abd27a717287a652d324f95cb77c3c6
> "gpio: switch drivers to use new callback"
> 
> At the time we were calling this from .irq_startup() and .irq_shutdown()
> and I am afraid that .irq_enable() and .irq_disable() will just recreate
> the same problem we had in 2014.
> 

And they are always called from atomic context

-- 
regards,
-grygorii

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Thomas Gleixner]

On Wed, 12 Jul 2017, Linus Walleij wrote:
> On Tue, Jul 11, 2017 at 7:55 PM, Davide Hug <d@videhug.ch> wrote:
> 
> > I'm looking at a driver (hwmon/sht15) that does not load with message:
> >
> >         gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output
> >
> > This happens because the driver uses a GPIO line alternately (and not at the
> > same time) as IRQ and output. To do this irq_enable/disable are used to
> > prevent setting a GPIO with enabled IRQ as output. This seems the right
> > approach to me since in the patch "gpio: add API to be strict about GPIO IRQ
> > usage"[1] I read:
> >
> >         The API is symmetric so that lines can also be flagged from
> >         .irq_enable() and unflagged from IRQ by .irq_disable().
> >
> > But I didn't find any GPIO driver implementing this API in irq_enable/disable.
> >
> > Since my platform uses omap_gpio which does not implement
> > irq_enable/disable at all, I added a default irq_enable/disable
> > implementation in gpio/gpiolib.c (in the patch below).
> >
> > Is it the right approach to call gpiochip_lock_as_irq/gpiochip_unlock_as_irq
> 
> We already call this in gpiochip_irq_reqres() and gpiochip_irq_relres().
> from the .irq_request_resources()/.irq_release_resources() callbacks.
> 
> Your patch makes us call the functions twice I think.
> 
> Why isn't gpiochip_irq_relres() called before you reuse the line
> for something else than IRQ?
>
> 
> If there is a legitimate reason why .irq_disable() and .irq_enable()
> are called but not .irq_request_resources() and .irq_release_resources()
> I want to know why that is so.

Well, because that driver is completely broken.

It does:

	ret = devm_gpio_request(&pdev->dev, data->pdata->gpio_data,
                                "SHT15 data");

	request_irq(gpio_to_irq(data->pdata->gpio_data),....);

and at random places it does:

	ret = gpio_direction_input(data->pdata->gpio_data);
	
	err = gpio_direction_output(data->pdata->gpio_data, 1);

and the 'protection' for that is to use disable_irq_nosync() and
enable_irq() at various places.

That of course breaks with the current semantics of the GPIO interrupt
facility, because it prevents switching a requested interrupt gpio line
from input to output.

The only sane option is to get rid of the interrupt and switch the whole
thing to timer based polling. I'm sure that's not more overhead and removes
half of the unpenetrable mess in that drivere.

Thanks,

	tglx


    

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Davide Hug]

Thanks. I probably should have pointed out that I'm just getting started.
I thought this driver that does not load would be a good place to do that. But
obviously a "completely broken" driver that "breaks with the current semantics
of the GPIO interrupt" is the wrong place to get started and learn how things
should be done.

So this driver should probably be rewritten using timer based polling and the
descriptor based gpio interface. But since it is a really specific driver for
sensors that are already discontinued it probably doesn't make much sense. And
I'm not yet up to the task anyway.

Are the drivers listed in Documentation/gpio/drivers-on-gpio.txt good examples
of GPIO consumer drivers?

Thanks again.
Davide

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Linus Walleij]

On Thu, Jul 13, 2017 at 10:17 PM, Davide Hug <d@videhug.ch> wrote:

> Thanks. I probably should have pointed out that I'm just getting started.
> I thought this driver that does not load would be a good place to do that. But
> obviously a "completely broken" driver that "breaks with the current semantics
> of the GPIO interrupt" is the wrong place to get started and learn how things
> should be done.
>
> So this driver should probably be rewritten using timer based polling and the
> descriptor based gpio interface. But since it is a really specific driver for
> sensors that are already discontinued it probably doesn't make much sense. And
> I'm not yet up to the task anyway.

I'm not so sure about that. You might be the only one with the right hardware
to fix the problem. Also it is never our intention to discourage anyone to
hack on the kernel.

If you have this hardware and know what you want to get out of it, go for it.

> Are the drivers listed in Documentation/gpio/drivers-on-gpio.txt good examples
> of GPIO consumer drivers?

I think some, I am fixing others, anything just including
<linux/gpio/consumer.h>
and no other GPIO header are usually good examples.

Yours,
Linus Walleij

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Davide Hug]

On Tue, 1 Aug 2017 09:37:12 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> I'm not so sure about that. You might be the only one with the right hardware
> to fix the problem. Also it is never our intention to discourage anyone to
> hack on the kernel.

You are right. I shouldn't give up that quickly.

I have tried a few things and I would like to ask if one of these is an appropriate approach.
Sorry this is long! If there is a better place for me to ask these, please point me there.

My main question is if "timer based polling" is the way to go, is my
approach with timers below (3.) appropriate?


Some info on the sensor
-----------------------

The sensor pulls the data line low to signal that the measurement data is ready
to be retrieved.

The datasheet[1] says the following concerning this data ready signal

> This takes a maximum of 20/80/320 ms for a 8/12/14bit measurement. The
> time varies with the speed of the internal oscillator and can be lower
> by up to 30%.

And

> The controller must wait for this Data Ready signal before restarting
> SCK to readout the data. Measurement data is stored until readout,
> therefore the controller can continue with other tasks and readout at
> its convenience.


My approaches
-------------

1. Naive approach: just `msleep` long enough.

static int sht15_measurement(struct sht15_data *data,
			     int command,
			     int timeout_msecs)
{

	(...)

	/* Just wait long enough for measurement to finish */
	if (command == SHT15_MEASURE_RH) {
		if (data->val_status & SHT15_STATUS_LOW_RESOLUTION)
			msleep(20);
		else
			msleep(80);
	} else {
		if (data->val_status & SHT15_STATUS_LOW_RESOLUTION)
			msleep(80);
		else
			msleep(320);
	}

	if (gpio_get_value(data->pdata->gpio_data)) {
		ret = sht15_connection_reset(data);
		if (ret)
			return ret;
		return -ETIME;
	}

	ret = sht15_read_data(data);

	(...)

}



2. Naive polling: `msleep` repeatedly in a loop and check for the data line to be low each time.

#define POLLING_INTERVAL=10 /* Is 10ms appropriate? */

(...)

static int sht15_measurement(struct sht15_data *data,
			     int command,
			     int timeout_msecs)
{

	(...)

	/* Poll for measurement-ready signal */
	for (i = 0; i <= timeout_msecs/POLLING_INTERVAL; ++i) {
		msleep(POLLING_INTERVAL);
		if (!gpio_get_value(data->pdata->gpio_data))
			break;
	}

	/* Timeout occurred */
	if (gpio_get_value(data->pdata->gpio_data)) {
		ret = sht15_connection_reset(data);
		if (ret)
			return ret;
		return -ETIME;
	}

	ret = sht15_read_data(data);

	(...)
}



3. Polling with timer: use a timer for polling and `wait_event_timeout` to wait for the polling to succeed.

#define POLLING_TIMER_INTERVAL=10 /* Is 10ms appropriate? */

(...)

static int sht15_measurement(struct sht15_data *data,
			     int command,
			     int timeout_msecs)
{

	(...)

	mod_timer(&data->polling_timer,
		  jiffies + msecs_to_jiffies(POLLING_TIMER_INTERVAL));

	ret = wait_event_timeout(data->wait_queue,
				 (data->measurement_state == SHT15_MEASUREMENT_READY),
				 msecs_to_jiffies(timeout_msecs));

	(...)


	ret = sht15_read_data(data);

	(...)
}

(...)

void poll_measurement_ready(unsigned long arg)
{
	struct sht15_data *data = (struct sht15_data *)arg;

	/* Check if the sensor is still measuring */
	if (gpio_get_value(data->pdata->gpio_data)) {
		mod_timer(&data->polling_timer,
			  jiffies + msecs_to_jiffies(POLLING_TIMER_INTERVAL));
	} else {
		data->measurement_state = SHT15_MEASUREMENT_READY;
		wake_up(&data->wait_queue);
	}
}


static int sht15_probe(struct platform_device *pdev)
{

(...)

	setup_timer(&data->polling_timer, poll_measurement_ready, (unsigned long)data);

(...)

}



4. Fix the irq problem in the current driver by calling `devm_request_irq` and
`devm_free_irq` instead of just using `disable_irq_nosync` and `enable_irq`,
to prevent setting as output a gpio requested as irq.



Stats
-----

I have some stats on 100 measurements for each approache above.

                 mean   min    max
1 naive sleep    0.443  0.438  0.444
2 naive polling  0.363  0.358  0.369
3 timer polling  0.344  0.334  0.357
4 irq fix        0.325  0.324  0.325

(This are times in seconds gathered with `time /sys/.../humidity1_input`)

Thanks!
Davide


[1]: https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/2_Humidity_Sensors/Sensirion_Humidity_Sensors_SHT1x_Datasheet_V5.pdf

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Linus Walleij]

On Sat, Aug 26, 2017 at 12:45 AM, Davide Hug <d@videhug.ch> wrote:
> On Tue, 1 Aug 2017 09:37:12 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> I'm not so sure about that. You might be the only one with the right hardware
>> to fix the problem. Also it is never our intention to discourage anyone to
>> hack on the kernel.
>
> You are right. I shouldn't give up that quickly.
>
> I have tried a few things and I would like to ask if one of these is an appropriate approach.
> Sorry this is long! If there is a better place for me to ask these, please point me there.
>
> My main question is if "timer based polling" is the way to go, is my
> approach with timers below (3.) appropriate?

...
> 1. Naive approach: just `msleep` long enough.

This is wrong atleastm since msleep() has no guarantees for the
time it sleeps, it's "this amount or more".

> 2. Naive polling: `msleep` repeatedly in a loop and check for the data line to be low each time.

Same problem.

> 3. Polling with timer: use a timer for polling and `wait_event_timeout` to wait for the polling to succeed.

This seems reasonable.

> 4. Fix the irq problem in the current driver by calling `devm_request_irq` and
> `devm_free_irq` instead of just using `disable_irq_nosync` and `enable_irq`,
> to prevent setting as output a gpio requested as irq.

This makes me feel there is something that needs to be fixed.

If IRQs are available we should always try to use them somehow.
I don't remember the discussuin fully, but I guess the timer in (3)
is started in response to an IRQ? That should work fine.

Yours,
Linus Walleij

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Davide Hug]

On Fri, 8 Sep 2017 16:22:40 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sat, Aug 26, 2017 at 12:45 AM, Davide Hug <d@videhug.ch> wrote:
> > 1. Naive approach: just `msleep` long enough.
> 
> This is wrong atleastm since msleep() has no guarantees for the
> time it sleeps, it's "this amount or more".

I read Documentation/timers/timers-howto.txt and msleep seemed the right choice
since it is suggested for delays >10ms and the measurement takes 20-230ms. Also
the sensor stores the measurement till it is retrieved so I thought it wouldn't
be a problem if msleep takes some more time.

Should I use usleep_range instead?

> 
> > 2. Naive polling: `msleep` repeatedly in a loop and check for the data line to be low each time.
> 
> Same problem.
> 
> > 3. Polling with timer: use a timer for polling and `wait_event_timeout` to wait for the polling to succeed.
> 
> This seems reasonable.
> 
> > 4. Fix the irq problem in the current driver by calling `devm_request_irq` and
> > `devm_free_irq` instead of just using `disable_irq_nosync` and `enable_irq`,
> > to prevent setting as output a gpio requested as irq.
> 
> This makes me feel there is something that needs to be fixed.
> 
> If IRQs are available we should always try to use them somehow.
> I don't remember the discussuin fully, but I guess the timer in (3)
> is started in response to an IRQ? That should work fine.

At each measurement the bidirectional data line is used by the sensor as data
ready signal. So the IRQ is used only to detect the data ready signal. After that
the IRQ is disabled to avoid triggering interrupts during the serial
communication.
In (3) the IRQ is replaced by polling. So it avoids using IRQs altogether. I
posted a patch implementing this in linux-hwmon.

Thanks
Davide

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Linus Walleij]

On Sun, Sep 10, 2017 at 4:16 PM, Davide Hug <d@videhug.ch> wrote:
> On Fri, 8 Sep 2017 16:22:40 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:

>> > 4. Fix the irq problem in the current driver by calling `devm_request_irq` and
>> > `devm_free_irq` instead of just using `disable_irq_nosync` and `enable_irq`,
>> > to prevent setting as output a gpio requested as irq.
>>
>> This makes me feel there is something that needs to be fixed.
>>
>> If IRQs are available we should always try to use them somehow.
>> I don't remember the discussuin fully, but I guess the timer in (3)
>> is started in response to an IRQ? That should work fine.
>
> At each measurement the bidirectional data line is used by the sensor as data
> ready signal. So the IRQ is used only to detect the data ready signal. After that
> the IRQ is disabled to avoid triggering interrupts during the serial
> communication.

That is a fully valid usecase and I think something gpiolib
should be able to support. If it bugs you, it will also soon bug
somebody else.

Somewhere the backing irqchip is talking to gpiochip by
issueing gpiochip_lock_as_irq() on this
line and it should be made to issue gpiochip_unlock_as_irq()
before used as serial line, and then gpiochip_lock_as_irq()
again when the go back to using it as irq.

Currently, this mostly gets done in the .irq_request_resources
and .irq_release_resources of the irqchip.

Intuitively I would say it should be done by
.irq_enable()/.irq_disable() but that has the problem that it
is fastpath, and not all GPIO irqchips can handle that.

Maybe we should do it in .irq_enable()/.irq_disable() in case
the .can_sleep bool on the gpiochip is not set, simply?
This way the irqchip should be able to lock/unlock the GPIO
lines dynamically.

We would need to patch a few drivers, but the generic code
for simple gpio irqchips is in gpiolib.c and can be fixed once
and for all. (Which is kind of why I insist on drivers to use
that...)

Yours,
Linus Walleij

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Davide Hug]

I'm really sorry that I come back to this only now.

On Tue, 12 Sep 2017 15:57:47 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> > At each measurement the bidirectional data line is used by the sensor as data
> > ready signal. So the IRQ is used only to detect the data ready signal. After that
> > the IRQ is disabled to avoid triggering interrupts during the serial
> > communication.
> 
> That is a fully valid usecase and I think something gpiolib
> should be able to support. If it bugs you, it will also soon bug
> somebody else.

Reading through the mailing list from when irq_request/release_resources were
introduced I see how more involved this is than I would first think.
Also, from the discussion above I understand that for the hwmon/sht15 driver it
is acceptable to use a simple polling loop without irq.
But it seems that iio/humidity/dht11 is another driver with a similar
situation.

> Somewhere the backing irqchip is talking to gpiochip by
> issueing gpiochip_lock_as_irq() on this
> line and it should be made to issue gpiochip_unlock_as_irq()
> before used as serial line, and then gpiochip_lock_as_irq()
> again when the go back to using it as irq.
> 
> Currently, this mostly gets done in the .irq_request_resources
> and .irq_release_resources of the irqchip.
> 
> Intuitively I would say it should be done by
> .irq_enable()/.irq_disable() but that has the problem that it
> is fastpath, and not all GPIO irqchips can handle that.

Do I understand this correctly? Some GPIO drivers can be slow and sleep, and
.irq_enable/disable can be called from places where this is not acceptable.

(I see that only gpio/gpio-pcf857x implements .irq_enable/disable and has
.can_sleep=true)

> Maybe we should do it in .irq_enable()/.irq_disable() in case
> the .can_sleep bool on the gpiochip is not set, simply?
> This way the irqchip should be able to lock/unlock the GPIO
> lines dynamically.

So, before a driver relays on locking/unlocking a GPIO line dynamically, the
driver would have to make sure that the GPIO has not .can_sleep=true. Right?

> We would need to patch a few drivers, but the generic code
> for simple gpio irqchips is in gpiolib.c and can be fixed once
> and for all. (Which is kind of why I insist on drivers to use
> that...)

With a default .irq_enable/disable implementation in gpiolib (something as in
the patch below) the GPIO driver implementing their own .irq_enable/disable
would have to be fixed, right?

Grepping around I get 12 drivers that call gpiochip_irqchip_add, that don't
have .can_sleep=true and implement their own .irq_enable/.irq_disable.
Also, 5 of these drivers only implement either .irq_enable or .irq_disable. This
would be a problem with my patch below which assumes that both are implemented
or neither.

> comm -3 <(grep -r 'gpiochip_irqchip_add' drivers/ -l | xargs grep "\.irq_enable" -l |sort) \
>         <(grep -r 'gpiochip_irqchip_add' drivers/ -l | xargs grep "\.irq_disable" -l |sort)
		drivers/gpio/gpio-xlp.c
drivers/gpio/gpio-zynq.c
drivers/pinctrl/intel/pinctrl-intel.c
		drivers/pinctrl/pinctrl-at91.c
		drivers/pinctrl/pinctrl-st.c


Thanks,
Davide




diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index aad84a6306c4..2435654d2ffd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1701,6 +1701,24 @@ static void gpiochip_irq_relres(struct irq_data *d)
 	module_put(chip->gpiodev->owner);
 }
 
+static void gpiochip_irq_disable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	gpiochip_unlock_as_irq(chip, data->hwirq);
+	if (data->chip->irq_mask)
+		data->chip->irq_mask(data);
+}
+
+static void gpiochip_irq_enable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	gpiochip_lock_as_irq(chip, data->hwirq);
+	if (data->chip->irq_unmask)
+		data->chip->irq_unmask(data);
+}
+
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
@@ -1834,6 +1852,8 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 	if (gpiochip->irq.chip) {
 		gpiochip->irq.chip->irq_request_resources = NULL;
 		gpiochip->irq.chip->irq_release_resources = NULL;
+		gpiochip->irq.chip->irq_disable = NULL;
+		gpiochip->irq.chip->irq_enable  = NULL;
 		gpiochip->irq.chip = NULL;
 	}
 
@@ -1931,6 +1951,13 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 		irqchip->irq_release_resources = gpiochip_irq_relres;
 	}
 
+	if (!gpiochip->can_sleep &&
+	    !irqchip->irq_disable &&
+	    !irqchip->irq_enable) {
+		irqchip->irq_disable = gpiochip_irq_disable;
+		irqchip->irq_enable  = gpiochip_irq_enable;
+	}
+
 	acpi_gpiochip_request_interrupts(gpiochip);
 
 	return 0;

Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Linus Walleij]

On Mon, Nov 20, 2017 at 11:48 PM, Davide Hug <d@videhug.ch> wrote:

> I'm really sorry that I come back to this only now.

Business as usual. We are all creative people with lots to do, do
not worry.

> Do I understand this correctly? Some GPIO drivers can be slow and sleep, and
> .irq_enable/disable can be called from places where this is not acceptable.

That is the essence of it I guess.

> (I see that only gpio/gpio-pcf857x implements .irq_enable/disable and has
> .can_sleep=true)

The crucial thing is  that PCF857x is an I2C expander GPIO chip.

I2C traffic is blocking, so cannot be called in IRQ context.

As long as the .irq_enable/.irq_disable callbacks does not
start to issue something blocking, like I2C traffic, e.g. reading
and writing registers in the chip, all is fine.

>> Maybe we should do it in .irq_enable()/.irq_disable() in case
>> the .can_sleep bool on the gpiochip is not set, simply?
>> This way the irqchip should be able to lock/unlock the GPIO
>> lines dynamically.
>
> So, before a driver relays on locking/unlocking a GPIO line dynamically, the
> driver would have to make sure that the GPIO has not .can_sleep=true. Right?

Something like that. I suspect you know it better than me
by now, I am rusty all the time on details.

I also changed the accessor gpiod_set/get_raw_value() so these
will work even if the line is used for IRQ. These are really violent
accessors, but they have their place.

Please check if that is something that would help your
usecase before venturing to deep into the infrastructure.

> With a default .irq_enable/disable implementation in gpiolib (something as in
> the patch below) the GPIO driver implementing their own .irq_enable/disable
> would have to be fixed, right?

Yeah. But I don't know if the solution is very good, because
it duct-tapes on a solution that will only work if the gpiochip is
non-sleeping. The driver will simply fail randomly on
sleeping cpiochips which is not what we want, we want GPIOs
and their IRQs to uniformly behave the same no matter
which GPIO driver and consumer combo we make.

Yours,
Linus Walleij