Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1559075 > unrolled thread

[PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class() Florian Fainelli <f.fainelli@gmail.com> - 2017-01-14 22:50 +0100
    Re: [PATCH net-next v3 06/10] net: dsa: Migrate to  device_find_class() Greg KH <gregkh@linuxfoundation.org> - 2017-01-15 12:10 +0100
      Re: [PATCH net-next v3 06/10] net: dsa: Migrate to  device_find_class() Florian Fainelli <f.fainelli@gmail.com> - 2017-01-15 18:30 +0100
        Re: [PATCH net-next v3 06/10] net: dsa: Migrate to  device_find_class() Greg KH <gregkh@linuxfoundation.org> - 2017-01-15 18:40 +0100
          Re: [PATCH net-next v3 06/10] net: dsa: Migrate to  device_find_class() Florian Fainelli <f.fainelli@gmail.com> - 2017-01-15 19:00 +0100
            Re: [PATCH net-next v3 06/10] net: dsa: Migrate to  device_find_class() Andrew Lunn <andrew@lunn.ch> - 2017-01-15 20:20 +0100

#1559075 — [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

Now that the base device driver code provides an identical
implementation of dev_find_class() utilize device_find_class() instead
of our own version of it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 2306d1b87c83..77fa4c4f5828 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -455,29 +455,11 @@ EXPORT_SYMBOL_GPL(dsa_switch_resume);
 #endif
 
 /* platform driver init and cleanup *****************************************/
-static int dev_is_class(struct device *dev, void *class)
-{
-	if (dev->class != NULL && !strcmp(dev->class->name, class))
-		return 1;
-
-	return 0;
-}
-
-static struct device *dev_find_class(struct device *parent, char *class)
-{
-	if (dev_is_class(parent, class)) {
-		get_device(parent);
-		return parent;
-	}
-
-	return device_find_child(parent, class, dev_is_class);
-}
-
 struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev)
 {
 	struct device *d;
 
-	d = dev_find_class(dev, "mdio_bus");
+	d = device_find_class(dev, "mdio_bus");
 	if (d != NULL) {
 		struct mii_bus *bus;
 
@@ -495,7 +477,7 @@ static struct net_device *dev_to_net_device(struct device *dev)
 {
 	struct device *d;
 
-	d = dev_find_class(dev, "net");
+	d = device_find_class(dev, "net");
 	if (d != NULL) {
 		struct net_device *nd;
 
-- 
2.9.3

[toc] | [next] | [standalone]

#1559207 — Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

On Sat, Jan 14, 2017 at 01:47:09PM -0800, Florian Fainelli wrote:
> Now that the base device driver code provides an identical
> implementation of dev_find_class() utilize device_find_class() instead
> of our own version of it.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/dsa/dsa.c | 22 ++--------------------
>  1 file changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 2306d1b87c83..77fa4c4f5828 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -455,29 +455,11 @@ EXPORT_SYMBOL_GPL(dsa_switch_resume);
>  #endif
>  
>  /* platform driver init and cleanup *****************************************/
> -static int dev_is_class(struct device *dev, void *class)
> -{
> -	if (dev->class != NULL && !strcmp(dev->class->name, class))
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static struct device *dev_find_class(struct device *parent, char *class)
> -{
> -	if (dev_is_class(parent, class)) {
> -		get_device(parent);
> -		return parent;
> -	}
> -
> -	return device_find_child(parent, class, dev_is_class);
> -}
> -
>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev)
>  {
>  	struct device *d;
>  
> -	d = dev_find_class(dev, "mdio_bus");
> +	d = device_find_class(dev, "mdio_bus");
>  	if (d != NULL) {
>  		struct mii_bus *bus;

You want a peer of your device on a specific class?  What is this for?

> @@ -495,7 +477,7 @@ static struct net_device *dev_to_net_device(struct device *dev)
>  {
>  	struct device *d;
>  
> -	d = dev_find_class(dev, "net");
> +	d = device_find_class(dev, "net");
>  	if (d != NULL) {
>  		struct net_device *nd;

Again, huh?  What is the device heirachy here that is so odd that this
type of function is needed?

totally confused,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1559307 — Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

On 01/15/2017 03:06 AM, Greg KH wrote:
> On Sat, Jan 14, 2017 at 01:47:09PM -0800, Florian Fainelli wrote:
>> Now that the base device driver code provides an identical
>> implementation of dev_find_class() utilize device_find_class() instead
>> of our own version of it.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  net/dsa/dsa.c | 22 ++--------------------
>>  1 file changed, 2 insertions(+), 20 deletions(-)
>>
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 2306d1b87c83..77fa4c4f5828 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -455,29 +455,11 @@ EXPORT_SYMBOL_GPL(dsa_switch_resume);
>>  #endif
>>  
>>  /* platform driver init and cleanup *****************************************/
>> -static int dev_is_class(struct device *dev, void *class)
>> -{
>> -	if (dev->class != NULL && !strcmp(dev->class->name, class))
>> -		return 1;
>> -
>> -	return 0;
>> -}
>> -
>> -static struct device *dev_find_class(struct device *parent, char *class)
>> -{
>> -	if (dev_is_class(parent, class)) {
>> -		get_device(parent);
>> -		return parent;
>> -	}
>> -
>> -	return device_find_child(parent, class, dev_is_class);
>> -}
>> -
>>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev)
>>  {
>>  	struct device *d;
>>  
>> -	d = dev_find_class(dev, "mdio_bus");
>> +	d = device_find_class(dev, "mdio_bus");
>>  	if (d != NULL) {
>>  		struct mii_bus *bus;
> 
> You want a peer of your device on a specific class?  What is this for?

It's not a peer of our device, it's a separate device reference from the
one looked up in the "net" class. In the classic, and now deprecated DSA
device driver model, a "dsa" platform device would represent one or more
Ethernet switches, connected via a MDIO bus (this reference above), and
one Ethernet device (the CPU/host/management interface). This was
completely violating the Linux device driver model and imposed
limitations on what bus would be used, and we did not have proper struct
device references (therefore no adequate hierarchy either).

Thanks to the work of Andrew, we now have proper MDIO, SPI, GPIO, I2C,
PCI, platform and drivers that allow us to register with DSA as a
specialized kind of device (so we are now finally using the right Linux
Device Driver model). What we still need though, in order to our switch
to the networking stack is a reference to the master/host network device
since we mangle packets in and out of it.

> 
>> @@ -495,7 +477,7 @@ static struct net_device *dev_to_net_device(struct device *dev)
>>  {
>>  	struct device *d;
>>  
>> -	d = dev_find_class(dev, "net");
>> +	d = device_find_class(dev, "net");
>>  	if (d != NULL) {
>>  		struct net_device *nd;
> 
> Again, huh?  What is the device heirachy here that is so odd that this
> type of function is needed?

An Ethernet switch managed by DSA needs to have one ore more references
to a host/CPU/management network interface, this is what this struct
device reference is here for.
--
Florian

[toc] | [prev] | [next] | [standalone]

#1559308 — Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

On Sun, Jan 15, 2017 at 09:27:22AM -0800, Florian Fainelli wrote:
> 
> 
> On 01/15/2017 03:06 AM, Greg KH wrote:
> > On Sat, Jan 14, 2017 at 01:47:09PM -0800, Florian Fainelli wrote:
> >> Now that the base device driver code provides an identical
> >> implementation of dev_find_class() utilize device_find_class() instead
> >> of our own version of it.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  net/dsa/dsa.c | 22 ++--------------------
> >>  1 file changed, 2 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> >> index 2306d1b87c83..77fa4c4f5828 100644
> >> --- a/net/dsa/dsa.c
> >> +++ b/net/dsa/dsa.c
> >> @@ -455,29 +455,11 @@ EXPORT_SYMBOL_GPL(dsa_switch_resume);
> >>  #endif
> >>  
> >>  /* platform driver init and cleanup *****************************************/
> >> -static int dev_is_class(struct device *dev, void *class)
> >> -{
> >> -	if (dev->class != NULL && !strcmp(dev->class->name, class))
> >> -		return 1;
> >> -
> >> -	return 0;
> >> -}
> >> -
> >> -static struct device *dev_find_class(struct device *parent, char *class)
> >> -{
> >> -	if (dev_is_class(parent, class)) {
> >> -		get_device(parent);
> >> -		return parent;
> >> -	}
> >> -
> >> -	return device_find_child(parent, class, dev_is_class);
> >> -}
> >> -
> >>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev)
> >>  {
> >>  	struct device *d;
> >>  
> >> -	d = dev_find_class(dev, "mdio_bus");
> >> +	d = device_find_class(dev, "mdio_bus");
> >>  	if (d != NULL) {
> >>  		struct mii_bus *bus;
> > 
> > You want a peer of your device on a specific class?  What is this for?
> 
> It's not a peer of our device, it's a separate device reference from the
> one looked up in the "net" class. In the classic, and now deprecated DSA
> device driver model, a "dsa" platform device would represent one or more
> Ethernet switches, connected via a MDIO bus (this reference above), and
> one Ethernet device (the CPU/host/management interface). This was
> completely violating the Linux device driver model and imposed
> limitations on what bus would be used, and we did not have proper struct
> device references (therefore no adequate hierarchy either).
> 
> Thanks to the work of Andrew, we now have proper MDIO, SPI, GPIO, I2C,
> PCI, platform and drivers that allow us to register with DSA as a
> specialized kind of device (so we are now finally using the right Linux
> Device Driver model). What we still need though, in order to our switch
> to the networking stack is a reference to the master/host network device
> since we mangle packets in and out of it.

Ok, but where in the tree are you trying to find this other device?  Are
you just going to randomly find any device that happens to be of the
specific class type?  That seems really fragile and broken :(

You should NEVER have to walk the device tree to find stuff.  Why can't
you have a pointer to the device you need to talk to some other way?

What exactly is the relationship between these devices (a ascii-art tree
or sysfs tree output might be nice) so I can try to understand what is
going on here.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1559316 — Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

On 01/15/2017 09:39 AM, Greg KH wrote:
> On Sun, Jan 15, 2017 at 09:27:22AM -0800, Florian Fainelli wrote:
>>
>>
>> On 01/15/2017 03:06 AM, Greg KH wrote:
>>> On Sat, Jan 14, 2017 at 01:47:09PM -0800, Florian Fainelli wrote:
>>>> Now that the base device driver code provides an identical
>>>> implementation of dev_find_class() utilize device_find_class() instead
>>>> of our own version of it.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>  net/dsa/dsa.c | 22 ++--------------------
>>>>  1 file changed, 2 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>>>> index 2306d1b87c83..77fa4c4f5828 100644
>>>> --- a/net/dsa/dsa.c
>>>> +++ b/net/dsa/dsa.c
>>>> @@ -455,29 +455,11 @@ EXPORT_SYMBOL_GPL(dsa_switch_resume);
>>>>  #endif
>>>>  
>>>>  /* platform driver init and cleanup *****************************************/
>>>> -static int dev_is_class(struct device *dev, void *class)
>>>> -{
>>>> -	if (dev->class != NULL && !strcmp(dev->class->name, class))
>>>> -		return 1;
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static struct device *dev_find_class(struct device *parent, char *class)
>>>> -{
>>>> -	if (dev_is_class(parent, class)) {
>>>> -		get_device(parent);
>>>> -		return parent;
>>>> -	}
>>>> -
>>>> -	return device_find_child(parent, class, dev_is_class);
>>>> -}
>>>> -
>>>>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev)
>>>>  {
>>>>  	struct device *d;
>>>>  
>>>> -	d = dev_find_class(dev, "mdio_bus");
>>>> +	d = device_find_class(dev, "mdio_bus");
>>>>  	if (d != NULL) {
>>>>  		struct mii_bus *bus;
>>>
>>> You want a peer of your device on a specific class?  What is this for?
>>
>> It's not a peer of our device, it's a separate device reference from the
>> one looked up in the "net" class. In the classic, and now deprecated DSA
>> device driver model, a "dsa" platform device would represent one or more
>> Ethernet switches, connected via a MDIO bus (this reference above), and
>> one Ethernet device (the CPU/host/management interface). This was
>> completely violating the Linux device driver model and imposed
>> limitations on what bus would be used, and we did not have proper struct
>> device references (therefore no adequate hierarchy either).
>>
>> Thanks to the work of Andrew, we now have proper MDIO, SPI, GPIO, I2C,
>> PCI, platform and drivers that allow us to register with DSA as a
>> specialized kind of device (so we are now finally using the right Linux
>> Device Driver model). What we still need though, in order to our switch
>> to the networking stack is a reference to the master/host network device
>> since we mangle packets in and out of it.
> 
> Ok, but where in the tree are you trying to find this other device?  Are
> you just going to randomly find any device that happens to be of the
> specific class type?  That seems really fragile and broken :(

The search is not really random, since the platform data for the DSA
switch we want to register gives us a reference to a device registered
via any bus type, and we know (by contract it has to) that it has to
provide a network device of some kind, we do a specific search in the
"net" class for that purpose. I agree this is not great.

> 
> You should NEVER have to walk the device tree to find stuff.  Why can't
> you have a pointer to the device you need to talk to some other way?

We do, I think the existing code just tries to be extra careful here and
make sure that the device reference we got is actually part of the "net"
class, indicating that the reference passed is indeed pointing to a
network device, and not a random device.

> 
> What exactly is the relationship between these devices (a ascii-art tree
> or sysfs tree output might be nice) so I can try to understand what is
> going on here.

OK, let me try and let's take the case of only one Ethernet switch in
the system for all simplicity:

- switch device drivers get probed by any kind of bus allocate and
register a dsa_switch with dsa_register_switch()

- this switch device has platform data (struct dsa_chip_data) that
describes its port layout (number of ports mostly) and has one struct
device references to which Ethernet network interface these ports connect to

- a dsa_switch_tree is created, this struct dsa_switch is attached to
the dsa_switch_tree at position 0 in the tree, we invoke a bunch of
switch driver operations

- we conclude with attaching this dsa_switch_tree to the network device
we looked up and assigning the tree to the dsa_ptr member

Documentation/networking/dsa/dsa.txt has some ascii art drawing that
sort of capture what is going on.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1559325 — Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

> > What exactly is the relationship between these devices (a ascii-art tree
> > or sysfs tree output might be nice) so I can try to understand what is
> > going on here.

Hi Greg, Florian

A few diagrams and trees which might help understand what is going on.

The first diagram comes from the 2008 patch which added all this code:

            +-----------+       +-----------+
            |           | RGMII |           |
            |           +-------+           +------ 1000baseT MDI ("WAN")
            |           |       |  6-port   +------ 1000baseT MDI ("LAN1")
            |    CPU    |       |  ethernet +------ 1000baseT MDI ("LAN2")
            |           |MIImgmt|  switch   +------ 1000baseT MDI ("LAN3")
            |           +-------+  w/5 PHYs +------ 1000baseT MDI ("LAN4")
            |           |       |           |
            +-----------+       +-----------+

We have an ethernet switch and a host CPU. The switch is connected to
the CPU in two different ways. RGMII allows us to get Ethernet frames
from the CPU into the switch. MIImgmt, is the management bus normally
used for Ethernet PHYs, but Marvell switches also use it for Managing
switches.

The diagram above is the simplest setup. You can have multiple
Ethernet switches, connected together via switch ports. Each switch
has its own MIImgmt connect to the CPU, but there is only one RGMII
link.

When this code was designed back in 2008, it was decided to represent
this is a platform device, and it has a platform_data, which i have
slightly edited to keep it simple:

struct dsa_platform_data {
        /*
         * Reference to a Linux network interface that connects
         * to the root switch chip of the tree.
         */
        struct device   *netdev;

        /*
         * Info structs describing each of the switch chips
         * connected via this network interface.
         */
        int             nr_chips;
        struct dsa_chip_data    *chip;
};

This netdev is the CPU side of the RGMII interface.

Each switch has a dsa_chip_data, again edited:

struct dsa_chip_data {
        /*
         * How to access the switch configuration registers.
         */
        struct device   *host_dev;
        int             sw_addr;
...
}

The host_dev is the CPU side of the MIImgmt, and we have the address
the switch is using on the bus.

During probe of this platform device, we need to get from the
struct device *netdev to a struct net_device *dev.

So the code looks in the device net class to find the device

|   |   |   |-- f1074000.ethernet
|   |   |   |   |-- deferred_probe
|   |   |   |   |-- driver -> ../../../../../bus/platform/drivers/mvneta
|   |   |   |   |-- driver_override
|   |   |   |   |-- modalias
|   |   |   |   |-- net
|   |   |   |   |   `-- eth1
|   |   |   |   |       |-- addr_assign_type
|   |   |   |   |       |-- address
|   |   |   |   |       |-- addr_len
|   |   |   |   |       |-- broadcast
|   |   |   |   |       |-- carrier
|   |   |   |   |       |-- carrier_changes
|   |   |   |   |       |-- deferred_probe
|   |   |   |   |       |-- device -> ../../../f1074000.ethernet

and then use container_of() to get the net_device.

Similarly, the code needs to get from struct device *host_dev to a struct mii_bus *.

|   |   |   |-- f1072004.mdio
|   |   |   |   |-- deferred_probe
|   |   |   |   |-- driver -> ../../../../../bus/platform/drivers/orion-mdio
|   |   |   |   |-- driver_override
|   |   |   |   |-- mdio_bus
|   |   |   |   |   `-- f1072004.mdio-mi
|   |   |   |   |       |-- deferred_probe
|   |   |   |   |       |-- device -> ../../../f1072004.mdio

    Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web