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