[RFC net-next/wireless-next v1 1/2] ethernet: check nvmem cells for mac-address

[RFC net-next/wireless-next v1 1/2] ethernet: check nvmem cells for mac-address

[Christian Lamparter]

of_get_mac_address() gained this function with
commit d01f449c008a ("of_net: add NVMEM support to of_get_mac_address")
with the following justification:
"Many embedded devices have information such as MAC addresses stored
inside NVMEMs like EEPROMs and so on. [...]

Adding support for NVMEM into every other driver would mean adding a lot
of repetitive code. This patch allows us to configure MAC addresses in
various devices like ethernet and wireless adapters directly [...]."

the common device_/fwnode_get_mac_address could use the
same functionality by just a few adjustments.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 net/ethernet/eth.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index c7d9e08107cb..5e55b08577b1 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -602,7 +602,9 @@ static int fwnode_get_mac_addr(struct fwnode_handle *fwnode,
  * checked first, because that is supposed to contain to "most recent" MAC
  * address. If that isn't set, then 'local-mac-address' is checked next,
  * because that is the default address.  If that isn't set, then the obsolete
- * 'address' is checked, just in case we're using an old device tree.
+ * 'address' is checked, just in case we're using an old device tree. If any
+ * of the above isn't set, then try to get MAC address from nvmem cell named
+ * 'mac-address'.
  *
  * Note that the 'address' property is supposed to contain a virtual address of
  * the register set, but some DTS files have redefined that property to be the
@@ -622,7 +624,7 @@ int fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr)
 	    !fwnode_get_mac_addr(fwnode, "address", addr))
 		return 0;
 
-	return -ENOENT;
+	return nvmem_get_mac_address(fwnode->dev, addr);
 }
 EXPORT_SYMBOL(fwnode_get_mac_address);
 
-- 
2.33.1

[RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain

[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@gmail.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);
+		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);
-- 
2.33.1

Re: [RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain

[Mathias Kresin]

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@gmail.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);
> 

Re: [RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain

[Christian Lamparter]

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, &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().

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