| Message ID | 20230412081929.173220-6-mschmidt@redhat.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: lower CPU usage with GNSS | expand |
>From: Michal Schmidt <mschmidt@redhat.com> >Sent: Wednesday, April 12, 2023 10:19 AM > >The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken. >'buf' is nowhere copied into 'buf_cpy'. > >The reason this does not cause problems is that all commands for which >'is_cmd_for_retry' is true go with a NULL buf. > >Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer >holds in the future. > >Signed-off-by: Michal Schmidt <mschmidt@redhat.com> >--- > drivers/net/ethernet/intel/ice/ice_common.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > >diff --git a/drivers/net/ethernet/intel/ice/ice_common.c >b/drivers/net/ethernet/intel/ice/ice_common.c >index 3638598d732b..c6200564304e 100644 >--- a/drivers/net/ethernet/intel/ice/ice_common.c >+++ b/drivers/net/ethernet/intel/ice/ice_common.c >@@ -1619,7 +1619,6 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct >ice_ctl_q_info *cq, > { > struct ice_aq_desc desc_cpy; > bool is_cmd_for_retry; >- u8 *buf_cpy = NULL; > u8 idx = 0; > u16 opcode; > int status; >@@ -1629,11 +1628,8 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct >ice_ctl_q_info *cq, > memset(&desc_cpy, 0, sizeof(desc_cpy)); > > if (is_cmd_for_retry) { >- if (buf) { >- buf_cpy = kzalloc(buf_size, GFP_KERNEL); >- if (!buf_cpy) >- return -ENOMEM; >- } >+ /* All retryable cmds are direct, without buf. */ >+ WARN_ON(buf); > > memcpy(&desc_cpy, desc, sizeof(desc_cpy)); > } >@@ -1645,17 +1641,12 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct >ice_ctl_q_info *cq, > hw->adminq.sq_last_status != ICE_AQ_RC_EBUSY) > break; > >- if (buf_cpy) >- memcpy(buf, buf_cpy, buf_size); >- > memcpy(desc, &desc_cpy, sizeof(desc_cpy)); > > mdelay(ICE_SQ_SEND_DELAY_TIME_MS); > > } while (++idx < ICE_SQ_SEND_MAX_EXECUTE); > >- kfree(buf_cpy); >- > return status; > } > >-- >2.39.2 Looks good, thank you Michal! Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
On Wed, Apr 12, 2023 at 10:19:28AM +0200, Michal Schmidt wrote: > The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken. > 'buf' is nowhere copied into 'buf_cpy'. > > The reason this does not cause problems is that all commands for which > 'is_cmd_for_retry' is true go with a NULL buf. > > Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer > holds in the future. > > Signed-off-by: Michal Schmidt <mschmidt@redhat.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt > Sent: Wednesday, April 12, 2023 1:19 AM > To: intel-wired-lan@lists.osuosl.org > Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com> > Subject: [Intel-wired-lan] [PATCH net-next v2 5/6] ice: remove unused buffer copy code in ice_sq_send_cmd_retry() > > The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken. > 'buf' is nowhere copied into 'buf_cpy'. > > The reason this does not cause problems is that all commands for which 'is_cmd_for_retry' is true go with a NULL buf. > > Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer holds in the future. > > Signed-off-by: Michal Schmidt <mschmidt@redhat.com> > --- > drivers/net/ethernet/intel/ice/ice_common.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 3638598d732b..c6200564304e 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -1619,7 +1619,6 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq, { struct ice_aq_desc desc_cpy; bool is_cmd_for_retry; - u8 *buf_cpy = NULL; u8 idx = 0; u16 opcode; int status; @@ -1629,11 +1628,8 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq, memset(&desc_cpy, 0, sizeof(desc_cpy)); if (is_cmd_for_retry) { - if (buf) { - buf_cpy = kzalloc(buf_size, GFP_KERNEL); - if (!buf_cpy) - return -ENOMEM; - } + /* All retryable cmds are direct, without buf. */ + WARN_ON(buf); memcpy(&desc_cpy, desc, sizeof(desc_cpy)); } @@ -1645,17 +1641,12 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq, hw->adminq.sq_last_status != ICE_AQ_RC_EBUSY) break; - if (buf_cpy) - memcpy(buf, buf_cpy, buf_size); - memcpy(desc, &desc_cpy, sizeof(desc_cpy)); mdelay(ICE_SQ_SEND_DELAY_TIME_MS); } while (++idx < ICE_SQ_SEND_MAX_EXECUTE); - kfree(buf_cpy); - return status; }
The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken. 'buf' is nowhere copied into 'buf_cpy'. The reason this does not cause problems is that all commands for which 'is_cmd_for_retry' is true go with a NULL buf. Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer holds in the future. Signed-off-by: Michal Schmidt <mschmidt@redhat.com> --- drivers/net/ethernet/intel/ice/ice_common.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)