diff mbox

[dpdk-dev,v4,18/32] net/qede: add missing 100G link speed capability

Message ID 1476850306-2141-19-git-send-email-rasesh.mody@qlogic.com (mailing list archive)
State Accepted, archived
Delegated to: Bruce Richardson
Headers

Commit Message

Rasesh Mody Oct. 19, 2016, 4:11 a.m. UTC 
  From: Harish Patil <harish.patil@qlogic.com>

This patch fixes the missing 100G link speed advertisement
when the 100G support was initially added.

Fixes: 2af14ca79c0a ("net/qede: support 100G")

Signed-off-by: Harish Patil <harish.patil@qlogic.com>
---
 doc/guides/nics/features/qede.ini    | 1 +
 doc/guides/nics/features/qede_vf.ini | 1 +
 drivers/net/qede/qede_ethdev.c       | 3 ++-
 3 files changed, 4 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon Oct. 26, 2016, 3:41 p.m. UTC | #1 
2016-10-18 21:11, Rasesh Mody:
> From: Harish Patil <harish.patil@qlogic.com>
> 
> This patch fixes the missing 100G link speed advertisement
> when the 100G support was initially added.
> 
> Fixes: 2af14ca79c0a ("net/qede: support 100G")
> 
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
[...]
>  [Features]
> +Speed capabilities   = Y

This feature should be checked only when it is fully implemented,
i.e. when you return the real capabilities of the device.

> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -599,7 +599,8 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
>  				     DEV_TX_OFFLOAD_UDP_CKSUM |
>  				     DEV_TX_OFFLOAD_TCP_CKSUM);
>  
> -	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G;
> +	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |
> +			       ETH_LINK_SPEED_100G;
>  }

It is only faking the capabilities at driver-level.
You should check if the underlying device is able to achieve 100G
before advertising this flag to the application.

I suggest to update this patch to remove the doc update.
The contract is to fill it only when the code is fixed.
By the way, we must call every other drivers to properly implement
this feature.
Bruce Richardson Oct. 26, 2016, 3:54 p.m. UTC | #2 
On Wed, Oct 26, 2016 at 05:41:58PM +0200, Thomas Monjalon wrote:
> 2016-10-18 21:11, Rasesh Mody:
> > From: Harish Patil <harish.patil@qlogic.com>
> > 
> > This patch fixes the missing 100G link speed advertisement
> > when the 100G support was initially added.
> > 
> > Fixes: 2af14ca79c0a ("net/qede: support 100G")
> > 
> > Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> [...]
> >  [Features]
> > +Speed capabilities   = Y
> 
> This feature should be checked only when it is fully implemented,
> i.e. when you return the real capabilities of the device.
> 
> > --- a/drivers/net/qede/qede_ethdev.c
> > +++ b/drivers/net/qede/qede_ethdev.c
> > @@ -599,7 +599,8 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
> >  				     DEV_TX_OFFLOAD_UDP_CKSUM |
> >  				     DEV_TX_OFFLOAD_TCP_CKSUM);
> >  
> > -	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G;
> > +	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |
> > +			       ETH_LINK_SPEED_100G;
> >  }
> 
> It is only faking the capabilities at driver-level.
> You should check if the underlying device is able to achieve 100G
> before advertising this flag to the application.
> 
> I suggest to update this patch to remove the doc update.
> The contract is to fill it only when the code is fixed.
> By the way, we must call every other drivers to properly implement
> this feature.

I agree that devices should only advertise speeds they support and the
doc should reflect this ability (or lack of this ability) in a driver.

Regards,
/Bruce
Harish Patil Oct. 26, 2016, 9:28 p.m. UTC | #3 
>2016-10-18 21:11, Rasesh Mody:

>> From: Harish Patil <harish.patil@qlogic.com>

>> 

>> This patch fixes the missing 100G link speed advertisement

>> when the 100G support was initially added.

>> 

>> Fixes: 2af14ca79c0a ("net/qede: support 100G")

>> 

>> Signed-off-by: Harish Patil <harish.patil@qlogic.com>

>[...]

>>  [Features]

>> +Speed capabilities   = Y

>

>This feature should be checked only when it is fully implemented,

>i.e. when you return the real capabilities of the device.

>

>> --- a/drivers/net/qede/qede_ethdev.c

>> +++ b/drivers/net/qede/qede_ethdev.c

>> @@ -599,7 +599,8 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,

