[PATCH] usb: typec: ucsi: acpi: Remove notifier before destroying handler

[PATCH] usb: typec: ucsi: acpi: Remove notifier before destroying handler

[Mark Pearson]

Was debugging another issue (since fixed) and noticed that the acpi
notify_handler should be removed before the ucsi object is destroyed.

This isn't fixing any issues that I'm aware of - but I assume could
potentially lead to a race condition if you were really unlucky?

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index ce0c8ef80c04..be3bf4f996d3 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -176,12 +176,12 @@ static int ucsi_acpi_remove(struct platform_device *pdev)
 {
 	struct ucsi_acpi *ua = platform_get_drvdata(pdev);
 
-	ucsi_unregister(ua->ucsi);
-	ucsi_destroy(ua->ucsi);
-
 	acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev), ACPI_DEVICE_NOTIFY,
 				   ucsi_acpi_notify);
 
+	ucsi_unregister(ua->ucsi);
+	ucsi_destroy(ua->ucsi);
+
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH] usb: typec: ucsi: acpi: Remove notifier before destroying handler

[Heikki Krogerus]

On Tue, Mar 21, 2023 at 03:01:36PM -0400, Mark Pearson wrote:
> Was debugging another issue (since fixed) and noticed that the acpi
> notify_handler should be removed before the ucsi object is destroyed.
> 
> This isn't fixing any issues that I'm aware of - but I assume could
> potentially lead to a race condition if you were really unlucky?
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/usb/typec/ucsi/ucsi_acpi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index ce0c8ef80c04..be3bf4f996d3 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -176,12 +176,12 @@ static int ucsi_acpi_remove(struct platform_device *pdev)
>  {
>  	struct ucsi_acpi *ua = platform_get_drvdata(pdev);
>  
> -	ucsi_unregister(ua->ucsi);
> -	ucsi_destroy(ua->ucsi);
> -
>  	acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev), ACPI_DEVICE_NOTIFY,
>  				   ucsi_acpi_notify);
>  
> +	ucsi_unregister(ua->ucsi);
> +	ucsi_destroy(ua->ucsi);
> +
>  	return 0;
>  }

Calling ucsi_desctroy() after removing the notifier makes sense to me,
but do you also need to unregister the instance after that?

You may still be in the middle of init or resume, so I think we need
to accept notifications until we are sure those have finished, i.e.
ucsi_unregister() has finished.

thanks,

-- 
heikki

Re: [PATCH] usb: typec: ucsi: acpi: Remove notifier before destroying handler

[Mark Pearson]

Hi Heikki,

On Thu, Mar 23, 2023, at 5:57 AM, Heikki Krogerus wrote:
> On Tue, Mar 21, 2023 at 03:01:36PM -0400, Mark Pearson wrote:
>> Was debugging another issue (since fixed) and noticed that the acpi
>> notify_handler should be removed before the ucsi object is destroyed.
>> 
>> This isn't fixing any issues that I'm aware of - but I assume could
>> potentially lead to a race condition if you were really unlucky?
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  drivers/usb/typec/ucsi/ucsi_acpi.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
>> index ce0c8ef80c04..be3bf4f996d3 100644
>> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
>> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
>> @@ -176,12 +176,12 @@ static int ucsi_acpi_remove(struct platform_device *pdev)
>>  {
>>  	struct ucsi_acpi *ua = platform_get_drvdata(pdev);
>>  
>> -	ucsi_unregister(ua->ucsi);
>> -	ucsi_destroy(ua->ucsi);
>> -
>>  	acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev), ACPI_DEVICE_NOTIFY,
>>  				   ucsi_acpi_notify);
>>  
>> +	ucsi_unregister(ua->ucsi);
>> +	ucsi_destroy(ua->ucsi);
>> +
>>  	return 0;
>>  }
>
> Calling ucsi_desctroy() after removing the notifier makes sense to me,
> but do you also need to unregister the instance after that?
>
> You may still be in the middle of init or resume, so I think we need
> to accept notifications until we are sure those have finished, i.e.
> ucsi_unregister() has finished.
>

That makes sense - I hadn't considered that.
I'll post an updated patch with the acpi_remove_notify_handler between the two calls.

Thanks for the review
Mark