Message-ID: <90704c5c-75fa-8eb1-ef2c-19073a5a8ac9@foss.st.com>
Date:   Wed, 17 Nov 2021 18:57:39 +0100
From:   Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To:     Christian Gmeiner <christian.gmeiner@...il.com>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        <linux-remoteproc@...r.kernel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Subject: Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver

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

Powered by blists - more mailing lists