Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Egil Hjelmeland]

On 15. sep. 2017 07:51, Jiri Pirko wrote:
> Thu, Sep 14, 2017 at 11:01:32PM CEST, and...@lunn.ch wrote:
>>> Can you clarify what type of registers it is you are wanting to read?
>>> We already have ethtool which is meant to allow reading the device
>>> registers for a given netdev. As long as the port has a netdev
>>> associated it then there is no need to be getting into debugfs since
>>> we should probably just be using ethtool.
>>
>> Not all ports of a DSA switch have a netdev. This is by design. The
>> presentation we gave to Netdev 2.1 gives some of the background.
>>
>> Plus a switch has a lot of registers not associated to port. Often a
>> switch has more global registers than port registers.
>>
>>> Also as Jiri pointed out there is already devlink which would probably
>>> be a better way to get the associated information for those pieces
>>> that don't have a netdev associated with them.
>>
>> We have looked at the devlink a few times. The current dpipe code is
>> not generic enough. It makes assumptions about the architecture of the
>> switch, that it is all match/action based. The niche of top of rack
>> switches might be like that, but average switches are not.
>>
>> If dpipe was to support simple generic two dimensional tables, we
>> probably would use it.
>>
>> David suggested making a class device for DSA. It is not ideal, but we
>> are probably going to go that way.
> 
> I believe that is also big mistake.
> 
> Could you put together your requirements so we can work it out to extend
> devlink to support them?
> 
> Thanks.
> 

$ ack -i devlink Documentation/
$ ack -i dpipe Documentation/
$

How you expect new mechanisms to be taken into use with zero documentation?

To all: Why does reviewers nitpick about undocumented formatting rules, 
but not ask about documentation?

Egil

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Egil Hjelmeland]

On 15. sep. 2017 07:51, Jiri Pirko wrote:
> Thu, Sep 14, 2017 at 11:01:32PM CEST, and...@lunn.ch wrote:
>>> Can you clarify what type of registers it is you are wanting to read?
>>> We already have ethtool which is meant to allow reading the device
>>> registers for a given netdev. As long as the port has a netdev
>>> associated it then there is no need to be getting into debugfs since
>>> we should probably just be using ethtool.
>>
>> Not all ports of a DSA switch have a netdev. This is by design. The
>> presentation we gave to Netdev 2.1 gives some of the background.
>>
>> Plus a switch has a lot of registers not associated to port. Often a
>> switch has more global registers than port registers.
>>
>>> Also as Jiri pointed out there is already devlink which would probably
>>> be a better way to get the associated information for those pieces
>>> that don't have a netdev associated with them.
>>
>> We have looked at the devlink a few times. The current dpipe code is
>> not generic enough. It makes assumptions about the architecture of the
>> switch, that it is all match/action based. The niche of top of rack
>> switches might be like that, but average switches are not.
>>
>> If dpipe was to support simple generic two dimensional tables, we
>> probably would use it.
>>
>> David suggested making a class device for DSA. It is not ideal, but we
>> are probably going to go that way.
> 
> I believe that is also big mistake.
> 
> Could you put together your requirements so we can work it out to extend
> devlink to support them?
> 
> Thanks.
> 

$ ack -i devlink Documentation/
$ ack -i dpipe Documentation/
$

How you expect new mechanisms to be taken into use with zero documentation?

To all: Why does reviewers nitpick about undocumented formatting rules, 
but not ask about documentation?

Egil

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Jiri Pirko]

Thu, Sep 14, 2017 at 11:01:32PM CEST, and...@lunn.ch wrote:
>> Can you clarify what type of registers it is you are wanting to read?
>> We already have ethtool which is meant to allow reading the device
>> registers for a given netdev. As long as the port has a netdev
>> associated it then there is no need to be getting into debugfs since
>> we should probably just be using ethtool.
>
>Not all ports of a DSA switch have a netdev. This is by design. The
>presentation we gave to Netdev 2.1 gives some of the background.
>
>Plus a switch has a lot of registers not associated to port. Often a
>switch has more global registers than port registers.
> 
>> Also as Jiri pointed out there is already devlink which would probably
>> be a better way to get the associated information for those pieces
>> that don't have a netdev associated with them.
>
>We have looked at the devlink a few times. The current dpipe code is
>not generic enough. It makes assumptions about the architecture of the
>switch, that it is all match/action based. The niche of top of rack
>switches might be like that, but average switches are not.
>
>If dpipe was to support simple generic two dimensional tables, we
>probably would use it.
>
>David suggested making a class device for DSA. It is not ideal, but we
>are probably going to go that way.

I believe that is also big mistake.

Could you put together your requirements so we can work it out to extend
devlink to support them?

Thanks.

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Jiri Pirko]

Thu, Sep 14, 2017 at 11:01:32PM CEST, and...@lunn.ch wrote:
>> Can you clarify what type of registers it is you are wanting to read?
>> We already have ethtool which is meant to allow reading the device
>> registers for a given netdev. As long as the port has a netdev
>> associated it then there is no need to be getting into debugfs since
>> we should probably just be using ethtool.
>
>Not all ports of a DSA switch have a netdev. This is by design. The
>presentation we gave to Netdev 2.1 gives some of the background.
>
>Plus a switch has a lot of registers not associated to port. Often a
>switch has more global registers than port registers.
> 
>> Also as Jiri pointed out there is already devlink which would probably
>> be a better way to get the associated information for those pieces
>> that don't have a netdev associated with them.
>
>We have looked at the devlink a few times. The current dpipe code is
>not generic enough. It makes assumptions about the architecture of the
>switch, that it is all match/action based. The niche of top of rack
>switches might be like that, but average switches are not.
>
>If dpipe was to support simple generic two dimensional tables, we
>probably would use it.
>
>David suggested making a class device for DSA. It is not ideal, but we
>are probably going to go that way.

I believe that is also big mistake.

Could you put together your requirements so we can work it out to extend
devlink to support them?

Thanks.

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Andrew Lunn]

> Can you clarify what type of registers it is you are wanting to read?
> We already have ethtool which is meant to allow reading the device
> registers for a given netdev. As long as the port has a netdev
> associated it then there is no need to be getting into debugfs since
> we should probably just be using ethtool.

Not all ports of a DSA switch have a netdev. This is by design. The
presentation we gave to Netdev 2.1 gives some of the background.

Plus a switch has a lot of registers not associated to port. Often a
switch has more global registers than port registers.
 
> Also as Jiri pointed out there is already devlink which would probably
> be a better way to get the associated information for those pieces
> that don't have a netdev associated with them.

We have looked at the devlink a few times. The current dpipe code is
not generic enough. It makes assumptions about the architecture of the
switch, that it is all match/action based. The niche of top of rack
switches might be like that, but average switches are not.

If dpipe was to support simple generic two dimensional tables, we
probably would use it.

David suggested making a class device for DSA. It is not ideal, but we
are probably going to go that way.

Andrew

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Andrew Lunn]

> Can you clarify what type of registers it is you are wanting to read?
> We already have ethtool which is meant to allow reading the device
> registers for a given netdev. As long as the port has a netdev
> associated it then there is no need to be getting into debugfs since
> we should probably just be using ethtool.

Not all ports of a DSA switch have a netdev. This is by design. The
presentation we gave to Netdev 2.1 gives some of the background.

Plus a switch has a lot of registers not associated to port. Often a
switch has more global registers than port registers.
 
> Also as Jiri pointed out there is already devlink which would probably
> be a better way to get the associated information for those pieces
> that don't have a netdev associated with them.

We have looked at the devlink a few times. The current dpipe code is
not generic enough. It makes assumptions about the architecture of the
switch, that it is all match/action based. The niche of top of rack
switches might be like that, but average switches are not.

If dpipe was to support simple generic two dimensional tables, we
probably would use it.

David suggested making a class device for DSA. It is not ideal, but we
are probably going to go that way.

Andrew

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Alexander Duyck]

On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov  wrote:
> debugfs here is very very useful to read registers directly and
> compare what use space tools see. Cool feature to get regs by port and
> use standard tools to diff and print them. Even might be better to
> allow drivers to decode register names and bits values. Once that is
> done driver mainaince will be much easy. I.e. you need only match regs
> with spec from one side and regs with user space tools from other
> side. Of course it's needed only for debuging, not for production. But
> even for production regs dump on something wrong might tell a lot.
>
> Maxim.

