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);