[PATCH] remoteproc: core: Move cdev add before device add

[Siddharth Gupta]

When cdev_add is called after device_add has been called there is no
way for the userspace to know about the addition of a cdev as cdev_add
itself doesn't trigger a uevent notification, or for the kernel to
know about the change to devt. This results in two problems:
 - mknod is never called for the cdev and hence no cdev appears on
   devtmpfs.
 - sysfs links to the new cdev are not established.

Based on how cdev_device_add[1] is written, it appears that the correct
way to use these APIs is to call cdev_add before device_add is called.
Since the cdev is an optional feature for remoteproc we cannot directly
use the existing API. Hence moving rproc_char_device_add() before
device_add() in rproc_add().

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/char_dev.c#n537

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 626a6b90f..562355a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2316,6 +2316,11 @@ int rproc_add(struct rproc *rproc)
struct device *dev = >dev;
int ret;
 
+   /* add char device for this remoteproc */
+   ret = rproc_char_device_add(rproc);
+   if (ret < 0)
+   return ret;
+
ret = device_add(dev);
if (ret < 0)
return ret;
@@ -2329,11 +2334,6 @@ int rproc_add(struct rproc *rproc)
/* create debugfs entries */
rproc_create_debug_dir(rproc);
 
-   /* add char device for this remoteproc */
-   ret = rproc_char_device_add(rproc);
-   if (ret < 0)
-   return ret;
-
/* if rproc is marked always-on, request it to boot */
if (rproc->auto_boot) {
ret = rproc_trigger_auto_boot(rproc);
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2] remoteproc: sysfs: Use sysfs_emit instead of sprintf

[Siddharth Gupta]

From: Raghavendra Rao Ananta 

For security reasons sysfs_emit() is preferred over sprintf().
Hence, convert the remoteproc's sysfs show functions accordingly.

Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c 
b/drivers/remoteproc/remoteproc_sysfs.c
index 1dbef89..6840dad 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -15,7 +15,7 @@ static ssize_t recovery_show(struct device *dev,
 {
struct rproc *rproc = to_rproc(dev);
 
-   return sprintf(buf, "%s", rproc->recovery_disabled ? "disabled\n" : 
"enabled\n");
+   return sysfs_emit(buf, "%s", rproc->recovery_disabled ? "disabled\n" : 
"enabled\n");
 }
 
 /*
@@ -82,7 +82,7 @@ static ssize_t coredump_show(struct device *dev,
 {
struct rproc *rproc = to_rproc(dev);
 
-   return sprintf(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);
+   return sysfs_emit(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);
 }
 
 /*
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] remoteproc: sysfs: Use scnprintf instead of sprintf

[Siddharth Gupta]

On 3/3/2021 12:56 PM, Bjorn Andersson wrote:

On Wed 03 Mar 14:01 CST 2021, Siddharth Gupta wrote:


From: Raghavendra Rao Ananta 

For security reasons scnprintf() is preferred over sprintf().
Hence, convert the remoteproc's sysfs show functions accordingly.


Thanks for the patch Siddharth.

There's no possibility for these calls to generate more than PAGE_SIZE
amount of data, so this isn't really necessary. But if you insist,
please let's use sysfs_emit() instead.

Regards,
Bjorn

Sure Bjorn, I'll push the new patch immediately.

Thanks,
Siddharth



Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Siddharth Gupta 
---
  drivers/remoteproc/remoteproc_sysfs.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c 
b/drivers/remoteproc/remoteproc_sysfs.c
index 1dbef89..853f569 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -15,7 +15,8 @@ static ssize_t recovery_show(struct device *dev,
  {
struct rproc *rproc = to_rproc(dev);
  
-	return sprintf(buf, "%s", rproc->recovery_disabled ? "disabled\n" : "enabled\n");

+   return scnprintf(buf, PAGE_SIZE, "%s",
+rproc->recovery_disabled ? "disabled\n" : "enabled\n");
  }
  
  /*

@@ -82,7 +83,7 @@ static ssize_t coredump_show(struct device *dev,
  {
struct rproc *rproc = to_rproc(dev);
  
-	return sprintf(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);

+   return scnprintf(buf, PAGE_SIZE, "%s\n", 
rproc_coredump_str[rproc->dump_conf]);
  }
  
  /*

--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] remoteproc: sysfs: Use scnprintf instead of sprintf

[Siddharth Gupta]

From: Raghavendra Rao Ananta 

For security reasons scnprintf() is preferred over sprintf().
Hence, convert the remoteproc's sysfs show functions accordingly.

Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c 
b/drivers/remoteproc/remoteproc_sysfs.c
index 1dbef89..853f569 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -15,7 +15,8 @@ static ssize_t recovery_show(struct device *dev,
 {
struct rproc *rproc = to_rproc(dev);
 
-   return sprintf(buf, "%s", rproc->recovery_disabled ? "disabled\n" : 
"enabled\n");
+   return scnprintf(buf, PAGE_SIZE, "%s",
+rproc->recovery_disabled ? "disabled\n" : "enabled\n");
 }
 
 /*
@@ -82,7 +83,7 @@ static ssize_t coredump_show(struct device *dev,
 {
struct rproc *rproc = to_rproc(dev);
 
-   return sprintf(buf, "%s\n", rproc_coredump_str[rproc->dump_conf]);
+   return scnprintf(buf, PAGE_SIZE, "%s\n", 
rproc_coredump_str[rproc->dump_conf]);
 }
 
 /*
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/20/2021 1:10 AM, Christoph Hellwig wrote:

Can you give this patch a spin?

Thanks! This patch fixed the fallback mechanism for me. Attaching logs:

[   84.410162][  T244] qcom_q6v5_pas .remoteproc-cdsp: Direct 
firmware load for cdsp.bX failed with error -2
[   84.418276][  T244] qcom_q6v5_pas .remoteproc-cdsp: Falling 
back to sysfs fallback for: cdsp.bX
[   84.471558][  T393] ueventd: firmware: loading 'cdsp.bX' for 
'/devices/platform/soc/.remoteproc-cdsp/firmware/cdsp.bX'
[   84.491936][  T393] ueventd: loading 
/devices/platform/soc/.remoteproc-cdsp/firmware/cdsp.bX took 22ms
[  103.331486][  T244] remoteproc remoteproc1: remote processor 
.remoteproc-cdsp is now up



- Sid



diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index f277d023ebcd14..4b5833b3059f9c 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -14,6 +14,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "kernfs-internal.h"
  
@@ -180,11 +181,10 @@ static const struct seq_operations kernfs_seq_ops = {

   * it difficult to use seq_file.  Implement simplistic custom buffering for
   * bin files.
   */
-static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
-  char __user *user_buf, size_t count,
-  loff_t *ppos)
+static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
  {
-   ssize_t len = min_t(size_t, count, PAGE_SIZE);
+   struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
+   ssize_t len = min_t(size_t, iov_iter_count(iter), PAGE_SIZE);
const struct kernfs_ops *ops;
char *buf;
  
@@ -210,7 +210,7 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,

of->event = atomic_read(>kn->attr.open->event);
ops = kernfs_ops(of->kn);
if (ops->read)
-   len = ops->read(of, buf, len, *ppos);
+   len = ops->read(of, buf, len, iocb->ki_pos);
else
len = -EINVAL;
  
@@ -220,12 +220,12 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,

if (len < 0)
goto out_free;
  
-	if (copy_to_user(user_buf, buf, len)) {

+   if (copy_to_iter(buf, len, iter) != len) {
len = -EFAULT;
goto out_free;
}
  
-	*ppos += len;

+   iocb->ki_pos += len;
  
   out_free:

if (buf == of->prealloc_buf)
@@ -235,31 +235,14 @@ static ssize_t kernfs_file_direct_read(struct 
kernfs_open_file *of,
return len;
  }
  
-/**

- * kernfs_fop_read - kernfs vfs read callback
- * @file: file pointer
- * @user_buf: data to write
- * @count: number of bytes
- * @ppos: starting offset
- */
-static ssize_t kernfs_fop_read(struct file *file, char __user *user_buf,
-  size_t count, loff_t *ppos)
+static ssize_t kernfs_fop_read_iter(struct kiocb *iocb, struct iov_iter *iter)
  {
-   struct kernfs_open_file *of = kernfs_of(file);
-
-   if (of->kn->flags & KERNFS_HAS_SEQ_SHOW)
-   return seq_read(file, user_buf, count, ppos);
-   else
-   return kernfs_file_direct_read(of, user_buf, count, ppos);
+   if (kernfs_of(iocb->ki_filp)->kn->flags & KERNFS_HAS_SEQ_SHOW)
+   return seq_read_iter(iocb, iter);
+   return kernfs_file_read_iter(iocb, iter);
  }
  
-/**

- * kernfs_fop_write - kernfs vfs write callback
- * @file: file pointer
- * @user_buf: data to write
- * @count: number of bytes
- * @ppos: starting offset
- *
+/*
   * Copy data in from userland and pass it to the matching kernfs write
   * operation.
   *
@@ -269,20 +252,18 @@ static ssize_t kernfs_fop_read(struct file *file, char 
__user *user_buf,
   * modify only the the value you're changing, then write entire buffer
   * back.
   */
-static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
-   size_t count, loff_t *ppos)
+static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter)
  {
-   struct kernfs_open_file *of = kernfs_of(file);
+   struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
+   ssize_t len = iov_iter_count(iter);
const struct kernfs_ops *ops;
-   ssize_t len;
char *buf;
  
  	if (of->atomic_write_len) {

-   len = count;
if (len > of->atomic_write_len)
return -E2BIG;
} else {
-   len = min_t(size_t, count, PAGE_SIZE);
+   len = min_t(size_t, len, PAGE_SIZE);
}
  
  	buf = of->prealloc_buf;

@@ -293,7 +274,7 @@ static ssize_t kernfs_fop_write(struct file *file, const 
char __user *user_buf,
if (!buf)
return -ENOMEM;
  
-	if (copy_from_user(buf, user_buf, len)) {

+   if (copy_from_iter(buf, len, iter) != len) {
len = -EFAULT;
goto out_free;
}
@@ -312,7 +293,7 @@ static ssize_t 

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/15/2021 8:20 AM, Greg KH wrote:

On Tue, Jan 12, 2021 at 10:31:26AM -0800, Siddharth Gupta wrote:

On 1/8/2021 6:44 AM, Greg KH wrote:

On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:

On 1/6/2021 2:33 AM, Greg KH wrote:

Since the binary attributes don't support splice_{read,write} functions the
calls to splice_{read,write} used the default kernel_{read,write} functions.
With the above change this results in an -EINVAL return from
do_splice_from[4].

This essentially means that sendfile will not work for any binary attribute
in the sysfs.

Have you tried fixing this with a patch much like what we did for the
proc files that needed this?  If not, can you?

I am not aware of this fix, could you provide me a link for reference? I
will try it out.

Look at the series of commits starting at fe33850ff798 ("proc: wire up
generic_file_splice_read for iter ops") for how this was fixed in procfs
as an example of what also needs to be done for binary sysfs files.

I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
the generic_file_splice_write function anymore on newer kernels, also AFAICT
kernfs_ops does not define {read,write}_iter operations. If the solution is
simple and someone could provide the patches I would be happy to test them
out. If not, some more information about how to proceed would be nice.

Can you try this tiny patch out below?

Sorry for the delay, I tried out the patch, but I am still seeing the error.
Please take a look at these logs with
android running in the userspace[1]:

[   62.295056][  T249] remoteproc remoteproc1: powering up
.remoteproc-cdsp
[   62.304138][  T249] remoteproc remoteproc1: Direct firmware load for
cdsp.mdt failed with error -2
[   62.312976][  T249] remoteproc remoteproc1: Falling back to sysfs
fallback for: cdsp.mdt
[   62.469748][  T394] ueventd: firmware: loading 'cdsp.mdt' for 
'/devices/platform/soc/.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt'
[   62.498700][  T394] ueventd: firmware: sendfile failed { 
'/sys/devices/platform/soc/.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt',
'cdsp.mdt' }: Invalid argument

Thanks,
Sid

[1]: 
https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#57

Thanks for letting me know.  I'll try to work on this on Monday and add
splice to the in-kernel firmware testing suite, as it would have caught
this...

If my understanding is correct these default (or generic if you prefer)
functions require the write_iter[1] and read_iter[2] file ops to be 
defined for
kernfs_file_fops. Are there any similar default {write,read}_iter 
functions that
I can use, or do these need to be thin wrappers for the read function[3] 
for bin

attributes itself?

Thanks,
Sid

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/splice.c#n686
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/splice.c#n311
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/sysfs/file.c#n235

greg k-h

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

On 1/14/2021 9:46 AM, Bjorn Andersson wrote:

On Wed 13 Jan 17:01 CST 2021, Siddharth Gupta wrote:


On 1/7/2021 4:21 PM, Bjorn Andersson wrote:

On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:


Since the split elf blobs will always contain the hash segment, we rely on

I think it will sounds better if we add "should" in "we should rely on..."

Sure

the blob file to get the hash rather than assume that it will be present in
the mdt file. This change uses the hash index to read the appropriate elf
blob to get the hash segment.

Signed-off-by: Siddharth Gupta
---
   drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
   drivers/soc/qcom/mdt_loader.c   | 38 
+++--
   include/linux/soc/qcom/mdt_loader.h |  3 ++-
   3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
b/drivers/remoteproc/qcom_q6v5_mss.c
index 66106ba..74c0229 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -4,7 +4,7 @@
*
* Copyright (C) 2016 Linaro Ltd.
* Copyright (C) 2014 Sony Mobile Communications AB
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
*/
   #include 
@@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const 
struct firmware *fw)
void *ptr;
int ret;
-   metadata = qcom_mdt_read_metadata(fw, );
+   metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, 
);
if (IS_ERR(metadata))
return PTR_ERR(metadata);
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index c9bbd8c..6876c0b 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -103,15 +103,18 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
*
* Return: pointer to data, or ERR_PTR()
*/
-void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
+void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, 
const char *firmware,
+size_t *data_len)
   {
const struct elf32_phdr *phdrs;
const struct elf32_hdr *ehdr;
-   size_t hash_offset;
+   const struct firmware *seg_fw;
size_t hash_index;
size_t hash_size;
size_t ehdr_size;
+   char *fw_name;
void *data;
+   int ret;
ehdr = (struct elf32_hdr *)fw->data;
phdrs = (struct elf32_phdr *)(ehdr + 1);
@@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
if (!data)
return ERR_PTR(-ENOMEM);
-   /* Is the header and hash already packed */
-   if (qcom_mdt_bins_are_split(fw))
-   hash_offset = phdrs[0].p_filesz;
-   else
-   hash_offset = phdrs[hash_index].p_offset;
-
+   /* copy elf header */
memcpy(data, fw->data, ehdr_size);
-   memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
+

This seems to duplicates parts of the loop in __qcom_mdt_load(), how
about breaking this out to a separate

static int mdt_load_segment(struct device *dev, const struct firmware *fw,
int idx, void *buf, size_t len, bool is_split)

Which either just memcpy from @fw or does the filename and loading
dance, based on @is_split?

Since mdt_load_segment won't know the name of the firmware without a global
variable
(which in turn will make it non-reentrant), the idea of creating such a
function and not passing
the actual name of the firmware seemed wrong.


Wouldn't you be able to pass "firmware" as an argument to the
load_segment function?

We can, which is what I guess you meant in the first place. What I thought
was that if we are creating something like mdt_load_segments passing
"firmware.mdt" with the index instead of "firmware.b" seemed
kind of wrong, but I guess that is just a matter of the naming convention.

If we want to pass the firmware name in this function the code size will be
more or equal to
what we started with. If that is not a problem I can make the changes.


Perhaps I'm missing something here, I do expect that you would end with
code similar to the hunk you add here. But in doing so we should be able
to reuse that in the __qcom_mdt_load(). Or am I too optimistic?

(In particular I'm not fond of the fw_name dance and doing it twice is
worse)
If I am creating the firmware name inside this function then we should 
definitely

see a code reduction. I'll make the changes and push the new patchset soon.

Thanks,
Sid


Regards,
Bjorn


Thanks,
Sid

Regards,
Bjorn


+   if (qcom_mdt_bins_are_split(fw)) {
+   fw_name = kstrdup(firmware, GFP_KERNEL);
+   if (!fw_name) {
+   kfree(data);
+   return ERR_PTR(-ENOMEM);
+   }
+

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

On 1/7/2021 4:21 PM, Bjorn Andersson wrote:

On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:


Since the split elf blobs will always contain the hash segment, we rely on

I think it will sounds better if we add "should" in "we should rely on..."

Sure



the blob file to get the hash rather than assume that it will be present in
the mdt file. This change uses the hash index to read the appropriate elf
blob to get the hash segment.

Signed-off-by: Siddharth Gupta 
---
  drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
  drivers/soc/qcom/mdt_loader.c   | 38 +++--
  include/linux/soc/qcom/mdt_loader.h |  3 ++-
  3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
b/drivers/remoteproc/qcom_q6v5_mss.c
index 66106ba..74c0229 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -4,7 +4,7 @@
   *
   * Copyright (C) 2016 Linaro Ltd.
   * Copyright (C) 2014 Sony Mobile Communications AB
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
   */
  
  #include 

@@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const 
struct firmware *fw)
void *ptr;
int ret;
  
-	metadata = qcom_mdt_read_metadata(fw, );

+   metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, 
);
if (IS_ERR(metadata))
return PTR_ERR(metadata);
  
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c

index c9bbd8c..6876c0b 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -103,15 +103,18 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
   *
   * Return: pointer to data, or ERR_PTR()
   */
-void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
+void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, 
const char *firmware,
+size_t *data_len)
  {
const struct elf32_phdr *phdrs;
const struct elf32_hdr *ehdr;
-   size_t hash_offset;
+   const struct firmware *seg_fw;
size_t hash_index;
size_t hash_size;
size_t ehdr_size;
+   char *fw_name;
void *data;
+   int ret;
  
  	ehdr = (struct elf32_hdr *)fw->data;

phdrs = (struct elf32_phdr *)(ehdr + 1);
@@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
if (!data)
return ERR_PTR(-ENOMEM);
  
-	/* Is the header and hash already packed */

-   if (qcom_mdt_bins_are_split(fw))
-   hash_offset = phdrs[0].p_filesz;
-   else
-   hash_offset = phdrs[hash_index].p_offset;
-
+   /* copy elf header */
memcpy(data, fw->data, ehdr_size);
-   memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
+

This seems to duplicates parts of the loop in __qcom_mdt_load(), how
about breaking this out to a separate

static int mdt_load_segment(struct device *dev, const struct firmware *fw,
int idx, void *buf, size_t len, bool is_split)

Which either just memcpy from @fw or does the filename and loading
dance, based on @is_split?
Since mdt_load_segment won't know the name of the firmware without a 
global variable
(which in turn will make it non-reentrant), the idea of creating such a 
function and not passing

the actual name of the firmware seemed wrong.

If we want to pass the firmware name in this function the code size will 
be more or equal to

what we started with. If that is not a problem I can make the changes.

Thanks,
Sid


Regards,
Bjorn


+   if (qcom_mdt_bins_are_split(fw)) {
+   fw_name = kstrdup(firmware, GFP_KERNEL);
+   if (!fw_name) {
+   kfree(data);
+   return ERR_PTR(-ENOMEM);
+   }
+   snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
+
+   ret = request_firmware_into_buf(_fw, fw_name, dev, data + 
ehdr_size, hash_size);
+   kfree(fw_name);
+
+   if (ret) {
+   kfree(data);
+   return ERR_PTR(ret);
+   }
+
+   release_firmware(seg_fw);
+   } else {
+   memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, 
hash_size);
+   }
  
  	*data_len = ehdr_size + hash_size;
  
