| 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
| ||
|
Message-ID: <87ec50bb-7040-451c-832b-27b93923d5cf@roeck-us.net>
Date: Fri, 2 Aug 2024 20:47:10 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Inochi Amaoto <inochiama@...look.com>, Jean Delvare <jdelvare@...e.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Chen Wang <unicorn_wang@...look.com>,
Jonathan Corbet <corbet@....net>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Guo Ren <guoren@...nel.org>, Chao Wei <chao.wei@...hgo.com>,
Jinyu Tang <tangjinyu@...ylab.org>, Hal Feng <hal.feng@...rfivetech.com>,
Yangyu Chen <cyy@...self.name>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc: linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v9 2/4] drivers: hwmon: sophgo: Add SG2042 external
hardware monitor support
On 8/2/24 16:28, Inochi Amaoto wrote:
> On Fri, Aug 02, 2024 at 06:59:22AM GMT, Guenter Roeck wrote:
>> On 8/2/24 05:38, Inochi Amaoto wrote:
>>> SG2042 use an external MCU to provide basic hardware information
>>> and thermal sensors.
>>>
>>> Add driver support for the onboard MCU of SG2042.
>>>
>>> Signed-off-by: Inochi Amaoto <inochiama@...look.com>
>>> Tested-by: Chen Wang <unicorn_wang@...look.com>
>>> Reviewed-by: Chen Wang <unicorn_wang@...look.com>
>>> ---
>>> Documentation/hwmon/index.rst | 1 +
>>> Documentation/hwmon/sg2042-mcu.rst | 39 +++
>>> drivers/hwmon/Kconfig | 11 +
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/sg2042-mcu.c | 394 +++++++++++++++++++++++++++++
>>> 5 files changed, 446 insertions(+)
>>> create mode 100644 Documentation/hwmon/sg2042-mcu.rst
>>> create mode 100644 drivers/hwmon/sg2042-mcu.c
>>>
>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>>> index 913c11390a45..ea3b5be8fe4f 100644
>>> --- a/Documentation/hwmon/index.rst
>>> +++ b/Documentation/hwmon/index.rst
>>> @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
>>> sch5636
>>> scpi-hwmon
>>> sfctemp
>>> + sg2042-mcu
>>> sht15
>>> sht21
>>> sht3x
>>> diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
>>> new file mode 100644
>>> index 000000000000..250016b47dd1
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/sg2042-mcu.rst
>>> @@ -0,0 +1,39 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +Kernel driver sg2042-mcu
>>> +=====================
>>> +
>>> +Supported chips:
>>> +
>>> + * Onboard MCU for sg2042
>>> +
>>> + Addresses scanned: -
>>> +
>>> + Prefix: 'sg2042-mcu'
>>> +
>>> +Authors:
>>> +
>>> + - Inochi Amaoto <inochiama@...look.com>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver supprts hardware monitoring for onboard MCU with
>>> +i2c interface.
>>> +
>>> +Usage Notes
>>> +-----------
>>> +
>>> +This driver does not auto-detect devices. You will have to instantiate
>>> +the devices explicitly.
>>> +Please see Documentation/i2c/instantiating-devices.rst for details.
>>> +
>>> +Sysfs Attributes
>>> +----------------
>>> +
>>> +================= =============================================
>>> +temp1_input Measured temperature of SoC
>>> +temp1_crit Critical high temperature
>>> +temp1_crit_hyst hysteresis temperature restore from Critical
>>> +temp2_input Measured temperature of the base board
>>> +================= =============================================
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index b60fe2e58ad6..7aa6c3f322e5 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
>>> This driver can also be built as a module. If so, the module
>>> will be called sfctemp.
>>> +config SENSORS_SG2042_MCU
>>> + tristate "Sophgo onboard MCU support"
>>> + depends on I2C
>>> + depends on ARCH_SOPHGO || COMPILE_TEST
>>> + help
>>> + Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
>>> + power control and some basic information.
>>> +
>>> + This driver can be built as a module. If so, the module
>>> + will be called sg2042-mcu.
>>> +
>>> config SENSORS_SURFACE_FAN
>>> tristate "Surface Fan Driver"
>>> depends on SURFACE_AGGREGATOR
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index b1c7056c37db..0bbe812a67ae 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>>> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
>>> obj-$(CONFIG_SENSORS_SCH5636) += sch5636.o
>>> obj-$(CONFIG_SENSORS_SFCTEMP) += sfctemp.o
>>> +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
>>> obj-$(CONFIG_SENSORS_SL28CPLD) += sl28cpld-hwmon.o
>>> obj-$(CONFIG_SENSORS_SHT15) += sht15.o
>>> obj-$(CONFIG_SENSORS_SHT21) += sht21.o
>>> diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
>>> new file mode 100644
>>> index 000000000000..6d8d677f86f3
>>> --- /dev/null
>>> +++ b/drivers/hwmon/sg2042-mcu.c
>>> @@ -0,0 +1,394 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2024 Inochi Amaoto <inochiama@...look.com>
>>> + *
>>> + * Sophgo power control mcu for SG2042
>>> + */
>>> +
>>> +#include <linux/cleanup.h>
>>> +#include <linux/debugfs.h>
>>> +#include <linux/err.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +/* fixed MCU registers */
>>> +#define REG_BOARD_TYPE 0x00
>>> +#define REG_MCU_FIRMWARE_VERSION 0x01
>>> +#define REG_PCB_VERSION 0x02
>>> +#define REG_PWR_CTRL 0x03
>>> +#define REG_SOC_TEMP 0x04
>>> +#define REG_BOARD_TEMP 0x05
>>> +#define REG_RST_COUNT 0x0a
>>> +#define REG_UPTIME 0x0b
>>> +#define REG_RESET_REASON 0x0d
>>> +#define REG_MCU_TYPE 0x18
>>> +#define REG_REPOWER_ACTION 0x65
>>> +#define REG_CRITICAL_TEMP 0x66
>>> +#define REG_REPOWER_TEMP 0x67
>>> +
>>> +#define REPOWER_ACTION_REBOOT 1
>>> +#define REPOWER_ACTION_POWEROFF 2
>>> +
>>> +#define MCU_POWER_MAX 0xff
>>> +
>>> +#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format) \
>>> + static int _name##_show(struct seq_file *seqf, \
>>> + void *unused) \
>>> + { \
>>> + struct sg2042_mcu_data *mcu = seqf->private; \
>>> + int ret; \
>>> + ret = i2c_smbus_read_byte_data(mcu->client, (_reg)); \
>>> + if (ret < 0) \
>>> + return ret; \
>>> + seq_printf(seqf, _format "\n", ret); \
>>> + return 0; \
>>> + } \
>>> + DEFINE_SHOW_ATTRIBUTE(_name) \
>>> +
>>> +struct sg2042_mcu_data {
>>> + struct i2c_client *client;
>>> + struct dentry *debugfs;
>>> + struct mutex mutex;
>>> +};
>>> +
>>> +static struct dentry *sgmcu_debugfs;
>>> +
>>> +static ssize_t reset_count_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return sprintf(buf, "%d\n", ret);
>>> +}
>>> +
>>> +static ssize_t uptime_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + u8 time_val[2];
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
>>> + sizeof(time_val), time_val);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return sprintf(buf, "%d\n",
>>> + (int)(time_val[0]) + (int)(time_val[1] << 8));
>>
>> Pointless type casts
>>
>>> +}
>>> +
>>> +static ssize_t reset_reason_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return sprintf(buf, "0x%02x\n", ret);
>>> +}
>>> +
>>> +static ssize_t repower_action_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + int ret;
>>> + const char *action;
>>> +
>>> + ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (ret == REPOWER_ACTION_REBOOT)
>>> + action = "reboot";
>>> + else if (ret == REPOWER_ACTION_POWEROFF)
>>> + action = "poweroff";
>>> + else
>>> + action = "unknown";
>>> +
>>> + return sprintf(buf, "%s\n", action);
>>> +}
>>> +
>>> +static ssize_t repower_action_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + u8 value;
>>> + int ret;
>>> +
>>> + if (sysfs_streq("reboot", buf))
>>> + value = REPOWER_ACTION_REBOOT;
>>> + else if (sysfs_streq("poweroff", buf))
>>> + value = REPOWER_ACTION_POWEROFF;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + guard(mutex)(&mcu->mutex);
>>> +
>> Pointless mutex protection.
>>
>
> OK, I have seen the protection in the i2c bus. I will remove this.
>
>>> + ret = i2c_smbus_write_byte_data(mcu->client,
>>> + REG_REPOWER_ACTION, value);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(reset_count);
>>> +static DEVICE_ATTR_RO(uptime);
>>> +static DEVICE_ATTR_RO(reset_reason);
>>> +static DEVICE_ATTR_RW(repower_action);
>>> +
>>> +DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
>>> +DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
>>> +DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
>>> +DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
>>> +
>>> +static struct attribute *sg2042_mcu_attrs[] = {
>>> + &dev_attr_reset_count.attr,
>>> + &dev_attr_uptime.attr,
>>> + &dev_attr_reset_reason.attr,
>>> + &dev_attr_repower_action.attr,
>>> + NULL
>>> +};
>>> +
>>> +static const struct attribute_group sg2042_mcu_attr_group = {
>>> + .attrs = sg2042_mcu_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *sg2042_mcu_groups[] = {
>>> + &sg2042_mcu_attr_group,
>>> + NULL
>>> +};
>>> +
>>> +static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
>>> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
>>> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
>>> + HWMON_T_CRIT_HYST,
>>> + HWMON_T_INPUT),
>>> + NULL
>>> +};
>>> +
>>> +static int sg2042_mcu_read(struct device *dev,
>>> + enum hwmon_sensor_types type,
>>> + u32 attr, int channel, long *val)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + int tmp;
>>> + u8 reg;
>>> +
>>> + switch (attr) {
>>> + case hwmon_temp_input:
>>> + reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
>>> + break;
>>> + case hwmon_temp_crit:
>>> + reg = REG_CRITICAL_TEMP;
>>> + break;
>>> + case hwmon_temp_crit_hyst:
>>> + reg = REG_REPOWER_TEMP;
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + tmp = i2c_smbus_read_byte_data(mcu->client, reg);
>>> + if (tmp < 0)
>>> + return tmp;
>>> + *val = tmp * 1000;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sg2042_mcu_write(struct device *dev,
>>> + enum hwmon_sensor_types type,
>>> + u32 attr, int channel, long val)
>>> +{
>>> + struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> + int temp = val / 1000;
>>> + int hyst_temp, crit_temp;
>>> + u8 reg;
>>> +
>>> + if (temp > MCU_POWER_MAX)
>>> + temp = MCU_POWER_MAX;
>>> + else if (temp < 0)
>>> + temp = 0;
>>> +
>> temp = clamp_val(temp, 0, MCU_POWER_MAX);
>>
>>> + guard(mutex)(&mcu->mutex);
>>> +
>>> + switch (attr) {
>>> + case hwmon_temp_crit:
>>> + hyst_temp = i2c_smbus_read_byte_data(mcu->client,
>>> + REG_REPOWER_TEMP);
>>> + if (hyst_temp < 0)
>>> + return hyst_temp;
>>> +
>>> + crit_temp = temp;
>>> + reg = REG_CRITICAL_TEMP;
>>> + break;
>>> + case hwmon_temp_crit_hyst:
>>> + crit_temp = i2c_smbus_read_byte_data(mcu->client,
>>> + REG_CRITICAL_TEMP);
>>> + if (crit_temp < 0)
>>> + return crit_temp;
>>> +
>>> + hyst_temp = temp;
>>> + reg = REG_REPOWER_TEMP;
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + /*
>>> + * ensure hyst_temp is smaller to avoid MCU from
>>> + * keeping triggering repower event.
>>> + */
>>> + if (crit_temp < hyst_temp)
>>> + return -EINVAL;
>>> +
>>> + return i2c_smbus_write_byte_data(mcu->client, reg, temp);
>>> +}
>>> +
>>> +static umode_t sg2042_mcu_is_visible(const void *_data,
>>> + enum hwmon_sensor_types type,
>>> + u32 attr, int channel)
>>> +{
>>> + switch (type) {
>>> + case hwmon_temp:
>>> + switch (attr) {
>>> + case hwmon_temp_input:
>>> + return 0444;
>>> + case hwmon_temp_crit:
>>> + case hwmon_temp_crit_hyst:
>>> + if (channel == 0)
>>> + return 0664;
>>> + break;
>>
>> In practice this is the only break; in this nested switch sequence,
>> making the return 0; at the end necessary.
>>
>> Please either use break; always when returning 0, or return 0 here
>> as well and drop the final return 0;
>>
>
> Thanks, but I have a question: as this break can be fallthrough to the
> default, is it better to use break;/return 0;, or just remove this
> break; and let it fallthrough?
>
Either always use break; when returning 0 or always use "return 0;".
Please don't rely on fallthrough; it doesn't improve anything since you'd
have to tag it as fallthrough and you might then as well use break;
or return 0; directly.
Guenter
>>> + default:
>>> + return 0;
>>> + }
>>> + break;
>>> + default:
>>> + return 0;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static const struct hwmon_ops sg2042_mcu_ops = {
>>> + .is_visible = sg2042_mcu_is_visible,
>>> + .read = sg2042_mcu_read,
>>> + .write = sg2042_mcu_write,
>>> +};
>>> +
>>> +static const struct hwmon_chip_info sg2042_mcu_chip_info = {
>>> + .ops = &sg2042_mcu_ops,
>>> + .info = sg2042_mcu_info,
>>> +};
>>> +
>>> +static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
>>> + struct device *dev)
>>> +{
>>> + mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
>>> + if (mcu->debugfs) {
>>
>> debugfs_create_dir() returns an ERR_PTR(), which is checked in debugfs_create_file().
>> This if() is pointless.
>>
>
> Thanks.
>
>>> + debugfs_create_file("firmware_version", 0444, mcu->debugfs,
>>> + mcu, &firmware_version_fops);
>>> + debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
>>> + &pcb_version_fops);
>>> + debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
>>> + &mcu_type_fops);
>>> + debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
>>> + &board_type_fops);
>>> + }
>>> +}
>>> +
>>> +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + struct sg2042_mcu_data *mcu;
>>> + struct device *hwmon_dev;
>>> +
>>> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_BLOCK_DATA))
>>> + return -EIO;
>>> +
>> return -ENODEV;
>>
>>> + mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
>>> + if (!mcu)
>>> + return -ENOMEM;
>>> +
>>> + mutex_init(&mcu->mutex);
>>> + mcu->client = client;
>>> +
>>> + i2c_set_clientdata(client, mcu);
>>> +
>>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
>>> + mcu,
>>> + &sg2042_mcu_chip_info,
>>> + NULL);
>>> + if (IS_ERR_OR_NULL(hwmon_dev))
>>> + return -EFAULT;
>>
>> devm_hwmon_device_register_with_info() returns an ERR_PTR(), and
>> returning -EFAULT instead of it is more than wrong (this does not indicate
>> a "bad address"). This needs to be
>>
>> if (IS_ERR(hwmon_dev))
>> return PTR_ERR(hwmon_dev);
>>
>> I am sure I asked before not to overwrite error codes.
>
> Yes, you have asked, it seems that I have missed this code when I
> check the driver. I apology for this mistake and thanks for your
> reminder.
>
>>
>>> +
>>> + sg2042_mcu_debugfs_init(mcu, dev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
>>> +{
>>> + struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
>>> +
>>> + debugfs_remove_recursive(mcu->debugfs);
>>> +}
>>> +
>>> +static const struct i2c_device_id sg2042_mcu_id[] = {
>>> + { "sg2042-hwmon-mcu", 0 },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
>>> +
>>> +static const struct of_device_id sg2042_mcu_of_id[] = {
>>> + { .compatible = "sophgo,sg2042-hwmon-mcu" },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
>>> +
>>> +static struct i2c_driver sg2042_mcu_driver = {
>>> + .driver = {
>>> + .name = "sg2042-mcu",
>>> + .of_match_table = sg2042_mcu_of_id,
>>> + .dev_groups = sg2042_mcu_groups,
>>> + },
>>> + .probe = sg2042_mcu_i2c_probe,
>>> + .remove = sg2042_mcu_i2c_remove,
>>> + .id_table = sg2042_mcu_id,
>>> +};
>>> +
>>> +static int __init sg2042_mcu_init(void)
>>> +{
>>> + sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
>>> + return i2c_add_driver(&sg2042_mcu_driver);
>>> +}
>>> +
>>> +static void __exit sg2042_mcu_exit(void)
>>> +{
>>> + debugfs_remove_recursive(sgmcu_debugfs);
>>> + i2c_del_driver(&sg2042_mcu_driver);
>>> +}
>>> +
>>> +module_init(sg2042_mcu_init);
>>> +module_exit(sg2042_mcu_exit);
>>> +
>>> +MODULE_AUTHOR("Inochi Amaoto <inochiama@...look.com>");
>>> +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
>>> +MODULE_LICENSE("GPL");
>>
Powered by blists - more mailing lists