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

Groups > linux.kernel > #1435973 > unrolled thread

[PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

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

  [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings Philippe Reynes <tremyfr@gmail.com> - 2016-07-03 17:40 +0200
    Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings David Miller <davem@davemloft.net> - 2016-07-05 01:10 +0200
      Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-05 06:40 +0200
        Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings Philippe Reynes <tremyfr@gmail.com> - 2016-07-05 23:10 +0200
          Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-05 23:20 +0200
            Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Ben Hutchings <ben@decadent.org.uk> - 2016-07-05 23:50 +0200
          Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-08 03:30 +0200

#1435973 — [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

There are two generics functions phy_ethtool_{get|set}_link_ksettings,
so we can use them instead of defining the same code in the driver.

Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c |   28 +----------------------
 1 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 8d4f849..76ed6df 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -450,30 +450,6 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
 			genet_dma_ring_regs[r]);
 }
 
-static int bcmgenet_get_settings(struct net_device *dev,
-				 struct ethtool_cmd *cmd)
-{
-	if (!netif_running(dev))
-		return -EINVAL;
-
-	if (!dev->phydev)
-		return -ENODEV;
-
-	return phy_ethtool_gset(dev->phydev, cmd);
-}
-
-static int bcmgenet_set_settings(struct net_device *dev,
-				 struct ethtool_cmd *cmd)
-{
-	if (!netif_running(dev))
-		return -EINVAL;
-
-	if (!dev->phydev)
-		return -ENODEV;
-
-	return phy_ethtool_sset(dev->phydev, cmd);
-}
-
 static int bcmgenet_set_rx_csum(struct net_device *dev,
 				netdev_features_t wanted)
 {
@@ -977,8 +953,6 @@ static struct ethtool_ops bcmgenet_ethtool_ops = {
 	.get_strings		= bcmgenet_get_strings,
 	.get_sset_count		= bcmgenet_get_sset_count,
 	.get_ethtool_stats	= bcmgenet_get_ethtool_stats,
-	.get_settings		= bcmgenet_get_settings,
-	.set_settings		= bcmgenet_set_settings,
 	.get_drvinfo		= bcmgenet_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
 	.get_msglevel		= bcmgenet_get_msglevel,
@@ -990,6 +964,8 @@ static struct ethtool_ops bcmgenet_ethtool_ops = {
 	.nway_reset		= bcmgenet_nway_reset,
 	.get_coalesce		= bcmgenet_get_coalesce,
 	.set_coalesce		= bcmgenet_set_coalesce,
+	.get_link_ksettings = phy_ethtool_get_link_ksettings,
+	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 };
 
 /* Power down the unimac, based on mode. */
-- 
1.7.4.4

[toc] | [next] | [standalone]

#1436642 — Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

From: Philippe Reynes <tremyfr@gmail.com>
Date: Sun,  3 Jul 2016 17:33:57 +0200

> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
> so we can use them instead of defining the same code in the driver.
> 
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>

Applied.

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

#1436750 — Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

Le 04/07/2016 16:03, David Miller a écrit :
> From: Philippe Reynes <tremyfr@gmail.com>
> Date: Sun,  3 Jul 2016 17:33:57 +0200
> 
>> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
>> so we can use them instead of defining the same code in the driver.
>>
>> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
> 
> Applied.
> 

The transformation is not equivalent, we lost the checks on
netif_running() in the process, and those are here for a reason, if the
interface is down and therefore clock gated, MDIO accesses to the PHY
will simply fail outright and cause bus errors.

Philippe, have you tested this?
-- 
Florian

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

#1437288

Hi Florian,

On 05/07/16 06:30, Florian Fainelli wrote:
> Le 04/07/2016 16:03, David Miller a écrit :
>> From: Philippe Reynes<tremyfr@gmail.com>
>> Date: Sun,  3 Jul 2016 17:33:57 +0200
>>
>>> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
>>> so we can use them instead of defining the same code in the driver.
>>>
>>> Signed-off-by: Philippe Reynes<tremyfr@gmail.com>
>>
>> Applied.
>>
>
> The transformation is not equivalent, we lost the checks on
> netif_running() in the process, and those are here for a reason, if the
> interface is down and therefore clock gated, MDIO accesses to the PHY
> will simply fail outright and cause bus errors.

Oh, I see, I've missed this. Sorry for this mistake.
We should revert this path.

I think that a lot of hardware had the same behaviour.
I'm going to look for a generic solution for this behaviour.
If someone has an idea ...
  
> Philippe, have you tested this?

I haven't tested, I don't have the hardware.


Philippe

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

#1437292 — Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

On 07/05/2016 02:07 PM, Philippe Reynes wrote:
> Hi Florian,
> 
> On 05/07/16 06:30, Florian Fainelli wrote:
>> Le 04/07/2016 16:03, David Miller a écrit :
>>> From: Philippe Reynes<tremyfr@gmail.com>
>>> Date: Sun,  3 Jul 2016 17:33:57 +0200
>>>
>>>> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
>>>> so we can use them instead of defining the same code in the driver.
>>>>
>>>> Signed-off-by: Philippe Reynes<tremyfr@gmail.com>
>>>
>>> Applied.
>>>
>>
>> The transformation is not equivalent, we lost the checks on
>> netif_running() in the process, and those are here for a reason, if the
>> interface is down and therefore clock gated, MDIO accesses to the PHY
>> will simply fail outright and cause bus errors.
> 
> Oh, I see, I've missed this. Sorry for this mistake.
> We should revert this path.

Well, maybe better than that, actually put the check in the generic
functions, because if the link is down, aka netif_running() returns
false, link parameters cannot be reliably queried and they are invalid.
-- 
Florian

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

#1437308 — Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-07-05 at 14:15 -0700, Florian Fainelli wrote:
> On 07/05/2016 02:07 PM, Philippe Reynes wrote:
> > Hi Florian,
> > 
> > On 05/07/16 06:30, Florian Fainelli wrote:
> > > Le 04/07/2016 16:03, David Miller a écrit :
> > > > From: Philippe Reynes<tremyfr@gmail.com>
> > > > Date: Sun,  3 Jul 2016 17:33:57 +0200
> > > > 
> > > > > There are two generics functions phy_ethtool_{get|set}_link_ksettings,
> > > > > so we can use them instead of defining the same code in the driver.
> > > > > 
> > > > > Signed-off-by: Philippe Reynes<tremyfr@gmail.com>
> > > > 
> > > > Applied.
> > > > 
> > > 
> > > The transformation is not equivalent, we lost the checks on
> > > netif_running() in the process, and those are here for a reason, if the
> > > interface is down and therefore clock gated, MDIO accesses to the PHY
> > > will simply fail outright and cause bus errors.
> > 
> > Oh, I see, I've missed this. Sorry for this mistake.
> > We should revert this path.
> 
> Well, maybe better than that, actually put the check in the generic
> functions, because if the link is down, aka netif_running() returns
> false, link parameters cannot be reliably queried and they are invalid.

Either the hardware or the driver needs to remember:

- Is auto-negotiation enabled
- If so, which modes are advertised
- If not, which mode is forced

And it should still be possible to get or set that information when the
interface is down.

Ben.

-- 

Ben Hutchings
Life is what happens to you while you're busy making other plans.
                                                               - John
Lennon

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

#1439051 — Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

On 07/05/2016 02:07 PM, Philippe Reynes wrote:
> Hi Florian,
> 
> On 05/07/16 06:30, Florian Fainelli wrote:
>> Le 04/07/2016 16:03, David Miller a écrit :
>>> From: Philippe Reynes<tremyfr@gmail.com>
>>> Date: Sun,  3 Jul 2016 17:33:57 +0200
>>>
>>>> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
>>>> so we can use them instead of defining the same code in the driver.
>>>>
>>>> Signed-off-by: Philippe Reynes<tremyfr@gmail.com>
>>>
>>> Applied.
>>>
>>
>> The transformation is not equivalent, we lost the checks on
>> netif_running() in the process, and those are here for a reason, if the
>> interface is down and therefore clock gated, MDIO accesses to the PHY
>> will simply fail outright and cause bus errors.
> 
> Oh, I see, I've missed this. Sorry for this mistake.
> We should revert this path.

Can you send such a revert please? Thanks!
-- 
Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web