| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2caec4e0-94f4-915c-60d1-c78e7bdc5364@kresin.me>
Date: Tue, 2 Nov 2021 23:08:43 +0100
From: Mathias Kresin <dev@...sin.me>
To: Christian Lamparter <chunkeey@...il.com>, netdev@...r.kernel.org,
linux-acpi@...r.kernel.org, linux-wireless@...r.kernel.org,
ath10k@...ts.infradead.org
Cc: Ansuel Smith <ansuelsmth@...il.com>,
Kalle Valo <kvalo@...eaurora.org>,
"David S . Miller" <davem@...emloft.net>,
"Rafael J . Wysocki" <rafael@...nel.org>,
Jakub Kicinski <kuba@...nel.org>, Len Brown <lenb@...nel.org>,
Andrew Lunn <andrew@...n.ch>, Michael Walle <michael@...le.cc>,
Arnd Bergmann <arnd@...db.de>
Subject: 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 <chunkeey@...il.com>
> ---
> @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, &pdev->dev, ATH10K_BUS_AHB,
> hw_rev, &ath10k_ahb_hif_ops);
> - if (!ar) {
> - dev_err(&pdev->dev, "failed to allocate core\n");
> - return -ENOMEM;
> + if (IS_ERR(ar)) {
> + ret = PTR_ERR(ar);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->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), &pdev->dev, ATH10K_BUS_PCI,
> hw_rev, &ath10k_pci_hif_ops);
> - if (!ar) {
> - dev_err(&pdev->dev, "failed to allocate core\n");
> - return -ENOMEM;
> + if (IS_ERR(ar)) {
> + ret = PTR_ERR(ar);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to allocate core: %d\n", ret);
> + return ret;
> }
>
> ath10k_dbg(ar, ATH10K_DBG_BOOT, "pci probe %04x:%04x %04x:%04x\n",
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index 63e1c2d783c5..87941e047d07 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -2526,9 +2526,11 @@ static int ath10k_sdio_probe(struct sdio_func *func,
>
> ar = ath10k_core_create(sizeof(*ar_sdio), &func->dev, ATH10K_BUS_SDIO,
> hw_rev, &ath10k_sdio_hif_ops);
> - if (!ar) {
> - dev_err(&func->dev, "failed to allocate core\n");
> - return -ENOMEM;
> + if (IS_ERR(ar)) {
> + ret = PTR_ERR(ar);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&func->dev, "failed to allocate core: %d\n", ret);
> + return ret;
> }
>
> netif_napi_add(&ar->napi_dev, &ar->napi, ath10k_sdio_napi_poll,
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 9513ab696fff..b9ac89e226a2 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1728,9 +1728,11 @@ static int ath10k_snoc_probe(struct platform_device *pdev)
>
> ar = ath10k_core_create(sizeof(*ar_snoc), dev, ATH10K_BUS_SNOC,
> drv_data->hw_rev, &ath10k_snoc_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;
> }
>
> ar_snoc = ath10k_snoc_priv(ar);
> diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
> index 3d98f19c6ec8..d6dc830a6fa8 100644
> --- a/drivers/net/wireless/ath/ath10k/usb.c
> +++ b/drivers/net/wireless/ath/ath10k/usb.c
> @@ -987,9 +987,11 @@ static int ath10k_usb_probe(struct usb_interface *interface,
>
> ar = ath10k_core_create(sizeof(*ar_usb), &dev->dev, ATH10K_BUS_USB,
> hw_rev, &ath10k_usb_hif_ops);
> - if (!ar) {
> - dev_err(&dev->dev, "failed to allocate core\n");
> - return -ENOMEM;
> + if (IS_ERR(ar)) {
> + ret = PTR_ERR(ar);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&dev->dev, "failed to allocate core: %d\n", ret);
> + return ret;
> }
>
> usb_get_dev(dev);
>
Powered by blists - more mailing lists