diff mbox series

[v2,2/2] Revert "net/virtio-user: fail if cq used with packed vq"

Message ID 20190110131751.32670-3-jfreimann@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio-user: add packed vq support |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Jens Freimann Jan. 10, 2019, 1:17 p.m. UTC 
  This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.

Not a clean revert, I had to resolve a conflict due to
  616ea5519 net/virtio-user: fix packed vq option parsing

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Maxime Coquelin Jan. 10, 2019, 2:25 p.m. UTC | #1 
On 1/10/19 2:17 PM, Jens Freimann wrote:
> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
> 
> Not a clean revert, I had to resolve a conflict due to
>    616ea5519 net/virtio-user: fix packed vq option parsing

Maybe good to explain why it can be reverted now.

> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
> 

With the clarification in commit message:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Tiwei Bie Jan. 10, 2019, 2:40 p.m. UTC | #2 
On Thu, Jan 10, 2019 at 02:17:51PM +0100, Jens Freimann wrote:
> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
> 
> Not a clean revert, I had to resolve a conflict due to
>   616ea5519 net/virtio-user: fix packed vq option parsing
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 49fcf48b9..b7059cb1e 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -478,15 +478,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>  	if (!in_order)
>  		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
>  
> -	if (packed_vq) {
> -		if (cq) {
> -			PMD_INIT_LOG(ERR, "control vq not supported yet with "
> -					  "packed virtqueues\n");
> -			return -1;
> -		}

I think it's natural to drop above code in the same patch that
introduces the control vq support.

> -	} else {
> +	if (packed_vq)
> +		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);

We shouldn't add this bit into device_features like this.
We just need this:

	if (!packed_vq)
		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
	

> +	else
>  		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
> -	}
>  
>  	if (dev->mac_specified)
>  		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
> -- 
> 2.17.2
>
Jens Freimann Jan. 10, 2019, 2:58 p.m. UTC | #3 
On Thu, Jan 10, 2019 at 10:40:25PM +0800, Tiwei Bie wrote:
>On Thu, Jan 10, 2019 at 02:17:51PM +0100, Jens Freimann wrote:
>> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
>>
>> Not a clean revert, I had to resolve a conflict due to
>>   616ea5519 net/virtio-user: fix packed vq option parsing
>>
>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index 49fcf48b9..b7059cb1e 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -478,15 +478,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>>  	if (!in_order)
>>  		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
>>
>> -	if (packed_vq) {
>> -		if (cq) {
>> -			PMD_INIT_LOG(ERR, "control vq not supported yet with "
>> -					  "packed virtqueues\n");
>> -			return -1;
>> -		}
>
>I think it's natural to drop above code in the same patch that
>introduces the control vq support.

I just split it out because Maxime requested it. 

>
>> -	} else {
>> +	if (packed_vq)
>> +		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
>
>We shouldn't add this bit into device_features like this.
>We just need this:

ok

regards,
Jens 

>
>	if (!packed_vq)
>		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
>	
>
>> +	else
>>  		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
>> -	}
>>
>>  	if (dev->mac_specified)
>>  		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
>> --
>> 2.17.2
>>
Maxime Coquelin Jan. 10, 2019, 3:01 p.m. UTC | #4 
On 1/10/19 3:58 PM, Jens Freimann wrote:
> On Thu, Jan 10, 2019 at 10:40:25PM +0800, Tiwei Bie wrote:
>> On Thu, Jan 10, 2019 at 02:17:51PM +0100, Jens Freimann wrote:
>>> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
>>>
>>> Not a clean revert, I had to resolve a conflict due to
>>>   616ea5519 net/virtio-user: fix packed vq option parsing
>>>
>>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>> ---
>>>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index 49fcf48b9..b7059cb1e 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -478,15 +478,10 @@ virtio_user_dev_init(struct virtio_user_dev 
>>> *dev, char *path, int queues,
>>>      if (!in_order)
>>>          dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
>>>
>>> -    if (packed_vq) {
>>> -        if (cq) {
>>> -            PMD_INIT_LOG(ERR, "control vq not supported yet with "
>>> -                      "packed virtqueues\n");
>>> -            return -1;
>>> -        }
>>
>> I think it's natural to drop above code in the same patch that
>> introduces the control vq support.
> 
> I just split it out because Maxime requested it.

Yeah, disagreements happen. :)

I understand this is not a clean revert anyway, so feel free to keep it
in a single patch.

Thanks,
Maxime

>>
>>> -    } else {
>>> +    if (packed_vq)
>>> +        dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
>>
>> We shouldn't add this bit into device_features like this.
>> We just need this:
> 
> ok
> 
> regards,
> Jens
>>
>>     if (!packed_vq)
>>         dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
>>
>>
>>> +    else
>>>          dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
>>> -    }
>>>
>>>      if (dev->mac_specified)
>>>          dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
>>> -- 
>>> 2.17.2
>>>
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 49fcf48b9..b7059cb1e 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -478,15 +478,10 @@  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	if (!in_order)
 		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
 
-	if (packed_vq) {
-		if (cq) {
-			PMD_INIT_LOG(ERR, "control vq not supported yet with "
-					  "packed virtqueues\n");
-			return -1;
-		}
-	} else {
+	if (packed_vq)
+		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
+	else
 		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
-	}
 
 	if (dev->mac_specified)
 		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);