[PATCH v2] counter: microchip-tcb-capture: Add watch validation support

[PATCH v2] counter: microchip-tcb-capture: Add watch validation support

[Dharma Balasubiramani]

The Timer Counter Block (TCB) exposes several kinds of events to the
Counter framework, but not every event is meaningful on every hardware
channel. Add a `watch_validate()` callback so userspace may register only
the combinations actually supported:

* Channel 0 (COUNTER_MCHP_EVCHN_CV, COUNTER_MCHP_EVCHN_RA)
   - COUNTER_EVENT_CAPTURE
   - COUNTER_EVENT_CHANGE_OF_STATE
   - COUNTER_EVENT_OVERFLOW

* Channel 1 (COUNTER_MCHP_EVCHN_RB)
   - COUNTER_EVENT_CAPTURE

* Channel 2 (COUNTER_MCHP_EVCHN_RC)
   - COUNTER_EVENT_THRESHOLD

Any other request is rejected with `-EINVAL`, preventing undefined
behaviour in userspace.

Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
---
Tested the code on target (sam9x60_curiosity) using the following commands

valid ones:
./counter_watch_events -d -wevt_change_of_state,chan=0
./counter_watch_events -d -wevt_ovf,chan=0
./counter_watch_events -d -wevt_capture,chan=0
./counter_watch_events -d -wevt_capture,chan=1
./counter_watch_events -d -wevt_threshold,chan=2

invalid ones:
./counter_watch_events -d -wevt_threshold,chan=0
./counter_watch_events -d -wevt_threshold,chan=1
Error adding watches[0]: Invalid argument
---
Changes in v2:
- Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness.
- Adjust the code to ensure channel limitations.
- Drop sorting in this patch, will be taken care seperately.
- Link to v1: https://lore.kernel.org/r/20250515-counter-tcb-v1-1-e547061ed80f@microchip.com
---
 drivers/counter/microchip-tcb-capture.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 1de3c50b9804..fe817f4f1edc 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -337,6 +337,27 @@ static struct counter_comp mchp_tc_count_ext[] = {
 	COUNTER_COMP_COMPARE(mchp_tc_count_compare_read, mchp_tc_count_compare_write),
 };
 
