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]

Date:   Fri, 15 May 2020 18:05:42 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>
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:50:07PM +0300, Serge Semin wrote:
> 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);

Thanks.
Indeed, in the above it looks clear.



-- 
With Best Regards,
Andy Shevchenko