[PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Hans de Goede]

Hi All,

For firmware instantiated serio devices, such as devices instantiated
through ACPI, it may be useful for userspace to know the firmware-id
(pnp-id in case of ACPI) through which the device was instantiated.

One concrete example of this is the new ps/2 touchpads found in Lenovo
Thinkpad X240 T440 and T540 laptops, which not only have a special
bottom right click area to emulate right clicks, but also top middle
and top right areas to emulate middle and right clicks for the trackpoint.

Lenove has given these touchpads a unique LENxxxx pnp-id and we would like
to use this to automatically enable emulation of middle and right buttons
in the top area of the clickpad.

Matthew Garret wrote a patch to set the parent of the serio port to the
/sys/devices/pnp0/00:xx device so that we could get the necessary info that
way, but Dmitry rightfully pointed out that that would break suspend/resume
ordering, see:
https://lkml.org/lkml/2014/2/23/63
https://lkml.org/lkml/2014/3/7/428

So while discussing this with Peter Hutterer I made the plan to add a
/dev/devices/platform/i8042/serioX/firmware_parent symlink pointing to the
relevant /sys/devices/pnp0/00:xx device, and add a little udev-helper + rules
file to read this symlink and add an attribute to the udevdb with the
pnp-id this way.

This however is harder then it sounds, I spend a couple of hours on how
to do this in a race free manner and I could not come up with one. The
problem is that the sysfs_create_symlink call needs to be done after the
device_add call, at which point an add uevent has already been fired.
Causing a theoretical race where the udev-helper would not find the symlink,
this can be fixed with an extra change event after adding the symlink, but
then any higher userspace levels need to re-check on a change event as well.
So after spending too much time on this I decided to go for an alternative
solution.

Which in the end turns out to be much nicer too, since it gets rid of needing
a udev-helper too. After this much too long introduction I'll let the patches
speak for themselves.

With this patch-set one can now do:

[hans@shalem linux]$ udevadm info -p /devices/platform/i8042/serio1
P: /devices/platform/i8042/serio1
E: DEVPATH=/devices/platform/i8042/serio1
E: MODALIAS=serio:ty01pr00id00ex00
E: SERIO_EXTRA=00
E: SERIO_FIRMWARE_ID=PNP0f03 PNP0f13
E: SERIO_ID=00
E: SERIO_PROTO=00
E: SERIO_TYPE=01
E: SUBSYSTEM=serio

And the info we need is just there in the new SERIO_FIRMWARE_ID attribute,
for completeness the patch-set also adds:

[hans@shalem linux]$ cat /sys/devices/platform/i8042/serio1/firmware_id 
PNP0f03 PNP0f13

Regards,

Hans

[PATCH 1/2] input/serio: Add a firmware_id sysfs attribute

[Hans de Goede]

serio devices exposed via platform firmware interfaces such as ACPI
may provide additional identifying information of use to userspace.

We don't associate the serio devices with the firmware device (we don't
set it as parent), so there's no way for userspace to make use of this
information.

We cannot change the parent for serio devices instantiated though a firmware
interface as that would break suspend / resume ordering.

Therefor this patch adds a new firmware_id sysfs attribute so that userspace
can get a string from there with any additional identifying information the
firmware interface may provide.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/serio/serio.c | 12 ++++++++++++
 include/linux/serio.h       |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index 8f4c4ab..1788a4d 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -451,6 +451,13 @@ static ssize_t serio_set_bind_mode(struct device *dev, struct device_attribute *
 	return retval;
 }
 
+static ssize_t firmware_id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct serio *serio = to_serio_port(dev);
+
+	return sprintf(buf, "%s\n", serio->firmware_id);
+}
+
 static DEVICE_ATTR_RO(type);
 static DEVICE_ATTR_RO(proto);
 static DEVICE_ATTR_RO(id);
@@ -473,12 +480,14 @@ static DEVICE_ATTR_RO(modalias);
 static DEVICE_ATTR_WO(drvctl);
 static DEVICE_ATTR(description, S_IRUGO, serio_show_description, NULL);
 static DEVICE_ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode);
