On Tue, Feb 14, 2012 at 10:10:12PM -0800, Michal Nazarewicz wrote:
> >On Fri, Feb 10, 2012 at 09:54:51AM +0100, Lukasz Majewski wrote:
> >>It is crucial to assign each req->context value to struct rndis.
> >>
> >>The problem happens for multi function gadget (g_multi) when multiple
> >>functions are calling common usb_composite_dev control request.
> >>
> >>It might happen that *_setup method from one usb function will
> >>alter some fields of this common request issued by other USB
> >>function.
> >this is a bit weird, why would a function change a request which isn't
> >supposed to handled by it ? I mean, functions need to check the ctrl
> >request to be sure they should be handling it.
>
> It's composite layer who directs requests to functions. Functions do
> not need to check the request.
well, composite does call into functions correctly, but functions need
to see if the control request is valid for them still.
> >Your patch shouldn't be necessary, really, because the context is
> >initialized when the request is allocated.
>
> Yes, but there is only one request for EP0 which is shared between
> all the functions.
true.
> >what happens exactly ? Who changes the context field and why ?
>
> The context is changed by all other functions.
I didn't see that, actually, look at f_acm:
static int acm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
/* lots of LA LA LA */
if (value >= 0) {
DBG(cdev, "acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n",
acm->port_num, ctrl->bRequestType, ctrl->bRequest,
w_value, w_index, w_length);
req->zero = 0;
req->length = value;
value = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
if (value < 0)
ERROR(cdev, "acm response on ttyGS%d, err %d\n",
acm->port_num, value);
}
/* device either stalls (value < 0) or reports success */
return value;
}
or f_eem:
static int eem_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
struct usb_composite_dev *cdev = f->config->cdev;
int value = -EOPNOTSUPP;
u16 w_index = le16_to_cpu(ctrl->wIndex);
u16 w_value = le16_to_cpu(ctrl->wValue);
u16 w_length = le16_to_cpu(ctrl->wLength);
DBG(cdev, "invalid control req%02x.%02x v%04x i%04x l%d\n",
ctrl->bRequestType, ctrl->bRequest,
w_value, w_index, w_length);
/* device either stalls (value < 0) or reports success */
return value;
}
or f_ecm:
static int ecm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
/* lots of LA LA LA */
/* respond with data transfer or status phase? */
if (value >= 0) {
DBG(cdev, "ecm req%02x.%02x v%04x i%04x l%d\n",
ctrl->bRequestType, ctrl->bRequest,
w_value, w_index, w_length);
req->zero = 0;
req->length = value;
value = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
if (value < 0)
ERROR(cdev, "ecm req %02x.%02x response err %d\n",
ctrl->bRequestType, ctrl->bRequest,
value);
}
/* device either stalls (value < 0) or reports success */
return value;
}
or f_audio:
static int
f_audio_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
/* lots of LA LA LA */
/* respond with data transfer or status phase? */
if (value >= 0) {
DBG(cdev, "audio req%02x.%02x v%04x i%04x l%d\n",
ctrl->bRequestType, ctrl->bRequest,
w_value, w_index, w_length);
req->zero = 0;
req->length = value;
value = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
if (value < 0)
ERROR(cdev, "audio response on err %d\n", value);
}
/* device either stalls (value < 0) or reports success */
return value;
}
or f_hid:
static int hidg_setup(struct usb_function *f,
const struct usb_ctrlrequest *ctrl)
{
/* lots of LA LA LA */
respond:
req->zero = 0;
req->length = length;
status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
if (status < 0)
ERROR(cdev, "usb_ep_queue error on ep0 %d\n", value);
return status;
}
or f_sourcesink:
static int sourcesink_setup(struct usb_configuration *c,
const struct usb_ctrlrequest *ctrl)
{
/* lots of LA LA LA */
/* respond with data transfer or status phase? */
if (value >= 0) {
VDBG(c->cdev, "source/sink req%02x.%02x v%04x i%04x l%d\n",
ctrl->bRequestType, ctrl->bRequest,
w_value, w_index, w_length);
req->zero = 0;
req->length = value;
value = usb_ep_queue(c->cdev->gadget->ep0, req, GFP_ATOMIC);
if (value < 0)
ERROR(c->cdev, "source/sinkc response, err %d\n",
value);
}
/* device either stalls (value < 0) or reports success */
return value;
}
or f_uvc:
static int
uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
{
struct uvc_device *uvc = to_uvc(f);
struct v4l2_event v4l2_event;
struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
/* printk(KERN_INFO "setup request %02x %02x value %04x index %04x %04x\n",
* ctrl->bRequestType, ctrl->bRequest, le16_to_cpu(ctrl->wValue),
* le16_to_cpu(ctrl->wIndex), le16_to_cpu(ctrl->wLength));
*/
if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
INFO(f->config->cdev, "invalid request type\n");
return -EINVAL;
}
/* Stall too big requests. */
if (le16_to_cpu(ctrl->wLength) > UVC_MAX_REQUEST_SIZE)
return -EINVAL;
memset(&v4l2_event, 0, sizeof(v4l2_event));
v4l2_event.type = UVC_EVENT_SETUP;
memcpy(&uvc_event->req, ctrl, sizeof(uvc_event->req));
v4l2_event_queue(uvc->vdev, &v4l2_event);
return 0;
}
For all I know, the buggy gadget drivers are f_ncm, f_mass_storage and
f_fs because they change a request which *does not* belong to them. The
real fix would be to sort that out in another way. The ep0 request
always belongs to composite.c so functions can't go around poking with
that request. Maybe the best solution would be to allow ep0 to have
multiple requests queued, then functions can allocate and maintain their
own ep0 request for cases when they need to send anyting to the host,
maybe.
--
balbi
Attachment:
signature.asc
Description: Digital signature
