Re: [PATCH v7 1/2] HID: add driver for Valve Steam Controller

From: Rodrigo Rivas Costa
Date: Wed Mar 28 2018 - 17:00:55 EST

Next message: tip-bot for Peter Zijlstra: "[tip:locking/core] locking/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()"
Previous message: Thomas Gleixner: "[patch 4/8] rslib: Remove GPL boilerplate"
In reply to: Benjamin Tissoires: "Re: [PATCH v7 1/2] HID: add driver for Valve Steam Controller"
Next in thread: Benjamin Tissoires: "Re: [PATCH v7 0/2] hid-steam driver with user mode client dection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Mar 26, 2018 at 11:20:30AM +0200, Benjamin Tissoires wrote:
> Hi Rodrigo,
>
> few comments inlined.
>
> On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@xxxxxxxxx> wrote:
> > There are two ways to connect the Steam Controller: directly to the USB
> > or with the USB wireless adapter. Both methods are similar, but the
> > wireless adapter can connect up to 4 devices at the same time.
> >
> > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > keyboard and a custom HID device.
> >
> > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > 4 custom HID devices, that will remain silent until a device is actually
> > connected.
> >
> > The custom HID device has a report descriptor with all vendor specific
> > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > Steam Client provices a software translation by using hidraw and a
> > creates a uinput virtual gamepad and XTest keyboard/mouse.
> >
> > This driver intercepts the hidraw usage, so it can get out of the way
> > when the Steam Client is in use.
> >
> > Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@xxxxxxxxx>
> > ---
> > drivers/hid/Kconfig | 8 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 4 +
> > drivers/hid/hid-steam.c | 840 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/hid.h | 1 +
> > 5 files changed, 854 insertions(+)
> > create mode 100644 drivers/hid/hid-steam.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 779c5ae47f36..de5f4849bfe4 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -811,6 +811,14 @@ config HID_SPEEDLINK
> > ---help---
> > Support for Speedlink Vicious and Divine Cezanne mouse.
> >
> > +config HID_STEAM
> > + tristate "Steam Controller support"
> > + depends on HID
> > + ---help---
> > + Say Y here if you have a Steam Controller if you want to use it
> > + without running the Steam Client. It supports both the wired and
> > + the wireless adaptor.
> > +
> > config HID_STEELSERIES
> > tristate "Steelseries SRW-S1 steering wheel support"
> > depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 235bd2a7b333..e146c257285a 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -94,6 +94,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
> > obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
> > obj-$(CONFIG_HID_SONY) += hid-sony.o
> > obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
> > +obj-$(CONFIG_HID_STEAM) += hid-steam.o
> > obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
> > obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
> > obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index a0baa5ba5b84..3014991e5d4b 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -987,6 +987,10 @@
> > #define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
> > #define USB_DEVICE_ID_MTP_SITRONIX 0x5001
> >
> > +#define USB_VENDOR_ID_VALVE 0x28de
> > +#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
> > +#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
> > +
> > #define USB_VENDOR_ID_STEELSERIES 0x1038
> > #define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
> >
> > diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> > new file mode 100644
> > index 000000000000..3504d2e2d0e5
> > --- /dev/null
> > +++ b/drivers/hid/hid-steam.c
> > @@ -0,0 +1,840 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * HID driver for Valve Steam Controller
> > + *
> > + * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@xxxxxxxxx>
> > + *
> > + * Supports both the wired and wireless interfaces.
> > + *
> > + * This controller has a builtin emulation of mouse and keyboard: the right pad
> > + * can be used as a mouse, the shoulder buttons are mouse buttons, A and B
> > + * buttons are ENTER and ESCAPE, and so on. This is implemented as additional
> > + * HID interfaces.
> > + *
> > + * This is known as the "lizard mode", because apparently lizards like to use
> > + * the computer from the coach, without a proper mouse and keyboard.
> > + *
> > + * This driver will disable the lizard mode when the input device is opened
> > + * and re-enable it when the input device is closed, so as not to break user
> > + * mode behaviour. The lizard_mode parameter can be used to change that.
> > + *
> > + * There are a few user space applications (notably Steam Client) that use
> > + * the hidraw interface directly to create input devices (XTest, uinput...).
> > + * In order to avoid breaking them this driver creates a layered hidraw device,
> > + * so it can detect when the client is running and then:
> > + * - it will not send any command to the controller.
> > + * - this input device will be disabled, to avoid double input of the same
> > + * user action.
> > + *
> > + * For additional functions, such as changing the right-pad margin or switching
> > + * the led, you can use the user-space tool at:
> > + *
> > + * https://github.com/rodrigorc/steamctrl
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/mutex.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/delay.h>
> > +#include <linux/power_supply.h>
> > +#include "hid-ids.h"
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@xxxxxxxxx>");
> > +
> > +static int lizard_mode = 1;
> > +module_param(lizard_mode, int, 0644);
> > +MODULE_PARM_DESC(lizard_mode,
> > + "Mouse and keyboard emulation (0 = always disabled; "
> > + "1 (default): enabled when gamepad is not in use; "
> > + "2: let userspace decide)");
>
> As mentioned in 0/2, I think a boolean might be better. Probably
> rename the parameter to something else more explicit too (like
> 'control_lizard_mode').
> Also you might want to hook up to changes to this values so users can
> control it better. But this can be added in a later patch.

