[PATCH] net/gve: fix bug in verify driver compatibility

[PATCH] net/gve: fix bug in verify driver compatibility

[Rushil Gupta]

gVNIC requires physical address to be passed in the adminq command.
This was initially rightly pointed by ferruh.yigit@.
Fixed by passing 'driver_info_mem->iova'.

Signed-off-by: Rushil Gupta <rushilg@google.com>
---
 drivers/net/gve/gve_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index 2c1e73d07a..1c5ce930a4 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
 			(char *)driver_info->os_version_str2);
 
 	err = gve_adminq_verify_driver_compatibility(priv,
-		sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
+		sizeof(struct gve_driver_info),
+                (dma_addr_t)driver_info_mem->iova);
 	/* It's ok if the device doesn't support this */
 	if (err == -EOPNOTSUPP)
 		err = 0;
-- 
2.41.0.rc2.161.g9c6817b8e7-goog

[PATCH] net/gve: fix bug in verify driver compatibility

[Rushil Gupta]

gVNIC requires physical address to be passed in the adminq command.
This was initially rightly pointed by ferruh.yigit@.
Fixed by passing 'driver_info_mem->iova'.

Signed-off-by: Rushil Gupta <rushilg@google.com>
---
 drivers/net/gve/gve_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index 2c1e73d07a..aa75abe102 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
 			(char *)driver_info->os_version_str2);
 
 	err = gve_adminq_verify_driver_compatibility(priv,
-		sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
+		sizeof(struct gve_driver_info),
+		(dma_addr_t)driver_info_mem->iova);
 	/* It's ok if the device doesn't support this */
 	if (err == -EOPNOTSUPP)
 		err = 0;
-- 
2.41.0.rc2.161.g9c6817b8e7-goog

RE: [PATCH] net/gve: fix bug in verify driver compatibility

[Guo, Junfeng]

> -----Original Message-----
> From: Rushil Gupta <rushilg@google.com>
> Sent: Thursday, June 1, 2023 12:50
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; ferruh.yigit@amd.com
> Cc: Guo, Junfeng <junfeng.guo@intel.com>; dev@dpdk.org; Rushil Gupta
> <rushilg@google.com>
> Subject: [PATCH] net/gve: fix bug in verify driver compatibility
> 
> gVNIC requires physical address to be passed in the adminq command.
> This was initially rightly pointed by ferruh.yigit@.
> Fixed by passing 'driver_info_mem->iova'.

For bug fixing, it is required to reference the id of the commit which introduced the bug.
You can generate the required lines using the following git alias:
''' git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'"  '''
and then put the output of ''' git fixline <SHA> ''' into the commit message.

And also be CCed to the stable@dpdk.org mailing list. Thanks!

Regards,
Junfeng

> 
> Signed-off-by: Rushil Gupta <rushilg@google.com>
> ---
>  drivers/net/gve/gve_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> index 2c1e73d07a..aa75abe102 100644
> --- a/drivers/net/gve/gve_ethdev.c
> +++ b/drivers/net/gve/gve_ethdev.c
> @@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv
> *priv)
>  			(char *)driver_info->os_version_str2);
> 
>  	err = gve_adminq_verify_driver_compatibility(priv,
> -		sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
> +		sizeof(struct gve_driver_info),
> +		(dma_addr_t)driver_info_mem->iova);
>  	/* It's ok if the device doesn't support this */
>  	if (err == -EOPNOTSUPP)
>  		err = 0;
> --
> 2.41.0.rc2.161.g9c6817b8e7-goog

Re: [PATCH] net/gve: fix bug in verify driver compatibility

[Ferruh Yigit]

On 6/1/2023 5:49 AM, Rushil Gupta wrote:
> gVNIC requires physical address to be passed in the adminq command.
> This was initially rightly pointed by ferruh.yigit@.
> Fixed by passing 'driver_info_mem->iova'.
> 
> Signed-off-by: Rushil Gupta <rushilg@google.com>
> ---
>  drivers/net/gve/gve_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> index 2c1e73d07a..aa75abe102 100644
> --- a/drivers/net/gve/gve_ethdev.c
> +++ b/drivers/net/gve/gve_ethdev.c
> @@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
>  			(char *)driver_info->os_version_str2);
>  
>  	err = gve_adminq_verify_driver_compatibility(priv,
> -		sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
> +		sizeof(struct gve_driver_info),
> +		(dma_addr_t)driver_info_mem->iova);
>

Yep, this was my point, let me squashed onto original patch in next-net.

Re: [PATCH] net/gve: fix bug in verify driver compatibility

[Ferruh Yigit]

