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