In the function __idxd_completed_ops() we have a parameter shadow warning due to a local variable having the same name as one of the function parameters. This is fixed by simply renaming the local variable. Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs") Reported-by: Sunil Pai G <sunil.pai.g@intel.com> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> --- drivers/raw/ioat/rte_idxd_rawdev_fns.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/raw/ioat/rte_idxd_rawdev_fns.h b/drivers/raw/ioat/rte_idxd_rawdev_fns.h index 862e0eb41d..dbd8dfc507 100644 --- a/drivers/raw/ioat/rte_idxd_rawdev_fns.h +++ b/drivers/raw/ioat/rte_idxd_rawdev_fns.h @@ -301,11 +301,11 @@ __idxd_completed_ops(int dev_id, uint8_t max_ops, uint32_t *status, uint8_t *num uint16_t idx_to_chk = idxd->batch_idx_ring[idxd->batch_idx_read]; volatile struct rte_idxd_completion *comp_to_chk = (struct rte_idxd_completion *)&idxd->desc_ring[idx_to_chk]; - uint8_t status = comp_to_chk->status; - if (status == 0) + uint8_t batch_status = comp_to_chk->status; + if (batch_status == 0) break; comp_to_chk->status = 0; - if (unlikely(status > 1)) { + if (unlikely(batch_status > 1)) { /* error occurred somewhere in batch, start where last checked */ uint16_t desc_count = comp_to_chk->completed_size; uint16_t batch_start = idxd->hdls_avail; -- 2.30.2
<snipped> Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
On Fri, May 07, 2021 at 05:20:25PM +0000, Kevin Laatz wrote:
> In the function __idxd_completed_ops() we have a parameter shadow warning
> due to a local variable having the same name as one of the function
> parameters. This is fixed by simply renaming the local variable.
>
> Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs")
>
> Reported-by: Sunil Pai G <sunil.pai.g@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
Please provide details in the commit log as to how/when this was seen. I
believe this issue was seen only with OVS because it passes the -Wshadow
flag when building - something DPDK probably should do, but doesn't. Is
that correct?
> On Fri, May 07, 2021 at 05:20:25PM +0000, Kevin Laatz wrote:
> > In the function __idxd_completed_ops() we have a parameter shadow
> warning
> > due to a local variable having the same name as one of the function
> > parameters. This is fixed by simply renaming the local variable.
> >
> > Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs")
> >
> > Reported-by: Sunil Pai G <sunil.pai.g@intel.com>
> > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> > ---
> Please provide details in the commit log as to how/when this was seen. I
> believe this issue was seen only with OVS because it passes the -Wshadow
> flag when building - something DPDK probably should do, but doesn't. Is
> that correct?
Correct, the OVS build has -Wshadow by default. When -Wshadow is
passed to the DPDK build, this warning is also visible.
Will send a V2 to include details in commit log.
In the function __idxd_completed_ops() we have a parameter shadow warning due to a local variable having the same name as one of the function parameters. This issue is fixed by simply renaming the local variable. This warning was discovered during an OVS build with DPDK 21.05-rc2. The OVS build passes the -Wshadow flag by default, allowing the warning to be seen. Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs") Reported-by: Sunil Pai G <sunil.pai.g@intel.com> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> Tested-by: Sunil Pai G <sunil.pai.g@intel.com> --- v2: add details of warning discovery --- drivers/raw/ioat/rte_idxd_rawdev_fns.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/raw/ioat/rte_idxd_rawdev_fns.h b/drivers/raw/ioat/rte_idxd_rawdev_fns.h index 862e0eb41d..dbd8dfc507 100644 --- a/drivers/raw/ioat/rte_idxd_rawdev_fns.h +++ b/drivers/raw/ioat/rte_idxd_rawdev_fns.h @@ -301,11 +301,11 @@ __idxd_completed_ops(int dev_id, uint8_t max_ops, uint32_t *status, uint8_t *num uint16_t idx_to_chk = idxd->batch_idx_ring[idxd->batch_idx_read]; volatile struct rte_idxd_completion *comp_to_chk = (struct rte_idxd_completion *)&idxd->desc_ring[idx_to_chk]; - uint8_t status = comp_to_chk->status; - if (status == 0) + uint8_t batch_status = comp_to_chk->status; + if (batch_status == 0) break; comp_to_chk->status = 0; - if (unlikely(status > 1)) { + if (unlikely(batch_status > 1)) { /* error occurred somewhere in batch, start where last checked */ uint16_t desc_count = comp_to_chk->completed_size; uint16_t batch_start = idxd->hdls_avail; -- 2.30.2
On Mon, May 10, 2021 at 12:55:14PM +0000, Kevin Laatz wrote:
> In the function __idxd_completed_ops() we have a parameter shadow warning
> due to a local variable having the same name as one of the function
> parameters. This issue is fixed by simply renaming the local variable.
>
> This warning was discovered during an OVS build with DPDK 21.05-rc2. The
> OVS build passes the -Wshadow flag by default, allowing the warning to be
> seen.
>
> Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs")
>
> Reported-by: Sunil Pai G <sunil.pai.g@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
On Mon, May 10, 2021 at 2:55 PM Kevin Laatz <kevin.laatz@intel.com> wrote: > > In the function __idxd_completed_ops() we have a parameter shadow warning > due to a local variable having the same name as one of the function > parameters. This issue is fixed by simply renaming the local variable. > > This warning was discovered during an OVS build with DPDK 21.05-rc2. The > OVS build passes the -Wshadow flag by default, allowing the warning to be > seen. A bit confusing. -Wshadow only affects OVS code and there is no code calling this in the OVS master branch. I did not see this issue while updating my dpdk-latest OVS branch and running builds in GHA. So I guess Sunil caught it with his patch: https://patchwork.ozlabs.org/project/openvswitch/patch/20201023094845.35652-2-sunil.pai.g@intel.com/ -- David Marchand
On Mon, May 10, 2021 at 04:06:00PM +0200, David Marchand wrote:
> On Mon, May 10, 2021 at 2:55 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
> >
> > In the function __idxd_completed_ops() we have a parameter shadow warning
> > due to a local variable having the same name as one of the function
> > parameters. This issue is fixed by simply renaming the local variable.
> >
> > This warning was discovered during an OVS build with DPDK 21.05-rc2. The
> > OVS build passes the -Wshadow flag by default, allowing the warning to be
> > seen.
>
> A bit confusing.
> -Wshadow only affects OVS code and there is no code calling this in
> the OVS master branch.
>
> I did not see this issue while updating my dpdk-latest OVS branch and
> running builds in GHA.
> So I guess Sunil caught it with his patch:
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201023094845.35652-2-sunil.pai.g@intel.com/
>
Yes, it was caught by Sunil in the course of his work.
Ideally, I think -Wshadow would be a good flag to add to our DPDK builds,
but it causes quite a number of errors right now to do so. Hopefully in a
future release.
/Bruce
10/05/2021 16:48, Bruce Richardson:
> On Mon, May 10, 2021 at 04:06:00PM +0200, David Marchand wrote:
> > On Mon, May 10, 2021 at 2:55 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
> > >
> > > In the function __idxd_completed_ops() we have a parameter shadow warning
> > > due to a local variable having the same name as one of the function
> > > parameters. This issue is fixed by simply renaming the local variable.
> > >
> > > This warning was discovered during an OVS build with DPDK 21.05-rc2. The
> > > OVS build passes the -Wshadow flag by default, allowing the warning to be
> > > seen.
> >
> > A bit confusing.
> > -Wshadow only affects OVS code and there is no code calling this in
> > the OVS master branch.
> >
> > I did not see this issue while updating my dpdk-latest OVS branch and
> > running builds in GHA.
> > So I guess Sunil caught it with his patch:
> > https://patchwork.ozlabs.org/project/openvswitch/patch/20201023094845.35652-2-sunil.pai.g@intel.com/
> >
> Yes, it was caught by Sunil in the course of his work.
So the commit message should be fixed please.
In the function __idxd_completed_ops() we have a parameter shadow warning due to a local variable having the same name as one of the function parameters. This issue is fixed by simply renaming the local variable. This warning was caught when additions were made to the OVS codebase, which include adding calls the IOAT APIs. The OVS build passes the -Wshadow flag by default, allowing the warning to be seen when building OVS with DPDK 21.05-rc2. Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs") Reported-by: Sunil Pai G <sunil.pai.g@intel.com> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> Tested-by: Sunil Pai G <sunil.pai.g@intel.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com> --- v2: add details of warning discovery v3: commit log update --- drivers/raw/ioat/rte_idxd_rawdev_fns.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/raw/ioat/rte_idxd_rawdev_fns.h b/drivers/raw/ioat/rte_idxd_rawdev_fns.h index 862e0eb41d..dbd8dfc507 100644 --- a/drivers/raw/ioat/rte_idxd_rawdev_fns.h +++ b/drivers/raw/ioat/rte_idxd_rawdev_fns.h @@ -301,11 +301,11 @@ __idxd_completed_ops(int dev_id, uint8_t max_ops, uint32_t *status, uint8_t *num uint16_t idx_to_chk = idxd->batch_idx_ring[idxd->batch_idx_read]; volatile struct rte_idxd_completion *comp_to_chk = (struct rte_idxd_completion *)&idxd->desc_ring[idx_to_chk]; - uint8_t status = comp_to_chk->status; - if (status == 0) + uint8_t batch_status = comp_to_chk->status; + if (batch_status == 0) break; comp_to_chk->status = 0; - if (unlikely(status > 1)) { + if (unlikely(batch_status > 1)) { /* error occurred somewhere in batch, start where last checked */ uint16_t desc_count = comp_to_chk->completed_size; uint16_t batch_start = idxd->hdls_avail; -- 2.30.2
12/05/2021 12:47, Kevin Laatz:
> In the function __idxd_completed_ops() we have a parameter shadow warning
> due to a local variable having the same name as one of the function
> parameters. This issue is fixed by simply renaming the local variable.
>
> This warning was caught when additions were made to the OVS codebase,
> which include adding calls the IOAT APIs. The OVS build passes the
> -Wshadow flag by default, allowing the warning to be seen when building
> OVS with DPDK 21.05-rc2.
>
> Fixes: 245efe544d8e ("raw/ioat: report status of completed jobs")
>
> Reported-by: Sunil Pai G <sunil.pai.g@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Applied, thanks.