I recently looked at some crash reports on ChromeOS devices that call
into this intel_scu_ipc driver. They were hitting timeouts, and it
certainly looks possible for those timeouts to be triggering because of
scheduling issues. Once things started going south, the timeouts kept
coming. Maybe that's because the other side got seriously confused? I
don't know.
I added some sleeps to these paths to trigger the timeout behavior to
make sure the code works. Simply sleeping for a long time in busy_loop()
hits the timeout, which could happen if the system is scheduling lots of
other things at the time.
I couldn't really test the last patch because forcing a timeout or
returning immediately wasn't fast enough to trigger the second
transaction to run into the first one being processed.
Changes from v3 (https://lore.kernel.org/r/[email protected]):
* Use readx_poll_timeout() to shorten a line
Changes from v2 (https://lore.kernel.org/r/[email protected]):
* Use read_poll_timeout() helper in patch #1 (again)
* New patch #3 to fix bug pointed out by Andy
* Consolidate more code into busy check in patch #4
Changes from v1 (https://lore.kernel.org/r/[email protected]):
* Don't use read_poll_timeout() helper in patch 1, just add code
* Rewrite patch 2 to be simpler
* Make intel_scu_ipc_busy() return -EBUSY when busy
* Downgrade dev_err() to dev_dbg() in intel_scu_ipc_busy()
Stephen Boyd (4):
platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()
platform/x86: intel_scu_ipc: Check status upon timeout in
ipc_wait_for_interrupt()
platform/x86: intel_scu_ipc: Don't override scu in
intel_scu_ipc_dev_simple_command()
platform/x86: intel_scu_ipc: Fail IPC send if still busy
drivers/platform/x86/intel_scu_ipc.c | 66 +++++++++++++++++-----------
1 file changed, 40 insertions(+), 26 deletions(-)
Cc: Prashant Malani <[email protected]>
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
--
https://chromeos.dev
Andy discovered this bug during patch review. The 'scu' argument to this
function shouldn't be overridden by the function itself. It doesn't make
any sense. Looking at the commit history, we see that commit
f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
removed the setting of the scu to ipcdev in other functions, but not
this one. That was an oversight. Remove this line so that we stop
overriding the scu instance that is used by this function.
Reported-by: Andy Shevchenko <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]
Cc: Prashant Malani <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Fixes: f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/platform/x86/intel_scu_ipc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 299c15312acb..3271f81a9c00 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -443,7 +443,6 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
mutex_unlock(&ipclock);
return -ENODEV;
}
- scu = ipcdev;
cmdval = sub << 12 | cmd;
ipc_command(scu, cmdval);
err = intel_scu_ipc_check_status(scu);
--
https://chromeos.dev
It's possible for the polling loop in busy_loop() to get scheduled away
for a long time.
status = ipc_read_status(scu); // status = IPC_STATUS_BUSY
<long time scheduled away>
if (!(status & IPC_STATUS_BUSY))
If this happens, then the status bit could change while the task is
scheduled away and this function would never read the status again after
timing out. Instead, the function will return -ETIMEDOUT when it's
possible that scheduling didn't work out and the status bit was cleared.
Bit polling code should always check the bit being polled one more time
after the timeout in case this happens.
Fix this by reading the status once more after the while loop breaks.
The readl_poll_timeout() macro implements all of this, and it is
shorter, so use that macro here to consolidate code and fix this.
There were some concerns with using readl_poll_timeout() because it uses
timekeeping, and timekeeping isn't running early on or during the late
stages of system suspend or early stages of system resume, but an audit
of the code concluded that this code isn't called during those times so
it is safe to use the macro.
Cc: Prashant Malani <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/platform/x86/intel_scu_ipc.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 6851d10d6582..4c774ee8bb1b 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -19,6 +19,7 @@
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/slab.h>
@@ -231,19 +232,15 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
/* Wait till scu status is busy */
static inline int busy_loop(struct intel_scu_ipc_dev *scu)
{
- unsigned long end = jiffies + IPC_TIMEOUT;
+ u8 status;
+ int err;
- do {
- u32 status;
+ err = readx_poll_timeout(ipc_read_status, scu, status, !(status & IPC_STATUS_BUSY),
+ 100, jiffies_to_usecs(IPC_TIMEOUT));
+ if (err)
+ return err;
- status = ipc_read_status(scu);
- if (!(status & IPC_STATUS_BUSY))
- return (status & IPC_STATUS_ERR) ? -EIO : 0;
-
- usleep_range(50, 100);
- } while (time_before(jiffies, end));
-
- return -ETIMEDOUT;
+ return (status & IPC_STATUS_ERR) ? -EIO : 0;
}
/* Wait till ipc ioc interrupt is received or timeout in 10 HZ */
--
https://chromeos.dev
It's possible for interrupts to get significantly delayed to the point
that callers of intel_scu_ipc_dev_command() and friends can call the
function once, hit a timeout, and call it again while the interrupt
still hasn't been processed. This driver will get seriously confused if
the interrupt is finally processed after the second IPC has been sent
with ipc_command(). It won't know which IPC has been completed. This
could be quite disastrous if calling code assumes something has happened
upon return from intel_scu_ipc_dev_simple_command() when it actually
hasn't.
Let's avoid this scenario by simply returning -EBUSY in this case.
Hopefully higher layers will know to back off or fail gracefully when
this happens. It's all highly unlikely anyway, but it's better to be
correct here as we have no way to know which IPC the status register is
telling us about if we send a second IPC while the previous IPC is still
processing.
Cc: Prashant Malani <[email protected]>
Cc: Kuppuswamy Sathyanarayanan <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/platform/x86/intel_scu_ipc.c | 40 +++++++++++++++++++---------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 3271f81a9c00..a68df4133403 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -265,6 +265,24 @@ static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
return scu->irq > 0 ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
}
+static struct intel_scu_ipc_dev *intel_scu_ipc_get(struct intel_scu_ipc_dev *scu)
+{
+ u8 status;
+
+ if (!scu)
+ scu = ipcdev;
+ if (!scu)
+ return ERR_PTR(-ENODEV);
+
+ status = ipc_read_status(scu);
+ if (status & IPC_STATUS_BUSY) {
+ dev_dbg(&scu->dev, "device is busy\n");
+ return ERR_PTR(-EBUSY);
+ }
+
+ return scu;
+}
+
/* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */
static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
u32 count, u32 op, u32 id)
@@ -278,11 +296,10 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
memset(cbuf, 0, sizeof(cbuf));
mutex_lock(&ipclock);
- if (!scu)
- scu = ipcdev;
- if (!scu) {
+ scu = intel_scu_ipc_get(scu);
+ if (IS_ERR(scu)) {
mutex_unlock(&ipclock);
- return -ENODEV;
+ return PTR_ERR(scu);
}
for (nc = 0; nc < count; nc++, offset += 2) {
@@ -437,12 +454,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
int err;
mutex_lock(&ipclock);
- if (!scu)
- scu = ipcdev;
- if (!scu) {
+ scu = intel_scu_ipc_get(scu);
+ if (IS_ERR(scu)) {
mutex_unlock(&ipclock);
- return -ENODEV;
+ return PTR_ERR(scu);
}
+
cmdval = sub << 12 | cmd;
ipc_command(scu, cmdval);
err = intel_scu_ipc_check_status(scu);
@@ -482,11 +499,10 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
return -EINVAL;
mutex_lock(&ipclock);
- if (!scu)
- scu = ipcdev;
- if (!scu) {
+ scu = intel_scu_ipc_get(scu);
+ if (IS_ERR(scu)) {
mutex_unlock(&ipclock);
- return -ENODEV;
+ return PTR_ERR(scu);
}
memcpy(inbuf, in, inlen);
--
https://chromeos.dev
On Wed, 13 Sep 2023, Stephen Boyd wrote:
> It's possible for interrupts to get significantly delayed to the point
> that callers of intel_scu_ipc_dev_command() and friends can call the
> function once, hit a timeout, and call it again while the interrupt
> still hasn't been processed. This driver will get seriously confused if
> the interrupt is finally processed after the second IPC has been sent
> with ipc_command(). It won't know which IPC has been completed. This
> could be quite disastrous if calling code assumes something has happened
> upon return from intel_scu_ipc_dev_simple_command() when it actually
> hasn't.
>
> Let's avoid this scenario by simply returning -EBUSY in this case.
> Hopefully higher layers will know to back off or fail gracefully when
> this happens. It's all highly unlikely anyway, but it's better to be
> correct here as we have no way to know which IPC the status register is
> telling us about if we send a second IPC while the previous IPC is still
> processing.
>
> Cc: Prashant Malani <[email protected]>
> Cc: Kuppuswamy Sathyanarayanan <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
> ---
> drivers/platform/x86/intel_scu_ipc.c | 40 +++++++++++++++++++---------
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 3271f81a9c00..a68df4133403 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -265,6 +265,24 @@ static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
> return scu->irq > 0 ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
> }
>
> +static struct intel_scu_ipc_dev *intel_scu_ipc_get(struct intel_scu_ipc_dev *scu)
> +{
> + u8 status;
> +
> + if (!scu)
> + scu = ipcdev;
> + if (!scu)
> + return ERR_PTR(-ENODEV);
> +
> + status = ipc_read_status(scu);
> + if (status & IPC_STATUS_BUSY) {
> + dev_dbg(&scu->dev, "device is busy\n");
> + return ERR_PTR(-EBUSY);
> + }
> +
> + return scu;
> +}
> +
> /* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */
> static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
> u32 count, u32 op, u32 id)
> @@ -278,11 +296,10 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
> memset(cbuf, 0, sizeof(cbuf));
>
> mutex_lock(&ipclock);
> - if (!scu)
> - scu = ipcdev;
> - if (!scu) {
> + scu = intel_scu_ipc_get(scu);
> + if (IS_ERR(scu)) {
> mutex_unlock(&ipclock);
> - return -ENODEV;
> + return PTR_ERR(scu);
> }
>
> for (nc = 0; nc < count; nc++, offset += 2) {
> @@ -437,12 +454,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
> int err;
>
> mutex_lock(&ipclock);
> - if (!scu)
> - scu = ipcdev;
> - if (!scu) {
> + scu = intel_scu_ipc_get(scu);
> + if (IS_ERR(scu)) {
> mutex_unlock(&ipclock);
> - return -ENODEV;
> + return PTR_ERR(scu);
> }
> +
> cmdval = sub << 12 | cmd;
> ipc_command(scu, cmdval);
> err = intel_scu_ipc_check_status(scu);
> @@ -482,11 +499,10 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
> return -EINVAL;
>
> mutex_lock(&ipclock);
> - if (!scu)
> - scu = ipcdev;
> - if (!scu) {
> + scu = intel_scu_ipc_get(scu);
> + if (IS_ERR(scu)) {
> mutex_unlock(&ipclock);
> - return -ENODEV;
> + return PTR_ERR(scu);
> }
>
> memcpy(inbuf, in, inlen);
>
On Wed, 13 Sep 2023, Stephen Boyd wrote:
> It's possible for the polling loop in busy_loop() to get scheduled away
> for a long time.
>
> status = ipc_read_status(scu); // status = IPC_STATUS_BUSY
> <long time scheduled away>
> if (!(status & IPC_STATUS_BUSY))
>
> If this happens, then the status bit could change while the task is
> scheduled away and this function would never read the status again after
> timing out. Instead, the function will return -ETIMEDOUT when it's
> possible that scheduling didn't work out and the status bit was cleared.
> Bit polling code should always check the bit being polled one more time
> after the timeout in case this happens.
>
> Fix this by reading the status once more after the while loop breaks.
> The readl_poll_timeout() macro implements all of this, and it is
> shorter, so use that macro here to consolidate code and fix this.
>
> There were some concerns with using readl_poll_timeout() because it uses
> timekeeping, and timekeeping isn't running early on or during the late
> stages of system suspend or early stages of system resume, but an audit
> of the code concluded that this code isn't called during those times so
> it is safe to use the macro.
>
> Cc: Prashant Malani <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
> ---
> drivers/platform/x86/intel_scu_ipc.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 6851d10d6582..4c774ee8bb1b 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -19,6 +19,7 @@
> #include <linux/init.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/slab.h>
>
> @@ -231,19 +232,15 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
> /* Wait till scu status is busy */
> static inline int busy_loop(struct intel_scu_ipc_dev *scu)
> {
> - unsigned long end = jiffies + IPC_TIMEOUT;
> + u8 status;
> + int err;
>
> - do {
> - u32 status;
> + err = readx_poll_timeout(ipc_read_status, scu, status, !(status & IPC_STATUS_BUSY),
> + 100, jiffies_to_usecs(IPC_TIMEOUT));
> + if (err)
> + return err;
>
> - status = ipc_read_status(scu);
> - if (!(status & IPC_STATUS_BUSY))
> - return (status & IPC_STATUS_ERR) ? -EIO : 0;
> -
> - usleep_range(50, 100);
> - } while (time_before(jiffies, end));
> -
> - return -ETIMEDOUT;
> + return (status & IPC_STATUS_ERR) ? -EIO : 0;
> }
>
> /* Wait till ipc ioc interrupt is received or timeout in 10 HZ */
>
On Wed, 13 Sep 2023, Stephen Boyd wrote:
> Andy discovered this bug during patch review. The 'scu' argument to this
> function shouldn't be overridden by the function itself. It doesn't make
> any sense. Looking at the commit history, we see that commit
> f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
> removed the setting of the scu to ipcdev in other functions, but not
> this one. That was an oversight. Remove this line so that we stop
> overriding the scu instance that is used by this function.
>
> Reported-by: Andy Shevchenko <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]
This looks somewhat unusual way to tag it. I'd just drop the Closes tag
as the email list is not a bug tracter.
Other than that,
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
> Cc: Prashant Malani <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Fixes: f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/platform/x86/intel_scu_ipc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 299c15312acb..3271f81a9c00 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -443,7 +443,6 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
> mutex_unlock(&ipclock);
> return -ENODEV;
> }
> - scu = ipcdev;
> cmdval = sub << 12 | cmd;
> ipc_command(scu, cmdval);
> err = intel_scu_ipc_check_status(scu);
>
On Fri, Sep 15, 2023 at 05:45:42PM +0300, Ilpo J?rvinen wrote:
> On Wed, 13 Sep 2023, Stephen Boyd wrote:
>
> > Andy discovered this bug during patch review. The 'scu' argument to this
> > function shouldn't be overridden by the function itself. It doesn't make
> > any sense. Looking at the commit history, we see that commit
> > f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
> > removed the setting of the scu to ipcdev in other functions, but not
> > this one. That was an oversight. Remove this line so that we stop
> > overriding the scu instance that is used by this function.
> >
> > Reported-by: Andy Shevchenko <[email protected]>
> > Closes: https://lore.kernel.org/r/[email protected]
>
> This looks somewhat unusual way to tag it. I'd just drop the Closes tag
> as the email list is not a bug tracter.
This is a new requirement enforced by checkpatch.pl. If commit message has
the Reported-by: tag it should have Closes: one as well.
--
With Best Regards,
Andy Shevchenko
Hi,
On 9/13/23 23:27, Stephen Boyd wrote:
> I recently looked at some crash reports on ChromeOS devices that call
> into this intel_scu_ipc driver. They were hitting timeouts, and it
> certainly looks possible for those timeouts to be triggering because of
> scheduling issues. Once things started going south, the timeouts kept
> coming. Maybe that's because the other side got seriously confused? I
> don't know.
>
> I added some sleeps to these paths to trigger the timeout behavior to
> make sure the code works. Simply sleeping for a long time in busy_loop()
> hits the timeout, which could happen if the system is scheduling lots of
> other things at the time.
>
> I couldn't really test the last patch because forcing a timeout or
> returning immediately wasn't fast enough to trigger the second
> transaction to run into the first one being processed.
Thank you for your patch, I've applied this patch to my fixes
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes
Note it will show up in my fixes branch once I've pushed my
local branch there, which might take a while.
I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
> Changes from v3 (https://lore.kernel.org/r/[email protected]):
> * Use readx_poll_timeout() to shorten a line
>
> Changes from v2 (https://lore.kernel.org/r/[email protected]):
> * Use read_poll_timeout() helper in patch #1 (again)
> * New patch #3 to fix bug pointed out by Andy
> * Consolidate more code into busy check in patch #4
>
> Changes from v1 (https://lore.kernel.org/r/[email protected]):
> * Don't use read_poll_timeout() helper in patch 1, just add code
> * Rewrite patch 2 to be simpler
> * Make intel_scu_ipc_busy() return -EBUSY when busy
> * Downgrade dev_err() to dev_dbg() in intel_scu_ipc_busy()
>
> Stephen Boyd (4):
> platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()
> platform/x86: intel_scu_ipc: Check status upon timeout in
> ipc_wait_for_interrupt()
> platform/x86: intel_scu_ipc: Don't override scu in
> intel_scu_ipc_dev_simple_command()
> platform/x86: intel_scu_ipc: Fail IPC send if still busy
>
> drivers/platform/x86/intel_scu_ipc.c | 66 +++++++++++++++++-----------
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> Cc: Prashant Malani <[email protected]>
>
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c