Re: [RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain
Hi Mathias, On 02/11/2021 23:08, Mathias Kresin wrote: 10/30/21 7:41 PM, Christian Lamparter: --- a/drivers/net/wireless/ath/ath10k/ahb.c +++ b/drivers/net/wireless/ath/ath10k/ahb.c @@ -745,9 +745,11 @@ static int ath10k_ahb_probe(struct platform_device *pdev) size = sizeof(*ar_pci) + sizeof(*ar_ahb); ar = ath10k_core_create(size, >dev, ATH10K_BUS_AHB, hw_rev, _ahb_hif_ops); - if (!ar) { - dev_err(>dev, "failed to allocate core\n"); - return -ENOMEM; + if (IS_ERR(ar)) { + ret = PTR_ERR(ar); + if (ret != -EPROBE_DEFER) + dev_err(>dev, "failed to allocate core: %d\n", ret); There's a helper for that: dev_err_probe(). I was looking for that. Thank you! :-) (I need to check if this device_get_mac_address() all works with 5.15-next or not. It's probably easier to wait until 5.16-rc1-wt gets released). Regards, Christian ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain
Hey Christian, just a drive-by comment inline. Mathias 10/30/21 7:41 PM, Christian Lamparter: device_get_mac_address() can now return -EPROBE_DEFER. This has to be passed back to the device subsystem so the driver will be probed again at a later time. This was somewhat involved because the best place for this seemed in ath10k_core_create() right after allocation. Thing is that ath10k_core_create() was setup to either return a valid ath10k* instance, or NULL. So each ath10k implementation has to be modified to account for ERR_PTR. This introduces a new side-effect: the returned error codes from ath10k_core_create() will now be passed along. It's no longer just -ENOMEM. Note: If device_get_mac_address() didn't get a valid MAC from either the DT/ACPI, nvmem, etc... the driver will just generate random MAC (same as it did before). Signed-off-by: Christian Lamparter --- @Kalle from what I can tell, this is how nvmem-mac could be done with the existing device_get_mac_address() - at a different place. The reason for the move was that -EPROBE_DEFER needs to be returned by the pci/usb/snoc/ahb _probe functions(). This wasn't possible in the old location. As ath10k deferres the "bring-up" process into a workqueue task which can't return any errors (it just printk/dev_err them at the end). Also, When I was asking around about this. The common consensus was to just post it and see. This is based on net-next + wireless-testing drivers/net/wireless/ath/ath10k/ahb.c | 8 +--- drivers/net/wireless/ath/ath10k/core.c | 14 -- drivers/net/wireless/ath/ath10k/pci.c | 8 +--- drivers/net/wireless/ath/ath10k/sdio.c | 8 +--- drivers/net/wireless/ath/ath10k/snoc.c | 8 +--- drivers/net/wireless/ath/ath10k/usb.c | 8 +--- 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c index ab8f77ae5e66..ad282a06b376 100644 --- a/drivers/net/wireless/ath/ath10k/ahb.c +++ b/drivers/net/wireless/ath/ath10k/ahb.c @@ -745,9 +745,11 @@ static int ath10k_ahb_probe(struct platform_device *pdev) size = sizeof(*ar_pci) + sizeof(*ar_ahb); ar = ath10k_core_create(size, >dev, ATH10K_BUS_AHB, hw_rev, _ahb_hif_ops); - if (!ar) { - dev_err(>dev, "failed to allocate core\n"); - return -ENOMEM; + if (IS_ERR(ar)) { + ret = PTR_ERR(ar); + if (ret != -EPROBE_DEFER) + dev_err(>dev, "failed to allocate core: %d\n", ret); There's a helper for that: dev_err_probe(). + return ret; } ath10k_dbg(ar, ATH10K_DBG_BOOT, "ahb probe\n"); diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 72a366aa9f60..85d2e8143101 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -3291,8 +3291,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar) ath10k_debug_print_board_info(ar); } - device_get_mac_address(ar->dev, ar->mac_addr); - ret = ath10k_core_init_firmware_features(ar); if (ret) { ath10k_err(ar, "fatal problem with firmware features: %d\n", @@ -3451,11 +3449,11 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev, const struct ath10k_hif_ops *hif_ops) { struct ath10k *ar; - int ret; + int ret = -ENOMEM; ar = ath10k_mac_create(priv_size); if (!ar) - return NULL; + goto err_out; ar->ath_common.priv = ar; ar->ath_common.hw = ar->hw; @@ -3464,6 +3462,10 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev, ar->hif.ops = hif_ops; ar->hif.bus = bus; + ret = device_get_mac_address(dev, ar->mac_addr); + if (ret == -EPROBE_DEFER) + goto err_free_mac; + switch (hw_rev) { case ATH10K_HW_QCA988X: case ATH10K_HW_QCA9887: @@ -3580,8 +3582,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev, destroy_workqueue(ar->workqueue); err_free_mac: ath10k_mac_destroy(ar); - - return NULL; +err_out: + return ERR_PTR(ret); } EXPORT_SYMBOL(ath10k_core_create); diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index 4d4e2f91e15c..f4736148a382 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -3602,9 +3602,11 @@ static int ath10k_pci_probe(struct pci_dev *pdev, ar = ath10k_core_create(sizeof(*ar_pci), >dev, ATH10K_BUS_PCI, hw_rev, _pci_hif_ops); - if (!ar) { - dev_err(>dev, "failed to allocate core\n"); - return -ENOMEM; + if (IS_ERR(ar)) { + ret = PTR_ERR(ar); +
[RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain
device_get_mac_address() can now return -EPROBE_DEFER. This has to be passed back to the device subsystem so the driver will be probed again at a later time. This was somewhat involved because the best place for this seemed in ath10k_core_create() right after allocation. Thing is that ath10k_core_create() was setup to either return a valid ath10k* instance, or NULL. So each ath10k implementation has to be modified to account for ERR_PTR. This introduces a new side-effect: the returned error codes from ath10k_core_create() will now be passed along. It's no longer just -ENOMEM. Note: If device_get_mac_address() didn't get a valid MAC from either the DT/ACPI, nvmem, etc... the driver will just generate random MAC (same as it did before). Signed-off-by: Christian Lamparter --- @Kalle from what I can tell, this is how nvmem-mac could be done with the existing device_get_mac_address() - at a different place. The reason for the move was that -EPROBE_DEFER needs to be returned by the pci/usb/snoc/ahb _probe functions(). This wasn't possible in the old location. As ath10k deferres the "bring-up" process into a workqueue task which can't return any errors (it just printk/dev_err them at the end). Also, When I was asking around about this. The common consensus was to just post it and see. This is based on net-next + wireless-testing drivers/net/wireless/ath/ath10k/ahb.c | 8 +--- drivers/net/wireless/ath/ath10k/core.c | 14 -- drivers/net/wireless/ath/ath10k/pci.c | 8 +--- drivers/net/wireless/ath/ath10k/sdio.c | 8 +--- drivers/net/wireless/ath/ath10k/snoc.c | 8 +--- drivers/net/wireless/ath/ath10k/usb.c | 8 +--- 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c index ab8f77ae5e66..ad282a06b376 100644 --- a/drivers/net/wireless/ath/ath10k/ahb.c +++ b/drivers/net/wireless/ath/ath10k/ahb.c @@ -745,9 +745,11 @@ static int ath10k_ahb_probe(struct platform_device *pdev) size = sizeof(*ar_pci) + sizeof(*ar_ahb); ar = ath10k_core_create(size, >dev, ATH10K_BUS_AHB, hw_rev, _ahb_hif_ops); - if (!ar) { - dev_err(>dev, "failed to allocate core\n"); - return -ENOMEM; + if (IS_ERR(ar)) { + ret = PTR_ERR(ar); + if (ret != -EPROBE_DEFER) + dev_err(>dev, "failed to allocate core: %d\n", ret); + return ret; } ath10k_dbg(ar, ATH10K_DBG_BOOT, "ahb probe\n"); diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 72a366aa9f60..85d2e8143101 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -3291,8 +3291,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar) ath10k_debug_print_board_info(ar); } - device_get_mac_address(ar->dev, ar->mac_addr); - ret = ath10k_core_init_firmware_features(ar); if (ret) { ath10k_err(ar, "fatal problem with firmware features: %d\n", @@ -3451,11 +3449,11 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev, const struct ath10k_hif_ops *hif_ops) { struct ath10k *ar; - int ret; + int ret = -ENOMEM; ar = ath10k_mac_create(priv_size); if (!ar) - return NULL; + goto err_out; ar->ath_common.priv = ar; ar->ath_common.hw = ar->hw; @@ -3464,6 +3462,10 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev, ar->hif.ops = hif_ops; ar->hif.bus = bus; + ret = device_get_mac_address(dev, ar->mac_addr); + if (ret == -EPROBE_DEFER) + goto err_free_mac; + switch (hw_rev) { case ATH10K_HW_QCA988X: case ATH10K_HW_QCA9887: @@ -3580,8 +3582,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev, destroy_workqueue(ar->workqueue); err_free_mac: ath10k_mac_destroy(ar); - - return NULL; +err_out: + return ERR_PTR(ret); } EXPORT_SYMBOL(ath10k_core_create); diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index 4d4e2f91e15c..f4736148a382 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -3602,9 +3602,11 @@ static int ath10k_pci_probe(struct pci_dev *pdev, ar = ath10k_core_create(sizeof(*ar_pci), >dev, ATH10K_BUS_PCI, hw_rev, _pci_hif_ops); - if (!ar) { - dev_err(>dev, "failed to allocate core\n"); - return -ENOMEM; + if (IS_ERR(ar)) { + ret = PTR_ERR(ar); + if (ret != -EPROBE_DEFER) + dev_err(>dev, "failed to allocate core: %d\n", ret); + return ret;