Re: [PATCH 3/8] tools/counter: add a flexible watch events tool

[William Breathitt Gray <william.gray@linaro.org>]

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

On Tue, Sep 19, 2023 at 05:37:34PM +0200, Fabrice Gasnier wrote:
> On 9/17/23 21:07, William Breathitt Gray wrote:
> > On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
> >> This adds a new counter tool to be able to test various watch events.
> >> A flexible watch array can be populated from command line, each field
> >> may be tuned with a dedicated command line argument.
> >> Each argument can be repeated several times: each time it gets repeated,
> >> a corresponding new watch element is allocated.
> >>
> >> It also comes with a simple default watch (to monitor overflows), used
> >> when no watch parameters are provided.
> >>
> >> The print_usage() routine proposes another example, from the command line,
> >> which generates a 2 elements watch array, to monitor:
> >> - overflow events
> >> - capture events, on channel 3, that reads read captured data by
> >>   specifying the component id (capture3_component_id being 7 here).
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Fabrice,

Sorry for the delay, I'm currently working through my backlog. I see you
have already submitted a version 2 of this patchset, so I'll continue my
review there but I do want to make some direct replys here below as
well.

> > This is a new tool, so would you add a MAINTAINERS entry for the
> > counter_watch_events.c file?
> 
> I haven't thought about it.
> I can add a MAINTAINERS entry, yes!
> Who would you suggest ?

Typically the author of the initial patch will also maintain what they
are introducing, but an alternative person is acceptable to serve as
maintainer if that's the plan that's agreed upon. I assume you're
introducing this tool because you're using it internally at ST for
testing, so I suppose the intention is not to get this merged upstream
just to abandon it (i.e. you intend to keep using and improving it). Is
the plan for you to maintain this utility, or is someone else at ST
interested in it?

> >> +	}
> >> +
> >> +	/* fallback to number */
> > 
> > I'm not sure it makes sense to support numbers. Although it's true that
> > the component type, component scope, and event type are passed as __u8
> > values, users are expected to treat those values are opaque and pass
> > them via the respective enum constants. Since we don't guarantee that
> > the specific enum constant values will remain consistent between kernel
> > versions, I don't think it's a good idea to give to users that sort of
> > implication by allowing them to use raw numbers for configuration
> > selection.
> 
> Ack, I can remove this.
> 
> I'm a bit surprised by this statement. I may be wrong... I'd expect a
> userland binary to be compatible when updating to a newer kernel: e.g.
> user API (ABI?) definitions to be stable (including enum constants) ?

I was wrong in my previous reply. You're absolutely correct[^1] that
userspace ABI must be consistent ("Breaking user programs simply isn't
acceptable"[^2]) so the specific values must remain the same between
kernel versions.

Regardless, I don't think raw numbers provide much benefit for the
use-case of this particular utility; users are testing watch
configurations for a particular device, not the specific constant values
in the data structures. So in the end I still think the raw numbers
code path should be removed.

[^1] Well technically Linux kernel ABI README documentation file
     (Documentation/ABI/README) states that backwards compatibility for
     stable interfaces is only guaranteed for at least 2 years, but in
     practice we strive to never change the user-facing ABI.
[^2] https://yarchive.net/comp/linux/gcc_vs_kernel_stability.html

William Breathitt Gray

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