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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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
>