LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Luka Kovacic <luka.kovacic@sartura.hr>
Cc: linux-doc@vger.kernel.org, linux-leds@vger.kernel.org,
	devicetree@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-kernel@vger.kernel.org, geert+renesas@glider.be,
	Max.Merchel@tq-group.com, linux@rempel-privat.de,
	daniel@0x0f.com, shawnguo@kernel.org, sam@ravnborg.org,
	arnd@arndb.de, krzysztof.kozlowski@canonical.com,
	pavo.banicevic@sartura.hr, corbet@lwn.net, lee.jones@linaro.org,
	pavel@ucw.cz, robh+dt@kernel.org, linux@roeck-us.net,
	jdelvare@suse.com, goran.medic@sartura.hr,
	luka.perkov@sartura.hr, robert.marko@sartura.hr
Subject: Re: [PATCH v9 2/7] drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU
Date: Wed, 22 Sep 2021 13:41:45 +0200	[thread overview]
Message-ID: <YUsWeWlfanGMCpT8@kroah.com> (raw)
In-Reply-To: <20210824124438.14519-3-luka.kovacic@sartura.hr>

On Tue, Aug 24, 2021 at 02:44:33PM +0200, Luka Kovacic wrote:
> +++ b/drivers/mfd/iei-wt61p803-puzzle.c
> @@ -0,0 +1,908 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* IEI WT61P803 PUZZLE MCU Driver
> + * System management microcontroller for fan control, temperature sensor reading,
> + * LED control and system identification on IEI Puzzle series ARM-based appliances.
> + *
> + * Copyright (C) 2020 Sartura Ltd.

It is 2021 now :(


> + * Author: Luka Kovacic <luka.kovacic@sartura.hr>
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/delay.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/iei-wt61p803-puzzle.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/property.h>
> +#include <linux/sched.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <asm/unaligned.h>

Did you run checkpatch.pl on this series?

> +struct iei_wt61p803_puzzle_device_attribute {
> +	struct device_attribute dev_attr;
> +	enum iei_wt61p803_puzzle_attribute_type type;
> +	u8 index;
> +};

Why does a driver need a special attribute structure?  What is wrong
with the existing one that this is required?

Your "type" and index can come from the device that the attribute is
called for, right?


> +/**
> + * struct iei_wt61p803_puzzle - IEI WT61P803 PUZZLE MCU Driver

This isn't a driver, it's a device.

> + * @serdev:		Pointer to underlying serdev device
> + * @dev:		Pointer to underlying dev device
> + * @reply_lock:		Reply mutex lock
> + * @reply:		Pointer to the iei_wt61p803_puzzle_reply struct
> + * @version:		MCU version related data
> + * @status:		MCU status related data
> + * @response_buffer	Command response buffer allocation
> + * @lock		General member mutex lock
> + */
> +struct iei_wt61p803_puzzle {
> +	struct serdev_device *serdev;
> +	struct device *dev;

What controls the lifespan of this object?

> +	struct mutex reply_lock; /* lock to prevent multiple firmware calls */
> +	struct iei_wt61p803_puzzle_reply *reply;
> +	struct iei_wt61p803_puzzle_mcu_version version;
> +	struct iei_wt61p803_puzzle_mcu_status status;
> +	unsigned char response_buffer[IEI_WT61P803_PUZZLE_BUF_SIZE];
> +	struct mutex lock; /* lock to protect response buffer */
> +};

> +static ssize_t show_output(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev);
> +	struct iei_wt61p803_puzzle_device_attribute *pattr = to_puzzle_dev_attr(attr);
> +	int ret;
> +
> +	switch (pattr->type) {
> +	case IEI_WT61P803_PUZZLE_VERSION:
> +		return scnprintf(buf, PAGE_SIZE, "%s\n", mcu->version.version);

Use sysfs_emit() for all of these please.

> +	case IEI_WT61P803_PUZZLE_BUILD_INFO:
> +		return scnprintf(buf, PAGE_SIZE, "%s\n", mcu->version.build_info);
> +	case IEI_WT61P803_PUZZLE_BOOTLOADER_MODE:
> +		return scnprintf(buf, PAGE_SIZE, "%d\n", mcu->version.bootloader_mode);
> +	case IEI_WT61P803_PUZZLE_PROTOCOL_VERSION:
> +		return scnprintf(buf, PAGE_SIZE, "%s\n", mcu->version.protocol_version);
> +	case IEI_WT61P803_PUZZLE_SERIAL_NUMBER:
> +		ret = iei_wt61p803_puzzle_get_serial_number(mcu);
> +		if (!ret)
> +			ret = scnprintf(buf, PAGE_SIZE, "%s\n", mcu->version.serial_number);
> +		else
> +			ret = 0;

No error?  Why not?

> +		return ret;
> +	case IEI_WT61P803_PUZZLE_MAC_ADDRESS:
> +		ret = iei_wt61p803_puzzle_get_mac_address(mcu, pattr->index);
> +		if (!ret)
> +			ret = scnprintf(buf, PAGE_SIZE, "%s\n",
> +					mcu->version.mac_address[pattr->index]);
> +		else
> +			ret = 0;
> +		return ret;
> +	case IEI_WT61P803_PUZZLE_AC_RECOVERY_STATUS:
> +	case IEI_WT61P803_PUZZLE_POWER_LOSS_RECOVERY:
> +	case IEI_WT61P803_PUZZLE_POWER_STATUS:
> +		ret = iei_wt61p803_puzzle_get_mcu_status(mcu);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&mcu->lock);
> +		switch (pattr->type) {
> +		case IEI_WT61P803_PUZZLE_AC_RECOVERY_STATUS:
> +			ret = scnprintf(buf, PAGE_SIZE, "%x\n",
> +					mcu->status.ac_recovery_status_flag);
> +			break;
> +		case IEI_WT61P803_PUZZLE_POWER_LOSS_RECOVERY:
> +			ret = scnprintf(buf, PAGE_SIZE, "%x\n", mcu->status.power_loss_recovery);
> +			break;
> +		case IEI_WT61P803_PUZZLE_POWER_STATUS:
> +			ret = scnprintf(buf, PAGE_SIZE, "%x\n", mcu->status.power_status);
> +			break;
> +		default:
> +			ret = 0;

No error to return?

> +			break;
> +		}
> +		mutex_unlock(&mcu->lock);
> +		return ret;
> +	default:
> +		return 0;

Again, no error to return?

> +	}
> +
> +	return 0;

Shouldn't you return the size of the buffer?

> +}
> +
> +static ssize_t store_output(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t len)
> +{
> +	unsigned char serial_number[IEI_WT61P803_PUZZLE_SN_LENGTH];
> +	unsigned char mac_address[IEI_WT61P803_PUZZLE_MAC_LENGTH];
> +	struct iei_wt61p803_puzzle *mcu = dev_get_drvdata(dev);
> +	struct iei_wt61p803_puzzle_device_attribute *pattr = to_puzzle_dev_attr(attr);
> +	int power_loss_recovery_action = 0;
> +	int ret;
> +
> +	switch (pattr->type) {
> +	case IEI_WT61P803_PUZZLE_SERIAL_NUMBER:
> +		if (len != (size_t)(IEI_WT61P803_PUZZLE_SN_LENGTH + 1))
> +			return -EINVAL;
> +		memcpy(serial_number, buf, sizeof(serial_number));
> +		ret = iei_wt61p803_puzzle_write_serial_number(mcu, serial_number);
> +		if (ret)
> +			return ret;
> +		return len;
> +	case IEI_WT61P803_PUZZLE_MAC_ADDRESS:
> +		if (len != (size_t)(IEI_WT61P803_PUZZLE_MAC_LENGTH + 1))
> +			return -EINVAL;
> +
> +		memcpy(mac_address, buf, sizeof(mac_address));
> +
> +		if (strlen(attr->attr.name) != 13)
> +			return -EIO;
> +
> +		ret = iei_wt61p803_puzzle_write_mac_address(mcu, mac_address, pattr->index);
> +		if (ret)
> +			return ret;
> +		return len;
> +	case IEI_WT61P803_PUZZLE_POWER_LOSS_RECOVERY:
> +		ret = kstrtoint(buf, 10, &power_loss_recovery_action);
> +		if (ret)
> +			return ret;
> +		ret = iei_wt61p803_puzzle_write_power_loss_recovery(mcu,
> +								    power_loss_recovery_action);
> +		if (ret)
> +			return ret;
> +		return len;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +#define IEI_WT61P803_PUZZLE_ATTR(_name, _mode, _show, _store, _type, _index) \
> +	struct iei_wt61p803_puzzle_device_attribute dev_attr_##_name = \
> +		{ .dev_attr	= __ATTR(_name, _mode, _show, _store), \
> +		  .type		= _type, \
> +		  .index	= _index }

Again, don't use your own attribute macro mess.  Use the
DEVICE_ATTR_RW() ones instead please.


> +
> +#define IEI_WT61P803_PUZZLE_ATTR_RO(_name, _type, _id) \
> +	IEI_WT61P803_PUZZLE_ATTR(_name, 0444, show_output, NULL, _type, _id)

DEVICE_ATTR_RO()

> +
> +#define IEI_WT61P803_PUZZLE_ATTR_RW(_name, _type, _id) \
> +	IEI_WT61P803_PUZZLE_ATTR(_name, 0644, show_output, store_output, _type, _id)

DEVICE_ATTR_RW()

> +
> +static IEI_WT61P803_PUZZLE_ATTR_RO(version, IEI_WT61P803_PUZZLE_VERSION, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RO(build_info, IEI_WT61P803_PUZZLE_BUILD_INFO, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RO(bootloader_mode, IEI_WT61P803_PUZZLE_BOOTLOADER_MODE, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RO(protocol_version, IEI_WT61P803_PUZZLE_PROTOCOL_VERSION, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(serial_number, IEI_WT61P803_PUZZLE_SERIAL_NUMBER, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_0, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_1, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 1);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_2, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 2);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_3, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 3);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_4, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 4);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_5, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 5);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_6, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 6);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(mac_address_7, IEI_WT61P803_PUZZLE_MAC_ADDRESS, 7);
> +static IEI_WT61P803_PUZZLE_ATTR_RO(ac_recovery_status, IEI_WT61P803_PUZZLE_AC_RECOVERY_STATUS, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RW(power_loss_recovery, IEI_WT61P803_PUZZLE_POWER_LOSS_RECOVERY, 0);
> +static IEI_WT61P803_PUZZLE_ATTR_RO(power_status, IEI_WT61P803_PUZZLE_POWER_STATUS, 0);
> +
> +static struct attribute *iei_wt61p803_puzzle_attrs[] = {
> +	&dev_attr_version.dev_attr.attr,
> +	&dev_attr_build_info.dev_attr.attr,
> +	&dev_attr_bootloader_mode.dev_attr.attr,
> +	&dev_attr_protocol_version.dev_attr.attr,
> +	&dev_attr_serial_number.dev_attr.attr,
> +	&dev_attr_mac_address_0.dev_attr.attr,
> +	&dev_attr_mac_address_1.dev_attr.attr,
> +	&dev_attr_mac_address_2.dev_attr.attr,
> +	&dev_attr_mac_address_3.dev_attr.attr,
> +	&dev_attr_mac_address_4.dev_attr.attr,
> +	&dev_attr_mac_address_5.dev_attr.attr,
> +	&dev_attr_mac_address_6.dev_attr.attr,
> +	&dev_attr_mac_address_7.dev_attr.attr,
> +	&dev_attr_ac_recovery_status.dev_attr.attr,
> +	&dev_attr_power_loss_recovery.dev_attr.attr,
> +	&dev_attr_power_status.dev_attr.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(iei_wt61p803_puzzle);
> +
> +static int iei_wt61p803_puzzle_sysfs_create(struct device *dev,
> +					    struct iei_wt61p803_puzzle *mcu)
> +{
> +	int ret;
> +
> +	ret = sysfs_create_groups(&mcu->dev->kobj, iei_wt61p803_puzzle_groups);

You just raced with userspace and lost :(

Please set the default attribute group for your device in the driver and
then the driver core will properly bind and create everything correctly
when the driver binds to the device.

As it is, userspace just did not notice these attributes showing up :(

thanks,

greg k-h

  parent reply	other threads:[~2021-09-22 11:41 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 12:44 [PATCH v9 0/7] Add support for the " Luka Kovacic
2021-08-24 12:44 ` [PATCH v9 1/7] dt-bindings: Add IEI vendor prefix and IEI WT61P803 PUZZLE driver bindings Luka Kovacic
2021-08-24 18:09   ` Rob Herring
2021-08-24 12:44 ` [PATCH v9 2/7] drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU Luka Kovacic
2021-09-22 10:54   ` Lee Jones
2021-09-22 11:42     ` Greg KH
2021-09-22 13:04       ` Lee Jones
2021-09-22 11:41   ` Greg KH [this message]
2021-09-22 12:18     ` Geert Uytterhoeven
2021-09-22 13:37       ` Greg KH
2021-08-24 12:44 ` [PATCH v9 3/7] drivers: hwmon: Add the IEI WT61P803 PUZZLE HWMON driver Luka Kovacic
2021-08-24 12:44 ` [PATCH v9 4/7] drivers: leds: Add the IEI WT61P803 PUZZLE LED driver Luka Kovacic
2021-08-24 12:44 ` [PATCH v9 5/7] Documentation/ABI: Add iei-wt61p803-puzzle driver sysfs interface documentation Luka Kovacic
2021-08-24 12:44 ` [PATCH v9 6/7] Documentation/hwmon: Add iei-wt61p803-puzzle hwmon driver documentation Luka Kovacic
2021-08-24 15:00   ` Guenter Roeck
2021-08-24 12:44 ` [PATCH v9 7/7] MAINTAINERS: Add an entry for the IEI WT61P803 PUZZLE driver Luka Kovacic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YUsWeWlfanGMCpT8@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=Max.Merchel@tq-group.com \
    --cc=arnd@arndb.de \
    --cc=corbet@lwn.net \
    --cc=daniel@0x0f.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=goran.medic@sartura.hr \
    --cc=jdelvare@suse.com \
    --cc=krzysztof.kozlowski@canonical.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux@rempel-privat.de \
    --cc=linux@roeck-us.net \
    --cc=luka.kovacic@sartura.hr \
    --cc=luka.perkov@sartura.hr \
    --cc=pavel@ucw.cz \
    --cc=pavo.banicevic@sartura.hr \
    --cc=robert.marko@sartura.hr \
    --cc=robh+dt@kernel.org \
    --cc=sam@ravnborg.org \
    --cc=shawnguo@kernel.org \
    --subject='Re: [PATCH v9 2/7] drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).