diff mbox series

[RFC] i2c: acpi: put device when verifying client fails

Message ID 20200312133244.9564-1-wsa@the-dreams.de (mailing list archive)
State Mainlined
Commit 8daee952b4389729358665fb91949460641659d4
Delegated to: Geert Uytterhoeven
Headers show
Series [RFC] i2c: acpi: put device when verifying client fails | expand

Commit Message

Wolfram Sang March 12, 2020, 1:32 p.m. UTC
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(-)

Comments

Geert Uytterhoeven March 12, 2020, 1:38 p.m. UTC | #1
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
Andy Shevchenko March 12, 2020, 2:47 p.m. UTC | #2
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
>
Andy Shevchenko March 12, 2020, 2:49 p.m. UTC | #3
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?
Andy Shevchenko March 12, 2020, 2:51 p.m. UTC | #4
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...
Andy Shevchenko March 12, 2020, 3:14 p.m. UTC | #5
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
>
Mika Westerberg March 13, 2020, 11:29 a.m. UTC | #6
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>
Wolfram Sang March 13, 2020, 2:16 p.m. UTC | #7
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!
diff mbox series

Patch

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,