netdev - Re: [PATCH net-next 2/3] net: phy: aquantia: report PHY details like firmware version

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:   Sat, 23 Mar 2019 18:04:50 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: phy: aquantia: report PHY details like
 firmware version



On 3/23/2019 6:15 AM, Heiner Kallweit wrote:
> Add reporting firmware details. These details are available only once
> the firmware has finished initializing the chip. This can take some
> time and we need to poll for init completion.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---

[snip]
>  
> +/* If we configure settings whilst firmware is still initializing the chip,
> + * then these settings may be overwritten. Therefore make sure chip
> + * initialization has completed. Use presence of the firmware ID as
> + * indicator for initialization having completed.
> + * The chip also provides a "reset completed" bit, but it's cleared after
> + * read. Therefore function would time out if called again.
> + */

Is there a way to say, run a checksum calculation on the firmware image
to assess its health/validity as well as read the firmware ID? What
happens if the PHY is not provisioned with a firmware image? Is it
expecting for a specific set of MDIO vendor commands to load it over MDIO?

> +static void aqr107_wait_reset_complete(struct phy_device *phydev)
> +{
> +	int val, retries = 100;
> +
> +	do {
> +		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
> +		if (val < 0)
> +			return;
> +		msleep(20);
> +	} while (!val && --retries);

Should not this return 0/-ETIMEDOUT and have the caller propagate that
error code?
-- 
Florian