[dpdk-dev,1/2] net/virtio: do not re-enter clean up routines
Checks
Commit Message
.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(-)
Comments
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!
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
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
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!
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;
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;
>
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
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
>
@@ -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");
@@ -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;
};