[PATCH 0/4] Show CP210x part number in sysfs

[PATCH 0/4] Show CP210x part number in sysfs

[Petr Tesarik]

The cp210x driver can be used for several devices (CP2101/2/3/4). It
is sometimes useful to know the actual part number, because there are
slight differences in their capabilities.

The first two patches are cleanups and not necessary to implement the
feature. I can send them in a separate patch set if that's preferred.

Petr Tesarik (4):
  cp210x: Replace USB magic numbers with symbolic names
  cp210x: Unify code for set/get config control messages
  cp210x: Store part number
  cp210x: Expose the part number in sysfs

 drivers/usb/serial/cp210x.c | 164 ++++++++++++++++++++++++++------------------
 1 file changed, 99 insertions(+), 65 deletions(-)

-- 
2.1.4

[PATCH 1/4] cp210x: Replace USB magic numbers with symbolic names

[Petr Tesarik]

From: Petr Tesarik <ptesarik@suse.cz>

The request type is in fact made of three fields that already have
symbolic constants.

While I was rewriting those lines, I also converted the pre-processor
defines into an enum, so they are seen by debuggers.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 drivers/usb/serial/cp210x.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eac7cca..1bae015 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -226,10 +226,16 @@ static struct usb_serial_driver * const serial_drivers[] = {
 };
 
 /* Config request types */
-#define REQTYPE_HOST_TO_INTERFACE	0x41
-#define REQTYPE_INTERFACE_TO_HOST	0xc1
-#define REQTYPE_HOST_TO_DEVICE	0x40
-#define REQTYPE_DEVICE_TO_HOST	0xc0
+enum cp210x_request_type {
+	REQTYPE_HOST_TO_INTERFACE =
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+	REQTYPE_INTERFACE_TO_HOST =
+		USB_DIR_IN  | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+	REQTYPE_HOST_TO_DEVICE =
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+	REQTYPE_DEVICE_TO_HOST =
+		USB_DIR_IN  | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+};
 
 /* Config request codes */
 #define CP210X_IFC_ENABLE	0x00
-- 
2.1.4

[PATCH 2/4] cp210x: Unify code for set/get config control messages

[Petr Tesarik]

From: Petr Tesarik <ptesarik@suse.cz>

There is a lot of overlap between the two functions (e.g. calculation
of the buffer size), so this removes a bit of code duplication, but
most importantly, a more generic function can be easily reused for
other message types.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 drivers/usb/serial/cp210x.c | 109 ++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 60 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 1bae015..69f03b6 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -307,14 +307,17 @@ enum cp210x_request_type {
 #define CONTROL_WRITE_RTS	0x0200
 
 /*
- * cp210x_get_config
- * Reads from the CP210x configuration registers
+ * cp210x_control_msg
+ * Sends a generic control message, taking care of endianness
+ * and error messages.
  * 'size' is specified in bytes.
- * 'data' is a pointer to a pre-allocated array of integers large
- * enough to hold 'size' bytes (with 4 bytes to each integer)
+ * 'data' is a pointer to the input/output buffer. For output, it holds
+ * the data (in host order) to be sent. For input, it receives data from
+ * the device and must be big enough to hold 'size' bytes.
  */
-static int cp210x_get_config(struct usb_serial_port *port, u8 request,
-		unsigned int *data, int size)
+static int cp210x_control_msg(struct usb_serial_port *port, u8 request,
+			      u8 requesttype, u16 value, u32 *data, int size,
+			      int timeout)
 {
 	struct usb_serial *serial = port->serial;
 	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
@@ -328,20 +331,22 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
 	if (!buf)
 		return -ENOMEM;
 
-	/* Issue the request, attempting to read 'size' bytes */
-	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-				request, REQTYPE_INTERFACE_TO_HOST, 0x0000,
-				spriv->bInterfaceNumber, buf, size,
-				USB_CTRL_GET_TIMEOUT);
+	if (!(requesttype & USB_DIR_IN)) {
+		for (i = 0; i < length; i++)
+			buf[i] = cpu_to_le32(data[i]);
+	}
 
-	/* Convert data into an array of integers */
-	for (i = 0; i < length; i++)
-		data[i] = le32_to_cpu(buf[i]);
+	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+				 request, requesttype, value,
+				 spriv->bInterfaceNumber, buf, size, timeout);
 
