Re: [PATCH] usb:gadget:hid: This patch adds support for set_protocol and get_protocol for remote wakeup
On Wed, 21 Jun 2017 abdulahha...@gmail.com wrote:
> Hi,
>
> Thanks for the reply! I'll be sure to send in another patch
> with fixes to all the formatting errors. But before that
> just a couple of points on your feedback.
>
> > > struct usb_request *req;
> > > unsigned long flags;
> > > ssize_t status = -ENOMEM;
> > >
> > > + usb_gadget_wakeup(cdev->gadget);
> >
> > this seems unrelated to implemeting GET/SET_PROTOCOL. Why do you need this?
>
> Right, I intially suspected support for these two features
> were dependant on one another. However, that does not seem to be the case.
> I'll send in a seperate patch to handle remote wake up support in
> the driver later.
>
> >
> > > @@ -528,6 +532,9 @@ static int hidg_setup(struct usb_function *f,
> > > | HID_REQ_GET_PROTOCOL):
> > > VDBG(cdev, "get_protocol\n");
> > > goto stall;
> > > + length = min_t(unsigned, length, 1);
> > > + ((u8 *) req->buf)[0]= hidg->protocol_is_report;
> > > + goto respond;
> > > break;
Felipe didn't mention this, but the lines you added here are dead code
because you forgot to remove the "goto stall".
Also, you have a number of excess space characters before the second
'=' sign.
> > >
> > > case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> > > @@ -539,6 +546,17 @@ static int hidg_setup(struct usb_function *f,
> > > case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> > > | HID_REQ_SET_PROTOCOL):
> > > VDBG(cdev, "set_protocol\n");
> > > + if (value > 1)
> >
> > why 1 here? Seems like this should be
> > USB_INTERFACE_PROTOCOL_KEYBOARD. And are you sure there's nobody using
> > this to emulate a mouse?
>
> According to 7.2.6 of the HID spec. For the wValue field, 0 indicates Boot
> Protocol and 1 indicates Report Protocol, which applies to both mice
> and keyboards. So the use of that macro wouldn't make sense.
>
> > > + goto stall;
> > > + length = 0;
> > > + /*
> > > +* We assume that programs implementing the Boot protocol
> > > +* are also compatible with the Report protocol.
> > > +*/
> >
> > Why is this a safe assumption?
>
> Any device in the Boot subclass supports both Report and Boot protocols by
> definition according to the HID spec. Although the implementations of
> these two procotols maybe the same in some cases, there is a possbility
> that they are different. In this case it would pose a problem to the
> current driver, which offers no switching capabiliy via SET_PROTOCOL while
> in BIOS.
Also, if the assumption turns out to be wrong, there's nothing you can
do about it. :-)
> > > + if(hidg->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
> > > + hidg->protocol_is_report = value;
> > > + goto respond;
> >
> > wrong indentation.
> >
> > > @@ -768,6 +786,7 @@ static int hidg_bind(struct usb_configuration *c,
> > > struct usb_function *f)
> > > /* set descriptor dynamic values */
> > > hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
> > > hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
> > > + hidg->protocol_is_report = 1;
> >
> > no idea why you called this "protocol_is_report" when "protocol"
> > would've been better.
>
> True, that name would probably make it more concise.
I originally called it "protocol_is_report" to improve readability for
things like:
if (protocol_is_report) ...
as opposed to:
if (protocol == 1)
But with new macros for HID_BOOT_PROTOCOL and HID_REPORT_PROTOCOL,
either way would be acceptable.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb:gadget:hid: This patch adds support for set_protocol and get_protocol for remote wakeup
Hi,
Thanks for the reply! I'll be sure to send in another patch
with fixes to all the formatting errors. But before that
just a couple of points on your feedback.
> > struct usb_request *req;
> > unsigned long flags;
> > ssize_t status = -ENOMEM;
> >
> > + usb_gadget_wakeup(cdev->gadget);
>
> this seems unrelated to implemeting GET/SET_PROTOCOL. Why do you need this?
Right, I intially suspected support for these two features
were dependant on one another. However, that does not seem to be the case.
I'll send in a seperate patch to handle remote wake up support in
the driver later.
>
> > @@ -528,6 +532,9 @@ static int hidg_setup(struct usb_function *f,
> > | HID_REQ_GET_PROTOCOL):
> > VDBG(cdev, "get_protocol\n");
> > goto stall;
> > + length = min_t(unsigned, length, 1);
> > + ((u8 *) req->buf)[0]= hidg->protocol_is_report;
> > + goto respond;
> > break;
> >
> > case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> > @@ -539,6 +546,17 @@ static int hidg_setup(struct usb_function *f,
> > case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> > | HID_REQ_SET_PROTOCOL):
> > VDBG(cdev, "set_protocol\n");
> > + if (value > 1)
>
> why 1 here? Seems like this should be
> USB_INTERFACE_PROTOCOL_KEYBOARD. And are you sure there's nobody using
> this to emulate a mouse?
According to 7.2.6 of the HID spec. For the wValue field, 0 indicates Boot
Protocol and 1 indicates Report Protocol, which applies to both mice
and keyboards. So the use of that macro wouldn't make sense.
> > + goto stall;
> > + length = 0;
> > + /*
> > +* We assume that programs implementing the Boot protocol
> > +* are also compatible with the Report protocol.
> > +*/
>
> Why is this a safe assumption?
Any device in the Boot subclass supports both Report and Boot protocols by
definition according to the HID spec. Although the implementations of
these two procotols maybe the same in some cases, there is a possbility
that they are different. In this case it would pose a problem to the
current driver, which offers no switching capabiliy via SET_PROTOCOL while
in BIOS.
>
> > + if(hidg->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
> > + hidg->protocol_is_report = value;
> > + goto respond;
>
> wrong indentation.
>
> > @@ -768,6 +786,7 @@ static int hidg_bind(struct usb_configuration *c,
> > struct usb_function *f)
> > /* set descriptor dynamic values */
> > hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
> > hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
> > + hidg->protocol_is_report = 1;
>
> no idea why you called this "protocol_is_report" when "protocol"
> would've been better.
True, that name would probably make it more concise.
- Abdulhadi Mohamed
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb:gadget:hid: This patch adds support for set_protocol and get_protocol for remote wakeup
Hi,
first of all, your subject line is too long. Something like:
usb: gadget: hid: Add support for {GET,SET}_PROTOCOL
would've been enough
Abdulhadi Mohamed writes:
> This patch is required to implement the set_procotol and get_procotol
> commands
> for HID gadgets to interact with the BIOS using the BOOT protocol according
> to
> the HID specification. These two commands are also required to implement
> remote
> wake up for HID gadgets once the BIOS takes control of the device.
>
> This issue was first raised by Golmer Palmer in May 25 2015 but was never
> followed
> up on. This version of the patch is also heavily based on the patch provided
> by
> Alan Stern with some added support for remote wakeup.
commit log needs to be broken at 72 characters.
> Abdul Mohamed
this is not how you sign a patch, although it's close. It needs to be:
Signed-off-by: Abdul Mohamed
> diff --git a/drivers/usb/gadget/function/f_hid.c
> b/drivers/usb/gadget/function/f_hid.c
> index 5eea448..ea38e36 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -44,6 +44,7 @@ struct f_hidg {
> /* configuration */
> unsigned char bInterfaceSubClass;
> unsigned char bInterfaceProtocol;
> + unsigned char protocol_is_report;
> unsigned short report_desc_length;
> char*report_desc;
> unsigned short report_length;
> @@ -338,10 +339,13 @@ static ssize_t f_hidg_write(struct file *file, const
> char __user *buffer,
> size_t count, loff_t *offp)
> {
> struct f_hidg *hidg = file->private_data;
> + struct usb_composite_dev*cdev = hidg->func.config->cdev;
why the tab here? Every other variable declaration is using one space
only. Just keep it consistent ;-)
> struct usb_request *req;
> unsigned long flags;
> ssize_t status = -ENOMEM;
>
> + usb_gadget_wakeup(cdev->gadget);
this seems unrelated to implemeting GET/SET_PROTOCOL. Why do you need this?
> @@ -528,6 +532,9 @@ static int hidg_setup(struct usb_function *f,
> | HID_REQ_GET_PROTOCOL):
> VDBG(cdev, "get_protocol\n");
> goto stall;
> + length = min_t(unsigned, length, 1);
> + ((u8 *) req->buf)[0]= hidg->protocol_is_report;
> + goto respond;
> break;
>
> case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> @@ -539,6 +546,17 @@ static int hidg_setup(struct usb_function *f,
> case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
> | HID_REQ_SET_PROTOCOL):
> VDBG(cdev, "set_protocol\n");
> + if (value > 1)
why 1 here? Seems like this should be
USB_INTERFACE_PROTOCOL_KEYBOARD. And are you sure there's nobody using
this to emulate a mouse?
> + goto stall;
> + length = 0;
> + /*
> + * We assume that programs implementing the Boot protocol
> + * are also compatible with the Report protocol.
> + */
Why is this a safe assumption?
> + if(hidg->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
> + hidg->protocol_is_report = value;
> + goto respond;
wrong indentation.
> @@ -768,6 +786,7 @@ static int hidg_bind(struct usb_configuration *c, struct
> usb_function *f)
> /* set descriptor dynamic values */
> hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
> hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
> + hidg->protocol_is_report = 1;
no idea why you called this "protocol_is_report" when "protocol"
would've been better.
--
balbi
signature.asc
Description: PGP signature
