Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH] i2c: acpi: put device when verifying client fails
@ 2020-03-12 13:32 Wolfram Sang
  2020-03-12 13:38 ` Geert Uytterhoeven
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Wolfram Sang @ 2020-03-12 13:32 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-acpi, linux-renesas-soc, Mika Westerberg, Andy Shevchenko,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

i2c_verify_client() can fail, so we need to put the device when that
happens.

Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

RFC because I don't know if it can be that the returned dev is not an
i2c_client. Yet, since it can happen theoretically, I think we should
have the checks.

 drivers/i2c/i2c-core-acpi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8f3dbc97a057..8b0ff780919b 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -394,9 +394,17 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
 static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
 {
 	struct device *dev;
+	struct i2c_client *client;
 
 	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
-	return dev ? i2c_verify_client(dev) : NULL;
+	if (!dev)
+		return NULL;
+
+	client = i2c_verify_client(dev);
+	if (!client)
+		put_device(dev);
+
+	return client;
 }
 
 static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
-- 
2.20.1


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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 13:32 [RFC PATCH] i2c: acpi: put device when verifying client fails Wolfram Sang
@ 2020-03-12 13:38 ` Geert Uytterhoeven
  2020-03-12 14:47 ` Andy Shevchenko
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2020-03-12 13:38 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Linux I2C, ACPI Devel Maling List, Linux-Renesas,
	Mika Westerberg, Andy Shevchenko, Jarkko Nikula, Wolfram Sang

On Thu, Mar 12, 2020 at 2:32 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> i2c_verify_client() can fail, so we need to put the device when that
> happens.
>
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 13:32 [RFC PATCH] i2c: acpi: put device when verifying client fails Wolfram Sang
  2020-03-12 13:38 ` Geert Uytterhoeven
@ 2020-03-12 14:47 ` Andy Shevchenko
  2020-03-12 14:49   ` Andy Shevchenko
  2020-03-12 15:14 ` Andy Shevchenko
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-12 14:47 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-acpi, linux-renesas-soc, Mika Westerberg,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.

NAK, this will do double put and messing up with reference counters.
Besides the fact, that device may disappear after looking up which leads us to
even more problems.

See how i2c_acpi_find_client_by_adev() is used in callers.

> 
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> RFC because I don't know if it can be that the returned dev is not an
> i2c_client. Yet, since it can happen theoretically, I think we should
> have the checks.
> 
>  drivers/i2c/i2c-core-acpi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 8f3dbc97a057..8b0ff780919b 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -394,9 +394,17 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
>  static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
>  {
>  	struct device *dev;
> +	struct i2c_client *client;
>  
>  	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> -	return dev ? i2c_verify_client(dev) : NULL;
> +	if (!dev)
> +		return NULL;
> +
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		put_device(dev);
> +
> +	return client;
>  }
>  
>  static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 14:47 ` Andy Shevchenko
@ 2020-03-12 14:49   ` Andy Shevchenko
  2020-03-12 14:51     ` Andy Shevchenko
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-12 14:49 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-acpi, linux-renesas-soc, Mika Westerberg,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

On Thu, Mar 12, 2020 at 04:47:39PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > i2c_verify_client() can fail, so we need to put the device when that
> > happens.
> 
> NAK, this will do double put and messing up with reference counters.
> Besides the fact, that device may disappear after looking up which leads us to
> even more problems.
> 
> See how i2c_acpi_find_client_by_adev() is used in callers.

Perhaps proper "fix" is to add the explanation to a comment in the code to
prevent false positive reports in the future?

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 14:49   ` Andy Shevchenko
@ 2020-03-12 14:51     ` Andy Shevchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-12 14:51 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-acpi, linux-renesas-soc, Mika Westerberg,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

