Skip to contentSkip to conversation

soc: qcom: aoss: Expose send for generic usecase

PATCH Superseded Deepak Kumar Singh 4 participants 10 messages Apr 2, 2021 – Apr 13, 2021

Subsystems (2)
Arm Qualcomm mailing list Arm Qualcomm support

v1 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Deepak Kumar Singh

author
#0 lore raw

cover soc: qcom: aoss: Expose send for generic usecase

[Change from V0]
Update qmp_get to parse qmp handle with binding qcom,qmp

Deepak Kumar Singh (2):
soc: qcom: aoss: Expose send for generic usecase
soc: qcom: aoss: Add debugfs entry

 drivers/soc/qcom/qcom_aoss.c | 77 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH 1/2 soc: qcom: aoss: Expose send for generic usecase

Not all upcoming usecases will have an interface to allow the aoss driver to hook onto. Expose the send api and create a get function to enable drivers to send their own messages to aoss.

Signed-off-by Chris Lew <[email protected]>
Signed-off-by Deepak Kumar Singh <[email protected]>
---
 drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)
drivers/soc/qcom/qcom_aoss.c +35 -1
index 53acb94..5c643f0 100644
@@ -8,10 +8,12 @@
 #include <linux/io.h>
 #include <linux/mailbox_client.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/thermal.h>
 #include <linux/slab.h>
+#include <linux/soc/qcom/qcom_aoss.h>
 
 #define QMP_DESC_MAGIC			0x0
 #define QMP_DESC_VERSION		0x4

@@ -223,11 +225,14 @@ static bool qmp_message_empty(struct qmp *qmp)
  *
  * Return: 0 on success, negative errno on failure
  */
-static int qmp_send(struct qmp *qmp, const void *data, size_t len)
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
 {
 	long time_left;
 	int ret;
 
+	if (!qmp || !data)
+		return -EINVAL;
+
 	if (WARN_ON(len + sizeof(u32) > qmp->size))
 		return -EINVAL;
 

@@ -261,6 +266,7 @@ static int qmp_send(struct qmp *qmp, const void *data, size_t len)
 
 	return ret;
 }
+EXPORT_SYMBOL(qmp_send);
 
 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {

@@ -515,6 +521,34 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
 		thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
 }
 
+/**
+ * qmp_get() - get a qmp handle from a device
+ * @dev: client device pointer
+ *
+ * Return: handle to qmp device on success, ERR_PTR() on failure
+ */
+struct qmp *qmp_get(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+	struct qmp *qmp;
+
+	if (!dev || !dev->of_node)
+		return ERR_PTR(-ENODEV);
+
+	np = of_parse_phandle(dev->of_node, "qcom,qmp", 0);
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return ERR_PTR(-EINVAL);
+
+	qmp = platform_get_drvdata(pdev);
+	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
build bot
#2 lore raw
Thank you for the patch! Yet something to improve:

Hi Deepak,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc5 next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Deepak-Kumar-Singh/soc-qcom-aoss-Expose-send-for-generic-usecase/20210402-141935
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1678e493d530e7977cce34e59a86bb86f3c5631e
config: powerpc64-randconfig-r001-20210401 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project b23a314146956dd29b719ab537608ced736fc036)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/799aeefaad3e944f54af76e20156de53331f5d3d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Deepak-Kumar-Singh/soc-qcom-aoss-Expose-send-for-generic-usecase/20210402-141935 git checkout 799aeefaad3e944f54af76e20156de53331f5d3d
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/soc/qcom/qcom_aoss.c:16:10: fatal error: 'linux/soc/qcom/qcom_aoss.h' file not found
   #include <linux/soc/qcom/qcom_aoss.h>
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

1 error generated.

vim +16 drivers/soc/qcom/qcom_aoss.c

  > 16	#include <linux/soc/qcom/qcom_aoss.h>
    17

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

build bot
#3 lore raw
In reply to Deepak Kumar Singh
Thank you for the patch! Yet something to improve:

Hi Deepak,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc5 next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Deepak-Kumar-Singh/soc-qcom-aoss-Expose-send-for-generic-usecase/20210402-141935
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1678e493d530e7977cce34e59a86bb86f3c5631e
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/799aeefaad3e944f54af76e20156de53331f5d3d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Deepak-Kumar-Singh/soc-qcom-aoss-Expose-send-for-generic-usecase/20210402-141935 git checkout 799aeefaad3e944f54af76e20156de53331f5d3d
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/soc/qcom/qcom_aoss.c:16:10: fatal error: linux/soc/qcom/qcom_aoss.h: No such file or directory
      16 | #include <linux/soc/qcom/qcom_aoss.h>
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

