by Rodrigo Rivas Costa

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] HID: add driver for Valve Steam Controller

On Tue, Feb 20, 2018 at 09:32:08PM -0800, Cameron Gutman wrote:
> On 02/20/2018 11:33 AM, Rodrigo Rivas Costa wrote:
> > +static void steam_work_connect_cb(struct work_struct *work)
> > +{
> > + struct steam_device *steam = container_of(work, struct steam_device,
> > + work_connect);
> > + unsigned long flags;
> > + bool connected;
> > + int ret;
> > +
> > + spin_lock_irqsave(&steam->lock, flags);
> > + connected = steam->connected;
> > + spin_unlock_irqrestore(&steam->lock, flags);
> > +
> > + if (connected) {
> > + if (steam->input) {
> > + dbg_hid("%s: already connected\n", __func__);
> > + return;
> > + }
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(steam->hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + return;
> > + }
> > + } else {
> > + steam_unregister(steam);
>
> I think you need synchronization here. You don't want to be in the middle of
> processing a HID event or power supply update and have your device freed out
> from underneath you.
>
> xpad uses RCU to avoid this race.

Ah, I see, if we get an input message just after the "disconnect"
packet, (unlikely) it could file. I'll don't know RCU very will but I'll
try and do my best.

Please, stay tuned for v3.
Thanks.
Rodrigo

2018-02-23 08:22:19

by Benjamin Tissoires

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] new driver for Valve Steam Controller

On Thu, Feb 22, 2018 at 6:48 PM, Rodrigo Rivas Costa
<[email protected]> wrote:
> On Thu, Feb 22, 2018 at 06:06:30PM +0100, Benjamin Tissoires wrote:
>> On Thu, Feb 22, 2018 at 5:31 PM, Rodrigo Rivas Costa
>> <[email protected]> wrote:
>> > On Thu, Feb 22, 2018 at 10:10:40AM +0100, Benjamin Tissoires wrote:
>> >> On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais
>> >> <[email protected]> wrote:
>> >> >
>> >> >
>> >> > On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>> >> >>
>> >> >> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
>> >> >>>
>> >> >>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
>> >> >>>>
>> >> >>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
>> >> >>>> About the left trackpad/joystick, currently I'm not treating them
>> >> >>>> different. I'm out of ABS axes, and anyway, it is not likely that the
>> >> >>>> left pad and the joystick be used at the same time (I only have one left
>> >> >>>> thumb). Nevertheless, if we really want to make them apart, we can use
>> >> >>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
>> >> >>>> details in [2], but I'm not currently doing that in this driver.
>> >> >>>
>> >> >>>
>> >> >>> I didn't necessarily mean exposing it, but in the event a user is using
>> >> >>> both
>> >> >>> at the same time (it happens, using claw grip with some games is
>> >> >>> necessary
>> >> >>> to use the D-pad while deflecting the stick), then you'll most likely run
>> >> >>> into issues unless you demux the inbound data, since we were also out of
>> >> >>> left analog fields and mux them together if used at the same time. Not
>> >> >>> sure
>> >> >>> if you're already handling that case, but not doing it would result in
>> >> >>> the
>> >> >>> stick appearing to fully deflect every other input report if the user is
>> >> >>> doing both at the same time.
>> >> >>
>> >> >>
>> >> >> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
>> >> >> right. I think that it will be best to fully separate the left-pad and
>> >> >> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
>> >> >>
>> >> >>>> About the gyroscope/acceleration/quaternion, you know the issue
>> >> >>>> that the wireless gives gyro plus quat but no accel, while the wired
>> >> >>>> gives all three. My general idea is to create an additional input device
>> >> >>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
>> >> >>>> the wireless gives no accel, maybe there is some command to enable it?
>> >> >>>
>> >> >>>
>> >> >>> It should be three neighboring bits for that setting; does this not work
>> >> >>> for
>> >> >>> you?
>> >> >>>
>> >> >>> // Bitmask that define which IMU features to enable.
>> >> >>> typedef enum
>> >> >>> {
>> >> >>> SETTING_GYRO_MODE_OFF = 0x0000,
>> >> >>> SETTING_GYRO_MODE_STEERING = 0x0001,
>> >> >>> SETTING_GYRO_MODE_TILT = 0x0002,
>> >> >>> SETTING_GYRO_MODE_SEND_ORIENTATION = 0x0004,
>> >> >>> SETTING_GYRO_MODE_SEND_RAW_ACCEL = 0x0008,
>> >> >>> SETTING_GYRO_MODE_SEND_RAW_GYRO = 0x0010,
>> >> >>> } SettingGyroMode;
>> >> >>
>> >> >>
>> >> >> Thanks for that, it will be useful! Those are the values to be written
>> >> >> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
>> >> >> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
>> >> >> bit-field...
>> >> >>
>> >> >>> In general I'm concerned about sending of the gyro-enable message
>> >> >>> potentially impairing Steam functionality if it's running at the same
>> >> >>> time
>> >> >>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
>> >> >>> sending any stateful messages to the device. For instance, are you ever
>> >> >>> sending the "blank configuration" setting? I assume you must be or users
>> >> >>> would still get keyboard/mouse input over these USB endpoints while
>> >> >>> trying
>> >> >>> to interact with the controller. But sending this after Steam programmed
>> >> >>> a
>> >> >>> setting, or instructed the controller to go back to lizard mode because
>> >> >>> it
>> >> >>> was requested by a configuration, would be problematic.
>> >> >>
>> >> >>
>> >> >> Sure, but this patchset should be safe, shouldn't it?
>> >> >> Maube future patches that play with the gyro/force-feedback could be
>> >> >> disabled by default, and could need a module parameter to be enabled.
>> >> >> That way Steam Client will work out-of-the-box anywhere but proficient
>> >> >> users could use the extra functionality easily.
>> >> >>
>> >> >> Related to that, Benjamin Tissoires replied to 1/3:
>> >> >>>>
>> >> >>>> --- a/drivers/hid/hid-quirks.c
>> >> >>>> +++ b/drivers/hid/hid-quirks.c
>> >> >>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
>> >> >>>> hid_have_special_driver[] = {
>> >> >>>> #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>> >> >>>> { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
>> >> >>>> USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>> >> >>>> #endif
>> >> >>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
>> >> >>>> + { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>> >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
>> >> >>>> + { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>> >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
>> >> >>>> +#endif
>> >> >>>
>> >> >>>
>> >> >>> In addition to the discussion in 0/3, I wonder if you should not
>> >> >>> remove this hunk. Unless having hid-generic binding the device before
>> >> >>> your hid-steam driver, it would be better not force the Steam boxes to
>> >> >>> use your driver.
>> >> >>
>> >> >>
>> >> >> I don't really see the point on that. If we do that I'll have to unbind
>> >> >> and bind the device manually, and that is a pain, and impossible without
>> >> >> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
>> >>
>> >> I guess you are not running v4.16-rc2 :) (see Clement answers too)
>> >>
>> >> Since v4.16, there is no need to absolutely blacklist the devices in
>> >> this list. hid-generic will bind them first, but as soon as an other
>> >> driver claims the device, hid-generic unbinds itself and let the other
>> >> driver to handle the device. Without any user input!
>> >
>> > No, I'm still in v4.15.3, and I thought I was bleeding edge...
>> > I've seen those patches in the hid git tree, but I thought it was
>> > experimental. Good to know it is upstream, that quirks array was
>> > weird... I'll try to upgrade and see what happens.
>> >>
>> >> >> And anyway this driver is mostly harmless, the only visible changes from
>> >> >> userspace are the new input and power devices, and that the virtual
>> >> >> keyboard/mouse are no more.
>> >>
>> >> Pierre-Loup mentioned that sometime the Steam client needs those
>> >> interfaces for the Desktop mode. I have the feeling things are going
>> >> to be too intricated for us to gracefully accept the driver unless
>> >> there is a clear ACK from Valve that they are happy with the new
>> >> driver.
>> >>
>> >> >> If the virtual devices are really missed we
>> >> >> could use:
>> >> >>
>> >> >> hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> >> >>
>> >> >> on them, maybe with a module parameter? Note that one of the first
>> >> >> things the Steam Client does is to disable the virtual devices (with a
>> >> >> command to the device).
>> >> >> (TBH I always had the impression that those virtual devices are there
>> >> >> to move the Grub menu...)
>> >> >>
>> >> >> If Valve people really feel that this driver is not needed for SteamOS,
>> >> >> because the Steam Client is always running, then SteamOS can always do
>> >> >> CONFIG_HID_STEAM=n.
>> >>
>> >> That is what I was expecting, but if the driver also breaks the
>> >> regular Steam client, we have a situation :)
>> >>
>> >> >
>> >> >
>> >> > Yes, certainly; that isn't really the usecase I'm worried about. What I'm
>> >> > worried about is behavior changing for existing users of the controller on
>> >> > normal desktop distributions. Currently the Steam client will program these
>> >> > pieces of state (enable/disable direct keyboard/mouse HID functionality,
>> >> > enable/disable gyro/accel) based on the user's configuration, and a user
>> >> > getting a kernel update that includes a driver that also programs that
>> >> > without their intervention is bound to affect the behavior. If there was a
>> >> > way to make it so the states won't be programmed until it's pretty clear the
>> >> > user is trying to use that driver's functionality, that would feel safer.
>> >>
>> >> I do not think there is a way to know beforehand, given that the
>> >> kernel module will be loaded first, and sometimes even without the
>> >> root file system if the driver has been included in the initramfs
>> >> (which is the point of not having the device blacklisted in
>> >> hid-quirks.c)
>> >>
>> >> I guess the only reasonable solution would be for the kernel HID
>> >> driver to expose all the interfaces (keyboard, touchpad, gyro, etc...)
>> >> but not take any action on whether which mode it switches into. Then,
>> >> (and unfortunatelly) though a custom sysfs API, we could teach SDL or
>> >> any other configurator to change the device into the desired mode.
>> >> I would prefer not having a custom sysfs API, but this would allow us
>> >> to change the owner to a non-root user while hidraw needs to have the
>> >> root access.
>> >>
>> >> An other solution than the sysfs API, would be to have a small driver
>> >> in libratbag (or a similar daemon) that sends the mode switch command
>> >> as if the joystick where a special gaming mouse.
>> >
>> > Ok, I agree that we cannot remove the keyboard/mouse interfaces. What
>> > about a module parameter? Something like `disable_lizard_mode` or
>> > `disable_hid_devices`. That would be 0 by default, so no user-mode
>> > breakage. BTW, are module parameters considered userspace API stable?
>>
>> There are 2 issues with parameters modules:
>> - yes, it's kernel API (if you change it and one user in the South
>> Pole has it in the grub config and it now breaks the device, it will
>> be considered as a regression)
>> - they are usually taken into account at loading time, and you need
>> root to change them
>
> Well, that parameter would be using at probe time. So you will need to
> unplug and replug the controller to take effect.

And this is not an acceptable solution for end users :)
Note that you can also get notified when a parameter changes while the
module is loaded and a device is already bound. But this is the same
than using sysfs with all the cons we already discussed about.

