[PATCH] net: mdio: thunder: Do not unregister buses that have not been registered

[PATCH] net: mdio: thunder: Do not unregister buses that have not been registered

[Christophe JAILLET]

In the probe, if 'of_mdiobus_register()' fails, 'nexus->buses[i]' will
still have a non-NULL value.
So in the remove function, we will try to unregister a bus that has not
been registered.

In order to avoid that NULLify 'nexus->buses[i]'.
'oct_mdio_writeq(0,...)' must also be called here.

Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Fixes: 379d7ac7ca31 ("phy: mdio-thunder: Add driver for Cavium Thunder SoC MDIO buses.")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Calling 'devm_mdiobus_free()' would also be cleaner, IMHO.
I've not added it because:
   - it should be fine, even without it
   - I'm not sure how to use it

The best I could think-of (not even compile tested) is:
   devm_mdiobus_free(&pdev->dev, container_of(mii_bus, struct mdiobus_devres, mii));
which is not very nice looking.
(unless I missed something obvious!)

If I'm correct, just passing 'mii_bus' to have something that look logical
would require changing 'devm_mdiobus_alloc_size()' and
'devm_mdiobus_free()'.
---
 drivers/net/mdio/mdio-thunder.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
index 822d2cdd2f35..140c405d4a41 100644
--- a/drivers/net/mdio/mdio-thunder.c
+++ b/drivers/net/mdio/mdio-thunder.c
@@ -97,8 +97,14 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
 		bus->mii_bus->write = cavium_mdiobus_write;
 
 		err = of_mdiobus_register(bus->mii_bus, node);
-		if (err)
+		if (err) {
 			dev_err(&pdev->dev, "of_mdiobus_register failed\n");
+			/* non-registered buses must not be unregistered in
+			 * the .remove function
+			 */
+			oct_mdio_writeq(0, bus->register_base + SMI_EN);
+			nexus->buses[i] = NULL;
+		}
 
 		dev_info(&pdev->dev, "Added bus at %llx\n", r.start);
 		if (i >= ARRAY_SIZE(nexus->buses))
-- 
2.30.2

Re: [PATCH] net: mdio: thunder: Do not unregister buses that have not been registered

[Russell King - ARM Linux admin]

On Thu, May 13, 2021 at 01:51:40PM +0200, Christophe JAILLET wrote:
> In the probe, if 'of_mdiobus_register()' fails, 'nexus->buses[i]' will
> still have a non-NULL value.
> So in the remove function, we will try to unregister a bus that has not
> been registered.
> 
> In order to avoid that NULLify 'nexus->buses[i]'.
> 'oct_mdio_writeq(0,...)' must also be called here.
> 
> Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Fixes: 379d7ac7ca31 ("phy: mdio-thunder: Add driver for Cavium Thunder SoC MDIO buses.")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Calling 'devm_mdiobus_free()' would also be cleaner, IMHO.
> I've not added it because:
>    - it should be fine, even without it
>    - I'm not sure how to use it

devm_mdiobus_free() is a static function not intended to be used by
drivers. There is no devm.*free() function available for this, so
this memory will only ever be freed when either probe fails or the
driver is unbound from its device.

That should be fine, but it would be nice to give that memory back
to the system. Without having a function for drivers to use though,
that's not possible. Such a function should take a struct device
pointer and the struct mii_bus pointer returned by the devm
allocation function.

So, unless Andrew things we really need to free that, what you're
doing below should be fine as far as setting the pointer to NULL.

I think I'd want comments from Cavium on setting the register to
zero - as we don't know how this hardware behaves, and whether that
would have implications we aren't aware of. So, I'm copying in
David Daney (the original driver author) for comment, if his email
address still works!

> ---
>  drivers/net/mdio/mdio-thunder.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
> index 822d2cdd2f35..140c405d4a41 100644
> --- a/drivers/net/mdio/mdio-thunder.c
> +++ b/drivers/net/mdio/mdio-thunder.c
> @@ -97,8 +97,14 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
>  		bus->mii_bus->write = cavium_mdiobus_write;
>  
>  		err = of_mdiobus_register(bus->mii_bus, node);
> -		if (err)
> +		if (err) {
>  			dev_err(&pdev->dev, "of_mdiobus_register failed\n");
> +			/* non-registered buses must not be unregistered in
> +			 * the .remove function
> +			 */
> +			oct_mdio_writeq(0, bus->register_base + SMI_EN);
> +			nexus->buses[i] = NULL;
> +		}
>  
>  		dev_info(&pdev->dev, "Added bus at %llx\n", r.start);
>  		if (i >= ARRAY_SIZE(nexus->buses))
> -- 
> 2.30.2
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mdio: thunder: Do not unregister buses that have not been registered

[Christophe JAILLET]

Le 13/05/2021 à 14:16, Russell King - ARM Linux admin a écrit :
> On Thu, May 13, 2021 at 01:51:40PM +0200, Christophe JAILLET wrote:
>> In the probe, if 'of_mdiobus_register()' fails, 'nexus->buses[i]' will
>> still have a non-NULL value.
>> So in the remove function, we will try to unregister a bus that has not
>> been registered.
>>
>> In order to avoid that NULLify 'nexus->buses[i]'.
>> 'oct_mdio_writeq(0,...)' must also be called here.
>>
>> Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
>> Fixes: 379d7ac7ca31 ("phy: mdio-thunder: Add driver for Cavium Thunder SoC MDIO buses.")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Calling 'devm_mdiobus_free()' would also be cleaner, IMHO.
>> I've not added it because:
>>     - it should be fine, even without it
>>     - I'm not sure how to use it
> 
> devm_mdiobus_free() is a static function not intended to be used by
> drivers. There is no devm.*free() function available for this, so
> this memory will only ever be freed when either probe fails or the
> driver is unbound from its device.
> 
> That should be fine, but it would be nice to give that memory back
> to the system. Without having a function for drivers to use though,
> that's not possible. Such a function should take a struct device
> pointer and the struct mii_bus pointer returned by the devm
> allocation function.
> 
> So, unless Andrew things we really need to free that, what you're
> doing below should be fine as far as setting the pointer to NULL.
> 
> I think I'd want comments from Cavium on setting the register to
> zero - as we don't know how this hardware behaves, and whether that
> would have implications we aren't aware of. So, I'm copying in
> David Daney (the original driver author) for comment, if his email
> address still works!

Hi,

drivers/net/mdio/mdio-thunder.c has been touched recently, so i take the 
opportunity to ping on this old patch.

It does not cleanly apply anymore, but still look valid to me.

CJ

> 
>> ---
>>   drivers/net/mdio/mdio-thunder.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c
>> index 822d2cdd2f35..140c405d4a41 100644
>> --- a/drivers/net/mdio/mdio-thunder.c
>> +++ b/drivers/net/mdio/mdio-thunder.c
>> @@ -97,8 +97,14 @@ static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
>>   		bus->mii_bus->write = cavium_mdiobus_write;
>>   
>>   		err = of_mdiobus_register(bus->mii_bus, node);
>> -		if (err)
>> +		if (err) {
>>   			dev_err(&pdev->dev, "of_mdiobus_register failed\n");
>> +			/* non-registered buses must not be unregistered in
>> +			 * the .remove function
>> +			 */
>> +			oct_mdio_writeq(0, bus->register_base + SMI_EN);
>> +			nexus->buses[i] = NULL;
>> +		}
>>   
>>   		dev_info(&pdev->dev, "Added bus at %llx\n", r.start);
>>   		if (i >= ARRAY_SIZE(nexus->buses))
>> -- 
>> 2.30.2
>>
>>
>