scmi: raw: Add RAW interface for testing and developemnt

[cristian.marussi_arm.com]

Summary

Add character-device based interface to inject and snoop SCMI bare messages
in little endian binary format: each injected message will be routed and
tracked by the normal SCMI core stack and any reply, regular or late, will
be provided on the same character-device used for injection.

This is meant to be used for testing and development purposes only,
definitely NOT in production: for these reasons when enabled, the normal
SCMI drivers operation is disrupted, unless SCMI_RAW_COEX option is also
enabled.

Testing tools, like the SCMI ACS compliance suite, can use this interface
to validate the SCMI platform Server implementation, or exercise the
freeBSD SCMI core stack layers.

Tested on: Arm Morello Board
Sponsored by: Arm Ltd
Signed-off-by: Cristian Marussi <[e-mail]>

[cristian.marussi_arm.com]

sys/arm64/conf/GENERIC-KVMTOOL

27 ↗

(On Diff #145974)

Apologies but this enabled-option on a non-existent file local to my setup should have not been of this series. I will update.

[cristian.marussi_arm.com]

Removed options that enabled SCMI RAW by default (on the wrong file)

[andrew]

sys/conf/files.arm64

284

Should this file depend on scmi_raw?

[cristian.marussi_arm.com]

Added scmi_raw dep and a few missing fields in scmi_softc, mistakenly added in
a following patch

[andrew]

What will happen if we read a partial message then close the file? Or if we duplicate the file descriptor?

sys/dev/firmware/arm/scmi_raw.c

140

Is this needed? (and below)

[cristian.marussi_arm.com]

sys/dev/firmware/arm/scmi_raw.c

140

nope...a leftover from prev experiments....sorry..removing it now.

[cristian.marussi_arm.com]

In D47429#1090265, @andrew wrote:

What will happen if we read a partial message then close the file? Or if we duplicate the file descriptor?

Good point...so...the pending message replies coming back from a previous raw requests has to be read out sooner or later ... only issuing a write on /raw/reset flushes the queues....
...from userspace you are supposed to issue a read with the classic read_loop that ends when you hit an EOF....which the kernel returns when one dequeued message has been dequeued fully.

if you fail to do this and just close the fd (or die) the devfs_clear_cdevpriv() will be called only on the last close....and anyway I am certainly missing to free the pending rb buffer .... and possibly to
instead track all the closes

In the case of dup, it could be even more tricky if the same cdev_priv is shared as a consequence (like with the sharing of the file_pos).....I'll to investigate this rework around this cases....

Thanks for the review.

[cristian.marussi_arm.com]

Simplified raw_buffer free routines to be easily callable from cdev dtors.

Reviewed RAW cdev handling to properly handle sudden closes in the middle
of a read operation. A dedicated d_close_t will be called on each close
(D_TRACKCLOSE) to relinquish the scmi_raw_buffer (possibly) currently
associated with that open.
Last close invokation will handle the scenario where the RAW fd has been
dup()ed; in case of multiple reads on dup()ed/forked fds the user thread
should take care to behave properly...like with any other fds that happens
to share the same file pos metadata.

[andrew]

sys/dev/firmware/arm/scmi_raw.c

466

Should be a positive error value.

483

Should this be return (ret);?

491

Do we need this printf?

576–577

If the device creation fails will it cause problems later, e.g. on cleanup? It might be safer for scmi_raw_init to return a failure if any calls to make_dev_s fail (cleaning up any devices already created).

[cristian.marussi_arm.com]

Thanks for the review, I will fix as stated in the replies.

sys/dev/firmware/arm/scmi_raw.c

466

Yep, I'll fix

483

No because the close will always succeed, the ret val is only used locally to check to see if there is something to cleanup on close, it it is not an error if there is nothing to clean and so I dont think it should impact the outcome of the close callback.

491

I would keep this because it is the only hint that RAW collected traffic has been flushed and because it will happen rarely, typically during test/debug tools startup. (and this whole RAW support is NOT meant to be enabled in production anyway.

576–577

Right, I mistakenly assumed that the NULL dev left by a faulting make_dev_s would have been handled by destroy_dev properly: this is not the case.
Anyway, it also does not make sense indeed to carry on with a reduced number of devices when you fail initialization here. I will fix following your advice, by bailing out and cleaning devices already created.