interference with hid-microsoft on kernel 4.20? about xpadneo HOT 47 CLOSED

I don't know exactly but at least we know that we cannot use it in a reliable way

from xpadneo.

Maybe bind is a thing only with dynamically loaded modules... I did compile hid modules into my kernel here. It reduces the chance that modules are missing in initramfs - which can be a major headache... ;-)

from xpadneo.

dkms does rebuild the initramfs too ;)

Until it doesn't... Murphy's law... you know... ;-)

from xpadneo.

Apparently, kernel 4.20 has some problems with my system currently. I need to wait a little more. But it should not conflict with each other unless you do the driver binding wrong. I don't use the udev rules, instead I've removed the VID/PID from the other driver via kernel patch and compiled xpadneo statically into the kernel:
https://github.com/kakra/linux/commits/rebase-4.20/xpadneo

Porting to 4.20 went without conflicts - as did previous kernel versions. So I'm confident it should just work. I cannot speak for the udev rules, tho.

BTW: Why 4.20 RC? 4.20 is already final...

from xpadneo.

BTW: Why 4.20 RC? 4.20 is already final...

Oh right, I missed the release somehow... I tested it and it still works on my system, but:

The problem is that, using DKMS, we now have two specialized drivers which support the Xbox One Wireless Gamepad (hid-microsoft and hid-xpadneo). Since v 4.16 the kernel does automatically load the specialized driver if any is registered - but what happens if more than one is registered?

Somehow hid-microsoft does only yet support the 0x02fd version, that is maybe one of reasons why it still works here.

Btw, I forgot that you don't use DKMS 😁

from xpadneo.

We may need to do the same as the hid_generic does:

Whenever a new driver is attached, hid_generic checks if it fits any of the available devices hid_generic currently handles - if it finds any, then it releases the them and the new driver can bind to it:

static int __unmap_hid_generic(struct device *dev, void *data)
{
	struct hid_driver *hdrv = data;
	struct hid_device *hdev = to_hid_device(dev);

	/* only unbind matching devices already bound to hid-generic */
	if (hdev->driver != &hid_generic ||
	    hid_match_device(hdev, hdrv) == NULL)
		return 0;

	if (dev->parent)	/* Needed for USB */
		device_lock(dev->parent);
	device_release_driver(dev);
	if (dev->parent)
		device_unlock(dev->parent);

	return 0;
}

static void hid_generic_add_driver(struct hid_driver *hdrv)
{
	bus_for_each_dev(&hid_bus_type, NULL, hdrv, __unmap_hid_generic);
}

The problem is, that we cannot do anything if hid_xpadneo is not the one which is loaded first... which we cannot guarantee...

In the end we can still use the udev rule to unbind from hid-microsoft too

edit: Doesn't some devices have more than one driver active at the same time? I think I have seen something like that before for some hid devices...

edit2: looks like they have simplified the hid_generic binding process to:

static bool hid_generic_match(struct hid_device *hdev,
			      bool ignore_special_driver)
{
	if (ignore_special_driver)
		return true;

	if (hdev->quirks & HID_QUIRK_HAVE_SPECIAL_DRIVER)
		return false;

	/*
	 * If any other driver wants the device, leave the device to this other
	 * driver.
	 */
	if (bus_for_each_drv(&hid_bus_type, NULL, hdev, __check_hid_generic))
		return false;

	return true;
}

from xpadneo.

Hum, I did some tests and it doesn't look good for the DKMS approach without an udev rule:
I installed two instances of xpadneo: hid-xpadneo1 and hid-xpadneo2

Obviously both drivers do match the gamepad, hence both drivers get loaded by udev when the gamepad appears at the hid bus.
The problem now is that only one the functions of one of both drivers are really called - which means nothing but: if hid-microsoft and hid-xpadneo are both claiming the same device-ids, then only the functions of one (the one which is registered first) are executed.

If the first one is hid-microsoft and if it is a gamepad with pid 0x2FD, then the only way to bind the gamepad to xpadneo is maybe a new UDEV rule... any other ideas (except recompiling hid-microsoft)?

from xpadneo.

Maybe the udev rules for xpadneo needs to be ordered before any hid-microsoft rule. Rules in udev should be processed in order of their lexical naming, that is they are prefixed with double-digit values. It may need a stop marker to instruct udev to process other rules. OTOH, udev rules may override their values in order of processing. So it could also be ordered after the other rule to override driver binding. Maybe it needs to be split into two rules: One defining the driver, the other doing the binding.

