| Message ID | 20230919233435.518620-5-jacob.e.keller@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | ice: fix linking with CONFIG_PTP_1588_CLOCK=n | expand |
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob Keller > Sent: Wednesday, September 20, 2023 5:05 AM > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-next 4/4] ice: cleanup ice_find_netlist_node > > The ice_find_netlist_node function was introduced in commit 8a3a565ff210 > ("ice: add admin commands to access cgu configuration"). Variations of this > function were reviewed concurrently on both intel-wired-lan[1][2], and > netdev [3][4] > > [1]: https://lore.kernel.org/intel-wired-lan/20230913204943.1051233-7-vadim.fedorenko@linux.dev/ > [2]: https://lore.kernel.org/intel-wired-lan/20230817000058.2433236-5-jacob.e.keller@intel.com/ > [3]: https://lore.kernel.org/netdev/20230918212814.435688-1-anthony.l.nguyen@intel.com/ > [4]: https://lore.kernel.org/netdev/20230913204943.1051233-7-vadim.fedorenko@linux.dev/ > > The variant I posted had a few changes due to review feedback which were > never incorporated into the DPLL series: > > * Replace the references to ancient and long removed ICE_SUCCESS and > ICE_ERR_DOES_NOT_EXIST status codes in the function comment. > > * Return -ENOENT instead of -ENOTBLK, as a more common way to indicate that > an entry doesn't exist. > > * Avoid the use of memset() and use simple static initialization for the > cmd variable. > > * Use FIELD_PREP to assign the node_type_ctx. > > * Remove an unnecessary local variable to keep track of rec_node_handle, > just pass the node_handle pointer directly into ice_aq_get_netlist_node. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_common.c | 30 ++++++++++----------- > 1 file changed, 15 insertions(+), 15 deletions(-) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Dear Jacob, Thank you for your patch. A small nit for the subject: The verb *clean up* is spelled with a space. Am 20.09.23 um 01:34 schrieb Jacob Keller: > The ice_find_netlist_node function was introduced in commit 8a3a565ff210 > ("ice: add admin commands to access cgu configuration"). Variations of this > function were reviewed concurrently on both intel-wired-lan[1][2], and > netdev [3][4] > > [1]: https://lore.kernel.org/intel-wired-lan/20230913204943.1051233-7-vadim.fedorenko@linux.dev/ > [2]: https://lore.kernel.org/intel-wired-lan/20230817000058.2433236-5-jacob.e.keller@intel.com/ > [3]: https://lore.kernel.org/netdev/20230918212814.435688-1-anthony.l.nguyen@intel.com/ > [4]: https://lore.kernel.org/netdev/20230913204943.1051233-7-vadim.fedorenko@linux.dev/ > > The variant I posted had a few changes due to review feedback which were > never incorporated into the DPLL series: > > * Replace the references to ancient and long removed ICE_SUCCESS and > ICE_ERR_DOES_NOT_EXIST status codes in the function comment. > > * Return -ENOENT instead of -ENOTBLK, as a more common way to indicate that > an entry doesn't exist. > > * Avoid the use of memset() and use simple static initialization for the > cmd variable. > > * Use FIELD_PREP to assign the node_type_ctx. > > * Remove an unnecessary local variable to keep track of rec_node_handle, > just pass the node_handle pointer directly into ice_aq_get_netlist_node. If you need to enumerate changes, with git it’s always good to do one commit per item. ;-) (Better to bisect and to revert.) Anyway, not important enough. > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_common.c | 30 ++++++++++----------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c > index 12c09374c2be..d4e259b816b9 100644 > --- a/drivers/net/ethernet/intel/ice/ice_common.c > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > @@ -473,41 +473,41 @@ ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd, > * @node_part_number: node part number to look for > * @node_handle: output parameter if node found - optional > * > - * Find and return the node handle for a given node type and part number in the > - * netlist. When found ICE_SUCCESS is returned, ICE_ERR_DOES_NOT_EXIST > - * otherwise. If node_handle provided, it would be set to found node handle. > + * Scan the netlist for a node handle of the given node type and part number. > + * > + * If node_handle is non-NULL it will be modified on function exit. It is only > + * valid if the function returns zero, and should be ignored on any non-zero > + * return value. > + * > + * Returns: 0 if the node is found, -ENOENT if no handle was found, and > + * a negative error code on failure to access the AQ. > */ > static int ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, > u8 node_part_number, u16 *node_handle) > { > - struct ice_aqc_get_link_topo cmd; > - u8 rec_node_part_number; > - u16 rec_node_handle; > u8 idx; > > for (idx = 0; idx < ICE_MAX_NETLIST_SIZE; idx++) { > + struct ice_aqc_get_link_topo cmd = {}; > + u8 rec_node_part_number; > int status; > > - memset(&cmd, 0, sizeof(cmd)); > - > cmd.addr.topo_params.node_type_ctx = > - (node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S); > + FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_TYPE_M, > + node_type_ctx); > cmd.addr.topo_params.index = idx; > > status = ice_aq_get_netlist_node(hw, &cmd, > &rec_node_part_number, > - &rec_node_handle); > + node_handle); > if (status) > return status; > > - if (rec_node_part_number == node_part_number) { > - if (node_handle) > - *node_handle = rec_node_handle; > + if (rec_node_part_number == node_part_number) > return 0; > - } > } > > - return -ENOTBLK; > + return -ENOENT; > } > > /** Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 12c09374c2be..d4e259b816b9 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -473,41 +473,41 @@ ice_aq_get_netlist_node(struct ice_hw *hw, struct ice_aqc_get_link_topo *cmd, * @node_part_number: node part number to look for * @node_handle: output parameter if node found - optional * - * Find and return the node handle for a given node type and part number in the - * netlist. When found ICE_SUCCESS is returned, ICE_ERR_DOES_NOT_EXIST - * otherwise. If node_handle provided, it would be set to found node handle. + * Scan the netlist for a node handle of the given node type and part number. + * + * If node_handle is non-NULL it will be modified on function exit. It is only + * valid if the function returns zero, and should be ignored on any non-zero + * return value. + * + * Returns: 0 if the node is found, -ENOENT if no handle was found, and + * a negative error code on failure to access the AQ. */ static int ice_find_netlist_node(struct ice_hw *hw, u8 node_type_ctx, u8 node_part_number, u16 *node_handle) { - struct ice_aqc_get_link_topo cmd; - u8 rec_node_part_number; - u16 rec_node_handle; u8 idx; for (idx = 0; idx < ICE_MAX_NETLIST_SIZE; idx++) { + struct ice_aqc_get_link_topo cmd = {}; + u8 rec_node_part_number; int status; - memset(&cmd, 0, sizeof(cmd)); - cmd.addr.topo_params.node_type_ctx = - (node_type_ctx << ICE_AQC_LINK_TOPO_NODE_TYPE_S); + FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_TYPE_M, + node_type_ctx); cmd.addr.topo_params.index = idx; status = ice_aq_get_netlist_node(hw, &cmd, &rec_node_part_number, - &rec_node_handle); + node_handle); if (status) return status; - if (rec_node_part_number == node_part_number) { - if (node_handle) - *node_handle = rec_node_handle; + if (rec_node_part_number == node_part_number) return 0; - } } - return -ENOTBLK; + return -ENOENT; } /**
The ice_find_netlist_node function was introduced in commit 8a3a565ff210 ("ice: add admin commands to access cgu configuration"). Variations of this function were reviewed concurrently on both intel-wired-lan[1][2], and netdev [3][4] [1]: https://lore.kernel.org/intel-wired-lan/20230913204943.1051233-7-vadim.fedorenko@linux.dev/ [2]: https://lore.kernel.org/intel-wired-lan/20230817000058.2433236-5-jacob.e.keller@intel.com/ [3]: https://lore.kernel.org/netdev/20230918212814.435688-1-anthony.l.nguyen@intel.com/ [4]: https://lore.kernel.org/netdev/20230913204943.1051233-7-vadim.fedorenko@linux.dev/ The variant I posted had a few changes due to review feedback which were never incorporated into the DPLL series: * Replace the references to ancient and long removed ICE_SUCCESS and ICE_ERR_DOES_NOT_EXIST status codes in the function comment. * Return -ENOENT instead of -ENOTBLK, as a more common way to indicate that an entry doesn't exist. * Avoid the use of memset() and use simple static initialization for the cmd variable. * Use FIELD_PREP to assign the node_type_ctx. * Remove an unnecessary local variable to keep track of rec_node_handle, just pass the node_handle pointer directly into ice_aq_get_netlist_node. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/ice/ice_common.c | 30 ++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-)