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:
> > > > +#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?
Right. This info is not exposed to userspace in the current patch.
> > +/* 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
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);
> > +/* 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...?
> > +/* 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?
I'm not sure there isn't something similar already present. I'll look for it.
>
> > +{
> > + 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...
> > +{
> > + 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.
You're right. This is a static function only called from the local compile
unit. I can clean up the unneeded error handling.
>
> > + 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.
>
> > + 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? I can solve this by moving the assigment of slen
up (slen = (klen-2)/2) and using t.len > slen instead.
>
> > + 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.
Yes, typo.
> 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 :)
I'm not at all familiar with "ioctl compatibility layer" and will do some
digging. Thanks for the heads up.
>
> > +/* 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.
OK.
>
> > + 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.
I'll get to work on your latest suggestions.
Thank you for your continued help.
Steve
--
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
