Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644604 > unrolled thread

[PATCH 10/24] thunderbolt: Read vendor and device name from DROM

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-18 16:40 +0200
    Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-18 21:20 +0200
      Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-19 10:30 +0200
    Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Lukas Wunner <lukas@wunner.de> - 2017-05-19 12:10 +0200
      Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-19 12:30 +0200
        Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Lukas Wunner <lukas@wunner.de> - 2017-05-21 07:40 +0200
          Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-21 09:50 +0200
            Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM Lukas Wunner <lukas@wunner.de> - 2017-05-21 11:40 +0200

#1644604 — [PATCH 10/24] thunderbolt: Read vendor and device name from DROM

The device DROM contains name of the vendor and device among other
things. Extract this information and expose it to the userspace via two
new attributes.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
Reviewed-by: Michael Jamet <michael.jamet@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-thunderbolt | 14 ++++++++++++++
 drivers/thunderbolt/eeprom.c                    | 23 ++++++++++++++++++++++-
 drivers/thunderbolt/switch.c                    | 22 ++++++++++++++++++++++
 drivers/thunderbolt/tb.h                        |  4 ++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index a3dac3becd1e..2f352c787431 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -5,6 +5,13 @@ Contact:	thunderbolt-software@lists.01.org
 Description:	This attribute contains id of this device extracted from
 		the device DROM.
 
+What:		/sys/bus/thunderbolt/devices/.../device_name
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	thunderbolt-software@lists.01.org
+Description:	This attribute contains name of this device extracted from
+		the device DROM.
+
 What:		/sys/bus/thunderbolt/devices/.../vendor
 Date:		Sep 2017
 KernelVersion:	4.13
@@ -12,6 +19,13 @@ Contact:	thunderbolt-software@lists.01.org
 Description:	This attribute contains vendor id of this device extracted
 		from the device DROM.
 
+What:		/sys/bus/thunderbolt/devices/.../vendor_name
+Date:		Sep 2017
+KernelVersion:	4.13
+Contact:	thunderbolt-software@lists.01.org
+Description:	This attribute contains vendor name of this device extracted
+		from the device DROM.
+
 What:		/sys/bus/thunderbolt/devices/.../unique_id
 Date:		Sep 2017
 KernelVersion:	4.13
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index e2c1f8a45522..f688fb255042 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -204,6 +204,11 @@ struct tb_drom_entry_header {
 	enum tb_drom_entry_type type:1;
 } __packed;
 
+struct tb_drom_entry_generic {
+	struct tb_drom_entry_header header;
+	u8 data[0];
+} __packed;
+
 struct tb_drom_entry_port {
 	/* BYTES 0-1 */
 	struct tb_drom_entry_header header;
@@ -304,6 +309,15 @@ static void tb_drom_parse_port_entry(struct tb_port *port,
 				&port->sw->ports[entry->dual_link_port_nr];
 }
 
+static void tb_drom_parse_generic_entry(struct tb_switch *sw,
+		struct tb_drom_entry_generic *entry)
+{
+	if (entry->header.index == 1)
+		sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL);
+	else if (entry->header.index == 2)
+		sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL);
+}
+
 static int tb_drom_parse_entry(struct tb_switch *sw,
 		struct tb_drom_entry_header *header)
 {
@@ -311,8 +325,15 @@ static int tb_drom_parse_entry(struct tb_switch *sw,
 	int res;
 	enum tb_port_type type;
 
-	if (header->type != TB_DROM_ENTRY_PORT)
+	switch (header->type) {
+	case TB_DROM_ENTRY_PORT:
+		break;
+	case TB_DROM_ENTRY_GENERIC:
+		tb_drom_parse_generic_entry(sw,
+			(struct tb_drom_entry_generic *)header);
+	default:
 		return 0;
+	}
 
 	port = &sw->ports[header->index];
 	port->disabled = header->port_disabled;
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 4a961d174cad..b06de0efbdfc 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -319,6 +319,15 @@ static ssize_t device_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(device);
 
+static ssize_t
+device_name_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
+
+	return sprintf(buf, "%s\n", sw->device_name ? sw->device_name : "");
+}
+static DEVICE_ATTR_RO(device_name);
+
 static ssize_t vendor_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
@@ -328,6 +337,15 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(vendor);
 
+static ssize_t
+vendor_name_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct tb_switch *sw = tb_to_switch(dev);
+
+	return sprintf(buf, "%s\n", sw->vendor_name ? sw->vendor_name : "");
+}
+static DEVICE_ATTR_RO(vendor_name);
+
 static ssize_t unique_id_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
@@ -339,7 +357,9 @@ static DEVICE_ATTR_RO(unique_id);
 
 static struct attribute *switch_attrs[] = {
 	&dev_attr_device.attr,
+	&dev_attr_device_name.attr,
 	&dev_attr_vendor.attr,
+	&dev_attr_vendor_name.attr,
 	&dev_attr_unique_id.attr,
 	NULL,
 };
