diff mbox series

[03/31] wifi: mwifiex: drop HostCmd_CMD_802_11_MAC_ADDRESS response handling

Message ID 20240820-mwifiex-cleanup-v1-3-320d8de4a4b7@pengutronix.de (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: mwifiex: cleanup driver | expand

Commit Message

Sascha Hauer Aug. 20, 2024, 11:55 a.m. UTC
The command response handler copies the new MAC address over to
priv->curr_addr. The same is done in the code issuing the call
already, so drop the unnecessary HostCmd_CMD_802_11_MAC_ADDRESS
handling.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 20 --------------------
 1 file changed, 20 deletions(-)

Comments

Brian Norris Aug. 22, 2024, 6:07 p.m. UTC | #1
Hi Sascha,

On Tue, Aug 20, 2024 at 01:55:28PM +0200, Sascha Hauer wrote:
> The command response handler copies the new MAC address over to
> priv->curr_addr. The same is done in the code issuing the call
> already, so drop the unnecessary HostCmd_CMD_802_11_MAC_ADDRESS
> handling.

It took a bit to figure out what you meant here -- I guess you're
referring to mwifiex_set_mac_address()? It could help to document what
you mean.

I'm also a bit torn; this command API ostensibly has a (unused so far,
for this command) HostCmd_ACT_GEN_GET mode, in which case this *is*
important.

If anything, I might consider dropping some of the handling in
mwifiex_set_mac_address(), because it seems to presume (and then has to
undo for failure) behavior of the underlying command.

