lkml.org 
[lkml]   [2017]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 :)

\
 
 \ /
  Last update: 2017-05-19 12:29    [W:0.172 / U:7.724 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site