Message-ID: <7c78ca62-bb7e-4b95-9590-ad21f1c6f171@linaro.org>
Date: Thu, 6 Feb 2025 14:34:04 +0000
From: James Clark <james.clark@...aro.org>
To: Jie Gan <quic_jiegan@...cinc.com>
Cc: quic_tingweiz@...cinc.com, quic_jinlmao@...cinc.com,
 coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 suzuki.poulose@....com, mike.leach@...aro.org,
 alexander.shishkin@...ux.intel.com, mcoquelin.stm32@...il.com,
 alexandre.torgue@...s.st.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, andersson@...nel.org, konradybcio@...nel.org
Subject: Re: [PATCH 1/3] coresight: Don't save handle in path



On 06/02/2025 3:02 am, Jie Gan wrote:
> 
> 
> On 2/1/2025 12:36 AM, James Clark wrote:
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c     | 10 +++++-----
>>   drivers/hwtracing/coresight/coresight-dummy.c    |  2 +-
>>   drivers/hwtracing/coresight/coresight-etb10.c    |  8 +++-----
>>   drivers/hwtracing/coresight/coresight-etm-perf.c |  3 +--
>>   drivers/hwtracing/coresight/coresight-priv.h     |  4 ++--
>>   drivers/hwtracing/coresight/coresight-sysfs.c    |  2 +-
>>   drivers/hwtracing/coresight/coresight-tmc-etf.c  |  9 ++++-----
>>   drivers/hwtracing/coresight/coresight-tmc-etr.c  | 13 +++++--------
>>   drivers/hwtracing/coresight/coresight-tmc.h      |  2 +-
>>   drivers/hwtracing/coresight/coresight-tpiu.c     |  2 +-
>>   drivers/hwtracing/coresight/coresight-trbe.c     |  4 +---
>>   drivers/hwtracing/coresight/ultrasoc-smb.c       |  8 +++-----
>>   include/linux/coresight.h                        |  2 +-
>>   13 files changed, 29 insertions(+), 40 deletions(-)
>>
> 
> Hi James
> 
> I removed the handle from coresight_path and placed these codes in a 
> separate patch. However, I believe this change is not related to the 
> CTCU driver or coresight_path. Therefore, I suggest we submit it 
> independently.
> 
> Thanks,
> Jie
> 
> 

Yeah if you've removed the handle from the path then we can do the other 
void* changes separately. Makes sense.