Brian

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 20 --------------------
>  1 file changed, 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index 9c53825f222d1..7f81e709bd6b7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -473,25 +473,6 @@ static int mwifiex_ret_rf_antenna(struct mwifiex_private *priv,
>  	return 0;
>  }
>  
> -/*
> - * This function handles the command response of set/get MAC address.
> - *
> - * Handling includes saving the MAC address in driver.
> - */
> -static int mwifiex_ret_802_11_mac_address(struct mwifiex_private *priv,
> -					  struct host_cmd_ds_command *resp)
> -{
> -	struct host_cmd_ds_802_11_mac_address *cmd_mac_addr =
> -							&resp->params.mac_addr;
> -
> -	memcpy(priv->curr_addr, cmd_mac_addr->mac_addr, ETH_ALEN);
> -
> -	mwifiex_dbg(priv->adapter, INFO,
> -		    "info: set mac address: %pM\n", priv->curr_addr);
> -
> -	return 0;
> -}
> -
>  /*
>   * This function handles the command response of set/get MAC multicast
>   * address.
> @@ -1232,7 +1213,6 @@ int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
>  	case HostCmd_CMD_MAC_CONTROL:
>  		break;
>  	case HostCmd_CMD_802_11_MAC_ADDRESS:
> -		ret = mwifiex_ret_802_11_mac_address(priv, resp);
>  		break;
>  	case HostCmd_CMD_MAC_MULTICAST_ADR:
>  		ret = mwifiex_ret_mac_multicast_adr(priv, resp);
> 
> -- 
> 2.39.2
>
Sascha Hauer Aug. 26, 2024, 9:07 a.m. UTC | #2
On Thu, Aug 22, 2024 at 11:07:35AM -0700, Brian Norris wrote:
> Hi Sascha,
> 
> On Tue, Aug 20, 2024 at 01:55:28PM +0200, Sascha Hauer wrote:
> > The command response handler copies the new MAC address over to
> > priv->curr_addr. The same is done in the code issuing the call
> > already, so drop the unnecessary HostCmd_CMD_802_11_MAC_ADDRESS
> > handling.
> 
> It took a bit to figure out what you meant here -- I guess you're
> referring to mwifiex_set_mac_address()? It could help to document what
> you mean.

Ok, I can clarify this a bit when sending this next time.

Right now what we have is:

1) mwifiex_set_mac_address() sets priv->curr_addr to the desired new MAC
   address
2) mwifiex_cmd_802_11_mac_address() (called from mwifiex_send_cmd())
   constructs the HostCmd_CMD_802_11_MAC_ADDRESS command, using the MAC
   address in priv->curr_addr
3) mwifiex_ret_802_11_mac_address(), called from the response handler,
   sets priv->curr_addr to the MAC address received with the command
   response, which of course is the same as we initially copied there
   in step 1), which makes 3) redundant and unnecessary

> 
> I'm also a bit torn; this command API ostensibly has a (unused so far,
> for this command) HostCmd_ACT_GEN_GET mode, in which case this *is*
> important.
> 
> If anything, I might consider dropping some of the handling in
> mwifiex_set_mac_address(), because it seems to presume (and then has to
> undo for failure) behavior of the underlying command.

What we could do instead of dropping 3) is:

1) pass the new MAC address in the data_buf argument to
   mwifiex_send_cmd()
2) instead of priv->curr_addr use data_buf in
   mwifiex_cmd_802_11_mac_address()

With this the response handler would still set priv->curr_addr in case
the command went through successfully. No need to undo priv->curr_addr
to the previous MAC address in case the command failed.

Sounds good to me. Is that where you aiming at?

Sascha
Brian Norris Aug. 26, 2024, 10:44 p.m. UTC | #3
Hi Sascha,

On Mon, Aug 26, 2024 at 11:07:03AM +0200, Sascha Hauer wrote:
> On Thu, Aug 22, 2024 at 11:07:35AM -0700, Brian Norris wrote:
> > Hi Sascha,
> > 
> > On Tue, Aug 20, 2024 at 01:55:28PM +0200, Sascha Hauer wrote:
> > > The command response handler copies the new MAC address over to
> > > priv->curr_addr. The same is done in the code issuing the call
> > > already, so drop the unnecessary HostCmd_CMD_802_11_MAC_ADDRESS
> > > handling.
> > 
> > It took a bit to figure out what you meant here -- I guess you're
> > referring to mwifiex_set_mac_address()? It could help to document what
> > you mean.
> 
> Ok, I can clarify this a bit when sending this next time.
> 
> Right now what we have is:
> 
> 1) mwifiex_set_mac_address() sets priv->curr_addr to the desired new MAC
>    address
> 2) mwifiex_cmd_802_11_mac_address() (called from mwifiex_send_cmd())
>    constructs the HostCmd_CMD_802_11_MAC_ADDRESS command, using the MAC
>    address in priv->curr_addr
> 3) mwifiex_ret_802_11_mac_address(), called from the response handler,
>    sets priv->curr_addr to the MAC address received with the command
>    response, which of course is the same as we initially copied there
>    in step 1), which makes 3) redundant and unnecessary

Ack, that's the understanding I got, but it took a bit of reading to get
there.

> > I'm also a bit torn; this command API ostensibly has a (unused so far,
> > for this command) HostCmd_ACT_GEN_GET mode, in which case this *is*
> > important.
> > 
> > If anything, I might consider dropping some of the handling in
> > mwifiex_set_mac_address(), because it seems to presume (and then has to
> > undo for failure) behavior of the underlying command.
> 
> What we could do instead of dropping 3) is:
> 
> 1) pass the new MAC address in the data_buf argument to
>    mwifiex_send_cmd()
> 2) instead of priv->curr_addr use data_buf in
>    mwifiex_cmd_802_11_mac_address()
> 
> With this the response handler would still set priv->curr_addr in case
> the command went through successfully. No need to undo priv->curr_addr
> to the previous MAC address in case the command failed.
> 
> Sounds good to me. Is that where you aiming at?

Yes, that seems about right.

Brian
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 9c53825f222d1..7f81e709bd6b7 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -473,25 +473,6 @@  static int mwifiex_ret_rf_antenna(struct mwifiex_private *priv,
 	return 0;
 }
 
-/*
- * This function handles the command response of set/get MAC address.
- *
- * Handling includes saving the MAC address in driver.
- */
-static int mwifiex_ret_802_11_mac_address(struct mwifiex_private *priv,
-					  struct host_cmd_ds_command *resp)
-{
-	struct host_cmd_ds_802_11_mac_address *cmd_mac_addr =
-							&resp->params.mac_addr;
-
-	memcpy(priv->curr_addr, cmd_mac_addr->mac_addr, ETH_ALEN);
-
-	mwifiex_dbg(priv->adapter, INFO,
-		    "info: set mac address: %pM\n", priv->curr_addr);
-
-	return 0;
-}
-
 /*
  * This function handles the command response of set/get MAC multicast
  * address.
@@ -1232,7 +1213,6 @@  int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
 	case HostCmd_CMD_MAC_CONTROL:
 		break;
 	case HostCmd_CMD_802_11_MAC_ADDRESS:
-		ret = mwifiex_ret_802_11_mac_address(priv, resp);
 		break;
 	case HostCmd_CMD_MAC_MULTICAST_ADR:
 		ret = mwifiex_ret_mac_multicast_adr(priv, resp);