| 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
| ||
|
Message-ID: <418949c9-9aa2-4845-be8a-3395413143c3@gmail.com>
Date: Sun, 24 Aug 2025 18:36:24 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Hauke Mehrtens <hauke@...ke-m.de>, Greg Ungerer <gerg@...ux-m68k.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>, Rafał Miłecki
<zajec5@...il.com>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Andrew Lunn <andrew@...n.ch>, Andrew Lunn <andrew+netdev@...n.ch>,
Russell King - ARM Linux <linux@...linux.org.uk>,
Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>,
Takumi Sueda <puhitaku@...il.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
linux-m68k@...ts.linux-m68k.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use
addr 0 and remove return value
On 8/24/2025 6:05 PM, Hauke Mehrtens wrote:
> On 8/22/25 22:36, Heiner Kallweit wrote:
>> We have only two users of fixed_phy_add(), both use address 0 and
>> ignore the return value. So simplify fixed_phy_add() accordingly.
>>
>> Whilst at it, constify the fixed_phy_status configs.
>>
>> Note:
>> fixed_phy_add() is a legacy function which shouldn't be used in new
>> code, as it's use may be problematic:
>> - No check whether a fixed phy exists already at the given address
>> - If fixed_phy_register() is called afterwards by any other driver,
>> then it will also use phy_addr 0, because fixed_phy_add() ignores
>> the ida which manages address assignment
>> Drivers using a fixed phy created by fixed_phy_add() in platform code,
>> should dynamically create a fixed phy with fixed_phy_register()
>> instead.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>> arch/m68k/coldfire/m5272.c | 4 ++--
>> arch/mips/bcm47xx/setup.c | 4 ++--
>> drivers/net/phy/fixed_phy.c | 4 ++--
>> include/linux/phy_fixed.h | 8 ++------
>> 4 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/m68k/coldfire/m5272.c b/arch/m68k/coldfire/m5272.c
>> index 5b70dfdab..918e2a323 100644
>> --- a/arch/m68k/coldfire/m5272.c
>> +++ b/arch/m68k/coldfire/m5272.c
>> @@ -108,7 +108,7 @@ void __init config_BSP(char *commandp, int size)
>> * an ethernet switch. In this case we need to use the fixed phy type,
>> * and we need to declare it early in boot.
>> */
>> -static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
>> +static const struct fixed_phy_status nettel_fixed_phy_status __initconst = {
>> .link = 1,
>> .speed = 100,
>> .duplex = 0,
>> @@ -119,7 +119,7 @@ static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
>> static int __init init_BSP(void)
>> {
>> m5272_uarts_init();
>> - fixed_phy_add(0, &nettel_fixed_phy_status);
>> + fixed_phy_add(&nettel_fixed_phy_status);
>> clkdev_add_table(m5272_clk_lookup, ARRAY_SIZE(m5272_clk_lookup));
>> return 0;
>> }
>> diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
>> index de426a474..a93a4266d 100644
>> --- a/arch/mips/bcm47xx/setup.c
>> +++ b/arch/mips/bcm47xx/setup.c
>> @@ -256,7 +256,7 @@ static int __init bcm47xx_cpu_fixes(void)
>> }
>> arch_initcall(bcm47xx_cpu_fixes);
>> -static struct fixed_phy_status bcm47xx_fixed_phy_status __initdata = {
>> +static const struct fixed_phy_status bcm47xx_fixed_phy_status __initconst = {
>> .link = 1,
>> .speed = SPEED_100,
>> .duplex = DUPLEX_FULL,
>> @@ -282,7 +282,7 @@ static int __init bcm47xx_register_bus_complete(void)
>> bcm47xx_leds_register();
>> bcm47xx_workarounds();
>> - fixed_phy_add(0, &bcm47xx_fixed_phy_status);
>> + fixed_phy_add(&bcm47xx_fixed_phy_status);
>> return 0;
>> }
>> device_initcall(bcm47xx_register_bus_complete);
>> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
>> index 7902b35c5..b39532abf 100644
>> --- a/drivers/net/phy/fixed_phy.c
>> +++ b/drivers/net/phy/fixed_phy.c
>> @@ -153,9 +153,9 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,
>> return 0;
>> }
>> -int fixed_phy_add(int phy_addr, const struct fixed_phy_status *status)
>> +void fixed_phy_add(const struct fixed_phy_status *status)
>> {
>> - return fixed_phy_add_gpiod(PHY_POLL, phy_addr, status, NULL);
>> + fixed_phy_add_gpiod(PHY_POLL, 0, status, NULL);
>> }
>> EXPORT_SYMBOL_GPL(fixed_phy_add);
>> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
>> index 5399b9e41..6227a1bde 100644
>> --- a/include/linux/phy_fixed.h
>> +++ b/include/linux/phy_fixed.h
>> @@ -17,7 +17,7 @@ struct net_device;
>> #if IS_ENABLED(CONFIG_FIXED_PHY)
>> extern int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier);
>> -int fixed_phy_add(int phy_id, const struct fixed_phy_status *status);
>> +void fixed_phy_add(const struct fixed_phy_status *status);
>> struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
>> struct device_node *np);
>> @@ -26,11 +26,7 @@ extern int fixed_phy_set_link_update(struct phy_device *phydev,
>> int (*link_update)(struct net_device *,
>> struct fixed_phy_status *));
>> #else
>> -static inline int fixed_phy_add(int phy_id,
>> - const struct fixed_phy_status *status)
>> -{
>> - return -ENODEV;
>> -}
>> +static inline void fixed_phy_add(const struct fixed_phy_status *status) {}
>> static inline struct phy_device *
>> fixed_phy_register(const struct fixed_phy_status *status,
>> struct device_node *np)
>
> Hi,
>
> I do not use this hardware any more, but Takumi reported that fixed_phy_add() is not working for the PHY registration on brcm47xx any more and we have to use fixed_phy_register(), see:
> https://github.com/openwrt/openwrt/pull/19610
>
I suspected already that fixed_phy_add() usage doesn't work any longer, but don't
have hw to test. I think bfa54812f0bc ("net: phy: fixed_phy: set phy_mask before
calling mdiobus_register()") is to blame.
Reverting this commit or changing the line to fmb->mii_bus->phy_mask = ~1;
should fix the problem. Would be great if Takumi could test this.
The cleaner alternative would be:
- remove call to fixed_phy_add() from platform code
- let b44 call fixed_phy_register() if a fixed phy is needed
Below is a WIP version of this change.
> Does this need a bigger refactoring anyway?
>
> Hauke
Heiner
---
drivers/net/ethernet/broadcom/Kconfig | 1 +
drivers/net/ethernet/broadcom/b44.c | 33 ++++++++++++++-------------
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 0fc10e6c6..99292392d 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -25,6 +25,7 @@ config B44
select SSB
select MII
select PHYLIB
+ select FIXED_PHY
help
If you have a network (Ethernet) controller of this type, say Y
or M here.
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 0353359c3..abbb5953c 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -31,6 +31,7 @@
#include <linux/ssb/ssb.h>
#include <linux/slab.h>
#include <linux/phy.h>
+#include <linux/phy_fixed.h>
#include <linux/uaccess.h>
#include <asm/io.h>
@@ -2233,7 +2234,6 @@ static int b44_register_phy_one(struct b44 *bp)
struct mii_bus *mii_bus;
struct ssb_device *sdev = bp->sdev;
struct phy_device *phydev;
- char bus_id[MII_BUS_ID_SIZE + 3];
struct ssb_sprom *sprom = &sdev->bus->sprom;
int err;
@@ -2260,27 +2260,26 @@ static int b44_register_phy_one(struct b44 *bp)
goto err_out_mdiobus;
}
- if (!mdiobus_is_registered_device(bp->mii_bus, bp->phy_addr) &&
- (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM))) {
-
+ phydev = mdiobus_get_phy(bp->mii_bus, bp->phy_addr);
+ if (!phydev &&
+ sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM)) {
dev_info(sdev->dev,
"could not find PHY at %i, use fixed one\n",
bp->phy_addr);
- bp->phy_addr = 0;
- snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, "fixed-0",
- bp->phy_addr);
- } else {
- snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, mii_bus->id,
- bp->phy_addr);
+ phydev = fixed_phy_register(NULL, NULL);
+ if (!IS_ERR(phydev))
+ bp->phy_addr = phydev->mdio.addr;
}
- phydev = phy_connect(bp->dev, bus_id, &b44_adjust_link,
- PHY_INTERFACE_MODE_MII);
- if (IS_ERR(phydev)) {
+ if (IS_ERR_OR_NULL(phydev))
+ err = -ENODEV;
+ else
+ err = phy_connect_direct(bp->dev, phydev, &b44_adjust_link,
+ PHY_INTERFACE_MODE_MII);
+ if (err) {
dev_err(sdev->dev, "could not attach PHY at %i\n",
bp->phy_addr);
- err = PTR_ERR(phydev);
goto err_out_mdiobus_unregister;
}
@@ -2293,7 +2292,6 @@ static int b44_register_phy_one(struct b44 *bp)
linkmode_copy(phydev->advertising, phydev->supported);
bp->old_link = 0;
- bp->phy_addr = phydev->mdio.addr;
phy_attached_info(phydev);
@@ -2313,8 +2311,11 @@ static void b44_unregister_phy_one(struct b44 *bp)
{
struct net_device *dev = bp->dev;
struct mii_bus *mii_bus = bp->mii_bus;
+ struct phy_device *phydev = dev->phydev);
- phy_disconnect(dev->phydev);
+ phy_disconnect(phydev);
+ if (phy_is_pseudo_fixed_link(phydev))
+ fixed_phy_unregister(phydev);
mdiobus_unregister(mii_bus);
mdiobus_free(mii_bus);
}
--
2.50.1
Powered by blists - more mailing lists