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.devOn 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
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.
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.
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.
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
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;
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
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.
© 2016 - 2025 Red Hat, Inc.