[PATCH v2 0/3] platform/x86: intel_scu_ipc: Timeout fixes

[PATCH v2 0/3] platform/x86: intel_scu_ipc: Timeout fixes

[Stephen Boyd]

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 third 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 v1 (https://lore.kernel.org/r/20230831011405.3246849-1-swboyd@chromium.org):
 * 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 (3):
  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: Fail IPC send if still busy

 drivers/platform/x86/intel_scu_ipc.c | 46 ++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 6 deletions(-)


base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
-- 
https://chromeos.dev

[PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Stephen Boyd]

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.

Cc: Prashant Malani <pmalani@chromium.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

This is sufficiently busy so I didn't add any tags from previous round.

 drivers/platform/x86/intel_scu_ipc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 6851d10d6582..b2a2de22b8ff 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -232,18 +232,21 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
 static inline int busy_loop(struct intel_scu_ipc_dev *scu)
 {
 	unsigned long end = jiffies + IPC_TIMEOUT;
+	u32 status;
 
 	do {
-		u32 status;
-
 		status = ipc_read_status(scu);
 		if (!(status & IPC_STATUS_BUSY))
-			return (status & IPC_STATUS_ERR) ? -EIO : 0;
+			goto not_busy;
 
 		usleep_range(50, 100);
 	} while (time_before(jiffies, end));
 
-	return -ETIMEDOUT;
+	status = ipc_read_status(scu);
+	if (status & IPC_STATUS_BUSY)
+		return -ETIMEDOUT;
+not_busy:
+	return (status & IPC_STATUS_ERR) ? -EIO : 0;
 }
 
 /* Wait till ipc ioc interrupt is received or timeout in 10 HZ */
-- 
https://chromeos.dev

[PATCH v2 2/3] platform/x86: intel_scu_ipc: Check status upon timeout in ipc_wait_for_interrupt()

[Stephen Boyd]

It's possible for the completion in ipc_wait_for_interrupt() to timeout,
simply because the interrupt was delayed in being processed. A timeout
in itself is not an error. This driver should check the status register
upon a timeout to ensure that scheduling or interrupt processing delays
don't affect the outcome of the IPC return value.

 CPU0                                                   SCU
 ----                                                   ---
 ipc_wait_for_interrupt()
  wait_for_completion_timeout(&scu->cmd_complete)
  [TIMEOUT]                                             status[IPC_STATUS_BUSY]=0

Fix this problem by reading the status bit in all cases, regardless of
the timeout. If the completion times out, we'll assume the problem was
that the IPC_STATUS_BUSY bit was still set, but if the status bit is
cleared in the meantime we know that we hit some scheduling delay and we
should just check the error bit.

Cc: Prashant Malani <pmalani@chromium.org>
Reviewed-by: 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index b2a2de22b8ff..3cea701d2bbd 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -254,10 +254,12 @@ static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
 {
 	int status;
 
-	if (!wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT))
-		return -ETIMEDOUT;
+	wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT);
 
 	status = ipc_read_status(scu);
+	if (status & IPC_STATUS_BUSY)
+		return -ETIMEDOUT;
+
 	if (status & IPC_STATUS_ERR)
 		return -EIO;
 
-- 
https://chromeos.dev

[PATCH v2 3/3] platform/x86: intel_scu_ipc: Fail IPC send if still busy

[Stephen Boyd]

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

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Andy Shevchenko]

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

...

>  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
>  {
>  	unsigned long end = jiffies + IPC_TIMEOUT;
> +	u32 status;
>  
>  	do {
> -		u32 status;
> -
>  		status = ipc_read_status(scu);
>  		if (!(status & IPC_STATUS_BUSY))

> -			return (status & IPC_STATUS_ERR) ? -EIO : 0;
> +			goto not_busy;

Wouldn't simple 'break' suffice here?

>  		usleep_range(50, 100);
>  	} while (time_before(jiffies, end));
>  
> -	return -ETIMEDOUT;
> +	status = ipc_read_status(scu);
> +	if (status & IPC_STATUS_BUSY)
> +		return -ETIMEDOUT;
> +not_busy:
> +	return (status & IPC_STATUS_ERR) ? -EIO : 0;



>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] platform/x86: intel_scu_ipc: Check status upon timeout in ipc_wait_for_interrupt()

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 11:09:42AM -0700, Stephen Boyd wrote:
> It's possible for the completion in ipc_wait_for_interrupt() to timeout,
> simply because the interrupt was delayed in being processed. A timeout
> in itself is not an error. This driver should check the status register
> upon a timeout to ensure that scheduling or interrupt processing delays
> don't affect the outcome of the IPC return value.
> 
>  CPU0                                                   SCU
>  ----                                                   ---
>  ipc_wait_for_interrupt()
>   wait_for_completion_timeout(&scu->cmd_complete)
>   [TIMEOUT]                                             status[IPC_STATUS_BUSY]=0
> 
> Fix this problem by reading the status bit in all cases, regardless of
> the timeout. If the completion times out, we'll assume the problem was
> that the IPC_STATUS_BUSY bit was still set, but if the status bit is
> cleared in the meantime we know that we hit some scheduling delay and we
> should just check the error bit.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] platform/x86: intel_scu_ipc: Fail IPC send if still busy

[Andy Shevchenko]

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

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Stephen Boyd]

Quoting Andy Shevchenko (2023-09-06 13:04:54)
> On Wed, Sep 06, 2023 at 11:09:41AM -0700, 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.
>
> ...
>
> >  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
> >  {
> >       unsigned long end = jiffies + IPC_TIMEOUT;
> > +     u32 status;
> >
> >       do {
> > -             u32 status;
> > -
> >               status = ipc_read_status(scu);
> >               if (!(status & IPC_STATUS_BUSY))
>
> > -                     return (status & IPC_STATUS_ERR) ? -EIO : 0;
> > +                     goto not_busy;
>
> Wouldn't simple 'break' suffice here?

Yes, at the cost of reading the status again when it isn't busy, or
checking the busy bit after the loop breaks out and reading it once
again when it is busy. I suppose the compiler would figure that out and
optimize so that break would simply goto the return statement.

The code could look like this without a goto.

	do {
		status = ipc_read_status(scu);
		if (!(status & IPC_STATUS_BUSY))
			break;
	} while (time_before(jiffies, end));

	if (status & IPC_STATUS_BUSY)
		status = ipc_read_status(scu);

	if (status & IPC_STATUS_BUSY)
		return -ETIMEDOUT;
	
	return (status & IPC_STATUS_ERR) ? -EIO : 0;

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Kuppuswamy Sathyanarayanan]

On 9/6/2023 1:14 PM, Stephen Boyd wrote:
> Quoting Andy Shevchenko (2023-09-06 13:04:54)
>> On Wed, Sep 06, 2023 at 11:09:41AM -0700, 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.
>>
>> ...
>>
>>>  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
>>>  {
>>>       unsigned long end = jiffies + IPC_TIMEOUT;
>>> +     u32 status;
>>>
>>>       do {
>>> -             u32 status;
>>> -
>>>               status = ipc_read_status(scu);
>>>               if (!(status & IPC_STATUS_BUSY))
>>
>>> -                     return (status & IPC_STATUS_ERR) ? -EIO : 0;
>>> +                     goto not_busy;
>>
>> Wouldn't simple 'break' suffice here?
> 
> Yes, at the cost of reading the status again when it isn't busy, or
> checking the busy bit after the loop breaks out and reading it once
> again when it is busy. I suppose the compiler would figure that out and
> optimize so that break would simply goto the return statement.
> 
> The code could look like this without a goto.
> 
> 	do {
> 		status = ipc_read_status(scu);
> 		if (!(status & IPC_STATUS_BUSY))
> 			break;
> 	} while (time_before(jiffies, end));
> 
> 	if (status & IPC_STATUS_BUSY)
> 		status = ipc_read_status(scu);

IMO, you can remove the if condition and read again the status in all cases.
It is more readable. But it is up to you.

/* Always read again to double check and get the latest status */
status = ipc_read_status(scu);

> 
> 	if (status & IPC_STATUS_BUSY)
> 		return -ETIMEDOUT;
> 	
> 	return (status & IPC_STATUS_ERR) ? -EIO : 0;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 3/3] platform/x86: intel_scu_ipc: Fail IPC send if still busy

[Stephen Boyd]

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.

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Stephen Boyd]

Quoting Kuppuswamy Sathyanarayanan (2023-09-06 13:20:49)
> On 9/6/2023 1:14 PM, Stephen Boyd wrote:
> > Quoting Andy Shevchenko (2023-09-06 13:04:54)
> >> On Wed, Sep 06, 2023 at 11:09:41AM -0700, Stephen Boyd wrote:
> >>>               status = ipc_read_status(scu);
> >>>               if (!(status & IPC_STATUS_BUSY))
> >>
> >>> -                     return (status & IPC_STATUS_ERR) ? -EIO : 0;
> >>> +                     goto not_busy;
> >>
> >> Wouldn't simple 'break' suffice here?
> >
> > Yes, at the cost of reading the status again when it isn't busy, or
> > checking the busy bit after the loop breaks out and reading it once
> > again when it is busy. I suppose the compiler would figure that out and
> > optimize so that break would simply goto the return statement.
> >
> > The code could look like this without a goto.
> >
> >       do {
> >               status = ipc_read_status(scu);
> >               if (!(status & IPC_STATUS_BUSY))
> >                       break;
> >       } while (time_before(jiffies, end));
> >
> >       if (status & IPC_STATUS_BUSY)
> >               status = ipc_read_status(scu);
>
> IMO, you can remove the if condition and read again the status in all cases.
> It is more readable. But it is up to you.
>

I don't really care either way. Just let me know what makes the
maintainers happy here.

Re: [PATCH v2 3/3] platform/x86: intel_scu_ipc: Fail IPC send if still busy

[Andy Shevchenko]

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

Re: [PATCH v2 3/3] platform/x86: intel_scu_ipc: Fail IPC send if still busy

[Stephen Boyd]

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.

Re: [PATCH v2 3/3] platform/x86: intel_scu_ipc: Fail IPC send if still busy

[Mika Westerberg]

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!

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Mika Westerberg]

