| Message ID | 20230224163513.27290-1-kyarlagadda@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: tegra210-quad: Fix iterator outside loop | expand |
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 <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
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 <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.
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 <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.
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
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:
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(-)