linux-kernel - Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to defer setup responses

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <op.vt6vlrdf3l0zgt@mnazarewicz-glaptop>
Date:	Tue, 19 Apr 2011 16:23:41 +0200
From:	"Michal Nazarewicz" <mina86@...a86.com>
To:	gregkh@...e.de, "Roger Quadros" <roger.quadros@...ia.com>
Cc:	stern@...land.harvard.edu, m-sonasath@...com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] usb: gadget: composite: Allow function drivers to
 defer setup responses

On Tue, 19 Apr 2011 15:33:52 +0200, Roger Quadros  
<roger.quadros@...ia.com> wrote:
> @@ -895,6 +904,14 @@ composite_setup(struct usb_gadget *gadget, const  
> struct usb_ctrlrequest *ctrl)
>  		if (w_value && !f->set_alt)
>  			break;
>  		value = f->set_alt(f, w_index, w_value);
> +		if (value == USB_GADGET_DELAYED_STATUS) {
> +			DBG(cdev,
> +			 "%s: interface %d (%s) requested delayed status\n",
> +					__func__, intf, f->name);
> +			cdev->delayed_status++;

Do we need it to be a counter? Won't simple flag do?

> +			DBG(cdev, "delayed_status count %d\n",
> +					cdev->delayed_status);
> +		}
>  		break;
>  	case USB_REQ_GET_INTERFACE:
>  		if (ctrl->bRequestType != (USB_DIR_IN|USB_RECIP_INTERFACE))

> @@ -1289,3 +1310,41 @@ void usb_composite_unregister(struct  
> usb_composite_driver *driver)
>  		return;
>  	usb_gadget_unregister_driver(&composite_driver);
>  }
> +
> +/**
> + * usb_composite_setup_continue() - Continue the delayed setup transfer
> + * @cdev: the composite device who's setup transfer was delayed
> + *
> + * This function must be called by the USB function driver to continue
> + * with the setup transfer's data/status phase in case it had requested  
> to
> + * delay the status phase. A USB function's setup handler (e.g.  
> set_alt())
> + * can request the composite framework to delay the setup request's  
> status phase
> + * by returning USB_GADGET_DELAYED_STATUS.
> + */
> +void usb_composite_setup_continue(struct usb_composite_dev *cdev)
> +{
> +	int			value;
> +	struct usb_request	*req = cdev->req;
> +	unsigned long		flags;
> +
> +	DBG(cdev, "%s\n", __func__);
> +	spin_lock_irqsave(&cdev->lock, flags);
> +
> +	if (cdev->delayed_status == 0) {
> +		WARN(cdev, "%s: Unexpected call\n", __func__);
> +
> +	} else if (--cdev->delayed_status == 0) {
> +		DBG(cdev, "%s: Completing delayed status\n", __func__);
> +		req->length = 0;
> +		req->zero = 1;
> +		value = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
> +		if (value < 0) {
> +			DBG(cdev, "ep_queue --> %d\n", value);
> +			req->status = 0;
> +			composite_setup_complete(cdev->gadget->ep0, req);
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&cdev->lock, flags);
> +}

It's just my opinion but I dislike how it looks almost identical to the end
of composite_setup.  I would rather make it more general (take value as
argument for instance) and let composite_setup() call it.

> diff --git a/include/linux/usb/composite.h @@ -37,6 +37,15 @@
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
> +/*
> + * USB function drivers should return USB_GADGET_DELAYED_STATUS if they
> + * wish to delay the status phase of the setup transfer till they are
> + * ready. The composite framework will then delay the data/status phase
> + * of the setup transfer till all the function drivers that requested  
> for
> + * USB_GADGET_DELAYED_STAUS, invoke usb_composite_setup_continue().
> + *
> + */
> +#define USB_GADGET_DELAYED_STATUS       0x7fff	/* Impossibly large  
> value */

I was wondering if we could change it to something like EDELAYED and put  
it to
<include/linux/errno.h>.  Just thinking, it may be a bad idea.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@...gle.com>-----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/