| Message ID | 20220110231255.v2.1.Ie4dcc45b0bf365077303c596891d460d716bb4c5@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2,1/2] ath10k: search for default BDF name provided in DT | expand |
Hi, On Mon, Jan 10, 2022 at 3:15 PM Abhishek Kumar <kuabhs@chromium.org> wrote: > > +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar) > +{ > + struct device_node *node; > + const char *board_name = NULL; > + > + ar->id.default_bdf[0] = '\0'; > + > + node = ar->dev->of_node; > + if (!node) > + return -ENOENT; > + > + of_property_read_string(node, "qcom,ath10k-default-bdf", > + &board_name); > + if (!board_name) > + return -ENODATA; > + > + if (strscpy(ar->id.default_bdf, > + board_name, sizeof(ar->id.default_bdf)) < 0) > + ath10k_warn(ar, > + "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n", > + board_name, sizeof(ar->id.default_bdf)); I suspect, but don't know for sure, that you're going to get another builder splat here. Just like sizeof() isn't guaranteed to return an "unsigned int", it's also not guaranteed to return an "unsigned long". I believe you want %zu. See Documentation/core-api/printk-formats.rst > + > + return 0; > +} > +EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt); Boy, that function seems like overkill for something that you need once at init time. ...and I also suspect that the lifetime of the string returned by of_property_read_string() is valid for as long as your "of_node" is held and thus probably you could use it directly (it likely has a longer lifetime than the location you're storing it). ...but I guess it matches the ath10k_core_check_dt() function above it, so I guess it's fine? -Doug
Hi Abhishek,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kvalo-ath/ath-next]
[also build test WARNING on kvalo-wireless-drivers-next/master kvalo-wireless-drivers/master v5.16 next-20220110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Abhishek-Kumar/ath10k-search-for-default-BDF-name-provided-in-DT/20220111-071636
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git ath-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220111/202201110851.5qAxfQJj-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/50c4c7cb02cc786afcd9aff27616a6e20296c703
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Abhishek-Kumar/ath10k-search-for-default-BDF-name-provided-in-DT/20220111-071636
git checkout 50c4c7cb02cc786afcd9aff27616a6e20296c703
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/net/wireless/ath/ath10k/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/net/wireless/ath/ath10k/core.c: In function 'ath10k_core_parse_default_bdf_dt':
>> drivers/net/wireless/ath/ath10k/core.c:1103:116: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'unsigned int' [-Wformat=]
1103 | "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n",
| ~~^
| |
| long int
| %d
1104 | board_name, sizeof(ar->id.default_bdf));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| unsigned int
vim +1103 drivers/net/wireless/ath/ath10k/core.c
1083
1084 int ath10k_core_parse_default_bdf_dt(struct ath10k *ar)
1085 {
1086 struct device_node *node;
1087 const char *board_name = NULL;
1088
1089 ar->id.default_bdf[0] = '\0';
1090
1091 node = ar->dev->of_node;
1092 if (!node)
1093 return -ENOENT;
1094
1095 of_property_read_string(node, "qcom,ath10k-default-bdf",
1096 &board_name);
1097 if (!board_name)
1098 return -ENODATA;
1099
1100 if (strscpy(ar->id.default_bdf,
1101 board_name, sizeof(ar->id.default_bdf)) < 0)
1102 ath10k_warn(ar,
> 1103 "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n",
1104 board_name, sizeof(ar->id.default_bdf));
1105
1106 return 0;
1107 }
1108 EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt);
1109
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Reviewers, On this patch I have a kernel bot warning, which I intend to fix along with all the comments and discussion and push out V3. So, any comments/next steps are appreciated. Thanks Abhishek On Mon, Jan 10, 2022 at 5:08 PM kernel test robot <lkp@intel.com> wrote: > > Hi Abhishek, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on kvalo-ath/ath-next] > [also build test WARNING on kvalo-wireless-drivers-next/master kvalo-wireless-drivers/master v5.16 next-20220110] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Abhishek-Kumar/ath10k-search-for-default-BDF-name-provided-in-DT/20220111-071636 > base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git ath-next > config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220111/202201110851.5qAxfQJj-lkp@intel.com/config) > compiler: arceb-elf-gcc (GCC) 11.2.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/50c4c7cb02cc786afcd9aff27616a6e20296c703 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Abhishek-Kumar/ath10k-search-for-default-BDF-name-provided-in-DT/20220111-071636 > git checkout 50c4c7cb02cc786afcd9aff27616a6e20296c703 > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/net/wireless/ath/ath10k/ > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > drivers/net/wireless/ath/ath10k/core.c: In function 'ath10k_core_parse_default_bdf_dt': > >> drivers/net/wireless/ath/ath10k/core.c:1103:116: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'unsigned int' [-Wformat=] > 1103 | "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n", > | ~~^ > | | > | long int > | %d > 1104 | board_name, sizeof(ar->id.default_bdf)); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~ > | | > | unsigned int > > > vim +1103 drivers/net/wireless/ath/ath10k/core.c > > 1083 > 1084 int ath10k_core_parse_default_bdf_dt(struct ath10k *ar) > 1085 { > 1086 struct device_node *node; > 1087 const char *board_name = NULL; > 1088 > 1089 ar->id.default_bdf[0] = '\0'; > 1090 > 1091 node = ar->dev->of_node; > 1092 if (!node) > 1093 return -ENOENT; > 1094 > 1095 of_property_read_string(node, "qcom,ath10k-default-bdf", > 1096 &board_name); > 1097 if (!board_name) > 1098 return -ENODATA; > 1099 > 1100 if (strscpy(ar->id.default_bdf, > 1101 board_name, sizeof(ar->id.default_bdf)) < 0) > 1102 ath10k_warn(ar, > > 1103 "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n", > 1104 board_name, sizeof(ar->id.default_bdf)); > 1105 > 1106 return 0; > 1107 } > 1108 EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt); > 1109 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Jan 10, 2022 at 4:51 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Jan 10, 2022 at 3:15 PM Abhishek Kumar <kuabhs@chromium.org> wrote: > > > > +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar) > > +{ > > + struct device_node *node; > > + const char *board_name = NULL; > > + > > + ar->id.default_bdf[0] = '\0'; > > + > > + node = ar->dev->of_node; > > + if (!node) > > + return -ENOENT; > > + > > + of_property_read_string(node, "qcom,ath10k-default-bdf", > > + &board_name); > > + if (!board_name) > > + return -ENODATA; > > + > > + if (strscpy(ar->id.default_bdf, > > + board_name, sizeof(ar->id.default_bdf)) < 0) > > + ath10k_warn(ar, > > + "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n", > > + board_name, sizeof(ar->id.default_bdf)); > > I suspect, but don't know for sure, that you're going to get another > builder splat here. Just like sizeof() isn't guaranteed to return an > "unsigned int", it's also not guaranteed to return an "unsigned long". > I believe you want %zu. See Documentation/core-api/printk-formats.rst Thanks for the tip, I will make this fix in V3. > > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt); > > Boy, that function seems like overkill for something that you need > once at init time. ...and I also suspect that the lifetime of the > string returned by of_property_read_string() is valid for as long as > your "of_node" is held and thus probably you could use it directly (it > likely has a longer lifetime than the location you're storing it). > > ...but I guess it matches the ath10k_core_check_dt() function above > it, so I guess it's fine? Ya, that was my idea to match it with ath10k_core_check_dt, initially, I was planning to remodify ath10k_core_check_dt to parse the new property, but looks it is used it multiple places, so I thought having a separate parser function would be cleaner, however, I am open to new ideas. - Abhishek
Abhishek Kumar <kuabhs@chromium.org> writes: > On this patch I have a kernel bot warning, which I intend to fix along > with all the comments and discussion and push out V3. So, any > comments/next steps are appreciated. Please wait my comments before sending v3, I think this is something which is also needed in ath11k and I need to look at it in detail.
Kalle, On Fri, Jan 14, 2022 at 6:46 AM Kalle Valo <kvalo@kernel.org> wrote: > > Abhishek Kumar <kuabhs@chromium.org> writes: > > > On this patch I have a kernel bot warning, which I intend to fix along > > with all the comments and discussion and push out V3. So, any > > comments/next steps are appreciated. > > Please wait my comments before sending v3, I think this is something > which is also needed in ath11k and I need to look at it in detail. I'm wondering if you have a timeframe for when you might post your comments. We've landed this patch locally in the Chrome OS kernel tree, but we are always also interested in it landing upstream. If you have ideas for a path forward that'd be keen! -Doug
Doug Anderson <dianders@chromium.org> writes: > On Fri, Jan 14, 2022 at 6:46 AM Kalle Valo <kvalo@kernel.org> wrote: >> >> Abhishek Kumar <kuabhs@chromium.org> writes: >> >> > On this patch I have a kernel bot warning, which I intend to fix along >> > with all the comments and discussion and push out V3. So, any >> > comments/next steps are appreciated. >> >> Please wait my comments before sending v3, I think this is something >> which is also needed in ath11k and I need to look at it in detail. > > I'm wondering if you have a timeframe for when you might post your > comments. We've landed this patch locally in the Chrome OS kernel > tree, but we are always also interested in it landing upstream. If you > have ideas for a path forward that'd be keen! You had a good comment on v1 so I replied to that one.
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 8f5b8eb368fa..756856a8eed3 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1081,6 +1081,32 @@ int ath10k_core_check_dt(struct ath10k *ar) } EXPORT_SYMBOL(ath10k_core_check_dt); +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar) +{ + struct device_node *node; + const char *board_name = NULL; + + ar->id.default_bdf[0] = '\0'; + + node = ar->dev->of_node; + if (!node) + return -ENOENT; + + of_property_read_string(node, "qcom,ath10k-default-bdf", + &board_name); + if (!board_name) + return -ENODATA; + + if (strscpy(ar->id.default_bdf, + board_name, sizeof(ar->id.default_bdf)) < 0) + ath10k_warn(ar, + "default board name is longer than allocated buffer, board_name: %s; allocated size: %ld\n", + board_name, sizeof(ar->id.default_bdf)); + + return 0; +} +EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt); + static int ath10k_download_fw(struct ath10k *ar) { u32 address, data_len; @@ -1441,6 +1467,10 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar, if (ret == -ENOENT && fallback_boardname2) ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len); + /* check default BDF name if provided in device tree */ + if (ret == -ENOENT && ar->id.default_bdf[0] != '\0') + ret = ath10k_core_search_bd(ar, ar->id.default_bdf, data, len); + if (ret == -ENOENT) { ath10k_err(ar, "failed to fetch board data for %s from %s/%s\n", diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 9f6680b3be0a..1201bb7bb8ab 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -79,6 +79,9 @@ /* The magic used by QCA spec */ #define ATH10K_SMBIOS_BDF_EXT_MAGIC "BDF_" +/* Default BDF board name buffer size */ +#define ATH10K_DEFAULT_BDF_BUFFER_SIZE 0x40 + /* Default Airtime weight multipler (Tuned for multiclient performance) */ #define ATH10K_AIRTIME_WEIGHT_MULTIPLIER 4 @@ -1102,6 +1105,7 @@ struct ath10k { bool ext_bid_supported; char bdf_ext[ATH10K_SMBIOS_BDF_EXT_STR_LENGTH]; + char default_bdf[ATH10K_DEFAULT_BDF_BUFFER_SIZE]; } id; int fw_api; @@ -1342,6 +1346,7 @@ int ath10k_core_register(struct ath10k *ar, void ath10k_core_unregister(struct ath10k *ar); int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type); int ath10k_core_check_dt(struct ath10k *ar); +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar); void ath10k_core_free_board_files(struct ath10k *ar); #endif /* _CORE_H_ */ diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 80fcb917fe4e..a57675308014 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -831,6 +831,10 @@ static int ath10k_qmi_fetch_board_file(struct ath10k_qmi *qmi) if (ret) ath10k_dbg(ar, ATH10K_DBG_QMI, "DT bdf variant name not set.\n"); + ret = ath10k_core_parse_default_bdf_dt(ar); + if (ret) + ath10k_dbg(ar, ATH10K_DBG_QMI, "Default BDF name not set in device tree.\n"); + return ath10k_core_fetch_board_file(qmi->ar, ATH10K_BD_IE_BOARD); }
There can be cases where the board-2.bin does not contain any BDF matching the chip-id+board-id+variant combination. This causes the wlan probe to fail and renders wifi unusable. For e.g. if the board-2.bin has default BDF as: bus=snoc,qmi-board-id=67 but for some reason the board-id on the wlan chip is not programmed and read 0xff as the default value. In such cases there won't be any matching BDF because the board-2.bin will be searched with following: bus=snoc,qmi-board-id=ff To address these scenarios, there can be an option to provide fallback default BDF name in the device tree. If none of the BDF names match then the board-2.bin file can be searched with default BDF names assigned in the device tree. The default BDF name can be set as: wifi@a000000 { status = "okay"; qcom,ath10k-default-bdf = "bus=snoc,qmi-board-id=67"; }; Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1 Signed-off-by: Abhishek Kumar <kuabhs@chromium.org> --- Changes in v2: Fix printf formatting issue. drivers/net/wireless/ath/ath10k/core.c | 30 ++++++++++++++++++++++++++ drivers/net/wireless/ath/ath10k/core.h | 5 +++++ drivers/net/wireless/ath/ath10k/qmi.c | 4 ++++ 3 files changed, 39 insertions(+)