linux-kernel - Re: [PATCH 05/24] thunderbolt: Rework capability handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170519081234.GC3267@lahna.fi.intel.com>
Date:   Fri, 19 May 2017 11:12:34 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <yehezkel.bernat@...el.com>,
        Lukas Wunner <lukas@...ner.de>,
        Amir Levy <amir.jer.levy@...el.com>,
        Andy Lutomirski <luto@...nel.org>,
        Mario Limonciello <Mario.Limonciello@...l.com>,
        Jared.Dominguez@...l.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 05/24] thunderbolt: Rework capability handling

On Thu, May 18, 2017 at 07:38:29PM +0300, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:38 PM, Mika Westerberg
> <mika.westerberg@...ux.intel.com> wrote:
> > Organization of the capabilities in switches and ports is not so random
> > after all. Rework the capability handling functionality so that it
> > follows how capabilities are organized and provide two new functions
> > (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to
> > extract capabilities for ports and switches. Then convert the current
> > users over these.
> 
> One nit here.
> 
> > +int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
> >  {
> 
> > +       u32 offset;
> >
> 
> > +       /*
> > +        * DP out adapters claim to implement TMU capability but in
> > +        * reality they do not so we hard code the adapter specific
> > +        * capability offset here.
> > +        */
> > +       if (port->config.type == TB_TYPE_DP_HDMI_OUT)
> > +               offset = 0x39;
> >         else
> 
> > +               offset = 0x1;
> > +
> > +       do {
> > +               struct tb_cap_any header;
> > +               int ret;
> > +
> > +               ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (header.basic.cap == cap)
> > +                       return offset;
> > +
> > +               offset = header.basic.next;
> > +       } while (offset);
> > +
> > +       return -ENOENT;
> >  }
> 
> > +static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
> >  {
> 
> > +       int offset = sw->config.first_cap_offset;
> > +
> > +       while (offset > 0 && offset < CAP_OFFSET_MAX) {
> > +               struct tb_cap_any header;
> > +               int ret;
> > +
> > +               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (header.basic.cap == cap)
> > +                       return offset;
> > +
> > +               offset = header.basic.next;
> >         }
> 
> > +
> > +       return -ENOENT;
> >  }
> 
> Both has quite similar bodies.
> Wouldn't be nice to split out a helper which takes initial offset and
> type as parameters?

The whole point of this rework was to separate port vs. switch
capability to follow how the hardware is organized instead of having one
more complex function handling everything :)

Sure, I can merge them back together but IMHO it will be not that
readable anymore.