Subject: Re: [RFC PATCH v2 1/1] rpmsg: add syslog driver
To:     Christian Gmeiner <christian.gmeiner@gmail.com>
CC:     <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>
References: <20211109083926.32052-1-christian.gmeiner@gmail.com>
 <20211109083926.32052-2-christian.gmeiner@gmail.com>
 <YYq4tjyv0qh+Zpqe@ripper>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Message-ID: <30de037d-3acd-8e9d-979c-b3d0c1c84002@foss.st.com>
Date:   Wed, 10 Nov 2021 17:55:05 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <YYq4tjyv0qh+Zpqe@ripper>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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