Hi Vladimir,
On Mon, 4 May 2020 21:38:26 +0300, Vladimir Oltean <[email protected]> wrote:
> Hi Vivien,
>
> On Mon, 4 May 2020 at 21:23, Vivien Didelot <[email protected]> wrote:
> >
> > On Mon, 4 May 2020 14:19:13 -0400, Vivien Didelot
> > <[email protected]> wrote:
> > > Hi Vladimir,
> > >
> > > On Mon, 4 May 2020 00:10:33 +0300, Vladimir Oltean <[email protected]>
> > > wrote:
> > > > + 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.
> >
> > >
> > > 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, 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)
{
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?
Vivien
