| Message ID | 20211109083926.32052-2-christian.gmeiner@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rpmsg: syslog driver | expand |
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 >
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@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? 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 >>
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@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? > > 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 > >>
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.
On Thu 11 Nov 04:36 CST 2021, Christian Gmeiner wrote: > 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. > I'd be happy to take any documentation updates - or perhaps we should reduce the duplication between the kerneldoc and Documentation/*... > > > + 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. > My feeling is "do we really need a dedicated driver for it", but I do recognize that it solves a problem for you. The only "objection" I have is that I personally wouldn't want all the firmware logs mixed up with the kernel logs. Somehow getting this into journald separately seems more useful. Regards, Bjorn > -- > greets > -- > Christian Gmeiner, MSc > > https://christian-gmeiner.info/privacypolicy
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@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? >> >> 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 >>>> > > >
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); + 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);
Allows the remote firmware to log into syslog. Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> --- drivers/rpmsg/Kconfig | 8 +++++ drivers/rpmsg/Makefile | 1 + drivers/rpmsg/rpmsg_syslog.c | 65 ++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 drivers/rpmsg/rpmsg_syslog.c