Can you clarify what type of registers it is you are wanting to read?
We already have ethtool which is meant to allow reading the device
registers for a given netdev. As long as the port has a netdev
associated it then there is no need to be getting into debugfs since
we should probably just be using ethtool.

Also as Jiri pointed out there is already devlink which would probably
be a better way to get the associated information for those pieces
that don't have a netdev associated with them.

- Alex

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Alexander Duyck]

On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov  wrote:
> debugfs here is very very useful to read registers directly and
> compare what use space tools see. Cool feature to get regs by port and
> use standard tools to diff and print them. Even might be better to
> allow drivers to decode register names and bits values. Once that is
> done driver mainaince will be much easy. I.e. you need only match regs
> with spec from one side and regs with user space tools from other
> side. Of course it's needed only for debuging, not for production. But
> even for production regs dump on something wrong might tell a lot.
>
> Maxim.

Can you clarify what type of registers it is you are wanting to read?
We already have ethtool which is meant to allow reading the device
registers for a given netdev. As long as the port has a netdev
associated it then there is no need to be getting into debugfs since
we should probably just be using ethtool.

Also as Jiri pointed out there is already devlink which would probably
be a better way to get the associated information for those pieces
that don't have a netdev associated with them.

- Alex

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Maxim Uvarov]

debugfs here is very very useful to read registers directly and
compare what use space tools see. Cool feature to get regs by port and
use standard tools to diff and print them. Even might be better to
allow drivers to decode register names and bits values. Once that is
done driver mainaince will be much easy. I.e. you need only match regs
with spec from one side and regs with user space tools from other
side. Of course it's needed only for debuging, not for production. But
even for production regs dump on something wrong might tell a lot.

Maxim.

2017-09-08 16:58 GMT+03:00 Vivien Didelot :
> Hi Greg,
>
> Greg KH  writes:
>
>> I agree you shouldn't be using debugfs for this, but in the future, if
>> you do write debugfs code, please take the following review into
>> account:
>
> Humm sorry I may not have given enough details. This was really meant
> for debug and dev only, because DSA makes it hard to query directly the
> hardware (some switch ports are not exposed to userspace as well.)
>
> This is not meant to be used for anything real at all, or even be
> compiled-in in a production kernel. That's why I found it appropriate.
>
> So I am still wondering why it doesn't fit here, can you tell me why?
>
>> You should _never_ care about the return value of a debugfs call, and
>> you should not need to ever propagate the error upward.  The api was
>> written to not need this.
>>
>> Just call the function, and return, that's it.  If you need to save the
>> return value (i.e. it's a dentry), you also don't care, just save it and
>> pass it to some other debugfs call, and all will still be fine.  Your
>> code should never do anything different if a debugfs call succeeds or
>> fails.
>
> Thank for your interesting review! I'll cleanup my out-of-tree patches.
>
>
>   Vivien



-- 
Best regards,
Maxim Uvarov

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Maxim Uvarov]

debugfs here is very very useful to read registers directly and
compare what use space tools see. Cool feature to get regs by port and
use standard tools to diff and print them. Even might be better to
allow drivers to decode register names and bits values. Once that is
done driver mainaince will be much easy. I.e. you need only match regs
with spec from one side and regs with user space tools from other
side. Of course it's needed only for debuging, not for production. But
even for production regs dump on something wrong might tell a lot.

Maxim.

2017-09-08 16:58 GMT+03:00 Vivien Didelot :
> Hi Greg,
>
> Greg KH  writes:
>
>> I agree you shouldn't be using debugfs for this, but in the future, if
>> you do write debugfs code, please take the following review into
>> account:
>
> Humm sorry I may not have given enough details. This was really meant
> for debug and dev only, because DSA makes it hard to query directly the
> hardware (some switch ports are not exposed to userspace as well.)
>
> This is not meant to be used for anything real at all, or even be
> compiled-in in a production kernel. That's why I found it appropriate.
>
> So I am still wondering why it doesn't fit here, can you tell me why?
>
>> You should _never_ care about the return value of a debugfs call, and
>> you should not need to ever propagate the error upward.  The api was
>> written to not need this.
>>
>> Just call the function, and return, that's it.  If you need to save the
>> return value (i.e. it's a dentry), you also don't care, just save it and
>> pass it to some other debugfs call, and all will still be fine.  Your
>> code should never do anything different if a debugfs call succeeds or
>> fails.
>
> Thank for your interesting review! I'll cleanup my out-of-tree patches.
>
>
>   Vivien



-- 
Best regards,
Maxim Uvarov

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Vivien Didelot]

