| Message ID | 20241003092113.912768-1-arkadiusz.kubalewski@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl-net,v2] ice: fix crash on probe for DPLL enabled E810 LOM | expand |
On Thu, Oct 3, 2024 at 11:26 AM Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> 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.kolacinski@intel.com> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > --- > v2: > - put define on top of the file > - fix smatch 'ret' uninitialized > - do not init first array dimmension, use sizeof to obtain array size > - raname ice_cgu_get_pin_num(..) -> ice_cgu_get_num_pins(..) > - fix kdoc typo > --- > drivers/net/ethernet/intel/ice/ice_dpll.c | 43 +++++++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 19 +++++++++ > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + > 3 files changed, 63 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c > index 74c0e7319a4c..e9966c6c7c0f 100644 > --- a/drivers/net/ethernet/intel/ice/ice_dpll.c > +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c > @@ -10,6 +10,7 @@ > #define ICE_DPLL_PIN_IDX_INVALID 0xff > #define ICE_DPLL_RCLK_NUM_PER_PF 1 > #define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25 > +#define ICE_DPLL_GEN_OUT_LEN 3 > > /** > * enum ice_dpll_pin_type - enumerate ice pin types: > @@ -2063,6 +2064,46 @@ 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) > +{ > + static const char labels[][ICE_DPLL_GEN_OUT_LEN] = { > + "0", "1", "2", "3", "4", "5", "6", "7", "8", > + "9", "10", "11", "12", "13", "14", "15" }; > + u32 cap = DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE; > + struct ice_dpll_pin *pins = pf->dplls.outputs; > + int i, ret = -EINVAL; > + > + if (pf->dplls.num_outputs > sizeof(labels) / ICE_DPLL_GEN_OUT_LEN) Just use ARRAY_SIZE(labels). I believe that's what Tony meant in his review of v1. Once you have that, you don't need to define ICE_DPLL_GEN_OUT_LEN at all. Just declare labels as: static const char labels[][3] = ... or, if you like this more: static const char labels[][sizeof("99")] = ... Michal
>From: Michal Schmidt <mschmidt@redhat.com> >Sent: Wednesday, October 9, 2024 12:21 PM > >On Thu, Oct 3, 2024 at 11:26 AM Arkadiusz Kubalewski ><arkadiusz.kubalewski@intel.com> 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.kolacinski@intel.com> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> --- >> v2: >> - put define on top of the file >> - fix smatch 'ret' uninitialized >> - do not init first array dimmension, use sizeof to obtain array size >> - raname ice_cgu_get_pin_num(..) -> ice_cgu_get_num_pins(..) >> - fix kdoc typo >> --- >> drivers/net/ethernet/intel/ice/ice_dpll.c | 43 +++++++++++++++++++++ >> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 19 +++++++++ >> drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + >> 3 files changed, 63 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c >> b/drivers/net/ethernet/intel/ice/ice_dpll.c >> index 74c0e7319a4c..e9966c6c7c0f 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c >> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c >> @@ -10,6 +10,7 @@ >> #define ICE_DPLL_PIN_IDX_INVALID 0xff >> #define ICE_DPLL_RCLK_NUM_PER_PF 1 >> #define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25 >> +#define ICE_DPLL_GEN_OUT_LEN 3 >> >> /** >> * enum ice_dpll_pin_type - enumerate ice pin types: >> @@ -2063,6 +2064,46 @@ 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) >> +{ >> + static const char labels[][ICE_DPLL_GEN_OUT_LEN] = { >> + "0", "1", "2", "3", "4", "5", "6", "7", "8", >> + "9", "10", "11", "12", "13", "14", "15" }; >> + u32 cap = DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE; >> + struct ice_dpll_pin *pins = pf->dplls.outputs; >> + int i, ret = -EINVAL; >> + >> + if (pf->dplls.num_outputs > sizeof(labels) / ICE_DPLL_GEN_OUT_LEN) > >Just use ARRAY_SIZE(labels). I believe that's what Tony meant in his >review of v1. >Once you have that, you don't need to define ICE_DPLL_GEN_OUT_LEN at all. >Just declare labels as: > static const char labels[][3] = ... >or, if you like this more: > static const char labels[][sizeof("99")] = ... > >Michal Hi Michal, Ok, sure, makes sense, will do in v3. Also, an update to the list: Got some feedback that it still brakes for some different LOM E810 designs then the one I have developed on. Thus v3 will also include similar fix for inputs pins. Thank you! Arkadiusz
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c index 74c0e7319a4c..e9966c6c7c0f 100644 --- a/drivers/net/ethernet/intel/ice/ice_dpll.c +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c @@ -10,6 +10,7 @@ #define ICE_DPLL_PIN_IDX_INVALID 0xff #define ICE_DPLL_RCLK_NUM_PER_PF 1 #define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25 +#define ICE_DPLL_GEN_OUT_LEN 3 /** * enum ice_dpll_pin_type - enumerate ice pin types: @@ -2063,6 +2064,46 @@ 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) +{ + static const char labels[][ICE_DPLL_GEN_OUT_LEN] = { + "0", "1", "2", "3", "4", "5", "6", "7", "8", + "9", "10", "11", "12", "13", "14", "15" }; + u32 cap = DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE; + struct ice_dpll_pin *pins = pf->dplls.outputs; + int i, ret = -EINVAL; + + if (pf->dplls.num_outputs > sizeof(labels) / ICE_DPLL_GEN_OUT_LEN) + return ret; + 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 +2138,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_num_pins(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..5d151daa9728 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_num_pins - get pin description array size + * @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_num_pins(struct ice_hw *hw, bool input) +{ + 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..6cedc1a906af 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_num_pins(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);
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.kolacinski@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- v2: - put define on top of the file - fix smatch 'ret' uninitialized - do not init first array dimmension, use sizeof to obtain array size - raname ice_cgu_get_pin_num(..) -> ice_cgu_get_num_pins(..) - fix kdoc typo --- drivers/net/ethernet/intel/ice/ice_dpll.c | 43 +++++++++++++++++++++ drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 19 +++++++++ drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + 3 files changed, 63 insertions(+)