[PATCH 08/16] virtio_net: drop config_enable

[PATCH 08/16] virtio_net: drop config_enable

[Michael S. Tsirkin]

Now that virtio core ensures config changes don't arrive during probing,
drop config_enable flag in virtio net.
On removal, flush is now sufficient to guarantee that no change work is
queued.

This help simplify the driver, and will allow setting DRIVER_OK earlier
without losing config change notifications.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59caa06..fa17afa 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -123,9 +123,6 @@ struct virtnet_info {
 	/* Host can handle any s/g split between our header and packet data */
 	bool any_header_sg;
 
-	/* enable config space updates */
-	bool config_enable;
-
 	/* Active statistics */
 	struct virtnet_stats __percpu *stats;
 
@@ -1408,9 +1405,6 @@ static void virtnet_config_changed_work(struct work_struct *work)
 	u16 v;
 
 	mutex_lock(&vi->config_lock);
-	if (!vi->config_enable)
-		goto done;
-
 	if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS,
 				 struct virtio_net_config, status, &v) < 0)
 		goto done;
@@ -1758,7 +1752,6 @@ static int virtnet_probe(struct virtio_device *vdev)
 	}
 
 	mutex_init(&vi->config_lock);
-	vi->config_enable = true;
 	INIT_WORK(&vi->config_work, virtnet_config_changed_work);
 
 	/* If we can receive ANY GSO packets, we must allocate large ones. */
@@ -1876,16 +1869,12 @@ static void virtnet_remove(struct virtio_device *vdev)
 	unregister_hotcpu_notifier(&vi->nb);
 
 	/* Prevent config work handler from accessing the device. */
-	mutex_lock(&vi->config_lock);
-	vi->config_enable = false;
-	mutex_unlock(&vi->config_lock);
+	flush_work(&vi->config_work);
 
 	unregister_netdev(vi->dev);
 
 	remove_vq_common(vi);
 
-	flush_work(&vi->config_work);
-
 	free_percpu(vi->stats);
 	free_netdev(vi->dev);
 }
@@ -1899,9 +1888,7 @@ static int virtnet_freeze(struct virtio_device *vdev)
 	unregister_hotcpu_notifier(&vi->nb);
 
 	/* Prevent config work handler from accessing the device */
-	mutex_lock(&vi->config_lock);
-	vi->config_enable = false;
-	mutex_unlock(&vi->config_lock);
+	flush_work(&vi->config_work);
 
 	netif_device_detach(vi->dev);
 	cancel_delayed_work_sync(&vi->refill);
@@ -1916,8 +1903,6 @@ static int virtnet_freeze(struct virtio_device *vdev)
 
 	remove_vq_common(vi);
 
-	flush_work(&vi->config_work);
-
 	return 0;
 }
 
@@ -1941,10 +1926,6 @@ static int virtnet_restore(struct virtio_device *vdev)
 
 	netif_device_attach(vi->dev);
 
-	mutex_lock(&vi->config_lock);
-	vi->config_enable = true;
-	mutex_unlock(&vi->config_lock);
-
 	rtnl_lock();
 	virtnet_set_queues(vi, vi->curr_queue_pairs);
 	rtnl_unlock();
-- 
MST

Re: [PATCH 08/16] virtio_net: drop config_enable

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:13 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Now that virtio core ensures config changes don't arrive during probing,
> drop config_enable flag in virtio net.
> On removal, flush is now sufficient to guarantee that no change work is
> queued.
> 
> This help simplify the driver, and will allow setting DRIVER_OK earlier
> without losing config change notifications.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/net/virtio_net.c | 23 ++---------------------
>  1 file changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 59caa06..fa17afa 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

> @@ -1876,16 +1869,12 @@ static void virtnet_remove(struct virtio_device *vdev)
>  	unregister_hotcpu_notifier(&vi->nb);
> 
>  	/* Prevent config work handler from accessing the device. */

Same comment as for the equivalent comment in the virtio-blk code.

