| 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: <df0e5774-3db7-4f0f-a9e8-c4369c2e6083@roeck-us.net>
Date: Thu, 12 Oct 2023 10:29:51 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Konrad Knitter <konrad.knitter@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
jdelvare@...e.com, Marcin Domagala <marcinx.domagala@...el.com>,
Eric Joyner <eric.joyner@...el.com>,
Marcin Szycik <marcin.szycik@...ux.intel.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-next v3] ice: read internal temperature sensor
On Thu, Oct 12, 2023 at 09:13:59AM +0200, Konrad Knitter wrote:
> Since 4.30 firmware exposes internal thermal sensor reading via admin
> queue commands. Expose those readouts via hwmon API when supported.
>
> Driver provides current reading from HW as well as device specific
> thresholds for thermal alarm (Warning, Critical, Fatal) events.
>
> $ sensors
>
> Output
> =========================================================
> ice-pci-b100
> Adapter: PCI adapter
> temp1: +62.0°C (high = +95.0°C, crit = +105.0°C)
> (emerg = +115.0°C)
>
> Co-developed-by: Marcin Domagala <marcinx.domagala@...el.com>
> Signed-off-by: Marcin Domagala <marcinx.domagala@...el.com>
> Co-developed-by: Eric Joyner <eric.joyner@...el.com>
> Signed-off-by: Eric Joyner <eric.joyner@...el.com>
> Reviewed-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Signed-off-by: Konrad Knitter <konrad.knitter@...el.com>
> ---
> v3: add SPDX identification to ice_hwmon files
> v2: fix formmating issues, added hwmon maintainers to Cc
> ---
> drivers/net/ethernet/intel/ice/Makefile | 1 +
The code seems to be unconditional, but I see no added
dependency on CONFIG_HWMON. Does this compile if
HWMON=m and this code is built into the kernel, or if HWMON=n ?
> drivers/net/ethernet/intel/ice/ice.h | 1 +
> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 28 ++++
> drivers/net/ethernet/intel/ice/ice_common.c | 57 +++++++-
> drivers/net/ethernet/intel/ice/ice_common.h | 2 +
> drivers/net/ethernet/intel/ice/ice_hwmon.c | 130 ++++++++++++++++++
> drivers/net/ethernet/intel/ice/ice_hwmon.h | 10 ++
> drivers/net/ethernet/intel/ice/ice_main.c | 5 +
> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
> 9 files changed, 237 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/ethernet/intel/ice/ice_hwmon.c
> create mode 100644 drivers/net/ethernet/intel/ice/ice_hwmon.h
>
> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> index 8757bec23fb3..b4c8f5303e57 100644
> --- a/drivers/net/ethernet/intel/ice/Makefile
> +++ b/drivers/net/ethernet/intel/ice/Makefile
> @@ -36,6 +36,7 @@ ice-y := ice_main.o \
> ice_repr.o \
> ice_tc_lib.o \
> ice_fwlog.o \
> + ice_hwmon.o \
> ice_debugfs.o
> ice-$(CONFIG_PCI_IOV) += \
> ice_sriov.o \
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index ad5614d4449c..61d26be502b2 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -650,6 +650,7 @@ struct ice_pf {
> #define ICE_MAX_VF_AGG_NODES 32
> struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
> struct ice_dplls dplls;
> + struct device *hwmon_dev;
> };
>
> extern struct workqueue_struct *ice_lag_wq;
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index 1202abfb9eb3..3c4295f8e4ba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -117,6 +117,7 @@ struct ice_aqc_list_caps_elem {
> #define ICE_AQC_CAPS_NET_VER 0x004C
> #define ICE_AQC_CAPS_PENDING_NET_VER 0x004D
> #define ICE_AQC_CAPS_RDMA 0x0051
> +#define ICE_AQC_CAPS_SENSOR_READING 0x0067
> #define ICE_AQC_CAPS_PCIE_RESET_AVOIDANCE 0x0076
> #define ICE_AQC_CAPS_POST_UPDATE_RESET_RESTRICT 0x0077
> #define ICE_AQC_CAPS_NVM_MGMT 0x0080
> @@ -1393,6 +1394,30 @@ struct ice_aqc_get_phy_rec_clk_out {
> __le16 node_handle;
> };
>
> +/* Get sensor reading (direct 0x0632) */
> +struct ice_aqc_get_sensor_reading {
> + u8 sensor;
> + u8 format;
> + u8 reserved[6];
> + __le32 addr_high;
> + __le32 addr_low;
> +};
> +
> +/* Get sensor reading response (direct 0x0632) */
> +struct ice_aqc_get_sensor_reading_resp {
> + union {
> + u8 raw[8];
> + /* Output data for sensor 0x00, format 0x00 */
> + struct {
> + s8 temp;
> + u8 temp_warning_threshold;
> + u8 temp_critical_threshold;
> + u8 temp_fatal_threshold;
> + u8 reserved[4];
> + } s0f0;
> + } data;
> +};
Kind of surprising that this doesn't need packed attributes.
> +
> struct ice_aqc_link_topo_params {
> u8 lport_num;
> u8 lport_num_valid;
> @@ -2438,6 +2463,8 @@ struct ice_aq_desc {
> struct ice_aqc_restart_an restart_an;
> struct ice_aqc_set_phy_rec_clk_out set_phy_rec_clk_out;
> struct ice_aqc_get_phy_rec_clk_out get_phy_rec_clk_out;
> + struct ice_aqc_get_sensor_reading get_sensor_reading;
> + struct ice_aqc_get_sensor_reading_resp get_sensor_reading_resp;
> struct ice_aqc_gpio read_write_gpio;
> struct ice_aqc_sff_eeprom read_write_sff_param;
> struct ice_aqc_set_port_id_led set_port_id_led;
> @@ -2617,6 +2644,7 @@ enum ice_adminq_opc {
> ice_aqc_opc_set_mac_lb = 0x0620,
> ice_aqc_opc_set_phy_rec_clk_out = 0x0630,
> ice_aqc_opc_get_phy_rec_clk_out = 0x0631,
> + ice_aqc_opc_get_sensor_reading = 0x0632,
> ice_aqc_opc_get_link_topo = 0x06E0,
> ice_aqc_opc_read_i2c = 0x06E2,
> ice_aqc_opc_write_i2c = 0x06E3,
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 283492314215..e566485a01b2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -2462,6 +2462,26 @@ ice_parse_fdir_dev_caps(struct ice_hw *hw, struct ice_hw_dev_caps *dev_p,
> dev_p->num_flow_director_fltr);
> }
>
> +/**
> + * ice_parse_sensor_reading_cap - Parse ICE_AQC_CAPS_SENSOR_READING cap
> + * @hw: pointer to the HW struct
> + * @dev_p: pointer to device capabilities structure
> + * @cap: capability element to parse
> + *
> + * Parse ICE_AQC_CAPS_SENSOR_READING for device capability for reading
> + * enabled sensors.
> + */
> +static void
> +ice_parse_sensor_reading_cap(struct ice_hw *hw, struct ice_hw_dev_caps *dev_p,
> + struct ice_aqc_list_caps_elem *cap)
> +{
> + dev_p->supported_sensors = le32_to_cpu(cap->number);
> +
> + ice_debug(hw, ICE_DBG_INIT,
> + "dev caps: supported sensors (bitmap) = 0x%x\n",
> + dev_p->supported_sensors);
> +}
> +
> /**
> * ice_parse_dev_caps - Parse device capabilities
> * @hw: pointer to the HW struct
> @@ -2507,9 +2527,12 @@ ice_parse_dev_caps(struct ice_hw *hw, struct ice_hw_dev_caps *dev_p,
> case ICE_AQC_CAPS_1588:
> ice_parse_1588_dev_caps(hw, dev_p, &cap_resp[i]);
> break;
> - case ICE_AQC_CAPS_FD:
> + case ICE_AQC_CAPS_FD:
> ice_parse_fdir_dev_caps(hw, dev_p, &cap_resp[i]);
> break;
> + case ICE_AQC_CAPS_SENSOR_READING:
> + ice_parse_sensor_reading_cap(hw, dev_p, &cap_resp[i]);
> + break;
> default:
> /* Don't list common capabilities as unknown */
> if (!found)
> @@ -5292,6 +5315,38 @@ ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 *phy_output, u8 *port_num,
> return status;
> }
>
> +/**
> + * ice_aq_get_sensor_reading
> + * @hw: pointer to the HW struct
> + * @sensor: sensor type
> + * @format: requested response format
> + * @data: pointer to data to be read from the sensor
> + *
> + * Get sensor reading (0x0632)
> + */
> +int ice_aq_get_sensor_reading(struct ice_hw *hw, u8 sensor, u8 format,
> + struct ice_aqc_get_sensor_reading_resp *data)
Are "sensor" and "format" ever going to be != 0 ? If not,
those parameters are just noise.
> +{
> + struct ice_aqc_get_sensor_reading *cmd;
> + struct ice_aq_desc desc;
> + int status;
> +
> + if (!data)
> + return -EINVAL;
This is never called with a NULL pointer. The check is pointless.
> +
> + ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_sensor_reading);
> + cmd = &desc.params.get_sensor_reading;
> + cmd->sensor = sensor;
> + cmd->format = format;
> +
> + status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
> + if (!status)
> + memcpy(data, &desc.params.get_sensor_reading_resp,
> + sizeof(*data));
> +
> + return status;
> +}
> +
> /**
> * ice_replay_pre_init - replay pre initialization
> * @hw: pointer to the HW struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
> index 4a75c0c89301..e23787c17505 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -240,6 +240,8 @@ ice_aq_set_phy_rec_clk_out(struct ice_hw *hw, u8 phy_output, bool enable,
> int
> ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 *phy_output, u8 *port_num,
> u8 *flags, u16 *node_handle);
> +int ice_aq_get_sensor_reading(struct ice_hw *hw, u8 sensor, u8 format,
> + struct ice_aqc_get_sensor_reading_resp *data);
> void
> ice_stat_update40(struct ice_hw *hw, u32 reg, bool prev_stat_loaded,
> u64 *prev_stat, u64 *cur_stat);
> diff --git a/drivers/net/ethernet/intel/ice/ice_hwmon.c b/drivers/net/ethernet/intel/ice/ice_hwmon.c
> new file mode 100644
> index 000000000000..6b23ae27169c
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_hwmon.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2022, Intel Corporation. */
> +
> +#include "ice.h"
> +#include "ice_hwmon.h"
> +#include "ice_adminq_cmd.h"
> +
> +#include <linux/hwmon.h>
> +
> +#define ICE_INTERNAL_TEMP_SENSOR 0
> +#define ICE_INTERNAL_TEMP_SENSOR_FORMAT 0
> +
Personally I very much prefer
#define<space>NAME<tab>value
but obviously that is a maintainer decision to make.
> +#define TEMP_FROM_REG(reg) ((reg) * 1000)
> +
> +static const struct hwmon_channel_info *ice_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_MAX |
> + HWMON_T_CRIT | HWMON_T_EMERGENCY),
> + NULL
> +};
> +
> +static int ice_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct ice_aqc_get_sensor_reading_resp resp;
> + struct ice_pf *pf = dev_get_drvdata(dev);
> + int ret;
> +
> + if (type != hwmon_temp)
> + return -EOPNOTSUPP;
> +
> + ret = ice_aq_get_sensor_reading(&pf->hw,
> + ICE_INTERNAL_TEMP_SENSOR,
> + ICE_INTERNAL_TEMP_SENSOR_FORMAT,
> + &resp);
> + if (ret) {
> + dev_warn(dev, "%s HW read failure (%d)\n", __func__, ret);
Up to maintainers to decide, but I do not support error messages
as result of normal operation because it may end up clogging
the log if the underlying HW has a problem.
> + return ret;
> + }
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + *val = TEMP_FROM_REG(resp.data.s0f0.temp);
> + break;
> + case hwmon_temp_max:
> + *val = TEMP_FROM_REG(resp.data.s0f0.temp_warning_threshold);
> + break;
> + case hwmon_temp_crit:
> + *val = TEMP_FROM_REG(resp.data.s0f0.temp_critical_threshold);
> + break;
> + case hwmon_temp_emergency:
> + *val = TEMP_FROM_REG(resp.data.s0f0.temp_fatal_threshold);
> + break;
> + default:
> + dev_warn(dev, "%s unsupported attribute (%d)\n",
> + __func__, attr);
Same here, especially since this won't ever happen and the code
just exists to make the compiler happy.
> + return -EINVAL;
Should be -EOPNOTSUPP.
> + }
> +
> + return 0;
> +}
> +
> +static umode_t ice_hwmon_is_visible(const void *data,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + if (type != hwmon_temp)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + case hwmon_temp_crit:
> + case hwmon_temp_max:
> + case hwmon_temp_emergency:
> + return 0444;
> + }
> +
> + return 0;
> +}
> +
> +static const struct hwmon_ops ice_hwmon_ops = {
> + .is_visible = ice_hwmon_is_visible,
> + .read = ice_hwmon_read
> +};
> +
> +static const struct hwmon_chip_info ice_chip_info = {
> + .ops = &ice_hwmon_ops,
> + .info = ice_hwmon_info
> +};
> +
> +static bool ice_is_internal_reading_supported(struct ice_pf *pf)
> +{
> + if (pf->hw.pf_id)
> + return false;
> +
This should be explained. From the rest of the code, it appears
that pf_id==0 reflects the first "pf". Why should this device not register
a hwmon device, and / or why is pf->hw.dev_caps.supported_sensors
unsupported or not set correctly for it ?
> + unsigned long sensors = pf->hw.dev_caps.supported_sensors;
> +
> + if (!_test_bit(ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT, &sensors))
> + return false;
> +
> + return true;
return _test_bit(ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT, &sensors);
would do the same.
> +};
> +
> +void ice_hwmon_init(struct ice_pf *pf)
> +{
> + struct device *dev = ice_pf_to_dev(pf);
> + struct device *hdev;
> +
> + if (!ice_is_internal_reading_supported(pf))
> + return;
> +
> + hdev = hwmon_device_register_with_info(dev, "ice", pf, &ice_chip_info,
> + NULL);
> + if (IS_ERR(hdev)) {
> + dev_warn(dev,
> + "hwmon_device_register_with_info returns error (%ld)",
> + PTR_ERR(hdev));
> + return;
> + }
> + pf->hwmon_dev = hdev;
> +}
> +
> +void ice_hwmon_exit(struct ice_pf *pf)
> +{
> + if (!ice_is_internal_reading_supported(pf))
> + return;
In this case hwmon_dev would be NULL, making the above check unnecessary.
> + if (!pf->hwmon_dev)
> + return;
> + hwmon_device_unregister(pf->hwmon_dev);
> +}
> diff --git a/drivers/net/ethernet/intel/ice/ice_hwmon.h b/drivers/net/ethernet/intel/ice/ice_hwmon.h
> new file mode 100644
> index 000000000000..8c74d19933d7
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_hwmon.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2022, Intel Corporation. */
> +
> +#ifndef _ICE_HWMON_H_
> +#define _ICE_HWMON_H_
> +
> +void ice_hwmon_init(struct ice_pf *pf);
> +void ice_hwmon_exit(struct ice_pf *pf);
> +
> +#endif /* _ICE_HWMON_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index afe19219a640..8f7e901a6c95 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -14,6 +14,7 @@
> #include "ice_dcb_lib.h"
> #include "ice_dcb_nl.h"
> #include "ice_devlink.h"
> +#include "ice_hwmon.h"
> /* Including ice_trace.h with CREATE_TRACE_POINTS defined will generate the
> * ice tracepoint functions. This must be done exactly once across the
> * ice driver.
> @@ -4785,6 +4786,8 @@ static void ice_init_features(struct ice_pf *pf)
>
> if (ice_init_lag(pf))
> dev_warn(dev, "Failed to init link aggregation support\n");
> +
> + ice_hwmon_init(pf);
> }
>
> static void ice_deinit_features(struct ice_pf *pf)
> @@ -5310,6 +5313,8 @@ static void ice_remove(struct pci_dev *pdev)
> ice_free_vfs(pf);
> }
>
> + ice_hwmon_exit(pf);
> +
> ice_service_task_stop(pf);
> ice_aq_cancel_waiting_tasks(pf);
> set_bit(ICE_DOWN, pf->state);
> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
> index 877a92099ef0..0b5425d33adf 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -378,6 +378,8 @@ struct ice_hw_func_caps {
> struct ice_ts_func_info ts_func_info;
> };
>
> +#define ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT 0
> +
> /* Device wide capabilities */
> struct ice_hw_dev_caps {
> struct ice_hw_common_caps common_cap;
> @@ -386,6 +388,8 @@ struct ice_hw_dev_caps {
> u32 num_flow_director_fltr; /* Number of FD filters available */
> struct ice_ts_dev_info ts_dev_info;
> u32 num_funcs;
> + /* bitmap of supported sensors */
> + u32 supported_sensors;
> };
>
> /* MAC info */
>
> base-commit: 2318d58f358e7aef726c038aff87a68bec8f09e0
> --
> 2.35.3
>
Powered by blists - more mailing lists