@@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,

return -ENOMEM;
  
  	if (pas_init) {

-   metadata = qcom_mdt_read_metadata(fw, _len);
+   metadata = qcom_mdt_read_metadata(dev, fw, firmware, 
_len);
if (IS_ERR(metadata)) {
ret = PTR_ERR(metadata);
goto out;
diff --git a/include/

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/8/2021 6:44 AM, Greg KH wrote:

On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:

On 1/6/2021 2:33 AM, Greg KH wrote:

Since the binary attributes don't support splice_{read,write} functions the
calls to splice_{read,write} used the default kernel_{read,write} functions.
With the above change this results in an -EINVAL return from
do_splice_from[4].

This essentially means that sendfile will not work for any binary attribute
in the sysfs.

Have you tried fixing this with a patch much like what we did for the
proc files that needed this?  If not, can you?

I am not aware of this fix, could you provide me a link for reference? I
will try it out.

Look at the series of commits starting at fe33850ff798 ("proc: wire up
generic_file_splice_read for iter ops") for how this was fixed in procfs
as an example of what also needs to be done for binary sysfs files.

I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
the generic_file_splice_write function anymore on newer kernels, also AFAICT
kernfs_ops does not define {read,write}_iter operations. If the solution is
simple and someone could provide the patches I would be happy to test them
out. If not, some more information about how to proceed would be nice.

Can you try this tiny patch out below?
Sorry for the delay, I tried out the patch, but I am still seeing the 
error. Please take a look at these logs with

android running in the userspace[1]:

[   62.295056][  T249] remoteproc remoteproc1: powering up 
.remoteproc-cdsp
[   62.304138][  T249] remoteproc remoteproc1: Direct firmware load for 
cdsp.mdt failed with error -2
[   62.312976][  T249] remoteproc remoteproc1: Falling back to sysfs 
fallback for: cdsp.mdt
[   62.469748][  T394] ueventd: firmware: loading 'cdsp.mdt' for 
'/devices/platform/soc/.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt'
[   62.498700][  T394] ueventd: firmware: sendfile failed { 
'/sys/devices/platform/soc/.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt', 
'cdsp.mdt' }: Invalid argument


Thanks,
Sid

[1]: 
https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#57


thanks,

greg k-h

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index f277d023ebcd..113bc816d430 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -968,6 +968,8 @@ const struct file_operations kernfs_file_fops = {
.release= kernfs_fop_release,
.poll   = kernfs_fop_poll,
.fsync  = noop_fsync,
+   .splice_read= generic_file_splice_read,
+   .splice_write   = iter_file_splice_write,
  };
  
  /**

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/6/2021 2:33 AM, Greg KH wrote:

On Tue, Jan 05, 2021 at 05:00:58PM -0800, Siddharth Gupta wrote:

On 1/4/2021 10:36 PM, Greg KH wrote:

On Mon, Jan 04, 2021 at 02:43:45PM -0800, Siddharth Gupta wrote:

Hi all,

With the introduction of the filesystem change "fs: don't allow splice
read/write without explicit ops"[1] the fallback mechanism of the firmware
loader[2] no longer works when using sendfile[3] from the userspace.

What userspace program are you using to load firmware?

The userspace program is in the android userspace which listens to a uevent
from the firmware loader and then loads the firmware using sendfile[1].

   Are you not using the in-kernel firmware loader for some reason?

We have certain non-standard firmware paths that should not be added to the
linux kernel, and the firmware_class.path only supports a single path.

That option is just for a single override, which should be all that you
need if the other paths that are built into the kernel do not work.
Surely one of the 5 different paths here are acceptable?
Unfortunately they are not, and we understand that such changes 
shouldn't make it to upstream hence it was not a part of the request. If 
the firmware loader fallback mechanism was being deprecated then we 
would have to look into our options. As of now the series of changes 
breaking the sysfs bin attributes is the only bug that affects us.


If not, how many more do you need?

We need 2 paths.


And last I looked, Android wants you to use the built-in kernel firmware
loader, and NOT an external firmware binary anymore.  So this shouldn't
be an issue for your newer systems anyway :)
In our discussion with the Android team that is not the case currently. 
In the future yes, but not now :)


Regardless this bug is in the kernel and not Android. If the firmware 
loader fallback mechanism is used on the current kernel we would see the 
issue with sendfile in the userspace whether Android is running or not.



Since the binary attributes don't support splice_{read,write} functions the
calls to splice_{read,write} used the default kernel_{read,write} functions.
With the above change this results in an -EINVAL return from
do_splice_from[4].

This essentially means that sendfile will not work for any binary attribute
in the sysfs.

Have you tried fixing this with a patch much like what we did for the
proc files that needed this?  If not, can you?

I am not aware of this fix, could you provide me a link for reference? I
will try it out.

Look at the series of commits starting at fe33850ff798 ("proc: wire up
generic_file_splice_read for iter ops") for how this was fixed in procfs
as an example of what also needs to be done for binary sysfs files.
I tried to follow these fixes, but I am unfamiliar with fs code. I don't 
see the generic_file_splice_write function anymore on newer kernels, 
also AFAICT kernfs_ops does not define {read,write}_iter operations. If 
the solution is simple and someone could provide the patches I would be 
happy to test them out. If not, some more information about how to 
proceed would be nice.


Thanks,
Sid


thanks,

greg k-h

[PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr

[Siddharth Gupta]

The assumption that the elf program header will always have the hash
segment program header at index 1 may not hold true in all cases. This
change updates the read metadata function to find the hash program header
dynamically.

Signed-off-by: Siddharth Gupta 
---
 drivers/soc/qcom/mdt_loader.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193..813216d 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -4,7 +4,7 @@
  *
  * Copyright (C) 2016 Linaro Ltd
  * Copyright (C) 2015 Sony Mobile Communications Inc
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
  */
 
 #include 
@@ -88,6 +88,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
const struct elf32_phdr *phdrs;
const struct elf32_hdr *ehdr;
size_t hash_offset;
+   size_t hash_index;
size_t hash_size;
size_t ehdr_size;
void *data;
@@ -98,14 +99,19 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
if (ehdr->e_phnum < 2)
return ERR_PTR(-EINVAL);
 
-   if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
+   if (phdrs[0].p_type == PT_LOAD)
return ERR_PTR(-EINVAL);
 
-   if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
+   for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
+   if (phdrs[hash_index].p_type != PT_LOAD &&
+  (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == 
QCOM_MDT_TYPE_HASH)
+   break;
+   }
+   if (hash_index >= ehdr->e_phnum)
return ERR_PTR(-EINVAL);
 
ehdr_size = phdrs[0].p_filesz;
-   hash_size = phdrs[1].p_filesz;
+   hash_size = phdrs[hash_index].p_filesz;
 
data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
if (!data)
@@ -115,7 +121,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
if (ehdr_size + hash_size == fw->size)
hash_offset = phdrs[0].p_filesz;
else
-   hash_offset = phdrs[1].p_offset;
+   hash_offset = phdrs[hash_index].p_offset;
 
memcpy(data, fw->data, ehdr_size);
memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly

[Siddharth Gupta]

It may be that the offset of the first program header lies inside the mdt's
filesize, in this case the loader would incorrectly assume that the bins
were not split. The loading would then continue on to fail for split bins.
This change updates the logic used by the mdt loader to understand whether
the firmware images are split or not. It figures this out by checking if
each program header's segment lies within the file or not.

Signed-off-by: Siddharth Gupta 
---
 drivers/soc/qcom/mdt_loader.c | 60 +++
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 813216d..c9bbd8c 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
return true;
 }
 
+static bool qcom_mdt_bins_are_split(const struct firmware *fw)
+{
+   const struct elf32_phdr *phdrs;
+   const struct elf32_hdr *ehdr;
+   uint64_t seg_start, seg_end;
+   int i;
+
+   ehdr = (struct elf32_hdr *)fw->data;
+   phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+   for (i = 0; i < ehdr->e_phnum; i++) {
+   seg_start = phdrs[i].p_offset;
+   seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
+   if (seg_start > fw->size || seg_end > fw->size)
+   return true;
+   }
+
+   return false;
+}
+
 /**
  * qcom_mdt_get_size() - acquire size of the memory region needed to load mdt
  * @fw:firmware object for the mdt file
@@ -118,7 +138,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
return ERR_PTR(-ENOMEM);
 
/* Is the header and hash already packed */
-   if (ehdr_size + hash_size == fw->size)
+   if (qcom_mdt_bins_are_split(fw))
hash_offset = phdrs[0].p_filesz;
else
hash_offset = phdrs[hash_index].p_offset;
@@ -150,6 +170,7 @@ static int __qcom_mdt_load(struct device *dev, const struct 
firmware *fw,
void *metadata;
char *fw_name;
bool relocate = false;
+   bool is_split;
void *ptr;
int ret = 0;
int i;
@@ -157,6 +178,7 @@ static int __qcom_mdt_load(struct device *dev, const struct 
firmware *fw,
if (!fw || !mem_region || !mem_phys || !mem_size)
return -EINVAL;
 
+   is_split = qcom_mdt_bins_are_split(fw);
ehdr = (struct elf32_hdr *)fw->data;
phdrs = (struct elf32_phdr *)(ehdr + 1);
 
@@ -238,28 +260,22 @@ static int __qcom_mdt_load(struct device *dev, const 
struct firmware *fw,
 
ptr = mem_region + offset;
 
-   if (phdr->p_filesz && phdr->p_offset < fw->size) {
-   /* Firmware is large enough to be non-split */
-   if (phdr->p_offset + phdr->p_filesz > fw->size) {
-   dev_err(dev,
-   "failed to load segment %d from 
truncated file %s\n",
-   i, firmware);
-   ret = -EINVAL;
-   break;
+   if (phdr->p_filesz) {
+   if (!is_split) {
+   /* Firmware is large enough to be non-split */
+   memcpy(ptr, fw->data + phdr->p_offset, 
phdr->p_filesz);
+   } else {
+   /* Firmware not large enough, load split-out 
segments */
+   snprintf(fw_name + fw_name_len - 3, 4, "b%02d", 
i);
+   ret = request_firmware_into_buf(_fw, 
fw_name, dev,
+   ptr, 
phdr->p_filesz);
+   if (ret) {
+   dev_err(dev, "failed to load %s\n", 
fw_name);
+   break;
+   }
+
+   release_firmware(seg_fw);
}
-
-   memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
-   } else if (phdr->p_filesz) {
-   /* Firmware not large enough, load split-out segments */
-   sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-   ret = request_firmware_into_buf(_fw, fw_name, dev,
-   ptr, phdr->p_filesz);
-   if (ret) {
-   dev_err(dev, "failed to load %s\n", fw_name);
-   break;
-   }
-
-   release_firmware(seg_fw);
}
 
if (

[PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

Since the split elf blobs will always contain the hash segment, we rely on
the blob file to get the hash rather than assume that it will be present in
the mdt file. This change uses the hash index to read the appropriate elf
blob to get the hash segment.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
 drivers/soc/qcom/mdt_loader.c   | 38 +++--
 include/linux/soc/qcom/mdt_loader.h |  3 ++-
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
b/drivers/remoteproc/qcom_q6v5_mss.c
index 66106ba..74c0229 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -4,7 +4,7 @@
  *
  * Copyright (C) 2016 Linaro Ltd.
  * Copyright (C) 2014 Sony Mobile Communications AB
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
  */
 
 #include 
@@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const 
struct firmware *fw)
void *ptr;
int ret;
 
-   metadata = qcom_mdt_read_metadata(fw, );
+   metadata = qcom_mdt_read_metadata(qproc->dev, fw, 
qproc->hexagon_mdt_image, );
if (IS_ERR(metadata))
return PTR_ERR(metadata);
 
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index c9bbd8c..6876c0b 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -103,15 +103,18 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
  *
  * Return: pointer to data, or ERR_PTR()
  */
-void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
+void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, 
const char *firmware,
+size_t *data_len)
 {
const struct elf32_phdr *phdrs;
const struct elf32_hdr *ehdr;
-   size_t hash_offset;
+   const struct firmware *seg_fw;
size_t hash_index;
size_t hash_size;
size_t ehdr_size;
+   char *fw_name;
void *data;
+   int ret;
 
ehdr = (struct elf32_hdr *)fw->data;
phdrs = (struct elf32_phdr *)(ehdr + 1);
@@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, 
size_t *data_len)
if (!data)
return ERR_PTR(-ENOMEM);
 
-   /* Is the header and hash already packed */
-   if (qcom_mdt_bins_are_split(fw))
-   hash_offset = phdrs[0].p_filesz;
-   else
-   hash_offset = phdrs[hash_index].p_offset;
-
+   /* copy elf header */
memcpy(data, fw->data, ehdr_size);
-   memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
+
+   if (qcom_mdt_bins_are_split(fw)) {
+   fw_name = kstrdup(firmware, GFP_KERNEL);
+   if (!fw_name) {
+   kfree(data);
+   return ERR_PTR(-ENOMEM);
+   }
+   snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
+
+   ret = request_firmware_into_buf(_fw, fw_name, dev, data + 
ehdr_size, hash_size);
+   kfree(fw_name);
+
+   if (ret) {
+   kfree(data);
+   return ERR_PTR(ret);
+   }
+
+   release_firmware(seg_fw);
+   } else {
+   memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, 
hash_size);
+   }
 
*data_len = ehdr_size + hash_size;
 
@@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct 
firmware *fw,
return -ENOMEM;
 
if (pas_init) {
-   metadata = qcom_mdt_read_metadata(fw, _len);
+   metadata = qcom_mdt_read_metadata(dev, fw, firmware, 
_len);
if (IS_ERR(metadata)) {
ret = PTR_ERR(metadata);
goto out;
diff --git a/include/linux/soc/qcom/mdt_loader.h 
b/include/linux/soc/qcom/mdt_loader.h
index e600bae..04ba5e8 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct 
firmware *fw,
  const char *fw_name, int pas_id, void *mem_region,
  phys_addr_t mem_phys, size_t mem_size,
  phys_addr_t *reloc_base);
-void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len);
+void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, 
const char *firmware,
+size_t *data_len);
 
 #endif
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 0/3] soc: qcom: mdt_loader: General improvements

[Siddharth Gupta]

This series of patches improves general functionality for the mdt loader.

Patch 1 adds the ability to dynamically detect hash segment location.
Patch 2 updates the logic used to identify whether the firmware is split or not.
Patch 3 updates the way the metadata is read and generated.

Siddharth Gupta (3):
  soc: qcom: mdt_loader: Allow hash at any phdr
  soc: qcom: mdt_loader: Handle split bins correctly
  soc: qcom: mdt_loader: Read hash from firmware blob

 drivers/remoteproc/qcom_q6v5_mss.c  |   4 +-
 drivers/soc/qcom/mdt_loader.c   | 110 
 include/linux/soc/qcom/mdt_loader.h |   3 +-
 3 files changed, 79 insertions(+), 38 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/4/2021 10:36 PM, Greg KH wrote:

On Mon, Jan 04, 2021 at 02:43:45PM -0800, Siddharth Gupta wrote:

Hi all,

With the introduction of the filesystem change "fs: don't allow splice
read/write without explicit ops"[1] the fallback mechanism of the firmware
loader[2] no longer works when using sendfile[3] from the userspace.

What userspace program are you using to load firmware?
The userspace program is in the android userspace which listens to a 
uevent from the firmware loader and then loads the firmware using 
sendfile[1].

  Are you not using the in-kernel firmware loader for some reason?
We have certain non-standard firmware paths that should not be added to 
the linux kernel, and the firmware_class.path only supports a single path.



Since the binary attributes don't support splice_{read,write} functions the
calls to splice_{read,write} used the default kernel_{read,write} functions.
With the above change this results in an -EINVAL return from
do_splice_from[4].

This essentially means that sendfile will not work for any binary attribute
in the sysfs.

Have you tried fixing this with a patch much like what we did for the
proc files that needed this?  If not, can you?
I am not aware of this fix, could you provide me a link for reference? I 
will try it out.



[1]: 
https://github.com/torvalds/linux/commit/36e2c7421f02a22f71c9283e55fdb672a9eb58e7#diff-70c49af2ed5805fc1406ed6e6532d6a029ada1abd90cca6442711b9cecd4d523
[2]: 
https://github.com/torvalds/linux/blob/master/drivers/base/firmware_loader/main.c#L831
[3]: https://github.com/torvalds/linux/blob/master/fs/read_write.c#L1257
[4]: https://github.com/torvalds/linux/blob/master/fs/splice.c#L753

kernel development is on git.kernel.org, not github :)

I use it because it is easier on the eyes when looking at diffs :D
But I'll be sure to use git.kernel.org from now on if that is what is 
preferred!


thanks,

greg k-h

Thanks,
Sid

[1]: 
https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#55

PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

Hi all,

With the introduction of the filesystem change "fs: don't allow splice 
read/write without explicit ops"[1] the fallback mechanism of the 
firmware loader[2] no longer works when using sendfile[3] from the 
userspace.


Since the binary attributes don't support splice_{read,write} functions 
the calls to splice_{read,write} used the default kernel_{read,write} 
functions. With the above change this results in an -EINVAL return from 
do_splice_from[4].


This essentially means that sendfile will not work for any binary 
attribute in the sysfs.


Thanks,
Siddharth

[1]: 
https://github.com/torvalds/linux/commit/36e2c7421f02a22f71c9283e55fdb672a9eb58e7#diff-70c49af2ed5805fc1406ed6e6532d6a029ada1abd90cca6442711b9cecd4d523
[2]: 
https://github.com/torvalds/linux/blob/master/drivers/base/firmware_loader/main.c#L831

[3]: https://github.com/torvalds/linux/blob/master/fs/read_write.c#L1257
[4]: https://github.com/torvalds/linux/blob/master/fs/splice.c#L753

[PATCH v8 3/4] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Co-developed-by: Rishabh Bhatnagar 
Signed-off-by: Rishabh Bhatnagar 
Co-developed-by: Gurbir Arora 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_common.c   | 147 +
 drivers/remoteproc/qcom_common.h   |   2 +
 drivers/remoteproc/qcom_q6v5_pas.c |  27 ++-
 3 files changed, 174 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 085fd73..c41c3a5 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "remoteproc_internal.h"
 #include "qcom_common.h"
@@ -25,6 +26,61 @@
 #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
 #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
 
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @valid  : This entry to be dumped (if set to 1)
+ * @address: Physical address of region to be dumped
+ * @size   : Size of the region
+ */
+struct minidump_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   __le32  seq_num;
+   __le32  valid;
+   __le64  address;
+   __le64  size;
+};
+
+/**
+ * struct minidump_subsystem_toc: Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @region_count : Number of regions added in this subsystem toc
+ * @regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem {
+   __le32  status;
+   __le32  enabled;
+   __le32  encryption_status;
+   __le32  encryption_required;
+   __le32  region_count;
+   __le64  regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc: Global Table of Content
+ * @status : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @enabled : Minidump enable status
+ * @subsystems : Array of subsystems toc
+ */
+struct minidump_global_toc {
+   __le32  status;
+   __le32  md_revision;
+   __le32  enabled;
+   struct minidump_subsystem   subsystems[MAX_NUM_OF_SS];
+};
+
 struct qcom_ssr_subsystem {
const char *name;
struct srcu_notifier_head notifier_list;
@@ -34,6 +90,97 @@ struct qcom_ssr_subsystem {
 static LIST_HEAD(qcom_ssr_subsystem_list);
 static DEFINE_MUTEX(qcom_ssr_subsys_lock);
 
+
+static void qcom_minidump_cleanup(struct rproc *rproc)
+{
+   struct rproc_dump_segment *entry, *tmp;
+
+   list_for_each_entry_safe(entry, tmp, >dump_segments, node) {
+   list_del(>node);
+   kfree(entry->priv);
+   kfree(entry);
+   }
+}
+
+static int qcom_add_minidump_segments(struct rproc *rproc, struct 
minidump_subsystem *subsystem)
+{
+   struct minidump_region __iomem *ptr;
+   struct minidump_region region;
+   int seg_cnt, i;
+   dma_addr_t da;
+   size_t size;
+   char *name;
+
+   if (WARN_ON(!list_empty(>dump_segments))) {
+   dev_err(>dev, "dump segment list already populated\n");
+   return -EUCLEAN;
+   }
+
+   seg_cnt = le32_to_cpu(subsystem->region_count);
+   ptr = ioremap((unsigned long)le64_to_cpu(subsystem->regions_baseptr),
+ seg_cnt * sizeof(struct minidump_region));
+   if (!ptr)
+   return -EFAULT;
+
+   for (i = 0; i < seg_cnt; i++) {
+   memcpy_fromio(, ptr + i, sizeof(region));
+   if (region.valid == MD_REGION_VALID) {
+   name = kstrdup(region.name, GFP_KERNEL);
+   if (!name) {
+   iounmap(ptr);
+   return -ENOMEM;
+   

[PATCH v8 2/4] remoteproc: coredump: Add minidump functionality

[Siddharth Gupta]

This change adds a new kind of core dump mechanism which instead of dumping
entire program segments of the firmware, dumps sections of the remoteproc
memory which are sufficient to allow debugging the firmware. This function
thus uses section headers instead of program headers during creation of the
core dump elf.

Co-developed-by: Rishabh Bhatnagar 
Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_coredump.c| 140 
 drivers/remoteproc/remoteproc_elf_helpers.h |  26 ++
 include/linux/remoteproc.h  |   1 +
 3 files changed, 167 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c
index 34530dc..81ec154 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -323,3 +323,143 @@ void rproc_coredump(struct rproc *rproc)
 */
wait_for_completion(_state.dump_done);
 }
+
+/**
+ * rproc_coredump_using_sections() - perform coredump using section headers
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered sections of
+ * segments and create a devcoredump device associated with rproc. Based on
+ * the coredump configuration this function will directly copy the segments
+ * from device memory to userspace or copy segments from device memory to
+ * a separate buffer, which can then be read by userspace.
+ * The first approach avoids using extra vmalloc memory. But it will stall
+ * recovery flow until dump is read by userspace.
+ */
+void rproc_coredump_using_sections(struct rproc *rproc)
+{
+   struct rproc_dump_segment *segment;
+   void *shdr;
+   void *ehdr;
+   size_t data_size;
+   size_t strtbl_size = 0;
+   size_t strtbl_index = 1;
+   size_t offset;
+   void *data;
+   u8 class = rproc->elf_class;
+   int shnum;
+   struct rproc_coredump_state dump_state;
+   unsigned int dump_conf = rproc->dump_conf;
+   char *str_tbl = "STR_TBL";
+
+   if (list_empty(>dump_segments) ||
+   dump_conf == RPROC_COREDUMP_DISABLED)
+   return;
+
+   if (class == ELFCLASSNONE) {
+   dev_err(>dev, "Elf class is not set\n");
+   return;
+   }
+
+   /*
+* We allocate two extra section headers. The first one is null.
+* Second section header is for the string table. Also space is
+* allocated for string table.
+*/
+   data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class);
+   shnum = 2;
+
+   /* the extra byte is for the null character at index 0 */
+   strtbl_size += strlen(str_tbl) + 2;
+
+   list_for_each_entry(segment, >dump_segments, node) {
+   data_size += elf_size_of_shdr(class);
+   strtbl_size += strlen(segment->priv) + 1;
+   if (dump_conf == RPROC_COREDUMP_ENABLED)
+   data_size += segment->size;
+   shnum++;
+   }
+
+   data_size += strtbl_size;
+
+   data = vmalloc(data_size);
+   if (!data)
+   return;
+
+   ehdr = data;
+   memset(ehdr, 0, elf_size_of_hdr(class));
+   /* e_ident field is common for both elf32 and elf64 */
+   elf_hdr_init_ident(ehdr, class);
+
+   elf_hdr_set_e_type(class, ehdr, ET_CORE);
+   elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);
+   elf_hdr_set_e_version(class, ehdr, EV_CURRENT);
+   elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);
+   elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));
+   elf_hdr_set_e_shnum(class, ehdr, shnum);
+   elf_hdr_set_e_shstrndx(class, ehdr, 1);
+
+   /*
+* The zeroth index of the section header is reserved and is rarely 
used.
+* Set the section header as null (SHN_UNDEF) and move to the next one.
+*/
+   shdr = data + elf_hdr_get_e_shoff(class, ehdr);
+   memset(shdr, 0, elf_size_of_shdr(class));
+   shdr += elf_size_of_shdr(class);
+
+   /* Initialize the string table. */
+   offset = elf_hdr_get_e_shoff(class, ehdr) +
+elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);
+   memset(data + offset, 0, strtbl_size);
+
+   /* Fill in the string table section header. */
+   memset(shdr, 0, elf_size_of_shdr(class));
+   elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);
+   elf_shdr_set_sh_offset(class, shdr, offset);
+   elf_shdr_set_sh_size(class, shdr, strtbl_size);
+   elf_shdr_set_sh_entsize(class, shdr, 0);
+   elf_shdr_set_sh_flags(class, shdr, 0);
+   elf_shdr_set_sh_name(class, shdr, elf_strtbl_add(str_tbl, ehdr, class, 
_index));
+   offset += elf_shdr_get_sh_size(class

[PATCH v8 4/4] remoteproc: qcom: Add minidump id for sm8150 modem

[Siddharth Gupta]

Add minidump id for modem in sm8150 chipset so that the regions to be
included in the coredump generated upon a crash is based on the minidump
tables in SMEM instead of those in the ELF header.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_pas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index ca05c2ef..e61ef88 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -630,6 +630,7 @@ static const struct adsp_data mpss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "modem.mdt",
.pas_id = 4,
+   .minidump_id = 3,
.has_aggre2_clk = false,
.auto_boot = false,
.active_pd_names = (char*[]){
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 0/4] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Sometimes firmware sizes can be in tens of MB's and reading all the memory
during coredump can consume lot of time and memory.

Introducing support for mini-dumps. Mini-dump contains smallest amount of
useful information, that could help to debug subsystem crashes.

During bootup memory is allocated in SMEM (Shared memory) in the form of a
table that contains the physical addresses and sizes of the regions that
are supposed to be collected during coredump. This memory is shared amongst
all processors in a Qualcomm platform, so all remoteprocs fill in their
entry in the global table once they are out of reset.

This patch series adds support for parsing the global minidump table and
uses the current coredump frameork to expose this memory to userspace
during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v7 -> v8:
- Addressed all comments from Bjorn:
* Renamed set_section_name to elf_strtbl_add.
* Renamed rproc_minidump to rproc_coredump_using_sections.
* Removed qcom_minidump header and moved structures to qcom_common source 
files.
* Moved minidump specific functions to qcom_common source files.
* Other minor fixes.

v6 -> v7:
- The STR_TAB size is calculated dynamically now instead of a predefined size.
- Added comments to indicate details about the reserved null section header. 
More
  details can be found at https://refspecs.linuxfoundation.org/elf/elf.pdf.

v5 -> v6:
- Removed priv_cleanup operation from rproc_ops. The dump_segments list is
  updated and cleaned up each time minidump is invoked.
- Split patch #2 into 2 parts - one that adds the rproc_minidump function, and
  the other that uses the new function in the qcom_q6v5_pas driver.
- Updated structs in qcom_minidump to explicitly indicate the endianness of the
  data stored in SMEM, also updated member names.
- Read the global table of contents in SMEM each time adsp_minidump is invoked.

v4 -> v5:
- Fixed adsp_add_minidump_segments to read IO memory using appropriate 
functions.

v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
  private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
  the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (4):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: coredump: Add minidump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem

 drivers/remoteproc/qcom_common.c| 147 
 drivers/remoteproc/qcom_common.h|   2 +
 drivers/remoteproc/qcom_q6v5_pas.c  |  28 +-
 drivers/remoteproc/remoteproc_core.c|   6 +-
 drivers/remoteproc/remoteproc_coredump.c| 140 ++
 drivers/remoteproc/remoteproc_elf_helpers.h |  26 +
 include/linux/remoteproc.h  |   3 +
 7 files changed, 349 insertions(+), 3 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 1/4] remoteproc: core: Add ops to enable custom coredump functionality

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index dab2c0f..eba7543 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1704,7 +1704,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2126,6 +2126,10 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   /* Default to rproc_coredump if no coredump function is specified */
+   if (!rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3fa3ba6..a419878 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -375,6 +375,7 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:collect firmware dump after the subsystem is shutdown
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -393,6 +394,7 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 3/4] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Co-developed-by: Rishabh Bhatnagar 
Signed-off-by: Rishabh Bhatnagar 
Co-developed-by: Gurbir Arora 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_minidump.h |  64 +++
 drivers/remoteproc/qcom_q6v5_pas.c | 104 -
 2 files changed, 166 insertions(+), 2 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_minidump.h 
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..5857d06
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @valid  : This entry to be dumped (if set to 1)
+ * @address: Physical address of region to be dumped
+ * @size   : Size of the region
+ */
+struct minidump_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   __le32  seq_num;
+   __le32  valid;
+   __le64  address;
+   __le64  size;
+};
+
+/**
+ * struct minidump_subsystem_toc: Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem_toc {
+   __le32  status;
+   __le32  enabled;
+   __le32  encryption_status;
+   __le32  encryption_required;
+   __le32  ss_region_count;
+   __le64  md_ss_smem_regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct minidump_global_toc {
+   __le32  status;
+   __le32  md_revision;
+   __le32  enabled;
+   struct minidump_subsystem_toc   md_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..349f725 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,13 @@
 #include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
+#include "qcom_minidump.h"
 
 struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
int pas_id;
+   unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
 
@@ -63,6 +65,7 @@ struct qcom_adsp {
int proxy_pd_count;
 
int pas_id;
+   unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
const char *info_name;
@@ -116,6 +119,88 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, 
struct device **pds,
}
 }
 
+static void adsp_minidump_cleanup(struct rproc *rproc)
+{
+   struct rproc_dump_segment *entry, *tmp;
+
+   list_for_each_entry_safe(entry, tmp, >dump_segments, node) {
+   list_del(>node);
+   kfree(entry->priv);
+   kfree(entry);
+   }
+}
+
+static void adsp_add_minidump_segments(struct rproc *rproc,
+  struct minidump_subsystem_toc 
*minidump_ss)
+{
+   struct minidump_region __iomem *ptr;
+   struct minidump_region region;
+   int seg_cnt, i;
+   dma_addr_t da;
+   size_t size;
+   char *name;
+
+   if (!list_empty(>dump_segments)) {
+   dev_err(>dev, "dump segment list already populated\n");
+   retur

[PATCH v7 4/4] remoteproc: qcom: Add minidump id for sm8150 modem

[Siddharth Gupta]

Add minidump id for modem in sm8150 chipset so that the regions to be
included in the coredump generated upon a crash is based on the minidump
tables in SMEM instead of those in the ELF header.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_pas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 349f725..23f4532 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -707,6 +707,7 @@ static const struct adsp_data mpss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "modem.mdt",
.pas_id = 4,
+   .minidump_id = 3,
.has_aggre2_clk = false,
.auto_boot = false,
.active_pd_names = (char*[]){
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 1/4] remoteproc: core: Add ops to enable custom coredump functionality

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index dab2c0f..eba7543 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1704,7 +1704,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2126,6 +2126,10 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   /* Default to rproc_coredump if no coredump function is specified */
+   if (!rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3fa3ba6..a419878 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -375,6 +375,7 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:collect firmware dump after the subsystem is shutdown
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -393,6 +394,7 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 0/4] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Sometimes firmware sizes can be in tens of MB's and reading all the memory
during coredump can consume lot of time and memory.

