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 <[email protected]>
---
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
+
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
* @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 */
--
2.7.4
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 <[email protected]>
> ---
> 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 */
On Tue, Jun 07, 2022 at 06:16:42PM -0700, 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 <[email protected]>
> ---
> 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
> +
> 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
> * @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;
Do you see a scenario where rpmsg_endpoint_ops::rx_done holds a valid pointer
but rpmsg_epndpoint::rx_done is set to false? If not please remove.
> 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 */
> --
> 2.7.4
>