Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Tue, 12 Sep 2023 17:48:02 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Pawel Dembicki <paweldembicki@gmail.com>
Cc:     netdev@vger.kernel.org, Dan Carpenter <dan.carpenter@linaro.org>,
        Simon Horman <simon.horman@corigine.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Russell King <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add
 port_stp_state_set function
Message-ID: <20230912144802.czdpb6hpn2yiewvf@skbuf>
References: <20230912122201.3752918-1-paweldembicki@gmail.com>
 <20230912122201.3752918-5-paweldembicki@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230912122201.3752918-5-paweldembicki@gmail.com>
Precedence: bulk

Hi Pawel,

On Tue, Sep 12, 2023 at 02:21:58PM +0200, Pawel Dembicki wrote:
> This isn't a fully functional implementation of 802.1D, but
> port_stp_state_set is required for a future tag8021q operations.
> 
> This implementation handles properly all states, but vsc73xx doesn't
> forward STP packets.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 8f2285a03e82..541fbc195df1 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1033,9 +1031,59 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
>  	return 9600 - ETH_HLEN - ETH_FCS_LEN;
>  }
>  
> +static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> +{

For bisectability, the series must build patch by patch.
Here, you are missing:

	struct vsc73xx *vsc = ds->priv;

../drivers/net/dsa/vitesse-vsc73xx-core.c:1038:3: error: use of undeclared identifier 'vsc'
                vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
                ^
../drivers/net/dsa/vitesse-vsc73xx-core.c:1041:3: error: use of undeclared identifier 'vsc'
                vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
                ^
2 errors generated.

> +	/* Configure forward map to CPU <-> port only */
> +	if (port == CPU_PORT)
> +		vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> +					     ~BIT(CPU_PORT);

		vsc->forward_map[CPU_PORT] = dsa_user_ports(ds);

> +	else
> +		vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> +					 BIT(CPU_PORT);

		vsc->forward_map[port] = BIT(CPU_PORT);

> +
> +	return 0;
> +}
> +
> +/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
> + * forwarded only from and to PI/SI interface. For more info see chapter
> + * 2.7.1 (CPU Forwarding) in datasheet.
> + * This function is required for tag8021q operations.
> + */
> +
> +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	if (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), 0);
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), BIT(port));
> +
> +	if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_LEARNMASK, BIT(port), BIT(port));
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_LEARNMASK, BIT(port), 0);
> +
> +	if (state == BR_STATE_FORWARDING)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK,
> +				    vsc->forward_map[port]);

To forward a packet between port A and port B, both of them must be in
BR_STATE_FORWARDING, not just A.

> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +}
> +
>  static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_tag_protocol = vsc73xx_get_tag_protocol,
>  	.setup = vsc73xx_setup,
> +	.port_setup = vsc73xx_port_setup,
>  	.phy_read = vsc73xx_phy_read,
>  	.phy_write = vsc73xx_phy_write,
>  	.phylink_get_caps = vsc73xx_phylink_get_caps,
> @@ -1049,6 +1097,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.port_disable = vsc73xx_port_disable,
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
> +	.port_stp_state_set = vsc73xx_port_stp_state_set,
>  };
>  
>  static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index f79d81ef24fb..224e284a5573 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -18,6 +18,7 @@
>  
>  /**
>   * struct vsc73xx - VSC73xx state container
> + * @forward_map: Forward table cache

If you start describing the member fields, shouldn't all be described?
I think there will be kdoc warnings otherwise.

>   */
>  struct vsc73xx {
>  	struct device			*dev;
> @@ -28,6 +29,7 @@ struct vsc73xx {
>  	u8				addr[ETH_ALEN];
>  	const struct vsc73xx_ops	*ops;
>  	void				*priv;
> +	u8				forward_map[VSC73XX_MAX_NUM_PORTS];
>  };
>  
>  struct vsc73xx_ops {
> -- 
> 2.34.1
>