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@xxxxxxxxx> > >> --- > >> 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
