[PATCH net] gianfar: Check if phydev present on ethtool -A

[PATCH net] gianfar: Check if phydev present on ethtool -A

[Claudiu Manoil]

This fixes a seg fault on 'ethtool -A' entry if the
interface is down.  Obviously we need to have the
phy device initialized / "connected" (see of_phy_connect())
to be able to advertise pause frame capabilities.

Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 891dbee..76d7070 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev,
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
 	u32 oldadv, newadv;
 
+	if (!phydev)
+		return -ENODEV;
+
 	if (!(phydev->supported & SUPPORTED_Pause) ||
 	    (!(phydev->supported & SUPPORTED_Asym_Pause) &&
 	     (epause->rx_pause != epause->tx_pause)))
-- 
1.7.11.7

Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Wed, 2014-04-23 at 16:38 +0300, Claudiu Manoil wrote:
> This fixes a seg fault on 'ethtool -A' entry if the
> interface is down.  Obviously we need to have the
> phy device initialized / "connected" (see of_phy_connect())
> to be able to advertise pause frame capabilities.

Why is the phydev detached while the interface is down?!

> Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
> ---
>  drivers/net/ethernet/freescale/gianfar_ethtool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 891dbee..76d7070 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev,
>  	struct gfar __iomem *regs = priv->gfargrp[0].regs;
>  	u32 oldadv, newadv;
>  
> +	if (!phydev)
> +		return -ENODEV;
> +
>  	if (!(phydev->supported & SUPPORTED_Pause) ||
>  	    (!(phydev->supported & SUPPORTED_Asym_Pause) &&
>  	     (epause->rx_pause != epause->tx_pause)))

I think you can instead remove the following check, as pause support is
a property of the MAC not the PHY.

Ben.

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

[Claudiu Manoil]

On 4/23/2014 7:21 PM, Ben Hutchings wrote:
> On Wed, 2014-04-23 at 16:38 +0300, Claudiu Manoil wrote:
>> This fixes a seg fault on 'ethtool -A' entry if the
>> interface is down.  Obviously we need to have the
>> phy device initialized / "connected" (see of_phy_connect())
>> to be able to advertise pause frame capabilities.
>
> Why is the phydev detached while the interface is down?!

Hi Ben,
Gianfar uses the phylib framework, and it's been always calling
phy_connect() during net_device open and the complementary
phy_disconnect() during net_device close (ndo_stop).
I think this is the general recommendation on how the net_device
driver should integrate with the phylib: i.e. "phy_disconnect - disable
interrupts, stop state machine, and detach a PHY device" while the
interface is down.  Many other net_device drivers call phy_disconnect()
on device close.

>
>> Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58
>> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
>> ---
>>   drivers/net/ethernet/freescale/gianfar_ethtool.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> index 891dbee..76d7070 100644
>> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev,
>>   	struct gfar __iomem *regs = priv->gfargrp[0].regs;
>>   	u32 oldadv, newadv;
>>
>> +	if (!phydev)
>> +		return -ENODEV;
>> +
>>   	if (!(phydev->supported & SUPPORTED_Pause) ||
>>   	    (!(phydev->supported & SUPPORTED_Asym_Pause) &&
>>   	     (epause->rx_pause != epause->tx_pause)))
>
> I think you can instead remove the following check, as pause support is
> a property of the MAC not the PHY.
>

I wish it was as easy as that.  But my understanding is that link
partners need to negotiate their pause frame capabilities at PHY
level.  If a phy is not able to signal (advertise) to the link
partner that the MAC supports pause frame handling then the flow
control btw link partners won't work properly (at least this is my
understanding from looking at other implementations, like tg3).
If it weren't so then the phydev's pause support and advertising
flags and mii_advertise_flowctrl()/ mii_resolve_flowctrl_fdx()
would be useless, right?

Thanks,
Claudiu

> Ben.
>

Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

[David Miller]

From: Claudiu Manoil <claudiu.manoil@freescale.com>
Date: Wed, 23 Apr 2014 16:38:47 +0300

> This fixes a seg fault on 'ethtool -A' entry if the
> interface is down.  Obviously we need to have the
> phy device initialized / "connected" (see of_phy_connect())
> to be able to advertise pause frame capabilities.
> 
> Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>

Applied, thanks Claudiu.

Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

On Thu, 2014-04-24 at 11:04 +0300, Claudiu Manoil wrote:
> On 4/23/2014 7:21 PM, Ben Hutchings wrote:
> > On Wed, 2014-04-23 at 16:38 +0300, Claudiu Manoil wrote:
> >> This fixes a seg fault on 'ethtool -A' entry if the
> >> interface is down.  Obviously we need to have the
> >> phy device initialized / "connected" (see of_phy_connect())
> >> to be able to advertise pause frame capabilities.
> >
> > Why is the phydev detached while the interface is down?!
> 
> Hi Ben,
> Gianfar uses the phylib framework, and it's been always calling
> phy_connect() during net_device open and the complementary
> phy_disconnect() during net_device close (ndo_stop).
> I think this is the general recommendation on how the net_device
> driver should integrate with the phylib: i.e. "phy_disconnect - disable
> interrupts, stop state machine, and detach a PHY device" while the
> interface is down.  Many other net_device drivers call phy_disconnect()
> on device close.

OK, I never actually used phylib so this is just surprising to me.

[...]
> >> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> >> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> >> @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev,
> >>   	struct gfar __iomem *regs = priv->gfargrp[0].regs;
> >>   	u32 oldadv, newadv;
> >>
> >> +	if (!phydev)
> >> +		return -ENODEV;
> >> +
> >>   	if (!(phydev->supported & SUPPORTED_Pause) ||
> >>   	    (!(phydev->supported & SUPPORTED_Asym_Pause) &&
> >>   	     (epause->rx_pause != epause->tx_pause)))
> >
> > I think you can instead remove the following check, as pause support is
> > a property of the MAC not the PHY.
> >
> 
> I wish it was as easy as that.  But my understanding is that link
> partners need to negotiate their pause frame capabilities at PHY
> level.  If a phy is not able to signal (advertise) to the link
> partner that the MAC supports pause frame handling then the flow
> control btw link partners won't work properly (at least this is my
> understanding from looking at other implementations, like tg3).

This is true but I don't see any reason why an MDIO-manageable
autonegotiating PHY would limit which pause flags you can set.

> If it weren't so then the phydev's pause support and advertising
> flags and mii_advertise_flowctrl()/ mii_resolve_flowctrl_fdx()
> would be useless, right?

The pause flags in phy_device::supported do seem to be useless.  The
advertising flags are not as they should be changeable through ethtool.

Ben.

-- 
Ben Hutchings
Power corrupts.  Absolute power is kind of neat.
                           - John Lehman, Secretary of the US Navy 1981-1987

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]