netdev - Re: [PATCH] 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]

Message-Id: <20140425.155057.855268491985585038.davem@davemloft.net>
Date:	Fri, 25 Apr 2014 15:50:57 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	Shengzhou.Liu@...escale.com
Cc:	netdev@...r.kernel.org, scottwood@...escale.com
Subject: Re: [PATCH] net/phy: tune get_phy_c45_ids to support more c45 phy

From: Shengzhou Liu <Shengzhou.Liu@...escale.com>
Date: Thu, 24 Apr 2014 10:17:36 +0800

>  		if ((c45_ids->devices_in_package & 0x1fffffff) == 0x1fffffff) {
> -			*phy_id = 0xffffffff;
> -			return 0;
> +			reg_addr = MII_ADDR_C45 | 0 << 16 | 6;
> +			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 | 5;
> +			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 ->devices_in_package is zero, we will not get to this code path, so
this comment makes no sense at all to me.

You need to explain the situation more accurately, and clearly.  I have
no idea what exactly the devices report in this situation, nor how your
code is trying to cope with that situation.

Please work slowly, and carefully, to write clear and accurate
comments as well as a commit message which describes (in detail) the
situation here.

Someone in the future will end up having to read this change and
try to understand it as well, so it's not just for me.

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