linux-kernel - Re: [PATCH net-next v3 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 3 Nov 2020 11:07:00 -0600
From:   Dan Murphy <dmurphy@...com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <davem@...emloft.net>, <f.fainelli@...il.com>,
        <hkallweit1@...il.com>, <robh@...nel.org>,
        <devicetree@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v3 4/4] net: phy: dp83td510: Add support for the
 DP83TD510 Ethernet PHY

Andrew

On 10/30/20 3:15 PM, Andrew Lunn wrote:
>> +static int dp83td510_config_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	int mst_slave_cfg;
>> +	int ret = 0;
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		if (dp83td510->rgmii_delay) {
>> +			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
>> +					       DP83TD510_MAC_CFG_1, dp83td510->rgmii_delay);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
> Hi Dan
>
> I'm getting a bit paranoid about RGMII delays...
Not sure what this means.
>
>> +static int dp83td510_read_straps(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	int strap;
>> +
>> +	strap = phy_read_mmd(phydev, DP83TD510_DEVADDR, DP83TD510_SOR_1);
>> +	if (strap < 0)
>> +		return strap;
>> +
>> +	if (strap & DP83TD510_RGMII)
>> +		dp83td510->is_rgmii = true;
>> +
>> +	return 0;
>> +};
> So dp83td510->is_rgmii is the strapping configuration. So if one of
> the four RGMII modes is selected, your appear to ignore which of the
> four is selected, and program the hardware with the strapping?
>
> That seems like a bad idea.
I will re-look at this code.
>
>> +#if IS_ENABLED(CONFIG_OF_MDIO)
>> +static int dp83td510_of_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct device_node *of_node = dev->of_node;
>> +	s32 rx_int_delay;
>> +	s32 tx_int_delay;
>> +	int ret;
>> +
>> +	if (!of_node)
>> +		return -ENODEV;
>> +
>> +	ret = dp83td510_read_straps(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dp83td510->hi_diff_output = device_property_read_bool(&phydev->mdio.dev,
>> +							      "tx-rx-output-high");
>> +
>> +	if (device_property_read_u32(&phydev->mdio.dev, "tx-fifo-depth",
>> +				     &dp83td510->tx_fifo_depth))
>> +		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB;
> Please don't use device_property_read_foo API, we don't want to give
> the impression it is O.K. to stuff DT properties in ACPI
> tables. Please use of_ API calls.

Hmm. Is this a new stance in DT handling for the networking tree?

If it is should I go back and rework some of my other drivers that use 
device_property APIs

Dan