Date:   Wed, 17 Nov 2021 09:07:33 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Christian Gmeiner <christian.gmeiner@gmail.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
Subject: Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver
Message-ID: <YZUatUlg72DQb8ui@builder.lan>
References: <20211109083926.32052-1-christian.gmeiner@gmail.com>
 <20211109083926.32052-2-christian.gmeiner@gmail.com>
 <YYq4tjyv0qh+Zpqe@ripper>
 <CAH9NwWcyKyaGrTZ1=N73gA+zjO0wH_oMzLG_zi2TUc3dN69SNg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAH9NwWcyKyaGrTZ1=N73gA+zjO0wH_oMzLG_zi2TUc3dN69SNg@mail.gmail.com>
Precedence: bulk

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