| 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: <9080a6ca-0fbe-44b4-bbd0-43c572378532@intel.com>
Date: Thu, 12 Oct 2023 11:56:01 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Guenter Roeck <linux@...ck-us.net>, 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 10/12/2023 10:29 AM, Guenter Roeck wrote:
> 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 ?
>
ice_hwmon.h file needs to check CONFIG_HWMON and provide no-op stubs, and...
>> 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 \
This should be ice-$(CONFIG_HWMON) += 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.
>
The layout is all u8s which pack correctly without using needing pack. I
think in principle it probably could use __packed to clarify the intent,
but I think the layout is the same regardless in this case.
>> +
>> 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.
I think we typically do align with either spaces or tabs when using
multiple definitions.
>
>> +#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.
Depending on how unexpected this is and how common its printed, I would
make it either dev_warn_ratelimited or dev_dbg.
Powered by blists - more mailing lists