-	kfree(buf);
+	if (requesttype & USB_DIR_IN) {
+		for (i = 0; i < length; i++)
+			data[i] = le32_to_cpu(buf[i]);
+	}
 
 	if (result != size) {
-		dev_dbg(&port->dev, "%s - Unable to send config request, request=0x%x size=%d result=%d\n",
+		dev_dbg(&port->dev, "%s - Unable to send request, request=0x%x size=%d result=%d\n",
 			__func__, request, size, result);
 		if (result > 0)
 			result = -EPROTO;
@@ -349,7 +354,23 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
 		return result;
 	}
 
-	return 0;
+	kfree(buf);
+
+	return result;
+}
+
+/*
+ * cp210x_get_config
+ * Reads from the CP210x configuration registers
+ * 'size' is specified in bytes.
+ * 'data' is a pointer to a pre-allocated array of integers large
+ * enough to hold 'size' bytes (with 4 bytes to each integer)
+ */
+static int cp210x_get_config(struct usb_serial_port *port, u8 request,
+		unsigned int *data, int size)
+{
+	return cp210x_control_msg(port, request, REQTYPE_INTERFACE_TO_HOST,
+				0x0000, data, size, USB_CTRL_GET_TIMEOUT);
 }
 
 /*
@@ -361,48 +382,14 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
 static int cp210x_set_config(struct usb_serial_port *port, u8 request,
 		unsigned int *data, int size)
 {
-	struct usb_serial *serial = port->serial;
-	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
-	__le32 *buf;
-	int result, i, length;
-
-	/* Number of integers required to contain the array */
-	length = (((size - 1) | 3) + 1) / 4;
-
-	buf = kmalloc(length * sizeof(__le32), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	/* Array of integers into bytes */
-	for (i = 0; i < length; i++)
-		buf[i] = cpu_to_le32(data[i]);
-
-	if (size > 2) {
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_INTERFACE, 0x0000,
-				spriv->bInterfaceNumber, buf, size,
-				USB_CTRL_SET_TIMEOUT);
-	} else {
-		result = usb_control_msg(serial->dev,
-				usb_sndctrlpipe(serial->dev, 0),
-				request, REQTYPE_HOST_TO_INTERFACE, data[0],
-				spriv->bInterfaceNumber, NULL, 0,
-				USB_CTRL_SET_TIMEOUT);
-	}
-
-	kfree(buf);
-
-	if ((size > 2 && result != size) || result < 0) {
-		dev_dbg(&port->dev, "%s - Unable to send request, request=0x%x size=%d result=%d\n",
-			__func__, request, size, result);
-		if (result > 0)
-			result = -EPROTO;
-
-		return result;
-	}
-
-	return 0;
+	if (size > 2)
+		return cp210x_control_msg(port, request,
+				REQTYPE_HOST_TO_INTERFACE, 0x0000,
+				data, size, USB_CTRL_SET_TIMEOUT);
+	else
+		return cp210x_control_msg(port, request,
+				REQTYPE_HOST_TO_INTERFACE, data[0],
+				NULL, 0, USB_CTRL_SET_TIMEOUT);
 }
 
 /*
@@ -413,7 +400,9 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
 static inline int cp210x_set_config_single(struct usb_serial_port *port,
 		u8 request, unsigned int data)
 {
-	return cp210x_set_config(port, request, &data, 2);
+	return cp210x_control_msg(port, request,
+			REQTYPE_HOST_TO_INTERFACE, data,
+			NULL, 0, USB_CTRL_SET_TIMEOUT);
 }
 
 /*
-- 
2.1.4

[PATCH 3/4] cp210x: Store part number

[Petr Tesarik]

From: Petr Tesarik <ptesarik@suse.cz>

Query and store the CP210x part number.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 drivers/usb/serial/cp210x.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 69f03b6..dbfc722 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -199,6 +199,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 struct cp210x_serial_private {
 	__u8			bInterfaceNumber;
+	__u8			bPartNumber;
 };
 
 static struct usb_serial_driver cp210x_device = {
@@ -264,6 +265,7 @@ enum cp210x_request_type {
 #define CP210X_SET_CHARS	0x19
 #define CP210X_GET_BAUDRATE	0x1D
 #define CP210X_SET_BAUDRATE	0x1E
+#define CP210X_VENDOR_SPECIFIC	0xFF
 
 /* CP210X_IFC_ENABLE */
 #define UART_ENABLE		0x0001
