On 3/6/2020 3:50 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 5, 2020 at 3:30 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>>>> + spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
>>>> + return -EINVAL;
>>> nit: why not add "int ret = 0" to the top of the function, then here:
>>>
>>> if (rpmh_flush(ctrl))
>>> ret = -EINVAL;
>>>
>>> ...then at the end "return ret". It avoids the 2nd copy of the unlock?
>> Done.
>>> Also: Why throw away the return value of rpmh_flush and replace it
>>> with -EINVAL? Trying to avoid -EBUSY? ...oh, should you handle
>>> -EBUSY? AKA:
>>>
>>> if (!psci_has_osi_support()) {
>>> do {
>>> ret = rpmh_flush(ctrl);
>>> } while (ret == -EBUSY);
>>> }
>> Done, the return value from rpmh_flush() can be -EAGAIN, not -EBUSY.
>>
>> i will update the comment accordingly and will include below change as well in next series.
>>
>> https://patchwork.kernel.org/patch/11364067/
>>
>> this should address for caller to not handle -EAGAIN.
> A few issues, I guess.
>
> 1. I _think_ it's important that you enable interrupts between
> retries. If you're on the same CPU that the interrupt is routed to
> and you were waiting for 'tcs_in_use' to be cleared you'll be in
> trouble otherwise. ...I think we need to audit all of the places that
> are looping based on -EAGAIN and confirm that interrupts are enabled
> between retries. Before your patch series the only looping I see was
> in rpmh_invalidate() and the lock wasn't held. After your series it's
> also in rpmh_flush() which is called under spin_lock_irqsave() which
> will be a problem.
I will take a look at interrupts part.
>
> 2. The RPMH code uses both -EBUSY and -EAGAIN so I looked carefully at
> this again. You're right that -EBUSY seems to be exclusively returned
> by things only called by rpmh_rsc_send_data() and that function
> handles the retries. ...but looking at this made me find a broken
> corner case with the "zero active tcs" case (assuming you care about
> this case as per your other thread). Specifically if you have "zero
> active tcs" then get_tcs_for_msg() can call rpmh_rsc_invalidate()
> which can return -EAGAIN. That will return the -EAGAIN out of
> tcs_write() into rpmh_rsc_send_data(). rpmh_rsc_send_data() only
> handles -EBUSY, not -EAGAIN.
>
> -Doug
Thanks Doug. I will have a patch to fix this.
Thanks,
Maulik
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
