| Message ID | 1364208327-14207-3-git-send-email-sachin.kamat@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
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. -----------------------------------------------------------------------------------
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.
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. -----------------------------------------------------------------------------------
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.
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. -----------------------------------------------------------------------------------
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.
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,
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(-)