netdev - Re: [PATCH net] net: phy: replace preliminary fix for PHY driver sometimes not binding to the device

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: <8b4f48b1-dc97-1f8d-c9a8-d67b41591d07@gmail.com>
Date:   Fri, 28 Dec 2018 18:42:22 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Norbert Jurkeit <norbert.jurkeit@....de>,
        Frank Crawford <frank@...wford.emu.id.au>
Subject: Re: [PATCH net] net: phy: replace preliminary fix for PHY driver
 sometimes not binding to the device

Le 12/24/18 à 3:21 AM, Heiner Kallweit a écrit :
> phy_device_create() uses request_module() to load the PHY driver module
> based on the PHY ID of the device. There is some timing issue which
> sometimes prevents the PHY driver to bind to the device. In such cases
> the genphy driver is used what can cause problems if genphy isn't
> compatible with the respective PHY.
> It turned out that the first fix can fix the issue in some but not all
> cases. Moving the call to device_initialize() before the call to
> request_module() was reported to fix the issue.
> I can't explain where the root cause of the issue is and why this fix
> works. AFAICS device_initialize() just initializes the device struct
> w/o doing anything that could interfere with e.g. bus_add_driver().
> This patch removes the first preliminary fix attempt.

Humm but phy_device is comprised of a mdio_device on which the actual
matching is done, so you do have to call device_initialize() first in
order for the phy_device instance to have its companion mdio_device's
kobject to be properly initialized.

Out of curiosity, do any of the people who tested that change have the
ability to run a kernel with list/kobject debugging enabled so we can
learn a bit more about the problematic code path?
> 
> Reference:
> https://bugzilla.redhat.com/show_bug.cgi?id=1650984
> 
> Fixes: c85ddecae6e5 ("net: phy: add workaround for issue where PHY driver doesn't bind to the device")
> Tested-by: Norbert Jurkeit <norbert.jurkeit@....de>
> Tested-by: Frank Crawford <frank@...wford.emu.id.au>
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>  drivers/net/phy/phy_device.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 26c41ede5..ac0a83c7d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -579,6 +579,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
>  		dev->c45_ids = *c45_ids;
>  	dev->irq = bus->irq[addr];
>  	dev_set_name(&mdiodev->dev, PHY_ID_FMT, bus->id, addr);
> +	device_initialize(&mdiodev->dev);
>  
>  	dev->state = PHY_DOWN;
>  
> @@ -598,8 +599,6 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
>  	 */
>  	request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, MDIO_ID_ARGS(phy_id));
>  
> -	device_initialize(&mdiodev->dev);
> -
>  	return dev;
>  }
>  EXPORT_SYMBOL(phy_device_create);
> @@ -2191,14 +2190,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
>  	new_driver->mdiodrv.driver.remove = phy_remove;
>  	new_driver->mdiodrv.driver.owner = owner;
>  
> -	/* The following works around an issue where the PHY driver doesn't bind
> -	 * to the device, resulting in the genphy driver being used instead of
> -	 * the dedicated driver. The root cause of the issue isn't known yet
> -	 * and seems to be in the base driver core. Once this is fixed we may
> -	 * remove this workaround.
> -	 */
> -	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
> -
>  	retval = driver_register(&new_driver->mdiodrv.driver);
>  	if (retval) {
>  		pr_err("%s: Error %d in registering driver\n",
> 


-- 
Florian