[2/2] Fix phy_attach - forward dev_flags for phy_attach

Commit Message

Kosta Zertsekel Jan. 10, 2013, noon UTC

Change-Id: Ie3191f95c36eada6d0c673460de5393641128182
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 2 +-
 net/dsa/slave.c                           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Florian Fainelli Jan. 10, 2013, 2:40 p.m. UTC | #1

Le 01/10/13 13:00, Kosta Zertsekel a écrit :
> Change-Id: Ie3191f95c36eada6d0c673460de5393641128182
> ---
>   drivers/net/ethernet/marvell/pxa168_eth.c | 2 +-
>   net/dsa/slave.c                           | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

I think that you should have actually two patches, one for pxa168_eth 
and one for net/dsa/slave.c.

Also, please prefix your patches with what was usually used on these 
files before: "pxa168_eth:" and "net: dsa:" respectively.

By the way, most, if not all of the phy_connect() users in 
drivers/net/ethernet/ also do not ensure they pass the phy device flags, 
so you might want to fix this globally and not just for Marvell driver.

Thanks.
--
Florian

Kosta Zertsekel Jan. 10, 2013, 3:57 p.m. UTC | #2

> By the way, most, if not all of the phy_connect() users in drivers/net/ethernet/ also do not ensure they pass the phy device flags, so you might 

want to fix this globally and not just for Marvell driver.
Indeed, phy_connect() mostly just pass zero intead of phy_dev->dev_flags.
But, I think, the guy that calls phy_connect() in its driver should know what he does, and,
probably, we should rely on him knowing his stuff.
The only evidence of the bug is when phy_dev->dev_flags was actually changed by PHY fixup callback
(see dns323-setup.c for example) and was *not* propagated to phy_connect() or phy_attach() as in pxa168_eth.c phy_init().
The code conforming to the former should be fixed IMHO.

Thanks,
--- KostaZ

Florian Fainelli Jan. 10, 2013, 5:12 p.m. UTC | #3

Le 01/10/13 16:57, Kosta Zertsekel a écrit :
>> By the way, most, if not all of the phy_connect() users in drivers/net/ethernet/ also do not ensure they pass the phy device flags, so you might
> want to fix this globally and not just for Marvell driver.
> Indeed, phy_connect() mostly just pass zero intead of phy_dev->dev_flags.
> But, I think, the guy that calls phy_connect() in its driver should know what he does, and,
> probably, we should rely on him knowing his stuff.
> The only evidence of the bug is when phy_dev->dev_flags was actually changed by PHY fixup callback
> (see dns323-setup.c for example) and was *not* propagated to phy_connect() or phy_attach() as in pxa168_eth.c phy_init().
> The code conforming to the former should be fixed IMHO.

Actually, I wonder if we should not rather remove entirely the flags 
argument, let the phy_connect() or phy_attach() callers modify the phy 
device dev_flags like it does today (e.g: tg3) and modify phy_connect() 
and phy_attach() to pass phy->dev_flags to phy_attach_direct().
--
Florian

Patch

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 10d678d..63baa3b 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -1391,7 +1391,7 @@  static void phy_init(struct pxa168_eth_private *pep, int speed, int duplex)
 	struct phy_device *phy = pep->phy;
 	ethernet_phy_reset(pep);
 
-	phy_attach(pep->dev, dev_name(&phy->dev), 0, PHY_INTERFACE_MODE_MII);
+	phy_attach(pep->dev, dev_name(&phy->dev), phy->dev_flags, PHY_INTERFACE_MODE_MII);
 
 	if (speed == 0) {
 		phy->autoneg = AUTONEG_ENABLE;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e32083d..bf09902 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -391,7 +391,7 @@  dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 
 	if (p->phy != NULL) {
 		phy_attach(slave_dev, dev_name(&p->phy->dev),
-			   0, PHY_INTERFACE_MODE_GMII);
+			   phy->dev_flags, PHY_INTERFACE_MODE_GMII);
 
 		p->phy->autoneg = AUTONEG_ENABLE;
 		p->phy->speed = 0;