> -	mutex_lock(&vi->config_lock);
> -	vi->config_enable = false;
> -	mutex_unlock(&vi->config_lock);
> +	flush_work(&vi->config_work);
> 
>  	unregister_netdev(vi->dev);
> 
>  	remove_vq_common(vi);
> 
> -	flush_work(&vi->config_work);
> -
>  	free_percpu(vi->stats);
>  	free_netdev(vi->dev);
>  }
> @@ -1899,9 +1888,7 @@ static int virtnet_freeze(struct virtio_device *vdev)
>  	unregister_hotcpu_notifier(&vi->nb);
> 
>  	/* Prevent config work handler from accessing the device */

dito

> -	mutex_lock(&vi->config_lock);
> -	vi->config_enable = false;
> -	mutex_unlock(&vi->config_lock);
> +	flush_work(&vi->config_work);
> 
>  	netif_device_detach(vi->dev);
>  	cancel_delayed_work_sync(&vi->refill);

Re: [PATCH 08/16] virtio_net: drop config_enable

[Michael S. Tsirkin]

On Mon, Oct 06, 2014 at 01:50:36PM +0200, Cornelia Huck wrote:
> On Sun, 5 Oct 2014 19:07:13 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Now that virtio core ensures config changes don't arrive during probing,
> > drop config_enable flag in virtio net.
> > On removal, flush is now sufficient to guarantee that no change work is
> > queued.
> > 
> > This help simplify the driver, and will allow setting DRIVER_OK earlier
> > without losing config change notifications.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/net/virtio_net.c | 23 ++---------------------
> >  1 file changed, 2 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 59caa06..fa17afa 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> 
> > @@ -1876,16 +1869,12 @@ static void virtnet_remove(struct virtio_device *vdev)
> >  	unregister_hotcpu_notifier(&vi->nb);
> > 
> >  	/* Prevent config work handler from accessing the device. */
> 
> Same comment as for the equivalent comment in the virtio-blk code.

Same answer :)

> > -	mutex_lock(&vi->config_lock);
> > -	vi->config_enable = false;
> > -	mutex_unlock(&vi->config_lock);
> > +	flush_work(&vi->config_work);
> > 
> >  	unregister_netdev(vi->dev);
> > 
> >  	remove_vq_common(vi);
> > 
> > -	flush_work(&vi->config_work);
> > -
> >  	free_percpu(vi->stats);
> >  	free_netdev(vi->dev);
> >  }
> > @@ -1899,9 +1888,7 @@ static int virtnet_freeze(struct virtio_device *vdev)
> >  	unregister_hotcpu_notifier(&vi->nb);
> > 
> >  	/* Prevent config work handler from accessing the device */
> 
> dito

Same here.

> > -	mutex_lock(&vi->config_lock);
> > -	vi->config_enable = false;
> > -	mutex_unlock(&vi->config_lock);
> > +	flush_work(&vi->config_work);
> > 
> >  	netif_device_detach(vi->dev);
> >  	cancel_delayed_work_sync(&vi->refill);

Re: [PATCH 08/16] virtio_net: drop config_enable

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Sun, 5 Oct 2014 19:07:13 +0300

> Now that virtio core ensures config changes don't arrive during probing,
> drop config_enable flag in virtio net.
> On removal, flush is now sufficient to guarantee that no change work is
> queued.
> 
> This help simplify the driver, and will allow setting DRIVER_OK earlier
> without losing config change notifications.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

It's hard for people on the networking side to review these changes
since you haven't CC:'d them on any of the postings necessary to
understand the context of the net/ and drivers/net/ changes.

Please at a minimum CC: everyone on your header [PATCH 0/N] posting
so we know at least at a high level what is going on, and why.

Thanks.

Re: [PATCH 08/16] virtio_net: drop config_enable

[Michael S. Tsirkin]

On Mon, Oct 06, 2014 at 03:02:38PM -0400, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Sun, 5 Oct 2014 19:07:13 +0300
> 
> > Now that virtio core ensures config changes don't arrive during probing,
> > drop config_enable flag in virtio net.
> > On removal, flush is now sufficient to guarantee that no change work is
> > queued.
> > 
> > This help simplify the driver, and will allow setting DRIVER_OK earlier
> > without losing config change notifications.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> It's hard for people on the networking side to review these changes
> since you haven't CC:'d them on any of the postings necessary to
> understand the context of the net/ and drivers/net/ changes.
> 
> Please at a minimum CC: everyone on your header [PATCH 0/N] posting
> so we know at least at a high level what is going on, and why.
> 
> Thanks.

