| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Cornucopia isn't maintained anymore and depended on
libsamsung-ipc's Vala bindings which have been removed by
commit ff8032e4c25f ("Deprecated and unmaintained vapi removal").
So beside using oFono patches to or libsamsung-ril through
libhybris, tools like ipc-modem, ipc-test are pretty much
the only way to test modems under GNU/Linux, and being able
to test the RFS support is useful.
In addition herolte (a Galaxy S7 variant), has a check
in its kernel that prevents ipc-modem to work if the RFS
channel is not opened[1]:
static bool rild_ready(struct link_device *ld)
{
[...]
fmt_iod = link_get_iod_with_channel(ld,
SIPC5_CH_ID_FMT_0);
if (!fmt_iod) {
mif_err("%s: No FMT io_device\n", ld->name);
return false;
}
rfs_iod = link_get_iod_with_channel(ld, SIPC5_CH_ID_RFS_0);
if (!rfs_iod) {
mif_err("%s: No RFS io_device\n", ld->name);
return false;
}
[...]
}
So when adding support for such devices, being able use ipc-modem
to easily test if it works is also very useful.
[1]drivers/misc/modem_v1/link_device_shmem.c from
https://github.com/ivanmeler/android_kernel_samsung_herolte
with the lineage-17.0 branch
Signed-off-by: Tony Garnock-Jones <tonyg@leastfixedpoint.com>
GNUtoo: rewrote the commit message, whitespace fixes.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
=======================================================================
=======================================================================
=======================================================================
TODO:
=====
- Finish addressing all the review concerns
- Send it for review again (if possible involve Tony for testing +
review)
Review things to address:
========================
> + return 0;
Thanks a lot: That check wasn't there before as we had that instead:
> rc = ipc_client_poll(client, NULL, NULL);
> if (rc < 0)
> continue;
So here when rc was 0, it would then run that:
> rc = ipc_client_recv(client, &resp);
> if (rc < 0) {
> [...]
> break;
> }
This is because for some devices this will call
xmm626_kernel_smdk4412_fmt_recv that has the following:
> rc = client->handlers->read(client, client->handlers->transport_data,
> buffer, length);
> if (rc < (int) sizeof(struct ipc_fmt_header)) {
> [...]
> goto error;
> }
And client->handlers->read can be xmm626_kernel_smdk4412_read which
just wraps a read call. And read can return 0 if there is nothing to
read yet.
So if I understood correctly, we could have read too soon, and that
would make ipc-modem stop due to an error.
In any case it's also best to create a separate patch to fix that
before this patch as if there is any regression we could easily pinpoint
that to this exact change.
Do you want me to do this patch? or do you want to do it?
%<------------------------------------------------------------------->%
> + if (resp.data != NULL)
> + free(resp.data);
> +
> + return 1;
> +}
If you switch the name of that function to modem_read, you could just
return rc (including when there is a timeout). This way it would look
like the semantics of the read() function. As read also return -1 in
case of errors that would look way better.
%<------------------------------------------------------------------->%
> +void modem_read_loop(struct ipc_client *client_fmt, struct
> ipc_client *client_rfs) +{
> while (1) {
> usleep(3000);
>
> - rc = ipc_client_poll(client, NULL, NULL);
> - if (rc < 0)
> - continue;
> -
> - rc = ipc_client_recv(client, &resp);
> - if (rc < 0) {
> - printf("[E] "
> - "Can't RECV from modem: please run
> this again"
> - "\n");
> - break;
> - }
> -
> - modem_response_handle(client, &resp);
> -
> - if (resp.data != NULL)
> - free(resp.data);
> + switch (modem_poll_client(client_fmt, "FMT")) {
> + case -1:
> + return;
This changes the program behavior here: before we had that:
> int modem_read_loop(struct ipc_client *client)> {
> [...]
> while (1) {
> usleep(3000);
>
> rc = ipc_client_poll(client, NULL, NULL);
> if (rc < 0)
> continue;
> [...]
> } [...]
> }
So when the modem crashed for instance, you would simply continue
trying to read ipc messages. While there are no comments that
indicates why it was done like that, in libsamsung-ril we just restart
to bootstrap the modem again when that happens: for instance in
xmm626_kernel_smdk4412_poll we have:
> rc = select(fd_max + 1, &set, NULL, NULL, timeout);
>
> if (FD_ISSET(fd, &set)) {
> status = ioctl(fd, IOCTL_MODEM_STATUS, 0);
> if (status != STATE_ONLINE && status != STATE_BOOTING)
> return -1;
> }
> [...]
> return rc;
So it's probably safe to assume that the modem is crashed if the is
return code is -1 as it's not booting.
It would probably be best to split that change somehow, as it could
help bisecting the issue in case of problems with that change.
For instance the patch to fix that could be applied before this patch
and look like that:
> int modem_read_loop(struct ipc_client *client)
> {
> [...]
> while (1) {
> usleep(3000);
>
> rc = ipc_client_poll(client, NULL, NULL);
> if (rc < 0)
> - continue;
> + return rc;
> [...]
> } [...]
>}
Do you want me to do it or do you prefer doing it?
%<------------------------------------------------------------------->%
> + case 0:
> + if (client_rfs != NULL) {
> + switch
> (modem_poll_client(client_rfs, "RFS")) {
The issue here is that if the IPC channel always has some data, the RFS
modem poll is never run.
The issue is that, as I understand the following won't work:
> switch (modem_poll_client(client_fmt, "FMT")) {
> case -1:
> return;
> case 0:
> /* Fall through */
> default:
> break;
> }
> switch (modem_poll_client(client_rfs, "RFS")) {
> case -1:
> return;
> case 0:
> /* Fall through */
> default:
> break;
> }
> void modem_log_handler(__attribute__((unused)) void *user_data,
> @@ -476,20 +508,24 @@ void print_help(void)
> printf("\tpower-off power off the modem\n");
> printf("arguments:\n");
> printf("\t--debug enable debug messages\n");
> + printf("\t--rfs enable RFS client in
> addition to FMT client\n"); printf("\t--pin=[PIN] provide
> SIM card PIN\n"); }
>
> int main(int argc, char *argv[])
> {
> - struct ipc_client *client_fmt;
> + struct ipc_client *client_fmt = 0;
> + struct ipc_client *client_rfs = 0;
> int c = 0;
> int opt_i = 0;
> int rc = -1;
> int debug = 0;
> + int rfs = 0;
>
> struct option opt_l[] = {
> {"help", no_argument, 0, 0 },
> {"debug", no_argument, 0, 0 },
> + {"rfs", no_argument, 0, 0 },
> {"pin", required_argument, 0, 0 },
> {0, 0, 0, 0 }
> };
> @@ -512,6 +548,9 @@ int main(int argc, char *argv[])
> } else if (strcmp(opt_l[opt_i].name,
> "debug") == 0) { debug = 1;
> printf("[I] Debug enabled\n");
> + } else if (strcmp(opt_l[opt_i].name, "rfs")
> == 0) {
> + rfs = 1;
> + printf("[I] RFS enabled\n");
> } else if (strcmp(opt_l[opt_i].name, "pin")
> == 0) { if (optarg) {
> if (strlen(optarg) < 8) {
> @@ -536,12 +575,30 @@ int main(int argc, char *argv[])
> goto modem_quit;
> }
>
> + if (rfs) {
> + client_rfs = ipc_client_create(IPC_CLIENT_TYPE_RFS);
> + if (client_rfs == 0) {
> + printf("[E] Could not create RFS client;
> aborting ...\n");
> + goto modem_quit;
> + }
> + } else {
> + client_rfs = 0;
> + }
> +
> if (debug == 0) {
> ipc_client_log_callback_register(client_fmt,
> modem_log_handler_quiet,
> NULL);
> + if (rfs) {
> + ipc_client_log_callback_register(client_rfs,
> +
> modem_log_handler_quiet, NULL);
> + }
> } else {
> ipc_client_log_callback_register(client_fmt,
> modem_log_handler, NULL);
> + if (rfs) {
> + ipc_client_log_callback_register(client_rfs,
> modem_log_handler,
> + NULL);
> + }
> }
>
> while (optind < argc) {
> @@ -561,18 +618,34 @@ int main(int argc, char *argv[])
> printf("[E] Something went wrong "
> "while bootstrapping
> modem\n"); } else if (strncmp(argv[optind], "start", 5) == 0) {
> - printf("[0] Starting modem on FMT client\n");
> + printf("[0] Starting modem FMT client\n");
> rc = modem_start(client_fmt);
> if (rc < 0) {
> - printf("[E] Something went wrong\n");
> + printf("[E] Something went wrong
> starting FMT client\n"); modem_stop(client_fmt);
> return 1;
> }
>
> - printf("[1] Starting modem_read_loop on FMT
> client\n");
> - modem_read_loop(client_fmt);
> + if (rfs) {
> + printf("[1] Starting modem RFS
> client\n");
> + ipc_client_data_create(client_rfs);
> + rc = ipc_client_open(client_rfs);
> + if (rc < 0) {
> + printf("[E] Something went
> wrong starting RFS client\n");
> + ipc_client_close(client_rfs);
> + modem_stop(client_fmt);
> + return 1;
> + }
> + } else {
> + printf("[1] Skipping modem RFS
> client start\n");
If you use thread and chose not to enable RFS by default for some good
reason, it would be a good idea to add a print here as it's probably
easy to forget to add --rfs to the command line. It also shows up in
logs so the information will appear when copy/pasting logs to get some
help.
However for someone that didn't read that code, that could be
interpreted as an error. To make it more clear you could do something
like that for instance:
> printf("[1] --rfs not selected, "
> "skipping modem RFS client start\n");
Here it explains why it's skipping the RFS client start.
Another option would be to enable the RFS handling by default. If there
are no downsides here that option would be way better. Otherwise people
that have a Galaxy S7 with the herolte codename, and potentially other
devices they want to add support for will not necessarily know that
--rfs is really required. They could also forget to add it during some
tests and draw wrong conclusions.
Still, it should probably be safe to enable it by default as we don't
write to the nv_data.bin
This won't be perfect as ipc-test doesn't have RFS support, but in the
worst case people might try both tools and assume that for some unknown
reason ipc-test doesn't work while ipc-modem does.
> + }
> +
> + printf("[2] Starting modem_read_loop on FMT
> client\n");
> + modem_read_loop(client_fmt, client_rfs);
>
> modem_stop(client_fmt);
> + if (client_rfs != 0)
> + ipc_client_close(client_rfs);
That could be moved in modem_stop:
- That code is also repeated when "Something went wrong starting RFS
client".
- It's not very consistent to have ipc_client_close(client) in
modem_stop for the ipc_client but have the same for the RFS client
outside of that function.
- The function name (modem_stop) implies that it's generic
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
There is ongoing work to make libsamsung-ipc and the modem drivers of
the Galaxy SIII (GT-I9300) work on top of an upstream kernel instead
of kernels derived from the vendor kernels.
For that work it's convenient to be able to store the kernel the
kernel logs in systemd-journald along with the output of ipc-modem.
If the kernel is built with CONFIG_LOCALVERSION_AUTO, we can track the
exact kernel revision used. And if the modem driver crashed, we can
still have the logs in journald at the next reboot. It also enables us
to store logs over a long period of time and get back to old logs.
However when running ipc-modem through a systemd service unit, the
output is piped to systemd, and we only get the following output with
journalctl -u <service> .service:
Started [Service Unit Description].
This is because when a program is piped, line buffering changes from
line buffered (the default when outputing to stdout on a terminal
according to setbuf(3)) to fully buffered.
To fix that we add a syslog log-target. Several programs already have
a --syslog (like OpenVPN) or --log-target= (like PulseAudio) command
line argument.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
| |
This enables to add other log callbacks than printf / stdout later on.
The ipc_modem_log function is inspired from ipc_client_log.
This change should contain no functional changes.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
| |
There is no use in making multiple calls to printf when only one call
is sufficient.
It's also a good practice to do that in general, especially when part
of the code can print to the console at any time, to ensure that that
all the buffer is printed contiguously and not interleaved with other
prints.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
| |
This enables to add more definitions later and to also add functions
prototypes there.
This change contains no functional changes.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
This makes the code more readable. It should contain no functional
changes.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
| |
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
This ensures that the content of theses arrays cannot be changed at
runtime.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Without the commit 355433ab5f17f25ca202ec092d989b267fd78c19
(tools: nv_data_imei: get rid of missing initializer warning),
we have the following warnings when building Replicant 6.0:
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:245:2:
warning: missing initializer for field 'option' of 'struct
command_option' [-Wmissing-field-initializers]
{ /* Sentinel */ },
^
In file included from
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:39:0:
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.h:65:10:
note: 'option' declared here
uint8_t option;
^
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:277:2:
warning: missing initializer for field 'name' of 'struct command'
[-Wmissing-field-initializers]
{ /* Sentinel */ },
^
In file included from
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:39:0:
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.h:57:14:
note: 'name' declared here
const char *name;
^
The addition of -Werror=pedantic in guix.scm enables to detect these
as the build would then fail:
CC nv_data-imei.o
nv_data-imei.c:245:2: error:
ISO C forbids empty initializer braces [-Werror=pedantic]
245 | { /* Sentinel */ },
| ^
nv_data-imei.c:277:2: error:
ISO C forbids empty initializer braces [-Werror=pedantic]
277 | { /* Sentinel */ },
| ^
CC nv_data-md5.o
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Without that fix, we have the following warnings on Replicant 6.0:
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:245:2:
warning: missing initializer for field 'option' of 'struct
command_option' [-Wmissing-field-initializers]
{ /* Sentinel */ },
^
In file included from
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:39:0:
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.h:65:10:
note: 'option' declared here
uint8_t option;
^
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:277:2:
warning: missing initializer for field 'name' of 'struct command'
[-Wmissing-field-initializers]
{ /* Sentinel */ },
^
In file included from
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.c:39:0:
hardware/replicant/libsamsung-ipc/tools/nv_data-imei.h:57:14:
note: 'name' declared here
const char *name;
^
Note that even if the code isn't valid C17 code, the warning is
harmless since the arrays of struct is declared outside functions
(static storage duration), so the "Sentinel" will be filled with zeros
anyway.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Without that fix, when building the 'libsamsung-ipc' package through
scripts/guix.scm, we have the following warning:
CC partitions/android/android.lo
partitions/android/android.c:
In function ‘open_android_modem_partition_by_name’:
partitions/android/android.c:87:3: warning:
‘strncpy’ specified bound depends on the length of
the source argument [-Wstringop-overflow=]
87 | strncpy(path, partitions_dirnames[i],
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
88 | strlen(partitions_dirnames[i]));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CC partitions/toc/toc.lo
CCLD libsamsung-ipc.la
The warning is hamless because:
- The source (partitions_dirnames[i]) is hardcoded.
- The desitination buffer size was allocated more space than
the length of partitions_dirnames[i].
However it's good to silence non problematic warnings in order to
better detect really serious issues.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
| |
This was introduced by the commit
5a643dd89e2636cea19d9642c3a205d2d20250ec (ipc_utils: add
ipc_group_string)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Without that fix we have compilation errors:
nv_data-imei.c: In function ‘get_imei’:
nv_data-imei.c:181:12: error: format ‘%d’ expects argument of type
‘int’, but argument 2 has type ‘size_t’ {aka ‘long unsigned int’}
[-Werror=format=]
181 | printf("In addition it is also invalid"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
......
184 | len, IMEI_LENGTH);
| ~~~
| |
| size_t {aka long unsigned int}
nv_data-imei.c:183:14: note: format string is defined here
183 | "%d digits instead of %d.\n",
| ~^
| |
| int
| %ld
nv_data-imei.c:193:12: error: format ‘%d’ expects argument of type
‘int’, but argument 3 has type ‘size_t’ {aka ‘long unsigned int’}
[-Werror=format=]
193 | printf("The '%s' "
| ^~~~~~~~~~~
......
196 | imei->optarg, len, IMEI_LENGTH);
| ~~~
| |
| size_t {aka long unsigned int}
nv_data-imei.c:195:14: note: format string is defined here
195 | "%d digits instead of %d.\n",
| ~^
| |
| int
| %ld
This issue was introduced in commit
1cd1a569bef21ed3bfaeda15fb210ef6fd218152 (tools: Add a new
nv_data-imei tool based on rfs-imei)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
This makes it easier to build-test all the latest commits before
sending patches or pushing patches.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
For now -DIPC_DEVICE_NAME is only used to force to use a specific
device driver instead of trying to find out automatically which driver
to use. So with it all the drivers are still compiled in and there is
only one string that changes between builds with different
-DIPC_DEVICE_NAME.
There are some advantages in being able to not use -DIPC_DEVICE_NAME:
- If one day, only the relevant driver(s) are compiled with
-DIPC_DEVICE_NAME, we would still be able to compile-test all the
code by not setting -DIPC_DEVICE_NAME.
- Not hardcoding the device name would be a good step toward having
Android images that work on multiple devices.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
The usage of this PKGBUILD is similar to the guix.scm file in the same
directory: it can be used to test the build of the current source.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When running the tests in an out of tree build directory (here in
scripts/src/), without that fix, we have the following error in
src/samsung-ipc/tests/test-suite.log when running 'make check':
FAIL: nv_data-md5
=================
Traceback (most recent call last):
File "[...]/scripts/src/tools/../../../tools/nv_data-md5.py", line 69, in <module>
rc = main()
File "[...]/scripts/src/tools/../../../tools/nv_data-md5.py", line 64, in main
nv_data_md5 = NvDataMD5()
File "[...]/scripts/src/tools/../../../tools/nv_data-md5.py", line 39, in __init__
self.nv_data_md5 = sh.Command(srcdir + os.sep + 'nv_data-md5')
File "/usr/lib/python3.9/site-packages/sh.py", line 1342, in __init__
raise CommandNotFound(path)
sh.CommandNotFound: ../../../tools/nv_data-md5
FAIL nv_data-md5.py (exit status: 1)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When running the tests in an out of tree build directory (here in
scripts/src/), without that fix, we have the following error in
src/samsung-ipc/tests/test-suite.log when running 'make check':
FAIL: nv_data-imei
==================
Traceback (most recent call last):
File "[...]/scripts/src/tools/../../../tools/nv_data-imei.py", line 122, in <module>
rc = main()
File "[...]/scripts/src/tools/../../../tools/nv_data-imei.py", line 117, in main
nv_data_imei = NvDataImei()
File "[...]/scripts/src/tools/../../../tools/nv_data-imei.py", line 53, in __init__
self.nv_data_imei = sh.Command(srcdir + os.sep + 'nv_data-imei')
File "/usr/lib/python3.9/site-packages/sh.py", line 1342, in __init__
raise CommandNotFound(path)
sh.CommandNotFound: ../../../tools/nv_data-imei
FAIL nv_data-imei.py (exit status: 1)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When running the tests in an out of tree build directory (here in
scripts/src/), without that fix, we have the following error in
src/samsung-ipc/tests/test-suite.log when running 'make check':
FAIL: ipc-modem
===============
Traceback (most recent call last):
File "[...]/scripts/src/tools/../../../tools/ipc-modem.py", line 79, in <module>
rc = main()
File "[...]/scripts/src/tools/../../../tools/ipc-modem.py", line 74, in main
ipc_modem = IpcModem()
File "[...]/scripts/src/tools/../../../tools/ipc-modem.py", line 39, in __init__
ipc_modem = sh.Command(srcdir + os.sep + 'ipc-modem')
File "/usr/lib/python3.9/site-packages/sh.py", line 1342, in __init__
raise CommandNotFound(path)
sh.CommandNotFound: ../../../tools/ipc-modem
FAIL ipc-modem.py (exit status: 1)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When running the tests in an out of tree build directory (here in
scripts/src/), without that fix, we have the following error in
src/samsung-ipc/tests/test-suite.log when running 'make check':
============================================================
libsamsung-ipc 0.7.0: samsung-ipc/tests/test-suite.log
============================================================
# TOTAL: 1
# PASS: 0
# SKIP: 0
# XFAIL: 0
# FAIL: 1
# XPASS: 0
# ERROR: 0
.. contents:: :depth: 2
FAIL: libsamsung-ipc-test
=========================
Traceback (most recent call last):
File "[...]/scripts/src/samsung-ipc/tests/../../../../samsung-ipc/tests/libsamsung-ipc-test.py", line 54, in <module>
main()
File "[...]/scripts/src/samsung-ipc/tests/../../../../samsung-ipc/tests/libsamsung-ipc-test.py", line 50, in main
tests = libsamsung_ipc_test()
File "[...]/scripts/src/samsung-ipc/tests/../../../../samsung-ipc/tests/libsamsung-ipc-test.py", line 33, in __init__
self.run = sh.Command(srcdir + os.sep + "libsamsung-ipc-test")
File "/usr/lib/python3.9/site-packages/sh.py", line 1342, in __init__
raise CommandNotFound(path)
sh.CommandNotFound: ../../../../samsung-ipc/tests/libsamsung-ipc-test
FAIL libsamsung-ipc-test.py (exit status: 1)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Having a code style makes the code easier to read.
As for which code style to use, the code style defined by the
PEP 8 [1] is used in the python standard library and in the main
Python distribution.
Its official nature (it's standardized by python) probably makes it
the most well known and used code style adopted by python programmers.
In it we have a section about string quotes:
String Quotes
-------------
In Python, single-quoted strings and double-quoted strings are the
same. This PEP does not make a recommendation for this. Pick a rule
and stick to it. When a string contains single or double quote
characters, however, use the other one to avoid backslashes in the
string. It improves readability.
For triple-quoted strings, always use double quote characters to be
consistent with the docstring convention in PEP 257.
Since "if __name__ == '__main__':" is widely used, we choose to use the
single quotes.
[1]https://www.python.org/dev/peps/pep-0008/
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
On Guix, if we have all the required dependencies installed, running
./autogen.sh && ./configure && make fails with the following error:
CCLD libsamsung-ipc-test
ld: /gnu/store/cm6wv72061h65lri2aqgwa8bvncls51h-openssl-1.1.1l/lib/libcrypto.so: undefined reference to `fstat@GLIBC_2.33'
ld: /gnu/store/cm6wv72061h65lri2aqgwa8bvncls51h-openssl-1.1.1l/lib/libcrypto.so: undefined reference to `stat@GLIBC_2.33'
The same command works when using guix shell or guix environment
(still with all the dependencies).
Having a manifest file in libsamsung-ipc will enable Guix system users
to easily work on libsamsung-ipc without having to use trial and error
to get a working setup.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
While the tests don't talk to a real modem, they are still useful
to detect regressions with the use of threads.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
| |
This enables to test things like the threads usage without the
need for a real modem implementing the samsung-ipc protocol. This
could for instance be used in libsamsung-ipc automated tests.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
As this is done once all options have been parsed,
it enables to only print some options when debug is on.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
| |
This enables to more easily enables to share the parsed command
line arguments across different functions and it also enables
to add new command line arguments more easily.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Without that fix, if some arguments were set and no
command was given, ipc-modem would proceed anyway:
$ ./tools/ipc-modem --debug
[I] Debug enabled
[...]
As it's expected to at least be used with a command
to do something meaningful, without that fix, it
could misslead users into thinking that everything
is fine when it's not or vice versa.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
| |
This makes the code easier to read as we separate the command line
handling from the IPC code.
In addition it also enables to more easily add RFS support later
on as the IPC code is not mixed with command line handling.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
While --help is parsed in the command line, it wasn't described
in the help itself.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
| |
The help and the code is sorted alphabetically.
It should contain no functional changes.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When working on applications using libsamsung-ipc, we sometimes
have functions that have an ipc client type argument and that
work for all 3 ipc client types, or want to refactorize the
code to do that in order to make the code more clean and generic.
However in these cases, these functions often needed to output
some error message or tell users what is going on through
logging prints, and the code ends up being way cleaner if there
is a generic function to get the name of the ipc client type.
In many cases it makes sense not to use the full
IPC_CLIENT_TYPE_<type> name but only the <name> type in these
messages, so because it's easier to add IPC_CLIENT_TYPE_ to the
<type> than removing it, it makes sense to only return the string
associated to the type (like "FMT", "RFS" or "DUMMY".
The least significant number of the library version was also
bumped as we are adding a new function, but the applications that
were built against older libsamsung-ipc revisions should still work.
However applications that depends on this ipc_client_type_string
will not work with previous versions of libsamsung-ipc.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
| |
There are already 4 string functions and combined together, they
already take more than 300 lines, so it makes sense to move
them in a separate file.
In addition, it will also clarify in which files new string functions
are supposed to be added in.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In Replicant 6.0, ipc-modem is shipped in the images.
When it will make it in released images, it will be very useful for
regression testing and for tests that don't involve the full Android
stack[1].
For such tests to work, users are expected to disable the modem (with
'modem.sh off') before.
In this configuration, the test utilities like ipc-modem use the
system libsamsung-ipc as they are linked dynamically against it.
However if we want to also do regression testing with code that
is being worked on, it's very convenient to build test utilities
that have libsamsung-ipc built-in: This way it can test our the
code that is being worked on without interfering in any way with
the Android stack.
The static version of ipc-modem can for instance be built with the
following commands in the Replicant source code directory:
$ source build/envsetup.sh
$ lunch replicant_i9300-userdebug
$ make ipc-modem-static
It can then be run with the following commands (for the GT-I9300):
$ adb root
$ adb remount
$ adb push \
out/target/product/i9300/system/bin/ipc-modem-static \
/system/bin/
$ adb shell "modem.sh off"
$ adb wait-for-device
$ adb root
$ adb shell ipc-modem-static [...]
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
| |
Running tests (and even just running autogen.sh) produces
files used during the tests, and so git has to ignore these
as they are produced during the build procedure.
Ideally they should have been added to gitignore when I added
the tests that generated these files but I forgot.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
Without this patch, after compiling libsamsung-ipc, git status
shows tools/nv_data-imei in the list of untracked files.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
| |
Text editors produce temporary files. They can often be used to
recover data in case of computer crash or abrupt shut down.
However when using text editors like Emacs and Vim, temporary
files often end up in the source directory, so as they are not
part of the official libsamsung-ipc source code we need to tell
git to ignore them.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When compiling nv-data-md5 under Replicant 6 we have:
hardware/replicant/libsamsung-ipc/tools/nv_data-md5.c:
In function 'main':
hardware/replicant/libsamsung-ipc/tools/nv_data-md5.c:103:1:
error:
control reaches end of non-void function [-Werror=return-type]
}
^
cc1: some warnings being treated as errors
So we simply need to add a return in the error path.
While we're at it, we use an exit code referenced in sysexits.h
to follow a standard. This can help differentiating between
different types of errors.
Thanks to that, it can also simplify the code when writing tests.
As Android doesn't have sysexits.h, we also need to use the one that
has been copied inside libsamsung-ipc in tools/include/glibc, else
it would also fail to compile because of the missing header file.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The ipc-modem tool contained commented code to call a specific
phone number.
This patch reuses that code and retrieve the number to call from
the command line.
The number to call is expected to be in the same country as the
caller SIM card and network. Adding support for international
calls could be done by computing the right prefix instead of
hardcoding the IPC_CALL_PREFIX_NONE prefix.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
| |
If for instance "1234" is given as pin, the size of optarg
should be 5 but memcpy would copy 8.
In addition, the current code also makes sure that there is a
terminating null byte ('\0') inside the sim_pin array.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
| |
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
| |
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Libraries typically have a public API and some internal
implementation of that API. In libsamsung-ipc, the public
API is defined in the include/ directory from the top directory.
When compiling and installing libsamsung-ipc, that include/
directory is installed as well.
Anything that is not defined in include/ is not part of that
public API.
However here we need to precisely test functions that are not
part of that public API: The open_android_modem_partition
function being tested here is only used by the herolte device,
and that device doesn't have a battery that is easily replaceable,
so most Replicant contributors will not want to get that device.
As currently all the non static symbols of libsamsung-ipc are
exported and that open_android_modem_partition isn't static,
we can simply link to libsamsung-ipc for now and use its
internal headers to access the functions to test.
If at some point, libsamsung-ipc only exports the symbols defined
in include/ we would need to find other ways to run such tests,
for instance by using test frameworks that take care of that or by
compiling libsamsung-ipc source code into the test programs.
Some of the code of the libsamsung-ipc-test utility was adapted
from code from the nv_data-md5 utility (which is currently in
tools/).
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
| |
This code could also be useful for other devices and in any cases
it's not device specific.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
While libsamsung-ipc is not limited to Replicant (it can be used
on GNU/Linux or in other Android distributions), it is currently
maintained by Replicant.
In order to make that more clear, it was renamed by the commit
aa4f61805b2d8943e94847cae825ec209060569e ("Move libsamsung-ril
and libsamsung-ipc in vendor/replicant") in the Replicant manifest
repository[1].
Because of that we also need to adjust the git URL in the MAINTAINERS
file.
In addition we use https because it's safer than git:// as it uses
TLS, and because we have redirects in place for http(s) but not for
git://, so if the repository URL changes again, the old URL will
most likely continue to work with http(s).
[1]https://git.replicant.us/replicant/manifest.git
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
| |
This could enable other devices to use the TOC handling functions.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
| |
The samsung-ipc/Makefile.am became pretty big.
Because of that, it's It's better to split it to have one
Makefile.am per subdirectory.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
If we configure Replicant 11, libsamsung-ipc and libsamsung-ril
to have both libraries in /system/lib/, tools work fine on the
Galaxy SIII (GT-I9300):
# find -name libsamsung-ril.so 2>/dev/null
./system/lib/libsamsung-ril.so
# find -name libsamsung-ipc.so 2>/dev/null
./system/lib/libsamsung-ipc.so
# find -name nv_data-md5 2>/dev/null
./system/bin/nv_data-md5
# nv_data-md5
Usage: nv_data-md5 [nv_data.bin]
And libsamsung-ril tries to load libsamsung-ipc from the right
location in /system/lib/, but it fails due to the vendor and system
separation:
01-14 15:50:57.739 1475 1475 E RILD : dlopen failed:
dlopen failed: library "/system/lib/libsamsung-ril.so"
needed or dlopened by "/system/vendor/bin/hw/rild" is not
accessible for the namespace "(default)"
Adding LOCAL_PROPRIETARY_MODULE fixes that. Note that the name
of that property can be misleading here: libsamsung-ipc is free
software and shall remain free software.
Instead we need to understand LOCAL_PROPRIETARY_MODULE as a way
to tell the Android build system that libsamsung-ipc is not part
of the base Android code but instead that it is code that is
specific to a device, set of devices and/or Android distribution.
LOCAL_MODULE_RELATIVE_PATH cannot be used instead: even if the
binaies end up in /vendor/bin/hw/, and that the libraries ends
up in /vendor/lib/hw/, for some reasons the libraries can't be
found:
i9300:/ # /system/vendor/bin/hw/nv_data-md5
CANNOT LINK EXECUTABLE "/system/vendor/bin/hw/nv_data-md5":
library "libsamsung-ipc.so" not found: needed by main executable
And most importantly, rild has the same issue than before:
E RILD : dlopen failed: dlopen failed: library
"libsamsung-ipc.so" not found: needed by
/system/vendor/lib/hw/libsamsung-ril.so
in namespace (default)
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
A previous version of this patch was tested on the herolte,
however since then, several light functional changes were
introduced.
With the previous patch, it was possible to boot the modem
and it was probably possible to send message to, and receive
messages from the modem.
For the TARGET_DEVICE we use in our Android.mk, In the
Android.mk of android_device_samsung_herolte[1] we have:
ifneq ($(filter herolte, $(TARGET_DEVICE)),)
include $(call all-makefiles-under,$(LOCAL_PATH))
endif
so we can safely assume that the TARGET_DEVICE is herolte.
[1]https://github.com/LineageOS/android_device_samsung_herolte
Signed-off-by: Tony Garnock-Jones <tonyg@leastfixedpoint.com>
GNUtoo: rebased, code cleanup, more debug prints,
commit message content but not its summary.
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
| |
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
| |
|
|
| |
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
|