compilation terminated.

vim +16 drivers/soc/qcom/qcom_aoss.c

  > 16	#include <linux/soc/qcom/qcom_aoss.h>
    17

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#4 lore raw
In reply to Deepak Kumar Singh
Quoted 15 lines · Deepak Kumar Singh

Not all upcoming usecases will have an interface to allow the aoss driver to hook onto. Expose the send api and create a get function to enable drivers to send their own messages to aoss.

Signed-off-by Chris Lew <[email protected]>
Signed-off-by Deepak Kumar Singh <[email protected]>
---
 drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)
drivers/soc/qcom/qcom_aoss.c +0 -0
index 53acb94..5c643f0 100644

drivers/soc/qcom/qcom_aoss.c line 16

Quoted 9 lines
drivers/soc/qcom/qcom_aoss.c +2 -0
@@ -8,10 +8,12 @@
 #include <linux/io.h>
 #include <linux/mailbox_client.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/thermal.h>
 #include <linux/slab.h>
+#include <linux/soc/qcom/qcom_aoss.h>

I believe you forgot to 'git add' this.

Quoted 4 lines
drivers/soc/qcom/qcom_aoss.c +0 -0
 #define QMP_DESC_MAGIC			0x0
 #define QMP_DESC_VERSION		0x4

drivers/soc/qcom/qcom_aoss.c static bool qmp_message_empty(struct qmp *qmp) · line 233

Quoted 10 lines
drivers/soc/qcom/qcom_aoss.c +2 -1
@@ -223,11 +225,14 @@ static bool qmp_message_empty(struct qmp *qmp)
  *
  * Return: 0 on success, negative errno on failure
  */
-static int qmp_send(struct qmp *qmp, const void *data, size_t len)
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
 {
 	long time_left;
 	int ret;
 
+	if (!qmp || !data)

I don't see a legit use case where these are NULL, so there's probably a developer staring at the kernel log wondering why their code isn't working. So better wrap this in a WARN_ON() to help him/her.

Also, a developer failing to check the return value of qmp_get() would get here with qmp being -ENODEV, -EINVAL or -EPROBE_DEFER. Which we would gladly dereference in the next conditional. So rather than !qmp, IS_ERR_OR_NULL(qmp) would be useful.

Quoted 21 lines
drivers/soc/qcom/qcom_aoss.c +7 -0
+		return -EINVAL;
+
 	if (WARN_ON(len + sizeof(u32) > qmp->size))
 		return -EINVAL;
 

@@ -261,6 +266,7 @@ static int qmp_send(struct qmp *qmp, const void *data, size_t len)
 
 	return ret;
 }
+EXPORT_SYMBOL(qmp_send);
 
 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {

@@ -515,6 +521,34 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
 		thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
 }
 
+/**
+ * qmp_get() - get a qmp handle from a device
+ * @dev: client device pointer
+ *

drivers/soc/qcom/qcom_aoss.c static void qmp_cooling_devices_remove(struct qmp *qmp) · line 537

Quoted 10 lines
drivers/soc/qcom/qcom_aoss.c +10 -0
+ * Return: handle to qmp device on success, ERR_PTR() on failure
+ */
+struct qmp *qmp_get(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+	struct qmp *qmp;
+
+	if (!dev || !dev->of_node)
+		return ERR_PTR(-ENODEV);

Value of @dev is an invalid argument, so I think -EINVAL is suitable.

drivers/soc/qcom/qcom_aoss.c static void qmp_cooling_devices_remove(struct qmp *qmp) · line 543

Quoted 6 lines
drivers/soc/qcom/qcom_aoss.c +6 -0
+
+	np = of_parse_phandle(dev->of_node, "qcom,qmp", 0);
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	pdev = of_find_device_by_node(np);

of_find_device_by_node() will increment the refcount of the underlying struct device of pdev, so you need to platform_device_put() once you're done with it.

As a side effect of not putting the struct device, the devm_kzalloc'ed qmp pointer will remain valid. So care is needed to make sure that the client doesn't end up with a dangling pointer if the qmp device is removed.

My suggestion is that you add a "qmp_put()" function, which invokes platform_device_put() and that you add some sort of tracking ("bool orphan"?) to the struct qmp and make qmp_send() fail if this is set.

That way if someone unbinds the aoss device, the client will still have a "valid" pointer, but won't be able to qmp_send() after qmp_close() has been called in the aoss remove function.

Regards,
Bjorn

Quoted 15 lines
drivers/soc/qcom/qcom_aoss.c +8 -0
+	if (!pdev)
+		return ERR_PTR(-EINVAL);
+
+	qmp = platform_get_drvdata(pdev);
+	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#5 lore raw
Quoted 17 lines · Bjorn Andersson
Quoted 15 lines · Deepak Kumar Singh

Not all upcoming usecases will have an interface to allow the aoss driver to hook onto. Expose the send api and create a get function to enable drivers to send their own messages to aoss.

Signed-off-by Chris Lew <[email protected]>
Signed-off-by Deepak Kumar Singh <[email protected]>
---
 drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)
