[PATCH] i2c: Allow recovery of the initial IRQ by a i2c client device.

[PATCH] i2c: Allow recovery of the initial IRQ by a i2c client device.

[Jim Broadus]

A previous change allowed i2c client devices to discover new IRQs upon
reprobe. By clearing the IRQ in i2c_device_remove. However, if an IRQ was
assigned in i2c_new_device, that information is lost.

For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
client device structures are initialized during an ACPI walk. After
removing the i2c_hid device, modprobe fails.

This change caches the initial IRQ value in i2c_new_device and then resets
the client device IRQ to the initial value in i2c_device_remove.

Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
Signed-off-by: Jim Broadus <jbroadus@gmail.com>
---
 drivers/i2c/i2c-core-base.c | 9 +++++----
 include/linux/i2c.h         | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 28460f6a60cc..af87a16ac3a5 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -430,7 +430,7 @@ static int i2c_device_remove(struct device *dev)
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
 
-	client->irq = 0;
+	client->irq = client->init_irq;
 
 	return status;
 }
@@ -741,10 +741,11 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->flags = info->flags;
 	client->addr = info->addr;
 
-	client->irq = info->irq;
-	if (!client->irq)
-		client->irq = i2c_dev_irq_from_resources(info->resources,
+	client->init_irq = info->irq;
+	if (!client->init_irq)
+		client->init_irq = i2c_dev_irq_from_resources(info->resources,
 							 info->num_resources);
+	client->irq = client->init_irq;
 
 	strlcpy(client->name, info->type, sizeof(client->name));
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..7e748648c7d3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -333,6 +333,7 @@ struct i2c_client {
 	char name[I2C_NAME_SIZE];
 	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
 	struct device dev;		/* the device structure		*/
+	int init_irq;			/* irq set at initialization	*/
 	int irq;			/* irq issued by device		*/
 	struct list_head detected;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
-- 
2.20.1

Re: [PATCH] i2c: Allow recovery of the initial IRQ by a i2c client device.

[Charles Keepax]

On Fri, Feb 15, 2019 at 04:15:33PM -0800, Jim Broadus wrote:
> A previous change allowed i2c client devices to discover new IRQs upon
> reprobe. By clearing the IRQ in i2c_device_remove. However, if an IRQ was
> assigned in i2c_new_device, that information is lost.
> 
> For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> client device structures are initialized during an ACPI walk. After
> removing the i2c_hid device, modprobe fails.
> 
> This change caches the initial IRQ value in i2c_new_device and then resets
> the client device IRQ to the initial value in i2c_device_remove.
> 
> Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Apologies for the issues caused. I think this looks like a good fix
to me.

Thanks,
Charles

Re: [PATCH] i2c: Allow recovery of the initial IRQ by a i2c client device.

[Jim Broadus]

Thank you Charles. I should probably correct the typos in my commit message.

On Mon, Feb 18, 2019 at 2:06 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Fri, Feb 15, 2019 at 04:15:33PM -0800, Jim Broadus wrote:
> > A previous change allowed i2c client devices to discover new IRQs upon
> > reprobe. By clearing the IRQ in i2c_device_remove. However, if an IRQ was
> > assigned in i2c_new_device, that information is lost.
> >
> > For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> > are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> > client device structures are initialized during an ACPI walk. After
> > removing the i2c_hid device, modprobe fails.
> >
> > This change caches the initial IRQ value in i2c_new_device and then resets
> > the client device IRQ to the initial value in i2c_device_remove.
> >
> > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> > Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> > ---
>
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>
> Apologies for the issues caused. I think this looks like a good fix
> to me.
>
> Thanks,
> Charles

[PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Jim Broadus]

A previous change allowed I2C client devices to discover new IRQs upon
reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
assigned in i2c_new_device, that information is lost.

For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
client device structures are initialized during an ACPI walk. After
removing the i2c_hid device, modprobe fails.

This change caches the initial IRQ value in i2c_new_device and then resets
the client device IRQ to the initial value in i2c_device_remove.

Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
Signed-off-by: Jim Broadus <jbroadus@gmail.com>
---
 drivers/i2c/i2c-core-base.c | 9 +++++----
 include/linux/i2c.h         | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 28460f6a60cc..af87a16ac3a5 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -430,7 +430,7 @@ static int i2c_device_remove(struct device *dev)
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
 
-	client->irq = 0;
+	client->irq = client->init_irq;
 
 	return status;
 }
@@ -741,10 +741,11 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->flags = info->flags;
 	client->addr = info->addr;
 
-	client->irq = info->irq;
-	if (!client->irq)
-		client->irq = i2c_dev_irq_from_resources(info->resources,
+	client->init_irq = info->irq;
+	if (!client->init_irq)
+		client->init_irq = i2c_dev_irq_from_resources(info->resources,
 							 info->num_resources);
+	client->irq = client->init_irq;
 
 	strlcpy(client->name, info->type, sizeof(client->name));
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..7e748648c7d3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -333,6 +333,7 @@ struct i2c_client {
 	char name[I2C_NAME_SIZE];
 	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
 	struct device dev;		/* the device structure		*/
+	int init_irq;			/* irq set at initialization	*/
 	int irq;			/* irq issued by device		*/
 	struct list_head detected;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
-- 
2.20.1

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Jim Broadus]

Apologies.. This should have been marked v2.

On Tue, Feb 19, 2019 at 11:30 AM Jim Broadus <jbroadus@gmail.com> wrote:
>
> A previous change allowed I2C client devices to discover new IRQs upon
> reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> assigned in i2c_new_device, that information is lost.
>
> For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> client device structures are initialized during an ACPI walk. After
> removing the i2c_hid device, modprobe fails.
>
> This change caches the initial IRQ value in i2c_new_device and then resets
> the client device IRQ to the initial value in i2c_device_remove.
>
> Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> ---
>  drivers/i2c/i2c-core-base.c | 9 +++++----
>  include/linux/i2c.h         | 1 +
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 28460f6a60cc..af87a16ac3a5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -430,7 +430,7 @@ static int i2c_device_remove(struct device *dev)
>         dev_pm_clear_wake_irq(&client->dev);
>         device_init_wakeup(&client->dev, false);
>
> -       client->irq = 0;
> +       client->irq = client->init_irq;
>
>         return status;
>  }
> @@ -741,10 +741,11 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>         client->flags = info->flags;
>         client->addr = info->addr;
>
> -       client->irq = info->irq;
> -       if (!client->irq)
> -               client->irq = i2c_dev_irq_from_resources(info->resources,
> +       client->init_irq = info->irq;
> +       if (!client->init_irq)
> +               client->init_irq = i2c_dev_irq_from_resources(info->resources,
>                                                          info->num_resources);
> +       client->irq = client->init_irq;
>
>         strlcpy(client->name, info->type, sizeof(client->name));
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..7e748648c7d3 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -333,6 +333,7 @@ struct i2c_client {
>         char name[I2C_NAME_SIZE];
>         struct i2c_adapter *adapter;    /* the adapter we sit on        */
>         struct device dev;              /* the device structure         */
> +       int init_irq;                   /* irq set at initialization    */
>         int irq;                        /* irq issued by device         */
>         struct list_head detected;
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> --
> 2.20.1
>

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> A previous change allowed I2C client devices to discover new IRQs upon
> reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> assigned in i2c_new_device, that information is lost.
> 
> For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> client device structures are initialized during an ACPI walk. After
> removing the i2c_hid device, modprobe fails.
> 
> This change caches the initial IRQ value in i2c_new_device and then resets
> the client device IRQ to the initial value in i2c_device_remove.
> 
> Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> Signed-off-by: Jim Broadus <jbroadus@gmail.com>

Adding Benjamin to CC

> ---
>  drivers/i2c/i2c-core-base.c | 9 +++++----
>  include/linux/i2c.h         | 1 +
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 28460f6a60cc..af87a16ac3a5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -430,7 +430,7 @@ static int i2c_device_remove(struct device *dev)
>  	dev_pm_clear_wake_irq(&client->dev);
>  	device_init_wakeup(&client->dev, false);
>  
> -	client->irq = 0;
> +	client->irq = client->init_irq;
>  
>  	return status;
>  }
> @@ -741,10 +741,11 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>  	client->flags = info->flags;
>  	client->addr = info->addr;
>  
> -	client->irq = info->irq;
> -	if (!client->irq)
> -		client->irq = i2c_dev_irq_from_resources(info->resources,
> +	client->init_irq = info->irq;
> +	if (!client->init_irq)
> +		client->init_irq = i2c_dev_irq_from_resources(info->resources,
>  							 info->num_resources);
> +	client->irq = client->init_irq;
>  
>  	strlcpy(client->name, info->type, sizeof(client->name));
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..7e748648c7d3 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -333,6 +333,7 @@ struct i2c_client {
>  	char name[I2C_NAME_SIZE];
>  	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
>  	struct device dev;		/* the device structure		*/
> +	int init_irq;			/* irq set at initialization	*/
>  	int irq;			/* irq issued by device		*/
>  	struct list_head detected;
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Benjamin Tissoires]

On Fri, Feb 22, 2019 at 12:26 AM Wolfram Sang <wsa@the-dreams.de> wrote:
>
> On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> > A previous change allowed I2C client devices to discover new IRQs upon
> > reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> > assigned in i2c_new_device, that information is lost.
> >
> > For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> > are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> > client device structures are initialized during an ACPI walk. After
> > removing the i2c_hid device, modprobe fails.
> >
> > This change caches the initial IRQ value in i2c_new_device and then resets
> > the client device IRQ to the initial value in i2c_device_remove.
> >
> > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> > Signed-off-by: Jim Broadus <jbroadus@gmail.com>
>
> Adding Benjamin to CC

Sorry, I should have answered earlier.

I am a little bit hesitant regarding this patch. The effect is
correct, and I indeed realized a few weeks ago that something were
wrong as we couldn't rmmod/modprobe i2c-hid.

But I still have the feeling that the problem is not solved at the
right place. In i2c_new_device() we are storing parts of the fields of
struct i2c_board_info, and when resetting the irq we are losing
information. This patch solves that, but I wonder if the IRQ should
not be 'simply' set in i2c_device_probe(). This means we also need to
store the .resources of info, but I have a feeling this will be less
error prone in the future.

But this is just my guts telling me something is not right. I would
perfectly understand if we want to get this merged ASAP.

So given that the code is correct, this is my:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

But at least I have expressed my feelings :)

Cheers,
Benjamin



>
> > ---
> >  drivers/i2c/i2c-core-base.c | 9 +++++----
> >  include/linux/i2c.h         | 1 +
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 28460f6a60cc..af87a16ac3a5 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -430,7 +430,7 @@ static int i2c_device_remove(struct device *dev)
> >       dev_pm_clear_wake_irq(&client->dev);
> >       device_init_wakeup(&client->dev, false);
> >
> > -     client->irq = 0;
> > +     client->irq = client->init_irq;
> >
> >       return status;
> >  }
> > @@ -741,10 +741,11 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> >       client->flags = info->flags;
> >       client->addr = info->addr;
> >
> > -     client->irq = info->irq;
> > -     if (!client->irq)
> > -             client->irq = i2c_dev_irq_from_resources(info->resources,
> > +     client->init_irq = info->irq;
> > +     if (!client->init_irq)
> > +             client->init_irq = i2c_dev_irq_from_resources(info->resources,
> >                                                        info->num_resources);
> > +     client->irq = client->init_irq;
> >
> >       strlcpy(client->name, info->type, sizeof(client->name));
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 65b4eaed1d96..7e748648c7d3 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -333,6 +333,7 @@ struct i2c_client {
> >       char name[I2C_NAME_SIZE];
> >       struct i2c_adapter *adapter;    /* the adapter we sit on        */
> >       struct device dev;              /* the device structure         */
> > +     int init_irq;                   /* irq set at initialization    */
> >       int irq;                        /* irq issued by device         */
> >       struct list_head detected;
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > --
> > 2.20.1
> >

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

On Fri, Feb 22, 2019 at 11:15:59AM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 22, 2019 at 12:26 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> > On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> > > A previous change allowed I2C client devices to discover new IRQs upon
> > > reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> > > assigned in i2c_new_device, that information is lost.
> > >
> > > For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> > > are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> > > client device structures are initialized during an ACPI walk. After
> > > removing the i2c_hid device, modprobe fails.
> > >
> > > This change caches the initial IRQ value in i2c_new_device and then resets
> > > the client device IRQ to the initial value in i2c_device_remove.
> > >
> > > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> > > Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> >
> > Adding Benjamin to CC
> 
> Sorry, I should have answered earlier.
> 
> I am a little bit hesitant regarding this patch. The effect is
> correct, and I indeed realized a few weeks ago that something were
> wrong as we couldn't rmmod/modprobe i2c-hid.
> 
> But I still have the feeling that the problem is not solved at the
> right place. In i2c_new_device() we are storing parts of the fields of
> struct i2c_board_info, and when resetting the irq we are losing
> information. This patch solves that, but I wonder if the IRQ should
> not be 'simply' set in i2c_device_probe(). This means we also need to
> store the .resources of info, but I have a feeling this will be less
> error prone in the future.
> 
> But this is just my guts telling me something is not right. I would
> perfectly understand if we want to get this merged ASAP.
> 
> So given that the code is correct, this is my:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> But at least I have expressed my feelings :)

Which I can relate to very much. I see the code solves the issue but my
feeling is that we are patching around something which should be handled
differently in general.

Is somebody willing to research this further?

Thanks for your input.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Charles Keepax]

On Fri, Feb 22, 2019 at 11:15:59AM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 22, 2019 at 12:26 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> > > A previous change allowed I2C client devices to discover new IRQs upon
> > > reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> > > assigned in i2c_new_device, that information is lost.
> > >
> > > For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> > > are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> > > client device structures are initialized during an ACPI walk. After
> > > removing the i2c_hid device, modprobe fails.
> > >
> > > This change caches the initial IRQ value in i2c_new_device and then resets
> > > the client device IRQ to the initial value in i2c_device_remove.
> > >
> > > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> > > Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> >
> > Adding Benjamin to CC
> 
> Sorry, I should have answered earlier.
> 
> I am a little bit hesitant regarding this patch. The effect is
> correct, and I indeed realized a few weeks ago that something were
> wrong as we couldn't rmmod/modprobe i2c-hid.
> 
> But I still have the feeling that the problem is not solved at the
> right place. In i2c_new_device() we are storing parts of the fields of
> struct i2c_board_info, and when resetting the irq we are losing
> information. This patch solves that, but I wonder if the IRQ should
> not be 'simply' set in i2c_device_probe(). This means we also need to
> store the .resources of info, but I have a feeling this will be less
> error prone in the future.
> 

I would be somewhat inclined to agree here, it does seem odd that
on some paths we are allocating the IRQ on the new_device side
and on some on the probe side.

> But this is just my guts telling me something is not right. I would
> perfectly understand if we want to get this merged ASAP.
> 
> So given that the code is correct, this is my:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 

This would be my thinking as well we should merge this to avoid
the regression.

Thanks,
Charles

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Charles Keepax]

