[dpdk-dev] [PATCH] ethdev: fix data type for port id

[dpdk-dev] [PATCH] ethdev: fix data type for port id

[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.

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

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 lib/librte_ethdev/rte_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b12bb3854d..d52215b9a7 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");
-- 
2.23.0

Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id

[Thomas Monjalon]

26/10/2020 13:24, 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.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")

It was 32-bit on purpose, to avoid overflow in this loop:
	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++)

It is now replaced by RTE_ETH_FOREACH_VALID_DEV,
but I wonder whether we still have this theoritical overflow risk.
If yes, we should change more variables to 32-bit.

Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id

[Andrew Rybchenko]

On 10/26/20 3:24 PM, wangyunjian wrote:
> 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.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id

[Andrew Rybchenko]

On 10/26/20 3:29 PM, Thomas Monjalon wrote:
> 26/10/2020 13:24, 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.
>>
>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> 
> It was 32-bit on purpose, to avoid overflow in this loop:
> 	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++)
> 
> It is now replaced by RTE_ETH_FOREACH_VALID_DEV,
> but I wonder whether we still have this theoritical overflow risk.
> If yes, we should change more variables to 32-bit.

Ah, it is too tricky. May be it is better to ensure that
RTE_MAX_ETHPORTS is less or equal to UINT16_MAX?

Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id

[Thomas Monjalon]

26/10/2020 13:33, Andrew Rybchenko:
> On 10/26/20 3:29 PM, Thomas Monjalon wrote:
> > 26/10/2020 13:24, 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.
> >>
> >> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > 
> > It was 32-bit on purpose, to avoid overflow in this loop:
> > 	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++)
> > 
> > It is now replaced by RTE_ETH_FOREACH_VALID_DEV,
> > but I wonder whether we still have this theoritical overflow risk.
> > If yes, we should change more variables to 32-bit.
> 
> Ah, it is too tricky. May be it is better to ensure that
> RTE_MAX_ETHPORTS is less or equal to UINT16_MAX?

Yes could be another option.

Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id

[wangyunjian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, October 26, 2020 8:34 PM
> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org;
> ferruh.yigit@intel.com; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>; matan@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id
> 
> 26/10/2020 13:33, Andrew Rybchenko:
> > On 10/26/20 3:29 PM, Thomas Monjalon wrote:
> > > 26/10/2020 13:24, 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.
> > >>
> > >> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > >
> > > It was 32-bit on purpose, to avoid overflow in this loop:
> > > 	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++)
> > >
> > > It is now replaced by RTE_ETH_FOREACH_VALID_DEV, but I wonder
> > > whether we still have this theoritical overflow risk.
> > > If yes, we should change more variables to 32-bit.
> >
> > Ah, it is too tricky. May be it is better to ensure that
> > RTE_MAX_ETHPORTS is less or equal to UINT16_MAX?
> 
> Yes could be another option.
> 

Add a check on RTE_MAX_ETHPORT in rte_eal_init()?

Re: [dpdk-dev] [PATCH] ethdev: fix data type for port id

[Thomas Monjalon]

27/10/2020 03:46, wangyunjian:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 26/10/2020 13:33, Andrew Rybchenko:
> > > On 10/26/20 3:29 PM, Thomas Monjalon wrote:
> > > > 26/10/2020 13:24, 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.
> > > >>
> > > >> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > > >
> > > > It was 32-bit on purpose, to avoid overflow in this loop:
> > > > 	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++)
> > > >
> > > > It is now replaced by RTE_ETH_FOREACH_VALID_DEV, but I wonder
> > > > whether we still have this theoritical overflow risk.
> > > > If yes, we should change more variables to 32-bit.
> > >
> > > Ah, it is too tricky. May be it is better to ensure that
> > > RTE_MAX_ETHPORTS is less or equal to UINT16_MAX?
> > 
> > Yes could be another option.
> > 
> 
> Add a check on RTE_MAX_ETHPORT in rte_eal_init()?

Could be a compilation check done with RTE_BUILD_BUG_ON.

[dpdk-dev] [PATCH v2] ethdev: fix data type for port id

[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.

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(-)

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);
 }
-- 
2.23.0

Re: [dpdk-dev] [PATCH v2] ethdev: fix data type for port id

[Thomas Monjalon]

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?

Re: [dpdk-dev] [PATCH v2] ethdev: fix data type for port id

[wangyunjian]

> -----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);

Re: [dpdk-dev] [PATCH v2] ethdev: fix data type for port id

[Andrew Rybchenko]

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.

Re: [dpdk-dev] [PATCH v2] ethdev: fix data type for port id

[Thomas Monjalon]

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.

[dpdk-dev] [PATCH v3] ethdev: fix data type for port id