On 6/1/2023 7:44 AM, Guo, Junfeng wrote:
> 
> 
>> -----Original Message-----
>> From: Rushil Gupta <rushilg@google.com>
>> Sent: Thursday, June 1, 2023 12:50
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; ferruh.yigit@amd.com
>> Cc: Guo, Junfeng <junfeng.guo@intel.com>; dev@dpdk.org; Rushil Gupta
>> <rushilg@google.com>
>> Subject: [PATCH] net/gve: fix bug in verify driver compatibility
>>
>> gVNIC requires physical address to be passed in the adminq command.
>> This was initially rightly pointed by ferruh.yigit@.
>> Fixed by passing 'driver_info_mem->iova'.
> 
> For bug fixing, it is required to reference the id of the commit which introduced the bug.
> You can generate the required lines using the following git alias:
> ''' git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'"  '''
> and then put the output of ''' git fixline <SHA> ''' into the commit message.
> 
> And also be CCed to the stable@dpdk.org mailing list. Thanks!
> 
> Regards,
> Junfeng
> 

Hi Junfeng,

You are right, but for this case patch is not pulled to main repo yet,
and change is trivial, so I will fix it in next-net.

RE: [PATCH] net/gve: fix bug in verify driver compatibility

[Guo, Junfeng]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, June 1, 2023 16:35
> To: Guo, Junfeng <junfeng.guo@intel.com>; Rushil Gupta
> <rushilg@google.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/gve: fix bug in verify driver compatibility
> 
> On 6/1/2023 7:44 AM, Guo, Junfeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Rushil Gupta <rushilg@google.com>
> >> Sent: Thursday, June 1, 2023 12:50
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; ferruh.yigit@amd.com
> >> Cc: Guo, Junfeng <junfeng.guo@intel.com>; dev@dpdk.org; Rushil
> Gupta
> >> <rushilg@google.com>
> >> Subject: [PATCH] net/gve: fix bug in verify driver compatibility
> >>
> >> gVNIC requires physical address to be passed in the adminq command.
> >> This was initially rightly pointed by ferruh.yigit@.
> >> Fixed by passing 'driver_info_mem->iova'.
> >
> > For bug fixing, it is required to reference the id of the commit which
> introduced the bug.
> > You can generate the required lines using the following git alias:
> > ''' git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h
> (\"%s\")%nCc: %ae'"  '''
> > and then put the output of ''' git fixline <SHA> ''' into the commit
> message.
> >
> > And also be CCed to the stable@dpdk.org mailing list. Thanks!
> >
> > Regards,
> > Junfeng
> >
> 
> Hi Junfeng,
> 
> You are right, but for this case patch is not pulled to main repo yet,
> and change is trivial, so I will fix it in next-net.

Sure, thanks for the efforts!

Re: [PATCH] net/gve: fix bug in verify driver compatibility

[Ferruh Yigit]

On 6/1/2023 9:26 AM, Ferruh Yigit wrote:
> On 6/1/2023 5:49 AM, Rushil Gupta wrote:
>> gVNIC requires physical address to be passed in the adminq command.
>> This was initially rightly pointed by ferruh.yigit@.
>> Fixed by passing 'driver_info_mem->iova'.
>>
>> Signed-off-by: Rushil Gupta <rushilg@google.com>
>> ---
>>  drivers/net/gve/gve_ethdev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
>> index 2c1e73d07a..aa75abe102 100644
>> --- a/drivers/net/gve/gve_ethdev.c
>> +++ b/drivers/net/gve/gve_ethdev.c
>> @@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
>>  			(char *)driver_info->os_version_str2);
>>  
>>  	err = gve_adminq_verify_driver_compatibility(priv,
>> -		sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
>> +		sizeof(struct gve_driver_info),
>> +		(dma_addr_t)driver_info_mem->iova);
>>
> 
> Yep, this was my point, let me squashed onto original patch in next-net.
> 
> 

Squashed into relevant commit in next-net, thanks.
Can you please verify latest code in next-net?

Re: [PATCH] net/gve: fix bug in verify driver compatibility

[Rushil Gupta]

Thanks for the quick response!
Just for my own knowledge, what Junfeng described is the process to
fix the bug if a bug is present in the main dpdk repo?

On Thu, Jun 1, 2023 at 3:24 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/1/2023 9:26 AM, Ferruh Yigit wrote:
> > On 6/1/2023 5:49 AM, Rushil Gupta wrote:
> >> gVNIC requires physical address to be passed in the adminq command.
> >> This was initially rightly pointed by ferruh.yigit@.
> >> Fixed by passing 'driver_info_mem->iova'.
> >>
> >> Signed-off-by: Rushil Gupta <rushilg@google.com>
> >> ---
> >>  drivers/net/gve/gve_ethdev.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> >> index 2c1e73d07a..aa75abe102 100644
> >> --- a/drivers/net/gve/gve_ethdev.c
> >> +++ b/drivers/net/gve/gve_ethdev.c
> >> @@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
> >>                      (char *)driver_info->os_version_str2);
> >>
> >>      err = gve_adminq_verify_driver_compatibility(priv,
> >> -            sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
> >> +            sizeof(struct gve_driver_info),
> >> +            (dma_addr_t)driver_info_mem->iova);
> >>
> >
> > Yep, this was my point, let me squashed onto original patch in next-net.
> >
> >
>
> Squashed into relevant commit in next-net, thanks.
> Can you please verify latest code in next-net?

