| Message ID | 20230117092746.1149155-1-equu@openmail.cc (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
On Tue, Jan 17, 2023 at 05:27:46PM +0800, Edward Chow wrote: > Loading calibration data from an OF device tree node not declared > compatible with the device (e.g. a PCI device with calibration data > from corresponding DT node gets replaced, so the newly installed > device become incompatible with the node) or driver may lead to fatal > result, e.g. kernel panic. Please include a link to a bug report and include a few lines of the oops or panic directly in the commit log so when users see this problem, they can search for the text and possibly find this fix. > The driver should check whether the DT node corresponding to the > device compatible with it, and load calibration data only from > compatible node. If you read this commit log carefully, it doesn't actually say what this patch *does*. It has some background and this assertion about what drivers *should* do, but it doesn't say what this patch does. Suggest structure like this (flesh out with the relevant DT property names, etc): For PCI ath9k devices, load calibration data only if there is a DT node corresponding to the device with XXX ... More details: https://chris.beams.io/posts/git-commit/ > Signed-off-by: Edward Chow <equu@openmail.cc> > --- > drivers/net/wireless/ath/ath9k/ath9k.h | 1 + > drivers/net/wireless/ath/ath9k/init.c | 26 ++++++++++++++++++++++++++ > drivers/net/wireless/ath/ath9k/pci.c | 2 +- > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h > index 2cc23605c9fc..4f6f0383a5f8 100644 > --- a/drivers/net/wireless/ath/ath9k/ath9k.h > +++ b/drivers/net/wireless/ath/ath9k/ath9k.h > @@ -35,6 +35,7 @@ struct ath_node; > struct ath_vif; > > extern struct ieee80211_ops ath9k_ops; > +extern struct pci_driver ath_pci_driver; > extern int ath9k_modparam_nohwcrypt; > extern int ath9k_led_blink; > extern bool is_ath9k_unloaded; > diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c > index 4f00400c7ffb..f88a48e8456b 100644 > --- a/drivers/net/wireless/ath/ath9k/init.c > +++ b/drivers/net/wireless/ath/ath9k/init.c > @@ -22,6 +22,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_net.h> > +#include <linux/pci.h> > #include <linux/nvmem-consumer.h> > #include <linux/relay.h> > #include <linux/dmi.h> > @@ -577,6 +578,31 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc) > size_t len; > int err; > > + /* 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 ath9k 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(sc->dev) && ( no OF node || caldata not from node > + * || not compatible ) -> do not use caldata . > + * > + * !dev_is_pci(sc->dev) -> always use caldata . > + */ > + if (dev_is_pci(sc->dev) && > + (!sc->dev->of_node || > + !of_property_match_string(sc->dev->of_node, > + "nvmem-cell-names", > + "calibration") || > + !of_pci_node_match_driver(sc->dev->of_node, > + &ath_pci_driver))) > + /* follow the "just return 0;" convention as > + * noted below. > + */ > + return 0; > + > cell = devm_nvmem_cell_get(sc->dev, "calibration"); > if (IS_ERR(cell)) { > err = PTR_ERR(cell); > diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c > index a074e23013c5..fcb19761e60d 100644 > --- a/drivers/net/wireless/ath/ath9k/pci.c > +++ b/drivers/net/wireless/ath/ath9k/pci.c > @@ -1074,7 +1074,7 @@ static SIMPLE_DEV_PM_OPS(ath9k_pm_ops, ath_pci_suspend, ath_pci_resume); > > MODULE_DEVICE_TABLE(pci, ath_pci_id_table); > > -static struct pci_driver ath_pci_driver = { > +struct pci_driver ath_pci_driver = { > .name = "ath9k", > .id_table = ath_pci_id_table, > .probe = ath_pci_probe, > -- > 2.39.0 >
在 2023/1/18 03:46, Bjorn Helgaas 写道: > On Tue, Jan 17, 2023 at 05:27:46PM +0800, Edward Chow wrote: >> Loading calibration data from an OF device tree node not declared >> compatible with the device (e.g. a PCI device with calibration data >> from corresponding DT node gets replaced, so the newly installed >> device become incompatible with the node) or driver may lead to fatal >> result, e.g. kernel panic. > Please include a link to a bug report and include a few lines of the > oops or panic directly in the commit log so when users see this > problem, they can search for the text and possibly find this fix. > >> The driver should check whether the DT node corresponding to the >> device compatible with it, and load calibration data only from >> compatible node. > If you read this commit log carefully, it doesn't actually say what > this patch *does*. It has some background and this assertion about > what drivers *should* do, but it doesn't say what this patch does. > > Suggest structure like this (flesh out with the relevant DT property > names, etc): > > For PCI ath9k devices, load calibration data only if there is a DT > node corresponding to the device with XXX ... > > More details: https://chris.beams.io/posts/git-commit/ > Thanks for pointing it out. The commit log of the last two patch has been reworded with required content, and sent to the current thread earlier.
On Wed, Feb 01, 2023 at 11:02:59AM +0800, Mad Horse wrote: > 在 2023/1/18 03:46, Bjorn Helgaas 写道: > > Suggest structure like this (flesh out with the relevant DT property > > names, etc): > > > > For PCI ath9k devices, load calibration data only if there is a DT > > node corresponding to the device with XXX ... > > > > More details: https://chris.beams.io/posts/git-commit/ > > > Thanks for pointing it out. The commit log of the last two patch has > been reworded with required content, and sent to the current thread > earlier. Thanks. Sending updated version as replies to the original series posting makes it hard to keep track of what's happening. Better if you send a "v2" series of *all* the patches, even if some of them haven't changed. There are a few hints here, although it's not very explicit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.1#n633 Also, you use multiple email addresses, and if you can figure out how to at least make the display names the same, it will make the conversation easier to follow. Here's what it looks like in mutt: - Jan 7 9:14 Mad Horse [PATCH] pci: Add functions ... - Jan 10 9:24 To Mad Horse ├─> - Jan 17 4:01 Mad Horse ├─>[PATCH 1/3] PCI: of: Match ... - Jan 17 4:02 Mad Horse ├─>[PATCH 2/3] wifi: ath9k: stop ... - Jan 17 4:02 Mad Horse ├─>[PATCH 3/3] wifi: ath10k: only ... - Jan 17 4:29 Mad Horse ├─>[PATCH 1/3] PCI: of: Match pci ... - Jan 17 3:27 Edward Chow ├─>[PATCH 1/3] PCI: of: Match pci ... - Jan 17 3:28 Edward Chow ├─>[PATCH 3/3] wifi: ath10k: only ... - Jan 21 4:00 persmule@gmail. ├─>[PATCH 2/3] wifi: ath9k: stop ... - Jan 21 4:06 persmule@gmail. ├─>[PATCH 3/3] wifi: ath10k: only ... - Jan 17 3:27 Edward Chow └─>[PATCH 2/3] wifi: ath9k: stop ... - Jan 17 13:46 To Edward Chow └─> - Jan 31 21:02 Mad Horse └─> This also shows the problem with updated versions in the same thread. I can work out that "Jan 17 1/3", "Jan 21 2/3", and "Jan 21 3/3" are supposed to be the newest versions, but tools like patchwork and b4 are not smart enough to do that, so it's really hard to apply this. If you just post an updated series, e.g., [PATCH v2 0/3] PCI: ... |-> [PATCH v2 1/3] PCI: of: Match ... |-> [PATCH v2 1/3] PCI: of: Match ... |-> [PATCH v2 1/3] PCI: of: Match ... then everything will be nice and clean and easy to review and apply. Bjorn
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h index 2cc23605c9fc..4f6f0383a5f8 100644 --- a/drivers/net/wireless/ath/ath9k/ath9k.h +++ b/drivers/net/wireless/ath/ath9k/ath9k.h @@ -35,6 +35,7 @@ struct ath_node; struct ath_vif; extern struct ieee80211_ops ath9k_ops; +extern struct pci_driver ath_pci_driver; extern int ath9k_modparam_nohwcrypt; extern int ath9k_led_blink; extern bool is_ath9k_unloaded; diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c index 4f00400c7ffb..f88a48e8456b 100644 --- a/drivers/net/wireless/ath/ath9k/init.c +++ b/drivers/net/wireless/ath/ath9k/init.c @@ -22,6 +22,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_net.h> +#include <linux/pci.h> #include <linux/nvmem-consumer.h> #include <linux/relay.h> #include <linux/dmi.h> @@ -577,6 +578,31 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc) size_t len; int err; + /* 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 ath9k 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(sc->dev) && ( no OF node || caldata not from node + * || not compatible ) -> do not use caldata . + * + * !dev_is_pci(sc->dev) -> always use caldata . + */ + if (dev_is_pci(sc->dev) && + (!sc->dev->of_node || + !of_property_match_string(sc->dev->of_node, + "nvmem-cell-names", + "calibration") || + !of_pci_node_match_driver(sc->dev->of_node, + &ath_pci_driver))) + /* follow the "just return 0;" convention as + * noted below. + */ + return 0; + cell = devm_nvmem_cell_get(sc->dev, "calibration"); if (IS_ERR(cell)) { err = PTR_ERR(cell); diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c index a074e23013c5..fcb19761e60d 100644 --- a/drivers/net/wireless/ath/ath9k/pci.c +++ b/drivers/net/wireless/ath/ath9k/pci.c @@ -1074,7 +1074,7 @@ static SIMPLE_DEV_PM_OPS(ath9k_pm_ops, ath_pci_suspend, ath_pci_resume); MODULE_DEVICE_TABLE(pci, ath_pci_id_table); -static struct pci_driver ath_pci_driver = { +struct pci_driver ath_pci_driver = { .name = "ath9k", .id_table = ath_pci_id_table, .probe = ath_pci_probe,
Loading calibration data from an OF device tree node not declared compatible with the device (e.g. a PCI device with calibration data from corresponding DT node gets replaced, so the newly installed device become incompatible with the node) or driver may lead to fatal result, e.g. kernel panic. The driver should check whether the DT node corresponding to the device compatible with it, and load calibration data only from compatible node. Signed-off-by: Edward Chow <equu@openmail.cc> --- drivers/net/wireless/ath/ath9k/ath9k.h | 1 + drivers/net/wireless/ath/ath9k/init.c | 26 ++++++++++++++++++++++++++ drivers/net/wireless/ath/ath9k/pci.c | 2 +- 3 files changed, 28 insertions(+), 1 deletion(-)