linux-kernel - Re: [PATCH] tty: serial: samsung: Correct clock selection logic

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: <20200511100836.GA16828@kozik-lap>
Date:   Mon, 11 May 2020 12:08:36 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Jonathan Bakker <xc-racer2@...e.ca>
Cc:     kgene@...nel.org, gregkh@...uxfoundation.org, jslaby@...e.com,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: serial: samsung: Correct clock selection logic

On Fri, May 08, 2020 at 06:34:33PM -0700, Jonathan Bakker wrote:
> Some variants of the samsung tty driver can pick which clock
> to use for their baud rate generation.  In the DT conversion,
> a default clock was selected to be used if a specific one wasn't
> assigned and then a comparison of which clock rate worked better
> was done.  Unfortunately, the comparison was implemented in such
> a way that only the default clock was ever actually compared.
> Fix this by iterating through all possible clocks, except when a
> specific clock has already been picked via clk_sel (which is
> only possible via board files).
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@...e.ca>
> ---
>  drivers/tty/serial/samsung_tty.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..9d2b4be44209 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1281,14 +1281,14 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
>  	struct s3c24xx_uart_info *info = ourport->info;
>  	struct clk *clk;
>  	unsigned long rate;
> -	unsigned int cnt, baud, quot, clk_sel, best_quot = 0;
> +	unsigned int cnt, baud, quot, best_quot = 0;
>  	char clkname[MAX_CLK_NAME_LENGTH];
>  	int calc_deviation, deviation = (1 << 30) - 1;
>  
> -	clk_sel = (ourport->cfg->clk_sel) ? ourport->cfg->clk_sel :
> -			ourport->info->def_clk_sel;
>  	for (cnt = 0; cnt < info->num_clks; cnt++) {
> -		if (!(clk_sel & (1 << cnt)))
> +		/* Keep selected clock if provided */

Makes sense and good catch.

Reviewed-by: Krzysztof Kozlowski <krzk@...nel.org>

I wonder about the s3c24xx_serial_enable_baudclk() which has similar
pattern - is there
testing only def_clk_sel on purpose?

Best regards,
Krzysztof

> +		if (ourport->cfg->clk_sel &&
> +			!(ourport->cfg->clk_sel & (1 << cnt)))
>  			continue;
>  
>  		sprintf(clkname, "clk_uart_baud%d", cnt);
> -- 
> 2.20.1
>