On Thu, Mar 12, 2020 at 04:49:08PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 12, 2020 at 04:47:39PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> > > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > 
> > > i2c_verify_client() can fail, so we need to put the device when that
> > > happens.
> > 
> > NAK, this will do double put and messing up with reference counters.
> > Besides the fact, that device may disappear after looking up which leads us to
> > even more problems.
> > 
> > See how i2c_acpi_find_client_by_adev() is used in callers.
> 
> Perhaps proper "fix" is to add the explanation to a comment in the code to
> prevent false positive reports in the future?

Ah, sorry, I need to read it more carefully...


-- 
With Best Regards,
Andy Shevchenko



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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 13:32 [RFC PATCH] i2c: acpi: put device when verifying client fails Wolfram Sang
  2020-03-12 13:38 ` Geert Uytterhoeven
  2020-03-12 14:47 ` Andy Shevchenko
@ 2020-03-12 15:14 ` Andy Shevchenko
  2020-03-13 11:29 ` Mika Westerberg
  2020-03-13 14:16 ` Wolfram Sang
  4 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2020-03-12 15:14 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-acpi, linux-renesas-soc, Mika Westerberg,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.

I hope it's not a CoVID-19 makes me mistakenly commented in the first place. :-)

So, theoretically below is possible, but practically it's doubtful.

The I2CSerialBusV2() ACPI resource can be present solely in I²C slave device
nodes according to the specification. However, we might have two possible cases
a) screwed up ACPI table;
b) I²C master which in turn is I²C slave.

While a) has been so far unseen, b) case sounds like plasible for I²C muxes IIUC.

So, I agree with the patch, and sorry for the first reaction.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> RFC because I don't know if it can be that the returned dev is not an
> i2c_client. Yet, since it can happen theoretically, I think we should
> have the checks.
> 
>  drivers/i2c/i2c-core-acpi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 8f3dbc97a057..8b0ff780919b 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -394,9 +394,17 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
>  static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
>  {
>  	struct device *dev;
> +	struct i2c_client *client;
>  
>  	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> -	return dev ? i2c_verify_client(dev) : NULL;
> +	if (!dev)
> +		return NULL;
> +
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		put_device(dev);
> +
> +	return client;
>  }
>  
>  static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 13:32 [RFC PATCH] i2c: acpi: put device when verifying client fails Wolfram Sang
                   ` (2 preceding siblings ...)
  2020-03-12 15:14 ` Andy Shevchenko
@ 2020-03-13 11:29 ` Mika Westerberg
  2020-03-13 14:16 ` Wolfram Sang
  4 siblings, 0 replies; 8+ messages in thread
From: Mika Westerberg @ 2020-03-13 11:29 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-acpi, linux-renesas-soc, Andy Shevchenko,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.
> 
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> RFC because I don't know if it can be that the returned dev is not an
> i2c_client. Yet, since it can happen theoretically, I think we should
> have the checks.

I agree,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

* Re: [RFC PATCH] i2c: acpi: put device when verifying client fails
  2020-03-12 13:32 [RFC PATCH] i2c: acpi: put device when verifying client fails Wolfram Sang
                   ` (3 preceding siblings ...)
  2020-03-13 11:29 ` Mika Westerberg
@ 2020-03-13 14:16 ` Wolfram Sang
  4 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2020-03-13 14:16 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-acpi, linux-renesas-soc, Mika Westerberg, Andy Shevchenko,
	Jarkko Nikula, Wolfram Sang, Geert Uytterhoeven

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

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.
> 
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-current, thanks!


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

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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-12 13:32 [RFC PATCH] i2c: acpi: put device when verifying client fails Wolfram Sang
2020-03-12 13:38 ` Geert Uytterhoeven
2020-03-12 14:47 ` Andy Shevchenko
2020-03-12 14:49   ` Andy Shevchenko
2020-03-12 14:51     ` Andy Shevchenko
2020-03-12 15:14 ` Andy Shevchenko
2020-03-13 11:29 ` Mika Westerberg
2020-03-13 14:16 ` Wolfram Sang

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git