linux-kernel - Re: [PATCH v4 2/7] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2875938.88bMQJbFj6@diego>
Date: Sun, 18 Aug 2024 23:15:58 +0200
From: Heiko Stübner <heiko@...ech.de>
To: Lee Jones <lee@...nel.org>
Cc: jdelvare@...e.com, linux@...ck-us.net, dmitry.torokhov@...il.com,
 pavel@....cz, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 ukleinek@...ian.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-hwmon@...r.kernel.org,
 linux-input@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v4 2/7] mfd: add base driver for qnap-mcu devices

Hi Lee,

thanks a lot for your review,

Am Freitag, 16. August 2024, 19:13:36 CEST schrieb Lee Jones:
> On Sat, 10 Aug 2024, Heiko Stuebner wrote:


> > +/*
> > + * MFD core driver for the MCU in Qnap NAS devices that is connected
> 
> No such thing as an "MFD".  Please describe the device.
> 
> Is it QNAP or Qnap?  Please be consistent.

Looks like QNAP spells itself in all upper case on their website, so I did
go with that


> > + * via a dedicated UART port
> > + *
> > + * Copyright (C) 2024 Heiko Stuebner <heiko@...ech.de>
> > + */
> > +
> > +#include <linux/export.h>
> 
> What is this used for?

the #define EXPORT_SYMBOL_GPL that gets used below lives in that file.


> > + * struct qnap_mcu - QNAP NAS embedded controller
> > + *
> > + * @serdev:	Pointer to underlying serdev
> > + * @bus_lock:	Lock to serialize access to the device
> > + * @reply_lock:	Lock protecting @reply
> > + * @reply:	Pointer to memory to store reply payload
> > + * @variant:	Device variant specific information
> > + * @version:	MCU firmware version
> > + */
> > +struct qnap_mcu {
> > +	struct serdev_device *serdev;
> > +	/* Serialize access to the device */
> 
> Comments and K-doc is OOT.

I don't really know what OOT means, but I guess you mean that only
one or the other is needed?

Though somehow checkpatch was very unhappy without those comments directly
above the mutex declaration. But I can of course remove these additional
comments


> > +	mutex_lock(&mcu->reply_lock);
> > +	if (!reply) {
> > +		dev_warn(dev, "received %zu bytes, we were not waiting for\n",
> > +			 size);
> > +		mutex_unlock(&mcu->reply_lock);
> 
> guard(mutex)?

thanks a lot for pointing out this neat feature


> > +int qnap_mcu_exec_with_ack(struct qnap_mcu *mcu,
> > +			   const u8 *cmd_data, size_t cmd_data_size)
> > +{
> > +	u8 ack[2];
> > +	int ret;
> > +
> > +	ret = qnap_mcu_exec(mcu, cmd_data, cmd_data_size, ack, sizeof(ack));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Should return @0 */
> > +	if (ack[0] != 0x40 || ack[1] != 0x30) {
> 
> Why not use the char variants?

Consistency, I guess.

While the basic commands _seem_ to use values equivalent to ascii
characters, I've also seen other commands where the values are not based
on those but use real numbers / hex values for data instead.

So I opted to go with hex values for all and kept the comments, if someone
wants to get started talking to their MCU via a terminal.


> > +
> > +/*
> > + * The MCU controls power to the peripherals but not the CPU.
> > + *
> > + * So using the pmic to power off the system keeps the MCU and hard-drives
> > + * running. This also then prevents the system from turning back on until
> > + * the MCU is turned off by unplugging the power-cable.
> > + * Turning off the MCU alone on the other hand turns off the hard-drives,
> > + * LEDs, etc while the main SoC stays running - including its network ports.
> > + */
> > +static int qnap_mcu_power_off(struct sys_off_data *data)
> > +{
> > +	struct qnap_mcu *mcu = data->cb_data;
> > +	int ret;
> > +	u8 cmd[] = {
> > +		[0] = 0x40, /* @ */
> > +		[1] = 0x43, /* C */
> > +		[2] = 0x30  /* 0 */
> > +	};
> 
> u8 cmd [] = { '@', 'C', '0' };  ?

see above.

I guess this is a style choice, we could of course go with
	u8 cmd[] = { 0x40, 0x43, 0x30 }
if you prefer that.


> > +static int qnap_mcu_probe(struct serdev_device *serdev)
> > +{
> > +	struct device *dev = &serdev->dev;
> > +	struct qnap_mcu *mcu;
> > +	int ret;
> > +
> > +	mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
> > +	if (!mcu)
> > +		return -ENOMEM;
> > +
> > +	mcu->serdev = serdev;
> > +	dev_set_drvdata(dev, mcu);
> > +
> > +	mcu->variant = of_device_get_match_data(dev);
> > +	if (!mcu->variant)
> > +		return -ENODEV;
> > +
> > +	mutex_init(&mcu->bus_lock);
> > +	mutex_init(&mcu->reply_lock);
> 
> Can you not get away with a single lock?

Of course all behaviour information of the device come from observation
alone right now, but it does look like the MCU does not cause
transmissions of its own, but only as a reply to a sent command.

So yes, I can probably do away with the whole reply assignment and just
use one dataset and thus just the main lock.


Heiko