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