Re: [PATCH] net/gve: fix bug in verify driver compatibility

[Ferruh Yigit]

On 6/1/2023 5:32 PM, Rushil Gupta wrote:
> Thanks for the quick response!
> Just for my own knowledge, what Junfeng described is the process to
> fix the bug if a bug is present in the main dpdk repo?
> 

Correct.

As main dpdk repo doesn't rewrite git history, any bug there needs to be
fixed with an incremental commit, and fix commit should have some
metadata to describe what it is fixing (this is to help LTS maintainers
and developers tracing down a change).

But sub-trees (like next-net) allowed to rewrite git history, so allowed
 to squash/rebase commits when it is more practical to do so as this case.
But changing directly in the git repo has its problems, like tracing,
justifying source of the change and missing consensus etc. So, even it
is squashed, the patch should be publicly available in the mail list first.

For developers it is best to send incremental patch as if it will be
merged as incremental commit, but can request a squash if the commit
still not pulled by main repo, or maintainer can prefer to squash the
fix to original commit, to reduce the churn & process overhead.

> On Thu, Jun 1, 2023 at 3:24 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 6/1/2023 9:26 AM, Ferruh Yigit wrote:
>>> On 6/1/2023 5:49 AM, Rushil Gupta wrote:
>>>> gVNIC requires physical address to be passed in the adminq command.
>>>> This was initially rightly pointed by ferruh.yigit@.
>>>> Fixed by passing 'driver_info_mem->iova'.
>>>>
>>>> Signed-off-by: Rushil Gupta <rushilg@google.com>
>>>> ---
>>>>  drivers/net/gve/gve_ethdev.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
>>>> index 2c1e73d07a..aa75abe102 100644
>>>> --- a/drivers/net/gve/gve_ethdev.c
>>>> +++ b/drivers/net/gve/gve_ethdev.c
>>>> @@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
>>>>                      (char *)driver_info->os_version_str2);
>>>>
>>>>      err = gve_adminq_verify_driver_compatibility(priv,
>>>> -            sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
>>>> +            sizeof(struct gve_driver_info),
>>>> +            (dma_addr_t)driver_info_mem->iova);
>>>>
>>>
>>> Yep, this was my point, let me squashed onto original patch in next-net.
>>>
>>>
>>
>> Squashed into relevant commit in next-net, thanks.
>> Can you please verify latest code in next-net?

Re: [PATCH] net/gve: fix bug in verify driver compatibility

[Rushil Gupta]

Thanks for the clarification and for squashing the bug into the main commit.
I just tested the net-next branch and it seems to log the right
message on the device.

On Fri, Jun 2, 2023 at 7:49 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/1/2023 5:32 PM, Rushil Gupta wrote:
> > Thanks for the quick response!
> > Just for my own knowledge, what Junfeng described is the process to
> > fix the bug if a bug is present in the main dpdk repo?
> >
>
> Correct.
>
> As main dpdk repo doesn't rewrite git history, any bug there needs to be
> fixed with an incremental commit, and fix commit should have some
> metadata to describe what it is fixing (this is to help LTS maintainers
> and developers tracing down a change).
>
> But sub-trees (like next-net) allowed to rewrite git history, so allowed
>  to squash/rebase commits when it is more practical to do so as this case.
> But changing directly in the git repo has its problems, like tracing,
> justifying source of the change and missing consensus etc. So, even it
> is squashed, the patch should be publicly available in the mail list first.
>
> For developers it is best to send incremental patch as if it will be
> merged as incremental commit, but can request a squash if the commit
> still not pulled by main repo, or maintainer can prefer to squash the
> fix to original commit, to reduce the churn & process overhead.
>
> > On Thu, Jun 1, 2023 at 3:24 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 6/1/2023 9:26 AM, Ferruh Yigit wrote:
> >>> On 6/1/2023 5:49 AM, Rushil Gupta wrote:
> >>>> gVNIC requires physical address to be passed in the adminq command.
> >>>> This was initially rightly pointed by ferruh.yigit@.
> >>>> Fixed by passing 'driver_info_mem->iova'.
> >>>>
> >>>> Signed-off-by: Rushil Gupta <rushilg@google.com>
> >>>> ---
> >>>>  drivers/net/gve/gve_ethdev.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> >>>> index 2c1e73d07a..aa75abe102 100644
> >>>> --- a/drivers/net/gve/gve_ethdev.c
> >>>> +++ b/drivers/net/gve/gve_ethdev.c
> >>>> @@ -276,7 +276,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
> >>>>                      (char *)driver_info->os_version_str2);
> >>>>
> >>>>      err = gve_adminq_verify_driver_compatibility(priv,
> >>>> -            sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
> >>>> +            sizeof(struct gve_driver_info),
> >>>> +            (dma_addr_t)driver_info_mem->iova);
> >>>>
> >>>
> >>> Yep, this was my point, let me squashed onto original patch in next-net.
> >>>
> >>>
> >>
> >> Squashed into relevant commit in next-net, thanks.
> >> Can you please verify latest code in next-net?
>