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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5358C582.8090808@freescale.com>
Date:	Thu, 24 Apr 2014 11:04:18 +0300
From:	Claudiu Manoil <claudiu.manoil@...escale.com>
To:	Ben Hutchings <ben@...adent.org.uk>
CC:	<netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

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@...escale.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.
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html