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 <pmalani@chromium.org>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/platform/x86/intel_scu_ipc.c | 29 ++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 3cea701d2bbd..8be24f48a0fa 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -271,6 +271,19 @@ 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 int intel_scu_ipc_busy(struct intel_scu_ipc_dev *scu)
+{
+	u8 status;
+
+	status = ipc_read_status(scu);
+	if (status & IPC_STATUS_BUSY) {
+		dev_dbg(&scu->dev, "device is busy\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 /* 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)
@@ -290,6 +303,11 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
+	err = intel_scu_ipc_busy(scu);
+	if (err) {
+		mutex_unlock(&ipclock);
+		return err;
+	}
 
 	for (nc = 0; nc < count; nc++, offset += 2) {
 		cbuf[offset] = addr[nc];
@@ -450,6 +468,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
 		return -ENODEV;
 	}
 	scu = ipcdev;
+	err = intel_scu_ipc_busy(scu);
+	if (err) {
+		mutex_unlock(&ipclock);
+		return err;
+	}
+
 	cmdval = sub << 12 | cmd;
 	ipc_command(scu, cmdval);
 	err = intel_scu_ipc_check_status(scu);
@@ -495,6 +519,11 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
+	err = intel_scu_ipc_busy(scu);
+	if (err) {
+		mutex_unlock(&ipclock);
+		return err;
+	}
 
 	memcpy(inbuf, in, inlen);
 	for (i = 0; i < inbuflen; i++)
-- 
https://chromeos.dev

On Wed, Sep 06, 2023 at 11:09:43AM -0700, 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Also see below.

...

> @@ -450,6 +468,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
>  		return -ENODEV;
>  	}

>  	scu = ipcdev;

Side observation: Isn't this a bug? We should not override the supplied parameter.

> +	err = intel_scu_ipc_busy(scu);
> +	if (err) {
> +		mutex_unlock(&ipclock);
> +		return err;
> +	}
> +
>  	cmdval = sub << 12 | cmd;
>  	ipc_command(scu, cmdval);
>  	err = intel_scu_ipc_check_status(scu);

-- 
With Best Regards,
Andy Shevchenko

Quoting Andy Shevchenko (2023-09-06 13:13:27)
> On Wed, Sep 06, 2023 at 11:09:43AM -0700, 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.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Also see below.
>
> ...
>
> > @@ -450,6 +468,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
> >               return -ENODEV;
> >       }
>
> >       scu = ipcdev;
>
> Side observation: Isn't this a bug? We should not override the supplied parameter.

If it is a bug that would be great to know. I wanted to make an API that
got the scu if it wasn't busy but then I ran across this code that
replaced the scu with ipcdev.

On Wed, Sep 06, 2023 at 03:22:43PM -0500, Stephen Boyd wrote:
> Quoting Andy Shevchenko (2023-09-06 13:13:27)
> > On Wed, Sep 06, 2023 at 11:09:43AM -0700, Stephen Boyd wrote:

...

> > > @@ -450,6 +468,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
> > >               return -ENODEV;
> > >       }
> >
> > >       scu = ipcdev;
> >
> > Side observation: Isn't this a bug? We should not override the supplied parameter.
> 
> If it is a bug that would be great to know. I wanted to make an API that
> got the scu if it wasn't busy but then I ran across this code that
> replaced the scu with ipcdev.

To me this seems like a bug, because in other similar code we don't do that.
And even reading this one, why do we have a parameter if it's always being
rewritten?

-- 
With Best Regards,
Andy Shevchenko

Quoting Andy Shevchenko (2023-09-06 13:46:26)
> On Wed, Sep 06, 2023 at 03:22:43PM -0500, Stephen Boyd wrote:
> > Quoting Andy Shevchenko (2023-09-06 13:13:27)
> > > On Wed, Sep 06, 2023 at 11:09:43AM -0700, Stephen Boyd wrote:
>
> ...
>
> > > > @@ -450,6 +468,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
> > > >               return -ENODEV;
> > > >       }
> > >
> > > >       scu = ipcdev;
> > >
> > > Side observation: Isn't this a bug? We should not override the supplied parameter.
> >
> > If it is a bug that would be great to know. I wanted to make an API that
> > got the scu if it wasn't busy but then I ran across this code that
> > replaced the scu with ipcdev.
>
> To me this seems like a bug, because in other similar code we don't do that.
> And even reading this one, why do we have a parameter if it's always being
> rewritten?

Yes. From what I can tell looking at commit f57fa18583f5 ("platform/x86:
intel_scu_ipc: Introduce new SCU IPC API") it was an unintentional bug
to leave that line there.

On Wed, Sep 06, 2023 at 03:59:33PM -0500, Stephen Boyd wrote:
> Quoting Andy Shevchenko (2023-09-06 13:46:26)
> > On Wed, Sep 06, 2023 at 03:22:43PM -0500, Stephen Boyd wrote:
> > > Quoting Andy Shevchenko (2023-09-06 13:13:27)
> > > > On Wed, Sep 06, 2023 at 11:09:43AM -0700, Stephen Boyd wrote:
> >
> > ...
> >
> > > > > @@ -450,6 +468,12 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
> > > > >               return -ENODEV;
> > > > >       }
> > > >
> > > > >       scu = ipcdev;
> > > >
> > > > Side observation: Isn't this a bug? We should not override the supplied parameter.
> > >
> > > If it is a bug that would be great to know. I wanted to make an API that
> > > got the scu if it wasn't busy but then I ran across this code that
> > > replaced the scu with ipcdev.
> >
> > To me this seems like a bug, because in other similar code we don't do that.
> > And even reading this one, why do we have a parameter if it's always being
> > rewritten?
> 
> Yes. From what I can tell looking at commit f57fa18583f5 ("platform/x86:
> intel_scu_ipc: Introduce new SCU IPC API") it was an unintentional bug
> to leave that line there.

Indeed it is. Good catch Andy!