It's a bit tricky for large patchsets - if I add everyone to 0/N
then vger isn't happy with Cc list that is too large.

What is your advice here? Cc just mailing lists on 0/N?

FWIW this patchset is inteded for the virtio tree.

-- 
MST

Re: [PATCH 08/16] virtio_net: drop config_enable

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 7 Oct 2014 09:49:15 +0300

> On Mon, Oct 06, 2014 at 03:02:38PM -0400, David Miller wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>> Date: Sun, 5 Oct 2014 19:07:13 +0300
>> 
>> > Now that virtio core ensures config changes don't arrive during probing,
>> > drop config_enable flag in virtio net.
>> > On removal, flush is now sufficient to guarantee that no change work is
>> > queued.
>> > 
>> > This help simplify the driver, and will allow setting DRIVER_OK earlier
>> > without losing config change notifications.
>> > 
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> 
>> It's hard for people on the networking side to review these changes
>> since you haven't CC:'d them on any of the postings necessary to
>> understand the context of the net/ and drivers/net/ changes.
>> 
>> Please at a minimum CC: everyone on your header [PATCH 0/N] posting
>> so we know at least at a high level what is going on, and why.
>> 
>> Thanks.
> 
> It's a bit tricky for large patchsets - if I add everyone to 0/N
> then vger isn't happy with Cc list that is too large.
> 
> What is your advice here? Cc just mailing lists on 0/N?
> 
> FWIW this patchset is inteded for the virtio tree.

CC: mailing lists and "focus" developers, a small carefully selected
group of people who would be strongly interested in this change.

I really don't understand why this is so complicated, I've never run
into a situation where I had to CC: 200 people in my two decades of
kernel development :-/

Re: [PATCH 08/16] virtio_net: drop config_enable

[Michael S. Tsirkin]

On Tue, Oct 07, 2014 at 11:36:43AM -0400, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Tue, 7 Oct 2014 09:49:15 +0300
> 
> > On Mon, Oct 06, 2014 at 03:02:38PM -0400, David Miller wrote:
> >> From: "Michael S. Tsirkin" <mst@redhat.com>
> >> Date: Sun, 5 Oct 2014 19:07:13 +0300
> >> 
> >> > Now that virtio core ensures config changes don't arrive during probing,
> >> > drop config_enable flag in virtio net.
> >> > On removal, flush is now sufficient to guarantee that no change work is
> >> > queued.
> >> > 
> >> > This help simplify the driver, and will allow setting DRIVER_OK earlier
> >> > without losing config change notifications.
> >> > 
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> 
> >> It's hard for people on the networking side to review these changes
> >> since you haven't CC:'d them on any of the postings necessary to
> >> understand the context of the net/ and drivers/net/ changes.
> >> 
> >> Please at a minimum CC: everyone on your header [PATCH 0/N] posting
> >> so we know at least at a high level what is going on, and why.
> >> 
> >> Thanks.
> > 
> > It's a bit tricky for large patchsets - if I add everyone to 0/N
> > then vger isn't happy with Cc list that is too large.
> > 
> > What is your advice here? Cc just mailing lists on 0/N?
> > 
> > FWIW this patchset is inteded for the virtio tree.
> 
> CC: mailing lists and "focus" developers, a small carefully selected
> group of people who would be strongly interested in this change.
> 
> I really don't understand why this is so complicated, I've never run
> into a situation where I had to CC: 200 people in my two decades of
> kernel development :-/

Will do for the next version, thanks!

[PATCH 09/16] virtio-net: drop config_mutex

[Michael S. Tsirkin]

config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.

Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
    workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.

