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

[Mika Westerberg]

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 :)