Introducing support for mini-dumps. Mini-dump contains smallest amount of
useful information, that could help to debug subsystem crashes.

During bootup memory is allocated in SMEM (Shared memory) in the form of a
table that contains the physical addresses and sizes of the regions that
are supposed to be collected during coredump. This memory is shared amongst
all processors in a Qualcomm platform, so all remoteprocs fill in their
entry in the global table once they are out of reset.

This patch series adds support for parsing the global minidump table and
uses the current coredump frameork to expose this memory to userspace
during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v6 -> v7:
- The STR_TAB size is calculated dynamically now instead of a predefined size.
- Added comments to indicate details about the reserved null section header. 
More
  details can be found at https://refspecs.linuxfoundation.org/elf/elf.pdf.

v5 -> v6:
- Removed priv_cleanup operation from rproc_ops. The dump_segments list is
  updated and cleaned up each time minidump is invoked.
- Split patch #2 into 2 parts - one that adds the rproc_minidump function, and
  the other that uses the new function in the qcom_q6v5_pas driver.
- Updated structs in qcom_minidump to explicitly indicate the endianness of the
  data stored in SMEM, also updated member names.
- Read the global table of contents in SMEM each time adsp_minidump is invoked.

v4 -> v5:
- Fixed adsp_add_minidump_segments to read IO memory using appropriate 
functions.

v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
  private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
  the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (4):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: coredump: Add minidump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem

 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 105 -
 drivers/remoteproc/remoteproc_core.c|   6 +-
 drivers/remoteproc/remoteproc_coredump.c| 140 
 drivers/remoteproc/remoteproc_elf_helpers.h |  26 ++
 include/linux/remoteproc.h  |   3 +
 6 files changed, 341 insertions(+), 3 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 2/4] remoteproc: coredump: Add minidump functionality

[Siddharth Gupta]

This change adds a new kind of core dump mechanism which instead of dumping
entire program segments of the firmware, dumps sections of the remoteproc
memory which are sufficient to allow debugging the firmware. This function
thus uses section headers instead of program headers during creation of the
core dump elf.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_coredump.c| 140 
 drivers/remoteproc/remoteproc_elf_helpers.h |  26 ++
 include/linux/remoteproc.h  |   1 +
 3 files changed, 167 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c
index 34530dc..a6c0099 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -323,3 +323,143 @@ void rproc_coredump(struct rproc *rproc)
 */
wait_for_completion(_state.dump_done);
 }
+
+/**
+ * rproc_minidump() - perform minidump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered sections of
+ * segments and create a devcoredump device associated with rproc. Based on
+ * the coredump configuration this function will directly copy the segments
+ * from device memory to userspace or copy segments from device memory to
+ * a separate buffer, which can then be read by userspace.
+ * The first approach avoids using extra vmalloc memory. But it will stall
+ * recovery flow until dump is read by userspace.
+ */
+void rproc_minidump(struct rproc *rproc)
+{
+   struct rproc_dump_segment *segment;
+   void *shdr;
+   void *ehdr;
+   size_t data_size;
+   size_t strtbl_size = 0;
+   size_t strtbl_index = 1;
+   size_t offset;
+   void *data;
+   u8 class = rproc->elf_class;
+   int shnum;
+   struct rproc_coredump_state dump_state;
+   unsigned int dump_conf = rproc->dump_conf;
+   char *str_tbl = "STR_TBL";
+
+   if (list_empty(>dump_segments) ||
+   dump_conf == RPROC_COREDUMP_DISABLED)
+   return;
+
+   if (class == ELFCLASSNONE) {
+   dev_err(>dev, "Elf class is not set\n");
+   return;
+   }
+
+   /*
+* We allocate two extra section headers. The first one is null.
+* Second section header is for the string table. Also space is
+* allocated for string table.
+*/
+   data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class);
+   shnum = 2;
+
+   /* the extra byte is for the null character at index 0 */
+   strtbl_size += strlen(str_tbl) + 2;
+
+   list_for_each_entry(segment, >dump_segments, node) {
+   data_size += elf_size_of_shdr(class);
+   strtbl_size += strlen(segment->priv) + 1;
+   if (dump_conf == RPROC_COREDUMP_ENABLED)
+   data_size += segment->size;
+   shnum++;
+   }
+
+   data_size += strtbl_size;
+
+   data = vmalloc(data_size);
+   if (!data)
+   return;
+
+   ehdr = data;
+   memset(ehdr, 0, elf_size_of_hdr(class));
+   /* e_ident field is common for both elf32 and elf64 */
+   elf_hdr_init_ident(ehdr, class);
+
+   elf_hdr_set_e_type(class, ehdr, ET_CORE);
+   elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);
+   elf_hdr_set_e_version(class, ehdr, EV_CURRENT);
+   elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);
+   elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));
+   elf_hdr_set_e_shnum(class, ehdr, shnum);
+   elf_hdr_set_e_shstrndx(class, ehdr, 1);
+
+   /*
+* The zeroth index of the section header is reserved and is rarely 
used.
+* Set the section header as null (SHN_UNDEF) and move to the next one.
+*/
+   shdr = data + elf_hdr_get_e_shoff(class, ehdr);
+   memset(shdr, 0, elf_size_of_shdr(class));
+   shdr += elf_size_of_shdr(class);
+
+   /* Initialize the string table. */
+   offset = elf_hdr_get_e_shoff(class, ehdr) +
+elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);
+   memset(data + offset, 0, strtbl_size);
+
+   /* Fill in the string table section header. */
+   memset(shdr, 0, elf_size_of_shdr(class));
+   elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);
+   elf_shdr_set_sh_offset(class, shdr, offset);
+   elf_shdr_set_sh_size(class, shdr, strtbl_size);
+   elf_shdr_set_sh_entsize(class, shdr, 0);
+   elf_shdr_set_sh_flags(class, shdr, 0);
+   elf_shdr_set_sh_name(class, shdr, set_section_name(str_tbl, ehdr, 
class, _index));
+   offset += elf_shdr_get_sh_size(class, shdr);
+   shdr += elf_size_of_shdr(class);
+
+   list_for_each_entry(segment, >

Re: [PATCH v6 2/4] remoteproc: coredump: Add minidump functionality

[Siddharth Gupta]

On 10/26/2020 2:09 PM, Bjorn Andersson wrote:

On Fri 02 Oct 21:05 CDT 2020, Siddharth Gupta wrote:


This change adds a new kind of core dump mechanism which instead of dumping
entire program segments of the firmware, dumps sections of the remoteproc
memory which are sufficient to allow debugging the firmware. This function
thus uses section headers instead of program headers during creation of the
core dump elf.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
  drivers/remoteproc/remoteproc_coredump.c| 132 
  drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
  include/linux/remoteproc.h  |   1 +
  3 files changed, 160 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c
index bb15a29..e7d1394 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -13,6 +13,8 @@
  #include "remoteproc_internal.h"
  #include "remoteproc_elf_helpers.h"
  
+#define MAX_STRTBL_SIZE 512

+
  struct rproc_coredump_state {
struct rproc *rproc;
void *header;
@@ -323,3 +325,133 @@ void rproc_coredump(struct rproc *rproc)
 */
wait_for_completion(_state.dump_done);
  }
+
+/**
+ * rproc_minidump() - perform minidump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered sections of
+ * segments and create a devcoredump device associated with rproc. Based on
+ * the coredump configuration this function will directly copy the segments
+ * from device memory to userspace or copy segments from device memory to
+ * a separate buffer, which can then be read by userspace.
+ * The first approach avoids using extra vmalloc memory. But it will stall
+ * recovery flow until dump is read by userspace.
+ */
+void rproc_minidump(struct rproc *rproc)

Just to confirm, this does the same thing as rproc_coredump() with the
difference that instead of storing the segments in program headers, you
reference them using section headers?


Yes, that is correct.




+{
+   struct rproc_dump_segment *segment;
+   void *shdr;
+   void *ehdr;
+   size_t data_size;
+   size_t offset;
+   void *data;
+   u8 class = rproc->elf_class;
+   int shnum;
+   struct rproc_coredump_state dump_state;
+   unsigned int dump_conf = rproc->dump_conf;
+   char *str_tbl = "STR_TBL";
+
+   if (list_empty(>dump_segments) ||
+   dump_conf == RPROC_COREDUMP_DISABLED)
+   return;
+
+   if (class == ELFCLASSNONE) {
+   dev_err(>dev, "Elf class is not set\n");
+   return;
+   }
+
+   /*
+* We allocate two extra section headers. The first one is null.
+* Second section header is for the string table. Also space is
+* allocated for string table.
+*/
+   data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class) +
+   MAX_STRTBL_SIZE;

Once you start populating the string table there's no checks that this
isn't overrun.

I will update the code to dynamically allocate space for the STRTBL.


But really


+   shnum = 2;
+
+   list_for_each_entry(segment, >dump_segments, node) {
+   data_size += elf_size_of_shdr(class);
+   if (dump_conf == RPROC_COREDUMP_DEFAULT)
+   data_size += segment->size;
+   shnum++;
+   }
+
+   data = vmalloc(data_size);
+   if (!data)
+   return;
+
+   ehdr = data;
+   memset(ehdr, 0, elf_size_of_hdr(class));
+   /* e_ident field is common for both elf32 and elf64 */
+   elf_hdr_init_ident(ehdr, class);
+
+   elf_hdr_set_e_type(class, ehdr, ET_CORE);
+   elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);
+   elf_hdr_set_e_version(class, ehdr, EV_CURRENT);
+   elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);
+   elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));
+   elf_hdr_set_e_shnum(class, ehdr, shnum);
+   elf_hdr_set_e_shstrndx(class, ehdr, 1);
+
+   /* Set the first section header as null and move to the next one. */
+   shdr = data + elf_hdr_get_e_shoff(class, ehdr);
+   memset(shdr, 0, elf_size_of_shdr(class));
+   shdr += elf_size_of_shdr(class);
+
+   /* Initialize the string table. */
+   offset = elf_hdr_get_e_shoff(class, ehdr) +
+elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);
+   memset(data + offset, 0, MAX_STRTBL_SIZE);
+
+   /* Fill in the string table section header. */
+   memset(shdr, 0, elf_size_of_shdr(class));
+   elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);
+   elf_shdr_set_sh_offset(class, shdr, of

[PATCH v6 1/4] remoteproc: core: Add ops to enable custom coredump functionality

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 7f90eee..dcc1341 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1681,7 +1681,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2103,6 +2103,10 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   /* Default to rproc_coredump if no coredump function is specified */
+   if (!rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2fa68bf..a66e2cb 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -375,6 +375,7 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:collect firmware dump after the subsystem is shutdown
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -393,6 +394,7 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 0/4] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Sometimes firmware sizes can be in tens of MB's and reading all the memory
during coredump can consume lot of time and memory.

Introducing support for mini-dumps. Mini-dump contains smallest amount of
useful information, that could help to debug subsystem crashes.

During bootup memory is allocated in SMEM (Shared memory) in the form of a
table that contains the physical addresses and sizes of the regions that
are supposed to be collected during coredump. This memory is shared amongst
all processors in a Qualcomm platform, so all remoteprocs fill in their
entry in the global table once they are out of reset.

This patch series adds support for parsing the global minidump table and
uses the current coredump frameork to expose this memory to userspace
during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v5 -> v6:
- Removed priv_cleanup operation from rproc_ops. The dump_segments list is
  updated and cleaned up each time minidump is invoked.
- Split patch #2 into 2 parts - one that adds the rproc_minidump function, and
  the other that uses the new function in the qcom_q6v5_pas driver.
- Updated structs in qcom_minidump to explicitly indicate the endianness of the
  data stored in SMEM, also updated member names.
- Read the global table of contents in SMEM each time adsp_minidump is invoked.

v4 -> v5:
- Fixed adsp_add_minidump_segments to read IO memory using appropriate 
functions.

v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
  private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
  the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (4):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: coredump: Add minidump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem

 drivers/remoteproc/qcom_minidump.h  |  64 ++
 drivers/remoteproc/qcom_q6v5_pas.c  | 105 +-
 drivers/remoteproc/remoteproc_core.c|   6 +-
 drivers/remoteproc/remoteproc_coredump.c| 132 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   3 +
 6 files changed, 334 insertions(+), 3 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 3/4] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Co-developed-by: Rishabh Bhatnagar 
