| Message ID | ed989af6-da88-4e0b-9ed8-126db6cad2e4@nskint.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] spi: spi-cadence-quadspi: Fix division by zero warning | expand |
On 15/07/21 04:21PM, Yoshitaka Ikeda wrote: > Fix below division by zero warning: > - Added an if statement because buswidth can be zero, resulting in division by zero. > - The modified code was based on another driver (atmel-quadspi). > > [ 0.795337] Division by zero in kernel. > : > [ 0.834051] [<807fd40c>] (__div0) from [<804e1acc>] (Ldiv0+0x8/0x10) > [ 0.839097] [<805f0710>] (cqspi_exec_mem_op) from [<805edb4c>] (spi_mem_exec_op+0x3b0/0x3f8) Please add a fixes tag. Fixes: 7512eaf54190 ("spi: cadence-quadspi: Fix dummy cycle calculation when buswidth > 1") > > Signed-off-by: Yoshitaka Ikeda <ikeda@nskint.co.jp> > --- > v2: > - Fix commit message. > v3: > - repost. > > drivers/spi/spi-cadence-quadspi.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index 7a00346ff9b9..13d1f0ce618e 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -307,11 +307,13 @@ static unsigned int cqspi_calc_rdreg(struct cqspi_flash_pdata *f_pdata) > > static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr) > { > - unsigned int dummy_clk; > + unsigned int dummy_clk = 0; I think you can just do: if (!op->dummy.nbytes) return 0; and leave the rest same. I don't think we should have to check for buswidth here, even though it is the one causing divide-by-zero. Any op with positive dummy nbytes but with 0 buswidth is invalid. That should be rejected by the SPI MEM core or the supports_op(), so it should never even get here. > > - dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); > - if (dtr) > - dummy_clk /= 2; > + if (op->dummy.buswidth && op->dummy.nbytes) { > + dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); > + if (dtr) > + dummy_clk /= 2; > + } > > return dummy_clk; > } > -- > 2.32.0
On 2021/07/16 3:52, Pratyush Yadav wrote: > On 15/07/21 04:21PM, Yoshitaka Ikeda wrote: >> Fix below division by zero warning: >> - Added an if statement because buswidth can be zero, resulting in division by zero. >> - The modified code was based on another driver (atmel-quadspi). >> >> [ 0.795337] Division by zero in kernel. >> : >> [ 0.834051] [<807fd40c>] (__div0) from [<804e1acc>] (Ldiv0+0x8/0x10) >> [ 0.839097] [<805f0710>] (cqspi_exec_mem_op) from [<805edb4c>] (spi_mem_exec_op+0x3b0/0x3f8) > > Please add a fixes tag. > > Fixes: 7512eaf54190 ("spi: cadence-quadspi: Fix dummy cycle calculation when buswidth > 1") > >> >> Signed-off-by: Yoshitaka Ikeda <ikeda@nskint.co.jp> >> --- >> v2: >> - Fix commit message. >> v3: >> - repost. >> >> drivers/spi/spi-cadence-quadspi.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c >> index 7a00346ff9b9..13d1f0ce618e 100644 >> --- a/drivers/spi/spi-cadence-quadspi.c >> +++ b/drivers/spi/spi-cadence-quadspi.c >> @@ -307,11 +307,13 @@ static unsigned int cqspi_calc_rdreg(struct cqspi_flash_pdata *f_pdata) >> >> static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr) >> { >> - unsigned int dummy_clk; >> + unsigned int dummy_clk = 0; > > I think you can just do: > > if (!op->dummy.nbytes) > return 0; > > and leave the rest same. > > I don't think we should have to check for buswidth here, even though it > is the one causing divide-by-zero. Any op with positive dummy nbytes but > with 0 buswidth is invalid. That should be rejected by the SPI MEM core > or the supports_op(), so it should never even get here. Thanks for the detailed explanation. I understand now. I will fix it as you suggested. >> >> - dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); >> - if (dtr) >> - dummy_clk /= 2; >> + if (op->dummy.buswidth && op->dummy.nbytes) { >> + dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); >> + if (dtr) >> + dummy_clk /= 2; >> + } >> >> return dummy_clk; >> } >> -- >> 2.32.0 >
Hi Mark, On 15/07/21 08:53PM, Mark Brown wrote: > On Thu, 15 Jul 2021 16:21:32 +0000, Yoshitaka Ikeda wrote: > > Fix below division by zero warning: > > - Added an if statement because buswidth can be zero, resulting in division by zero. > > - The modified code was based on another driver (atmel-quadspi). > > > > [ 0.795337] Division by zero in kernel. > > : > > [ 0.834051] [<807fd40c>] (__div0) from [<804e1acc>] (Ldiv0+0x8/0x10) > > [ 0.839097] [<805f0710>] (cqspi_exec_mem_op) from [<805edb4c>] (spi_mem_exec_op+0x3b0/0x3f8) > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > > Thanks! > > [1/1] spi: spi-cadence-quadspi: Fix division by zero warning > commit: 55cef88bbf12f3bfbe5c2379a8868a034707e755 FYI, I commented on the patch yesterday and Yoshitaka re-rolled it [0]. Your call whether you want to replace this patch with v4. In either case, I think this patch should be backported to stable so it needs a Fixes tag (which v4 has). [0] https://lore.kernel.org/linux-spi/958bb93b-db83-b685-5fa1-8e5dc40a5975@nskint.co.jp/ > > 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 -- Regards, Pratyush Yadav Texas Instruments Inc.
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 7a00346ff9b9..13d1f0ce618e 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -307,11 +307,13 @@ static unsigned int cqspi_calc_rdreg(struct cqspi_flash_pdata *f_pdata) static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr) { - unsigned int dummy_clk; + unsigned int dummy_clk = 0; - dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); - if (dtr) - dummy_clk /= 2; + if (op->dummy.buswidth && op->dummy.nbytes) { + dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); + if (dtr) + dummy_clk /= 2; + } return dummy_clk; }
Fix below division by zero warning: - Added an if statement because buswidth can be zero, resulting in division by zero. - The modified code was based on another driver (atmel-quadspi). [ 0.795337] Division by zero in kernel. : [ 0.834051] [<807fd40c>] (__div0) from [<804e1acc>] (Ldiv0+0x8/0x10) [ 0.839097] [<805f0710>] (cqspi_exec_mem_op) from [<805edb4c>] (spi_mem_exec_op+0x3b0/0x3f8) Signed-off-by: Yoshitaka Ikeda <ikeda@nskint.co.jp> --- v2: - Fix commit message. v3: - repost. drivers/spi/spi-cadence-quadspi.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)