| Message ID | 20201102112239.19218-1-zhang.lyra@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: add runtime PM for transfer_one_message | expand |
On Mon, Nov 02, 2020 at 07:22:39PM +0800, Chunyan Zhang wrote: > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > Before transfer message, spi devices probably have been in runtime suspended, > that would cause the kernel crash on some platforms once access spi > registers, such as on Unisoc's SoCs. The spi devices can be suspended > until message transfer completed. This commit message is a bit hard to follow so I don't really understand what the issue is. We only ever call transfer_one_message() from within __spi_pump_messages() which already handles auto_runtime_pm so I'm not seeing the situation where we might get to transfer_one_message() without having already runtime resumed the controller. What exactly is the error situation here? This code has been around for a while and I'm not aware of reports of issues here and I can't see anything unusual that the Spreadtrum driver is doing. Also why are we doing this in transfer_one_message() where it will only work for controllers using that? If we're missing runtime PM in some paths then presumably controllers with a custom implementation are also going to be affected as well, auto_runtime_pm is supposed to work for them as well.
On Tue, 3 Nov 2020 at 02:17, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Nov 02, 2020 at 07:22:39PM +0800, Chunyan Zhang wrote: > > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > Before transfer message, spi devices probably have been in runtime suspended, > > that would cause the kernel crash on some platforms once access spi > > registers, such as on Unisoc's SoCs. The spi devices can be suspended > > until message transfer completed. > > This commit message is a bit hard to follow so I don't really understand > what the issue is. We only ever call transfer_one_message() from within > __spi_pump_messages() which already handles auto_runtime_pm so I'm not > seeing the situation where we might get to transfer_one_message() > without having already runtime resumed the controller. What exactly is > the error situation here? This code has been around for a while and I'm > not aware of reports of issues here and I can't see anything unusual > that the Spreadtrum driver is doing. With further tests and looking into this part of code, the problem we found recently on our platform which runs kernel 4.14 can be fixed by this commit [1]. In a word, there's no issue here indeed, this patch should be ignored, sorry for the noise and thanks for your review. Chunyan [1] https://lkml.org/lkml/2019/10/30/173 > > Also why are we doing this in transfer_one_message() where it will only > work for controllers using that? If we're missing runtime PM in some > paths then presumably controllers with a custom implementation are also > going to be affected as well, auto_runtime_pm is supposed to work for > them as well.
On Tue, Nov 03, 2020 at 03:30:24PM +0800, Chunyan Zhang wrote: > On Tue, 3 Nov 2020 at 02:17, Mark Brown <broonie@kernel.org> wrote: > > the error situation here? This code has been around for a while and I'm > > not aware of reports of issues here and I can't see anything unusual > > that the Spreadtrum driver is doing. > With further tests and looking into this part of code, the problem we > found recently on our platform which runs kernel 4.14 can be fixed by > this commit [1]. > In a word, there's no issue here indeed, this patch should be > ignored, sorry for the noise and thanks for your review. Ah, great - glad it's sorted.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 0cab239d8e7f..63f7ebea7076 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1224,6 +1224,14 @@ static void _spi_transfer_cs_change_delay(struct spi_message *msg, } } +static void spi_idle_runtime_pm(struct spi_controller *ctlr) +{ + if (ctlr->auto_runtime_pm) { + pm_runtime_mark_last_busy(ctlr->dev.parent); + pm_runtime_put_autosuspend(ctlr->dev.parent); + } +} + /* * spi_transfer_one_message - Default implementation of transfer_one_message() * @@ -1240,6 +1248,16 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, struct spi_statistics *statm = &ctlr->statistics; struct spi_statistics *stats = &msg->spi->statistics; + if (ctlr->auto_runtime_pm) { + ret = pm_runtime_get_sync(ctlr->dev.parent); + if (ret < 0) { + pm_runtime_put_noidle(ctlr->dev.parent); + dev_err(&ctlr->dev, "Failed to power device: %d\n", + ret); + return ret; + } + } + spi_set_cs(msg->spi, true); SPI_STATISTICS_INCREMENT_FIELD(statm, messages); @@ -1329,6 +1347,8 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, spi_finalize_current_message(ctlr); + spi_idle_runtime_pm(ctlr); + return ret; } @@ -1346,14 +1366,6 @@ void spi_finalize_current_transfer(struct spi_controller *ctlr) } EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); -static void spi_idle_runtime_pm(struct spi_controller *ctlr) -{ - if (ctlr->auto_runtime_pm) { - pm_runtime_mark_last_busy(ctlr->dev.parent); - pm_runtime_put_autosuspend(ctlr->dev.parent); - } -} - /** * __spi_pump_messages - function which processes spi message queue * @ctlr: controller to process queue for