[permalink] [raw]

Subject: Re: [PATCH v2 3/3] wifi: ath10k: only load compatible DT cal data

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on helgaas-pci/for-linus wireless-next/main wireless/main linus/master v6.2-rc6 next-20230202]
[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#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/equu-openmail-cc/wifi-ath9k-stop-loading-incompatible-DT-cal-data/20230202-131914
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
patch link: https://lore.kernel.org/r/20230202041823.2879262-4-equu%40openmail.cc
patch subject: [PATCH v2 3/3] wifi: ath10k: only load compatible DT cal data
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230202/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/d3f328c8809b6349090ad2a2ee78250d657edb58
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review equu-openmail-cc/wifi-ath9k-stop-loading-incompatible-DT-cal-data/20230202-131914
git checkout d3f328c8809b6349090ad2a2ee78250d657edb58
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/net/wireless/ath/ath10k/core.c: In function 'ath10k_download_cal_nvmem':
drivers/net/wireless/ath/ath10k/core.c:1985:15: error: implicit declaration of function 'of_pci_node_match_device' [-Werror=implicit-function-declaration]
1985 | !of_pci_node_match_device(ar->dev->of_node,
| ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/ath/ath10k/core.c:1987:24: warning: returning 'void *' from a function with return type 'int' makes integer from pointer without a cast [-Wint-conversion]
1987 | return ERR_PTR(-ENOENT);
| ^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +1987 drivers/net/wireless/ath/ath10k/core.c

1954
1955 static int ath10k_download_cal_nvmem(struct ath10k *ar, const char *cell_name)
1956 {
1957 struct nvmem_cell *cell;
1958 void *buf;
1959 size_t len;
1960 int ret;
1961
1962 /* devm_nvmem_cell_get() will get a cell first from the OF
1963 * DT node representing the given device with nvmem-cell-name
1964 * "calibration", and from the global lookup table as a fallback,
1965 * and an ath9k device could be either a pci one or a platform one.
1966 *
1967 * If the OF DT node is not compatible with the real device, the
1968 * calibration data got from the node should not be applied.
1969 *
1970 * dev_is_pci(ar->dev) && ( no OF node || caldata not from node
1971 * || not compatible ) -> do not use caldata .
1972 *
1973 * !dev_is_pci(ar->dev) -> always use caldata .
1974 *
1975 * The judgement for compatibility differs with ath9k for many
1976 * DT using "qcom,ath10k" as compatibility string.
1977 */
1978 if (dev_is_pci(ar->dev) &&
1979 (!ar->dev->of_node ||
1980 (of_property_match_string(ar->dev->of_node,
1981 "nvmem-cell-names",
1982 cell_name) < 0) ||
1983 !of_device_is_compatible(ar->dev->of_node,
1984 "qcom,ath10k") ||
1985 !of_pci_node_match_device(ar->dev->of_node,
1986 &ath10k_pci_driver)))
> 1987 return ERR_PTR(-ENOENT);
1988
1989 cell = devm_nvmem_cell_get(ar->dev, cell_name);
1990 if (IS_ERR(cell)) {
1991 ret = PTR_ERR(cell);
1992 return ret;
1993 }
1994
1995 buf = nvmem_cell_read(cell, &len);
1996 if (IS_ERR(buf))
1997 return PTR_ERR(buf);
1998
1999 if (ar->hw_params.cal_data_len != len) {
2000 kfree(buf);
2001 ath10k_warn(ar, "invalid calibration data length in nvmem-cell '%s': %zu != %u\n",
2002 cell_name, len, ar->hw_params.cal_data_len);
2003 return -EMSGSIZE;
2004 }
2005
2006 ret = ath10k_download_board_data(ar, buf, len);
2007 kfree(buf);
2008 if (ret)
2009 ath10k_warn(ar, "failed to download calibration data from nvmem-cell '%s': %d\n",
2010 cell_name, ret);
2011
2012 return ret;
2013 }
2014

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-02-02 07:55:55

by Edward Chow

[permalink] [raw]

Subject: [PATCH v3 0/3] PCI: of: Load extra data only from compatible DT nodes

From: Edward Chow <[email protected]>

In order to solve the issue reported in
https://github.com/openwrt/openwrt/pull/11345 , this patchset attempt
to add mechanisms to ckeck whether an OF DT node is compatible to the
PCI device installed on the corresponding location or the driver for
it, and make ath9k and ath10k only load extra data from compatible
nodes.

V2 -> V3:
2, include <linux/of_pci.h> instead for necessary functions.

3, include <linux/of_pci.h> instead for necessary functions,
return error as int variable.

Signed-off-by: Edward Chow <[email protected]>
Reported-by: kernel test robot <[email protected]>

Edward Chow (3):
PCI: of: Match pci devices or drivers against OF DT nodes
wifi: ath9k: stop loading incompatible DT cal data
wifi: ath10k: only load compatible DT cal data

drivers/net/wireless/ath/ath10k/core.c | 29 +++
drivers/net/wireless/ath/ath10k/pci.c | 2 +-
drivers/net/wireless/ath/ath10k/pci.h | 2 +
drivers/net/wireless/ath/ath9k/ath9k.h | 1 +
drivers/net/wireless/ath/ath9k/init.c | 26 +++
drivers/net/wireless/ath/ath9k/pci.c | 2 +-
drivers/pci/of.c | 299 +++++++++++++++++++++++++
drivers/pci/pci-driver.c | 5 -
drivers/pci/pci.h | 56 +++++
include/linux/of_pci.h | 25 +++
include/linux/pci.h | 6 +
11 files changed, 446 insertions(+), 7 deletions(-)

--
2.39.1

2023-02-02 07:56:06

by Edward Chow

[permalink] [raw]

From: Edward Chow <[email protected]>

ath10k might also be sensitive to the issue reported on
https://github.com/openwrt/openwrt/pull/11345 , loading calibration
data from a device tree node declared incompatible.

ath10k will first check whether the device tree node is compatible
with it, using the functionality introduced with the first patch of
this series, ("PCI: of: Match pci devices or drivers against OF DT
nodes") and only proceed loading calibration data from compatible node.

Signed-off-by: Edward Chow <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 31 ++++++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/hw.h | 4 ++++
drivers/net/wireless/ath/ath10k/pci.c | 18 ++++++++++++++-
drivers/net/wireless/ath/ath10k/pci.h | 2 ++
4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 5eb131ab916f..4c9e8140aeff 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -13,6 +13,8 @@
#include <linux/ctype.h>
#include <linux/pm_qos.h>
#include <linux/nvmem-consumer.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
#include <asm/byteorder.h>

#include "core.h"
@@ -26,6 +28,7 @@
#include "testmode.h"
#include "wmi-ops.h"
#include "coredump.h"
+#include "pci.h"

unsigned int ath10k_debug_mask;
EXPORT_SYMBOL(ath10k_debug_mask);
@@ -1958,6 +1961,34 @@ static int ath10k_download_cal_nvmem(struct ath10k *ar, const char *cell_name)
size_t len;
int ret;

+ /* devm_nvmem_cell_get() will get a cell first from the OF
+ * DT node representing the given device with nvmem-cell-name
+ * "calibration", and from the global lookup table as a fallback,
+ * and an ath10k device could be either a pci one or a platform one.
+ *
+ * If the OF DT node is not compatible with the real device, the
+ * calibration data got from the node should not be applied.
+ *
+ * dev_is_pci(ar->dev) && ( no OF node || caldata not from node
+ * || not compatible ) -> do not use caldata .
+ *
+ * !dev_is_pci(ar->dev) -> always use caldata .
+ *
+ * The judgement for compatibility differs with ath9k for many
+ * DT using "qcom,ath10k" as compatibility string.
+ */
+ if (dev_is_pci(ar->dev) &&
+ (!ar->dev->of_node ||
+ (of_property_match_string(ar->dev->of_node,
+ "nvmem-cell-names",
+ cell_name) < 0) ||
+ !of_device_get_match_data(ar->dev) ||
+ !(((const struct ath10k_hw_misc_flags *)
+ of_device_get_match_data(ar->dev))->need_calibration) ||
+ !of_pci_node_match_driver(ar->dev->of_node,
+ &ath10k_pci_driver)))
+ return -ENOENT;
+
cell = devm_nvmem_cell_get(ar->dev, cell_name);
if (IS_ERR(cell)) {
ret = PTR_ERR(cell);
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 9643031a4427..8e19fa637905 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -384,6 +384,10 @@ struct ath10k_hw_values {
bool rfkill_on_level;
};

+struct ath10k_hw_misc_flags {
+ bool need_calibration;
+};
+
extern const struct ath10k_hw_values qca988x_values;
extern const struct ath10k_hw_values qca6174_values;
extern const struct ath10k_hw_values qca99x0_values;
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 728d607289c3..b6ea40631e1b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -97,6 +97,22 @@ static const struct ath10k_pci_supp_chip ath10k_pci_supp_chips[] = {
{ QCA9887_1_0_DEVICE_ID, QCA9887_HW_1_0_CHIP_ID_REV },
};

+static const struct ath10k_hw_misc_flags ath10k_pci_of_flag = {
+ .need_calibration = true,
+}
+
+static const struct of_device_id ath10k_pci_of_match[] = {
+ /* OF DT nodes for a generic ath10k pci devices
+ * usually use this compatibility string.
+ */
+ { .compatible = "qcom,ath10k",
+ .data = &ath10k_pci_of_flag
+ },
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, ath10k_pci_of_match);
+
static void ath10k_pci_buffer_cleanup(struct ath10k *ar);
static int ath10k_pci_cold_reset(struct ath10k *ar);
static int ath10k_pci_safe_chip_reset(struct ath10k *ar);
@@ -3780,7 +3796,7 @@ static SIMPLE_DEV_PM_OPS(ath10k_pci_pm_ops,
ath10k_pci_pm_suspend,
ath10k_pci_pm_resume);

-static struct pci_driver ath10k_pci_driver = {
+struct pci_driver ath10k_pci_driver = {
.name = "ath10k_pci",
.id_table = ath10k_pci_id_table,
.probe = ath10k_pci_probe,
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 480cd97ab739..de676797b736 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -209,6 +209,8 @@ static inline struct ath10k_pci *ath10k_pci_priv(struct ath10k *ar)
#define DIAG_ACCESS_CE_TIMEOUT_US 10000 /* 10 ms */
#define DIAG_ACCESS_CE_WAIT_US 50

+extern struct pci_driver ath10k_pci_driver;
+
void ath10k_pci_write32(struct ath10k *ar, u32 offset, u32 value);
void ath10k_pci_soc_write32(struct ath10k *ar, u32 addr, u32 val);
void ath10k_pci_reg_write32(struct ath10k *ar, u32 addr, u32 val);
--
2.39.1

2023-02-09 16:09:29

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v6 3/3] wifi: ath10k: only load compatible DT cal data

On Wed, Feb 8, 2023 at 10:51 PM <[email protected]> wrote:
>
> From: Edward Chow <[email protected]>
>
> ath10k might also be sensitive to the issue reported on
> https://github.com/openwrt/openwrt/pull/11345 , loading calibration
> data from a device tree node declared incompatible.
>
> ath10k will first check whether the device tree node is compatible
> with it, using the functionality introduced with the first patch of
> this series, ("PCI: of: Match pci devices or drivers against OF DT
> nodes") and only proceed loading calibration data from compatible node.
>
> Signed-off-by: Edward Chow <[email protected]>
> Reported-by: kernel test robot <[email protected]>

This is for fixes created as a result of kernel test robot report.
Reports on your broken patches should not have this.

> ---
> drivers/net/wireless/ath/ath10k/core.c | 31 ++++++++++++++++++++++++++
> drivers/net/wireless/ath/ath10k/hw.h | 4 ++++
> drivers/net/wireless/ath/ath10k/pci.c | 18 ++++++++++++++-
> drivers/net/wireless/ath/ath10k/pci.h | 2 ++
> 4 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 5eb131ab916f..4c9e8140aeff 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -13,6 +13,8 @@
> #include <linux/ctype.h>
> #include <linux/pm_qos.h>
> #include <linux/nvmem-consumer.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> #include <asm/byteorder.h>
>
> #include "core.h"
> @@ -26,6 +28,7 @@
> #include "testmode.h"
> #include "wmi-ops.h"
> #include "coredump.h"
> +#include "pci.h"
>
> unsigned int ath10k_debug_mask;
> EXPORT_SYMBOL(ath10k_debug_mask);
> @@ -1958,6 +1961,34 @@ static int ath10k_download_cal_nvmem(struct ath10k *ar, const char *cell_name)
> size_t len;
> int ret;
>
> + /* devm_nvmem_cell_get() will get a cell first from the OF
> + * DT node representing the given device with nvmem-cell-name
> + * "calibration", and from the global lookup table as a fallback,
> + * and an ath10k device could be either a pci one or a platform one.
> + *
> + * If the OF DT node is not compatible with the real device, the
> + * calibration data got from the node should not be applied.
> + *
> + * dev_is_pci(ar->dev) && ( no OF node || caldata not from node
> + * || not compatible ) -> do not use caldata .
> + *
> + * !dev_is_pci(ar->dev) -> always use caldata .
> + *
> + * The judgement for compatibility differs with ath9k for many
> + * DT using "qcom,ath10k" as compatibility string.
> + */
> + if (dev_is_pci(ar->dev) &&
> + (!ar->dev->of_node ||
> + (of_property_match_string(ar->dev->of_node,
> + "nvmem-cell-names",
> + cell_name) < 0) ||
> + !of_device_get_match_data(ar->dev) ||
> + !(((const struct ath10k_hw_misc_flags *)
> + of_device_get_match_data(ar->dev))->need_calibration) ||
> + !of_pci_node_match_driver(ar->dev->of_node,
> + &ath10k_pci_driver)))

That is just plain ugly and not understandable. Why do you still need
of_pci_node_match_driver()? If compatible using VID/PID doesn't match
the actual VID/PID, then you should never probe.

The prior explanations didn't really clear things up either. I'm
really at a loss as to what are the scenarios you need to work. Please
enumerate what are the different scenarios of what's in the DTs and
how you need the kernel/driver to respond.

Rob