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

From: Lukas Wunner <lukas@wunner.de>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: linux-kernel@vger.kernel.org,
	Michael Jamet <michael.jamet@intel.com>,
	Yehezkel Bernat <YehezkelShB@gmail.com>,
	Andreas Noever <andreas.noever@gmail.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v2 13/28] thunderbolt: Add helper function to iterate from one port to another
Date: Tue, 12 Feb 2019 14:55:42 +0100	[thread overview]
Message-ID: <20190212135542.xkxjjai3wvnq2mk6@wunner.de> (raw)
In-Reply-To: <20190211095436.GW7875@lahna.fi.intel.com>

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?

> > 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?

Thanks,

Lukas