Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
so that they can be used by other entities.
Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
include/linux/rpmsg_ns.h | 62 ++++++++++++++++++++++++++++++++
include/uapi/linux/rpmsg.h | 3 ++
3 files changed, 67 insertions(+), 56 deletions(-)
create mode 100644 include/linux/rpmsg_ns.h
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 793fe924671f..85f2acc4ed9f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -19,7 +19,7 @@
#include <linux/mutex.h>
#include <linux/of_device.h>
#include <linux/rpmsg.h>
-#include <linux/rpmsg_byteorder.h>
+#include <linux/rpmsg_ns.h>
#include <linux/scatterlist.h>
#include <linux/slab.h>
#include <linux/sched.h>
@@ -27,6 +27,7 @@
#include <linux/virtio_ids.h>
#include <linux/virtio_config.h>
#include <linux/wait.h>
+#include <uapi/linux/rpmsg.h>
#include "rpmsg_internal.h"
@@ -70,58 +71,6 @@ struct virtproc_info {
struct rpmsg_endpoint *ns_ept;
};
-/* The feature bitmap for virtio rpmsg */
-#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
-
-/**
- * struct rpmsg_hdr - common header for all rpmsg messages
- * @src: source address
- * @dst: destination address
- * @reserved: reserved for future use
- * @len: length of payload (in bytes)
- * @flags: message flags
- * @data: @len bytes of message payload data
- *
- * Every message sent(/received) on the rpmsg bus begins with this header.
- */
-struct rpmsg_hdr {
- __rpmsg32 src;
- __rpmsg32 dst;
- __rpmsg32 reserved;
- __rpmsg16 len;
- __rpmsg16 flags;
- u8 data[];
-} __packed;
-
-/**
- * struct rpmsg_ns_msg - dynamic name service announcement message
- * @name: name of remote service that is published
- * @addr: address of remote service that is published
- * @flags: indicates whether service is created or destroyed
- *
- * This message is sent across to publish a new service, or announce
- * about its removal. When we receive these messages, an appropriate
- * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
- * or ->remove() handler of the appropriate rpmsg driver will be invoked
- * (if/as-soon-as one is registered).
- */
-struct rpmsg_ns_msg {
- char name[RPMSG_NAME_SIZE];
- __rpmsg32 addr;
- __rpmsg32 flags;
-} __packed;
-
-/**
- * enum rpmsg_ns_flags - dynamic name service announcement flags
- *
- * @RPMSG_NS_CREATE: a new remote service was just created
- * @RPMSG_NS_DESTROY: a known remote service was just destroyed
- */
-enum rpmsg_ns_flags {
- RPMSG_NS_CREATE = 0,
- RPMSG_NS_DESTROY = 1,
-};
-
/**
* @vrp: the remote processor this channel belongs to
*/
@@ -162,9 +111,6 @@ struct virtio_rpmsg_channel {
*/
#define RPMSG_RESERVED_ADDRESSES (1024)
-/* Address 53 is reserved for advertising remote services */
-#define RPMSG_NS_ADDR (53)
-
static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept);
static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
new file mode 100644
index 000000000000..3d836b8580b2
--- /dev/null
+++ b/include/linux/rpmsg_ns.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_RPMSG_NS_H
+#define _LINUX_RPMSG_NS_H
+
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+#include <linux/rpmsg_byteorder.h>
+
+/**
+ * struct rpmsg_hdr - common header for all rpmsg messages
+ * @src: source address
+ * @dst: destination address
+ * @reserved: reserved for future use
+ * @len: length of payload (in bytes)
+ * @flags: message flags
+ * @data: @len bytes of message payload data
+ *
+ * Every message sent(/received) on the rpmsg bus begins with this header.
+ */
+struct rpmsg_hdr {
+ __rpmsg32 src;
+ __rpmsg32 dst;
+ __rpmsg32 reserved;
+ __rpmsg16 len;
+ __rpmsg16 flags;
+ u8 data[];
+} __packed;
+
+/**
+ * struct rpmsg_ns_msg - dynamic name service announcement message
+ * @name: name of remote service that is published
+ * @addr: address of remote service that is published
+ * @flags: indicates whether service is created or destroyed
+ *
+ * This message is sent across to publish a new service, or announce
+ * about its removal. When we receive these messages, an appropriate
+ * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
+ * or ->remove() handler of the appropriate rpmsg driver will be invoked
+ * (if/as-soon-as one is registered).
+ */
+struct rpmsg_ns_msg {
+ char name[RPMSG_NAME_SIZE];
+ __rpmsg32 addr;
+ __rpmsg32 flags;
+} __packed;
+
+/**
+ * enum rpmsg_ns_flags - dynamic name service announcement flags
+ *
+ * @RPMSG_NS_CREATE: a new remote service was just created
+ * @RPMSG_NS_DESTROY: a known remote service was just destroyed
+ */
+enum rpmsg_ns_flags {
+ RPMSG_NS_CREATE = 0,
+ RPMSG_NS_DESTROY = 1,
+};
+
+/* Address 53 is reserved for advertising remote services */
+#define RPMSG_NS_ADDR (53)
+
+#endif
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6dab4223..d669c04ef289 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
#define RPMSG_CREATE_EPT_IOCTL _IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
#define RPMSG_DESTROY_EPT_IOCTL _IO(0xb5, 0x2)
+/* The feature bitmap for virtio rpmsg */
+#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
+
#endif
--
2.25.1
Hi Mathieu,
On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
> so that they can be used by other entities.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
> include/linux/rpmsg_ns.h | 62 ++++++++++++++++++++++++++++++++
> include/uapi/linux/rpmsg.h | 3 ++
> 3 files changed, 67 insertions(+), 56 deletions(-)
> create mode 100644 include/linux/rpmsg_ns.h
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 793fe924671f..85f2acc4ed9f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,7 +19,7 @@
> #include <linux/mutex.h>
> #include <linux/of_device.h>
> #include <linux/rpmsg.h>
> -#include <linux/rpmsg_byteorder.h>
> +#include <linux/rpmsg_ns.h>
> #include <linux/scatterlist.h>
> #include <linux/slab.h>
> #include <linux/sched.h>
> @@ -27,6 +27,7 @@
> #include <linux/virtio_ids.h>
> #include <linux/virtio_config.h>
> #include <linux/wait.h>
> +#include <uapi/linux/rpmsg.h>
>
> #include "rpmsg_internal.h"
>
> @@ -70,58 +71,6 @@ struct virtproc_info {
> struct rpmsg_endpoint *ns_ept;
> };
>
> -/* The feature bitmap for virtio rpmsg */
> -#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> -
> -/**
> - * struct rpmsg_hdr - common header for all rpmsg messages
> - * @src: source address
> - * @dst: destination address
> - * @reserved: reserved for future use
> - * @len: length of payload (in bytes)
> - * @flags: message flags
> - * @data: @len bytes of message payload data
> - *
> - * Every message sent(/received) on the rpmsg bus begins with this header.
> - */
> -struct rpmsg_hdr {
> - __rpmsg32 src;
> - __rpmsg32 dst;
> - __rpmsg32 reserved;
> - __rpmsg16 len;
> - __rpmsg16 flags;
> - u8 data[];
> -} __packed;
> -
> -/**
> - * struct rpmsg_ns_msg - dynamic name service announcement message
> - * @name: name of remote service that is published
> - * @addr: address of remote service that is published
> - * @flags: indicates whether service is created or destroyed
> - *
> - * This message is sent across to publish a new service, or announce
> - * about its removal. When we receive these messages, an appropriate
> - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> - * or ->remove() handler of the appropriate rpmsg driver will be invoked
> - * (if/as-soon-as one is registered).
> - */
> -struct rpmsg_ns_msg {
> - char name[RPMSG_NAME_SIZE];
> - __rpmsg32 addr;
> - __rpmsg32 flags;
> -} __packed;
> -
> -/**
> - * enum rpmsg_ns_flags - dynamic name service announcement flags
> - *
> - * @RPMSG_NS_CREATE: a new remote service was just created
> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> - */
> -enum rpmsg_ns_flags {
> - RPMSG_NS_CREATE = 0,
> - RPMSG_NS_DESTROY = 1,
> -};
> -
> /**
> * @vrp: the remote processor this channel belongs to
> */
> @@ -162,9 +111,6 @@ struct virtio_rpmsg_channel {
> */
> #define RPMSG_RESERVED_ADDRESSES (1024)
>
> -/* Address 53 is reserved for advertising remote services */
> -#define RPMSG_NS_ADDR (53)
> -
> static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept);
> static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
> static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
> new file mode 100644
> index 000000000000..3d836b8580b2
> --- /dev/null
> +++ b/include/linux/rpmsg_ns.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_RPMSG_NS_H
> +#define _LINUX_RPMSG_NS_H
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +#include <linux/rpmsg_byteorder.h>
> +
> +/**
> + * struct rpmsg_hdr - common header for all rpmsg messages
> + * @src: source address
> + * @dst: destination address
> + * @reserved: reserved for future use
> + * @len: length of payload (in bytes)
> + * @flags: message flags
> + * @data: @len bytes of message payload data
> + *
> + * Every message sent(/received) on the rpmsg bus begins with this header.
> + */
> +struct rpmsg_hdr {
> + __rpmsg32 src;
> + __rpmsg32 dst;
> + __rpmsg32 reserved;
> + __rpmsg16 len;
> + __rpmsg16 flags;
> + u8 data[];
> +} __packed;
This structure is not related to the rpmsg ns service but to the rpmsg bus.
If this structure has to be exposed to rpmsg client should be in rpmsg.h, but
Is there a need to expose it for now?
I suppose that it is for vhost...As the need will depends on the implementation,
I would suggest leaving it internally and expose only if needed, in the
related series.
> +
> +/**
> + * struct rpmsg_ns_msg - dynamic name service announcement message
> + * @name: name of remote service that is published
> + * @addr: address of remote service that is published
> + * @flags: indicates whether service is created or destroyed
> + *
> + * This message is sent across to publish a new service, or announce
> + * about its removal. When we receive these messages, an appropriate
> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> + * (if/as-soon-as one is registered).
> + */
> +struct rpmsg_ns_msg {
> + char name[RPMSG_NAME_SIZE];
> + __rpmsg32 addr;
> + __rpmsg32 flags;
> +} __packed;
> +
> +/**
> + * enum rpmsg_ns_flags - dynamic name service announcement flags
> + *
> + * @RPMSG_NS_CREATE: a new remote service was just created
> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> + */
> +enum rpmsg_ns_flags {
> + RPMSG_NS_CREATE = 0,
> + RPMSG_NS_DESTROY = 1,
> +};
> +
> +/* Address 53 is reserved for advertising remote services */
> +#define RPMSG_NS_ADDR (53)
What about my proposal [1] to put this in rpmsg.h, to create a list of
reserved Address
[1] https://lkml.org/lkml/2020/7/31/442
> +
> +#endif
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index e14c6dab4223..d669c04ef289 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
> #define RPMSG_CREATE_EPT_IOCTL _IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
> #define RPMSG_DESTROY_EPT_IOCTL _IO(0xb5, 0x2)
>
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> +
Same suggestion here,i would drop this from this series
Thanks,
Arnaud
> #endif
>
On Thu, Oct 15, 2020 at 10:33:25AM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
>
> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> > Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
> > so that they can be used by other entities.
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
> > include/linux/rpmsg_ns.h | 62 ++++++++++++++++++++++++++++++++
> > include/uapi/linux/rpmsg.h | 3 ++
> > 3 files changed, 67 insertions(+), 56 deletions(-)
> > create mode 100644 include/linux/rpmsg_ns.h
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 793fe924671f..85f2acc4ed9f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,7 +19,7 @@
> > #include <linux/mutex.h>
> > #include <linux/of_device.h>
> > #include <linux/rpmsg.h>
> > -#include <linux/rpmsg_byteorder.h>
> > +#include <linux/rpmsg_ns.h>
> > #include <linux/scatterlist.h>
> > #include <linux/slab.h>
> > #include <linux/sched.h>
> > @@ -27,6 +27,7 @@
> > #include <linux/virtio_ids.h>
> > #include <linux/virtio_config.h>
> > #include <linux/wait.h>
> > +#include <uapi/linux/rpmsg.h>
> >
> > #include "rpmsg_internal.h"
> >
> > @@ -70,58 +71,6 @@ struct virtproc_info {
> > struct rpmsg_endpoint *ns_ept;
> > };
> >
> > -/* The feature bitmap for virtio rpmsg */
> > -#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> > -
> > -/**
> > - * struct rpmsg_hdr - common header for all rpmsg messages
> > - * @src: source address
> > - * @dst: destination address
> > - * @reserved: reserved for future use
> > - * @len: length of payload (in bytes)
> > - * @flags: message flags
> > - * @data: @len bytes of message payload data
> > - *
> > - * Every message sent(/received) on the rpmsg bus begins with this header.
> > - */
> > -struct rpmsg_hdr {
> > - __rpmsg32 src;
> > - __rpmsg32 dst;
> > - __rpmsg32 reserved;
> > - __rpmsg16 len;
> > - __rpmsg16 flags;
> > - u8 data[];
> > -} __packed;
> > -
> > -/**
> > - * struct rpmsg_ns_msg - dynamic name service announcement message
> > - * @name: name of remote service that is published
> > - * @addr: address of remote service that is published
> > - * @flags: indicates whether service is created or destroyed
> > - *
> > - * This message is sent across to publish a new service, or announce
> > - * about its removal. When we receive these messages, an appropriate
> > - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> > - * or ->remove() handler of the appropriate rpmsg driver will be invoked
> > - * (if/as-soon-as one is registered).
> > - */
> > -struct rpmsg_ns_msg {
> > - char name[RPMSG_NAME_SIZE];
> > - __rpmsg32 addr;
> > - __rpmsg32 flags;
> > -} __packed;
> > -
> > -/**
> > - * enum rpmsg_ns_flags - dynamic name service announcement flags
> > - *
> > - * @RPMSG_NS_CREATE: a new remote service was just created
> > - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> > - */
> > -enum rpmsg_ns_flags {
> > - RPMSG_NS_CREATE = 0,
> > - RPMSG_NS_DESTROY = 1,
> > -};
> > -
> > /**
> > * @vrp: the remote processor this channel belongs to
> > */
> > @@ -162,9 +111,6 @@ struct virtio_rpmsg_channel {
> > */
> > #define RPMSG_RESERVED_ADDRESSES (1024)
> >
> > -/* Address 53 is reserved for advertising remote services */
> > -#define RPMSG_NS_ADDR (53)
> > -
> > static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept);
> > static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
> > static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
> > diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
> > new file mode 100644
> > index 000000000000..3d836b8580b2
> > --- /dev/null
> > +++ b/include/linux/rpmsg_ns.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_RPMSG_NS_H
> > +#define _LINUX_RPMSG_NS_H
> > +
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/types.h>
> > +#include <linux/rpmsg_byteorder.h>
> > +
> > +/**
> > + * struct rpmsg_hdr - common header for all rpmsg messages
> > + * @src: source address
> > + * @dst: destination address
> > + * @reserved: reserved for future use
> > + * @len: length of payload (in bytes)
> > + * @flags: message flags
> > + * @data: @len bytes of message payload data
> > + *
> > + * Every message sent(/received) on the rpmsg bus begins with this header.
> > + */
> > +struct rpmsg_hdr {
> > + __rpmsg32 src;
> > + __rpmsg32 dst;
> > + __rpmsg32 reserved;
> > + __rpmsg16 len;
> > + __rpmsg16 flags;
> > + u8 data[];
> > +} __packed;
>
> This structure is not related to the rpmsg ns service but to the rpmsg bus.
> If this structure has to be exposed to rpmsg client should be in rpmsg.h, but
> Is there a need to expose it for now?
> I suppose that it is for vhost...As the need will depends on the implementation,
> I would suggest leaving it internally and expose only if needed, in the
> related series.
>
I also thought about moving rpmsg_hdr to rpmsg.h but decided against because in
most cases using the name space service usually means that a message header will
be required. I also thought it would be easier to use, i.e include one header
rather than two. That too is a little thin because anyone using a name service
will also need to get access to rpmsg_device, which is in rpmsg.h.
I'm definitely not strongly opinionated on where it should go, or I can leave it
in virtio_rpmsg_bus.c too...
> > +
> > +/**
> > + * struct rpmsg_ns_msg - dynamic name service announcement message
> > + * @name: name of remote service that is published
> > + * @addr: address of remote service that is published
> > + * @flags: indicates whether service is created or destroyed
> > + *
> > + * This message is sent across to publish a new service, or announce
> > + * about its removal. When we receive these messages, an appropriate
> > + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
> > + * or ->remove() handler of the appropriate rpmsg driver will be invoked
> > + * (if/as-soon-as one is registered).
> > + */
> > +struct rpmsg_ns_msg {
> > + char name[RPMSG_NAME_SIZE];
> > + __rpmsg32 addr;
> > + __rpmsg32 flags;
> > +} __packed;
> > +
> > +/**
> > + * enum rpmsg_ns_flags - dynamic name service announcement flags
> > + *
> > + * @RPMSG_NS_CREATE: a new remote service was just created
> > + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> > + */
> > +enum rpmsg_ns_flags {
> > + RPMSG_NS_CREATE = 0,
> > + RPMSG_NS_DESTROY = 1,
> > +};
> > +
> > +/* Address 53 is reserved for advertising remote services */
> > +#define RPMSG_NS_ADDR (53)
>
> What about my proposal [1] to put this in rpmsg.h, to create a list of
> reserved Address
That too is a grey area... Moving RPMSG_NS_ADDR to rpmsg.h means we have a name
service #define in rpmsg.h. I think that one should stay in rpmsg_ns.h.
>
> [1] https://lkml.org/lkml/2020/7/31/442
>
> > +
> > +#endif
> > diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> > index e14c6dab4223..d669c04ef289 100644
> > --- a/include/uapi/linux/rpmsg.h
> > +++ b/include/uapi/linux/rpmsg.h
> > @@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
> > #define RPMSG_CREATE_EPT_IOCTL _IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
> > #define RPMSG_DESTROY_EPT_IOCTL _IO(0xb5, 0x2)
> >
> > +/* The feature bitmap for virtio rpmsg */
> > +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> > +
>
> Same suggestion here,i would drop this from this series
Will do.
Thanks for the feedback,
Mathieu
>
> Thanks,
> Arnaud
>
> > #endif
> >
On 10/15/20 10:19 PM, Mathieu Poirier wrote:
> On Thu, Oct 15, 2020 at 10:33:25AM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
>>> Move structures rpmsg_hdr and rpmsg_ns_msg to their own header file
>>> so that they can be used by other entities.
>>>
>>> Signed-off-by: Mathieu Poirier <[email protected]>
>>> ---
>>> drivers/rpmsg/virtio_rpmsg_bus.c | 58 ++----------------------------
>>> include/linux/rpmsg_ns.h | 62 ++++++++++++++++++++++++++++++++
>>> include/uapi/linux/rpmsg.h | 3 ++
>>> 3 files changed, 67 insertions(+), 56 deletions(-)
>>> create mode 100644 include/linux/rpmsg_ns.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 793fe924671f..85f2acc4ed9f 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -19,7 +19,7 @@
>>> #include <linux/mutex.h>
>>> #include <linux/of_device.h>
>>> #include <linux/rpmsg.h>
>>> -#include <linux/rpmsg_byteorder.h>
>>> +#include <linux/rpmsg_ns.h>
>>> #include <linux/scatterlist.h>
>>> #include <linux/slab.h>
>>> #include <linux/sched.h>
>>> @@ -27,6 +27,7 @@
>>> #include <linux/virtio_ids.h>
>>> #include <linux/virtio_config.h>
>>> #include <linux/wait.h>
>>> +#include <uapi/linux/rpmsg.h>
>>>
>>> #include "rpmsg_internal.h"
>>>
>>> @@ -70,58 +71,6 @@ struct virtproc_info {
>>> struct rpmsg_endpoint *ns_ept;
>>> };
>>>
>>> -/* The feature bitmap for virtio rpmsg */
>>> -#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
>>> -
>>> -/**
>>> - * struct rpmsg_hdr - common header for all rpmsg messages
>>> - * @src: source address
>>> - * @dst: destination address
>>> - * @reserved: reserved for future use
>>> - * @len: length of payload (in bytes)
>>> - * @flags: message flags
>>> - * @data: @len bytes of message payload data
>>> - *
>>> - * Every message sent(/received) on the rpmsg bus begins with this header.
>>> - */
>>> -struct rpmsg_hdr {
>>> - __rpmsg32 src;
>>> - __rpmsg32 dst;
>>> - __rpmsg32 reserved;
>>> - __rpmsg16 len;
>>> - __rpmsg16 flags;
>>> - u8 data[];
>>> -} __packed;
>>> -
>>> -/**
>>> - * struct rpmsg_ns_msg - dynamic name service announcement message
>>> - * @name: name of remote service that is published
>>> - * @addr: address of remote service that is published
>>> - * @flags: indicates whether service is created or destroyed
>>> - *
>>> - * This message is sent across to publish a new service, or announce
>>> - * about its removal. When we receive these messages, an appropriate
>>> - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>>> - * or ->remove() handler of the appropriate rpmsg driver will be invoked
>>> - * (if/as-soon-as one is registered).
>>> - */
>>> -struct rpmsg_ns_msg {
>>> - char name[RPMSG_NAME_SIZE];
>>> - __rpmsg32 addr;
>>> - __rpmsg32 flags;
>>> -} __packed;
>>> -
>>> -/**
>>> - * enum rpmsg_ns_flags - dynamic name service announcement flags
>>> - *
>>> - * @RPMSG_NS_CREATE: a new remote service was just created
>>> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>>> - */
>>> -enum rpmsg_ns_flags {
>>> - RPMSG_NS_CREATE = 0,
>>> - RPMSG_NS_DESTROY = 1,
>>> -};
>>> -
>>> /**
>>> * @vrp: the remote processor this channel belongs to
>>> */
>>> @@ -162,9 +111,6 @@ struct virtio_rpmsg_channel {
>>> */
>>> #define RPMSG_RESERVED_ADDRESSES (1024)
>>>
>>> -/* Address 53 is reserved for advertising remote services */
>>> -#define RPMSG_NS_ADDR (53)
>>> -
>>> static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept);
>>> static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
>>> static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
>>> diff --git a/include/linux/rpmsg_ns.h b/include/linux/rpmsg_ns.h
>>> new file mode 100644
>>> index 000000000000..3d836b8580b2
>>> --- /dev/null
>>> +++ b/include/linux/rpmsg_ns.h
>>> @@ -0,0 +1,62 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>> +#ifndef _LINUX_RPMSG_NS_H
>>> +#define _LINUX_RPMSG_NS_H
>>> +
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/types.h>
>>> +#include <linux/rpmsg_byteorder.h>
>>> +
>>> +/**
>>> + * struct rpmsg_hdr - common header for all rpmsg messages
>>> + * @src: source address
>>> + * @dst: destination address
>>> + * @reserved: reserved for future use
>>> + * @len: length of payload (in bytes)
>>> + * @flags: message flags
>>> + * @data: @len bytes of message payload data
>>> + *
>>> + * Every message sent(/received) on the rpmsg bus begins with this header.
>>> + */
>>> +struct rpmsg_hdr {
>>> + __rpmsg32 src;
>>> + __rpmsg32 dst;
>>> + __rpmsg32 reserved;
>>> + __rpmsg16 len;
>>> + __rpmsg16 flags;
>>> + u8 data[];
>>> +} __packed;
>>
>> This structure is not related to the rpmsg ns service but to the rpmsg bus.
>> If this structure has to be exposed to rpmsg client should be in rpmsg.h, but
>> Is there a need to expose it for now?
>> I suppose that it is for vhost...As the need will depends on the implementation,
>> I would suggest leaving it internally and expose only if needed, in the
>> related series.
>>
>
> I also thought about moving rpmsg_hdr to rpmsg.h but decided against because in
> most cases using the name space service usually means that a message header will
> be required. I also thought it would be easier to use, i.e include one header
> rather than two. That too is a little thin because anyone using a name service
> will also need to get access to rpmsg_device, which is in rpmsg.h.
>
> I'm definitely not strongly opinionated on where it should go, or I can leave it
> in virtio_rpmsg_bus.c too...
The rpmsg_ns service does not use this structure. So seems to me not at the good place
The virtio_rpmsg.c does, and a vhost_rpmsg.c should do it as well.
I would be in favor of moving it to rpmsg_internal.h to expose it to
the rpmg_buses (e.g. rpmsg_vhost) as a generic message header.
Leaving it in virtio_rpmsg_bus.c also seems also a good alternative.
>
>>> +
>>> +/**
>>> + * struct rpmsg_ns_msg - dynamic name service announcement message
>>> + * @name: name of remote service that is published
>>> + * @addr: address of remote service that is published
>>> + * @flags: indicates whether service is created or destroyed
>>> + *
>>> + * This message is sent across to publish a new service, or announce
>>> + * about its removal. When we receive these messages, an appropriate
>>> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>>> + * or ->remove() handler of the appropriate rpmsg driver will be invoked
>>> + * (if/as-soon-as one is registered).
>>> + */
>>> +struct rpmsg_ns_msg {
>>> + char name[RPMSG_NAME_SIZE];
>>> + __rpmsg32 addr;
>>> + __rpmsg32 flags;
>>> +} __packed;
>>> +
>>> +/**
>>> + * enum rpmsg_ns_flags - dynamic name service announcement flags
>>> + *
>>> + * @RPMSG_NS_CREATE: a new remote service was just created
>>> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>>> + */
>>> +enum rpmsg_ns_flags {
>>> + RPMSG_NS_CREATE = 0,
>>> + RPMSG_NS_DESTROY = 1,
>>> +};
>>> +
>>> +/* Address 53 is reserved for advertising remote services */
>>> +#define RPMSG_NS_ADDR (53)
>>
>> What about my proposal [1] to put this in rpmsg.h, to create a list of
>> reserved Address
>
> That too is a grey area... Moving RPMSG_NS_ADDR to rpmsg.h means we have a name
> service #define in rpmsg.h. I think that one should stay in rpmsg_ns.h.
That seems reasonable, very simple to change this in future if needed.
Regards
Arnaud
>
>>
>> [1] https://lkml.org/lkml/2020/7/31/442
>>
>>> +
>>> +#endif
>>> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
>>> index e14c6dab4223..d669c04ef289 100644
>>> --- a/include/uapi/linux/rpmsg.h
>>> +++ b/include/uapi/linux/rpmsg.h
>>> @@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
>>> #define RPMSG_CREATE_EPT_IOCTL _IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
>>> #define RPMSG_DESTROY_EPT_IOCTL _IO(0xb5, 0x2)
>>>
>>> +/* The feature bitmap for virtio rpmsg */
>>> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
>>> +
>>
>> Same suggestion here,i would drop this from this series
>
> Will do.
>
> Thanks for the feedback,
> Mathieu
>
>>
>> Thanks,
>> Arnaud
>>
>>> #endif
>>>