diff mbox series

[v2] ethdev: fix data type for port id

Message ID 1603806082-21484-1-git-send-email-wangyunjian@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] ethdev: fix data type for port id |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Yunjian Wang Oct. 27, 2020, 1:41 p.m. UTC 
  From: Yunjian Wang <wangyunjian@huawei.com>

The ethdev port id should be 16 bits now. This patch fixes the data
type of the variable for 'pid', changing from uint32_t to uint16_t.

We also need use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS
is less or equal to UINT16_MAX.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
v2:
   add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
---
 lib/librte_ethdev/rte_ethdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon Oct. 27, 2020, 2:29 p.m. UTC | #1 
27/10/2020 14:41, wangyunjian:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> The ethdev port id should be 16 bits now. This patch fixes the data
> type of the variable for 'pid', changing from uint32_t to uint16_t.
> 
> We also need use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS
> is less or equal to UINT16_MAX.

Actually the need is to check that we have room for an increment
after RTE_MAX_ETHPORTS, meaning RTE_MAX_ETHPORTS < UINT16_MAX.

> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
> v2:
>    add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
> ---
>  rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>  {
> -	uint32_t pid;
> +	uint16_t pid;
>  
>  	if (name == NULL) {
>  		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> @@ -4292,6 +4292,8 @@ RTE_INIT(eth_dev_init_cb_lists)
>  {
>  	int i;
>  
> +	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS > UINT16_MAX);

As explained above, should check >=

This check is global for ethdev.
Please could you move it somewhere else?
Yunjian Wang Oct. 29, 2020, 12:18 p.m. UTC | #2 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, October 27, 2020 10:29 PM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru;
> Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix data type for port id
> 
> 27/10/2020 14:41, wangyunjian:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > The ethdev port id should be 16 bits now. This patch fixes the data
> > type of the variable for 'pid', changing from uint32_t to uint16_t.
> >
> > We also need use RTE_BUILD_BUG_ON() to ensure that
> RTE_MAX_ETHPORTS is
> > less or equal to UINT16_MAX.
> 
> Actually the need is to check that we have room for an increment after
> RTE_MAX_ETHPORTS, meaning RTE_MAX_ETHPORTS < UINT16_MAX.

OK.

> 
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> > v2:
> >    add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
> > ---
> >  rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)  {
> > -	uint32_t pid;
> > +	uint16_t pid;
> >
> >  	if (name == NULL) {
> >  		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n"); @@ -4292,6
> > +4292,8 @@ RTE_INIT(eth_dev_init_cb_lists)  {
> >  	int i;
> >
> > +	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS > UINT16_MAX);
> 
> As explained above, should check >=
> 
> This check is global for ethdev.
> Please could you move it somewhere else?
> 
> 

What about following:

# git diff
diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
index ccea60afe7..ff878f4d4d 100644
--- a/lib/librte_eal/freebsd/eal.c
+++ b/lib/librte_eal/freebsd/eal.c
@@ -672,6 +672,8 @@ rte_eal_init(int argc, char **argv)
        struct internal_config *internal_conf =
                eal_get_internal_configuration();
 
+       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
        /* checks if the machine is adequate */
        if (!rte_cpu_is_supported()) {
                rte_eal_init_alert("unsupported cpu type.");
diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
index 9b579b8200..bd7fb7b5f1 100644
--- a/lib/librte_eal/linux/eal.c
+++ b/lib/librte_eal/linux/eal.c
@@ -970,6 +970,8 @@ rte_eal_init(int argc, char **argv)
        struct internal_config *internal_conf =
                eal_get_internal_configuration();
 
+       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
        /* checks if the machine is adequate */
        if (!rte_cpu_is_supported()) {
                rte_eal_init_alert("unsupported cpu type.");
diff --git a/lib/librte_eal/windows/eal.c b/lib/librte_eal/windows/eal.c
index 141f22adb7..27c2ebd114 100644
--- a/lib/librte_eal/windows/eal.c
+++ b/lib/librte_eal/windows/eal.c
@@ -265,6 +265,8 @@ rte_eal_init(int argc, char **argv)
        struct internal_config *internal_conf =
                eal_get_internal_configuration();
 
+       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
        rte_eal_log_init(NULL, 0);
Andrew Rybchenko Oct. 29, 2020, 12:40 p.m. UTC | #3 
On 10/29/20 3:18 PM, wangyunjian wrote:
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Tuesday, October 27, 2020 10:29 PM
>> To: wangyunjian <wangyunjian@huawei.com>
>> Cc: dev@dpdk.org; ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru;
>> Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
>> wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix data type for port id
>>
>> 27/10/2020 14:41, wangyunjian:
>>> From: Yunjian Wang <wangyunjian@huawei.com>
>>>
>>> The ethdev port id should be 16 bits now. This patch fixes the data
>>> type of the variable for 'pid', changing from uint32_t to uint16_t.
>>>
>>> We also need use RTE_BUILD_BUG_ON() to ensure that
>> RTE_MAX_ETHPORTS is
>>> less or equal to UINT16_MAX.
>>
>> Actually the need is to check that we have room for an increment after
>> RTE_MAX_ETHPORTS, meaning RTE_MAX_ETHPORTS < UINT16_MAX.
> 
> OK.
> 
>>
>>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>>> ---
>>> v2:
>>>    add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
>>> ---
>>>  rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)  {
>>> -	uint32_t pid;
>>> +	uint16_t pid;
>>>
>>>  	if (name == NULL) {
>>>  		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n"); @@ -4292,6
>>> +4292,8 @@ RTE_INIT(eth_dev_init_cb_lists)  {
>>>  	int i;
>>>
>>> +	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS > UINT16_MAX);
>>
>> As explained above, should check >=
>>
>> This check is global for ethdev.
>> Please could you move it somewhere else?
>>
>>
> 
> What about following:
> 
> # git diff
> diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
> index ccea60afe7..ff878f4d4d 100644
> --- a/lib/librte_eal/freebsd/eal.c
> +++ b/lib/librte_eal/freebsd/eal.c
> @@ -672,6 +672,8 @@ rte_eal_init(int argc, char **argv)
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
>  
> +       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
> +
>         /* checks if the machine is adequate */
>         if (!rte_cpu_is_supported()) {
>                 rte_eal_init_alert("unsupported cpu type.");
> diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
> index 9b579b8200..bd7fb7b5f1 100644
> --- a/lib/librte_eal/linux/eal.c
> +++ b/lib/librte_eal/linux/eal.c
> @@ -970,6 +970,8 @@ rte_eal_init(int argc, char **argv)
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
>  
> +       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
> +
>         /* checks if the machine is adequate */
>         if (!rte_cpu_is_supported()) {
>                 rte_eal_init_alert("unsupported cpu type.");
> diff --git a/lib/librte_eal/windows/eal.c b/lib/librte_eal/windows/eal.c
> index 141f22adb7..27c2ebd114 100644
> --- a/lib/librte_eal/windows/eal.c
> +++ b/lib/librte_eal/windows/eal.c
> @@ -265,6 +265,8 @@ rte_eal_init(int argc, char **argv)
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
>  
> +       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
> +
>         rte_eal_log_init(NULL, 0);
> 

If RTE_MAX_ETHPORTS is a maximum number of ports starting from
0 and loops are port_id < RTE_MAX_ETHPORTS, RTE_MAX_ETHPORTS
may be UINT16_MAX and it avoid 16-bit unsigned integer
overflow, but may be it is safer to cut one more port from
space do checks as above.
Thomas Monjalon Oct. 29, 2020, 12:43 p.m. UTC | #4 
29/10/2020 13:18, wangyunjian:
> What about following:
> 
> --- a/lib/librte_eal/freebsd/eal.c
> +++ b/lib/librte_eal/freebsd/eal.c
> @@ -672,6 +672,8 @@ rte_eal_init(int argc, char **argv)
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
>  
> +       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
> +

This is an ethdev value, so I would prefer having this test
inside librte_ethdev.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b12bb3854d..ecaca5c600 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -816,7 +816,7 @@  rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 int
 rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 {
-	uint32_t pid;
+	uint16_t pid;
 
 	if (name == NULL) {
 		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
@@ -4292,6 +4292,8 @@  RTE_INIT(eth_dev_init_cb_lists)
 {
 	int i;
 
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS > UINT16_MAX);
+
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
 		TAILQ_INIT(&rte_eth_devices[i].link_intr_cbs);
 }