Re: [PATCH v9 2/7] drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU
flat view
From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-09-22 11:41:51
Also in:
linux-doc, linux-hwmon, linux-leds, lkml
On Tue, Aug 24, 2021 at 02:44:33PM +0200, Luka Kovacic wrote:
quoted hunk ↗ jump to hunk
+++ 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 :(
quoted hunk ↗ jump to hunk
+ * 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?
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ * @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?
quoted hunk ↗ jump to hunk
+ 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 */ +};
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ break; + } + mutex_unlock(&mcu->lock); + return ret; + default: + return 0;
Again, no error to return?
quoted hunk ↗ jump to hunk
+ } + + return 0;
Shouldn't you return the size of the buffer?
quoted hunk ↗ jump to hunk
+} + +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.
quoted hunk ↗ jump to hunk
+ +#define IEI_WT61P803_PUZZLE_ATTR_RO(_name, _type, _id) \ + IEI_WT61P803_PUZZLE_ATTR(_name, 0444, show_output, NULL, _type, _id)
DEVICE_ATTR_RO()
quoted hunk ↗ jump to hunk
+ +#define IEI_WT61P803_PUZZLE_ATTR_RW(_name, _type, _id) \ + IEI_WT61P803_PUZZLE_ATTR(_name, 0644, show_output, store_output, _type, _id)
DEVICE_ATTR_RW()
quoted hunk ↗ jump to hunk
+ +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