[PATCH net] net: mvpp2: cls: Add missing NETIF_F_NTUPLE flag

[PATCH net] net: mvpp2: cls: Add missing NETIF_F_NTUPLE flag

[Maxime Chevallier]

Now that the mvpp2 driver supports classification offloading, we must
add the NETIF_F_NTUPLE to the features list.

Fixes: 90b509b39ac9 ("net: mvpp2: cls: Add Classification offload support")
Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
Hello David,

This patch applies on top of a commit 90b509b39ac9, which is in net-next
but hasn't made it to -net yet.

Thanks,

Maxime

 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 25fbed2b8d94..1f164c893936 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5040,8 +5040,10 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 	dev->hw_features |= features | NETIF_F_RXCSUM | NETIF_F_GRO |
 			    NETIF_F_HW_VLAN_CTAG_FILTER;
 
-	if (mvpp22_rss_is_supported())
+	if (mvpp22_rss_is_supported()) {
 		dev->hw_features |= NETIF_F_RXHASH;
+		dev->features |= NETIF_F_NTUPLE;
+	}
 
 	if (port->pool_long->id == MVPP2_BM_JUMBO && port->id != 0) {
 		dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-- 
2.20.1

Re: [PATCH net] net: mvpp2: cls: Add missing NETIF_F_NTUPLE flag

[Jakub Kicinski]

On Tue,  7 May 2019 14:36:35 +0200, Maxime Chevallier wrote:
> Now that the mvpp2 driver supports classification offloading, we must
> add the NETIF_F_NTUPLE to the features list.
> 
> Fixes: 90b509b39ac9 ("net: mvpp2: cls: Add Classification offload support")
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> Hello David,
> 
> This patch applies on top of a commit 90b509b39ac9, which is in net-next
> but hasn't made it to -net yet.
> 
> Thanks,
> 
> Maxime
> 
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 25fbed2b8d94..1f164c893936 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -5040,8 +5040,10 @@ static int mvpp2_port_probe(struct platform_device *pdev,
>  	dev->hw_features |= features | NETIF_F_RXCSUM | NETIF_F_GRO |
>  			    NETIF_F_HW_VLAN_CTAG_FILTER;
>  
> -	if (mvpp22_rss_is_supported())
> +	if (mvpp22_rss_is_supported()) {
>  		dev->hw_features |= NETIF_F_RXHASH;
> +		dev->features |= NETIF_F_NTUPLE;

Hm, why not in hw_features?

> +	}
>  
>  	if (port->pool_long->id == MVPP2_BM_JUMBO && port->id != 0) {
>  		dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);

Re: [PATCH net] net: mvpp2: cls: Add missing NETIF_F_NTUPLE flag

[Maxime Chevallier]

Hello Jakub, David,

On Tue, 7 May 2019 10:28:03 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

>> -	if (mvpp22_rss_is_supported())
>> +	if (mvpp22_rss_is_supported()) {
>>  		dev->hw_features |= NETIF_F_RXHASH;
>> +		dev->features |= NETIF_F_NTUPLE;  
>
>Hm, why not in hw_features?

Because as of today, there's nothing implemented to disable
classification offload in the driver, so the feature can't be toggled.

Is this an issue ? Sorry if I'm doing this wrong, but I didn't see any
indication that this feature has to be host-writeable.

I can make so that it's toggle-able, but it's not as straightforward as
we would think, since the classifier is also used for RSS (so, we can't
just disable the classifier as a whole, we would have to invalidate
each registered flow).

Thanks,

Maxime

Re: [PATCH net] net: mvpp2: cls: Add missing NETIF_F_NTUPLE flag

[Jakub Kicinski]

On Thu, 9 May 2019 07:14:08 +0200, Maxime Chevallier wrote:
> Hello Jakub, David,
> 
> On Tue, 7 May 2019 10:28:03 -0700
> Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> 
> >> -	if (mvpp22_rss_is_supported())
> >> +	if (mvpp22_rss_is_supported()) {
> >>  		dev->hw_features |= NETIF_F_RXHASH;
> >> +		dev->features |= NETIF_F_NTUPLE;    
> >
> >Hm, why not in hw_features?  
> 
> Because as of today, there's nothing implemented to disable
> classification offload in the driver, so the feature can't be toggled.
> 
> Is this an issue ? Sorry if I'm doing this wrong, but I didn't see any
> indication that this feature has to be host-writeable.

No I don't think it's an issue, I was expecting you'd flush all the
filters when feature is disabled (remove them entirely), I didn't
expect that to be too hard.

> I can make so that it's toggle-able, but it's not as straightforward as
> we would think, since the classifier is also used for RSS (so, we can't
> just disable the classifier as a whole, we would have to invalidate
> each registered flow).

Ack, I don't think disabling the hardware components is required.
Just remove the existing filters, and don't allow new ones.  
But no strong feelings here, feel free to repost with:

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

if flushing the filters is too much hassle.

Re: [PATCH net] net: mvpp2: cls: Add missing NETIF_F_NTUPLE flag

[David Miller]

From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: Tue,  7 May 2019 14:36:35 +0200

> Now that the mvpp2 driver supports classification offloading, we must
> add the NETIF_F_NTUPLE to the features list.
> 
> Fixes: 90b509b39ac9 ("net: mvpp2: cls: Add Classification offload support")
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> Hello David,
> 
> This patch applies on top of a commit 90b509b39ac9, which is in net-next
> but hasn't made it to -net yet.
> 
> Thanks,
> 
> Maxime
> 
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 25fbed2b8d94..1f164c893936 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -5040,8 +5040,10 @@ static int mvpp2_port_probe(struct platform_device *pdev,
>  	dev->hw_features |= features | NETIF_F_RXCSUM | NETIF_F_GRO |
>  			    NETIF_F_HW_VLAN_CTAG_FILTER;
>  
> -	if (mvpp22_rss_is_supported())
> +	if (mvpp22_rss_is_supported()) {
>  		dev->hw_features |= NETIF_F_RXHASH;
> +		dev->features |= NETIF_F_NTUPLE;
> +	}

As Jakub said, this definitely looks like a typo and this should
be hw_features.