[PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Kane Chen]

While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P devices,
We found the data from CMOS_READ is not reasonable and causes RTC wake up fail.

With the below changes, we don't see unreasonable data from cmos and issue is gone.

cd17420: rtc: cmos: avoid UIP when writing alarm time
cdedc45: rtc: cmos: avoid UIP when reading alarm time
ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
ea6fa49: rtc: mc146818-lib: fix RTC presence check
13be2ef: rtc: cmos: Disable irq around direct invocation of cmos_interrupt()
0dd8d6c: rtc: Check return value from mc146818_get_time()
e1aba37: rtc: cmos: remove stale REVISIT comments
6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ
d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
211e5db: rtc: mc146818: Detect and handle broken RTCs
dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
05a0302: rtc: mc146818: Prevent reading garbage


All of the above patches are landed on 6.0.11 stable kernel
I'd like pick the above patches back to 5.10 longterm kernel
Thanks


-- 
2.17.1

Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Sasha Levin]

On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
>While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P devices,
>We found the data from CMOS_READ is not reasonable and causes RTC wake up fail.
>
>With the below changes, we don't see unreasonable data from cmos and issue is gone.

Thanks for the analysis, I can queue most of these up. There are two
which won't go in:

>cd17420: rtc: cmos: avoid UIP when writing alarm time
>cdedc45: rtc: cmos: avoid UIP when reading alarm time
>ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>ea6fa49: rtc: mc146818-lib: fix RTC presence check
>13be2ef: rtc: cmos: Disable irq around direct invocation of cmos_interrupt()
>0dd8d6c: rtc: Check return value from mc146818_get_time()
>e1aba37: rtc: cmos: remove stale REVISIT comments
>6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ

This one fixes a commit which isn't in the 5.10 tree.

>d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>211e5db: rtc: mc146818: Detect and handle broken RTCs
>dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()

This one looks like an optimization.

-- 
Thanks,
Sasha

RE: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Chen, Kane]

> -----Original Message-----
> From: Sasha Levin <sashal@kernel.org>
> Sent: Wednesday, December 7, 2022 1:13 PM
> To: Chen, Kane <kane.chen@intel.com>
> Cc: stable@vger.kernel.org
> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time
> 
> On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
> >While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P
> >devices, We found the data from CMOS_READ is not reasonable and causes
> RTC wake up fail.
> >
> >With the below changes, we don't see unreasonable data from cmos and
> issue is gone.
> 
> Thanks for the analysis, I can queue most of these up. There are two which
> won't go in:
> 
> >cd17420: rtc: cmos: avoid UIP when writing alarm time
> >cdedc45: rtc: cmos: avoid UIP when reading alarm time
> >ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
> >ea6fa49: rtc: mc146818-lib: fix RTC presence check
> >13be2ef: rtc: cmos: Disable irq around direct invocation of
> >cmos_interrupt()
> >0dd8d6c: rtc: Check return value from mc146818_get_time()
> >e1aba37: rtc: cmos: remove stale REVISIT comments
> >6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard
> >IRQ
> 
> This one fixes a commit which isn't in the 5.10 tree.
> 
> >d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
> >ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
> >211e5db: rtc: mc146818: Detect and handle broken RTCs
> >dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
> 
> This one looks like an optimization.
> 
> --

I'm sorry,
I thought dcf257e and  6950d04, 13be2ef  are also required to avoid cherry-pick conflict
After checking again, dcf257e, 6950d04, 13be2ef are not needed.

Here is the list I would like to pick
cd17420: rtc: cmos: avoid UIP when writing alarm time
cdedc45: rtc: cmos: avoid UIP when reading alarm time
ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
ea6fa49: rtc: mc146818-lib: fix RTC presence check
0dd8d6c: rtc: Check return value from mc146818_get_time()
e1aba37: rtc: cmos: remove stale REVISIT comments
d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
211e5db: rtc: mc146818: Detect and handle broken RTCs
05a0302: rtc: mc146818: Prevent reading garbage

Thanks a lot

> Thanks,
> Sasha

Re: RE: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Mateusz Jończyk]

