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 <[email protected]>
Link:https://lore.kernel.org/all/[email protected]/
Fixes: 8777dd9dff40 ("spi: tegra210-quad: Fix combined sequence")
Signed-off-by: Krishna Yarlagadda <[email protected]>
---
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
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?
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: 24 February 2023 22:17
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Jonathan Hunter
> <[email protected]>; Sowjanya Komatineni
> <[email protected]>; Laxman Dewangan <[email protected]>
> 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
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?
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: 24 February 2023 23:26
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Jonathan Hunter
> <[email protected]>; Sowjanya Komatineni
> <[email protected]>; Laxman Dewangan <[email protected]>
> 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.
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.
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: 27 February 2023 23:53
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Jonathan Hunter
> <[email protected]>; Sowjanya Komatineni
> <[email protected]>; Laxman Dewangan <[email protected]>
> 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.
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 <[email protected]>
> Link:https://lore.kernel.org/all/[email protected]/
> 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