netdev - Re: [Patch net-next v2 0/9] net: dsa: microchip: add support for phylink mac config and link up

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:   Thu, 1 Sep 2022 15:47:37 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Arun.Ramadoss@...rochip.com, andrew@...n.ch,
        linux-kernel@...r.kernel.org, UNGLinuxDriver@...rochip.com,
        vivien.didelot@...il.com, san@...v.dk, linux@...linux.org.uk,
        f.fainelli@...il.com, kuba@...nel.org, edumazet@...gle.com,
        pabeni@...hat.com, netdev@...r.kernel.org,
        Woojung.Huh@...rochip.com, davem@...emloft.net
Subject: Re: [Patch net-next v2 0/9] net: dsa: microchip: add support for
 phylink mac config and link up

On Thu, Sep 01, 2022 at 01:27:21PM +0200, Oleksij Rempel wrote:
> > The global register 0x06 responsibilities are bit 4 for 10/100mbps
> > speed selection, bit 5 for flow control and bit 6  for duplex
> > operation. Since these three are new features added during refactoring
> > I overlooked it. 
> > To fix this, either I need to return from the ksz_set_100_10mbit &
> > ksz_duplex_flowctrl function if the chip_id is ksz87xx or add
> > dev->dev_ops for this alone.  Kindly suggest on how to proceed.
> 
> I would prefer to got ops way, to clean things up.

I can't say that that one approach is better or worse than the other.
Indirect function calls are going to be more expensive than conditionals
on dev->chip_id, but we aren't in a fast path here, so it doesn't matter
too much.

Having indirect function calls will in theory help simplify the logic of
the main function, but will require good forethought for what constitutes
an atom of functionality, in a high enough level such as to abstract
switch differences. Whereas conditionals don't require thinking that far,
you put them where you need them.

Also, indirect function calls will move the bloat somewhere else. I have
seen complaints in the past about the mv88e6xxx driver's layered structure,
making it difficult to see exactly what gets done for a certain chip.

It is probable that we don't want to mix these styles too much within a
single driver, so if work has already started towards dev_ops for
everything, then dev_ops be it, I guess.

Oleksij, are you going to submit patches with your proposal?