[RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[Boris BREZILLON]

Hello,

This patch is a proposal to support the register/unregister/register
sequence on a given mdio bus.

I use the register/unregister/register sequence to add a fallback when the
of_mdiobus_register (this function calls mdiobus_register with phy_mask
set to ~0) does not register any phy device (because the device tree does
not define any phy).
In this case I call mdiobus_unregister and then call mdiobus_register with
a phy_mask set to 0 to trigger a full mdio bus scan.

I'm not sure this is the right way to do it (this is why I added RFC in the
subject).

Could someone help me figuring out what I should use to implement my fallback ?

1) use the register/unregister/register sequence
2) reimplement the "for (i = 0; i < PHY_MAX_ADDR; i++)" mdiobus_scan loop

Thanks.

Best Regards,
Boris

Boris BREZILLON (1):
  phylib: mdio: handle register/unregister/register sequence

 drivers/net/phy/mdio_bus.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[Boris BREZILLON]

The current implementation does not accept this sequence on a given mdio
bus: register/unregister/register.

The device core framework complain about already initialized kobject
struct:

"kobject (df9e9848): tried to init an initialized object, something is
seriously wrong."

This patch replaces the device_register call with device_add in
mdiobus_register and move the device struct initialization in the
mdiobus_alloc_size function.
Thus the device struct is only initialized once.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 drivers/net/phy/mdio_bus.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index dc92097..a256de3 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -68,6 +68,8 @@ struct mii_bus *mdiobus_alloc_size(size_t size)
 			bus->priv = (void *)bus + aligned_size;
 	}
 
+	device_initialize(&bus->dev);
+
 	return bus;
 }
 EXPORT_SYMBOL(mdiobus_alloc_size);
@@ -151,7 +153,7 @@ int mdiobus_register(struct mii_bus *bus)
 	bus->dev.groups = NULL;
 	dev_set_name(&bus->dev, "%s", bus->id);
 
-	err = device_register(&bus->dev);
+	err = device_add(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);
 		return -EINVAL;
@@ -195,12 +197,12 @@ void mdiobus_unregister(struct mii_bus *bus)
 	BUG_ON(bus->state != MDIOBUS_REGISTERED);
 	bus->state = MDIOBUS_UNREGISTERED;
 
-	device_del(&bus->dev);
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if (bus->phy_map[i])
 			device_unregister(&bus->phy_map[i]->dev);
 		bus->phy_map[i] = NULL;
 	}
+	device_del(&bus->dev);
 }
 EXPORT_SYMBOL(mdiobus_unregister);
 
-- 
1.7.9.5

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[Florian Fainelli]

Hello Boris,

2013/8/22 Boris BREZILLON <b.brezillon@overkiz.com>:
> Hello,
>
> This patch is a proposal to support the register/unregister/register
> sequence on a given mdio bus.
>
> I use the register/unregister/register sequence to add a fallback when the
> of_mdiobus_register (this function calls mdiobus_register with phy_mask
> set to ~0) does not register any phy device (because the device tree does
> not define any phy).
> In this case I call mdiobus_unregister and then call mdiobus_register with
> a phy_mask set to 0 to trigger a full mdio bus scan.
>
> I'm not sure this is the right way to do it (this is why I added RFC in the
> subject).
>
> Could someone help me figuring out what I should use to implement my fallback ?
>
> 1) use the register/unregister/register sequence
> 2) reimplement the "for (i = 0; i < PHY_MAX_ADDR; i++)" mdiobus_scan loop

I think solution 2 is nicer, in that case, would it be enough in your
case to export a function called mdiobus_scan()? You could call at a
time you know PHY devices have a chance of having been probed?
-- 
Florian

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[boris brezillon]

Hello Florian,

Thanks for your answer.

On 22/08/2013 14:43, Florian Fainelli wrote:
> Hello Boris,
>
> 2013/8/22 Boris BREZILLON <b.brezillon@overkiz.com>:
>> Hello,
>>
>> This patch is a proposal to support the register/unregister/register
>> sequence on a given mdio bus.
>>
>> I use the register/unregister/register sequence to add a fallback when the
>> of_mdiobus_register (this function calls mdiobus_register with phy_mask
>> set to ~0) does not register any phy device (because the device tree does
>> not define any phy).
>> In this case I call mdiobus_unregister and then call mdiobus_register with
>> a phy_mask set to 0 to trigger a full mdio bus scan.
>>
>> I'm not sure this is the right way to do it (this is why I added RFC in the
>> subject).
>>
>> Could someone help me figuring out what I should use to implement my fallback ?
>>
>> 1) use the register/unregister/register sequence
>> 2) reimplement the "for (i = 0; i < PHY_MAX_ADDR; i++)" mdiobus_scan loop
> I think solution 2 is nicer, in that case, would it be enough in your
> case to export a function called mdiobus_scan()? You could call at a
> time you know PHY devices have a chance of having been probed?
mdiobus_scan is already exported:
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);

