Re: [PATCH v13 4/5] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches

[Doug Anderson]

Hi,

Quoting below may look funny since you replied with HTML mail and all
old quoting was lost.  :(  I tried my best.

On Thu, Mar 12, 2020 at 3:32 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>
> > Specifically I wouldn't trust all callers of rpmh_write() /
> > rpmh_write_async() to never send the same data.  If it was just an
> > speed/power optimization then sure you could trust them, but this is
> > for correctness.
>
> yes we should trust callers not to send duplicate data.

I guess we'll see how this works out.  Since this only affects the
"zero active-only" case and I'm pretty sure that case has more
important issues, I'm OK w/ ignoring for now.


> > Hrmm, thinking about this again, though, I'm still not convinced I
> > understand what prevents the firmware from triggering "sleep mode"
> > while the sleep/wake TCS is being borrowed for an active-only
> > transaction.  If we're sitting in rpmh_write() and sleeping in
> > wait_for_completion_timeout() couldn't the system go idle and trigger
> > sleep mode?  In OSI-mode (with last man down) you'll always have a
> > rpmh_flush() called by the last man down so you know you can make sure
> > you're in a consistent state (one final flush and no more active-only
> transactions will happen).  Without last man down you have to assume
> > it can happen at any time don't you?
>>
> > ...so I guess I'll go back to asserting that zero-active-TCS is
> > incompatible with non-OSI unless you have some way to prevent the
> > sleep mode from being triggered while you've borrowed the wake TCS.
>
> i had change for this in v4 to handle same.
>
> Link: https://patchwork.kernel.org/patch/11366205/
>
> + /*
> + * RPMh domain can not be powered off when there is pending ACK for
> + * ACTIVE_TCS request. Exit when controller is busy.
> + */
>
> before calling rpmh_flush() we check ctrlr is idle (ensuring
>
> tcs_is_free() check passes)  this will ensure that
>
> previous active transaction is completed before going ahead.
>
> i will add this check in v14.
>
> since this curretntly check for ACTIVE TCS only, i will update it to check the repurposed "WAKE TCS" is also free.

The difficulty isn't in adding a simple check, it's in adding a check
that is race free and handles locking properly.  Specifically looking
at your the v4 you pointed to, I see things like this:

  if (!rpmh_rsc_ctrlr_is_idle(drv))
    return -EBUSY;
  return rpmh_flush(&drv->client);

The rpmh_rsc_ctrlr_is_idle() grabs a spin lock implying that there
could be other people acting on RPMh at the same time (otherwise, why
do you even need locks?).  ...but when it returns the lock is
released.  Once the lock is dropped then other clients are free to
start using RPMH because nothing prevents them.  ...then you go ahead
and start flushing.

Said another way: due to the way you've structured locking in that
patch rpmh_rsc_ctrlr_is_idle() is inherently dangerous because it
returns an instantaneous state that may well have changed between the
spin_unlock_irqrestore() at the end of the function and when the
function returns.

You could, of course, fix this by requiring that the caller hold the
'drv->lock' for both the calls to rpmh_rsc_ctrlr_is_idle() and
rpmh_flush() (ignoring the fact the drv->lock is nominally part of
rpmh-rsc.c and not rpmh.c).  Now it would be safe from the problem I
described.  ...but now you get into a new problem.  If you ever hold
two locks you always need to make sure you grab them in the same order
any time you grab them both.  ...but tcs_write() we first grab a
tcs_lock and _then_ drv->lock.  That means the "fix" of holding
drv->lock for both the calls to rpmh_rsc_ctrlr_is_idle() and
rpmh_flush() won't work because rpmh_flush() will need to grab a
tcs_lock.  Possibly we could make this work by eliminating the "tcs
lock" and just having the one big "drv->lock" protect everything (or
introducing a new "super lock" making the other two meaningless).  It
would certainly be easier to reason about...

NOTE: the only way I'm able to reason about all the above things is
because I spent all the time to document what rpmh-rsc is doing and
what assumptions the various functions had [1].  It'd be great if that
could get a review.


> > This whole "no active TCS" is really quite a mess.  Given how broken
> > it seems to me it almost feels like we should remove "no active TCS"
> > from the driver for now and then re-add it in a later patch when we
> > can really validate everything.  I tried addressing this in my
> > rpmh-rsc cleanup series and every time I thought I had a good solution
> > I could find another way to break it.
> >
> > Do you actually have the "no active TCS" case working on the current
> > code, or does it only work on some downstream variant of the driver?
> >
> > It works fine on downstream variant. Some checks are still needed like above from v4
> >
> > since we do rpmh_flush() immediatly for dirty caches now.

OK.  So I take it you haven't tested the "zero active" case with the
upstream code?  In theory it should be easy to test, right?  Just hack
the driver to pretend there are zero active TCSs?

Which SoCs in specific need the zero active TCSs?  We are spending a
lot of time talking about this and reviewing the code with this in
mind.  It adds a lot of complexity to the driver.  If nothing under
active development needs it I think we should do ourselves a favor and
remove it for now, then add it back later.  Otherwise this whole
process is just going to take a lot more time.


> > I'm not saying we should limit the total number of requests to 1000.
> > I'm saying that if there are 1000 active clients then that's a
> > problem.  Right now there are something like 4 clients.  It doesn't
> > matter how fast those clients are sending, active_clients will only be
> > at most 4 and that would only be if they were all running their code
> > at the exact same time.
> >
> > I want to be able to quickly detect this type of bug:
> >
> > start_transaction()
> > ret = rpmh_write()
> > if (ret)
> >   return ret;
> > ret = rpmh_write()
> > end_transaction()
> > return ret;
> >
> > ...in other words: someone has a code path where start_transaction()
> > is called but never end_transaction().  I'm proposing that if we ever
> > see the ridiculous value of 1000 active clients the only way it could
> > happen is if one of the clients started more times than they ended.
> >
> >
> > I guess maybe by the time there were 1000 it would be too late,
> > though, because we'd have skipped A LOT of flushes by then?  Maybe
> > instead we should add something where if RPMH is "ditry" for more than
> > a certain amount of time we put a warning?
>
> IMO, we should not add any such warning with any number.
> There are only 4 clients and unlikely to have any new ones. those 4 we should be able to ensure
> that they invoke end_transaction(), if they have already done start_transaction().beside,
> Function description also says that "this must be ended by invoking rpmh_end_transaction()"
> i am ok to also add  saying that "rpmh do not check this, so its caller's responsibility to
> end it"

I don't agree but I won't argue further.  If you want to leave out the
WARN() then so be it.

-Doug

[1] 20200311161104.RFT.v2.5.I52653eb85d7dc8981ee0dafcd0b6cc0f273e9425@changeid">https://lore.kernel.org/r/20200311161104.RFT.v2.5.I52653eb85d7dc8981ee0dafcd0b6cc0f273e9425@changeid