Re: [PATCH 1/1] coresight: Fix TRCCONFIGR.QE sysfs interface

[James Clark <james.clark@arm.com>]

On 19/01/2022 15:22, Mike Leach wrote:
> Hi James
> 
> On Fri, 14 Jan 2022 at 17:33, James Clark <james.clark@arm.com> wrote:
>>
>> It's impossible to program a valid value for TRCCONFIGR.QE
>> when TRCIDR0.QSUPP==0b10. In that case the following is true:
>>
>>   Q element support is implemented, and only supports Q elements without
>>   instruction counts. TRCCONFIGR.QE can only take the values 0b00 or 0b11.
>>
>> Currently the low bit of QSUPP is checked to see if the low bit of QE can
>> be written to, but as you can see when QSUPP==0b10 the low bit is cleared
>> making it impossible to ever write the only valid value of 0b11 to QE.
>> 0b10 would be written instead, which is a reserved QE value even for all
>> values of QSUPP.
>>
>> The fix is to allow writing the low bit of QE for any non zero value of
>> QSUPP.
>>
>> This change doesn't go any further to validate if the user supplied value
>> is valid, because none of the other parts this function do, but it does fix
>> the case where it was impossible to ever set a valid value.
>>
> 
> I concur that the input is not checked as valid, However all the other
> fields are single bit - with no invalid values - other than the cond
> field, which controls tracing of non-branch conditionals -
> architecturally unsupported for A profile in ETMv4/.
> 
> 
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
>> index a0640fa5c55b..a99bb537ea23 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
>> @@ -368,7 +368,7 @@ static ssize_t mode_store(struct device *dev,
>>         /* start by clearing QE bits */
>>         config->cfg &= ~(BIT(13) | BIT(14));
>>         /* if supported, Q elements with instruction counts are enabled */
>> -       if ((mode & BIT(0)) && (drvdata->q_support & BIT(0)))
>> +       if ((mode & BIT(0)) && drvdata->q_support)
>>                 config->cfg |= BIT(13);
> 
> This can be trivially changed to
> 
> if ((mode)  && drvdata->q_support)
>          config->cfg |= BIT(13);
> 
> to ensure that any input mode 2'b01, 2'b10, 2'b11 results in output
> settings of 2'b01, 2'b11, 2'b11 respectively
> 
> With that
> 
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
> 

Thanks for the review. Can I also get your opinion about this one? At the very least
I think the comment is not detailed enough, but I also think that case 0 should set
BIT(0) rather than BIT(1). Or is there some edge case and it's correct?


	/* start by clearing all instruction event enable bits */
	config->eventctrl1 &= ~(BIT(0) | BIT(1) | BIT(2) | BIT(3));
	switch (drvdata->nr_event) {
	case 0x0:
		/* generate Event element for event 1 */
		config->eventctrl1 |= val & BIT(1);
		break;
	case 0x1:
		/* generate Event element for event 1 and 2 */
		config->eventctrl1 |= val & (BIT(0) | BIT(1));
		break;
	case 0x2:
		/* generate Event element for event 1, 2 and 3 */
		config->eventctrl1 |= val & (BIT(0) | BIT(1) | BIT(2));
		break;
	case 0x3:
		/* generate Event element for all 4 events */
		config->eventctrl1 |= val & 0xF;
		break;
	default:
		break;
	}


>>         /*
>>          * if supported, Q elements with and without instruction
>> --
>> 2.17.1
>>
> 
>