Signed-off-by: Rishabh Bhatnagar 
Co-developed-by: Gurbir Arora 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_minidump.h |  64 +++
 drivers/remoteproc/qcom_q6v5_pas.c | 104 -
 2 files changed, 166 insertions(+), 2 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_minidump.h 
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..5857d06
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @valid  : This entry to be dumped (if set to 1)
+ * @address: Physical address of region to be dumped
+ * @size   : Size of the region
+ */
+struct minidump_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   __le32  seq_num;
+   __le32  valid;
+   __le64  address;
+   __le64  size;
+};
+
+/**
+ * struct minidump_subsystem_toc: Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem_toc {
+   __le32  status;
+   __le32  enabled;
+   __le32  encryption_status;
+   __le32  encryption_required;
+   __le32  ss_region_count;
+   __le64  md_ss_smem_regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct minidump_global_toc {
+   __le32  status;
+   __le32  md_revision;
+   __le32  enabled;
+   struct minidump_subsystem_toc   md_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..349f725 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,13 @@
 #include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
+#include "qcom_minidump.h"
 
 struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
int pas_id;
+   unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
 
@@ -63,6 +65,7 @@ struct qcom_adsp {
int proxy_pd_count;
 
int pas_id;
+   unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
const char *info_name;
@@ -116,6 +119,88 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, 
struct device **pds,
}
 }
 
+static void adsp_minidump_cleanup(struct rproc *rproc)
+{
+   struct rproc_dump_segment *entry, *tmp;
+
+   list_for_each_entry_safe(entry, tmp, >dump_segments, node) {
+   list_del(>node);
+   kfree(entry->priv);
+   kfree(entry);
+   }
+}
+
+static void adsp_add_minidump_segments(struct rproc *rproc,
+  struct minidump_subsystem_toc 
*minidump_ss)
+{
+   struct minidump_region __iomem *ptr;
+   struct minidump_region region;
+   int seg_cnt, i;
+   dma_addr_t da;
+   size_t size;
+   char *name;
+
+   if (!list_empty(>dump_segments)) {
+   dev_err(>dev, "dump segment list already populated\n");
+   retur

[PATCH v6 4/4] remoteproc: qcom: Add minidump id for sm8150 modem

[Siddharth Gupta]

Add minidump id for modem in sm8150 chipset so that the regions to be
included in the coredump generated upon a crash is based on the minidump
tables in SMEM instead of those in the ELF header.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_pas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 349f725..23f4532 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -707,6 +707,7 @@ static const struct adsp_data mpss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "modem.mdt",
.pas_id = 4,
+   .minidump_id = 3,
.has_aggre2_clk = false,
.auto_boot = false,
.active_pd_names = (char*[]){
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 2/4] remoteproc: coredump: Add minidump functionality

[Siddharth Gupta]

This change adds a new kind of core dump mechanism which instead of dumping
entire program segments of the firmware, dumps sections of the remoteproc
memory which are sufficient to allow debugging the firmware. This function
thus uses section headers instead of program headers during creation of the
core dump elf.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_coredump.c| 132 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   1 +
 3 files changed, 160 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c
index bb15a29..e7d1394 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -13,6 +13,8 @@
 #include "remoteproc_internal.h"
 #include "remoteproc_elf_helpers.h"
 
+#define MAX_STRTBL_SIZE 512
+
 struct rproc_coredump_state {
struct rproc *rproc;
void *header;
@@ -323,3 +325,133 @@ void rproc_coredump(struct rproc *rproc)
 */
wait_for_completion(_state.dump_done);
 }
+
+/**
+ * rproc_minidump() - perform minidump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for the registered sections of
+ * segments and create a devcoredump device associated with rproc. Based on
+ * the coredump configuration this function will directly copy the segments
+ * from device memory to userspace or copy segments from device memory to
+ * a separate buffer, which can then be read by userspace.
+ * The first approach avoids using extra vmalloc memory. But it will stall
+ * recovery flow until dump is read by userspace.
+ */
+void rproc_minidump(struct rproc *rproc)
+{
+   struct rproc_dump_segment *segment;
+   void *shdr;
+   void *ehdr;
+   size_t data_size;
+   size_t offset;
+   void *data;
+   u8 class = rproc->elf_class;
+   int shnum;
+   struct rproc_coredump_state dump_state;
+   unsigned int dump_conf = rproc->dump_conf;
+   char *str_tbl = "STR_TBL";
+
+   if (list_empty(>dump_segments) ||
+   dump_conf == RPROC_COREDUMP_DISABLED)
+   return;
+
+   if (class == ELFCLASSNONE) {
+   dev_err(>dev, "Elf class is not set\n");
+   return;
+   }
+
+   /*
+* We allocate two extra section headers. The first one is null.
+* Second section header is for the string table. Also space is
+* allocated for string table.
+*/
+   data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class) +
+   MAX_STRTBL_SIZE;
+   shnum = 2;
+
+   list_for_each_entry(segment, >dump_segments, node) {
+   data_size += elf_size_of_shdr(class);
+   if (dump_conf == RPROC_COREDUMP_DEFAULT)
+   data_size += segment->size;
+   shnum++;
+   }
+
+   data = vmalloc(data_size);
+   if (!data)
+   return;
+
+   ehdr = data;
+   memset(ehdr, 0, elf_size_of_hdr(class));
+   /* e_ident field is common for both elf32 and elf64 */
+   elf_hdr_init_ident(ehdr, class);
+
+   elf_hdr_set_e_type(class, ehdr, ET_CORE);
+   elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);
+   elf_hdr_set_e_version(class, ehdr, EV_CURRENT);
+   elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);
+   elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));
+   elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));
+   elf_hdr_set_e_shnum(class, ehdr, shnum);
+   elf_hdr_set_e_shstrndx(class, ehdr, 1);
+
+   /* Set the first section header as null and move to the next one. */
+   shdr = data + elf_hdr_get_e_shoff(class, ehdr);
+   memset(shdr, 0, elf_size_of_shdr(class));
+   shdr += elf_size_of_shdr(class);
+
+   /* Initialize the string table. */
+   offset = elf_hdr_get_e_shoff(class, ehdr) +
+elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);
+   memset(data + offset, 0, MAX_STRTBL_SIZE);
+
+   /* Fill in the string table section header. */
+   memset(shdr, 0, elf_size_of_shdr(class));
+   elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);
+   elf_shdr_set_sh_offset(class, shdr, offset);
+   elf_shdr_set_sh_size(class, shdr, MAX_STRTBL_SIZE);
+   elf_shdr_set_sh_entsize(class, shdr, 0);
+   elf_shdr_set_sh_flags(class, shdr, 0);
+   elf_shdr_set_sh_name(class, shdr, set_section_name(str_tbl, ehdr, 
class));
+   offset += elf_shdr_get_sh_size(class, shdr);
+   shdr += elf_size_of_shdr(class);
+
+   list_for_each_entry(segment, >dump_segments, node) {
+   memset(shdr, 0, elf_size_of_shdr(class));
+   elf_shdr_set_sh_type

Re: [PATCH v5 2/3] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

On 9/25/2020 9:10 PM, Bjorn Andersson wrote:

On Thu 24 Sep 16:51 PDT 2020, Siddharth Gupta wrote:


This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 

If the three of you authored this patch you should add Co-developed-by
for Rishabh and Gurbir as well.

Sure

---
  drivers/remoteproc/qcom_minidump.h  |  64 +
  drivers/remoteproc/qcom_q6v5_pas.c  | 106 -
  drivers/remoteproc/remoteproc_coredump.c| 138 
  drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
  include/linux/remoteproc.h  |   1 +
  5 files changed, 334 insertions(+), 2 deletions(-)
  create mode 100644 drivers/remoteproc/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_minidump.h 
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..437e030
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * md_ss_region - Minidump region

Valid kerneldoc should be:

  * struct md_ss_region - Minidump region


+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @md_valid   : This entry to be dumped (if set to 1)
+ * @region_base_address: Physical address of region to be dumped
+ * @region_size: Size of the region
+ */
+struct md_ss_region {

But why don't you call this struct minidump_region?


+   charname[MAX_REGION_NAME_LENGTH];
+   u32 seq_num;

Is this __le32 or __be32?


It is __le32, in that case should I update the structure to have all 
__leX values (__le32 seq_num,
__le64 address, etc.) or do a conversion using leX_to_cpu while reading 
the iomem region?



+   u32 md_valid;

"valid" would be sufficient


+   u64 region_base_address;

"address"


+   u64 region_size;

"size"


+};
+
+/**
+ * md_ss_toc: Subsystem's SMEM Table of content
+ * @md_ss_toc_init : Subsystem toc init status
+ * @md_ss_enable_status : if set to 1, this region would be copied during 
coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct md_ss_toc {

minidump_subsystem_toc


+   u32 md_ss_toc_init;

"status"


+   u32 md_ss_enable_status;

"enabled"


+   u32 encryption_status;
+   u32 encryption_required;
+   u32 ss_region_count;
+   u64 md_ss_smem_regions_baseptr;
+};
+
+/**
+ * md_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct md_global_toc {

minidump_global_toc

Sure, I will update the structs accordingly.

+   u32 md_toc_init;
+   u32 md_revision;
+   u32 md_enable_status;
+   struct md_ss_tocmd_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..752c862 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,13 @@
  #include "qcom_pil_info.h"
  #include "qcom_q6v5.h"
  #include "remoteproc_internal.h"
+#include "qcom_minidump.h"
  
  struct adsp_data {

int crash_reason_smem;
const char *firmware_name;
int pas_id;
+   unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
  
@@ -63,6 +65,7 @@ struct qcom_adsp {

int proxy_pd_count;
  
  	int pas_id

[PATCH v5 3/3] remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

[Siddharth Gupta]

Add minidump id for modem in sm8150 chipset.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_pas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 752c862..26958e1 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -709,6 +709,7 @@ static const struct adsp_data mpss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "modem.mdt",
.pas_id = 4,
+   .minidump_id = 3,
.has_aggre2_clk = false,
.auto_boot = false,
.active_pd_names = (char*[]){
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 2/3] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 106 -
 drivers/remoteproc/remoteproc_coredump.c| 138 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   1 +
 5 files changed, 334 insertions(+), 2 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_minidump.h 
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..437e030
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * md_ss_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @md_valid   : This entry to be dumped (if set to 1)
+ * @region_base_address: Physical address of region to be dumped
+ * @region_size: Size of the region
+ */
+struct md_ss_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   u32 seq_num;
+   u32 md_valid;
+   u64 region_base_address;
+   u64 region_size;
+};
+
+/**
+ * md_ss_toc: Subsystem's SMEM Table of content
+ * @md_ss_toc_init : Subsystem toc init status
+ * @md_ss_enable_status : if set to 1, this region would be copied during 
coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct md_ss_toc {
+   u32 md_ss_toc_init;
+   u32 md_ss_enable_status;
+   u32 encryption_status;
+   u32 encryption_required;
+   u32 ss_region_count;
+   u64 md_ss_smem_regions_baseptr;
+};
+
+/**
+ * md_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct md_global_toc {
+   u32 md_toc_init;
+   u32 md_revision;
+   u32 md_enable_status;
+   struct md_ss_tocmd_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..752c862 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,13 @@
 #include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
+#include "qcom_minidump.h"
 
 struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
int pas_id;
+   unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
 
@@ -63,6 +65,7 @@ struct qcom_adsp {
int proxy_pd_count;
 
int pas_id;
+   unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
const char *info_name;
@@ -81,6 +84,8 @@ struct qcom_adsp {
struct qcom_sysmon *sysmon;
 };
 
+static struct md_global_toc *g_md_toc;
+
 static int adsp_pds_enable(struct qcom_adsp *adsp, struct device **pds,
   size_t pd_count)
 {
@@ -116,6 +121,88 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, 
struct device **pds,
}
 }
 
+static void adsp_add_minidump_segments(struct rproc *rproc, struct md_ss_toc 
*minidump_ss)
+{
+   struct md_ss_region __iomem *region_info;
+   int seg_cnt = 0, i;
+   void __iomem *ptr;
+   dma_addr_t da;
+   size_t size;
+   char *name;
+
+   seg_cnt = minidump_ss->ss_region_count;
+   

[PATCH v5 1/3] remoteproc: core: Add ops to enable custom coredump functionality

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.The
priv_cleanup op cleans up the private resources used by the remoteproc.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 4 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 7f90eee..dcc1341 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1681,7 +1681,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2103,6 +2103,10 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   /* Default to rproc_coredump if no coredump function is specified */
+   if (!rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2fa68bf..a489aec 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -375,6 +375,8 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:collect firmware dump after the subsystem is shutdown
+ * @priv_cleanup: cleans up the private resources used by the rproc
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -393,6 +395,8 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
+   void (*priv_cleanup)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 0/3] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Sometimes firmware sizes can be in tens of MB's and reading
all the memory during coredump can consume lot of time and
memory.
Introducing support for mini-dumps. Mini-dump contains smallest
amount of useful information, that could help to debug subsystem
crashes.
During bootup memory is allocated in SMEM (Shared memory)
in the form of a table that contains the physical
addresses and sizes of the regions that are supposed to be
collected during coredump. This memory is shared amongst all
processors in a Qualcomm platform, so all remoteprocs
fill in their entry in the global table once they are out
of reset.
This patch series adds support for parsing the global minidump
table and uses the current coredump frameork to expose this memory
to userspace during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v4 -> v5:
- Fixed adsp_add_minidump_segments to read IO memory using appropriate 
functions.