@@ -306,6 +308,17 @@ enum cp210x_request_type {
 #define CONTROL_WRITE_DTR	0x0100
 #define CONTROL_WRITE_RTS	0x0200
 
+/* CP210X_VENDOR_SPECIFIC */
+#define CP210X_GET_PARTNUM	0x370B
+
+/* Part number definitions */
+#define CP2101_PARTNUM	0x01
+#define CP2102_PARTNUM	0x02
+#define CP2103_PARTNUM	0x03
+#define CP2104_PARTNUM	0x04
+#define CP2105_PARTNUM	0x05
+#define CP2108_PARTNUM	0x08
+
 /*
  * cp210x_control_msg
  * Sends a generic control message, taking care of endianness
@@ -862,6 +875,7 @@ static int cp210x_startup(struct usb_serial *serial)
 {
 	struct usb_host_interface *cur_altsetting;
 	struct cp210x_serial_private *spriv;
+	unsigned int partnum;
 
 	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
 	if (!spriv)
@@ -872,6 +886,12 @@ static int cp210x_startup(struct usb_serial *serial)
 
 	usb_set_serial_data(serial, spriv);
 
+	/* Get the 1-byte part number of the cp210x device */
+	cp210x_control_msg(serial->port[0], CP210X_VENDOR_SPECIFIC,
+			   REQTYPE_DEVICE_TO_HOST, CP210X_GET_PARTNUM,
+			   &partnum, 1, USB_CTRL_GET_TIMEOUT);
+	spriv->bPartNumber = partnum & 0xFF;
+
 	return 0;
 }
 
-- 
2.1.4

[PATCH 4/4] cp210x: Expose the part number in sysfs

[Petr Tesarik]

From: Petr Tesarik <ptesarik@suse.cz>

Make it possible to read the cp210x part number from userspace by making
it a sysfs attribute.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 drivers/usb/serial/cp210x.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index dbfc722..66de350 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 	cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
 }
 
+static ssize_t part_number_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_serial *serial = usb_get_intfdata(intf);
+	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
+
+	return sprintf(buf, "%i\n", spriv->bPartNumber);
+}
+
+static DEVICE_ATTR_RO(part_number);
+
 static int cp210x_startup(struct usb_serial *serial)
 {
 	struct usb_host_interface *cur_altsetting;
 	struct cp210x_serial_private *spriv;
 	unsigned int partnum;
+	int result;
 
 	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
 	if (!spriv)
@@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial *serial)
 			   &partnum, 1, USB_CTRL_GET_TIMEOUT);
 	spriv->bPartNumber = partnum & 0xFF;
 
-	return 0;
+	result = device_create_file(&serial->interface->dev,
+				    &dev_attr_part_number);
+	if (result)
+		kfree(spriv);
+
+	return result;
 }
 
 static void cp210x_release(struct usb_serial *serial)
 {
 	struct cp210x_serial_private *spriv;
 
+	device_remove_file(&serial->interface->dev, &dev_attr_part_number);
 	spriv = usb_get_serial_data(serial);
 	kfree(spriv);
 }
-- 
2.1.4

Re: [PATCH 4/4] cp210x: Expose the part number in sysfs

[Greg Kroah-Hartman]

On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.cz>
> 
> Make it possible to read the cp210x part number from userspace by making
> it a sysfs attribute.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>

All sysfs files need to be documented in Documentation/ABI/

