Message-ID: <fb4f6f99-4b87-aba3-7ca3-2f1a291f7569@arinc9.com>
Date:   Mon, 17 Apr 2023 15:39:02 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Daniel Golle <daniel@...rotopia.org>
Cc:     netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH net-next v2] net: dsa: mt7530: fix support for MT7531BE

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@...nc9.com>

> 
> 
>>
>>>
>>> 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ç

Powered by blists - more mailing lists