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

[Rob Herring <robh@kernel.org>]

On Wed, Feb 8, 2023 at 10:51 PM <equu@openmail.cc> wrote:
>
> From: Edward Chow <equu@openmail.cc>
>
> 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 <equu@openmail.cc>
> Reported-by: kernel test robot <lkp@intel.com>

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