linux-kernel - Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

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] [day] [month] [year] [list]
Message-ID: <e723febb6f6e794d292423b33efdf3e6@codeaurora.org>
Date:   Fri, 22 May 2020 23:26:23 +0530
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach <mike.leach@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        coresight@...ts.linaro.org
Subject: Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context
 is lost

Hi Mathieu,

On 2020-05-22 23:10, Mathieu Poirier wrote:
> Hi Sai,
> 
> On Tue, May 19, 2020 at 07:06:02PM +0530, Sai Prakash Ranjan wrote:
>> On some QCOM SoCs, replicators in Always-On domain loses its
>> context as soon as the clock is disabled. Currently as a part
>> of pm_runtime workqueue, clock is disabled after the replicator
>> is initialized by amba_pm_runtime_suspend assuming that context
>> is not lost which is not true for replicators with such
>> limitations. So add a new property "qcom,replicator-loses-context"
>> to identify such replicators and reset them.
>> 
>> Suggested-by: Mike Leach <mike.leach@...aro.org>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
>> ---
>> 
>> Added Mike's suggested by for parts other than the DT property.
>> Perhaps I should add Co-developed-by Mike since the full skeletal
>> was given by Mike. I can add that if required on the next version.
> 
>  I will let Mike decide what he wants to do - I'm fine either way.
> 

Mike was ok with suggested-by, so I will go with that.

>> 
>> ---
>>  .../coresight/coresight-replicator.c          | 53 
>> +++++++++++++------
>>  1 file changed, 36 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c 
>> b/drivers/hwtracing/coresight/coresight-replicator.c
>> index c619b456f55a..ba66160c8140 100644
>> --- a/drivers/hwtracing/coresight/coresight-replicator.c
>> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
>> @@ -38,6 +38,7 @@ struct replicator_drvdata {
>>  	struct clk		*atclk;
>>  	struct coresight_device	*csdev;
>>  	spinlock_t		spinlock;
>> +	bool			check_idfilter_val;
> 
> Please add documentation for the new field, the same way other fields 
> are
> documented.
> 

Sure will add that.

>>  };
>> 
>>  static void dynamic_replicator_reset(struct replicator_drvdata 
>> *drvdata)
>> @@ -66,29 +67,43 @@ static int dynamic_replicator_enable(struct 
>> replicator_drvdata *drvdata,
>>  				     int inport, int outport)
>>  {
>>  	int rc = 0;
>> -	u32 reg;
>> -
>> -	switch (outport) {
>> -	case 0:
>> -		reg = REPLICATOR_IDFILTER0;
>> -		break;
>> -	case 1:
>> -		reg = REPLICATOR_IDFILTER1;
>> -		break;
>> -	default:
>> -		WARN_ON(1);
>> -		return -EINVAL;
>> -	}
>> +	u32 id0val, id1val;
>> 
>>  	CS_UNLOCK(drvdata->base);
>> 
>> -	if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0xff) &&
>> -	    (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0xff))
>> +	id0val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0);
>> +	id1val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1);
>> +
>> +	/*
>> +	 * Some replicator designs lose context when AMBA clocks are 
>> removed,
>> +	 * so have a check for this.
>> +	 */
>> +	if (drvdata->check_idfilter_val && id0val == 0x0 && id1val == 0x0)
>> +		id0val = id1val = 0xff;
>> +
>> +	if (id0val == 0xff && id1val == 0xff)
>>  		rc = coresight_claim_device_unlocked(drvdata->base);
>> 
>> +	if (!rc) {
>> +		switch (outport) {
>> +		case 0:
>> +			id0val = 0x0;
>> +			break;
>> +		case 1:
>> +			id1val = 0x0;
>> +			break;
>> +		default:
>> +			WARN_ON(1);
>> +			rc = -EINVAL;
>> +		}
>> +	}
>> +
>>  	/* Ensure that the outport is enabled. */
>> -	if (!rc)
>> -		writel_relaxed(0x00, drvdata->base + reg);
>> +	if (!rc) {
>> +		writel_relaxed(id0val, drvdata->base + REPLICATOR_IDFILTER0);
>> +		writel_relaxed(id1val, drvdata->base + REPLICATOR_IDFILTER1);
>> +	}
>> +
>>  	CS_LOCK(drvdata->base);
>> 
>>  	return rc;
>> @@ -239,6 +254,10 @@ static int replicator_probe(struct device *dev, 
>> struct resource *res)
>>  		desc.groups = replicator_groups;
>>  	}
>> 
>> +	if (fwnode_property_present(dev_fwnode(dev),
>> +				    "qcom,replicator-loses-context"))
>> +		drvdata->check_idfilter_val = true;
>> +
> 
> The header <linux/property.h> needs to be added for function
> fwnode_property_present().
> 

Sure will add in next version.

> What is the clock situation with other QC components like funnels?  
> Have they
> also been designed the same way?  If so the binding should probably be
> "qcom,component-loses-context", otherwise what you have suggested will 
> work just
> fine.  My goal here is to avoid having "qcom,replicator-loses-context" 
> and
> "qcom,funnel-loses-context".
> 

Yes I understand it is quite ugly, but AFAIK we do not have any SoCs 
already released
and coming in near future with such limitations in funnels or other 
components. So I will
stick to the replicator specific property.

> Lastly, I have applied patch 1 and 2 of this set to my tree so no need 
> to resend
> them again with the next revision.
> 

Thanks for reviewing these patches.

Thanks,
Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation