> -----Original Message-----
> From: Sasha Levin <sashal@xxxxxxxxxx>
> Sent: Sunday, December 11, 2022 2:10 PM
> To: Mateusz Jończyk <mat.jonczyk@xxxxx>
> Cc: Chen, Kane <kane.chen@xxxxxxxxx>; stable@xxxxxxxxxxxxxxx; Alexandre
> Belloni <alexandre.belloni@xxxxxxxxxxx>
> 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@xxxxxxxxxx>
> >>>>> Sent: Wednesday, December 7, 2022 1:13 PM
> >>>>> To: Chen, Kane <kane.chen@xxxxxxxxx>
> >>>>> Cc: stable@xxxxxxxxxxxxxxx
> >>>>> 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
