[PATCH] spi: tegra210-quad: Fix iterator outside loop

[PATCH] spi: tegra210-quad: Fix iterator outside loop

[Krishna Yarlagadda]

Fix warn: iterator used outside loop: 'xfer'
xfer variable may contain invalid value in few conditions.
Move transfer complete check within list loop.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Link:https://lore.kernel.org/all/202210191211.46FkzKmv-lkp@intel.com/
Fixes: 8777dd9dff40 ("spi: tegra210-quad: Fix combined sequence")

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index 95706153f340..2291effc2c44 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -1178,12 +1178,12 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
 			goto exit;
 		}
 		msg->actual_length += xfer->len;
+		if (!xfer->cs_change && transfer_phase == DATA_TRANSFER) {
+			tegra_qspi_transfer_end(spi);
+			spi_transfer_delay_exec(xfer);
+		}
 		transfer_phase++;
 	}
-	if (!xfer->cs_change) {
-		tegra_qspi_transfer_end(spi);
-		spi_transfer_delay_exec(xfer);
-	}
 	ret = 0;
 
 exit:
-- 
2.17.1

Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On Fri, Feb 24, 2023 at 10:05:13PM +0530, Krishna Yarlagadda wrote:

>  		msg->actual_length += xfer->len;
> +		if (!xfer->cs_change && transfer_phase == DATA_TRANSFER) {
> +			tegra_qspi_transfer_end(spi);
> +			spi_transfer_delay_exec(xfer);
> +		}
>  		transfer_phase++;
>  	}
> -	if (!xfer->cs_change) {
> -		tegra_qspi_transfer_end(spi);
> -		spi_transfer_delay_exec(xfer);
> -	}

This looks like it'll do the wrong thing and do a change on every
transfer if cs_change isn't set?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 24 February 2023 22:17
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> kernel@vger.kernel.org; thierry.reding@gmail.com; Jonathan Hunter
> <jonathanh@nvidia.com>; Sowjanya Komatineni
> <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>
> Subject: Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop
> 
> On Fri, Feb 24, 2023 at 10:05:13PM +0530, Krishna Yarlagadda wrote:
> 
> >  		msg->actual_length += xfer->len;
> > +		if (!xfer->cs_change && transfer_phase == DATA_TRANSFER)
> {
> > +			tegra_qspi_transfer_end(spi);
> > +			spi_transfer_delay_exec(xfer);
> > +		}
> >  		transfer_phase++;
> >  	}
> > -	if (!xfer->cs_change) {
> > -		tegra_qspi_transfer_end(spi);
> > -		spi_transfer_delay_exec(xfer);
> > -	}
> 
> This looks like it'll do the wrong thing and do a change on every
> transfer if cs_change isn't set?
This condition is hit only in data phase which is end of message.
KY

Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Fri, Feb 24, 2023 at 04:50:00PM +0000, Krishna Yarlagadda wrote:

> > >  		msg->actual_length += xfer->len;
> > > +		if (!xfer->cs_change && transfer_phase == DATA_TRANSFER)
> > {
> > > +			tegra_qspi_transfer_end(spi);
> > > +			spi_transfer_delay_exec(xfer);
> > > +		}
> > >  		transfer_phase++;
> > >  	}
> > > -	if (!xfer->cs_change) {
> > > -		tegra_qspi_transfer_end(spi);
> > > -		spi_transfer_delay_exec(xfer);
> > > -	}

> > This looks like it'll do the wrong thing and do a change on every
> > transfer if cs_change isn't set?

> This condition is hit only in data phase which is end of message.