As I replied to your previous comment, I really like the options
enable_lizard_mode=true/false. If you add to the mix the no_magic,
then you have the current situation.
I admit that it can be confusing to the user, and that the no_magic may
not have a practical use case. So I'd stick to the enable/disable for
now, if you agree.

> > +
> > +#define STEAM_QUIRK_WIRELESS BIT(0)
> > +
> > +#define STEAM_SERIAL_LEN 10
> > +/* Touch pads are 40 mm in diameter and 65535 units */
> > +#define STEAM_PAD_RESOLUTION 1638
> > +/* Trigger runs are about 5 mm and 256 units */
> > +#define STEAM_TRIGGER_RESOLUTION 51
> > +/* Joystick runs are about 5 mm and 256 units */
> > +#define STEAM_JOYSTICK_RESOLUTION 51
> > +
> > +#define STEAM_PAD_FUZZ 256
> > +
> > +struct steam_device {
> > + spinlock_t lock;
> > + struct hid_device *hdev, *client_hdev;
> > + struct mutex mutex;
> > + bool client_opened, input_opened;
> > + struct input_dev __rcu *input;
> > + unsigned long quirks;
> > + struct work_struct work_connect;
> > + bool connected;
> > + char serial_no[STEAM_SERIAL_LEN + 1];
> > +};
> > +
> > +static int steam_recv_report(struct steam_device *steam,
> > + u8 *data, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
> > +
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /*
> > + * The report ID is always 0, so strip the first byte from the output.
> > + * hid_report_len() is not counting the report ID, so +1 to the length
> > + * or else we get a EOVERFLOW. We are safe from a buffer overflow
> > + * because hid_alloc_report_buf() allocates +7 bytes.
> > + */
> > + ret = hid_hw_raw_request(steam->hdev, 0x00,
> > + buf, hid_report_len(r) + 1,
> > + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > + if (ret > 0)
> > + memcpy(data, buf + 1, min(size, ret - 1));
> > + kfree(buf);
> > + return ret;
> > +}
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > + u8 *cmd, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + unsigned int retries = 10;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
> > +
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /* The report ID is always 0 */
> > + memcpy(buf + 1, cmd, size);
> > +
> > + /*
> > + * Sometimes the wireless controller fails with EPIPE
> > + * when sending a feature report.
> > + * Doing a HID_REQ_GET_REPORT and waiting for a while
> > + * seems to fix that.
> > + */
> > + do {
> > + ret = hid_hw_raw_request(steam->hdev, 0,
> > + buf, size + 1,
> > + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > + if (ret != -EPIPE)
> > + break;
> > + steam_recv_report(steam, NULL, 0);
> > + msleep(50);
> > + } while (--retries);
> > +
> > + kfree(buf);
> > + if (ret < 0)
> > + hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
> > + ret, size, cmd);
> > + return ret;
> > +}
> > +
> > +static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
> > +{
> > + return steam_send_report(steam, &cmd, 1);
> > +}
> > +
> > +static inline int steam_write_register(struct steam_device *steam,
> > + u8 reg, u16 value)
> > +{
> > + u8 cmd[] = {0x87, 0x03, reg, value & 0xFF, value >> 8};
> > +
> > + return steam_send_report(steam, cmd, sizeof(cmd));
> > +}
> > +
> > +static int steam_get_serial(struct steam_device *steam)
> > +{
> > + /*
> > + * Send: 0xae 0x15 0x01
> > + * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> > + */
> > + int ret;
> > + u8 cmd[] = {0xae, 0x15, 0x01};
> > + u8 reply[3 + STEAM_SERIAL_LEN + 1];
> > +
> > + ret = steam_send_report(steam, cmd, sizeof(cmd));
> > + if (ret < 0)
> > + return ret;
> > + ret = steam_recv_report(steam, reply, sizeof(reply));
> > + if (ret < 0)
> > + return ret;
> > + reply[3 + STEAM_SERIAL_LEN] = 0;
> > + strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
> > + return 0;
> > +}
> > +
> > +/*
> > + * This command requests the wireless adaptor to post an event
> > + * with the connection status. Useful if this driver is loaded when
> > + * the controller is already connected.
> > + */
> > +static inline int steam_request_conn_status(struct steam_device *steam)
> > +{
> > + return steam_send_report_byte(steam, 0xb4);
> > +}
> > +
> > +static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
> > +{
> > + if (enable) {
> > + /* enable esc, enter, cursors */
> > + steam_send_report_byte(steam, 0x85);
> > + /* enable mouse */
> > + steam_send_report_byte(steam, 0x8e);
> > + } else {
> > + /* disable esc, enter, cursor */
> > + steam_send_report_byte(steam, 0x81);
> > + /* disable mouse */
> > + steam_write_register(steam, 0x08, 0x07);
> > + }
> > +}
> > +
> > +static int steam_input_open(struct input_dev *dev)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = hid_hw_open(steam->hdev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->input_opened = true;
> > + if (!steam->client_opened && lizard_mode == 1)
> > + steam_set_lizard_mode(steam, false);
> > + mutex_unlock(&steam->mutex);
> > + return 0;
> > +}
> > +
> > +static void steam_input_close(struct input_dev *dev)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > +
> > + hid_hw_close(steam->hdev);
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->input_opened = false;
> > + if (!steam->client_opened && lizard_mode == 1)
> > + steam_set_lizard_mode(steam, true);
> > + mutex_unlock(&steam->mutex);
>
> hid_hw_close() should be called after setting the lizard mode.

Done.

> > +}
> > +
> > +static int steam_register(struct steam_device *steam)
> > +{
> > + struct hid_device *hdev = steam->hdev;
> > + struct input_dev *input;
> > + int ret;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > + if (input) {
> > + dbg_hid("%s: already connected\n", __func__);
> > + return 0;
> > + }
> > +
> > + ret = steam_get_serial(steam);
> > + if (ret)
> > + return ret;
> > +
> > + hid_info(hdev, "Steam Controller '%s' connected",
> > + steam->serial_no);
> > +
> > + input = input_allocate_device();
> > + if (!input)
> > + return -ENOMEM;
> > +
> > + input_set_drvdata(input, steam);
> > + input->dev.parent = &hdev->dev;
> > + input->open = steam_input_open;
> > + input->close = steam_input_close;
> > +
> > + input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
> > + "Wireless Steam Controller" :
> > + "Steam Controller";
> > + input->phys = hdev->phys;
> > + input->uniq = steam->serial_no;
> > + input->id.bustype = hdev->bus;
> > + input->id.vendor = hdev->vendor;
> > + input->id.product = hdev->product;
> > + input->id.version = hdev->version;
> > +
> > + input_set_capability(input, EV_KEY, BTN_TR2);
> > + input_set_capability(input, EV_KEY, BTN_TL2);
> > + input_set_capability(input, EV_KEY, BTN_TR);
> > + input_set_capability(input, EV_KEY, BTN_TL);
> > + input_set_capability(input, EV_KEY, BTN_Y);
> > + input_set_capability(input, EV_KEY, BTN_B);
> > + input_set_capability(input, EV_KEY, BTN_X);
> > + input_set_capability(input, EV_KEY, BTN_A);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_UP);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_RIGHT);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_LEFT);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_DOWN);
> > + input_set_capability(input, EV_KEY, BTN_SELECT);
> > + input_set_capability(input, EV_KEY, BTN_MODE);
> > + input_set_capability(input, EV_KEY, BTN_START);
> > + input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
> > + input_set_capability(input, EV_KEY, BTN_GEAR_UP);
> > + input_set_capability(input, EV_KEY, BTN_THUMBR);
> > + input_set_capability(input, EV_KEY, BTN_THUMBL);
> > + input_set_capability(input, EV_KEY, BTN_THUMB);
> > + input_set_capability(input, EV_KEY, BTN_THUMB2);
> > +
> > + input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_RX, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_RY, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_HAT0X, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_HAT0Y, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
> > + input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
> > + input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
> > +
> > + ret = input_register_device(input);
> > + if (ret)
> > + goto input_register_fail;
> > +
> > + rcu_assign_pointer(steam->input, input);
> > +
> > + return 0;
> > +
> > +input_register_fail:
> > + input_free_device(input);
> > + return ret;
> > +}
> > +
> > +static void steam_unregister(struct steam_device *steam)
> > +{
> > + struct input_dev *input;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > +
> > + if (input) {
> > + RCU_INIT_POINTER(steam->input, NULL);
> > + synchronize_rcu();
> > + hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> > + steam->serial_no);
> > + input_unregister_device(input);
> > + }
> > +}
> > +
> > +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) {
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(steam->hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + }
> > + } else {
> > + steam_unregister(steam);
> > + }
> > +}
> > +
> > +static bool steam_is_valve_interface(struct hid_device *hdev)
> > +{
> > + struct hid_report_enum *rep_enum;
> > +
> > + /*
> > + * The wired device creates 3 interfaces:
> > + * 0: emulated mouse.
> > + * 1: emulated keyboard.
> > + * 2: the real game pad.
> > + * The wireless device creates 5 interfaces:
> > + * 0: emulated keyboard.
> > + * 1-4: slots where up to 4 real game pads will be connected to.
> > + * We know which one is the real gamepad interface because they are the
> > + * only ones with a feature report.
> > + */
> > + rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
> > + return !list_empty(&rep_enum->report_list);
> > +}
> > +
> > +static int steam_client_ll_parse(struct hid_device *hdev)
> > +{
> > + return 0;
>
> Instead of returning 0 here, you should probably call
> hid_parse_report() on the report descriptors from the parent node.
> Some clients might want to have a look at them or might even rely on
> them.

Ah, but hid_parse_report() just copies the descriptor from the parent,
and I am already doing that from steam_probe(). I'll move the code to
here and change to call hid_parse_report().

> > +}
> > +
> > +static int steam_client_ll_start(struct hid_device *hdev)
> > +{
> > + return 0;
> > +}
> > +
> > +static void steam_client_ll_stop(struct hid_device *hdev)
> > +{
> > +}
> > +
> > +static int steam_client_ll_open(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
>
> You probably want to check if steam is not null here. Given the
> ordering of the initialization, you might have someone attempting to
> open the hidraw node before steam is created.

Tricky. Particularly since I've moved hid_parse_report() to
steam_client_ll_parse(), if steam is null there it all will break
apart. And parse is called from hid_add_device(), so I have to reorder
things a bit. Maybe calling hid_add_device() later.

> > + int ret;
> > +
> > + ret = hid_hw_open(steam->hdev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->client_opened = true;
> > + mutex_unlock(&steam->mutex);
> > + return ret;
> > +}
> > +
> > +static void steam_client_ll_close(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
>
> Same here, you might want to check on the validity of steam.

Now that I've reordered the probe, steam cannot be NULL.

> > +
> > + hid_hw_close(steam->hdev);
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->client_opened = false;
> > + if (lizard_mode != 2) {
> > + if (steam->input_opened)
> > + steam_set_lizard_mode(steam, false);
> > + else
> > + steam_set_lizard_mode(steam, lizard_mode);
> > + }
> > + mutex_unlock(&steam->mutex);
>
> You should call hid_hw_close(steam->hdev); after sending the commands
> and not before.

Done.

> > +}
> > +
> > +static int steam_client_ll_raw_request(struct hid_device *hdev,
> > + unsigned char reportnum, u8 *buf,
> > + size_t count, unsigned char report_type,
> > + int reqtype)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
> > + report_type, reqtype);
> > +}
>
> I wish we could reuse directly the pointer in
> hdev->ll_driver->raw_request to avoid adding an indirection.
> OTOH, the raw_requests are not happening that often, so we should be good.

