Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 11 Feb 2019 11:54:36 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Lukas Wunner <lukas@wunner.de>
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
Message-ID: <20190211095436.GW7875@lahna.fi.intel.com>
References: <20190206131738.43696-1-mika.westerberg@linux.intel.com>
 <20190206131738.43696-14-mika.westerberg@linux.intel.com>
 <20190211061600.6ty733qfagk7f6fp@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190211061600.6ty733qfagk7f6fp@wunner.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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:
> > We need to be able to walk from one port to another when we are creating
> > paths where there are multiple switches between two ports. For this
> > reason introduce a new function tb_port_get_next() and a new macro
> > tb_for_each_port().
> 
> These names seem fairly generic, they might as well refer to the next port
> on a switch or iterate over the ports on a switch.  E.g. I've proposed a
> tb_sw_for_each_port() macro in this patch:
> 
> https://lore.kernel.org/patchwork/patch/983863/
> 
> I'd suggest renaming tb_port_get_next() to something like
> tb_next_port_on_path() or tb_path_next_port() or tb_path_walk().

OK, tb_next_port_on_path() sounds good to me.

> And I'd suggest dropping tb_for_each_port() because there are only
> two occurrences where it's used, one calculates the path length,
> and I think that's simply the route string length plus 2, and the
> other one in patch 17 isn't even interested in the ports along a path,
> but rather in the switches between the root switch and the end of a path.
> It seems simpler to just iterate from the switch at the end upwards to
> the root switch by following the parent pointer in the switch's
> struct device, or alternatively by bytewise iterating over the route
> string and calling get_switch_at_route() each time.

OK.

> > +/**
> > + * 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"

?

> > +struct tb_port *tb_port_get_next(struct tb_port *start, struct tb_port *end,
> > +				 struct tb_port *prev)
> > +{
> > +	struct tb_port *port, *next;
> > +
> > +	if (!prev)
> > +		return start;
> > +
> > +	if (prev->sw == end->sw) {
> > +		if (prev != end)
> > +			return end;
> > +		return NULL;
> > +	}
> > +
> > +	/* Switch back to use primary links for walking */
> 
> "Switch back" requires that you switched to something else before,
> which you didn't.  I'd suggest something like:
> 
> "use primary link to discover next port"

OK.

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

> > +	if (prev->dual_link_port && prev->link_nr)
> > +		port = prev->dual_link_port;
> > +	else
> > +		port = prev;
> > +
> > +	if (start->sw->config.depth < end->sw->config.depth) {
> > +		if (port->remote &&
> > +		    port->remote->sw->config.depth > port->sw->config.depth)
> 
> Can we use "if (!tb_is_upstream_port(port))" for consistency with the
> if-clause below?

Yes, I think that should work.

> > +			next = port->remote;
> > +		else
> > +			next = tb_port_at(tb_route(end->sw), port->sw);
> > +	} else if (start->sw->config.depth > end->sw->config.depth) {
> > +		if (tb_is_upstream_port(port))
> > +			next = port->remote;
> > +		else
> > +			next = tb_upstream_port(port->sw);
> > +	} else {
> > +		/* Must be the same switch then */
> > +		if (start->sw != end->sw)
> > +			return NULL;
> > +		return end;
> > +	}
> 
> The else-clause here appears to be dead code, you've already checked
> further up whether prev and end are on the same switch.

OK.

> > +
> > +	/* If prev was dual link return another end of that link then */
> 
> *Here* a "switch back" comment would be appropriate.  Nit: Please either
> end code comments with a period or don't start them with an upper case
> letter.

That's the style I've been using in this driver and elsewhere and is my
preference anyway.

I'll update the comment content, though. :)