Hi,
On Wed, Apr 15, 2020 at 12:06 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Douglas Anderson (2020-04-14 10:23:26)
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index 732316bb67dc..4e45a8ac6cde 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -791,36 +790,36 @@ static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
> > {
> > struct rsc_drv *drv = container_of(nfb, struct rsc_drv, rsc_pm);
> > int ret = NOTIFY_OK;
> > -
> > - spin_lock(&drv->pm_lock);
> > + int cpus_in_pm;
> >
> > switch (action) {
> > case CPU_PM_ENTER:
> > - cpumask_set_cpu(smp_processor_id(), &drv->cpus_entered_pm);
> > -
> > - if (!cpumask_equal(&drv->cpus_entered_pm, cpu_online_mask))
> > - goto exit;
> > + cpus_in_pm = atomic_inc_return(&drv->cpus_in_pm);
> > + if (cpus_in_pm < num_online_cpus())
>
> Might be worth adding a comment here explaining that num_online_cpus()
> is stable because this is called from the cpu PM notifier path and a CPU
> can't go offline or come online without stopping the world.
Good idea.
> > + return NOTIFY_OK;
> > break;
> > case CPU_PM_ENTER_FAILED:
> > case CPU_PM_EXIT:
> > - cpumask_clear_cpu(smp_processor_id(), &drv->cpus_entered_pm);
> > - goto exit;
> > - }
> > -
> > - ret = rpmh_rsc_ctrlr_is_busy(drv);
> > - if (ret) {
> > - ret = NOTIFY_BAD;
> > - goto exit;
> > + atomic_dec(&drv->cpus_in_pm);
>
> We should also handle the cluster PM enums. I'm actually confused the
> compiler didn't complain about that already. Presumably we want to just
> ignore the cluster PM notifications because the counter handles it
> already. Looks like other code uses NOTIFY_DONE for the default case.
Hrm, I guess my compiler isn't set to warn for that? :-/
...in any case I think the right thing to do here is to add
"default:". Really we _only_ care about the ones we already have
cases for and if anyone adds any other notifications we really don't
care about them.
> > + return NOTIFY_OK;
> > }
> >
> > - ret = rpmh_flush(&drv->client);
> > - if (ret)
> > + /*
> > + * It's likely we're on the last CPU. Grab the drv->lock and write
> > + * out the sleep/wake commands to RPMH hardware. Grabbing the lock
> > + * means that if we race with another CPU coming up we are still
> > + * guaranteed to be safe. If another CPU came up just after we checked
> > + * and has already started an active transfer then we'll notice we're
> > + * busy and abort. If another CPU comes up after we start flushing it
> > + * will be blocked from starting an active transfer until we're done
> > + * flushing. If another CPU starts an active transfer after we release
> > + * the lock we're still OK because we're no longer the last CPU.
> > + */
> > + spin_lock(&drv->lock);
>
> This should probably be a raw spinlock given that this is called from
> the idle path and sleeping there is not very nice for RT. That can come
> later of course.
Actually, maybe I should just do a spin_trylock(). If I fail to get
the lock I can just return NOTIFY_BAD, right?
> > + if (rpmh_rsc_ctrlr_is_busy(drv) || !rpmh_flush(&drv->client))
>
> It almost feels like rpmh_rsc_ctrlr_is_busy() shold be rolled straight
> into rpmh_flush() so that rpmh_flush() fails if there are active
> requests in flight.
I'm going to leave that change out for now. Maulik says there are
other code paths in future patches that will call rpmh_flush(). If we
see every call to rpmh_flush() follow the same pattern then we can
roll it in then?
> > ret = NOTIFY_BAD;
Oh, I think we have a bug here. If we return NOTIFY_BAD we probably
need to decrement our count. From reading the code I think
CPU_PM_ENTER_FAILED doesn't get called for the person that returned
NOTIFY_BAD. I'll try to confirm, then fix.
> > - else
> > - ret = NOTIFY_OK;
> > + spin_unlock(&drv->lock);
>
> I'm looking at the latest linux-next code that I think has all the
> patches on the list for rpmh (latest commit is 1d3c6f86fd3f ("soc: qcom:
> rpmh: Allow RPMH driver to be loaded as a module")). I see that
> tcs->lock is taken first, and then drv->lock is taken next in
> tcs_write(). But then this takes drv->lock first and then calls
> rpmh_flush() (which goes to a different file.. yay!) and that calls
> flush_batch() which calls rpmh_rsc_write_ctrl_data() (back to this
> file... yay again!) which then locks tcs->lock. Isn't that an ABBA
> deadlock?
Oops. Somehow I thought I had checked that and the ABBA was only
there before all the cleanup patches, but I think you're right. I
think I can fix this by just changing the order we grab locks in
tcs_write(). At first I was bummed because I thought that would mean
I'd have to hold both locks while calling:
__tcs_buffer_write(drv, tcs_id, 0, msg);
__tcs_set_trigger(drv, tcs_id, true);
...but I just realized that even if I change the order I grab the
locks it doesn't mean I have to change the order I release the locks!
I'll plan to send another spin tomorrow since my day is about over now.
-Doug