hid_hw_raw_request() is an inline function that does exactly that, so I
would not worry about that.

However the call to hid_input_report() from steam_raw_event()... I wanted
to call client_hid->driver->raw_event() directly, but I didn't dare.

> > +static struct hid_ll_driver steam_client_ll_driver = {
> > + .parse = steam_client_ll_parse,
> > + .start = steam_client_ll_start,
> > + .stop = steam_client_ll_stop,
> > + .open = steam_client_ll_open,
> > + .close = steam_client_ll_close,
> > + .raw_request = steam_client_ll_raw_request,
> > +};
> > +
> > +static int steam_probe(struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct steam_device *steam;
> > + struct hid_device *client_hdev;
> > + int ret;
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:parse of hid interface failed\n", __func__);
> > + return ret;
> > + }
> > +
> > + /*
> > + * The non-valve interfaces (mouse and keyboard emulation) and
> > + * the client_hid are connected without changes.
> > + */
> > + if (hdev->group == HID_GROUP_STEAM ||
> > + !steam_is_valve_interface(hdev)) {
> > + return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > + }
>
> To make thinks clearer, I would split these calls in 2:
> - if (!steam_is_valve_interface(hdev)) return hid_hw_start(hdev,
> HID_CONNECT_DEFAULT);
> and
> - if (hdev->group == HID_GROUP_STEAM) return hid_hw_start(hdev,
> HID_CONNECT_HIDRAW);
>
> Explicitly having HID_CONNECT_HIDRAW would also make it clearer you
> are just exporting the hidraw interface here. It'll prevent any other
> interface to mess your detection of the hidraw usage.

