>From: Michal Schmidt <mschm...@redhat.com>
>Sent: Wednesday, October 9, 2024 12:21 PM
>
>On Thu, Oct 3, 2024 at 11:26 AM Arkadiusz Kubalewski
><arkadiusz.kubalew...@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.kolacin...@intel.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalew...@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
