[PATCH V1 0/2] soc: qcom: aoss: Expose send for generic usecase

[PATCH V1 0/2] soc: qcom: aoss: Expose send for generic usecase

[Deepak Kumar Singh]

[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(-)

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

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

[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 <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 53acb94..5c643f0 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -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;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[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 <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/soc/qcom/qcom_aoss.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 5c643f0..1789880 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 1/2] soc: qcom: aoss: Expose send for generic usecase

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

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 <lkp@intel.com>

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@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39060 bytes --]

Re: [PATCH V1 1/2] soc: qcom: aoss: Expose send for generic usecase

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

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 <lkp@intel.com>

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@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67493 bytes --]

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

[Bjorn Andersson]

On Fri 02 Apr 01:17 CDT 2021, Deepak Kumar Singh wrote:

> 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 <clew@codeaurora.org>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/soc/qcom/qcom_aoss.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 5c643f0..1789880 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -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;

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

> +
> +	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

> +	}
> +
> +	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);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH V1 1/2] soc: qcom: aoss: Expose send for generic usecase

[Bjorn Andersson]

On Fri 02 Apr 01:17 CDT 2021, Deepak Kumar Singh wrote:

> 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 <clew@codeaurora.org>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 53acb94..5c643f0 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -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.

>  
>  #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)

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.

> +		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);

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

> +
> +	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

> +	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;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH V1 0/2] soc: qcom: aoss: Expose send for generic usecase

[Bjorn Andersson]

On Fri 02 Apr 01:17 CDT 2021, Deepak Kumar Singh wrote:

> [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".

> 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

> 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(-)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH V1 1/2] soc: qcom: aoss: Expose send for generic usecase

[Manivannan Sadhasivam]

On Sun, Apr 04, 2021 at 12:17:52PM -0500, Bjorn Andersson wrote:
> On Fri 02 Apr 01:17 CDT 2021, Deepak Kumar Singh wrote:
> 
> > 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 <clew@codeaurora.org>
> > Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> > ---
> >  drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> > index 53acb94..5c643f0 100644
> > --- a/drivers/soc/qcom/qcom_aoss.c
> > +++ b/drivers/soc/qcom/qcom_aoss.c
> > @@ -8,10 +8,12 @@

[...]

> > +	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.
> 

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".

> 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

> Regards,
> Bjorn
> 
> > +	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;
> > -- 
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> > 

Re: [PATCH V1 1/2] soc: qcom: aoss: Expose send for generic usecase

[Bjorn Andersson]

On Fri 09 Apr 02:31 CDT 2021, Manivannan Sadhasivam wrote:

> On Sun, Apr 04, 2021 at 12:17:52PM -0500, Bjorn Andersson wrote:
> > On Fri 02 Apr 01:17 CDT 2021, Deepak Kumar Singh wrote:
> > 
> > > 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 <clew@codeaurora.org>
> > > Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> > > ---
> > >  drivers/soc/qcom/qcom_aoss.c | 36 +++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 35 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> > > index 53acb94..5c643f0 100644
> > > --- a/drivers/soc/qcom/qcom_aoss.c
> > > +++ b/drivers/soc/qcom/qcom_aoss.c
> > > @@ -8,10 +8,12 @@
> 
> [...]
> 
> > > +	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.
> > 
> 
> 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".
> 
> > 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

> Thanks,
> Mani
> 
> > Regards,
> > Bjorn
> > 
> > > +	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;
> > > -- 
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > >