netdev - Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

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]

Date:   Sat, 17 Aug 2019 20:03:42 +0200
From:   Marek Behun <marek.behun@....cz>
To:     Vivien Didelot <vivien.didelot@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vladimir Oltean <olteanv@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq
 also for CPU/DSA ports

Hi Vivien,

On Fri, 16 Aug 2019 19:05:37 -0400
Vivien Didelot <vivien.didelot@...il.com> wrote:

> I think the DSA switch port_setup/port_teardown operations are fine, but the
> idea would be that the drivers must no longer setup their ports directly
> in their .setup function. So for mv88e6xxx precisely, we should rename
> mv88e6xxx_setup_port to mv88e6xxx_port_setup, and move all the port-related
> code from mv88e6xxx_setup into mv88e6xxx_port_setup.

I looked into the driver, and found out that mv88e6xxx_setup calls many
other setup functions after the calls to mv88e6xxx_setup_port for each
port:
   1. setup errata
   2. cache cmode
   3. for each port setup_port
   4. irl setup
   5. mac setup
   6. phy setup
   7. vtu setup
   8. pvt setup
   9. atu setup
  10. broadcast setuo
  11. pot setup
  12. rmu setup
  13. rsvd2cpu setup
  14. trunk setup
  15. devmap setup
  16. pri setup
  17. ptp setup
  18. hwtstamp setup
  19. stats setup

The problem is that some of these steps (after step 3) may depend on
some of the work done by step 3. Some of these functions iterate again
over the port array (mv88e6xxx_hwtstamp_setup, for example).
We cannot simply move step 3 to be called from DSA after
mv88e6xxx_setup.

I now do not know exactly what to do about the error prone naming of
setup_port vs port_setup.

One way would be to rename the mv88e6xxx_setup_port function to
mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something
like that. Would the names mv88e6xxx_port_setup and
mv88e6xxx_setup_port_regs still be very confusing and error prone?
I think maybe yes...

Other solution would be to, instead of the .port_setup()
and .port_teardown() DSA ops, create the .after_setup()
and .before_teardown() ops I mentioned in the previous mail.

And yet another (in my opinion very improper) solution could be that
the .setup() method could call dsa_port_setup() from within itself, to
ensure that the needed structres exist.

Please let me know what you think about this.

The first solution to me currently seems as the easiest.

Marek