Re: [PATCH net-next v2] net: dsa: mt7530: fix support for MT7531BE

[Arınç ÜNAL]

On 17.04.2023 14:39, Daniel Golle wrote:
> On Mon, Apr 17, 2023 at 12:28:57AM +0300, Arınç ÜNAL wrote:
> > On 16/04/2023 17:18, Daniel Golle wrote:
> > > On Sun, Apr 16, 2023 at 04:48:23PM +0300, Arınç ÜNAL wrote:
> > > > On 16.04.2023 15:08, Daniel Golle wrote:
> > > > > [...]
> > > > >    * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
> > > > >      only required for MDIO-connected switches
> > > > >      (with your patch we would have to move the dependency on PCS_MTK_LYNXI
> > > > >      from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
> > > >
> > > > Maybe this is what should happen. Maybe the PCS creation (and therefore
> > > > mt7530_regmap_bus) should be on the core driver. Both are on the MDIO driver
> > > > for the sole reason of only the devices on the MDIO driver currently using
> > > > it. It's not an MDIO-specific operation as far as I can tell. Having it on
> > > > the core driver would make more sense in the long run.
> > >
> > > Which "long run" are you talking about?
> > > regmap creation is bus-specific, and so is the existence of LynxI PCS.
> > > There simply aren't any MMIO-connected switches which come with that IP.
> > > And I strongly doubt there ever will be. And even if, why should we now
> > > prepare for an entirely speculative future? If it actually happens, ie.
> > > in case there is going to be a new SoC with MMIO-connected switch which
> > > does comes with LynxI PCS (e.g. for port 5 only) we can still move the
> > > code.
> >
> > Makes sense.
>
>
> > > > [...]
> > > >
> > > > A similar logic is already there on the U-Boot MediaTek ethernet driver.
> > > >
> > > > https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> > > >
> > > > So this patch fixes the issue with the only consideration being changing as
> > > > less lines of code as possible.
> > >
> > > You are ignore two more important arguments:
> > >    * It doesn't requrire additional export of mt7530_regmap_bus
> > >      (which would imply significantly more storage overhead compared to
> > >      an additional function pointer in a priv struct)
> > >
> > >    * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
> > >      only required for MDIO-connected switches
> > >      (with your patch we would have to move the dependency on PCS_MTK_LYNXI
> > >      from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
> >
> > Understood.
>
> > > > And that's okay. We can make the least
> > > > amount of changes to fix the issue first, then improve the driver. But
> > > > there's nothing new made on the driver after the commit that caused this
> > > > issue, backportability to the stable trees is a non-issue. So why not do it
> > > > properly the first time?
> > >
> > > Most of all I'd rather have it fixed before net-next is merged to Linus'
> > > tree and also before net-next will close again.
> > >
> > > However, I also simply don't see what would be more "proper" about your
> > > solution.
> >
> > Nothing. Your patch here is perfectly fine after reading your points. One
> > thing I'd like to ask is, if I understand correctly, instead of exporting
> > mt7531_create_sgmii(), defining a pointer that points to it causes less
> > overhead?
>
> Yes. Depending on build configuration and debugging options an exported
> function or constant will require different amounts of storage, ie.
> function name and parameter prototypes need to be stored in the kernel
> symbol table, any module calling the exporting functions and in the
> exporting modules ELF header, the latter being the most significant.
> Even if kernel modules aren't used and it's all built-in the overhead
> is still more than a few bytes for the struct member definition as well
> as the growth of the per-instance allocated struct member itself --
> especially given that I have only heard about one board using two
> MT7531AE, most boards use exactly one of them.
>
> > The current patch looks very similar to exporting a function. Instead of
> > putting EXPORT_SYMBOL_GPL and declaring the function prototype on the header
> > file, you declare a function pointer on the priv structure, then assign it
> > to the function.
>
> The effect is similar, just limited in scope as a caller needs to have
> access to the priv struct (opposed to an EXPORT_SYMBOL* which will
> make the function or const available globally).
>
> Also note that exporting mt7531_create_sgmii() would not work equally
> well as the result would be a hard dependency of NET_DSA_MT7530 on
> NET_DSA_MT7530_MDIO for the exported function being linkable.
> The function pointer has the advantage that it can be set to NULL and
> in that way we can model a weak dependency.

Very nice, thanks for the explanation.

> > > > Whatever the outcome with this patch is, on my upcoming patch series, I
> > > > intend to move mt7531_create_sgmii to mt7530.c. Then introduce
> > > > priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().
> > >
> > > What is the argument for that?
> >
> > Nothing for moving mt7531_create_sgmii() but I think introducing
> > priv->p5_sgmii with later patches is in the clear?
>
> Yes, I agree that introducing priv->p5_sgmii can make sense, given that
> it would prevent having to export mt7531_dual_sgmii_supported() or
> passing its return value as a function parameter, or even just having
> to call it many times.
>
> Regarding this current patch (see subject), do you still agree that we
> should apply it as-is and then either you or me will prepare another
> series further refactoring the driver?

This patch is fine as is and should go in, I will base my upcoming RFC 
series on top of this.

Acked-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>

> > > There is not a single MMIO-connected switch which comes with LynxI PCS.
> > > (see above)
> > >
> > > Imho we should rather try to work into the opposite direction and move
> > > more code only used on either MDIO or MMIO from core to the
> > > bus-specific drivers. If needed we can even split them more, eg. have
> > > different modules for MT7530 and MT7531, so that even the driver for
> > > MDIO-connected MT7530 would not require MTK_PCS_LYNXI.
> >
> > Interesting, I may work on this in the future. This could benefit my folks
> > too.
> >
> > > In that sense I'm a big fan of the structure of the mt76 wireless
> > > driver: Have a core module for shared helper functions and then
> > > device-specific driver modules. Unfortunately many if not most drivers
> > > are doing the exact opposite approach, ie. having some abstration layer
> > > which will always need to be extended and changed with every
> > > unforeseeable new hardware to be supported which just results in lots
> > > of overhead and is a burden to maintain. You can see that in the rt2x00
> > > wireless driver which I also worked on a lot: Most of the abstractions
> > > aren't even useful with any of the latest hardware generations.
> > >
> > > tl;dr: What's wrong with moving functions specific to either variant
> > > (MMIO vs. MDIO) into the corresponding modules and keeping the core
> > > slim and really only cover shared functionality? This is also why I
> > > originally wanted the names of files and Kconfig symbols to reflect the
> > > supported hardware rather than the supported bus-type -- I've changed
> > > that upon your request and now believe I should have argued more
> > > clearly why I made my choice like I did...
> >
> > Ah that makes sense. I'd like to address this. I was already planning to to
> > do some renaming on the driver. Please, allow me to do the work.
>
> Sure, your efforts are appreciated, and I'll happily review and test
> your suggestions.

Sounds good, till then.

Arınç