v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
  private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
  the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (3):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 107 -
 drivers/remoteproc/remoteproc_core.c|   6 +-
 drivers/remoteproc/remoteproc_coredump.c| 138 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   5 +
 6 files changed, 344 insertions(+), 3 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 0/3] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Gentle remind to review this patch series.

Thanks,
Sid

On 9/10/2020 11:57 AM, Siddharth Gupta wrote:

Sometimes firmware sizes can be in ten's of MB's and reading
all the memory during coredump can consume lot of time and
memory.
Introducing support for mini-dumps. Mini-dump contains smallest
amount of useful information, that could help to debug subsystem
crashes.
During bootup memory is allocated in SMEM (Shared memory)
in the form of a table that contains the physical
addresses and sizes of the regions that are supposed to be
collected during coredump. This memory is shared amongst all
processors in a Qualcomm platform, so all remoteprocs
fill in their entry in the global table once they are out
of reset.
This patch series adds support for parsing the global minidump
table and uses the current coredump frameork to expose this memory
to userspace during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
   symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
   private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep
   the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (3):
   remoteproc: core: Add ops to enable custom coredump functionality
   remoteproc: qcom: Add capability to collect minidumps
   remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

  drivers/remoteproc/qcom_minidump.h  |  64 +
  drivers/remoteproc/qcom_q6v5_pas.c  | 107 -
  drivers/remoteproc/remoteproc_core.c|   6 +-
  drivers/remoteproc/remoteproc_coredump.c| 138 
  drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
  include/linux/remoteproc.h  |   5 +
  6 files changed, 344 insertions(+), 3 deletions(-)
  create mode 100644 drivers/remoteproc/qcom_minidump.h

[PATCH v4 3/3] remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

[Siddharth Gupta]

Add minidump id for modem in sm8150 chipset.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_pas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index c01e81e..5760995 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -709,6 +709,7 @@ static const struct adsp_data mpss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "modem.mdt",
.pas_id = 4,
+   .minidump_id = 3,
.has_aggre2_clk = false,
.auto_boot = false,
.active_pd_names = (char*[]){
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/3] remoteproc: core: Add ops to enable custom coredump functionality

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.The
priv_cleanup op cleans up the private resources used by the remoteproc.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 4 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 7f90eee..dcc1341 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1681,7 +1681,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2103,6 +2103,10 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   /* Default to rproc_coredump if no coredump function is specified */
+   if (!rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2fa68bf..a489aec 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -375,6 +375,8 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:collect firmware dump after the subsystem is shutdown
+ * @priv_cleanup: cleans up the private resources used by the rproc
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -393,6 +395,8 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
+   void (*priv_cleanup)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 0/3] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Sometimes firmware sizes can be in ten's of MB's and reading
all the memory during coredump can consume lot of time and
memory.
Introducing support for mini-dumps. Mini-dump contains smallest
amount of useful information, that could help to debug subsystem
crashes.
During bootup memory is allocated in SMEM (Shared memory)
in the form of a table that contains the physical
addresses and sizes of the regions that are supposed to be
collected during coredump. This memory is shared amongst all
processors in a Qualcomm platform, so all remoteprocs
fill in their entry in the global table once they are out
of reset.
This patch series adds support for parsing the global minidump
table and uses the current coredump frameork to expose this memory
to userspace during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
  private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
  the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (3):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 107 -
 drivers/remoteproc/remoteproc_core.c|   6 +-
 drivers/remoteproc/remoteproc_coredump.c| 138 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   5 +
 6 files changed, 344 insertions(+), 3 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/3] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 106 -
 drivers/remoteproc/remoteproc_coredump.c| 138 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   1 +
 5 files changed, 334 insertions(+), 2 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_minidump.h 
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..437e030
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * md_ss_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @md_valid   : This entry to be dumped (if set to 1)
+ * @region_base_address: Physical address of region to be dumped
+ * @region_size: Size of the region
+ */
+struct md_ss_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   u32 seq_num;
+   u32 md_valid;
+   u64 region_base_address;
+   u64 region_size;
+};
+
+/**
+ * md_ss_toc: Subsystem's SMEM Table of content
+ * @md_ss_toc_init : Subsystem toc init status
+ * @md_ss_enable_status : if set to 1, this region would be copied during 
coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct md_ss_toc {
+   u32 md_ss_toc_init;
+   u32 md_ss_enable_status;
+   u32 encryption_status;
+   u32 encryption_required;
+   u32 ss_region_count;
+   u64 md_ss_smem_regions_baseptr;
+};
+
+/**
+ * md_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct md_global_toc {
+   u32 md_toc_init;
+   u32 md_revision;
+   u32 md_enable_status;
+   struct md_ss_tocmd_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..c01e81e 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,13 @@
 #include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
+#include "qcom_minidump.h"
 
 struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
int pas_id;
+   unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
 
@@ -63,6 +65,7 @@ struct qcom_adsp {
int proxy_pd_count;
 
int pas_id;
+   unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
const char *info_name;
@@ -81,6 +84,8 @@ struct qcom_adsp {
struct qcom_sysmon *sysmon;
 };
 
+static struct md_global_toc *g_md_toc;
+
 static int adsp_pds_enable(struct qcom_adsp *adsp, struct device **pds,
   size_t pd_count)
 {
@@ -116,6 +121,88 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, 
struct device **pds,
}
 }
 
+static void adsp_add_minidump_segments(struct rproc *rproc, struct md_ss_toc 
*minidump_ss)
+{
+   struct md_ss_region __iomem *region_info;
+   int seg_cnt = 0, i;
+   void __iomem *ptr;
+   dma_addr_t da;
+   size_t size;
+   char *name;
+
+   seg_cnt = minidump_ss->ss_region_count;
+   

[PATCH v3 0/3] Introduce mini-dump support for remoteproc

[Siddharth Gupta]

Sometimes firmware sizes can be in ten's of MB's and reading
all the memory during coredump can consume lot of time and
memory.
Introducing support for mini-dumps. Mini-dump contains smallest
amount of useful information, that could help to debug subsystem
crashes.
During bootup memory is allocated in SMEM (Shared memory)
in the form of a table that contains the physical
addresses and sizes of the regions that are supposed to be
collected during coredump. This memory is shared amongst all
processors in a Qualcomm platform, so all remoteprocs
fill in their entry in the global table once they are out
of reset.
This patch series adds support for parsing the global minidump
table and uses the current coredump frameork to expose this memory
to userspace during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog: 
v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (3):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 107 -
 drivers/remoteproc/remoteproc_core.c|   6 +-
 drivers/remoteproc/remoteproc_coredump.c| 138 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   5 +
 6 files changed, 344 insertions(+), 3 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/3] remoteproc: qcom: Add minidump id for sm8150 modem remoteproc

[Siddharth Gupta]

Add minidump id for modem in sm8150 chipset.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_q6v5_pas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3879921..fa2d077 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -709,6 +709,7 @@ static const struct adsp_data mpss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "modem.mdt",
.pas_id = 4,
+   .minidump_id = 3,
.has_aggre2_clk = false,
.auto_boot = false,
.active_pd_names = (char*[]){
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 1/3] remoteproc: core: Add ops to enable custom coredump functionality

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.The
priv_cleanup op cleans up the private resources used by the remoteproc.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 4 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 7f90eee..dcc1341 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1681,7 +1681,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2103,6 +2103,10 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   /* Default to rproc_coredump if no coredump function is specified */
+   if (!rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2fa68bf..a489aec 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -375,6 +375,8 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:collect firmware dump after the subsystem is shutdown
+ * @priv_cleanup: cleans up the private resources used by the rproc
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -393,6 +395,8 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
+   void (*priv_cleanup)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/3] remoteproc: qcom: Add capability to collect minidumps

[Siddharth Gupta]

This patch adds support for collecting minidump in the event of remoteproc
crash. Parse the minidump table based on remoteproc's unique minidump-id,
read all memory regions from the remoteproc's minidump table entry and
expose the memory to userspace. The remoteproc platform driver can choose
to collect a full/mini dump by specifying the coredump op.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Gurbir Arora 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/qcom_minidump.h  |  64 +
 drivers/remoteproc/qcom_q6v5_pas.c  | 106 -
 drivers/remoteproc/remoteproc_coredump.c| 138 
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++
 include/linux/remoteproc.h  |   1 +
 5 files changed, 334 insertions(+), 2 deletions(-)
 create mode 100644 drivers/remoteproc/qcom_minidump.h

diff --git a/drivers/remoteproc/qcom_minidump.h 
b/drivers/remoteproc/qcom_minidump.h
new file mode 100644
index 000..437e030
--- /dev/null
+++ b/drivers/remoteproc/qcom_minidump.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_MINIDUMP_H
+#define __QCOM_MINIDUMP_H
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MD_REGION_VALID('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
+#define MD_SS_ENCR_DONE('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
+#define MD_SS_ENABLED  ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * md_ss_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @md_valid   : This entry to be dumped (if set to 1)
+ * @region_base_address: Physical address of region to be dumped
+ * @region_size: Size of the region
+ */
+struct md_ss_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   u32 seq_num;
+   u32 md_valid;
+   u64 region_base_address;
+   u64 region_size;
+};
+
+/**
+ * md_ss_toc: Subsystem's SMEM Table of content
+ * @md_ss_toc_init : Subsystem toc init status
+ * @md_ss_enable_status : if set to 1, this region would be copied during 
coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @ss_region_count : Number of regions added in this subsystem toc
+ * @md_ss_smem_regions_baseptr : regions base pointer of the subsystem
+ */
+struct md_ss_toc {
+   u32 md_ss_toc_init;
+   u32 md_ss_enable_status;
+   u32 encryption_status;
+   u32 encryption_required;
+   u32 ss_region_count;
+   u64 md_ss_smem_regions_baseptr;
+};
+
+/**
+ * md_global_toc: Global Table of Content
+ * @md_toc_init : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @md_enable_status : Minidump enable status
+ * @md_ss_toc : Array of subsystems toc
+ */
+struct md_global_toc {
+   u32 md_toc_init;
+   u32 md_revision;
+   u32 md_enable_status;
+   struct md_ss_tocmd_ss_toc[MAX_NUM_OF_SS];
+};
+
+#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..3879921 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -28,11 +28,15 @@
 #include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
+#include "qcom_minidump.h"
+
+static struct md_global_toc *g_md_toc;
 
 struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
int pas_id;
+   unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
 
@@ -63,6 +67,7 @@ struct qcom_adsp {
int proxy_pd_count;
 
int pas_id;
+   unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
const char *info_name;
@@ -116,6 +121,88 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, 
struct device **pds,
}
 }
 
