Message-ID: <0740c897-4db9-47f7-3ac9-e9428e634589@gmail.com>
Date:   Fri, 4 Dec 2020 19:06:45 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Grant Edwards <grant.b.edwards@...il.com>, netdev@...r.kernel.org
Subject: Re: net: macb: fail when there's no PHY



On 12/4/2020 6:52 PM, Grant Edwards wrote:
> On 2020-12-04, Florian Fainelli <f.fainelli@...il.com> wrote:
>> On 12/2/2020 7:54 PM, Grant Edwards wrote:
>>> On 2020-12-03, Florian Fainelli <f.fainelli@...il.com> wrote:
>>>
>>>> You would have to have a local hack that intercepts the macb_ioctl()
>>>> and instead of calling phylink_mii_ioctl() it would have to
>>>> implement a custom ioctl() that does what
>>>> drivers/net/phy/phy.c::phy_mii_ioctl does except the mdiobus should
>>>> be pointed to the MACB MDIO bus instance and not be derived from the
>>>> phy_device instance (because that one points to the fixed PHY).
>>>
>>> So I can avoid my local hack to macb_main.c by doing a doing a local
>>> hack to macb_main.c?
>>
>> There is value in having the macb driver support the scheme that was
>> just described which is to support a fixed PHY as far as the MAC link
>> parameters go, and also support registering the MACB internal MDIO bus
>> to interface with other devices. People using DSA would typically fall
>> under that category.
> 
> My hack does support both a fixed PHY as far as the MAC link
> parameters go and allows interfacing with other devices via the mdio
> bus, so I assume you're saying that there's value in doing that in the
> "standard" way instead of the way I invented 10 years ago.

All I was doing was explaining how this can be done today, in a way that
is useful to you and other people. You want to keep doing things your
own way, go ahead.

> 
> That would certainly be true if we were talking about something to be
> incorporated "upstream", but like you said: it would be a local
> hack. I see no intrinsic value in changing to the "standard" DSA
> scheme. Switching to a different API would actually create additional
> cost above and beyond the cost of writing and testing the new local
> hack, since all of the applications which need to access the mdio bus
> would also have to change.

I was not trying to convince you to switch to DSA, and if this is an
area of technical debt that has a low cost for your platform compared to
others, so be it. What could have been useful was to support the
standard fixed PHY plus the internal MDIO bus.

> 
> If I could avoid the local hack completely by using a standard,
> existing feature and API, then I could make an arguement for modifying
> the applications. But proposing the writing of a new, more comlex
> local hack and modifying the applications just so that we can feel
> good about using a standard API would be laughed at.

Only you can be the judge of that, I have no visibility into what
constitutes an acceptable change and what does not.

> 
>> [...]
>>
>> I don't have a dog in the fight, but dealing myself with cute little
>> hacks in our downstream Linux kernel, the better it fits with useful
>> functionality to other people, the better.
> 
> I don't see why it makes any difference how well suited a local hack
> is to people who will never see it or use it. It would seem to me that
> the the most important attribute of a local hack would be simplicity
> and ease of understanding.  My hack is 7 lines of code and one line of
> a static structure declaration and initializer, all
> enabled/disabled/controlled by a single preprocessor symbol.

A change can be 7 lines of code and be broken any time you update to a
newer version of the kernel, and that is probably even truer in your
case since you have such a big difference between 2.6.x and 5.4 that
anything in between could have been rewritten a dozen time.

Over the course of the past 6 years, we have managed to get our
downstream kernel from ~1300 patches downstream to only about 65 as of
5.10, but it took 6 years and counting and we only targeted LTS kernels.
Maybe I am overly sensitive to how maintainable a change is, who knows.
-- 
Florian

Powered by blists - more mailing lists