+static DEVICE_ATTR_RO(firmware_id);
 
 static struct attribute *serio_device_attrs[] = {
 	&dev_attr_modalias.attr,
 	&dev_attr_description.attr,
 	&dev_attr_drvctl.attr,
 	&dev_attr_bind_mode.attr,
+	&dev_attr_firmware_id.attr,
 	NULL
 };
 
@@ -923,6 +932,9 @@ static int serio_uevent(struct device *dev, struct kobj_uevent_env *env)
 	SERIO_ADD_UEVENT_VAR("SERIO_EXTRA=%02x", serio->id.extra);
 	SERIO_ADD_UEVENT_VAR("MODALIAS=serio:ty%02Xpr%02Xid%02Xex%02X",
 				serio->id.type, serio->id.proto, serio->id.id, serio->id.extra);
+	if (serio->firmware_id[0])
+		SERIO_ADD_UEVENT_VAR("SERIO_FIRMWARE_ID=%s",
+				     serio->firmware_id);
 
 	return 0;
 }
diff --git a/include/linux/serio.h b/include/linux/serio.h
index 36aac73..9f779c7 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -23,6 +23,7 @@ struct serio {
 
 	char name[32];
 	char phys[32];
+	char firmware_id[128];
 
 	bool manual_bind;
 
-- 
1.9.0

[PATCH 2/2] input/serio/8042: Add firmware_id support

[Hans de Goede]

Fill in the new serio firmware_id sysfs attribute for pnp instantiated
8042 serio ports.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/serio/i8042-x86ia64io.h | 26 ++++++++++++++++++++++++++
 drivers/input/serio/i8042.c           |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
index 0ec9abb..3f9da83 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -704,6 +704,8 @@ static char i8042_pnp_aux_name[32];
 
 static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *did)
 {
+	struct pnp_id *id = dev->id;
+
 	if (pnp_port_valid(dev, 0) && pnp_port_len(dev, 0) == 1)
 		i8042_pnp_data_reg = pnp_port_start(dev,0);
 
@@ -719,6 +721,17 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
 		strlcat(i8042_pnp_kbd_name, pnp_dev_name(dev), sizeof(i8042_pnp_kbd_name));
 	}
 
+	if (id) {
+		strlcpy(i8042_kbd_firmware_id, id->id,
+			sizeof(i8042_kbd_firmware_id));
+		for (id = id->next; id; id = id->next) {
+			strlcat(i8042_kbd_firmware_id, " ",
+				sizeof(i8042_kbd_firmware_id));
+			strlcat(i8042_kbd_firmware_id, id->id,
+				sizeof(i8042_kbd_firmware_id));
+		}
+	}
+
 	/* Keyboard ports are always supposed to be wakeup-enabled */
 	device_set_wakeup_enable(&dev->dev, true);
 
@@ -728,6 +741,8 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
 
 static int i8042_pnp_aux_probe(struct pnp_dev *dev, const struct pnp_device_id *did)
 {
+	struct pnp_id *id = dev->id;
+
 	if (pnp_port_valid(dev, 0) && pnp_port_len(dev, 0) == 1)
 		i8042_pnp_data_reg = pnp_port_start(dev,0);
 
@@ -743,6 +758,17 @@ static int i8042_pnp_aux_probe(struct pnp_dev *dev, const struct pnp_device_id *
 		strlcat(i8042_pnp_aux_name, pnp_dev_name(dev), sizeof(i8042_pnp_aux_name));
 	}
 
+	if (id) {
+		strlcpy(i8042_aux_firmware_id, id->id,
+			sizeof(i8042_aux_firmware_id));
+		for (id = id->next; id; id = id->next) {
+			strlcat(i8042_aux_firmware_id, " ",
+				sizeof(i8042_aux_firmware_id));
+			strlcat(i8042_aux_firmware_id, id->id,
+				sizeof(i8042_aux_firmware_id));
+		}
+	}
+
 	i8042_pnp_aux_devices++;
 	return 0;
 }
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 020053f..3807c3e 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -87,6 +87,8 @@ MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
 #endif
 
 static bool i8042_bypass_aux_irq_test;
+static char i8042_kbd_firmware_id[128];
+static char i8042_aux_firmware_id[128];
 
 #include "i8042.h"
 
