The data in the directory entry structure is meant to be used in opening
a file. It should not change during the program.
For the virtual directory, the pointer to constant data must be wrapped
because the data for the open file is not constant.
There is extra debug logging in the code which can be activated by
`meson configure build -Dc_args=-DHEXAGONRPC_VERBOSE`. This may be
difficult to trigger, since it requires -D twice in the same argument.
Add the hexagonrpcd_verbose option to make it easier to configure the
extra logging.
This can be activated with `-Dhexagonrpcd_verbose=true` and deactivated
with `-Dhexagonrpcd_verbose=false`.
Most subdirectories in this project have all, pedantic, and some extra
warnings enabled. Set this for tests as well, as there is no real
justification otherwise.
Signed character arrays can overflow when specifying values greater than
127. The actual data is not affected, but this can cause compiler
warnings. Use unsigned character arrays so the compiler does not
complain about overflows.
This file seems to implicitly return 0 when everything is successful,
but this is unclear when reading it. Explicitly return zero when all
tests are successful.
This variable represents the subdirectory in the dsp/ directory where
libraries should be searched. This is the argument to the optional `-d`
and is used when creating the filesystem. Initialize it to an empty
string so HexagonFS searches in /usr/share/qcom/dsp/ by default.
The FastRPC implementation is loosely related to the sensor shell. It is
useful to provide interfaces over FastRPC for sensor DSP firmware, and
enables the sensor shell to provide most physical sensors. However, the
FastRPC implementation has no meaningful connection with the sensor
shell in terms of code. Move the FastRPC implementation to the project
root to make it more intuitive to install and use and to separate it
from a distinct project.
The CHRE client is not part of the core functionality of the daemon. The
client is only meant to run on sensorspd, and should not always run,
such as on non-Android or on rootpd. It should not be stuck in the file
with the program's entry point, as it needs to do more than simply
starting CHRE, like starting any useful nanoapps. Move it into a new
directory as a client that can use the same session as the daemon that
listens for remote method calls to the Application Processor, so the
session can make calls back to fetch files.
Also modify the calling conventions for starting the reverse tunnel,
since the calling conventions are not limited to a single pointer.
The HexagonRPCD daemon has support for sharing sessions with other
programs by executing them with the HEXAGONRPC_FD environment variable
set. Add a library function to obtain the file descriptor used in this
session for FastRPC communication.
The Linux kernel FastRPC driver creates a unique session for every
process that opens a file descriptor. Session sharing allows the sharing
of a reverse tunnel across clients that need it. Add support for
managing the session for clients.
The argument to -p is a path to any executable file, such as /bin/sh.
The file descriptor to the FastRPC device node is in the HEXAGONRPC_FD
environment variable.
Remove the brackets around the -f option which imply that it is
optional. The default value is a NULL pointer.
FastRPC is not specialized to serving files to the DSPs and can do other
things such as logging and profiling, pending discovery. Change the
description of the program accordingly.
Referring to a "FastRPC node" is a bit vague and misses the detail that
the program is expecting a path to a file (a device node). Refer to the
file as a FastRPC device node so the user can intuitively look for what
the program wants.
The letter in the -R option was seemingly chosen arbitrarily. Add the
word which it originated from ("root") to make it memorable.
The INIT_ATTACH and INIT_ATTACH_SNS terms have no meaning in the usage
text. They are terms of jargon originating in the userspace API for the
Linux kernel driver, as abbreviations of FASTRPC_IOCTL_INIT_ATTACH and
FASTRPC_IOCTL_INIT_ATTACH_SNS. It is unclear what the difference is
between the two, and SNS is ambiguous with no clear connection to
sensors (did Qualcomm design some DSP communication protocol around a
Social Networking Service?). Instead of deriving terms used to inform
the driver how to interact with the DSP, use the name of the
PD ("sensorspd") and describe the effective interaction with the DSP.
If the file name for the FastRPC device node is a NULL pointer, then the
error message always prints out a NULL pointer, made redundant by the
word "invalid". An unspecified device node is an error made by the user,
and the usage text should be given to the user instead of an internal
diagnosis of invalid usage.
This is useful for most programs using FastRPC, so it does not make
sense to copy the interface every time. Move it to libhexagonrpc, even
though it may not be strictly necessary to communicate over FastRPC,
since it is common.
Warnings on unused variables or functions can be fixed by removing the
unnecessary variable or function. Unused parameters are a regular
occurrence, and cannot be removed. Warnings about these may be useful
for new internal APIs with long development histories, but clutter any
build logs with meaningless warnings about unused parameters when
defining functions to be used with existing APIs, such as the reverse
tunnel API. Suppress these warnings to bring any more meaningful issues
to attention.
Despite the modularity of the FastRPC implementation, there are no unit
tests. This makes it unpredictable when the implementation encounters an
unlikely scenario, such as when reading uninitialized memory that is
non-zero, or when allocating zero bytes of memory. Add a unit test for
the I/O buffer encoder and decoder so there can be more confidence in
its expected operation.
This does not test edge cases in the malloc() function, such as
allocating zero bytes or failures.
When a pathname navigates to the parent directory via the special "../"
name, the current directory may be assigned a numeric file descriptor.
With a numeric file descriptor, the current directory can still be
referenced, and should not be closed. Only close the directory if it is
not assigned.
The file descriptor for the root directory is required for file
operations. Without the root directory open, opening or getting stats of
a file always fails. Currently, the apps_std methods check if the root
directory is open every time they are called. Fail to initialize if the
root directory cannot be opened to allow the simplification of remote
methods accepting a path name.
The path name is already NULL-terminated by the caller and does not need
to be copied to a larger buffer to add more NULL termination. Allocating
an extra buffer can cause an unnecessary crash or failure when out of
memory. Do not copy the path name. Instead, check that the path name is
NULL-terminated by the caller.
Normally, the CHRE performs a full configuration on streaming sensors
with a stream type of zero. This made it seem like a simple enable would
not properly activate sensors with this stream type, but it is possible
to use the watch command on streaming sensors. Remove this statement
which may be misleading.
The decoding error was added in the previous commit with the -1 return
value. Propagate it as a fatal error (-1).
Also remove a newline in an existing perror statement, where the error
message would be split between lines.
The decoder dynamically allocates memory while decoding so the decoded
buffers have the correct size. This can fail, often when the memory runs
out. Propagate any errors from malloc() and do not attempt to decode the
buffer.
There is no real reason for the position attributes to be signed. It
does not make sense to have a negative offset in the current input
buffer, negative alignment, or negative offset in the size of the
current input buffer. A negative number of input buffers also makes no
sense. Reorganize the decoder context struct and the I/O buffer struct
so offsets and maximums are together, nothing has the unnecessary
capability of being negative, and the sizes are not wider than possible
in the format.
Also include the definition for uint32_t.
Also update the error message format in listener.c to read a 32-bit
integer.
When the decoding continues from a misaligned part of the buffer, the
decoder attempts to skip some of the buffer to realign itself, even if
it is not the beginning of the buffer. To allow the decoding of an input
with a size indivisible by 8, only consume the alignment at the
beginning of an input buffer.
The bug outlined here does not happen in practice because the size of
the encoded input is hardcoded to 256 in listener.c.
Fixes: 77f855ec84 ("fastrpc: decode input buffers")
According to malloc(3p), a call to malloc with a size of zero can result
in a NULL pointer. If this occurs, the initialized decoding context is
still valid and the program may proceed without failing. Allow a NULL
pointer when there are zero input buffers to be processed.
Fixes: 77f855ec84 ("fastrpc: decode input buffers")
The amount of output buffers needed should be computed before the check,
otherwise it checks an uninitialized value. Initialize it first so that
the zero check works as intended.
Fixes: 9ea9bed77b ("fastrpc: hexagonrpcd: listener: do not attempt to allocate zero outbufs")
The mapped_or_empty file implementation uses NULL to denote when an
empty directory should be in place of a non-existent file. Changing the
pointer to the output has no effect. Change the implementation data to a
NULL pointer so empty directories function properly.
The is_assigned attribute should only be true when it is assigned a
numeric ID, otherwise it will not be closed. The from_dirent operations
in the implementations of a file type do not use the calloc() function,
so the is_assigned attribute is uninitialized. Initialize it to zero so
the open file can be closed when requested.
The malloc(3p) manual page states that an implementation may choose to
return a non-NULL pointer when the size is zero. This function assumes
that there is an element in the array if the pointer is not NULL. When
the size is zero, skip the allocation and return NULL.
The qmic utility is rarely packaged in mainstream distributions, making
it inconvenient to set up a working build environment. Add generated
source files to enable compilation without qmic installed.
These variables went unused since the input buffer lengths were switched
to the sizes parsed by iobuffer.c, instead of the sizes in the primary
input argument. This makes it unnecessary at times to unpack the primary
input buffer, although the variables were being kept around to document
calling conventions. Remove the variables since they add unnecessary
warnings.
The calling conventions can still be found in the Android implementation
of FastRPC.
This interface implementation no longer opens files directly, but sends
requests to hexagonfs. Hence, fcntl.h and dirent.h are no longer needed.
Also, it uses 32-bit integers from stdint.h.
The data types for the ioctl wrapper are important, as it has variable
arguments. Document them.
Also mention terminology used in QAIC output to aid in understanding
this project in relation to existing FastRPC implementations.
The sensor configuration is not required and is normally used just to
generate the sensor registry, but the sensors do not appear unless the
directory is present. The ADSP_LIBRARY_PATH virtual environment variable
may be used when opening absolute paths as a firmware quirk, even if
nothing is read from it. To remove the requirement of empty directories
in the physical filesystem, use the new hexagonfs_mapped_or_empty_ops.
Some directories are not required for the normal operation of the sensor
firmware, such as the sensor configuration directory. Add support for
directories that are empty if they are not present in the physical
filesystem.
The -R option specifies the root of the files served to the remote
processor (before mapping them to the virtual filesystem). The -d option
specifies the DSP variant, used to select the subdirectory storing the
libraries for the remote processor.
There are multiple printf statements for each line of the help text.
Each line can be concatenated to one string at compile-time without
impacting readability of the intended output, with the exception of the
first line, with special formatting. Join the rest of the help text into
one printf statement.
The directory that stores the files for the remote processor is
specified relative to /usr/share/qcom. This is arbitrary and confuses
the user into thinking that the path is relative to the current working
directory. Specify it as absolute.
The output of the adsp_listener interface provides the size of each
buffer in addition to the length provided to the remote method in the
primary buffer. Use the size of the buffer, which always corresponds to
the size allocated to it, regardless of whether there are consistency
checks.
The output of the adsp_listener interface provides the size of each
buffer in addition to the length provided to the remote method in the
primary buffer. Use the size of the buffer, which always corresponds to
the size allocated to it, regardless of whether there are consistency
checks.
The only named structs for parameters in the primary input and output
buffers are for the fread remote method. Move them to the the place
where they are used, consistent with the other remote method
implementations.
The file descriptor array maps file descriptors to their corresponding
file descriptions, and is in static storage. Since there is range
checking for file descriptors, there is still a benefit to having access
to the file descriptor array. Accept the array as a parameter.
In the future, this parameter can be removed in secondary functions if
needed.
The apps_std interface stores file descriptors in static variables,
opening them when needed, and configures HexagonFS with a hardcoded root
directory. Configure HexagonFS and open the file descriptors in
initialization instead.
The root directory should not be hardcoded by hexagonfs, but the user of
it. Receive the root directory entry as an argument to the function that
opens the root directory.
This interface depends on static variables to function properly. Add
code to initialize a context with the required data.
Do not use the context yet because it is not always initialized.
The reverse tunnel finds interfaces in a static variable. As a
consequence, the interfaces cannot be customized by the user of the
reverse tunnel. Move the interfaces to the user of the listener.
As an variable-length array within a struct, the procedure array is not
included in the sizeof() macro and cannot be trivially copied. Declare
it as a pointer to an array of procedures so the local fastrpc interface
definitions have a predictable size.
Some interfaces preserve their states between invocations in static
storage. This is not ideal, as interfaces cannot be initialized with
information from the user of the reverse tunnel. Add support for
interface-defined contexts.
This property should only be defined for file operations that need it.
Remove it from the common hexagonfs_fd struct and expect each file
operations to add it in the data pointer if necessary.
These functions are only referenced in the same compilation unit to
define file operations. Set these functions to static so they don't get
exported directly for no reason.
2023-06-05 18:07:40 -04:00
55 changed files with 1666 additions and 1974 deletions
Loaded 20 of 55 files, more files were not shown because too many files have changed in this diff.
Show more
Reference in new issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Joel Selvaraj
Richard Acayan