lkml.org 
[lkml]   [2019]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 13/28] thunderbolt: Add helper function to iterate from one port to another
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. :)

\
 
 \ /
  Last update: 2019-02-11 10:55    [W:0.060 / U:10.444 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site