Re: [PATCH] soc: qcom: rpmh-rsc: Don't use ktime for timeout in write_tcs_reg_sync()
Reviewed-by: Maulik Shah
Thanks,
Maulik
On 5/28/2020 8:18 PM, Douglas Anderson wrote:
The write_tcs_reg_sync() may be called after timekeeping is suspended
so it's not OK to use ktime. The readl_poll_timeout_atomic() macro
implicitly uses ktime. This was causing a warning at suspend time.
Change to just loop 100 times with a delay of 1 us between loops.
This may give a timeout of more than 1 second but never less and is
safe even if timekeeping is suspended.
NOTE: I don't have any actual evidence that we need to loop here.
It's possibly that all we really need to do is just read the value
back to ensure that the pipes are cleaned and the looping/comparing is
totally not needed. I never saw the loop being needed in my tests.
However, the loop shouldn't hurt.
Fixes: 91160150aba0 ("soc: qcom: rpmh-rsc: Timeout after 1 second in
write_tcs_reg_sync()")
Reported-by: Maulik Shah
Signed-off-by: Douglas Anderson
---
drivers/soc/qcom/rpmh-rsc.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 076fd27f3081..906778e2c1fa 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -175,13 +175,21 @@ static void write_tcs_reg(const struct rsc_drv *drv, int
reg, int tcs_id,
static void write_tcs_reg_sync(const struct rsc_drv *drv, int reg, int tcs_id,
u32 data)
{
- u32 new_data;
+ int i;
writel(data, tcs_reg_addr(drv, reg, tcs_id));
- if (readl_poll_timeout_atomic(tcs_reg_addr(drv, reg, tcs_id), new_data,
- new_data == data, 1, USEC_PER_SEC))
- pr_err("%s: error writing %#x to %d:%#x\n", drv->name,
- data, tcs_id, reg);
+
+ /*
+* Wait until we read back the same value. Use a counter rather than
+* ktime for timeout since this may be called after timekeeping stops.
+*/
+ for (i = 0; i < USEC_PER_SEC; i++) {
+ if (readl(tcs_reg_addr(drv, reg, tcs_id)) == data)
+ return;
+ udelay(1);
+ }
+ pr_err("%s: error writing %#x to %d:%#x\n", drv->name,
+ data, tcs_id, reg);
}
/**
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH] soc: qcom: rpmh-rsc: Don't use ktime for timeout in write_tcs_reg_sync()
Bjorn and Andy,
On Thu, May 28, 2020 at 7:48 AM Douglas Anderson wrote:
>
> The write_tcs_reg_sync() may be called after timekeeping is suspended
> so it's not OK to use ktime. The readl_poll_timeout_atomic() macro
> implicitly uses ktime. This was causing a warning at suspend time.
>
> Change to just loop 100 times with a delay of 1 us between loops.
> This may give a timeout of more than 1 second but never less and is
> safe even if timekeeping is suspended.
>
> NOTE: I don't have any actual evidence that we need to loop here.
> It's possibly that all we really need to do is just read the value
> back to ensure that the pipes are cleaned and the looping/comparing is
> totally not needed. I never saw the loop being needed in my tests.
> However, the loop shouldn't hurt.
>
> Fixes: 91160150aba0 ("soc: qcom: rpmh-rsc: Timeout after 1 second in
> write_tcs_reg_sync()")
> Reported-by: Maulik Shah
> Signed-off-by: Douglas Anderson
> ---
>
> drivers/soc/qcom/rpmh-rsc.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
Is it a good time to land this change now that -rc1 has come out?
It'd be nice to get this resolved.
Thanks!
-Doug
Re: [PATCH] soc: qcom: rpmh-rsc: Don't use ktime for timeout in write_tcs_reg_sync()
Hi,
On Thu, May 28, 2020 at 3:44 PM Stephen Boyd wrote:
>
> Quoting Douglas Anderson (2020-05-28 07:48:34)
> > The write_tcs_reg_sync() may be called after timekeeping is suspended
> > so it's not OK to use ktime. The readl_poll_timeout_atomic() macro
> > implicitly uses ktime. This was causing a warning at suspend time.
> >
> > Change to just loop 100 times with a delay of 1 us between loops.
> > This may give a timeout of more than 1 second but never less and is
> > safe even if timekeeping is suspended.
> >
> > NOTE: I don't have any actual evidence that we need to loop here.
> > It's possibly that all we really need to do is just read the value
> > back to ensure that the pipes are cleaned and the looping/comparing is
> > totally not needed. I never saw the loop being needed in my tests.
> > However, the loop shouldn't hurt.
> >
> > Fixes: 91160150aba0 ("soc: qcom: rpmh-rsc: Timeout after 1 second in
> > write_tcs_reg_sync()")
> > Reported-by: Maulik Shah
> > Signed-off-by: Douglas Anderson
> > ---
>
> Reviewed-by: Stephen Boyd
Thanks!
> Although I don't think ktime_get() inside of readl_poll_timeout_atomic()
> is correct. The timekeeping base won't be able to update when a loop is
> spinning in an irq disabled region. We need the tick interrupt to come
> in and update the base.
Is this really a problem? I'm not totally familiar with the
timekeeping code, but I know I've used ktime to time things while
interrupts are disabled in the past. It looks as if things are OK as
long as the base is updated every once in a while and it just does
deltas from there...
> Spinning for a second with irqs disabled is also
> insane for realtime so there's that problem too.
Yeah. I just arbitrarily picked 1 second originally so we didn't loop
infinitely. The expectation is that we'd never actually hit this
timeout. If we do then there's (presumably) some type of serious
problem that needs to be debugged.
-Doug
Re: [PATCH] soc: qcom: rpmh-rsc: Don't use ktime for timeout in write_tcs_reg_sync()
Quoting Douglas Anderson (2020-05-28 07:48:34)
> The write_tcs_reg_sync() may be called after timekeeping is suspended
> so it's not OK to use ktime. The readl_poll_timeout_atomic() macro
> implicitly uses ktime. This was causing a warning at suspend time.
>
> Change to just loop 100 times with a delay of 1 us between loops.
> This may give a timeout of more than 1 second but never less and is
> safe even if timekeeping is suspended.
>
> NOTE: I don't have any actual evidence that we need to loop here.
> It's possibly that all we really need to do is just read the value
> back to ensure that the pipes are cleaned and the looping/comparing is
> totally not needed. I never saw the loop being needed in my tests.
> However, the loop shouldn't hurt.
>
> Fixes: 91160150aba0 ("soc: qcom: rpmh-rsc: Timeout after 1 second in
> write_tcs_reg_sync()")
> Reported-by: Maulik Shah
> Signed-off-by: Douglas Anderson
> ---
Reviewed-by: Stephen Boyd
Although I don't think ktime_get() inside of readl_poll_timeout_atomic()
is correct. The timekeeping base won't be able to update when a loop is
spinning in an irq disabled region. We need the tick interrupt to come
in and update the base. Spinning for a second with irqs disabled is also
insane for realtime so there's that problem too. Maybe we should try to
kick timekeeping forward from these loops manually. Anyway, not problems
with this patch so not important to fix immediately.
[PATCH] soc: qcom: rpmh-rsc: Don't use ktime for timeout in write_tcs_reg_sync()
The write_tcs_reg_sync() may be called after timekeeping is suspended
so it's not OK to use ktime. The readl_poll_timeout_atomic() macro
implicitly uses ktime. This was causing a warning at suspend time.
Change to just loop 100 times with a delay of 1 us between loops.
This may give a timeout of more than 1 second but never less and is
safe even if timekeeping is suspended.
NOTE: I don't have any actual evidence that we need to loop here.
It's possibly that all we really need to do is just read the value
back to ensure that the pipes are cleaned and the looping/comparing is
totally not needed. I never saw the loop being needed in my tests.
However, the loop shouldn't hurt.
Fixes: 91160150aba0 ("soc: qcom: rpmh-rsc: Timeout after 1 second in
write_tcs_reg_sync()")
Reported-by: Maulik Shah
Signed-off-by: Douglas Anderson
---
drivers/soc/qcom/rpmh-rsc.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 076fd27f3081..906778e2c1fa 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -175,13 +175,21 @@ static void write_tcs_reg(const struct rsc_drv *drv, int
reg, int tcs_id,
static void write_tcs_reg_sync(const struct rsc_drv *drv, int reg, int tcs_id,
u32 data)
{
- u32 new_data;
+ int i;
writel(data, tcs_reg_addr(drv, reg, tcs_id));
- if (readl_poll_timeout_atomic(tcs_reg_addr(drv, reg, tcs_id), new_data,
- new_data == data, 1, USEC_PER_SEC))
- pr_err("%s: error writing %#x to %d:%#x\n", drv->name,
- data, tcs_id, reg);
+
+ /*
+* Wait until we read back the same value. Use a counter rather than
+* ktime for timeout since this may be called after timekeeping stops.
+*/
+ for (i = 0; i < USEC_PER_SEC; i++) {
+ if (readl(tcs_reg_addr(drv, reg, tcs_id)) == data)
+ return;
+ udelay(1);
+ }
+ pr_err("%s: error writing %#x to %d:%#x\n", drv->name,
+ data, tcs_id, reg);
}
/**
--
2.27.0.rc0.183.gde8f92d652-goog