drivers/soc/qcom/qcom_aoss.c +0 -0
index 53acb94..5c643f0 100644
@@ -8,10 +8,12 @@

[...]

Quoted 6 lines

drivers/soc/qcom/qcom_aoss.c static void qmp_cooling_devices_remove(struct qmp *qmp) · line 543

Quoted 1 line
drivers/soc/qcom/qcom_aoss.c +1 -0
+	pdev = of_find_device_by_node(np);

of_find_device_by_node() will increment the refcount of the underlying struct device of pdev, so you need to platform_device_put() once you're done with it.

Quoted 9 lines

As a side effect of not putting the struct device, the devm_kzalloc'ed qmp pointer will remain valid. So care is needed to make sure that the client doesn't end up with a dangling pointer if the qmp device is removed.

My suggestion is that you add a "qmp_put()" function, which invokes platform_device_put() and that you add some sort of tracking ("bool orphan"?) to the struct qmp and make qmp_send() fail if this is set.

I think this is a duplication of what the struct device offers. Why can't we use the generic infrastructure for this usecase?

Like using device_initialize() in qmp_probe() along with a release callback for "struct device", then using get_device() in qmp_get(). Then there should also be a qmp_put() API which calls put_device() to decrease the refcount.

Ideally, the final refcount should be dropped in qmp_remove() and then the release callback will be called automatically to free "struct qmp".

Quoted 4 lines

That way if someone unbinds the aoss device, the client will still have a "valid" pointer, but won't be able to qmp_send() after qmp_close() has been called in the aoss remove function.

How can someone remove the qmp device if a client is holding its reference?

Thanks,
Mani

Quoted 18 lines

Regards,
Bjorn

Quoted 15 lines
drivers/soc/qcom/qcom_aoss.c +8 -0
+	if (!pdev)
+		return ERR_PTR(-EINVAL);
+
+	qmp = platform_get_drvdata(pdev);
+	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#6 lore raw
Quoted 48 lines · Manivannan Sadhasivam
Quoted 17 lines · Bjorn Andersson
Quoted 15 lines · Deepak Kumar Singh

Not all upcoming usecases will have an interface to allow the aoss driver to hook onto. Expose the send api and create a get function to enable drivers to send their own messages to aoss.

Signed-off-by Chris Lew <[email protected]>
Signed-off-by Deepak Kumar Singh <[email protected]>
---
 drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)
drivers/soc/qcom/qcom_aoss.c +0 -0
index 53acb94..5c643f0 100644
@@ -8,10 +8,12 @@

[...]

Quoted 6 lines

drivers/soc/qcom/qcom_aoss.c static void qmp_cooling_devices_remove(struct qmp *qmp) · line 543

Quoted 1 line
drivers/soc/qcom/qcom_aoss.c +1 -0
+	pdev = of_find_device_by_node(np);

of_find_device_by_node() will increment the refcount of the underlying struct device of pdev, so you need to platform_device_put() once you're done with it.

Quoted 9 lines

As a side effect of not putting the struct device, the devm_kzalloc'ed qmp pointer will remain valid. So care is needed to make sure that the client doesn't end up with a dangling pointer if the qmp device is removed.

My suggestion is that you add a "qmp_put()" function, which invokes platform_device_put() and that you add some sort of tracking ("bool orphan"?) to the struct qmp and make qmp_send() fail if this is set.