@@ -350,6 +370,8 @@ static void tb_switch_release(struct device *dev)
 	struct tb_switch *sw = tb_to_switch(dev);
 
 	kfree(sw->uuid);
+	kfree(sw->device_name);
+	kfree(sw->vendor_name);
 	kfree(sw->ports);
 	kfree(sw->drom);
 	kfree(sw);
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 350c3f21924e..5e66dce53c65 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -23,6 +23,8 @@
  * @uuid: UUID of the switch (or %NULL if not supported)
  * @vendor: Vendor ID of the switch
  * @device: Device ID of the switch
+ * @vendor_name: Name of the vendor (or %NULL if not known)
+ * @device_name: Name of the device (or %NULL if not known)
  * @cap_plug_events: Offset to the plug events capability (%0 if not found)
  * @is_unplugged: The switch is going away
  * @drom: DROM of the switch (%NULL if not found)
@@ -36,6 +38,8 @@ struct tb_switch {
 	uuid_be *uuid;
 	u16 vendor;
 	u16 device;
+	const char *vendor_name;
+	const char *device_name;
 	int cap_plug_events;
 	bool is_unplugged;
 	u8 *drom;
-- 
2.11.0

[toc] | [next] | [standalone]

#1644851

On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> The device DROM contains name of the vendor and device among other
> things. Extract this information and expose it to the userspace via two
> new attributes.

One nit below.

> +       switch (header->type) {
> +       case TB_DROM_ENTRY_PORT:
> +               break;
> +       case TB_DROM_ENTRY_GENERIC:

> +               tb_drom_parse_generic_entry(sw,
> +                       (struct tb_drom_entry_generic *)header);

Can it be one line?
Is fall through intended?

> +       default:
>                 return 0;
> +       }

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1645353

On Thu, May 18, 2017 at 10:19:11PM +0300, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > The device DROM contains name of the vendor and device among other
> > things. Extract this information and expose it to the userspace via two
> > new attributes.
> 
> One nit below.
> 
> > +       switch (header->type) {
> > +       case TB_DROM_ENTRY_PORT:
> > +               break;
> > +       case TB_DROM_ENTRY_GENERIC:
> 
> > +               tb_drom_parse_generic_entry(sw,
> > +                       (struct tb_drom_entry_generic *)header);
> 
> Can it be one line?

It does not fit into 80 char limit.

> Is fall through intended?

Yes.

> > +       default:
> >                 return 0;
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1645490

Hi Mika,

nice work, by now I've picked up my jaw from the floor and can
offer a few comments...


On Thu, May 18, 2017 at 05:39:00PM +0300, Mika Westerberg wrote:
> The device DROM contains name of the vendor and device among other
> things.

What exactly are these other things?  Apple uses 0x30 to store a
serial number.  Is this attribute number assigned by Intel to Apple
or is it reserved for vendor use or did they arbitrarily choose it?

If there can be many attributes, should they be stored in a list
rather than adding a char* pointer for each one to struct tb_switch?
The latter doesn't scale.


> +static void tb_drom_parse_generic_entry(struct tb_switch *sw,
> +		struct tb_drom_entry_generic *entry)
> +{
> +	if (entry->header.index == 1)
> +		sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL);
> +	else if (entry->header.index == 2)
> +		sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL);
> +}

This assumes that these are properly null-terminated strings, but the DROM
may contain complete garbage.  The existing drom parser is very careful
to validate and sanitize everything.


