Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ioana Ciornei <ioana.ciornei@nxp.com>
To:     Vladimir Oltean <olteanv@gmail.com>
CC:     Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bridge@lists.linux-foundation.org" 
        <bridge@lists.linux-foundation.org>,
        Roopa Prabhu <roopa@nvidia.com>,
        Nikolay Aleksandrov <nikolay@nvidia.com>,
        Jiri Pirko <jiri@resnulli.us>,
        Ido Schimmel <idosch@idosch.org>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        Vadym Kochan <vkochan@marvell.com>,
        Taras Chornyi <tchornyi@marvell.com>,
        Grygorii Strashko <grygorii.strashko@ti.com>,
        Ivan Vecera <ivecera@redhat.com>,
        "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>
Subject: Re: [PATCH net-next 5/9] net: squash switchdev attributes
 PRE_BRIDGE_FLAGS and BRIDGE_FLAGS
Thread-Topic: [PATCH net-next 5/9] net: squash switchdev attributes
 PRE_BRIDGE_FLAGS and BRIDGE_FLAGS
Thread-Index: AQHW/ag09t/cx1zZKkyxRCLhvjkPzapObMsA
Date:   Mon, 8 Feb 2021 16:04:20 +0000
Message-ID: <20210208160418.pen47yf3xhtzuwkb@skbuf>
References: <20210207232141.2142678-1-olteanv@gmail.com>
 <20210207232141.2142678-6-olteanv@gmail.com>
In-Reply-To: <20210207232141.2142678-6-olteanv@gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EBBD033502BAE849A5733453869BC577@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ae0942a4-3a6d-4c9c-0c85-08d8cc4b31ee
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Feb 2021 16:04:20.4093
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: PjuF+LhwS14xExlxmO6YIlouOWRnQfdrqXoihsN69VHtFvKvN0QE51zxpnph8j/7ce+QEppmKltZsuTpEROtAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3326
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Feb 08, 2021 at 01:21:37AM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>=20
> There does not appear to be any strong reason why
> br_switchdev_set_port_flag issues a separate notification for checking
> the supported brport flags rather than just attempting to apply them and
> propagating the error if that fails.
>=20
> However, there is a reason why this switchdev API is counterproductive
> for a driver writer, and that is because although br_switchdev_set_port_f=
lag
> gets passed a "flags" and a "mask", those are passed piecemeal to the
> driver, so while the PRE_BRIDGE_FLAGS listener knows what changed
> because it has the "mask", the BRIDGE_FLAGS listener doesn't, because it
> only has the final value. This means that "edge detection" needs to be
> done by each individual BRIDGE_FLAGS listener by XOR-ing the old and the
> new flags, which in turn means that copying the flags into a driver
> private variable is strictly necessary.
>=20
> This can be solved by passing the "flags" and the "value" together into
> a single switchdev attribute, and it also reduces some boilerplate in
> the drivers that offload this.
>=20
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/b53/b53_common.c              | 16 ++++-------
>  drivers/net/dsa/mv88e6xxx/chip.c              | 17 ++++-------
>  .../marvell/prestera/prestera_switchdev.c     | 16 +++++------
>  .../mellanox/mlxsw/spectrum_switchdev.c       | 28 ++++++-------------
>  drivers/net/ethernet/rocker/rocker_main.c     | 24 +++-------------
>  drivers/net/ethernet/ti/cpsw_switchdev.c      | 20 ++++---------
>  drivers/staging/fsl-dpaa2/ethsw/ethsw.c       | 22 ++++-----------
>  include/net/dsa.h                             |  4 +--
>  include/net/switchdev.h                       |  8 ++++--
>  net/bridge/br_switchdev.c                     | 19 ++++---------
>  net/dsa/dsa_priv.h                            |  4 +--
>  net/dsa/port.c                                | 15 ++--------
>  net/dsa/slave.c                               |  3 --
>  13 files changed, 58 insertions(+), 138 deletions(-)
>=20

(..)

> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> @@ -908,28 +908,22 @@ static int dpaa2_switch_port_attr_stp_state_set(str=
uct net_device *netdev,
>  	return dpaa2_switch_port_set_stp_state(port_priv, state);
>  }
> =20
> -static int dpaa2_switch_port_attr_br_flags_pre_set(struct net_device *ne=
tdev,
> -						   unsigned long flags)
> -{
> -	if (flags & ~(BR_LEARNING | BR_FLOOD))
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static int dpaa2_switch_port_attr_br_flags_set(struct net_device *netdev=
,
> -					       unsigned long flags)
> +					       struct switchdev_brport_flags val)
>  {
>  	struct ethsw_port_priv *port_priv =3D netdev_priv(netdev);
>  	int err =3D 0;
> =20
> +	if (val.mask & ~(BR_LEARNING | BR_FLOOD))
> +		return -EINVAL;
> +
>  	/* Learning is enabled per switch */
>  	err =3D dpaa2_switch_set_learning(port_priv->ethsw_data,
> -					!!(flags & BR_LEARNING));
> +					!!(val.flags & BR_LEARNING));
>  	if (err)
>  		goto exit;
> =20
> -	err =3D dpaa2_switch_port_set_flood(port_priv, !!(flags & BR_FLOOD));
> +	err =3D dpaa2_switch_port_set_flood(port_priv, !!(val.flags & BR_FLOOD)=
);


Could you also check the mask to see if the flag needs to be actually chang=
ed?

> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -621,10 +621,8 @@ struct dsa_switch_ops {
>  	void	(*port_stp_state_set)(struct dsa_switch *ds, int port,
>  				      u8 state);
>  	void	(*port_fast_age)(struct dsa_switch *ds, int port);
> -	int	(*port_pre_bridge_flags)(struct dsa_switch *ds, int port,
> -					 unsigned long mask);
>  	int	(*port_bridge_flags)(struct dsa_switch *ds, int port,
> -				     unsigned long flags);
> +				     struct switchdev_brport_flags val);
>  	int	(*port_set_mrouter)(struct dsa_switch *ds, int port,
>  				    bool mrouter);
> =20

In the previous patch you add the .port_pre_bridge_flags()
dsa_switch_ops only to remove it here. Couldn't these two patches be in
reverse order so that there is no need to actually add the callback in
the first place?

Ioana=