linux-kernel - Re: [PATCH 1/2] rtc: m48t59: Accommodate chips that lack a century bit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0a0888fd-dc82-bc85-e1ad-6aebc9c450ff@linux-m68k.org>
Date: Fri, 4 Oct 2024 08:20:26 +1000 (AEST)
From: Finn Thain <fthain@...ux-m68k.org>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
cc: Alexandre Belloni <alexandre.belloni@...tlin.com>, 
    Daniel Palmer <daniel@...f.com>, Michael Pavone <pavone@...rodev.com>, 
    linux-m68k@...ts.linux-m68k.org, linux-rtc@...r.kernel.org, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] rtc: m48t59: Accommodate chips that lack a century
 bit


On Thu, 3 Oct 2024, Geert Uytterhoeven wrote:

> Thanks for your patch!
> 

Thanks for your review.

> > --- a/drivers/rtc/rtc-m48t59.c
> > +++ b/drivers/rtc/rtc-m48t59.c
> > @@ -57,6 +57,17 @@ m48t59_mem_readb(struct device *dev, u32 ofs)
> >         return readb(m48t59->ioaddr+ofs);
> >  }
> >
> > +/*
> > + * Sun SPARC machines count years since 1968. MVME machines running Linux
> > + * count years since 1970.
> > + */
> > +
> > +#ifdef CONFIG_SPARC
> > +#define YEAR0 68
> > +#else
>  +#define YEAR0 70
> > +#endif
> 
> This causes a change in behavior on other non-SPARC platforms,
> if any out-of-tree platform exists that uses this driver.
> 

I'm unaware of any need to support out-of-tree code. Do you see think such 
a requirement would be feasible somehow? Is this documented somewhere?

> So I'd rather use:
> 
>     #elif defined(CONFIG_VME)
>     #define YEAR0 70
>     #else
>     #define YEAR0 0
>     #endif
> 

That is a Y2K bug, right?

> > +
> >  /*
> >   * NOTE: M48T59 only uses BCD mode
> >   */
> > @@ -82,10 +93,7 @@ static int m48t59_rtc_read_time(struct device *dev, struct rtc_time *tm)
> >                 dev_dbg(dev, "Century bit is enabled\n");
> >                 tm->tm_year += 100;     /* one century */
> >         }
> > -#ifdef CONFIG_SPARC
> > -       /* Sun SPARC machines count years since 1968 */
> > -       tm->tm_year += 68;
> > -#endif
> > +       tm->tm_year += YEAR0;
> 
> Upon closer look, the driver uses platform data, so a better solution 
> would be to add the year0 offset to struct m48t59_plat_data.
> 

I agree.

> Another suggestion for improvement, not related to this patch, would be 
> to differentiate among M48T59, M48T02, and M48T08 by using 
> platform_driver.id_table and platform_device_id.driver_data, instead of 
> m48t59_plat_data.type.
> 

Yes, that's well out-of-scope I think.