Received: by 2002:a05:7412:2a91:b0:fc:a2b0:25d7 with SMTP id u17csp468670rdh; Wed, 14 Feb 2024 02:17:43 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCVItGxMDfIbOQ8nf+QJ12loOX7I9dn0fCVBM3Wt3DjiVbvXEIMZ7AQr7UIa5OxEiu8GgMsBCO5NAaJF7oeW38UkFSnekd7j5aOInHZZ2w== X-Google-Smtp-Source: AGHT+IFCnHM3GVV0Vba7Hn6ZlGZq7czWBs/bI47mrw5bRMKSEKIy9R+1/0K/YR2hslX2X7VOXlPJ X-Received: by 2002:a17:906:68ce:b0:a3d:6637:9d6a with SMTP id y14-20020a17090668ce00b00a3d66379d6amr58304ejr.61.1707905862839; Wed, 14 Feb 2024 02:17:42 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1707905862; cv=pass; d=google.com; s=arc-20160816; b=Un88IS9Hp/n0NJsIYglE37TxVahu4htunlPdDHM99qBEi4+rijIQhaJzJjGnutO/Jb R7+oVuhCCCcLWcHhd1fgh7Bn/u09s6MIAKXBH6ebcIlEFCV0+C8LKOACn0cHquTvSNPQ Haj7mD9rWhxh4ZOUPRQ50JnpPXht9UpKS8Qagg3v3Msi5ZUsdbrQuWbNs5B+TXb4fHic gT3lbQSXi3gK3mmKTn4Zb5ewZNgr4L8cCn2rKXwOuSHF9pvLEnzXmqr83Gr44vFjrV2Y Yt2jwSr3mutBb5R0P2URDjY9oSEUQBl7IiVFPOytkl3u7tvnPNyPhu9tOk3D0f3R9fwp THgg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:date:message-id; bh=RHWsyrlBXACGEL73z8k1V+0EOkVD8W6KLM904i9wdds=; fh=hG6TUiq4wzBRIfh6nQOeBAeWpnfdak3Mo4fL77NQ22I=; b=d3fMY32DgQGBRXfhqQmc9e3BURCGLxAk9L6uPBbKLcPclZUzpZXGZ+cSkUXMw/m8Lq poviW7u8GojjKUT7SktccMvNHpv9Xe09S7jzW1hMQvrBU7aF6h2S9oYckNoryEB41MLj N1DcVlnTCkdCtbbz7DBcxx8PtjKnkWWpgqEKKigrI99N2fVYQ7YlllF/obHi8EaHfx16 wuMr+EtPEy+BZUjTupS/SbqOi6TIEK/IcmE8Jkl+nBNUEyztHPfsTdExGrHr75qaIQRf yjd/WJr7cyEE+OkDDYA1RXOdWC/p8Or/0sX11YLWRFiVy+qCDeViIsSZ95Ye6b4+IX2A cHJQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; arc=pass (i=1 spf=pass spfdomain=arm.com dmarc=pass fromdomain=arm.com); spf=pass (google.com: domain of linux-kernel+bounces-65033-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-65033-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com X-Forwarded-Encrypted: i=2; AJvYcCWjbln077/Yswa0vYH1mWTXfEmAlSncBPFQdRzAh7ZWROnQtj/Oo7nLAYGzH8vLAjBHAgfbgW38sRtUSzZDB/Ou4lM3oYZtVmNcggLf4g== Return-Path: Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id d18-20020a170906041200b00a396c26cb08si2123164eja.64.2024.02.14.02.17.42 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Feb 2024 02:17:42 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-65033-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; arc=pass (i=1 spf=pass spfdomain=arm.com dmarc=pass fromdomain=arm.com); spf=pass (google.com: domain of linux-kernel+bounces-65033-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-65033-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 807AA1F297D1 for ; Wed, 14 Feb 2024 10:17:13 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 8B3CD17543; Wed, 14 Feb 2024 10:17:04 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 048AE171C7; Wed, 14 Feb 2024 10:17:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707905823; cv=none; b=duumN1ivP8Gpcetn/o06Dcy85G3jjiI1apb7eUh24N/d2Y5L3ob/7pMJXmltlR3EO+rqy8t38BCLtdy8XDgoWMfJkGXUzmIAfYRUaoojKKylEjc/pj8hxIg+hRTalOAwBybis8gBy3I9FzzkbESyiZCyZnKd0lIeHzQHJJcIz8U= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707905823; c=relaxed/simple; bh=mF6lO2sdQD9YAA/NozjMiNCvZQnvV+ufcNUv6TC82aM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=rRWPttobllyZJsU0taQcxKMEzK44x23iVILoughyMlSbteKaPbKDVg5JBJjB8F6iI/lsK3ffbcarCjpkNB9bCFzB3hevatdl3Z0eiIup2Y6x4+7zLn+uc8w2Y5BU5nhSHjqjTSA3jHK9gHmF/SRUGj3rN20yl+ZeVl2/n+P4D+Q= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0D9DF1FB; Wed, 14 Feb 2024 02:17:41 -0800 (PST) Received: from [10.57.48.111] (unknown [10.57.48.111]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BF0F13F766; Wed, 14 Feb 2024 02:16:57 -0800 (PST) Message-ID: <653d8ae0-4eed-4fa4-b7ac-0668af93c030@arm.com> Date: Wed, 14 Feb 2024 10:16:56 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH V4 03/11] coresight: tmc: Extract device properties from AMBA pid based table lookup Content-Language: en-GB To: Anshuman Khandual , linux-arm-kernel@lists.infradead.org Cc: Lorenzo Pieralisi , Sudeep Holla , Mike Leach , Maxime Coquelin , Alexandre Torgue , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, coresight@lists.linaro.org, linux-stm32@st-md-mailman.stormreply.com References: <20240123054608.1790189-1-anshuman.khandual@arm.com> <20240123054608.1790189-4-anshuman.khandual@arm.com> <44597c9a-79bd-40f8-87a7-b53582132583@arm.com> <0c65f3b0-a879-444c-b0a4-4af485e72166@arm.com> <39c01eae-049b-4f5b-b86e-4af22c8246c1@arm.com> <14d4c862-0880-408c-bc4e-5047c7eace87@arm.com> From: Suzuki K Poulose In-Reply-To: <14d4c862-0880-408c-bc4e-5047c7eace87@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 14/02/2024 03:35, Anshuman Khandual wrote: > > > On 2/13/24 16:02, Suzuki K Poulose wrote: >> On 13/02/2024 03:13, Anshuman Khandual wrote: >>> >>> >>> On 2/12/24 17:43, Suzuki K Poulose wrote: >>>> On 23/01/2024 05:46, Anshuman Khandual wrote: >>>>> This extracts device properties from AMBA pid based table lookup. This also >>>>> defers tmc_etr_setup_caps() after the coresight device has been initialized >>>>> so that PID value can be read. >>>>> >>>>> Cc: Suzuki K Poulose >>>>> Cc: Mike Leach >>>>> Cc: James Clark >>>>> Cc: coresight@lists.linaro.org >>>>> Cc: linux-arm-kernel@lists.infradead.org >>>>> Cc: linux-kernel@vger.kernel.org >>>>> Signed-off-by: Anshuman Khandual >>>>> --- >>>>>    .../hwtracing/coresight/coresight-tmc-core.c  | 19 +++++++++++++------ >>>>>    1 file changed, 13 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> index 7ec5365e2b64..e71db3099a29 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> @@ -370,16 +370,24 @@ static inline bool tmc_etr_has_non_secure_access(struct tmc_drvdata *drvdata) >>>>>        return (auth & TMC_AUTH_NSID_MASK) == 0x3; >>>>>    } >>>>>    +#define TMC_AMBA_MASK 0xfffff >>>>> + >>>>> +static const struct amba_id tmc_ids[]; >>>>> + >>>>>    /* Detect and initialise the capabilities of a TMC ETR */ >>>>> -static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps) >>>>> +static int tmc_etr_setup_caps(struct device *parent, u32 devid) >>>>>    { >>>>>        int rc; >>>>> -    u32 dma_mask = 0; >>>>> +    u32 tmc_pid, dma_mask = 0; >>>>>        struct tmc_drvdata *drvdata = dev_get_drvdata(parent); >>>>> +    void *dev_caps; >>>>>          if (!tmc_etr_has_non_secure_access(drvdata)) >>>>>            return -EACCES; >>>>>    +    tmc_pid = coresight_get_pid(&drvdata->csdev->access) & TMC_AMBA_MASK; >>>>> +    dev_caps = coresight_get_uci_data_from_amba(tmc_ids, tmc_pid); >>>>> + >>>>>        /* Set the unadvertised capabilities */ >>>>>        tmc_etr_init_caps(drvdata, (u32)(unsigned long)dev_caps); >>>>>    @@ -497,10 +505,6 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>>>>            desc.type = CORESIGHT_DEV_TYPE_SINK; >>>>>            desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; >>>>>            desc.ops = &tmc_etr_cs_ops; >>>>> -        ret = tmc_etr_setup_caps(dev, devid, >>>>> -                     coresight_get_uci_data(id)); >>>>> -        if (ret) >>>>> -            goto out; >>>>>            idr_init(&drvdata->idr); >>>>>            mutex_init(&drvdata->idr_mutex); >>>>>            dev_list = &etr_devs; >>>>> @@ -539,6 +543,9 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) >>>>>            goto out; >>>>>        } >>>>>    +    if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) >>>>> +        ret = tmc_etr_setup_caps(dev, devid); >>>>> + >>>> >>>> With this change, we silently accept an ETR that may only have "SECURE" access only and crash later while we try to enable tracing. You could >>>> pass in the "access" (which is already in 'desc.access' in the original >>>> call site and deal with it ? >>> >>> Just wondering, if something like the following will help ? A failed tmc_etr_setup_caps() >>> because of failed tmc_etr_has_non_secure_access(), will unregister the coresight device >>> before returning. >>> >>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >>> @@ -538,8 +538,13 @@ static int __tmc_probe(struct device *dev, struct resource *res) >>>                  goto out; >>>          } >>>   -       if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) >>> +       if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { >>>                  ret = tmc_etr_setup_caps(dev, devid); >>> +               if (ret) { >>> +                       coresight_unregister(drvdata->csdev); >>> +                       goto out; >>> +               } >>> +       } >> >> Why do we move the tmc_etr_setup_caps() in the first place ? We could retain where that was and pass "desc.access" parameter rather than registering the csdev and then relying csdev->access ? > > Agreed, and after implementing the changes suggested above, the entire patch > will look something like the following. Please do confirm if this looks good > enough. > > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > @@ -370,16 +370,25 @@ static inline bool tmc_etr_has_non_secure_access(struct tmc_drvdata *drvdata) > return (auth & TMC_AUTH_NSID_MASK) == 0x3; > } > > +#define TMC_AMBA_MASK 0xfffff > + > +static const struct amba_id tmc_ids[]; > + > /* Detect and initialise the capabilities of a TMC ETR */ > -static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps) > +static int tmc_etr_setup_caps(struct device *parent, u32 devid, > + struct csdev_access *access) > { > int rc; > - u32 dma_mask = 0; > + u32 tmc_pid, dma_mask = 0; > struct tmc_drvdata *drvdata = dev_get_drvdata(parent); > + void *dev_caps; > > if (!tmc_etr_has_non_secure_access(drvdata)) > return -EACCES; > > + tmc_pid = coresight_get_pid(access) & TMC_AMBA_MASK; > + dev_caps = coresight_get_uci_data_from_amba(tmc_ids, tmc_pid); > + > /* Set the unadvertised capabilities */ > tmc_etr_init_caps(drvdata, (u32)(unsigned long)dev_caps); > > @@ -497,8 +506,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > desc.type = CORESIGHT_DEV_TYPE_SINK; > desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; > desc.ops = &tmc_etr_cs_ops; > - ret = tmc_etr_setup_caps(dev, devid, > - coresight_get_uci_data(id)); > + ret = tmc_etr_setup_caps(dev, devid, &desc.access); > if (ret) > goto out; > idr_init(&drvdata->idr); > That looks much better. Thanks Suzuki