Re: [PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jul 28, 2022 at 11:07:20AM -0700, Vicki Pfau wrote:
> 
> 
> On 7/28/22 01:08, Greg Kroah-Hartman wrote:
> > On Mon, Jul 25, 2022 at 05:58:25PM -0700, Vicki Pfau wrote:
> >> --- /dev/null
> >> +++ b/include/uapi/linux/usb/g_hid.h
> >> @@ -0,0 +1,38 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> >> +/*
> >> + * g_hid.h -- Header file for USB HID gadget driver
> >> + *
> >> + * Copyright (C) 2022 Valve Software
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, write to the Free Software
> >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> > 
> > This whole license "boilerplate" is not needed, and should be removed
> > (especially things like addresses, that's crazy).
> > 
> > Only thing that is needed is the SPDX line.
> 
> I was just copying the header from g_printer.h and changing as needed. It hasn't been touched since 2017, so if that's no longer the convention you might want to update it too.

Good point, I will look into that.

> >> + */
> >> +
> >> +#ifndef __UAPI_LINUX_USB_G_HID_H
> >> +#define __UAPI_LINUX_USB_G_HID_H
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +struct usb_hidg_report {
> >> +	__u16 length;
> >> +	__u8 data[512];
> > 
> > Why 512?
> 
> I was reading the specs and one of them said the maximum report length for USB 3 (I believe) was 512 bytes (in contrast with USB 2's 64). I can try to find where it said this, or add a define for max report length.

Either is fine, magic values like this should always be documented
somehow.

> >> +};
> >> +
> >> +/* The 'g' code is also used by gadgetfs and hid gadget ioctl requests.
> >> + * Don't add any colliding codes to either driver, and keep
> >> + * them in unique ranges (size 0x20 for now).
> >> + */
> >> +#define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)
> > 
> > This should be in the same .h file so that we don't get confused and
> > accidentally use the same ioctl.
> 
> The same .h file as which? g_printer.h and gadgetfs.h are separate files.

Whatever the uapi .h file is, should have the list of ioctls, not
scattered around the core kernel files.

> 
> > 
> >> +
> >> +#endif /* __UAPI_LINUX_USB_G_HID_H */
> >> diff --git a/include/uapi/linux/usb/gadgetfs.h b/include/uapi/linux/usb/gadgetfs.h
> >> index 835473910a49..9754822b2a40 100644
> >> --- a/include/uapi/linux/usb/gadgetfs.h
> >> +++ b/include/uapi/linux/usb/gadgetfs.h
> >> @@ -62,7 +62,7 @@ struct usb_gadgetfs_event {
> >>  };
> >>  
> >>  
> >> -/* The 'g' code is also used by printer gadget ioctl requests.
> >> +/* The 'g' code is also used by printer and hid gadget ioctl requests.
> > 
> > Yeah, put the definition here.
> 
> Should I move the g_printer.h definitions here at the same time? Maybe stub out g_printer.h and make it include gadgetfs.h?

Move them in the first patch in the series would be great.  And no need
for stubbed out .h file, that's not needed for internal .h files.

thanks,

greg k-h