| 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
| ||
|
Date: Fri, 11 Jul 2014 09:15:39 -0400
From: Benjamin Tissoires <benjamin.tissoires@...il.com>
To: Jason Gerecke <killertofu@...il.com>
Cc: Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Jiri Kosina <jkosina@...e.cz>,
Ping Cheng <pinglinux@...il.com>,
Linux Input <linux-input@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linuxwacom-devel <linuxwacom-devel@...ts.sourceforge.net>
Subject: Re: [PATCH 08/15] Input - wacom: remove usb dependency for siblings devices
On Thu, Jul 10, 2014 at 8:10 PM, Jason Gerecke <killertofu@...il.com> wrote:
> On Mon, Jun 30, 2014 at 2:26 PM, Benjamin Tissoires
> <benjamin.tissoires@...hat.com> wrote:
>> Wacom tablets can share different physical sensors on one physical device.
>> These are called siblings in the code. The current way of implementation
>> relies on the USB topology to be able to share data amongs those sensors.
>>
>> We can replace the code to match a HID subsystem, without involving the USB
>> topology:
>> - the first probed sensor does not find any siblings in the list
>> wacom_udev_list, so it creates its own wacom_hdev_data with its own
>> struct hid_device
>> - the other sensor checks the current list of siblings in wacom_hdev_data,
>> and if there is a match, it associates itself to the matched device.
>>
>> To be sure that we are not associating different sensors from different
>> physical devices, we also check for the phys path of the hid device which
>> contains the USB topology.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
>> ---
>> drivers/input/tablet/wacom_sys.c | 75 +++++++++++++++++++---------------------
>> 1 file changed, 35 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
>> index 0d0397d..6fe7a6c 100644
>> --- a/drivers/input/tablet/wacom_sys.c
>> +++ b/drivers/input/tablet/wacom_sys.c
>> @@ -488,46 +488,45 @@ static int wacom_retrieve_hid_descriptor(struct hid_device *hdev,
>> return error;
>> }
>>
>> -struct wacom_usbdev_data {
>> +struct wacom_hdev_data {
>> struct list_head list;
>> struct kref kref;
>> - struct usb_device *dev;
>> + struct hid_device *dev;
>> struct wacom_shared shared;
>> };
>>
>> static LIST_HEAD(wacom_udev_list);
>> static DEFINE_MUTEX(wacom_udev_list_lock);
>>
>> -static struct usb_device *wacom_get_sibling(struct usb_device *dev, int vendor, int product)
>> +static bool wacom_are_sibling(struct hid_device *hdev,
>> + struct hid_device *sibling)
>> {
>> - int port1;
>> - struct usb_device *sibling;
>> -
>> - if (vendor == 0 && product == 0)
>> - return dev;
>> -
>> - if (dev->parent == NULL)
>> - return NULL;
>> -
>> - usb_hub_for_each_child(dev->parent, port1, sibling) {
>> - struct usb_device_descriptor *d;
>> - if (sibling == NULL)
>> - continue;
>> + struct wacom *wacom = hid_get_drvdata(hdev);
>> + struct wacom_features *features = &wacom->wacom_wac.features;
>> + int vid = features->oVid;
>> + int pid = features->oPid;
>>
>> - d = &sibling->descriptor;
>> - if (d->idVendor == vendor && d->idProduct == product)
>> - return sibling;
>> + if (vid == 0 && pid == 0) {
>> + vid = hdev->vendor;
>> + pid = hdev->product;
>> }
>>
>> - return NULL;
>> + if (vid != sibling->vendor || pid != sibling->product)
>> + return false;
>> +
>> + /*
>> + * Compare the physical path.
>> + * Dump the last two chars which should contain the input number.
>> + */
>> + return !strncmp(hdev->phys, sibling->phys, strlen(hdev->phys) - 2);
>
> Good idea, but this implementation doesn't work. "Siblings" (such as
> you'd find on a 24HDT) are separate USB devices but share the same USB
> hub. Trimming off just the input number isn't sufficient since the USB
> path preceding it is going to be different for the two devices. For
> example, I the touch and pen interfaces on my 24HDT show up as
> "usb-0000:00:1d.0-1.4.3/input0" and "usb-0000:00:1d.0-1.4.2/input1".
You are right. I thought I managed to make the 22HDT working with my
patches, but maybe I overlooked it
>
> Something like the following could work though:
>
> int n1 = strrchr(hdev->phys, '.') - hdev->phys;
> int n2 = strrchr(sibling->phys, '.') - sibling->phys;
> if (n1 != n2 || n1 <= 0 || n2 <= 0)
> return false;
> else
> return !strncmp(hdev->phys, sibling->phys, n1);
Seems fair. I'll give a try and update the patches.
Thanks!
Cheers,
Benjamin
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
>> }
>>
>> -static struct wacom_usbdev_data *wacom_get_usbdev_data(struct usb_device *dev)
>> +static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
>> {
>> - struct wacom_usbdev_data *data;
>> + struct wacom_hdev_data *data;
>>
>> list_for_each_entry(data, &wacom_udev_list, list) {
>> - if (data->dev == dev) {
>> + if (wacom_are_sibling(hdev, data->dev)) {
>> kref_get(&data->kref);
>> return data;
>> }
>> @@ -536,28 +535,29 @@ static struct wacom_usbdev_data *wacom_get_usbdev_data(struct usb_device *dev)
>> return NULL;
>> }
>>
>> -static int wacom_add_shared_data(struct wacom_wac *wacom,
>> - struct usb_device *dev)
>> +static int wacom_add_shared_data(struct hid_device *hdev)
>> {
>> - struct wacom_usbdev_data *data;
>> + struct wacom *wacom = hid_get_drvdata(hdev);
>> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> + struct wacom_hdev_data *data;
>> int retval = 0;
>>
>> mutex_lock(&wacom_udev_list_lock);
>>
>> - data = wacom_get_usbdev_data(dev);
>> + data = wacom_get_hdev_data(hdev);
>> if (!data) {
>> - data = kzalloc(sizeof(struct wacom_usbdev_data), GFP_KERNEL);
>> + data = kzalloc(sizeof(struct wacom_hdev_data), GFP_KERNEL);
>> if (!data) {
>> retval = -ENOMEM;
>> goto out;
>> }
>>
>> kref_init(&data->kref);
>> - data->dev = dev;
>> + data->dev = hdev;
>> list_add_tail(&data->list, &wacom_udev_list);
>> }
>>
>> - wacom->shared = &data->shared;
>> + wacom_wac->shared = &data->shared;
>>
>> out:
>> mutex_unlock(&wacom_udev_list_lock);
>> @@ -566,8 +566,8 @@ out:
>>
>> static void wacom_release_shared_data(struct kref *kref)
>> {
>> - struct wacom_usbdev_data *data =
>> - container_of(kref, struct wacom_usbdev_data, kref);
>> + struct wacom_hdev_data *data =
>> + container_of(kref, struct wacom_hdev_data, kref);
>>
>> mutex_lock(&wacom_udev_list_lock);
>> list_del(&data->list);
>> @@ -578,10 +578,10 @@ static void wacom_release_shared_data(struct kref *kref)
>>
>> static void wacom_remove_shared_data(struct wacom_wac *wacom)
>> {
>> - struct wacom_usbdev_data *data;
>> + struct wacom_hdev_data *data;
>>
>> if (wacom->shared) {
>> - data = container_of(wacom->shared, struct wacom_usbdev_data, shared);
>> + data = container_of(wacom->shared, struct wacom_hdev_data, shared);
>> kref_put(&data->kref, wacom_release_shared_data);
>> wacom->shared = NULL;
>> }
>> @@ -1311,8 +1311,6 @@ static int wacom_probe(struct hid_device *hdev,
>> "%s Pad", features->name);
>>
>> if (features->quirks & WACOM_QUIRK_MULTI_INPUT) {
>> - struct usb_device *other_dev;
>> -
>> /* Append the device type to the name */
>> if (features->device_type != BTN_TOOL_FINGER)
>> strlcat(wacom_wac->name, " Pen", WACOM_NAME_MAX);
>> @@ -1321,10 +1319,7 @@ static int wacom_probe(struct hid_device *hdev,
>> else
>> strlcat(wacom_wac->name, " Pad", WACOM_NAME_MAX);
>>
>> - other_dev = wacom_get_sibling(dev, features->oVid, features->oPid);
>> - if (other_dev == NULL || wacom_get_usbdev_data(other_dev) == NULL)
>> - other_dev = dev;
>> - error = wacom_add_shared_data(wacom_wac, other_dev);
>> + error = wacom_add_shared_data(hdev);
>> if (error)
>> goto fail1;
>> }
>> --
>> 2.0.0
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists