Patchwork [2/6] drivers/rtc/rtc-tps6586x.c: Use devm_rtc_device_unregister

login
register
mail settings
Submitter Sachin Kamat
Date March 25, 2013, 10:45 a.m.
Message ID <1364208327-14207-3-git-send-email-sachin.kamat@linaro.org>
Download mbox | patch
Permalink /patch/230617/
State New
Headers show

Comments

Sachin Kamat - March 25, 2013, 10:45 a.m.
Using non-device managed unregister function with device
managed register is not correct. Use the device managed
version instead.
Though device managed functions do not generally need explicit
freeing/unregistering, in this case it is used to avoid any
problems due to the ordering.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Cc: Laxman dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/rtc-tps6586x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Laxman Dewangan - March 25, 2013, 11:45 a.m.
On Monday 25 March 2013 04:15 PM, Sachin Kamat wrote:
> Using non-device managed unregister function with device
> managed register is not correct. Use the device managed
> version instead.
> Though device managed functions do not generally need explicit
> freeing/unregistering, in this case it is used to avoid any
> problems due to the ordering.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Laxman dewangan <ldewangan@nvidia.com>
> ---
>   drivers/rtc/rtc-tps6586x.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
> index badfea4..ded8f74 100644
> --- a/drivers/rtc/rtc-tps6586x.c
> +++ b/drivers/rtc/rtc-tps6586x.c
> @@ -296,7 +296,7 @@ static int tps6586x_rtc_probe(struct platform_device *pdev)
>   	return 0;
>   
>   fail_req_irq:
> -	rtc_device_unregister(rtc->rtc);
> +	devm_rtc_device_unregister(&pdev->dev, rtc->rtc);
>   

I think we should remove this line at all. On devm_*, we should not 
worry about cleanup path.



-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Sachin Kamat - March 25, 2013, 11:51 a.m.
On 25 March 2013 17:15, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Monday 25 March 2013 04:15 PM, Sachin Kamat wrote:
>>
>> Using non-device managed unregister function with device
>> managed register is not correct. Use the device managed
>> version instead.
>> Though device managed functions do not generally need explicit
>> freeing/unregistering, in this case it is used to avoid any
>> problems due to the ordering.
>>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> Cc: Laxman dewangan <ldewangan@nvidia.com>
>> ---
>>   drivers/rtc/rtc-tps6586x.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
>> index badfea4..ded8f74 100644
>> --- a/drivers/rtc/rtc-tps6586x.c
>> +++ b/drivers/rtc/rtc-tps6586x.c
>> @@ -296,7 +296,7 @@ static int tps6586x_rtc_probe(struct platform_device
>> *pdev)
>>         return 0;
>>     fail_req_irq:
>> -       rtc_device_unregister(rtc->rtc);
>> +       devm_rtc_device_unregister(&pdev->dev, rtc->rtc);
>>
>
>
> I think we should remove this line at all. On devm_*, we should not worry
> about cleanup path.

Yes you are right. But I was not sure about the dependency of
tps6586x_update() on the order (as mentioned in the commit message).
If you are really sure about it, i will send an updated patch removing
the unregister call altogether.
Laxman Dewangan - March 25, 2013, 11:51 a.m.
On Monday 25 March 2013 05:21 PM, Sachin Kamat wrote:
> On 25 March 2013 17:15, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Monday 25 March 2013 04:15 PM, Sachin Kamat wrote:
>>> Using non-device managed unregister function with device
>>> managed register is not correct. Use the device managed
>>> version instead.
>>> Though device managed functions do not generally need explicit
>>> freeing/unregistering, in this case it is used to avoid any
>>> problems due to the ordering.
>>>
>>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>>> Cc: Laxman dewangan <ldewangan@nvidia.com>
>>> ---
>>>    drivers/rtc/rtc-tps6586x.c |    2 +-
>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
>>> index badfea4..ded8f74 100644
>>> --- a/drivers/rtc/rtc-tps6586x.c
>>> +++ b/drivers/rtc/rtc-tps6586x.c
>>> @@ -296,7 +296,7 @@ static int tps6586x_rtc_probe(struct platform_device
>>> *pdev)
>>>          return 0;
>>>      fail_req_irq:
>>> -       rtc_device_unregister(rtc->rtc);
>>> +       devm_rtc_device_unregister(&pdev->dev, rtc->rtc);
>>>
>>
>> I think we should remove this line at all. On devm_*, we should not worry
>> about cleanup path.
> Yes you are right. But I was not sure about the dependency of
> tps6586x_update() on the order (as mentioned in the commit message).
> If you are really sure about it, i will send an updated patch removing
> the unregister call altogether.

