On Mon, Jan 28, 2019 at 8:28 AM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> Hi Roderick,
>
> On Mon, Jan 28, 2019 at 6:11 AM Roderick Colenbrander
> <thunderbird2k@xxxxxxxxx> wrote:
> >
> > Hi Daniel,
> >
> > Thanks for updating the patch series. It is definitely improved and
> > easier to read. See some of my comments below.
> >
> > One other thing I noticed, see the comment in v2 review about breaking
> > userspace. I think since you change the button / stick mapping
> > compared to what SDL2 supports for Pro controller. You need to do
> > something e.g. different version like hid-sony does.
> >
> > Out of curiosity did you test the driver on Bluetooth? The reason I'm
> > asking is, because I suspect you would have hit a deadlock issue,
> > which we hit in hid-sony before. I'm not sure how you got around that
> > one..
> >
> > Let me explain the issue. When using Bluetooth, many Bluetooth stacks
> > rely on uhid (either Bluez on desktop and even Android's one). I
> > recall when the BT stack requests uhid to "create" an input device,
> > this ultimately triggers "_probe" in your driver. hid-sony at the time
> > and similar for hid-switchcon triggers various requests to the
> > hardware during probe. However uhid is single threaded and it is
> > holding a lock. This caused a deadlock and caused hardware request
> > timeouts. I don't see how this is not happening for you.
>
> Actually, I think you are missing `hid_device_io_start(hdev);` in hid-sony.c
>
> What this driver does and which is race free (in pseudo code):
> probe() {
> hid_parse();
> hid_hw_start(CONNECT_HIDRAW);
> hid_hw_open(hdev);
> hid_device_io_start(hdev);
> // now you can do whatever IO you want, .raw_event() will also be called
> hid_hw_close(hdev);
> switchcon_input_create(ctlr);
> }
>
> If you want to rely on HIDINPUT to create the inputs for you and not
> prematurely expose a hidraw node, then you can first hid_hwstart with
> CONNECT_DRIVER, do your init, then stop the hid device and finally
> start with CONNECT_DEFAULT.
>
> Cheers,
> Benjamin
I dug through my old notes and commit logs again. There were 2
unrelated issues regarding similar code, which had blurred together.
The "uhid" issue I mentioned was indeed resolved by the fix I added at
the time and I/O start related calls helped there. hid-sony probably
could use a hid_device_io_start as you pointed out.
The second issue which there was, was the one where in which
"hid_hw_start" was taking care of input node creation on
CONNECT_DEFAULT. At the time "sony_probe" was doing some remaining
work (creating sysfs options, adding FF..) after hid_hw_start, so this
caused a race condition. This is why we added "sony_input_configured"
as it is called during hid_hw_start. It all starts making sense again.
On a related node for switchcon, what is preferred for device node
creation? Right now switchcon does its own input_device_create. Other
drivers often override the HID mapping through hid callbacks if they
don't like the default mappings. I'm not sure what other benefits
there would be from using functionality from the linux hid layer.
Thanks,
Roderick
