ipc-modem: Add support for RFSpatches-todo/ipc-modem-rfs-from-Tony-Garnock-Jones

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:
=====
- Check the pthread implementation, especially check if the threads
  do exit cleanly.
- Finish addressing all the review concerns
- Send it for review again (if possible involve Tony)

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;
> }

I've looked rapidly and it seems that it would work on most of the
devices with an XMM626 modem, because in xmm626_kernel_smdk4412_open
the ipc channels kernel dev nodes are opened in non blocking mode:
> case IPC_CLIENT_TYPE_FMT:
> 	fd = open(XMM626_SEC_MODEM_IPC0_DEVICE,
> 		  O_RDWR | O_NOCTTY | O_NONBLOCK);
> 	break;
> case IPC_CLIENT_TYPE_RFS:
> 	fd = open(XMM626_SEC_MODEM_RFS0_DEVICE,
> 		  O_RDWR | O_NOCTTY | O_NONBLOCK);
> 	break;

However it doesn't seem to be the case for devices like aries (Galaxy S
(GT-I9000)), and I might be wrong with the above too as I didn't fully
check if it was completely not blocking (I don't remember if select can
still block somehow when the fd is opened in non-blocking mode).

So the solution here is either to spawn two threads or to make the code
handle either the RFS or FMT channels but not both at the same time.

Using threads has the advantage that it's better for users as they need
to run only one tool, but it's longer and more complicated to implement
as it needs more code.

If you choose to not use threads, it would be a really good idea to
find a way to add a very visible warning that users won't miss in order
to tell them to remember to also run the tool with the other channel
(so --rfs if --fmt is being used and vice versa). A big warning sign
should be good enough for that. For instance:
> +------------------------------------------------------------------+
> | /!\ Don't forget to also run this tool with --fmt, otherwise it  |
> | won't work on some devices like the Galaxy S 7 variants with the |
> | herolte code name.                                               |
> +------------------------------------------------------------------+
We don't need to scare users but just tell them why this is needed.
If you find some generic enough text not to need two variants (for fmt
and rfs) that would also work fine.

>  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>

3 files changed, 168 insertions, 26 deletions

diff --git a/samsung-ipc/ipc_utils.c b/samsung-ipc/ipc_utils.c
index d8b69b7..3b824f2 100644
--- a/samsung-ipc/ipc_utils.c
+++ b/samsung-ipc/ipc_utils.c
@@ -33,6 +33,24 @@
 
 #include "ipc.h"
 
+const char *ipc_client_string(unsigned char type)
+{
+	static char type_string[5] = { 0 };
+
+	switch (type) {
+	case IPC_CLIENT_TYPE_FMT:
+		return "FMT";
+	case IPC_CLIENT_TYPE_RFS:
+		return "RFS";
+	case IPC_CLIENT_TYPE_DUMMY:
+		return "DUMMY";
+	default:
+		snprintf((char *) &type_string, sizeof(type_string),
+			 "0x%02x", type);
+		return type_string;
+	}
+}
+
 int ipc_seq_valid(unsigned char seq)
 {
 	if (seq == 0x00 || seq == 0xff)
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 4d5fa8b..4d1663f 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -20,7 +20,7 @@ TESTS = nv_data-imei.py \
 
 ipc_modem_SOURCES = ipc-modem.c
 ipc_modem_LDADD = $(top_builddir)/samsung-ipc/libsamsung-ipc.la
-ipc_modem_LDFLAGS =
+ipc_modem_LDFLAGS = -lpthread
 
 ipc_test_SOURCES = ipc-test.c
 ipc_test_LDADD = $(top_builddir)/samsung-ipc/libsamsung-ipc.la
diff --git a/tools/ipc-modem.c b/tools/ipc-modem.c
index 9314de6..2492092 100644
--- a/tools/ipc-modem.c
+++ b/tools/ipc-modem.c
@@ -50,6 +50,14 @@ int call_done;
 
 char sim_pin[8];
 
+static struct ipc_modem_client {
+	const char *name;
+	pthread_t thread;
+	pthread_mutex_t mutex;
+	pthread_attr_t attr;
+	struct ipc_client *client;
+};
+
 int seq_get(void)
 {
 	if (seq == 0xff)
@@ -92,7 +100,6 @@ void modem_snd_audio_path_ctrl(struct ipc_client *client)
 			IPC_TYPE_SET, (void *) &data, 1);
 }
 
-
 void modem_exec_call_out(struct ipc_client *client, char *num)
 {
 	struct ipc_call_outgoing_data call_out;
@@ -351,7 +358,8 @@ void modem_response_net(__attribute__((unused)) struct ipc_client *client,
 	}
 }
 
