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