On Thu, Oct 3, 2024 at 11:26 AM Arkadiusz Kubalewski
<[email protected]> 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 <[email protected]>
> Signed-off-by: Arkadiusz Kubalewski <[email protected]>
> ---
> 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