I think this is a duplication of what the struct device offers. Why can't we use the generic infrastructure for this usecase?

Like using device_initialize() in qmp_probe() along with a release callback for "struct device", then using get_device() in qmp_get(). Then there should also be a qmp_put() API which calls put_device() to decrease the refcount.

Ideally, the final refcount should be dropped in qmp_remove() and then the release callback will be called automatically to free "struct qmp".

Quoted 7 lines
Quoted 4 lines

That way if someone unbinds the aoss device, the client will still have a "valid" pointer, but won't be able to qmp_send() after qmp_close() has been called in the aoss remove function.

How can someone remove the qmp device if a client is holding its reference?

The device could be unbound using sysfs, in which case remove() is called and I assumed that devres wouldn't be released until the struct device's refcount hit 0.

Apparently this does not seems to be how it works, following the unbind path I see that devres is shot down regardless of the struct device's refcount.

So we would need to ensure that struct qmp is refcounted on its own. For this we don't need a separate struct device, we can simply add a kref to the struct and avoid using devres to keep track of its lifetime.

Regards,
Bjorn

Quoted 21 lines

Thanks,
Mani

Quoted 18 lines

Regards,
Bjorn

Quoted 15 lines
drivers/soc/qcom/qcom_aoss.c +8 -0
+	if (!pdev)
+		return ERR_PTR(-EINVAL);
+
+	qmp = platform_get_drvdata(pdev);
+	return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#7 lore raw

PATCH 2/2 soc: qcom: aoss: Add debugfs entry

In reply to Deepak Kumar Singh

It can be useful to control the different power states of various parts of hardware for device testing. Add a debugfs node for qmp so messages can be sent to aoss for debugging and testing purposes.

Signed-off-by Chris Lew <[email protected]>
Signed-off-by Deepak Kumar Singh <[email protected]>
---
 drivers/soc/qcom/qcom_aoss.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
drivers/soc/qcom/qcom_aoss.c +41 -0
index 5c643f0..1789880 100644
@@ -4,6 +4,7 @@
  */
 #include <dt-bindings/power/qcom-aoss-qmp.h>
 #include <linux/clk-provider.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/mailbox_client.h>

@@ -86,6 +87,9 @@ struct qmp {
 	struct clk_hw qdss_clk;
 	struct genpd_onecell_data pd_data;
 	struct qmp_cooling_device *cooling_devs;
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	struct dentry *debugfs_file;
+#endif /* CONFIG_DEBUG_FS */
 };
 
 struct qmp_pd {

@@ -549,6 +553,34 @@ struct qmp *qmp_get(struct device *dev)
 }
 EXPORT_SYMBOL(qmp_get);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static ssize_t aoss_dbg_write(struct file *file, const char __user *userstr,
+			      size_t len, loff_t *pos)
+{
+	struct qmp *qmp = file->private_data;
+	char buf[QMP_MSG_LEN] = {};
+	int ret;
+
+	if (!len || len >= QMP_MSG_LEN)
+		return len;
+
+	ret  = copy_from_user(buf, userstr, len);
+	if (ret) {
+		dev_err(qmp->dev, "copy from user failed, ret:%d\n", ret);
+		return len;
+	}
+
+	ret = qmp_send(qmp, buf, QMP_MSG_LEN);
+
+	return ret ? ret : len;
+}
+
+static const struct file_operations aoss_dbg_fops = {
+	.open = simple_open,
+	.write = aoss_dbg_write,
+};
+#endif /* CONFIG_DEBUG_FS */
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;

@@ -603,6 +635,11 @@ static int qmp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, qmp);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	qmp->debugfs_file = debugfs_create_file("aoss_send_message", 0220, NULL,
+						qmp, &aoss_dbg_fops);
+#endif /* CONFIG_DEBUG_FS */
+
 	return 0;
 
 err_remove_qdss_clk:

@@ -619,6 +656,10 @@ static int qmp_remove(struct platform_device *pdev)
 {
 	struct qmp *qmp = platform_get_drvdata(pdev);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	debugfs_remove(qmp->debugfs_file);
+#endif /* CONFIG_DEBUG_FS */
+
 	qmp_qdss_clk_remove(qmp);
 	qmp_pd_remove(qmp);
 	qmp_cooling_devices_remove(qmp);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#8 lore raw

Re: [PATCH V1 2/2] soc: qcom: aoss: Add debugfs entry

Quoted 36 lines · Deepak Kumar Singh

It can be useful to control the different power states of various parts of hardware for device testing. Add a debugfs node for qmp so messages can be sent to aoss for debugging and testing purposes.

Signed-off-by Chris Lew <[email protected]>
Signed-off-by Deepak Kumar Singh <[email protected]>
---
 drivers/soc/qcom/qcom_aoss.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
drivers/soc/qcom/qcom_aoss.c +4 -0
index 5c643f0..1789880 100644
@@ -4,6 +4,7 @@
  */
 #include <dt-bindings/power/qcom-aoss-qmp.h>
 #include <linux/clk-provider.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/mailbox_client.h>

@@ -86,6 +87,9 @@ struct qmp {
 	struct clk_hw qdss_clk;
 	struct genpd_onecell_data pd_data;
 	struct qmp_cooling_device *cooling_devs;
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	struct dentry *debugfs_file;
+#endif /* CONFIG_DEBUG_FS */
 };
 
 struct qmp_pd {

@@ -549,6 +553,34 @@ struct qmp *qmp_get(struct device *dev)
 }
 EXPORT_SYMBOL(qmp_get);

drivers/soc/qcom/qcom_aoss.c struct qmp *qmp_get(struct device *dev) · line 565

Quoted 10 lines
drivers/soc/qcom/qcom_aoss.c +10 -0
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static ssize_t aoss_dbg_write(struct file *file, const char __user *userstr,
+			      size_t len, loff_t *pos)
+{
+	struct qmp *qmp = file->private_data;
+	char buf[QMP_MSG_LEN] = {};
+	int ret;
+
+	if (!len || len >= QMP_MSG_LEN)
+		return len;

If len >= QMP_MSG_LEN we shouldn't lie to the caller and say that all went well, better return -EINVAL in this case.

drivers/soc/qcom/qcom_aoss.c struct qmp *qmp_get(struct device *dev) · line 570

Quoted 5 lines
drivers/soc/qcom/qcom_aoss.c +5 -0
+
+	ret  = copy_from_user(buf, userstr, len);
+	if (ret) {
+		dev_err(qmp->dev, "copy from user failed, ret:%d\n", ret);
+		return len;

		return -EFAULT;

And you don't have to print here.

The rest looks good.

Regards,
Bjorn

Quoted 43 lines
drivers/soc/qcom/qcom_aoss.c +22 -0
+	}
+
+	ret = qmp_send(qmp, buf, QMP_MSG_LEN);
+
+	return ret ? ret : len;
+}
+
+static const struct file_operations aoss_dbg_fops = {
+	.open = simple_open,
+	.write = aoss_dbg_write,
+};
+#endif /* CONFIG_DEBUG_FS */
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct resource *res;

@@ -603,6 +635,11 @@ static int qmp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, qmp);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	qmp->debugfs_file = debugfs_create_file("aoss_send_message", 0220, NULL,
+						qmp, &aoss_dbg_fops);
+#endif /* CONFIG_DEBUG_FS */
+
 	return 0;
 
 err_remove_qdss_clk:

@@ -619,6 +656,10 @@ static int qmp_remove(struct platform_device *pdev)
 {
 	struct qmp *qmp = platform_get_drvdata(pdev);
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+	debugfs_remove(qmp->debugfs_file);
+#endif /* CONFIG_DEBUG_FS */
+
 	qmp_qdss_clk_remove(qmp);
 	qmp_pd_remove(qmp);
 	qmp_cooling_devices_remove(qmp);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#9 lore raw
In reply to Deepak Kumar Singh
Quoted 1 line · Deepak Kumar Singh

[Change from V0]

It's typical that the first patchset, without a version specified, is considered "version 1", and as such the second submission would be "v2".

Quoted 2 lines

Update qmp_get to parse qmp handle with binding qcom,qmp

I won't be able to merge this until we have a user of the API, so would it be possible to get at least one consumer introduced?

Regards,
Bjorn

Quoted 11 lines

Deepak Kumar Singh (2):
soc: qcom: aoss: Expose send for generic usecase
soc: qcom: aoss: Add debugfs entry

 drivers/soc/qcom/qcom_aoss.c | 77 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project