Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[kernel test robot]

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

Hi Deepak,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.15]
[cannot apply to next-20211108]
[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/rpmsg-core-Add-signal-API-support/20211001-003225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
config: x86_64-randconfig-c007-20211001 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4)
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/706239528659b90d79d28e52fcd0538747e66a86
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review Deepak-Kumar-Singh/rpmsg-core-Add-signal-API-support/20211001-003225
         git checkout 706239528659b90d79d28e52fcd0538747e66a86
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)

 >> drivers/rpmsg/rpmsg_char.c:310:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
                    ret = put_user(eptdev->rsigs, (int __user *)arg);
                    ^
    drivers/rpmsg/rpmsg_char.c:317:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
                    ret = rpmsg_set_flow_control(eptdev->ept, set);
                    ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/rpmsg/rpmsg_char.c:320:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
                    ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
                    ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/rpmsg/rpmsg_char.c:323:3: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
                    ret = -EINVAL;
                    ^     ~~~~~~~


vim +/ret +310 drivers/rpmsg/rpmsg_char.c

c0cdc19f84a4712 Bjorn Andersson    2017-01-11  295
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  296  static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  297  			       unsigned long arg)
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  298  {
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  299  	struct rpmsg_eptdev *eptdev = fp->private_data;
706239528659b90 Deepak Kumar Singh 2021-09-30  300  	bool set;
706239528659b90 Deepak Kumar Singh 2021-09-30  301  	u32 val;
706239528659b90 Deepak Kumar Singh 2021-09-30  302  	int ret;
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  303
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  304  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  305  		return -EINVAL;
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  306
706239528659b90 Deepak Kumar Singh 2021-09-30  307  	switch (cmd) {
706239528659b90 Deepak Kumar Singh 2021-09-30  308  	case TIOCMGET:
706239528659b90 Deepak Kumar Singh 2021-09-30  309  		eptdev->sig_pending = false;
706239528659b90 Deepak Kumar Singh 2021-09-30 @310  		ret = put_user(eptdev->rsigs, (int __user *)arg);
706239528659b90 Deepak Kumar Singh 2021-09-30  311  		break;
706239528659b90 Deepak Kumar Singh 2021-09-30  312  	case TIOCMSET:
706239528659b90 Deepak Kumar Singh 2021-09-30  313  		ret = get_user(val, (int __user *)arg);
706239528659b90 Deepak Kumar Singh 2021-09-30  314  		if (ret)
706239528659b90 Deepak Kumar Singh 2021-09-30  315  			break;
706239528659b90 Deepak Kumar Singh 2021-09-30  316  		set = (val & TIOCM_DTR) ? true : false;
706239528659b90 Deepak Kumar Singh 2021-09-30 @317  		ret = rpmsg_set_flow_control(eptdev->ept, set);
706239528659b90 Deepak Kumar Singh 2021-09-30  318  		break;
706239528659b90 Deepak Kumar Singh 2021-09-30  319  	case RPMSG_DESTROY_EPT_IOCTL:
706239528659b90 Deepak Kumar Singh 2021-09-30 @320  		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
706239528659b90 Deepak Kumar Singh 2021-09-30  321  		break;
706239528659b90 Deepak Kumar Singh 2021-09-30  322  	default:
706239528659b90 Deepak Kumar Singh 2021-09-30 @323  		ret = -EINVAL;
706239528659b90 Deepak Kumar Singh 2021-09-30  324  	}
706239528659b90 Deepak Kumar Singh 2021-09-30  325
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  326  	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  327  }
c0cdc19f84a4712 Bjorn Andersson    2017-01-11  328

---
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: 37761 bytes --]

Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Mathieu Poirier]

On Thu, Sep 30, 2021 at 09:02:04PM +0530, Deepak Kumar Singh wrote:
> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
> to get/set the low level transport signals.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/rpmsg/rpmsg_char.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 

I would have expected this patch to be 02 rather than 03.  That way the
framework is sent in place and then used by platform code.

> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 2bebc9b..60a889b 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -19,6 +19,7 @@
>  #include <linux/rpmsg.h>
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
> +#include <linux/termios.h>
>  #include <linux/uaccess.h>
>  #include <uapi/linux/rpmsg.h>
>  
> @@ -76,6 +77,9 @@ struct rpmsg_eptdev {
>  	spinlock_t queue_lock;
>  	struct sk_buff_head queue;
>  	wait_queue_head_t readq;
> +
> +	u32 rsigs;
> +	bool sig_pending;
>  };
>  
>  static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> @@ -120,6 +124,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>  	return 0;
>  }
>  
> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
> +{
> +	struct rpmsg_eptdev *eptdev = priv;
> +
> +	eptdev->rsigs = sigs;
> +	eptdev->sig_pending = true;

If two signals are sent in a row without user space having the time to process
the first one, the second message will overwrite the first one.

> +
> +	/* wake up any blocking processes, waiting for signal notification */
> +	wake_up_interruptible(&eptdev->readq);
> +	return 0;
> +}
> +
>  static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  {
>  	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
> @@ -139,6 +155,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  		return -EINVAL;
>  	}
>  
> +	ept->sig_cb = rpmsg_sigs_cb;
>  	eptdev->ept = ept;
>  	filp->private_data = eptdev;
>  
> @@ -157,6 +174,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>  		eptdev->ept = NULL;
>  	}
>  	mutex_unlock(&eptdev->ept_lock);
> +	eptdev->sig_pending = false;
>  
>  	/* Discard all SKBs */
>  	skb_queue_purge(&eptdev->queue);
> @@ -267,6 +285,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>  	if (!skb_queue_empty(&eptdev->queue))
>  		mask |= EPOLLIN | EPOLLRDNORM;
>  
> +	if (eptdev->sig_pending)
> +		mask |= EPOLLPRI;
> +
>  	mask |= rpmsg_poll(eptdev->ept, filp, wait);
>  
>  	return mask;
> @@ -276,10 +297,32 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>  			       unsigned long arg)
>  {
>  	struct rpmsg_eptdev *eptdev = fp->private_data;
> +	bool set;
> +	u32 val;
> +	int ret;
>  
>  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>  		return -EINVAL;
>  
> +	switch (cmd) {
> +	case TIOCMGET:

The IOCTLs should be generic so that any kind of out-of-band signals.

> +		eptdev->sig_pending = false;
> +		ret = put_user(eptdev->rsigs, (int __user *)arg);
> +		break;
> +	case TIOCMSET:
> +		ret = get_user(val, (int __user *)arg);
> +		if (ret)
> +			break;
> +		set = (val & TIOCM_DTR) ? true : false;
> +		ret = rpmsg_set_flow_control(eptdev->ept, set);
> +		break;

But as said in patch 01, I'm not sure about the path from AP to RP.  Function
rpmsg_set_flow_control() turns into qcom_glink_tx().  In this case it should be
possible for user space to send this information as it does with other kind of
data destined for the remote processor.  At the very least the send interface
should be decoupled from the implementation specific nature of this use case.

Lastly, Arnaud has sent patches that refactor rpmsg_eptdev_ioctl().  I would
like that patchset to be dealth with before we move forward with this one.  That
way we make sure to avoid supporting features that are incompatible with each
other.

Regards,
Mathieu

> +	case RPMSG_DESTROY_EPT_IOCTL:
> +		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
>  	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>  }
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-09-30 08:32:04)
> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
> to get/set the low level transport signals.
>
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Is Chris the author? Because if so then there should be a From: Chris
line before the commit text starts.

> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/rpmsg/rpmsg_char.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 2bebc9b..60a889b 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -19,6 +19,7 @@
>  #include <linux/rpmsg.h>
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
> +#include <linux/termios.h>
>  #include <linux/uaccess.h>
>  #include <uapi/linux/rpmsg.h>
>
> @@ -76,6 +77,9 @@ struct rpmsg_eptdev {
>         spinlock_t queue_lock;
>         struct sk_buff_head queue;
>         wait_queue_head_t readq;
> +
> +       u32 rsigs;
> +       bool sig_pending;
>  };
>
>  static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> @@ -120,6 +124,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>         return 0;
>  }
>
> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
> +{
> +       struct rpmsg_eptdev *eptdev = priv;
> +
> +       eptdev->rsigs = sigs;
> +       eptdev->sig_pending = true;
> +
> +       /* wake up any blocking processes, waiting for signal notification */

Comment would be better if it indicated what function these waiters are
in instead of saying what wake_up_interruptible() does. Also, what is
interruptible for?

> +       wake_up_interruptible(&eptdev->readq);

Nitpick: Add newline here.

> +       return 0;
> +}
> +
>  static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  {
>         struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
> @@ -276,10 +297,32 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>                                unsigned long arg)
>  {
>         struct rpmsg_eptdev *eptdev = fp->private_data;
> +       bool set;
> +       u32 val;
> +       int ret;
>
>         if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>                 return -EINVAL;

This should be deleted?

>
> +       switch (cmd) {
> +       case TIOCMGET:
> +               eptdev->sig_pending = false;
> +               ret = put_user(eptdev->rsigs, (int __user *)arg);
> +               break;
> +       case TIOCMSET:
> +               ret = get_user(val, (int __user *)arg);
> +               if (ret)
> +                       break;
> +               set = (val & TIOCM_DTR) ? true : false;
> +               ret = rpmsg_set_flow_control(eptdev->ept, set);
> +               break;
> +       case RPMSG_DESTROY_EPT_IOCTL:
> +               ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +       }
> +
>         return rpmsg_eptdev_destroy(&eptdev->dev, NULL);

This should be replaced with return ret?

[PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Deepak Kumar Singh]

Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
to get/set the low level transport signals.

Signed-off-by: Chris Lew <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/rpmsg/rpmsg_char.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 2bebc9b..60a889b 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -19,6 +19,7 @@
 #include <linux/rpmsg.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
+#include <linux/termios.h>
 #include <linux/uaccess.h>
 #include <uapi/linux/rpmsg.h>
 
@@ -76,6 +77,9 @@ struct rpmsg_eptdev {
 	spinlock_t queue_lock;
 	struct sk_buff_head queue;
 	wait_queue_head_t readq;
+
+	u32 rsigs;
+	bool sig_pending;
 };
 
 static int rpmsg_eptdev_destroy(struct device *dev, void *data)
@@ -120,6 +124,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
 	return 0;
 }
 
+static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
+{
+	struct rpmsg_eptdev *eptdev = priv;
+
+	eptdev->rsigs = sigs;
+	eptdev->sig_pending = true;
+
+	/* wake up any blocking processes, waiting for signal notification */
+	wake_up_interruptible(&eptdev->readq);
+	return 0;
+}
+
 static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 {
 	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
@@ -139,6 +155,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 		return -EINVAL;
 	}
 
+	ept->sig_cb = rpmsg_sigs_cb;
 	eptdev->ept = ept;
 	filp->private_data = eptdev;
 
@@ -157,6 +174,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
 		eptdev->ept = NULL;
 	}
 	mutex_unlock(&eptdev->ept_lock);
+	eptdev->sig_pending = false;
 
 	/* Discard all SKBs */
 	skb_queue_purge(&eptdev->queue);
@@ -267,6 +285,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
 	if (!skb_queue_empty(&eptdev->queue))
 		mask |= EPOLLIN | EPOLLRDNORM;
 
+	if (eptdev->sig_pending)
+		mask |= EPOLLPRI;
+
 	mask |= rpmsg_poll(eptdev->ept, filp, wait);
 
 	return mask;
@@ -276,10 +297,32 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
 			       unsigned long arg)
 {
 	struct rpmsg_eptdev *eptdev = fp->private_data;
+	bool set;
+	u32 val;
+	int ret;
 
 	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
 		return -EINVAL;
 
+	switch (cmd) {
+	case TIOCMGET:
+		eptdev->sig_pending = false;
+		ret = put_user(eptdev->rsigs, (int __user *)arg);
+		break;
+	case TIOCMSET:
+		ret = get_user(val, (int __user *)arg);
+		if (ret)
+			break;
+		set = (val & TIOCM_DTR) ? true : false;
+		ret = rpmsg_set_flow_control(eptdev->ept, set);
+		break;
+	case RPMSG_DESTROY_EPT_IOCTL:
+		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
 	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project