linux-kernel - Re: [PATCH v3] USB: serial: ftdi_sio: implement GPIO support for FT-X 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]

Date:   Fri, 14 Sep 2018 16:54:34 +0000
From:   "Karoly Pados" <pados@...os.hu>
To:     "Johan Hovold" <johan@...nel.org>
Cc:     "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Loic Poulain" <loic.poulain@...aro.org>
Subject: Re: [PATCH v3] USB: serial: ftdi_sio: implement GPIO support for
 FT-X devices

Hi,

Thanks again for the review.

>> #include <linux/usb/serial.h>
>> +#if defined(CONFIG_GPIOLIB)
>> +#include <linux/gpio/driver.h>
>> +#endif
> 
> Hmm. I already commented on this in v1.

Yeah, and I changed it too, but I now realized I misunderstood your intentions.
You want me to remove the conditional compilation completely, while I thought
you just prefer the #if defined() style instead of #ifdef. A misunderstanding
on my part.

> 
>> #include "ftdi_sio.h"
>> #include "ftdi_sio_ids.h"
>> 
>> @@ -72,6 +75,14 @@ struct ftdi_private {
>> unsigned int latency; /* latency setting in use */
>> unsigned short max_packet_size;
>> struct mutex cfg_lock; /* Avoid mess by parallel calls of config ioctl() and change_speed() */
>> +#if defined(CONFIG_GPIOLIB)
>> + struct gpio_chip gc;
>> + bool gpio_registered; /* is the gpiochip in kernel registered */
>> + bool gpio_used; /* true if the user requested a gpio */
>> + u8 gpio_altfunc; /* which pins are in gpio mode */
>> + u8 gpio_input; /* pin directions cache */
> 
> And I asked you to invert this one (i.e. replace with gpio_output).

Later when we discussed and you replied to my comments, I interpreted your response
it can stay this way. Another misunderstanding, sorry.

>> +/* Returns the number of bytes read */
>> +static int ftdi_read_eeprom(struct usb_serial *serial,
>> + void *dst, /* must be kmalloc'd using GFP_KERNEL*/
> 
> Whether GFP_KERNEL was used is not really relevant, but highlighting
> that the buffer needs to be DMA-able is good.
> 
>> + u16 addr, /* must be aligned to 16 bits */
>> + u16 nbytes) /* must be a multiple of 16 bits */
> 
> I think checkpatch gets confused by you odd argument comments here. Use
> a kernel doc comment, if you want to be this specific instead.

I'll just remove the comments on the parameters of this function. The 
sb_control_msg is a dead giveaway the buffer must be DMA-able, and the
error checks at the start make the requirements on the other parameters also
obvious.

>> +
>> + /* Chip-type guessing logic based on libftdi. */
>> + priv->gc.ngpio = 4; /* FT230X, FT231X */
>> + if (le16_to_cpu(serial->dev->descriptor.bcdDevice) != 0x1000)
>> + priv->gc.ngpio = 1; /* FT234XD */
> 
> No known way to identify FT234XD here?
> 
> After taking a quick peek at libftdi, it seems we really have no clue
> how to detect these device types and 0x1000 could be for all FTX
> devices. Heck, the current kernel driver just assumes anything we don't
> recognise to be an FTX, something which would now hit this code path...
> 
> What devices did you and Loic have? Could you post the lsusb -v output
> for these? Perhaps someone with an FT234XD can chime in as well.
> 

No clue about this one. I only own FT230X and FT231X devices, but it looks
like they cannot be told apart, except for eeprom strings which are reconfigurable
by the user. I wouldn't rely on such things. Anyway, lsusb -v outputs attached.

Karoly

Download attachment "ft230x" of type "application/octet-stream" (2227 bytes)

Download attachment "ft231x" of type "application/octet-stream" (2205 bytes)