netdev - Re: [PATCH net-next 2/2] net/mlx5: Expose NIC temperature via hardware monitoring kernel API

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 2 Aug 2023 15:10:49 +0300
From: Gal Pressman <gal@...dia.com>
To: Guenter Roeck <linux@...ck-us.net>, Saeed Mahameed <saeed@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>
Cc: Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org,
 Tariq Toukan <tariqt@...dia.com>, linux-hwmon@...r.kernel.org,
 Jean Delvare <jdelvare@...e.com>, Adham Faris <afaris@...dia.com>
Subject: Re: [PATCH net-next 2/2] net/mlx5: Expose NIC temperature via
 hardware monitoring kernel API

Hi Guenter,

On 27/07/2023 22:54, Guenter Roeck wrote:
> On 7/27/23 11:59, Saeed Mahameed wrote:
>> From: Adham Faris <afaris@...dia.com>
>> $ 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.