| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190616152859.6f1c247c@archlinux>
Date: Sun, 16 Jun 2019 15:28:59 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ronald Tschalär <ronald@...ovation.ch>
Cc: Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
linux-input@...r.kernel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] HID: apple-ib-tb: Add driver for the Touch Bar
on MacBook Pro's.
On Wed, 12 Jun 2019 01:33:59 -0700
Ronald Tschalär <ronald@...ovation.ch> wrote:
> This driver enables basic touch bar functionality: enabling it, switching
> between modes on FN key press, and dimming and turning the display
> off/on when idle/active.
>
> Signed-off-by: Ronald Tschalär <ronald@...ovation.ch>
A few minor comments inline from me, but as before well outside of my
areas of knowledge!
Jonathan
> ---
> drivers/hid/Kconfig | 10 +
> drivers/hid/Makefile | 1 +
> drivers/hid/apple-ib-tb.c | 1389 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 1400 insertions(+)
> create mode 100644 drivers/hid/apple-ib-tb.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 545d3691fc1c..7621c2500d71 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -149,6 +149,16 @@ config HID_APPLE_IBRIDGE
> To compile this driver as a module, choose M here: the
> module will be called apple-ibridge.
>
> +config HID_APPLE_IBRIDGE_TB
> + tristate "Apple iBridge Touch Bar"
> + depends on HID_APPLE_IBRIDGE
> + ---help---
> + Say Y here if you want support for the Touch Bar on recent
> + MacBook Pros.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ib-tb.
> +
> config HID_APPLEIR
> tristate "Apple infrared receiver"
> depends on (USB_HID)
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index a4da5663a541..0c46e5f70db1 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS) += hid-alps.o
> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
> obj-$(CONFIG_HID_APPLE) += hid-apple.o
> obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o
> +obj-$(CONFIG_HID_APPLE_IBRIDGE_TB) += apple-ib-tb.o
> obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
> obj-$(CONFIG_HID_ASUS) += hid-asus.o
> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
> diff --git a/drivers/hid/apple-ib-tb.c b/drivers/hid/apple-ib-tb.c
> new file mode 100644
> index 000000000000..6daee80060ce
> --- /dev/null
> +++ b/drivers/hid/apple-ib-tb.c
> @@ -0,0 +1,1389 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Touch Bar Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + */
> +
> +/*
> + * Recent MacBookPro models (13,[23] and 14,[23]) have a touch bar, which
> + * is exposed via several USB interfaces. MacOS supports a fancy mode
> + * where arbitrary buttons can be defined; this driver currently only
> + * supports the simple mode that consists of 3 predefined layouts
> + * (escape-only, esc + special keys, and esc + function keys).
> + *
> + * The first USB HID interface supports two reports, an input report that
> + * is used to report the key presses, and an output report which can be
> + * used to set the touch bar "mode": touch bar off (in which case no touches
> + * are reported at all), escape key only, escape + 12 function keys, and
> + * escape + several special keys (including brightness, audio volume,
> + * etc). The second interface supports several, complex reports, most of
> + * which are unknown at this time, but one of which has been determined to
> + * allow for controlling of the touch bar's brightness: off (though touches
> + * are still reported), dimmed, and full brightness. This driver makes
> + * use of these two reports.
> + */
> +
> +#define dev_fmt(fmt) "tb: " fmt
> +
> +#include <linux/apple-ibridge.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb.h>
> +#include <linux/workqueue.h>
> +
> +#define HID_UP_APPLE 0xff120000
> +#define HID_USAGE_MODE (HID_UP_CUSTOM | 0x0004)
> +#define HID_USAGE_APPLE_APP (HID_UP_APPLE | 0x0001)
> +#define HID_USAGE_DISP (HID_UP_APPLE | 0x0021)
> +#define HID_USAGE_DISP_AUX1 (HID_UP_APPLE | 0x0020)
> +
> +#define APPLETB_MAX_TB_KEYS 13 /* ESC, F1-F12 */
> +
> +#define APPLETB_CMD_MODE_ESC 0
> +#define APPLETB_CMD_MODE_FN 1
> +#define APPLETB_CMD_MODE_SPCL 2
> +#define APPLETB_CMD_MODE_OFF 3
> +#define APPLETB_CMD_MODE_UPD 254
> +#define APPLETB_CMD_MODE_NONE 255
> +
> +#define APPLETB_CMD_DISP_ON 1
> +#define APPLETB_CMD_DISP_DIM 2
> +#define APPLETB_CMD_DISP_OFF 4
> +#define APPLETB_CMD_DISP_UPD 254
> +#define APPLETB_CMD_DISP_NONE 255
> +
> +#define APPLETB_FN_MODE_FKEYS 0
> +#define APPLETB_FN_MODE_NORM 1
> +#define APPLETB_FN_MODE_INV 2
> +#define APPLETB_FN_MODE_SPCL 3
> +#define APPLETB_FN_MODE_MAX APPLETB_FN_MODE_SPCL
> +
> +#define APPLETB_DEVID_KEYBOARD 1
> +#define APPLETB_DEVID_TOUCHPAD 2
> +
> +#define APPLETB_MAX_DIM_TIME 30
> +
> +static int appletb_tb_def_idle_timeout = 5 * 60;
> +module_param_named(idle_timeout, appletb_tb_def_idle_timeout, int, 0444);
> +MODULE_PARM_DESC(idle_timeout, "Default touch bar idle timeout:\n"
> + " >0 - turn touch bar display off after no keyboard, trackpad, or touch bar input has been received for this many seconds;\n"
> + " the display will be turned back on as soon as new input is received\n"
> + " 0 - turn touch bar display off (input does not turn it on again)\n"
> + " -1 - turn touch bar display on (does not turn off automatically)\n"
> + " -2 - disable touch bar completely");
> +
> +static int appletb_tb_def_dim_timeout = -2;
> +module_param_named(dim_timeout, appletb_tb_def_dim_timeout, int, 0444);
> +MODULE_PARM_DESC(dim_timeout, "Default touch bar dim timeout:\n"
> + " >0 - dim touch bar display after no keyboard, trackpad, or touch bar input has been received for this many seconds\n"
> + " the display will be returned to full brightness as soon as new input is received\n"
> + " 0 - dim touch bar display (input does not return it to full brightness)\n"
> + " -1 - disable timeout (touch bar never dimmed)\n"
> + " [-2] - calculate timeout based on idle-timeout");
> +
> +static int appletb_tb_def_fn_mode = APPLETB_FN_MODE_NORM;
> +module_param_named(fnmode, appletb_tb_def_fn_mode, int, 0444);
> +MODULE_PARM_DESC(fnmode, "Default Fn key mode:\n"
> + " 0 - function-keys only\n"
> + " [1] - fn key switches from special to function-keys\n"
> + " 2 - inverse of 1\n"
> + " 3 - special keys only");
> +
> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(idle_timeout);
> +
> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(dim_timeout);
> +
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(fnmode);
> +
> +static struct attribute *appletb_attrs[] = {
> + &dev_attr_idle_timeout.attr,
> + &dev_attr_dim_timeout.attr,
> + &dev_attr_fnmode.attr,
> + NULL,
Documentation for these?
> +};
> +
> +static const struct attribute_group appletb_attr_group = {
> + .attrs = appletb_attrs,
> +};
> +
...
> +/*
> + * While the mode functionality is listed as a valid hid report in the usb
> + * interface descriptor, it's not sent that way. Instead it's sent with
> + * different request-type and without a leading report-id in the data. Hence
> + * we need to send it as a custom usb control message rather via any of the
> + * standard hid_hw_*request() functions.
> + */
> +static int appletb_set_tb_mode(struct appletb_device *tb_dev,
> + unsigned char mode)
> +{
> + void *buf;
> + bool autopm_off = false;
> + int rc;
> +
> + if (!tb_dev->mode_iface.hdev)
> + return -ENOTCONN;
> +
> + buf = kmemdup(&mode, 1, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + autopm_off = appletb_disable_autopm(tb_dev->mode_iface.hdev);
> +
> + rc = appletb_send_usb_ctrl(&tb_dev->mode_iface,
> + USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE,
Odd indentation.
> + tb_dev->mode_field->report, buf, 1);
> + if (rc < 0)
> + dev_err(tb_dev->log_dev,
> + "Failed to set touch bar mode to %u (%d)\n", mode, rc);
> +
> + if (autopm_off)
> + hid_hw_power(tb_dev->mode_iface.hdev, PM_HINT_NORMAL);
> +
> + kfree(buf);
> +
> + return rc;
> +}
...
> +
> +static int appletb_set_tb_disp(struct appletb_device *tb_dev,
> + unsigned char disp)
> +{
> + struct hid_report *report;
> + int rc;
> +
> + if (!tb_dev->disp_iface.hdev)
> + return -ENOTCONN;
> +
> + report = tb_dev->disp_field->report;
> +
> + rc = hid_set_field(tb_dev->disp_field_aux1, 0, 1);
> + if (!rc)
> + rc = hid_set_field(tb_dev->disp_field, 0, disp);
This stacked error handling is less readable than just having two
separate error blocks with very similar comments.
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to set display report fields (%d)\n", rc);
> + return rc;
> + }
> +
> + /*
> + * Keep the USB interface powered on while the touch bar display is on
> + * for better responsiveness.
> + */
> + if (disp != APPLETB_CMD_DISP_OFF && !tb_dev->tb_autopm_off)
> + tb_dev->tb_autopm_off =
> + appletb_disable_autopm(report->device);
> +
> + rc = appletb_send_hid_report(&tb_dev->disp_iface, report);
> +
Nitpick. For consistency should probably not be a blank line here.
> + if (rc < 0)
> + dev_err(tb_dev->log_dev,
> + "Failed to set touch bar display to %u (%d)\n", disp,
> + rc);
> +
> + if (disp == APPLETB_CMD_DISP_OFF && tb_dev->tb_autopm_off) {
> + hid_hw_power(tb_dev->disp_iface.hdev, PM_HINT_NORMAL);
> + tb_dev->tb_autopm_off = false;
> + }
> +
> + return rc;
> +}
> +
...
> +static int appletb_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appletb_device *tb_dev = appletb_dev;
> + struct appletb_iface_info *iface_info;
> + unsigned long flags;
> + int rc;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (!tb_dev->log_dev)
> + tb_dev->log_dev = &hdev->dev;
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + hid_set_drvdata(hdev, tb_dev);
> +
> + /* initialize the report info */
> + rc = hid_parse(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "als: hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + rc = appletb_extract_report_info(tb_dev, hdev);
> + if (rc < 0)
> + goto error;
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER | HID_CONNECT_HIDINPUT);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw start failed (%d)\n", rc);
> + goto clear_iface_info;
> + }
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw open failed (%d)\n", rc);
> + goto stop_hid;
> + }
> +
> + /* do setup if we have both interfaces */
> + if (appletb_test_and_mark_active(tb_dev)) {
> + /* initialize the touch bar */
> + if (appletb_tb_def_fn_mode >= 0 &&
> + appletb_tb_def_fn_mode <= APPLETB_FN_MODE_MAX)
> + tb_dev->fn_mode = appletb_tb_def_fn_mode;
> + else
> + tb_dev->fn_mode = APPLETB_FN_MODE_NORM;
> + appletb_set_idle_timeout(tb_dev, appletb_tb_def_idle_timeout);
> + appletb_set_dim_timeout(tb_dev, appletb_tb_def_dim_timeout);
> + tb_dev->last_event_time = ktime_get();
> +
> + tb_dev->pnd_tb_mode = APPLETB_CMD_MODE_UPD;
> + tb_dev->pnd_tb_disp = APPLETB_CMD_DISP_UPD;
> +
> + appletb_update_touchbar(tb_dev, false);
> +
> + /* set up the input handler */
> + tb_dev->inp_handler.event = appletb_inp_event;
> + tb_dev->inp_handler.connect = appletb_inp_connect;
> + tb_dev->inp_handler.disconnect = appletb_inp_disconnect;
> + tb_dev->inp_handler.name = "appletb";
> + tb_dev->inp_handler.id_table = appletb_input_devices;
> + tb_dev->inp_handler.private = tb_dev;
> +
> + rc = input_register_handler(&tb_dev->inp_handler);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Unable to register keyboard handler (%d)\n",
> + rc);
> + goto mark_inactive;
> + }
> +
> + /* initialize sysfs attributes */
> + rc = sysfs_create_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to create sysfs attributes (%d)\n", rc);
> + goto unreg_handler;
> + }
> +
> + dev_dbg(tb_dev->log_dev, "Touchbar activated\n");
> + }
> +
> + return 0;
> +
> +unreg_handler:
> + input_unregister_handler(&tb_dev->inp_handler);
> +mark_inactive:
> + appletb_test_and_mark_inactive(tb_dev, hdev);
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + hid_hw_close(hdev);
> +stop_hid:
> + hid_hw_stop(hdev);
> +clear_iface_info:
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info) {
> + usb_put_intf(iface_info->usb_iface);
> + iface_info->usb_iface = NULL;
> + iface_info->hdev = NULL;
> + }
It might be cleaner to put this block into a utility function
named appropriately to make it clear it's undoing stuff from
the *_extract_report_info call. Initially I wondered where
this stuff we are undoing came from.
> +error:
> + return rc;
> +}
> +
> +static void appletb_remove(struct hid_device *hdev)
> +{
> + struct appletb_device *tb_dev = hid_get_drvdata(hdev);
> + struct appletb_iface_info *iface_info;
> + unsigned long flags;
> +
> + if (appletb_test_and_mark_inactive(tb_dev, hdev)) {
> + sysfs_remove_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> +
> + input_unregister_handler(&tb_dev->inp_handler);
> +
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + appletb_set_tb_mode(tb_dev, APPLETB_CMD_MODE_OFF);
> + appletb_set_tb_disp(tb_dev, APPLETB_CMD_DISP_ON);
> +
> + if (tb_dev->tb_autopm_off)
> + hid_hw_power(tb_dev->disp_iface.hdev, PM_HINT_NORMAL);
> +
> + dev_info(tb_dev->log_dev, "Touchbar deactivated\n");
> + }
> +
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
> +
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info) {
> + usb_put_intf(iface_info->usb_iface);
> + iface_info->usb_iface = NULL;
> + iface_info->hdev = NULL;
> + }
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (tb_dev->log_dev == &hdev->dev) {
> + if (tb_dev->mode_iface.hdev)
> + tb_dev->log_dev = &tb_dev->mode_iface.hdev->dev;
> + else if (tb_dev->disp_iface.hdev)
> + tb_dev->log_dev = &tb_dev->disp_iface.hdev->dev;
> + else
> + tb_dev->log_dev = NULL;
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +}
...
Powered by blists - more mailing lists