Get rid of the unnecessary lock.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fa17afa..d80fef4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -132,9 +132,6 @@ struct virtnet_info {
 	/* Work struct for config space updates */
 	struct work_struct config_work;
 
-	/* Lock for config space updates */
-	struct mutex config_lock;
-
 	/* Does the affinity hint is set for virtqueues? */
 	bool affinity_hint_set;
 
@@ -1404,7 +1401,6 @@ static void virtnet_config_changed_work(struct work_struct *work)
 		container_of(work, struct virtnet_info, config_work);
 	u16 v;
 
-	mutex_lock(&vi->config_lock);
 	if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS,
 				 struct virtio_net_config, status, &v) < 0)
 		goto done;
@@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
 		netif_tx_stop_all_queues(vi->dev);
 	}
 done:
-	mutex_unlock(&vi->config_lock);
+	return;
 }
 
 static void virtnet_config_changed(struct virtio_device *vdev)
@@ -1751,7 +1747,6 @@ static int virtnet_probe(struct virtio_device *vdev)
 		u64_stats_init(&virtnet_stats->rx_syncp);
 	}
 
-	mutex_init(&vi->config_lock);
 	INIT_WORK(&vi->config_work, virtnet_config_changed_work);
 
 	/* If we can receive ANY GSO packets, we must allocate large ones. */
-- 
MST

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Sergei Shtylyov]

Hello.

On 10/5/2014 8:07 PM, Michael S. Tsirkin wrote:

> config_mutex served two purposes: prevent multiple concurrent config
> change handlers, and synchronize access to config_enable flag.

> Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
>      workqueue: make all workqueues non-reentrant
> all workqueues are non-reentrant, and config_enable
> is now gone.

> Get rid of the unnecessary lock.

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   drivers/net/virtio_net.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fa17afa..d80fef4 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
[...]
> @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
>   		netif_tx_stop_all_queues(vi->dev);
>   	}
>   done:
> -	mutex_unlock(&vi->config_lock);
> +	return;

    There's no need for this *return*.

>   }
>
>   static void virtnet_config_changed(struct virtio_device *vdev)

WBR, Sergei

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Michael S. Tsirkin]

On Mon, Oct 06, 2014 at 03:46:15PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 10/5/2014 8:07 PM, Michael S. Tsirkin wrote:
> 
> >config_mutex served two purposes: prevent multiple concurrent config
> >change handlers, and synchronize access to config_enable flag.
> 
> >Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
> >     workqueue: make all workqueues non-reentrant
> >all workqueues are non-reentrant, and config_enable
> >is now gone.
> 
> >Get rid of the unnecessary lock.
> 
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> >  drivers/net/virtio_net.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> >diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >index fa17afa..d80fef4 100644
> >--- a/drivers/net/virtio_net.c
> >+++ b/drivers/net/virtio_net.c
> [...]
> >@@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
> >  		netif_tx_stop_all_queues(vi->dev);
> >  	}
> >  done:
> >-	mutex_unlock(&vi->config_lock);
> >+	return;
> 
>    There's no need for this *return*.
>

I know - it's removed by the follow-up patch.
It's formatted like this to make diff smaller
and make review easier.
 
> >  }
> >
> >  static void virtnet_config_changed(struct virtio_device *vdev)
> 
> WBR, Sergei

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Sergei Shtylyov]

On 10/6/2014 3:56 PM, Michael S. Tsirkin wrote:

>>> config_mutex served two purposes: prevent multiple concurrent config
>>> change handlers, and synchronize access to config_enable flag.

>>> Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
>>>      workqueue: make all workqueues non-reentrant
>>> all workqueues are non-reentrant, and config_enable
>>> is now gone.

>>> Get rid of the unnecessary lock.

>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>   drivers/net/virtio_net.c | 7 +------
>>>   1 file changed, 1 insertion(+), 6 deletions(-)

>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index fa17afa..d80fef4 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>> [...]
>>> @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
>>>   		netif_tx_stop_all_queues(vi->dev);
>>>   	}
>>>   done:
>>> -	mutex_unlock(&vi->config_lock);
>>> +	return;

>>     There's no need for this *return*.

> I know - it's removed by the follow-up patch.

    Yeah, I saw.

> It's formatted like this to make diff smaller
> and make review easier.

    Don't understand how adding this line makes diff smaller though...
You first need to add it and then to delete it, where's the save?

WBR, Sergei

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Michael S. Tsirkin]

