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] [day] [month] [year] [list]

Message-ID: <af9e7960-ae36-43f9-bf54-0b6b5588b340@gmail.com>
Date: Tue, 3 Dec 2024 10:52:05 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Zhiyuan Wan <kmlinuxm@...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 03.12.2024 09:35, Zhiyuan Wan wrote:
> 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.
> 

This was not my point. Just use phy_modify_paged().

> 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?
> 

The patch here seems to be independent and should be properly submitted
as single patch. Regarding the other patch I have nothing to add to what
Andrew stated already.

> Sincerely,
> 
> Zhiyuan Wan