+static int mchp_tc_watch_validate(struct counter_device *counter,
+				  const struct counter_watch *watch)
+{
+	if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA) {
+		switch (watch->event) {
+		case COUNTER_EVENT_CHANGE_OF_STATE:
+		case COUNTER_EVENT_OVERFLOW:
+		case COUNTER_EVENT_CAPTURE:
+			return 0;
+		default:
+			return -EINVAL;
+		}
+	} else if (watch->channel == COUNTER_MCHP_EVCHN_RB) {
+		return (watch->event == COUNTER_EVENT_CAPTURE) ? 0 : -EINVAL;
+	} else if (watch->channel == COUNTER_MCHP_EVCHN_RC) {
+		return (watch->event == COUNTER_EVENT_THRESHOLD) ? 0 : -EINVAL;
+	} else {
+		return -EINVAL;
+	}
+}
+
 static struct counter_count mchp_tc_counts[] = {
 	{
 		.id = 0,
@@ -356,7 +377,8 @@ static const struct counter_ops mchp_tc_ops = {
 	.function_read  = mchp_tc_count_function_read,
 	.function_write = mchp_tc_count_function_write,
 	.action_read    = mchp_tc_count_action_read,
-	.action_write   = mchp_tc_count_action_write
+	.action_write   = mchp_tc_count_action_write,
+	.watch_validate = mchp_tc_watch_validate,
 };
 
 static const struct atmel_tcb_config tcb_rm9200_config = {

---
base-commit: 8566fc3b96539e3235909d6bdda198e1282beaed
change-id: 20250515-counter-tcb-b6ae1945210b

Best regards,
-- 
Dharma Balasubiramani <dharma.b@microchip.com>

Re: [PATCH v2] counter: microchip-tcb-capture: Add watch validation support

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 4314 bytes --]

On Tue, May 20, 2025 at 09:36:42AM +0530, Dharma Balasubiramani wrote:
> The Timer Counter Block (TCB) exposes several kinds of events to the
> Counter framework, but not every event is meaningful on every hardware
> channel. Add a `watch_validate()` callback so userspace may register only
> the combinations actually supported:
> 
> * Channel 0 (COUNTER_MCHP_EVCHN_CV, COUNTER_MCHP_EVCHN_RA)
>    - COUNTER_EVENT_CAPTURE
>    - COUNTER_EVENT_CHANGE_OF_STATE
>    - COUNTER_EVENT_OVERFLOW
> 
> * Channel 1 (COUNTER_MCHP_EVCHN_RB)
>    - COUNTER_EVENT_CAPTURE
> 
> * Channel 2 (COUNTER_MCHP_EVCHN_RC)
>    - COUNTER_EVENT_THRESHOLD
> 
> Any other request is rejected with `-EINVAL`, preventing undefined
> behaviour in userspace.

Hi Dharma

The requesting an invalid watch configuration wouldn't necessarily lead
to undefined beaviour in userspace -- at least as far as the Counter
character device interface is concerned. What would happen is that the
requested event is never pushed to that particular channel, so userspace
is left watching for an event that never arrives for that particular
watch: not an ideal situation, but not undefined.

> 
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---
> Tested the code on target (sam9x60_curiosity) using the following commands
> 
> valid ones:
> ./counter_watch_events -d -wevt_change_of_state,chan=0
> ./counter_watch_events -d -wevt_ovf,chan=0
> ./counter_watch_events -d -wevt_capture,chan=0
> ./counter_watch_events -d -wevt_capture,chan=1
> ./counter_watch_events -d -wevt_threshold,chan=2
> 
> invalid ones:
> ./counter_watch_events -d -wevt_threshold,chan=0
> ./counter_watch_events -d -wevt_threshold,chan=1
> Error adding watches[0]: Invalid argument
> ---
> Changes in v2:
> - Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness.
> - Adjust the code to ensure channel limitations.
> - Drop sorting in this patch, will be taken care seperately.
> - Link to v1: https://lore.kernel.org/r/20250515-counter-tcb-v1-1-e547061ed80f@microchip.com

Thank you for the changes. I have a minor adjustment suggestion below
that I believe makes the code look a little nicer.

> ---
>  drivers/counter/microchip-tcb-capture.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index 1de3c50b9804..fe817f4f1edc 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -337,6 +337,27 @@ static struct counter_comp mchp_tc_count_ext[] = {
>  	COUNTER_COMP_COMPARE(mchp_tc_count_compare_read, mchp_tc_count_compare_write),
>  };
> 
> +static int mchp_tc_watch_validate(struct counter_device *counter,
> +				  const struct counter_watch *watch)
> +{
> +	if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA) {
> +		switch (watch->event) {
> +		case COUNTER_EVENT_CHANGE_OF_STATE:
> +		case COUNTER_EVENT_OVERFLOW:
> +		case COUNTER_EVENT_CAPTURE:
> +			return 0;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else if (watch->channel == COUNTER_MCHP_EVCHN_RB) {
> +		return (watch->event == COUNTER_EVENT_CAPTURE) ? 0 : -EINVAL;
> +	} else if (watch->channel == COUNTER_MCHP_EVCHN_RC) {
> +		return (watch->event == COUNTER_EVENT_THRESHOLD) ? 0 : -EINVAL;
> +	} else {
> +		return -EINVAL;
> +	}

You can use the early returns to avoid the else statements, and some
other additional cleanups can be done as well:

    if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA)
    	switch (watch->event) {
    	case COUNTER_EVENT_CHANGE_OF_STATE:
    	case COUNTER_EVENT_OVERFLOW:
    	case COUNTER_EVENT_CAPTURE:
    		return 0;
    	default:
    		return -EINVAL;
    	}
    
    if (watch->channel == COUNTER_MCHP_EVCHN_RB && watch->event == COUNTER_EVENT_CAPTURE)
    	return 0;
    
    if (watch->channel == COUNTER_MCHP_EVCHN_RC && watch->event == COUNTER_EVENT_THRESHOLD)
    	return 0;
    
    return -EINVAL;

I think something like that looks a bit closer to the Linux kernel style
present in the other drivers, so that we're all consistent.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] counter: microchip-tcb-capture: Add watch validation support

[Dharma.B]

On 20/05/25 5:24 pm, William Breathitt Gray wrote:
> On Tue, May 20, 2025 at 09:36:42AM +0530, Dharma Balasubiramani wrote:
>> The Timer Counter Block (TCB) exposes several kinds of events to the
>> Counter framework, but not every event is meaningful on every hardware
>> channel. Add a `watch_validate()` callback so userspace may register only
>> the combinations actually supported:
>>
>> * Channel 0 (COUNTER_MCHP_EVCHN_CV, COUNTER_MCHP_EVCHN_RA)
>>     - COUNTER_EVENT_CAPTURE
>>     - COUNTER_EVENT_CHANGE_OF_STATE
>>     - COUNTER_EVENT_OVERFLOW
>>
>> * Channel 1 (COUNTER_MCHP_EVCHN_RB)
>>     - COUNTER_EVENT_CAPTURE
>>
>> * Channel 2 (COUNTER_MCHP_EVCHN_RC)
>>     - COUNTER_EVENT_THRESHOLD
>>
>> Any other request is rejected with `-EINVAL`, preventing undefined
>> behaviour in userspace.
> 

Hi William,

> Hi Dharma
> 
> The requesting an invalid watch configuration wouldn't necessarily lead
> to undefined beaviour in userspace -- at least as far as the Counter
> character device interface is concerned. What would happen is that the
> requested event is never pushed to that particular channel, so userspace
> is left watching for an event that never arrives for that particular
> watch: not an ideal situation, but not undefined.

Got it, Thanks. (I'll rephrase the commit message in the next revision)

> 
>>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
>> ---
>> Tested the code on target (sam9x60_curiosity) using the following commands
>>
>> valid ones:
>> ./counter_watch_events -d -wevt_change_of_state,chan=0
>> ./counter_watch_events -d -wevt_ovf,chan=0
>> ./counter_watch_events -d -wevt_capture,chan=0
>> ./counter_watch_events -d -wevt_capture,chan=1
>> ./counter_watch_events -d -wevt_threshold,chan=2
>>
>> invalid ones:
>> ./counter_watch_events -d -wevt_threshold,chan=0
>> ./counter_watch_events -d -wevt_threshold,chan=1
>> Error adding watches[0]: Invalid argument
>> ---
>> Changes in v2:
>> - Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness.
>> - Adjust the code to ensure channel limitations.
>> - Drop sorting in this patch, will be taken care seperately.
>> - Link to v1: https://lore.kernel.org/r/20250515-counter-tcb-v1-1-e547061ed80f@microchip.com
> 
> Thank you for the changes. I have a minor adjustment suggestion below
> that I believe makes the code look a little nicer.
> 
>> ---
>>   drivers/counter/microchip-tcb-capture.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
>> index 1de3c50b9804..fe817f4f1edc 100644
>> --- a/drivers/counter/microchip-tcb-capture.c
>> +++ b/drivers/counter/microchip-tcb-capture.c
>> @@ -337,6 +337,27 @@ static struct counter_comp mchp_tc_count_ext[] = {
>>   	COUNTER_COMP_COMPARE(mchp_tc_count_compare_read, mchp_tc_count_compare_write),
>>   };
>>
>> +static int mchp_tc_watch_validate(struct counter_device *counter,
>> +				  const struct counter_watch *watch)
>> +{
>> +	if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA) {
>> +		switch (watch->event) {
>> +		case COUNTER_EVENT_CHANGE_OF_STATE:
>> +		case COUNTER_EVENT_OVERFLOW:
>> +		case COUNTER_EVENT_CAPTURE:
>> +			return 0;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	} else if (watch->channel == COUNTER_MCHP_EVCHN_RB) {
>> +		return (watch->event == COUNTER_EVENT_CAPTURE) ? 0 : -EINVAL;
>> +	} else if (watch->channel == COUNTER_MCHP_EVCHN_RC) {
>> +		return (watch->event == COUNTER_EVENT_THRESHOLD) ? 0 : -EINVAL;
>> +	} else {
>> +		return -EINVAL;
>> +	}
> 
> You can use the early returns to avoid the else statements, and some
> other additional cleanups can be done as well:
> 
>      if (watch->channel == COUNTER_MCHP_EVCHN_CV || watch->channel == COUNTER_MCHP_EVCHN_RA)
>      	switch (watch->event) {
>      	case COUNTER_EVENT_CHANGE_OF_STATE:
>      	case COUNTER_EVENT_OVERFLOW:
>      	case COUNTER_EVENT_CAPTURE:
>      		return 0;
>      	default:
>      		return -EINVAL;
>      	}
>      
>      if (watch->channel == COUNTER_MCHP_EVCHN_RB && watch->event == COUNTER_EVENT_CAPTURE)
>      	return 0;
>      
>      if (watch->channel == COUNTER_MCHP_EVCHN_RC && watch->event == COUNTER_EVENT_THRESHOLD)
>      	return 0;
>      
>      return -EINVAL;
> 
> I think something like that looks a bit closer to the Linux kernel style
> present in the other drivers, so that we're all consistent.

Thanks, I will stick to it.

> 
> William Breathitt Gray


-- 
With Best Regards,
Dharma B.