diff mbox series

bus/pci: don't open uio device in secondary process

Message ID 20240828104002.226704-1-konrad.sztyber@intel.com (mailing list archive)
State Changes Requested
Delegated to: Thomas Monjalon
Headers
Series bus/pci: don't open uio device in secondary process |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing fail Testing issues
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Konrad Sztyber Aug. 28, 2024, 10:40 a.m. UTC 
The uio_pci_generic driver clears the bus master bit when the device
file is closed.  So, when the secondary process terminates after probing
a device, that device becomes unusable in the primary process.

To avoid that, the device file is now opened only in the primary
process.  The commit that introduced this regression, 847d78fb95
("bus/pci: fix FD in secondary process"), only mentioned enabling access
to config space from secondary process, which still works, as it doesn't
rely on the device file.

Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
---
 drivers/bus/pci/linux/pci_uio.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Chaoyong He Aug. 29, 2024, 5:53 a.m. UTC | #1 
> 
> The uio_pci_generic driver clears the bus master bit when the device file is
> closed.  So, when the secondary process terminates after probing a device,
> that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary process.  The
> commit that introduced this regression, 847d78fb95
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access to
> config space from secondary process, which still works, as it doesn't rely on
> the device file.

Yes, we can still access the config space from secondary process.

> 
> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")

Maybe here also need a 'Cc: stable@dpdk.org' ?

With this, It looks good to me, thanks.
Acked-by: Chaoyong He <chaoyong.he@corigine.com>

> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 4c1d3327a9..432316afcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>                         loc->domain, loc->bus, loc->devid, loc->function);
>                 return 1;
>         }
> -       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> -
> -       /* save fd */
> -       fd = open(devname, O_RDWR);
> -       if (fd < 0) {
> -               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> -               goto error;
> -       }
> -
> -       if (rte_intr_fd_set(dev->intr_handle, fd))
> -               goto error;
> -
>         snprintf(cfgname, sizeof(cfgname),
>                         "/sys/class/uio/uio%u/device/config", uio_num);
> 
> @@ -273,6 +261,19 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;
> 
> +       /* the uio_pci_generic driver clears the bus master enable bit when the
> device file is
> +        * closed, so open it only in the primary process */
> +       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> +       /* save fd */
> +       fd = open(devname, O_RDWR);
> +       if (fd < 0) {
> +               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> +               goto error;
> +       }
> +
> +       if (rte_intr_fd_set(dev->intr_handle, fd))
> +               goto error;
> +
>         /* allocate the mapping details for secondary processes*/
>         *uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
>         if (*uio_res == NULL) {
> --
> 2.45.0
Chenbo Xia Aug. 29, 2024, 8:06 a.m. UTC | #2 
> On Aug 28, 2024, at 18:40, Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> The uio_pci_generic driver clears the bus master bit when the device
> file is closed.  So, when the secondary process terminates after probing

Should be one space before ‘So'

> a device, that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary
> process.  The commit that introduced this regression, 847d78fb95
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> to config space from secondary process, which still works, as it doesn't
> rely on the device file.
> 
> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")

Besides the cc stable tag mentioned by Chaoyong, commit ID should be 12-digit.

Please also fix the coding style:

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#176: FILE: drivers/bus/pci/linux/pci_uio.c:265:
+ * closed, so open it only in the primary process */

With above fixed:

Reviewed-by: Chenbo Xia <chenbox@nvidia.com>

> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> ---
> drivers/bus/pci/linux/pci_uio.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 4c1d3327a9..432316afcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>                        loc->domain, loc->bus, loc->devid, loc->function);
>                return 1;
>        }
> -       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> -
> -       /* save fd */
> -       fd = open(devname, O_RDWR);
> -       if (fd < 0) {
> -               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> -               goto error;
> -       }
> -
> -       if (rte_intr_fd_set(dev->intr_handle, fd))
> -               goto error;
> -
>        snprintf(cfgname, sizeof(cfgname),
>                        "/sys/class/uio/uio%u/device/config", uio_num);
> 
> @@ -273,6 +261,19 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>        if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                return 0;
> 
> +       /* the uio_pci_generic driver clears the bus master enable bit when the device file is
> +        * closed, so open it only in the primary process */
> +       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> +       /* save fd */
> +       fd = open(devname, O_RDWR);
> +       if (fd < 0) {
> +               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> +               goto error;
> +       }
> +
> +       if (rte_intr_fd_set(dev->intr_handle, fd))
> +               goto error;
> +
>        /* allocate the mapping details for secondary processes*/
>        *uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
>        if (*uio_res == NULL) {
> --
> 2.45.0
>
Stephen Hemminger Oct. 7, 2024, 5:49 p.m. UTC | #3 
On Wed, 28 Aug 2024 12:40:02 +0200
Konrad Sztyber <konrad.sztyber@intel.com> wrote:

> The uio_pci_generic driver clears the bus master bit when the device
> file is closed.  So, when the secondary process terminates after probing
> a device, that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary
> process.  The commit that introduced this regression, 847d78fb95
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> to config space from secondary process, which still works, as it doesn't
> rely on the device file.
> 
> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")
> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>

Wouldn't this break use of interrupts in the secondary process?

The patch does need the minor fix of the comment style.
So resubmit
Konrad Sztyber Oct. 9, 2024, 10:11 a.m. UTC | #4 
On 10/7/24 19:49, Stephen Hemminger wrote:
> On Wed, 28 Aug 2024 12:40:02 +0200
> Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> 
>> The uio_pci_generic driver clears the bus master bit when the device
>> file is closed.  So, when the secondary process terminates after probing
>> a device, that device becomes unusable in the primary process.
>>
>> To avoid that, the device file is now opened only in the primary
>> process.  The commit that introduced this regression, 847d78fb95
>> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
>> to config space from secondary process, which still works, as it doesn't
>> rely on the device file.
>>
>> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")
>>
>> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> 
> Wouldn't this break use of interrupts in the secondary process?

Yes, it will. But I don't think we can support interrupts in the 
secondary process *and*, at the same time, keep the device usable in the 
primary process when secondary terminates. Maybe we could pass the fd 
via SCM_RIGHTS? But I don't know if that results in the same struct file 
being used by both processes.

> The patch does need the minor fix of the comment style.
> So resubmit

I already did, see: 
https://inbox.dpdk.org/dev/20240829085724.270041-1-konrad.sztyber@intel.com
Stephen Hemminger Oct. 9, 2024, 3:12 p.m. UTC | #5 
On Wed, 9 Oct 2024 12:11:32 +0200
Konrad Sztyber <konrad.sztyber@intel.com> wrote:

> On 10/7/24 19:49, Stephen Hemminger wrote:
> > On Wed, 28 Aug 2024 12:40:02 +0200
> > Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> >   
> >> The uio_pci_generic driver clears the bus master bit when the device
> >> file is closed.  So, when the secondary process terminates after probing
> >> a device, that device becomes unusable in the primary process.
> >>
> >> To avoid that, the device file is now opened only in the primary
> >> process.  The commit that introduced this regression, 847d78fb95
> >> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> >> to config space from secondary process, which still works, as it doesn't
> >> rely on the device file.
> >>
> >> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")
> >>
> >> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>  
> > 
> > Wouldn't this break use of interrupts in the secondary process?  
> 
> Yes, it will. But I don't think we can support interrupts in the 
> secondary process *and*, at the same time, keep the device usable in the 
> primary process when secondary terminates. Maybe we could pass the fd 
> via SCM_RIGHTS? But I don't know if that results in the same struct file 
> being used by both processes.

That is what tap, and xdp are doing.
Konrad Sztyber Oct. 11, 2024, 6:38 a.m. UTC | #6 
On 10/9/24 17:12, Stephen Hemminger wrote:
> That is what tap, and xdp are doing.

Thanks for the info. I'll take a look and will send a next rev doing 
something similar in uio.

Konrad
David Marchand Oct. 11, 2024, 6:46 a.m. UTC | #7 
On Fri, Oct 11, 2024 at 8:38 AM Konrad Sztyber <konrad.sztyber@intel.com> wrote:
>
> On 10/9/24 17:12, Stephen Hemminger wrote:
> > That is what tap, and xdp are doing.
>
> Thanks for the info. I'll take a look and will send a next rev doing
> something similar in uio.

My two cents.
net/tap uses the rte_mp_* infrastructure and this is what you need.
net/af_xdp has two cases, so don't get confused with the custom
SCM_RIGHTS stuff and stick to rte_mp_*.
diff mbox series

Patch

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 4c1d3327a9..432316afcc 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -232,18 +232,6 @@  pci_uio_alloc_resource(struct rte_pci_device *dev,
 			loc->domain, loc->bus, loc->devid, loc->function);
 		return 1;
 	}
-	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
-
-	/* save fd */
-	fd = open(devname, O_RDWR);
-	if (fd < 0) {
-		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
-		goto error;
-	}
-
-	if (rte_intr_fd_set(dev->intr_handle, fd))
-		goto error;
-
 	snprintf(cfgname, sizeof(cfgname),
 			"/sys/class/uio/uio%u/device/config", uio_num);
 
@@ -273,6 +261,19 @@  pci_uio_alloc_resource(struct rte_pci_device *dev,
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	/* the uio_pci_generic driver clears the bus master enable bit when the device file is
+	 * closed, so open it only in the primary process */
+	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
+	/* save fd */
+	fd = open(devname, O_RDWR);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
+		goto error;
+	}
+
+	if (rte_intr_fd_set(dev->intr_handle, fd))
+		goto error;
+
 	/* allocate the mapping details for secondary processes*/
 	*uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
 	if (*uio_res == NULL) {