On Wed, Sep 06, 2023 at 11:09:41AM -0700, 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.
> 
> Cc: Prashant Malani <pmalani@chromium.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> This is sufficiently busy so I didn't add any tags from previous round.
> 
>  drivers/platform/x86/intel_scu_ipc.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 6851d10d6582..b2a2de22b8ff 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -232,18 +232,21 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
>  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
>  {
>  	unsigned long end = jiffies + IPC_TIMEOUT;
> +	u32 status;
>  
>  	do {
> -		u32 status;
> -
>  		status = ipc_read_status(scu);
>  		if (!(status & IPC_STATUS_BUSY))
> -			return (status & IPC_STATUS_ERR) ? -EIO : 0;
> +			goto not_busy;
>  
>  		usleep_range(50, 100);
>  	} while (time_before(jiffies, end));
>  
> -	return -ETIMEDOUT;
> +	status = ipc_read_status(scu);

Does the issue happen again if we get scheduled away here for a long
time? ;-)

Regardless, I'm fine with this as is but if you make any changes, I
would prefer see readl_busy_timeout() used here instead (as was in the
previous version).

> +	if (status & IPC_STATUS_BUSY)
> +		return -ETIMEDOUT;
> +not_busy:
> +	return (status & IPC_STATUS_ERR) ? -EIO : 0;
>  }
>  
>  /* Wait till ipc ioc interrupt is received or timeout in 10 HZ */
> -- 
> https://chromeos.dev

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Stephen Boyd]

Quoting Mika Westerberg (2023-09-06 22:35:13)
> On Wed, Sep 06, 2023 at 11:09:41AM -0700, 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.
> >
> > Cc: Prashant Malani <pmalani@chromium.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >
> > This is sufficiently busy so I didn't add any tags from previous round.
> >
> >  drivers/platform/x86/intel_scu_ipc.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> > index 6851d10d6582..b2a2de22b8ff 100644
> > --- a/drivers/platform/x86/intel_scu_ipc.c
> > +++ b/drivers/platform/x86/intel_scu_ipc.c
> > @@ -232,18 +232,21 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
> >  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
> >  {
> >       unsigned long end = jiffies + IPC_TIMEOUT;
> > +     u32 status;
> >
> >       do {
> > -             u32 status;
> > -
> >               status = ipc_read_status(scu);
> >               if (!(status & IPC_STATUS_BUSY))
> > -                     return (status & IPC_STATUS_ERR) ? -EIO : 0;
> > +                     goto not_busy;
> >
> >               usleep_range(50, 100);
> >       } while (time_before(jiffies, end));
> >
> > -     return -ETIMEDOUT;
> > +     status = ipc_read_status(scu);
>
> Does the issue happen again if we get scheduled away here for a long
> time? ;-)

