linux-kernel - Re: [PATCH v9 4/9] mfd: add base driver for qnap-mcu devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5047567.0VBMTVartN@diego>
Date: Thu, 19 Dec 2024 21:01:57 +0100
From: Heiko Stübner <heiko@...ech.de>
To: lee@...nel.org, jikos@...nel.org, jic23@...nel.org,
 Kees Bakker <kees@...erbout.nl>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 jdelvare@...e.com, linux@...ck-us.net, srinivas.pandruvada@...ux.intel.com,
 bentiss@...nel.org, dmitry.torokhov@...il.com, pavel@....cz,
 ukleinek@...ian.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-input@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-leds@...r.kernel.org
Subject: Re: [PATCH v9 4/9] mfd: add base driver for qnap-mcu devices

Am Donnerstag, 19. Dezember 2024, 20:51:41 CET schrieb Kees Bakker:
> Op 19-12-2024 om 20:43 schreef Heiko Stübner:
> > Hi Kees,
> >
> > Am Donnerstag, 19. Dezember 2024, 20:18:38 CET schrieb Kees Bakker:
> >> Op 07-11-2024 om 12:47 schreef Heiko Stuebner:
> >>> These microcontroller units are used in network-attached-storage devices
> >>> made by QNAP and provide additional functionality to the system.
> >>>
> >>> This adds the base driver that implements the serial protocol via
> >>> serdev and additionally hooks into the poweroff handlers to turn
> >>> off the parts of the system not supplied by the general PMIC.
> >>>
> >>> Turning off (at least the TSx33 devices using Rockchip SoCs) consists of
> >>> two separate actions. Turning off the MCU alone does not turn off the main
> >>> SoC and turning off only the SoC/PMIC does not turn off the hard-drives.
> >>> Also if the MCU is not turned off, the system also won't start again until
> >>> it is unplugged from power.
> >>>
> >>> So on shutdown the MCU needs to be turned off separately before the
> >>> main PMIC.
> >>>
> >>> The protocol spoken by the MCU is sadly not documented, but was
> >>> obtained by listening to the chatter on the serial port, as thankfully
> >>> the "hal_app" program from QNAPs firmware allows triggering all/most
> >>> MCU actions from the command line.
> >>>
> >>> The implementation of how to talk to the serial device got some
> >>> inspiration from the rave-sp servdev driver.
> >>>
> >>> Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> >>> ---
> >>>    MAINTAINERS                  |   6 +
> >>>    drivers/mfd/Kconfig          |  13 ++
> >>>    drivers/mfd/Makefile         |   2 +
> >>>    drivers/mfd/qnap-mcu.c       | 338 +++++++++++++++++++++++++++++++++++
> >>>    include/linux/mfd/qnap-mcu.h |  26 +++
> >>>    5 files changed, 385 insertions(+)
> >>>    create mode 100644 drivers/mfd/qnap-mcu.c
> >>>    create mode 100644 include/linux/mfd/qnap-mcu.h
> >>>
> >>> [...]
> >>> diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
> >>> new file mode 100644
> >>> index 000000000000..4be39d8b2905
> >>> --- /dev/null
> >>> +++ b/drivers/mfd/qnap-mcu.c
> >>> [...]
> >>> +int qnap_mcu_exec(struct qnap_mcu *mcu,
> >>> +		  const u8 *cmd_data, size_t cmd_data_size,
> >>> +		  u8 *reply_data, size_t reply_data_size)
> >>> +{
> >>> +	unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
> >>> +	size_t length = reply_data_size + QNAP_MCU_CHECKSUM_SIZE;
> >>> +	struct qnap_mcu_reply *reply = &mcu->reply;
> >>> +	int ret = 0;
> >>> +
> >>> +	if (length > sizeof(rx)) {
> >>> +		dev_err(&mcu->serdev->dev, "expected data too big for receive buffer");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	mutex_lock(&mcu->bus_lock);
> >>> +
> >>> +	reply->data = rx,
> >>> +	reply->length = length,
> >>> +	reply->received = 0,
> >>> +	reinit_completion(&reply->done);
> >>> +
> >>> +	qnap_mcu_write(mcu, cmd_data, cmd_data_size);
> >>> +
> >>> +	serdev_device_wait_until_sent(mcu->serdev, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS));
> >>> +
> >>> +	if (!wait_for_completion_timeout(&reply->done, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS))) {
> >>> +		dev_err(&mcu->serdev->dev, "Command timeout\n");
> >>> +		ret = -ETIMEDOUT;
> >>> +	} else {
> >>> +		u8 crc = qnap_mcu_csum(rx, reply_data_size);
> >> Here `rx` is still not initialized.
> > The MCU works in a way that a sent command always causes "reply_data_size"
> > bytes to be returned.
> >
> > So for each qnap_mcu_write() above we know that this amount of bytes has
> > been returned (and thus written into rx) if the completion above finishes
> > sucessfully.
> >
> > "rx" is assigned to reply->data above (same as the expected received size).
> > When characters are received on the serial line, this will trigger
> > qnap_mcu_receive_buf() from the serdev and thus fill those elements in rx.
> >
> > So if we land at the qnap_mcu_csum() call, we do have received the expected
> > amount of bytes from the serdev and thus rx is filled accordingly.
> >
> > If we don't receive the needed amount of bytes, we end up in the timeout
> > above that.
> >
> > What did I miss?
> Sorry, my fault. I missed the essential part of the external event (external
> for this function that is).
> Thanks for explaining.

no worries :-) .

The more eyes, the better.

Heiko