On 9/30/2024 11:40 AM, Arkadiusz Kubalewski wrote:
The E810 Lan On Motherboard (LOM) design is vendor specific. Intel
provides the reference design, but it is up to vendor on the final
product design. For some cases, like Linux DPLL support, the static
values defined in the driver does not reflect the actual LOM design.
Current implementation of output pins is causing the crash on probe
of the ice driver for such DPLL enabled E810 LOM designs:
WARNING: (...) at drivers/dpll/dpll_core.c:495 dpll_pin_get+0x2c4/0x330
...
Call Trace:
<TASK>
? __warn+0x83/0x130
? dpll_pin_get+0x2c4/0x330
? report_bug+0x1b7/0x1d0
? handle_bug+0x42/0x70
? exc_invalid_op+0x18/0x70
? asm_exc_invalid_op+0x1a/0x20
? dpll_pin_get+0x117/0x330
? dpll_pin_get+0x2c4/0x330
? dpll_pin_get+0x117/0x330
ice_dpll_get_pins.isra.0+0x52/0xe0 [ice]
...
The number of output pins enabled by LOM vendor is greater than expected
and defined in the driver for Intel designed NICs, which causes the crash.
Prevent the crash and allow generic output pin initialization within
Linux DPLL subsystem for DPLL enabled E810 LOM designs.
Newly designed solution for described issue will be based on "per HW
design" pin initialization. It requires pin information dynamically
acquired from the firmware and is already in progress, planned for
next-tree only.
Fixes: d7999f5ea64b ("ice: implement dpll interface to control cgu")
Reviewed-by: Karol Kolacinski <karol.kolacin...@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalew...@intel.com>
---
drivers/net/ethernet/intel/ice/ice_dpll.c | 44 +++++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 19 +++++++++
drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 +
3 files changed, 64 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 74c0e7319a4c..4bb4d74a7eb8 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -2063,6 +2063,48 @@ static int ice_dpll_init_worker(struct ice_pf *pf)
return 0;
}
+/**
+ * ice_dpll_init_info_output_pins_generic - initializes generic output pins
info
+ * @pf: board private structure
+ *
+ * Init information for generic output pins, cache them in PF's pins
structures.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - init failure reason
+ */
+static int ice_dpll_init_info_output_pins_generic(struct ice_pf *pf)
+{
+#define ICE_DPLL_GEN_OUT_NUM 16
+#define ICE_DPLL_GEN_OUT_LEN 3
inline defines are frowned upon. I'd put it above the function, at top
of file or .h file...
+ static const char labels[ICE_DPLL_GEN_OUT_NUM][ICE_DPLL_GEN_OUT_LEN] = {
+ "0", "1", "2", "3", "4", "5", "6", "7", "8",
+ "9", "10", "11", "12", "13", "14", "15" };
... however, could we declare these without the sizes and use array size
for size?
+ u32 cap = DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
+ struct ice_dpll_pin *pins = pf->dplls.outputs;
+ int i, ret;
smatch reports:
../drivers/net/ethernet/intel/ice/ice_dpll.c:2105
ice_dpll_init_info_output_pins_generic() error: uninitialized symbol 'ret'.
+
+ if (pf->dplls.num_outputs > ICE_DPLL_GEN_OUT_NUM)
+ return -EINVAL;
+ for (i = 0; i < pf->dplls.num_outputs; i++) {
+ pins[i].idx = i;
+ pins[i].prop.board_label = labels[i];
+ pins[i].prop.type = DPLL_PIN_TYPE_EXT;
+ pins[i].prop.phase_range.min =
+ pf->dplls.output_phase_adj_max;
+ pins[i].prop.phase_range.max =
+ -pf->dplls.output_phase_adj_max;
+ pins[i].prop.capabilities = cap;
+ pins[i].pf = pf;
+ ret = ice_dpll_pin_state_update(pf, &pins[i],
+ ICE_DPLL_PIN_TYPE_OUTPUT, NULL);
+ if (ret)
+ break;
+ }
+
+ return ret;
+}
+
/**
* ice_dpll_init_info_direct_pins - initializes direct pins info
* @pf: board private structure
@@ -2097,6 +2139,8 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
pins = pf->dplls.outputs;
num_pins = pf->dplls.num_outputs;
input = false;
+ if (num_pins != ice_cgu_get_pin_num(hw, input))
+ return ice_dpll_init_info_output_pins_generic(pf);
break;
default:
return -EINVAL;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 3a33e6b9b313..e4ab76496d3a 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -5964,6 +5964,25 @@ ice_cgu_get_pin_desc(struct ice_hw *hw, bool input, int
*size)
return t;
}
+/**
+ * ice_cgu_get_pin_desc - get pin description array size
missed updating kdoc from copy/paste...
+ * @hw: pointer to the hw struct
+ * @input: if request is done against input or output pins
+ *
+ * Return: size of pin description array for given hw.
+ */
+int ice_cgu_get_pin_num(struct ice_hw *hw, bool input)
... though I wonder if ice_cgu_get_num_pins() would be a better name?
Thanks,
Tony
+{
+ const struct ice_cgu_pin_desc *t;
+ int size;
+
+ t = ice_cgu_get_pin_desc(hw, input, &size);
+ if (t)
+ return size;
+
+ return 0;
+}
+
/**
* ice_cgu_get_pin_type - get pin's type
* @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 0852a34ade91..f28cbae924dd 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -404,6 +404,7 @@ int ice_read_sma_ctrl_e810t(struct ice_hw *hw, u8 *data);
int ice_write_sma_ctrl_e810t(struct ice_hw *hw, u8 data);
int ice_read_pca9575_reg_e810t(struct ice_hw *hw, u8 offset, u8 *data);
bool ice_is_pca9575_present(struct ice_hw *hw);
+int ice_cgu_get_pin_num(struct ice_hw *hw, bool input);
enum dpll_pin_type ice_cgu_get_pin_type(struct ice_hw *hw, u8 pin, bool
input);
struct dpll_pin_frequency *
ice_cgu_get_pin_freq_supp(struct ice_hw *hw, u8 pin, bool input, u8 *num);
