[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <961258a0-3cc6-4935-a305-80bb2c2c0597@arm.com>
Date: Thu, 4 Sep 2025 09:50:31 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Jie Gan <jie.gan@....qualcomm.com>, Mike Leach <mike.leach@...aro.org>,
James Clark <james.clark@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Tao Zhang <quic_taozha@...cinc.com>, Mao Jinlong <quic_jinlmao@...cinc.com>
Cc: tingwei.zhang@....qualcomm.com, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v1] coresight: tpda: fix the logic to setup the element
size
On 04/09/2025 02:12, Jie Gan wrote:
>
>
> On 9/3/2025 5:45 PM, Jie Gan wrote:
>>
>>
>> On 9/3/2025 4:57 PM, Suzuki K Poulose wrote:
>>> On 06/08/2025 09:09, Jie Gan wrote:
>>>> Some TPDM devices support both CMB and DSB datasets, requiring
>>>> the system to enable the port with both corresponding element sizes.
>>>>
>>>> Currently, the logic treats tpdm_read_element_size as successful if
>>>> the CMB element size is retrieved correctly, regardless of whether
>>>> the DSB element size is obtained. This behavior causes issues
>>>> when parsing data from TPDM devices that depend on both element sizes.
>>>>
>>>> To address this, the function should explicitly fail if the DSB
>>>> element size cannot be read correctly.
>>>
>>> But what is the device only has CMB ? Back when this was originally
>>
>> We have CMB TPDM, DSB TPDM and CMB&&DSB TPDM.
>>
>>> merged, we raised this question and the answer was, "Only one is
>>> supported, not both." But this sounds like that is wrong.
>>
>> I think we may not answer the previous question clearly. But it
>> definitely has issue here.
>>
>>> Could we defer the "Warning" to the caller. i.e., Let the caller
>>> figure out the if the DSB size is found and predicate that on the
>>> DSB support on the TPDM.
>>
>> Understood, below codes will be added in the caller to check the error:
>> if ((tpdm_data->dsb && !drvdata->dsb_esize) ||
>> (tpdm_data->cmb && !drvdata->cmb_esize))
>> goto err;
>>
>> Thanks,
>> Jie
>>
>
> Hi Suzuki,
>
> I've reviewed the logic here. It's not feasible for the caller to
> perform the check, since we first retrieve TPDM's drvdata, which adds
> complexity to the code. I believe it's better to handle this within the
> function itself.
>
> We are expecting the element_size for cmb if the condition is true, as
> well as dsb:
> if (tpdm_data->dsb)
> ...
> should obtain a valid element size for dsb.
> ...
>
> if (tpdm_data->cmb)
> ...
> should obtain a valid element size for cmb.
> ...
>
Ok, fair enough. Please resend the patch without the dependency on the
static TPDM patch. Given this is a fix, this could go in without waiting
for the new series.
Suzuki
> Thanks,
> Jie
>
>>>
>>> Suzuki
>>>
>>>>
>>>> Fixes: e6d7f5252f73 ("coresight-tpda: Add support to configure CMB
>>>> element")
>>>> Signed-off-by: Jie Gan <jie.gan@....qualcomm.com>
>>>> ---
>>>> drivers/hwtracing/coresight/coresight-tpda.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/
>>>> hwtracing/coresight/coresight-tpda.c
>>>> index 0633f04beb24..333b3cb23685 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -71,6 +71,8 @@ static int tpdm_read_element_size(struct
>>>> tpda_drvdata *drvdata,
>>>> if (tpdm_data->dsb) {
>>>> rc = fwnode_property_read_u32(dev_fwnode(csdev->dev.parent),
>>>> "qcom,dsb-element-bits", &drvdata->dsb_esize);
>>>> + if (rc)
>>>> + goto out;
>>>> }
>>>> if (tpdm_data->cmb) {
>>>> @@ -78,6 +80,7 @@ static int tpdm_read_element_size(struct
>>>> tpda_drvdata *drvdata,
>>>> "qcom,cmb-element-bits", &drvdata->cmb_esize);
>>>> }
>>>> +out:
>>>> if (rc)
>>>> dev_warn_once(&csdev->dev,
>>>> "Failed to read TPDM Element size: %d\n", rc);
>>>
>>>
>>
>
Powered by blists - more mailing lists