Re: [PATCH RESEND v2 02/11] net/tap: check if name is null
On Wed, 2023-01-18 at 10:57 +, Ferruh Yigit wrote: > I assume this is highlighted by a tool but in practice > > 'rte_vdev_device_name()' should not return NULL, and there are many > > other location this check is missing. That's correct. Warning was found by codeql static analysis tool. When I search the codebase, I do see a mixture of null checks and no checks for this function. We can ignore this patch, if you feel strongly against it.
Re: [PATCH RESEND v2 01/11] ethdev: check return result of rte_eth_dev_info_get
On Wed, 2023-01-18 at 09:42 +0100, Thomas Monjalon wrote: > 22/11/2022 16:30, > ok...@kernel.org > : > > > rte_class_eth: eth_mac_cmp: The status of this call to > > rte_eth_dev_info_get > > is not checked, potentially leaving dev_info uninitialized. > > [...] > > >/* Return 0 if devargs MAC is matching one of the device > > MACs. */ > > - rte_eth_dev_info_get(data->port_id, &dev_info); > > + if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0) > > + return -1; > > + > >for (index = 0; index < dev_info.max_mac_addrs; index++) > >if (rte_is_same_ether_addr(&mac, &data- > > >mac_addrs[index])) > >return 0; > > > > Acked-by: Thomas Monjalon < > tho...@monjalon.net > > > > Thanks. > > You would get more attention if you Cc the maintainers. > > You can use this git option: --cc-cmd devtools/get-maintainer.sh Good to know that we have get-maintainer script.
Re: [PATCH v3 00/10] codeql fixes for various subsystems
On Thu, 2023-01-19 at 23:41 -0500, ok...@kernel.org wrote: > From: Sinan Kaya > > Following up the codeql reported problems first submitted > by Stephen Hemminger here: > > https://lore.kernel.org/all/20220527161210.77212d0b@hermes.local/t/ > > Posting a series of fixes about potential null pointer accesses. > > Changes from v3: > - Dropped net/tap: check if name is null > - Moved the comment to the appropriate position in ethdev: check > return result of rte_eth_dev_info_get > > Changes from v2: > - Remove braces around single line statements > - use NULL comparisons > > > Sinan Kaya (10): > ethdev: check return result of rte_eth_dev_info_get > memzone: check result of rte_fbarray_get > memzone: check result of malloc_elem_from_data > malloc: malloc_elem_join_adjacent_free can return null > malloc: check result of rte_mem_virt2memseg_list > malloc: check result of rte_fbarray_get > malloc: check result of rte_mem_virt2memseg > malloc: check result of malloc_elem_free > malloc: check result of elem_start_pt > bus/vdev: check result of rte_vdev_device_name > > lib/eal/common/eal_common_memalloc.c | 5 - > lib/eal/common/eal_common_memzone.c | 10 +- > lib/eal/common/malloc_elem.c | 14 +++--- > lib/eal/common/malloc_heap.c | 9 - > lib/ethdev/ethdev_vdev.h | 2 ++ > lib/ethdev/rte_class_eth.c | 4 +++- > 6 files changed, 37 insertions(+), 7 deletions(-) > Anatoly, any feedback on the series? You seem to be a maintainer for 2-9 patches.
Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name
On Sun, 2023-01-22 at 21:51 +0100, Thomas Monjalon wrote:
> 20/01/2023 17:47, Stephen Hemminger:
> > On Thu, 19 Jan 2023 23:41:40 -0500
> > ok...@kernel.org wrote:
> >
> > > diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
> > > index 364f140f91..6d94a65d97 100644
> > > --- a/lib/ethdev/ethdev_vdev.h
> > > +++ b/lib/ethdev/ethdev_vdev.h
> > > @@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device
> > > *dev, size_t private_data_size)
> > > {
> > > struct rte_eth_dev *eth_dev;
> > > const char *name = rte_vdev_device_name(dev);
> > > + if (name == NULL)
> > > + return NULL;
> >
> > Please add a blank line after declarations and before code.
> > For some reason the DPDK version of checkpatch suppresses this
> > warning.
>
> The check of "name" is related to the line above,
> so I don't think we should insert a blank line.
>
>
While I'm waiting for maintainer feedback, let me know which
way I should go. I can update this for next rev.
Re: [PATCH v3 00/10] codeql fixes for various subsystems
On Thu, 2023-07-06 at 15:43 -0700, Stephen Hemminger wrote: > > lib/eal/common/eal_common_memalloc.c | 5 - > > lib/eal/common/eal_common_memzone.c | 10 +- > > lib/eal/common/malloc_elem.c | 14 +++--- > > lib/eal/common/malloc_heap.c | 9 - > > lib/ethdev/ethdev_vdev.h | 2 ++ > > lib/ethdev/rte_class_eth.c | 4 +++- > > 6 files changed, 37 insertions(+), 7 deletions(-) > > > The patches are fine, only warning was from the requirement that all > > submitters be in .mailmap now. Do you mind if I rebase these, add a > mailmap > > and combine the like ones together? > Please go ahead. > > > Acked-by: Stephen Hemminger
Re: [PATCH v1 0/7] support reinit flow
On Mon, 2023-08-14 at 19:12 -0700, Stephen Hemminger wrote: > On Mon, 14 Aug 2023 21:38:19 -0400 > > ok...@kernel.org wrote: > > > > > From: Sinan Kaya > > We want to be able to call rte_eal_init() and rte_eal_cleanup() > > APIs back to back for maintanance reasons. > > > Why? > > This change exposes lots of drivers, API's and other things to > untested code paths. > > If you just want to do a hard restart, maybe just reexecing the same > application would > > be better. It is not always ideal to reinitialize a DPDK process. Memory needs to be reinitialized, hugetables need to warm up etc. Given the cost of change is not huge, I believe this will be important contribution for certain application types. Changeset is not doing anything different from what already exists.
Re: [PATCH v4 2/8] eal: fixes for re-initialization issues
On Tue, 2023-08-15 at 10:49 -0700, Stephen Hemminger wrote: > > > > + __atomic_store_n(&run_once, 0, __ATOMIC_RELAXED); > >return 0; > > > Interesting, other flags don't use atomic. Why here? > > > > And is already set elsewhere? Looking at the history, this variable used to be an atomic variable. Later, it got replaced with a uint32_t and write is done with __atomic_store_n instead. I followed other examples in the same source code file.
Re: [PATCH v4 8/8] eal: initialize worker threads once
On Tue, 2023-08-15 at 10:46 -0700, Stephen Hemminger wrote: > > static uint32_t run_once; > > +static bool worker_initialized; > > > > > I see a pattern here. Many places are using static to > > only initialize once. Could you name these variables the > > same; suggest using run_once everywhere. sure, will do.
Re: [PATCH v4 0/8] support reinit flow
On Tue, 2023-08-15 at 10:45 -0700, Stephen Hemminger wrote: > > Why? > > It is not always ideal to reinitialize a DPDK process. Memory needs > > to be reinitialized, hugetables need to warm up etc. > > > > > I am familiar with the backstory of why this is desirable in your > case. > > But others may not be. It will work for you, but for the wider the > > range of libraries and drivers it probably won't. > > Fair enough. > > As a compromise, can this restart be officially tagged as > unsupported. any pointers how to do this? I have no idea how to mark something unsupported in code. If this is acceptable in cover letter, I'm happy to do that too. > > I.e. it may work for some drivers and libraries but not all of them. > > If nothing else many parts of DPDK still do leak memory on cleanup > > and currently this is harmless. > > > > This has enough impact that it probably needs to wait past 23.11 > release. > > Sure, no rush. Happy to wait for time slot and accumulate review feedbacks. > > Could you add a test for restart into standalone tests and test-pmd? Will do.
Re: [PATCH 02/11] net/tap: check if name is null
On Mon, 2022-11-21 at 22:41 +0100, Thomas Monjalon wrote:
> 21/11/2022 21:40, ok...@kernel.org:
> > --- a/drivers/net/tap/rte_eth_tap.c+++
> > b/drivers/net/tap/rte_eth_tap.c@@ -2340,6 +2340,10 @@
> > rte_pmd_tun_probe(struct rte_vdev_device *dev) struct
> > rte_eth_dev *eth_dev; name = rte_vdev_device_name(dev);+ if
> > (!name) {
>
> Please it is preferred to check against NULL,because name is not a
> boolean, thanks.I know it's longer but it is more explicit.
Sure, I can do that. Getting used to dpdk coding style. I wasn't sure
what to do with braces on single line too. At least, I got a warning on
that too.
> Thanks for the fixes in this series.
>
Cheers
Re: [PATCH v2 00/11] codeql fixes for various subsystems
On Tue, 2022-11-22 at 16:24 +0100, David Marchand wrote: > Thanks for the fixes, but it looks like the v2 series is truncated. > > Can you resend it so it passes through the CI? Sure
Re: [PATCH RESEND v2 00/11] codeql fixes for various subsystems
On Tue, 2022-11-22 at 10:30 -0500, ok...@kernel.org wrote: > From: Sinan Kaya > Following up the codeql reported problems first submittedby Stephen > Hemminger here: > https://lore.kernel.org/all/20220527161210.77212d0b@hermes.local/t/ > > Posting a series of fixes about potential null pointer accesses. > Changes from v1:- Remove braces around single line statements- use > NULL comparisons > Sinan Kaya (11): ethdev: check return result of > rte_eth_dev_info_get net/tap: check if name is null memzone: check > result of rte_fbarray_get memzone: check result of > malloc_elem_from_data malloc: malloc_elem_join_adjacent_free can > return null malloc: check result of > rte_mem_virt2memseg_list malloc: check result of > rte_fbarray_get malloc: check result of rte_mem_virt2memseg malloc: > check result of malloc_elem_free malloc: check result of > elem_start_pt bus/vdev: check result of rte_vdev_device_name > drivers/net/tap/rte_eth_tap.c| 3 +++ > lib/eal/common/eal_common_memalloc.c | 5 - > lib/eal/common/eal_common_memzone.c | 10 +- > lib/eal/common/malloc_elem.c | 14 +++--- > lib/eal/common/malloc_heap.c | 9 - > lib/ethdev/ethdev_vdev.h | 2 ++ > lib/ethdev/rte_class_eth.c | 4 +++- 7 files changed, 40 > insertions(+), 7 deletions(-) > Checking to see if I need to cover any feedback.
Re: [PATCH RESEND v2 00/11] codeql fixes for various subsystems
On Tue, 2022-12-13 at 19:59 +0300, Dmitry Kozlyuk wrote: > 2022-12-13 11:43 (UTC-0500), Sinan Kaya: > > Checking to see if I need to cover any feedback. > > There was some feedback to pre-resend v2 that is not visible on > patchwork: > > http://inbox.dpdk.org/dev/cajfav8yee9awya3ovluqavfw0mzonsqjq-k+2q4wampmyrj...@mail.gmail.com/ > > Yes, I sent it again with RESEND tag this time on this series. Just checking to see if it went through this time. I do see the RESEND series on patchwork.
Re: [PATCH RESEND v2 00/11] codeql fixes for various subsystems
On Tue, 2022-12-13 at 12:02 -0500, Sinan Kaya wrote: > On Tue, 2022-12-13 at 19:59 +0300, Dmitry Kozlyuk wrote: > > 2022-12-13 11:43 (UTC-0500), Sinan Kaya: > > > Checking to see if I need to cover any feedback. > > > > There was some feedback to pre-resend v2 that is not visible on > > patchwork: > > > > http://inbox.dpdk.org/dev/cajfav8yee9awya3ovluqavfw0mzonsqjq-k+2q4wampmyrj...@mail.gmail.com/ > > > > > > > > Yes, I sent it again with RESEND tag this time on this series. > Just checking to see if it went through this time. > I do see the RESEND series on patchwork. > > I was planning to send the fixes in batches hoping this one to merge first. I guess I'll post the remaning bugfixes using a different series.
[dpdk-dev] [PATCH] ethdev: support PCI domains
The current code is enumerating devices based on bus, device and function
pairs. This does not work well for architectures with multiple PCI
segments/domains. Multiple PCI devices will have the same BDF value but
different segment numbers (01:01:01.0 and 02:01:01.0) for instance.
Adding segment numbers to device naming so that we can uniquely identify
devices.
Signed-off-by: Sinan Kaya
---
lib/librte_ether/rte_ethdev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0a6e3f1..929240f 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -226,7 +226,8 @@ rte_eth_dev_create_unique_device_name(char *name, size_t
size,
{
int ret;
- ret = snprintf(name, size, "%d:%d.%d",
+ ret = snprintf(name, size, "%d:%d:%d.%d",
+ pci_dev->addr.domain,
pci_dev->addr.bus, pci_dev->addr.devid,
pci_dev->addr.function);
if (ret < 0)
--
1.8.2.1
[dpdk-dev] [PATCH] ethdev: support PCI domains
On 7/22/2016 5:12 PM, Stephen Hemminger wrote: > On Fri, 22 Jul 2016 11:34:10 -0400 > Sinan Kaya wrote: > >> The current code is enumerating devices based on bus, device and function >> pairs. This does not work well for architectures with multiple PCI >> segments/domains. Multiple PCI devices will have the same BDF value but >> different segment numbers (01:01:01.0 and 02:01:01.0) for instance. >> >> Adding segment numbers to device naming so that we can uniquely identify >> devices. >> >> Signed-off-by: Sinan Kaya > > I ran into this yes. There is a small risk of breaking some application that > assumed something about names though. > > Acked-by: Stephen Hemminger > Thanks, hopefully the change is minor and can be contained until next release. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[dpdk-dev] [PATCH] ethdev: support PCI domains
On 10/4/2016 4:15 AM, Thomas Monjalon wrote: > 2016-07-22 18:56, Sinan Kaya: >> On 7/22/2016 5:12 PM, Stephen Hemminger wrote: >>> On Fri, 22 Jul 2016 11:34:10 -0400 >>> Sinan Kaya wrote: >>> >>>> The current code is enumerating devices based on bus, device and function >>>> pairs. This does not work well for architectures with multiple PCI >>>> segments/domains. Multiple PCI devices will have the same BDF value but >>>> different segment numbers (01:01:01.0 and 02:01:01.0) for instance. >>>> >>>> Adding segment numbers to device naming so that we can uniquely identify >>>> devices. >>>> >>>> Signed-off-by: Sinan Kaya >>> >>> I ran into this yes. There is a small risk of breaking some application that >>> assumed something about names though. >>> >>> Acked-by: Stephen Hemminger >>> >> >> Thanks, hopefully the change is minor and can be contained until next >> release. > > It is part of the EAL rework. > The function has been moved in EAL and includes the PCI domain: > http://dpdk.org/commit/affe1cdc > Thanks for taking care of it. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
