netdev - Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

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]

Date:	Fri, 09 Jul 2010 14:33:35 -0300
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Alan Ott <alan@...nal11.us>
Cc:	David S Miller <davem@...emloft.net>,
	Jiri Kosina <jkosina@...e.cz>,
	Michael Poole <mdpoole@...ilus.org>,
	Bastien Nocera <hadess@...ess.net>,
	Eric Dumazet <eric.dumazet@...il.com>,
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw   
 HIDIOCGFEATURE    and HIDIOCSFEATURE

Hi Alan,

> >>>>> I looked at this and I am bit worried that this should not be done in
> >>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
> >>>>> driver. It should not handle all these details. Can we make this a bit
> >>>>> easier for the transport drivers to support such features?
> >>>>>
> >>>>>            
> >>>> I put these changes (most notably the addition of hidp_get_raw_report())
> >>>> in hidp because that's where the parallel function
> >>>> hidp_output_raw_report() was already located. I figured the input should
> >>>> go with the output. That said, if there's a better place for both of
> >>>> them (input and output) to go, let me know where you think it should be,
> >>>> and I'll get them moved into the proper spot.
> >>>>
> >>>> I'm not sure what you mean about HIDP being a pure transport driver.
> >>>>
> >>>>          
> >>> what is usb-hid.ko doing here? I would expect a bunch of code
> >>> duplication with minor difference between USB and Bluetooth.
> >>>        
> >> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
> >>       usbhid_output_raw_report()
> >>           - calls usb_control_msg() with Get_Report
> >>       usbhid_get_raw_report()
> >>           - calls usb_control_msg() with Set_Report
> >>               OR
> >>           - calls usb_interrupt_msg() on the Ouput pipe.
> >>
> >> This is of course easier than bluetooth because usb_control_msg() is
> >> synchronous, even when requesting reports, mostly because of the nature
> >> of USB, where the request and response are part of the same transfer.
> >>
> >> For Bluetooth, it's a bit more complicated since the kernel treats it
> >> more like a networking interface (and indeed it is). My understanding is
> >> that to make a synchronous transfer in bluetooth, one must:
> >>       - send the request packet
> >>       - block (wait_event_*())
> >>       - when the response is received in the input handler, wake_up_*().
> >>
> >> There's not really any code duplication, mostly because initiating
> >> synchronous USB transfers (input and output) is easy (because of the
> >> usb_*_msg() functions), while making synchronous Bluetooth transfers
> >> must be done manually. If there's a nice, convenient, synchronous
> >> function in Bluetooth similar to usb_control_msg() that I've missed,
> >> then let me know, as it would simplify this whole thing.
> >>      
> > there is not and I don't think we ever get one. My question here was
> > more in the direction why HID core is doing these synchronously in the
> > first place. Especially since USB can do everything async as well.
>
> I'm open to suggestions. The way I see it is from a user space 
> perspective. With Get_Feature being on an ioctl(), I don't see any clean 
> way to do it other than synchronously. Other operating systems (I can 
> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the 
> same way (synchronously) from user space.
> 
> You seem to be proposing an asynchronous interface. What would that look 
> like from user space?

not necessarily from user space, but at least from HID core to HIDP and
usb-hid transports. At least that is what I would expect, Jiri?

Regards

Marcel


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html