The IRQ is requested before the struct rtc is allocated and registered, but
this struct is used in the IRQ handler. This may lead to a NULL pointer
dereference.
Switch to devm_rtc_allocate_device/rtc_register_device to allocate the rtc
struct before requesting the IRQ.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/rtc/rtc-digicolor.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
index b253bf1b3531..5bb14c56bc9a 100644
--- a/drivers/rtc/rtc-digicolor.c
+++ b/drivers/rtc/rtc-digicolor.c
@@ -192,6 +192,10 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
if (IS_ERR(rtc->regs))
return PTR_ERR(rtc->regs);
+ rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
+ if (IS_ERR(rtc->rtc_dev))
+ return PTR_ERR(rtc->rtc_dev);
+
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
@@ -200,12 +204,10 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
return ret;
platform_set_drvdata(pdev, rtc);
- rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
- &dc_rtc_ops, THIS_MODULE);
- if (IS_ERR(rtc->rtc_dev))
- return PTR_ERR(rtc->rtc_dev);
- return 0;
+ rtc->rtc_dev->ops = &dc_rtc_ops;
+
+ return rtc_register_device(rtc->rtc_dev);
}
static const struct of_device_id dc_dt_ids[] = {
--
2.20.1
While the range of REFERENCE + TIME is actually 33 bits, the counter
itself (TIME) is a 32-bits seconds counter.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/rtc/rtc-digicolor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
index 5bb14c56bc9a..e6e16aaac254 100644
--- a/drivers/rtc/rtc-digicolor.c
+++ b/drivers/rtc/rtc-digicolor.c
@@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, rtc);
rtc->rtc_dev->ops = &dc_rtc_ops;
+ rtc->rtc_dev->range_max = U32_MAX;
return rtc_register_device(rtc->rtc_dev);
}
--
2.20.1
Hi Alexandre,
On Tue, Apr 30 2019, Alexandre Belloni wrote:
> The IRQ is requested before the struct rtc is allocated and registered, but
> this struct is used in the IRQ handler. This may lead to a NULL pointer
> dereference.
>
> Switch to devm_rtc_allocate_device/rtc_register_device to allocate the rtc
> struct before requesting the IRQ.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Acked-by: Baruch Siach <[email protected]>
baruch
> ---
> drivers/rtc/rtc-digicolor.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
> index b253bf1b3531..5bb14c56bc9a 100644
> --- a/drivers/rtc/rtc-digicolor.c
> +++ b/drivers/rtc/rtc-digicolor.c
> @@ -192,6 +192,10 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
> if (IS_ERR(rtc->regs))
> return PTR_ERR(rtc->regs);
>
> + rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> + if (IS_ERR(rtc->rtc_dev))
> + return PTR_ERR(rtc->rtc_dev);
> +
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
> @@ -200,12 +204,10 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
> return ret;
>
> platform_set_drvdata(pdev, rtc);
> - rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
> - &dc_rtc_ops, THIS_MODULE);
> - if (IS_ERR(rtc->rtc_dev))
> - return PTR_ERR(rtc->rtc_dev);
>
> - return 0;
> + rtc->rtc_dev->ops = &dc_rtc_ops;
> +
> + return rtc_register_device(rtc->rtc_dev);
> }
>
> static const struct of_device_id dc_dt_ids[] = {
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
Hi Alexandre,
On Tue, Apr 30 2019, Alexandre Belloni wrote:
> While the range of REFERENCE + TIME is actually 33 bits, the counter
> itself (TIME) is a 32-bits seconds counter.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> drivers/rtc/rtc-digicolor.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
> index 5bb14c56bc9a..e6e16aaac254 100644
> --- a/drivers/rtc/rtc-digicolor.c
> +++ b/drivers/rtc/rtc-digicolor.c
> @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, rtc);
>
> rtc->rtc_dev->ops = &dc_rtc_ops;
> + rtc->rtc_dev->range_max = U32_MAX;
Where can I find documentation on the meaning and usage of the range_max
value? I could not find anything in the kernel source.
baruch
>
> return rtc_register_device(rtc->rtc_dev);
> }
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
On 30/04/2019 14:36:24+0300, Baruch Siach wrote:
> Hi Alexandre,
>
> On Tue, Apr 30 2019, Alexandre Belloni wrote:
>
> > While the range of REFERENCE + TIME is actually 33 bits, the counter
> > itself (TIME) is a 32-bits seconds counter.
> >
> > Signed-off-by: Alexandre Belloni <[email protected]>
> > ---
> > drivers/rtc/rtc-digicolor.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
> > index 5bb14c56bc9a..e6e16aaac254 100644
> > --- a/drivers/rtc/rtc-digicolor.c
> > +++ b/drivers/rtc/rtc-digicolor.c
> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
> > platform_set_drvdata(pdev, rtc);
> >
> > rtc->rtc_dev->ops = &dc_rtc_ops;
> > + rtc->rtc_dev->range_max = U32_MAX;
>
> Where can I find documentation on the meaning and usage of the range_max
> value? I could not find anything in the kernel source.
>
It should be set to the maximum UNIX timestamp the RTC can be set to
while keeping range_min to range_max contiguous.
In the digicolor case, you could go up to 8589934590 (Wed Mar 16
12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm
not sure it can also update DC_RTC_TIME safely.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Alexandre,
On Tue, Apr 30 2019, Alexandre Belloni wrote:
> On 30/04/2019 14:36:24+0300, Baruch Siach wrote:
>> Hi Alexandre,
>>
>> On Tue, Apr 30 2019, Alexandre Belloni wrote:
>>
>> > While the range of REFERENCE + TIME is actually 33 bits, the counter
>> > itself (TIME) is a 32-bits seconds counter.
>> >
>> > Signed-off-by: Alexandre Belloni <[email protected]>
>> > ---
>> > drivers/rtc/rtc-digicolor.c | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
>> > index 5bb14c56bc9a..e6e16aaac254 100644
>> > --- a/drivers/rtc/rtc-digicolor.c
>> > +++ b/drivers/rtc/rtc-digicolor.c
>> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
>> > platform_set_drvdata(pdev, rtc);
>> >
>> > rtc->rtc_dev->ops = &dc_rtc_ops;
>> > + rtc->rtc_dev->range_max = U32_MAX;
>>
>> Where can I find documentation on the meaning and usage of the range_max
>> value? I could not find anything in the kernel source.
>
> It should be set to the maximum UNIX timestamp the RTC can be set to
> while keeping range_min to range_max contiguous.
>
> In the digicolor case, you could go up to 8589934590 (Wed Mar 16
> 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm
> not sure it can also update DC_RTC_TIME safely.
DC_RTC_TIME resets to zero whenever dc_rtc_write writes CMD_RESET to the
DC_RTC_CONTROL register. DC_RTC_REFERENCE keeps the value that
dc_rtc_write stores there. So the driver will return values larger than
U32_MAX if you happen to cross this point in time between dc_rtc_write
and dc_rtc_read. But you can't store a value larger than U32_MAX in
DC_RTC_REFERENCE.
Will the core RTC code handle the U32_MAX cross correctly?
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
On 30/04/2019 15:20:08+0300, Baruch Siach wrote:
> Hi Alexandre,
>
> On Tue, Apr 30 2019, Alexandre Belloni wrote:
> > On 30/04/2019 14:36:24+0300, Baruch Siach wrote:
> >> Hi Alexandre,
> >>
> >> On Tue, Apr 30 2019, Alexandre Belloni wrote:
> >>
> >> > While the range of REFERENCE + TIME is actually 33 bits, the counter
> >> > itself (TIME) is a 32-bits seconds counter.
> >> >
> >> > Signed-off-by: Alexandre Belloni <[email protected]>
> >> > ---
> >> > drivers/rtc/rtc-digicolor.c | 1 +
> >> > 1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
> >> > index 5bb14c56bc9a..e6e16aaac254 100644
> >> > --- a/drivers/rtc/rtc-digicolor.c
> >> > +++ b/drivers/rtc/rtc-digicolor.c
> >> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
> >> > platform_set_drvdata(pdev, rtc);
> >> >
> >> > rtc->rtc_dev->ops = &dc_rtc_ops;
> >> > + rtc->rtc_dev->range_max = U32_MAX;
> >>
> >> Where can I find documentation on the meaning and usage of the range_max
> >> value? I could not find anything in the kernel source.
> >
> > It should be set to the maximum UNIX timestamp the RTC can be set to
> > while keeping range_min to range_max contiguous.
> >
> > In the digicolor case, you could go up to 8589934590 (Wed Mar 16
> > 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm
> > not sure it can also update DC_RTC_TIME safely.
>
> DC_RTC_TIME resets to zero whenever dc_rtc_write writes CMD_RESET to the
> DC_RTC_CONTROL register. DC_RTC_REFERENCE keeps the value that
> dc_rtc_write stores there. So the driver will return values larger than
> U32_MAX if you happen to cross this point in time between dc_rtc_write
> and dc_rtc_read. But you can't store a value larger than U32_MAX in
> DC_RTC_REFERENCE.
>
> Will the core RTC code handle the U32_MAX cross correctly?
>
Yes, this is ok to return a valid value that is higher than range_max.
However, at that time, you will not be able to set any alarms anymore as
the core doesn't allow to set alarms after range_max.
I would think that this is fine because this will happen in 2106 and we
have a way to offset the time (the whole goal of setting the range)
using device tree.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Alexandre,
On Tue, Apr 30 2019, Alexandre Belloni wrote:
> On 30/04/2019 15:20:08+0300, Baruch Siach wrote:
>> On Tue, Apr 30 2019, Alexandre Belloni wrote:
>> > On 30/04/2019 14:36:24+0300, Baruch Siach wrote:
>> >> Hi Alexandre,
>> >>
>> >> On Tue, Apr 30 2019, Alexandre Belloni wrote:
>> >>
>> >> > While the range of REFERENCE + TIME is actually 33 bits, the counter
>> >> > itself (TIME) is a 32-bits seconds counter.
>> >> >
>> >> > Signed-off-by: Alexandre Belloni <[email protected]>
>> >> > ---
>> >> > drivers/rtc/rtc-digicolor.c | 1 +
>> >> > 1 file changed, 1 insertion(+)
>> >> >
>> >> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c
>> >> > index 5bb14c56bc9a..e6e16aaac254 100644
>> >> > --- a/drivers/rtc/rtc-digicolor.c
>> >> > +++ b/drivers/rtc/rtc-digicolor.c
>> >> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev)
>> >> > platform_set_drvdata(pdev, rtc);
>> >> >
>> >> > rtc->rtc_dev->ops = &dc_rtc_ops;
>> >> > + rtc->rtc_dev->range_max = U32_MAX;
>> >>
>> >> Where can I find documentation on the meaning and usage of the range_max
>> >> value? I could not find anything in the kernel source.
>> >
>> > It should be set to the maximum UNIX timestamp the RTC can be set to
>> > while keeping range_min to range_max contiguous.
>> >
>> > In the digicolor case, you could go up to 8589934590 (Wed Mar 16
>> > 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm
>> > not sure it can also update DC_RTC_TIME safely.
>>
>> DC_RTC_TIME resets to zero whenever dc_rtc_write writes CMD_RESET to the
>> DC_RTC_CONTROL register. DC_RTC_REFERENCE keeps the value that
>> dc_rtc_write stores there. So the driver will return values larger than
>> U32_MAX if you happen to cross this point in time between dc_rtc_write
>> and dc_rtc_read. But you can't store a value larger than U32_MAX in
>> DC_RTC_REFERENCE.
>>
>> Will the core RTC code handle the U32_MAX cross correctly?
>
> Yes, this is ok to return a valid value that is higher than range_max.
> However, at that time, you will not be able to set any alarms anymore as
> the core doesn't allow to set alarms after range_max.
>
> I would think that this is fine because this will happen in 2106 and we
> have a way to offset the time (the whole goal of setting the range)
> using device tree.
That's the sort of documentation that I'm missing. The 'start-year'
property is mentioned in the DT binding documentation. But I don't see
where range_max is documented as a facility for the time offset feature.
Anyway,
Acked-by: Baruch Siach <[email protected]>
Thanks,
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
On 30/04/2019 18:25:52+0300, Baruch Siach wrote:
> > Yes, this is ok to return a valid value that is higher than range_max.
> > However, at that time, you will not be able to set any alarms anymore as
> > the core doesn't allow to set alarms after range_max.
> >
> > I would think that this is fine because this will happen in 2106 and we
> > have a way to offset the time (the whole goal of setting the range)
> > using device tree.
>
> That's the sort of documentation that I'm missing. The 'start-year'
> property is mentioned in the DT binding documentation. But I don't see
> where range_max is documented as a facility for the time offset feature.
>
Sure, I'm planning to document better how a proper RTC driver should be
written. I needed to cleanup the digicolor driver because I4m removing
.set_mmss and .set_mmss64 this cycle.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com