| Message ID | 20190331174232.22060-7-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | NXP SJA1105 DSA driver | expand |
On Sun, Mar 31, 2019 at 08:42:21PM +0300, Vladimir Oltean wrote: > This allows the driver to perform some manipulations of its own during > setup, using generic code. > One current usage scenario is for the driver to request DSA to set up > 802.1Q based switch tagging for its ports. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > net/dsa/dsa2.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index c00ee464afc7..5beceb18b7e2 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -360,14 +360,14 @@ static int dsa_switch_setup(struct dsa_switch *ds) > if (err) > return err; > > - err = ds->ops->setup(ds); > - if (err < 0) > - return err; > - > err = dsa_switch_register_notifier(ds); > if (err) > return err; It seems that notifiers are the important thing here? Maybe state that in the commit message? I'm also wondering how safe this is in general. If we have not yet called the driver setup, the switch is potentially not yet ready to actually handle an requests that come via the notifier. If such notifiers can only come from the driver itself, it should be safe. However, if they could come from the rest of the stack, i could see bad things happening. Andrew
On 4/3/19 12:03 AM, Andrew Lunn wrote: > On Sun, Mar 31, 2019 at 08:42:21PM +0300, Vladimir Oltean wrote: >> This allows the driver to perform some manipulations of its own during >> setup, using generic code. >> One current usage scenario is for the driver to request DSA to set up >> 802.1Q based switch tagging for its ports. >> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> net/dsa/dsa2.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c >> index c00ee464afc7..5beceb18b7e2 100644 >> --- a/net/dsa/dsa2.c >> +++ b/net/dsa/dsa2.c >> @@ -360,14 +360,14 @@ static int dsa_switch_setup(struct dsa_switch *ds) >> if (err) >> return err; >> >> - err = ds->ops->setup(ds); >> - if (err < 0) >> - return err; >> - >> err = dsa_switch_register_notifier(ds); >> if (err) >> return err; > > It seems that notifiers are the important thing here? Maybe state that > in the commit message? > > I'm also wondering how safe this is in general. If we have not yet > called the driver setup, the switch is potentially not yet ready to > actually handle an requests that come via the notifier. If such > notifiers can only come from the driver itself, it should be > safe. However, if they could come from the rest of the stack, i could > see bad things happening. > > Andrew > Hi Andrew, Correct me if I'm wrong, but the switchdev notifiers coming from the stack are coming through a net device, and the slave net devices are only created in dsa_port_setup, which is later than the code I'm modifying anyway? Do you have an example of potentially racy situation caused by this change? Thanks, -Vladimir
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index c00ee464afc7..5beceb18b7e2 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -360,14 +360,14 @@ static int dsa_switch_setup(struct dsa_switch *ds) if (err) return err; - err = ds->ops->setup(ds); - if (err < 0) - return err; - err = dsa_switch_register_notifier(ds); if (err) return err; + err = ds->ops->setup(ds); + if (err < 0) + return err; + if (!ds->slave_mii_bus && ds->ops->phy_read) { ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); if (!ds->slave_mii_bus)