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

From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Deepak Kumar Singh <deesin@codeaurora.org>
Cc: bjorn.andersson@linaro.org, swboyd@chromium.org,
	clew@codeaurora.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
Date: Mon, 11 Oct 2021 12:22:04 -0600	[thread overview]
Message-ID: <20211011182204.GB3817586@p14s> (raw)
In-Reply-To: <1633015924-881-5-git-send-email-deesin@codeaurora.org>

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
> 