@@ -1218,6 +1220,8 @@ static int __init i8042_create_kbd_port(void)
 	serio->dev.parent	= &i8042_platform_device->dev;
 	strlcpy(serio->name, "i8042 KBD port", sizeof(serio->name));
 	strlcpy(serio->phys, I8042_KBD_PHYS_DESC, sizeof(serio->phys));
+	strlcpy(serio->firmware_id, i8042_kbd_firmware_id,
+		sizeof(serio->firmware_id));
 
 	port->serio = serio;
 	port->irq = I8042_KBD_IRQ;
@@ -1244,6 +1248,8 @@ static int __init i8042_create_aux_port(int idx)
 	if (idx < 0) {
 		strlcpy(serio->name, "i8042 AUX port", sizeof(serio->name));
 		strlcpy(serio->phys, I8042_AUX_PHYS_DESC, sizeof(serio->phys));
+		strlcpy(serio->firmware_id, i8042_aux_firmware_id,
+			sizeof(serio->firmware_id));
 		serio->close = i8042_port_close;
 	} else {
 		snprintf(serio->name, sizeof(serio->name), "i8042 AUX%d port", idx);
-- 
1.9.0

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Matthew Garrett]

On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:

> Which in the end turns out to be much nicer too, since it gets rid of needing
> a udev-helper too. After this much too long introduction I'll let the patches
> speak for themselves.

Yeah, I was coming to the conclusion that this was probably the best we 
could do. It's unfortunate that "id" is already in use - we'd be able to 
get away without any X server modifications otherwise.

Long term we probably still want to tie serio devices to the ACPI 
devices in case the vendor provides power management calls there, but we 
can leave that until there's an actual example.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Peter Hutterer]

On 21/03/14 03:21, Matthew Garrett wrote:
> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
>
>> Which in the end turns out to be much nicer too, since it gets rid of needing
>> a udev-helper too. After this much too long introduction I'll let the patches
>> speak for themselves.
>
> Yeah, I was coming to the conclusion that this was probably the best we
> could do. It's unfortunate that "id" is already in use - we'd be able to
> get away without any X server modifications otherwise.

IMO we don't need to worry about that. this is needed for a set of 
devices that need a kernel patch, several synaptics patches and a server 
patch to work properly anyway, so changes are already required. Approach 
looks good, ACK from me.

Cheers,
   Peter

>
> Long term we probably still want to tie serio devices to the ACPI
> devices in case the vendor provides power management calls there, but we
> can leave that until there's an actual example.
>

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Dmitry Torokhov]

On Thu, Mar 20, 2014 at 05:21:59PM +0000, Matthew Garrett wrote:
> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
> 
> > Which in the end turns out to be much nicer too, since it gets rid of needing
> > a udev-helper too. After this much too long introduction I'll let the patches
> > speak for themselves.
> 
> Yeah, I was coming to the conclusion that this was probably the best we 
> could do. It's unfortunate that "id" is already in use - we'd be able to 
> get away without any X server modifications otherwise.
> 
> Long term we probably still want to tie serio devices to the ACPI 
> devices in case the vendor provides power management calls there, but we 
> can leave that until there's an actual example.

I am still unsure if we shoudl be adding these new IDs to serio core...
Can't the X driver take a peek at ACPI devices on it's own?

Thanks.

-- 
Dmitry

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Hans de Goede]

Hi,

On 03/28/2014 08:56 AM, Dmitry Torokhov wrote:
> On Thu, Mar 20, 2014 at 05:21:59PM +0000, Matthew Garrett wrote:
>> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
>>
>>> Which in the end turns out to be much nicer too, since it gets rid of needing
>>> a udev-helper too. After this much too long introduction I'll let the patches
>>> speak for themselves.
>>
>> Yeah, I was coming to the conclusion that this was probably the best we 
>> could do. It's unfortunate that "id" is already in use - we'd be able to 
>> get away without any X server modifications otherwise.
>>
>> Long term we probably still want to tie serio devices to the ACPI 
>> devices in case the vendor provides power management calls there, but we 
>> can leave that until there's an actual example.
> 
> I am still unsure if we shoudl be adding these new IDs to serio core...
> Can't the X driver take a peek at ACPI devices on it's own?

The problem is there is no way for userspace to know which /sys/devices/pnp0/00:xx
device is the serio bus host.

Regards,

Hans

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Dmitry Torokhov]

