DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
@ 2017-07-17 23:05 Charles (Chas) Williams
  2017-07-17 23:05 ` [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines Charles (Chas) Williams
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Charles (Chas) Williams @ 2017-07-17 23:05 UTC (permalink / raw)
  To: dev; +Cc: yliu, maxime.coquelin

Just a couple small changes to net/virtio that make it a little more
well behaved.

Brian Russell (1):
  net/virtio: register/unregister intr handler on start/stop

Charles (Chas) Williams (1):
  net/virtio: do not re-enter clean up routines

 drivers/net/virtio/virtio_ethdev.c | 41 +++++++++++++++++++++++++-------------
 drivers/net/virtio/virtio_pci.h    |  4 +++-
 2 files changed, 30 insertions(+), 15 deletions(-)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2017-07-17 23:05 [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Charles (Chas) Williams
@ 2017-07-17 23:05 ` Charles (Chas) Williams
  2018-11-01 14:45   ` Luca Boccassi
  2017-07-17 23:05 ` [dpdk-dev] [PATCH 2/2] net/virtio: register/unregister intr handler on start/stop Charles (Chas) Williams
  2017-07-18 11:50 ` [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Ferruh Yigit
  2 siblings, 1 reply; 17+ messages in thread
From: Charles (Chas) Williams @ 2017-07-17 23:05 UTC (permalink / raw)
  To: dev; +Cc: yliu, maxime.coquelin, Charles (Chas) Williams

.dev_uninit calls .dev_stop and .dev_close.  The work that is done in
those routines doesn't need repeated.  Use started and opened to track
the adapter's status.

Signed-off-by: Chas Williams <ciwillia@brocade.com>
---
 drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++---
 drivers/net/virtio/virtio_pci.h    |  4 +++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 00a3122..eff0545 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -609,6 +609,10 @@ virtio_dev_close(struct rte_eth_dev *dev)
 
 	PMD_INIT_LOG(DEBUG, "virtio_dev_close");
 
+	if (!hw->opened)
+		return;
+	hw->opened = false;
+
 	/* reset the NIC */
 	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		VTPCI_OPS(hw)->set_config_irq(hw, VIRTIO_MSI_NO_VECTOR);
@@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			return -EBUSY;
 		}
 
+	hw->opened = true;
+
 	return 0;
 }
 
@@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
 		VIRTQUEUE_DUMP(txvq->vq);
 	}
 
-	hw->started = 1;
+	hw->started = true;
 
 	/* Initialize Link state */
 	virtio_dev_link_update(dev, 0);
@@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_LOG(DEBUG, "stop");
 
+	if (!hw->started)
+		return;
+	hw->started = false;
+
 	if (intr_conf->lsc || intr_conf->rxq)
 		rte_intr_disable(dev->intr_handle);
 
-	hw->started = 0;
 	memset(&link, 0, sizeof(link));
 	virtio_dev_atomic_write_link_status(dev, &link);
 }
@@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	link.link_speed  = SPEED_10G;
 
-	if (hw->started == 0) {
+	if (!hw->started) {
 		link.link_status = ETH_LINK_DOWN;
 	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
 		PMD_INIT_LOG(DEBUG, "Get link status from hw");
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 18caebd..65bad2d 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -35,6 +35,7 @@
 #define _VIRTIO_PCI_H_
 
 #include <stdint.h>
+#include <stdbool.h>
 
 #include <rte_pci.h>
 #include <rte_ethdev.h>
@@ -253,7 +254,7 @@ struct virtio_hw {
 	uint64_t    req_guest_features;
 	uint64_t    guest_features;
 	uint32_t    max_queue_pairs;
-	uint16_t    started;
+	bool        started;
 	uint16_t	max_mtu;
 	uint16_t    vtnet_hdr_size;
 	uint8_t	    vlan_strip;
@@ -268,6 +269,7 @@ struct virtio_hw {
 	struct virtio_pci_common_cfg *common_cfg;
 	struct virtio_net_config *dev_cfg;
 	void	    *virtio_user_dev;
+	bool        opened;
 
 	struct virtqueue **vqs;
 };
-- 
2.1.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH 2/2] net/virtio: register/unregister intr handler on start/stop
  2017-07-17 23:05 [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Charles (Chas) Williams
  2017-07-17 23:05 ` [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines Charles (Chas) Williams
@ 2017-07-17 23:05 ` Charles (Chas) Williams
  2017-07-18 11:50 ` [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Ferruh Yigit
  2 siblings, 0 replies; 17+ messages in thread
From: Charles (Chas) Williams @ 2017-07-17 23:05 UTC (permalink / raw)
  To: dev; +Cc: yliu, maxime.coquelin, Brian Russell

From: Brian Russell <brussell@brocade.com>

Register and unregister the virtio interrupt handler when the device is
started and stopped.

Signed-off-by: Brian Russell <brussell@brocade.com>
---
 drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index eff0545..103e778 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1581,11 +1581,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	if (ret < 0)
 		return ret;
 
-	/* Setup interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_register(eth_dev->intr_handle,
-			virtio_interrupt_handler, eth_dev);
-
 	return 0;
 }
 
@@ -1607,11 +1602,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
 
-	/* reset interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_unregister(eth_dev->intr_handle,
-						virtio_interrupt_handler,
-						eth_dev);
 	if (eth_dev->device)
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
 
@@ -1730,6 +1720,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	    dev->data->dev_conf.intr_conf.rxq) {
 		rte_intr_disable(dev->intr_handle);
 
+		/* Setup interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
+			rte_intr_callback_register(dev->intr_handle,
+						   virtio_interrupt_handler,
+						   dev);
+
 		if (rte_intr_enable(dev->intr_handle) < 0) {
 			PMD_DRV_LOG(ERR, "interrupt enable failed");
 			return -EIO;
@@ -1834,9 +1830,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 		return;
 	hw->started = false;
 
-	if (intr_conf->lsc || intr_conf->rxq)
+	if (intr_conf->lsc || intr_conf->rxq) {
 		rte_intr_disable(dev->intr_handle);
 
+		/* Reset interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
+			rte_intr_callback_unregister(dev->intr_handle,
+						     virtio_interrupt_handler,
+						     dev);
+		}
+	}
+
 	memset(&link, 0, sizeof(link));
 	virtio_dev_atomic_write_link_status(dev, &link);
 }
-- 
2.1.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
  2017-07-17 23:05 [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Charles (Chas) Williams
  2017-07-17 23:05 ` [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines Charles (Chas) Williams
  2017-07-17 23:05 ` [dpdk-dev] [PATCH 2/2] net/virtio: register/unregister intr handler on start/stop Charles (Chas) Williams
@ 2017-07-18 11:50 ` Ferruh Yigit
  2017-07-18 11:52   ` Charles (Chas) Williams
  2 siblings, 1 reply; 17+ messages in thread
From: Ferruh Yigit @ 2017-07-18 11:50 UTC (permalink / raw)
  To: Charles (Chas) Williams, dev; +Cc: yliu, maxime.coquelin

On 7/18/2017 12:05 AM, Charles (Chas) Williams wrote:
> Just a couple small changes to net/virtio that make it a little more
> well behaved.

Same question here, is this patchset targets the 17.08-rc2? Can this be
postponed?

> 
> Brian Russell (1):
>   net/virtio: register/unregister intr handler on start/stop
> 
> Charles (Chas) Williams (1):
>   net/virtio: do not re-enter clean up routines
> 
>  drivers/net/virtio/virtio_ethdev.c | 41 +++++++++++++++++++++++++-------------
>  drivers/net/virtio/virtio_pci.h    |  4 +++-
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
  2017-07-18 11:50 ` [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Ferruh Yigit
@ 2017-07-18 11:52   ` Charles (Chas) Williams
  2018-08-15 13:51     ` Luca Boccassi
  0 siblings, 1 reply; 17+ messages in thread
From: Charles (Chas) Williams @ 2017-07-18 11:52 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: yliu, maxime.coquelin



On 07/18/2017 07:50 AM, Ferruh Yigit wrote:
> On 7/18/2017 12:05 AM, Charles (Chas) Williams wrote:
>> Just a couple small changes to net/virtio that make it a little more
>> well behaved.
>
> Same question here, is this patchset targets the 17.08-rc2? Can this be
> postponed?

Yes, these can all be postponed to 17.11.  They are too late for a -rc2.

>
>>
>> Brian Russell (1):
>>   net/virtio: register/unregister intr handler on start/stop
>>
>> Charles (Chas) Williams (1):
>>   net/virtio: do not re-enter clean up routines
>>
>>  drivers/net/virtio/virtio_ethdev.c | 41 +++++++++++++++++++++++++-------------
>>  drivers/net/virtio/virtio_pci.h    |  4 +++-
>>  2 files changed, 30 insertions(+), 15 deletions(-)
>>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
  2017-07-18 11:52   ` Charles (Chas) Williams
@ 2018-08-15 13:51     ` Luca Boccassi
  2018-08-27 13:41       ` Ferruh Yigit
  0 siblings, 1 reply; 17+ messages in thread
From: Luca Boccassi @ 2018-08-15 13:51 UTC (permalink / raw)
  To: maxime.coquelin, Ferruh Yigit, dev; +Cc: 3chas3

On Tue, 2017-07-18 at 07:52 -0400, Charles (Chas) Williams wrote:
> 
> On 07/18/2017 07:50 AM, Ferruh Yigit wrote:
> > On 7/18/2017 12:05 AM, Charles (Chas) Williams wrote:
> > > Just a couple small changes to net/virtio that make it a little
> > > more
> > > well behaved.
> > 
> > Same question here, is this patchset targets the 17.08-rc2? Can
> > this be
> > postponed?
> 
> Yes, these can all be postponed to 17.11.  They are too late for a
> -rc2.

Hi Ferruh and Maxime,

Looks like this series fell through the cracks. Any chance you folks
could please have a look at it for 18.11? Thanks!

https://patches.dpdk.org/patch/26994/
https://patches.dpdk.org/patch/26995/

-- 
Kind regards,
Luca Boccassi

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
  2018-08-15 13:51     ` Luca Boccassi
@ 2018-08-27 13:41       ` Ferruh Yigit
  2018-08-27 13:48         ` Ferruh Yigit
  0 siblings, 1 reply; 17+ messages in thread
From: Ferruh Yigit @ 2018-08-27 13:41 UTC (permalink / raw)
  To: maxime.coquelin, Tiwei Bie; +Cc: Luca Boccassi, dev, 3chas3

On 8/15/2018 2:51 PM, Luca Boccassi wrote:
> On Tue, 2017-07-18 at 07:52 -0400, Charles (Chas) Williams wrote:
>>
>> On 07/18/2017 07:50 AM, Ferruh Yigit wrote:
>>> On 7/18/2017 12:05 AM, Charles (Chas) Williams wrote:
>>>> Just a couple small changes to net/virtio that make it a little
>>>> more
>>>> well behaved.
>>>
>>> Same question here, is this patchset targets the 17.08-rc2? Can
>>> this be
>>> postponed?
>>
>> Yes, these can all be postponed to 17.11.  They are too late for a
>> -rc2.
> 
> Hi Ferruh and Maxime,
> 
> Looks like this series fell through the cracks. Any chance you folks
> could please have a look at it for 18.11? Thanks!
> 
> https://patches.dpdk.org/patch/26994/
> https://patches.dpdk.org/patch/26995/

Hi Maxime, Tiwei,

Those patches seems missed for a while, they date back to July 2011. Can you
guys please look at it?

If there is no objection/comment for a little more, they will be automerged as a
part of process.

Thanks,
ferruh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
  2018-08-27 13:41       ` Ferruh Yigit
@ 2018-08-27 13:48         ` Ferruh Yigit
  2018-08-27 14:52           ` Gavin Hu
  0 siblings, 1 reply; 17+ messages in thread
From: Ferruh Yigit @ 2018-08-27 13:48 UTC (permalink / raw)
  To: maxime.coquelin, Tiwei Bie; +Cc: Luca Boccassi, dev, 3chas3

On 8/27/2018 2:41 PM, Ferruh Yigit wrote:
> On 8/15/2018 2:51 PM, Luca Boccassi wrote:
>> On Tue, 2017-07-18 at 07:52 -0400, Charles (Chas) Williams wrote:
>>>
>>> On 07/18/2017 07:50 AM, Ferruh Yigit wrote:
>>>> On 7/18/2017 12:05 AM, Charles (Chas) Williams wrote:
>>>>> Just a couple small changes to net/virtio that make it a little
>>>>> more
>>>>> well behaved.
>>>>
>>>> Same question here, is this patchset targets the 17.08-rc2? Can
>>>> this be
>>>> postponed?
>>>
>>> Yes, these can all be postponed to 17.11.  They are too late for a
>>> -rc2.
>>
>> Hi Ferruh and Maxime,
>>
>> Looks like this series fell through the cracks. Any chance you folks
>> could please have a look at it for 18.11? Thanks!
>>
>> https://patches.dpdk.org/patch/26994/
>> https://patches.dpdk.org/patch/26995/
> 
> Hi Maxime, Tiwei,
> 
> Those patches seems missed for a while, they date back to July 2011. Can you
> guys please look at it?

July 2017 :)

> 
> If there is no objection/comment for a little more, they will be automerged as a
> part of process.
> 
> Thanks,
> ferruh
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
  2018-08-27 13:48         ` Ferruh Yigit
@ 2018-08-27 14:52           ` Gavin Hu
  0 siblings, 0 replies; 17+ messages in thread
From: Gavin Hu @ 2018-08-27 14:52 UTC (permalink / raw)
  To: Ferruh Yigit, maxime.coquelin, Tiwei Bie; +Cc: Luca Boccassi, dev, 3chas3

Why not combine" started" and "opened" into "status" with two bits represent each respectively?

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> Sent: Monday, August 27, 2018 9:48 PM
> To: maxime.coquelin@redhat.com; Tiwei Bie <tiwei.bie@intel.com>
> Cc: Luca Boccassi <bluca@debian.org>; dev@dpdk.org; 3chas3@gmail.com
> Subject: Re: [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio
>
> On 8/27/2018 2:41 PM, Ferruh Yigit wrote:
> > On 8/15/2018 2:51 PM, Luca Boccassi wrote:
> >> On Tue, 2017-07-18 at 07:52 -0400, Charles (Chas) Williams wrote:
> >>>
> >>> On 07/18/2017 07:50 AM, Ferruh Yigit wrote:
> >>>> On 7/18/2017 12:05 AM, Charles (Chas) Williams wrote:
> >>>>> Just a couple small changes to net/virtio that make it a little
> >>>>> more well behaved.
> >>>>
> >>>> Same question here, is this patchset targets the 17.08-rc2? Can
> >>>> this be postponed?
> >>>
> >>> Yes, these can all be postponed to 17.11.  They are too late for a
> >>> -rc2.
> >>
> >> Hi Ferruh and Maxime,
> >>
> >> Looks like this series fell through the cracks. Any chance you folks
> >> could please have a look at it for 18.11? Thanks!
> >>
> >> https://patches.dpdk.org/patch/26994/
> >> https://patches.dpdk.org/patch/26995/
> >
> > Hi Maxime, Tiwei,
> >
> > Those patches seems missed for a while, they date back to July 2011.
> > Can you guys please look at it?
>
> July 2017 :)
>
> >
> > If there is no objection/comment for a little more, they will be
> > automerged as a part of process.
> >
> > Thanks,
> > ferruh
> >

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2017-07-17 23:05 ` [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines Charles (Chas) Williams
@ 2018-11-01 14:45   ` Luca Boccassi
  2018-11-02  9:57     ` Maxime Coquelin
  2018-11-02 14:33     ` Ferruh Yigit
  0 siblings, 2 replies; 17+ messages in thread
From: Luca Boccassi @ 2018-11-01 14:45 UTC (permalink / raw)
  To: maxime.coquelin, dev; +Cc: ferruh.yigit, 3chas3

On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
> those routines doesn't need repeated.  Use started and opened to
> track
> the adapter's status.
> 
> Signed-off-by: Chas Williams <ciwillia@brocade.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++---
>  drivers/net/virtio/virtio_pci.h    |  4 +++-
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 00a3122..eff0545 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -609,6 +609,10 @@ virtio_dev_close(struct rte_eth_dev *dev)
>  
>  	PMD_INIT_LOG(DEBUG, "virtio_dev_close");
>  
> +	if (!hw->opened)
> +		return;
> +	hw->opened = false;
> +
>  	/* reset the NIC */
>  	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>  		VTPCI_OPS(hw)->set_config_irq(hw,
> VIRTIO_MSI_NO_VECTOR);
> @@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  			return -EBUSY;
>  		}
>  
> +	hw->opened = true;
> +
>  	return 0;
>  }
>  
> @@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  		VIRTQUEUE_DUMP(txvq->vq);
>  	}
>  
> -	hw->started = 1;
> +	hw->started = true;
>  
>  	/* Initialize Link state */
>  	virtio_dev_link_update(dev, 0);
> @@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>  
>  	PMD_INIT_LOG(DEBUG, "stop");
>  
> +	if (!hw->started)
> +		return;
> +	hw->started = false;
> +
>  	if (intr_conf->lsc || intr_conf->rxq)
>  		rte_intr_disable(dev->intr_handle);
>  
> -	hw->started = 0;
>  	memset(&link, 0, sizeof(link));
>  	virtio_dev_atomic_write_link_status(dev, &link);
>  }
> @@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev,
> __rte_unused int wait_to_complet
>  	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>  	link.link_speed  = SPEED_10G;
>  
> -	if (hw->started == 0) {
> +	if (!hw->started) {
>  		link.link_status = ETH_LINK_DOWN;
>  	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
>  		PMD_INIT_LOG(DEBUG, "Get link status from hw");
> diff --git a/drivers/net/virtio/virtio_pci.h
> b/drivers/net/virtio/virtio_pci.h
> index 18caebd..65bad2d 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -35,6 +35,7 @@
>  #define _VIRTIO_PCI_H_
>  
>  #include <stdint.h>
> +#include <stdbool.h>
>  
>  #include <rte_pci.h>
>  #include <rte_ethdev.h>
> @@ -253,7 +254,7 @@ struct virtio_hw {
>  	uint64_t    req_guest_features;
>  	uint64_t    guest_features;
>  	uint32_t    max_queue_pairs;
> -	uint16_t    started;
> +	bool        started;
>  	uint16_t	max_mtu;
>  	uint16_t    vtnet_hdr_size;
>  	uint8_t	    vlan_strip;
> @@ -268,6 +269,7 @@ struct virtio_hw {
>  	struct virtio_pci_common_cfg *common_cfg;
>  	struct virtio_net_config *dev_cfg;
>  	void	    *virtio_user_dev;
> +	bool        opened;
>  
>  	struct virtqueue **vqs;
>  };

Hi Maxime,

Any chance this could be reviewed and taken care of, please? It's been
forgotten in the queue for a year, but we still use it.
I have sent 2/2 separately so ignore that (I didn't notice this series
was still pending review).

Thanks!

-- 
Kind regards,
Luca Boccassi

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-01 14:45   ` Luca Boccassi
@ 2018-11-02  9:57     ` Maxime Coquelin
  2018-11-02 11:03       ` Maxime Coquelin
  2018-11-02 14:33     ` Ferruh Yigit
  1 sibling, 1 reply; 17+ messages in thread
From: Maxime Coquelin @ 2018-11-02  9:57 UTC (permalink / raw)
  To: Luca Boccassi, dev; +Cc: ferruh.yigit, 3chas3



On 11/1/18 3:45 PM, Luca Boccassi wrote:
> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>> those routines doesn't need repeated.  Use started and opened to
>> track
>> the adapter's status.
>>
>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>> ---
>>   drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++---
>>   drivers/net/virtio/virtio_pci.h    |  4 +++-
>>   2 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index 00a3122..eff0545 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -609,6 +609,10 @@ virtio_dev_close(struct rte_eth_dev *dev)
>>   
>>   	PMD_INIT_LOG(DEBUG, "virtio_dev_close");
>>   
>> +	if (!hw->opened)
>> +		return;
>> +	hw->opened = false;
>> +
>>   	/* reset the NIC */
>>   	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>   		VTPCI_OPS(hw)->set_config_irq(hw,
>> VIRTIO_MSI_NO_VECTOR);
>> @@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>   			return -EBUSY;
>>   		}
>>   
>> +	hw->opened = true;
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
>>   		VIRTQUEUE_DUMP(txvq->vq);
>>   	}
>>   
>> -	hw->started = 1;
>> +	hw->started = true;
>>   
>>   	/* Initialize Link state */
>>   	virtio_dev_link_update(dev, 0);
>> @@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>>   
>>   	PMD_INIT_LOG(DEBUG, "stop");
>>   
>> +	if (!hw->started)
>> +		return;
>> +	hw->started = false;
>> +
>>   	if (intr_conf->lsc || intr_conf->rxq)
>>   		rte_intr_disable(dev->intr_handle);
>>   
>> -	hw->started = 0;
>>   	memset(&link, 0, sizeof(link));
>>   	virtio_dev_atomic_write_link_status(dev, &link);
>>   }
>> @@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev,
>> __rte_unused int wait_to_complet
>>   	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>>   	link.link_speed  = SPEED_10G;
>>   
>> -	if (hw->started == 0) {
>> +	if (!hw->started) {
>>   		link.link_status = ETH_LINK_DOWN;
>>   	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
>>   		PMD_INIT_LOG(DEBUG, "Get link status from hw");
>> diff --git a/drivers/net/virtio/virtio_pci.h
>> b/drivers/net/virtio/virtio_pci.h
>> index 18caebd..65bad2d 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -35,6 +35,7 @@
>>   #define _VIRTIO_PCI_H_
>>   
>>   #include <stdint.h>
>> +#include <stdbool.h>
>>   
>>   #include <rte_pci.h>
>>   #include <rte_ethdev.h>
>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>   	uint64_t    req_guest_features;
>>   	uint64_t    guest_features;
>>   	uint32_t    max_queue_pairs;
>> -	uint16_t    started;
>> +	bool        started;
>>   	uint16_t	max_mtu;
>>   	uint16_t    vtnet_hdr_size;
>>   	uint8_t	    vlan_strip;
>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>   	struct virtio_pci_common_cfg *common_cfg;
>>   	struct virtio_net_config *dev_cfg;
>>   	void	    *virtio_user_dev;
>> +	bool        opened;
>>   
>>   	struct virtqueue **vqs;
>>   };
> 
> Hi Maxime,
> 
> Any chance this could be reviewed and taken care of, please? It's been
> forgotten in the queue for a year, but we still use it.
> I have sent 2/2 separately so ignore that (I didn't notice this series
> was still pending review).
> 
> Thanks!
> 


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I'll certainly have to rebase it, but it should be trivial.
And as discussed on IRC, it fixes a real issue, so will have to be
backported to stable. I'll take care of it.

Regards,
Maxime

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-02  9:57     ` Maxime Coquelin
@ 2018-11-02 11:03       ` Maxime Coquelin
  2018-11-02 11:14         ` Luca Boccassi
  0 siblings, 1 reply; 17+ messages in thread
From: Maxime Coquelin @ 2018-11-02 11:03 UTC (permalink / raw)
  To: Luca Boccassi, dev; +Cc: ferruh.yigit, 3chas3



On 11/2/18 10:57 AM, Maxime Coquelin wrote:
> 
> 
> On 11/1/18 3:45 PM, Luca Boccassi wrote:
>> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>>> those routines doesn't need repeated.  Use started and opened to
>>> track
>>> the adapter's status.
>>>
>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>> ---
>>>   drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++---
>>>   drivers/net/virtio/virtio_pci.h    |  4 +++-
>>>   2 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>>> b/drivers/net/virtio/virtio_ethdev.c
>>> index 00a3122..eff0545 100644
>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>> @@ -609,6 +609,10 @@ virtio_dev_close(struct rte_eth_dev *dev)
>>>       PMD_INIT_LOG(DEBUG, "virtio_dev_close");
>>> +    if (!hw->opened)
>>> +        return;
>>> +    hw->opened = false;
>>> +
>>>       /* reset the NIC */
>>>       if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>>           VTPCI_OPS(hw)->set_config_irq(hw,
>>> VIRTIO_MSI_NO_VECTOR);
>>> @@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>>               return -EBUSY;
>>>           }
>>> +    hw->opened = true;
>>> +
>>>       return 0;
>>>   }
>>> @@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
>>>           VIRTQUEUE_DUMP(txvq->vq);
>>>       }
>>> -    hw->started = 1;
>>> +    hw->started = true;
>>>       /* Initialize Link state */
>>>       virtio_dev_link_update(dev, 0);
>>> @@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>>>       PMD_INIT_LOG(DEBUG, "stop");
>>> +    if (!hw->started)
>>> +        return;
>>> +    hw->started = false;
>>> +
>>>       if (intr_conf->lsc || intr_conf->rxq)
>>>           rte_intr_disable(dev->intr_handle);
>>> -    hw->started = 0;
>>>       memset(&link, 0, sizeof(link));
>>>       virtio_dev_atomic_write_link_status(dev, &link);
>>>   }
>>> @@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev,
>>> __rte_unused int wait_to_complet
>>>       link.link_duplex = ETH_LINK_FULL_DUPLEX;
>>>       link.link_speed  = SPEED_10G;
>>> -    if (hw->started == 0) {
>>> +    if (!hw->started) {
>>>           link.link_status = ETH_LINK_DOWN;
>>>       } else if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
>>>           PMD_INIT_LOG(DEBUG, "Get link status from hw");
>>> diff --git a/drivers/net/virtio/virtio_pci.h
>>> b/drivers/net/virtio/virtio_pci.h
>>> index 18caebd..65bad2d 100644
>>> --- a/drivers/net/virtio/virtio_pci.h
>>> +++ b/drivers/net/virtio/virtio_pci.h
>>> @@ -35,6 +35,7 @@
>>>   #define _VIRTIO_PCI_H_
>>>   #include <stdint.h>
>>> +#include <stdbool.h>
>>>   #include <rte_pci.h>
>>>   #include <rte_ethdev.h>
>>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>>       uint64_t    req_guest_features;
>>>       uint64_t    guest_features;
>>>       uint32_t    max_queue_pairs;
>>> -    uint16_t    started;
>>> +    bool        started;
>>>       uint16_t    max_mtu;
>>>       uint16_t    vtnet_hdr_size;
>>>       uint8_t        vlan_strip;
>>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>>       struct virtio_pci_common_cfg *common_cfg;
>>>       struct virtio_net_config *dev_cfg;
>>>       void        *virtio_user_dev;
>>> +    bool        opened;
>>>       struct virtqueue **vqs;
>>>   };
>>
>> Hi Maxime,
>>
>> Any chance this could be reviewed and taken care of, please? It's been
>> forgotten in the queue for a year, but we still use it.
>> I have sent 2/2 separately so ignore that (I didn't notice this series
>> was still pending review).
>>
>> Thanks!
>>
> 
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I'll certainly have to rebase it, but it should be trivial.
> And as discussed on IRC, it fixes a real issue, so will have to be
> backported to stable. I'll take care of it.
> 
> Regards,
> Maxime

Applied to dpdk-next-virtio/master

If you have some time, please have a look at the branch to ensure the
rebase is correct.

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-02 11:03       ` Maxime Coquelin
@ 2018-11-02 11:14         ` Luca Boccassi
  0 siblings, 0 replies; 17+ messages in thread
From: Luca Boccassi @ 2018-11-02 11:14 UTC (permalink / raw)
  To: Maxime Coquelin, dev; +Cc: ferruh.yigit, 3chas3

On Fri, 2018-11-02 at 12:03 +0100, Maxime Coquelin wrote:
> 
> On 11/2/18 10:57 AM, Maxime Coquelin wrote:
> > 
> > 
> > On 11/1/18 3:45 PM, Luca Boccassi wrote:
> > > On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
> > > > .dev_uninit calls .dev_stop and .dev_close.  The work that is
> > > > done in
> > > > those routines doesn't need repeated.  Use started and opened
> > > > to
> > > > track
> > > > the adapter's status.
> > > > 
> > > > Signed-off-by: Chas Williams <ciwillia@brocade.com>
> > > > ---
> > > >   drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++++---
> > > >   drivers/net/virtio/virtio_pci.h    |  4 +++-
> > > >   2 files changed, 15 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > > > b/drivers/net/virtio/virtio_ethdev.c
> > > > index 00a3122..eff0545 100644
> > > > --- a/drivers/net/virtio/virtio_ethdev.c
> > > > +++ b/drivers/net/virtio/virtio_ethdev.c
> > > > @@ -609,6 +609,10 @@ virtio_dev_close(struct rte_eth_dev *dev)
> > > >       PMD_INIT_LOG(DEBUG, "virtio_dev_close");
> > > > +    if (!hw->opened)
> > > > +        return;
> > > > +    hw->opened = false;
> > > > +
> > > >       /* reset the NIC */
> > > >       if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
> > > >           VTPCI_OPS(hw)->set_config_irq(hw,
> > > > VIRTIO_MSI_NO_VECTOR);
> > > > @@ -1696,6 +1700,8 @@ virtio_dev_configure(struct rte_eth_dev
> > > > *dev)
> > > >               return -EBUSY;
> > > >           }
> > > > +    hw->opened = true;
> > > > +
> > > >       return 0;
> > > >   }
> > > > @@ -1759,7 +1765,7 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > > >           VIRTQUEUE_DUMP(txvq->vq);
> > > >       }
> > > > -    hw->started = 1;
> > > > +    hw->started = true;
> > > >       /* Initialize Link state */
> > > >       virtio_dev_link_update(dev, 0);
> > > > @@ -1824,10 +1830,13 @@ virtio_dev_stop(struct rte_eth_dev
> > > > *dev)
> > > >       PMD_INIT_LOG(DEBUG, "stop");
> > > > +    if (!hw->started)
> > > > +        return;
> > > > +    hw->started = false;
> > > > +
> > > >       if (intr_conf->lsc || intr_conf->rxq)
> > > >           rte_intr_disable(dev->intr_handle);
> > > > -    hw->started = 0;
> > > >       memset(&link, 0, sizeof(link));
> > > >       virtio_dev_atomic_write_link_status(dev, &link);
> > > >   }
> > > > @@ -1844,7 +1853,7 @@ virtio_dev_link_update(struct rte_eth_dev
> > > > *dev,
> > > > __rte_unused int wait_to_complet
> > > >       link.link_duplex = ETH_LINK_FULL_DUPLEX;
> > > >       link.link_speed  = SPEED_10G;
> > > > -    if (hw->started == 0) {
> > > > +    if (!hw->started) {
> > > >           link.link_status = ETH_LINK_DOWN;
> > > >       } else if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
> > > >           PMD_INIT_LOG(DEBUG, "Get link status from hw");
> > > > diff --git a/drivers/net/virtio/virtio_pci.h
> > > > b/drivers/net/virtio/virtio_pci.h
> > > > index 18caebd..65bad2d 100644
> > > > --- a/drivers/net/virtio/virtio_pci.h
> > > > +++ b/drivers/net/virtio/virtio_pci.h
> > > > @@ -35,6 +35,7 @@
> > > >   #define _VIRTIO_PCI_H_
> > > >   #include <stdint.h>
> > > > +#include <stdbool.h>
> > > >   #include <rte_pci.h>
> > > >   #include <rte_ethdev.h>
> > > > @@ -253,7 +254,7 @@ struct virtio_hw {
> > > >       uint64_t    req_guest_features;
> > > >       uint64_t    guest_features;
> > > >       uint32_t    max_queue_pairs;
> > > > -    uint16_t    started;
> > > > +    bool        started;
> > > >       uint16_t    max_mtu;
> > > >       uint16_t    vtnet_hdr_size;
> > > >       uint8_t        vlan_strip;
> > > > @@ -268,6 +269,7 @@ struct virtio_hw {
> > > >       struct virtio_pci_common_cfg *common_cfg;
> > > >       struct virtio_net_config *dev_cfg;
> > > >       void        *virtio_user_dev;
> > > > +    bool        opened;
> > > >       struct virtqueue **vqs;
> > > >   };
> > > 
> > > Hi Maxime,
> > > 
> > > Any chance this could be reviewed and taken care of, please? It's
> > > been
> > > forgotten in the queue for a year, but we still use it.
> > > I have sent 2/2 separately so ignore that (I didn't notice this
> > > series
> > > was still pending review).
> > > 
> > > Thanks!
> > > 
> > 
> > 
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > 
> > I'll certainly have to rebase it, but it should be trivial.
> > And as discussed on IRC, it fixes a real issue, so will have to be
> > backported to stable. I'll take care of it.
> > 
> > Regards,
> > Maxime
> 
> Applied to dpdk-next-virtio/master
> 
> If you have some time, please have a look at the branch to ensure the
> rebase is correct.
> 
> Thanks,
> Maxime

Looks good to me, thank you very much!

-- 
Kind regards,
Luca Boccassi

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-01 14:45   ` Luca Boccassi
  2018-11-02  9:57     ` Maxime Coquelin
@ 2018-11-02 14:33     ` Ferruh Yigit
  2018-11-02 15:19       ` Chas Williams
  1 sibling, 1 reply; 17+ messages in thread
From: Ferruh Yigit @ 2018-11-02 14:33 UTC (permalink / raw)
  To: Luca Boccassi, maxime.coquelin, dev; +Cc: 3chas3

On 11/1/2018 2:45 PM, Luca Boccassi wrote:
> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>> those routines doesn't need repeated.  Use started and opened to
>> track
>> the adapter's status.
>>
>> Signed-off-by: Chas Williams <ciwillia@brocade.com>

<...>

>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>  	uint64_t    req_guest_features;
>>  	uint64_t    guest_features;
>>  	uint32_t    max_queue_pairs;
>> -	uint16_t    started;
>> +	bool        started;
>>  	uint16_t	max_mtu;
>>  	uint16_t    vtnet_hdr_size;
>>  	uint8_t	    vlan_strip;
>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>  	struct virtio_pci_common_cfg *common_cfg;
>>  	struct virtio_net_config *dev_cfg;
>>  	void	    *virtio_user_dev;
>> +	bool        opened;

This is already merged into next-net-virtio, but I would like to hightlight the
checkpatch warning about `bool` usage in struct [1].
Briefly it suggests preferring primitive data types against `bool` in structures
because its size is not clear.

What do you think about it, do you have strong opinion to have them as bool?


[1]
CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#85: FILE: drivers/net/virtio/virtio_pci.h:234:
+       bool        started;

CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#93: FILE: drivers/net/virtio/virtio_pci.h:260:
+       bool        opened;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-02 14:33     ` Ferruh Yigit
@ 2018-11-02 15:19       ` Chas Williams
  2018-11-02 16:48         ` Maxime Coquelin
  0 siblings, 1 reply; 17+ messages in thread
From: Chas Williams @ 2018-11-02 15:19 UTC (permalink / raw)
  To: Ferruh Yigit, Luca Boccassi, maxime.coquelin, dev



On 11/02/2018 10:33 AM, Ferruh Yigit wrote:
> On 11/1/2018 2:45 PM, Luca Boccassi wrote:
>> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>>> those routines doesn't need repeated.  Use started and opened to
>>> track
>>> the adapter's status.
>>>
>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
> 
> <...>
> 
>>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>>   	uint64_t    req_guest_features;
>>>   	uint64_t    guest_features;
>>>   	uint32_t    max_queue_pairs;
>>> -	uint16_t    started;
>>> +	bool        started;
>>>   	uint16_t	max_mtu;
>>>   	uint16_t    vtnet_hdr_size;
>>>   	uint8_t	    vlan_strip;
>>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>>   	struct virtio_pci_common_cfg *common_cfg;
>>>   	struct virtio_net_config *dev_cfg;
>>>   	void	    *virtio_user_dev;
>>> +	bool        opened;
> 
> This is already merged into next-net-virtio, but I would like to hightlight the
> checkpatch warning about `bool` usage in struct [1].
> Briefly it suggests preferring primitive data types against `bool` in structures
> because its size is not clear.
> 
> What do you think about it, do you have strong opinion to have them as bool?

Yes, I suppose I do.  bool is the "proper" representation for yes/no.
The arguments cited aren't convincing.

The size of bool is the size of bool. The compiler gets to make that
decision.  I don't get to decide the size of int either.  The size and
alignemnt of bool should be optimal because your compiler probably knows
more about the target than you do.  If your compiler can't figure out
alignment in a structure, please fix the compiler.

bool is a primitive data type per the C99 standard.

> [1]
> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
> #85: FILE: drivers/net/virtio/virtio_pci.h:234:
> +       bool        started;
> 
> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
> #93: FILE: drivers/net/virtio/virtio_pci.h:260:
> +       bool        opened;
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-02 15:19       ` Chas Williams
@ 2018-11-02 16:48         ` Maxime Coquelin
  2018-11-02 17:17           ` Ferruh Yigit
  0 siblings, 1 reply; 17+ messages in thread
From: Maxime Coquelin @ 2018-11-02 16:48 UTC (permalink / raw)
  To: Chas Williams, Ferruh Yigit, Luca Boccassi, dev



On 11/2/18 4:19 PM, Chas Williams wrote:
> 
> 
> On 11/02/2018 10:33 AM, Ferruh Yigit wrote:
>> On 11/1/2018 2:45 PM, Luca Boccassi wrote:
>>> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>>>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>>>> those routines doesn't need repeated.  Use started and opened to
>>>> track
>>>> the adapter's status.
>>>>
>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>
>> <...>
>>
>>>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>>>       uint64_t    req_guest_features;
>>>>       uint64_t    guest_features;
>>>>       uint32_t    max_queue_pairs;
>>>> -    uint16_t    started;
>>>> +    bool        started;
>>>>       uint16_t    max_mtu;
>>>>       uint16_t    vtnet_hdr_size;
>>>>       uint8_t        vlan_strip;
>>>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>>>       struct virtio_pci_common_cfg *common_cfg;
>>>>       struct virtio_net_config *dev_cfg;
>>>>       void        *virtio_user_dev;
>>>> +    bool        opened;
>>
>> This is already merged into next-net-virtio, but I would like to 
>> hightlight the
>> checkpatch warning about `bool` usage in struct [1].
>> Briefly it suggests preferring primitive data types against `bool` in 
>> structures
>> because its size is not clear.
>>
>> What do you think about it, do you have strong opinion to have them as 
>> bool?
> 
> Yes, I suppose I do.  bool is the "proper" representation for yes/no.
> The arguments cited aren't convincing.
> 
> The size of bool is the size of bool. The compiler gets to make that
> decision.  I don't get to decide the size of int either.  The size and
> alignemnt of bool should be optimal because your compiler probably knows
> more about the target than you do.  If your compiler can't figure out
> alignment in a structure, please fix the compiler.
> 
> bool is a primitive data type per the C99 standard.

I would like to keep it as bool too.

>> [1]
>> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
>> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
>> #85: FILE: drivers/net/virtio/virtio_pci.h:234:
>> +       bool        started;
>>
>> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
>> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
>> #93: FILE: drivers/net/virtio/virtio_pci.h:260:
>> +       bool        opened;
>>

BTW, I don't get this warning when running checkpatch, what kenrel
version is it coming from?

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines
  2018-11-02 16:48         ` Maxime Coquelin
@ 2018-11-02 17:17           ` Ferruh Yigit
  0 siblings, 0 replies; 17+ messages in thread
From: Ferruh Yigit @ 2018-11-02 17:17 UTC (permalink / raw)
  To: Maxime Coquelin, Chas Williams, Luca Boccassi, dev

On 11/2/2018 4:48 PM, Maxime Coquelin wrote:
> 
> 
> On 11/2/18 4:19 PM, Chas Williams wrote:
>>
>>
>> On 11/02/2018 10:33 AM, Ferruh Yigit wrote:
>>> On 11/1/2018 2:45 PM, Luca Boccassi wrote:
>>>> On Mon, 2017-07-17 at 19:05 -0400, Charles (Chas) Williams wrote:
>>>>> .dev_uninit calls .dev_stop and .dev_close.  The work that is done in
>>>>> those routines doesn't need repeated.  Use started and opened to
>>>>> track
>>>>> the adapter's status.
>>>>>
>>>>> Signed-off-by: Chas Williams <ciwillia@brocade.com>
>>>
>>> <...>
>>>
>>>>> @@ -253,7 +254,7 @@ struct virtio_hw {
>>>>>       uint64_t    req_guest_features;
>>>>>       uint64_t    guest_features;
>>>>>       uint32_t    max_queue_pairs;
>>>>> -    uint16_t    started;
>>>>> +    bool        started;
>>>>>       uint16_t    max_mtu;
>>>>>       uint16_t    vtnet_hdr_size;
>>>>>       uint8_t        vlan_strip;
>>>>> @@ -268,6 +269,7 @@ struct virtio_hw {
>>>>>       struct virtio_pci_common_cfg *common_cfg;
>>>>>       struct virtio_net_config *dev_cfg;
>>>>>       void        *virtio_user_dev;
>>>>> +    bool        opened;
>>>
>>> This is already merged into next-net-virtio, but I would like to 
>>> hightlight the
>>> checkpatch warning about `bool` usage in struct [1].
>>> Briefly it suggests preferring primitive data types against `bool` in 
>>> structures
>>> because its size is not clear.
>>>
>>> What do you think about it, do you have strong opinion to have them as 
>>> bool?
>>
>> Yes, I suppose I do.  bool is the "proper" representation for yes/no.
>> The arguments cited aren't convincing.
>>
>> The size of bool is the size of bool. The compiler gets to make that
>> decision.  I don't get to decide the size of int either.  The size and
>> alignemnt of bool should be optimal because your compiler probably knows
>> more about the target than you do.  If your compiler can't figure out
>> alignment in a structure, please fix the compiler.
>>
>> bool is a primitive data type per the C99 standard.
> 
> I would like to keep it as bool too.

OK

> 
>>> [1]
>>> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
>>> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
>>> #85: FILE: drivers/net/virtio/virtio_pci.h:234:
>>> +       bool        started;
>>>
>>> CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible
>>> alignment issues - see: https://lkml.org/lkml/2017/11/21/384
>>> #93: FILE: drivers/net/virtio/virtio_pci.h:260:
>>> +       bool        opened;
>>>
> 
> BTW, I don't get this warning when running checkpatch, what kenrel
> version is it coming from?

v4.18-11078-gd729593e492e
d729593e492e ("checkpatch: add a --strict test for structs with bool member
definitions")

> 
> Thanks,
> Maxime
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-11-02 17:17 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-17 23:05 [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Charles (Chas) Williams
2017-07-17 23:05 ` [dpdk-dev] [PATCH 1/2] net/virtio: do not re-enter clean up routines Charles (Chas) Williams
2018-11-01 14:45   ` Luca Boccassi
2018-11-02  9:57     ` Maxime Coquelin
2018-11-02 11:03       ` Maxime Coquelin
2018-11-02 11:14         ` Luca Boccassi
2018-11-02 14:33     ` Ferruh Yigit
2018-11-02 15:19       ` Chas Williams
2018-11-02 16:48         ` Maxime Coquelin
2018-11-02 17:17           ` Ferruh Yigit
2017-07-17 23:05 ` [dpdk-dev] [PATCH 2/2] net/virtio: register/unregister intr handler on start/stop Charles (Chas) Williams
2017-07-18 11:50 ` [dpdk-dev] [PATCH 0/2] Some small changes to net/virtio Ferruh Yigit
2017-07-18 11:52   ` Charles (Chas) Williams
2018-08-15 13:51     ` Luca Boccassi
2018-08-27 13:41       ` Ferruh Yigit
2018-08-27 13:48         ` Ferruh Yigit
2018-08-27 14:52           ` Gavin Hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).