| 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: <f4c1f3af-c73f-423c-8404-3d0d4bb95d98@amd.com>
Date: Wed, 10 Sep 2025 10:26:02 +0200
From: Michal Simek <michal.simek@....com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: linux-kernel@...r.kernel.org, monstr@...str.eu, michal.simek@...inx.com,
git@...inx.com, Salih Erim <salih.erim@....com>,
Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>,
Andy Shevchenko <andy@...nel.org>, David Lechner <dlechner@...libre.com>,
Jonathan Cameron <jic23@...nel.org>, Nuno Sá
<nuno.sa@...log.com>,
"open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 2/6] iio: versal-sysmon: add driver for Versal Sysmon
Hi Andy,
On 9/9/25 12:33, Andy Shevchenko wrote:
> On Fri, Sep 5, 2025 at 11:42 AM Michal Simek <michal.simek@....com> wrote:
>>
>> Sysmon Driver uses Linux IIO framework, which was used to abstract the
>> supply voltages and temperatures across the chip as Voltage and Temperature
>> Channels in the framework. Since there are only 160 supply voltage
>> registers and 184 measurement points, there is no constant mapping of
>> supply voltage registers and the measurement points. User has to select
>> the voltages to monitor in design tool. Depending on the selection, a
>
> in the design
>
>> voltage supply gets mapped to one of the supply registers. So, this mapping
>> information is provided to the driver via a device tree. Depending on the
>> number of supplies enabled in the design, the device tree will contain the
>> information of name of the supply enabled and the supply register it maps
>> to.
>
> ...
>
>> +config VERSAL_SYSMON
>> + tristate "Xilinx Sysmon driver for Versal"
>> + depends on HAS_IOMEM
>> + select VERSAL_SYSMON_CORE
>> + help
>> + Say yes here to have support for the Xilinx Sysmon.
>> + The driver will enable users to monitor temperature and voltage on the
>> + Xilinx Versal platform.
>> +
>> + The driver can also be build as a module. If so, the module will be called
>
> built
>
>> + versal-sysmon.
>
> ...
>
>> +#include <linux/bits.h>
>
> Quite a few headers are absent...
>
> + export.h
> + types.h
>
>> +#include "versal-sysmon.h"
>
> ...
>
>> +static u32 sysmon_temp_offset(int address)
>> +{
>> + switch (address) {
>> + case TEMP_MAX:
>> + return SYSMON_TEMP_MAX;
>> + case TEMP_MIN:
>> + return SYSMON_TEMP_MIN;
>> + case TEMP_MAX_MAX:
>> + return SYSMON_TEMP_MAX_MAX;
>> + case TEMP_MIN_MIN:
>> + return SYSMON_TEMP_MIN_MIN;
>> + case TEMP_HBM:
>> + return SYSMON_TEMP_HBM;
>> + default:
>> + return -EINVAL;
>> + }
>
>> + return -EINVAL;
>
> Here and in many more, eliminate dead code.
>
>> +}
>
> ...
>
>> +static u32 sysmon_supply_thresh_offset(int address,
>> + enum iio_event_direction dir)
>> +{
>> + if (dir == IIO_EV_DIR_RISING)
>> + return (address * 4) + SYSMON_SUPPLY_TH_UP;
>
>> + else if (dir == IIO_EV_DIR_FALLING)
>> + return (address * 4) + SYSMON_SUPPLY_TH_LOW;
>
> Redundant 'else'.
>
>> + return -EINVAL;
>> +}
>
> ...
>
>> +static void sysmon_hbm_to_millicelsius(int raw_data, int *val, int *val2)
>> +{
>> + *val = ((raw_data >> SYSMON_HBM_TEMP_SHIFT) & SYSMON_HBM_TEMP_MASK) *
>> + SYSMON_MILLI_SCALE;
>
> Can the whole driver be switched to FIELD_PREP()/FIELD_GET()/FIELD_MODIFY()?
>
>> + *val2 = 0;
>> +}
>
> ...
>
>> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val, int val2)
>> +{
>
>> + (void)val2;
>
> Unneeded.
>
>> + *raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / SYSMON_MILLI_SCALE;
>> +}
>
> ...
>
>> +static void sysmon_supply_processedtoraw(int val, int val2, u32 reg_val,
>> + u32 *raw_data)
>> +{
>> + int exponent = (reg_val & SYSMON_MODE_MASK) >> SYSMON_MODE_SHIFT;
>> + int format = (reg_val & SYSMON_FMT_MASK) >> SYSMON_FMT_SHIFT;
>> + int scale = 1 << (16 - exponent);
>> + int tmp;
>> +
>> + tmp = (val * scale) / SYSMON_MILLI_SCALE;
>> +
>> + /* Set out of bound values to saturation levels */
>> + if (format) {
>> + if (tmp > SYSMON_UPPER_SATURATION_SIGNED)
>> + tmp = 0x7fff;
>> + else if (tmp < SYSMON_LOWER_SATURATION_SIGNED)
>> + tmp = 0x8000;
>
> This looks like s16 limits, can you use them?
>
>> + } else {
>> + if (tmp > SYSMON_UPPER_SATURATION)
>> + tmp = 0xffff;
>> + else if (tmp < SYSMON_LOWER_SATURATION)
>> + tmp = 0x0000;
>
> u16 respectively.
>
>> + }
>> +
>> + *raw_data = tmp & 0xffff;
>> +}
>
>
>> + u32 ret = -EINVAL;
>
>> + mutex_lock(&sysmon->mutex);
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + switch (chan->type) {
>> + case IIO_TEMP:
>> + offset = sysmon_temp_offset(chan->address);
>> + *val = sysmon->temp_read(sysmon, offset);
>> + *val2 = 0;
>> + ret = IIO_VAL_INT;
>> + break;
>> +
>> + case IIO_VOLTAGE:
>> + offset = sysmon_supply_offset(chan->address);
>> + sysmon_read_reg(sysmon, offset, ®val);
>> + *val = (int)regval;
>> + *val2 = 0;
>> + ret = IIO_VAL_INT;
>> + break;
>> +
>> + default:
>> + break;
>> + }> +
>> + spin_unlock_irqrestore(&sysmon->lock, flags);
>> + mutex_unlock(&sysmon->mutex);
>> +
>> + return 0;
>> +}
>
> ...
>
>> +#include <linux/bits.h>
>
> + io.h
>
>> +#include <linux/moduleparam.h>
>> +#include <linux/firmware/xlnx-zynqmp.h>
>> +#include "versal-sysmon.h"
>> +
>> +static LIST_HEAD(sysmon_list_head);
>
> list.h?
>
>> +static struct iio_map sysmon_therm_static_maps[] = {
>> + IIO_MAP("temp", "versal-thermal", "sysmon-temp-channel"),
>
> Where are the IIO_MAP() and struct iio_map defined?
>
>> + {}
>> +};
>> +
>> +static inline int sysmon_direct_read_reg(struct sysmon *sysmon, u32 offset, u32 *data)
>
> + types.h for uXX.
>
>> +{
>> + *data = readl(sysmon->base + offset);
>> +
>> + return 0;
>> +}
>
> ...
>
>> + mutex_init(&sysmon->mutex);
>
> devm?
>
> ...
>
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->dev.of_node = pdev->dev.of_node;
>
> Drop these, IIO core does the same and even better.
>
> ...
>
>> + mutex_lock(&sysmon->mutex);
>
> scoped_guard() from cleanup.h
>
>> + if (list_empty(&sysmon_list_head)) {
>> + sysmon->master_slr = true;
>
> No need, just make a sane default and !exist will work in the other
> branch the same way. I believe it's
>
> bool exist = false;
>
>> + } else {
>> + list_for_each_entry(temp_sysmon, &sysmon_list_head, list) {
>> + if (temp_sysmon->master_slr)
>> + exist = true;
>
> Why to continue?
>
>> + }
>> + sysmon->master_slr = !exist;
>> + }
>> +
>> + mutex_unlock(&sysmon->mutex);
>
> ...
>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/adc/versal-sysmon-events.h>
>> +#include <linux/iopoll.h>
>
>> +#include <linux/kernel.h>
>
> Mustn't be used in new drivers.
>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_address.h>
>
> Huh?! Please, make sure you are following IWYU principle, please
> *drop* all of the headers that are *not* being used (by this header),
> there are a LOT of them to be dropped.
>
> ...
>
>> +/* Channel IDs for Temp Channels */
>> +/* TEMP_MAX gives the current temperature for Production
>> + * silicon.
>> + * TEMP_MAX gives the current maximum temperature for ES1
>> + * silicon.
>> + */
>> +#define TEMP_MAX 160
>
> Here and everywhere else, bad namings.
>
> ...
>
> This is an unfinished review due to the following reasons (from more
> important to less):
> - AMD is not the first day contributor, the code is awful, it missed a
> lot of reviews and updates regarding the last several years of the
> kernel development (IIO subsystem in particular). Please, avoid
> letting reviewers do *your* job.
Sorry about this. Firstly we need to find a way to have DT binding correct which
affect code too.
> - The patches are too long to review, it would be nicer to make them
> feature-by-feature, where each one is < 1000 LoCs.
We will look at doing one more split. It is 2k LOC now. There is actually also
i2c part of this which is not included.
> - I started this a few days ago, but I'm busy at the moment, that's
> another reason.
Thanks for your review so far.
Thanks,
Michal
Powered by blists - more mailing lists