+static void adsp_add_minidump_segments(struct rproc *rproc, struct md_ss_toc 
*minidump_ss)
+{
+   struct md_ss_region __iomem *region_info;
+   int seg_cnt = 0, i;
+   void __iomem *ptr;
+   dma_addr_t da;
+   size_t size;
+   char *name;
+
+   seg_cnt = minidump_ss->ss_region_count;
+   ptr = ioremap((unsigned long)minidump_ss->md_ss_smem_regions_baseptr,
+ seg_cnt * sizeof(struct md_ss_region));
+
+   if (!ptr)
+   return;
+
+   region_info = p

[PATCH] remoteproc: core: Add coredump ops to remoteproc

[Siddharth Gupta]

Each remoteproc might have different requirements for coredumps and might
want to choose the type of dumps it wants to collect. This change allows
remoteproc drivers to specify their own custom dump function to be executed
in place of rproc_coredump. If the coredump op is not specified by the
remoteproc driver it will be set to rproc_coredump by default.

Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 6 +-
 include/linux/remoteproc.h   | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 277d3bf..8ea61b0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1681,7 +1681,8 @@ int rproc_trigger_recovery(struct rproc *rproc)
goto unlock_mutex;
 
/* generate coredump */
-   rproc_coredump(rproc);
+   if (rproc->ops->coredump)
+   rproc->ops->coredump(rproc);
 
/* load firmware */
ret = request_firmware(_p, rproc->firmware, dev);
@@ -2098,6 +2099,9 @@ static int rproc_alloc_ops(struct rproc *rproc, const 
struct rproc_ops *ops)
if (!rproc->ops)
return -ENOMEM;
 
+   if (rproc->ops->coredump)
+   rproc->ops->coredump = rproc_coredump;
+
if (rproc->ops->load)
return 0;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 0e8d2ff..d22c33d 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -374,6 +374,7 @@ enum rsc_handling_status {
  * @get_boot_addr: get boot address to entry point specified in firmware
  * @panic: optional callback to react to system panic, core will delay
  * panic at least the returned number of milliseconds
+ * @coredump:  do coredump for the specified remoteproc
  */
 struct rproc_ops {
int (*prepare)(struct rproc *rproc);
@@ -392,6 +393,7 @@ struct rproc_ops {
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
unsigned long (*panic)(struct rproc *rproc);
+   void (*coredump)(struct rproc *rproc);
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 2/2] remoteproc: core: Register the character device interface

[Siddharth Gupta]

Add the character device during rproc_add. This would create
a character device node at /dev/remoteproc. Userspace
applications can interact with the remote processor using this
interface.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 277d3bf..7f90eee 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1986,6 +1986,11 @@ int rproc_add(struct rproc *rproc)
/* create debugfs entries */
rproc_create_debug_dir(rproc);
 
+   /* add char device for this remoteproc */
+   ret = rproc_char_device_add(rproc);
+   if (ret < 0)
+   return ret;
+
/*
 * Remind ourselves the remote processor has been attached to rather
 * than booted by the remoteproc core.  This is important because the
@@ -2262,6 +2267,7 @@ int rproc_del(struct rproc *rproc)
mutex_unlock(>lock);
 
rproc_delete_debug_dir(rproc);
+   rproc_char_device_remove(rproc);
 
/* the rproc is downref'ed as soon as it's removed from the klist */
mutex_lock(_list_mutex);
@@ -2430,6 +2436,7 @@ static int __init remoteproc_init(void)
 {
rproc_init_sysfs();
rproc_init_debugfs();
+   rproc_init_cdev();
rproc_init_panic();
 
return 0;
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 0/2] Add character device interface to remoteproc

[Siddharth Gupta]

This patch series adds a character device interface to remoteproc
framework. Currently there is only a sysfs interface which the userspace
clients can use. If a usersapce application crashes after booting
the remote processor through the sysfs interface the remote processor
does not get any indication about the crash. It might still assume
that the  application is running.
For example modem uses remotefs service to data from disk/flash memory.
If the remotefs service crashes, modem still keeps on requesting data
which might lead to crash on modem. Even if the service is restarted the
file handles modem requested previously would become stale.
Adding a character device interface makes the remote processor tightly
coupled with the user space application. A crash of the application
leads to a close on the file descriptors therefore shutting down the
remoteproc.

Changelog:
v4 -> v5:
- Addressed comments from Bjorn and Mathieu.
- Added cdev_set_parent call to set remoteproc device as parent of cdev.
- Fixed error with rproc_major introduced in the last patch.
- Fixed implementation for compat calls. With previous implementation 64bit
  userspace applications failed to perform the ioctl call, returning errno 25,
  or "Inappropriate ioctl for device."
- Removed exit functions from the driver as remoteproc framework is statically
  compiled.

v3 -> v4:
- Addressed comments from Mathieu and Arnaud.
- Added locks while writing/reading from the automatic-shutdown-on-release bool.
- Changed return value when failing to copy to/from userspace.
- Changed logic for calling shutdown on release.
- Moved around code after the increase of max line length from 80 to 100.
- Moved the call adding character device before device_add in rproc_add to add
  both sysfs and character device interface together.

v2 -> v3:
- Move booting of remoteproc from open to a write call.
- Add ioctl interface for future functionality extension.
- Add an ioctl call to default to rproc shutdown on release.

v1 -> v2:
- Fixed comments from Bjorn and Matthew.

Siddharth Gupta (2):
  remoteproc: Add remoteproc character device interface
  remoteproc: core: Register the character device interface

 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig |   9 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/remoteproc_cdev.c   | 124 +
 drivers/remoteproc/remoteproc_core.c   |   7 ++
 drivers/remoteproc/remoteproc_internal.h   |  28 +
 include/linux/remoteproc.h |   5 +
 include/uapi/linux/remoteproc_cdev.h   |  37 ++
 8 files changed, 212 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 1/2] remoteproc: Add remoteproc character device interface

[Siddharth Gupta]

Add the character device interface into remoteproc framework.
This interface can be used in order to boot/shutdown remote
subsystems and provides a basic ioctl based interface to implement
supplementary functionality. An ioctl call is implemented to enable
the shutdown on release feature which will allow remote processors to
be shutdown when the controlling userpsace application crashes or hangs.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig |   9 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/remoteproc_cdev.c   | 124 +
 drivers/remoteproc/remoteproc_internal.h   |  28 +
 include/linux/remoteproc.h |   5 +
 include/uapi/linux/remoteproc_cdev.h   |  37 ++
 7 files changed, 205 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 59472cd..2a19883 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -339,6 +339,7 @@ Code  Seq#Include File  
 Comments
 0xB4  00-0F  linux/gpio.h
<mailto:linux-g...@vger.kernel.org>
 0xB5  00-0F  uapi/linux/rpmsg.h  
<mailto:linux-remotep...@vger.kernel.org>
 0xB6  alllinux/fpga-dfl.h
+0xB7  alluapi/linux/remoteproc_cdev.h
<mailto:linux-remotep...@vger.kernel.org>
 0xC0  00-0F  linux/usb/iowarrior.h
 0xCA  00-0F  uapi/misc/cxl.h
 0xCA  10-2F  uapi/misc/ocxl.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48315dc..c6659dfe 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -14,6 +14,15 @@ config REMOTEPROC
 
 if REMOTEPROC
 
+config REMOTEPROC_CDEV
+   bool "Remoteproc character device interface"
+   help
+ Say y here to have a character device interface for the remoteproc
+ framework. Userspace can boot/shutdown remote processors through
+ this interface.
+
+ It's safe to say N if you don't want to use this interface.
+
 config IMX_REMOTEPROC
tristate "IMX6/7 remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 4d4307d..3dfa28e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,6 +10,7 @@ remoteproc-y  += remoteproc_debugfs.o
 remoteproc-y   += remoteproc_sysfs.o
 remoteproc-y   += remoteproc_virtio.o
 remoteproc-y   += remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_CDEV)  += remoteproc_cdev.o
 obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
 obj-$(CONFIG_INGENIC_VPU_RPROC)+= ingenic_rproc.o
 obj-$(CONFIG_MTK_SCP)  += mtk_scp.o mtk_scp_ipi.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c 
b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 000..a7ac11c
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Character device interface driver for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES  64
+static dev_t rproc_major;
+
+static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, 
size_t len, loff_t *pos)
+{
+   struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, 
cdev);
+   int ret = 0;
+   char cmd[10];
+
+   if (!len || len > sizeof(cmd))
+   return -EINVAL;
+
+   ret = copy_from_user(cmd, buf, len);
+   if (ret)
+   return -EFAULT;
+
+   if (!strncmp(cmd, "start", len)) {
+   if (rproc->state == RPROC_RUNNING)
+   return -EBUSY;
+
+   ret = rproc_boot(rproc);
+   } else if (!strncmp(cmd, "stop", len)) {
+   if (rproc->state != RPROC_RUNNING)
+   return -EINVAL;
+
+   rproc_shutdown(rproc);
+   } else {
+   dev_err(>dev, "Unrecognized option\n");
+   ret = -EINVAL;
+   }
+
+   return ret ? ret : len;
+}
+
+static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned 
long arg)
+{
+   struct rproc *rproc = container_of(filp->f_inode-&

Re: [PATCH v4 1/2] remoteproc: Add remoteproc character device interface

[Siddharth Gupta]

On 7/22/2020 10:18 AM, Mathieu Poirier wrote:

On Tue, Jul 21, 2020 at 01:56:35PM -0700, Bjorn Andersson wrote:

On Tue 21 Jul 12:16 PDT 2020, Siddharth Gupta wrote:

On 7/15/2020 2:51 PM, Mathieu Poirier wrote:

On Wed, Jul 15, 2020 at 02:18:39PM -0600, Mathieu Poirier wrote:

On Tue, Jul 07, 2020 at 12:07:49PM -0700, Siddharth Gupta wrote:

[..]

diff --git a/drivers/remoteproc/remoteproc_cdev.c 
b/drivers/remoteproc/remoteproc_cdev.c

[..]

+int rproc_char_device_add(struct rproc *rproc)
+{
+   int ret;
+   dev_t cdevt;
+
+   cdev_init(>char_dev, _fops);
+   rproc->char_dev.owner = THIS_MODULE;
+
+   cdevt = MKDEV(rproc_major, rproc->index);
+   ret = cdev_add(>char_dev, cdevt, 1);

Trying this patchset on my side gave me the following splat[1].  After finding
the root case I can't understand how you haven't see it on your side when you
tested the feature.

[1]. https://pastebin.com/aYTUUCdQ

Mathieu, I've looked at this back and forth. Afaict this implies that
rproc_major is still 0. Could it be that either alloc_chrdev_region()
failed or somehow has yet to be called when you hit this point?

That is exacly what I thought when I first stumbled on this but instrumenting
the code showed otherwise.

After function rproc_init_cdev() has been called @rproc_major contains the
dynamically allocated major number in the upper 12 bits and the base minor
number in the lower 20 bits.

In rproc_char_device_add() we find this line:

 cdevt = MKDEV(rproc_major, rproc->index);

Macro MKDEV() builds a device number by shifting @rproc_major by 20 bits to the
left and OR'ing that with @rproc->index.  But the device's major number is
already occupying the upper 12bits, so shifthing another 20 bits to the left
makes the major portion of the device number '0'.  That is causing cdev_add() to
complain bitterly.

The right way to do this is:

 cdevt = MKDEV(MAJOR(rproc_major), rproc->index);

Once I found the problem I thought about 32/64 bit issues.  Since Siddharth is
using a 64bit application processor shifting another 20 bits would still have
yielded a non-zero value.  But that can't be since dev_t is a u32 in
linux/types.h.

As such I can't see how it is possible to not hit that problem on a 64bit
platform.

Hey Mathieu,

I just checked my testing tree for our devices and realized that I have 
an older version
of the patch. Hence I was unable to reproduce the error. I will fix this 
problem, and

send out a new patchset today.

Sorry about this error!

Thanks,
Sid

Hey Mathieu,

We aren't able to reproduce the error that you are seeing, the splat is
coming
from the check for whiteout device[1] - which shouldn't happen because of
the
find_dynamic_major call[2], right?

We are successfully seeing all our character device files and able to
successfully boot remoteprocs. From what I read and understood about
whiteout
devices they will be hidden in the fs.

Could you provide more details about your configuration and testing?

[1]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486
<https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123>
[2]: https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L123

<https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L486>

+   if (ret < 0)
+   goto out;
+
+   rproc->dev.devt = cdevt;
+out:
+   return ret;
+}
+
+void rproc_char_device_remove(struct rproc *rproc)
+{
+   __unregister_chrdev(rproc_major, rproc->index, 1, "remoteproc");
+}
+
+void __init rproc_init_cdev(void)
+{
+   int ret;
+
+   ret = alloc_chrdev_region(_major, 0, NUM_RPROC_DEVICES, 
"remoteproc");
+   if (ret < 0)
+   pr_err("Failed to alloc rproc_cdev region, err %d\n", ret);
+}
+
+void __exit rproc_exit_cdev(void)
+{
+   unregister_chrdev_region(MKDEV(rproc_major, 0), NUM_RPROC_DEVICES);

Please go back to the comment I made on this during my last review and respin.

After digging in the code while debugging the above problem, I don't see how
unregistering the chrdev region the way it is done here would have worked.

Since this is compiled statically and not built as a module, we will never
exercise the code path, so I will remove it in the next patchset.


You're right Siddharth, since we changed CONFIG_REMOTEPROC to bool it's no 
longer
possible to hit remoteproc_exit(), so you can omit this function
entirely. (And we should clean up the rest of that as well)

[..]

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h

[..]

@@ -488,6 +489,8 @@ struct rproc_dump_segment {
* @auto_boot: flag to indicate if remote processor should be auto-started
* @dump_segments: list of segments in the firmware
* @nb_vdev: number of vdev currently handled by rproc
+ * @char_dev: character device of the rproc
+ * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on 
@char_de

[PATCH v4 0/2] Add character device interface to remoteproc

[Siddharth Gupta]

This patch series adds a character device interface to remoteproc
framework. Currently there is only a sysfs interface which the userspace
clients can use. If a usersapce application crashes after booting
the remote processor through the sysfs interface the remote processor
does not get any indication about the crash. It might still assume
that the  application is running.
For example modem uses remotefs service to data from disk/flash memory.
If the remotefs service crashes, modem still keeps on requesting data
which might lead to crash on modem. Even if the service is restarted the
file handles modem requested previously would become stale.
Adding a character device interface makes the remote processor tightly
coupled with the user space application. A crash of the application
leads to a close on the file descriptors therefore shutting down the
remoteproc.

Changelog:
v3 -> v4:
- Addressed comments from Mathieu and Arnaud.
- Added locks while writing/reading from the automatic-shutdown-on-release bool.
- Changed return value when failing to copy to/from userspace.
- Changed logic for calling shutdown on release.
- Moved around code after the increase of max line length from 80 to 100.
- Moved the call adding character device before device_add in rproc_add to add
  both sysfs and character device interface together.

v2 -> v3:
- Move booting of remoteproc from open to a write call.
- Add ioctl interface for future functionality extension.
- Add an ioctl call to default to rproc shutdown on release.

v1 -> v2:
- Fixed comments from Bjorn and Matthew.

Siddharth Gupta (2):
  remoteproc: Add remoteproc character device interface
  remoteproc: core: Register the character device interface

 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig |   9 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/remoteproc_cdev.c   | 146 +
 drivers/remoteproc/remoteproc_core.c   |  10 ++
 drivers/remoteproc/remoteproc_internal.h   |  28 
 include/linux/remoteproc.h |   5 +
 include/uapi/linux/remoteproc_cdev.h   |  37 ++
 8 files changed, 237 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/2] remoteproc: Add remoteproc character device interface

[Siddharth Gupta]

Add the character device interface into remoteproc framework.
This interface can be used in order to boot/shutdown remote
subsystems and provides a basic ioctl based interface to implement
supplementary functionality. An ioctl call is implemented to enable
the shutdown on release feature which will allow remote processors to
be shutdown when the controlling userpsace application crashes or hangs.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig |   9 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/remoteproc_cdev.c   | 146 +
 drivers/remoteproc/remoteproc_internal.h   |  28 
 include/linux/remoteproc.h |   5 +
 include/uapi/linux/remoteproc_cdev.h   |  37 ++
 7 files changed, 227 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 59472cd..2a19883 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -339,6 +339,7 @@ Code  Seq#Include File  
 Comments
 0xB4  00-0F  linux/gpio.h
<mailto:linux-g...@vger.kernel.org>
 0xB5  00-0F  uapi/linux/rpmsg.h  
<mailto:linux-remotep...@vger.kernel.org>
 0xB6  alllinux/fpga-dfl.h
+0xB7  alluapi/linux/remoteproc_cdev.h
<mailto:linux-remotep...@vger.kernel.org>
 0xC0  00-0F  linux/usb/iowarrior.h
 0xCA  00-0F  uapi/misc/cxl.h
 0xCA  10-2F  uapi/misc/ocxl.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c4d1731..652060f 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -14,6 +14,15 @@ config REMOTEPROC
 
 if REMOTEPROC
 
+config REMOTEPROC_CDEV
+   bool "Remoteproc character device interface"
+   help
+ Say y here to have a character device interface for the remoteproc
+ framework. Userspace can boot/shutdown remote processors through
+ this interface.
+
+ It's safe to say N if you don't want to use this interface.
+
 config IMX_REMOTEPROC
tristate "IMX6/7 remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e8b886e..311ae3f 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,6 +9,7 @@ remoteproc-y+= remoteproc_debugfs.o
 remoteproc-y   += remoteproc_sysfs.o
 remoteproc-y   += remoteproc_virtio.o
 remoteproc-y   += remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_CDEV)  += remoteproc_cdev.o
 obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
 obj-$(CONFIG_INGENIC_VPU_RPROC)+= ingenic_rproc.o
 obj-$(CONFIG_MTK_SCP)  += mtk_scp.o mtk_scp_ipi.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c 
b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 000..8a0eb47
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Character device interface driver for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES  64
+static dev_t rproc_major;
+
+static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, 
size_t len, loff_t *pos)
+{
+   struct rproc *rproc = container_of(filp->f_inode->i_cdev, struct rproc, 
char_dev);
+   int ret = 0;
+   char cmd[10];
+
+   if (!len || len > sizeof(cmd))
+   return -EINVAL;
+
+   ret = copy_from_user(cmd, buf, sizeof(cmd));
+   if (ret)
+   return -EFAULT;
+
+   if (sysfs_streq(cmd, "start")) {
+   if (rproc->state == RPROC_RUNNING)
+   return -EBUSY;
+
+   ret = rproc_boot(rproc);
+   if (ret)
+   dev_err(>dev, "Boot failed:%d\n", ret);
+   } else if (sysfs_streq(cmd, "stop")) {
+   if (rproc->state != RPROC_RUNNING)
+   return -EINVAL;
+
+   rproc_shutdown(rproc);
+   } else {
+   dev_err(>dev, "Unrecognized option\n");
+   ret = -EINVAL;
+   }
+
+   return ret ? ret : len;
+}
+
+static long rproc_device_ioctl(struct fi

[PATCH v4 2/2] remoteproc: core: Register the character device interface

[Siddharth Gupta]

Add the character device during rproc_add. This would create
a character device node at /dev/remoteproc. Userspace
applications can interact with the remote processor using this
interface.

Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Siddharth Gupta 
---
 drivers/remoteproc/remoteproc_core.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index 0f95e02..ec7fb49 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1966,6 +1966,13 @@ int rproc_add(struct rproc *rproc)
struct device *dev = >dev;
int ret;
 
+   /* add char device for this remoteproc */
+   ret = rproc_char_device_add(rproc);
+   if (ret) {
+   dev_err(dev, "Failed to add char dev for %s\n", rproc->name);
+   return ret;
+   }
+
ret = device_add(dev);
if (ret < 0)
return ret;
@@ -2241,6 +2248,7 @@ int rproc_del(struct rproc *rproc)
mutex_unlock(>lock);
 
rproc_delete_debug_dir(rproc);
+   rproc_char_device_remove(rproc);
 
/* the rproc is downref'ed as soon as it's removed from the klist */
mutex_lock(_list_mutex);
@@ -2409,6 +2417,7 @@ static int __init remoteproc_init(void)
 {
rproc_init_sysfs();
rproc_init_debugfs();
+   rproc_init_cdev();
rproc_init_panic();
 
return 0;
@@ -2420,6 +2429,7 @@ static void __exit remoteproc_exit(void)
ida_destroy(_dev_index);
 
rproc_exit_panic();
+   rproc_exit_cdev();
rproc_exit_debugfs();
rproc_exit_sysfs();
 }
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface

[Siddharth Gupta]

On 6/30/2020 12:43 AM, Arnaud POULIQUEN wrote:


On 6/30/20 7:38 AM, Siddharth Gupta wrote:

On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:

On 6/16/20 9:56 PM, risha...@codeaurora.org wrote:

On 2020-04-30 01:30, Arnaud POULIQUEN wrote:

Hi Rishabh,


On 4/21/20 8:10 PM, Rishabh Bhatnagar wrote:

Add the character device interface into remoteproc framework.
This interface can be used in order to boot/shutdown remote
subsystems and provides a basic ioctl based interface to implement
supplementary functionality. An ioctl call is implemented to enable
the shutdown on release feature which will allow remote processors to
be shutdown when the controlling userpsace application crashes or
hangs.


Thanks for intruducing Ioctl, this will help for future evolutions.


Signed-off-by: Rishabh Bhatnagar 
---
   Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
   drivers/remoteproc/Kconfig |   9 ++
   drivers/remoteproc/Makefile|   1 +
   drivers/remoteproc/remoteproc_cdev.c   | 143
+
   drivers/remoteproc/remoteproc_internal.h   |  21 +++
   include/linux/remoteproc.h |   3 +
   include/uapi/linux/remoteproc_cdev.h   |  20 +++
   7 files changed, 198 insertions(+)
   create mode 100644 drivers/remoteproc/remoteproc_cdev.c
   create mode 100644 include/uapi/linux/remoteproc_cdev.h

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst
b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 2e91370..412b2a0 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -337,6 +337,7 @@ Code  Seq#Include File
Comments
   0xB4  00-0F  linux/gpio.h
<mailto:linux-g...@vger.kernel.org>
   0xB5  00-0F  uapi/linux/rpmsg.h
<mailto:linux-remotep...@vger.kernel.org>
   0xB6  alllinux/fpga-dfl.h
+0xB7  alluapi/linux/remoteproc_cdev.h  
<mailto:linux-remotep...@vger.kernel.org>
   0xC0  00-0F  linux/usb/iowarrior.h
   0xCA  00-0F  uapi/misc/cxl.h
   0xCA  10-2F  uapi/misc/ocxl.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index de3862c..6374b79 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -14,6 +14,15 @@ config REMOTEPROC

   if REMOTEPROC

+config REMOTEPROC_CDEV
+   bool "Remoteproc character device interface"
+   help
+ Say y here to have a character device interface for Remoteproc
+ framework. Userspace can boot/shutdown remote processors through
+ this interface.
+
+ It's safe to say N if you don't want to use this interface.
+
   config IMX_REMOTEPROC
tristate "IMX6/7 remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e30a1b1..b7d4f77 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,6 +9,7 @@ remoteproc-y+= remoteproc_debugfs.o
   remoteproc-y += remoteproc_sysfs.o
   remoteproc-y += remoteproc_virtio.o
   remoteproc-y += remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_CDEV)  += remoteproc_cdev.o
   obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
   obj-$(CONFIG_MTK_SCP)+= mtk_scp.o mtk_scp_ipi.o
   obj-$(CONFIG_OMAP_REMOTEPROC)+= omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c
b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 000..65142ec
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Character device interface driver for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES  64
+static dev_t rproc_major;
+
+static ssize_t rproc_cdev_write(struct file *filp, const char __user
*buf,
+size_t len, loff_t *pos)
+{
+   struct rproc *rproc = container_of(filp->f_inode->i_cdev,
+  struct rproc, char_dev);
+   int ret = 0;
+   char cmd[10];
+
+   if (!len || len > sizeof(cmd))
+   return -EINVAL;
+
+   ret = copy_from_user(cmd, buf, sizeof(cmd));
+   if (ret)
+   return -EFAULT;
+
+   if (sysfs_streq(cmd, "start")) {
+   if (rproc->state == RPROC_RUNNING)
+   return -EBUSY;
+
+   ret = rproc_boot(rproc);
+   if (ret)
+   dev_err(>dev, "Boot failed:%d\n", ret);
+   } else if (sysfs_streq(cmd, "stop")) {
+

Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface

[Siddharth Gupta]

On 6/17/2020 1:44 AM, Arnaud POULIQUEN wrote:


On 6/16/20 9:56 PM, risha...@codeaurora.org wrote:

On 2020-04-30 01:30, Arnaud POULIQUEN wrote:

Hi Rishabh,


On 4/21/20 8:10 PM, Rishabh Bhatnagar wrote:

Add the character device interface into remoteproc framework.
This interface can be used in order to boot/shutdown remote
subsystems and provides a basic ioctl based interface to implement
supplementary functionality. An ioctl call is implemented to enable
the shutdown on release feature which will allow remote processors to
be shutdown when the controlling userpsace application crashes or
hangs.


Thanks for intruducing Ioctl, this will help for future evolutions.


Signed-off-by: Rishabh Bhatnagar 
---
  Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
  drivers/remoteproc/Kconfig |   9 ++
  drivers/remoteproc/Makefile|   1 +
  drivers/remoteproc/remoteproc_cdev.c   | 143
+
  drivers/remoteproc/remoteproc_internal.h   |  21 +++
  include/linux/remoteproc.h |   3 +
  include/uapi/linux/remoteproc_cdev.h   |  20 +++
  7 files changed, 198 insertions(+)
  create mode 100644 drivers/remoteproc/remoteproc_cdev.c
  create mode 100644 include/uapi/linux/remoteproc_cdev.h

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst
b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 2e91370..412b2a0 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -337,6 +337,7 @@ Code  Seq#Include File
   Comments
  0xB4  00-0F  linux/gpio.h

  0xB5  00-0F  uapi/linux/rpmsg.h

  0xB6  alllinux/fpga-dfl.h
+0xB7  alluapi/linux/remoteproc_cdev.h  

  0xC0  00-0F  linux/usb/iowarrior.h
  0xCA  00-0F  uapi/misc/cxl.h
  0xCA  10-2F  uapi/misc/ocxl.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index de3862c..6374b79 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -14,6 +14,15 @@ config REMOTEPROC

  if REMOTEPROC

+config REMOTEPROC_CDEV
+   bool "Remoteproc character device interface"
+   help
+ Say y here to have a character device interface for Remoteproc
+ framework. Userspace can boot/shutdown remote processors through
+ this interface.
+
+ It's safe to say N if you don't want to use this interface.
+
  config IMX_REMOTEPROC
tristate "IMX6/7 remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e30a1b1..b7d4f77 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,6 +9,7 @@ remoteproc-y+= remoteproc_debugfs.o
  remoteproc-y  += remoteproc_sysfs.o
  remoteproc-y  += remoteproc_virtio.o
  remoteproc-y  += remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_CDEV)  += remoteproc_cdev.o
  obj-$(CONFIG_IMX_REMOTEPROC)  += imx_rproc.o
  obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
  obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c
b/drivers/remoteproc/remoteproc_cdev.c
new file mode 100644
index 000..65142ec
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Character device interface driver for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES  64
+static dev_t rproc_major;
+
+static ssize_t rproc_cdev_write(struct file *filp, const char __user
*buf,
+size_t len, loff_t *pos)
+{
+   struct rproc *rproc = container_of(filp->f_inode->i_cdev,
+  struct rproc, char_dev);
+   int ret = 0;
+   char cmd[10];
+
+   if (!len || len > sizeof(cmd))
+   return -EINVAL;
+
+   ret = copy_from_user(cmd, buf, sizeof(cmd));
+   if (ret)
+   return -EFAULT;
+
+   if (sysfs_streq(cmd, "start")) {
+   if (rproc->state == RPROC_RUNNING)
+   return -EBUSY;
+
+   ret = rproc_boot(rproc);
+   if (ret)
+   dev_err(>dev, "Boot failed:%d\n", ret);
+   } else if (sysfs_streq(cmd, "stop")) {
+   if (rproc->state == RPROC_OFFLINE)
+   return -ENXIO;

returning ENXIO in this case seems to me no appropriate , what about
EPERM or
EINVAL (rproc_sysfs) ?


I think EPERM would indicate the operation is 

[PATCH v3] scripts: headers_install: Exit with error on config leak

[Siddharth Gupta]

Misuse of CONFIG_* in UAPI headers should result in an error. These config
options can be set in userspace by the user application which includes
these headers to control the APIs and structures being used in a kernel
which supports multiple targets.

Signed-off-by: Siddharth Gupta 
---
 scripts/headers_install.sh | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
index a07668a..94a8335 100755
--- a/scripts/headers_install.sh
+++ b/scripts/headers_install.sh
@@ -64,7 +64,7 @@ configs=$(sed -e '
d
 ' $OUTFILE)
 
-# The entries in the following list are not warned.
+# The entries in the following list do not result in an error.
 # Please do not add a new entry. This list is only for existing ones.
 # The list will be reduced gradually, and deleted eventually. (hopefully)
 #
@@ -98,18 +98,19 @@ include/uapi/linux/raw.h:CONFIG_MAX_RAW_DEVS
 
 for c in $configs
 do
-   warn=1
+   leak_error=1
 
for ignore in $config_leak_ignores
do
if echo "$INFILE:$c" | grep -q "$ignore$"; then
-   warn=
+   leak_error=
break
fi
done
 
-   if [ "$warn" = 1 ]; then
-   echo "warning: $INFILE: leak $c to user-space" >&2
+   if [ "$leak_error" = 1 ]; then
+   echo "error: $INFILE: leak $c to user-space" >&2
+   exit 1
fi
 done
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] scripts: headers_install: Exit with error on config leak

[Siddharth Gupta]

On 5/4/2020 12:18 AM, Masahiro Yamada wrote:

On Sun, May 3, 2020 at 12:29 PM Siddharth Gupta  wrote:

Misuse of CONFIG_* in UAPI headers should result in an error as it exposes
configuration of different targets to userspace.


Sorry, I missed to point out this.

This statement is not precious; it does not expose the kernel
configuration at all.

include/generated/autoconf.h is not available from userspace.
So, all the CONFIG options are undefined.

Could you reword the commit somehow?

Sure. I just meant that if a CONFIG_* is present in the UAPI header it 
can be set in a
userspace application that includes the header to manipulate the control 
flow in a

kernel that might support multiple targets.

I'll update the commit text and send out an updated patch.

Thanks,
Siddharth






Signed-off-by: Siddharth Gupta 
---
  scripts/headers_install.sh | 11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
index a07668a..94a8335 100755
--- a/scripts/headers_install.sh
+++ b/scripts/headers_install.sh
@@ -64,7 +64,7 @@ configs=$(sed -e '
 d
  ' $OUTFILE)

-# The entries in the following list are not warned.
+# The entries in the following list do not result in an error.
  # Please do not add a new entry. This list is only for existing ones.
  # The list will be reduced gradually, and deleted eventually. (hopefully)
  #
@@ -98,18 +98,19 @@ include/uapi/linux/raw.h:CONFIG_MAX_RAW_DEVS

  for c in $configs
  do
-   warn=1
+   leak_error=1

 for ignore in $config_leak_ignores
 do
 if echo "$INFILE:$c" | grep -q "$ignore$"; then
-   warn=
+   leak_error=
 break
 fi
 done

-   if [ "$warn" = 1 ]; then
-   echo "warning: $INFILE: leak $c to user-space" >&2
+   if [ "$leak_error" = 1 ]; then
+   echo "error: $INFILE: leak $c to user-space" >&2
+   exit 1
 fi
  done

--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2] scripts: headers_install: Exit with error on config leak

[Siddharth Gupta]

Misuse of CONFIG_* in UAPI headers should result in an error as it exposes
configuration of different targets to userspace.

Signed-off-by: Siddharth Gupta 
---
 scripts/headers_install.sh | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
index a07668a..94a8335 100755
--- a/scripts/headers_install.sh
+++ b/scripts/headers_install.sh
@@ -64,7 +64,7 @@ configs=$(sed -e '
d
 ' $OUTFILE)
 
-# The entries in the following list are not warned.
+# The entries in the following list do not result in an error.
 # Please do not add a new entry. This list is only for existing ones.
 # The list will be reduced gradually, and deleted eventually. (hopefully)
 #
@@ -98,18 +98,19 @@ include/uapi/linux/raw.h:CONFIG_MAX_RAW_DEVS
 
 for c in $configs
 do
-   warn=1
+   leak_error=1
 
for ignore in $config_leak_ignores
do
if echo "$INFILE:$c" | grep -q "$ignore$"; then
-   warn=
+   leak_error=
break
fi
done
 
-   if [ "$warn" = 1 ]; then
-   echo "warning: $INFILE: leak $c to user-space" >&2
+   if [ "$leak_error" = 1 ]; then
+   echo "error: $INFILE: leak $c to user-space" >&2
+   exit 1
fi
 done
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] scripts: headers_install: Exit with error on config leak

[Siddharth Gupta]

Sure I will make the recommended changes and send a v2 of the patch.

Thanks,
Siddharth

On 5/2/2020 8:03 AM, Masahiro Yamada wrote:

On Sat, May 2, 2020 at 6:55 AM Siddharth Gupta  wrote:

Misuse of CONFIG_* in UAPI headers should result in an error as it exposes
configuration of different targets to userspace.

Signed-off-by: Siddharth Gupta 
---
  scripts/headers_install.sh | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
index a07668a..bd6c93a 100755
--- a/scripts/headers_install.sh
+++ b/scripts/headers_install.sh
@@ -109,7 +109,8 @@ do
 done

 if [ "$warn" = 1 ]; then
-   echo "warning: $INFILE: leak $c to user-space" >&2
+   echo "error: $INFILE: leak $c to user-space" >&2
+   exit 1
 fi
  done


If you want to change this,
please update the comment at line 67.

Also, rename the variable $warn to
something else, $error or $leak_error, etc. ?

[PATCH] scripts: headers_install: Exit with error on config leak

[Siddharth Gupta]

Misuse of CONFIG_* in UAPI headers should result in an error as it exposes
configuration of different targets to userspace.

Signed-off-by: Siddharth Gupta 
---
 scripts/headers_install.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
index a07668a..bd6c93a 100755
--- a/scripts/headers_install.sh
+++ b/scripts/headers_install.sh
@@ -109,7 +109,8 @@ do
done
 
if [ "$warn" = 1 ]; then
-   echo "warning: $INFILE: leak $c to user-space" >&2
+   echo "error: $INFILE: leak $c to user-space" >&2
+   exit 1
fi
 done
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project