LinuxLists.cc - [PATCH 0/2] change "client->irq >= 0" to "client->irq

2015-06-02 21:34:57

Subject: [PATCH 0/2] change "client->irq >= 0" to "client->irq > 0"

This fixes an issue introduces by commit dab472eb931b ("i2c / ACPI:
Use 0 to indicate that device does not have interrupt assigned") where
drivers will try to request IRQ 0 when no GpioInt is defined in ACPI.

The same issue occurs when the device is instantiated via device tree
with no IRQ, or from the i2c sysfs interface, even before the patch
above.

Linus, since the commit above was already merged in the GPIO tree,
should these fixes be merged also via the GPIO tree (with ACKs from
the others subsystem maintainers)?

Octavian Purdila (2):
iio: change "client->irq >= 0" to "client->irq > 0"
rtc: change "client->irq >= 0" to "client->irq > 0"

drivers/iio/accel/bmc150-accel.c | 2 +-
drivers/iio/accel/kxcjk-1013.c | 2 +-
drivers/iio/accel/mma9553.c | 2 +-
drivers/iio/imu/kmx61.c | 8 ++++----
drivers/rtc/rtc-ds1374.c | 4 ++--
drivers/rtc/rtc-ds3232.c | 2 +-
6 files changed, 10 insertions(+), 10 deletions(-)

--
1.9.1

2015-06-02 21:35:06

by Octavian Purdila

[permalink] [raw]

Subject: [PATCH 1/2] iio: change "client->irq >= 0" to "client->irq > 0"

Since commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device
does not have interrupt assigned"), 0 is not a valid i2c client irq
anymore, so change all driver's checks accordingly.

Signed-off-by: Octavian Purdila <[email protected]>
---
drivers/iio/accel/bmc150-accel.c | 2 +-
drivers/iio/accel/kxcjk-1013.c | 2 +-
drivers/iio/accel/mma9553.c | 2 +-
drivers/iio/imu/kmx61.c | 8 ++++----
4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 4e70f51..55751d9 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -1663,7 +1663,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
if (client->irq < 0)
client->irq = bmc150_accel_gpio_probe(client, data);

- if (client->irq >= 0) {
+ if (client->irq > 0) {
ret = devm_request_threaded_irq(
&client->dev, client->irq,
bmc150_accel_irq_handler,
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 0d9bd35..aa93cbd 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1243,7 +1243,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
if (client->irq < 0)
client->irq = kxcjk1013_gpio_probe(client, data);

- if (client->irq >= 0) {
+ if (client->irq > 0) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
kxcjk1013_data_rdy_trig_poll,
kxcjk1013_event_handler,
diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
index 9d649c4..df043b3 100644
--- a/drivers/iio/accel/mma9553.c
+++ b/drivers/iio/accel/mma9553.c
@@ -1143,7 +1143,7 @@ static int mma9553_probe(struct i2c_client *client,
if (client->irq < 0)
client->irq = mma9553_gpio_probe(client);

- if (client->irq >= 0) {
+ if (client->irq > 0) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
mma9553_irq_handler,
mma9553_event_handler,
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index 462a010..82cdf50 100644
--- a/drivers/iio/imu/kmx61.c
+++ b/drivers/iio/imu/kmx61.c
@@ -1363,7 +1363,7 @@ static int kmx61_probe(struct i2c_client *client,
if (client->irq < 0)
client->irq = kmx61_gpio_probe(client, data);

- if (client->irq >= 0) {
+ if (client->irq > 0) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
kmx61_data_rdy_trig_poll,
kmx61_event_handler,
@@ -1445,10 +1445,10 @@ err_iio_unregister_mag:
err_iio_unregister_acc:
iio_device_unregister(data->acc_indio_dev);
err_buffer_cleanup_mag:
- if (client->irq >= 0)
+ if (client->irq > 0)
iio_triggered_buffer_cleanup(data->mag_indio_dev);
err_buffer_cleanup_acc:
- if (client->irq >= 0)
+ if (client->irq > 0)
iio_triggered_buffer_cleanup(data->acc_indio_dev);
err_trigger_unregister_motion:
iio_trigger_unregister(data->motion_trig);
@@ -1472,7 +1472,7 @@ static int kmx61_remove(struct i2c_client *client)
iio_device_unregister(data->acc_indio_dev);
iio_device_unregister(data->mag_indio_dev);

- if (client->irq >= 0) {
+ if (client->irq > 0) {
iio_triggered_buffer_cleanup(data->acc_indio_dev);
iio_triggered_buffer_cleanup(data->mag_indio_dev);
iio_trigger_unregister(data->acc_dready_trig);
--
1.9.1

2015-06-02 21:35:14

by Octavian Purdila

[permalink] [raw]

Subject: [PATCH 2/2] rtc: change "client->irq >= 0" to "client->irq > 0"

2015-06-02 22:54:41

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [PATCH 2/2] rtc: change "client->irq >= 0" to "client->irq > 0"

Hi,

On 03/06/2015 at 00:34:13 +0300, Octavian Purdila wrote :
> Since commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device

You can't reference that commit ID as Linus Torvalds didn't pull that
patch yet.

I'd also prefer a more descriptive subject line.

> does not have interrupt assigned"), 0 is not a valid i2c client irq
> anymore, so change all driver's checks accordingly.
>
> Signed-off-by: Octavian Purdila <[email protected]>
> ---
> drivers/rtc/rtc-ds1374.c | 4 ++--
> drivers/rtc/rtc-ds3232.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
> index 167783f..592458c 100644
> --- a/drivers/rtc/rtc-ds1374.c
> +++ b/drivers/rtc/rtc-ds1374.c
> @@ -689,7 +689,7 @@ static int ds1374_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
>
> - if (client->irq >= 0 && device_may_wakeup(&client->dev))
> + if (client->irq > 0 && device_may_wakeup(&client->dev))
> enable_irq_wake(client->irq);
> return 0;
> }
> @@ -698,7 +698,7 @@ static int ds1374_resume(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
>
> - if (client->irq >= 0 && device_may_wakeup(&client->dev))
> + if (client->irq > 0 && device_may_wakeup(&client->dev))
> disable_irq_wake(client->irq);
> return 0;
> }
> diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
> index 7e48e53..f280dd1 100644
> --- a/drivers/rtc/rtc-ds3232.c
> +++ b/drivers/rtc/rtc-ds3232.c
> @@ -443,7 +443,7 @@ static int ds3232_remove(struct i2c_client *client)
> {
> struct ds3232 *ds3232 = i2c_get_clientdata(client);
>
> - if (client->irq >= 0) {
> + if (client->irq > 0) {
> mutex_lock(&ds3232->mutex);
> ds3232->exiting = 1;
> mutex_unlock(&ds3232->mutex);
> --
> 1.9.1
>

--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2015-06-03 10:27:30

by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCH 1/2] iio: change "client->irq >= 0" to "client->irq > 0"

On Wed, Jun 03, 2015 at 12:34:12AM +0300, Octavian Purdila wrote:
> Since commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device
> does not have interrupt assigned"), 0 is not a valid i2c client irq
> anymore, so change all driver's checks accordingly.
>
> Signed-off-by: Octavian Purdila <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2015-06-03 10:32:32

by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCH 2/2] rtc: change "client->irq >= 0" to "client->irq > 0"

On Wed, Jun 03, 2015 at 12:34:13AM +0300, Octavian Purdila wrote:
> Since commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device
> does not have interrupt assigned"), 0 is not a valid i2c client irq
> anymore, so change all driver's checks accordingly.
>
> Signed-off-by: Octavian Purdila <[email protected]>

In addition to Alexandre's comments, the change itself looks good to me
so once you address them,

Reviewed-by: Mika Westerberg <[email protected]>

Thanks for taking care of this.

2015-06-03 11:02:54

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [PATCH 0/2] change "client->irq >= 0" to "client->irq > 0"

On 03/06/2015 at 00:34:11 +0300, Octavian Purdila wrote :
> This fixes an issue introduces by commit dab472eb931b ("i2c / ACPI:
> Use 0 to indicate that device does not have interrupt assigned") where
> drivers will try to request IRQ 0 when no GpioInt is defined in ACPI.
>
> The same issue occurs when the device is instantiated via device tree
> with no IRQ, or from the i2c sysfs interface, even before the patch
> above.
>
> Linus, since the commit above was already merged in the GPIO tree,
> should these fixes be merged also via the GPIO tree (with ACKs from
> the others subsystem maintainers)?
>

Side question, has it been considered that IRQ 0 is valid on some
platform and that means i2c devices will not be able to be wired to that
IRQ anymore? Though, I don't think there are any existing design that
does so.

--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2015-06-03 17:06:07

by Octavian Purdila

[permalink] [raw]

Subject: Re: [PATCH 0/2] change "client->irq >= 0" to "client->irq > 0"

On Wed, Jun 3, 2015 at 2:02 PM, Alexandre Belloni
<[email protected]> wrote:
> On 03/06/2015 at 00:34:11 +0300, Octavian Purdila wrote :
>> This fixes an issue introduces by commit dab472eb931b ("i2c / ACPI:
>> Use 0 to indicate that device does not have interrupt assigned") where
>> drivers will try to request IRQ 0 when no GpioInt is defined in ACPI.
>>
>> The same issue occurs when the device is instantiated via device tree
>> with no IRQ, or from the i2c sysfs interface, even before the patch
>> above.
>>
>> Linus, since the commit above was already merged in the GPIO tree,
>> should these fixes be merged also via the GPIO tree (with ACKs from
>> the others subsystem maintainers)?
>>
>
> Side question, has it been considered that IRQ 0 is valid on some
> platform and that means i2c devices will not be able to be wired to that
> IRQ anymore? Though, I don't think there are any existing design that
> does so.
>

Device tree instantiation does not allow you to used IRQ 0 anyway. And
here is what Linus said about this:

http://yarchive.net/comp/linux/no_irq.html

2015-06-03 17:14:09

by Lars-Peter Clausen

[permalink] [raw]

Subject: Re: [PATCH 0/2] change "client->irq >= 0" to "client->irq > 0"

On 06/03/2015 01:02 PM, Alexandre Belloni wrote:
> On 03/06/2015 at 00:34:11 +0300, Octavian Purdila wrote :
>> This fixes an issue introduces by commit dab472eb931b ("i2c / ACPI:
>> Use 0 to indicate that device does not have interrupt assigned") where
>> drivers will try to request IRQ 0 when no GpioInt is defined in ACPI.
>>
>> The same issue occurs when the device is instantiated via device tree
>> with no IRQ, or from the i2c sysfs interface, even before the patch
>> above.
>>
>> Linus, since the commit above was already merged in the GPIO tree,
>> should these fixes be merged also via the GPIO tree (with ACKs from
>> the others subsystem maintainers)?
>>
>
> Side question, has it been considered that IRQ 0 is valid on some
> platform and that means i2c devices will not be able to be wired to that
> IRQ anymore? Though, I don't think there are any existing design that
> does so.

If IRQ 0 is valid, that's a bug. IRQ 0 has been an invalid IRQ number in the
global IRQ namespace for a while now. Though architectures are allowed to
have a valid IRQ 0, but it may only be used inside the architecture code
itself and must not be used for IRQs that are potentially be used by drivers.

- Lars

2015-06-03 19:20:37

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [PATCH 0/2] change "client->irq >= 0" to "client->irq > 0"

On 03/06/2015 at 20:05:56 +0300, Octavian Purdila wrote :
> On Wed, Jun 3, 2015 at 2:02 PM, Alexandre Belloni
> <[email protected]> wrote:
> > On 03/06/2015 at 00:34:11 +0300, Octavian Purdila wrote :
> >> This fixes an issue introduces by commit dab472eb931b ("i2c / ACPI:
> >> Use 0 to indicate that device does not have interrupt assigned") where
> >> drivers will try to request IRQ 0 when no GpioInt is defined in ACPI.
> >>
> >> The same issue occurs when the device is instantiated via device tree
> >> with no IRQ, or from the i2c sysfs interface, even before the patch
> >> above.
> >>
> >> Linus, since the commit above was already merged in the GPIO tree,
> >> should these fixes be merged also via the GPIO tree (with ACKs from
> >> the others subsystem maintainers)?
> >>
> >
> > Side question, has it been considered that IRQ 0 is valid on some
> > platform and that means i2c devices will not be able to be wired to that
> > IRQ anymore? Though, I don't think there are any existing design that
> > does so.
> >
>
> Device tree instantiation does not allow you to used IRQ 0 anyway. And
> here is what Linus said about this:
>
> http://yarchive.net/comp/linux/no_irq.html

I'm pretty sure his point doesn't hold anymore 10 years later. I don't
believe ARM is "the small percentage of a small percentage of a small
percentage" anymore and it is probably more tested than it was at the
time. Anyway, I'm fine with the change, you can add my
Acked-by: Alexandre Belloni <[email protected]>
for your v2.

--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com