On 6/8/22 03:16, Chris Lew wrote:
> In order to reduce the amount of copies in the rpmsg framework, it is
> necessary for clients to take brief ownership of the receive buffer.
>
> Add the capability for clients to notify the rpmsg framework and the
> underlying transports when it is going to hold onto a buffer and also
> notify when the client is done with the buffer.
>
> In the .rx_cb of the rpmsg drivers, if they wish to use the received
> buffer at a later point, they should return RPMSG_DEFER. Otherwise
> returning RPMSG_HANDLED (0) will signal the framework that the client
> is done with the resources and can continue with cleanup.
>
> The clients should check if their rpmsg endpoint supports the rx_done
> operation with the new state variable in the rpmsg_endpoint since not
> all endpoints will have the ability to support this operation.
>
> Signed-off-by: Chris Lew <quic_clew@xxxxxxxxxxx>
> ---
> drivers/rpmsg/rpmsg_core.c | 20 ++++++++++++++++++++
> drivers/rpmsg/rpmsg_internal.h | 1 +
> include/linux/rpmsg.h | 24 ++++++++++++++++++++++++
> 3 files changed, 45 insertions(+)
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 290c1f02da10..359be643060f 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -351,6 +351,26 @@ ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
> }
> EXPORT_SYMBOL(rpmsg_get_mtu);
>
> +/**
> + * rpmsg_rx_done() - release resources related to @data from a @rx_cb
> + * @ept: the rpmsg endpoint
> + * @data: payload from a message
> + *
> + * Returns 0 on success and an appropriate error value on failure.
> + */
> +int rpmsg_rx_done(struct rpmsg_endpoint *ept, void *data)
> +{
> + if (WARN_ON(!ept))
> + return -EINVAL;
> + if (!ept->ops->rx_done)
> + return -ENXIO;
> + if (!ept->rx_done)
> + return -EINVAL;
> +
> + return ept->ops->rx_done(ept, data);
> +}
> +EXPORT_SYMBOL(rpmsg_rx_done);
> +
> /*
> * match a rpmsg channel with a channel info struct.
> * this is used to make sure we're not creating rpmsg devices for channels
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index a22cd4abe7d1..99cb86ce638e 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -76,6 +76,7 @@ struct rpmsg_endpoint_ops {
> __poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
> poll_table *wait);
> ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
> + int (*rx_done)(struct rpmsg_endpoint *ept, void *data);
> };
>
> struct device *rpmsg_find_device(struct device *parent,
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 523c98b96cb4..8e34222e8bca 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -63,6 +63,18 @@ struct rpmsg_device {
> const struct rpmsg_device_ops *ops;
> };
>
> +/**
> + * rpmsg rx callback return definitions
> + * @RPMSG_HANDLED: rpmsg user is done processing data, framework can free the
> + * resources related to the buffer
> + * @RPMSG_DEFER: rpmsg user is not done processing data, framework will hold
> + * onto resources related to the buffer until rpmsg_rx_done is
> + * called. User should check their endpoint to see if rx_done
> + * is a supported operation.
> + */
> +#define RPMSG_HANDLED 0
> +#define RPMSG_DEFER 1
DEFER or HOLD?
In both case, would be nice to update the up-streamed RPMSG service
devices to reflect this update, even if the compatibility is preserved.
> +
> typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>
> /**
> @@ -71,6 +83,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> * @refcount: when this drops to zero, the ept is deallocated
> * @cb: rx callback handler
> * @cb_lock: must be taken before accessing/changing @cb
> + * @rx_done: if set, rpmsg endpoint supports rpmsg_rx_done
Same: done or hold?
Perhaps a bitmap here would be better for future.
I guess that similar feature could be requested for TX...
Regards,
Arnaud
> * @addr: local rpmsg address
> * @priv: private data for the driver's use
> *
> @@ -93,6 +106,7 @@ struct rpmsg_endpoint {
> struct kref refcount;
> rpmsg_rx_cb_t cb;
> struct mutex cb_lock;
> + bool rx_done;
> u32 addr;
> void *priv;
>
> @@ -192,6 +206,8 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>
> ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
>
> +int rpmsg_rx_done(struct rpmsg_endpoint *ept, void *data);
> +
> #else
>
> static inline int rpmsg_register_device_override(struct rpmsg_device *rpdev,
> @@ -316,6 +332,14 @@ static inline ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
> return -ENXIO;
> }
>
> +static inline int rpmsg_rx_done(struct rpmsg_endpoint *ept, void *data)
> +{
> + /* This shouldn't be possible */
> + WARN_ON(1);
> +
> + return -ENXIO;
> +}
> +
> #endif /* IS_ENABLED(CONFIG_RPMSG) */
>
> /* use a macro to avoid include chaining to get THIS_MODULE */
