[PATCH v2] spi: spi-mtk-nor: fix timeout calculation overflow

[PATCH v2] spi: spi-mtk-nor: fix timeout calculation overflow

[Chuanhong Guo]

CLK_TO_US macro is used to calculate potential transfer time for various
timeout handling. However it overflows on transfer bigger than 512 bytes
because it first did (len * 8 * 1000000).
This controller typically operates at 45MHz. This patch did 2 things:
1. calculate clock / 1000000 first
2. add a 4M transfer size cap so that the final timeout in DMA reading
   doesn't overflow

Fixes: 881d1ee9fe81f ("spi: add support for mediatek spi-nor controller")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---

Change since v1: fix transfer size cap to 4M

 drivers/spi/spi-mtk-nor.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 6e6ca2b8e6c82..62f5ff2779884 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -89,7 +89,7 @@
 // Buffered page program can do one 128-byte transfer
 #define MTK_NOR_PP_SIZE			128
 
-#define CLK_TO_US(sp, clkcnt)		((clkcnt) * 1000000 / sp->spi_freq)
+#define CLK_TO_US(sp, clkcnt)		DIV_ROUND_UP(clkcnt, sp->spi_freq / 1000000)
 
 struct mtk_nor {
 	struct spi_controller *ctlr;
@@ -177,6 +177,10 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 	if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
 		if ((op->data.dir == SPI_MEM_DATA_IN) &&
 		    mtk_nor_match_read(op)) {
+			// limit size to prevent timeout calculation overflow
+			if (op->data.nbytes > 0x400000)
+				op->data.nbytes = 0x400000;
+
 			if ((op->addr.val & MTK_NOR_DMA_ALIGN_MASK) ||
 			    (op->data.nbytes < MTK_NOR_DMA_ALIGN))
 				op->data.nbytes = 1;
-- 
2.26.2

Re: [PATCH v2] spi: spi-mtk-nor: fix timeout calculation overflow

[Mark Brown]

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

On Tue, Sep 22, 2020 at 07:49:02PM +0800, Chuanhong Guo wrote:

>  		if ((op->data.dir == SPI_MEM_DATA_IN) &&
>  		    mtk_nor_match_read(op)) {
> +			// limit size to prevent timeout calculation overflow
> +			if (op->data.nbytes > 0x400000)
> +				op->data.nbytes = 0x400000;

If there's a limit on transfer sizes there should also be a
max_transfer_size or max_message_size set (which we should pay attention
to in the core for flash stuff but IIRC we didn't do that yet).

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

Re: [PATCH v2] spi: spi-mtk-nor: fix timeout calculation overflow

[Chuanhong Guo]

Hi!

On Tue, Sep 22, 2020 at 8:02 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Sep 22, 2020 at 07:49:02PM +0800, Chuanhong Guo wrote:
>
> >               if ((op->data.dir == SPI_MEM_DATA_IN) &&
> >                   mtk_nor_match_read(op)) {
> > +                     // limit size to prevent timeout calculation overflow
> > +                     if (op->data.nbytes > 0x400000)
> > +                             op->data.nbytes = 0x400000;
>
> If there's a limit on transfer sizes there should also be a
> max_transfer_size or max_message_size set (which we should pay attention
> to in the core for flash stuff but IIRC we didn't do that yet).

There's already a 6-byte max_message_size limit on this controller.
spi-mem dma read is the only operation which allows such a long transfer.

-- 
Regards,
Chuanhong Guo

Re: [PATCH v2] spi: spi-mtk-nor: fix timeout calculation overflow

[Chuanhong Guo]

On Tue, Sep 22, 2020 at 8:02 PM Mark Brown <broonie@kernel.org> wrote:
> (which we should pay attention to in the core for flash
> stuff but IIRC we didn't do that yet).

BTW we do have that taken care. spi_mem_adjust_op_size will
adjust the transfer size according to max_transfer/message_size
if no custom adjust_op_size hook is defined in the driver. If a custom
adjust_op_size is defined, the driver adjusts the transfer size for it's
exec_op hook. The size limit between exec_op and transfer_one_message
can be different. (this spi-mtk-nor is an example of that.)
-- 
Regards,
Chuanhong Guo

Re: [PATCH v2] spi: spi-mtk-nor: fix timeout calculation overflow

[Mark Brown]

On Tue, 22 Sep 2020 19:49:02 +0800, Chuanhong Guo wrote:
> CLK_TO_US macro is used to calculate potential transfer time for various
> timeout handling. However it overflows on transfer bigger than 512 bytes
> because it first did (len * 8 * 1000000).
> This controller typically operates at 45MHz. This patch did 2 things:
> 1. calculate clock / 1000000 first
> 2. add a 4M transfer size cap so that the final timeout in DMA reading
>    doesn't overflow

Applied to

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

Thanks!

[1/1] spi: spi-mtk-nor: fix timeout calculation overflow
      commit: 4cafaddedb5fbef9531202ee547784409fd0de33

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