| Message ID | 87zh0nbnha.fsf@nanos.tec.linutronix.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [V2] rtc: mc146818: Dont test for bit 0-5 in Register D | expand |
On Mon, Feb 1, 2021 at 11:24 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > While it cures the problem on the reporters machine it breaks machines > with Intel chipsets which use bit 0-5 of the D register. So check only > for bit 6 being 0 which is the case on these Intel machines as well. This looks fine, but it might also be worth it simply just checking for the only really special value: 0xff, and going "ok, that looks like missing hardware". That's what a few other drivers historically do in their probing routines, so it's not unheard of (ie you can find drivers doing that kind of /* If we read 0xff from the LSR, there is no UART here. */ if (inb(.. port ..) == 0xff) in their init routines. Not a big deal either way, I just think it would be more in like with what other places do in similar situations Linus
On 01/02/2021 20:24:17+0100, Thomas Gleixner wrote: > The recent change to validate the RTC turned out to be overly tight. > > While it cures the problem on the reporters machine it breaks machines > with Intel chipsets which use bit 0-5 of the D register. So check only > for bit 6 being 0 which is the case on these Intel machines as well. > > Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") > Reported-by: Serge Belyshev <belyshev@depni.sinp.msu.ru> > Reported-by: Dirk Gouders <dirk@gouders.net> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> I'm still fine with that going through your tree. Thanks for this work I do hope this will be the last issue...
On Mon, Feb 01 2021 at 11:32, Linus Torvalds wrote: > On Mon, Feb 1, 2021 at 11:24 AM Thomas Gleixner <tglx@linutronix.de> wrote: >> >> While it cures the problem on the reporters machine it breaks machines >> with Intel chipsets which use bit 0-5 of the D register. So check only >> for bit 6 being 0 which is the case on these Intel machines as well. > > This looks fine, but it might also be worth it simply just checking > for the only really special value: 0xff, and going "ok, that looks > like missing hardware". > > That's what a few other drivers historically do in their probing > routines, so it's not unheard of (ie you can find drivers doing that > kind of > > /* If we read 0xff from the LSR, there is no UART here. */ > if (inb(.. port ..) == 0xff) > > in their init routines. > > Not a big deal either way, I just think it would be more in like with > what other places do in similar situations Yeah, we can do that as well. Either way is fine. Thanks, tglx
On Mon, Feb 01, 2021 at 08:24:17PM +0100, Thomas Gleixner wrote: > The recent change to validate the RTC turned out to be overly tight. > > While it cures the problem on the reporters machine it breaks machines > with Intel chipsets which use bit 0-5 of the D register. So check only > for bit 6 being 0 which is the case on these Intel machines as well. > > Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") > Reported-by: Serge Belyshev <belyshev@depni.sinp.msu.ru> > Reported-by: Dirk Gouders <dirk@gouders.net> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Provide the actual delta patch. Should have stayed away from > computers today.... > --- > drivers/rtc/rtc-cmos.c | 4 ++-- > drivers/rtc/rtc-mc146818-lib.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) FWIW: Reported-by: Borislav Petkov <bp@suse.de> Tested-by: Borislav Petkov <bp@suse.de> Thx.
Thomas Gleixner <tglx@linutronix.de> writes: > The recent change to validate the RTC turned out to be overly tight. > > While it cures the problem on the reporters machine it breaks machines > with Intel chipsets which use bit 0-5 of the D register. So check only > for bit 6 being 0 which is the case on these Intel machines as well. > > Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") > Reported-by: Serge Belyshev <belyshev@depni.sinp.msu.ru> > Reported-by: Dirk Gouders <dirk@gouders.net> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Provide the actual delta patch. Should have stayed away from > computers today.... I tested V2 and it eliminates the warning, here. Thank you, Dirk > --- > drivers/rtc/rtc-cmos.c | 4 ++-- > drivers/rtc/rtc-mc146818-lib.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > --- a/drivers/rtc/rtc-cmos.c > +++ b/drivers/rtc/rtc-cmos.c > @@ -805,8 +805,8 @@ cmos_do_probe(struct device *dev, struct > > spin_lock_irq(&rtc_lock); > > - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ > - if ((CMOS_READ(RTC_VALID) & 0x7f) != 0) { > + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ > + if ((CMOS_READ(RTC_VALID) & 0x40) != 0) { > spin_unlock_irq(&rtc_lock); > dev_warn(dev, "not accessible\n"); > retval = -ENXIO; > --- a/drivers/rtc/rtc-mc146818-lib.c > +++ b/drivers/rtc/rtc-mc146818-lib.c > @@ -21,8 +21,8 @@ unsigned int mc146818_get_time(struct rt > > again: > spin_lock_irqsave(&rtc_lock, flags); > - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ > - if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x7f) != 0)) { > + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ > + if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x40) != 0)) { > spin_unlock_irqrestore(&rtc_lock, flags); > memset(time, 0xff, sizeof(*time)); > return 0;
Thanks for the update, Thomas. V1 prevented rc6 automated suspend/resume testing on all 13 of my local machines. V2 applied, and they are back in business. tested-by: Len Brown <len.brown@intel.com> On Mon, Feb 1, 2021 at 2:25 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > The recent change to validate the RTC turned out to be overly tight. > > While it cures the problem on the reporters machine it breaks machines > with Intel chipsets which use bit 0-5 of the D register. So check only > for bit 6 being 0 which is the case on these Intel machines as well. > > Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") > Reported-by: Serge Belyshev <belyshev@depni.sinp.msu.ru> > Reported-by: Dirk Gouders <dirk@gouders.net> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Provide the actual delta patch. Should have stayed away from > computers today.... > --- > drivers/rtc/rtc-cmos.c | 4 ++-- > drivers/rtc/rtc-mc146818-lib.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > --- a/drivers/rtc/rtc-cmos.c > +++ b/drivers/rtc/rtc-cmos.c > @@ -805,8 +805,8 @@ cmos_do_probe(struct device *dev, struct > > spin_lock_irq(&rtc_lock); > > - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ > - if ((CMOS_READ(RTC_VALID) & 0x7f) != 0) { > + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ > + if ((CMOS_READ(RTC_VALID) & 0x40) != 0) { > spin_unlock_irq(&rtc_lock); > dev_warn(dev, "not accessible\n"); > retval = -ENXIO; > --- a/drivers/rtc/rtc-mc146818-lib.c > +++ b/drivers/rtc/rtc-mc146818-lib.c > @@ -21,8 +21,8 @@ unsigned int mc146818_get_time(struct rt > > again: > spin_lock_irqsave(&rtc_lock, flags); > - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ > - if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x7f) != 0)) { > + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ > + if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x40) != 0)) { > spin_unlock_irqrestore(&rtc_lock, flags); > memset(time, 0xff, sizeof(*time)); > return 0;
FWIW, it's still OK for me. Tested-by: Mickaël Salaün <mic@linux.microsoft.com> On 01/02/2021 20:24, Thomas Gleixner wrote: > The recent change to validate the RTC turned out to be overly tight. > > While it cures the problem on the reporters machine it breaks machines > with Intel chipsets which use bit 0-5 of the D register. So check only > for bit 6 being 0 which is the case on these Intel machines as well. > > Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") > Reported-by: Serge Belyshev <belyshev@depni.sinp.msu.ru> > Reported-by: Dirk Gouders <dirk@gouders.net> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Provide the actual delta patch. Should have stayed away from > computers today.... > --- > drivers/rtc/rtc-cmos.c | 4 ++-- > drivers/rtc/rtc-mc146818-lib.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > --- a/drivers/rtc/rtc-cmos.c > +++ b/drivers/rtc/rtc-cmos.c > @@ -805,8 +805,8 @@ cmos_do_probe(struct device *dev, struct > > spin_lock_irq(&rtc_lock); > > - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ > - if ((CMOS_READ(RTC_VALID) & 0x7f) != 0) { > + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ > + if ((CMOS_READ(RTC_VALID) & 0x40) != 0) { > spin_unlock_irq(&rtc_lock); > dev_warn(dev, "not accessible\n"); > retval = -ENXIO; > --- a/drivers/rtc/rtc-mc146818-lib.c > +++ b/drivers/rtc/rtc-mc146818-lib.c > @@ -21,8 +21,8 @@ unsigned int mc146818_get_time(struct rt > > again: > spin_lock_irqsave(&rtc_lock, flags); > - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ > - if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x7f) != 0)) { > + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ > + if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x40) != 0)) { > spin_unlock_irqrestore(&rtc_lock, flags); > memset(time, 0xff, sizeof(*time)); > return 0; >
On Mon, 1 Feb 2021, Thomas Gleixner wrote: > >> While it cures the problem on the reporters machine it breaks machines > >> with Intel chipsets which use bit 0-5 of the D register. So check only > >> for bit 6 being 0 which is the case on these Intel machines as well. > > > > This looks fine, but it might also be worth it simply just checking > > for the only really special value: 0xff, and going "ok, that looks > > like missing hardware". > > > > That's what a few other drivers historically do in their probing > > routines, so it's not unheard of (ie you can find drivers doing that > > kind of > > > > /* If we read 0xff from the LSR, there is no UART here. */ > > if (inb(.. port ..) == 0xff) > > > > in their init routines. > > > > Not a big deal either way, I just think it would be more in like with > > what other places do in similar situations > > Yeah, we can do that as well. Either way is fine. Given that evidently vendors appear to start playing with 146818 clones it may be worth it to peek at the D and the C register and checking they are not 0xff both at a time for robustness before concluding no RTC is present. The C register is supposed to hold zeros in bits 3:0. A read of the C register will drop interrupt bits, but I guess it does not matter at the probe time. FWIW, Maciej
--- a/drivers/rtc/rtc-cmos.c +++ b/drivers/rtc/rtc-cmos.c @@ -805,8 +805,8 @@ cmos_do_probe(struct device *dev, struct spin_lock_irq(&rtc_lock); - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ - if ((CMOS_READ(RTC_VALID) & 0x7f) != 0) { + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ + if ((CMOS_READ(RTC_VALID) & 0x40) != 0) { spin_unlock_irq(&rtc_lock); dev_warn(dev, "not accessible\n"); retval = -ENXIO; --- a/drivers/rtc/rtc-mc146818-lib.c +++ b/drivers/rtc/rtc-mc146818-lib.c @@ -21,8 +21,8 @@ unsigned int mc146818_get_time(struct rt again: spin_lock_irqsave(&rtc_lock, flags); - /* Ensure that the RTC is accessible. Bit 0-6 must be 0! */ - if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x7f) != 0)) { + /* Ensure that the RTC is accessible. Bit 6 must be 0! */ + if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x40) != 0)) { spin_unlock_irqrestore(&rtc_lock, flags); memset(time, 0xff, sizeof(*time)); return 0;
The recent change to validate the RTC turned out to be overly tight. While it cures the problem on the reporters machine it breaks machines with Intel chipsets which use bit 0-5 of the D register. So check only for bit 6 being 0 which is the case on these Intel machines as well. Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") Reported-by: Serge Belyshev <belyshev@depni.sinp.msu.ru> Reported-by: Dirk Gouders <dirk@gouders.net> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- V2: Provide the actual delta patch. Should have stayed away from computers today.... --- drivers/rtc/rtc-cmos.c | 4 ++-- drivers/rtc/rtc-mc146818-lib.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)