[dpdk-stable] [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[dpdk-stable] [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Zhiyong Yang]

In the function rte_pci_ioport_map, if uio_pci_generic is used on X86
platform, pci_ioport_map() is invoked, the operation
ev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; is execused directly,
it causes the wrong assignment for uio_pci_generic, the patch fixes it.

Fixes: 756ce64b1ecd ("eal: introduce PCI ioport API")
Cc: stable@dpdk.org
Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
 drivers/bus/pci/linux/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 5da6728fb..8ff7dbfd7 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -723,7 +723,9 @@ pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
 	if (!found)
 		return -1;
 
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
+	if (dev->kdrv == RTE_KDRV_NONE)
+		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
+
 	p->base = start;
 	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
 
-- 
2.13.3

Re: [dpdk-stable] [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Thomas Monjalon]

28/12/2017 07:12, Zhiyong Yang:
> In the function rte_pci_ioport_map, if uio_pci_generic is used on X86
> platform, pci_ioport_map() is invoked, the operation
> ev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; is execused directly,
> it causes the wrong assignment for uio_pci_generic, the patch fixes it.
[...]
> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -723,7 +723,9 @@ pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
>  	if (!found)
>  		return -1;
>  
> -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> +	if (dev->kdrv == RTE_KDRV_NONE)
> +		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;

I don't understand the logic.
NONE is different of UNKNOWN.

Your are talking about uio_pci_generic. In this case, it should be
RTE_KDRV_UIO_GENERIC.

Re: [dpdk-stable] [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Yang, Zhiyong]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, December 28, 2017 5:05 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic
> 
> 28/12/2017 07:12, Zhiyong Yang:
> > In the function rte_pci_ioport_map, if uio_pci_generic is used on X86
> > platform, pci_ioport_map() is invoked, the operation
> > ev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; is execused directly,
> > it causes the wrong assignment for uio_pci_generic, the patch fixes it.
> [...]
> > --- a/drivers/bus/pci/linux/pci.c
> > +++ b/drivers/bus/pci/linux/pci.c
> > @@ -723,7 +723,9 @@ pci_ioport_map(struct rte_pci_device *dev, int bar
> __rte_unused,
> >  	if (!found)
> >  		return -1;
> >
> > -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> > +	if (dev->kdrv == RTE_KDRV_NONE)
> > +		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> 
> I don't understand the logic.
> NONE is different of UNKNOWN.
> 
> Your are talking about uio_pci_generic. In this case, it should be
> RTE_KDRV_UIO_GENERIC.

If we use uio_pci_generic,  dev->intr_handle.type has already been assigned to
The right value RTE_INTR_HANDLE_UIO_INTX before it, but in the function pci_ioport_map
the wrong value is assigned to dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
Two cases both call pci_ioport_map  on x86 platform.
one is RTE_KDRV_UIO_GENERIC
the other is RTE_KDRV_NONE 
if I understand right, for uio_generic, it should not be assigned to RTE_INTR_HANDLE_UNKNOWN;
This case has already the right value, don't need to assign again. 
The original code should be considered to handle RTE_KDRV_NONE case only.

thanks
Zhiyong

Re: [dpdk-stable] [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Thomas Monjalon]

28/12/2017 10:37, Yang, Zhiyong:
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, December 28, 2017 5:05 PM
> > To: Yang, Zhiyong <zhiyong.yang@intel.com>
> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
> > Subject: Re: [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic
> > 
> > 28/12/2017 07:12, Zhiyong Yang:
> > > In the function rte_pci_ioport_map, if uio_pci_generic is used on X86
> > > platform, pci_ioport_map() is invoked, the operation
> > > ev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; is execused directly,
> > > it causes the wrong assignment for uio_pci_generic, the patch fixes it.
> > [...]
> > > --- a/drivers/bus/pci/linux/pci.c
> > > +++ b/drivers/bus/pci/linux/pci.c
> > > @@ -723,7 +723,9 @@ pci_ioport_map(struct rte_pci_device *dev, int bar
> > __rte_unused,
> > >  	if (!found)
> > >  		return -1;
> > >
> > > -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> > > +	if (dev->kdrv == RTE_KDRV_NONE)
> > > +		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> > 
> > I don't understand the logic.
> > NONE is different of UNKNOWN.
> > 
> > Your are talking about uio_pci_generic. In this case, it should be
> > RTE_KDRV_UIO_GENERIC.
> 
> If we use uio_pci_generic,  dev->intr_handle.type has already been assigned to
> The right value RTE_INTR_HANDLE_UIO_INTX before it, but in the function pci_ioport_map
> the wrong value is assigned to dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> Two cases both call pci_ioport_map  on x86 platform.
> one is RTE_KDRV_UIO_GENERIC
> the other is RTE_KDRV_NONE 
> if I understand right, for uio_generic, it should not be assigned to RTE_INTR_HANDLE_UNKNOWN;
> This case has already the right value, don't need to assign again. 
> The original code should be considered to handle RTE_KDRV_NONE case only.

OK thanks for the explanation.
I had overlooked your patch.

Please, could add some of this text in v2?
I think it is good to explain that the right value is already set,
and we must avoid overwriting the default "UNKNOWN" value.

One more question, why is it needed to set the value to "UNKNOWN" in this
function? It should already be set to this value (0) at init.

Re: [dpdk-stable] [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Yang, Zhiyong]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, December 28, 2017 6:49 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH] bus/pci: fix wrong intr_handle.type with uio_pci_generic
> 
> 28/12/2017 10:37, Yang, Zhiyong:
> > Hi Thomas,
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Thursday, December 28, 2017 5:05 PM
> > > To: Yang, Zhiyong <zhiyong.yang@intel.com>
> > > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > stable@dpdk.org
> > > Subject: Re: [PATCH] bus/pci: fix wrong intr_handle.type with
> > > uio_pci_generic
> > >
> > > 28/12/2017 07:12, Zhiyong Yang:
> > > > In the function rte_pci_ioport_map, if uio_pci_generic is used on
> > > > X86 platform, pci_ioport_map() is invoked, the operation
> > > > ev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; is execused
> > > > ev->directly,
> > > > it causes the wrong assignment for uio_pci_generic, the patch fixes it.
> > > [...]
> > > > --- a/drivers/bus/pci/linux/pci.c
> > > > +++ b/drivers/bus/pci/linux/pci.c
> > > > @@ -723,7 +723,9 @@ pci_ioport_map(struct rte_pci_device *dev, int
> > > > bar
> > > __rte_unused,
> > > >  	if (!found)
> > > >  		return -1;
> > > >
> > > > -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> > > > +	if (dev->kdrv == RTE_KDRV_NONE)
> > > > +		dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> > >
> > > I don't understand the logic.
> > > NONE is different of UNKNOWN.
> > >
> > > Your are talking about uio_pci_generic. In this case, it should be
> > > RTE_KDRV_UIO_GENERIC.
> >
> > If we use uio_pci_generic,  dev->intr_handle.type has already been
> > assigned to The right value RTE_INTR_HANDLE_UIO_INTX before it, but in
> > the function pci_ioport_map the wrong value is assigned to
> > dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN; Two cases both call
> pci_ioport_map  on x86 platform.
> > one is RTE_KDRV_UIO_GENERIC
> > the other is RTE_KDRV_NONE
> > if I understand right, for uio_generic, it should not be assigned to
> > RTE_INTR_HANDLE_UNKNOWN; This case has already the right value, don't
> need to assign again.
> > The original code should be considered to handle RTE_KDRV_NONE case only.
> 
> OK thanks for the explanation.
> I had overlooked your patch.
> 
> Please, could add some of this text in v2?
> I think it is good to explain that the right value is already set, and we must avoid
> overwriting the default "UNKNOWN" value.

Ok.  add the text in V2 later.

> 
> One more question, why is it needed to set the value to "UNKNOWN" in this
> function? It should already be set to this value (0) at init.

Good suggestion, we should remove it to fix issue . Any different opinion?  Everyone.

BTW,  The bug can be seen if run the following test case.

Testpmd startup fails  using uio_pci_generic running on virtio legacy device.

Thanks
Zhiyong

[dpdk-stable] [PATCH v2] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Zhiyong Yang]

For virtio legacy device, testpmd startup fails when using
uio_pci_generic. The issue is caused by invoking the function
pci_ioport_map. The right intr_handle.type is already set before
calling it, we should avoid overwriting the default value "RTE_
INTR_HANDLE_UNKNOWN" in it. Besides, the removal has no harm to
other cases since it already is set to this value (0) at init.

Fixes: 756ce64b1ecd ("eal: introduce PCI ioport API")
Cc: stable@dpdk.org
Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---

Changes in V2:
1. reword the commit log.
2. remove the assignment operation which causes the issue.

 drivers/bus/pci/linux/pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 5da6728fb..ec31216bc 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -723,7 +723,6 @@ pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
 	if (!found)
 		return -1;
 
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 	p->base = start;
 	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
 
-- 
2.13.3

Re: [dpdk-stable] [PATCH v2] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Thomas Monjalon]

29/12/2017 08:55, Zhiyong Yang:
> For virtio legacy device, testpmd startup fails when using
> uio_pci_generic. The issue is caused by invoking the function
> pci_ioport_map. The right intr_handle.type is already set before
> calling it, we should avoid overwriting the default value "RTE_
> INTR_HANDLE_UNKNOWN" in it. Besides, the removal has no harm to
> other cases since it already is set to this value (0) at init.

To be more precise, it is set to 0 by a memset on the whole struct
during allocation in the scan function (pci_scan_one).

> --- a/drivers/bus/pci/linux/pci.c
> +++ b/drivers/bus/pci/linux/pci.c
> @@ -723,7 +723,6 @@ pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
>  	if (!found)
>  		return -1;
>  
> -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;

There is the same assignment in pci_vfio_map_resource_primary(),
pci_vfio_map_resource_secondary() and pci_uio_map_resource().

Please could you check why there is such assignments?

Re: [dpdk-stable] [PATCH v2] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Yang, Zhiyong]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, December 29, 2017 7:07 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] bus/pci: fix wrong intr_handle.type with uio_pci_generic
> 
> 29/12/2017 08:55, Zhiyong Yang:
> > For virtio legacy device, testpmd startup fails when using
> > uio_pci_generic. The issue is caused by invoking the function
> > pci_ioport_map. The right intr_handle.type is already set before
> > calling it, we should avoid overwriting the default value "RTE_
> > INTR_HANDLE_UNKNOWN" in it. Besides, the removal has no harm to other
> > cases since it already is set to this value (0) at init.
> 
> To be more precise, it is set to 0 by a memset on the whole struct during
> allocation in the scan function (pci_scan_one).