W dniu 7.12.2022 o 07:51, Chen, Kane pisze:
>> -----Original Message-----
>> From: Sasha Levin <sashal@kernel.org>
>> Sent: Wednesday, December 7, 2022 1:13 PM
>> To: Chen, Kane <kane.chen@intel.com>
>> Cc: stable@vger.kernel.org
>> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time
>>
>> On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
>>> While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P
>>> devices, We found the data from CMOS_READ is not reasonable and causes
>> RTC wake up fail.
>>> With the below changes, we don't see unreasonable data from cmos and
>> issue is gone.
>>
>> Thanks for the analysis, I can queue most of these up. There are two which
>> won't go in:
>>
>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>>> 13be2ef: rtc: cmos: Disable irq around direct invocation of
>>> cmos_interrupt()
>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>>> e1aba37: rtc: cmos: remove stale REVISIT comments
>>> 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard
>>> IRQ
>> This one fixes a commit which isn't in the 5.10 tree.
>>
>>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>>> dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
>> This one looks like an optimization.
>>
>> --
> I'm sorry,
> I thought dcf257e and  6950d04, 13be2ef  are also required to avoid cherry-pick conflict
> After checking again, dcf257e, 6950d04, 13be2ef are not needed.
>
> Here is the list I would like to pick
> cd17420: rtc: cmos: avoid UIP when writing alarm time
> cdedc45: rtc: cmos: avoid UIP when reading alarm time
> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
> ea6fa49: rtc: mc146818-lib: fix RTC presence check
> 0dd8d6c: rtc: Check return value from mc146818_get_time()
> e1aba37: rtc: cmos: remove stale REVISIT comments
> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
> 211e5db: rtc: mc146818: Detect and handle broken RTCs
> 05a0302: rtc: mc146818: Prevent reading garbage
>
> Thanks a lot
>
>> Thanks,
>> Sasha
>>
Hello,

I think that you should also pick these patches which fix issues in the series
that is prepared for 5.4:

1) commit 7372971c1be5 ("rtc: mc146818-lib: fix signedness bug in mc146818_get_time()")

which fixes d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()

2) commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")

which fixes 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ
and is not prepared for 5.4 stable even though it was mentioned above.

3) commit 811f5559270f ("rtc: mc146818-lib: fix locking in mc146818_set_time")

which fixes dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()


When it comes to these two patches:

cd17420: rtc: cmos: avoid UIP when writing alarm time
cdedc45: rtc: cmos: avoid UIP when reading alarm time

I used to think that the issues they fix were very unlikely to happen in practice,
so I did not submit them into stable. So thanks for proving me wrong.

Greetings,
Mateusz

Re: RE: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Sasha Levin]

On Thu, Dec 08, 2022 at 10:47:17PM +0100, Mateusz Jończyk wrote:
>W dniu 7.12.2022 o 07:51, Chen, Kane pisze:
>>> -----Original Message-----
>>> From: Sasha Levin <sashal@kernel.org>
>>> Sent: Wednesday, December 7, 2022 1:13 PM
>>> To: Chen, Kane <kane.chen@intel.com>
>>> Cc: stable@vger.kernel.org
>>> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time
>>>
>>> On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
>>>> While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P
>>>> devices, We found the data from CMOS_READ is not reasonable and causes
>>> RTC wake up fail.
>>>> With the below changes, we don't see unreasonable data from cmos and
>>> issue is gone.
>>>
>>> Thanks for the analysis, I can queue most of these up. There are two which
>>> won't go in:
>>>
>>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>>>> 13be2ef: rtc: cmos: Disable irq around direct invocation of
>>>> cmos_interrupt()
>>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>>>> e1aba37: rtc: cmos: remove stale REVISIT comments
>>>> 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard
>>>> IRQ
>>> This one fixes a commit which isn't in the 5.10 tree.
>>>
>>>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>>>> dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
>>> This one looks like an optimization.
>>>
>>> --
>> I'm sorry,
>> I thought dcf257e and  6950d04, 13be2ef  are also required to avoid cherry-pick conflict
>> After checking again, dcf257e, 6950d04, 13be2ef are not needed.
>>
>> Here is the list I would like to pick
>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>> e1aba37: rtc: cmos: remove stale REVISIT comments
>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>> 05a0302: rtc: mc146818: Prevent reading garbage
>>
>> Thanks a lot
>>
>>> Thanks,
>>> Sasha
>>>
>Hello,
>
>I think that you should also pick these patches which fix issues in the series
>that is prepared for 5.4:
>
>1) commit 7372971c1be5 ("rtc: mc146818-lib: fix signedness bug in mc146818_get_time()")
>
>which fixes d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>
>2) commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")
>
>which fixes 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ
>and is not prepared for 5.4 stable even though it was mentioned above.
>
>3) commit 811f5559270f ("rtc: mc146818-lib: fix locking in mc146818_set_time")
>
>which fixes dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()

Ack, will do, thanks.

-- 
Thanks,
Sasha

Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Mateusz Jończyk]

