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]

Message-ID: <alpine.LNX.2.00.1007221612450.4741@pobox.suse.cz>
Date:	Thu, 22 Jul 2010 16:14:57 +0200 (CEST)
From:	Jiri Kosina <jkosina@...e.cz>
To:	Marcel Holtmann <marcel@...tmann.org>
Cc:	Alan Ott <alan@...nal11.us>, David S Miller <davem@...emloft.net>,
	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

On Fri, 9 Jul 2010, Marcel Holtmann wrote:

> > >>> 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?

Sorry for this taking too long (vacations, conferences, you name it) for 
me to respond.

As all the _raw() callbacks are purely intended for userspace interaction 
anyway, it's perfectly fine (and in fact desirable) for the low-level 
transport drivers to perform these operations synchronously (and that's 
what USB implementation does as well).

Marcel, if your opposition to synchronous interface is strong, we'll have 
to think about other aproaches, but from my POV, the patch is fine as-is 
for Bluetooth.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
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