ethdev: fix data type for port id
diff mbox series

Message ID e17cf818a98e4dd5109acacd8c2bd754a0c1abda.1603713753.git.wangyunjian@huawei.com
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series
  • ethdev: fix data type for port id
Related show

Checks

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

Commit Message

wangyunjian Oct. 26, 2020, 12:24 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.

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

Comments

Thomas Monjalon Oct. 26, 2020, 12:29 p.m. UTC | #1
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.
Andrew Rybchenko Oct. 26, 2020, 12:29 p.m. UTC | #2
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>
Andrew Rybchenko Oct. 26, 2020, 12:33 p.m. UTC | #3
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?
Thomas Monjalon Oct. 26, 2020, 12:34 p.m. UTC | #4
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.
wangyunjian Oct. 27, 2020, 2:46 a.m. UTC | #5
> -----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()?
Thomas Monjalon Oct. 27, 2020, 8:47 a.m. UTC | #6
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.

Patch
diff mbox series

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