netdev - Re: i.mx6ul with DSA in multi chip addressing mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190705143647.GC4428@lunn.ch>
Date:   Fri, 5 Jul 2019 16:36:47 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Benjamin Beckmeyer <beb@...-engel.de>
Cc:     netdev@...r.kernel.org
Subject: Re: i.mx6ul with DSA in multi chip addressing mode - no MDIO access

On Fri, Jul 05, 2019 at 02:41:43PM +0200, Benjamin Beckmeyer wrote:
> >> &mdio0 {
> >>         interrupt-parent = <&gpio1>;
> >>         interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >>         switch0: switch0@2 {
> >>                 compatible = "marvell,mv88e6190";
> >>                 reg = <2>;
> >>                 pinctrl-0 = <&pinctrl_gpios>;
> >>                 reset-gpios = <&gpio4 16 GPIO_ACTIVE_LOW>;
> >>                 dsa,member = <0 0>;
> > This is wrong. The interrupt is a switch property, not an MDIO bus
> > property. So it belongs inside the switch node.
> >
> > 	  Andrew
> 
> Hi Andrew,
> 
> in the documentation for Marvell DSA the interrupt properties are in 
> the MDIO part. Maybe the documentation for device tree is wrong or 
> unclear?

Ah. Yes. The documentation is wrong. I will fix that.

> 
> I switched to the kernel 5.1.16 to take advantage of your new code.
> At the moment I deleted all interrupt properties from my device tree 
> and if I get you right now the access should be trigger all 100ms but 
> I have accesses within the tracing about 175 times a second.
> 
> Here is a snip from my trace without IRQ
> 2188000.etherne-223   [000] ....   109.932406: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x01 val:0x40a8
>  2188000.etherne-223   [000] ....   109.932501: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x00 val:0x1b64
>  2188000.etherne-223   [000] ....   109.933113: mdio_access: 2188000.ethernet-1 write phy:0x02 reg:0x00 val:0x9b60
>  2188000.etherne-223   [000] ....   109.933261: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x00 val:0x1b60
>  2188000.etherne-223   [000] ....   109.933359: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x01 val:0xc801

>  2188000.etherne-223   [000] ....   110.041683: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x00 val:0x1b60
>  2188000.etherne-223   [000] ....   110.041817: mdio_access: 2188000.ethernet-1 write phy:0x02 reg:0x00 val:0x9b60
>  2188000.etherne-223   [000] ....   110.041919: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x00 val:0x1b60
>  2188000.etherne-223   [000] ....   110.042025: mdio_access: 2188000.ethernet-1 read  phy:0x02 reg:0x01 val:0xc801

These four access are one switch register access. The first read will
be checking that the busy bit is not set. The second sets up a read to
switch register 0x00 device address 1b, i.e. global 1. So this is the
interrupt status register. The third read is checking that the busy
bit is cleared. And the last is the actual value of the register.

> 
> Am I doing it right with the tracing points? I run just
> 
> echo 1 > /sys/kernel/debug/tracing/events/mdio/mdio_access/enable
> cat /sys/kernel/debug/tracing/trace

That looks correct.

I think you are going to have to parse the register writes/reads to
figure out what switch registers it is accessing. That should
hopefully make it clearer why it is making so many accesses.

> Here is the another device tree I tried, but with this I get accesses 
> on the bus in about every 50 microseconds!
> 
> --snip
> &mdio0 {
>         switch0: switch0@2 {
>                 compatible = "marvell,mv88e6190";
>                 reg = <2>;
>                 pinctrl-0 = <&pinctrl_switch_irq>;
>                 interrupt-parent = <&gpio1>;
>                 interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>                 dsa,member = <0 0>;
> 
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> --snip

That looks sensible.

     Andrew