W dniu 9.12.2022 o 01:37, Sasha Levin pisze:
> On Thu, Dec 08, 2022 at 10:47:17PM +0100, Mateusz Jończyk wrote:
>> W dniu 7.12.2022 o 07:51, Chen, Kane pisze:
>>>> -----Original Message-----
>>>> From: Sasha Levin <sashal@kernel.org>
>>>> Sent: Wednesday, December 7, 2022 1:13 PM
>>>> To: Chen, Kane <kane.chen@intel.com>
>>>> Cc: stable@vger.kernel.org
>>>> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time
>>>>
>>>> On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
>>>>> While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P
>>>>> devices, We found the data from CMOS_READ is not reasonable and causes
>>>> RTC wake up fail.
>>>>> With the below changes, we don't see unreasonable data from cmos and
>>>> issue is gone.
>>>>
>>>> Thanks for the analysis, I can queue most of these up. There are two which
>>>> won't go in:
>>>>
>>>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>>>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>>>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>>>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>>>>> 13be2ef: rtc: cmos: Disable irq around direct invocation of
>>>>> cmos_interrupt()
>>>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>>>>> e1aba37: rtc: cmos: remove stale REVISIT comments
>>>>> 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard
>>>>> IRQ
>>>> This one fixes a commit which isn't in the 5.10 tree.
>>>>
>>>>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>>>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>>>>> dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
>>>> This one looks like an optimization.
>>>>
>>>> -- 
>>> I'm sorry,
>>> I thought dcf257e and  6950d04, 13be2ef  are also required to avoid cherry-pick conflict
>>> After checking again, dcf257e, 6950d04, 13be2ef are not needed.
>>>
>>> Here is the list I would like to pick
>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>>> e1aba37: rtc: cmos: remove stale REVISIT comments
>>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>>> 05a0302: rtc: mc146818: Prevent reading garbage
>>>
>>> Thanks a lot
>>>
>>>> Thanks,
>>>> Sasha
>>>>
>> Hello,
>>
>> I think that you should also pick these patches which fix issues in the series
>> that is prepared for 5.4:
>>
>> 1) commit 7372971c1be5 ("rtc: mc146818-lib: fix signedness bug in mc146818_get_time()")
>>
>> which fixes d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>
>> 2) commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")
>>
>> which fixes 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ
>> and is not prepared for 5.4 stable even though it was mentioned above.
>>
>> 3) commit 811f5559270f ("rtc: mc146818-lib: fix locking in mc146818_set_time")
>>
>> which fixes dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
>
> Ack, will do, thanks.

Hello,

However, I would like to ask: is it really necessary to include all of
these patches in stable? I think that it is very likely that only these patches
are sufficient to fix the original problem reported by Kane Chen:

cd17420: rtc: cmos: avoid UIP when writing alarm time
cdedc45: rtc: cmos: avoid UIP when reading alarm time
ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP

These patches should be most relevant when using the RTC alarm and are
self-contained. So I would like to ask Kane Chen to recheck with these patches
only. Other patches change the CMOS RTC reading routines significantly
and have a higher risk of introducing regressions (but I am not aware of any
outstanding problems).

The last patch of the three:
ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
introduces only a new function; it won't apply cleanly over kernel 5.4,
but this is straightforward to fix.

Greetings,

Mateusz

Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Sasha Levin]

