[Replicant] [RFC PATCH 2/3] Quick-and-dirty support for FMT and RFS simultaneously in ipc-modem

Denis 'GNUtoo' Carikli GNUtoo at cyberdimension.org
Wed Sep 30 18:03:04 UTC 2020


On Tue, 29 Sep 2020 22:03:07 +0200
Tony Garnock-Jones <tonyg at leastfixedpoint.com> wrote:

Thanks a lot for the patch.

I've replied to what need to be fixed below.

Note that I might still have missed some things as I did only a first
pass here (it's starting to be late here and I need to go to eat but I
also don't want to delay too much the reply).

> Subject: [Replicant] [RFC PATCH 2/3] Quick-and-dirty support for FMT
> and RFS  simultaneously in ipc-modem

This patch lacks a proper commit message. Here's an example that can be
used for that:
>     ipc-modem: Add support for RFS
>     
>     This enables to test the RFS channel as well.
>     
>     In addition with the Galaxy S7 variants with the herolte codename,
>     a check in at least one of its kernels 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;
>     >       }
>     > [...]
>     > }
>     
>     With this RFS support, this should not be a problem anymore.
>     
>     [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 at leastfixedpoint.com>

In addition of the comments below, it contains whitespace issues and
lines over 80 characters, so that will need to be fixed.

We imported the checkpatch.pl script from Linux to help finding such
issues, however that this tool is not perfect as there are sometimes
conflicting rules.

For instance when fixing this patch, you still end up with the choice
between having lines over 80 characters and quoted strings split across
multiple lines. In that case the best is just to split the strings in a
way that minimize the risk of not finding it when grepping the source
code.

The nice thing about the way this tool is supposed to be used is that
it's not run automatically, so we can simply ignore some of the
suggestions if we have good reasons to do that.

> --- a/tools/ipc-modem.c
> +++ b/tools/ipc-modem.c
> @@ -377,35 +377,67 @@ void modem_response_handle(struct ipc_client
> *client, struct ipc_message *resp) }
>  
>  
> -int modem_read_loop(struct ipc_client *client)
> +int modem_poll_client(struct ipc_client *client, char const
That could probably be renamed in something better like modem_read:
- modem_read_loop has the main loop so it would be consistent with that
- This new function doesn't only poll but also reads the modem data if
  there is some. The 'while (1) {read();}' summary looks more accurate
  than 'while(1) {poll/select}'.

> *variation) {
Variation could at least be renamed in something more meaningful like:
- channel_name (the kernel name for FMT/RFS)
- client_type_name

Alternatively we have can easily differentiate between FMT and RFS
through client->type. The type is defined in include/samsung-ipc.h with:
> #define IPC_CLIENT_TYPE_FMT	0x00
> #define IPC_CLIENT_TYPE_RFS	0x01
> #define IPC_CLIENT_TYPE_DUMMY 0x02

So we could do some if or even go as far as adding something like that
in samsung-ipc/ipc_utils.c for that:
> const char *ipc_client_type_string(unsigned char type)
> {
> 	static char type_string[5] = { 0 };
> 
> 	switch (type) {
> 	case IPC_CLIENT_TYPE_FMT:
> 		return "IPC_CLIENT_TYPE_FMT";
>	case IPC_CLIENT_TYPE_RFS:
>		return "IPC_CLIENT_TYPE_RFS";
> 	case IPC_CLIENT_TYPE_DUMMY:
>		return "IPC_CLIENT_TYPE_DUMMY";
> 	default:
> 		snprintf((char *) &type_string, sizeof(type_string),
> 			 "0x%02x", type);
>		return type_string;
> 	}
> }
If you go this route, do you want to do that patch? or do you want me
to do it? Just changing the variable name also work for me.

>  	struct ipc_message resp;
> -	int rc;
> +        int rc;
> +        struct timeval timeout;
>  
>  	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", variation);
> +                return -1;
> +        }
> +
> +        if (rc == 0) {
> +                /* timeout. */
It's really nice to add comments: As the libsamsung-ipc code is not
much documented that helps understanding it.

The dot in the comment can be removed here as it's not necessary.


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

> +        }
> +
> +        rc = ipc_client_recv(client, &resp);
> +        if (rc < 0) {
> +                printf("[E] "
> +                       "Can't RECV from modem %s: please run this
> again"
> +                       "\n", variation);
> +                return -1;
> +        }
> +
> +        modem_response_handle(client, &resp);
This could be an issue as it would end up handling the RFS messages
with by comparing them to IPC messages.

You could do something like that to fix that for instance:
> if (client->type == IPC_CLIENT_TYPE_FMT)
> 	modem_response_handle(client, &resp);

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

Thanks again for this patch.

Denis.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.osuosl.org/pipermail/replicant/attachments/20200930/acdd4df0/attachment.asc>


More information about the Replicant mailing list