netdev - Re: [PATCH v7 09/11] net: pch_gbe: Convert to mdiobus and phylib

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: <20180627175144.GG885@lunn.ch>
Date:   Wed, 27 Jun 2018 19:51:44 +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 09/11] net: pch_gbe: Convert to mdiobus and phylib

> @@ -5,7 +5,8 @@
>  config PCH_GBE
>  	tristate "OKI SEMICONDUCTOR IOH(ML7223/ML7831) GbE"
>  	depends on PCI && (X86_32 || COMPILE_TEST)
> -	select MII
> +	select PHYLIB
> +	imply AT803X_PHY if X86_32
>  	select PTP_1588_CLOCK_PCH
>  	select NET_PTP_CLASSIFY

That is unusual. I don't think any other MAC driver does this.

If the AT803X driver is not available, it will fall back to the
generic PHY driver. That means RGMII delays will not get set
correctly, no interrupts, no wol, and no workaround for the 8030.

Are any of these relevant to your board?

> @@ -197,16 +151,14 @@ static void pch_gbe_get_regs(struct net_device *netdev,
>  	struct pch_gbe_hw *hw = &adapter->hw;
>  	struct pci_dev *pdev = adapter->pdev;
>  	u32 *regs_buff = p;
> -	u16 i, tmp;
> +	u16 i;
>  
>  	regs->version = 0x1000000 | (__u32)pdev->revision << 16 | pdev->device;
>  	for (i = 0; i < PCH_GBE_MAC_REGS_LEN; i++)
>  		*regs_buff++ = ioread32(&hw->reg->INT_ST + i);
>  	/* PHY register */
> -	for (i = 0; i < PCH_GBE_PHY_REGS_LEN; i++) {
> -		pch_gbe_phy_read_reg_miic(&adapter->hw, i, &tmp);
> -		*regs_buff++ = tmp;
> -	}
> +	for (i = 0; i < PCH_GBE_PHY_REGS_LEN; i++)
> +		*regs_buff++ = phy_read(adapter->phydev, i);
>  }

In general, that is not safe. Some PHYs have pages, and you have no
idea what page is currently selected. If you don't need it, i would
drop this. There are other ways to get access to phy registers, like
miitool, which should do a better job.

	 Andrew