On Fri, Dec 09, 2022 at 11:40:56PM +0100, Mateusz Jończyk wrote:
>W dniu 9.12.2022 o 01:37, Sasha Levin pisze:
>> On Thu, Dec 08, 2022 at 10:47:17PM +0100, Mateusz Jończyk wrote:
>>> W dniu 7.12.2022 o 07:51, Chen, Kane pisze:
>>>>> -----Original Message-----
>>>>> From: Sasha Levin <sashal@kernel.org>
>>>>> Sent: Wednesday, December 7, 2022 1:13 PM
>>>>> To: Chen, Kane <kane.chen@intel.com>
>>>>> Cc: stable@vger.kernel.org
>>>>> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time
>>>>>
>>>>> On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
>>>>>> While runnings s0ix cycling test based on rtc alarm wakeup on ADL-P
>>>>>> devices, We found the data from CMOS_READ is not reasonable and causes
>>>>> RTC wake up fail.
>>>>>> With the below changes, we don't see unreasonable data from cmos and
>>>>> issue is gone.
>>>>>
>>>>> Thanks for the analysis, I can queue most of these up. There are two which
>>>>> won't go in:
>>>>>
>>>>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>>>>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>>>>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>>>>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>>>>>> 13be2ef: rtc: cmos: Disable irq around direct invocation of
>>>>>> cmos_interrupt()
>>>>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>>>>>> e1aba37: rtc: cmos: remove stale REVISIT comments
>>>>>> 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard
>>>>>> IRQ
>>>>> This one fixes a commit which isn't in the 5.10 tree.
>>>>>
>>>>>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>>>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>>>>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>>>>>> dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
>>>>> This one looks like an optimization.
>>>>>
>>>>> --
>>>> I'm sorry,
>>>> I thought dcf257e and  6950d04, 13be2ef  are also required to avoid cherry-pick conflict
>>>> After checking again, dcf257e, 6950d04, 13be2ef are not needed.
>>>>
>>>> Here is the list I would like to pick
>>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
>>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
>>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
>>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
>>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
>>>> e1aba37: rtc: cmos: remove stale REVISIT comments
>>>> d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
>>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
>>>> 05a0302: rtc: mc146818: Prevent reading garbage
>>>>
>>>> Thanks a lot
>>>>
>>>>> Thanks,
>>>>> Sasha
>>>>>
>>> Hello,
>>>
>>> I think that you should also pick these patches which fix issues in the series
>>> that is prepared for 5.4:
>>>
>>> 1) commit 7372971c1be5 ("rtc: mc146818-lib: fix signedness bug in mc146818_get_time()")
>>>
>>> which fixes d35786b: rtc: mc146818-lib: change return values of mc146818_get_time()
>>>
>>> 2) commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")
>>>
>>> which fixes 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ
>>> and is not prepared for 5.4 stable even though it was mentioned above.
>>>
>>> 3) commit 811f5559270f ("rtc: mc146818-lib: fix locking in mc146818_set_time")
>>>
>>> which fixes dcf257e: rtc: mc146818: Reduce spinlock section in mc146818_set_time()
>>
>> Ack, will do, thanks.
>
>Hello,
>
>However, I would like to ask: is it really necessary to include all of
>these patches in stable? I think that it is very likely that only these patches
>are sufficient to fix the original problem reported by Kane Chen:
>
>cd17420: rtc: cmos: avoid UIP when writing alarm time
>cdedc45: rtc: cmos: avoid UIP when reading alarm time
>ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP

I would agree that this could be enough to fix the issue that was
described, but...

>These patches should be most relevant when using the RTC alarm and are
>self-contained. So I would like to ask Kane Chen to recheck with these patches
>only. Other patches change the CMOS RTC reading routines significantly
>and have a higher risk of introducing regressions (but I am not aware of any
>outstanding problems).

The rest of the patches do look like fixes that should have been added
to stable (or are dependencies of such fixes) on their own. Given
those patches are pretty old, any reason to not just include them?

The process works better if we address issues before making users come
and complain on the mailing list :)

-- 
Thanks,
Sasha

RE: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time

[Chen, Kane]

