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

Groups > linux.kernel > #1732544 > unrolled thread

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

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

  Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Maxim Uvarov <muvarov@gmail.com> - 2017-09-14 22:00 +0200
    Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Alexander Duyck <alexander.duyck@gmail.com> - 2017-09-14 22:20 +0200
      Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Andrew Lunn <andrew@lunn.ch> - 2017-09-14 23:10 +0200
        Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Jiri Pirko <jiri@resnulli.us> - 2017-09-15 08:00 +0200
          Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-09-15 10:10 +0200

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

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 <vivien.didelot@savoirfairelinux.com>:
> Hi Greg,
>
> Greg KH <gregkh@linuxfoundation.org> 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

[toc] | [next] | [standalone]

#1732552

On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov <muvarov@gmail.com> 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

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

#1732575

> 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

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

#1732721

Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@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.

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

#1732770

On 15. sep. 2017 07:51, Jiri Pirko wrote:
> Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web