Re: [RFC PATCH v2 1/5] rtc: migrate driver data to RTC device

[Ke Sun <sk.alvin.x@gmail.com>]

On 1/8/26 07:18, Ke Sun wrote:
>
> On 1/8/26 00:12, Greg KH wrote:
>> On Wed, Jan 07, 2026 at 10:37:33PM +0800, Ke Sun wrote:
>>> Unify RTC driver interface by storing driver data on the RTC device
>>> instead of the parent device. Update RTC ops callbacks to pass the RTC
>>> device itself rather than its parent. This change enables better
>>> support for Rust RTC drivers that store data on the RTC device.
>>>
>>> Signed-off-by: Ke Sun <sunke@kylinos.cn>
>>> ---
>>>   drivers/rtc/dev.c       |  4 ++--
>>>   drivers/rtc/interface.c | 18 +++++++++---------
>>>   drivers/rtc/rtc-pl031.c |  9 ++-------
>>>   3 files changed, 13 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c
>>> index baf1a8ca8b2b1..0f62ba9342e3e 100644
>>> --- a/drivers/rtc/dev.c
>>> +++ b/drivers/rtc/dev.c
>>> @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file,
>>>           }
>>>           default:
>>>               if (rtc->ops->param_get)
>>> -                err = rtc->ops->param_get(rtc->dev.parent, &param);
>>> +                err = rtc->ops->param_get(&rtc->dev, &param);
>>>               else
>>>                   err = -EINVAL;
>>>           }
>>> @@ -440,7 +440,7 @@ static long rtc_dev_ioctl(struct file *file,
>>>             default:
>>>               if (rtc->ops->param_set)
>>> -                err = rtc->ops->param_set(rtc->dev.parent, &param);
>>> +                err = rtc->ops->param_set(&rtc->dev, &param);
>>>               else
>>>                   err = -EINVAL;
>>>           }
>>> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
>>> index b8b298efd9a9c..783a3ec3bb93d 100644
>>> --- a/drivers/rtc/interface.c
>>> +++ b/drivers/rtc/interface.c
>>> @@ -91,7 +91,7 @@ static int __rtc_read_time(struct rtc_device *rtc, 
>>> struct rtc_time *tm)
>>>           err = -EINVAL;
>>>       } else {
>>>           memset(tm, 0, sizeof(struct rtc_time));
>>> -        err = rtc->ops->read_time(rtc->dev.parent, tm);
>>> +        err = rtc->ops->read_time(&rtc->dev, tm);
>>>           if (err < 0) {
>>>               dev_dbg(&rtc->dev, "read_time: fail to read: %d\n",
>>>                   err);
>>> @@ -155,7 +155,7 @@ int rtc_set_time(struct rtc_device *rtc, struct 
>>> rtc_time *tm)
>>>       if (!rtc->ops)
>>>           err = -ENODEV;
>>>       else if (rtc->ops->set_time)
>>> -        err = rtc->ops->set_time(rtc->dev.parent, tm);
>>> +        err = rtc->ops->set_time(&rtc->dev, tm);
>>>       else
>>>           err = -EINVAL;
>>>   @@ -200,7 +200,7 @@ static int rtc_read_alarm_internal(struct 
>>> rtc_device *rtc,
>>>           alarm->time.tm_wday = -1;
>>>           alarm->time.tm_yday = -1;
>>>           alarm->time.tm_isdst = -1;
>>> -        err = rtc->ops->read_alarm(rtc->dev.parent, alarm);
>>> +        err = rtc->ops->read_alarm(&rtc->dev, alarm);
>>>       }
>>>         mutex_unlock(&rtc->ops_lock);
>>> @@ -441,7 +441,7 @@ static int __rtc_set_alarm(struct rtc_device 
>>> *rtc, struct rtc_wkalrm *alarm)
>>>       else if (!test_bit(RTC_FEATURE_ALARM, rtc->features))
>>>           err = -EINVAL;
>>>       else
>>> -        err = rtc->ops->set_alarm(rtc->dev.parent, alarm);
>>> +        err = rtc->ops->set_alarm(&rtc->dev, alarm);
>>>         /*
>>>        * Check for potential race described above. If the waiting 
>>> for next
>>> @@ -568,7 +568,7 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, 
>>> unsigned int enabled)
>>>       else if (!test_bit(RTC_FEATURE_ALARM, rtc->features) || 
>>> !rtc->ops->alarm_irq_enable)
>>>           err = -EINVAL;
>>>       else
>>> -        err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
>>> +        err = rtc->ops->alarm_irq_enable(&rtc->dev, enabled);
>>>         mutex_unlock(&rtc->ops_lock);
>>>   @@ -618,7 +618,7 @@ int rtc_update_irq_enable(struct rtc_device 
>>> *rtc, unsigned int enabled)
>>>           rtc->uie_rtctimer.period = ktime_set(1, 0);
>>>           err = rtc_timer_enqueue(rtc, &rtc->uie_rtctimer);
>>>           if (!err && rtc->ops && rtc->ops->alarm_irq_enable)
>>> -            err = rtc->ops->alarm_irq_enable(rtc->dev.parent, 1);
>>> +            err = rtc->ops->alarm_irq_enable(&rtc->dev, 1);
>>>           if (err)
>>>               goto out;
>>>       } else {
>>> @@ -874,7 +874,7 @@ static void rtc_alarm_disable(struct rtc_device 
>>> *rtc)
>>>       if (!rtc->ops || !test_bit(RTC_FEATURE_ALARM, rtc->features) 
>>> || !rtc->ops->alarm_irq_enable)
>>>           return;
>>>   -    rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
>>> +    rtc->ops->alarm_irq_enable(&rtc->dev, false);
>>>       trace_rtc_alarm_irq_enable(0, 0);
>>>   }
>>>   @@ -1076,7 +1076,7 @@ int rtc_read_offset(struct rtc_device *rtc, 
>>> long *offset)
>>>           return -EINVAL;
>>>         mutex_lock(&rtc->ops_lock);
>>> -    ret = rtc->ops->read_offset(rtc->dev.parent, offset);
>>> +    ret = rtc->ops->read_offset(&rtc->dev, offset);
>>>       mutex_unlock(&rtc->ops_lock);
>>>         trace_rtc_read_offset(*offset, ret);
>>> @@ -1111,7 +1111,7 @@ int rtc_set_offset(struct rtc_device *rtc, 
>>> long offset)
>>>           return -EINVAL;
>>>         mutex_lock(&rtc->ops_lock);
>>> -    ret = rtc->ops->set_offset(rtc->dev.parent, offset);
>>> +    ret = rtc->ops->set_offset(&rtc->dev, offset);
>>>       mutex_unlock(&rtc->ops_lock);
>>>         trace_rtc_set_offset(offset, ret);
>>> diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c
>>> index eab39dfa4e5fe..a605034d44cb7 100644
>>> --- a/drivers/rtc/rtc-pl031.c
>>> +++ b/drivers/rtc/rtc-pl031.c
>>> @@ -284,10 +284,6 @@ static int pl031_set_alarm(struct device *dev, 
>>> struct rtc_wkalrm *alarm)
>>>     static void pl031_remove(struct amba_device *adev)
>>>   {
>>> -    struct pl031_local *ldata = dev_get_drvdata(&adev->dev);
>>> -
>>> -    if (adev->irq[0])
>>> -        free_irq(adev->irq[0], ldata);
>>>       amba_release_regions(adev);
>>>   }
>>>   @@ -320,8 +316,6 @@ static int pl031_probe(struct amba_device 
>>> *adev, const struct amba_id *id)
>>>           goto out;
>>>       }
>>>   -    amba_set_drvdata(adev, ldata);
>>> -
>>>       dev_dbg(&adev->dev, "designer ID = 0x%02x\n", amba_manf(adev));
>>>       dev_dbg(&adev->dev, "revision = 0x%01x\n", amba_rev(adev));
>>>   @@ -356,6 +350,7 @@ static int pl031_probe(struct amba_device 
>>> *adev, const struct amba_id *id)
>>>           ret = PTR_ERR(ldata->rtc);
>>>           goto out;
>>>       }
>>> +    dev_set_drvdata(&ldata->rtc->dev, ldata);
>>>         if (!adev->irq[0])
>>>           clear_bit(RTC_FEATURE_ALARM, ldata->rtc->features);
>>> @@ -369,7 +364,7 @@ static int pl031_probe(struct amba_device *adev, 
>>> const struct amba_id *id)
>>>           goto out;
>>>         if (adev->irq[0]) {
>>> -        ret = request_irq(adev->irq[0], pl031_interrupt,
>>> +        ret = devm_request_irq(&adev->dev, adev->irq[0], 
>>> pl031_interrupt,
>>>                     vendor->irqflags, "rtc-pl031", ldata);
>> Are you _SURE_ you can use devm for this?  it is a functional change,
>
> Since ldata's lifecycle is now tied to the RTC device (stored via
> dev_set_drvdata(&ldata->rtc->dev, ldata)), and the RTC device's lifecycle
> is tied to the amba_device (via devm_rtc_allocate_device(&adev->dev)),
> using devm_request_irq(&adev->dev, ...) allows us to remove the manual 
> IRQ
> release in pl031_remove, as the IRQ will be automatically released along
> with the amba_device lifecycle.
>
>
> Is this reasoning correct, or should I handle this differently?

Correction: Should use devm_request_irq(&ldata->rtc->dev, ...) instead

of devm_request_irq(&adev->dev, ...).


While a lifecycle chain exists (ldata -> rtc -> amba_device), the IRQ 
should

be bound to the device that uses it (the RTC device), matching where driver

data is stored, to avoid UAF.

>
>
> thanks,
>
> Ke Sun
>
>> one that trips lots of people up.  I wouldn't make this change without
>> at least saying why you are doing so in the changelog text, which I
>> didn't see at all here.
>>
>> thanks,
>>
>> greg k-h