Re: [PATCH net-next v2 06/16] net: bridge: Stop calling switchdev_port_attr_get()

[Ido Schimmel]

On Sun, Feb 10, 2019 at 11:34:14AM -0800, Florian Fainelli wrote:
> Le 2/10/19 à 11:05 AM, Ido Schimmel a écrit :
> > On Sun, Feb 10, 2019 at 09:50:55AM -0800, Florian Fainelli wrote:
> >> Now that all switchdev drivers have been converted to checking the
> >> bridge port flags during the prepare phase of the
> >> switchdev_port_attr_set(), we can move straight to trying to set the
> >> desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS.
> >>
> >> Acked-by: Jiri Pirko <jiri@xxxxxxxxxxxx>
> >> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> >> ---
> >>  net/bridge/br_switchdev.c | 20 +++-----------------
> >>  1 file changed, 3 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> >> index db9e8ab96d48..939f300522c5 100644
> >> --- a/net/bridge/br_switchdev.c
> >> +++ b/net/bridge/br_switchdev.c
> >> @@ -64,29 +64,15 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
> >>  {
> >>  	struct switchdev_attr attr = {
> >>  		.orig_dev = p->dev,
> >> -		.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT,
> >> +		.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
> >> +		.flags = SWITCHDEV_F_DEFER,
> > 
> > How does this work? You defer the operation, so the driver cannot veto
> > it. This is why we have SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT
> > which is not deferred.
> 
> I missed that indeed, how would you feel about splitting the attribute
> setting into different phases:
> 
> - checking that the attribute setting is supported (caller context, so
> possibly atomic)
> - allocating and committing resources (deferred context)

Yes, this is what I suggested in the other thread. Lets continue
discussion there.

We are doing that when processing route notifications (for example), but
we are missing a check in caller context that resources can actually be
allocated. That's part of a bigger task to track resources in mlxsw.

> 
> For pretty much any DSA driver, we will have to be in sleepable context
> anyway because of MDIO, SPI, I2C, whatever transport layer.
> 
> Not sure how to best approach this now...
> -- 
> Florian