On Mon, Oct 06, 2014 at 04:07:32PM +0400, Sergei Shtylyov wrote:
> On 10/6/2014 3:56 PM, Michael S. Tsirkin wrote:
> 
> >>>config_mutex served two purposes: prevent multiple concurrent config
> >>>change handlers, and synchronize access to config_enable flag.
> 
> >>>Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
> >>>     workqueue: make all workqueues non-reentrant
> >>>all workqueues are non-reentrant, and config_enable
> >>>is now gone.
> 
> >>>Get rid of the unnecessary lock.
> 
> >>>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>---
> >>>  drivers/net/virtio_net.c | 7 +------
> >>>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> >>>diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>index fa17afa..d80fef4 100644
> >>>--- a/drivers/net/virtio_net.c
> >>>+++ b/drivers/net/virtio_net.c
> >>[...]
> >>>@@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
> >>>  		netif_tx_stop_all_queues(vi->dev);
> >>>  	}
> >>>  done:
> >>>-	mutex_unlock(&vi->config_lock);
> >>>+	return;
> 
> >>    There's no need for this *return*.
> 
> >I know - it's removed by the follow-up patch.
> 
>    Yeah, I saw.
> 
> >It's formatted like this to make diff smaller
> >and make review easier.
> 
>    Don't understand how adding this line makes diff smaller though...
> You first need to add it and then to delete it, where's the save?
> 
> WBR, Sergei


If I don't add it, gcc generates a compiler warning: it does not like
labels at the end of functions.
So I don't want to just drop the return function: I must drop the label too.

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Sergei Shtylyov]

On 10/6/2014 4:22 PM, Michael S. Tsirkin wrote:

>>>>> config_mutex served two purposes: prevent multiple concurrent config
>>>>> change handlers, and synchronize access to config_enable flag.

>>>>> Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
>>>>>      workqueue: make all workqueues non-reentrant
>>>>> all workqueues are non-reentrant, and config_enable
>>>>> is now gone.

>>>>> Get rid of the unnecessary lock.

>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>   drivers/net/virtio_net.c | 7 +------
>>>>>   1 file changed, 1 insertion(+), 6 deletions(-)

>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>> index fa17afa..d80fef4 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>> [...]
>>>>> @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
>>>>>   		netif_tx_stop_all_queues(vi->dev);
>>>>>   	}
>>>>>   done:
>>>>> -	mutex_unlock(&vi->config_lock);
>>>>> +	return;

>>>>     There's no need for this *return*.

>>> I know - it's removed by the follow-up patch.

>>     Yeah, I saw.

>>> It's formatted like this to make diff smaller
>>> and make review easier.

>>     Don't understand how adding this line makes diff smaller though...
>> You first need to add it and then to delete it, where's the save?

>> WBR, Sergei

> If I don't add it, gcc generates a compiler warning: it does not like
> labels at the end of functions.

    Ahh... nevermind then.

WBR, Sergei

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:16 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> config_mutex served two purposes: prevent multiple concurrent config
> change handlers, and synchronize access to config_enable flag.
> 
> Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
>     workqueue: make all workqueues non-reentrant
> all workqueues are non-reentrant, and config_enable
> is now gone.
> 
> Get rid of the unnecessary lock.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/net/virtio_net.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fa17afa..d80fef4 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

> @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
>  		netif_tx_stop_all_queues(vi->dev);
>  	}
>  done:
> -	mutex_unlock(&vi->config_lock);
> +	return;
>  }
> 
>  static void virtnet_config_changed(struct virtio_device *vdev)

I'd probably return directly in the remaining 'goto done;' cases, but still

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Michael S. Tsirkin]

On Mon, Oct 06, 2014 at 01:56:10PM +0200, Cornelia Huck wrote:
> On Sun, 5 Oct 2014 19:07:16 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > config_mutex served two purposes: prevent multiple concurrent config
> > change handlers, and synchronize access to config_enable flag.
> > 
> > Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
> >     workqueue: make all workqueues non-reentrant
> > all workqueues are non-reentrant, and config_enable
> > is now gone.
> > 
> > Get rid of the unnecessary lock.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/net/virtio_net.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index fa17afa..d80fef4 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> 
> > @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
> >  		netif_tx_stop_all_queues(vi->dev);
> >  	}
> >  done:
> > -	mutex_unlock(&vi->config_lock);
> > +	return;
> >  }
> > 
> >  static void virtnet_config_changed(struct virtio_device *vdev)
> 
> I'd probably return directly in the remaining 'goto done;' cases, but still
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Yes: this is exactly what
[PATCH 11/16] virtio_net: minor cleanup
does

