Message ID | 20240930184030.897689-1-arkadiusz.kubalewski@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [iwl-net,v1] ice: fix crash on probe for DPLL enabled E810 LOM | expand |
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.kolacinski@intel.com> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@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);
>From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> >Sent: Thursday, October 3, 2024 12:18 AM > >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.kolacinski@intel.com> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@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... > Sure, fixed in v2. >> + 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? > I have removed first dimension and used sizeof later. >> + 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'. > Fixed in v2. > >> + >> + 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... Fixed in v2. > >> + * @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 > All fixed in v2 :) Thank you! Arkadiusz >> +{ >> + 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);
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 + 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" }; + u32 cap = DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE; + struct ice_dpll_pin *pins = pf->dplls.outputs; + int i, 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 + * @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) +{ + 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);