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: <20221204091247.GA11195@nyquist.nev>
Date:   Sun, 4 Dec 2022 22:12:47 +1300
From:   Daniel Beer <daniel.beer@...rinstitute.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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 Sun, Dec 04, 2022 at 09:18:52AM +0100, Greg Kroah-Hartman wrote:
> 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?

Hi Greg,

Thanks for reviewing. This device is quite strange -- it presents itself
as a USB HID, but it provides both an I2C master and a UART. The
existing driver supports only the I2C functionality currently.

> > +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.

Yes, that really is the data structure. Is there a better way to do
this?

> > --- 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.

As far as I understand it, I don't think usb-serial is usable, due to
the fact that this is already an HID driver.

I'll change to use dynamic majors, unless there's a better option.

> > +
> >  #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.

It probably isn't. I'd taken another driver as an example when
implementing this, and that's what it did. Should I instead set the port
field to PORT_UNKNOWN and return NULL from uart_type?

Cheers,
Daniel

-- 
Daniel Beer
Firmware Engineer at Igor Institute
daniel.beer@...rinstitute.com or +64-27-420-8101
Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312