| Message ID | 918382e19fdeb172f3836234d07e706460b7d06b.1620906605.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mdio: thunder: Do not unregister buses that have not been registered | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 15 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 > >
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 >> >> >
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))
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(-)