| 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 |
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 >
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
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 --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);
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(-)