Since I couldn't use 4.20 yet, I still need to look into how this works out with the new driver bindings in the kernel. I may need to amend the patch to also remove claiming the IDs in the other driver now.

Why couldn't Valve just use xpadneo instead of putting more stuff into the drivers which already mess up button order a lot (which is then corrected outside of the kernel by patches to SDL). That will never work out well because some driver paths (the old joystick API) would never see correct button/axis order then. Well, the problem in the first place is probably that SDL folks corrected it in the user-land instead of putting a quirks-fix into the kernel correcting it there. Life would be easier then now.

from xpadneo.

the only solution I could find so far is, as already mentioned, another udev rule. I added it accordingly - that's a dirty fix in my eyes since I am not a fan of udev rules but at least it works.

from xpadneo.

Maybe we should enhance the kernel itself with some kind of priority flag... Would be the cleanest solution

from xpadneo.

Well, the "priority" would be to flag each driver a specialized driver, a generic driver, or a fallback driver. hid-generic would be fallback then, hid-microsoft would be generic, and finally drivers handling a subset of devices only (as xpadneo does) would be the specialized drivers, in that order with raising priority.

from xpadneo.

related: #72

from xpadneo.

I don't know yet if this would work but wouldn't it be possible to use the following in modprobes xpadneo.conf

remove hid_microsoft
remove hid_generic
install hid_microsoft
install hid_generic

This way xpadneo would be the first one in the list of registered drivers and therefore loaded whenever a fitting device appears.

I am just unsure if remove works if there are active devices for hid_generic for example.

from xpadneo.

sadly the approach above doesn't work because hid_generic is quite often builtin and therefore you cannot unload it.

It looks like editing modules.order (see here) would be the right way to go, unfortunately this wouldn't work either if hid-microsoft is builtin I think?!

from xpadneo.

Yes, the most promising way would be to adjust the driver before udev does the binding. Removing hid drivers would also disconnect input devices during the process and may lead to lost input events during connection of the gamepad - this would be surprising at best but most probably very annoying. I'd recommend against that. Especially after a kernel rebuild you may end up with an unloaded module that cannot be inserted again afterwards until you rebooted.

Also, module reordering may not work because modules also have dependencies which may force specific load orders.

from xpadneo.

What do you mean by

Yes, the most promising way would be to adjust the driver before udev does the binding.

from xpadneo.

Udev rules allow to set DRIVER= early in the rule chain which finally defines the binding - according to the man-page. But you need to catch this early before udev walks up the parent device chain, because the device may have been bound by then. Using udevadm monitor and matching the first device node listed with a udev rule to set the driver should do the trick. I cannot properly test this here because my kernel is patched to exclude the conflicting device ids from the other "matching" drivers.

PS: Using a final assignment DRIVER:= may prevent later rules from resetting the driver to something else.

from xpadneo.

Well this is nearly what we are doing at the moment, right?

ACTION=="add", KERNEL=="0005:045E:02FD.*|0005:045E:02E0.*", SUBSYSTEM=="hid",\
RUN+="/bin/bash -c 'echo $kernel > /sys/bus/hid/drivers/hid-generic/unbind'", \
RUN+="/bin/bash -c 'echo $kernel > /sys/bus/hid/drivers/xpadneo/bind'"

ACTION=="add", KERNEL=="0005:045E:02FD.*|0005:045E:02E0.*", SUBSYSTEM=="hid",\
RUN+="/bin/bash -c 'echo $kernel > /sys/bus/hid/drivers/hid-microsoft/unbind'", \
RUN+="/bin/bash -c 'echo $kernel > /sys/bus/hid/drivers/xpadneo/bind'"

Previously we had a rule that contained the DRIVER= thingy but I changed it to the curent one because it didn't worked on Raspbian for whatever reason.

from xpadneo.

PS: Using a final assignment DRIVER:= may prevent later rules from resetting the driver to something else.

Ah, that's interesting - I will give it a try

from xpadneo.

https://github.com/atar-axis/xpadneo/tree/a2755d7244ffe7b36d0cf36a02fd8fe1c84384f5/src/udev_rules

from xpadneo.

