All of lore.kernel.org
 help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Mark Brown <broonie@kernel.org>, <linux-spi@vger.kernel.org>
Cc: Richard Weinberger <richard@nod.at>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Ryan Wanner <ryan.wanner@microchip.com>,
	Ronald Wahl <ronald.wahl@raritan.com>,
	David Laight <David.Laight@ACULAB.COM>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Claudiu Beznea <claudiu.beznea@tuxon.dev>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	<linux-arm-kernel@lists.infradead.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	stable@vger.kernel.org
Subject: [PATCH] spi: atmel: Prevent spi transfers from being killed
Date: Tue,  5 Dec 2023 09:31:02 +0100	[thread overview]
Message-ID: <20231205083102.16946-1-miquel.raynal@bootlin.com> (raw)

Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
long transfers") has tried to mitigate the problem of getting spi
transfers canceled because they were lasting too long. On slow buses,
transfers in the MiB range can take more than one second and thus a
calculation was added to progressively increment the timeout value. In
order to not be too problematic from a user point of view (waiting dozen
of seconds or even minutes), the wait call was turned interruptible.

Turning the wait interruptible was a mistake as what we really wanted to
do was to be able to kill a transfer. Any signal interrupting our
transfer would not be suitable at all so a second attempt was made at
turning the wait killable instead.

Link: https://lore.kernel.org/linux-spi/20231127095842.389631-1-miquel.raynal@bootlin.com/

All being well, it was reported that JFFS2 was showing a splat when
interrupting a transfer. After some more debate about whether JFFS2
should be fixed and how, it was also pointed out that the whole
consistency of the filesystem in case of parallel I/O would be
compromised. Changing JFFS2 behavior would in theory be possible but
nobody has the energy and time and knowledge to do this now, so better
prevent spi transfers to be interrupted by the user.

Partially revert the blamed commit to no longer use the interruptible
nor the killable variant of wait_for_completion().

Fixes: e0205d6203c2 ("spi: atmel: Prevent false timeouts on long transfers")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-atmel.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 0197c25f5029..54277de30161 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1333,12 +1333,10 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
 		}
 
 		dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
-		ret_timeout = wait_for_completion_killable_timeout(&as->xfer_completion,
-								   dma_timeout);
-		if (ret_timeout <= 0) {
-			dev_err(&spi->dev, "spi transfer %s\n",
-				!ret_timeout ? "timeout" : "canceled");
-			as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
+		ret_timeout = wait_for_completion_timeout(&as->xfer_completion, dma_timeout);
+		if (!ret_timeout) {
+			dev_err(&spi->dev, "spi transfer timeout\n");
+			as->done_status = -EIO;
 		}
 
 		if (as->done_status)
-- 
2.34.1


WARNING: multiple messages have this Message-ID (diff)
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Mark Brown <broonie@kernel.org>, <linux-spi@vger.kernel.org>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Ryan Wanner <ryan.wanner@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	stable@vger.kernel.org, Claudiu Beznea <claudiu.beznea@tuxon.dev>,
	David Laight <David.Laight@ACULAB.COM>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Ronald Wahl <ronald.wahl@raritan.com>,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH] spi: atmel: Prevent spi transfers from being killed
Date: Tue,  5 Dec 2023 09:31:02 +0100	[thread overview]
Message-ID: <20231205083102.16946-1-miquel.raynal@bootlin.com> (raw)

Upstream commit e0205d6203c2 ("spi: atmel: Prevent false timeouts on
long transfers") has tried to mitigate the problem of getting spi
transfers canceled because they were lasting too long. On slow buses,
transfers in the MiB range can take more than one second and thus a
calculation was added to progressively increment the timeout value. In
order to not be too problematic from a user point of view (waiting dozen
of seconds or even minutes), the wait call was turned interruptible.

Turning the wait interruptible was a mistake as what we really wanted to
do was to be able to kill a transfer. Any signal interrupting our
transfer would not be suitable at all so a second attempt was made at
turning the wait killable instead.

Link: https://lore.kernel.org/linux-spi/20231127095842.389631-1-miquel.raynal@bootlin.com/

All being well, it was reported that JFFS2 was showing a splat when
interrupting a transfer. After some more debate about whether JFFS2
should be fixed and how, it was also pointed out that the whole
consistency of the filesystem in case of parallel I/O would be
compromised. Changing JFFS2 behavior would in theory be possible but
nobody has the energy and time and knowledge to do this now, so better
prevent spi transfers to be interrupted by the user.

Partially revert the blamed commit to no longer use the interruptible
nor the killable variant of wait_for_completion().

Fixes: e0205d6203c2 ("spi: atmel: Prevent false timeouts on long transfers")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-atmel.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 0197c25f5029..54277de30161 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1333,12 +1333,10 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
 		}
 
 		dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
-		ret_timeout = wait_for_completion_killable_timeout(&as->xfer_completion,
-								   dma_timeout);
-		if (ret_timeout <= 0) {
-			dev_err(&spi->dev, "spi transfer %s\n",
-				!ret_timeout ? "timeout" : "canceled");
-			as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
+		ret_timeout = wait_for_completion_timeout(&as->xfer_completion, dma_timeout);
+		if (!ret_timeout) {
+			dev_err(&spi->dev, "spi transfer timeout\n");
+			as->done_status = -EIO;
 		}
 
 		if (as->done_status)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

             reply	other threads:[~2023-12-05  8:31 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-05  8:31 Miquel Raynal [this message]
2023-12-05  8:31 ` [PATCH] spi: atmel: Prevent spi transfers from being killed Miquel Raynal
2023-12-05  9:22 ` Richard Weinberger
2023-12-05  9:22   ` Richard Weinberger
2023-12-05  9:28   ` Miquel Raynal
2023-12-05  9:28     ` Miquel Raynal
2023-12-05  9:23 ` Ronald Wahl
2023-12-05  9:23   ` Ronald Wahl
2023-12-05 15:41 ` Mark Brown
2023-12-05 15:41   ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231205083102.16946-1-miquel.raynal@bootlin.com \
    --to=miquel.raynal@bootlin.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=alexandre.belloni@bootlin.com \
    --cc=broonie@kernel.org \
    --cc=claudiu.beznea@tuxon.dev \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=nicolas.ferre@microchip.com \
    --cc=richard@nod.at \
    --cc=ronald.wahl@raritan.com \
    --cc=ryan.wanner@microchip.com \
    --cc=stable@vger.kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.