Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690391 > unrolled thread

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops Florian Fainelli <f.fainelli@gmail.com> - 2017-07-18 18:10 +0200
    Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect  ops David Miller <davem@davemloft.net> - 2017-07-18 19:50 +0200
      Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-18 21:10 +0200
        Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops Andrew Lunn <andrew@lunn.ch> - 2017-07-18 22:00 +0200

#1690391 — Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

On 07/17/2017 02:10 PM, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Mon, 17 Jul 2017 23:04:05 +0200
> 
>> On Mon, Jul 17, 2017 at 01:45:49PM -0700, David Miller wrote:
>>> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>> Date: Mon, 17 Jul 2017 15:32:52 -0400
>>>
>>>> Hi Andrew,
>>>>
>>>> Andrew Lunn <andrew@lunn.ch> writes:
>>>>
>>>>> I never liked this. I think it is architecturally wrong for the switch
>>>>> to be poking around in the PHY. It should ask the PHY driver. This is
>>>>> especially true for external PHYs which might not be a Marvell PHY.
>>>>
>>>> I share the same concern. However this patch is just isolating the
>>>> existing code so that we get rid of the last caps and flags and stop
>>>> writing (without reading them first) arbitrary registers.
>>>>
>>>> Once this portion is moved to the PHY driver, one can remove it from
>>>> mv88e6xxx.
>>>
>>> Seems a reasonable plan of action.
>>>
>>> Andrew, do you agree?
>>
>> Hi David
>>
>> I just fear it will not get fixed, just put into a corner to
>> fester. Having to fix it properly before these patches are merged
>> provides some incentive.
> 
> If Vivien doesn't make good on his promises to do so, tell me and
> I will revert all of these changes.
> 
> Ok?

This seems to be completely unfair to Vivien, there is nothing wrong
with his patch series per-se other than he was unfortunate enough he
highlighted something that needs fixing. This was not a serious enough
problem before and it cannot possibly be one now either with just a code
move.

On a general note, we cannot have whoever was the last one to touch a
piece of code that makes us see that this or that said piece of code is
less than ideal be selected as the random victim for doing that cleanup,
this just does not work. I know this is standard practice in Linux and
other open source software (been there before with the USB maintainers),
but this creates only one thing: making you want to runaway and scream
lalalalala.

So let's be pragmatic and maintain a public TODO list for this driver
that people can pick items to fix/cleanup/change that have been
identified as candidates for patches.
-- 
Florian

[toc] | [next] | [standalone]

#1690522 — Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 18 Jul 2017 09:01:01 -0700

> On 07/17/2017 02:10 PM, David Miller wrote:
>> From: Andrew Lunn <andrew@lunn.ch>
>> Date: Mon, 17 Jul 2017 23:04:05 +0200
>> 
>>> On Mon, Jul 17, 2017 at 01:45:49PM -0700, David Miller wrote:
>>>> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>>> Date: Mon, 17 Jul 2017 15:32:52 -0400
>>>>
>>>>> Hi Andrew,
>>>>>
>>>>> Andrew Lunn <andrew@lunn.ch> writes:
>>>>>
>>>>>> I never liked this. I think it is architecturally wrong for the switch
>>>>>> to be poking around in the PHY. It should ask the PHY driver. This is
>>>>>> especially true for external PHYs which might not be a Marvell PHY.
>>>>>
>>>>> I share the same concern. However this patch is just isolating the
>>>>> existing code so that we get rid of the last caps and flags and stop
>>>>> writing (without reading them first) arbitrary registers.
>>>>>
>>>>> Once this portion is moved to the PHY driver, one can remove it from
>>>>> mv88e6xxx.
>>>>
>>>> Seems a reasonable plan of action.
>>>>
>>>> Andrew, do you agree?
>>>
>>> Hi David
>>>
>>> I just fear it will not get fixed, just put into a corner to
>>> fester. Having to fix it properly before these patches are merged
>>> provides some incentive.
>> 
>> If Vivien doesn't make good on his promises to do so, tell me and
>> I will revert all of these changes.
>> 
>> Ok?
> 
> This seems to be completely unfair to Vivien, there is nothing wrong
> with his patch series per-se other than he was unfortunate enough he
> highlighted something that needs fixing. This was not a serious enough
> problem before and it cannot possibly be one now either with just a code
> move.
> 
> On a general note, we cannot have whoever was the last one to touch a
> piece of code that makes us see that this or that said piece of code is
> less than ideal be selected as the random victim for doing that cleanup,
> this just does not work. I know this is standard practice in Linux and
> other open source software (been there before with the USB maintainers),
> but this creates only one thing: making you want to runaway and scream
> lalalalala.
> 
> So let's be pragmatic and maintain a public TODO list for this driver
> that people can pick items to fix/cleanup/change that have been
> identified as candidates for patches.

However, in this particular case, this issue was brought to Vivien's
attention multiple times in the past.

And I think the direct PHY poking issue is much more important than
these seemingly endless reorganizations of the driver that Vivien is
doing.

So I personally share Andrew's serious frustration that we are doing
constant reorgs but not addressing directly the specific issues that
one has been made clearly aware of.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1690574

Hi David,

David Miller <davem@davemloft.net> writes:

> However, in this particular case, this issue was brought to Vivien's
> attention multiple times in the past.
>
> And I think the direct PHY poking issue is much more important than
> these seemingly endless reorganizations of the driver that Vivien is
> doing.
>
> So I personally share Andrew's serious frustration that we are doing
> constant reorgs but not addressing directly the specific issues that
> one has been made clearly aware of.

We support 26 Marvell Ethernet switch chips, so I am often comparing the
documentation of many of them to make sure the driver stops writing
arbitrary registers, ending up with many inconsistencies (like Remote
Management being currently enabled on all chips with an RMU, fix coming)
mainly due to poor documentation and device setup.

I know this looks boring, I do not particularly enjoy it myself, but I
think this is also important. I don't mind fixing the poking function as
well in the near future.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1690612

> I know this looks boring, I do not particularly enjoy it myself, but I
> think this is also important. I don't mind fixing the poking function as
> well in the near future.

It would be great if you do. It could be as simple as using
phy_ethtool_get_eee() and phy_ethtool_set_eee().

      Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web