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]

Date:   Fri, 28 Dec 2018 18:48:21 -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/28/18 à 6:42 PM, Florian Fainelli a écrit :
> 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?

Heiner; are you positive that the PHY is not in a power down mode
(BMCR.PDOWN = 1) at the time the r8169 probe is done? Because if that
was the case, there is no guarantee (per 802.3 clause 22 spec) that the
PHY must correctly respond to MDIO operations other than read/write to
BMCR register and this could explain that problem, as well as the
general lack of connectivity for these users.

Do you have any way to check that by any chance? We should probably take
care of that in the PHY library at some point.

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