On Fri, Feb 22, 2019 at 11:23:35AM +0100, Wolfram Sang wrote:
> On Fri, Feb 22, 2019 at 11:15:59AM +0100, Benjamin Tissoires wrote:
> > On Fri, Feb 22, 2019 at 12:26 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> > > > A previous change allowed I2C client devices to discover new IRQs upon
> > > > reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> > > > assigned in i2c_new_device, that information is lost.
> > > >
> > > > For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> > > > are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> > > > client device structures are initialized during an ACPI walk. After
> > > > removing the i2c_hid device, modprobe fails.
> > > >
> > > > This change caches the initial IRQ value in i2c_new_device and then resets
> > > > the client device IRQ to the initial value in i2c_device_remove.
> > > >
> > > > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> > > > Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> > >
> > > Adding Benjamin to CC
> > 
> > Sorry, I should have answered earlier.
> > 
> > I am a little bit hesitant regarding this patch. The effect is
> > correct, and I indeed realized a few weeks ago that something were
> > wrong as we couldn't rmmod/modprobe i2c-hid.
> > 
> > But I still have the feeling that the problem is not solved at the
> > right place. In i2c_new_device() we are storing parts of the fields of
> > struct i2c_board_info, and when resetting the irq we are losing
> > information. This patch solves that, but I wonder if the IRQ should
> > not be 'simply' set in i2c_device_probe(). This means we also need to
> > store the .resources of info, but I have a feeling this will be less
> > error prone in the future.
> > 
> > But this is just my guts telling me something is not right. I would
> > perfectly understand if we want to get this merged ASAP.
> > 
> > So given that the code is correct, this is my:
> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > 
> > But at least I have expressed my feelings :)
> 
> Which I can relate to very much. I see the code solves the issue but my
> feeling is that we are patching around something which should be handled
> differently in general.
> 
> Is somebody willing to research this further?
> 
> Thanks for your input.
> 

