On 10/18/2018 7:00 AM, Joel Fernandes wrote:
On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote:
With compression initialization now separated from pstore_register(),
there is no longer a good reason to do compression method selection
during fs init. Instead, merge everything together into the late init.
Additionally cleans up the reporting to be more clear.

Signed-off-by: Kees Cook <keesc...@chromium.org>
---
This patch goes on top of Joel's. I'll have both in linux-next shortly...
---
  fs/pstore/inode.c    |  2 --
  fs/pstore/internal.h |  3 --
  fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
  3 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 5fcb845b9fec..d814723fb27d 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
  {
        int err;
- pstore_choose_compression();
-
        /* Create a convenient mount point for people to access pstore */
        err = sysfs_create_mount_point(fs_kobj, "pstore");
        if (err)
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index fb767e28aeb2..c029314478fa 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -37,7 +37,4 @@ extern bool   pstore_is_mounted(void);
  extern void   pstore_record_init(struct pstore_record *record,
                                   struct pstore_info *psi);
-/* Called during module_init() */
-extern void __init pstore_choose_compression(void);
-
  #endif
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index f09066db2d4d..e4737a5fa68c 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
static void allocate_buf_for_compression(void)
  {
+       struct crypto_comp *ctx;
+       int size;
+       char *buf;
+
+       /* Skip if not built-in or compression backend not selected yet. */
        if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
                return;
+ /* Skip if no pstore backend yet or compression init already done. */
+       if (!psinfo || tfm)
+               return;
+
        if (!crypto_has_comp(zbackend->name, 0, 0)) {
-               pr_err("No %s compression\n", zbackend->name);
+               pr_err("Unknown compression: %s\n", zbackend->name);
                return;
        }
- big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize);
-       if (big_oops_buf_sz <= 0)
+       size = zbackend->zbufsize(psinfo->bufsize);
+       if (size <= 0) {
+               pr_err("Invalid compression size for %s: %d\n",
+                      zbackend->name, size);
                return;
+       }
- big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-       if (!big_oops_buf) {
-               pr_err("allocate compression buffer error!\n");
+       buf = kmalloc(size, GFP_KERNEL);
+       if (!buf) {
+               pr_err("Failed %d byte compression buffer allocation for: %s\n",
+                      size, zbackend->name);
                return;
        }
- tfm = crypto_alloc_comp(zbackend->name, 0, 0);
-       if (IS_ERR_OR_NULL(tfm)) {
-               kfree(big_oops_buf);
-               big_oops_buf = NULL;
-               pr_err("crypto_alloc_comp() failed!\n");
+       ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+       if (IS_ERR_OR_NULL(ctx)) {
+               kfree(buf);
+               pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+                      PTR_ERR(ctx));
                return;
        }
+
+       /* A non-NULL big_oops_buf indicates compression is available. */
+       tfm = ctx;
+       big_oops_buf_sz = size;
+       big_oops_buf = buf;
+
+       pr_info("Using compression: %s\n", zbackend->name);
  }
static void free_buf_for_compression(void)
  {
-       if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm))
+       if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm)
                crypto_free_comp(tfm);
        kfree(big_oops_buf);
        big_oops_buf = NULL;
@@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list *unused)
                          jiffies + msecs_to_jiffies(pstore_update_ms));
  }
-void __init pstore_choose_compression(void)
+static int __init pstore_compression_late_init(void)
  {
        const struct pstore_zbackend *step;
if (!compress)
-               return;
+               return 0;
+ /* Locate desired compression method. */
        for (step = zbackends; step->name; step++) {
                if (!strcmp(compress, step->name)) {
                        zbackend = step;
-                       pr_info("using %s compression\n", zbackend->name);
-                       return;
+                       break;
                }
        }
-}
-static int __init pstore_compression_late_init(void)
-{
        /*
-        * Check if any pstore backends registered earlier but did not allocate
-        * for compression because crypto was not ready, if so then initialize
-        * compression.
+        * Check if any pstore backends registered earlier but did not
+        * initialize compression because crypto was not ready. If so,
+        * then initialize compression now.
         */
-       if (psinfo && !tfm)
-               allocate_buf_for_compression();
+       allocate_buf_for_compression();

We can also get rid of the 'zbackend' global variable since choosing the
compression backend and allocating the buffers are done at the same time?

Otherwise looks good to me,

Reviewed-by: Joel Fernandes (Google) <j...@joelfernandes.org>


Tested this on top of Joel's patch, works fine on getting early crash pstore dmesg logs, thanks.

Tested-by: Sai Prakash Ranjan <saiprakash.ran...@codeaurora.org>

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Reply via email to