Yes, those rules work but I kinda do not like the fact to bind a different driver first, then run commands to unbind and rebind the device. This seems wrong somehow and feels like a work-around. Also, hard-coding bash usage seems fishy: Some distributions do not install bash by default. Using the /bin/sh symlink seems more correct.

from xpadneo.

Absolutely, that's why it is flagged as a dirty fix 😁 I will dig a bit deeper, I was quite new to linux when I wrote the udev rule

from xpadneo.

https://github.com/atar-axis/xpadneo/tree/a2755d7244ffe7b36d0cf36a02fd8fe1c84384f5/src/udev_rules

I'm not sure if ACTION=="bind" is really a thing. I found this in a forum but udev does not document this action. It instead documents that you can set DRIVER during add but only early in the rule chain, because otherwise it can no longer have an effect because binding is done at that point.

from xpadneo.

bind is absolutely an action that appears in udevadm on my system, and therefore the rule works there - but bind is also exactly the thing that is missing when you examine it at Raspbian

from xpadneo.

bind is absolutely something that appears in udevadm on my system, and therefore the rule works there - but bind is also exactly the thing that is missing when you examine it at Raspbian

Difference in the udev implementation? Maybe systemd-udev vs eudev, or different versions?

PS: I do not see bind here, too. This is systemd-udev-240.

from xpadneo.

KERNEL[3502.705886] bind     /devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:045E:02E0.0004 (hid)
UDEV  [3502.713605] bind     /devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/bluetooth/hci0/hci0:256/0005:045E:02E0.0004 (hid)

from xpadneo.

Hum, that's a damn good idea and I think that's the solution since hid-generic was builtin on the Raspbian where I tried it.

from xpadneo.

dkms does rebuild the initramfs too ;) but yes, I had a hard time to learn that 😁

from xpadneo.

PS: Using a final assignment DRIVER:= may prevent later rules from resetting the driver to something else.

Doesn't look like DRIVER is a key that accepts an assignment, unfortunately

Furthermore, I think that the device is not bound to e.g. 'hid-generic' in the first instance but to the bus master 'hid' which then searches for a fitting driver in its list of registered drivers. The driver which was registered as first is served first.

from xpadneo.

This is where bind and unbind uevents where introduced (kernel 4.14): torvalds/linux@1455cf8

from xpadneo.

Just for documentation, this is how currently binding is done...

 * device_attach - try to attach device to a driver.
 * @dev: device.
 *
 * Walk the list of drivers that the bus has and call
 * driver_probe_device() for each pair. If a compatible
 * pair is found, break out and return.
 *
 * Returns 1 if the device was bound to a driver;
 * 0 if no matching driver was found;
 * -ENODEV if the device is not registered.

device_attach() (here) calls __device_attach() (here) which does check if the device already has a driver information (dev->driver), if so it calls device_bind_driver() (see 1.), if not it calls bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver) (see 2.)

1. (device_bind_driver()) actually binds the device to the driver by calling driver_sysfs_add() here, then it calls driver_bound() (here) where finally the 'bind' uevent is generated (here) which you can see via udevadm monitor.

2. bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver) (defined here) does nothing else but calling __device_attach_driver() (here) for each driver until the first one is found that returns '1'.

   In __device_attach_driver() then driver_match_device(drv, dev) and, if they match, driver_probe_device(drv, dev) is called. driver_probe_device() attempts to bind the device & driver together - to do that it calls really_probe() (defined here. really_probe() does a lot of things to bind device & driver together, it does e.g. call the drivers and busses probe() functions. When everything went fine and both do fit eachother, then dev->driver is set to the fitting driver which was registered at the bus, then the device is bound to the driver by calling driver_sysfs_add() and driver_bound()` is called as above.

What we learn is that really the first fitting driver is the one which is bound to the device. Next step would be to examine how the order is.

from xpadneo.

damn it, wrong line numbers... bootlin/elixir#46

from xpadneo.

the drivers are registered at the bus using bus_add_driver() which is called by driver_register() which is called by __hid_register_driver() which is another name for hid_register_driver() which is called in xpadneo here.

That means that it is indeed as we assumed, the driver which is registered first at the bus is the one which is served when a device is plugged in which fits. Furthermore that means that binding has nothing to do with udev and builtin drivers are always registered earlier than loadable modules.

from xpadneo.

Some information is also available here: https://github.com/torvalds/linux/blob/master/Documentation/driver-model/binding.txt

from xpadneo.

I've updated my kernel branch: https://github.com/kakra/linux/commits/rebase-4.20/xpadneo

It includes a commit "xpadneo: Exclude xpadneo devices from hid-microsoft" to remove the interference with the new hid-microsoft driver, otherwise you'll see unsupported buttons (the battery indicator still works, tho).

from xpadneo.

otherwise you'll see unsupported buttons (the battery indicator still works, tho).

Sounds strange to me, really. It's not possible to use features of both drivers - so how does the battery indicator work in that case?
Have they fixed it?

from xpadneo.

It looks like the driver only implements a "rumble quirk", leaving the rest to the standard hid driver. dmesg states that xpadneo still gets activated when turning the gamepad on. But rumbling is already stolen away from xpadneo. I can only guess that hid input handling is also "stolen" by passing it to hid-generic. But xpadneo may still be used to read the battery. Does this make sense?

But if this is the case, xpadneo could try to hook the default drivers in a similar way, implementing just battery reading and button mapping. Tho, I'd like to see that we could make anything useful out of the trigger rumble. Currently, no game I tried can use it.

from xpadneo.

Hum,... according to my research I would say that only one driver is getting bounded to a device, hence xpadneo or hid-microsoft, but never both.
They are for sure both loaded, since they both have the same modalias.

But your observation is clearly contrary to that, so I think I may have to rethink that part...

Regarding the Trigger Feedback: Absolutely! But I think no game will ever use it as long as there is no native support in the kernel for that kind of rumble/ff. But I am always open to any kind of suggestions.

from xpadneo.

Maybe ask the kernel input experts for any trigger rumble proposals? I think with the Elite controller, there's a second device now supporting this kind of stuff...

from xpadneo.

Hum,... according to my research I would say that only one driver is getting bounded to a device, hence xpadneo or hid-microsoft, but never both.
They are for sure both loaded, since they both have the same modalias.

But your observation is clearly contrary to that, so I think I may have to rethink that part...

Maybe it's possible to pass events to an "upstream" driver so you could implement the special functions in one driver and register there but pass any non-handled events to another driver... I wonder how the ff-hook of 4.20 does it for the Xbox One S controller... The comment in the commit says "this only implements the ff-quirk, the rest is handled by the generic hid driver" (or something along that line).

from xpadneo.

Hum, right that's a good point - FF is often handled separately... hum

Maybe ask the kernel input experts for any trigger rumble proposals? I think with the Elite controller, there's a second device now supporting this kind of stuff...

Yeah would definitely be the way to go,.. even if I would love if there would be any other way - hadn't had any good experiences yet with questions in the LKML, never got an useful response.

from xpadneo.

Ah, I remember now - it's like that:
Every Input event that is not handled directly is handled by hid-core, thus it is still a single driver which is responsible for the device, but those drivers do only intervene if necessary.

That's why hid-generic is so small, it does nearly nothing - leaving all the work to hid-core. I could in fact reduce the size of xpadneo too, there is a lot of code which isn't exactly necessary ;)

from xpadneo.

So should I downgrade to 4.19 to use my xbox one controllers atm?

from xpadneo.

@Hedronmx If you're compiling your own kernel, you can use my patchset for 4.20 which integrates xpadneo natively into the kernel (no DKMS and no udev rules required):

kakra/linux@master...kakra:rebase-4.20/xpadneo

or as a single patch download:

https://github.com/kakra/linux/compare/master...kakra:rebase-4.20/xpadneo.patch

from xpadneo.

@Hedronmx no, everything ist good! Don't worry. The Driver does Install a Set of udev rules which do the job

from xpadneo.

That's why hid-generic is so small, it does nearly nothing - leaving all the work to hid-core. I could in fact reduce the size of xpadneo too, there is a lot of code which isn't exactly necessary ;)

@atar-axis Cutting down the driver code to re-use existing kernel infrastructure may benefit the process of getting this driver into the kernel...

from xpadneo.

yeah, "a lot of code" was maybe a bit exaggerated... it's just the mapping in case the right descriptor is sent which is not exactly necessary, but it's much easier to maintain the way it is at the moment.

anyway, your observation is still mystic to me - two features from two different drivers. I don't really have any clue how that should work

from xpadneo.