linux-kernel - Re: [PATCH] hid-ft260: add UART support.

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: <Y4xX7ILXMFHZtJkv@kroah.com>
Date:   Sun, 4 Dec 2022 09:18:52 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Daniel Beer <daniel.beer@...rinstitute.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        linux-i2c@...r.kernel.org,
        Michael Zaidman <michael.zaidman@...il.com>,
        Christina Quast <contact@...istina-quast.de>,
        linux-serial@...r.kernel.org, Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH] hid-ft260: add UART support.

On Sat, Oct 22, 2022 at 11:19:20AM +1300, Daniel Beer wrote:
> Based on an earlier patch submitted by Christina Quast:
> 
>     https://patches.linaro.org/project/linux-serial/patch/20220928192421.11908-1-contact@christina-quast.de/

Please link to lore.kernel.org, we have no idea what will happen over
time to other domains/links.

> Simplified and reworked to use the UART API rather than the TTY layer
> directly. Transmit, receive and baud rate changes are supported.

Why use the uart layer?  Did you just change how the existing driver
works?

> +struct ft260_input_report {
> +	u8 report;		/* FT260_I2C_REPORT or FT260_UART_REPORT */
>  	u8 length;		/* data payload length */
> -	u8 data[2];		/* data payload */
> +	u8 data[0];		/* data payload */

Please do not use [0], use [], people are working to replace all [0]
instances in the kernel.

> +struct ft260_configure_uart_request {
> +	u8 report;		/* FT260_SYSTEM_SETTINGS */
> +	u8 request;		/* FT260_SET_UART_CONFIG */
> +	u8 flow_ctrl;		/* 0: OFF, 1: RTS_CTS, 2: DTR_DSR */
> +				/* 3: XON_XOFF, 4: No flow ctrl */
> +	__le32 baudrate;	/* little endian, 9600 = 0x2580, 19200 = 0x4B00 */

The data structure in the device really looks like this?  Unaligned
accesses are odd.

> +static void ft260_uart_set_termios(struct uart_port *port,
> +				   struct ktermios *termios,
> +				   const struct ktermios *old_termios)
> +{
> +	struct ft260_device *dev = container_of(port, struct ft260_device, port);
> +	struct hid_device *hdev = dev->hdev;
> +	unsigned int baud;
> +	struct ft260_configure_uart_request req;
> +	int ret;
> +
> +	ft260_dbg("%s uart\n", __func__);

Please just use ftrace, no need for any of these "I am here!" lines.

Also dev_dbg() functions already have __func__ in them, no need to ever
add them again.

> --- a/include/uapi/linux/major.h
> +++ b/include/uapi/linux/major.h
> @@ -175,4 +175,6 @@
>  #define BLOCK_EXT_MAJOR		259
>  #define SCSI_OSD_MAJOR		260	/* open-osd's OSD scsi device */
>  
> +#define FT260_MAJOR		261

A whole new major for just a single tty port?  Please no, use dynamic
majors if you have to, or better yet, tie into the usb-serial
implementation (this is a USB device, right?) and then you don't have to
mess with this at all.

> +
>  #endif
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 3ba34d8378bd..d9a7025f467e 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -276,4 +276,7 @@
>  /* Sunplus UART */
>  #define PORT_SUNPLUS	123
>  
> +/* FT260 HID UART */
> +#define PORT_FT260	124

Why is this required?  What userspace code needs this new id?  I want to
remove all of these ids, not add new ones.

thanks,

greg k-h