> ---
>  drivers/usb/serial/cp210x.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index dbfc722..66de350 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
>  	cp210x_set_config(port, CP210X_SET_BREAK, &state, 2);
>  }
>  
> +static ssize_t part_number_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct usb_interface *intf = to_usb_interface(dev);
> +	struct usb_serial *serial = usb_get_intfdata(intf);
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +
> +	return sprintf(buf, "%i\n", spriv->bPartNumber);
> +}
> +
> +static DEVICE_ATTR_RO(part_number);
> +
>  static int cp210x_startup(struct usb_serial *serial)
>  {
>  	struct usb_host_interface *cur_altsetting;
>  	struct cp210x_serial_private *spriv;
>  	unsigned int partnum;
> +	int result;
>  
>  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
>  	if (!spriv)
> @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial *serial)
>  			   &partnum, 1, USB_CTRL_GET_TIMEOUT);
>  	spriv->bPartNumber = partnum & 0xFF;
>  
> -	return 0;
> +	result = device_create_file(&serial->interface->dev,
> +				    &dev_attr_part_number);

You just raced with userspace, it will not properly see this attribute
:(

Please never use device_create_file, use an attribute group assigned to
the tty device.  Not the USB interface, that is only for USB interface
"things".

thanks,

greg k-h

Re: [PATCH 4/4] cp210x: Expose the part number in sysfs

[Petr Tesarik]

On Fri, 24 Jul 2015 11:17:55 -0700
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > From: Petr Tesarik <ptesarik@suse.cz>
> > 
> > Make it possible to read the cp210x part number from userspace by making
> > it a sysfs attribute.
> > 
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> 
> All sysfs files need to be documented in Documentation/ABI/

Is this a recently added requirement? FWIW there are many undocumented
sysfs attributes, even in code maintained by you. E.g. each usbserial
(ttyUSB*) device has an attribute called "port_number" which is not
documented. Or I'm blind...

> > ---
> >  drivers/usb/serial/cp210x.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/serial/cp210x.c
> > b/drivers/usb/serial/cp210x.c index dbfc722..66de350 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct
> > tty_struct *tty, int break_state) cp210x_set_config(port,
> > CP210X_SET_BREAK, &state, 2); }
> >  
> > +static ssize_t part_number_show(struct device *dev,
> > +				struct device_attribute *attr,
> > char *buf) +{
> > +	struct usb_interface *intf = to_usb_interface(dev);
> > +	struct usb_serial *serial = usb_get_intfdata(intf);
> > +	struct cp210x_serial_private *spriv =
> > usb_get_serial_data(serial); +
> > +	return sprintf(buf, "%i\n", spriv->bPartNumber);
> > +}
> > +
> > +static DEVICE_ATTR_RO(part_number);
> > +
> >  static int cp210x_startup(struct usb_serial *serial)
> >  {
> >  	struct usb_host_interface *cur_altsetting;
> >  	struct cp210x_serial_private *spriv;
> >  	unsigned int partnum;
> > +	int result;
> >  
> >  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> >  	if (!spriv)
> > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial
> > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT);
> >  	spriv->bPartNumber = partnum & 0xFF;
> >  
> > -	return 0;
> > +	result = device_create_file(&serial->interface->dev,
> > +				    &dev_attr_part_number);
> 
> You just raced with userspace, it will not properly see this attribute
> :(

Can you elaborate on this, please? AFAICS the file is created after all
required objects had been instantiated already. Where's the race?

> Please never use device_create_file, use an attribute group assigned
> to the tty device.  Not the USB interface, that is only for USB
> interface "things".

Well, I hesitated with adding it to the USB interface, but adding it to
the tty device is definitely wrong. This is indeed an attribute of the
device, not of the tty. If you look at the other CP210x thread, there's
also a gpio device in the chip. I think it's totally silly to look
inside a tty interface to see if there are any GPIO pins...

OK, if the USB interface is the wrong place, what's a good place for
such a thing? I don't insist on a sysfs attribute, but I don't agree
with the tty device.

Petr T

Re: [PATCH 4/4] cp210x: Expose the part number in sysfs

[Greg Kroah-Hartman]

On Fri, Jul 24, 2015 at 11:00:38PM +0200, Petr Tesarik wrote:
> On Fri, 24 Jul 2015 11:17:55 -0700
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > > From: Petr Tesarik <ptesarik@suse.cz>
> > > 
> > > Make it possible to read the cp210x part number from userspace by making
> > > it a sysfs attribute.
> > > 
> > > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > 
> > All sysfs files need to be documented in Documentation/ABI/
> 
> Is this a recently added requirement? FWIW there are many undocumented
> sysfs attributes, even in code maintained by you. E.g. each usbserial
> (ttyUSB*) device has an attribute called "port_number" which is not
> documented. Or I'm blind...

It's been a requirement for years.  If we have missed any, please let me
know and we will add them.  Sometimes we miss this when adding new
attributes, and many very old attributes never got documented.

> 
> > > ---
> > >  drivers/usb/serial/cp210x.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/serial/cp210x.c
> > > b/drivers/usb/serial/cp210x.c index dbfc722..66de350 100644
> > > --- a/drivers/usb/serial/cp210x.c
> > > +++ b/drivers/usb/serial/cp210x.c
> > > @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct
> > > tty_struct *tty, int break_state) cp210x_set_config(port,
> > > CP210X_SET_BREAK, &state, 2); }
> > >  
> > > +static ssize_t part_number_show(struct device *dev,
> > > +				struct device_attribute *attr,
> > > char *buf) +{
> > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > +	struct usb_serial *serial = usb_get_intfdata(intf);
> > > +	struct cp210x_serial_private *spriv =
> > > usb_get_serial_data(serial); +
> > > +	return sprintf(buf, "%i\n", spriv->bPartNumber);
> > > +}
> > > +
> > > +static DEVICE_ATTR_RO(part_number);
> > > +
> > >  static int cp210x_startup(struct usb_serial *serial)
> > >  {
> > >  	struct usb_host_interface *cur_altsetting;
> > >  	struct cp210x_serial_private *spriv;
> > >  	unsigned int partnum;
> > > +	int result;
> > >  
> > >  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> > >  	if (!spriv)
> > > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial
> > > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT);
> > >  	spriv->bPartNumber = partnum & 0xFF;
> > >  
> > > -	return 0;
> > > +	result = device_create_file(&serial->interface->dev,
> > > +				    &dev_attr_part_number);
> > 
> > You just raced with userspace, it will not properly see this attribute
> > :(
> 
> Can you elaborate on this, please? AFAICS the file is created after all
> required objects had been instantiated already. Where's the race?

That's the race.  See this blog post for all the details:
	http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

> > Please never use device_create_file, use an attribute group assigned
> > to the tty device.  Not the USB interface, that is only for USB
> > interface "things".
> 
> Well, I hesitated with adding it to the USB interface, but adding it to
> the tty device is definitely wrong. This is indeed an attribute of the
> device, not of the tty. If you look at the other CP210x thread, there's
> also a gpio device in the chip. I think it's totally silly to look
> inside a tty interface to see if there are any GPIO pins...
> 
> OK, if the USB interface is the wrong place, what's a good place for
> such a thing? I don't insist on a sysfs attribute, but I don't agree
> with the tty device.

Being a usb-serial driver, you don't have "access" to the main USB
device, so you are kind of violating some layering rules by taking this
on to the interface.

Who / what is going to use this file?  What is going to be done with
the information and to what device is that information going to be
associated with?

thanks,

greg k-h

Re: [PATCH 4/4] cp210x: Expose the part number in sysfs

[Petr Tesarik]

On Fri, 24 Jul 2015 14:33:23 -0700
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Jul 24, 2015 at 11:00:38PM +0200, Petr Tesarik wrote:
> > On Fri, 24 Jul 2015 11:17:55 -0700
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > > > From: Petr Tesarik <ptesarik@suse.cz>
> > > > 
> > > > Make it possible to read the cp210x part number from userspace by making
> > > > it a sysfs attribute.
> > > > 
> > > > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > > 
> > > All sysfs files need to be documented in Documentation/ABI/
> > 
> > Is this a recently added requirement? FWIW there are many undocumented
> > sysfs attributes, even in code maintained by you. E.g. each usbserial
> > (ttyUSB*) device has an attribute called "port_number" which is not
> > documented. Or I'm blind...
> 
> It's been a requirement for years.  If we have missed any, please let me
> know and we will add them.  Sometimes we miss this when adding new
> attributes, and many very old attributes never got documented.

Fair enough. The example I gave is ancient...

>[...]
> > > > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial
> > > > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT);
> > > >  	spriv->bPartNumber = partnum & 0xFF;
> > > >  
> > > > -	return 0;
> > > > +	result = device_create_file(&serial->interface->dev,
> > > > +				    &dev_attr_part_number);
> > > 
> > > You just raced with userspace, it will not properly see this attribute
> > > :(
> > 
> > Can you elaborate on this, please? AFAICS the file is created after all
> > required objects had been instantiated already. Where's the race?
> 
> That's the race.  See this blog post for all the details:
> 	http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

Thank you for the link!

> > > Please never use device_create_file, use an attribute group assigned
> > > to the tty device.  Not the USB interface, that is only for USB
> > > interface "things".
>[...]
> > OK, if the USB interface is the wrong place, what's a good place for
> > such a thing? I don't insist on a sysfs attribute, but I don't agree
> > with the tty device.
> 
> Being a usb-serial driver, you don't have "access" to the main USB
> device, so you are kind of violating some layering rules by taking this
> on to the interface.

True. This is still one of my concerns, but the GPIO functionality is
minor compared to the serial bridge, so strictly following layering
rules would be overkill. See Johan Hovold's answer in the thread about
GPIO support for Silicon Labs cp210x USB serial:

  Indeed, you should just hang the gpio device directly off the
  usb-serial device [...]

> Who / what is going to use this file?  What is going to be done with
> the information and to what device is that information going to be
> associated with?

Many thanks again. Thinking about it some more, once I'm done with my
work, userspace can enumerate the gpio devices using sysfs without
having to care about the specific model. The part number is only
relevant internally to the cp210x driver.

In short, there is in fact no user of this file, so the best option is
to report the model in syslog and forget about a sysfs file. Simple.

Petr

Re: [PATCH 3/4] cp210x: Store part number

[Oliver Neukum]

On Fri, 2015-07-24 at 08:48 +0200, Petr Tesarik wrote:
> @@ -872,6 +886,12 @@ static int cp210x_startup(struct usb_serial
> *serial)
>  
>         usb_set_serial_data(serial, spriv);
>  
> +       /* Get the 1-byte part number of the cp210x device */
> +       cp210x_control_msg(serial->port[0], CP210X_VENDOR_SPECIFIC,
> +                          REQTYPE_DEVICE_TO_HOST, CP210X_GET_PARTNUM,
> +                          &partnum, 1, USB_CTRL_GET_TIMEOUT);
> +       spriv->bPartNumber = partnum & 0xFF;

DMA on the stack. That breaks the cache coherency rules.
You must kmalloc the buffer.

	Regards
		Oliver

Re: [PATCH 3/4] cp210x: Store part number

[Petr Tesarik]

On Sun, 26 Jul 2015 15:32:54 +0200
Oliver Neukum <oneukum@suse.com> wrote:

> On Fri, 2015-07-24 at 08:48 +0200, Petr Tesarik wrote:
> > @@ -872,6 +886,12 @@ static int cp210x_startup(struct usb_serial
> > *serial)
> >  
> >         usb_set_serial_data(serial, spriv);
> >  
> > +       /* Get the 1-byte part number of the cp210x device */
> > +       cp210x_control_msg(serial->port[0], CP210X_VENDOR_SPECIFIC,
> > +                          REQTYPE_DEVICE_TO_HOST, CP210X_GET_PARTNUM,
> > +                          &partnum, 1, USB_CTRL_GET_TIMEOUT);
> > +       spriv->bPartNumber = partnum & 0xFF;
> 
> DMA on the stack. That breaks the cache coherency rules.
> You must kmalloc the buffer.

I don't understand. While you're right that I copied this part from
Sillicon Labs' driver without much thinking, and &spriv->bPartNumber
can be used directly, I can't see any DMA on stack. FWIW
cp210x_control_msg always allocates a buffer using kcalloc:

        buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
/* ... */
        result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
                                 request, requesttype, value,
                                 spriv->bInterfaceNumber, buf, size, timeout);

