linux-kernel - Re: [PATCH v2 13/28] thunderbolt: Add helper function to iterate from one port to another

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: <20190212190317.GK7875@lahna.fi.intel.com>
Date:   Tue, 12 Feb 2019 21:03:17 +0200
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     linux-kernel@...r.kernel.org,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        Andreas Noever <andreas.noever@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v2 13/28] thunderbolt: Add helper function to iterate
 from one port to another

On Tue, Feb 12, 2019 at 02:55:42PM +0100, Lukas Wunner wrote:
> On Mon, Feb 11, 2019 at 11:54:36AM +0200, Mika Westerberg wrote:
> > On Mon, Feb 11, 2019 at 07:16:00AM +0100, Lukas Wunner wrote:
> > > On Wed, Feb 06, 2019 at 04:17:23PM +0300, Mika Westerberg wrote:
> > > > +/**
> > > > + * tb_port_get_next() - Return next port for given port
> > > > + * @start: Start port of the walk
> > > > + * @end: End port of the walk
> > > > + * @prev: Previous port (%NULL if this is the first)
> > > > + *
> > > > + * This function can be used to walk from one port to another if they
> > > > + * are connected through zero or more switches. If the @prev is dual
> > > > + * link port, the function follows that link and returns another end on
> > > > + * that same link.
> > > > + *
> > > > + * If the walk cannot be continued, returns %NULL.
> > > 
> > > This sounds as if NULL is returned if an error occurs but that doesn't
> > > seem to be what the function does.  I'd suggest:
> > > 
> > > "If the @end port has been reached, return %NULL."
> > 
> > It returns NULL if @end cannot be reached. So what about:
> > 
> >   "If @end cannot be reached, returns %NULL"
> > 
> > ?
> 
> That doesn't appear to match what the function does.  There are two places
> where NULL is returned:
> 
> The first is at the top of the function and returns NULL if
> ((prev->sw == end->sw) && (prev == end)).  So this happens when the
> entire path has been traversed and "end" is passed in as prev argument.
> 
> The second is at the bottom and is presumably never executed because
> it only happens if (start->sw->config.depth == end->sw->config.depth),
> which I believe is only the case if (start->sw == end->sw), which implies
> that prev can only be either "start" or "end", and both cases are already
> handled at the top of the function.
> 
> Bottom line is that NULL is returned once the traversal has concluded.
> Am I missing something?

No, you are right. I'll update the comment accordingly.

> > > Why is it necessary to use the primary link anyway?  Is the
> > > ->remote member not set on the secondary link port?  The reason
> > > should probably be spelled out in the code comment.
> > 
> > IIRC it was because you may have something in the middle with only one
> > port (the primary). I'll add a comment here explaining that.
> 
> Hm, I'm wondering if it wouldn't be more straightforward to also set
> the remote member on secondary links to avoid all this special casing?
> Any downside to that?

I think it is useful to distinguish between primary and secondary links
as we do when we establish DP tunnels. Granted we could rename them to
"primary" and "secondary" instead of "remote" and "dual_link_port".  Or
alternatively have two remotes and then link_nr or something like that.
I'll try and see if it simplifies the code.