MIME-Version: 1.0
References: <20211109083926.32052-1-christian.gmeiner@gmail.com>
 <20211109083926.32052-2-christian.gmeiner@gmail.com> <YYq4tjyv0qh+Zpqe@ripper>
 <30de037d-3acd-8e9d-979c-b3d0c1c84002@foss.st.com>
In-Reply-To: <30de037d-3acd-8e9d-979c-b3d0c1c84002@foss.st.com>
From:   Christian Gmeiner <christian.gmeiner@gmail.com>
Date:   Thu, 11 Nov 2021 11:29:04 +0100
Message-ID: <CAH9NwWdy9WsaXW+XE06=yA9pGXGs_Ye6kRiNPMfM5D=Jd1rSGg@mail.gmail.com>
Subject: Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver
To:     Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Ohad Ben-Cohen <ohad@wizery.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-remoteproc@vger.kernel.org,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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


-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy