linux-kernel - RE: [PATCH v2] net/phy: tune get_phy_c45

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] [day] [month] [year] [list]

Date:	Wed, 15 Apr 2015 08:27:05 +0000
From:	"Shengzhou.Liu@...escale.com" <Shengzhou.Liu@...escale.com>
To:	Florian Fainelli <f.fainelli@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] net/phy: tune get_phy_c45_ids to support more c45 phy

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@...il.com]
> Sent: Wednesday, April 15, 2015 1:45 AM
> To: Liu Shengzhou-B36685; netdev@...r.kernel.org; davem@...emloft.net
> Cc: linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2] net/phy: tune get_phy_c45_ids to support more c45 phy
> 
> On 14/04/15 03:09, Shengzhou Liu wrote:
> > As some C45 10G PHYs(e.g. Cortina CS4315/CS4340 PHY) have zero Devices
> > In package, current driver can't get correct devices_in_package value
> > by non-zero Devices In package.
> > so let's probe more with zero Devices In package to support more C45
> > PHYs.
> 
> I cannot remember exactly how many times this patch has been posted, but it
> still is not clear to me what you are doing here is helping with these
> Cortina PHYs.
> 
> Could you post a dump of the mdiobus_read() arguments and values for the old
> code and the new code you are proposing? That way it might be clearer what is
> the goal here?
> 
The key point is that standard C45 PHYs use non-zero Devices in package i(reg_addr = MII_ADDR_C45 | i << 16 | MDIO_DEVS) to read  devices_in_package, device zero is reserved, but Cortina CS4315/CS4340 PHY use zero Devices in package(reg_addr = MII_ADDR_C45 | 0 << 16 | MDIO_DEVS) to read devices_in_package. 

This is caused by Cortina non-standard PHY, e.g. standard PHY has MII_PHYSID1=0x02, MII_PHYSID2=0x03, but CS4315/CS4340 PHY has non-standard offset of PHY ID registers(MII_PHYSID1=0x00, MII_PHYSID2=0x01).
  
> >
> > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@...escale.com>
> > ---
> > v2: use MDIO_DEVS1 and MDIO_DEVS2 instead of constant '6', '5'
> >
> >  drivers/net/phy/phy_device.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy_device.c
> > b/drivers/net/phy/phy_device.c index bdfe51f..c4f836f 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -242,12 +242,29 @@ static int get_phy_c45_ids(struct mii_bus *bus, int
> addr, u32 *phy_id,
> >  			return -EIO;
> >  		c45_ids->devices_in_package |= (phy_reg & 0xffff);
> >
> > -		/* If mostly Fs, there is no device there,
> > -		 * let's get out of here.
> > +		/* If mostly Fs, let's continue to probe more
> > +		 * as some c45 PHYs have zero Devices In package,
> > +		 * e.g. Cortina CS4315/CS4340 PHY.
> >  		 */
> >  		if ((c45_ids->devices_in_package & 0x1fffffff) == 0x1fffffff) {
> > -			*phy_id = 0xffffffff;
> > -			return 0;
> > +			reg_addr = MII_ADDR_C45 | 0 << 16 | MDIO_DEVS2;
> > +			phy_reg = mdiobus_read(bus, addr, reg_addr);
> > +			if (phy_reg < 0)
> > +				return -EIO;
> > +			c45_ids->devices_in_package = (phy_reg & 0xffff) << 16;
> > +			reg_addr = MII_ADDR_C45 | 0 << 16 | MDIO_DEVS1;
> > +			phy_reg = mdiobus_read(bus, addr, reg_addr);
> > +			if (phy_reg < 0)
> > +				return -EIO;
> > +			c45_ids->devices_in_package |= (phy_reg & 0xffff);
> > +			/* If mostly Fs, there is no device there,
> > +			 * let's get out of here.
> > +			 */
> > +			if ((c45_ids->devices_in_package & 0x1fffffff) ==
> > +							0x1fffffff) {
> > +				*phy_id = 0xffffffff;
> > +				return 0;
> > +			}
> 
> Could not we somehow be a bit more clever and utilize the loop, with an
> adjusted i = 0 during this condition? Some something like this (untested):
> 
> Florian

I had done it to utilize the loop with i = 0 as your thought, but David Miller said that the way of utilizing the loop makes no sense to test 'i' for zero vs. non-zero until the looping construct it is contained within can actually hit a zero value, adding such a check here makes the code confusing.
So I dropped the way of utilizing the loop to make code readable.