linux-kernel - Re: [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 08 May 2018 10:14:33 -0600
From:   ilina@...eaurora.org
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     andy.gross@...aro.org, david.brown@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        rnayak@...eaurora.org, bjorn.andersson@...aro.org,
        linux-kernel@...r.kernel.org, sboyd@...nel.org,
        evgreen@...omium.org, dianders@...omium.org
Subject: Re: [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of
 sleep/wake TCS

On 2018-05-03 16:06, Matthias Kaehlcke wrote:
Hi Matthias,

> Hi Lina,
> 
> On Wed, May 02, 2018 at 01:37:45PM -0600, Lina Iyer wrote:
>> Allow sleep and wake commands to be cleared from the respective TCSes,
>> so that they can be re-populated.
>> 
>> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
>> ---
>> 
>> Changes in v7:
>> 	- Move bitmap_zero() outside the loop
>> 
>> Changes in v6:
>> 	- remove unnecessary locks around __tcs_invalidate
>> 	- rename function to tcs_invaldiate
>> 
>> Changes in v4:
>> 	- refactored the rphm_rsc_invalidate()
>> ---
>>  drivers/soc/qcom/rpmh-rsc.c | 45 
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>> 
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index 4e2144a14c31..42aedf2d80fe 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct 
>> rsc_drv *drv, int type)
>>  	return &drv->tcs[type];
>>  }
>> 
>> +static int tcs_invalidate(struct rsc_drv *drv, int type)
>> +{
>> +	int m;
> 
> nit: use tcs_id?
> 
It's an iterator. Hence didn't change this.

>> +	struct tcs_group *tcs;
>> +
>> +	tcs = get_tcs_of_type(drv, type);
>> +	if (IS_ERR(tcs))
>> +		return PTR_ERR(tcs);
>> +
>> +	spin_lock(&tcs->lock);
>> +	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
>> +		spin_unlock(&tcs->lock);
>> +		return 0;
>> +	}
>> +
>> +	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>> +		if (!tcs_is_free(drv, m)) {
>> +			spin_unlock(&tcs->lock);
>> +			return -EAGAIN;
>> +		}
>> +		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
>> +	}
>> +	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> 
> You didn't reply to (or address) my comment on v6:
> 
> It could occur that one or more TCSes are disabled, then tcs_is_free()
> returns false for the next TCS and the function returns without having
> updated tcs->slots for the TCSes that have been disabled.
How do you mean TCS are disabled? I think I asked that question in my 
mail. Sorry, if I forgot to ask.

TCSes are either available for sending requests or they are busy sending 
requests. They cannot be disabled if they are present.
Individual commands, however are enabled or disabled based on whether 
they have active requests or not.

What we are trying to do here is to cleanup the TCSes of their existing 
requests. Generally, sleep and wake TCSes are not used to send active 
state requests, they are sent through AMC/Active TCS. So they will be 
free. However, in the case of the Display RSC, there is no explicit TCS 
available for sending active state requests. So we overload the wake TCS 
to send the active state requests. Even in that case, TCS would have 
finished and should be free when this function is called. The 
tcs_is_free() check is addition to make sure the requirement is not 
violated.

Hope that clears it up.

Thanks,
Lina