MIME-Version: 1.0
References: <ab8ff515-19ec-fe3f-0237-c30275e9744d@openmail.cc>
 <20230203104822.361415-1-equu@openmail.cc> <20230203104822.361415-4-equu@openmail.cc>
In-Reply-To: <20230203104822.361415-4-equu@openmail.cc>
From:   Rob Herring <robh@kernel.org>
Date:   Fri, 3 Feb 2023 09:57:59 -0600
Message-ID: <CAL_JsqKq1Yv+svKMS3R=TmDui1VJEjinoPFoDAAgr8tBbV1aSQ@mail.gmail.com>
Subject: Re: [PATCH v5 3/3] wifi: ath10k: only load compatible DT cal data
To:     equu@openmail.cc
Cc:     lpieralisi@kernel.org, toke@toke.dk, kvalo@kernel.org,
        linux-pci@vger.kernel.org, linux-wireless@vger.kernel.org,
        ath10k@lists.infradead.org, kernel test robot <lkp@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 3, 2023 at 4:48 AM <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>
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 30 ++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/pci.c  |  2 +-
>  drivers/net/wireless/ath/ath10k/pci.h  |  2 ++
>  3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 5eb131ab916f..a776b06f49b5 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,33 @@ 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_is_compatible(ar->dev->of_node,
> +                                     "qcom,ath10k") ||
> +            !of_pci_node_match_driver(ar->dev->of_node,
> +                                      &ath10k_pci_driver)))
> +               return -ENOENT;

I think this can be done a bit cleaner and like other drivers. I see 2 options.

The first way is use VID/PID compatible strings and don't set the
of_node pointer if there is a mismatch.

If you must use "qcom,ath10k" (and 9k) only, then we should make
of_device_get_match_data() work on PCI drivers. This should just
require adding of_match_table ptr and it needs a data struct with a
flag saying use cal data or not.

Upon further thought, why can't you decide all this just on PCI
VID/PID? The giant switch statement in ath10k_pci_probe() could all
just be struct of driver_data from the PCI match table.

Rob