Date:   Thu, 16 Mar 2023 12:20:48 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Tom Rix <trix@redhat.com>
Cc:     andreas.noever@gmail.com, michael.jamet@intel.com,
        YehezkelShB@gmail.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Mario Limonciello <mario.limonciello@amd.com>
Subject: Re: [PATCH] thunderbolt: rename shadowed variables bit to
 interrupt_bit and auto_clear_bit
Message-ID: <20230316102048.GR62143@black.fi.intel.com>
References: <20230315220450.1470815-1-trix@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230315220450.1470815-1-trix@redhat.com>
Precedence: bulk

+Cc Mario

On Wed, Mar 15, 2023 at 06:04:50PM -0400, Tom Rix wrote:
> cppcheck reports
> drivers/thunderbolt/nhi.c:74:7: style: Local variable 'bit' shadows outer variable [shadowVariable]
>   int bit;
>       ^
> drivers/thunderbolt/nhi.c:66:6: note: Shadowed declaration
>  int bit = ring_interrupt_index(ring) & 31;
>      ^
> drivers/thunderbolt/nhi.c:74:7: note: Shadow variable
>   int bit;
>       ^
> For readablity rename the outer to interrupt_bit and the innner
> to auto_clear_bit.

Thanks for the patch! Yeah, this did not show up in any of the kbuild
tests perhaps they are missing cppcheck :(

I'm thinking that I'll just move the two commits from "fixes" to "next"
and add this one on top (and drop the stable tags) as the code that we
should be sending to stable should not need additional fixes IMHO. I
know Mario is on vacation so probably cannot answer here so let's deal
with this when he is back.

> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/thunderbolt/nhi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 318d20bd5b69..d0d26d288be8 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -63,15 +63,15 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
>  {
>  	int reg = REG_RING_INTERRUPT_BASE +
>  		  ring_interrupt_index(ring) / 32 * 4;
> -	int bit = ring_interrupt_index(ring) & 31;
> -	int mask = 1 << bit;
> +	int interrupt_bit = ring_interrupt_index(ring) & 31;
> +	int mask = 1 << interrupt_bit;
>  	u32 old, new;
>  
>  	if (ring->irq > 0) {
>  		u32 step, shift, ivr, misc;
>  		void __iomem *ivr_base;
> +		int auto_clear_bit;
>  		int index;
> -		int bit;
>  
>  		if (ring->is_tx)
>  			index = ring->hop;
> @@ -91,11 +91,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
>  		 */
>  		misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
>  		if (ring->nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> -			bit = REG_DMA_MISC_INT_AUTO_CLEAR;
> +			auto_clear_bit = REG_DMA_MISC_INT_AUTO_CLEAR;
>  		else
> -			bit = REG_DMA_MISC_DISABLE_AUTO_CLEAR;
> -		if (!(misc & bit))
> -			iowrite32(misc | bit, ring->nhi->iobase + REG_DMA_MISC);
> +			auto_clear_bit = REG_DMA_MISC_DISABLE_AUTO_CLEAR;
> +		if (!(misc & auto_clear_bit))
> +			iowrite32(misc | auto_clear_bit, ring->nhi->iobase + REG_DMA_MISC);
>  
>  		ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE;
>  		step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
> @@ -115,7 +115,7 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
>  
>  	dev_dbg(&ring->nhi->pdev->dev,
>  		"%s interrupt at register %#x bit %d (%#x -> %#x)\n",
> -		active ? "enabling" : "disabling", reg, bit, old, new);
> +		active ? "enabling" : "disabling", reg, interrupt_bit, old, new);
>  
>  	if (new == old)
>  		dev_WARN(&ring->nhi->pdev->dev,
> -- 
> 2.27.0