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 for Android: free password hash cracker in your pocket

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

Message-ID: <BLUPR03MB14753A6DDD02A9F944F2723EE28E0@BLUPR03MB1475.namprd03.prod.outlook.com>
Date:	Mon, 27 Jul 2015 08:30:31 +0000
From:	Shaohui Xie <Shaohui.Xie@...escale.com>
To:	Florian Fainelli <f.fainelli@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH] phylib: add driver for aquantia phy

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

> 
> > +
> > +	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,

Or handle the SUPPORTED_10000baseT_Full separately?

Thanks for reviewing!
Shaohui