[dpdk-dev] [PATCH] raw/ioat: fix parameter shadow warning

[dpdk-dev] [PATCH] raw/ioat: fix parameter shadow warning

[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 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

Re: [dpdk-dev] [PATCH] raw/ioat: fix parameter shadow warning

[Pai G, Sunil]

<snipped>

Tested-by: Sunil Pai G <sunil.pai.g@intel.com>

Re: [dpdk-dev] [PATCH] raw/ioat: fix parameter shadow warning

[Bruce Richardson]

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?

Re: [dpdk-dev] [PATCH] raw/ioat: fix parameter shadow warning

[Laatz, Kevin]

> 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.

[dpdk-dev] [PATCH v2] raw/ioat: fix parameter shadow warning

[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 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

Re: [dpdk-dev] [PATCH v2] raw/ioat: fix parameter shadow warning

[Bruce Richardson]

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>

Re: [dpdk-dev] [PATCH v2] raw/ioat: fix parameter shadow warning

[David Marchand]

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

Re: [dpdk-dev] [PATCH v2] raw/ioat: fix parameter shadow warning

[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.

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

Re: [dpdk-dev] [PATCH v2] raw/ioat: fix parameter shadow warning

[Thomas Monjalon]

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.

[dpdk-dev] [PATCH v3] raw/ioat: fix parameter shadow warning

[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>

---
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

Re: [dpdk-dev] [PATCH v3] raw/ioat: fix parameter shadow warning

[Thomas Monjalon]

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.