Re: [PATCH net-next 4/6] net: dsa: sja1105: support flow-based redirection via virtual links

On Mon, 4 May 2020 at 22:24, Vivien Didelot [off-list ref] wrote:

Hi Vladimir,

On Mon, 4 May 2020 21:38:26 +0300, Vladimir Oltean [off-list ref] wrote:

quoted

Hi Vivien,

On Mon, 4 May 2020 at 21:23, Vivien Didelot [off-list ref] wrote:

quoted

On Mon, 4 May 2020 14:19:13 -0400, Vivien Didelot [off-list ref] wrote:

quoted

Hi Vladimir,

On Mon,  4 May 2020 00:10:33 +0300, Vladimir Oltean [off-list ref] wrote:

quoted

+           case FLOW_ACTION_REDIRECT: {
+                   struct dsa_port *to_dp;
+
+                   if (!dsa_slave_dev_check(act->dev)) {
+                           NL_SET_ERR_MSG_MOD(extack,
+                                              "Destination not a switch port");
+                           return -EOPNOTSUPP;
+                   }
+
+                   to_dp = dsa_slave_to_port(act->dev);

Instead of exporting two DSA core internal functions, I would rather expose
a new helper for drivers, such as this one:

    struct dsa_port *dsa_dev_to_port(struct net_device *dev)
    {
        if (!dsa_slave_dev_check(dev))
            return -EOPNOTSUPP;

Oops, NULL, not an integer error code, but you get the idea of public helpers.

quoted

        return dsa_slave_to_port(dev);
    }

The naming might not be the best, this helper could even be mirroring-specific,
I didn't really check the requirements for this functionality yet.


Thank you,

      Vivien

How about

int dsa_slave_get_port_index(struct net_device *dev)
{
    if (!dsa_slave_dev_check(dev))
        return -EINVAL;

    return dsa_slave_to_port(dev)->index;
}
EXPORT_SYMBOL_GPL(dsa_slave_get_port_index);

also, where to put it? slave.c I suppose?

dsa.c is the place for private implementation of public functions. "slave"
is a core term, no need to expose it. Public helpers exposed in dsa.h usually
scope the dsa_switch structure and an optional port index. mv88e6xxx allows
mirroring an external device port,

For mirroring an entire port (via tc-matchall), the tc structures are
already parsed by DSA core and a simple API is given to drivers. The
discussion we're having is for flow-based mirroring (via tc-flower)
where that is not the case.

so dsa_port would be preferred, but this
can wait. So I'm thinking about implementing the following:

net/dsa/dsa.c:

int dsa_to_port_index(struct dsa_switch *ds, struct net_device *dev)

But let's assume for a second that mv88e6xxx supports flow-based
mirroring/redirection too.
Aren't we limiting ourselves uselessly here, by requiring the caller
to pass a ds pointer just to perform validation on it? I think it's a
valid use case to want to support cross-chip mirroring/redirection
sometime in the future. Both sja1105 and mv88e6xxx support that kind
of setup, you just need to set the destination port to
dsa_towards_port() in case the dp->ds found by dsa_slave_to_port does
not coincide with ours. But surprise, using the syntactic sugar API
we're introducing here, we'd get -EINVAL and we would have to somehow
try again and guess with a ds pointer we don't have.

{
    struct dsa_port *dp;

    if (!dsa_slave_dev_check(dev))
        return -ENODEV;

    dp = dsa_slave_to_port(dev);

    if (dp->ds != ds)
        return -EINVAL;

    return dp->index;
}

include/net/dsa.h:

int dsa_to_port_index(struct dsa_switch *ds, struct net_device *dev);


What do you think?

I'm actually not convinced about this idea. I think the function that
should be called should be named dsa_slave_to_port, and it should
return a struct dsa_port. Quite conveniently, that function already
exists. I'm not actually sure what are the issues of exposing the
existing functions.

        Vivien

Thanks,
-Vladimir