>>  				     DEV_TX_OFFLOAD_UDP_CKSUM |

>>  				     DEV_TX_OFFLOAD_TCP_CKSUM);

>>  

>> -	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G;

>> +	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |

>> +			       ETH_LINK_SPEED_100G;

>>  }

>

>It is only faking the capabilities at driver-level.

>You should check if the underlying device is able to achieve 100G

>before advertising this flag to the application.

>

>I suggest to update this patch to remove the doc update.

>The contract is to fill it only when the code is fixed.

>By the way, we must call every other drivers to properly implement

>this feature.

>


Hi Thomas,
Its not really a faking. The same driver supports all three link speeds.
The required support for 100G was already present in the 16.07 inbox
driver.
We just had missed out advertising 100G link speed via
dev_info->speed_capa.
Hence it is - Fixes: 2af14ca79c0a ("net/qede: support 100G”).
Hope it is okay.


Thanks,
Harish
Thomas Monjalon Oct. 26, 2016, 9:43 p.m. UTC | #4 
2016-10-26 21:28, Harish Patil:
> 
> >2016-10-18 21:11, Rasesh Mody:
> >> From: Harish Patil <harish.patil@qlogic.com>
> >> 
> >> This patch fixes the missing 100G link speed advertisement
> >> when the 100G support was initially added.
> >> 
> >> Fixes: 2af14ca79c0a ("net/qede: support 100G")
> >> 
> >> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> >[...]
> >>  [Features]
> >> +Speed capabilities   = Y
> >
> >This feature should be checked only when it is fully implemented,
> >i.e. when you return the real capabilities of the device.
> >
> >> --- a/drivers/net/qede/qede_ethdev.c
> >> +++ b/drivers/net/qede/qede_ethdev.c
> >> @@ -599,7 +599,8 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
> >>  				     DEV_TX_OFFLOAD_UDP_CKSUM |
> >>  				     DEV_TX_OFFLOAD_TCP_CKSUM);
> >>  
> >> -	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G;
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |
> >> +			       ETH_LINK_SPEED_100G;
> >>  }
> >
> >It is only faking the capabilities at driver-level.
> >You should check if the underlying device is able to achieve 100G
> >before advertising this flag to the application.
> >
> >I suggest to update this patch to remove the doc update.
> >The contract is to fill it only when the code is fixed.
> >By the way, we must call every other drivers to properly implement
> >this feature.
> >
> 
> Hi Thomas,
> Its not really a faking. The same driver supports all three link speeds.
> The required support for 100G was already present in the 16.07 inbox
> driver.
> We just had missed out advertising 100G link speed via
> dev_info->speed_capa.
> Hence it is - Fixes: 2af14ca79c0a ("net/qede: support 100G”).
> Hope it is okay.

Yes the code is okay. But it is not complete.
Please tell me you understand that you must fill speed_capa depending
of the device capabilities.
Then you will be able to fill "Speed capabilities" box in the doc.
diff mbox

Patch

diff --git a/doc/guides/nics/features/qede.ini b/doc/guides/nics/features/qede.ini
index 7690773..1d28a23 100644
--- a/doc/guides/nics/features/qede.ini
+++ b/doc/guides/nics/features/qede.ini
@@ -4,6 +4,7 @@ 
 ; Refer to default.ini for the full list of available PMD features.
 ;
 [Features]
+Speed capabilities   = Y
 Link status          = Y
 Link status event    = Y
 MTU update           = Y
diff --git a/doc/guides/nics/features/qede_vf.ini b/doc/guides/nics/features/qede_vf.ini
index aeb20d2..b4eba0c 100644
--- a/doc/guides/nics/features/qede_vf.ini
+++ b/doc/guides/nics/features/qede_vf.ini
@@ -4,6 +4,7 @@ 
 ; Refer to default.ini for the full list of available PMD features.
 ;
 [Features]
+Speed capabilities   = Y
 Link status          = Y
 Link status event    = Y
 MTU update           = Y
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index ec5e8a2..5da540a 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -599,7 +599,8 @@  qede_dev_info_get(struct rte_eth_dev *eth_dev,
 				     DEV_TX_OFFLOAD_UDP_CKSUM |
 				     DEV_TX_OFFLOAD_TCP_CKSUM);
 
-	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G;
+	dev_info->speed_capa = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_40G |
+			       ETH_LINK_SPEED_100G;
 }
 
 /* return 0 means link status changed, -1 means not changed */