>  static int tb_drom_parse_entry(struct tb_switch *sw,
>  		struct tb_drom_entry_header *header)
>  {
> @@ -311,8 +325,15 @@ static int tb_drom_parse_entry(struct tb_switch *sw,
>  	int res;
>  	enum tb_port_type type;
>  
> -	if (header->type != TB_DROM_ENTRY_PORT)
> +	switch (header->type) {
> +	case TB_DROM_ENTRY_PORT:
> +		break;
> +	case TB_DROM_ENTRY_GENERIC:
> +		tb_drom_parse_generic_entry(sw,
> +			(struct tb_drom_entry_generic *)header);
> +	default:
>  		return 0;
> +	}
>  
>  	port = &sw->ports[header->index];
>  	port->disabled = header->port_disabled;

I'm afraid this control flow is not very pretty, the stuff below the
switch/case statement is essentially the parser for TB_DROM_ENTRY_PORT
whereas the parser for TB_DROM_ENTRY_GENERIC is in a separate function.
It would be easier to follow the control flow if the parser for
TB_DROM_ENTRY_PORT was in a separate function tb_drom_parse_port_entry().

In fact I wrote patches to do just that one and a half years ago but
haven't upstreamed them so far, mostly because I was unsure how many
attributes there can be, if they should be stored in a list, etc.
I didn't have access to the same resources as you do.

https://github.com/l1k/linux/commit/b6c9db73258b
https://github.com/l1k/linux/commit/d1b46362b528

Feel free to include them in full or in part in your series
or modify as you see fit.

The latter patch also includes a sanitizer for generic entries.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1645496

On Fri, May 19, 2017 at 12:07:10PM +0200, Lukas Wunner wrote:
> Hi Mika,
> 
> nice work, by now I've picked up my jaw from the floor and can
> offer a few comments...

Thanks! :)

> On Thu, May 18, 2017 at 05:39:00PM +0300, Mika Westerberg wrote:
> > The device DROM contains name of the vendor and device among other
> > things.
> 
> What exactly are these other things?  Apple uses 0x30 to store a
> serial number.  Is this attribute number assigned by Intel to Apple
> or is it reserved for vendor use or did they arbitrarily choose it?

It is part of the DROM specification. The 0x30 - 0x3e are vendor
specific entries.

There are couple of other things but I don't think they are useful to
us to be honest.

> If there can be many attributes, should they be stored in a list
> rather than adding a char* pointer for each one to struct tb_switch?
> The latter doesn't scale.

I don't think we need other attributes (well, at least right now). The
device/vendor name is useful because that's what we expose to the
userspace for device identification along with the device/vendor ID.

> > +static void tb_drom_parse_generic_entry(struct tb_switch *sw,
> > +		struct tb_drom_entry_generic *entry)
> > +{
> > +	if (entry->header.index == 1)
> > +		sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > +	else if (entry->header.index == 2)
> > +		sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > +}
> 
> This assumes that these are properly null-terminated strings, but the DROM
> may contain complete garbage.  The existing drom parser is very careful
> to validate and sanitize everything.

The DROM specification says they must be null-terminated but I yes, it
is possible that some of the devices have it wrong. The generic entry
includes length field so I suppose we can use that + kmemdup() instead
here?

> >  static int tb_drom_parse_entry(struct tb_switch *sw,
> >  		struct tb_drom_entry_header *header)
> >  {
> > @@ -311,8 +325,15 @@ static int tb_drom_parse_entry(struct tb_switch *sw,
> >  	int res;
> >  	enum tb_port_type type;
> >  
> > -	if (header->type != TB_DROM_ENTRY_PORT)
> > +	switch (header->type) {
> > +	case TB_DROM_ENTRY_PORT:
> > +		break;
> > +	case TB_DROM_ENTRY_GENERIC:
> > +		tb_drom_parse_generic_entry(sw,
> > +			(struct tb_drom_entry_generic *)header);
> > +	default:
> >  		return 0;
> > +	}
> >  
> >  	port = &sw->ports[header->index];
> >  	port->disabled = header->port_disabled;
> 
> I'm afraid this control flow is not very pretty, the stuff below the
> switch/case statement is essentially the parser for TB_DROM_ENTRY_PORT
> whereas the parser for TB_DROM_ENTRY_GENERIC is in a separate function.
> It would be easier to follow the control flow if the parser for
> TB_DROM_ENTRY_PORT was in a separate function tb_drom_parse_port_entry().
> 
> In fact I wrote patches to do just that one and a half years ago but
> haven't upstreamed them so far, mostly because I was unsure how many
> attributes there can be, if they should be stored in a list, etc.
> I didn't have access to the same resources as you do.
> 
> https://github.com/l1k/linux/commit/b6c9db73258b
> https://github.com/l1k/linux/commit/d1b46362b528

Cool.

> Feel free to include them in full or in part in your series
> or modify as you see fit.
> 
> The latter patch also includes a sanitizer for generic entries.

OK, I'll take a look at them and see if we can use them here :)

[toc] | [prev] | [next] | [standalone]

#1646261

On Fri, May 19, 2017 at 01:28:36PM +0300, Mika Westerberg wrote:
> On Fri, May 19, 2017 at 12:07:10PM +0200, Lukas Wunner wrote:
> > Apple uses 0x30 to store a
> > serial number.  Is this attribute number assigned by Intel to Apple
> > or is it reserved for vendor use or did they arbitrarily choose it?
> 
> It is part of the DROM specification. The 0x30 - 0x3e are vendor
> specific entries.

Ah, so I have to qualify the vendor number with Apple's ID before I know
that it's a serial number.  Thanks.


> > If there can be many attributes, should they be stored in a list
> > rather than adding a char* pointer for each one to struct tb_switch?
> > The latter doesn't scale.
> 
> I don't think we need other attributes (well, at least right now). The
> device/vendor name is useful because that's what we expose to the
> userspace for device identification along with the device/vendor ID.

Okay.  It might be worth to log additional attributes with info level.


> > > +static void tb_drom_parse_generic_entry(struct tb_switch *sw,
> > > +		struct tb_drom_entry_generic *entry)
> > > +{
> > > +	if (entry->header.index == 1)
> > > +		sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > > +	else if (entry->header.index == 2)
> > > +		sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > > +}
> > 
> > This assumes that these are properly null-terminated strings, but the DROM
> > may contain complete garbage.  The existing drom parser is very careful
> > to validate and sanitize everything.
> 
> The DROM specification says they must be null-terminated but I yes, it
> is possible that some of the devices have it wrong. The generic entry
> includes length field so I suppose we can use that + kmemdup() instead
> here?

Yes, as long as you check that the last character is null.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1646277

On Sun, May 21, 2017 at 07:31:14AM +0200, Lukas Wunner wrote:
> On Fri, May 19, 2017 at 01:28:36PM +0300, Mika Westerberg wrote:
> > On Fri, May 19, 2017 at 12:07:10PM +0200, Lukas Wunner wrote:
> > > Apple uses 0x30 to store a
> > > serial number.  Is this attribute number assigned by Intel to Apple
> > > or is it reserved for vendor use or did they arbitrarily choose it?
> > 
> > It is part of the DROM specification. The 0x30 - 0x3e are vendor
> > specific entries.
> 
> Ah, so I have to qualify the vendor number with Apple's ID before I know
> that it's a serial number.  Thanks.

Yes, something like that works.

> > > If there can be many attributes, should they be stored in a list
> > > rather than adding a char* pointer for each one to struct tb_switch?
> > > The latter doesn't scale.
> > 
> > I don't think we need other attributes (well, at least right now). The
> > device/vendor name is useful because that's what we expose to the
> > userspace for device identification along with the device/vendor ID.
> 
> Okay.  It might be worth to log additional attributes with info level.

I don't think we want to log anything with info level to be honest. The
driver currently already is pretty noisy so adding even more information
there just makes it worse ;-)

I would rather convert debugging information to use tracepoints and get
rid of the tb_*_info() things completely.

The whole DROM content is already available through nvm_active/nvmem
file under each device (well starting with Alpine Ridge) so the
userspace can investigate it as much as it likes without spamming the
kernel dmesg :)

> > > > +static void tb_drom_parse_generic_entry(struct tb_switch *sw,
> > > > +		struct tb_drom_entry_generic *entry)
> > > > +{
> > > > +	if (entry->header.index == 1)
> > > > +		sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > > > +	else if (entry->header.index == 2)
> > > > +		sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > > > +}
> > > 
> > > This assumes that these are properly null-terminated strings, but the DROM
> > > may contain complete garbage.  The existing drom parser is very careful
> > > to validate and sanitize everything.
> > 
> > The DROM specification says they must be null-terminated but I yes, it
> > is possible that some of the devices have it wrong. The generic entry
> > includes length field so I suppose we can use that + kmemdup() instead
> > here?
> 
> Yes, as long as you check that the last character is null.

OK, I'll do that then. Thanks.

[toc] | [prev] | [next] | [standalone]

#1646298

On Sun, May 21, 2017 at 10:48:19AM +0300, Mika Westerberg wrote:
> On Sun, May 21, 2017 at 07:31:14AM +0200, Lukas Wunner wrote:
> > On Fri, May 19, 2017 at 01:28:36PM +0300, Mika Westerberg wrote:
> > > On Fri, May 19, 2017 at 12:07:10PM +0200, Lukas Wunner wrote:
> > > > If there can be many attributes, should they be stored in a list
> > > > rather than adding a char* pointer for each one to struct tb_switch?
> > > > The latter doesn't scale.
> > > 
> > > I don't think we need other attributes (well, at least right now). The
> > > device/vendor name is useful because that's what we expose to the
> > > userspace for device identification along with the device/vendor ID.
> > 
> > Okay.  It might be worth to log additional attributes with info level.
> 
> I don't think we want to log anything with info level to be honest. The
> driver currently already is pretty noisy so adding even more information
> there just makes it worse ;-)
> 
> I would rather convert debugging information to use tracepoints and get
> rid of the tb_*_info() things completely.

The noisiness has value in that it helps with reverse-engineering:
Just google for dmesg output and check what other machines are
reporting for unknown registers. :-)

If there was public documentation available or Intel would be okay
with answering specific questions (as you've done with the 0x30
attribute id), then the value obviously diminishes.

Can't say anything about converting to tracepoints, that's Andreas'
call.


> The whole DROM content is already available through nvm_active/nvmem
> file under each device (well starting with Alpine Ridge) so the
> userspace can investigate it as much as it likes without spamming the
> kernel dmesg :)

Okay, fair enough, that should indeed suffice.

Thanks,

Lukas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web