diff mbox

[dpdk-dev,v2,4/4] virtio: check if any kernel driver is manipulating the virtio device

Message ID 1451843773-103006-5-git-send-email-huawei.xie@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Huawei Xie Jan. 3, 2016, 5:56 p.m. UTC 
  v2 changes:
 change LOG level from ERR to INFO

virtio PMD could use IO port to configure the virtio device without
using uio driver.

There are two issues with previous implementation:
1) virtio PMD will take over each virtio device blindly even if some
are not intended for DPDK.
2) driver conflict between virtio PMD and virtio-net kernel driver.

This patch checks if there is any kernel driver manipulating the virtio
device before virtio PMD uses IO port to configure the device.

Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Stephen Hemminger Jan. 4, 2016, 5:24 p.m. UTC | #1 
On Mon,  4 Jan 2016 01:56:13 +0800
Huawei Xie <huawei.xie@intel.com> wrote:

> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(INFO,
> +			"kernel driver is manipulating this device." \
> +			" Please unbind the kernel driver.");

Splitting strings in general is a bad idea since it makes it harder to find log messages.
Also the first clause is lower case and the second is captialized.

Lastly, the backslash continuation is unnecessary here and will cause checkpatch warning.
Huawei Xie Jan. 4, 2016, 5:56 p.m. UTC | #2 
On 1/5/2016 1:24 AM, Stephen Hemminger wrote:
> On Mon,  4 Jan 2016 01:56:13 +0800
> Huawei Xie <huawei.xie@intel.com> wrote:
>
>> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
>> +		PMD_INIT_LOG(INFO,
>> +			"kernel driver is manipulating this device." \
>> +			" Please unbind the kernel driver.");
> Splitting strings in general is a bad idea since it makes it harder to find log messages.
> Also the first clause is lower case and the second is captialized.
Got it. This is to avoid 80 char warning. Will put it in one line to
make it friendly for searching.
The first clause is lower is because it actually follows "%s():".
>
> Lastly, the backslash continuation is unnecessary here and will cause checkpatch warning.
>
Yuanhan Liu Jan. 5, 2016, 1:56 a.m. UTC | #3 
On Mon, Jan 04, 2016 at 05:56:49PM +0000, Xie, Huawei wrote:
> On 1/5/2016 1:24 AM, Stephen Hemminger wrote:
> > On Mon,  4 Jan 2016 01:56:13 +0800
> > Huawei Xie <huawei.xie@intel.com> wrote:
> >
> >> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> >> +		PMD_INIT_LOG(INFO,
> >> +			"kernel driver is manipulating this device." \
> >> +			" Please unbind the kernel driver.");
> > Splitting strings in general is a bad idea since it makes it harder to find log messages.
> > Also the first clause is lower case and the second is captialized.
> Got it. This is to avoid 80 char warning. Will put it in one line to
> make it friendly for searching.

I agree with Stephen that _in general_ it's a bad idea. But for this
case, I think it's okay, as it'd be enough to locate the code by
searching "manipulating this device", or "unbind the kernel driver",
or other combinations. I mean, nobody would try searching with:

  "kernel driver is manipulating this device. Please unbind the kernel driver."

Right?

	--yliu

> The first clause is lower is because it actually follows "%s():".
> >
> > Lastly, the backslash continuation is unnecessary here and will cause checkpatch warning.
> >
>
Panu Matilainen Jan. 7, 2016, 2:17 p.m. UTC | #4 
On 01/03/2016 07:56 PM, Huawei Xie wrote:
> v2 changes:
>   change LOG level from ERR to INFO
>
> virtio PMD could use IO port to configure the virtio device without
> using uio driver.
>
> There are two issues with previous implementation:
> 1) virtio PMD will take over each virtio device blindly even if some
> are not intended for DPDK.
> 2) driver conflict between virtio PMD and virtio-net kernel driver.
>
> This patch checks if there is any kernel driver manipulating the virtio
> device before virtio PMD uses IO port to configure the device.
>
> Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e815acd..7a50dac 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
>   	int found = 0;
>   	size_t linesz;
>
> +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> +		PMD_INIT_LOG(INFO,
> +			"kernel driver is manipulating this device." \
> +			" Please unbind the kernel driver.");

At the very least this message needs to be changed.

Like said earlier, I think the message could just as well be dropped 
entirely, but at least it should be something to the tune of "ignoring 
kernel owned device" instead of asking the user to break their 
configuration.

	- Panu -
Thomas Monjalon Jan. 27, 2016, 12:43 p.m. UTC | #5 
2016-01-07 16:17, Panu Matilainen:
> On 01/03/2016 07:56 PM, Huawei Xie wrote:
> > v2 changes:
> >   change LOG level from ERR to INFO
> >
> > virtio PMD could use IO port to configure the virtio device without
> > using uio driver.
> >
> > There are two issues with previous implementation:
> > 1) virtio PMD will take over each virtio device blindly even if some
> > are not intended for DPDK.
> > 2) driver conflict between virtio PMD and virtio-net kernel driver.
> >
> > This patch checks if there is any kernel driver manipulating the virtio
> > device before virtio PMD uses IO port to configure the device.
> >
> > Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")
> >
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > ---
> >   drivers/net/virtio/virtio_ethdev.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index e815acd..7a50dac 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1138,6 +1138,13 @@ static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
> >   	int found = 0;
> >   	size_t linesz;
> >
> > +	if (pci_dev->kdrv != RTE_KDRV_NONE) {
> > +		PMD_INIT_LOG(INFO,
> > +			"kernel driver is manipulating this device." \
> > +			" Please unbind the kernel driver.");
> 
> At the very least this message needs to be changed.
> 
> Like said earlier, I think the message could just as well be dropped 
> entirely, but at least it should be something to the tune of "ignoring 
> kernel owned device" instead of asking the user to break their 
> configuration.

Huawei, a v3 is required. Thanks
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e815acd..7a50dac 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1138,6 +1138,13 @@  static int virtio_resource_init_by_ioports(struct rte_pci_device *pci_dev)
 	int found = 0;
 	size_t linesz;
 
+	if (pci_dev->kdrv != RTE_KDRV_NONE) {
+		PMD_INIT_LOG(INFO,
+			"kernel driver is manipulating this device." \
+			" Please unbind the kernel driver.");
+		return -1;
+	}
+
 	snprintf(pci_id, sizeof(pci_id), PCI_PRI_FMT,
 		 pci_dev->addr.domain,
 		 pci_dev->addr.bus,