net/gve: fix bug in verify driver compatibility
Checks
Commit Message
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(-)
Comments
> -----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
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.
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.
> -----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!
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?
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?
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?
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?
>
@@ -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;