Date:   Wed, 6 Jun 2018 23:34:14 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool
 ops

On 06/04/2018 02:07 PM, Vladimir Zapolskiy wrote:

>>>>> The change replaces a custom implementation of .set_link_ksettings
>>>>> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
>>>>> sleep in atomic context bug, which is encountered every time when link
>>>>> settings are changed by ethtool.
>>>>
>>>>    Seeing it now...
>>
>>    And to say that this is *fixed* by removing the custom method is err...
>> simply misleading. The sleep in atomic context is fixed solely by the removal
>> of the spinlock grabbing before the phylib call.

> As I've already said, "the removal of the spinlock grabbing before the phylib
> call" is not a valid fix, but it will introduce another regression.

   It's the necessary part of the fix, unlike using the generic phylib method.

>>>>> Now duplex mode setting is enforced in ravb_adjust_link() only, also
>>>>> now TX/RX is disabled when link is put down or modifications to E-MAC
>>>>> registers ECMR and GECMR are expected for both cases of checked and
>>>>> ignored link status pin state from E-MAC interrupt handler.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
>>>>> ---
>>>>>  drivers/net/ethernet/renesas/ravb_main.c | 58 +++++++++-----------------------
>>>>>  1 file changed, 15 insertions(+), 43 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index 3d91caa44176..0d811c02ff34 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>>>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>>>  	struct phy_device *phydev = ndev->phydev;
>>>>>  	bool new_state = false;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	spin_lock_irqsave(&priv->lock, flags);
>>>>> +
>>>>> +	/* Disable TX and RX right over here, if E-MAC change is ignored */
>>>>> +	if (priv->no_avb_link)
>>>>> +		ravb_rcv_snd_disable(ndev);
>>>>>  
>>>>>  	if (phydev->link) {
>>>>>  		if (phydev->duplex != priv->duplex) {
>>>>> @@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
>>>>>  			ravb_modify(ndev, ECMR, ECMR_TXF, 0);
>>>>>  			new_state = true;
>>>>>  			priv->link = phydev->link;
>>>>> -			if (priv->no_avb_link)
>>>>> -				ravb_rcv_snd_enable(ndev);
>>>>>  		}
>>>>>  	} else if (priv->link) {
>>>>>  		new_state = true;
>>>>>  		priv->link = 0;
>>>>>  		priv->speed = 0;
>>>>>  		priv->duplex = -1;
>>>>> -		if (priv->no_avb_link)
>>>>> -			ravb_rcv_snd_disable(ndev);
>>>>>  	}
>>>>>  
>>>>> +	/* Enable TX and RX right over here, if E-MAC change is ignored */
>>>>> +	if (priv->no_avb_link && phydev->link)
>>>>> +		ravb_rcv_snd_enable(ndev);
>>>>> +
>>>>> +	mmiowb();
>>>>> +	spin_unlock_irqrestore(&priv->lock, flags);
>>>>> +
>>>>
>>>>    I like this part. :-)
>>>>
>>>
>>> A weight off my mind :) And I hope that this change will remain the less
>>> questionable one, other ones from the series are trivial.
>>>
>>> Anyway I hope it is understandable that this part of the change can not
>>> be simply extracted from the rest one below, otherwise there'll be bugs of
>>> another type intorduced.
>>
>>    I never said I'd like to apply this part alone, my idea was more like removing
>> the spinlock grabbing and the duplex handling down below.
>>
> 
> As I've already said, "the removal of the spinlock grabbing" is not a valid fix,
> but it will introduce another regression.
> 
> Please tell me, a removal of duplex handling change should be done before
> a removal of the spinlock grabbing? Or after?

   As much as I was able to figure out, at the same time.

>> [...]
>>>>> @@ -1357,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>>>>>  	.set_ringparam		= ravb_set_ringparam,
>>>>>  	.get_ts_info		= ravb_get_ts_info,
>>>>>  	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
>>>>> -	.set_link_ksettings	= ravb_set_link_ksettings,
>>>>> +	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
>>>>
>>>>    Should have been a part of the final patch in the fix/enhancement chain...
>>>
>>> Please elaborate.
>>>
>>> Do you mean that firstly I have to make erroneous ravb_set_link_ksettings()
>>> to look similar to phy_ethtool_set_link_ksettings() and then remove it?
>>
>>    Yes.

> Then this change of "ravb_set_link_ksettings() looks similar to
> phy_ethtool_set_link_ksettings()" will be a single commit, and it will be
> a fix. Does it sound good?

   It does, as I was trying to tell you. :-)

>>> As I see it in the current context (removal of ravb_set_duplex() call and
>>> so on), the problem with this approach is that the actual fix change will
>>> be done on top of a number of enchancement changes, thus it contradicts to
>>
>>    Now I have to ask you to elaborate. I have no idea what you mean. :-(
> 
> My statement is based on the next facts:

  s/next/following/?

> 1. ravb_set_duplex() call in ravb_set_link_ksettings() is unnecessary,
>    however its removal is an enchancement,
> 2. removal of the spinlock grabbing is just a *part* of the proper fix,
>    and the complete proper fix includes ravb_set_duplex() call removal,
>    adding spinlock grabbing to ravb_adjust_link() and other modifications
>    to ravb_adjust_link() from this commit.

   So far, so good. :-)

> 3. commits with fixes must precede commits with enchancements in the
>    series, because enchancements are not backported.

   Enhancements?
   Yes, if at all possible.

> The question remains the same, what do you ask me to do? 

   Mainly, to separate fixes from clean-ups, as much as possible. That'll simplify
the -stable backport handling for DaveM and the people maintaining the earlier kernel
versions.

>>    And of course, sometimes the things are broken in a so subtle way, that
>> only as pile of "cleanups" fixed them, we had that situation in e.g. the
>> R-Car I2C driver -- *none* of AFAIR 9 patches was good as a -stable patch...
>>
>>> the accepted development/maintenace model "fixes first", and most probably
>>> it won't be possible to backport the real fix, however this sole change can
>>> be backported.
>>
>>    My idea was to move the [G]ECMR writes to the adjust_link() callback and
>> to stop grabbing the spinlock where it *was* grabbed in the same fix patch.
>> Then just a single clean up, to start using the new phylib method.

> It will be okay iff ravb_set_duplex() call removal is added to the first
> part ("fixes" part) of two changes. Please confirm that our understanding
> is aligned.

   Yes, and I've tried to communicate that to you but somehow failed. :-)
> --
> With best wishes,
> Vladimir

MBR, Sergei

Powered by blists - more mailing lists