Ok

> 
> > --- a/drivers/bus/pci/linux/pci.c
> > +++ b/drivers/bus/pci/linux/pci.c
> > @@ -723,7 +723,6 @@ pci_ioport_map(struct rte_pci_device *dev, int bar
> __rte_unused,
> >  	if (!found)
> >  		return -1;
> >
> > -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> 
> There is the same assignment in pci_vfio_map_resource_primary(),
> pci_vfio_map_resource_secondary() and pci_uio_map_resource().
> 
> Please could you check why there is such assignments?

Well, Let me check them later.

Thanks
Zhiyong

Re: [dpdk-stable] [PATCH v2] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Yang, Zhiyong]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, December 29, 2017 7:07 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] bus/pci: fix wrong intr_handle.type with uio_pci_generic
> 
> 29/12/2017 08:55, Zhiyong Yang:
> > For virtio legacy device, testpmd startup fails when using
> > uio_pci_generic. The issue is caused by invoking the function
> > pci_ioport_map. The right intr_handle.type is already set before
> > calling it, we should avoid overwriting the default value "RTE_
> > INTR_HANDLE_UNKNOWN" in it. Besides, the removal has no harm to other
> > cases since it already is set to this value (0) at init.
> 
> To be more precise, it is set to 0 by a memset on the whole struct during
> allocation in the scan function (pci_scan_one).
> 
> > --- a/drivers/bus/pci/linux/pci.c
> > +++ b/drivers/bus/pci/linux/pci.c
> > @@ -723,7 +723,6 @@ pci_ioport_map(struct rte_pci_device *dev, int bar
> __rte_unused,
> >  	if (!found)
> >  		return -1;
> >
> > -	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> 
> There is the same assignment in pci_vfio_map_resource_primary(),
> pci_vfio_map_resource_secondary() and pci_uio_map_resource().
> 
> Please could you check why there is such assignments?

In general, the operation in the three functions intends to initialize the "intr_handle.type",
For example,
For pci_uio_map_resource(),  it wants to get "unknown" status once the code returns abnormally after initializing.
If the code goes smoothly,  dev->intr_handle.type must be assigned to "RTE_INTR_HANDLE_UIO"  for bsd environment,
Or must be assigned to "RTE_INTR_HANDLE_UIO" or " RTE_INTR_HANDLE_UIO_INTX" for linux environment
In consideration of the "memset" in pci_scan_one, it can be removed to has no harm to the existing logic. 
Of course, keeping it is ok.

pci_vfio_map_resource_primary() and pci_vfio_map_resource_secondary() are similar.

The author was emphasizing that  intr_handle.type should be initialized (0) and can be assigned to a right value after it.
Once fails, we can read a status "unknown". I guess.

Turn back  to the patch, it is crude to assign "unknown" directly since  pci_ioport_map is not only used by real "unknown"
But also is used to handle uio_pci_generic driver on X86 platform. It is a special case to cause error for uio_pci_generic. 

Thanks
Zhiyong 

[dpdk-stable] [PATCH v3] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Zhiyong Yang]

