* [PATCH v2] counter: microchip-tcb-capture: Add watch validation support @ 2025-05-20 4:06 ` Dharma Balasubiramani 2025-05-20 11:54 ` William Breathitt Gray 0 siblings, 1 reply; 3+ messages in thread From: Dharma Balasubiramani @ 2025-05-20 4:06 UTC (permalink / raw) To: Kamel Bouhara, William Breathitt Gray Cc: linux-arm-kernel, linux-iio, linux-kernel, 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> ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] counter: microchip-tcb-capture: Add watch validation support 2025-05-20 4:06 ` [PATCH v2] counter: microchip-tcb-capture: Add watch validation support Dharma Balasubiramani @ 2025-05-20 11:54 ` William Breathitt Gray 2025-05-20 14:23 ` Dharma.B 0 siblings, 1 reply; 3+ messages in thread From: William Breathitt Gray @ 2025-05-20 11:54 UTC (permalink / raw) To: Dharma Balasubiramani Cc: Kamel Bouhara, linux-arm-kernel, linux-iio, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] counter: microchip-tcb-capture: Add watch validation support 2025-05-20 11:54 ` William Breathitt Gray @ 2025-05-20 14:23 ` Dharma.B 0 siblings, 0 replies; 3+ messages in thread From: Dharma.B @ 2025-05-20 14:23 UTC (permalink / raw) To: wbg; +Cc: kamel.bouhara, linux-arm-kernel, linux-iio, linux-kernel 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-20 14:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <F_LcZtjhYzQUlCmEka_20DiefdWFYYoq-u3JOct5ctrcMrJfTi3APjAWNAK97Mpluwkqgr9rQ-35KzO0Uuifow==@protonmail.internalid>
2025-05-20 4:06 ` [PATCH v2] counter: microchip-tcb-capture: Add watch validation support Dharma Balasubiramani
2025-05-20 11:54 ` William Breathitt Gray
2025-05-20 14:23 ` Dharma.B
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox