netdev - Re: [PATCH v7 03/11] net: pch_gbe: Probe PHY ID & initialize only once

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180627172131.GB885@lunn.ch>
Date:   Wed, 27 Jun 2018 19:21:31 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Paul Burton <paul.burton@...s.com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v7 03/11] net: pch_gbe: Probe PHY ID & initialize only
 once

On Tue, Jun 26, 2018 at 05:06:04PM -0700, Paul Burton wrote:
> The pch_gbe driver currently probes for the PHY ID & configures the PHY
> every time the MAC is reset, even though we know that the PHY won't have
> changed since the last MAC reset [1].
> 
> This patch moves the PHY probe to instead happen only once when the
> driver is probed, saving time & moving us closer to the behavior we'll
> have with phylib.
> 
> [1] Please, someone patent PHY hotplugging & rigorously enforce said
>     patent such that nobody can do it. At least not with an EG20T MAC.

Hi Paul

It is already possible, and probably patented. SFP cages are usually
used for fibre optical modules. But it is also possible to have copper
modules, which contain a standard PHY. And SFP modules are
hot-plugable...

> 
> Signed-off-by: Paul Burton <paul.burton@...s.com>
> Cc: Andrew Lunn <andrew@...n.ch>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: netdev@...r.kernel.org
> ---
> 
> Changes in v7: New patch
> 
>  .../ethernet/oki-semi/pch_gbe/pch_gbe_main.c  | 26 ++++++++++---------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> index 9651fa02d4bb..5157cea16773 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> @@ -617,8 +617,10 @@ static void pch_gbe_init_stats(struct pch_gbe_adapter *adapter)
>  static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter)
>  {
>  	struct net_device *netdev = adapter->netdev;
> +	struct pch_gbe_hw *hw = &adapter->hw;
>  	u32 addr;
>  	u16 bmcr, stat;
> +	s32 ret_val;
>  
>  	/* Discover phy addr by searching addrs in order {1,0,2,..., 31} */
>  	for (addr = 0; addr < PCH_GBE_PHY_REGS_LEN; addr++) {
> @@ -652,6 +654,16 @@ static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter)
>  	adapter->mii.mdio_read = pch_gbe_mdio_read;
>  	adapter->mii.mdio_write = pch_gbe_mdio_write;
>  	adapter->mii.supports_gmii = mii_check_gmii_support(&adapter->mii);
> +
> +	ret_val = pch_gbe_phy_get_id(hw);
> +	if (ret_val) {
> +		netdev_err(adapter->netdev, "pch_gbe_phy_get_id error\n");
> +		return -EIO;
> +	}
> +	pch_gbe_phy_init_setting(hw);
> +	/* Setup Mac interface option RGMII */
> +	pch_gbe_phy_set_rgmii(hw);
> +
>  	return 0;
>  }
>  
> @@ -721,22 +733,12 @@ void pch_gbe_reset(struct pch_gbe_adapter *adapter)
>  {
>  	struct net_device *netdev = adapter->netdev;
>  	struct pch_gbe_hw *hw = &adapter->hw;
> -	s32 ret_val;
>  
>  	pch_gbe_mac_reset_hw(hw);
>  	/* reprogram multicast address register after reset */
>  	pch_gbe_set_multi(netdev);
>  	/* Setup the receive address. */
>  	pch_gbe_mac_init_rx_addrs(hw, PCH_GBE_MAR_ENTRIES);
> -
> -	ret_val = pch_gbe_phy_get_id(hw);
> -	if (ret_val) {
> -		netdev_err(adapter->netdev, "pch_gbe_phy_get_id error\n");
> -		return;
> -	}
> -	pch_gbe_phy_init_setting(hw);
> -	/* Setup Mac interface option RGMII */
> -	pch_gbe_phy_set_rgmii(hw);
>  }
>  
>  /**

Up to here, everything looks O.K.

> @@ -2577,6 +2579,8 @@ static int pch_gbe_probe(struct pci_dev *pdev,
>  	if (ret)
>  		goto err_free_netdev;
>  
> +	pch_gbe_check_options(adapter);
> +
>  	/* Initialize PHY */
>  	ret = pch_gbe_init_phy(adapter);
>  	if (ret) {
> @@ -2606,8 +2610,6 @@ static int pch_gbe_probe(struct pci_dev *pdev,
>  
>  	INIT_WORK(&adapter->reset_task, pch_gbe_reset_task);
>  
> -	pch_gbe_check_options(adapter);
> -
>  	/* initialize the wol settings based on the eeprom settings */
>  	adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING;
>  	dev_info(&pdev->dev, "MAC address : %pM\n", netdev->dev_addr);

But these two changes seem unrelated. Should they be in a different
patch?

	Andrew