linux-kernel - Re: [PATCH v13, 2/2] net: Add dm9051 driver

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]

Message-ID: <YfACRXnsAUVxlZze@lunn.ch>
Date:   Tue, 25 Jan 2022 14:59:33 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Joseph CHAMG <josright123@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, joseph_chang@...icom.com.tw,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, andy.shevchenko@...il.com,
        leon@...nel.org
Subject: Re: [PATCH v13, 2/2] net: Add dm9051 driver

> +static int dm9051_mdiobus_read(struct mii_bus *mdiobus, int phy_id, int reg)
> +{
> +	struct board_info *db = mdiobus->priv;
> +	unsigned int val = 0;
> +	int ret;
> +
> +	if (phy_id == DM9051_PHY_ID) {

phy_id is a poor choice of name. It normally means the value you find
in register 2 and 3 of the PHY which identifies the manufacture, make
and possibly revision.

If you look at the read function prototype in struct mii_bus:

https://elixir.bootlin.com/linux/v5.17-rc1/source/include/linux/phy.h#L357

the normal name is addr.

Ideally your driver needs to look similar to other drivers. Ideally
you use the same variable names for the same things. That makes it
easier for somebody else to read your driver and debug it. It makes it
easier to review, etc. It is worth spending time reading a few other
drivers and looking for common patterns, and making use of those
patterns in your driver.

> +static int dm9051_map_phyup(struct board_info *db)
> +{
> +	int ret;
> +
> +	/* ~BMCR_PDOWN to power-up the internal phy
> +	 */
> +	ret = mdiobus_modify(db->mdiobus, DM9051_PHY_ID, MII_BMCR, BMCR_PDOWN, 0);
> +	if (ret < 0)
> +		return ret;

You are still touching PHY registers from the MAC driver. Why is your
PHY driver not going this as part of the _config() function?

    Andrew