[RFC PATCH v2 0/1] rpmsg: syslog driver

[RFC PATCH v2 0/1] rpmsg: syslog driver

[Christian Gmeiner]

I am sending this patch as RFC I am am aware that this is
not ready for inclusion. I am however want to get some
feedback on this choosen approach. In the end I want to be
able to get log messages from the remote firmware into syslog.

Christian Gmeiner (1):
  rpmsg: add syslog driver

 drivers/rpmsg/Kconfig        |  8 +++++
 drivers/rpmsg/Makefile       |  1 +
 drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)
 create mode 100644 drivers/rpmsg/rpmsg_syslog.c

-- 
2.33.1

[RFC PATCH v2 1/1] rpmsg: add syslog driver

[Christian Gmeiner]

Allows the remote firmware to log into syslog.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/rpmsg/Kconfig        |  8 +++++
 drivers/rpmsg/Makefile       |  1 +
 drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)
 create mode 100644 drivers/rpmsg/rpmsg_syslog.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 0b4407abdf13..801f9956ec21 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -73,4 +73,12 @@ config RPMSG_VIRTIO
 	select RPMSG_NS
 	select VIRTIO
 
+config RPMSG_SYSLOG
+	tristate "SYSLOG device interface"
+	depends on RPMSG
+	help
+	  Say Y here to export rpmsg endpoints as device files, usually found
+	  in /dev. They make it possible for user-space programs to send and
+	  receive rpmsg packets.
+
 endmenu
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..75b2ec7133a5 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
 obj-$(CONFIG_RPMSG_QCOM_SMD)	+= qcom_smd.o
 obj-$(CONFIG_RPMSG_VIRTIO)	+= virtio_rpmsg_bus.o
+obj-$(CONFIG_RPMSG_SYSLOG)	+= rpmsg_syslog.o
diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
new file mode 100644
index 000000000000..b3fdae495fd9
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_syslog.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+
+static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
+			   void *priv, u32 src)
+{
+	const char *buffer = data;
+
+	switch (buffer[0]) {
+	case 'e':
+		dev_err(&rpdev->dev, "%s", buffer + 1);
+		break;
+	case 'w':
+		dev_warn(&rpdev->dev, "%s", buffer + 1);
+		break;
+	case 'i':
+		dev_info(&rpdev->dev, "%s", buffer + 1);
+		break;
+	default:
+		dev_info(&rpdev->dev, "%s", buffer);
+		break;
+	}
+
+	return 0;
+}
+
+static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_endpoint *syslog_ept;
+	struct rpmsg_channel_info syslog_chinfo = {
+		.src = 42,
+		.dst = 42,
+		.name = "syslog",
+	};
+
+	/*
+	 * Create the syslog service endpoint associated to the RPMsg
+	 * device. The endpoint will be automatically destroyed when the RPMsg
+	 * device will be deleted.
+	 */
+	syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);
+	if (!syslog_ept) {
+		dev_err(&rpdev->dev, "failed to create the syslog ept\n");
+		return -ENOMEM;
+	}
+	rpdev->ept = syslog_ept;
+
+	return 0;
+}
+
+static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
+	{ .name = "syslog" },
+	{ },
+};
+MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
+
+static struct rpmsg_driver rpmsg_syslog_client = {
+	.drv.name       = KBUILD_MODNAME,
+	.id_table       = rpmsg_driver_syslog_id_table,
+	.probe          = rpmsg_syslog_probe,
+};
+module_rpmsg_driver(rpmsg_syslog_client);
-- 
2.33.1

Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

[Bjorn Andersson]

On Tue 09 Nov 00:39 PST 2021, Christian Gmeiner wrote:

> Allows the remote firmware to log into syslog.
> 

This allows the remote firmware to print log messages in the kernel log,
not the syslog (although your system might inject the kernel log into
the syslog as well)

> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  drivers/rpmsg/Kconfig        |  8 +++++
>  drivers/rpmsg/Makefile       |  1 +
>  drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++

drivers/rpmsg is for rpmsg bus and transport drivers. Client drivers
should live elsewhere.

But perhaps, rather than having a driver for this, you could simply use
rpmsg_char and a userspace tool; if you want to get the remote processor
logs into syslog, instead of the kernel log?

>  3 files changed, 74 insertions(+)
>  create mode 100644 drivers/rpmsg/rpmsg_syslog.c
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..801f9956ec21 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -73,4 +73,12 @@ config RPMSG_VIRTIO
>  	select RPMSG_NS
>  	select VIRTIO
>  
> +config RPMSG_SYSLOG
> +	tristate "SYSLOG device interface"
> +	depends on RPMSG
> +	help
> +	  Say Y here to export rpmsg endpoints as device files, usually found
> +	  in /dev. They make it possible for user-space programs to send and
> +	  receive rpmsg packets.
> +
>  endmenu
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..75b2ec7133a5 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
>  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
>  obj-$(CONFIG_RPMSG_QCOM_SMD)	+= qcom_smd.o
>  obj-$(CONFIG_RPMSG_VIRTIO)	+= virtio_rpmsg_bus.o
> +obj-$(CONFIG_RPMSG_SYSLOG)	+= rpmsg_syslog.o
> diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
> new file mode 100644
> index 000000000000..b3fdae495fd9
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_syslog.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/rpmsg.h>
> +
> +static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
> +			   void *priv, u32 src)
> +{
> +	const char *buffer = data;
> +
> +	switch (buffer[0]) {
> +	case 'e':
> +		dev_err(&rpdev->dev, "%s", buffer + 1);
> +		break;
> +	case 'w':
> +		dev_warn(&rpdev->dev, "%s", buffer + 1);
> +		break;
> +	case 'i':
> +		dev_info(&rpdev->dev, "%s", buffer + 1);
> +		break;
> +	default:
> +		dev_info(&rpdev->dev, "%s", buffer);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_endpoint *syslog_ept;
> +	struct rpmsg_channel_info syslog_chinfo = {
> +		.src = 42,
> +		.dst = 42,
> +		.name = "syslog",
> +	};
> +
> +	/*
> +	 * Create the syslog service endpoint associated to the RPMsg
> +	 * device. The endpoint will be automatically destroyed when the RPMsg
> +	 * device will be deleted.
> +	 */
> +	syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);

The rpmsg_device_id below should cause the device to probe on the
presence of a "syslog" channel announcement, so why are you creating a
new endpoint with the same here?

Why aren't you just specifying the callback of the driver?

> +	if (!syslog_ept) {
> +		dev_err(&rpdev->dev, "failed to create the syslog ept\n");
> +		return -ENOMEM;
> +	}
> +	rpdev->ept = syslog_ept;
> +
> +	return 0;
> +}
> +
> +static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
> +	{ .name = "syslog" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
> +
> +static struct rpmsg_driver rpmsg_syslog_client = {
> +	.drv.name       = KBUILD_MODNAME,
> +	.id_table       = rpmsg_driver_syslog_id_table,
> +	.probe          = rpmsg_syslog_probe,
> +};
> +module_rpmsg_driver(rpmsg_syslog_client);

I would expect that building this as a module gives you complaints about
lacking MODULE_LICENSE().

Regards,
Bjorn

> -- 
> 2.33.1
> 

Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

[Arnaud POULIQUEN]

Hi Christian,

On 11/9/21 7:06 PM, Bjorn Andersson wrote:
> On Tue 09 Nov 00:39 PST 2021, Christian Gmeiner wrote:
> 
>> Allows the remote firmware to log into syslog.
>>

For you information a similar patch has been sent few years ago:
https://www.spinics.net/lists/kernel/msg3045824.html

The suspend /resume mechanism seems interesting to manage the low power use case.

> 
> This allows the remote firmware to print log messages in the kernel log,
> not the syslog (although your system might inject the kernel log into
> the syslog as well)
> 
>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>> ---
>>  drivers/rpmsg/Kconfig        |  8 +++++
>>  drivers/rpmsg/Makefile       |  1 +
>>  drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
> 
> drivers/rpmsg is for rpmsg bus and transport drivers. Client drivers
> should live elsewhere.
> 
> But perhaps, rather than having a driver for this, you could simply use
> rpmsg_char and a userspace tool; if you want to get the remote processor
> logs into syslog, instead of the kernel log?

This is also a question that comes to me while looking at the patch.
rpmsg_tty service (if available in 5.16) could be another alternative.

Regards,
Arnaud

> 
>>  3 files changed, 74 insertions(+)
>>  create mode 100644 drivers/rpmsg/rpmsg_syslog.c
>>
>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>> index 0b4407abdf13..801f9956ec21 100644
>> --- a/drivers/rpmsg/Kconfig
>> +++ b/drivers/rpmsg/Kconfig
>> @@ -73,4 +73,12 @@ config RPMSG_VIRTIO
>>  	select RPMSG_NS
>>  	select VIRTIO
>>  
>> +config RPMSG_SYSLOG
>> +	tristate "SYSLOG device interface"
>> +	depends on RPMSG
>> +	help
>> +	  Say Y here to export rpmsg endpoints as device files, usually found
>> +	  in /dev. They make it possible for user-space programs to send and
>> +	  receive rpmsg packets.
>> +
>>  endmenu
>> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
>> index 8d452656f0ee..75b2ec7133a5 100644
>> --- a/drivers/rpmsg/Makefile
>> +++ b/drivers/rpmsg/Makefile
>> @@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
>>  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
>>  obj-$(CONFIG_RPMSG_QCOM_SMD)	+= qcom_smd.o
>>  obj-$(CONFIG_RPMSG_VIRTIO)	+= virtio_rpmsg_bus.o
>> +obj-$(CONFIG_RPMSG_SYSLOG)	+= rpmsg_syslog.o
>> diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
>> new file mode 100644
>> index 000000000000..b3fdae495fd9
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_syslog.c
>> @@ -0,0 +1,65 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/rpmsg.h>
>> +
>> +static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
>> +			   void *priv, u32 src)
>> +{
>> +	const char *buffer = data;
>> +
>> +	switch (buffer[0]) {
>> +	case 'e':
>> +		dev_err(&rpdev->dev, "%s", buffer + 1);
>> +		break;
>> +	case 'w':
>> +		dev_warn(&rpdev->dev, "%s", buffer + 1);
>> +		break;
>> +	case 'i':
>> +		dev_info(&rpdev->dev, "%s", buffer + 1);
>> +		break;
>> +	default:
>> +		dev_info(&rpdev->dev, "%s", buffer);
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_endpoint *syslog_ept;
>> +	struct rpmsg_channel_info syslog_chinfo = {
>> +		.src = 42,
>> +		.dst = 42,
>> +		.name = "syslog",
>> +	};
>> +
>> +	/*
>> +	 * Create the syslog service endpoint associated to the RPMsg
>> +	 * device. The endpoint will be automatically destroyed when the RPMsg
>> +	 * device will be deleted.
>> +	 */
>> +	syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);
> 
> The rpmsg_device_id below should cause the device to probe on the
> presence of a "syslog" channel announcement, so why are you creating a
> new endpoint with the same here?
> 
> Why aren't you just specifying the callback of the driver?
> 
>> +	if (!syslog_ept) {
>> +		dev_err(&rpdev->dev, "failed to create the syslog ept\n");
>> +		return -ENOMEM;
>> +	}
>> +	rpdev->ept = syslog_ept;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
>> +	{ .name = "syslog" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
>> +
>> +static struct rpmsg_driver rpmsg_syslog_client = {
>> +	.drv.name       = KBUILD_MODNAME,
>> +	.id_table       = rpmsg_driver_syslog_id_table,
>> +	.probe          = rpmsg_syslog_probe,
>> +};
>> +module_rpmsg_driver(rpmsg_syslog_client);
> 
> I would expect that building this as a module gives you complaints about
> lacking MODULE_LICENSE().
> 
> Regards,
> Bjorn
> 
>> -- 
>> 2.33.1
>>

Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

[Christian Gmeiner]

Hi Arnaud

>
> On 11/9/21 7:06 PM, Bjorn Andersson wrote:
> > On Tue 09 Nov 00:39 PST 2021, Christian Gmeiner wrote:
> >
> >> Allows the remote firmware to log into syslog.
> >>
>
> For you information a similar patch has been sent few years ago:
> https://www.spinics.net/lists/kernel/msg3045824.html
>

Interesting - do you know why the patch was not taken?

> The suspend /resume mechanism seems interesting to manage the low power use case.
>

Yeah .. nothing I thought about.

> >
> > This allows the remote firmware to print log messages in the kernel log,
> > not the syslog (although your system might inject the kernel log into
> > the syslog as well)
> >
> >> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> >> ---
> >>  drivers/rpmsg/Kconfig        |  8 +++++
> >>  drivers/rpmsg/Makefile       |  1 +
> >>  drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
> >
> > drivers/rpmsg is for rpmsg bus and transport drivers. Client drivers
> > should live elsewhere.
> >
> > But perhaps, rather than having a driver for this, you could simply use
> > rpmsg_char and a userspace tool; if you want to get the remote processor
> > logs into syslog, instead of the kernel log?
>
> This is also a question that comes to me while looking at the patch.
> rpmsg_tty service (if available in 5.16) could be another alternative.
>

I thought about it too but I do not see how the syslog/journald can read the log
messages from this tty device without an extra user space component.

With a syslog redirection rpmsg service this happens automatically without any
extra user space daemon that reads from tty and writes to syslog.

Maybe I am missing something.

> Regards,
> Arnaud
>
> >
> >>  3 files changed, 74 insertions(+)
> >>  create mode 100644 drivers/rpmsg/rpmsg_syslog.c
> >>
> >> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> >> index 0b4407abdf13..801f9956ec21 100644
> >> --- a/drivers/rpmsg/Kconfig
> >> +++ b/drivers/rpmsg/Kconfig
> >> @@ -73,4 +73,12 @@ config RPMSG_VIRTIO
> >>      select RPMSG_NS
> >>      select VIRTIO
> >>
> >> +config RPMSG_SYSLOG
> >> +    tristate "SYSLOG device interface"
> >> +    depends on RPMSG
> >> +    help
> >> +      Say Y here to export rpmsg endpoints as device files, usually found
> >> +      in /dev. They make it possible for user-space programs to send and
> >> +      receive rpmsg packets.
> >> +
> >>  endmenu
> >> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> >> index 8d452656f0ee..75b2ec7133a5 100644
> >> --- a/drivers/rpmsg/Makefile
> >> +++ b/drivers/rpmsg/Makefile
> >> @@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
> >>  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
> >>  obj-$(CONFIG_RPMSG_QCOM_SMD)        += qcom_smd.o
> >>  obj-$(CONFIG_RPMSG_VIRTIO)  += virtio_rpmsg_bus.o
> >> +obj-$(CONFIG_RPMSG_SYSLOG)  += rpmsg_syslog.o
> >> diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
> >> new file mode 100644
> >> index 000000000000..b3fdae495fd9
> >> --- /dev/null
> >> +++ b/drivers/rpmsg/rpmsg_syslog.c
> >> @@ -0,0 +1,65 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/rpmsg.h>
> >> +
> >> +static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
> >> +                       void *priv, u32 src)
> >> +{
> >> +    const char *buffer = data;
> >> +
> >> +    switch (buffer[0]) {
> >> +    case 'e':
> >> +            dev_err(&rpdev->dev, "%s", buffer + 1);
> >> +            break;
> >> +    case 'w':
> >> +            dev_warn(&rpdev->dev, "%s", buffer + 1);
> >> +            break;
> >> +    case 'i':
> >> +            dev_info(&rpdev->dev, "%s", buffer + 1);
> >> +            break;
> >> +    default:
> >> +            dev_info(&rpdev->dev, "%s", buffer);
> >> +            break;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
> >> +{
> >> +    struct rpmsg_endpoint *syslog_ept;
> >> +    struct rpmsg_channel_info syslog_chinfo = {
> >> +            .src = 42,
> >> +            .dst = 42,
> >> +            .name = "syslog",
> >> +    };
> >> +
> >> +    /*
> >> +     * Create the syslog service endpoint associated to the RPMsg
> >> +     * device. The endpoint will be automatically destroyed when the RPMsg
> >> +     * device will be deleted.
> >> +     */
> >> +    syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);
> >
> > The rpmsg_device_id below should cause the device to probe on the
> > presence of a "syslog" channel announcement, so why are you creating a
> > new endpoint with the same here?
> >
> > Why aren't you just specifying the callback of the driver?
> >
> >> +    if (!syslog_ept) {
> >> +            dev_err(&rpdev->dev, "failed to create the syslog ept\n");
> >> +            return -ENOMEM;
> >> +    }
> >> +    rpdev->ept = syslog_ept;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
> >> +    { .name = "syslog" },
> >> +    { },
> >> +};
> >> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
> >> +
> >> +static struct rpmsg_driver rpmsg_syslog_client = {
> >> +    .drv.name       = KBUILD_MODNAME,
> >> +    .id_table       = rpmsg_driver_syslog_id_table,
> >> +    .probe          = rpmsg_syslog_probe,
> >> +};
> >> +module_rpmsg_driver(rpmsg_syslog_client);
> >
> > I would expect that building this as a module gives you complaints about
> > lacking MODULE_LICENSE().
> >
> > Regards,
> > Bjorn
> >
> >> --
> >> 2.33.1
> >>



-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

[Arnaud POULIQUEN]

Hi Christian,

On 11/11/21 11:29 AM, Christian Gmeiner wrote:
> Hi Arnaud
> 
>>
>> On 11/9/21 7:06 PM, Bjorn Andersson wrote:
>>> On Tue 09 Nov 00:39 PST 2021, Christian Gmeiner wrote:
>>>
>>>> Allows the remote firmware to log into syslog.
>>>>
>>
>> For you information a similar patch has been sent few years ago:
>> https://www.spinics.net/lists/kernel/msg3045824.html
>>
> 
> Interesting - do you know why the patch was not taken?

I don't know. It might be worthwhile to contact Xiang Xiao for more details.

> 
>> The suspend /resume mechanism seems interesting to manage the low power use case.
>>
> 
> Yeah .. nothing I thought about.
> 
>>>
>>> This allows the remote firmware to print log messages in the kernel log,
>>> not the syslog (although your system might inject the kernel log into
>>> the syslog as well)
>>>
>>>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>>>> ---
>>>>  drivers/rpmsg/Kconfig        |  8 +++++
>>>>  drivers/rpmsg/Makefile       |  1 +
>>>>  drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
>>>
>>> drivers/rpmsg is for rpmsg bus and transport drivers. Client drivers
>>> should live elsewhere.
>>>
>>> But perhaps, rather than having a driver for this, you could simply use
>>> rpmsg_char and a userspace tool; if you want to get the remote processor
>>> logs into syslog, instead of the kernel log?
>>
>> This is also a question that comes to me while looking at the patch.
>> rpmsg_tty service (if available in 5.16) could be another alternative.
>>
> 
> I thought about it too but I do not see how the syslog/journald can read the log
> messages from this tty device without an extra user space component.
> 
> With a syslog redirection rpmsg service this happens automatically without any
> extra user space daemon that reads from tty and writes to syslog.
> 
> Maybe I am missing something.

That's true, this is one constraint. I suppose that you already have user code
to start the remoteproc. In this case it could also launch a deamon which could
redirects and/or perhaps analyzes traces to detect errors...

That said it is my point of view, working on a general purpose platform
(stm32mp15), I guess other people have other feedbacks.

A last question: Do you manage the traces enable/disable and trace level during
runtime?

Regards,
Arnaud

> 
>> Regards,
>> Arnaud
>>
>>>
>>>>  3 files changed, 74 insertions(+)
>>>>  create mode 100644 drivers/rpmsg/rpmsg_syslog.c
>>>>
>>>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>>>> index 0b4407abdf13..801f9956ec21 100644
>>>> --- a/drivers/rpmsg/Kconfig
>>>> +++ b/drivers/rpmsg/Kconfig
>>>> @@ -73,4 +73,12 @@ config RPMSG_VIRTIO
>>>>      select RPMSG_NS
>>>>      select VIRTIO
>>>>
>>>> +config RPMSG_SYSLOG
>>>> +    tristate "SYSLOG device interface"
>>>> +    depends on RPMSG
>>>> +    help
>>>> +      Say Y here to export rpmsg endpoints as device files, usually found
>>>> +      in /dev. They make it possible for user-space programs to send and
>>>> +      receive rpmsg packets.
>>>> +
>>>>  endmenu
>>>> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
>>>> index 8d452656f0ee..75b2ec7133a5 100644
>>>> --- a/drivers/rpmsg/Makefile
>>>> +++ b/drivers/rpmsg/Makefile
>>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
>>>>  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
>>>>  obj-$(CONFIG_RPMSG_QCOM_SMD)        += qcom_smd.o
>>>>  obj-$(CONFIG_RPMSG_VIRTIO)  += virtio_rpmsg_bus.o
>>>> +obj-$(CONFIG_RPMSG_SYSLOG)  += rpmsg_syslog.o
>>>> diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
>>>> new file mode 100644
>>>> index 000000000000..b3fdae495fd9
>>>> --- /dev/null
>>>> +++ b/drivers/rpmsg/rpmsg_syslog.c
>>>> @@ -0,0 +1,65 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/rpmsg.h>
>>>> +
>>>> +static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
>>>> +                       void *priv, u32 src)
>>>> +{
>>>> +    const char *buffer = data;
>>>> +
>>>> +    switch (buffer[0]) {
>>>> +    case 'e':
>>>> +            dev_err(&rpdev->dev, "%s", buffer + 1);
>>>> +            break;
>>>> +    case 'w':
>>>> +            dev_warn(&rpdev->dev, "%s", buffer + 1);
>>>> +            break;
>>>> +    case 'i':
>>>> +            dev_info(&rpdev->dev, "%s", buffer + 1);
>>>> +            break;
>>>> +    default:
>>>> +            dev_info(&rpdev->dev, "%s", buffer);
>>>> +            break;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
>>>> +{
>>>> +    struct rpmsg_endpoint *syslog_ept;
>>>> +    struct rpmsg_channel_info syslog_chinfo = {
>>>> +            .src = 42,
>>>> +            .dst = 42,
>>>> +            .name = "syslog",
>>>> +    };
>>>> +
>>>> +    /*
>>>> +     * Create the syslog service endpoint associated to the RPMsg
>>>> +     * device. The endpoint will be automatically destroyed when the RPMsg
>>>> +     * device will be deleted.
>>>> +     */
>>>> +    syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);
>>>
>>> The rpmsg_device_id below should cause the device to probe on the
>>> presence of a "syslog" channel announcement, so why are you creating a
>>> new endpoint with the same here?
>>>
>>> Why aren't you just specifying the callback of the driver?
>>>
>>>> +    if (!syslog_ept) {
>>>> +            dev_err(&rpdev->dev, "failed to create the syslog ept\n");
>>>> +            return -ENOMEM;
>>>> +    }
>>>> +    rpdev->ept = syslog_ept;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
>>>> +    { .name = "syslog" },
>>>> +    { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
>>>> +
>>>> +static struct rpmsg_driver rpmsg_syslog_client = {
>>>> +    .drv.name       = KBUILD_MODNAME,
>>>> +    .id_table       = rpmsg_driver_syslog_id_table,
>>>> +    .probe          = rpmsg_syslog_probe,
>>>> +};
>>>> +module_rpmsg_driver(rpmsg_syslog_client);
>>>
>>> I would expect that building this as a module gives you complaints about
>>> lacking MODULE_LICENSE().
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> --
>>>> 2.33.1
>>>>
> 
> 
> 

Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

[Christian Gmeiner]

Hi Bjorn


> > Allows the remote firmware to log into syslog.
> >
>
> This allows the remote firmware to print log messages in the kernel log,
> not the syslog (although your system might inject the kernel log into
> the syslog as well)
>

Correct.

> > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> > ---
> >  drivers/rpmsg/Kconfig        |  8 +++++
> >  drivers/rpmsg/Makefile       |  1 +
> >  drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
>
> drivers/rpmsg is for rpmsg bus and transport drivers. Client drivers
> should live elsewhere.
>

Ahh .. yes.

> But perhaps, rather than having a driver for this, you could simply use
> rpmsg_char and a userspace tool; if you want to get the remote processor
> logs into syslog, instead of the kernel log?
>

I thought about that too (also regarding the rpmsg tty driver) but that means I
need to start/supervise a user space tool.

> >  3 files changed, 74 insertions(+)
> >  create mode 100644 drivers/rpmsg/rpmsg_syslog.c
> >
> > diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> > index 0b4407abdf13..801f9956ec21 100644
> > --- a/drivers/rpmsg/Kconfig
> > +++ b/drivers/rpmsg/Kconfig
> > @@ -73,4 +73,12 @@ config RPMSG_VIRTIO
> >       select RPMSG_NS
> >       select VIRTIO
> >
> > +config RPMSG_SYSLOG
> > +     tristate "SYSLOG device interface"
> > +     depends on RPMSG
> > +     help
> > +       Say Y here to export rpmsg endpoints as device files, usually found
> > +       in /dev. They make it possible for user-space programs to send and
> > +       receive rpmsg packets.
> > +
> >  endmenu
> > diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> > index 8d452656f0ee..75b2ec7133a5 100644
> > --- a/drivers/rpmsg/Makefile
> > +++ b/drivers/rpmsg/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
> >  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
> >  obj-$(CONFIG_RPMSG_QCOM_SMD) += qcom_smd.o
> >  obj-$(CONFIG_RPMSG_VIRTIO)   += virtio_rpmsg_bus.o
> > +obj-$(CONFIG_RPMSG_SYSLOG)   += rpmsg_syslog.o
> > diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
> > new file mode 100644
> > index 000000000000..b3fdae495fd9
> > --- /dev/null
> > +++ b/drivers/rpmsg/rpmsg_syslog.c
> > @@ -0,0 +1,65 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/rpmsg.h>
> > +
> > +static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
> > +                        void *priv, u32 src)
> > +{
> > +     const char *buffer = data;
> > +
> > +     switch (buffer[0]) {
> > +     case 'e':
> > +             dev_err(&rpdev->dev, "%s", buffer + 1);
> > +             break;
> > +     case 'w':
> > +             dev_warn(&rpdev->dev, "%s", buffer + 1);
> > +             break;
> > +     case 'i':
> > +             dev_info(&rpdev->dev, "%s", buffer + 1);
> > +             break;
> > +     default:
> > +             dev_info(&rpdev->dev, "%s", buffer);
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
> > +{
> > +     struct rpmsg_endpoint *syslog_ept;
> > +     struct rpmsg_channel_info syslog_chinfo = {
> > +             .src = 42,
> > +             .dst = 42,
> > +             .name = "syslog",
> > +     };
> > +
> > +     /*
> > +      * Create the syslog service endpoint associated to the RPMsg
> > +      * device. The endpoint will be automatically destroyed when the RPMsg
> > +      * device will be deleted.
> > +      */
> > +     syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);
>
> The rpmsg_device_id below should cause the device to probe on the
> presence of a "syslog" channel announcement, so why are you creating a
> new endpoint with the same here?
>
> Why aren't you just specifying the callback of the driver?
>

Good question. I think I was happy that I got work working somehow. I
also want to send out
a documentation update as it is not up to date with the current API.

> > +     if (!syslog_ept) {
> > +             dev_err(&rpdev->dev, "failed to create the syslog ept\n");
> > +             return -ENOMEM;
> > +     }
> > +     rpdev->ept = syslog_ept;
> > +
> > +     return 0;
> > +}
> > +
> > +static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
> > +     { .name = "syslog" },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
> > +
> > +static struct rpmsg_driver rpmsg_syslog_client = {
> > +     .drv.name       = KBUILD_MODNAME,
> > +     .id_table       = rpmsg_driver_syslog_id_table,
> > +     .probe          = rpmsg_syslog_probe,
> > +};
> > +module_rpmsg_driver(rpmsg_syslog_client);
>
> I would expect that building this as a module gives you complaints about
> lacking MODULE_LICENSE().
>

Yeah .. I never built it as a module.

The biggest question I have: do you see any possibility to get such a
redirection
driver into mainline? At the moment I have not heard a big no.

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

[Bjorn Andersson]

On Thu 11 Nov 04:36 CST 2021, Christian Gmeiner wrote:

> Hi Bjorn
> 
> 
> > > Allows the remote firmware to log into syslog.
> > >
> >
> > This allows the remote firmware to print log messages in the kernel log,
> > not the syslog (although your system might inject the kernel log into
> > the syslog as well)
> >
> 
> Correct.
> 
> > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> > > ---
> > >  drivers/rpmsg/Kconfig        |  8 +++++
> > >  drivers/rpmsg/Makefile       |  1 +
> > >  drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++
> >
> > drivers/rpmsg is for rpmsg bus and transport drivers. Client drivers
> > should live elsewhere.
> >
> 
> Ahh .. yes.
> 
> > But perhaps, rather than having a driver for this, you could simply use
> > rpmsg_char and a userspace tool; if you want to get the remote processor
> > logs into syslog, instead of the kernel log?
> >
> 
> I thought about that too (also regarding the rpmsg tty driver) but that means I
> need to start/supervise a user space tool.
> 
> > >  3 files changed, 74 insertions(+)
> > >  create mode 100644 drivers/rpmsg/rpmsg_syslog.c
> > >
> > > diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> > > index 0b4407abdf13..801f9956ec21 100644
> > > --- a/drivers/rpmsg/Kconfig
> > > +++ b/drivers/rpmsg/Kconfig
> > > @@ -73,4 +73,12 @@ config RPMSG_VIRTIO
> > >       select RPMSG_NS
> > >       select VIRTIO
> > >
> > > +config RPMSG_SYSLOG
> > > +     tristate "SYSLOG device interface"
> > > +     depends on RPMSG
> > > +     help
> > > +       Say Y here to export rpmsg endpoints as device files, usually found
> > > +       in /dev. They make it possible for user-space programs to send and
> > > +       receive rpmsg packets.
> > > +
> > >  endmenu
> > > diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> > > index 8d452656f0ee..75b2ec7133a5 100644
> > > --- a/drivers/rpmsg/Makefile
> > > +++ b/drivers/rpmsg/Makefile
> > > @@ -9,3 +9,4 @@ obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
> > >  obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
> > >  obj-$(CONFIG_RPMSG_QCOM_SMD) += qcom_smd.o
> > >  obj-$(CONFIG_RPMSG_VIRTIO)   += virtio_rpmsg_bus.o
> > > +obj-$(CONFIG_RPMSG_SYSLOG)   += rpmsg_syslog.o
> > > diff --git a/drivers/rpmsg/rpmsg_syslog.c b/drivers/rpmsg/rpmsg_syslog.c
> > > new file mode 100644
> > > index 000000000000..b3fdae495fd9
> > > --- /dev/null
> > > +++ b/drivers/rpmsg/rpmsg_syslog.c
> > > @@ -0,0 +1,65 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/rpmsg.h>
> > > +
> > > +static int rpmsg_syslog_cb(struct rpmsg_device *rpdev, void *data, int len,
> > > +                        void *priv, u32 src)
> > > +{
> > > +     const char *buffer = data;
> > > +
> > > +     switch (buffer[0]) {
> > > +     case 'e':
> > > +             dev_err(&rpdev->dev, "%s", buffer + 1);
> > > +             break;
> > > +     case 'w':
> > > +             dev_warn(&rpdev->dev, "%s", buffer + 1);
> > > +             break;
> > > +     case 'i':
> > > +             dev_info(&rpdev->dev, "%s", buffer + 1);
> > > +             break;
> > > +     default:
> > > +             dev_info(&rpdev->dev, "%s", buffer);
> > > +             break;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int rpmsg_syslog_probe(struct rpmsg_device *rpdev)
> > > +{
> > > +     struct rpmsg_endpoint *syslog_ept;
> > > +     struct rpmsg_channel_info syslog_chinfo = {
> > > +             .src = 42,
> > > +             .dst = 42,
> > > +             .name = "syslog",
> > > +     };
> > > +
> > > +     /*
> > > +      * Create the syslog service endpoint associated to the RPMsg
> > > +      * device. The endpoint will be automatically destroyed when the RPMsg
> > > +      * device will be deleted.
> > > +      */
> > > +     syslog_ept = rpmsg_create_ept(rpdev, rpmsg_syslog_cb, NULL, syslog_chinfo);
> >
> > The rpmsg_device_id below should cause the device to probe on the
> > presence of a "syslog" channel announcement, so why are you creating a
> > new endpoint with the same here?
> >
> > Why aren't you just specifying the callback of the driver?
> >
> 
> Good question. I think I was happy that I got work working somehow. I
> also want to send out
> a documentation update as it is not up to date with the current API.
> 

I'd be happy to take any documentation updates - or perhaps we should
reduce the duplication between the kerneldoc and Documentation/*...

> > > +     if (!syslog_ept) {
> > > +             dev_err(&rpdev->dev, "failed to create the syslog ept\n");
> > > +             return -ENOMEM;
> > > +     }
> > > +     rpdev->ept = syslog_ept;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static struct rpmsg_device_id rpmsg_driver_syslog_id_table[] = {
> > > +     { .name = "syslog" },
> > > +     { },
> > > +};
> > > +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_syslog_id_table);
> > > +
> > > +static struct rpmsg_driver rpmsg_syslog_client = {
> > > +     .drv.name       = KBUILD_MODNAME,
> > > +     .id_table       = rpmsg_driver_syslog_id_table,
> > > +     .probe          = rpmsg_syslog_probe,
> > > +};
> > > +module_rpmsg_driver(rpmsg_syslog_client);
> >
> > I would expect that building this as a module gives you complaints about
> > lacking MODULE_LICENSE().
> >
> 
> Yeah .. I never built it as a module.
> 
> The biggest question I have: do you see any possibility to get such a
> redirection
> driver into mainline? At the moment I have not heard a big no.
> 

My feeling is "do we really need a dedicated driver for it", but I do
recognize that it solves a problem for you.

The only "objection" I have is that I personally wouldn't want all the
firmware logs mixed up with the kernel logs. Somehow getting this into
journald separately seems more useful.

Regards,
Bjorn

> -- 
> greets
> --
> Christian Gmeiner, MSc
> 
> https://christian-gmeiner.info/privacypolicy