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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200522174052.GA3379@xps15>
Date:   Fri, 22 May 2020 11:40:52 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.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 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.

> 
> ---
>  .../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.

>  };
>  
>  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().

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".  

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,
Mathieu

>  	dev_set_drvdata(dev, drvdata);
>  
>  	pdata = coresight_get_platform_data(dev);
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>