On Fri, Mar 28, 2014 at 09:12:43AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 03/28/2014 08:56 AM, Dmitry Torokhov wrote:
> > On Thu, Mar 20, 2014 at 05:21:59PM +0000, Matthew Garrett wrote:
> >> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
> >>
> >>> Which in the end turns out to be much nicer too, since it gets rid of needing
> >>> a udev-helper too. After this much too long introduction I'll let the patches
> >>> speak for themselves.
> >>
> >> Yeah, I was coming to the conclusion that this was probably the best we 
> >> could do. It's unfortunate that "id" is already in use - we'd be able to 
> >> get away without any X server modifications otherwise.
> >>
> >> Long term we probably still want to tie serio devices to the ACPI 
> >> devices in case the vendor provides power management calls there, but we 
> >> can leave that until there's an actual example.
> > 
> > I am still unsure if we shoudl be adding these new IDs to serio core...
> > Can't the X driver take a peek at ACPI devices on it's own?
> 
> The problem is there is no way for userspace to know which /sys/devices/pnp0/00:xx
> device is the serio bus host.

Practically speaking you should not care - there is only one touchpad in
Lenovos.

Thanks.

-- 
Dmitry

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Matthew Garrett]

On Fri, Mar 28, 2014 at 12:56:55AM -0700, Dmitry Torokhov wrote:

> I am still unsure if we shoudl be adding these new IDs to serio core...
> Can't the X driver take a peek at ACPI devices on it's own?

In the (admittedly unlikely) event of multiple PS/2 trackpads, how do 
you know which one corresponds to which ACPI device?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Dmitry Torokhov]

On Fri, Mar 28, 2014 at 08:20:04AM +0000, Matthew Garrett wrote:
> On Fri, Mar 28, 2014 at 12:56:55AM -0700, Dmitry Torokhov wrote:
> 
> > I am still unsure if we shoudl be adding these new IDs to serio core...
> > Can't the X driver take a peek at ACPI devices on it's own?
> 
> In the (admittedly unlikely) event of multiple PS/2 trackpads, how do 
> you know which one corresponds to which ACPI device?

So far I have not seen a single instance of a laptop with 2 touchpads
and I doubt external PS/2 ones will ever make come back.

-- 
Dmitry

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Matthew Garrett]

On Fri, Mar 28, 2014 at 01:24:52AM -0700, Dmitry Torokhov wrote:
> On Fri, Mar 28, 2014 at 08:20:04AM +0000, Matthew Garrett wrote:
> > On Fri, Mar 28, 2014 at 12:56:55AM -0700, Dmitry Torokhov wrote:
> > 
> > > I am still unsure if we shoudl be adding these new IDs to serio core...
> > > Can't the X driver take a peek at ACPI devices on it's own?
> > 
> > In the (admittedly unlikely) event of multiple PS/2 trackpads, how do 
> > you know which one corresponds to which ACPI device?
> 
> So far I have not seen a single instance of a laptop with 2 touchpads
> and I doubt external PS/2 ones will ever make come back.

Right, we can hack around it based on what we've seen so far. But it 
seems more attractive to fix it in such a way that we won't behave 
inappropriately even if someone does do something utterly unexpected in 
future. For instance, some ARM devices have i8042-like serio - if the 
underlying device is exposed in device tree, it'd be nice to be able to 
expose that to userspace without having to modify the core X code.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Hans de Goede]

Hi,

On 03/28/2014 09:17 AM, Dmitry Torokhov wrote:
> On Fri, Mar 28, 2014 at 09:12:43AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 03/28/2014 08:56 AM, Dmitry Torokhov wrote:
>>> On Thu, Mar 20, 2014 at 05:21:59PM +0000, Matthew Garrett wrote:
>>>> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
>>>>
>>>>> Which in the end turns out to be much nicer too, since it gets rid of needing
>>>>> a udev-helper too. After this much too long introduction I'll let the patches
>>>>> speak for themselves.
>>>>
>>>> Yeah, I was coming to the conclusion that this was probably the best we 
>>>> could do. It's unfortunate that "id" is already in use - we'd be able to 
>>>> get away without any X server modifications otherwise.
>>>>
>>>> Long term we probably still want to tie serio devices to the ACPI 
>>>> devices in case the vendor provides power management calls there, but we 
>>>> can leave that until there's an actual example.
>>>
>>> I am still unsure if we shoudl be adding these new IDs to serio core...
>>> Can't the X driver take a peek at ACPI devices on it's own?
>>
>> The problem is there is no way for userspace to know which /sys/devices/pnp0/00:xx
>> device is the serio bus host.
> 
> Practically speaking you should not care - there is only one touchpad in
> Lenovos.

