Messages in this thread | | | Date | Fri, 19 May 2017 13:28:36 +0300 | From | Mika Westerberg <> | Subject | Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM |
| |
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 :)
| |