On Thu, 20 Jun 2019 01:26:47 +0200, Luis Chamberlain wrote:
Sorry for the late review... Ah!
No problem, thanks for review.
On Tue, Jun 11, 2019 at 02:26:25PM +0200, Takashi Iwai wrote:
@@ -354,7 +454,12 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644); MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path"); static int -fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv) +fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
const char *suffix,int (*decompress)(struct device *dev,struct fw_priv *fw_priv,size_t in_size,const void *in_buffer))I *think* this could be cleaner, I'll elaborate below.
@@ -645,7 +768,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (ret <= 0) /* error or already assigned */ goto out;
- ret = fw_get_filesystem_firmware(device, fw->priv);
- ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
+#ifdef CONFIG_FW_LOADER_COMPRESS
- if (ret == -ENOENT)
ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",fw_decompress_xz);+#endif
Hrm, and let more #ifdef'ery.
And so if someone wants to add bzip, we'd add yet-another if else on the return value of this call... and yet more #ifdefs.
We already have a list of paths supported. It seems what we need instead is a list of supported suffixes, and a respective structure which then has its set of callbacks for posthandling.
This way, this could all be handled inside fw_get_filesystem_firmware() neatly, and we can just strive towards avoiding #ifdef'ery.
Yes, I had similar idea. Actually my plan for multiple compression formats was:
- Move the decompression part into another file, e.g. decompress_xz.c and change in Makefile: firmware_class-$(CONFIG_FW_LOADER_COMPRESS_XZ) += decompress_xz.o
- Create a table of the extension and the decompression,
static struct fw_decompression_table fw_decompressions[] = { { "", NULL }, #ifdef CONFIG_FW_LOADER_COMPRESS_XZ { ".xz", fw_decompress_xz }, #endif #ifdef CONFIG_FW_LOADER_COMPRESS_BZIP2 { ".bz2", fw_decompress_bzip2 }, #endif ..... };
and call it
for (i = 0; i < ARRAY_SIZE(fw_decompressions); i++) { ret = fw_get_filesystem_firmware(device, fw->priv, fw_decompressions[i].extesnion, fw_decompressions[i].func); if (ret != -ENOENT) break; }
The patch was submitted in the current form just because it's simpler for a single compression case. But as you can see in the v2 patchset change, it has already the decompression function pointer, so that the code can be cleaned up / extended easily later if multiple formats are supported like the above.
BTW, I took a look at other compression methods, and found that LZ4 is a bit tough with the current API. ZSTD should be relatively easy, as it can get the whole decompressed size at first, so we'll be able to do vmalloc(). For others, we may need a streaming decompression and growing buffer per page. But LZ4 decompression API doesn't seem allowing the partial decompression-and-continue as I used for XZ, so it'll be tricky. GZIP and BZIP2 should work in a streaming mode like XZ, I suppose.
thanks,
Takashi