> -----Original Message-----
> From: Sasha Levin <sashal@kernel.org>
> Sent: Sunday, December 11, 2022 2:10 PM
> To: Mateusz Jończyk <mat.jonczyk@o2.pl>
> Cc: Chen, Kane <kane.chen@intel.com>; stable@vger.kernel.org; Alexandre
> Belloni <alexandre.belloni@bootlin.com>
> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading alarm time
> 
> On Fri, Dec 09, 2022 at 11:40:56PM +0100, Mateusz Jończyk wrote:
> >W dniu 9.12.2022 o 01:37, Sasha Levin pisze:
> >> On Thu, Dec 08, 2022 at 10:47:17PM +0100, Mateusz Jończyk wrote:
> >>> W dniu 7.12.2022 o 07:51, Chen, Kane pisze:
> >>>>> -----Original Message-----
> >>>>> From: Sasha Levin <sashal@kernel.org>
> >>>>> Sent: Wednesday, December 7, 2022 1:13 PM
> >>>>> To: Chen, Kane <kane.chen@intel.com>
> >>>>> Cc: stable@vger.kernel.org
> >>>>> Subject: Re: [PATCH v1] rtc: cmos: avoid UIP when writing/reading
> >>>>> alarm time
> >>>>>
> >>>>> On Wed, Dec 07, 2022 at 11:57:22AM +0800, Kane Chen wrote:
> >>>>>> While runnings s0ix cycling test based on rtc alarm wakeup on
> >>>>>> ADL-P devices, We found the data from CMOS_READ is not reasonable
> >>>>>> and causes
> >>>>> RTC wake up fail.
> >>>>>> With the below changes, we don't see unreasonable data from cmos
> >>>>>> and
> >>>>> issue is gone.
> >>>>>
> >>>>> Thanks for the analysis, I can queue most of these up. There are
> >>>>> two which won't go in:
> >>>>>
> >>>>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
> >>>>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
> >>>>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
> >>>>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
> >>>>>> 13be2ef: rtc: cmos: Disable irq around direct invocation of
> >>>>>> cmos_interrupt()
> >>>>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
> >>>>>> e1aba37: rtc: cmos: remove stale REVISIT comments
> >>>>>> 6950d04: rtc: cmos: Replace spin_lock_irqsave with spin_lock in
> >>>>>> hard IRQ
> >>>>> This one fixes a commit which isn't in the 5.10 tree.
> >>>>>
> >>>>>> d35786b: rtc: mc146818-lib: change return values of
> >>>>>> mc146818_get_time()
> >>>>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
> >>>>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
> >>>>>> dcf257e: rtc: mc146818: Reduce spinlock section in
> >>>>>> mc146818_set_time()
> >>>>> This one looks like an optimization.
> >>>>>
> >>>>> --
> >>>> I'm sorry,
> >>>> I thought dcf257e and  6950d04, 13be2ef  are also required to avoid
> >>>> cherry-pick conflict After checking again, dcf257e, 6950d04, 13be2ef are
> not needed.
> >>>>
> >>>> Here is the list I would like to pick
> >>>> cd17420: rtc: cmos: avoid UIP when writing alarm time
> >>>> cdedc45: rtc: cmos: avoid UIP when reading alarm time
> >>>> ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
> >>>> ea6fa49: rtc: mc146818-lib: fix RTC presence check
> >>>> 0dd8d6c: rtc: Check return value from mc146818_get_time()
> >>>> e1aba37: rtc: cmos: remove stale REVISIT comments
> >>>> d35786b: rtc: mc146818-lib: change return values of
> >>>> mc146818_get_time()
> >>>> ebb22a0: rtc: mc146818: Dont test for bit 0-5 in Register D
> >>>> 211e5db: rtc: mc146818: Detect and handle broken RTCs
> >>>> 05a0302: rtc: mc146818: Prevent reading garbage
> >>>>
> >>>> Thanks a lot
> >>>>
> >>>>> Thanks,
> >>>>> Sasha
> >>>>>
> >>> Hello,
> >>>
> >>> I think that you should also pick these patches which fix issues in
> >>> the series that is prepared for 5.4:
> >>>
> >>> 1) commit 7372971c1be5 ("rtc: mc146818-lib: fix signedness bug in
> >>> mc146818_get_time()")
> >>>
> >>> which fixes d35786b: rtc: mc146818-lib: change return values of
> >>> mc146818_get_time()
> >>>
> >>> 2) commit 13be2efc390a ("rtc: cmos: Disable irq around direct
> >>> invocation of cmos_interrupt()")
> >>>
> >>> which fixes 6950d04: rtc: cmos: Replace spin_lock_irqsave with
> >>> spin_lock in hard IRQ and is not prepared for 5.4 stable even though it was
> mentioned above.
> >>>
> >>> 3) commit 811f5559270f ("rtc: mc146818-lib: fix locking in
> >>> mc146818_set_time")
> >>>
> >>> which fixes dcf257e: rtc: mc146818: Reduce spinlock section in
> >>> mc146818_set_time()
> >>
> >> Ack, will do, thanks.
> >
> >Hello,
> >
> >However, I would like to ask: is it really necessary to include all of
> >these patches in stable? I think that it is very likely that only these
> >patches are sufficient to fix the original problem reported by Kane Chen:
> >
> >cd17420: rtc: cmos: avoid UIP when writing alarm time
> >cdedc45: rtc: cmos: avoid UIP when reading alarm time
> >ec5895c: rtc: mc146818-lib: extract mc146818_avoid_UIP
> 

"05a0302: rtc: mc146818: Prevent reading garbage" this helps the issue as well I think.

I didn't test these 3 patches alone because it will create conflicts when I pick ec5895c.
I'm worried I made things wrong and thought those dependencies are landed for a long time
So decided to pick related dependencies eventually.

Pls let me know if you have concern :)


> I would agree that this could be enough to fix the issue that was described, but...
> 
> >These patches should be most relevant when using the RTC alarm and are
> >self-contained. So I would like to ask Kane Chen to recheck with these
> >patches only. Other patches change the CMOS RTC reading routines
> >significantly and have a higher risk of introducing regressions (but I
> >am not aware of any outstanding problems).
> 
> The rest of the patches do look like fixes that should have been added to stable
> (or are dependencies of such fixes) on their own. Given those patches are pretty
> old, any reason to not just include them?
> 
> The process works better if we address issues before making users come and
> complain on the mailing list :)
> 
> --
> Thanks,
> Sasha