Ok, done.

> > + /*
> > + * With the real steam controller interface, do not connect hidraw.
> > + * Instead, create the client_hid and connect that.
> > + */
> > + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> > + if (ret)
> > + return ret;
>
> this should likely be at the end of the probe, when you are done
> allocating your data.

Changed.

> > + client_hdev = hid_allocate_device();
> > + if (IS_ERR(client_hdev)) {
> > + ret = PTR_ERR(client_hdev);
> > + goto client_hdev_fail;
> > + }
> > +
> > + client_hdev->ll_driver = &steam_client_ll_driver;
> > + client_hdev->dev.parent = hdev->dev.parent;
> > + client_hdev->bus = hdev->bus;
> > + client_hdev->vendor = hdev->vendor;
> > + client_hdev->product = hdev->product;
> > + strlcpy(client_hdev->name, hdev->name,
> > + sizeof(client_hdev->name));
> > + strlcpy(client_hdev->phys, hdev->phys,
> > + sizeof(client_hdev->phys));
> > + client_hdev->dev_rdesc = kmemdup(hdev->dev_rdesc,
> > + hdev->dev_rsize, GFP_KERNEL);
> > + client_hdev->dev_rsize = hdev->dev_rsize;
> > + /*
> > + * Since we use the same device info than the real interface to
> > + * trick userspace, we will be calling steam_probe recursively.
> > + * We need to recognize the client interface somehow.
> > + */
> > + client_hdev->group = HID_GROUP_STEAM;
>
> I'd extract out the client_hdev initialization in its own function to
> keep .probe() clean.

Yeah, better. Done.

> > + ret = hid_add_device(client_hdev);
> > + if (ret)
> > + goto client_hdev_add_fail;
>
> This should likely be called after initializing steam. It'll keep the
> cleanup path cleaner and make sure all fields are properly initialized
> before they are used.

I've rewritten the probe so much that this does not apply anymore.
>
> > +
> > + steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
> > + if (!steam) {
> > + ret = -ENOMEM;
> > + goto steam_alloc_fail;
> > + }
> > +
> > + steam->client_hdev = client_hdev;
> > + hid_set_drvdata(client_hdev, steam);
> > +
> > + spin_lock_init(&steam->lock);
> > + mutex_init(&steam->mutex);
> > + steam->hdev = hdev;
> > + hid_set_drvdata(hdev, steam);
> > + steam->quirks = id->driver_data;
> > + INIT_WORK(&steam->work_connect, steam_work_connect_cb);
>
> I'd call hid_hw_start(hdev, ...) here, and then hid_add_device(client_hdev);