Is that what you mean?

TIA,
Petr T

Re: [PATCH 3/4] cp210x: Store part number

[Oliver Neukum]

On Mon, 2015-07-27 at 08:50 +0200, Petr Tesarik wrote:
> I don't understand. While you're right that I copied this part from
> Sillicon Labs' driver without much thinking, and &spriv->bPartNumber
> can be used directly, I can't see any DMA on stack. FWIW
> cp210x_control_msg always allocates a buffer using kcalloc:
> 
>         buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
> /* ... */
>         result = usb_control_msg(serial->dev,
> usb_rcvctrlpipe(serial->dev, 0),
>                                  request, requesttype, value,
>                                  spriv->bInterfaceNumber, buf, size,
> timeout);
> 
> Is that what you mean?

Yes, sorry, that part wasn't so clear from the previous patch.

	Sorry
		Oliver

Re: [PATCH 2/4] cp210x: Unify code for set/get config control messages

[Johan Hovold]

On Fri, Jul 24, 2015 at 08:48:09AM +0200, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.cz>
> 
> There is a lot of overlap between the two functions (e.g. calculation
> of the buffer size), so this removes a bit of code duplication, but
> most importantly, a more generic function can be easily reused for
> other message types.

I'm not sure I consider this is an improvement yet.

> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
>  drivers/usb/serial/cp210x.c | 109 ++++++++++++++++++++------------------------
>  1 file changed, 49 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 1bae015..69f03b6 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -307,14 +307,17 @@ enum cp210x_request_type {
>  #define CONTROL_WRITE_RTS	0x0200
>  
>  /*
> - * cp210x_get_config
> - * Reads from the CP210x configuration registers
> + * cp210x_control_msg
> + * Sends a generic control message, taking care of endianness
> + * and error messages.
>   * 'size' is specified in bytes.
> - * 'data' is a pointer to a pre-allocated array of integers large
> - * enough to hold 'size' bytes (with 4 bytes to each integer)
> + * 'data' is a pointer to the input/output buffer. For output, it holds
> + * the data (in host order) to be sent. For input, it receives data from
> + * the device and must be big enough to hold 'size' bytes.
>   */
> -static int cp210x_get_config(struct usb_serial_port *port, u8 request,
> -		unsigned int *data, int size)
> +static int cp210x_control_msg(struct usb_serial_port *port, u8 request,
> +			      u8 requesttype, u16 value, u32 *data, int size,
> +			      int timeout)

Should you not use your new request type enum here?

>  {
>  	struct usb_serial *serial = port->serial;
>  	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> @@ -328,20 +331,22 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	/* Issue the request, attempting to read 'size' bytes */
> -	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> -				request, REQTYPE_INTERFACE_TO_HOST, 0x0000,
> -				spriv->bInterfaceNumber, buf, size,
> -				USB_CTRL_GET_TIMEOUT);
> +	if (!(requesttype & USB_DIR_IN)) {
> +		for (i = 0; i < length; i++)
> +			buf[i] = cpu_to_le32(data[i]);
> +	}
>  
> -	/* Convert data into an array of integers */
> -	for (i = 0; i < length; i++)
> -		data[i] = le32_to_cpu(buf[i]);
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),

And this should be usb_sndctrlpipe for outgoing messages.

> +				 request, requesttype, value,
> +				 spriv->bInterfaceNumber, buf, size, timeout);

Please resend this when you start using your generalised function (for
the gpio work?).

I'll drop all four for now.

Thanks,
Johan