>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ 
>> hwtracing/coresight/coresight-core.c
>> index 11d5d5320b1a..253ef02fde12 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -272,9 +272,9 @@ void coresight_add_helper(struct coresight_device 
>> *csdev,
>>   EXPORT_SYMBOL_GPL(coresight_add_helper);
>>   static int coresight_enable_sink(struct coresight_device *csdev,
>> -                 enum cs_mode mode, void *data)
>> +                 enum cs_mode mode, struct perf_output_handle *handle)
>>   {
>> -    return sink_ops(csdev)->enable(csdev, mode, data);
>> +    return sink_ops(csdev)->enable(csdev, mode, handle);
>>   }
>>   static void coresight_disable_sink(struct coresight_device *csdev)
>> @@ -448,7 +448,8 @@ static int coresight_enable_helpers(struct 
>> coresight_device *csdev,
>>       return 0;
>>   }
>> -int coresight_enable_path(struct coresight_path *cs_path, enum 
>> cs_mode mode)
>> +int coresight_enable_path(struct coresight_path *cs_path, enum 
>> cs_mode mode,
>> +              struct perf_output_handle *handle)
>>   {
>>       int ret = 0;
>>       u32 type;
>> @@ -479,7 +480,7 @@ int coresight_enable_path(struct coresight_path 
>> *cs_path, enum cs_mode mode)
>>           switch (type) {
>>           case CORESIGHT_DEV_TYPE_SINK:
>> -            ret = coresight_enable_sink(csdev, mode, cs_path);
>> +            ret = coresight_enable_sink(csdev, mode, handle);
>>               /*
>>                * Sink is the first component turned on. If we
>>                * failed to enable the sink, there are no components
>> @@ -807,7 +808,6 @@ void coresight_release_path(struct coresight_path 
>> *cs_path)
>>           kfree(nd);
>>       }
>> -    cs_path->handle = NULL;
>>       kfree(cs_path->path);
>>       kfree(cs_path);
>>   }
>> diff --git a/drivers/hwtracing/coresight/coresight-dummy.c b/drivers/ 
>> hwtracing/coresight/coresight-dummy.c
>> index dfcf24e9c49a..6f86d33efef4 100644
>> --- a/drivers/hwtracing/coresight/coresight-dummy.c
>> +++ b/drivers/hwtracing/coresight/coresight-dummy.c
>> @@ -54,7 +54,7 @@ static int dummy_source_trace_id(struct 
>> coresight_device *csdev, enum cs_mode mo
>>   }
>>   static int dummy_sink_enable(struct coresight_device *csdev, enum 
>> cs_mode mode,
>> -                void *data)
>> +                struct perf_output_handle *handle)
>>   {
>>       dev_dbg(csdev->dev.parent, "Dummy sink enabled\n");
>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/ 
>> hwtracing/coresight/coresight-etb10.c
>> index 05430d8931d1..e373b0f590bf 100644
>> --- a/drivers/hwtracing/coresight/coresight-etb10.c
>> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
>> @@ -167,14 +167,12 @@ static int etb_enable_sysfs(struct 
>> coresight_device *csdev)
>>       return ret;
>>   }
>> -static int etb_enable_perf(struct coresight_device *csdev, void *data)
>> +static int etb_enable_perf(struct coresight_device *csdev, struct 
>> perf_output_handle *handle)
>>   {
>>       int ret = 0;
>>       pid_t pid;
>>       unsigned long flags;
>>       struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> -    struct coresight_path *cs_path = (struct coresight_path *)data;
>> -    struct perf_output_handle *handle = cs_path->handle;
>>       struct cs_buffers *buf = etm_perf_sink_config(handle);
>>       spin_lock_irqsave(&drvdata->spinlock, flags);
>> @@ -225,7 +223,7 @@ static int etb_enable_perf(struct coresight_device 
>> *csdev, void *data)
>>   }
>>   static int etb_enable(struct coresight_device *csdev, enum cs_mode 
>> mode,
>> -              void *data)
>> +              struct perf_output_handle *handle)
>>   {
>>       int ret;
>> @@ -234,7 +232,7 @@ static int etb_enable(struct coresight_device 
>> *csdev, enum cs_mode mode,
>>           ret = etb_enable_sysfs(csdev);
>>           break;
>>       case CS_MODE_PERF:
>> -        ret = etb_enable_perf(csdev, data);
>> +        ret = etb_enable_perf(csdev, handle);
>>           break;
>>       default:
>>           ret = -EINVAL;
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/ 
>> drivers/hwtracing/coresight/coresight-etm-perf.c
>> index b6765abb0a26..0fad9968c2c0 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -501,9 +501,8 @@ static void etm_event_start(struct perf_event 
>> *event, int flags)
>>       if (WARN_ON_ONCE(!sink))
>>           goto fail_end_stop;
>> -    cs_path->handle = handle;
>>       /* Nothing will happen without a path */
>> -    if (coresight_enable_path(cs_path, CS_MODE_PERF))
>> +    if (coresight_enable_path(cs_path, CS_MODE_PERF, handle))
>>           goto fail_end_stop;
>>       /* Finally enable the tracer */
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/ 
>> hwtracing/coresight/coresight-priv.h
>> index 8e02a222b9f8..7bd43304f461 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -112,7 +112,6 @@ struct cs_buffers {
>>    * @trace_id:        trace_id of the whole path.
>>    */
>>   struct coresight_path {
>> -    struct perf_output_handle    *handle;
>>       struct list_head        *path;
>>       u8                trace_id;
>>   };
>> @@ -142,7 +141,8 @@ static inline void CS_UNLOCK(void __iomem *addr)
>>   }
>>   void coresight_disable_path(struct coresight_path *cs_path);
>> -int coresight_enable_path(struct coresight_path *cs_path, enum 
>> cs_mode mode);
>> +int coresight_enable_path(struct coresight_path *cs_path, enum 
>> cs_mode mode,
>> +              struct perf_output_handle *handle);
>>   struct coresight_device *coresight_get_sink(struct list_head *path);
>>   struct coresight_device *coresight_get_sink_by_id(u32 id);
>>   struct coresight_device *
>> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/ 
>> hwtracing/coresight/coresight-sysfs.c
>> index 04e76cc1bfdf..f9a6b838726c 100644
>> --- a/drivers/hwtracing/coresight/coresight-sysfs.c
>> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
>> @@ -209,7 +209,7 @@ int coresight_enable_sysfs(struct coresight_device 
>> *csdev)
>>           goto out;
>>       }
>> -    ret = coresight_enable_path(cs_path, CS_MODE_SYSFS);
>> +    ret = coresight_enable_path(cs_path, CS_MODE_SYSFS, NULL);
>>       if (ret)
>>           goto err_path;
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/ 
>> drivers/hwtracing/coresight/coresight-tmc-etf.c
>> index e6b07f917556..fdf1c2511d67 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> @@ -244,14 +244,13 @@ static int tmc_enable_etf_sink_sysfs(struct 
>> coresight_device *csdev)
>>       return ret;
>>   }
>> -static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, 
>> void *data)
>> +static int tmc_enable_etf_sink_perf(struct coresight_device *csdev,
>> +                    struct perf_output_handle *handle)
>>   {
>>       int ret = 0;
>>       pid_t pid;
>>       unsigned long flags;
>>       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> -    struct coresight_path *cs_path= (struct coresight_path *)data;
>> -    struct perf_output_handle *handle = cs_path->handle;
>>       struct cs_buffers *buf = etm_perf_sink_config(handle);
>>       spin_lock_irqsave(&drvdata->spinlock, flags);
>> @@ -303,7 +302,7 @@ static int tmc_enable_etf_sink_perf(struct 
>> coresight_device *csdev, void *data)
>>   }
>>   static int tmc_enable_etf_sink(struct coresight_device *csdev,
>> -                   enum cs_mode mode, void *data)
>> +                   enum cs_mode mode, struct perf_output_handle *handle)
>>   {
>>       int ret;
>> @@ -312,7 +311,7 @@ static int tmc_enable_etf_sink(struct 
>> coresight_device *csdev,
>>           ret = tmc_enable_etf_sink_sysfs(csdev);
>>           break;
>>       case CS_MODE_PERF:
>> -        ret = tmc_enable_etf_sink_perf(csdev, data);
>> +        ret = tmc_enable_etf_sink_perf(csdev, handle);
>>           break;
>>       /* We shouldn't be here */
>>       default:
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ 
>> drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 82a872882e24..2d0bd06bab2a 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -1252,10 +1252,8 @@ static int tmc_enable_etr_sink_sysfs(struct 
>> coresight_device *csdev)
>>   }
>>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>> -                   enum cs_mode mode, void *data)
>> +                   enum cs_mode mode, struct perf_output_handle *handle)
>>   {
>> -    struct coresight_path *cs_path = (struct coresight_path *)data;
>> -    struct perf_output_handle *handle = cs_path->handle;
>>       struct etr_perf_buffer *etr_perf;
>>       switch (mode) {
>> @@ -1643,14 +1641,13 @@ tmc_update_etr_buffer(struct coresight_device 
>> *csdev,
>>       return size;
>>   }
>> -static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, 
>> void *data)
>> +static int tmc_enable_etr_sink_perf(struct coresight_device *csdev,
>> +                    struct perf_output_handle *handle)
>>   {
>>       int rc = 0;
>>       pid_t pid;
>>       unsigned long flags;
>>       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> -    struct coresight_path *cs_path = (struct coresight_path *)data;
>> -    struct perf_output_handle *handle = cs_path->handle;
>>       struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
>>       spin_lock_irqsave(&drvdata->spinlock, flags);
>> @@ -1698,13 +1695,13 @@ static int tmc_enable_etr_sink_perf(struct 
>> coresight_device *csdev, void *data)
>>   }
>>   static int tmc_enable_etr_sink(struct coresight_device *csdev,
>> -                   enum cs_mode mode, void *data)
>> +                   enum cs_mode mode, struct perf_output_handle *handle)
>>   {
>>       switch (mode) {
>>       case CS_MODE_SYSFS:
>>           return tmc_enable_etr_sink_sysfs(csdev);
>>       case CS_MODE_PERF:
>> -        return tmc_enable_etr_sink_perf(csdev, data);
>> +        return tmc_enable_etr_sink_perf(csdev, handle);
>>       default:
>>           return -EINVAL;
>>       }
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/ 
>> hwtracing/coresight/coresight-tmc.h
>> index 2671926be62a..e991afd43742 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>> @@ -336,7 +336,7 @@ struct coresight_device 
>> *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);
>>   void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);
>>   void tmc_etr_remove_catu_ops(void);
>>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>> -                   enum cs_mode mode, void *data);
>> +                   enum cs_mode mode, struct perf_output_handle 
>> *handle);
>>   extern const struct attribute_group coresight_etr_group;
>>   #endif
>> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/ 
>> hwtracing/coresight/coresight-tpiu.c
>> index 97ef36f03ec2..ccf463ac7bf5 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpiu.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpiu.c
>> @@ -75,7 +75,7 @@ static void tpiu_enable_hw(struct csdev_access *csa)
>>   }
>>   static int tpiu_enable(struct coresight_device *csdev, enum cs_mode 
>> mode,
>> -               void *__unused)
>> +               struct perf_output_handle *__unused)
>>   {
>>       struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/ 
>> hwtracing/coresight/coresight-trbe.c
>> index 5005efd88a66..997d5976d2d2 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -1009,12 +1009,10 @@ static int __arm_trbe_enable(struct trbe_buf 
>> *buf,
>>   }
>>   static int arm_trbe_enable(struct coresight_device *csdev, enum 
>> cs_mode mode,
>> -               void *data)
>> +               struct perf_output_handle *handle)
>>   {
>>       struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>       struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
>> -    struct coresight_path *cs_path = (struct coresight_path *)data;
>> -    struct perf_output_handle *handle = cs_path->handle;
>>       struct trbe_buf *buf = etm_perf_sink_config(handle);
>>       WARN_ON(cpudata->cpu != smp_processor_id());
>> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/ 
>> hwtracing/coresight/ultrasoc-smb.c
>> index 9be88394b3bb..1574b5067206 100644
>> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
>> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
>> @@ -213,11 +213,9 @@ static void smb_enable_sysfs(struct 
>> coresight_device *csdev)
>>       coresight_set_mode(csdev, CS_MODE_SYSFS);
>>   }
>> -static int smb_enable_perf(struct coresight_device *csdev, void *data)
>> +static int smb_enable_perf(struct coresight_device *csdev, struct 
>> perf_output_handle *handle)
>>   {
>>       struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>> -    struct coresight_path *cs_path = (struct coresight_path *)data;
>> -    struct perf_output_handle *handle = cs_path->handle;
>>       struct cs_buffers *buf = etm_perf_sink_config(handle);
>>       pid_t pid;
>> @@ -241,7 +239,7 @@ static int smb_enable_perf(struct coresight_device 
>> *csdev, void *data)
>>   }
>>   static int smb_enable(struct coresight_device *csdev, enum cs_mode 
>> mode,
>> -              void *data)
>> +              struct perf_output_handle *handle)
>>   {
>>       struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>>       int ret = 0;
>> @@ -262,7 +260,7 @@ static int smb_enable(struct coresight_device 
>> *csdev, enum cs_mode mode,
>>           smb_enable_sysfs(csdev);
>>           break;
>>       case CS_MODE_PERF:
>> -        ret = smb_enable_perf(csdev, data);
>> +        ret = smb_enable_perf(csdev, handle);
>>           break;
>>       default:
>>           ret = -EINVAL;
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index 87f9baa7fefe..a859fc00eef9 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -353,7 +353,7 @@ enum cs_mode {
>>    */
>>   struct coresight_ops_sink {
>>       int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
>> -              void *data);
>> +              struct perf_output_handle *handle);
>>       int (*disable)(struct coresight_device *csdev);
>>       void *(*alloc_buffer)(struct coresight_device *csdev,
>>                     struct perf_event *event, void **pages,
> 

Powered by blists - more mailing lists