On Tue, May 18, 2010 at 08:44:59AM -0600, Steve McKown wrote:
> On Monday 17 May 2010 10:19:31 pm Greg KH wrote:
> > On Mon, May 17, 2010 at 06:50:13PM -0600, R. Steve McKown wrote:
> > > +/* 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.
>
> Would following the guidance of ftdi_sio be a better approach?
>
> /* FTDI_SIO_RESET */
> #define FTDI_SIO_RESET_REQUEST FTDI_SIO_RESET
> #define FTDI_SIO_RESET_REQUEST_TYPE 0x40
No, ftdi_sio is not the best thing to be following :)
> then
>
> rv = usb_control_msg(port->serial->dev,
> usb_sndctrlpipe(port->serial->dev, 0),
> FTDI_SIO_SET_MODEM_CTRL_REQUEST,
> FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
> urb_value, priv->interface,
> NULL, 0, WDR_TIMEOUT);
No, please just spell out the "real" usb flags here, don't bury them
behind a #define chain.
> > > +/* 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.
>
> I can fix the unhelpful comment.
>
> Determining bytes needed to store a string properly encoded to send to the
> cp210x happens in several places through the code. I thought it would be
> more correct to encapsulate the logic behind this mapping in a single
> location...?
I agree it is good to put it in a consistant place, but you are using
"strlen" which kind of implies that this is a real function that will
take a string as a parameter, right?
> > > +{
> > > + 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? :)
>
> OK, I'll try to get up to speed on unicode and figure out something better
> here. If you have an example reference off the top of your head...
No, sorry, it was more a joke. USB strings were always supposed to be a
"real" 16bit value per character, but yet, it is very rare that anyone
ever used that other 8 bits. You are doing the same thing here, never
setting those other bits. It's fine if that's all you need to do.
> > > + 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?
>
> Yes. The snippet above is in cp210x_usbstr_from_user. All callers are in
> cp210x_ioctl and must see a non-zero return from cp210x_usbstr_from_user else
> they return with -EFAULT.
Ok, but how are you detecting the two different error modes (invalid
parameters and bad copy)?
As I think you will be getting rid of your ioctl call, this isn't going
to be needed.
> > > + 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.
>
> Do you mean that t.len*2+2, which is what the USBSTRLEN(t.len) resolves to,
> might wrap in its size_t?
Yes. What happens if I pass a "negative" or very big value as len here?
It will wrap when multiplying by 2, which would pass the check.
> I can solve this by moving the assigment of slen
> up (slen = (klen-2)/2) and using t.len > slen instead.
Yes 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?
>
> It's already on my todo list; I just haven't got to it yet.
Ok, that's the big change that I would like to see happen. I don't want
to add new ioctls to the kernel if at all possible, especially one-off
ones like this that only work for one specific driver/device.
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
