diff mbox series

[1/1] i2c: acpi: Unbind mux adapters before delete

Message ID 20240228023925.2814638-2-hamish.martin@alliedtelesis.co.nz
State Superseded
Headers show
Series I2C mux ACPI SSDT overlay removal issue | expand

Commit Message

Hamish Martin Feb. 28, 2024, 2:39 a.m. UTC
Unbind any i2c_adapters matching an ACPI device being removed. This
ensures that the linkage between the ACPI device and the i2c_adapter is
correctly broken before the ACPI device is destroyed.

Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
 drivers/i2c/i2c-core-acpi.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Mika Westerberg Feb. 29, 2024, 9:54 a.m. UTC | #1
On Wed, Feb 28, 2024 at 03:39:25PM +1300, Hamish Martin wrote:
> Unbind any i2c_adapters matching an ACPI device being removed. This
> ensures that the linkage between the ACPI device and the i2c_adapter is
> correctly broken before the ACPI device is destroyed.

I suggest to move some of the information from the cover letter here to
make sure it is not lost. It includes good analysis of the problem so
should be part of the changelog.

> Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
> ---
>  drivers/i2c/i2c-core-acpi.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index d6037a328669..67fa8deccef6 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -445,6 +445,11 @@ static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
>  	return i2c_find_device_by_fwnode(acpi_fwnode_handle(adev));
>  }
>  
> +static struct i2c_adapter *i2c_acpi_find_adapter_by_adev(struct acpi_device *adev)
> +{
> +	return i2c_find_adapter_by_fwnode(acpi_fwnode_handle(adev));
> +}
> +
>  static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
>  			   void *arg)
>  {
> @@ -471,11 +476,17 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
>  			break;
>  
>  		client = i2c_acpi_find_client_by_adev(adev);
> -		if (!client)
> -			break;
> +		if (client) {
> +			i2c_unregister_device(client);
> +			put_device(&client->dev);
> +		}
> +
> +		adapter = i2c_acpi_find_adapter_by_adev(adev);
> +		if (adapter) {
> +			acpi_device_notify_remove(&adapter->dev);
> +			put_device(&adapter->dev);
> +		}

This looks good to me. I'm just wondering whether we have the same issue
on DT side too?

>  
> -		i2c_unregister_device(client);
> -		put_device(&client->dev);
>  		break;
>  	}
>  
> -- 
> 2.43.0
Hamish Martin Feb. 29, 2024, 9:40 p.m. UTC | #2
On Thu, 2024-02-29 at 11:54 +0200, Mika Westerberg wrote:
> On Wed, Feb 28, 2024 at 03:39:25PM +1300, Hamish Martin wrote:
> > Unbind any i2c_adapters matching an ACPI device being removed. This
> > ensures that the linkage between the ACPI device and the
> > i2c_adapter is
> > correctly broken before the ACPI device is destroyed.

> I suggest to move some of the information from the cover letter here
> to
> make sure it is not lost. It includes good analysis of the problem so
> should be part of the changelog.
OK, thanks Mika. I will do that.

> 
> > Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
> > ---
> >  drivers/i2c/i2c-core-acpi.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-
> > acpi.c
> > index d6037a328669..67fa8deccef6 100644
> > --- a/drivers/i2c/i2c-core-acpi.c
> > +++ b/drivers/i2c/i2c-core-acpi.c
> > @@ -445,6 +445,11 @@ static struct i2c_client
> > *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
> >         return i2c_find_device_by_fwnode(acpi_fwnode_handle(adev));
> >  }
> >  
> > +static struct i2c_adapter *i2c_acpi_find_adapter_by_adev(struct
> > acpi_device *adev)
> > +{
> > +       return
> > i2c_find_adapter_by_fwnode(acpi_fwnode_handle(adev));
> > +}
> > +
> >  static int i2c_acpi_notify(struct notifier_block *nb, unsigned
> > long value,
> >                            void *arg)
> >  {
> > @@ -471,11 +476,17 @@ static int i2c_acpi_notify(struct
> > notifier_block *nb, unsigned long value,
> >                         break;
> >  
> >                 client = i2c_acpi_find_client_by_adev(adev);
> > -               if (!client)
> > -                       break;
> > +               if (client) {
> > +                       i2c_unregister_device(client);
> > +                       put_device(&client->dev);
> > +               }
> > +
> > +               adapter = i2c_acpi_find_adapter_by_adev(adev);
> > +               if (adapter) {
> > +                       acpi_device_notify_remove(&adapter->dev);
> > +                       put_device(&adapter->dev);
> > +               }
> 
> This looks good to me. I'm just wondering whether we have the same
> issue
> on DT side too?
We don't see that. We have plenty of systems with the same basic
hardware layout using device tree and don't observe this kind of issue.

> 
> >  
> > -               i2c_unregister_device(client);
> > -               put_device(&client->dev);
> >                 break;
> >         }
> >  
> > -- 
> > 2.43.0
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index d6037a328669..67fa8deccef6 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -445,6 +445,11 @@  static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
 	return i2c_find_device_by_fwnode(acpi_fwnode_handle(adev));
 }
 
+static struct i2c_adapter *i2c_acpi_find_adapter_by_adev(struct acpi_device *adev)
+{
+	return i2c_find_adapter_by_fwnode(acpi_fwnode_handle(adev));
+}
+
 static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
 			   void *arg)
 {
@@ -471,11 +476,17 @@  static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
 			break;
 
 		client = i2c_acpi_find_client_by_adev(adev);
-		if (!client)
-			break;
+		if (client) {
+			i2c_unregister_device(client);
+			put_device(&client->dev);
+		}
+
+		adapter = i2c_acpi_find_adapter_by_adev(adev);
+		if (adapter) {
+			acpi_device_notify_remove(&adapter->dev);
+			put_device(&adapter->dev);
+		}
 
-		i2c_unregister_device(client);
-		put_device(&client->dev);
 		break;
 	}