| Message ID | 1529006926-11401-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 14, 2018 at 10:08:46PM +0200, Loic Poulain wrote: > Most of FTDI's devices have an EEPROM which records FTDI devices > configuration setting (e.g. the VID, PID, I/O config...) and user > data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte > eeprom... > > This patch adds support for FTDI EEPROM read/write via USB control > transfers and register a new nvm device to the nvmem core. > > This permits to expose the eeprom as a sysfs file, allowing userspace > to read/modify FTDI configuration and its user data without having to > rely on a specific userspace USB driver. > > Moreover, any upcoming new tentative to add CBUS GPIO support could > integrate CBUS EEPROM configuration reading in order to determine > which of the CBUS pins are available as GPIO. I'm not necessarily against the idea, but nvmem core needs to be fixed so that it can handle hotplugging before this can be considered for merging. Right now it just returns -EBUSY from nvmem_unregister(), which results in all kinds of memory leaks, use-after-frees and crashes when user space holds the character device open while the device is being unplugged. > +static void unregister_eeprom(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + > + if (priv->eeprom) > + nvmem_unregister(priv->eeprom); > +} > @@ -1931,6 +2036,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) > { > struct ftdi_private *priv = usb_get_serial_port_data(port); > > +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) > + unregister_eeprom(port); > +#endif > remove_sysfs_attrs(port); > > kfree(priv); Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/06/18 09:46, Johan Hovold wrote: > On Thu, Jun 14, 2018 at 10:08:46PM +0200, Loic Poulain wrote: >> Most of FTDI's devices have an EEPROM which records FTDI devices >> configuration setting (e.g. the VID, PID, I/O config...) and user >> data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte >> eeprom... >> >> This patch adds support for FTDI EEPROM read/write via USB control >> transfers and register a new nvm device to the nvmem core. >> >> This permits to expose the eeprom as a sysfs file, allowing userspace >> to read/modify FTDI configuration and its user data without having to >> rely on a specific userspace USB driver. >> >> Moreover, any upcoming new tentative to add CBUS GPIO support could >> integrate CBUS EEPROM configuration reading in order to determine >> which of the CBUS pins are available as GPIO. > > I'm not necessarily against the idea, but nvmem core needs to be fixed > so that it can handle hotplugging before this can be considered for > merging. > > Right now it just returns -EBUSY from nvmem_unregister(), Yes, this is the behavior if there are active users/references for the nvmem provider. As this use case seems to have come up more than once. I will take a closer look at making nvmem_unregister() return void, but with a BIG warn when there are active users. This is already done in devm_nvmem_register/unregister apis. I can also suggest you to try devm_nvmem_register(). thanks, srini which results > in all kinds of memory leaks, use-after-frees and crashes when user > space holds the character device open while the device is being > unplugged. > >> +static void unregister_eeprom(struct usb_serial_port *port) >> +{ >> + struct ftdi_private *priv = usb_get_serial_port_data(port); >> + >> + if (priv->eeprom) >> + nvmem_unregister(priv->eeprom); >> +} > >> @@ -1931,6 +2036,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) >> { >> struct ftdi_private *priv = usb_get_serial_port_data(port); >> >> +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) >> + unregister_eeprom(port); >> +#endif >> remove_sysfs_attrs(port); >> >> kfree(priv); > > Thanks, > Johan > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johan, Srini, On 18 June 2018 at 11:47, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > On 18/06/18 09:46, Johan Hovold wrote: >> >> On Thu, Jun 14, 2018 at 10:08:46PM +0200, Loic Poulain wrote: >>> >>> Most of FTDI's devices have an EEPROM which records FTDI devices >>> configuration setting (e.g. the VID, PID, I/O config...) and user >>> data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte >>> eeprom... >>> >>> This patch adds support for FTDI EEPROM read/write via USB control >>> transfers and register a new nvm device to the nvmem core. >>> >>> This permits to expose the eeprom as a sysfs file, allowing userspace >>> to read/modify FTDI configuration and its user data without having to >>> rely on a specific userspace USB driver. >>> >>> Moreover, any upcoming new tentative to add CBUS GPIO support could >>> integrate CBUS EEPROM configuration reading in order to determine >>> which of the CBUS pins are available as GPIO. >> >> >> I'm not necessarily against the idea, but nvmem core needs to be fixed >> so that it can handle hotplugging before this can be considered for >> merging. >> >> Right now it just returns -EBUSY from nvmem_unregister(), which results >> in all kinds of memory leaks, use-after-frees and crashes when user >> space holds the character device open while the device is being >> unplugged. Correct me if I'm wrong, but nvmem is just exposed to userspace as a simple sysfs device attribute (nvmem), removing a device and its attribute(s) dynamically is well managed by sysfs, even if userspace has file open. The only risk here is when a kernel internal consumer still has a reference to the nvmem device on removal, which is not the case in our context. > I can also suggest you to try devm_nvmem_register(). Yes, I'm going to send a v2. Regards, Loic -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 19, 2018 at 02:32:11PM +0200, Loic Poulain wrote: > Hi Johan, Srini, > > On 18 June 2018 at 11:47, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: > > On 18/06/18 09:46, Johan Hovold wrote: > >> I'm not necessarily against the idea, but nvmem core needs to be fixed > >> so that it can handle hotplugging before this can be considered for > >> merging. > >> > >> Right now it just returns -EBUSY from nvmem_unregister(), which results > >> in all kinds of memory leaks, use-after-frees and crashes when user > >> space holds the character device open while the device is being > >> unplugged. > > Correct me if I'm wrong, but nvmem is just exposed to userspace as a > simple sysfs device attribute (nvmem), removing a device and its attribute(s) > dynamically is well managed by sysfs, even if userspace has file open. My bad, I misremembered what interface nvmem was using and only saw the deregistration refusal in nvmem_unregister() during a quick check. > The only risk here is when a kernel internal consumer still has a reference to > the nvmem device on removal, which is not the case in our context. Right. > > I can also suggest you to try devm_nvmem_register(). > > Yes, I'm going to send a v2. I'll take a closer look soonish too. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johan, Srini, On 19 June 2018 at 15:06, Johan Hovold <johan@kernel.org> wrote: > On Tue, Jun 19, 2018 at 02:32:11PM +0200, Loic Poulain wrote: >> Hi Johan, Srini, >> >> On 18 June 2018 at 11:47, Srinivas Kandagatla >> <srinivas.kandagatla@linaro.org> wrote: >> > On 18/06/18 09:46, Johan Hovold wrote: > >> >> I'm not necessarily against the idea, but nvmem core needs to be fixed >> >> so that it can handle hotplugging before this can be considered for >> >> merging. >> >> >> >> Right now it just returns -EBUSY from nvmem_unregister(), which results >> >> in all kinds of memory leaks, use-after-frees and crashes when user >> >> space holds the character device open while the device is being >> >> unplugged. >> >> Correct me if I'm wrong, but nvmem is just exposed to userspace as a >> simple sysfs device attribute (nvmem), removing a device and its attribute(s) >> dynamically is well managed by sysfs, even if userspace has file open. > > My bad, I misremembered what interface nvmem was using and only saw the > deregistration refusal in nvmem_unregister() during a quick check. > >> The only risk here is when a kernel internal consumer still has a reference to >> the nvmem device on removal, which is not the case in our context. > > Right. > >> > I can also suggest you to try devm_nvmem_register(). >> >> Yes, I'm going to send a v2. > > I'll take a closer look soonish too. I changed my mind here, indeed, in order to avoid any potential use-after-free, we have to manually unregister/release the nvmem device before freeing of the private port structure (used as nvmem context). So I will not send a V2 with nvmem devm usage. You can consider this V1 for review. Regards, Loic -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Overall nvmem side looks good! Minor nits below. On 14/06/18 21:08, Loic Poulain wrote: > Most of FTDI's devices have an EEPROM which records FTDI devices > configuration setting (e.g. the VID, PID, I/O config...) and user > data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte > eeprom... > > This patch adds support for FTDI EEPROM read/write via USB control > transfers and register a new nvm device to the nvmem core. > > This permits to expose the eeprom as a sysfs file, allowing userspace > to read/modify FTDI configuration and its user data without having to > rely on a specific userspace USB driver. > > Moreover, any upcoming new tentative to add CBUS GPIO support could > integrate CBUS EEPROM configuration reading in order to determine > which of the CBUS pins are available as GPIO. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/usb/serial/Kconfig | 11 +++++ > drivers/usb/serial/ftdi_sio.c | 108 ++++++++++++++++++++++++++++++++++++++++++ > drivers/usb/serial/ftdi_sio.h | 28 +++++++++++ > 3 files changed, 147 insertions(+) > > diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig > index 533f127..2dd2f5d 100644 > --- a/drivers/usb/serial/Kconfig > +++ b/drivers/usb/serial/Kconfig > @@ -181,6 +181,17 @@ config USB_SERIAL_FTDI_SIO > To compile this driver as a module, choose M here: the > module will be called ftdi_sio. > > +config USB_SERIAL_FTDI_SIO_NVMEM > + bool "FTDI MTP non-volatile memory support" > + depends on USB_SERIAL_FTDI_SIO Looks inconsistent tab and spaces here. > + select NVMEM > + default y ?? Does that mean all the FTDIs have EEPROM? > + help > + Say yes here to add support for the MTP non-volatile memory > + present on FTDI. Most of FTDI's devices have an EEPROM which > + records FTDI device's configuration setting as well as user > + data. > + > config USB_SERIAL_VISOR > tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver" > help > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c > index 7ea221d..03c9c75 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -40,6 +40,7 @@ > #include <linux/usb.h> > #include <linux/serial.h> > #include <linux/usb/serial.h> > +#include <linux/nvmem-provider.h> > #include "ftdi_sio.h" > #include "ftdi_sio_ids.h" > > @@ -73,6 +74,8 @@ 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() */ > + > + struct nvmem_device *eeprom; > }; > > /* struct ftdi_sio_quirk is used by devices requiring special attention. */ > @@ -1529,6 +1532,104 @@ static int get_lsr_info(struct usb_serial_port *port, > return 0; > } > > +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) I think only ifdef should be Okay here as this Kconfig is just bool > + ... > +static int register_eeprom(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + struct nvmem_config nvmconf = {}; > + > + switch (priv->chip_type) { > + case FTX: > + nvmconf.size = 2048; > + break; > + case FT232RL: > + nvmconf.size = 128; > + break; > + default: > + return 0; > + } > + > + nvmconf.word_size = 2; > + nvmconf.stride = 2; > + nvmconf.read_only = false; > + nvmconf.priv = port; > + nvmconf.dev = &port->dev; > + nvmconf.reg_read = read_eeprom; > + nvmconf.reg_write = write_eeprom; > + nvmconf.owner = THIS_MODULE; > + > + priv->eeprom = nvmem_register(&nvmconf); > + if (IS_ERR(priv->eeprom)) { > + priv->eeprom = NULL; > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static void unregister_eeprom(struct usb_serial_port *port) > +{ > + struct ftdi_private *priv = usb_get_serial_port_data(port); > + > + if (priv->eeprom) > + nvmem_unregister(priv->eeprom); > +} > + > +#endif /* CONFIG_USB_SERIAL_FTDI_SIO_NVMEM */ > > /* Determine type of FTDI chip based on USB config and descriptor. */ > static void ftdi_determine_type(struct usb_serial_port *port) > @@ -1814,6 +1915,10 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) > priv->latency = 16; > write_latency_timer(port); > create_sysfs_attrs(port); > +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) > + register_eeprom(port); Users will be clueless if the register_eeprom fails here. > +#endif > + > return 0; > } > > @@ -1931,6 +2036,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) > { > struct ftdi_private *priv = usb_get_serial_port_data(port); > > +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) > + unregister_eeprom(port); > +#endif > remove_sysfs_attrs(port); > > kfree(priv); thanks, srini -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig index 533f127..2dd2f5d 100644 --- a/drivers/usb/serial/Kconfig +++ b/drivers/usb/serial/Kconfig @@ -181,6 +181,17 @@ config USB_SERIAL_FTDI_SIO To compile this driver as a module, choose M here: the module will be called ftdi_sio. +config USB_SERIAL_FTDI_SIO_NVMEM + bool "FTDI MTP non-volatile memory support" + depends on USB_SERIAL_FTDI_SIO + select NVMEM + default y + help + Say yes here to add support for the MTP non-volatile memory + present on FTDI. Most of FTDI's devices have an EEPROM which + records FTDI device's configuration setting as well as user + data. + config USB_SERIAL_VISOR tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver" help diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index 7ea221d..03c9c75 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -40,6 +40,7 @@ #include <linux/usb.h> #include <linux/serial.h> #include <linux/usb/serial.h> +#include <linux/nvmem-provider.h> #include "ftdi_sio.h" #include "ftdi_sio_ids.h" @@ -73,6 +74,8 @@ 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() */ + + struct nvmem_device *eeprom; }; /* struct ftdi_sio_quirk is used by devices requiring special attention. */ @@ -1529,6 +1532,104 @@ static int get_lsr_info(struct usb_serial_port *port, return 0; } +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) + +static int write_eeprom(void *priv, unsigned int off, void *_val, size_t bytes) +{ + struct usb_serial_port *port = priv; + struct usb_serial *serial = port->serial; + struct usb_device *udev = serial->dev; + unsigned char *buf = _val; + + while (bytes) { + uint16_t val; + int rv; + + val = buf[0] + (buf[1] << 8); + + rv = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), + FTDI_SIO_WRITE_EEPROM_REQUEST, + FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE, + val, off / 2, NULL, 0, WDR_TIMEOUT); + if (rv < 0) + return rv; + + off += 2; + buf += 2; + bytes -= 2; + } + + return 0; +} + +static int read_eeprom(void *priv, unsigned int off, void *val, size_t bytes) +{ + struct usb_serial_port *port = priv; + struct usb_serial *serial = port->serial; + struct usb_device *udev = serial->dev; + unsigned char *buf = val; + + while (bytes) { + int rv; + + rv = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), + FTDI_SIO_READ_EEPROM_REQUEST, + FTDI_SIO_READ_EEPROM_REQUEST_TYPE, + 0, off / 2, buf, 2, WDR_TIMEOUT); + if (rv < 0) + return rv; + + off += 2; + buf += 2; + bytes -= 2; + } + + return 0; +} + +static int register_eeprom(struct usb_serial_port *port) +{ + struct ftdi_private *priv = usb_get_serial_port_data(port); + struct nvmem_config nvmconf = {}; + + switch (priv->chip_type) { + case FTX: + nvmconf.size = 2048; + break; + case FT232RL: + nvmconf.size = 128; + break; + default: + return 0; + } + + nvmconf.word_size = 2; + nvmconf.stride = 2; + nvmconf.read_only = false; + nvmconf.priv = port; + nvmconf.dev = &port->dev; + nvmconf.reg_read = read_eeprom; + nvmconf.reg_write = write_eeprom; + nvmconf.owner = THIS_MODULE; + + priv->eeprom = nvmem_register(&nvmconf); + if (IS_ERR(priv->eeprom)) { + priv->eeprom = NULL; + return -ENOMEM; + } + + return 0; +} + +static void unregister_eeprom(struct usb_serial_port *port) +{ + struct ftdi_private *priv = usb_get_serial_port_data(port); + + if (priv->eeprom) + nvmem_unregister(priv->eeprom); +} + +#endif /* CONFIG_USB_SERIAL_FTDI_SIO_NVMEM */ /* Determine type of FTDI chip based on USB config and descriptor. */ static void ftdi_determine_type(struct usb_serial_port *port) @@ -1814,6 +1915,10 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port) priv->latency = 16; write_latency_timer(port); create_sysfs_attrs(port); +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) + register_eeprom(port); +#endif + return 0; } @@ -1931,6 +2036,9 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port) { struct ftdi_private *priv = usb_get_serial_port_data(port); +#if IS_ENABLED(CONFIG_USB_SERIAL_FTDI_SIO_NVMEM) + unregister_eeprom(port); +#endif remove_sysfs_attrs(port); kfree(priv); diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h index dcd0b6e..9eab961 100644 --- a/drivers/usb/serial/ftdi_sio.h +++ b/drivers/usb/serial/ftdi_sio.h @@ -36,6 +36,8 @@ #define FTDI_SIO_SET_ERROR_CHAR 7 /* Set the error character */ #define FTDI_SIO_SET_LATENCY_TIMER 9 /* Set the latency timer */ #define FTDI_SIO_GET_LATENCY_TIMER 10 /* Get the latency timer */ +#define FTDI_SIO_READ_EEPROM 0x90 /* Read eeprom */ +#define FTDI_SIO_WRITE_EEPROM 0x91 /* Write eeprom */ /* Interface indices for FT2232, FT2232H and FT4232H devices */ #define INTERFACE_A 1 @@ -400,6 +402,32 @@ enum ftdi_sio_baudrate { * */ + /* FTDI_SIO_READ_EEPROM */ +#define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0 +#define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM +/* + * BmRequestType: 1100 0000b + * bRequest: FTDI_SIO_READ_EEPROM + * wValue: 0 + * wIndex: Word Index + * wLength: 2 + * Data: return data (a word) + * + */ + +/* FTDI_SIO_WRITE_EEPROM */ +#define FTDI_SIO_WRITE_EEPROM_REQUEST_TYPE 0x40 +#define FTDI_SIO_WRITE_EEPROM_REQUEST FTDI_SIO_WRITE_EEPROM +/* + * BmRequestType: 0100 0000b + * bRequest: FTDI_SIO_WRITE_EEPROM + * wValue: Data (word) + * wIndex: Word Index + * wLength: 0 + * Data: None + * + */ + /* FTDI_SIO_GET_MODEM_STATUS */ /* Retrieve the current value of the modem status register */
Most of FTDI's devices have an EEPROM which records FTDI devices configuration setting (e.g. the VID, PID, I/O config...) and user data. FT230R chip integrates a 128-byte eeprom, FT230X a 2048-byte eeprom... This patch adds support for FTDI EEPROM read/write via USB control transfers and register a new nvm device to the nvmem core. This permits to expose the eeprom as a sysfs file, allowing userspace to read/modify FTDI configuration and its user data without having to rely on a specific userspace USB driver. Moreover, any upcoming new tentative to add CBUS GPIO support could integrate CBUS EEPROM configuration reading in order to determine which of the CBUS pins are available as GPIO. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/usb/serial/Kconfig | 11 +++++ drivers/usb/serial/ftdi_sio.c | 108 ++++++++++++++++++++++++++++++++++++++++++ drivers/usb/serial/ftdi_sio.h | 28 +++++++++++ 3 files changed, 147 insertions(+)