linux-kernel - Re: [PATCH v2 18/20] mips: csrc-r4k: Decrease r4k-clocksource rating if CPU

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: <20200520122125.opn53q5wxvcc4ath@mobilestation>
Date:   Wed, 20 May 2020 15:21:25 +0300
From:   Serge Semin <Sergey.Semin@...kalelectronics.ru>
To:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>
CC:     Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>, <linux-pm@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        <linux-mips@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 18/20] mips: csrc-r4k: Decrease r4k-clocksource rating
 if CPU_FREQ enabled

On Wed, May 20, 2020 at 03:12:02PM +0300, Serge Semin wrote:
> On Tue, May 19, 2020 at 05:50:53PM +0200, Thomas Bogendoerfer wrote:
> > On Mon, May 18, 2020 at 11:57:52PM +0300, Serge Semin wrote:
> > > On Mon, May 18, 2020 at 06:32:06PM +0200, Thomas Bogendoerfer wrote:
> > > > On Mon, May 18, 2020 at 04:48:20PM +0300, Serge Semin wrote:

[nip]

> > > > > ├─>[PATCH v2 13/20] mips: early_printk_8250: Use offset-sized IO-mem accessors
> > > > 
> > > > that's broken. A reg shift of 2 doesn't mean we could use 32bit access
> > > > to the registers on other platforms. As I don't think adding some ifdefery
> > > > makes things nicer, just implement the your prom_putchar in board code.
> > > 
> > > I thought about that initially, but then I decided to alter the generic
> > > early_printk_8250 code instead. My version of prom_putchar() would be almost
> > > the same as one implemented in the early_printk_8250 module except minor
> > > modification of replacing readb/writeb methods with readl/writel. So I didn't
> > > want to duplicate the code, but wanted to provide a general way to fix the
> > > problem potentially also for another platforms.
> > > 
> > > Since you don't like this fix alternatively I'd suggest to add the reg_width
> > > parameter passed to the setup_8250_early_printk_port() method like this:
> > > -setup_8250_early_printk_port(unsigned long base, unsigned int reg_shift,
> > > -                             unsigned int timeout)
> > > +setup_8250_early_printk_port(unsigned long base, unsigned int reg_shift,
> > > +                             unsigned int reg_width, unsigned int timeout)
> > > 
> > > By reg_width parameter we could determine the actual width of the register:
> > >  static inline u8 serial_in(int offset)
> > >  {
> > > -       return readb(serial8250_base + (offset << serial8250_reg_shift));
> > > +       u8 ret = 0xFF;
> > > +
> > > +       offset <<= serial8250_reg_shift;
> > > +       switch (serial8250_reg_width) {
> > > +       case 1:
> > > +               ret = readb(serial8250_base + offset);
> > > +               break;
> > > +       case 2:
> > > +               ret = readw(serial8250_base + offset);
> > > +               break;
> > > +       case 4:
> > > +               ret = readl(serial8250_base + offset);
> > > +               break;
> > > +       default:
> > > +               break;
> > > +       }
> > > +
> > > +       return ret;
> > >  }
> > > 
> > > The similar modification will be implemented for serial_out(). I'll also modify
> > 
> > look at the lines of code you are adding. Doing your own prom_putchar will
> > probably have less lines.
> > 
> > > What do you think about this?
> > 
> > please do your own prom_putchar.
> 
> One more time regarding this problem but in appliance to another part of the
> MIPS code. I've missed the patch to draw your attention to:
> [PATCH v2 14/20] mips: Use offset-sized IO-mem accessors in CPS debug printout
> 
> There I've applied the same fix as in the patch:
> [PATCH v2 13/20] mips: early_printk_8250: Use offset-sized IO-mem accessors
> 
> Since you don't like the way I initially fixed it, suppose there we don't have
> another way but to introduce something like CONFIG_MIPS_CPS_NS16550_WIDTH
> parameter to select a proper accessors, like sw in our case, and sb by defaul).
> Right?
> 

> (Note UART_L is incorrectly created in that patch, I'll remove that macro in
> v3.)

Hm, actually no. The macro is correct. According to the code _mips_cps_putc()
always perform lw from the UART MMIO registers. This must be a bug. Don't you
think?

-Sergey

> 
> -Sergey
>