Given the smiley I'll assume you're making a joke. But to clarify, the
issue can't happen again because we've already waited at least
IPC_TIMEOUT jiffies, maybe quite a bit more, so if we get scheduled away
again it's a non-issue. If the status is still busy here then it's a
timeout guaranteed.

>
> Regardless, I'm fine with this as is but if you make any changes, I
> would prefer see readl_busy_timeout() used here instead (as was in the
> previous version).

We can't use readl_busy_timeout() (you mean readl_poll_timeout() right?)
because that implements the timeout with timekeeping and we don't know
if this is called from suspend paths after timekeeping is suspended or
from early boot paths where timekeeping isn't started.

We could use readl_poll_timeout_atomic() and then the usleep would be
changed to udelay. Not sure that is acceptable though to delay 50
microseconds vs. intentionally schedule away like the usleep call is
doing.

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Mika Westerberg]

On Thu, Sep 07, 2023 at 01:11:17PM -0700, Stephen Boyd wrote:
> Quoting Mika Westerberg (2023-09-06 22:35:13)
> > On Wed, Sep 06, 2023 at 11:09:41AM -0700, 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.
> > >
> > > Cc: Prashant Malani <pmalani@chromium.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >
> > > This is sufficiently busy so I didn't add any tags from previous round.
> > >
> > >  drivers/platform/x86/intel_scu_ipc.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> > > index 6851d10d6582..b2a2de22b8ff 100644
> > > --- a/drivers/platform/x86/intel_scu_ipc.c
> > > +++ b/drivers/platform/x86/intel_scu_ipc.c
> > > @@ -232,18 +232,21 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
> > >  static inline int busy_loop(struct intel_scu_ipc_dev *scu)
> > >  {
> > >       unsigned long end = jiffies + IPC_TIMEOUT;
> > > +     u32 status;
> > >
> > >       do {
> > > -             u32 status;
> > > -
> > >               status = ipc_read_status(scu);
> > >               if (!(status & IPC_STATUS_BUSY))
> > > -                     return (status & IPC_STATUS_ERR) ? -EIO : 0;
> > > +                     goto not_busy;
> > >
> > >               usleep_range(50, 100);
> > >       } while (time_before(jiffies, end));
> > >
> > > -     return -ETIMEDOUT;
> > > +     status = ipc_read_status(scu);
> >
> > Does the issue happen again if we get scheduled away here for a long
> > time? ;-)
> 
> Given the smiley I'll assume you're making a joke. But to clarify, the
> issue can't happen again because we've already waited at least
> IPC_TIMEOUT jiffies, maybe quite a bit more, so if we get scheduled away
> again it's a non-issue. If the status is still busy here then it's a
> timeout guaranteed.

Got it thanks!

> > Regardless, I'm fine with this as is but if you make any changes, I
> > would prefer see readl_busy_timeout() used here instead (as was in the
> > previous version).
> 
> We can't use readl_busy_timeout() (you mean readl_poll_timeout() right?)
> because that implements the timeout with timekeeping and we don't know
> if this is called from suspend paths after timekeeping is suspended or
> from early boot paths where timekeeping isn't started.

Yes readl_poll_timeout(). :)

I don't think this code is used anymore outside of regular paths. It
used to be with the Moorestown/Medfield board support code but that's
gone already. Grepping for the users also don't reveal anything that
could be using it early at boot.

Re: [PATCH v2 1/3] platform/x86: intel_scu_ipc: Check status after timeout in busy_loop()

[Stephen Boyd]

Quoting Mika Westerberg (2023-09-07 21:59:46)
> On Thu, Sep 07, 2023 at 01:11:17PM -0700, Stephen Boyd wrote:
> >
> > We can't use readl_busy_timeout() (you mean readl_poll_timeout() right?)
> > because that implements the timeout with timekeeping and we don't know
> > if this is called from suspend paths after timekeeping is suspended or
> > from early boot paths where timekeeping isn't started.
>
> Yes readl_poll_timeout(). :)
>
> I don't think this code is used anymore outside of regular paths. It
> used to be with the Moorestown/Medfield board support code but that's
> gone already. Grepping for the users also don't reveal anything that
> could be using it early at boot.

Ok. Assuming this isn't used from paths during suspend/resume when
timekeeping is suspended it look like readl_poll_timeout() is the
shorter and simpler approach. So if that works for you I'll send another
round with that and a fix for the ipcdev being overwritten.