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

[Sasha Levin <sashal@kernel.org>]

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