[wangyunjian]

From: Yunjian Wang <wangyunjian@huawei.com>

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

RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
the user. To avoid 16-bit unsigned integer overflow, the valid value
of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
safer to cut one more port from space.

So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
to UINT16_MAX.

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

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
v3:
   fix code styles suggested by Thomas Monjalon and Andrew Rybchenko
v2:
   add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
---
 lib/librte_ethdev/rte_ethdev.c | 36 +++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b12bb3854d..5594855c6e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -332,6 +332,8 @@ rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
 uint16_t
 rte_eth_find_next(uint16_t port_id)
 {
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	while (port_id < RTE_MAX_ETHPORTS &&
 			rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
 		port_id++;
@@ -354,6 +356,8 @@ rte_eth_find_next(uint16_t port_id)
 uint16_t
 rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent)
 {
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	port_id = rte_eth_find_next(port_id);
 	while (port_id < RTE_MAX_ETHPORTS &&
 			rte_eth_devices[port_id].device != parent)
@@ -411,7 +415,9 @@ eth_dev_is_allocated(const struct rte_eth_dev *ethdev)
 static struct rte_eth_dev *
 eth_dev_allocated(const char *name)
 {
-	unsigned i;
+	uint16_t i;
+
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (rte_eth_devices[i].data != NULL &&
@@ -440,7 +446,9 @@ rte_eth_dev_allocated(const char *name)
 static uint16_t
 eth_dev_find_free_port(void)
 {
-	unsigned i;
+	uint16_t i;
+
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		/* Using shared name field to find a free port. */
@@ -528,6 +536,8 @@ rte_eth_dev_attach_secondary(const char *name)
 	/* Synchronize port attachment to primary port creation and release. */
 	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
 
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
 			break;
@@ -609,6 +619,8 @@ eth_is_valid_owner_id(uint64_t owner_id)
 uint64_t
 rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 {
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	port_id = rte_eth_find_next(port_id);
 	while (port_id < RTE_MAX_ETHPORTS &&
 			rte_eth_devices[port_id].data->owner.id != owner_id)
@@ -713,6 +725,8 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 
 	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
 
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++)
 			if (rte_eth_devices[port_id].data->owner.id == owner_id)
@@ -816,7 +830,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");
@@ -4290,7 +4304,9 @@ rte_eth_mirror_rule_reset(uint16_t port_id, uint8_t rule_id)
 
 RTE_INIT(eth_dev_init_cb_lists)
 {
-	int i;
+	uint16_t 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);
@@ -4303,9 +4319,11 @@ rte_eth_dev_callback_register(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_callback *user_cb;
-	uint32_t next_port; /* size is 32-bit to prevent loop wrap-around */
+	uint16_t next_port;
 	uint16_t last_port;
 
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	if (!cb_fn)
 		return -EINVAL;
 
@@ -4366,9 +4384,11 @@ rte_eth_dev_callback_unregister(uint16_t port_id,
 	int ret;
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_callback *cb, *next;
-	uint32_t next_port; /* size is 32-bit to prevent loop wrap-around */
+	uint16_t next_port;
 	uint16_t last_port;
 
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	if (!cb_fn)
 		return -EINVAL;
 
@@ -5507,10 +5527,12 @@ static struct rte_eth_dev_switch {
 int
 rte_eth_switch_domain_alloc(uint16_t *domain_id)
 {
-	unsigned int i;
+	uint16_t i;
 
 	*domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
 
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
+
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (eth_dev_switch_domains[i].state ==
 			RTE_ETH_SWITCH_DOMAIN_UNUSED) {
-- 
2.23.0

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] ethdev: fix data type for port id

[Thomas Monjalon]

02/11/2020 10:18, wangyunjian:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> The ethdev port id is 16 bits now. This patch fixes the data type
> of the variable for 'pid', which changing from uint32_t to uint16_t.
> 
> RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
> the user. To avoid 16-bit unsigned integer overflow, the valid value
> of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
> safer to cut one more port from space.
> 
> So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
> to UINT16_MAX.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
> v3:
>    fix code styles suggested by Thomas Monjalon and Andrew Rybchenko

I don't understand why you add the compile check in several place.
We should just find one good place for checking RTE_MAX_ETHPORTS
at compilation time.

I suggest rte_eth_find_next() or rte_eth_dev_allocate().
Or maybe rte_eth_dev_is_valid_port().

Please choose only one.

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] ethdev: fix data type for port id

[Stephen Hemminger]

On Tue, 03 Nov 2020 19:44:21 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 02/11/2020 10:18, wangyunjian:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> > 
> > The ethdev port id is 16 bits now. This patch fixes the data type
> > of the variable for 'pid', which changing from uint32_t to uint16_t.
> > 
> > RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
> > the user. To avoid 16-bit unsigned integer overflow, the valid value
> > of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
> > safer to cut one more port from space.
> > 
> > So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
> > to UINT16_MAX.
> > 
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> > v3:
> >    fix code styles suggested by Thomas Monjalon and Andrew Rybchenko  
> 
> I don't understand why you add the compile check in several place.
> We should just find one good place for checking RTE_MAX_ETHPORTS
> at compilation time.
> 
> I suggest rte_eth_find_next() or rte_eth_dev_allocate().
> Or maybe rte_eth_dev_is_valid_port().
> 
> Please choose only one.
> 
> 

That was my reaction, check once.
It doesn't even need to be inside a function, it can be anywhere.

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] ethdev: fix data type for port id

[wangyunjian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, November 4, 2020 2:44 AM
> 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>;
> stable@dpdk.org
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3] ethdev: fix data type for port id
> 
> 02/11/2020 10:18, wangyunjian:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > The ethdev port id is 16 bits now. This patch fixes the data type of
> > the variable for 'pid', which changing from uint32_t to uint16_t.
> >
> > RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
> > the user. To avoid 16-bit unsigned integer overflow, the valid value
> > of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
> > safer to cut one more port from space.
> >
> > So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
> > to UINT16_MAX.
> >
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> > v3:
> >    fix code styles suggested by Thomas Monjalon and Andrew Rybchenko
> 
> I don't understand why you add the compile check in several place.
> We should just find one good place for checking RTE_MAX_ETHPORTS at
> compilation time.
> 
> I suggest rte_eth_find_next() or rte_eth_dev_allocate().
> Or maybe rte_eth_dev_is_valid_port().
> 
> Please choose only one.
> 
OK, I will add the compile check in function rte_eth_dev_allocate().

Thanks
Yunjian

[dpdk-dev] [PATCH v4] ethdev: fix data type for port id

[wangyunjian]

From: Yunjian Wang <wangyunjian@huawei.com>

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

RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
the user. To avoid 16-bit unsigned integer overflow, the valid value
of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
safer to cut one more port from space.

So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
to UINT16_MAX.

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

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
v4:
   remove redundant compile checks
v3:
   fix code styles suggested by Thomas Monjalon and Andrew Rybchenko
v2:
   add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
---
 lib/librte_ethdev/rte_ethdev.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b12bb3854d..0af8e4d76c 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -411,7 +411,9 @@ eth_dev_is_allocated(const struct rte_eth_dev *ethdev)
 static struct rte_eth_dev *
 eth_dev_allocated(const char *name)
 {
-	unsigned i;
+	uint16_t i;
+
+	RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (rte_eth_devices[i].data != NULL &&
@@ -440,7 +442,7 @@ rte_eth_dev_allocated(const char *name)
 static uint16_t
 eth_dev_find_free_port(void)
 {
-	unsigned i;
+	uint16_t i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		/* Using shared name field to find a free port. */
@@ -816,7 +818,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");
@@ -4290,7 +4292,7 @@ rte_eth_mirror_rule_reset(uint16_t port_id, uint8_t rule_id)
 
 RTE_INIT(eth_dev_init_cb_lists)
 {
-	int i;
+	uint16_t i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
 		TAILQ_INIT(&rte_eth_devices[i].link_intr_cbs);
@@ -4303,7 +4305,7 @@ rte_eth_dev_callback_register(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_callback *user_cb;
-	uint32_t next_port; /* size is 32-bit to prevent loop wrap-around */
+	uint16_t next_port;
 	uint16_t last_port;
 
 	if (!cb_fn)
@@ -4366,7 +4368,7 @@ rte_eth_dev_callback_unregister(uint16_t port_id,
 	int ret;
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_callback *cb, *next;
-	uint32_t next_port; /* size is 32-bit to prevent loop wrap-around */
+	uint16_t next_port;
 	uint16_t last_port;
 
 	if (!cb_fn)
@@ -5507,7 +5509,7 @@ static struct rte_eth_dev_switch {
 int
 rte_eth_switch_domain_alloc(uint16_t *domain_id)
 {
-	unsigned int i;
+	uint16_t i;
 
 	*domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
 
-- 
2.18.1

Re: [dpdk-dev] [PATCH v4] ethdev: fix data type for port id

[Ajit Khaparde]

On Tue, Nov 3, 2020 at 6:58 PM wangyunjian <wangyunjian@huawei.com> wrote:
>
> From: Yunjian Wang <wangyunjian@huawei.com>
>
> The ethdev port id is 16 bits now. This patch fixes the data type
> of the variable for 'pid', which changing from uint32_t to uint16_t.
>
> RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
> the user. To avoid 16-bit unsigned integer overflow, the valid value
> of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
> safer to cut one more port from space.
>
> So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
> to UINT16_MAX.
>
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Cc: stable@dpdk.org
>
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>

Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

> ---
> v4:
>    remove redundant compile checks
> v3:
>    fix code styles suggested by Thomas Monjalon and Andrew Rybchenko
> v2:
>    add RTE_BUILD_BUG_ON() check for RTE_MAX_ETHPORTS validity
> ---
>  lib/librte_ethdev/rte_ethdev.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b12bb3854d..0af8e4d76c 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -411,7 +411,9 @@ eth_dev_is_allocated(const struct rte_eth_dev *ethdev)
>  static struct rte_eth_dev *
>  eth_dev_allocated(const char *name)
>  {
> -       unsigned i;
> +       uint16_t i;
> +
> +       RTE_BUILD_BUG_ON(RTE_MAX_ETHPORTS >= UINT16_MAX);
>
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>                 if (rte_eth_devices[i].data != NULL &&
> @@ -440,7 +442,7 @@ rte_eth_dev_allocated(const char *name)
>  static uint16_t
>  eth_dev_find_free_port(void)
>  {
> -       unsigned i;
> +       uint16_t i;
>
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>                 /* Using shared name field to find a free port. */
> @@ -816,7 +818,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");
> @@ -4290,7 +4292,7 @@ rte_eth_mirror_rule_reset(uint16_t port_id, uint8_t rule_id)
>
>  RTE_INIT(eth_dev_init_cb_lists)
>  {
> -       int i;
> +       uint16_t i;
>
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++)
>                 TAILQ_INIT(&rte_eth_devices[i].link_intr_cbs);
> @@ -4303,7 +4305,7 @@ rte_eth_dev_callback_register(uint16_t port_id,
>  {
>         struct rte_eth_dev *dev;
>         struct rte_eth_dev_callback *user_cb;
> -       uint32_t next_port; /* size is 32-bit to prevent loop wrap-around */
> +       uint16_t next_port;
>         uint16_t last_port;
>
>         if (!cb_fn)
> @@ -4366,7 +4368,7 @@ rte_eth_dev_callback_unregister(uint16_t port_id,
>         int ret;
>         struct rte_eth_dev *dev;
>         struct rte_eth_dev_callback *cb, *next;
> -       uint32_t next_port; /* size is 32-bit to prevent loop wrap-around */
> +       uint16_t next_port;
>         uint16_t last_port;
>
>         if (!cb_fn)
> @@ -5507,7 +5509,7 @@ static struct rte_eth_dev_switch {
>  int
>  rte_eth_switch_domain_alloc(uint16_t *domain_id)
>  {
> -       unsigned int i;
> +       uint16_t i;
>
>         *domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
>
> --
> 2.18.1
>

Re: [dpdk-dev] [PATCH v4] ethdev: fix data type for port id

[Ferruh Yigit]

On 11/4/2020 3:26 AM, Ajit Khaparde wrote:
> On Tue, Nov 3, 2020 at 6:58 PM wangyunjian <wangyunjian@huawei.com> wrote:
>>
>> From: Yunjian Wang <wangyunjian@huawei.com>
>>
>> The ethdev port id is 16 bits now. This patch fixes the data type
>> of the variable for 'pid', which changing from uint32_t to uint16_t.
>>
>> RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
>> the user. To avoid 16-bit unsigned integer overflow, the valid value
>> of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
>> safer to cut one more port from space.
>>
>> So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
>> to UINT16_MAX.
>>
>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> 
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH v4] ethdev: fix data type for port id

[Ferruh Yigit]

On 11/4/2020 11:46 AM, Ferruh Yigit wrote:
> On 11/4/2020 3:26 AM, Ajit Khaparde wrote:
>> On Tue, Nov 3, 2020 at 6:58 PM wangyunjian <wangyunjian@huawei.com> wrote:
>>>
>>> From: Yunjian Wang <wangyunjian@huawei.com>
>>>
>>> The ethdev port id is 16 bits now. This patch fixes the data type
>>> of the variable for 'pid', which changing from uint32_t to uint16_t.
>>>
>>> RTE_MAX_ETHPORTS is the maximum number of ports, which customized by
>>> the user. To avoid 16-bit unsigned integer overflow, the valid value
>>> of RTE_MAX_ETHPORTS should be set from 0 to UINT16_MAX, and it is
>>> safer to cut one more port from space.
>>>
>>> So we use RTE_BUILD_BUG_ON() to ensure that RTE_MAX_ETHPORTS is less
>>> to UINT16_MAX.
>>>
>>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/main, thanks.