| Message ID | 20230203104822.361415-4-equu@openmail.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: of: Load extra data only from compatible DT nodes | expand |
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
> 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. Where should I do this? In pci_set_of_node() from drivers/pci/of.c? > 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. I cannot decide all this just on PCI VID/PID because PCI VID/PID cannot tell whether calibration data are stored in the device (like most expansion cards) or not (for example, in an NVRAM cell referenced by the device tree).
On Fri, Feb 3, 2023 at 11:15 AM <equu@openmail.cc> wrote: > > > 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. > Where should I do this? In pci_set_of_node() from drivers/pci/of.c? Off the top of my head, I think so. > > 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. > > I cannot decide all this just on PCI VID/PID because PCI VID/PID cannot tell whether calibration data are stored in the device (like most expansion cards) or not (for example, in an NVRAM cell referenced by the device tree). > For a given VID/PID, you could have calibration data in DT that you want to ignore sometimes and not other times (because the compatible is wrong)? Rob
> >>> 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. >> I cannot decide all this just on PCI VID/PID because PCI VID/PID cannot tell whether calibration data are stored in the device (like most expansion cards) or not (for example, in an NVRAM cell referenced by the device tree). >> > For a given VID/PID, you could have calibration data in DT that you > want to ignore sometimes and not other times (because the compatible > is wrong)? Some devices will change their VID/PID after applied with calibration data (e.g. AR922X will do 168c:ff1d -> 168c:0029), but most device trees only record their post-calibration VID/PID in their compatibility string. Should we match such device against their pre-calibration VID/PID only, and break all current device trees for them? I think we could add these pre-calibration VID/PIDs to the ID-list of the PCI driver, but had better match compatibility strings against drivers, not devices.
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; + 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/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index 728d607289c3..5d9f6046f8cf 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -3780,7 +3780,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);