Re: [PATCH v2 1/6] drivers/can: add multi message support to sendmsg

[Dario Binacchi <dariobin@libero.it>]

Hi Jan,

> Il 01/12/2021 07:15 Jan Kiszka <jan.kiszka@siemens.com> ha scritto:
> 
>  
> On 30.11.21 09:33, Jan Kiszka via Xenomai wrote:
> > On 29.11.21 23:07, Dario Binacchi wrote:
> >> The `user_msghdr' structure is designed to send multiple messages as
> >> well, so rtcan_raw_sendmsg() can also send multiple messages. This
> >> avoids having to add the sendmmsg system call which requires more
> >> extensive Xenomai changes.
> >>
> >> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> >> ---
> >>
> >> (no changes since v1)
> >>
> > 
> > I asked for testing the changes also smokey (via rtcan_virt). How about
> > that?
> > 
> 
> Adding C Smith: There is also the series that fixes compat support for
> RTCAN [1], switching to rtdm helper to access iov structs. Please align
> your activities so that we get the best of all.
> 

Ok, I'll do it

> And I can only stress my point above that we need at least basic testing
> for CAN along this.
> 

I agree with you. I already have a draft that I have to test first and I 
hope to submit it this weekend.

Thanks and regards
Dario

> Thanks,
> Jan
> 
> [1] https://xenomai.org/pipermail/xenomai/2021-November/046722.html
> 
> > Jan
> > 
> >>  kernel/drivers/can/rtcan_raw.c | 239 ++++++++++++++++++---------------
> >>  1 file changed, 128 insertions(+), 111 deletions(-)
> >>
> >> diff --git a/kernel/drivers/can/rtcan_raw.c b/kernel/drivers/can/rtcan_raw.c
> >> index 693b927fe..b17c1709d 100644
> >> --- a/kernel/drivers/can/rtcan_raw.c
> >> +++ b/kernel/drivers/can/rtcan_raw.c
> >> @@ -762,113 +762,13 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_fd *fd,
> >>  }
> >>  
> >>  
> >> -ssize_t rtcan_raw_sendmsg(struct rtdm_fd *fd,
> >> -			  const struct user_msghdr *msg, int flags)
> >> +static ssize_t __rtcan_raw_sendmsg(struct rtcan_device *dev, struct rtcan_socket *sock,
> >> +				   can_frame_t *frame, nanosecs_rel_t timeout)
> >>  {
> >> -    struct rtcan_socket *sock = rtdm_fd_to_private(fd);
> >> -    struct sockaddr_can *scan = (struct sockaddr_can *)msg->msg_name;
> >> -    struct sockaddr_can scan_buf;
> >> -    struct iovec *iov = (struct iovec *)msg->msg_iov;
> >> -    struct iovec iov_buf;
> >> -    can_frame_t *frame;
> >> -    can_frame_t frame_buf;
> >> -    rtdm_lockctx_t lock_ctx;
> >> -    nanosecs_rel_t timeout = 0;
> >>      struct tx_wait_queue tx_wait;
> >> -    struct rtcan_device *dev;
> >> -    int ifindex = 0;
> >> -    int ret  = 0;
> >> +    rtdm_lockctx_t lock_ctx;
> >>      spl_t s;
> >> -
> >> -
> >> -    if (flags & MSG_OOB)   /* Mirror BSD error message compatibility */
> >> -	return -EOPNOTSUPP;
> >> -
> >> -    /* Only MSG_DONTWAIT is a valid flag. */
> >> -    if (flags & ~MSG_DONTWAIT)
> >> -	return -EINVAL;
> >> -
> >> -    /* Check msg_iovlen, only one buffer allowed */
> >> -    if (msg->msg_iovlen != 1)
> >> -	return -EMSGSIZE;
> >> -
> >> -    if (scan == NULL) {
> >> -	/* No socket address. Will use bound interface for sending */
> >> -
> >> -	if (msg->msg_namelen != 0)
> >> -	    return -EINVAL;
> >> -
> >> -
> >> -	/* We only want a consistent value here, a spin lock would be
> >> -	 * overkill. Nevertheless, the binding could change till we have
> >> -	 * the chance to send. Blame the user, though. */
> >> -	ifindex = atomic_read(&sock->ifindex);
> >> -
> >> -	if (!ifindex)
> >> -	    /* Socket isn't bound or bound to all interfaces. Go out. */
> >> -	    return -ENXIO;
> >> -    } else {
> >> -	/* Socket address given */
> >> -	if (msg->msg_namelen < sizeof(struct sockaddr_can))
> >> -	    return -EINVAL;
> >> -
> >> -	if (rtdm_fd_is_user(fd)) {
> >> -	    /* Copy socket address from userspace */
> >> -	    if (!rtdm_read_user_ok(fd, msg->msg_name,
> >> -				   sizeof(struct sockaddr_can)) ||
> >> -		rtdm_copy_from_user(fd, &scan_buf, msg->msg_name,
> >> -				    sizeof(struct sockaddr_can)))
> >> -		return -EFAULT;
> >> -
> >> -	    scan = &scan_buf;
> >> -	}
> >> -
> >> -	/* Check address family */
> >> -	if (scan->can_family != AF_CAN)
> >> -	    return -EINVAL;
> >> -
> >> -	ifindex = scan->can_ifindex;
> >> -    }
> >> -
> >> -    if (rtdm_fd_is_user(fd)) {
> >> -	/* Copy IO vector from userspace */
> >> -	if (!rtdm_rw_user_ok(fd, msg->msg_iov,
> >> -			     sizeof(struct iovec)) ||
> >> -	    rtdm_copy_from_user(fd, &iov_buf, msg->msg_iov,
> >> -				sizeof(struct iovec)))
> >> -	    return -EFAULT;
> >> -
> >> -	iov = &iov_buf;
> >> -    }
> >> -
> >> -    /* Check size of buffer */
> >> -    if (iov->iov_len != sizeof(can_frame_t))
> >> -	return -EMSGSIZE;
> >> -
> >> -    frame = (can_frame_t *)iov->iov_base;
> >> -
> >> -    if (rtdm_fd_is_user(fd)) {
> >> -	/* Copy CAN frame from userspace */
> >> -	if (!rtdm_read_user_ok(fd, iov->iov_base,
> >> -			       sizeof(can_frame_t)) ||
> >> -	    rtdm_copy_from_user(fd, &frame_buf, iov->iov_base,
> >> -				sizeof(can_frame_t)))
> >> -	    return -EFAULT;
> >> -
> >> -	frame = &frame_buf;
> >> -    }
> >> -
> >> -    /* Adjust iovec in the common way */
> >> -    iov->iov_base += sizeof(can_frame_t);
> >> -    iov->iov_len -= sizeof(can_frame_t);
> >> -    /* ... and copy it back to userspace if necessary */
> >> -    if (rtdm_fd_is_user(fd)) {
> >> -	if (rtdm_copy_to_user(fd, msg->msg_iov, iov,
> >> -			      sizeof(struct iovec)))
> >> -	    return -EFAULT;
> >> -    }
> >> -
> >> -    /* At last, we've got the frame ... */
> >> +    int ret = 0;
> >>  
> >>      /* Check if DLC between 0 and 15 */
> >>      if (frame->can_dlc > 15)
> >> @@ -881,11 +781,6 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_fd *fd,
> >>  	    return -EINVAL;
> >>      }
> >>  
> >> -    if ((dev = rtcan_dev_get_by_index(ifindex)) == NULL)
> >> -	return -ENXIO;
> >> -
> >> -    timeout = (flags & MSG_DONTWAIT) ? RTDM_TIMEOUT_NONE : sock->tx_timeout;
> >> -
> >>      tx_wait.rt_task = rtdm_task_current();
> >>  
> >>      /* Register the task at the socket's TX wait queue and decrement
> >> @@ -931,7 +826,6 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_fd *fd,
> >>  
> >>      /* We got access */
> >>  
> >> -
> >>      /* Push message onto stack for loopback when TX done */
> >>      if (rtcan_loopback_enabled(sock))
> >>  	rtcan_tx_push(dev, sock, frame);
> >> @@ -960,10 +854,133 @@ ssize_t rtcan_raw_sendmsg(struct rtdm_fd *fd,
> >>   send_out2:
> >>      rtdm_lock_put_irqrestore(&dev->device_lock, lock_ctx);
> >>   send_out1:
> >> -    rtcan_dev_dereference(dev);
> >>      return ret;
> >>  }
> >>  
> >> +ssize_t rtcan_raw_sendmsg(struct rtdm_fd *fd,
> >> +			  const struct user_msghdr *msg, int flags)
> >> +{
> >> +    struct rtcan_socket *sock = rtdm_fd_to_private(fd);
> >> +    struct sockaddr_can *scan = (struct sockaddr_can *)msg->msg_name;
> >> +    struct sockaddr_can scan_buf;
> >> +    struct iovec *iov = (struct iovec *)msg->msg_iov;
> >> +    struct iovec iov_buf;
> >> +    can_frame_t *frame;
> >> +    can_frame_t frame_buf;
> >> +    nanosecs_rel_t timeout;
> >> +    struct rtcan_device *dev;
> >> +    int ret = 0, ifindex = 0, i = 0, n, sent = 0;
> >> +
> >> +    if (flags & MSG_OOB)   /* Mirror BSD error message compatibility */
> >> +	return -EOPNOTSUPP;
> >> +
> >> +    /* Only MSG_DONTWAIT is a valid flag. */
> >> +    if (flags & ~MSG_DONTWAIT)
> >> +	return -EINVAL;
> >> +
> >> +    timeout = (flags & MSG_DONTWAIT) ? RTDM_TIMEOUT_NONE : sock->tx_timeout;
> >> +
> >> +    if (scan == NULL) {
> >> +	/* No socket address. Will use bound interface for sending */
> >> +	if (msg->msg_namelen != 0)
> >> +	    return -EINVAL;
> >> +
> >> +
> >> +	/* We only want a consistent value here, a spin lock would be
> >> +	 * overkill. Nevertheless, the binding could change till we have
> >> +	 * the chance to send. Blame the user, though. */
> >> +	ifindex = atomic_read(&sock->ifindex);
> >> +	if (!ifindex)
> >> +	    /* Socket isn't bound or bound to all interfaces. Go out. */
> >> +	    return -ENXIO;
> >> +    } else {
> >> +	/* Socket address given */
> >> +	if (msg->msg_namelen < sizeof(struct sockaddr_can))
> >> +	    return -EINVAL;
> >> +
> >> +	if (rtdm_fd_is_user(fd)) {
> >> +	    /* Copy socket address from userspace */
> >> +	    if (!rtdm_read_user_ok(fd, msg->msg_name,
> >> +				   sizeof(struct sockaddr_can)) ||
> >> +		rtdm_copy_from_user(fd, &scan_buf, msg->msg_name,
> >> +				    sizeof(struct sockaddr_can)))
> >> +		return -EFAULT;
> >> +
> >> +	    scan = &scan_buf;
> >> +	}
> >> +
> >> +	/* Check address family */
> >> +	if (scan->can_family != AF_CAN)
> >> +	    return -EINVAL;
> >> +
> >> +	ifindex = scan->can_ifindex;
> >> +    }
> >> +
> >> +    dev = rtcan_dev_get_by_index(ifindex);
> >> +    if (!dev)
> >> +	return -ENXIO;
> >> +
> >> +    if (rtdm_fd_is_user(fd)) {
> >> +
> >> +	/* Copy IO vector from userspace */
> >> +	if (!rtdm_rw_user_ok(fd, msg->msg_iov,
> >> +			     sizeof(struct iovec)) ||
> >> +	    rtdm_copy_from_user(fd, &iov_buf, msg->msg_iov,
> >> +				sizeof(struct iovec))) {
> >> +	    ret = -EFAULT;
> >> +	    goto finally;
> >> +	}
> >> +
> >> +	iov = &iov_buf;
> >> +    }
> >> +
> >> +    n = msg->msg_iovlen;
> >> +    while (i < n) {
> >> +	if (iov->iov_len < sizeof(can_frame_t)) {
> >> +	    ret = -EMSGSIZE;
> >> +	    goto finally;
> >> +	}
> >> +
> >> +	frame = (can_frame_t *)iov->iov_base;
> >> +
> >> +	if (rtdm_fd_is_user(fd)) {
> >> +	    /* Copy CAN frame from userspace */
> >> +	    if (!rtdm_read_user_ok(fd, iov->iov_base, sizeof(can_frame_t)) ||
> >> +		rtdm_copy_from_user(fd, &frame_buf, iov->iov_base,
> >> +				    sizeof(can_frame_t))) {
> >> +	        ret = -EFAULT;
> >> +	        goto finally;
> >> +	    }
> >> +
> >> +	    frame = &frame_buf;
> >> +	}
> >> +
> >> +	iov->iov_base += sizeof(can_frame_t);
> >> +	iov->iov_len -= sizeof(can_frame_t);
> >> +
> >> +	ret = __rtcan_raw_sendmsg(dev, sock, frame, timeout);
> >> +	if (ret < 0)
> >> +	    goto finally;
> >> +
> >> +	sent += ret;
> >> +    }
> >> +
> >> +    /* copy it back to userspace if necessary */
> >> +    if (rtdm_fd_is_user(fd)) {
> >> +	if (rtdm_copy_to_user(fd, msg->msg_iov, iov, sizeof(struct iovec))) {
> >> +	    ret = -EFAULT;
> >> +	    goto finally;
> >> +	}
> >> +    }
> >> +
> >> +finally:
> >> +    rtcan_dev_dereference(dev);
> >> +
> >> +    if (sent > 0)
> >> +	    return sent;
> >> +
> >> +    return ret;
> >> +}
> >>  
> >>  static struct rtdm_driver rtcan_driver = {
> >>  	.profile_info		= RTDM_PROFILE_INFO(rtcan,
> >>
> > 
> 
> -- 
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux