linux-kernel - Re: [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure

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: <20200515145007.xjrx5mminxrh374d@mobilestation>
Date:   Fri, 15 May 2020 17:50:07 +0300
From:   Serge Semin <Sergey.Semin@...kalelectronics.ru>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     Serge Semin <fancer.lancer@...il.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Arnd Bergmann <arnd@...db.de>,
        Long Cheng <long.cheng@...iatek.com>,
        Maxime Ripard <mripard@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        <linux-mips@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        <linux-serial@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate
 setting procedure

On Fri, May 15, 2020 at 05:05:47PM +0300, Andy Shevchenko wrote:
> On Thu, May 07, 2020 at 02:31:34AM +0300, Serge Semin wrote:
> > Really instead of twice checking the clk_round_rate() return value
> > we could do it once, and if it isn't error the clock rate can be changed.
> > By doing so we decrease a number of ret-value tests and remove a weird
> > goto-based construction implemented in the dw8250_set_termios() method.
> 
> >  	rate = clk_round_rate(d->clk, baud * 16);
> > -	if (rate < 0)
> > -		ret = rate;
> 
> > -	else if (rate == 0)
> > -		ret = -ENOENT;
> 
> This case now handled differently.
> I don't think it's good idea to change semantics.
> 
> So, I don't see how this, after leaving the rate==0 case, would be better than
> original one.

Semantic doesn't change. The code does exactly the same as before. If it didn't
I either would have provided a comment about this or just didn't introduce the
change in the first place. I guess you just don't see the whole picture of the
method. Take a look in the code. The ret variable's been used to skip the
"p->uartclk = rate" assignment. That's it. So the (rate == 0) will still be
considered as error condition, which causes the clock rate left unchanged.
Here is the code diff so you wouldn't need to dive deep into the driver
sources:

<	clk_disable_unprepare(d->clk);
<	rate = clk_round_rate(d->clk, baud * 16);
<	if (rate < 0)
<		ret = rate;
<	else if (rate == 0)
<		ret = -ENOENT;
<	else
<		ret = clk_set_rate(d->clk, rate);
<	clk_prepare_enable(d->clk);
<
<	if (ret)
<		goto out;
<
<	p->uartclk = rate;
<
<out:
---
>       clk_disable_unprepare(d->clk);
>       rate = clk_round_rate(d->clk, baud * 16);
>       if (rate > 0) {
>              ret = clk_set_rate(d->clk, rate);
>              if (!ret)
>                      p->uartclk = rate;
>       }
>       clk_prepare_enable(d->clk);

-Sergey

> 
> > -	else
> > +	if (rate > 0) {
> >  		ret = clk_set_rate(d->clk, rate);
> > +		if (!ret)
> > +			p->uartclk = rate;
> > +	}
> >  	clk_prepare_enable(d->clk);
> >  
> > -	if (ret)
> > -		goto out;
> > -
> > -	p->uartclk = rate;
> > -
> > -out:
> >  	p->status &= ~UPSTAT_AUTOCTS;
> >  	if (termios->c_cflag & CRTSCTS)
> >  		p->status |= UPSTAT_AUTOCTS;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>