On Mon, May 17, 2010 at 06:50:13PM -0600, R. Steve McKown wrote:
> Hi Greg,
>
> I believe I've addressed your earlier points except for kernel gpio, and
> part number visibility via sysfs. I'll try to add the former in the
> coming weeks. The patch passes checkpatch.pl and the code shows no
> errors from 'sparse'. I hope you are willing to continue the mentorship
> for a few more rounds. What additional changes are needed? Thanks again
> for taking the extra time with me on this.
>
> You had some questions:
>
> > > +/* FIXME: where is IOCTL_SETMFG? */
> >
> > What is this message?
>
> As of a year or so ago, CP210X internal firmware didn't offer the ability
> to change the USB Manufacturer descriptor. SiLabs said they might change
> this. See cp210x_has_setmfg(). Perhaps I should yank all the mfg set
> code for now (aka as simple as possible)?
Yes, please keep it simple :)
> > > +#define CP210x_CP2101_VERSION 0x01
> > > +#define CP210x_CP2102_VERSION 0x02
> > > +#define CP210x_CP2103_VERSION 0x03
> >
> > Why is this needed? Can't we just expose the version in a sysfs file
> > instead of using an ioctl?
>
> cp210x_get_partnum() is used internally only to interact with the cp210x
> according to what part it actually is. For example, only cp2103 supports
> GPIO. Not sure how useful it is to export this information to userspace.
Internally is fine, but I don't think we need an ioctl for something
like this, right?
> And now for the patch proper
> ============================
>
> Add GPIO, port configuration and USB descriptor management
> via IOCTL to cp210x.
>
> Signed-off-by: R. Steve McKown <rsmckown@xxxxxxxxx>
> ---
>
> This patch is based on linus' v2.6.34-rc7.
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ec9b044..3086ab6 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -23,6 +23,7 @@
> #include <linux/usb.h>
> #include <linux/uaccess.h>
> #include <linux/usb/serial.h>
> +#include "cp210x.h"
>
> /*
> * Version Information
> @@ -50,6 +51,8 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port, struct file *,
> static void cp210x_break_ctl(struct tty_struct *, int);
> static int cp210x_startup(struct usb_serial *);
> static void cp210x_disconnect(struct usb_serial *);
> +static int cp210x_ioctl(struct tty_struct *, struct file *,
> + unsigned int, unsigned long);
> static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
> static int cp210x_carrier_raised(struct usb_serial_port *p);
>
> @@ -140,6 +143,7 @@ static struct usb_serial_driver cp210x_device = {
> .num_ports = 1,
> .open = cp210x_open,
> .close = cp210x_close,
> + .ioctl = cp210x_ioctl,
> .break_ctl = cp210x_break_ctl,
> .set_termios = cp210x_set_termios,
> .tiocmget = cp210x_tiocmget,
> @@ -150,9 +154,13 @@ static struct usb_serial_driver cp210x_device = {
> .carrier_raised = cp210x_carrier_raised
> };
>
> +/* Control request types */
> +#define REQTYPE_CTL_TO_DEVICE USB_TYPE_VENDOR
Don't redefine an existing, well-known, define. That forces people to
try to have to dig back and figure out what is going on here. What's
wrong with just using USB_TYPE_VENDOR?
> +#define REQTYPE_CTL_TO_HOST (USB_DIR_IN|REQTYPE_CTL_TO_DEVICE)
Same here, why even need this?
> /* Config request types */
> -#define REQTYPE_HOST_TO_DEVICE 0x41
> -#define REQTYPE_DEVICE_TO_HOST 0xc1
> +#define REQTYPE_HOST_TO_DEVICE (USB_TYPE_VENDOR|USB_RECIP_INTERFACE)
> +#define REQTYPE_DEVICE_TO_HOST (USB_DIR_IN|REQTYPE_HOST_TO_DEVICE)
Same here, just spell these out where you use them.
> /* Config request codes */
> #define CP210X_IFC_ENABLE 0x00
> @@ -221,6 +229,290 @@ static struct usb_serial_driver cp210x_device = {
> #define CONTROL_WRITE_DTR 0x0100
> #define CONTROL_WRITE_RTS 0x0200
>
> +/* cp210x_get_partnum() return codes */
> +#define CP210x_PART_UNKNOWN 0x00
> +#define CP210x_PART_CP2101 0x01
> +#define CP210x_PART_CP2102 0x02
> +#define CP210x_PART_CP2103 0x03
> +
> +/* Helper to make usb string size */
> +#define USBSTRLEN(x) (x * 2 + 2)
Helper based on what? 16bit character string length? Again, just spell
it out.
> +/* Populates usbstr with: (len) + (0x03) + unicode(str). Each char in str
> + * takes two bytes in unicode format.
> + * Returns the resulting length of the string in usbstr.
> + * This function can accept overlapping usbstr and str as long as the overlap
> + * does not cause data written to usbstr to overwrite data not yet read from
> + * str.
> + */
> +static int make_usb_string(char *usbstr, size_t usblen, char *src,
> + size_t srclen)
Is this something that we need/want in the usb core? Are you sure we
don't already have something like this in the kernel somewhere?
> +{
> + int len = 0;
> +
> + if (usbstr && usblen >= 2 && src && *src && srclen) {
> + char *p;
> +
> + if (usblen > 255)
> + usblen = 255;
> +
> + p = usbstr + 1;
> + *p++ = 0x03;
> + len = 2;
> + while (srclen && len < usblen) {
> + *p++ = *src++;
> + *p++ = 0;
So, no unicode? :)
> + len += 2;
> + srclen--;
> + }
> + *usbstr = (char)len;
> + }
> + return len;
> +}
> +
> +/*
> + * cp210x_usbstr_from_user
> + * Populate kbuf with a usb string derived from a user space variable. klen
> + * is the size of the buffer at kbuf.
> + * Returns the number of bytes used in kbuf.
> + */
> +static size_t cp210x_usbstr_from_user(char *kbuf, unsigned long ubuf,
> + size_t klen)
Make ubuf be the real __user pointer to the structure here please.
> +{
> + struct cp210x_buffer t;
> + char *str;
> + size_t slen;
> +
> + if (!kbuf || !ubuf || !klen)
> + return 0;
How would this ever be true? You control the callers of this function.
> + if (copy_from_user(&t, (struct cp210x_buffer __user *)ubuf, sizeof(t)))
> + return 0;
No, you need to fail this properly with -EFAULT somehow back to
userspace. Are you doing that?
> + if (!t.buf || !t.len || USBSTRLEN(t.len) > klen)
Could you just have overflowed that t.len check? Be _very_ careful
about that, as it's a userspace parameter.
> + return 0;
> + slen = (klen - 2) / 2;
> + if (t.len < slen)
> + slen = t.len;
> + str = kbuf + klen - slen;
> + if (copy_from_user(str, (u8 __user *)t.buf, slen))
t.buf should already be a __u8 __user *, right? Don't use the same
structure types in userspace and in the kernel.
And how does this handle a 64bit kernel in a 32bit userspace? Have you
hooked up the ioctl compatibility layer properly?
And yeah, ioctls are a pain, that's one reason we hate them so much :)
> +/* cp210x_has_setmfg
> + * Returns 1 if the CP210X part includes firmware that allows setting the
> + * USB MFG descriptor, else 0. As of this writing, no CP210X firmware allows
> + * this. SiLabs has suggested this may change in future firmware versions or
> + * parts.
> + */
> +static inline int cp210x_has_setmfg(void)
> +{
> + return 0;
> +}
Just drop it for now please.
> + case CP210x_IOCTL_GPIOGET:
> + if (cp210x_get_partnum(port) == CP210x_PART_CP2103) {
> + u8 gpio = 0;
> + if (!cp210x_gpioget(port, &gpio) && !copy_to_user(
> + (u8 __user *)arg, &gpio, sizeof(gpio)))
> + return 0;
Wait, this isn't the standard Linux GPIO layer. What happened to using
that instead of a custom ioctl? That is the correct way to do this,
don't create custom messages here please, it makes support much more
difficult.
Just hook up these GPIO pins to the already-in-kernel gpio layer and
that will export them to userspace properly and you will not have to add
any new ioctls.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
