| Message ID | 20250220061143.1417420-2-jeff.chen_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Resolve the failure in downloading | expand |
Hello Jeff, On Thu, Feb 20, 2025 at 02:11:42PM +0800, Jeff Chen wrote: > This patch corrects the command format used for downloading RF > calibration data to the firmware. Do we need any fixes tag? is this supposed to be backported to stable? Was the command format always broken? Do this format depends on the firmware version? We would need to explain why changing the format of this command here is safe. > > This patch is a split from the previous submission. > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > --- > drivers/net/wireless/marvell/mwifiex/fw.h | 7 +++++++ > drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 14 +++++++++----- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 4a96281792cc..0c75a574a7ee 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -2352,6 +2352,12 @@ struct host_cmd_ds_add_station { > u8 tlv[]; > } __packed; > > +struct host_cmd_ds_802_11_cfg_data { > + __le16 action; > + __le16 type; > + __le16 data_len; > +} __packed; > + > struct host_cmd_ds_command { > __le16 command; > __le16 size; > @@ -2431,6 +2437,7 @@ struct host_cmd_ds_command { > struct host_cmd_ds_pkt_aggr_ctrl pkt_aggr_ctrl; > struct host_cmd_ds_sta_configure sta_cfg; > struct host_cmd_ds_add_station sta_info; > + struct host_cmd_ds_802_11_cfg_data cfg_data; > } params; > } __packed; > > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > index e2800a831c8e..6e7b2b5c7dc5 100644 > --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > @@ -1500,18 +1500,19 @@ int mwifiex_dnld_dt_cfgdata(struct mwifiex_private *priv, > > /* This function prepares command of set_cfg_data. */ > static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv, > - struct host_cmd_ds_command *cmd, void *data_buf) > + struct host_cmd_ds_command *cmd, void *data_buf, u16 cmd_action) > { > struct mwifiex_adapter *adapter = priv->adapter; > struct property *prop = data_buf; > u32 len; > u8 *data = (u8 *)cmd + S_DS_GEN; > int ret; > + struct host_cmd_ds_802_11_cfg_data *pcfg_data = &cmd->params.cfg_data; > > if (prop) { > len = prop->length; > ret = of_property_read_u8_array(adapter->dt_node, prop->name, > - data, len); > + data + sizeof(*pcfg_data), len); > if (ret) > return ret; > mwifiex_dbg(adapter, INFO, > @@ -1519,15 +1520,18 @@ static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv, > prop->name); > } else if (adapter->cal_data->data && adapter->cal_data->size > 0) { > len = mwifiex_parse_cal_cfg((u8 *)adapter->cal_data->data, > - adapter->cal_data->size, data); > + adapter->cal_data->size, data + sizeof(*pcfg_data)); > mwifiex_dbg(adapter, INFO, > "download cfg_data from config file\n"); > } else { > return -1; > } > > + pcfg_data->action = cpu_to_le16(cmd_action); > + pcfg_data->type = cpu_to_le16(2); > + pcfg_data->data_len = cpu_to_le16(len); > cmd->command = cpu_to_le16(HostCmd_CMD_CFG_DATA); > - cmd->size = cpu_to_le16(S_DS_GEN + len); > + cmd->size = cpu_to_le16(S_DS_GEN + sizeof(*pcfg_data) + len); > > return 0; > } > @@ -1949,7 +1953,7 @@ int mwifiex_sta_prepare_cmd(struct mwifiex_private *priv, uint16_t cmd_no, > ret = mwifiex_cmd_get_hw_spec(priv, cmd_ptr); > break; > case HostCmd_CMD_CFG_DATA: > - ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf); > + ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf, cmd_action); > break; > case HostCmd_CMD_MAC_CONTROL: > ret = mwifiex_cmd_mac_control(priv, cmd_ptr, cmd_action, > -- > 2.34.1 >
Hi Francesco, Thank you for reviewing and providing great comments. > > Hello Jeff, > > On Thu, Feb 20, 2025 at 02:11:42PM +0800, Jeff Chen wrote: > > This patch corrects the command format used for downloading RF > > calibration data to the firmware. > > Do we need any fixes tag? is this supposed to be backported to stable? > > Was the command format always broken? Do this format depends on the > firmware version? We would need to explain why changing the format of this > command here is safe. > Yes, we need a Fixes tag. It appears that the feature to download CAL from file was broken by the commit below. commit d39fbc88956ef56a67b8030d53c7e8fa645a4e00 Author: Bing Zhao <bzhao@marvell.com> Date: Fri Dec 13 18:33:00 2013 -0800 mwifiex: remove cfg_data construction The cfg_data buffer will include the cfg_data structure header (action, type, data_len). This makes it work for all data types without extra parsing. This patch enables the feature to download RF calibration data from a file. I don't think it needs to be backported to stable. It doesn't depend on the firmware version. However, I think it will break the feature to download CAL data from the device tree. See the code segment below. 'mwifiex_dnld_dt_cfgdata()' also uses 'mwifiex_cmd_cfg_data()' to prepare a command. It would be better to add a new function to download CAL data from a file, instead of modifying the function `mwifiex_cmd_cfg_data()` int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init) { ... /* Download calibration data to firmware. * The cal-data can be read from device tree and/or * a configuration file and downloaded to firmware. */ if (adapter->dt_node) { if (of_property_read_u32(adapter->dt_node, "marvell,wakeup-pin", &data) == 0) { pr_debug("Wakeup pin = 0x%x\n", data); adapter->hs_cfg.gpio = data; } mwifiex_dnld_dt_cfgdata(priv, adapter->dt_node, "marvell,caldata"); } ... } > > > > This patch is a split from the previous submission. > > > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > --- > > drivers/net/wireless/marvell/mwifiex/fw.h | 7 +++++++ > > drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 14 +++++++++----- > > 2 files changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h > > b/drivers/net/wireless/marvell/mwifiex/fw.h > > index 4a96281792cc..0c75a574a7ee 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > > @@ -2352,6 +2352,12 @@ struct host_cmd_ds_add_station { > > u8 tlv[]; > > } __packed; > > > > +struct host_cmd_ds_802_11_cfg_data { > > + __le16 action; > > + __le16 type; > > + __le16 data_len; > > +} __packed; > > + > > struct host_cmd_ds_command { > > __le16 command; > > __le16 size; > > @@ -2431,6 +2437,7 @@ struct host_cmd_ds_command { > > struct host_cmd_ds_pkt_aggr_ctrl pkt_aggr_ctrl; > > struct host_cmd_ds_sta_configure sta_cfg; > > struct host_cmd_ds_add_station sta_info; > > + struct host_cmd_ds_802_11_cfg_data cfg_data; > > } params; > > } __packed; > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > index e2800a831c8e..6e7b2b5c7dc5 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > @@ -1500,18 +1500,19 @@ int mwifiex_dnld_dt_cfgdata(struct > > mwifiex_private *priv, > > > > /* This function prepares command of set_cfg_data. */ static int > > mwifiex_cmd_cfg_data(struct mwifiex_private *priv, > > - struct host_cmd_ds_command *cmd, > void *data_buf) > > + struct host_cmd_ds_command *cmd, > void > > + *data_buf, u16 cmd_action) > > { > > struct mwifiex_adapter *adapter = priv->adapter; > > struct property *prop = data_buf; > > u32 len; > > u8 *data = (u8 *)cmd + S_DS_GEN; > > int ret; > > + struct host_cmd_ds_802_11_cfg_data *pcfg_data = > > + &cmd->params.cfg_data; > > > > if (prop) { > > len = prop->length; > > ret = of_property_read_u8_array(adapter->dt_node, > prop->name, > > - data, len); > > + data + > > + sizeof(*pcfg_data), len); > > if (ret) > > return ret; > > mwifiex_dbg(adapter, INFO, @@ -1519,15 +1520,18 @@ > > static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv, > > prop->name); > > } else if (adapter->cal_data->data && adapter->cal_data->size > 0) { > > len = mwifiex_parse_cal_cfg((u8 > *)adapter->cal_data->data, > > - adapter->cal_data->size, > data); > > + adapter->cal_data->size, > > + data + sizeof(*pcfg_data)); > > mwifiex_dbg(adapter, INFO, > > "download cfg_data from config file\n"); > > } else { > > return -1; > > } > > > > + pcfg_data->action = cpu_to_le16(cmd_action); > > + pcfg_data->type = cpu_to_le16(2); > > + pcfg_data->data_len = cpu_to_le16(len); > > cmd->command = cpu_to_le16(HostCmd_CMD_CFG_DATA); > > - cmd->size = cpu_to_le16(S_DS_GEN + len); > > + cmd->size = cpu_to_le16(S_DS_GEN + sizeof(*pcfg_data) + len); > > > > return 0; > > } > > @@ -1949,7 +1953,7 @@ int mwifiex_sta_prepare_cmd(struct > mwifiex_private *priv, uint16_t cmd_no, > > ret = mwifiex_cmd_get_hw_spec(priv, cmd_ptr); > > break; > > case HostCmd_CMD_CFG_DATA: > > - ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf); > > + ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf, > > + cmd_action); > > break; > > case HostCmd_CMD_MAC_CONTROL: > > ret = mwifiex_cmd_mac_control(priv, cmd_ptr, > cmd_action, > > -- > > 2.34.1 > >
Hello Jeff, On Tue, Mar 18, 2025 at 01:07:39PM +0800, Jeff Chen wrote: > While this patch restores the ability to download RF calibration data > from a file, it may inadvertently break the feature to download > calibration data from the device tree. I do not think this is acceptable. Fixing something by adding another bug is not ok. You should fix the calibration data from file, without breaking the device tree calibration functionality. Francesco
On Tue, Mar 18, 2025 at 01:07:39PM +0800, Jeff Chen wrote: > This patch resolves an issue where RF calibration data from a > file could not be downloaded to the firmware. The feature to > download calibration data from a file was broken by the commit: > d39fbc88956e. > > The issue arose because the function `mwifiex_cmd_cfg_data()` > was modified in a way that prevented proper handling of > file-based calibration data. While this patch restores the ability > to download RF calibration data from a file, it may inadvertently > break the feature to download calibration data from the device > tree. This is because the function `mwifiex_dnld_dt_cfgdata()`, > which also relies on `mwifiex_cmd_cfg_data()`, is still used for > device tree-based calibration data downloads. > > Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > --- > drivers/net/wireless/marvell/mwifiex/fw.h | 14 ++++++++++++++ > drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 11 +++++++++-- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 4a96281792cc..91458f3bd14a 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -454,6 +454,11 @@ enum mwifiex_channel_flags { > #define HostCmd_RET_BIT 0x8000 > #define HostCmd_ACT_GEN_GET 0x0000 > #define HostCmd_ACT_GEN_SET 0x0001 > +#define HOST_CMD_ACT_GEN_SET 0x0001 > +/* Add this non-CamelCase-style macro to comply with checkpatch requirements. > + * This macro will eventually replace all existing CamelCase-style macros in > + * the future for consistency. > + */ Just ignore this checkpatch warning. We don't want to have duplicated defines just for silencing checkpatch. If anything we could change all the CamelCase defines throughout the driver in one go. Sascha
> Hello Jeff, > > On Tue, Mar 18, 2025 at 01:07:39PM +0800, Jeff Chen wrote: > > While this patch restores the ability to download RF calibration data > > from a file, it may inadvertently break the feature to download > > calibration data from the device tree. > > I do not think this is acceptable. Fixing something by adding another bug is not > ok. You should fix the calibration data from file, without breaking the device > tree calibration functionality. > > Francesco Hello Francesco, There seems to be a misunderstanding. The issue with breaking the device tree calibration functionality was actually introduced in version 3 of the patch. Version 4 resolves this problem by ensuring that both file-based and device tree-based calibration work as intended. The current patch restores the ability to download RF calibration data from a file without affecting the device tree functionality. I'll clarify this in the commit message to avoid further confusion. Best regards, Jeff
> -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: Monday, March 24, 2025 3:02 PM > To: Jeff Chen <jeff.chen_1@nxp.com> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > briannorris@chromium.org; johannes@sipsolutions.net; francesco@dolcini.it; > Pete Hsieh <tsung-hsien.hsieh@nxp.com> > Subject: [EXT] Re: [PATCH v4 2/2] wifi: mwifiex: Fix RF calibration data > download from file > > > @@ -454,6 +454,11 @@ enum mwifiex_channel_flags { > > #define HostCmd_RET_BIT 0x8000 > > #define HostCmd_ACT_GEN_GET 0x0000 > > #define HostCmd_ACT_GEN_SET 0x0001 > > +#define HOST_CMD_ACT_GEN_SET 0x0001 > > +/* Add this non-CamelCase-style macro to comply with checkpatch > requirements. > > + * This macro will eventually replace all existing CamelCase-style > > +macros in > > + * the future for consistency. > > + */ > > Just ignore this checkpatch warning. We don't want to have duplicated defines > just for silencing checkpatch. If anything we could change all the CamelCase > defines throughout the driver in one go. > > Sascha > Hello Sascha, I'll proceed with reverting the duplicated defines and ignoring the checkpatch warning for now. Thank you for pointing this out. Best regards, Jeff
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 4a96281792cc..0c75a574a7ee 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -2352,6 +2352,12 @@ struct host_cmd_ds_add_station { u8 tlv[]; } __packed; +struct host_cmd_ds_802_11_cfg_data { + __le16 action; + __le16 type; + __le16 data_len; +} __packed; + struct host_cmd_ds_command { __le16 command; __le16 size; @@ -2431,6 +2437,7 @@ struct host_cmd_ds_command { struct host_cmd_ds_pkt_aggr_ctrl pkt_aggr_ctrl; struct host_cmd_ds_sta_configure sta_cfg; struct host_cmd_ds_add_station sta_info; + struct host_cmd_ds_802_11_cfg_data cfg_data; } params; } __packed; diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c index e2800a831c8e..6e7b2b5c7dc5 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -1500,18 +1500,19 @@ int mwifiex_dnld_dt_cfgdata(struct mwifiex_private *priv, /* This function prepares command of set_cfg_data. */ static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv, - struct host_cmd_ds_command *cmd, void *data_buf) + struct host_cmd_ds_command *cmd, void *data_buf, u16 cmd_action) { struct mwifiex_adapter *adapter = priv->adapter; struct property *prop = data_buf; u32 len; u8 *data = (u8 *)cmd + S_DS_GEN; int ret; + struct host_cmd_ds_802_11_cfg_data *pcfg_data = &cmd->params.cfg_data; if (prop) { len = prop->length; ret = of_property_read_u8_array(adapter->dt_node, prop->name, - data, len); + data + sizeof(*pcfg_data), len); if (ret) return ret; mwifiex_dbg(adapter, INFO, @@ -1519,15 +1520,18 @@ static int mwifiex_cmd_cfg_data(struct mwifiex_private *priv, prop->name); } else if (adapter->cal_data->data && adapter->cal_data->size > 0) { len = mwifiex_parse_cal_cfg((u8 *)adapter->cal_data->data, - adapter->cal_data->size, data); + adapter->cal_data->size, data + sizeof(*pcfg_data)); mwifiex_dbg(adapter, INFO, "download cfg_data from config file\n"); } else { return -1; } + pcfg_data->action = cpu_to_le16(cmd_action); + pcfg_data->type = cpu_to_le16(2); + pcfg_data->data_len = cpu_to_le16(len); cmd->command = cpu_to_le16(HostCmd_CMD_CFG_DATA); - cmd->size = cpu_to_le16(S_DS_GEN + len); + cmd->size = cpu_to_le16(S_DS_GEN + sizeof(*pcfg_data) + len); return 0; } @@ -1949,7 +1953,7 @@ int mwifiex_sta_prepare_cmd(struct mwifiex_private *priv, uint16_t cmd_no, ret = mwifiex_cmd_get_hw_spec(priv, cmd_ptr); break; case HostCmd_CMD_CFG_DATA: - ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf); + ret = mwifiex_cmd_cfg_data(priv, cmd_ptr, data_buf, cmd_action); break; case HostCmd_CMD_MAC_CONTROL: ret = mwifiex_cmd_mac_control(priv, cmd_ptr, cmd_action,
This patch corrects the command format used for downloading RF calibration data to the firmware. This patch is a split from the previous submission. Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> --- drivers/net/wireless/marvell/mwifiex/fw.h | 7 +++++++ drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 14 +++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-)