>From: Nguyen, Anthony L <anthony.l.ngu...@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.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...
>
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);