I would be willing to have more of a look at it but am slightly
nervous I am not right person as all the systems I currently work
with are DT based so don't really exemplify the issue at all.

Thanks,
Charles

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]


> > > But I still have the feeling that the problem is not solved at the
> > > right place. In i2c_new_device() we are storing parts of the fields of
> > > struct i2c_board_info, and when resetting the irq we are losing
> > > information. This patch solves that, but I wonder if the IRQ should
> > > not be 'simply' set in i2c_device_probe(). This means we also need to
> > > store the .resources of info, but I have a feeling this will be less
> > > error prone in the future.
> > > 
> > > But this is just my guts telling me something is not right. I would
> > > perfectly understand if we want to get this merged ASAP.
> > > 
> > > So given that the code is correct, this is my:
> > > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > 
> > > But at least I have expressed my feelings :)
> > 
> > Which I can relate to very much. I see the code solves the issue but my
> > feeling is that we are patching around something which should be handled
> > differently in general.
> > 
> > Is somebody willing to research this further?
> > 
> > Thanks for your input.
> > 
> 
> I would be willing to have more of a look at it but am slightly
> nervous I am not right person as all the systems I currently work
> with are DT based so don't really exemplify the issue at all.

Thank you! Well, I'll be there, too. Discussing, reviewing, testing. And
if we have Benjamin for that on board as well, then I think we have a
good start for that task :) Others are more than welcome to join, too,
of course.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Jim Broadus]