devm_* will get called after .remove() get called.

>


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Sachin Kamat - March 25, 2013, 11:59 a.m.
On 25 March 2013 17:21, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Monday 25 March 2013 05:21 PM, Sachin Kamat wrote:
>>
>> On 25 March 2013 17:15, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>
>>> On Monday 25 March 2013 04:15 PM, Sachin Kamat wrote:
>>>>
>>>> Using non-device managed unregister function with device
>>>> managed register is not correct. Use the device managed
>>>> version instead.
>>>> Though device managed functions do not generally need explicit
>>>> freeing/unregistering, in this case it is used to avoid any
>>>> problems due to the ordering.
>>>>
>>>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>>>> Cc: Laxman dewangan <ldewangan@nvidia.com>
>>>> ---
>>>>    drivers/rtc/rtc-tps6586x.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
>>>> index badfea4..ded8f74 100644
>>>> --- a/drivers/rtc/rtc-tps6586x.c
>>>> +++ b/drivers/rtc/rtc-tps6586x.c
>>>> @@ -296,7 +296,7 @@ static int tps6586x_rtc_probe(struct platform_device
>>>> *pdev)
>>>>          return 0;
>>>>      fail_req_irq:
>>>> -       rtc_device_unregister(rtc->rtc);
>>>> +       devm_rtc_device_unregister(&pdev->dev, rtc->rtc);
>>>>
>>>
>>> I think we should remove this line at all. On devm_*, we should not worry
>>> about cleanup path.
>>
>> Yes you are right. But I was not sure about the dependency of
>> tps6586x_update() on the order (as mentioned in the commit message).
>> If you are really sure about it, i will send an updated patch removing
>> the unregister call altogether.
>
>
> devm_* will get called after .remove() get called.

That is exactly why I am concerned about removing it as without
devm_rtc_device_unregister, the order of call in error path was:
rtc_device_unregister(rtc->rtc); and then
tps6586x_update()

Now if we don't use devm_rtc_device_unregister , the order gets reversed as
tps6586x_update()
and then
devm_rtc_device_unregister (implicit call).

Hope you got my point.
Laxman Dewangan - March 25, 2013, noon
On Monday 25 March 2013 05:29 PM, Sachin Kamat wrote:
> On 25 March 2013 17:21, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Monday 25 March 2013 05:21 PM, Sachin Kamat wrote:
>>> On 25 March 2013 17:15, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>> devm_* will get called after .remove() get called. 
> That is exactly why I am concerned about removing it as without
> devm_rtc_device_unregister, the order of call in error path was:
> rtc_device_unregister(rtc->rtc); and then
> tps6586x_update()
>
> Now if we don't use devm_rtc_device_unregister , the order gets reversed as
> tps6586x_update()
> and then
> devm_rtc_device_unregister (implicit call).
>
> Hope you got my point.

No issue here as tps6586x_update() just write register to clear all 
enables and that can be done before rtc_unregister() also.


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Sachin Kamat - March 25, 2013, 12:03 p.m.
On 25 March 2013 17:30, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Monday 25 March 2013 05:29 PM, Sachin Kamat wrote:
>>
>> On 25 March 2013 17:21, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>
>>> On Monday 25 March 2013 05:21 PM, Sachin Kamat wrote:
>>>>
>>>> On 25 March 2013 17:15, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>> devm_* will get called after .remove() get called.
>>
>> That is exactly why I am concerned about removing it as without
>> devm_rtc_device_unregister, the order of call in error path was:
>> rtc_device_unregister(rtc->rtc); and then
>> tps6586x_update()
>>
>> Now if we don't use devm_rtc_device_unregister , the order gets reversed
>> as
>> tps6586x_update()
>> and then
>> devm_rtc_device_unregister (implicit call).
>>
>> Hope you got my point.
>
>
> No issue here as tps6586x_update() just write register to clear all enables
> and that can be done before rtc_unregister() also.

OK great. I wanted that confirmation. I will resend this patch.

Patch

diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
index badfea4..ded8f74 100644
--- a/drivers/rtc/rtc-tps6586x.c
+++ b/drivers/rtc/rtc-tps6586x.c
@@ -296,7 +296,7 @@  static int tps6586x_rtc_probe(struct platform_device *pdev)
 	return 0;
 
 fail_req_irq:
-	rtc_device_unregister(rtc->rtc);
+	devm_rtc_device_unregister(&pdev->dev, rtc->rtc);
 
 fail_rtc_register:
 	tps6586x_update(tps_dev, RTC_CTRL, 0,