| Message ID | 20241206193542.4121545-5-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix E825 initialization | expand |
On 12/6/24 20:35, Tony Nguyen wrote: > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c > index 496d86cbd13f..ab25ccd7e8ec 100644 > --- a/drivers/net/ethernet/intel/ice/ice_common.c > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > @@ -4095,6 +4095,51 @@ ice_aq_set_port_option(struct ice_hw *hw, u8 lport, u8 lport_valid, > return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL); > } > > +/** > + * ice_get_phy_lane_number - Get PHY lane number for current adapter > + * @hw: pointer to the hw struct > + * > + * Return: PHY lane number on success, negative error code otherwise. > + */ > +int ice_get_phy_lane_number(struct ice_hw *hw) > +{ > + struct ice_aqc_get_port_options_elem *options __free(kfree); Please avoid the __free() construct: https://elixir.bootlin.com/linux/v6.13-rc2/source/Documentation/process/maintainer-netdev.rst#L393 Thanks, Paolo
On 12/10/24 15:54, Paolo Abeni wrote: > On 12/6/24 20:35, Tony Nguyen wrote: >> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c >> index 496d86cbd13f..ab25ccd7e8ec 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_common.c >> +++ b/drivers/net/ethernet/intel/ice/ice_common.c >> @@ -4095,6 +4095,51 @@ ice_aq_set_port_option(struct ice_hw *hw, u8 lport, u8 lport_valid, >> return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL); >> } >> >> +/** >> + * ice_get_phy_lane_number - Get PHY lane number for current adapter >> + * @hw: pointer to the hw struct >> + * >> + * Return: PHY lane number on success, negative error code otherwise. >> + */ >> +int ice_get_phy_lane_number(struct ice_hw *hw) >> +{ >> + struct ice_aqc_get_port_options_elem *options __free(kfree); > > Please avoid the __free() construct: > > https://elixir.bootlin.com/linux/v6.13-rc2/source/Documentation/process/maintainer-netdev.rst#L393 My understanding was that conversions to __free() (w/o any other reason) are bad. But for new code, it's fine. I get the "discourage" part from the doc, as "don't use __free() by default, for all pointers, or all allocations, but apply your judgement and sanity to tell if that makes given function better". I still believe that this function is better with __free(). We develop (new code) with such assumptions for the better part of the year. I think that static analysis tools/Reviewers already got used to that (after the first false-positive memleak reported). Developers (and Reviewers for sure) know that those pointers could not be left uninitialized at function return. The only concern that is unresolved for me yet, is: "there is a lot of characters to type", but that is also good in some way, as one needs bigger function to justify the added "complexity". > > Thanks, > > Paolo >
On 12/11/24 11:36, Przemek Kitszel wrote: > On 12/10/24 15:54, Paolo Abeni wrote: >> On 12/6/24 20:35, Tony Nguyen wrote: >>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/ >>> net/ethernet/intel/ice/ice_common.c >>> index 496d86cbd13f..ab25ccd7e8ec 100644 >>> --- a/drivers/net/ethernet/intel/ice/ice_common.c >>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c >>> @@ -4095,6 +4095,51 @@ ice_aq_set_port_option(struct ice_hw *hw, u8 >>> lport, u8 lport_valid, >>> return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL); >>> } >>> +/** >>> + * ice_get_phy_lane_number - Get PHY lane number for current adapter >>> + * @hw: pointer to the hw struct >>> + * >>> + * Return: PHY lane number on success, negative error code otherwise. >>> + */ >>> +int ice_get_phy_lane_number(struct ice_hw *hw) >>> +{ >>> + struct ice_aqc_get_port_options_elem *options __free(kfree); >> >> Please avoid the __free() construct: >> >> https://elixir.bootlin.com/linux/v6.13-rc2/source/Documentation/ >> process/maintainer-netdev.rst#L393 > > My understanding was that conversions to __free() (w/o any other reason) > are bad. But for new code, it's fine. I get the "discourage" part from > the doc, as "don't use __free() by default, for all pointers, or all > allocations, but apply your judgement and sanity to tell if that makes > given function better". > I still believe that this function is better with __free(). We develop > (new code) with such assumptions for the better part of the year. > > I think that static analysis tools/Reviewers already got used to that > (after the first false-positive memleak reported). Developers (and > Reviewers for sure) know that those pointers could not be left > uninitialized at function return. The only concern that is unresolved > for me yet, is: "there is a lot of characters to type", but that is also > good in some way, as one needs bigger function to justify the added > "complexity". > >> >> Thanks, >> >> Paolo >> > Paolo, could you please revisit this? In short I propose: let the developer and reviewers to decide if __free() makes given code better, instead of an umbrella "just no" statement. I believe that Jiri would be happy too, to back it up as another HW Vendor. https://lore.kernel.org/lkml/ZfW9lVhnClqr9Han@nanopsycho/T/
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h index 1489a8ceec51..4d27cf309646 100644 --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h @@ -1665,6 +1665,7 @@ struct ice_aqc_get_port_options_elem { #define ICE_AQC_PORT_OPT_MAX_LANE_25G 5 #define ICE_AQC_PORT_OPT_MAX_LANE_50G 6 #define ICE_AQC_PORT_OPT_MAX_LANE_100G 7 +#define ICE_AQC_PORT_OPT_MAX_LANE_200G 8 u8 global_scid[2]; u8 phy_scid[2]; diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 496d86cbd13f..ab25ccd7e8ec 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -4095,6 +4095,51 @@ ice_aq_set_port_option(struct ice_hw *hw, u8 lport, u8 lport_valid, return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL); } +/** + * ice_get_phy_lane_number - Get PHY lane number for current adapter + * @hw: pointer to the hw struct + * + * Return: PHY lane number on success, negative error code otherwise. + */ +int ice_get_phy_lane_number(struct ice_hw *hw) +{ + struct ice_aqc_get_port_options_elem *options __free(kfree); + unsigned int lport = 0; + unsigned int lane; + int err; + + options = kcalloc(ICE_AQC_PORT_OPT_MAX, sizeof(*options), GFP_KERNEL); + if (!options) + return -ENOMEM; + + for (lane = 0; lane < ICE_MAX_PORT_PER_PCI_DEV; lane++) { + u8 options_count = ICE_AQC_PORT_OPT_MAX; + u8 speed, active_idx, pending_idx; + bool active_valid, pending_valid; + + err = ice_aq_get_port_options(hw, options, &options_count, lane, + true, &active_idx, &active_valid, + &pending_idx, &pending_valid); + if (err) + return err; + + if (!active_valid) + continue; + + speed = options[active_idx].max_lane_speed; + /* If we don't get speed for this lane, it's unoccupied */ + if (speed > ICE_AQC_PORT_OPT_MAX_LANE_200G) + continue; + + if (hw->pf_id == lport) + return lane; + lport++; + } + + /* PHY lane not found */ + return -ENXIO; +} + /** * ice_aq_sff_eeprom * @hw: pointer to the HW struct diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h index 27208a60cece..fe6f88cfd948 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.h +++ b/drivers/net/ethernet/intel/ice/ice_common.h @@ -193,6 +193,7 @@ ice_aq_get_port_options(struct ice_hw *hw, int ice_aq_set_port_option(struct ice_hw *hw, u8 lport, u8 lport_valid, u8 new_option); +int ice_get_phy_lane_number(struct ice_hw *hw); int ice_aq_sff_eeprom(struct ice_hw *hw, u16 lport, u8 bus_addr, u16 mem_addr, u8 page, u8 set_page, u8 *data, u8 length, diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 0ab35607e5d5..89fa3d53d317 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -1144,7 +1144,7 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up, if (link_up == old_link && link_speed == old_link_speed) return 0; - ice_ptp_link_change(pf, pf->hw.pf_id, link_up); + ice_ptp_link_change(pf, link_up); if (ice_is_dcb_active(pf)) { if (test_bit(ICE_FLAG_DCB_ENA, pf->flags)) @@ -6790,7 +6790,7 @@ static int ice_up_complete(struct ice_vsi *vsi) ice_print_link_msg(vsi, true); netif_tx_start_all_queues(vsi->netdev); netif_carrier_on(vsi->netdev); - ice_ptp_link_change(pf, pf->hw.pf_id, true); + ice_ptp_link_change(pf, true); } /* Perform an initial read of the statistics registers now to @@ -7260,7 +7260,7 @@ int ice_down(struct ice_vsi *vsi) if (vsi->netdev) { vlan_err = ice_vsi_del_vlan_zero(vsi); - ice_ptp_link_change(vsi->back, vsi->back->hw.pf_id, false); + ice_ptp_link_change(vsi->back, false); netif_carrier_off(vsi->netdev); netif_tx_disable(vsi->netdev); } diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index a999fface272..efd770dfec44 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1388,10 +1388,9 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port) /** * ice_ptp_link_change - Reconfigure PTP after link status change * @pf: Board private structure - * @port: Port for which the PHY start is set * @linkup: Link is up or down */ -void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) +void ice_ptp_link_change(struct ice_pf *pf, bool linkup) { struct ice_ptp_port *ptp_port; struct ice_hw *hw = &pf->hw; @@ -1399,14 +1398,7 @@ void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) if (pf->ptp.state != ICE_PTP_READY) return; - if (WARN_ON_ONCE(port >= hw->ptp.num_lports)) - return; - ptp_port = &pf->ptp.port; - if (ice_is_e825c(hw) && hw->ptp.is_2x50g_muxed_topo) - port *= 2; - if (WARN_ON_ONCE(ptp_port->port_num != port)) - return; /* Update cached link status for this port immediately */ ptp_port->link_up = linkup; @@ -3164,10 +3156,17 @@ void ice_ptp_init(struct ice_pf *pf) { struct ice_ptp *ptp = &pf->ptp; struct ice_hw *hw = &pf->hw; - int err; + int lane_num, err; ptp->state = ICE_PTP_INITIALIZING; + lane_num = ice_get_phy_lane_number(hw); + if (lane_num < 0) { + err = lane_num; + goto err_exit; + } + + ptp->port.port_num = (u8)lane_num; ice_ptp_init_hw(hw); ice_ptp_init_tx_interrupt_mode(pf); @@ -3188,10 +3187,6 @@ void ice_ptp_init(struct ice_pf *pf) if (err) goto err_exit; - ptp->port.port_num = hw->pf_id; - if (ice_is_e825c(hw) && hw->ptp.is_2x50g_muxed_topo) - ptp->port.port_num = hw->pf_id * 2; - err = ice_ptp_init_port(pf, &ptp->port); if (err) goto err_exit; diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h index 824e73b677a4..c490d98fd9c6 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h @@ -310,7 +310,7 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type); void ice_ptp_init(struct ice_pf *pf); void ice_ptp_release(struct ice_pf *pf); -void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup); +void ice_ptp_link_change(struct ice_pf *pf, bool linkup); #else /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */ static inline int ice_ptp_set_ts_config(struct ice_pf *pf, struct ifreq *ifr) { @@ -358,7 +358,7 @@ static inline void ice_ptp_prepare_for_reset(struct ice_pf *pf, } static inline void ice_ptp_init(struct ice_pf *pf) { } static inline void ice_ptp_release(struct ice_pf *pf) { } -static inline void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) +static inline void ice_ptp_link_change(struct ice_pf *pf, bool linkup) { } diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index e606f1049742..23fd830a364d 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -2722,26 +2722,6 @@ static int ice_get_phy_tx_tstamp_ready_eth56g(struct ice_hw *hw, u8 port, return 0; } -/** - * ice_is_muxed_topo - detect breakout 2x50G topology for E825C - * @hw: pointer to the HW struct - * - * Return: true if it's 2x50 breakout topology, false otherwise - */ -static bool ice_is_muxed_topo(struct ice_hw *hw) -{ - u8 link_topo; - bool mux; - u32 val; - - val = rd32(hw, GLGEN_SWITCH_MODE_CONFIG); - mux = FIELD_GET(GLGEN_SWITCH_MODE_CONFIG_25X4_QUAD_M, val); - val = rd32(hw, GLGEN_MAC_LINK_TOPO); - link_topo = FIELD_GET(GLGEN_MAC_LINK_TOPO_LINK_TOPO_M, val); - - return (mux && link_topo == ICE_LINK_TOPO_UP_TO_2_LINKS); -} - /** * ice_ptp_init_phy_e825 - initialize PHY parameters * @hw: pointer to the HW struct @@ -2764,12 +2744,8 @@ static void ice_ptp_init_phy_e825(struct ice_hw *hw) ice_sb_access_ena_eth56g(hw, true); err = ice_read_phy_eth56g(hw, hw->pf_id, PHY_REG_REVISION, &phy_rev); - if (err || phy_rev != PHY_REVISION_ETH56G) { + if (err || phy_rev != PHY_REVISION_ETH56G) ptp->phy_model = ICE_PHY_UNSUP; - return; - } - - ptp->is_2x50g_muxed_topo = ice_is_muxed_topo(hw); } /* E822 family functions diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index a61026970be5..4a9ef722635f 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -880,7 +880,6 @@ struct ice_ptp_hw { union ice_phy_params phy; u8 num_lports; u8 ports_per_phy; - bool is_2x50g_muxed_topo; }; /* Port hardware description */