Hi Guenter,
On 27/07/2023 22:54, Guenter Roeck wrote:
> On 7/27/23 11:59, Saeed Mahameed wrote:
>> From: Adham Faris <afaris@xxxxxxxxxx>
>> $ grep -H -d skip . /sys/class/hwmon/hwmon0/*
>>
>> Output
>> =======================================================================
>> /sys/class/hwmon/hwmon0/name:0000:08:00.0
>
> That name doesn't seem to be very useful. You might want to consider
> using a different name, such as a simple "mlx5". Since the parent is
> a pci device, the "sensors" command would translate that into something
> like "mlx5-pci-XXXX" which would be much more useful than the
> "0000:08:00.0-pci-0000" which is what you'll see with the current
> name.
>
>> /sys/class/hwmon/hwmon0/temp1_crit:105000
>> /sys/class/hwmon/hwmon0/temp1_highest:68000
>> /sys/class/hwmon/hwmon0/temp1_input:68000
>> /sys/class/hwmon/hwmon0/temp1_label:sensor0
>
> I don't really see the value of that label. A label provided by the driver
> should be meaningful and indicate something such as the sensor location.
> Otherwise the default of "temp1" seems to be just as useful to me.
Agree, will change.
>> +int mlx5_hwmon_dev_register(struct mlx5_core_dev *mdev)
>> +{
>> + struct device *dev = mdev->device;
>> + struct mlx5_hwmon *hwmon;
>> + int err;
>> +
>> + if (!MLX5_CAP_MCAM_REG(mdev, mtmp))
>> + return 0;
>> +
>> + hwmon = mlx5_hwmon_alloc(mdev);
>> + if (IS_ERR(hwmon))
>> + return PTR_ERR(hwmon);
>> +
>> + err = mlx5_hwmon_dev_init(hwmon);
>> + if (err)
>> + goto err_free_hwmon;
>> +
>> + hwmon->hwmon_dev = hwmon_device_register_with_info(dev, hwmon->name,
>> + hwmon,
>> + &hwmon->chip,
>> + NULL);
>> + if (IS_ERR(hwmon->hwmon_dev)) {
>> + err = PTR_ERR(hwmon->hwmon_dev);
>> + goto err_free_hwmon;
>> + }
>> +
>> + mdev->hwmon = hwmon;
>> + return 0;
>> +
>> +err_free_hwmon:
>> + mlx5_hwmon_free(hwmon);
>> + return err;
>> +}
>> +
>
> At first glance it seems to me that the hwmon device lifetime matches
> the lifetime of the pci device. If so, it would be much easier and safe
> to use devm_ functions and to tie unregistration to pci device removal.
> Is there a reason for not doing that ?
You're right, but devm_ interface isn't used in anywhere mlx5, we'd
rather not mix it just for hwmon.
>
> Thanks,
> Guenter
>
>> +void mlx5_hwmon_dev_unregister(struct mlx5_core_dev *mdev)
>> +{
>> + struct mlx5_hwmon *hwmon = mdev->hwmon;
>> +
>> + if (!hwmon)
>> + return;
>> +
>> + hwmon_device_unregister(hwmon->hwmon_dev);
>> + mlx5_hwmon_free(hwmon);
>> + mdev->hwmon = NULL;
>> +}
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/hwmon.h
>> b/drivers/net/ethernet/mellanox/mlx5/core/hwmon.h
>> new file mode 100644
>> index 000000000000..999654a9b9da
>> --- /dev/null
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/hwmon.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
>> reserved
>> + */
>> +#ifndef __MLX5_HWMON_H__
>> +#define __MLX5_HWMON_H__
>> +
>> +#include <linux/mlx5/driver.h>
>> +
>> +#if IS_ENABLED(CONFIG_HWMON)
>
> This may need IS_REACHABLE() - unless I am missing something, it is
> possible to configure the mlx5 core with =y even if hwmon is built
> as module. An alternative would be to add a dependency such as
> depends on HWMON || HWMON=n
> to "config MLX5_CORE".
Ack.