-- 
MST

[PATCH 11/16] virtio_net: minor cleanup

[Michael S. Tsirkin]

	goto done;
done:
	return;
is ugly, it was put there to make diff review easier.
replace by open-coded return.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d80fef4..4c8c314 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1403,7 +1403,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
 
 	if (virtio_cread_feature(vi->vdev, VIRTIO_NET_F_STATUS,
 				 struct virtio_net_config, status, &v) < 0)
-		goto done;
+		return;
 
 	if (v & VIRTIO_NET_S_ANNOUNCE) {
 		netdev_notify_peers(vi->dev);
@@ -1414,7 +1414,7 @@ static void virtnet_config_changed_work(struct work_struct *work)
 	v &= VIRTIO_NET_S_LINK_UP;
 
 	if (vi->status == v)
-		goto done;
+		return;
 
 	vi->status = v;
 
@@ -1425,8 +1425,6 @@ static void virtnet_config_changed_work(struct work_struct *work)
 		netif_carrier_off(vi->dev);
 		netif_tx_stop_all_queues(vi->dev);
 	}
-done:
-	return;
 }
 
 static void virtnet_config_changed(struct virtio_device *vdev)
-- 
MST

Re: [PATCH 11/16] virtio_net: minor cleanup

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:21 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 	goto done;
> done:
> 	return;
> is ugly, it was put there to make diff review easier.
> replace by open-coded return.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/net/virtio_net.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 

If you don't want to merge it into the mutex removal patch, maybe move
it one up in the series?

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[PATCH 12/16] virtio_net: enable VQs early

[Michael S. Tsirkin]

virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio net violated this
rule by using receive VQs within probe.

To fix, call virtio_early_enable_vqs before using VQs.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4c8c314..7afc990 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1792,6 +1792,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto free_vqs;
 	}
 
+	virtio_early_enable_vqs(vdev);
+
 	/* Last of all, set up some receive buffers. */
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		try_fill_recv(&vi->rq[i], GFP_KERNEL);
-- 
MST

[PATCH 15/16] 9p/trans_virtio: enable VQs early

[Michael S. Tsirkin]

virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, but virtio 9p device
adds self to channel list within probe, at which point VQ can be
used in violation of the spec.

To fix, call virtio_early_enable_vqs before using VQs.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 net/9p/trans_virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 6940d8f..0e8bbb3 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -575,6 +575,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	/* Ceiling limit to avoid denial of service attacks */
 	chan->p9_max_pages = nr_free_buffer_pages()/4;
 
+	virtio_early_enable_vqs(vdev);
+
 	mutex_lock(&virtio_9p_lock);
 	list_add_tail(&chan->chan_list, &virtio_chan_list);
 	mutex_unlock(&virtio_9p_lock);
-- 
MST

[PATCH 16/16] virtio_net: fix use after free on allocation failure

[Michael S. Tsirkin]

In the extremely unlikely event that driver initialization fails after
RX buffers are added, virtio net frees RX buffers while VQs are
still active, potentially causing device to use a freed buffer.

To fix, reset device first - same as we do on device removal.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7afc990..85e6098 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1830,6 +1830,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 	return 0;
 
 free_recv_bufs:
+	vi->vdev->config->reset(vdev);
+
 	free_receive_bufs(vi);
 	unregister_netdev(dev);
 free_vqs:
-- 
MST

Re: [PATCH 16/16] virtio_net: fix use after free on allocation failure

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:38 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> In the extremely unlikely event that driver initialization fails after
> RX buffers are added, virtio net frees RX buffers while VQs are
> still active, potentially causing device to use a freed buffer.
> 
> To fix, reset device first - same as we do on device removal.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/net/virtio_net.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>