Shouldn't this just be moved into the DATA_TRANSFER case statement?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 24 February 2023 23:26
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> kernel@vger.kernel.org; thierry.reding@gmail.com; Jonathan Hunter
> <jonathanh@nvidia.com>; Sowjanya Komatineni
> <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>
> Subject: Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop
> 
> On Fri, Feb 24, 2023 at 04:50:00PM +0000, Krishna Yarlagadda wrote:
> 
> > > >  		msg->actual_length += xfer->len;
> > > > +		if (!xfer->cs_change && transfer_phase == DATA_TRANSFER)
> > > {
> > > > +			tegra_qspi_transfer_end(spi);
> > > > +			spi_transfer_delay_exec(xfer);
> > > > +		}
> > > >  		transfer_phase++;
> > > >  	}
> > > > -	if (!xfer->cs_change) {
> > > > -		tegra_qspi_transfer_end(spi);
> > > > -		spi_transfer_delay_exec(xfer);
> > > > -	}
> updating the length of the message.
> > > This looks like it'll do the wrong thing and do a change on every
> > > transfer if cs_change isn't set?
> 
> > This condition is hit only in data phase which is end of message.
> 
> Shouldn't this just be moved into the DATA_TRANSFER case statement?
Calling transfer_end after updating message length.

Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On Mon, Feb 27, 2023 at 10:36:18AM +0000, Krishna Yarlagadda wrote:

> > > > > -	if (!xfer->cs_change) {
> > > > > -		tegra_qspi_transfer_end(spi);
> > > > > -		spi_transfer_delay_exec(xfer);
> > > > > -	}
> > updating the length of the message.
> > > > This looks like it'll do the wrong thing and do a change on every
> > > > transfer if cs_change isn't set?

> > > This condition is hit only in data phase which is end of message.

> > Shouldn't this just be moved into the DATA_TRANSFER case statement?
> Calling transfer_end after updating message length.

Something seems to be mangled with your quoting/new text here so it's a
bit unclear what you're saying here but if you're saying that this is
due to needing to call tegra_qspi_transfer_end() after updating the
length I'm not sure why - AFAICT that function doesn't reference the
transfer length at all, it just writes out a command to configure the
chip select?  There's no issue with the message being finalised since
that happens in the caller.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Krishna Yarlagadda]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 27 February 2023 23:53
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> kernel@vger.kernel.org; thierry.reding@gmail.com; Jonathan Hunter
> <jonathanh@nvidia.com>; Sowjanya Komatineni
> <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>
> Subject: Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop
> 
> On Mon, Feb 27, 2023 at 10:36:18AM +0000, Krishna Yarlagadda wrote:
> 
> > > > > > -	if (!xfer->cs_change) {
> > > > > > -		tegra_qspi_transfer_end(spi);
> > > > > > -		spi_transfer_delay_exec(xfer);
> > > > > > -	}
> > > updating the length of the message.
> > > > > This looks like it'll do the wrong thing and do a change on every
> > > > > transfer if cs_change isn't set?
> 
> > > > This condition is hit only in data phase which is end of message.
> 
> > > Shouldn't this just be moved into the DATA_TRANSFER case statement?
> > Calling transfer_end after updating message length.
> 
> Something seems to be mangled with your quoting/new text here so it's a
> bit unclear what you're saying here but if you're saying that this is
> due to needing to call tegra_qspi_transfer_end() after updating the
> length I'm not sure why - AFAICT that function doesn't reference the
> transfer length at all, it just writes out a command to configure the
> chip select?  There's no issue with the message being finalised since
> that happens in the caller.
Yes. Length should not matter. Will move into DATA_TRANSFER case.
Also need to handle error case.

Re: [PATCH] spi: tegra210-quad: Fix iterator outside loop

[Mark Brown]

On Fri, 24 Feb 2023 22:05:13 +0530, Krishna Yarlagadda wrote:
> Fix warn: iterator used outside loop: 'xfer'
> xfer variable may contain invalid value in few conditions.
> Move transfer complete check within list loop.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Link:https://lore.kernel.org/all/202210191211.46FkzKmv-lkp@intel.com/
> Fixes: 8777dd9dff40 ("spi: tegra210-quad: Fix combined sequence")
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: tegra210-quad: Fix iterator outside loop
      commit: 2449d436681d40bc63ec2c766fd51b632270d8a7

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark