diff mbox series

usb: typec: ucsi: acpi: Remove notifier before destroying handler

Message ID 20230321190136.449485-1-mpearson-lenovo@squebb.ca (mailing list archive)
State New, archived
Headers show
Series usb: typec: ucsi: acpi: Remove notifier before destroying handler | expand

Commit Message

Mark Pearson March 21, 2023, 7:01 p.m. UTC
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(-)

Comments

Heikki Krogerus March 23, 2023, 9:57 a.m. UTC | #1
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,
Mark Pearson March 24, 2023, 12:50 p.m. UTC | #2
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
diff mbox series

Patch

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;
 }