linux-kernel - Re: [PATCH v2 1/3] usb: misc: add driver for FT232H based FPGA configuration devices

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: <1542675372.30311.573.camel@impinj.com>
Date:   Tue, 20 Nov 2018 00:56:13 +0000
From:   Trent Piepho <tpiepho@...inj.com>
To:     "agust@...x.de" <agust@...x.de>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "atull@...nel.org" <atull@...nel.org>,
        "mdf@...nel.org" <mdf@...nel.org>
Subject: Re: [PATCH v2 1/3] usb: misc: add driver for FT232H based FPGA
 configuration devices

On Tue, 2018-11-20 at 01:28 +0100, Anatolij Gustschin wrote:
> Add USB interface driver for ARRI FPGA configuration devices based on
> FTDI FT232H chip. Depending on USB PID the driver registers different
> platform devices describing an FPGA configuration interface.

Is ARRI different than Arria?

> +/* Use baudrate calculation borrowed from libftdi */
> +static int ftdi_to_clkbits(int baudrate, unsigned int clk, int clk_div,

Linux uses unsigned values for clocks.  Does it make any sense to mix
the unsigned clk with signed values?  Seems like baudrate and clk_div
should also be unsigned.

> +			   unsigned long *encoded_divisor)

unsigned long is an odd choice here.  Is there any to reason to use an
unsigned long to store the result of right shifting a signed int
(best_div)?  It can't be longer than a int, but it can be negative.

> +{
> +	static const char frac_code[8] = { 0, 3, 2, 4, 1, 5, 6, 7 };
> +	int best_baud = 0;
> +	int div, best_div;
> +
> +	if (baudrate >= clk / clk_div) {
> +		*encoded_divisor = 0;
> +		best_baud = clk / clk_div;
> +	} else if (baudrate >= clk / (clk_div + clk_div / 2)) {
> +		*encoded_divisor = 1;
> +		best_baud = clk / (clk_div + clk_div / 2);
> +	} else if (baudrate >= clk / (2 * clk_div)) {
> +		*encoded_divisor = 2;
> +		best_baud = clk / (2 * clk_div);
> +	} else {
> +		/*
> +		 * Divide by 16 to have 3 fractional bits and
> +		 * one bit for rounding
> +		 */
> +		div = clk * 16 / clk_div / baudrate;
> 
> +		if (div & 1)	/* Decide if to round up or down */
> +			best_div = div / 2 + 1;
> +		else
> +			best_div = div / 2;

In Linux we would write:

best_div = DIV_ROUND_UP(div, 2);

Though I think you can combine that with the above to get:

best_div = DIV_ROUND_CLOSEST(clk * 8 / clk_div, baudrate);

That what the above is trying to accomplish in a round about way

> +		if (best_div > 0x20000)
> +			best_div = 0x1ffff;

Looks like the above was probably supposed to be >=

> +		best_baud = clk * 16 / clk_div / best_div;
> +		if (best_baud & 1)	/* Decide if to round up or down */
> +			best_baud = best_baud / 2 + 1;
> +		else
> +			best_baud = best_baud / 2;

Again, looks like a complicated way to round to the nearest.

> +		*encoded_divisor = (best_div >> 3) |
> +				   (frac_code[best_div & 0x7] << 14);
> +	}
> +	return best_baud;
>