| Message ID | 20190611123101.25264-5-ckeepax@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | I2C IRQ Probe Improvements | expand |
On Tue, Jun 11, 2019 at 01:30:58PM +0100, Charles Keepax wrote: > In preparation for more refactoring make i2c_acpi_get_irq available > outside i2c-core-acpi.c. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Tue, Jun 11, 2019 at 2:31 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > In preparation for more refactoring make i2c_acpi_get_irq available > outside i2c-core-acpi.c. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- So, my bisect fails here: when I recompile the vmlinuz image, it fails after loading the initrd. There is no boot up message, so not sure, but the problem with the non working touchpad is either this one or the next one. Cheers, Benjamin > > Changes since v3: > - Move the change to use the helper function from i2c-core-base into its own patch. > > Thanks, > Charles > > drivers/i2c/i2c-core-acpi.c | 15 +++++++++++++-- > drivers/i2c/i2c-core.h | 7 +++++++ > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 7d4d66ba752d4..35966cc337dde 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -144,8 +144,17 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data) > return 1; /* No need to add resource to the list */ > } > > -static int i2c_acpi_get_irq(struct acpi_device *adev) > +/** > + * i2c_acpi_get_irq - get device IRQ number from ACPI > + * @client: Pointer to the I2C client device > + * > + * Find the IRQ number used by a specific client device. > + * > + * Return: The IRQ number or an error code. > + */ > +int i2c_acpi_get_irq(struct i2c_client *client) > { > + struct acpi_device *adev = ACPI_COMPANION(&client->adapter->dev); > struct list_head resource_list; > int irq = -ENOENT; > int ret; > @@ -162,6 +171,8 @@ static int i2c_acpi_get_irq(struct acpi_device *adev) > return irq; > } > > +static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev); > + > static int i2c_acpi_get_info(struct acpi_device *adev, > struct i2c_board_info *info, > struct i2c_adapter *adapter, > @@ -198,7 +209,7 @@ static int i2c_acpi_get_info(struct acpi_device *adev, > *adapter_handle = lookup.adapter_handle; > > /* Then fill IRQ number if any */ > - ret = i2c_acpi_get_irq(adev); > + ret = i2c_acpi_get_irq(i2c_acpi_find_client_by_adev(adev)); > if (ret > 0) > info->irq = ret; > > diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h > index 2a3b28bf826b1..517d98be68d25 100644 > --- a/drivers/i2c/i2c-core.h > +++ b/drivers/i2c/i2c-core.h > @@ -63,6 +63,8 @@ const struct acpi_device_id * > i2c_acpi_match_device(const struct acpi_device_id *matches, > struct i2c_client *client); > void i2c_acpi_register_devices(struct i2c_adapter *adap); > + > +int i2c_acpi_get_irq(struct i2c_client *client); > #else /* CONFIG_ACPI */ > static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { } > static inline const struct acpi_device_id * > @@ -71,6 +73,11 @@ i2c_acpi_match_device(const struct acpi_device_id *matches, > { > return NULL; > } > + > +static inline int i2c_acpi_get_irq(struct i2c_client *client) > +{ > + return 0; > +} > #endif /* CONFIG_ACPI */ > extern struct notifier_block i2c_acpi_notifier; > > -- > 2.11.0 >
On Tue, Jun 11, 2019 at 01:30:58PM +0100, Charles Keepax wrote: > In preparation for more refactoring make i2c_acpi_get_irq available > outside i2c-core-acpi.c. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > > Changes since v3: > - Move the change to use the helper function from i2c-core-base into its own patch. > > Thanks, > Charles > > drivers/i2c/i2c-core-acpi.c | 15 +++++++++++++-- > drivers/i2c/i2c-core.h | 7 +++++++ > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 7d4d66ba752d4..35966cc337dde 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -144,8 +144,17 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data) > return 1; /* No need to add resource to the list */ > } > > -static int i2c_acpi_get_irq(struct acpi_device *adev) > +/** > + * i2c_acpi_get_irq - get device IRQ number from ACPI > + * @client: Pointer to the I2C client device > + * > + * Find the IRQ number used by a specific client device. > + * > + * Return: The IRQ number or an error code. > + */ > +int i2c_acpi_get_irq(struct i2c_client *client) > { > + struct acpi_device *adev = ACPI_COMPANION(&client->adapter->dev); Is this adev checked for being NULL somewhere below before it is being dereferenced? It could explain the issue Benjamin is seeing. > struct list_head resource_list; > int irq = -ENOENT; > int ret;
On Wed, Jun 12, 2019 at 06:27:18PM +0300, Mika Westerberg wrote: > On Tue, Jun 11, 2019 at 01:30:58PM +0100, Charles Keepax wrote: > > In preparation for more refactoring make i2c_acpi_get_irq available > > outside i2c-core-acpi.c. > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > --- > > > > Changes since v3: > > - Move the change to use the helper function from i2c-core-base into its own patch. > > > > Thanks, > > Charles > > > > drivers/i2c/i2c-core-acpi.c | 15 +++++++++++++-- > > drivers/i2c/i2c-core.h | 7 +++++++ > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > > index 7d4d66ba752d4..35966cc337dde 100644 > > --- a/drivers/i2c/i2c-core-acpi.c > > +++ b/drivers/i2c/i2c-core-acpi.c > > @@ -144,8 +144,17 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data) > > return 1; /* No need to add resource to the list */ > > } > > > > -static int i2c_acpi_get_irq(struct acpi_device *adev) > > +/** > > + * i2c_acpi_get_irq - get device IRQ number from ACPI > > + * @client: Pointer to the I2C client device > > + * > > + * Find the IRQ number used by a specific client device. > > + * > > + * Return: The IRQ number or an error code. > > + */ > > +int i2c_acpi_get_irq(struct i2c_client *client) > > { > > + struct acpi_device *adev = ACPI_COMPANION(&client->adapter->dev); > > Is this adev checked for being NULL somewhere below before it is being > dereferenced? > > It could explain the issue Benjamin is seeing. > Yeah could be that or just for some reason this isn't returning the same adev as we previously had. I will do some digging see if I can find any likely culprits. Thanks, Charles
On Thu, Jun 13, 2019 at 10:49 AM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > On Wed, Jun 12, 2019 at 06:27:18PM +0300, Mika Westerberg wrote: > > On Tue, Jun 11, 2019 at 01:30:58PM +0100, Charles Keepax wrote: > > > In preparation for more refactoring make i2c_acpi_get_irq available > > > outside i2c-core-acpi.c. > > > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > > --- > > > > > > Changes since v3: > > > - Move the change to use the helper function from i2c-core-base into its own patch. > > > > > > Thanks, > > > Charles > > > > > > drivers/i2c/i2c-core-acpi.c | 15 +++++++++++++-- > > > drivers/i2c/i2c-core.h | 7 +++++++ > > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > > > index 7d4d66ba752d4..35966cc337dde 100644 > > > --- a/drivers/i2c/i2c-core-acpi.c > > > +++ b/drivers/i2c/i2c-core-acpi.c > > > @@ -144,8 +144,17 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data) > > > return 1; /* No need to add resource to the list */ > > > } > > > > > > -static int i2c_acpi_get_irq(struct acpi_device *adev) > > > +/** > > > + * i2c_acpi_get_irq - get device IRQ number from ACPI > > > + * @client: Pointer to the I2C client device > > > + * > > > + * Find the IRQ number used by a specific client device. > > > + * > > > + * Return: The IRQ number or an error code. > > > + */ > > > +int i2c_acpi_get_irq(struct i2c_client *client) > > > { > > > + struct acpi_device *adev = ACPI_COMPANION(&client->adapter->dev); > > > > Is this adev checked for being NULL somewhere below before it is being > > dereferenced? > > > > It could explain the issue Benjamin is seeing. > > > > Yeah could be that or just for some reason this isn't returning > the same adev as we previously had. I will do some digging see if > I can find any likely culprits. That was almost the culprit: client is NULL here. So the call of i2c_acpi_find_client_by_adev(adev) fails. I guess this explains why the next commit is also not working :) Cheers, Benjamin
On Thu, Jun 13, 2019 at 11:32:47AM +0200, Benjamin Tissoires wrote: > On Thu, Jun 13, 2019 at 10:49 AM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > > On Wed, Jun 12, 2019 at 06:27:18PM +0300, Mika Westerberg wrote: > > > On Tue, Jun 11, 2019 at 01:30:58PM +0100, Charles Keepax wrote: > > > > In preparation for more refactoring make i2c_acpi_get_irq available > > > > outside i2c-core-acpi.c. > > > > > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > > > --- > > > > > > > > Changes since v3: > > > > - Move the change to use the helper function from i2c-core-base into its own patch. > > > > > > > > Thanks, > > > > Charles > > > > > > > > drivers/i2c/i2c-core-acpi.c | 15 +++++++++++++-- > > > > drivers/i2c/i2c-core.h | 7 +++++++ > > > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > > > > index 7d4d66ba752d4..35966cc337dde 100644 > > > > --- a/drivers/i2c/i2c-core-acpi.c > > > > +++ b/drivers/i2c/i2c-core-acpi.c > > > > @@ -144,8 +144,17 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data) > > > > return 1; /* No need to add resource to the list */ > > > > } > > > > > > > > -static int i2c_acpi_get_irq(struct acpi_device *adev) > > > > +/** > > > > + * i2c_acpi_get_irq - get device IRQ number from ACPI > > > > + * @client: Pointer to the I2C client device > > > > + * > > > > + * Find the IRQ number used by a specific client device. > > > > + * > > > > + * Return: The IRQ number or an error code. > > > > + */ > > > > +int i2c_acpi_get_irq(struct i2c_client *client) > > > > { > > > > + struct acpi_device *adev = ACPI_COMPANION(&client->adapter->dev); > > > > > > Is this adev checked for being NULL somewhere below before it is being > > > dereferenced? > > > > > > It could explain the issue Benjamin is seeing. > > > > > > > Yeah could be that or just for some reason this isn't returning > > the same adev as we previously had. I will do some digging see if > > I can find any likely culprits. > > That was almost the culprit: client is NULL here. > So the call of i2c_acpi_find_client_by_adev(adev) fails. > > I guess this explains why the next commit is also not working :) > Ah.. yeah because at this stage this code is probably running before that client has been created I think, this crept in on that last splitting of the patch into two. Let me see if I can come up with a new solution and do a new version. Thank you very much for the help going through this. Thanks, Charles
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c index 7d4d66ba752d4..35966cc337dde 100644 --- a/drivers/i2c/i2c-core-acpi.c +++ b/drivers/i2c/i2c-core-acpi.c @@ -144,8 +144,17 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data) return 1; /* No need to add resource to the list */ } -static int i2c_acpi_get_irq(struct acpi_device *adev) +/** + * i2c_acpi_get_irq - get device IRQ number from ACPI + * @client: Pointer to the I2C client device + * + * Find the IRQ number used by a specific client device. + * + * Return: The IRQ number or an error code. + */ +int i2c_acpi_get_irq(struct i2c_client *client) { + struct acpi_device *adev = ACPI_COMPANION(&client->adapter->dev); struct list_head resource_list; int irq = -ENOENT; int ret; @@ -162,6 +171,8 @@ static int i2c_acpi_get_irq(struct acpi_device *adev) return irq; } +static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev); + static int i2c_acpi_get_info(struct acpi_device *adev, struct i2c_board_info *info, struct i2c_adapter *adapter, @@ -198,7 +209,7 @@ static int i2c_acpi_get_info(struct acpi_device *adev, *adapter_handle = lookup.adapter_handle; /* Then fill IRQ number if any */ - ret = i2c_acpi_get_irq(adev); + ret = i2c_acpi_get_irq(i2c_acpi_find_client_by_adev(adev)); if (ret > 0) info->irq = ret; diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h index 2a3b28bf826b1..517d98be68d25 100644 --- a/drivers/i2c/i2c-core.h +++ b/drivers/i2c/i2c-core.h @@ -63,6 +63,8 @@ const struct acpi_device_id * i2c_acpi_match_device(const struct acpi_device_id *matches, struct i2c_client *client); void i2c_acpi_register_devices(struct i2c_adapter *adap); + +int i2c_acpi_get_irq(struct i2c_client *client); #else /* CONFIG_ACPI */ static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { } static inline const struct acpi_device_id * @@ -71,6 +73,11 @@ i2c_acpi_match_device(const struct acpi_device_id *matches, { return NULL; } + +static inline int i2c_acpi_get_irq(struct i2c_client *client) +{ + return 0; +} #endif /* CONFIG_ACPI */ extern struct notifier_block i2c_acpi_notifier;
In preparation for more refactoring make i2c_acpi_get_irq available outside i2c-core-acpi.c. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- Changes since v3: - Move the change to use the helper function from i2c-core-base into its own patch. Thanks, Charles drivers/i2c/i2c-core-acpi.c | 15 +++++++++++++-- drivers/i2c/i2c-core.h | 7 +++++++ 2 files changed, 20 insertions(+), 2 deletions(-)