linux-kernel - Re: [PATCH v7 02/10] Coresight: Add coresight TPDM source driver

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]

Date:   Wed, 1 Jun 2022 10:30:54 +0100
From:   Suzuki K Poulose <suzuki.poulose@....com>
To:     Jinlong Mao <quic_jinlmao@...cinc.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Konrad Dybcio <konradybcio@...il.com>,
        Mike Leach <mike.leach@...aro.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        Tingwei Zhang <quic_tingweiz@...cinc.com>,
        Yuanfang Zhang <quic_yuanfang@...cinc.com>,
        Tao Zhang <quic_taozha@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Hao Zhang <quic_hazha@...cinc.com>,
        linux-arm-msm@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Subject: Re: [PATCH v7 02/10] Coresight: Add coresight TPDM source driver

On 01/06/2022 10:21, Jinlong Mao wrote:
> Hi Suzuki,
> 
> On 5/24/2022 3:00 PM, Jinlong Mao wrote:
>> Hi Suzuki,
>>
>> Thank you for the review.
>>
>> On 5/23/2022 4:57 PM, Suzuki K Poulose wrote:
>>> Hi
>>>
>>> On 09/05/2022 14:39, Mao Jinlong wrote:
>>>> Add driver to support Coresight device TPDM (Trace, Profiling and
>>>> Diagnostics Monitor). TPDM is a monitor to collect data from
>>>> different datasets. This change is to add probe/enable/disable
>>>> functions for tpdm source.
>>>>
>>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>>> Signed-off-by: Mao Jinlong <quic_jinlmao@...cinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/Kconfig          |  13 ++
>>>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>>>   drivers/hwtracing/coresight/coresight-core.c |   5 +-
>>>>   drivers/hwtracing/coresight/coresight-tpdm.c | 146 
>>>> +++++++++++++++++++
>>>>   drivers/hwtracing/coresight/coresight-tpdm.h |  26 ++++
>>>>   include/linux/coresight.h                    |   1 +
>>>>   6 files changed, 191 insertions(+), 1 deletion(-)
>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c
>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.h
>>>>
> 
>>>> +/**
>>>> + * struct tpdm_drvdata - specifics associated to an TPDM component
>>>> + * @base:       memory mapped base address for this component.
>>>> + * @dev:        The device entity associated to this component.
>>>> + * @csdev:      component vitals needed by the framework.
>>>> + * @lock:       lock for the enable value.
>>>> + * @enable:     enable status of the component.
>>>> + */
>>>> +
>>>> +struct tpdm_drvdata {
>>>> +    void __iomem        *base;
>>>> +    struct device        *dev;
>>>> +    struct coresight_device    *csdev;
>>>> +    struct mutex        lock;
>>>
>>> Why mutex lock ? Couldn't this be a spinlock ?
>> 1. There is no irq for TPDM
>> 2. As there are 7 dataset types, there will be some FOR loop to configure
>> tpdm registers which may cause some time.

How long does it take to configure ? Is it too long enough to trigger
RCU stalls ? as long as we don't do any sleeping/blocking operations
we should be fine with a spinlock.

Suzuki

>>
> I think we can use mutex lock here. Do you have any more comments for 
> this ?

> 
> Thanks
> Jinlong Mao
>>>
>>>> +    bool            enable;
>>>> +};
>>>> +
>>>> +#endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
>>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>>> index 247147c11231..a9efac55029d 100644
>>>> --- a/include/linux/coresight.h
>>>> +++ b/include/linux/coresight.h
>>>> @@ -61,6 +61,7 @@ enum coresight_dev_subtype_source {
>>>>       CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
>>>>       CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
>>>>       CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
>>>> +    CORESIGHT_DEV_SUBTYPE_SOURCE_DATA_ONLY,
>>>
>>> super minor nit: I find the choice of name a bit odd.
>>> We could simply make it something like :
>>>
>>>     CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
>>>
>>> Suzuki
>> I will check and update.
>>>
>>>>   };
>>>>     enum coresight_dev_subtype_helper {
>>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@...ts.linaro.org
>>> To unsubscribe send an email to coresight-leave@...ts.linaro.org
>> _______________________________________________
>> CoreSight mailing list -- coresight@...ts.linaro.org
>> To unsubscribe send an email to coresight-leave@...ts.linaro.org