Re: [PATCH v1] coresight: tpda: fix the logic to setup the element size

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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@oss.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);
>>>
>>>
>>
>