>
>> >
>> > Note that when this parameter is set, the driver will not send any
>> > command to the controller, (as Steam Client does). Instead, it will
>> > simply not call hid_hw_start() on those interfaces. I'm not sure how the
>> > no-quirks hid-generic will behave in this case... I'll try and see...
>> >
>> > In the future, we could add a few other modules paramteres to enable the
>> > gyro and the force-feedback, as these functions could very well break
>> > Steam. But these functions will be welcomed by DIY enthusiasts.
>>
>> The more I think of it, the more I think you need to split the "driver" in 2:
>> - the kernel part that will be able to understand the raw values from
>> the device and inject the correct events in the correct input nodes
>> - the config part that will control how the device behaves. This
>> config part is the one interfering with Steam, so you might simply
>> want to have a standalone tool (or in libratbag, and integrate it in
>> SDL) to send the commands to switch the device into the desired mode.
>>
>> Note that we all tried to use custom sysfs API for configuring our
>> fancy input devices, and we all came down to the conclusion that it
>> was better to leave the kernel to the minimum and have external tool
>> to talk to the hardware for the config. This way, you can break the
>> API as you want (it's internal to your tool), and you can also adapt
>> much quicker to new devices.
>
> Nice idea. If we are relying in user-mode external tools, no need for
> sysfs, we already can send commands using hidraw. Maybe I can write a
> simple command-line tool to do that. Would adding a link to my github
> page with that tool be considered too much self-promotion?

Please go ahead. It's not like we are writing the things in stone, and
on top of that, you are the author of the driver. 2 reasons to not
consider this as self-promotion :)

>
> The future situation with the gyro will be trikier, though. Because the
> driver should send the enable/disable gyro commands when the
> corresponding input-gyro device is opened/closed. And that cannot be
> done with a user-mode tool.

But it can be done directly in the kernel. If user space opens the
gyro, you are called in the .open() callback and you can trigger the
correct modes.
One other solution is to use IIO, but I do not think we like this for
gaming devices.

>
> But one problem at a time... I'll modify the driver to leave the lizard
> mode alone, and see what you all think, ok?

Sounds good.

Cheers,
Benjamin