diff mbox series

[V4,03/11] coresight: tmc: Extract device properties from AMBA pid based table lookup

Message ID 20240123054608.1790189-4-anshuman.khandual@arm.com
State New
Headers
Series coresight: Move remaining AMBA ACPI devices into platform driver |

Commit Message

Anshuman Khandual Jan. 23, 2024, 5:46 a.m. UTC 
  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 <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: James Clark <james.clark@arm.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 .../hwtracing/coresight/coresight-tmc-core.c  | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Suzuki K Poulose Feb. 12, 2024, 12:13 p.m. UTC | #1 
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 <suzuki.poulose@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: James Clark <james.clark@arm.com>
> Cc: coresight@lists.linaro.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>   .../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 ?

Suzuki



>   	drvdata->miscdev.name = desc.name;
>   	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>   	drvdata->miscdev.fops = &tmc_fops;
Anshuman Khandual Feb. 13, 2024, 3:13 a.m. UTC | #2 
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 <suzuki.poulose@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: James Clark <james.clark@arm.com>
>> Cc: coresight@lists.linaro.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>   .../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;
+               }
+       }
 
        drvdata->miscdev.name = desc.name;
        drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
Suzuki K Poulose Feb. 13, 2024, 10:32 a.m. UTC | #3 
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 <suzuki.poulose@arm.com>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: James Clark <james.clark@arm.com>
>>> Cc: coresight@lists.linaro.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>    .../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 ?

Suzuki

>   
>          drvdata->miscdev.name = desc.name;
>          drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
Anshuman Khandual Feb. 14, 2024, 3:35 a.m. UTC | #4 
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 <suzuki.poulose@arm.com>
>>>> Cc: Mike Leach <mike.leach@linaro.org>
>>>> Cc: James Clark <james.clark@arm.com>
>>>> Cc: coresight@lists.linaro.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>>    .../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);
Suzuki K Poulose Feb. 14, 2024, 10:16 a.m. UTC | #5 
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 <suzuki.poulose@arm.com>
>>>>> Cc: Mike Leach <mike.leach@linaro.org>
>>>>> Cc: James Clark <james.clark@arm.com>
>>>>> Cc: coresight@lists.linaro.org
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>> ---
>>>>>     .../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
diff mbox series

Patch

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);
+
 	drvdata->miscdev.name = desc.name;
 	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
 	drvdata->miscdev.fops = &tmc_fops;