On Fri, Feb 22, 2019 at 3:32 AM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > > > But I still have the feeling that the problem is not solved at the
> > > > right place. In i2c_new_device() we are storing parts of the fields of
> > > > struct i2c_board_info, and when resetting the irq we are losing
> > > > information. This patch solves that, but I wonder if the IRQ should
> > > > not be 'simply' set in i2c_device_probe(). This means we also need to
> > > > store the .resources of info, but I have a feeling this will be less
> > > > error prone in the future.
> > > >
> > > > But this is just my guts telling me something is not right. I would
> > > > perfectly understand if we want to get this merged ASAP.
> > > >
> > > > So given that the code is correct, this is my:
> > > > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > But at least I have expressed my feelings :)
> > >
> > > Which I can relate to very much. I see the code solves the issue but my
> > > feeling is that we are patching around something which should be handled
> > > differently in general.
> > >
> > > Is somebody willing to research this further?
> > >
> > > Thanks for your input.
> > >
> >
> > I would be willing to have more of a look at it but am slightly
> > nervous I am not right person as all the systems I currently work
> > with are DT based so don't really exemplify the issue at all.
>
> Thank you! Well, I'll be there, too. Discussing, reviewing, testing. And
> if we have Benjamin for that on board as well, then I think we have a
> good start for that task :) Others are more than welcome to join, too,
> of course.
>

I'm also more familiar with device-tree (just came across this on my personal
laptop) but happy to review and test at the risk of learning something.

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

> I'm also more familiar with device-tree (just came across this on my personal
> laptop) but happy to review and test at the risk of learning something.

:) Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> A previous change allowed I2C client devices to discover new IRQs upon
> reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> assigned in i2c_new_device, that information is lost.
> 
> For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> client device structures are initialized during an ACPI walk. After
> removing the i2c_hid device, modprobe fails.
> 
> This change caches the initial IRQ value in i2c_new_device and then resets
> the client device IRQ to the initial value in i2c_device_remove.
> 
> Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> Signed-off-by: Jim Broadus <jbroadus@gmail.com>

Applied to for-next, thanks!

I didn't want to apply to for-current because I didn't feel comfortable
changing the I2C core in this very last days before the release.

It will hit linus tree during next merge window and go back to older
releases via stable.

That all being said, I'd really love to see the proper fix (move irq
assignment from init to probe time) being worked on rather soonish
before we forget all the details we know at this moment. I'll be there
for it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]