Hi Greg,

Greg KH  writes:

> I agree you shouldn't be using debugfs for this, but in the future, if
> you do write debugfs code, please take the following review into
> account:

Humm sorry I may not have given enough details. This was really meant
for debug and dev only, because DSA makes it hard to query directly the
hardware (some switch ports are not exposed to userspace as well.)

This is not meant to be used for anything real at all, or even be
compiled-in in a production kernel. That's why I found it appropriate.

So I am still wondering why it doesn't fit here, can you tell me why?

> You should _never_ care about the return value of a debugfs call, and
> you should not need to ever propagate the error upward.  The api was
> written to not need this.
>
> Just call the function, and return, that's it.  If you need to save the
> return value (i.e. it's a dentry), you also don't care, just save it and
> pass it to some other debugfs call, and all will still be fine.  Your
> code should never do anything different if a debugfs call succeeds or
> fails.

Thank for your interesting review! I'll cleanup my out-of-tree patches.


  Vivien

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Vivien Didelot]

Hi Greg,

Greg KH  writes:

> I agree you shouldn't be using debugfs for this, but in the future, if
> you do write debugfs code, please take the following review into
> account:

Humm sorry I may not have given enough details. This was really meant
for debug and dev only, because DSA makes it hard to query directly the
hardware (some switch ports are not exposed to userspace as well.)

This is not meant to be used for anything real at all, or even be
compiled-in in a production kernel. That's why I found it appropriate.

So I am still wondering why it doesn't fit here, can you tell me why?

> You should _never_ care about the return value of a debugfs call, and
> you should not need to ever propagate the error upward.  The api was
> written to not need this.
>
> Just call the function, and return, that's it.  If you need to save the
> return value (i.e. it's a dentry), you also don't care, just save it and
> pass it to some other debugfs call, and all will still be fine.  Your
> code should never do anything different if a debugfs call succeeds or
> fails.

Thank for your interesting review! I'll cleanup my out-of-tree patches.


  Vivien

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Greg KH]

I agree you shouldn't be using debugfs for this, but in the future, if
you do write debugfs code, please take the following review into
account:

On Mon, Aug 28, 2017 at 03:17:39PM -0400, Vivien Didelot wrote:
> +static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
> +{
> + struct dentry *dir;
> + char name[32];
> +
> + snprintf(name, sizeof(name), DSA_PORT_FMT, port);
> +
> + dir = debugfs_create_dir(name, ds->debugfs_dir);
> + if (IS_ERR_OR_NULL(dir))
> + return -EFAULT;

You should _never_ care about the return value of a debugfs call, and
you should not need to ever propagate the error upward.  The api was
written to not need this.

Just call the function, and return, that's it.  If you need to save the
return value (i.e. it's a dentry), you also don't care, just save it and
pass it to some other debugfs call, and all will still be fine.  Your
code should never do anything different if a debugfs call succeeds or
fails.

> +static int dsa_debugfs_create_switch(struct dsa_switch *ds)
> +{
> + char name[32];
> + int i, err;
> +
> + /* skip if there is no debugfs support */
> + if (!dsa_debugfs_dir)
> + return 0;

Again, you don't care, all of these functions should return void.

> + snprintf(name, sizeof(name), DSA_SWITCH_FMT, ds->index);
> +
> + ds->debugfs_dir = debugfs_create_dir(name, dsa_debugfs_dir);
> + if (IS_ERR_OR_NULL(ds->debugfs_dir))
> + return -EFAULT;

See, that's horrid, you should never need to make such a bad check.

Also, even if it were the correct way to do this you never return EFAULT
unless there is a memory copy error to/from userspace.  That is not the
case here, or in any of this code, right?

> +static void dsa_debugfs_destroy_switch(struct dsa_switch *ds)
> +{
> + /* handles NULL */
> + debugfs_remove_recursive(ds->debugfs_dir);

Of course it handles NULL, why comment that?  That's the whole goal of
debugfs, to be dirt simple, allow you to do anything you want, in almost
no lines of code.

Also, it will never be mounted on a "real" system, so you better not
rely on it for anything "real".

> + err = dsa_debugfs_create_switch(ds);
> + if (err) {
> + pr_warn("DSA: failed to create debugfs interface for 
> switch %d (%d)\n",
> + ds->index, err);

Never complain to the syslog about a debugfs issue.

> +void dsa_debugfs_destroy_module(void)
> +{
> + /* handles NULL */
> + debugfs_remove_recursive(dsa_debugfs_dir);

again, of course it does, do you think we don't know how to write an
api?  :)

thanks,

greg k-h

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Greg KH]

I agree you shouldn't be using debugfs for this, but in the future, if
you do write debugfs code, please take the following review into
account:

On Mon, Aug 28, 2017 at 03:17:39PM -0400, Vivien Didelot wrote:
> +static int dsa_debugfs_create_port(struct dsa_switch *ds, int port)
> +{
> + struct dentry *dir;
> + char name[32];
> +
> + snprintf(name, sizeof(name), DSA_PORT_FMT, port);
> +
> + dir = debugfs_create_dir(name, ds->debugfs_dir);
> + if (IS_ERR_OR_NULL(dir))
> + return -EFAULT;

You should _never_ care about the return value of a debugfs call, and
you should not need to ever propagate the error upward.  The api was
written to not need this.

Just call the function, and return, that's it.  If you need to save the
return value (i.e. it's a dentry), you also don't care, just save it and
pass it to some other debugfs call, and all will still be fine.  Your
code should never do anything different if a debugfs call succeeds or
fails.

> +static int dsa_debugfs_create_switch(struct dsa_switch *ds)
> +{
> + char name[32];
> + int i, err;
> +
> + /* skip if there is no debugfs support */
> + if (!dsa_debugfs_dir)
> + return 0;

Again, you don't care, all of these functions should return void.

> + snprintf(name, sizeof(name), DSA_SWITCH_FMT, ds->index);
> +
> + ds->debugfs_dir = debugfs_create_dir(name, dsa_debugfs_dir);
> + if (IS_ERR_OR_NULL(ds->debugfs_dir))
> + return -EFAULT;

See, that's horrid, you should never need to make such a bad check.

Also, even if it were the correct way to do this you never return EFAULT
unless there is a memory copy error to/from userspace.  That is not the
case here, or in any of this code, right?

> +static void dsa_debugfs_destroy_switch(struct dsa_switch *ds)
> +{
> + /* handles NULL */
> + debugfs_remove_recursive(ds->debugfs_dir);

Of course it handles NULL, why comment that?  That's the whole goal of
debugfs, to be dirt simple, allow you to do anything you want, in almost
no lines of code.

Also, it will never be mounted on a "real" system, so you better not
rely on it for anything "real".

> + err = dsa_debugfs_create_switch(ds);
> + if (err) {
> + pr_warn("DSA: failed to create debugfs interface for 
> switch %d (%d)\n",
> + ds->index, err);

Never complain to the syslog about a debugfs issue.

> +void dsa_debugfs_destroy_module(void)
> +{
> + /* handles NULL */
> + debugfs_remove_recursive(dsa_debugfs_dir);

again, of course it does, do you think we don't know how to write an
api?  :)

thanks,

greg k-h

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Andrew Lunn]

On Mon, Aug 28, 2017 at 03:17:39PM -0400, Vivien Didelot wrote:
> This commit adds a DEBUG_FS dependent DSA core file creating a generic
> debug filesystem interface for the DSA switch devices.
> 
> The interface can be mounted with:
> 
> # mount -t debugfs none /sys/kernel/debug
> 
> The dsa directory contains one directory per switch chip:
> 
> # cd /sys/kernel/debug/dsa/
> # ls
> switch0  switch1 switch2
> 
> Each chip directory contains one directory per port:
> 
> # ls -l switch0/
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
> 
> Future patches will add entry files to these directories.
> 
> Signed-off-by: Vivien Didelot 

Reviewed-by: Andrew Lunn 

Andrew

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Andrew Lunn]

On Mon, Aug 28, 2017 at 03:17:39PM -0400, Vivien Didelot wrote:
> This commit adds a DEBUG_FS dependent DSA core file creating a generic
> debug filesystem interface for the DSA switch devices.
> 
> The interface can be mounted with:
> 
> # mount -t debugfs none /sys/kernel/debug
> 
> The dsa directory contains one directory per switch chip:
> 
> # cd /sys/kernel/debug/dsa/
> # ls
> switch0  switch1 switch2
> 
> Each chip directory contains one directory per port:
> 
> # ls -l switch0/
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
> drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
> 
> Future patches will add entry files to these directories.
> 
> Signed-off-by: Vivien Didelot 

Reviewed-by: Andrew Lunn 

Andrew

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:58:12PM CEST, f.faine...@gmail.com wrote:
>On 08/28/2017 12:50 PM, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.dide...@savoirfairelinux.com 
>> wrote:
>>> This commit adds a DEBUG_FS dependent DSA core file creating a generic
>>> debug filesystem interface for the DSA switch devices.
>>>
>>> The interface can be mounted with:
>>>
>>># mount -t debugfs none /sys/kernel/debug
>>>
>>> The dsa directory contains one directory per switch chip:
>>>
>>># cd /sys/kernel/debug/dsa/
>>># ls
>>>switch0  switch1 switch2
>>>
>>> Each chip directory contains one directory per port:
>>>
>>># ls -l switch0/
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
>>>
>>> Future patches will add entry files to these directories.
>>>
>>> Signed-off-by: Vivien Didelot 
>> 
>> Oh no, no debugfs please!
>> 
>> What do you need to expose? I'm sure we can find out some generic, well
>> defined and reusable way.
>
>We have no CPU or DSA (cross switches) net_device reprensentors because
>those would be two ends of the same pipe so it would be both confusing

So? That is certainly not an argument for debugfs. Just have all ports
as devlink port, and you can introduce special new kind of port for cpu
port. Note that devlink port does not have to have netdev association.


>and a duplication. For a CPU interface, one side goes to the switch, the
>other one is the master net_device (normal Ethernet MAC). For a DSA
>interface, one interface is on one switch, and the other is on the other
>switch.
>
>If you look at the patch series it's pretty obvious what is being exposed :)

Sure. But lets use existing interfaces and extend them if needed. Please
don't use some made-up debugfs mess. That is never the correct answer :/

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:58:12PM CEST, f.faine...@gmail.com wrote:
>On 08/28/2017 12:50 PM, Jiri Pirko wrote:
>> Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.dide...@savoirfairelinux.com 
>> wrote:
>>> This commit adds a DEBUG_FS dependent DSA core file creating a generic
>>> debug filesystem interface for the DSA switch devices.
>>>
>>> The interface can be mounted with:
>>>
>>># mount -t debugfs none /sys/kernel/debug
>>>
>>> The dsa directory contains one directory per switch chip:
>>>
>>># cd /sys/kernel/debug/dsa/
>>># ls
>>>switch0  switch1 switch2
>>>
>>> Each chip directory contains one directory per port:
>>>
>>># ls -l switch0/
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
>>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
>>>
>>> Future patches will add entry files to these directories.
>>>
>>> Signed-off-by: Vivien Didelot 
>> 
>> Oh no, no debugfs please!
>> 
>> What do you need to expose? I'm sure we can find out some generic, well
>> defined and reusable way.
>
>We have no CPU or DSA (cross switches) net_device reprensentors because
>those would be two ends of the same pipe so it would be both confusing

So? That is certainly not an argument for debugfs. Just have all ports
as devlink port, and you can introduce special new kind of port for cpu
port. Note that devlink port does not have to have netdev association.


>and a duplication. For a CPU interface, one side goes to the switch, the
>other one is the master net_device (normal Ethernet MAC). For a DSA
>interface, one interface is on one switch, and the other is on the other
>switch.
>
>If you look at the patch series it's pretty obvious what is being exposed :)

Sure. But lets use existing interfaces and extend them if needed. Please
don't use some made-up debugfs mess. That is never the correct answer :/

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Florian Fainelli]

On 08/28/2017 12:50 PM, Jiri Pirko wrote:
> Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.dide...@savoirfairelinux.com 
> wrote:
>> This commit adds a DEBUG_FS dependent DSA core file creating a generic
>> debug filesystem interface for the DSA switch devices.
>>
>> The interface can be mounted with:
>>
>># mount -t debugfs none /sys/kernel/debug
>>
>> The dsa directory contains one directory per switch chip:
>>
>># cd /sys/kernel/debug/dsa/
>># ls
>>switch0  switch1 switch2
>>
>> Each chip directory contains one directory per port:
>>
>># ls -l switch0/
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
>>
>> Future patches will add entry files to these directories.
>>
>> Signed-off-by: Vivien Didelot 
> 
> Oh no, no debugfs please!
> 
> What do you need to expose? I'm sure we can find out some generic, well
> defined and reusable way.

We have no CPU or DSA (cross switches) net_device reprensentors because
those would be two ends of the same pipe so it would be both confusing
and a duplication. For a CPU interface, one side goes to the switch, the
other one is the master net_device (normal Ethernet MAC). For a DSA
interface, one interface is on one switch, and the other is on the other
switch.

If you look at the patch series it's pretty obvious what is being exposed :)
-- 
Florian

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Florian Fainelli]

On 08/28/2017 12:50 PM, Jiri Pirko wrote:
> Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.dide...@savoirfairelinux.com 
> wrote:
>> This commit adds a DEBUG_FS dependent DSA core file creating a generic
>> debug filesystem interface for the DSA switch devices.
>>
>> The interface can be mounted with:
>>
>># mount -t debugfs none /sys/kernel/debug
>>
>> The dsa directory contains one directory per switch chip:
>>
>># cd /sys/kernel/debug/dsa/
>># ls
>>switch0  switch1 switch2
>>
>> Each chip directory contains one directory per port:
>>
>># ls -l switch0/
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
>>drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
>>
>> Future patches will add entry files to these directories.
>>
>> Signed-off-by: Vivien Didelot 
> 
> Oh no, no debugfs please!
> 
> What do you need to expose? I'm sure we can find out some generic, well
> defined and reusable way.

We have no CPU or DSA (cross switches) net_device reprensentors because
those would be two ends of the same pipe so it would be both confusing
and a duplication. For a CPU interface, one side goes to the switch, the
other one is the master net_device (normal Ethernet MAC). For a DSA
interface, one interface is on one switch, and the other is on the other
switch.

If you look at the patch series it's pretty obvious what is being exposed :)
-- 
Florian

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.dide...@savoirfairelinux.com wrote:
>This commit adds a DEBUG_FS dependent DSA core file creating a generic
>debug filesystem interface for the DSA switch devices.
>
>The interface can be mounted with:
>
># mount -t debugfs none /sys/kernel/debug
>
>The dsa directory contains one directory per switch chip:
>
># cd /sys/kernel/debug/dsa/
># ls
>switch0  switch1 switch2
>
>Each chip directory contains one directory per port:
>
># ls -l switch0/
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
>
>Future patches will add entry files to these directories.
>
>Signed-off-by: Vivien Didelot 

Oh no, no debugfs please!

What do you need to expose? I'm sure we can find out some generic, well
defined and reusable way.

Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface

[Jiri Pirko]

Mon, Aug 28, 2017 at 09:17:39PM CEST, vivien.dide...@savoirfairelinux.com wrote:
>This commit adds a DEBUG_FS dependent DSA core file creating a generic
>debug filesystem interface for the DSA switch devices.
>
>The interface can be mounted with:
>
># mount -t debugfs none /sys/kernel/debug
>
>The dsa directory contains one directory per switch chip:
>
># cd /sys/kernel/debug/dsa/
># ls
>switch0  switch1 switch2
>
>Each chip directory contains one directory per port:
>
># ls -l switch0/
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port0
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port1
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port2
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port5
>drwxr-xr-x 2 root root 0 Jan  1 00:00 port6
>
>Future patches will add entry files to these directories.
>
>Signed-off-by: Vivien Didelot 

Oh no, no debugfs please!

What do you need to expose? I'm sure we can find out some generic, well
defined and reusable way.