netdev - Re: [PATCH net-next 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

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: <e707044d-a1d3-40c4-aeef-fad68c6a1785@gmail.com>
Date: Tue, 3 Dec 2024 16:35:09 +0800
From: Zhiyuan Wan <kmlinuxm@...il.com>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: linux@...linux.org.uk, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, willy.liu@...ltek.com,
 Yuki Lee <febrieac@...look.com>, andrew@...n.ch
Subject: Re: [PATCH net-next 1/2] net: phy: realtek: disable broadcast address
 feature of rtl8211f

On 2024/12/3 15:38, Heiner Kallweit wrote:
> 
> Take care to remove the Rb tag if you make changes to the patch.
> 
Roger that.
> 
> This still uses the _changed version even if not needed.
> 
I'm not sure is it okay to directly write PHYCR1 register, because not
only it controls ALDPS and broadcast PHY address, but also controls
MDI mode/Jabber detection.

I'm afraid that maybe it causes problem if I don't use RMW to clear
the PHYAD_EN bit. Because the following code in `rtl8211f_config_init`
also utilizes `phy_modify_paged_changed` to change ALDPS setting
without touching anything else.

But if you insist, I can modify code to this if you like:


	ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1);
	if (ret < 0)
		return ret;

	dev_dbg(dev, "disabling MDIO address 0 for this phy");
	priv->phycr1 = ret & (u16)~RTL8211F_PHYAD0_EN;
	ret = phy_write_paged(phydev, 0xa43, RTL8211F_PHYCR1,
			      priv->phycr1);
	if (ret < 0) {
		return dev_err_probe(dev, ret,
			             "disabling MDIO address 0 failed\n");
	}
	/* Don't allow using broadcast address as PHY address */
	if (phydev->mdio.addr == 0)
		return -ENODEV;

 	priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);
...



>> +	if (ret < 0) {
>> +		dev_err(dev, "disabling MDIO address 0 failed: %pe\n",
>> +			ERR_PTR(ret));
> 
> You may want to use dev_err_probe() here. And is it by intent that
> the error is ignored and you go on?
> 

I'm sorry that I made a mistake.

> 
> And one more formal issue:
> You annotated the patch as 1/2, but submit it as single patch.
> 
I have another patch to enable support optical/copper combo support
of RTL8211FS PHY in this mail thread, but since Andrew said that patch
(migrated from Rockchip SDK) is low quality and I'm too busy with my
job, don't have much time to read and improve it, so I decided to
suspend that patch's submission and I'll resume to submit that patch
when I'm free. Could you please give me some advice or recommends on it?

Sincerely,

Zhiyuan Wan