So are you suggesting we simply go over all /sys/devices/pnp0/00:xx devices looking
for a pnp-id we're interested  in ?  I'm sorry but that is just a non-sense solution,
which reminds me of the good old days of random poking io-ports to probe stuff.

We're not blindly going to read every /sys/devices/pnp0/00:xx/id attribute on a
system, assuming that if it contains a pnp-id we're interested in it happens to
belong to the input device we're enumerating at that time.

Regards,

Hans

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Dmitry Torokhov]

On Fri, Mar 28, 2014 at 08:27:04AM +0000, Matthew Garrett wrote:
> On Fri, Mar 28, 2014 at 01:24:52AM -0700, Dmitry Torokhov wrote:
> > On Fri, Mar 28, 2014 at 08:20:04AM +0000, Matthew Garrett wrote:
> > > On Fri, Mar 28, 2014 at 12:56:55AM -0700, Dmitry Torokhov wrote:
> > > 
> > > > I am still unsure if we shoudl be adding these new IDs to serio core...
> > > > Can't the X driver take a peek at ACPI devices on it's own?
> > > 
> > > In the (admittedly unlikely) event of multiple PS/2 trackpads, how do 
> > > you know which one corresponds to which ACPI device?
> > 
> > So far I have not seen a single instance of a laptop with 2 touchpads
> > and I doubt external PS/2 ones will ever make come back.
> 
> Right, we can hack around it based on what we've seen so far. But it 
> seems more attractive to fix it in such a way that we won't behave 
> inappropriately even if someone does do something utterly unexpected in 
> future. For instance, some ARM devices have i8042-like serio - if the 
> underlying device is exposed in device tree, it'd be nice to be able to 
> expose that to userspace without having to modify the core X code.

I wonder if on ARM they have the same split description for AUX and KBD
ports or of they have proper i8042 parent. ACPI way of describing PS/2
devices is ugly - keyboard gets ports and mice/touchpads are portless
and everyone knows that they should use the same as keyboard.

I am unconvinced that just storing a string as firmware ID would solve
anything other than quirky Lenovos.

Thanks.

-- 
Dmitry

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Dmitry Torokhov]

On Fri, Mar 28, 2014 at 09:29:50AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 03/28/2014 09:17 AM, Dmitry Torokhov wrote:
> > On Fri, Mar 28, 2014 at 09:12:43AM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 03/28/2014 08:56 AM, Dmitry Torokhov wrote:
> >>> On Thu, Mar 20, 2014 at 05:21:59PM +0000, Matthew Garrett wrote:
> >>>> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
> >>>>
> >>>>> Which in the end turns out to be much nicer too, since it gets rid of needing
> >>>>> a udev-helper too. After this much too long introduction I'll let the patches
> >>>>> speak for themselves.
> >>>>
> >>>> Yeah, I was coming to the conclusion that this was probably the best we 
> >>>> could do. It's unfortunate that "id" is already in use - we'd be able to 
> >>>> get away without any X server modifications otherwise.
> >>>>
> >>>> Long term we probably still want to tie serio devices to the ACPI 
> >>>> devices in case the vendor provides power management calls there, but we 
> >>>> can leave that until there's an actual example.
> >>>
> >>> I am still unsure if we shoudl be adding these new IDs to serio core...
> >>> Can't the X driver take a peek at ACPI devices on it's own?
> >>
> >> The problem is there is no way for userspace to know which /sys/devices/pnp0/00:xx
> >> device is the serio bus host.
> > 
> > Practically speaking you should not care - there is only one touchpad in
> > Lenovos.
> 
> So are you suggesting we simply go over all /sys/devices/pnp0/00:xx devices looking
> for a pnp-id we're interested  in ?  I'm sorry but that is just a non-sense solution,
> which reminds me of the good old days of random poking io-ports to probe stuff.
> 
> We're not blindly going to read every /sys/devices/pnp0/00:xx/id attribute on a
> system, assuming that if it contains a pnp-id we're interested in it happens to
> belong to the input device we're enumerating at that time.

Are we even certain that they will be consistent in use of these special
PNP ID's? Maybe you should really do DMI match...

Thanks.

-- 
Dmitry

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Matthew Garrett]

On Fri, Mar 28, 2014 at 01:52:07AM -0700, Dmitry Torokhov wrote:
> 
> Are we even certain that they will be consistent in use of these special
> PNP ID's? Maybe you should really do DMI match...

The Windows drivers bind on PNP IDs, not DMI.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Hans de Goede]

Hi,

On 03/28/2014 09:52 AM, Dmitry Torokhov wrote:
> On Fri, Mar 28, 2014 at 09:29:50AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 03/28/2014 09:17 AM, Dmitry Torokhov wrote:
>>> On Fri, Mar 28, 2014 at 09:12:43AM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 03/28/2014 08:56 AM, Dmitry Torokhov wrote:
>>>>> On Thu, Mar 20, 2014 at 05:21:59PM +0000, Matthew Garrett wrote:
>>>>>> On Thu, Mar 20, 2014 at 11:12:08AM +0100, Hans de Goede wrote:
>>>>>>
>>>>>>> Which in the end turns out to be much nicer too, since it gets rid of needing
>>>>>>> a udev-helper too. After this much too long introduction I'll let the patches
>>>>>>> speak for themselves.
>>>>>>
>>>>>> Yeah, I was coming to the conclusion that this was probably the best we 
>>>>>> could do. It's unfortunate that "id" is already in use - we'd be able to 
>>>>>> get away without any X server modifications otherwise.
>>>>>>
>>>>>> Long term we probably still want to tie serio devices to the ACPI 
>>>>>> devices in case the vendor provides power management calls there, but we 
>>>>>> can leave that until there's an actual example.
>>>>>
>>>>> I am still unsure if we shoudl be adding these new IDs to serio core...
>>>>> Can't the X driver take a peek at ACPI devices on it's own?
>>>>
>>>> The problem is there is no way for userspace to know which /sys/devices/pnp0/00:xx
>>>> device is the serio bus host.
>>>
>>> Practically speaking you should not care - there is only one touchpad in
>>> Lenovos.
>>
>> So are you suggesting we simply go over all /sys/devices/pnp0/00:xx devices looking
>> for a pnp-id we're interested  in ?  I'm sorry but that is just a non-sense solution,
>> which reminds me of the good old days of random poking io-ports to probe stuff.
>>
>> We're not blindly going to read every /sys/devices/pnp0/00:xx/id attribute on a
>> system, assuming that if it contains a pnp-id we're interested in it happens to
>> belong to the input device we're enumerating at that time.
> 
> Are we even certain that they will be consistent in use of these special
> PNP ID's? Maybe you should really do DMI match...

They are more consistent with PNP ids then with their DMI strings, that is if they
re-use the exact same touchpad the PNP id stays the same. So PNP ids give us a
better chance of things just working without the user needing to wait for a distro
update which has the new DMI strings in there.

And yes so far the PNP ids use we want to-do is Lenovo only, as we've just discovered
that they are actually storing some useful info in there. If we need quirks for other
vendor laptops in the future, chances are we may want to do pnp-id matching there too.

I've deliberately tried to write this patch-set so that it adds a generic way to export
extra info the platform specific code may have. I could have added a pnp-id pointer to
the serio struct, but I deliberately went with a free-form string so as to make this
as generic as possible. As Matthew has already said we may want to also export extra
info from devicetree through this mechanism for example.

Regards,

Hans

Re: [PATCH 0/2] input/serio: Add a firmware_id sysfs attribute

[Dmitry Torokhov]

On Fri, Mar 28, 2014 at 09:00:53AM +0000, Matthew Garrett wrote:
> On Fri, Mar 28, 2014 at 01:52:07AM -0700, Dmitry Torokhov wrote:
> > 
> > Are we even certain that they will be consistent in use of these special
> > PNP ID's? Maybe you should really do DMI match...
> 
> The Windows drivers bind on PNP IDs, not DMI.

OK, fair enough.

-- 
Dmitry