-void modem_response_handle(struct ipc_client *client, struct ipc_message *resp)
+void modem_response_fmt_handle(struct ipc_client *client,
+			       struct ipc_message *resp)
 {
 	switch (IPC_GROUP(resp->command)) {
 	case IPC_GROUP_NET:
@@ -376,36 +384,63 @@ void modem_response_handle(struct ipc_client *client, struct ipc_message *resp)
 	}
 }
 
-
-int modem_read_loop(struct ipc_client *client)
+int modem_read(struct ipc_client *client, char const *channel_name)
 {
 	struct ipc_message resp;
+	struct timeval timeout;
 	int rc;
 
 	memset(&resp, 0, sizeof(resp));
 
+	timeout.tv_sec = 0;
+	timeout.tv_usec = 50000;
+	rc = ipc_client_poll(client, NULL, &timeout);
+	if (rc < 0) {
+		printf("[E] poll of %s client failed\n", channel_name);
+		return -1;
+	}
+
+	if (rc == 0) {
+		/* timeout */
+		return 0;
+	}
+
+	rc = ipc_client_recv(client, &resp);
+	if (rc < 0) {
+		printf("[E] "
+		       "Can't RECV from %s channel: please run this again"
+		       "\n", channel_name);
+		return -1;
+	}
+
+	/* We have no RFS handler (yet) in ipc-modem */
+	if (channel_name == "FMT")
+		modem_response_fmt_handle(client, &resp);
+
+	if (resp.data != NULL)
+		free(resp.data);
+
+	return rc;
+}
+
+void modem_read_loop(struct ipc_modem_client *modem_client)
+{
+	int rc;
+
 	while (1) {
 		usleep(3000);
 
-		rc = ipc_client_poll(client, NULL, NULL);
-		if (rc < 0)
-			continue;
+		rc = modem_read(modem_client->client, modem_client->name);
 
-		rc = ipc_client_recv(client, &resp);
-		if (rc < 0) {
-			printf("[E] "
-			       "Can't RECV from modem: please run this again"
-			       "\n");
+		switch (rc) {
+		case -1:
+			return;
+		case 0:
+			break;
+		default:
 			break;
 		}
-
-		modem_response_handle(client, &resp);
-
-		if (resp.data != NULL)
-			free(resp.data);
 	}
-
-	return 0;
 }
 
 void modem_log_handler(__attribute__((unused)) void *user_data,
@@ -466,6 +501,45 @@ int modem_stop(struct ipc_client *client)
 	return 0;
 }
 
+int modem_create_channel_thread(struct ipc_client *client, const char* name)
+{
+	int rc;
+
+	struct ipc_modem_client modem_client;
+
+	printf("[2] Starting modem_read_loop on %s client\n", name);
+
+	modem_client.client = client;
+	modem_client.name = name;
+
+	rc = pthread_attr_init(&modem_client.attr);
+	if (rc) {
+		printf("[E] %s pthread_attr_init failed with error %d\n",
+		       name, rc);
+		return rc;
+	}
+
+	rc = pthread_attr_setdetachstate(&modem_client.attr,
+					 PTHREAD_CREATE_DETACHED);
+	if (rc) {
+		printf("[E] %s pthread_attr_setdetachstate failed with error %d\n",
+		       name, rc);
+		return rc;
+	}
+
+	rc = pthread_create(&modem_client.thread,
+			    &modem_client.attr,
+			    (void *) modem_read_loop,
+			    (void *) &modem_client);
+	if (rc) {
+		printf("[E] %s pthread_create failed with error %d\n",
+		       name, rc);
+		return rc;
+	}
+
+	return rc;
+}
+
 void print_help(void)
 {
 	printf("usage: ipc-modem <command>\n");
@@ -476,20 +550,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 +590,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 +617,31 @@ 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 +661,40 @@ 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);
+			/* RFS should be started before 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;
+				}
+				rc = modem_create_channel_thread(client_rfs,
+								 "RFS");
+				if (rc)
+					return rc;
+
+			} else {
+				printf("[1] Skipping modem RFS client start\n");
+			}
+
+			rc = modem_create_channel_thread(client_fmt, "FMT");
+			if (rc)
+				return rc;
 
-			modem_stop(client_fmt);
 		} else {
 			printf("[E] Unknown argument: '%s'\n", argv[optind]);
 			print_help();
@@ -585,6 +707,8 @@ int main(int argc, char *argv[])
 modem_quit:
 	if (client_fmt != 0)
 		ipc_client_destroy(client_fmt);
+	if (client_rfs != 0)
+		ipc_client_destroy(client_rfs);
 
 	return 0;
 }