linux-kernel - Re: [PATCH 3/6] net: bcmgenet: enable automatic phy discovery

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:   Sat, 1 Feb 2020 14:02:01 -0600
From:   Jeremy Linton <jeremy.linton@....com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, opendmb@...il.com, f.fainelli@...il.com,
        davem@...emloft.net, bcm-kernel-feedback-list@...adcom.com,
        linux-kernel@...r.kernel.org, wahrenst@....net,
        hkallweit1@...il.com
Subject: Re: [PATCH 3/6] net: bcmgenet: enable automatic phy discovery

Hi,

On 2/1/20 9:25 AM, Andrew Lunn wrote:
> On Sat, Feb 01, 2020 at 01:46:22AM -0600, Jeremy Linton wrote:
>> The unimac mdio driver falls back to scanning the
>> entire bus if its given an appropriate mask. In ACPI
>> mode we expect that the system is well behaved and
>> conforms to recent versions of the specification.
>>
>> We then utilize phy_find_first(), and
>> phy_connect_direct() to find and attach to the
>> discovered phy during net_device open.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
>> ---
>>   drivers/net/ethernet/broadcom/genet/bcmmii.c | 40 +++++++++++++++++---
>>   1 file changed, 34 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index 2049f8218589..f3271975b375 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -5,7 +5,7 @@
>>    * Copyright (c) 2014-2017 Broadcom
>>    */
>>   
>> -
>> +#include <linux/acpi.h>
>>   #include <linux/types.h>
>>   #include <linux/delay.h>
>>   #include <linux/wait.h>
>> @@ -311,7 +311,9 @@ int bcmgenet_mii_config(struct net_device *dev, bool init)
>>   int bcmgenet_mii_probe(struct net_device *dev)
>>   {
>>   	struct bcmgenet_priv *priv = netdev_priv(dev);
>> -	struct device_node *dn = priv->pdev->dev.of_node;
>> +	struct device *kdev = &priv->pdev->dev;
>> +	struct device_node *dn = kdev->of_node;
>> +
>>   	struct phy_device *phydev;
>>   	u32 phy_flags = 0;
>>   	int ret;
>> @@ -334,7 +336,27 @@ int bcmgenet_mii_probe(struct net_device *dev)
>>   			return -ENODEV;
>>   		}
>>   	} else {
>> -		phydev = dev->phydev;
>> +		if (has_acpi_companion(kdev)) {
>> +			char mdio_bus_id[MII_BUS_ID_SIZE];
>> +			struct mii_bus *unimacbus;
>> +
>> +			snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
>> +				 UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
>> +
>> +			unimacbus = mdio_find_bus(mdio_bus_id);
>> +			if (!unimacbus) {
>> +				pr_err("Unable to find mii\n");
>> +				return -ENODEV;
>> +			}
>> +			phydev = phy_find_first(unimacbus);
>> +			put_device(&unimacbus->dev);
>> +			if (!phydev) {
>> +				pr_err("Unable to find PHY\n");
>> +				return -ENODEV;
> 
> Hi Jeremy
> 
> phy_find_first() is not recommended. Only use it if you have no other
> option. If the hardware is more complex, two PHYs on one bus, you are
> going to have a problem. So i suggest this is used only for PCI cards
> where the hardware is very fixed, and there is only ever one MAC and
> PHY on the PCI card. When you do have this split between MAC and MDIO
> bus, each being independent devices, it is more likely that you do
> have multiple PHYs on one shared MDIO bus.
> 
> In the DT world, you use a phy-handle to point to the PHY node in the
> device tree. Does ACPI have the same concept, a pointer to some other
> device in ACPI?


I though I should clarify the direct question here about ACPI. ACPI does 
have the ability to do what you describe, but it a more rigorous way. If 
you look at the ACPI GenericSerialBus abstraction you will see how ACPI 
would likely handle this situation. I've been considering making a 
similar comment in that large fwnode patch set posted the other day.