This function scans the presence of a phy device at a given address.

What I need is a loop which scan all the possible address on the given
mdio bus:

struct phy_device *mdiobus_full_scan(struct mii_bus *bus)
{
     int i;
     for (i = 0; i < PHY_MAX_ADDR; i++) {
         if ((bus->phy_mask & (1 << i)) == 0) {
             struct phy_device *phydev;

             phydev = mdiobus_scan(bus, i);
             if (IS_ERR(phydev)) {
                 err = PTR_ERR(phydev);
                 goto error;
             }
         }
     }
     return 0;

error:
     while (--i >= 0) {
         if (bus->phy_map[i])
             device_unregister(&bus->phy_map[i]->dev);
     }
}
EXPORT_SYMBOL(mdiobus_full_scan);

Since I am the only one who need this kind of functionnality right now, 
I'm not sure
this is a good idea to export a new function.

This behaviour may be implemented in the of_mdiobus_register function:
when no dt phy node are found in the mdio bus dt node, we could launch a 
full
scan.

What do you think ?

Best Regards,

Boris

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[Florian Fainelli]

2013/8/22 boris brezillon <b.brezillon@overkiz.com>:
> Hello Florian,
>
> Thanks for your answer.
>
>
> On 22/08/2013 14:43, Florian Fainelli wrote:
>>
>> Hello Boris,
>>
>> 2013/8/22 Boris BREZILLON <b.brezillon@overkiz.com>:
>>>
>>> Hello,
>>>
>>> This patch is a proposal to support the register/unregister/register
>>> sequence on a given mdio bus.
>>>
>>> I use the register/unregister/register sequence to add a fallback when
>>> the
>>> of_mdiobus_register (this function calls mdiobus_register with phy_mask
>>> set to ~0) does not register any phy device (because the device tree does
>>> not define any phy).
>>> In this case I call mdiobus_unregister and then call mdiobus_register
>>> with
>>> a phy_mask set to 0 to trigger a full mdio bus scan.
>>>
>>> I'm not sure this is the right way to do it (this is why I added RFC in
>>> the
>>> subject).
>>>
>>> Could someone help me figuring out what I should use to implement my
>>> fallback ?
>>>
>>> 1) use the register/unregister/register sequence
>>> 2) reimplement the "for (i = 0; i < PHY_MAX_ADDR; i++)" mdiobus_scan loop
>>
>> I think solution 2 is nicer, in that case, would it be enough in your
>> case to export a function called mdiobus_scan()? You could call at a
>> time you know PHY devices have a chance of having been probed?
>
> mdiobus_scan is already exported:
> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>
> This function scans the presence of a phy device at a given address.
>
> What I need is a loop which scan all the possible address on the given
> mdio bus:
>
> struct phy_device *mdiobus_full_scan(struct mii_bus *bus)
> {
>     int i;
>     for (i = 0; i < PHY_MAX_ADDR; i++) {
>         if ((bus->phy_mask & (1 << i)) == 0) {
>             struct phy_device *phydev;
>
>             phydev = mdiobus_scan(bus, i);
>             if (IS_ERR(phydev)) {
>                 err = PTR_ERR(phydev);
>                 goto error;
>             }
>         }
>     }
>     return 0;
>
> error:
>     while (--i >= 0) {
>
>         if (bus->phy_map[i])
>             device_unregister(&bus->phy_map[i]->dev);
>     }
> }
> EXPORT_SYMBOL(mdiobus_full_scan);
>
> Since I am the only one who need this kind of functionnality right now, I'm
> not sure
> this is a good idea to export a new function.

A possible other use case for this full-scan is when you do not detect
a PHY connected to your MDIO bus, and that you did not register a
fixed PHY early enough for it to have been scanned by the fixed MDIO
bus emulation. In that case the driver may:

- scan hardware MDIO bus
- do not find any PHY, register a fixed PHY
- trigger a fixed MDIO bus full-rescan
- attach to the discovered fixed PHY

this is something currently done by the TI CPMAC driver in
drivers/net/ethernet/ti/cpmac.c (altough fixed_phy_add() is called
from platform code).

>
> This behaviour may be implemented in the of_mdiobus_register function:
> when no dt phy node are found in the mdio bus dt node, we could launch a
> full
> scan.
>
> What do you think ?

There is an existing kind of "autoscan" feature in
drivers/of/of_mdio.c, starting with the second foreach_child_node()
loop, so maybe that specific part could be exported and would achieve
what you are looking for? It relies on the Ethernet PHY nodes to be
attached to the MDIO bus node, but I assume this is what ultimately
happens in your case as well?
-- 
Florian

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[boris brezillon]

On 22/08/2013 15:15, Florian Fainelli wrote:
> 2013/8/22 boris brezillon <b.brezillon@overkiz.com>:
>> Hello Florian,
>>
>> Thanks for your answer.
>>
>>
>> On 22/08/2013 14:43, Florian Fainelli wrote:
>>> Hello Boris,
>>>
>>> 2013/8/22 Boris BREZILLON <b.brezillon@overkiz.com>:
>>>> Hello,
>>>>
>>>> This patch is a proposal to support the register/unregister/register
>>>> sequence on a given mdio bus.
>>>>
>>>> I use the register/unregister/register sequence to add a fallback when
>>>> the
>>>> of_mdiobus_register (this function calls mdiobus_register with phy_mask
>>>> set to ~0) does not register any phy device (because the device tree does
>>>> not define any phy).
>>>> In this case I call mdiobus_unregister and then call mdiobus_register
>>>> with
>>>> a phy_mask set to 0 to trigger a full mdio bus scan.
>>>>
>>>> I'm not sure this is the right way to do it (this is why I added RFC in
>>>> the
>>>> subject).
>>>>
>>>> Could someone help me figuring out what I should use to implement my
>>>> fallback ?
>>>>
>>>> 1) use the register/unregister/register sequence
>>>> 2) reimplement the "for (i = 0; i < PHY_MAX_ADDR; i++)" mdiobus_scan loop
>>> I think solution 2 is nicer, in that case, would it be enough in your
>>> case to export a function called mdiobus_scan()? You could call at a
>>> time you know PHY devices have a chance of having been probed?
>> mdiobus_scan is already exported:
>> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>>
>> This function scans the presence of a phy device at a given address.
>>
>> What I need is a loop which scan all the possible address on the given
>> mdio bus:
>>
>> struct phy_device *mdiobus_full_scan(struct mii_bus *bus)
>> {
>>      int i;
>>      for (i = 0; i < PHY_MAX_ADDR; i++) {
>>          if ((bus->phy_mask & (1 << i)) == 0) {
>>              struct phy_device *phydev;
>>
>>              phydev = mdiobus_scan(bus, i);
>>              if (IS_ERR(phydev)) {
>>                  err = PTR_ERR(phydev);
>>                  goto error;
>>              }
>>          }
>>      }
>>      return 0;
>>
>> error:
>>      while (--i >= 0) {
>>
>>          if (bus->phy_map[i])
>>              device_unregister(&bus->phy_map[i]->dev);
>>      }
>> }
>> EXPORT_SYMBOL(mdiobus_full_scan);
>>
>> Since I am the only one who need this kind of functionnality right now, I'm
>> not sure
>> this is a good idea to export a new function.
> A possible other use case for this full-scan is when you do not detect
> a PHY connected to your MDIO bus, and that you did not register a
> fixed PHY early enough for it to have been scanned by the fixed MDIO
> bus emulation. In that case the driver may:
>
> - scan hardware MDIO bus
> - do not find any PHY, register a fixed PHY
> - trigger a fixed MDIO bus full-rescan
> - attach to the discovered fixed PHY
>
> this is something currently done by the TI CPMAC driver in
> drivers/net/ethernet/ti/cpmac.c (altough fixed_phy_add() is called
> from platform code).
>

Okay, then we should consider this option.

