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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
