netdev - Re: [PATCH] phylib: add driver for aquantia phy

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]

Date:	Mon, 27 Jul 2015 14:49:38 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Shaohui Xie <Shaohui.Xie@...escale.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH] phylib: add driver for aquantia phy

On 27/07/15 01:30, Shaohui Xie wrote:
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@...il.com]
>> Sent: Friday, July 24, 2015 12:39 PM
>> To: shh.xie@...il.com; netdev@...r.kernel.org; davem@...emloft.net
>> Cc: Xie Shaohui-B21989
>> Subject: Re: [PATCH] phylib: add driver for aquantia phy
>>
>> Le 07/23/15 20:46, shh.xie@...il.com a écrit :
>>> From: Shaohui Xie <Shaohui.Xie@...escale.com>
>>>
>>> This patch added driver to support Aquantia PHYs AQ1202, AQ2104,
>>> AQR105, AQR405, which accessed through clause 45.
>>
>> Could you prefix your patches with "net: phy: " in the future to be
>> consistent with what is typically used?
> [S.H] OK.
> 
>>
>> See comments below
>>
>>>
>>> Signed-off-by: Shaohui Xie <Shaohui.Xie@...escale.com>
>>> ---
>>
>> [snip]
>>
>>> +static int aquantia_read_status(struct phy_device *phydev) {
>>> +	int reg;
>>> +
>>> +	phydev->speed = SPEED_10000;
>>> +	phydev->duplex = DUPLEX_FULL;
>>> +
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
>>> +	if (reg & MDIO_STAT1_LSTATUS)
>>> +		phydev->link = 1;
>>> +	else
>>> +		phydev->link = 0;
>>> +
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, 0xc800);
>>> +	mdelay(10);
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, 0xc800);
>>> +	if (reg == 0x9)
>>> +		phydev->speed = SPEED_2500;
>>> +	else if (reg == 0x5)
>>> +		phydev->speed = SPEED_1000;
>>> +	else if (reg == 0x3)
>>> +		phydev->speed = SPEED_100;
>>
>> Could we use a switch/case here? 
> [S.H] OK.
> 
> How about 10Mbits/sec and duplex are we
>> guaranteed to be full-duplex at e.g: 100 or 10Mbits/sec?
> [S.H] The PHY does not support 10M bits/sec. 
> When link to 100M. the phy is full-duplex.

Ok, that means you need to restrict the supported flags accordingly not
to advertise these modes as being supported in the first place, see below:

> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct phy_driver aquantia_driver[] = { {
>>> +	.phy_id		= PHY_ID_AQ1202,
>>> +	.phy_id_mask	= 0xfffffff0,
>>> +	.name		= "Aquantia AQ1202",
>>> +	.features	= PHY_GBIT_FEATURES,
>>
>> If these are 10GbE PHYs, should not we start defining a new features
>> bitmask here to reflect that accordingly? That way MAC
> [S.H] there are several defines for 10G PHYs, should be used by a given 10G PHY. 
> 
> for this Aquantia PHY, SUPPORTED_10000baseT_Full is a valid define, should I set it as below:
> .features	= PHY_GBIT_FEATURES | SUPPORTED_10000baseT_Full,

PHY_GBIT_FEATURES means 10/100/1000 half and full-duplex are supported,
which are not supported as you indicated above, I would go with adding
only the supported modes here, this is really important since this is
the contract between the PHY driver and the Ethernet MAC using it
through the PHY library.

Thanks!
-- 
Florian
--
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