Ok.

> To have a better cleanup path, you porbably should allocate
> client_hdev here too (before hid_add_device, of course)

> > +
> > + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > + ret = hid_hw_open(hdev);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:hid_hw_open for wireless\n",
> > + __func__);
> > + goto hid_hw_open_fail;
> > + }
> > + hid_info(hdev, "Steam wireless receiver connected");
> > + steam_request_conn_status(steam);
> > + } else {
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + goto input_register_fail;
> > + }
> > + }
> > + if (lizard_mode != 2)
> > + steam_set_lizard_mode(steam, lizard_mode);
> > +
> > + return 0;
> > +
> > +input_register_fail:
> > +hid_hw_open_fail:
> > + cancel_work_sync(&steam->work_connect);
> > + hid_set_drvdata(hdev, NULL);
> > +steam_alloc_fail:
> > + hid_hw_stop(client_hdev);
> > +client_hdev_add_fail:
> > + hid_destroy_device(client_hdev);
> > +client_hdev_fail:
> > + hid_err(hdev, "%s: failed with error %d\n",
> > + __func__, ret);
> > + return 0;
> > +}
> > +
> > +static void steam_remove(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + if (hdev->group == HID_GROUP_STEAM)
>
> Why don't you call hid_hw_stop() here instead of calling it later in
> the parent device?

I tried to keep together these two lines:

hid_hw_stop(steam->client_hdev);
hid_destroy_device(steam->client_hdev);

but I guess that it is better to the first one here and the last one
there.

> > + return;
> > +
> > + if (!steam) {
> > + hid_hw_stop(hdev);
> > + return;
> > + }
> > +
>
> You should reorder these cleanup calls:
> - you first call hid_destroy_device(), this will clean up properly
> everything related to the hidraw node and should set client_opened to
> false (just to be on the safe side, you might want to overwrite it
> after to be sure not to forward events to the destoryed hid node).
> - then you take care of the rest of the hid device:
> - cancel_work_sync should happen before calling hid_hw_close() and
> hid_hw_stop() on the hdev.
> - steam_unregister(steam);
> - if needed call hid_hw_close()
> - hid_hw_stop()

That was a bit messy. Fixed.

That's all for now... I'll wait a couple of days more, in case I get any
more feedback and reroll.

Best regards.
Rodrigo

> > + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > + hid_info(hdev, "Steam wireless receiver disconnected");
> > + hid_hw_close(hdev);
> > + }
> > + hid_hw_stop(hdev);
> > + cancel_work_sync(&steam->work_connect);
> > + hid_hw_stop(steam->client_hdev);
> > + hid_destroy_device(steam->client_hdev);
> > +
> > +}
> > +
> > +static void steam_do_connect_event(struct steam_device *steam, bool connected)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&steam->lock, flags);
> > + steam->connected = connected;
> > + spin_unlock_irqrestore(&steam->lock, flags);
> > +
> > + if (schedule_work(&steam->work_connect) == 0)
> > + dbg_hid("%s: connected=%d event already queued\n",
> > + __func__, connected);
> > +}
> > +
> > +/*
> > + * The size for this message payload is 60.
> > + * The known values are:
> > + * (* values are not sent through wireless)
> > + * (* accelerator/gyro is disabled by default)
> > + * Offset| Type | Mapped to |Meaning
> > + * -------+-------+-----------+--------------------------
> > + * 4-7 | u32 | -- | sequence number
> > + * 8-10 | 24bit | see below | buttons
> > + * 11 | u8 | ABS_HAT2Y | left trigger
> > + * 12 | u8 | ABS_HAT2X | right trigger
> > + * 13-15 | -- | -- | always 0
> > + * 16-17 | s16 | ABS_X/ABS_HAT0X | X value
> > + * 18-19 | s16 | ABS_Y/ABS_HAT0Y | Y value
> > + * 20-21 | s16 | ABS_RX | right-pad X value
> > + * 22-23 | s16 | ABS_RY | right-pad Y value
> > + * 24-25 | s16 | -- | * left trigger
> > + * 26-27 | s16 | -- | * right trigger
> > + * 28-29 | s16 | -- | * accelerometer X value
> > + * 30-31 | s16 | -- | * accelerometer Y value
> > + * 32-33 | s16 | -- | * accelerometer Z value
> > + * 34-35 | s16 | -- | gyro X value
> > + * 36-36 | s16 | -- | gyro Y value
> > + * 38-39 | s16 | -- | gyro Z value
> > + * 40-41 | s16 | -- | quaternion W value
> > + * 42-43 | s16 | -- | quaternion X value
> > + * 44-45 | s16 | -- | quaternion Y value
> > + * 46-47 | s16 | -- | quaternion Z value
> > + * 48-49 | -- | -- | always 0
> > + * 50-51 | s16 | -- | * left trigger (uncalibrated)
> > + * 52-53 | s16 | -- | * right trigger (uncalibrated)
> > + * 54-55 | s16 | -- | * joystick X value (uncalibrated)
> > + * 56-57 | s16 | -- | * joystick Y value (uncalibrated)
> > + * 58-59 | s16 | -- | * left-pad X value
> > + * 60-61 | s16 | -- | * left-pad Y value
> > + * 62-63 | u16 | -- | * battery voltage
> > + *
> > + * The buttons are:
> > + * Bit | Mapped to | Description
> > + * ------+------------+--------------------------------
> > + * 8.0 | BTN_TR2 | right trigger fully pressed
> > + * 8.1 | BTN_TL2 | left trigger fully pressed
> > + * 8.2 | BTN_TR | right shoulder
> > + * 8.3 | BTN_TL | left shoulder
> > + * 8.4 | BTN_Y | button Y
> > + * 8.5 | BTN_B | button B
> > + * 8.6 | BTN_X | button X
> > + * 8.7 | BTN_A | button A
> > + * 9.0 | BTN_DPAD_UP | lef-pad up
> > + * 9.1 | BTN_DPAD_RIGHT | lef-pad right
> > + * 9.2 | BTN_DPAD_LEFT | lef-pad left
> > + * 9.3 | BTN_DPAD_DOWN | lef-pad down
> > + * 9.4 | BTN_SELECT | menu left
> > + * 9.5 | BTN_MODE | steam logo
> > + * 9.6 | BTN_START | menu right
> > + * 9.7 | BTN_GEAR_DOWN | left back lever
> > + * 10.0 | BTN_GEAR_UP | right back lever
> > + * 10.1 | -- | left-pad clicked
> > + * 10.2 | BTN_THUMBR | right-pad clicked
> > + * 10.3 | BTN_THUMB | left-pad touched (but see explanation below)
> > + * 10.4 | BTN_THUMB2 | right-pad touched
> > + * 10.5 | -- | unknown
> > + * 10.6 | BTN_THUMBL | joystick clicked
> > + * 10.7 | -- | lpad_and_joy
> > + */
> > +
> > +static void steam_do_input_event(struct steam_device *steam,
> > + struct input_dev *input, u8 *data)
> > +{
> > + /* 24 bits of buttons */
> > + u8 b8, b9, b10;
> > + bool lpad_touched, lpad_and_joy;
> > +
> > + b8 = data[8];
> > + b9 = data[9];
> > + b10 = data[10];
> > +
> > + input_report_abs(input, ABS_HAT2Y, data[11]);
> > + input_report_abs(input, ABS_HAT2X, data[12]);
> > +
> > + /*
> > + * These two bits tells how to interpret the values X and Y.
> > + * lpad_and_joy tells that the joystick and the lpad are used at the
> > + * same time.
> > + * lpad_touched tells whether X/Y are to be read as lpad coord or
> > + * joystick values.
> > + * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> > + */
> > + lpad_touched = b10 & BIT(3);
> > + lpad_and_joy = b10 & BIT(7);
> > + input_report_abs(input, lpad_touched ? ABS_HAT0X : ABS_X,
> > + (s16) le16_to_cpup((__le16 *)(data + 16)));
> > + input_report_abs(input, lpad_touched ? ABS_HAT0Y : ABS_Y,
> > + -(s16) le16_to_cpup((__le16 *)(data + 18)));
> > + /* Check if joystick is centered */
> > + if (lpad_touched && !lpad_and_joy) {
> > + input_report_abs(input, ABS_X, 0);
> > + input_report_abs(input, ABS_Y, 0);
> > + }
> > + /* Check if lpad is untouched */
> > + if (!(lpad_touched || lpad_and_joy)) {
> > + input_report_abs(input, ABS_HAT0X, 0);
> > + input_report_abs(input, ABS_HAT0Y, 0);
> > + }
> > +
> > + input_report_abs(input, ABS_RX,
> > + (s16) le16_to_cpup((__le16 *)(data + 20)));
> > + input_report_abs(input, ABS_RY,
> > + -(s16) le16_to_cpup((__le16 *)(data + 22)));
> > +
> > + input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
> > + input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
> > + input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
> > + input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
> > + input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
> > + input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
> > + input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
> > + input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
> > + input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
> > + input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
> > + input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
> > + input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
> > +
> > + input_sync(input);
> > +}
> > +
> > +static int steam_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data,
> > + int size)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > + struct input_dev *input;
> > +
> > + if (!steam)
> > + return 0;
> > +
> > + if (steam->client_opened)
> > + hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
> > + data, size, 0);
> > + /*
> > + * All messages are size=64, all values little-endian.
> > + * The format is:
> > + * Offset| Meaning
> > + * -------+--------------------------------------------
> > + * 0-1 | always 0x01, 0x00, maybe protocol version?
> > + * 2 | type of message
> > + * 3 | length of the real payload (not checked)
> > + * 4-n | payload data, depends on the type
> > + *
> > + * There are these known types of message:
> > + * 0x01: input data (60 bytes)
> > + * 0x03: wireless connect/disconnect (1 byte)
> > + * 0x04: battery status (11 bytes)
> > + */
> > +
> > + if (size != 64 || data[0] != 1 || data[1] != 0)
> > + return 0;
> > +
> > + switch (data[2]) {
> > + case 0x01:
> > + if (steam->client_opened)
> > + return 0;
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + if (likely(input)) {
> > + steam_do_input_event(steam, input, data);
> > + } else {
> > + dbg_hid("%s: input data without connect event\n",
> > + __func__);
> > + steam_do_connect_event(steam, true);
> > + }
> > + rcu_read_unlock();
> > + break;
> > + case 0x03:
> > + /*
> > + * The payload of this event is a single byte:
> > + * 0x01: disconnected.
> > + * 0x02: connected.
> > + */
> > + switch (data[4]) {
> > + case 0x01:
> > + steam_do_connect_event(steam, false);
> > + break;
> > + case 0x02:
> > + steam_do_connect_event(steam, true);
> > + break;
> > + }
> > + break;
> > + case 0x04:
> > + /* TODO: battery status */
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > +static const struct hid_device_id steam_controllers[] = {
> > + { /* Wired Steam Controller */
> > + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> > + USB_DEVICE_ID_STEAM_CONTROLLER)
> > + },
> > + { /* Wireless Steam Controller */
> > + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> > + USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
> > + .driver_data = STEAM_QUIRK_WIRELESS
> > + },
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, steam_controllers);
> > +
> > +static struct hid_driver steam_controller_driver = {
> > + .name = "hid-steam",
> > + .id_table = steam_controllers,
> > + .probe = steam_probe,
> > + .remove = steam_remove,
> > + .raw_event = steam_raw_event,
> > +};
> > +
> > +module_hid_driver(steam_controller_driver);
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index d491027a7c22..5e5d76589954 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -364,6 +364,7 @@ struct hid_item {
> > #define HID_GROUP_RMI 0x0100
> > #define HID_GROUP_WACOM 0x0101
> > #define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
> > +#define HID_GROUP_STEAM 0x0103
> >
> > /*
> > * HID protocol status
> > --
> > 2.16.2
> >
>
> Cheers,
> Benjamin

Next message: tip-bot for Peter Zijlstra: "[tip:locking/core] locking/rtmutex: Handle non enqueued waiters gracefully in remove_waiter()"
Previous message: Thomas Gleixner: "[patch 4/8] rslib: Remove GPL boilerplate"
In reply to: Benjamin Tissoires: "Re: [PATCH v7 1/2] HID: add driver for Valve Steam Controller"
Next in thread: Benjamin Tissoires: "Re: [PATCH v7 0/2] hid-steam driver with user mode client dection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]