>> This behaviour may be implemented in the of_mdiobus_register function:
>> when no dt phy node are found in the mdio bus dt node, we could launch a
>> full
>> scan.
>>
>> What do you think ?
> There is an existing kind of "autoscan" feature in
> drivers/of/of_mdio.c, starting with the second foreach_child_node()
> loop, so maybe that specific part could be exported and would achieve
> what you are looking for? It relies on the Ethernet PHY nodes to be
> attached to the MDIO bus node, but I assume this is what ultimately
> happens in your case as well?

The second foreach_child_node loop only registers the dt phy nodes
which does not define any reg property (automatic address asssignement ?).

Indeed, what I need is a fallback when the device tree does not define 
any phy
device (for old device tree backward compatibility).

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[boris brezillon]

On 22/08/2013 14:33, Boris BREZILLON wrote:
> Hello,
>
> This patch is a proposal to support the register/unregister/register
> sequence on a given mdio bus.

I forgot to ask, if this limitation was made on purpose ?
In other terms: no one should ever try to register, unregister and 
register again
a given mii_bus structure.

>
> I use the register/unregister/register sequence to add a fallback when the
> of_mdiobus_register (this function calls mdiobus_register with phy_mask
> set to ~0) does not register any phy device (because the device tree does
> not define any phy).
> In this case I call mdiobus_unregister and then call mdiobus_register with
> a phy_mask set to 0 to trigger a full mdio bus scan.
>
> I'm not sure this is the right way to do it (this is why I added RFC in the
> subject).
>
> Could someone help me figuring out what I should use to implement my fallback ?
>
> 1) use the register/unregister/register sequence
> 2) reimplement the "for (i = 0; i < PHY_MAX_ADDR; i++)" mdiobus_scan loop
>
> Thanks.
>
> Best Regards,
> Boris
>
> Boris BREZILLON (1):
>    phylib: mdio: handle register/unregister/register sequence
>
>   drivers/net/phy/mdio_bus.c |    6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[Greg Kroah-Hartman]

On Thu, Aug 22, 2013 at 02:33:56PM +0200, Boris BREZILLON wrote:
> Hello,
> 
> This patch is a proposal to support the register/unregister/register
> sequence on a given mdio bus.
> 
> I use the register/unregister/register sequence to add a fallback when the
> of_mdiobus_register (this function calls mdiobus_register with phy_mask
> set to ~0) does not register any phy device (because the device tree does
> not define any phy).
> In this case I call mdiobus_unregister and then call mdiobus_register with
> a phy_mask set to 0 to trigger a full mdio bus scan.
> 
> I'm not sure this is the right way to do it (this is why I added RFC in the
> subject).
> 
> Could someone help me figuring out what I should use to implement my fallback ?
> 
> 1) use the register/unregister/register sequence

That will not work.  Well, you might think it would work, but then
things randomly start breaking later on.  Try it with the
KOBJECT_DELAYED_DESTROY build option in linux-next, and watch things go
"boom" :)

The rule is, you should never register a kobject/struct device that you
have previously unregistered before, as you really don't know if
unregistering has finished or not.

sorry,

greg k-h

Re: [RFC PATCH] phylib: mdio: handle register/unregister/register sequence

[boris brezillon]

On 22/08/2013 17:27, Greg Kroah-Hartman wrote:
> On Thu, Aug 22, 2013 at 02:33:56PM +0200, Boris BREZILLON wrote:
>> Hello,
>>
>> This patch is a proposal to support the register/unregister/register
>> sequence on a given mdio bus.
>>
>> I use the register/unregister/register sequence to add a fallback when the
>> of_mdiobus_register (this function calls mdiobus_register with phy_mask
>> set to ~0) does not register any phy device (because the device tree does
>> not define any phy).
>> In this case I call mdiobus_unregister and then call mdiobus_register with
>> a phy_mask set to 0 to trigger a full mdio bus scan.
>>
>> I'm not sure this is the right way to do it (this is why I added RFC in the
>> subject).
>>
>> Could someone help me figuring out what I should use to implement my fallback ?
>>
>> 1) use the register/unregister/register sequence
> That will not work.  Well, you might think it would work, but then
> things randomly start breaking later on.  Try it with the
> KOBJECT_DELAYED_DESTROY build option in linux-next, and watch things go
> "boom" :)
>
> The rule is, you should never register a kobject/struct device that you
> have previously unregistered before, as you really don't know if
> unregistering has finished or not.

Thanks for your answer.

>
> sorry,
>
> greg k-h