For virtio legacy device, testpmd startup fails when using uio_pci_generic.

The issue is caused by invoking the function pci_ioport_map. The correct
value of intr_handle.type is already set before calling it, we should avoid
overwriting the default value "RTE_INTR_HANDLE_UNKNOWN" in this function.
Besides, the removal has no harm to other cases because it is set to 0 by a
memset on the whole struct during allocation in the function pci_scan_one.

Such assignments are removed in the meanwhile in pci_uio_map_resource(),
pci_vfio_map_resource_primary() and pci_vfio_map_resource_secondary() in
order to keep consistencies and avoid future questions.

Fixes: 756ce64b1ecd ("eal: introduce PCI ioport API")
Cc: stable@dpdk.org
Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---

Changes in V3:
1. Such assignments are removed in order to keep consistencies and avoid
future questions.

Changes in V2:
1. Reword the commit log.
2. Remove the assignment operation which causes the issue.

 drivers/bus/pci/linux/pci.c      | 1 -
 drivers/bus/pci/linux/pci_vfio.c | 2 --
 drivers/bus/pci/pci_common_uio.c | 1 -
 3 files changed, 4 deletions(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 25f907e04..3e97aa48e 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -694,7 +694,6 @@ pci_ioport_map(struct rte_pci_device *dev, int bar __rte_unused,
 	if (!found)
 		return -1;
 
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 	p->base = start;
 	RTE_LOG(DEBUG, EAL, "PCI Port IO found start=0x%x\n", start);
 
diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index be986cbdb..aeeaa9ed8 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -430,7 +430,6 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -547,7 +546,6 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index dd84ec8bf..54bc20b59 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -90,7 +90,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
 
 	dev->intr_handle.fd = -1;
 	dev->intr_handle.uio_cfg_fd = -1;
-	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* secondary processes - use already recorded details */
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-- 
2.13.3

Re: [dpdk-stable] [dpdk-dev] [PATCH v3] bus/pci: fix wrong intr_handle.type with uio_pci_generic

[Thomas Monjalon]

10/01/2018 03:32, Zhiyong Yang:
> For virtio legacy device, testpmd startup fails when using uio_pci_generic.
> 
> The issue is caused by invoking the function pci_ioport_map. The correct
> value of intr_handle.type is already set before calling it, we should avoid
> overwriting the default value "RTE_INTR_HANDLE_UNKNOWN" in this function.
> Besides, the removal has no harm to other cases because it is set to 0 by a
> memset on the whole struct during allocation in the function pci_scan_one.
> 
> Such assignments are removed in the meanwhile in pci_uio_map_resource(),
> pci_vfio_map_resource_primary() and pci_vfio_map_resource_secondary() in
> order to keep consistencies and avoid future questions.
> 
> Fixes: 756ce64b1ecd ("eal: introduce PCI ioport API")
> Cc: stable@dpdk.org
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>

Reviewed-by: Thomas Monjalon <thomas@monjalon.net>

Applied, thanks