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

Christian Gmeiner
2021-11-11T10:37:10Z

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