[Replicant] [RFC PATCH 1/3] Initial support for herolte (Samsung Galaxy S7 GSM).
Denis 'GNUtoo' Carikli
GNUtoo at cyberdimension.org
Fri Oct 2 16:36:07 UTC 2020
> From: Tony Garnock-Jones <tonyg at leastfixedpoint.com>
> To: replicant at osuosl.org
> Subject: [Replicant] [RFC PATCH 1/3] Initial support for herolte
> (Samsung Galaxy S7 GSM).
Even if it wasn't done for the previous patches adding new devices,
it would be a good idea to add the status.
For instance:
> Status:
> - The phone calls connect, both inbound and outbound
> - Audio is not yet tested
>
> It was tested with 'ipc-modem --debug --rfs start'
>
> - it will not interact with userland unless something opens
> /dev/umts_rfs0 at the same time as /dev/umts_ipc0 is open (see uses
> of "rild_ready()" in the kernel source code). The patchset includes
> a quick-and-dirty RFS read loop incorporated into ipc-modem:
>
> ipc-modem --debug --rfs start
This would help a lot to understand what is supposed to work and what
is not. That device will probably never be supported by Replicant, so
people wanting to use libsamsung-ipc on it might have very different
environments, and so having that information would help them a lot .
Like the ipc-modem patch, the whitespaces and some minor style issues
will also need to be fixed at some point (checkpatch helps a lot with
that).
> #include <sys/ioctl.h>
> #include <sys/stat.h>
> #include <sys/types.h>
>
> #include <fcntl.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <errno.h>
>
> #include "ipc.h"
> #include "devices/herolte/herolte.h"
> #include "modems/xmm626/xmm626.h"
> #include "modems/xmm626/xmm626_modem_prj.h"
> #include "modems/xmm626/xmm626_kernel_smdk4412.h"
Ideally the includes should be ordered alphabetically: this helps to
not include multiple times the same header by mistake.
> struct __attribute__((__packed__)) modem_firmware {
> uint64_t binary;
> uint32_t size;
> uint32_t m_offset;
> uint32_t b_offset;
> uint32_t mode;
> uint32_t len;
> };
This should probably be renamed to modem_firmware_partition or
something that indicates that it represent a speccific partition of the
modem firmware.
> /* We don't know, yet, details of the TOC format; for now, we assume two things:
> 1. reading 512 bytes of TOC is enough, and
> 2. the first entry with an empty name field ends the list. */
As some details are known thanks to your research, you could instead
point that we don't know all the details. For instance:
> /* We don't know all the details of the TOC format yet; for now, we
> * assume two things:
> * 1. reading 512 bytes of TOC is enough, and
> * 2. the first entry with an empty name field ends the list.
> */
> static int upload_chunk(struct ipc_client *client,
> int device_fd,
> int firmware_fd,
> struct firmware_toc_entry const *toc,
> char const *name,
> uint32_t *size)
> {
> int rc = -1;
> uint8_t *buffer = NULL;
> struct modem_firmware header;
> struct firmware_toc_entry const *boot_toc_entry;
> struct firmware_toc_entry const *current_toc_entry;
> uint32_t remaining;
>
> ipc_client_log(client, "Uploading %s", name);
>
> boot_toc_entry = find_toc_entry("BOOT", toc);
> if (boot_toc_entry == NULL)
> goto exit;
I think that we should add prints in case of failure. For instance:
> ipc_client_log(client, "Failed to find BOOT entry in the TOC");
This will make finding potential issue much faster for people not
already familiar with the code. Even if we can read the code and
pinpoint the location where it failed (if it does fail) due to the
things that it will not print if it fails, I find it way much faster
to debug issues if it's already telling why it fails.
> current_toc_entry = find_toc_entry(name, toc);
> if (current_toc_entry == NULL)
> goto exit;
Same here (for the prints).
> if (size != NULL)
> *size = current_toc_entry->size;
> ipc_client_log(client, " - blob size for %s is %lu", name, current_toc_entry->size);
>
> buffer = calloc(1, MAX_CHUNK_LEN);
> if (buffer == NULL)
> goto exit;
same here for the prints.
> header.binary = (uint64_t) buffer;
This creates the following compilation warning with some strict CFLAGS:
> devices/herolte/herolte.c: In function ‘upload_chunk’:
> devices/herolte/herolte.c:121:25: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
> 121 | header.binary = (uint64_t) buffer;
> | ^
This will really need to be fixed somehow as otherwise it would break the build
on Replicant >= 9. Here is the script I used to test that on GNU/Linux:
> #!/bin/sh
> export CFLAGS="-W -Wall -Wno-unused --no-create --no-recursion"
>
> ./autogen.sh
> ./configure CC=gcc CFLAGS="${CFLAGS}"
> make clean
> make -j3
>
> ./configure CC=clang CFLAGS="${CFLAGS}"
> make clean
> make -j3
> if (lseek(firmware_fd, header.b_offset, SEEK_SET) < 0)
> goto exit;
Here too we need to add some print (ipc_client_log) in case of failure.
> remaining = header.size;
> while (remaining > 0) {
> header.len = remaining < MAX_CHUNK_LEN ? remaining : MAX_CHUNK_LEN;
> if (read(firmware_fd, buffer, header.len) != header.len)
> goto exit;
man 2 read has the following in "RETURN VALUE": "It is not an error
if this number [the return value] is smaller than the number of bytes
requested; this may happen for example because fewer bytes are actually
available right now (maybe because we were close to end-of-file, or
because we are reading from a pipe, or from a terminal), or because
read() was interrupted by a signal."
So even if it rarely happens, we should still fix it. For instance
something like that can be done instead instead:
[...]
while (remaining > 0) {
int count;
int err;
header.len = (remaining < MAX_CHUNK_LEN) ?
remaining : MAX_CHUNK_LEN;
count = read(firmware_fd, buffer, header.len);
if (count == -1) {
err = errno;
ipc_client_log(client, "%s read error %d with %s: %s",
__func__, err, name, strerror(err));
goto exit;
}
err = ioctl(device_fd, IOCTL_DPRAM_SEND_BOOT, &header)
if (err < 0) {
ipc_client_log(client, "%s: IOCTL_DPRAM_SEND_BOOT "
"ioctl failed with %s with error %d",
__func__, name, err);
goto exit;
}
header.m_offset += header.len;
header.b_offset += header.len;
remaining -= count;
}
> if (ioctl(device_fd, IOCTL_DPRAM_SEND_BOOT, &header) < 0)
> goto exit;
Here we need a print too in case of error.
> static int select_secure_mode(struct ipc_client *client,
> int boot0_fd,
> int secure,
> uint32_t size_boot,
> uint32_t size_main)
> {
> struct security_req req;
>
> ipc_client_log(client,
> "Issuing IOCTL_SECURITY_REQ - setting %s mode",
> secure ? "secure" : "insecure");
>
> req.mode = secure ? 0 : 2;
> req.size_boot = size_boot;
> req.size_main = size_main;
> req.pad_zero = 0;
>
> if (ioctl(boot0_fd, IOCTL_SECURITY_REQ, &req) < 0)
Here it needs a print in case of error too.
> static char const * const modem_image_devices[] = {
> "/dev/disk/by-partlabel/RADIO", /* PostmarketOS */
> "/dev/block/platform/155a0000.ufs/by-name/RADIO", /* LineageOS */
> NULL
> };
While you're at it the comments could also be aligned. They are really
nice though as it's probably not obvious for most people which is which
and it also explain why this modem_image_devices is needed in a self
explanatory way.
> static int open_image_device(struct ipc_client *client)
> {
> for (int i = 0; modem_image_devices[i] != NULL; i++) {
Even if that's valid C89 and that it compiles fine, it's probably
better to do that instead for consistency accross how it's done in
libsamsung-ipc and Linux whose who borrow the code style from:
> int i;
>
> for (i = 0; modem_image_devices[i] != NULL; i++) {
> int herolte_boot(struct ipc_client *client)
> {
> [...]
> imagefd = open_image_device(client);
> if (imagefd < 0)
> goto exit;
Here we really need some prints in case of open failure, and in a way
that is verbose enough to understand why it failed. For instance:
imagefd = open_image_device(client);
if (imagefd == -1) {
int err = errno;
ipc_client_log(client,
"%s: open_image_device failed with error %d: %s",
__func__, err, strerror(err));
goto exit;
}
This way it would catch all permission denied and other similar issues.
> if (read(imagefd, &toc[0], sizeof(toc)) != sizeof(toc))
> goto exit;
The same comments than before apply:
- We need a loop to read the data as it could read a size < sizeof(toc)
- We also need a print in case of error.
> ipc_client_log(client, "Loaded firmware TOC");
>
> nvfd = open(herolte_nv_data_specs.nv_data_path, O_RDONLY | O_NOCTTY);
> if (nvfd < 0)
> goto exit;
Same here, here we also really need a verbose print like the other open before:
nv_data_path is set to /efs/nv_data.bin, and on most devices the EFS filesystem
is in an ext4 filesystem. In Android, the uid<->gid are hardcoded somewhere in
some C files (I don't remember where though), and so it might differ from the
/etc/passwd in use in various GNU/Linux distributions. So if for some reasons
libsamsung-ipc doesn't run as root or that for some other reasons it cannot
access the files we really need to inform users about it in the log.
> ipc_client_log(client, "Opened NV data file");
>
> boot0_fd = open(XMM626_SEC_MODEM_BOOT0_DEVICE, O_RDWR | O_NOCTTY);
> if (boot0_fd < 0)
> goto exit;
Same here, we need some prints.
> ipc_client_log(client, "Resetting modem");
> if (ioctl(boot0_fd, IOCTL_MODEM_RESET, 0) < 0)
> goto exit;
Here too it would be good to have some prints.
> if (select_secure_mode(client, boot0_fd, 0, 0, 0) < 0)
> goto exit;
>
> if (upload_chunk(client, boot0_fd, imagefd, toc, "BOOT", &size_boot) < 0)
> goto exit;
>
> if (upload_chunk(client, boot0_fd, imagefd, toc, "MAIN", &size_main) < 0)
> goto exit;
>
> if (upload_chunk(client, boot0_fd, nvfd, toc, "NV", NULL) < 0)
> goto exit;
> if (select_secure_mode(client, boot0_fd, 1, size_boot, size_main) < 0)
> goto exit;
Here the prints aren't necessary if you choose to add them in upload_chunk and in
select_secure_mode.
> ipc_client_log(client, "Powering on modem");
> if (xmm626_kernel_smdk4412_power(client, boot0_fd, 1) < 0)
> goto exit;
Here we also need to add prints. For instance:
ipc_client_log(client, "Powering on modem");
if (xmm626_kernel_smdk4412_power(client, boot0_fd, 1) < 0) {
ipc_client_log(client, "Powering on modem failed");
goto exit;
}
> ipc_client_log(client, "Starting modem boot process");
> if (xmm626_kernel_smdk4412_boot_power(client, boot0_fd, 1) < 0)
> goto exit;
>
> ipc_client_log(client, "Kicking off firmware download");
> if (ioctl(boot0_fd, IOCTL_MODEM_DL_START, 0) < 0)
> goto exit;
Same thing here (for the prints).
> ipc_client_log(client, "Handshaking with modem");
> /* At this point, cbd engages in a little dance with the
> * newly-booted modem, apparently to verify that it is running
> * as expected. I don’t know the sources of these magic
> * numbers, I just faithfully reproduce them. */
> {
> uint32_t buf;
>
> buf = 0x900d;
> if (write(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
> goto exit;
> if (read(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
> goto exit;
> if (buf != 0xa00d)
> goto exit;
> ipc_client_log(client, "Handshake stage I passed");
> buf = 0x9f00;
> if (write(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
> goto exit;
> if (read(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
> goto exit;
> if (buf != 0xaf00)
> goto exit;
> ipc_client_log(client, "Handshake stage II passed");
There is probably no need to check read and write for error here as
the comparison with 0xa00d/0xaf00 does the check, however we still probably
want to still make sure that we read/write the correct ammount with a loop:
'man 2 write' has the same thing than 'man 2 read' with its return value, it
might write less than needed.
> }
>
> ipc_client_log(client, "Finishing modem boot process");
> if (xmm626_kernel_smdk4412_boot_power(client, boot0_fd, 0) < 0)
> goto exit;
Here we might want to add a print in case of errors.
> ipc_client_log(client, "Modem boot complete");
> rc = 0;
>
> /* Samsung's official daemons continue to read from umts_boot0
> * at this point, but at present we don't have any means of
> * doing that within the design of this framework. This is
> * probably (?) ok, since I have never seen anything actually
> * come out of umts_boot0 after booting is complete! */
Here the comment constitute some very important information. It could
probably be updated to reflect the current knowledge. For instance:
/* Samsung's official daemons continue to read from umts_boot0
* (XMM626_SEC_MODEM_BOOT0_DEVICE) at this point. It may be
* done to restart the modem in case of errors. The fact that
* I have never seen anything actually come out of umts_boot0
* after booting is complete with libsamsung-ipc seem to be
* consistent with that hypothesis. With libsamsung-ipc,
* it's up to the daemon using it (like libsamsung-ril) to restart
* the boot sequence if .poll (here herolte_poll) fails.
* For that to work, the kenrel driver needs to return an error
* if the modem crash for instance.
*/
> /* One thing that could be an issue, though, is the kernel's
> * use of its function rild_ready(). The phone wants *both*
> * umts_ipc0 and umts_rfs0 to be open for it to be fully
> * happy. Any upper-level clients of this library will have to
> * make sure to open both for things to work. */
This could be updated with something like that:
/* The kernel modem driver for this device[1] checks if both the FMT
* and the RFS channels are open and will refuse to work otherwise
* and produce an error like that: "<the error from dmesg>"
* [1]See the rild_ready function and its usage in
* drivers/misc/modem_v1/link_device_shmem.c in the lineage-17.0 branch
* https://github.com/ivanmeler/android_kernel_samsung_herolte
*/
If you don't have the dmesg error available easily, you could also avoid it as
the rest of the comment is already good enough to understand the requirements
here.
Thanks a lot again for the patch. When they are merged, I'll probably try to
factorize the firmware loading code at some point as other devices could use
that code as well. I'll probably also include your struct in some standalone
tool for parsing the TOC.
The fact that there is a NV offset is also really interesting here. I'd like
to find it for the XMM616 devices but unfortunately they have no TOC.
As for the TOC I've already started collecting some in this repository:
https://git.replicant.us/replicant/vendor_replicant-data/tree/devices/TOC
Though I added only one device so far.
The idea behind that repository is to enable tools to generate some
documentation (wiki pages) automatically, to enable people to more easily
support devices without having them, etc.
Denis.
More information about the Replicant
mailing list