Re: [PATCH] net: smsc911x: Fix unload crash when link is up

[Jeremy Linton]

Hi,

On 03/06/2018 09:23 AM, Andrew Lunn wrote:

This is caused by the mdiobus being unregistered/free'd
and the code in phy_detach() attempting to manipulate mdio
related structures from unregister_netdev() calling close()

To fix this, we delay the mdiobus teardown until after
the netdev is deregistered.

Reported-by: Matt Sealey <matt.sea...@arm.com>
Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
  drivers/net/ethernet/smsc/smsc911x.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 012fb66eed8d..f0afb88d7bc2 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
-   WARN_ON(dev->phydev);


Hi Jeremy

I assume this WARN_ON() also fired? It would be good to comment about
why you removed it, that the code now handles that case.


Yes, the phydev is started and assigned in the netdev _open and 
stopped/set to null in the _stop. Since the module remove is not blocked 
by having the netdev active, and unregister_netdev closes out active 
connections, the WARN_ON would needlessly trigger if the netdev was 
still open.




Apart from that

Reviewed-by: Andrew Lunn <and...@lunn.ch>


Thanks for looking at this.

[PATCH] net: smsc911x: Fix unload crash when link is up

[Jeremy Linton]

The smsc911x driver will crash if it is rmmod'ed while the netdev
is up like:

Call trace:
  phy_detach+0x94/0x150
  phy_disconnect+0x40/0x50
  smsc911x_stop+0x104/0x128 [smsc911x]
  __dev_close_many+0xb4/0x138
  dev_close_many+0xbc/0x190
  rollback_registered_many+0x140/0x460
  rollback_registered+0x68/0xb0
  unregister_netdevice_queue+0x100/0x118
  unregister_netdev+0x28/0x38
  smsc911x_drv_remove+0x58/0x130 [smsc911x]
  platform_drv_remove+0x30/0x50
  device_release_driver_internal+0x15c/0x1f8
  driver_detach+0x54/0x98
  bus_remove_driver+0x64/0xe8
  driver_unregister+0x34/0x60
  platform_driver_unregister+0x20/0x30
  smsc911x_cleanup_module+0x14/0xbca8 [smsc911x]
  SyS_delete_module+0x1e8/0x238
  __sys_trace_return+0x0/0x4

This is caused by the mdiobus being unregistered/free'd
and the code in phy_detach() attempting to manipulate mdio
related structures from unregister_netdev() calling close()

To fix this, we delay the mdiobus teardown until after
the netdev is deregistered.

Reported-by: Matt Sealey <matt.sea...@arm.com>
Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 012fb66eed8d..f0afb88d7bc2 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
-   WARN_ON(dev->phydev);
 
SMSC_TRACE(pdata, ifdown, "Stopping driver");
 
+   unregister_netdev(dev);
+
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
 
-   unregister_netdev(dev);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
   "smsc911x-memory");
if (!res)
-- 
2.13.6

Re: mvpp2 crash under load.

[Jeremy Linton]

Hi,

First, thanks for taking a look at this.


On 01/23/2018 01:53 AM, Antoine Tenart wrote:

Hi Jeremy,

On Mon, Jan 22, 2018 at 05:14:27PM -0600, Jeremy Linton wrote:


I'm running 4.15rc7 and hitting the following crash on the MACCHIATObin.
This is 100% reproducible once the adapter is given any load. Within a few
seconds of starting a scp or nfs copies inbound to the machine it dies like
this:


[12544.192436] mvpp2 f400.ethernet eth2: wrong cpu on the end of Tx
processing
[12548.513734] mvpp2 f400.ethernet eth2: wrong cpu on the end of Tx
processing
[12548.623574] mvpp2 f400.ethernet eth2: wrong cpu on the end of Tx
processing


I believe this is the root cause of this issue: txq_done() is scheduled
on the wrong CPU and we know it can't run on 2 CPUs at the same time. We
had a similar issue (same stack trace, different root cause):
082297e61480c4d72ed75b31077e74aca0e7c799


I'm pretty sure I already had that patch, I've rebased to 4.15rc9 and it 
continues. I also cherry picked "net: mvpp2: only free the TSO header 
buffers when it was allocated" from net-next which didn't appear to fix 
it either.


Thanks,




Thanks for reporting this!

Antoine


[12548.630943] Unable to handle kernel paging request at virtual address
97ffd6fdd28000e8
[12548.638897] Mem abort info:
[12548.641703]   ESR = 0x9604
[12548.644775]   Exception class = DABT (current EL), IL = 32 bits
[12548.650720]   SET = 0, FnV = 0
[12548.653795]   EA = 0, S1PTW = 0
[12548.656952] Data abort info:
[12548.659846]   ISV = 0, ISS = 0x0004
[12548.663700]   CM = 0, WnR = 0
[12548.84] [97ffd6fdd28000e8] address between user and kernel address
ranges
[12548.673855] Internal error: Oops: 9604 [#1] SMP
[12548.678757] Modules linked in: ax88179_178a usbnet ip6t_rpfilter
ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat
ebtable_brox
[12548.749992]  xhci_plat_hcd ahci_platform [last unloaded: usbnet]
[12548.756034] CPU: 3 PID: 0 Comm: swapper/3 Not tainted
4.15.0-0.rc7.git0.1.fc28.aarch64 #1
[12548.764249] Hardware name: Marvell Armada 8040 MacchiatoBin/Armada 8040
MacchiatoBin, BIOS EDK II Oct  2 2017
[12548.774210] pstate: 4045 (nZcv daif +PAN -UAO)
[12548.779033] pc : consume_skb+0x1c/0xd8
[12548.782802] lr : __dev_kfree_skb_any+0x58/0x68
[12548.787264] sp : 0801bc30
[12548.790594] x29: 0801bc30 x28: 831bed412a40
[12548.795934] x27: 831bf7ce8000 x26: 0001
[12548.801273] x25: 27e28d746120 x24: 831bed412948
[12548.806612] x23: 0018 x22: 27e28d746120
[12548.811950] x21: 0007 x20: 0001
[12548.817289] x19: 97ffd6fdd284 x18: 0010
[12548.822627] x17:  x16: 27e28d5bb4a0
[12548.827966] x15:  x14: 737365636f727020
[12548.833305] x13: 785420666f20646e x12: 6520656874206e6f
[12548.838643] x11: 27e28e07b448 x10: 27e28d35eb00
[12548.843981] x9 : 2074656e72656874 x8 : 0005
[12548.849319] x7 : b26f x6 : b66f
[12548.854658] x5 : 0001 x4 : 
[12548.859995] x3 : 0001 x2 : 97ffd6fdd284
[12548.865333] x1 : 0001 x0 : 27e28d5bb4f8
[12548.870673] Process swapper/3 (pid: 0, stack limit = 0x71feb006)
[12548.877404] Call trace:
[12548.879863]  consume_skb+0x1c/0xd8
[12548.883281]  __dev_kfree_skb_any+0x58/0x68
[12548.887411]  mvpp2_txq_bufs_free.isra.53+0xd0/0x118 [mvpp2]
[12548.893017]  mvpp2_txq_done.isra.68+0xb0/0xf8 [mvpp2]
[12548.898100]  mvpp2_tx_done+0xb4/0x118 [mvpp2]
[12548.902484]  mvpp2_poll+0x5c4/0x658 [mvpp2]
[12548.906688]  net_rx_action+0x160/0x3f8
[12548.910456]  __do_softirq+0x138/0x344
[12548.914137]  irq_exit+0xd0/0x100
[12548.917381]  __handle_domain_irq+0x6c/0xc0
[12548.921497]  gic_handle_irq+0x60/0xb0
[12548.925175]  el1_irq+0xd8/0x180
[12548.928331]  arch_cpu_idle+0x30/0x188
[12548.932011]  do_idle+0x138/0x1f8
[12548.935255]  cpu_startup_entry+0x2c/0x30
[12548.939197]  secondary_start_kernel+0x11c/0x130
[12548.943750] Code: aa0003f3 aa1e03e0 d503201f b4000153 (b940e660)
[12548.949876] ---[ end trace c9cfd11479961f0c ]---
[12548.954515] Kernel panic - not syncing: Fatal exception in interrupt
[12548.960900] SMP: stopping secondary CPUs
[12548.964845] Kernel Offset: 0x27e284d5 from 0x0800
[12548.970967] CPU features: 0x002000
[12548.974384] Memory Limit: none

Its interesting that the wrong CPU messages are still appearing despite the
irqbalance change from MarkZ. I disabled irqbalance and tried starting it in
single queue mode and it did the same thing.

Re: [PATCH net] amd-xgbe: Enable IRQs only if napi_complete_done() is true

[Jeremy Linton]

On 03/09/2017 05:48 PM, Tom Lendacky wrote:

Depending on the hardware, the amd-xgbe driver may use disable_irq_nosync()
and enable_irq() when an interrupt is received to process Rx packets. If
the napi_complete_done() return value isn't checked an unbalanced enable
for the IRQ could result, generating a warning stack trace.

Update the driver to only enable interrupts if napi_complete_done() returns
true.


I've been running this for a few hours now and haven't seen the warning. 
So it appears to be fixed. Thanks!


I guess Dave M picked it up already, but I will toss this in anyway.


Tested-by: Jeremy Linton <jeremy.lin...@arm.com>



Reported-by: Jeremy Linton <jeremy.lin...@arm.com>
Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c |   10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c 
b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 248f60d..ffea985 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -2272,10 +2272,7 @@ static int xgbe_one_poll(struct napi_struct *napi, int 
budget)
processed = xgbe_rx_poll(channel, budget);

/* If we processed everything, we are done */
-   if (processed < budget) {
-   /* Turn off polling */
-   napi_complete_done(napi, processed);
-
+   if ((processed < budget) && napi_complete_done(napi, processed)) {
/* Enable Tx and Rx interrupts */
if (pdata->channel_irq_mode)
xgbe_enable_rx_tx_int(pdata, channel);
@@ -2317,10 +2314,7 @@ static int xgbe_all_poll(struct napi_struct *napi, int 
budget)
} while ((processed < budget) && (processed != last_processed));

/* If we processed everything, we are done */
-   if (processed < budget) {
-   /* Turn off polling */
-   napi_complete_done(napi, processed);
-
+   if ((processed < budget) && napi_complete_done(napi, processed)) {
/* Enable Tx and Rx interrupts */
xgbe_enable_rx_tx_ints(pdata);
}

Re: xgbe unbalanced enable for IRQ XX in 4.11-rc1

[Jeremy Linton]

Hi,

On 03/09/2017 03:39 PM, Tom Lendacky wrote:

On 3/9/2017 3:26 PM, Jeremy Linton wrote:

Hi,


Hi Jeremy,

I'll have a look at it.  Can you send me your kernel config just in
case?


Sure, i will send it to you off list to avoid spamming everyone with a 
43k gziped file.


Thanks,



Thanks,
Tom



I have a softiron 3k and under network load (nfs copies, vnc with gnome,
etc) it is now throwing these messages as fast as the console will
accept them. This is booted DT mode.


[  430.111324] Unbalanced enable for IRQ 33
[  430.115239] [ cut here ]
[  430.119849] WARNING: CPU: 0 PID: 6 at kernel/irq/manage.c:529
__enable_irq+0x7c/0x88
[  430.127583] Modules linked in: fuse xt_CHECKSUM ipt_MASQUERADE
nf_nat_masquerade_ipv4 tun ip6t_rpfilter ip6t_REJECT nf_reject_ipv6
xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc
ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6
ip6table_mangle ip6table_raw ip6table_security iptable_nat
nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack
iptable_mangle iptable_raw iptable_security ebtable_filter ebtables
ip6table_filter ip6_tables vfat fat crc32_ce crct10dif_ce amd_xgbe
ghash_ce ptp pps_core spi_pl022 ipmi_si i2c_designware_platform
ipmi_devintf i2c_designware_core ccp ipmi_msghandler nfsd auth_rpcgss
nfs_acl lockd grace sunrpc xfs libcrc32c
[  430.188350]
[  430.189833] CPU: 0 PID: 6 Comm: ksoftirqd/0 Tainted: GW I
4.11.0-0.rc1.git0.1.fc27.aarch64 #1
[  430.199391] Hardware name: AMD Overdrive/Supercharger/Default string,
BIOS ROD1002C 04/08/2016
[  430.207994] task: 8003e4057900 task.stack: 8003f459
[  430.213904] PC is at __enable_irq+0x7c/0x88
[  430.218078] LR is at __enable_irq+0x7c/0x88
[  430.52] pc : [] lr : []
pstate: 01c5
[  430.229639] sp : 8003f4593c50
[  430.232944] x29: 8003f4593c50 x28: 8003dfeddce0
[  430.238248] x27: 0002 x26: 0001
[  430.243551] x25: 0040 x24: 8003f60a
[  430.248855] x23: 8003d5d0a900 x22: 0040
[  430.254158] x21: 0001 x20: 0021
[  430.259462] x19: 8003dc507200 x18: 
[  430.264765] x17:  x16: 
[  430.270069] x15: 0010 x14: 89005e7f
[  430.275372] x13: 09005e8d x12: 08e8d000
[  430.280676] x11: 08e65458 x10: 085adce8
[  430.285979] x9 : ffd0 x8 : 0005
[  430.291283] x7 : 636e616c61626e55 x6 : 8003fee74d98
[  430.296586] x5 : 8003fee74d98 x4 : 
[  430.301890] x3 : 8003fee88730 x2 : 8003fee74d98
[  430.307193] x1 : 8003f60a x0 : 001c
[  430.312496]
[  430.313978] ---[ end trace 5664787410723389 ]---
[  430.318586] Call trace:
[  430.321023] Exception stack(0x8003f4593a80 to 0x8003f4593bb0)
[  430.327454] 3a80: 8003dc507200 0001 8003f4593c50
08137b14
[  430.335277] 3aa0: 8003f4593c50 8003f4593c50 8003f4593c10
ffc8
[  430.343099] 3ac0: 8003f4593b00 08134838 08b7a570
8003f4593bd0
[  430.350921] 3ae0: 8003f4593c50 8003f4593c50 8003f4593c10
ffc8
[  430.358743] 3b00: 8003f4593bb0 081f8130 0001
0021
[  430.366566] 3b20: 001c 8003f60a 8003fee74d98
8003fee88730
[  430.374388] 3b40:  8003fee74d98 8003fee74d98
636e616c61626e55
[  430.382210] 3b60: 0005 ffd0 085adce8
08e65458
[  430.390033] 3b80: 08e8d000 09005e8d 89005e7f
0010
[  430.397854] 3ba0:  
[  430.402723] [] __enable_irq+0x7c/0x88
[  430.407939] [] enable_irq+0x40/0x78
[  430.412999] [] xgbe_one_poll+0xc0/0xe8 [amd_xgbe]
[  430.419257] [] net_rx_action+0x150/0x3c8
[  430.424734] [] __do_softirq+0x138/0x358
[  430.430123] [] run_ksoftirqd+0x4c/0x78
[  430.435426] [] smpboot_thread_fn+0x184/0x1b8
[  430.441249] [] kthread+0x12c/0x130
[  430.446205] [] ret_from_fork+0x10/0x20
[  430.451600] Unbalanced enable for IRQ 33
[  430.455516] [ cut here ]

xgbe unbalanced enable for IRQ XX in 4.11-rc1

[Jeremy Linton]

Hi,

I have a softiron 3k and under network load (nfs copies, vnc with gnome, 
etc) it is now throwing these messages as fast as the console will 
accept them. This is booted DT mode.



[  430.111324] Unbalanced enable for IRQ 33
[  430.115239] [ cut here ]
[  430.119849] WARNING: CPU: 0 PID: 6 at kernel/irq/manage.c:529 
__enable_irq+0x7c/0x88
[  430.127583] Modules linked in: fuse xt_CHECKSUM ipt_MASQUERADE 
nf_nat_masquerade_ipv4 tun ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 
xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc 
ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 
ip6table_mangle ip6table_raw ip6table_security iptable_nat 
nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack 
iptable_mangle iptable_raw iptable_security ebtable_filter ebtables 
ip6table_filter ip6_tables vfat fat crc32_ce crct10dif_ce amd_xgbe 
ghash_ce ptp pps_core spi_pl022 ipmi_si i2c_designware_platform 
ipmi_devintf i2c_designware_core ccp ipmi_msghandler nfsd auth_rpcgss 
nfs_acl lockd grace sunrpc xfs libcrc32c

[  430.188350]
[  430.189833] CPU: 0 PID: 6 Comm: ksoftirqd/0 Tainted: GW I 
4.11.0-0.rc1.git0.1.fc27.aarch64 #1
[  430.199391] Hardware name: AMD Overdrive/Supercharger/Default string, 
BIOS ROD1002C 04/08/2016

[  430.207994] task: 8003e4057900 task.stack: 8003f459
[  430.213904] PC is at __enable_irq+0x7c/0x88
[  430.218078] LR is at __enable_irq+0x7c/0x88
[  430.52] pc : [] lr : [] 
pstate: 01c5

[  430.229639] sp : 8003f4593c50
[  430.232944] x29: 8003f4593c50 x28: 8003dfeddce0
[  430.238248] x27: 0002 x26: 0001
[  430.243551] x25: 0040 x24: 8003f60a
[  430.248855] x23: 8003d5d0a900 x22: 0040
[  430.254158] x21: 0001 x20: 0021
[  430.259462] x19: 8003dc507200 x18: 
[  430.264765] x17:  x16: 
[  430.270069] x15: 0010 x14: 89005e7f
[  430.275372] x13: 09005e8d x12: 08e8d000
[  430.280676] x11: 08e65458 x10: 085adce8
[  430.285979] x9 : ffd0 x8 : 0005
[  430.291283] x7 : 636e616c61626e55 x6 : 8003fee74d98
[  430.296586] x5 : 8003fee74d98 x4 : 
[  430.301890] x3 : 8003fee88730 x2 : 8003fee74d98
[  430.307193] x1 : 8003f60a x0 : 001c
[  430.312496]
[  430.313978] ---[ end trace 5664787410723389 ]---
[  430.318586] Call trace:
[  430.321023] Exception stack(0x8003f4593a80 to 0x8003f4593bb0)
[  430.327454] 3a80: 8003dc507200 0001 8003f4593c50 
08137b14
[  430.335277] 3aa0: 8003f4593c50 8003f4593c50 8003f4593c10 
ffc8
[  430.343099] 3ac0: 8003f4593b00 08134838 08b7a570 
8003f4593bd0
[  430.350921] 3ae0: 8003f4593c50 8003f4593c50 8003f4593c10 
ffc8
[  430.358743] 3b00: 8003f4593bb0 081f8130 0001 
0021
[  430.366566] 3b20: 001c 8003f60a 8003fee74d98 
8003fee88730
[  430.374388] 3b40:  8003fee74d98 8003fee74d98 
636e616c61626e55
[  430.382210] 3b60: 0005 ffd0 085adce8 
08e65458
[  430.390033] 3b80: 08e8d000 09005e8d 89005e7f 
0010

[  430.397854] 3ba0:  
[  430.402723] [] __enable_irq+0x7c/0x88
[  430.407939] [] enable_irq+0x40/0x78
[  430.412999] [] xgbe_one_poll+0xc0/0xe8 [amd_xgbe]
[  430.419257] [] net_rx_action+0x150/0x3c8
[  430.424734] [] __do_softirq+0x138/0x358
[  430.430123] [] run_ksoftirqd+0x4c/0x78
[  430.435426] [] smpboot_thread_fn+0x184/0x1b8
[  430.441249] [] kthread+0x12c/0x130
[  430.446205] [] ret_from_fork+0x10/0x20
[  430.451600] Unbalanced enable for IRQ 33
[  430.455516] [ cut here ]

[PATCH] net: sky2: Fix shutdown crash

[Jeremy Linton]

The sky2 frequently crashes during machine shutdown with:

sky2_get_stats+0x60/0x3d8 [sky2]
dev_get_stats+0x68/0xd8
rtnl_fill_stats+0x54/0x140
rtnl_fill_ifinfo+0x46c/0xc68
rtmsg_ifinfo_build_skb+0x7c/0xf0
rtmsg_ifinfo.part.22+0x3c/0x70
rtmsg_ifinfo+0x50/0x5c
netdev_state_change+0x4c/0x58
linkwatch_do_dev+0x50/0x88
__linkwatch_run_queue+0x104/0x1a4
linkwatch_event+0x30/0x3c
process_one_work+0x140/0x3e0
worker_thread+0x60/0x44c
kthread+0xdc/0xf0
ret_from_fork+0x10/0x50

This is caused by the sky2 being called after it has been shutdown.
A previous thread about this can be found here:

https://lkml.org/lkml/2016/4/12/410

An alternative fix is to assure that IFF_UP gets cleared by
calling dev_close() during shutdown. This is similar to what the
bnx2/tg3/xgene and maybe others are doing to assure that the driver
isn't being called following _shutdown().

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/marvell/sky2.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/marvell/sky2.c 
b/drivers/net/ethernet/marvell/sky2.c
index f05ea56..941c8e2 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -5220,6 +5220,19 @@ static SIMPLE_DEV_PM_OPS(sky2_pm_ops, sky2_suspend, 
sky2_resume);
 
 static void sky2_shutdown(struct pci_dev *pdev)
 {
+   struct sky2_hw *hw = pci_get_drvdata(pdev);
+   int port;
+
+   for (port = 0; port < hw->ports; port++) {
+   struct net_device *ndev = hw->dev[port];
+
+   rtnl_lock();
+   if (netif_running(ndev)) {
+   dev_close(ndev);
+   netif_device_detach(ndev);
+   }
+   rtnl_unlock();
+   }
sky2_suspend(>dev);
pci_wake_from_d3(pdev, device_may_wakeup(>dev));
pci_set_power_state(pdev, PCI_D3hot);
-- 
2.5.5

Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend

[Jeremy Linton]

Hi,

On 10/27/2016 06:23 AM, Ulf Hansson wrote:

The smsc911c driver puts its device into low power state when entering
system suspend. Although it doesn't update the device's runtime PM status
to RPM_SUSPENDED, which causes problems for a parent device.

In particular, when the runtime PM status of the parent is requested to be
updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
forbidden to runtime suspend a device, which has an active child.

Fix this by updating the runtime PM status of the smsc911x device to
RPM_SUSPENDED during system suspend. In system resume, let's reverse that
action by runtime resuming the device and thus also the parent.

Signed-off-by: Ulf Hansson 
Tested-by: Geert Uytterhoeven 
Cc: Steve Glendinning 
Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active 
child")
---

Note that the commit this change fixes is currently queued for 4.10 via
Rafael's linux-pm tree. So this fix should go via that tree as well.

---
 drivers/net/ethernet/smsc/smsc911x.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index e9b8579..65fca9c 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev)
PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);

+   pm_runtime_disable(dev);
+   pm_runtime_set_suspended(dev);



+
return 0;
 }

@@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev)
struct smsc911x_data *pdata = netdev_priv(ndev);
unsigned int to = 100;

+   pm_runtime_enable(dev);
+   pm_runtime_resume(dev);
+
/* Note 3.11 from the datasheet:
 *  "When the LAN9220 is in a power saving state, a write of any
 *   data to the BYTE_TEST register will wake-up the device."



This seems an unusual change/sequence. I thought a successful return 
from the suspend callback would set the device state to suspended.


I just checked a few other ethernet drivers suspend/resume sequences and 
directly calling the pm_runtime seems a little unusual. Most of the 
other drivers are checking to see if the interface is running then doing 
a netif_device_detach()/attach() sequence which is missing from this 
drivers suspend/resume path. Could that be part of the problem?


Of course my knowledge of the power management system is a little thin 
so I could be really off base.

Re: [RFC] net: phy: smsc: Disable auto-negotiation on startup

[Jeremy Linton]

On 10/10/2016 12:41 PM, Kyle Roeschley wrote:

Because the SMSC PHY completes auto-negotiation before the driver is
ready to handle interrupts, the PHY state machine never realizes that we
have a link. Clear the ANENABLE bit on initialization, which lets
genphy_config_aneg do its thing when that code is hit later.

While this patch does fix the problem we see (no link on boot without
re-plugging the cable), it seems like the generic PHY code should be
able to handle auto-negotiation completing before interrupts are
enabled. Submitted as an RFC in the hopes that someone has an idea as to
how that could be done.


Hi,

	Which smsc chip/driver? Maybe assuring the device interrupts are 
enabled before the phy is started is a solution?


	The whole problem sounds similar to what was recently happening in the 
smsc911x driver, but AFAIK that driver is basically only polling at this 
point so connecting the phy before the interrupts are enabled shouldn't 
be a problem.







This fix is copied from commit 99f81afc139c ("phy: micrel: Disable auto
negotiation on startup").

Signed-off-by: Kyle Roeschley 
---
 drivers/net/phy/smsc.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b62c4aa..8de8011 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -62,6 +62,16 @@ static int smsc_phy_config_init(struct phy_device *phydev)
return rc;
}

+   if (phy_interrupt_is_valid(phydev)) {
+   rc = phy_read(phydev, MII_BMCR);
+   if (rc < 0)
+   return rc;
+
+   rc = phy_write(phydev, MII_BMCR, rc & ~BMCR_ANENABLE);
+   if (rc < 0)
+   return rc;
+   }
+
return smsc_phy_ack_interrupt(phydev);
 }

Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

[Jeremy Linton]

Hi Robert,

On 10/03/2016 04:05 AM, Robert Jarzmik wrote:

Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
which must be aligned on 32 bits addresses.

Signed-off-by: Robert Jarzmik 
---
 Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
 1 file changed, 2 insertions(+)


I think this might be the wrong doc file. I think you want the 
smsc-lan91c111.txt file.



Thanks,



diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt 
b/Documentation/devicetree/bindings/net/smsc911x.txt
index 3fed3c124411..224965b7453c 100644
--- a/Documentation/devicetree/bindings/net/smsc911x.txt
+++ b/Documentation/devicetree/bindings/net/smsc911x.txt
@@ -13,6 +13,8 @@ Optional properties:
 - reg-io-width : Specify the size (in bytes) of the IO accesses that
   should be performed on the device.  Valid value for SMSC LAN is
   2 or 4.  If it's omitted or invalid, the size would be 2.
+- reg-u16-align4 : Boolean, put in place the workaround the force all
+  u16 writes to be 32 bits aligned
 - smsc,irq-active-high : Indicates the IRQ polarity is active-high
 - smsc,irq-push-pull : Indicates the IRQ type is push-pull
 - smsc,force-internal-phy : Forces SMSC LAN controller to use

Re: [PATCH 2/2 v3] net: smsc911x: request and deassert optional RESET GPIO

[Jeremy Linton]

Hi,

On 09/07/2016 08:53 AM, Linus Walleij wrote:

On some systems (such as the Qualcomm APQ8060 Dragonboard) the
RESET signal of the SMSC911x is not pulled up by a resistor (or
the internal pull-up that will pull it up if the pin is not
even connected) but instead connected to a GPIO line, so that
the operating system must explicitly deassert RESET before use.

Support this in the SMSC911x driver so this ethernet connector
can be used on such targets.

Notice that we request the line to go logical low (deassert)
whilst the line on the actual component is active low. This
is managed in the respective hardware description when
specifying the GPIO line with e.g. device tree or ACPI. With
device tree it looks like this in one case:

  reset-gpios = < 30 GPIO_ACTIVE_LOW>;

Which means that logically requesting the RESET line to be
deasserted will result in the line being driven high, taking
the device out of reset.

Cc: Jeremy Linton <jeremy.lin...@arm.com>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>


Thanks for clarifying the GPIO_ACTIVE_LOW state on this pin. I've 
reviewed this patch, and tested that it doesn't have any affect on a 
JunoR2 (ACPI or DT) system. I've got some personal reservations about 
making changes for a single board that might better be handled in 
firmware. But, I don't think those should be an excuse to keep that 
hardware from working. Particularly since this is a fairly trivial change.


So FWIW,

Reviewed-by: Jeremy Linton <jeremy.lin...@arm.com>


Thanks,

[PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop

[Jeremy Linton]

The /proc/irq/xx information is incorrect for smsc911x because
the request_irq is happening before the register_netdev has the
proper device name. Moving it to the open also fixes the case
of when the device is renamed.

Reported-by: Will Deacon <will.dea...@arm.com>
Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 47 ++--
 1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index c2e56f0..4f8910b 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1575,6 +1575,7 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
int retval;
+   int irq_flags;
 
/* find and start the given phy */
if (!dev->phydev) {
@@ -1645,6 +1646,15 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();
 
+   irq_flags = irq_get_trigger_type(dev->irq);
+   retval = request_irq(dev->irq, smsc911x_irqhandler,
+irq_flags | IRQF_SHARED, dev->name, dev);
+   if (retval) {
+   SMSC_WARN(pdata, probe,
+ "Unable to claim requested irq: %d", dev->irq);
+   goto mii_free_out;
+   }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1660,7 +1670,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
-   goto mii_free_out;
+   goto irq_stop_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
   dev->irq);
@@ -1707,6 +1717,8 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
 
+irq_stop_out:
+   free_irq(dev->irq, dev);
 mii_free_out:
phy_disconnect(dev->phydev);
dev->phydev = NULL;
@@ -1733,6 +1745,8 @@ static int smsc911x_stop(struct net_device *dev)
dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
smsc911x_tx_update_txcounters(dev);
 
+   free_irq(dev->irq, dev);
+
/* Bring the PHY down */
if (dev->phydev) {
phy_stop(dev->phydev);
@@ -2308,7 +2322,6 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
mdiobus_free(pdata->mii_bus);
 
unregister_netdev(dev);
-   free_irq(dev->irq, dev);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
   "smsc911x-memory");
if (!res)
@@ -2393,8 +2406,7 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(>dev);
struct resource *res;
-   unsigned int intcfg = 0;
-   int res_size, irq, irq_flags;
+   int res_size, irq;
int retval;
 
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -2433,7 +2445,6 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
 
pdata = netdev_priv(dev);
dev->irq = irq;
-   irq_flags = irq_get_trigger_type(irq);
pdata->ioaddr = ioremap_nocache(res->start, res_size);
 
pdata->dev = dev;
@@ -2480,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
if (retval < 0)
goto out_disable_resources;
 
-   /* configure irq polarity and type before connecting isr */
-   if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-   intcfg |= INT_CFG_IRQ_POL_;
-
-   if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-   intcfg |= INT_CFG_IRQ_TYPE_;
-
-   smsc911x_reg_write(pdata, INT_CFG, intcfg);
-
-   /* Ensure interrupts are globally disabled before connecting ISR */
-   smsc911x_disable_irq_chip(dev);
-
-   retval = request_irq(dev->irq, smsc911x_irqhandler,
-irq_flags | IRQF_SHARED, dev->name, dev);
-   if (retval) {
-   SMSC_WARN(pdata, probe,
- "Unable to claim requested irq: %d", dev->irq);
-   goto out_disable_resources;
-   }
-
netif_carrier_off(dev);
 
retval = smsc911x_mii_init(pdev, dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
-   goto out_free_irq;
+   goto out_disable_resources;
}
 
retval = register_netdev(dev);
if (retval) {
S

[PATCH 1/4] net: smsc911x: Remove multiple exit points from smsc911x_open

[Jeremy Linton]

Rework the error handling in smsc911x open in preparation
for the mdio startup being moved here.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index ca31345..c9b0e05 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1520,17 +1520,20 @@ static int smsc911x_open(struct net_device *dev)
unsigned int timeout;
unsigned int temp;
unsigned int intcfg;
+   int retval;
 
/* if the phy is not yet registered, retry later*/
if (!dev->phydev) {
SMSC_WARN(pdata, hw, "phy_dev is NULL");
-   return -EAGAIN;
+   retval = -EAGAIN;
+   goto out;
}
 
/* Reset the LAN911x */
-   if (smsc911x_soft_reset(pdata)) {
+   retval = smsc911x_soft_reset(pdata);
+   if (retval) {
SMSC_WARN(pdata, hw, "soft reset failed");
-   return -EIO;
+   goto out;
}
 
smsc911x_reg_write(pdata, HW_CFG, 0x0005);
@@ -1600,7 +1603,8 @@ static int smsc911x_open(struct net_device *dev)
if (!pdata->software_irq_signal) {
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
-   return -ENODEV;
+   retval = -ENODEV;
+   goto out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
   dev->irq);
@@ -1646,6 +1650,8 @@ static int smsc911x_open(struct net_device *dev)
 
netif_start_queue(dev);
return 0;
+out:
+   return retval;
 }
 
 /* Entry point for stopping the interface */
-- 
2.5.5

[PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering

[Jeremy Linton]

Move phy startup/shutdown into the smsc911x_open/stop routines. This
allows the module to be unloaded because phy_connect_direct is no longer
always holding the module use count. This one change also resolves a
number of other problems.

The link status of a downed interface no longer reflects a stale state.
Errors caused by the net device being opened before the mdio/phy was
configured. There is also a potential power savings as the phy's don't
remain powered when the interface isn't running.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 48 ++--
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index c9b0e05..823ad3f 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1099,15 +1099,8 @@ static int smsc911x_mii_init(struct platform_device 
*pdev,
goto err_out_free_bus_2;
}
 
-   if (smsc911x_mii_probe(dev) < 0) {
-   SMSC_WARN(pdata, probe, "Error registering mii bus");
-   goto err_out_unregister_bus_3;
-   }
-
return 0;
 
-err_out_unregister_bus_3:
-   mdiobus_unregister(pdata->mii_bus);
 err_out_free_bus_2:
mdiobus_free(pdata->mii_bus);
 err_out_1:
@@ -1522,18 +1515,20 @@ static int smsc911x_open(struct net_device *dev)
unsigned int intcfg;
int retval;
 
-   /* if the phy is not yet registered, retry later*/
+   /* find and start the given phy */
if (!dev->phydev) {
-   SMSC_WARN(pdata, hw, "phy_dev is NULL");
-   retval = -EAGAIN;
-   goto out;
+   retval = smsc911x_mii_probe(dev);
+   if (retval < 0) {
+   SMSC_WARN(pdata, probe, "Error starting phy");
+   goto out;
+   }
}
 
/* Reset the LAN911x */
retval = smsc911x_soft_reset(pdata);
if (retval) {
SMSC_WARN(pdata, hw, "soft reset failed");
-   goto out;
+   goto mii_free_out;
}
 
smsc911x_reg_write(pdata, HW_CFG, 0x0005);
@@ -1604,7 +1599,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
-   goto out;
+   goto mii_free_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
   dev->irq);
@@ -1650,6 +1645,10 @@ static int smsc911x_open(struct net_device *dev)
 
netif_start_queue(dev);
return 0;
+
+mii_free_out:
+   phy_disconnect(dev->phydev);
+   dev->phydev = NULL;
 out:
return retval;
 }
@@ -1674,8 +1673,12 @@ static int smsc911x_stop(struct net_device *dev)
smsc911x_tx_update_txcounters(dev);
 
/* Bring the PHY down */
-   if (dev->phydev)
+   if (dev->phydev) {
phy_stop(dev->phydev);
+   phy_disconnect(dev->phydev);
+   dev->phydev = NULL;
+   }
+   netif_carrier_off(dev);
 
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2297,11 +2300,10 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
-   BUG_ON(!dev->phydev);
+   WARN_ON(dev->phydev);
 
SMSC_TRACE(pdata, ifdown, "Stopping driver");
 
-   phy_disconnect(dev->phydev);
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
 
@@ -2500,6 +2502,12 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
 
netif_carrier_off(dev);
 
+   retval = smsc911x_mii_init(pdev, dev);
+   if (retval) {
+   SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+   goto out_free_irq;
+   }
+
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering device", retval);
@@ -2509,12 +2517,6 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
   "Network interface: \"%s\"", dev->name);
}
 
-   retval = smsc911x_mii_init(pdev, dev);
-   if (retval) {
-   SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
-   goto out_unregister_netdev_5;
-   }
-
spin_lock_irq(>mac_lock);
 
/* Check if mac address has been specified when bringing interface up */
@@ -2550,8 +2552,6 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
 
return 0;
 
-out_unregister_netdev_5:
-   unregister_netdev(dev);
 out_free_irq:
free_irq(dev->irq, dev);
 out_disable_resources:
-- 
2.5.5

[PATCH v3 0/4] net: smsc911x: Move phy and interrupt config

[Jeremy Linton]

v2-v3: Move error handing into separate patch, replace a couple cases
 of fixed errors with the errors being returned from the failing functions.
 Hoist irq handler.

The smsc911x driver is doing a number of things in its probe routine that
should be delayed until the interface is started. Because of this, the module
cannot be unloaded, the phy states are incorrect/stale if the interface isn't
running, open's unnecessarily fail causing network configuration problems, and
the /proc/irq nodes are incorrectly named.

Clean up a number of these problems by moving the mdio and interrupt
configuration into the smsc911x_open routine.

Jeremy Linton (4):
  net: smsc911x: Remove multiple exit points from smsc911x_open
  net: smsc911x: Fix register_netdev, phy startup, driver unload
ordering
  net: smsc911x: Move interrupt handler before open
  net: smsc911x: Move interrupt allocation to open/stop

 drivers/net/ethernet/smsc/smsc911x.c | 213 +--
 1 file changed, 104 insertions(+), 109 deletions(-)

-- 
2.5.5

[PATCH 3/4] net: smsc911x: Move interrupt handler before open

[Jeremy Linton]

In preparation for the allocating/enabling interrupts
in the ndo_open routine move the irq handler before it.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 122 +--
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 823ad3f..c2e56f0 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1507,6 +1507,67 @@ static void smsc911x_disable_irq_chip(struct net_device 
*dev)
smsc911x_reg_write(pdata, INT_STS, 0x);
 }
 
+static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
+{
+   struct net_device *dev = dev_id;
+   struct smsc911x_data *pdata = netdev_priv(dev);
+   u32 intsts = smsc911x_reg_read(pdata, INT_STS);
+   u32 inten = smsc911x_reg_read(pdata, INT_EN);
+   int serviced = IRQ_NONE;
+   u32 temp;
+
+   if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
+   temp = smsc911x_reg_read(pdata, INT_EN);
+   temp &= (~INT_EN_SW_INT_EN_);
+   smsc911x_reg_write(pdata, INT_EN, temp);
+   smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
+   pdata->software_irq_signal = 1;
+   smp_wmb();
+   serviced = IRQ_HANDLED;
+   }
+
+   if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
+   /* Called when there is a multicast update scheduled and
+* it is now safe to complete the update */
+   SMSC_TRACE(pdata, intr, "RX Stop interrupt");
+   smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
+   if (pdata->multicast_update_pending)
+   smsc911x_rx_multicast_update_workaround(pdata);
+   serviced = IRQ_HANDLED;
+   }
+
+   if (intsts & inten & INT_STS_TDFA_) {
+   temp = smsc911x_reg_read(pdata, FIFO_INT);
+   temp |= FIFO_INT_TX_AVAIL_LEVEL_;
+   smsc911x_reg_write(pdata, FIFO_INT, temp);
+   smsc911x_reg_write(pdata, INT_STS, INT_STS_TDFA_);
+   netif_wake_queue(dev);
+   serviced = IRQ_HANDLED;
+   }
+
+   if (unlikely(intsts & inten & INT_STS_RXE_)) {
+   SMSC_TRACE(pdata, intr, "RX Error interrupt");
+   smsc911x_reg_write(pdata, INT_STS, INT_STS_RXE_);
+   serviced = IRQ_HANDLED;
+   }
+
+   if (likely(intsts & inten & INT_STS_RSFL_)) {
+   if (likely(napi_schedule_prep(>napi))) {
+   /* Disable Rx interrupts */
+   temp = smsc911x_reg_read(pdata, INT_EN);
+   temp &= (~INT_EN_RSFL_EN_);
+   smsc911x_reg_write(pdata, INT_EN, temp);
+   /* Schedule a NAPI poll */
+   __napi_schedule(>napi);
+   } else {
+   SMSC_WARN(pdata, rx_err, "napi_schedule_prep failed");
+   }
+   serviced = IRQ_HANDLED;
+   }
+
+   return serviced;
+}
+
 static int smsc911x_open(struct net_device *dev)
 {
struct smsc911x_data *pdata = netdev_priv(dev);
@@ -1820,67 +1881,6 @@ static void smsc911x_set_multicast_list(struct 
net_device *dev)
spin_unlock_irqrestore(>mac_lock, flags);
 }
 
-static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
-{
-   struct net_device *dev = dev_id;
-   struct smsc911x_data *pdata = netdev_priv(dev);
-   u32 intsts = smsc911x_reg_read(pdata, INT_STS);
-   u32 inten = smsc911x_reg_read(pdata, INT_EN);
-   int serviced = IRQ_NONE;
-   u32 temp;
-
-   if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
-   temp = smsc911x_reg_read(pdata, INT_EN);
-   temp &= (~INT_EN_SW_INT_EN_);
-   smsc911x_reg_write(pdata, INT_EN, temp);
-   smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
-   pdata->software_irq_signal = 1;
-   smp_wmb();
-   serviced = IRQ_HANDLED;
-   }
-
-   if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
-   /* Called when there is a multicast update scheduled and
-* it is now safe to complete the update */
-   SMSC_TRACE(pdata, intr, "RX Stop interrupt");
-   smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
-   if (pdata->multicast_update_pending)
-   smsc911x_rx_multicast_update_workaround(pdata);
-   serviced = IRQ_HANDLED;
-   }
-
-   if (intsts & inten & INT_STS_TDFA_) {
-   temp = smsc911x_reg_read(pdata, FIFO_INT);
-   temp |= FIFO_INT_TX_AVAIL_LEVEL_;
-   sms

Re: [PATCH 1/2] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering

[Jeremy Linton]

On 09/01/2016 11:58 AM, Andrew Lunn wrote:

@@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
unsigned int timeout;
unsigned int temp;
unsigned int intcfg;
+   int retval;

-   /* if the phy is not yet registered, retry later*/
+   /* find and start the given phy */
if (!dev->phydev) {
-   SMSC_WARN(pdata, hw, "phy_dev is NULL");
-   return -EAGAIN;
+   if (smsc911x_mii_probe(dev) < 0) {
+   SMSC_WARN(pdata, probe, "Error starting phy");
+   retval = -EAGAIN;


smsc911x_mii_probe() returns an error code. It is better to use that,
than -EAGAIN, which is rather odd to start with.


	Thats a good point, I was just maintaining the existing behavior, but 
its definitely better to just return the actual error.





+   goto out;
+   }
}

/* Reset the LAN911x */
if (smsc911x_soft_reset(pdata)) {
SMSC_WARN(pdata, hw, "soft reset failed");
-   return -EIO;
+   retval = -EIO;
+   goto mii_free_out;


smsc911x_soft_reset() also returns an error code you should use.

This patch also seems to do quite a few different things. Please can
you break it up into multiple patches.



	That was the comment from the previous patch, but It confused me 
because it was only really moving the MDIO startup, the rest was a side 
effect of that and atomic to it (AKA it wasn't clear how to make it 
smaller). I read it as Steve misinterpreting the laundry list of fixes 
as things being individually fixed, rather than one thing fixing a bunch 
of stuff.


This patch does add additional code I overlooked to cleanup the phy if 
it fails, I guess in theory that portion could be a prereq patch, I will 
break that portion out. I'm still not sure how to partially move the 
MDIO startup...

Re: [PATCH 2/2] net/smsc911x: Move interrupt allocation to open/stop

[Jeremy Linton]

Hi Andrew,

Thanks for taking a look at this!

On 09/01/2016 12:06 PM, Andrew Lunn wrote:

Hi Jeremy

Please don't add forward references. Move the function earlier in the
file.


Ok, but I thought it was a fairly large move due to further dependent 
functions..





 static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
 {
if (pdata->config.flags & SMSC911X_USE_32BIT)
@@ -1514,6 +1516,7 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
int retval;
+   int irq_flags;

/* find and start the given phy */
if (!dev->phydev) {
@@ -1584,6 +1587,14 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();

+   irq_flags = irq_get_trigger_type(dev->irq);
+   if (request_irq(dev->irq, smsc911x_irqhandler,
+   irq_flags | IRQF_SHARED, dev->name, dev)) {
+   SMSC_WARN(pdata, probe,
+ "Unable to claim requested irq: %d", dev->irq);
+   goto mii_free_out;
+   }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1599,7 +1610,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
-   goto mii_free_out;
+   goto irq_stop_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
   dev->irq);
@@ -1645,7 +1656,8 @@ static int smsc911x_open(struct net_device *dev)

netif_start_queue(dev);
return 0;
-
+irq_stop_out:
+   free_irq(dev->irq, dev);
 mii_free_out:
phy_disconnect(dev->phydev);
dev->phydev = NULL;
@@ -1672,12 +1684,15 @@ static int smsc911x_stop(struct net_device *dev)
dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
smsc911x_tx_update_txcounters(dev);

+   free_irq(dev->irq, dev);
+
/* Bring the PHY down */
if (dev->phydev) {
phy_stop(dev->phydev);
phy_disconnect(dev->phydev);
dev->phydev = NULL;
}
+   netif_carrier_off(dev);


What has this change got to do with interrupt handling?


This is a whoops, it should be in the previous patch..




@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
if (retval < 0)
goto out_disable_resources;

-   /* configure irq polarity and type before connecting isr */
-   if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-   intcfg |= INT_CFG_IRQ_POL_;
-
-   if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-   intcfg |= INT_CFG_IRQ_TYPE_;
-
-   smsc911x_reg_write(pdata, INT_CFG, intcfg);



I see these removes, but where are the adds?



The functionality is duplicated in open, when the IRQ handler is tested.

Re: [PATCH 2/2] net/smsc911x: Move interrupt allocation to open/stop

[Jeremy Linton]

Hi,

On 09/01/2016 02:45 PM, Andrew Lunn wrote:

On Thu, Sep 01, 2016 at 01:47:44PM -0500, Jeremy Linton wrote:

Hi Andrew,

Thanks for taking a look at this!

On 09/01/2016 12:06 PM, Andrew Lunn wrote:

Hi Jeremy

Please don't add forward references. Move the function earlier in the
file.


Ok, but I thought it was a fairly large move due to further
dependent functions..


There are a few other options, like moving smsc911x_open() rather than
the interrupt handler.

And i would suggest what ever you do, make it a separate patch. A
patch which says: No functional changes, just move functions around as
needed by later patches, is going to be quick and easy to review.


Yes I put it in another patch, I was busy blasting it out rather than 
checking my email...





+   netif_carrier_off(dev);


What has this change got to do with interrupt handling?


This is a whoops, it should be in the previous patch..


Or a patch of its own? You also needs to be careful with ordering
against the phy_connect.


@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
if (retval < 0)
goto out_disable_resources;

-   /* configure irq polarity and type before connecting isr */
-   if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-   intcfg |= INT_CFG_IRQ_POL_;
-
-   if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-   intcfg |= INT_CFG_IRQ_TYPE_;
-
-   smsc911x_reg_write(pdata, INT_CFG, intcfg);



I see these removes, but where are the adds?



The functionality is duplicated in open, when the IRQ handler is tested.


Ah, it is obfusticated by SMC_SET_IRQ_CFG().

If you say it is duplicated, how about a separate patch removing it,
with a clear pointer to where the duplicate is.


? Well, I didn't do that part, but I'm confused by your 
SMC_SET_IRQ_CFG(). AFAIK, that is smc911x not smsc911x. The code in 
question is in smsc911x_open() following "Set interrupt deassertion to 
100uS". It looks a little different but its reprogramming the INT_CFG 
preceding the interrupt being enabled.


Really, if I were feeling brave (because this driver is used in so many 
strange pieces of hardware) I would rewrite a large portion of the 
interrupt management in this driver. I started looking at it last month, 
while looking for the mdio polling issue, because I wanted to get the 
link state changes directly. While at it I noticed the WOL functionality 
could use some attention, etc, one problem after another.

[PATCH 1/2] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering

[Jeremy Linton]

Move phy startup/shutdown into the smsc911x_open/stop routines. This
allows the module to be unloaded because phy_connect_direct is no longer
always holding the module use count. This one change also resolves a
number of other problems.

The link status of a downed interface no longer reflects a stale state.
Errors caused by the net device being opened before the mdio/phy was
configured. There is also a potential power savings as the phy's don't
remain powered when the interface isn't running.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 51 
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index ca31345..6d6dcfe 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1099,15 +1099,8 @@ static int smsc911x_mii_init(struct platform_device 
*pdev,
goto err_out_free_bus_2;
}
 
-   if (smsc911x_mii_probe(dev) < 0) {
-   SMSC_WARN(pdata, probe, "Error registering mii bus");
-   goto err_out_unregister_bus_3;
-   }
-
return 0;
 
-err_out_unregister_bus_3:
-   mdiobus_unregister(pdata->mii_bus);
 err_out_free_bus_2:
mdiobus_free(pdata->mii_bus);
 err_out_1:
@@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
unsigned int timeout;
unsigned int temp;
unsigned int intcfg;
+   int retval;
 
-   /* if the phy is not yet registered, retry later*/
+   /* find and start the given phy */
if (!dev->phydev) {
-   SMSC_WARN(pdata, hw, "phy_dev is NULL");
-   return -EAGAIN;
+   if (smsc911x_mii_probe(dev) < 0) {
+   SMSC_WARN(pdata, probe, "Error starting phy");
+   retval = -EAGAIN;
+   goto out;
+   }
}
 
/* Reset the LAN911x */
if (smsc911x_soft_reset(pdata)) {
SMSC_WARN(pdata, hw, "soft reset failed");
-   return -EIO;
+   retval = -EIO;
+   goto mii_free_out;
}
 
smsc911x_reg_write(pdata, HW_CFG, 0x0005);
@@ -1600,7 +1598,8 @@ static int smsc911x_open(struct net_device *dev)
if (!pdata->software_irq_signal) {
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
-   return -ENODEV;
+   retval = -ENODEV;
+   goto mii_free_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
   dev->irq);
@@ -1646,6 +1645,12 @@ static int smsc911x_open(struct net_device *dev)
 
netif_start_queue(dev);
return 0;
+
+mii_free_out:
+   phy_disconnect(dev->phydev);
+   dev->phydev = NULL;
+out:
+   return retval;
 }
 
 /* Entry point for stopping the interface */
@@ -1668,8 +1673,11 @@ static int smsc911x_stop(struct net_device *dev)
smsc911x_tx_update_txcounters(dev);
 
/* Bring the PHY down */
-   if (dev->phydev)
+   if (dev->phydev) {
phy_stop(dev->phydev);
+   phy_disconnect(dev->phydev);
+   dev->phydev = NULL;
+   }
 
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2291,11 +2299,10 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
-   BUG_ON(!dev->phydev);
+   WARN_ON(dev->phydev);
 
SMSC_TRACE(pdata, ifdown, "Stopping driver");
 
-   phy_disconnect(dev->phydev);
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
 
@@ -2494,6 +2501,12 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
 
netif_carrier_off(dev);
 
+   retval = smsc911x_mii_init(pdev, dev);
+   if (retval) {
+   SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+   goto out_free_irq;
+   }
+
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering device", retval);
@@ -2503,12 +2516,6 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
   "Network interface: \"%s\"", dev->name);
}
 
-   retval = smsc911x_mii_init(pdev, dev);
-   if (retval) {
-   SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
-   goto out_unregister_netdev_5;
-   }
-
spin_lock_irq(>mac_lock);
 
/* Check if mac address has been specified when bringing interface up */
@@ 

[PATCH 0/2] net: smsc911x: Move phy and interrupt config

[Jeremy Linton]

Hi,

The smsc911x driver is doing a number of things in its probe routine that
should be delayed until the interface is started. Because of this, the module
cannot be unloaded, the phy states are incorrect/stale if the interface isn't
running, open's unnecessarily fail causing network configuration problems, and
the /proc/irq nodes are incorrectly named.

Clean up a number of these problems by moving the mdio and interrupt
configuration into the smsc911x_open routine.

Jeremy Linton (2):
  net: smsc911x: Fix register_netdev, phy startup, driver unload
ordering
  net/smsc911x: Move interrupt allocation to open/stop

 drivers/net/ethernet/smsc/smsc911x.c | 89 +---
 1 file changed, 42 insertions(+), 47 deletions(-)

-- 
2.5.5

[PATCH 2/2] net/smsc911x: Move interrupt allocation to open/stop

[Jeremy Linton]

The /proc/irq/xx information is incorrect for smsc911x because
the request_irq is happening before the register_netdev has the
proper device name. Moving it to the open also fixes the case
of when the device is renamed.

Reported-by: Will Deacon <will.dea...@arm.com>
Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 50 +++-
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 6d6dcfe..8ef9776 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -154,6 +154,8 @@ struct smsc911x_data {
 /* Easy access to information */
 #define __smsc_shift(pdata, reg) ((reg) << ((pdata)->config.shift))
 
+static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id);
+
 static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
 {
if (pdata->config.flags & SMSC911X_USE_32BIT)
@@ -1514,6 +1516,7 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
int retval;
+   int irq_flags;
 
/* find and start the given phy */
if (!dev->phydev) {
@@ -1584,6 +1587,14 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();
 
+   irq_flags = irq_get_trigger_type(dev->irq);
+   if (request_irq(dev->irq, smsc911x_irqhandler,
+   irq_flags | IRQF_SHARED, dev->name, dev)) {
+   SMSC_WARN(pdata, probe,
+ "Unable to claim requested irq: %d", dev->irq);
+   goto mii_free_out;
+   }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1599,7 +1610,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
-   goto mii_free_out;
+   goto irq_stop_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
   dev->irq);
@@ -1645,7 +1656,8 @@ static int smsc911x_open(struct net_device *dev)
 
netif_start_queue(dev);
return 0;
-
+irq_stop_out:
+   free_irq(dev->irq, dev);
 mii_free_out:
phy_disconnect(dev->phydev);
dev->phydev = NULL;
@@ -1672,12 +1684,15 @@ static int smsc911x_stop(struct net_device *dev)
dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
smsc911x_tx_update_txcounters(dev);
 
+   free_irq(dev->irq, dev);
+
/* Bring the PHY down */
if (dev->phydev) {
phy_stop(dev->phydev);
phy_disconnect(dev->phydev);
dev->phydev = NULL;
}
+   netif_carrier_off(dev);
 
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2307,7 +2322,6 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
mdiobus_free(pdata->mii_bus);
 
unregister_netdev(dev);
-   free_irq(dev->irq, dev);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
   "smsc911x-memory");
if (!res)
@@ -2392,8 +2406,7 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(>dev);
struct resource *res;
-   unsigned int intcfg = 0;
-   int res_size, irq, irq_flags;
+   int res_size, irq;
int retval;
 
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -2432,7 +2445,6 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
 
pdata = netdev_priv(dev);
dev->irq = irq;
-   irq_flags = irq_get_trigger_type(irq);
pdata->ioaddr = ioremap_nocache(res->start, res_size);
 
pdata->dev = dev;
@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
if (retval < 0)
goto out_disable_resources;
 
-   /* configure irq polarity and type before connecting isr */
-   if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
-   intcfg |= INT_CFG_IRQ_POL_;
-
-   if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
-   intcfg |= INT_CFG_IRQ_TYPE_;
-
-   smsc911x_reg_write(pdata, INT_CFG, intcfg);
-
-   /* Ensure interrupts are globally disabled before connecting ISR */
-   smsc911x_disable_irq_chip(dev);
-
-   retval = request_irq(dev->irq, smsc911x_irqhandler,
-irq_flags | IRQF_SHARED, dev->name, dev);
-   if (r

Re: [PATCH 3/3 v2] net: smsc911x: add wake-up event interrupt support

[Jeremy Linton]

Hi,

On 08/24/2016 07:59 AM, Linus Walleij wrote:

The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.

This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.

The patch is inspired by the wakeup code in the RTC
subsystem.

The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.



Having looked at a couple of the supported smsc chips, it seems they can 
route the wakeup through the chip's interrupt as well. If you add code 
to support this, it should work on a lot of the smsc911x devices rather 
than just the dragonboard.



Thanks,






Cc: Sudeep Holla 
Cc: Tony Lindgren 
Cc: Florian Fainelli 
Signed-off-by: Linus Walleij 
---
ChangeLog v1->v2:
- Call pm_wakeup_event() in the wakeup IRQ thread to
  account for the wakeup event.
- Drop the enable/disable_irq_wake() calls from suspend/resume:
  this is handled from the irq core when you call
  dev_pm_set_wake_irq() as we do.
---
 drivers/net/ethernet/smsc/smsc911x.c | 42 
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 8ab8d4b9614b..8fffc1dc2bdd 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -63,6 +63,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "smsc911x.h"

@@ -151,6 +152,9 @@ struct smsc911x_data {
/* Reset GPIO */
struct gpio_desc *reset_gpiod;

+   /* PME interrupt */
+   int pme_irq;
+
/* clock */
struct clk *clk;
 };
@@ -1881,6 +1885,19 @@ static irqreturn_t smsc911x_irqhandler(int irq, void 
*dev_id)
return serviced;
 }

+static irqreturn_t smsc911x_pme_irq_thread(int irq, void *dev_id)
+{
+   struct net_device *dev = dev_id;
+   struct smsc911x_data *pdata __maybe_unused = netdev_priv(dev);
+
+   SMSC_TRACE(pdata, pm, "wakeup event");
+   pm_wakeup_event(>dev, 50);
+   /* This signal is active for 50 ms, wait for it to deassert */
+   usleep_range(5, 10);
+
+   return IRQ_HANDLED;
+}
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void smsc911x_poll_controller(struct net_device *dev)
 {
@@ -2501,6 +2518,31 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
goto out_disable_resources;
}

+   irq = platform_get_irq(pdev, 1);
+   if (irq == -EPROBE_DEFER) {
+   retval = -EPROBE_DEFER;
+   goto out_disable_resources;
+   /* It's perfectly fine to not have a PME IRQ */
+   } else if (irq > 0) {
+   /*
+* The Power Management Event (PME) IRQ appears as
+* a pulse waking up the system from sleep in response to  a
+* network event.
+*/
+   retval = request_threaded_irq(irq, NULL,
+ smsc911x_pme_irq_thread,
+ IRQF_ONESHOT, "smsc911x-pme",
+ dev);
+   if (retval) {
+   SMSC_WARN(pdata, probe,
+   "Unable to claim requested PME irq: %d", irq);
+   goto out_disable_resources;
+   }
+   pdata->pme_irq = irq;
+   device_init_wakeup(>dev, true);
+   dev_pm_set_wake_irq(>dev, irq);
+   }
+
netif_carrier_off(dev);

retval = register_netdev(dev);

Re: [PATCH 3/3 v2] net: smsc911x: add wake-up event interrupt support

[Jeremy Linton]

Hi Linus,


On 08/24/2016 07:59 AM, Linus Walleij wrote:

The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.

This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.

The patch is inspired by the wakeup code in the RTC
subsystem.

The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.

Cc: Sudeep Holla 
Cc: Tony Lindgren 
Cc: Florian Fainelli 
Signed-off-by: Linus Walleij 
---
ChangeLog v1->v2:
- Call pm_wakeup_event() in the wakeup IRQ thread to
  account for the wakeup event.
- Drop the enable/disable_irq_wake() calls from suspend/resume:
  this is handled from the irq core when you call
  dev_pm_set_wake_irq() as we do.
---
 drivers/net/ethernet/smsc/smsc911x.c | 42 
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 8ab8d4b9614b..8fffc1dc2bdd 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -63,6 +63,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "smsc911x.h"

@@ -151,6 +152,9 @@ struct smsc911x_data {
/* Reset GPIO */
struct gpio_desc *reset_gpiod;

+   /* PME interrupt */
+   int pme_irq;
+
/* clock */
struct clk *clk;
 };
@@ -1881,6 +1885,19 @@ static irqreturn_t smsc911x_irqhandler(int irq, void 
*dev_id)
return serviced;
 }

+static irqreturn_t smsc911x_pme_irq_thread(int irq, void *dev_id)
+{
+   struct net_device *dev = dev_id;
+   struct smsc911x_data *pdata __maybe_unused = netdev_priv(dev);
+
+   SMSC_TRACE(pdata, pm, "wakeup event");
+   pm_wakeup_event(>dev, 50);
+   /* This signal is active for 50 ms, wait for it to deassert */
+   usleep_range(5, 10);
+
+   return IRQ_HANDLED;
+}
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void smsc911x_poll_controller(struct net_device *dev)
 {
@@ -2501,6 +2518,31 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
goto out_disable_resources;
}

+   irq = platform_get_irq(pdev, 1);
+   if (irq == -EPROBE_DEFER) {
+   retval = -EPROBE_DEFER;
+   goto out_disable_resources;
+   /* It's perfectly fine to not have a PME IRQ */
+   } else if (irq > 0) {
+   /*
+* The Power Management Event (PME) IRQ appears as
+* a pulse waking up the system from sleep in response to  a
+* network event.
+*/
+   retval = request_threaded_irq(irq, NULL,
+ smsc911x_pme_irq_thread,
+ IRQF_ONESHOT, "smsc911x-pme",
+ dev);
+   if (retval) {
+   SMSC_WARN(pdata, probe,
+   "Unable to claim requested PME irq: %d", irq);
+   goto out_disable_resources;
+   }
+   pdata->pme_irq = irq;
+   device_init_wakeup(>dev, true);
+   dev_pm_set_wake_irq(>dev, irq);
+   }
+
netif_carrier_off(dev);

retval = register_netdev(dev);




The cleanup code in drv_remove seems to be missing, am I missing something?

Also, do you want the wake-up to be active if the interface is downed?


Thanks,

Re: [PATCH 2/3 v2] net: smsc911x: request and deassert optional RESET GPIO

[Jeremy Linton]

Hi Linus,

On 08/24/2016 07:59 AM, Linus Walleij wrote:

On some systems (such as the Qualcomm APQ8060 Dragonboard) the
RESET signal of the SMSC911x is not pulled up by a resistor but
connected to a GPIO line, so that the operating system must
explicitly deassert RESET before use.

Support this in the SMSC911x driver so this ethernet connector
can be used on such targets.


Hmm, at least in our hardware case AFAIK (juno/lan9118) the hardware 
reset line on the lan9118 is active low, but the chip itself is 
documented as having internal pullups so that it may be left unconnected.


Which microchip/smsc chip are we talking about? because it seems that 
you probably want the GPIO pin to be in any state (hi-Z, or just high) 
besides the one you selected here.


Beyond that, is it not possible for the firmware to get the reset pin in 
the correct configuration, so that linux doesn't have to mess with it?






Signed-off-by: Linus Walleij 
---
ChangeLog v1->v2:
- Use devm_gpiod_request_optiona() and request the line with
  GPIOD_OUT_LOW so it is deasserted immediately if active.
---
 drivers/net/ethernet/smsc/smsc911x.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index ca3134540d2d..8ab8d4b9614b 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -62,6 +62,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "smsc911x.h"

@@ -147,6 +148,9 @@ struct smsc911x_data {
/* regulators */
struct regulator_bulk_data supplies[SMSC911X_NUM_SUPPLIES];

+   /* Reset GPIO */
+   struct gpio_desc *reset_gpiod;
+
/* clock */
struct clk *clk;
 };
@@ -438,6 +442,11 @@ static int smsc911x_request_resources(struct 
platform_device *pdev)
netdev_err(ndev, "couldn't get regulators %d\n",
ret);

+   /* Request optional RESET GPIO */
+   pdata->reset_gpiod = devm_gpiod_get_optional(>dev,
+"reset",
+GPIOD_OUT_LOW);
+
/* Request clock */
pdata->clk = clk_get(>dev, NULL);
if (IS_ERR(pdata->clk))

[PATCH v2] net: smc91x: ACPI Enable lan91x adapters

[Jeremy Linton]

Enable lan91x adapters in some ARM machines and models
when booted with an ACPI kernel.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smc91x.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc91x.c 
b/drivers/net/ethernet/smsc/smc91x.c
index 18ac52d..726b80f 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2195,6 +2195,12 @@ static void smc_release_datacs(struct platform_device 
*pdev, struct net_device *
}
 }
 
+static const struct acpi_device_id smc91x_acpi_match[] = {
+   { "LNRO0003", 0 },
+   { }
+};
+MODULE_DEVICE_TABLE(acpi, smc91x_acpi_match);
+
 #if IS_BUILTIN(CONFIG_OF)
 static const struct of_device_id smc91x_match[] = {
{ .compatible = "smsc,lan91c94", },
@@ -2274,7 +2280,6 @@ static int smc_drv_probe(struct platform_device *pdev)
 #if IS_BUILTIN(CONFIG_OF)
match = of_match_device(of_match_ptr(smc91x_match), >dev);
if (match) {
-   struct device_node *np = pdev->dev.of_node;
u32 val;
 
/* Optional pwrdwn GPIO configured? */
@@ -2300,7 +2305,8 @@ static int smc_drv_probe(struct platform_device *pdev)
usleep_range(750, 1000);
 
/* Combination of IO widths supported, default to 16-bit */
-   if (!of_property_read_u32(np, "reg-io-width", )) {
+   if (!device_property_read_u32(>dev, "reg-io-width",
+ )) {
if (val & 1)
lp->cfg.flags |= SMC91X_USE_8BIT;
if ((val == 0) || (val & 2))
@@ -2478,7 +2484,8 @@ static struct platform_driver smc_driver = {
.driver = {
.name   = CARDNAME,
.pm = _drv_pm_ops,
-   .of_match_table = of_match_ptr(smc91x_match),
+   .of_match_table   = of_match_ptr(smc91x_match),
+   .acpi_match_table = smc91x_acpi_match,
},
 };
 
-- 
2.5.5

Re: [RESEND/BUG PATCH v3] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0

[Jeremy Linton]

On 07/05/2016 01:45 PM, Sergei Shtylyov wrote:

The patch has been merged to 4.7-rc6, why resend it?


Sorry, I must have missed the merge.

Thanks,

[RESEND/BUG PATCH v3] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0

[Jeremy Linton]

By default, mdiobus_alloc() sets the PHYs to polling mode, but a
pointer size memcpy means that a couple IRQs end up being overwritten
with a value of 0. This means that PHY_POLL is disabled and results
in unpredictable behavior depending on the PHY's location on the
MDIO bus. Remove that memcpy and the now unused phy_irq member to
force the SMSC911x PHYs into polling mode 100% of the time.

Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core")

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
Reviewed-by: Andrew Lunn <and...@lunn.ch>
Acked-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..b5ab5e1 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -116,7 +116,6 @@ struct smsc911x_data {
 
struct phy_device *phy_dev;
struct mii_bus *mii_bus;
-   int phy_irq[PHY_MAX_ADDR];
unsigned int using_extphy;
int last_duplex;
int last_carrier;
@@ -1073,7 +1072,6 @@ static int smsc911x_mii_init(struct platform_device *pdev,
pdata->mii_bus->priv = pdata;
pdata->mii_bus->read = smsc911x_mii_read;
pdata->mii_bus->write = smsc911x_mii_write;
-   memcpy(pdata->mii_bus->irq, pdata->phy_irq, sizeof(pdata->mii_bus));
 
pdata->mii_bus->parent = >dev;
 
-- 
2.5.5

[PATCH] net: smc91x: ACPI Enable lan91x adapters

[Jeremy Linton]

Enable lan91x adapters in some ARM machines and models
when booted with an ACPI kernel.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smc91x.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smc91x.c 
b/drivers/net/ethernet/smsc/smc91x.c
index 18ac52d..fcf69f9 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2203,6 +2203,12 @@ static const struct of_device_id smc91x_match[] = {
 };
 MODULE_DEVICE_TABLE(of, smc91x_match);
 
+static const struct acpi_device_id smsc91x_acpi_match[] = {
+   { "LNRO0003", 0 },
+   { }
+};
+MODULE_DEVICE_TABLE(acpi, smsc91x_acpi_match);
+
 /**
  * of_try_set_control_gpio - configure a gpio if it exists
  */
@@ -2274,7 +2280,6 @@ static int smc_drv_probe(struct platform_device *pdev)
 #if IS_BUILTIN(CONFIG_OF)
match = of_match_device(of_match_ptr(smc91x_match), >dev);
if (match) {
-   struct device_node *np = pdev->dev.of_node;
u32 val;
 
/* Optional pwrdwn GPIO configured? */
@@ -2300,7 +2305,8 @@ static int smc_drv_probe(struct platform_device *pdev)
usleep_range(750, 1000);
 
/* Combination of IO widths supported, default to 16-bit */
-   if (!of_property_read_u32(np, "reg-io-width", )) {
+   if (!device_property_read_u32(>dev, "reg-io-width",
+ )) {
if (val & 1)
lp->cfg.flags |= SMC91X_USE_8BIT;
if ((val == 0) || (val & 2))
@@ -2479,6 +2485,7 @@ static struct platform_driver smc_driver = {
.name   = CARDNAME,
.pm = _drv_pm_ops,
.of_match_table = of_match_ptr(smc91x_match),
+   .acpi_match_table = smsc91x_acpi_match,
},
 };
 
-- 
2.5.5

[PATCH v3] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0

[Jeremy Linton]

By default, mdiobus_alloc() sets the PHYs to polling mode, but a
pointer size memcpy means that a couple IRQs end up being overwritten
with a value of 0. This means that PHY_POLL is disabled and results
in unpredictable behavior depending on the PHY's location on the
MDIO bus. Remove that memcpy and the now unused phy_irq member to
force the SMSC911x PHYs into polling mode 100% of the time.

Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core")

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
Reviewed-by: Andrew Lunn <and...@lunn.ch>
Acked-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..b5ab5e1 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -116,7 +116,6 @@ struct smsc911x_data {
 
struct phy_device *phy_dev;
struct mii_bus *mii_bus;
-   int phy_irq[PHY_MAX_ADDR];
unsigned int using_extphy;
int last_duplex;
int last_carrier;
@@ -1073,7 +1072,6 @@ static int smsc911x_mii_init(struct platform_device *pdev,
pdata->mii_bus->priv = pdata;
pdata->mii_bus->read = smsc911x_mii_read;
pdata->mii_bus->write = smsc911x_mii_write;
-   memcpy(pdata->mii_bus->irq, pdata->phy_irq, sizeof(pdata->mii_bus));
 
pdata->mii_bus->parent = >dev;
 
-- 
2.5.5

[PATCH 2/2] net: smsc911x: Fix register_netdev, phy startup ordering and driver unload

[Jeremy Linton]

Previously the mdio and phy's were started in the drv probe following
register_netdev(). This could lead to a situation where if the netdev was
opened before the mdio/phys were configured, it would fail with a
EAGAIN.

Also, the use of phy_connect_direct() in the drv_probe
routine results in a situation where the module use count would never
decrease sufficiently to unload the driver.

With this patch the mdio bus is allocated/configured before
register_netdev(), and the phy's are brought online/started in the
ndo_open and stopped in the ndo_stop. Because of this, the behavior
of ethtool changes a little if the interface is stopped. Before the
phy's would remain up, and their last state would be displayed with
ethtool. Now ethtool reports link has been severed/Link detected: no
when the net dev is stopped.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 48 +++-
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index b5ab5e1..abb1842 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1100,15 +1100,8 @@ static int smsc911x_mii_init(struct platform_device 
*pdev,
goto err_out_free_bus_2;
}
 
-   if (smsc911x_mii_probe(dev) < 0) {
-   SMSC_WARN(pdata, probe, "Error registering mii bus");
-   goto err_out_unregister_bus_3;
-   }
-
return 0;
 
-err_out_unregister_bus_3:
-   mdiobus_unregister(pdata->mii_bus);
 err_out_free_bus_2:
mdiobus_free(pdata->mii_bus);
 err_out_1:
@@ -1516,10 +1509,12 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
 
-   /* if the phy is not yet registered, retry later*/
+   /* find and start the given phy */
if (!pdata->phy_dev) {
-   SMSC_WARN(pdata, hw, "phy_dev is NULL");
-   return -EAGAIN;
+   if (smsc911x_mii_probe(dev) < 0) {
+   SMSC_WARN(pdata, probe, "Error starting phy");
+   return -EAGAIN;
+   }
}
 
/* Reset the LAN911x */
@@ -1663,8 +1658,11 @@ static int smsc911x_stop(struct net_device *dev)
smsc911x_tx_update_txcounters(dev);
 
/* Bring the PHY down */
-   if (pdata->phy_dev)
+   if (pdata->phy_dev) {
phy_stop(pdata->phy_dev);
+   phy_disconnect(pdata->phy_dev);
+   pdata->phy_dev = NULL;
+   }
 
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -1917,8 +1915,12 @@ smsc911x_ethtool_getsettings(struct net_device *dev, 
struct ethtool_cmd *cmd)
 {
struct smsc911x_data *pdata = netdev_priv(dev);
 
+   if (!netif_running(dev) || !pdata->phy_dev)
+   return -ENOLINK;
+
cmd->maxtxpkt = 1;
cmd->maxrxpkt = 1;
+
return phy_ethtool_gset(pdata->phy_dev, cmd);
 }
 
@@ -1927,6 +1929,9 @@ smsc911x_ethtool_setsettings(struct net_device *dev, 
struct ethtool_cmd *cmd)
 {
struct smsc911x_data *pdata = netdev_priv(dev);
 
+   if (!netif_running(dev) || !pdata->phy_dev)
+   return -ENOLINK;
+
return phy_ethtool_sset(pdata->phy_dev, cmd);
 }
 
@@ -1943,6 +1948,9 @@ static int smsc911x_ethtool_nwayreset(struct net_device 
*dev)
 {
struct smsc911x_data *pdata = netdev_priv(dev);
 
+   if (!netif_running(dev) || !pdata->phy_dev)
+   return -ENOLINK;
+
return phy_start_aneg(pdata->phy_dev);
 }
 
@@ -2308,12 +2316,10 @@ static int smsc911x_drv_remove(struct platform_device 
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
-   BUG_ON(!pdata->phy_dev);
+   WARN_ON(pdata->phy_dev);
 
SMSC_TRACE(pdata, ifdown, "Stopping driver");
 
-   phy_disconnect(pdata->phy_dev);
-   pdata->phy_dev = NULL;
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
 
@@ -2512,6 +2518,12 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
 
netif_carrier_off(dev);
 
+   retval = smsc911x_mii_init(pdev, dev);
+   if (retval) {
+   SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+   goto out_free_irq;
+   }
+
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering device", retval);
@@ -2521,12 +2533,6 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
   "Network interface: \"%s\"", dev->name);
}
 
-   retval = smsc911x_mii_init(pdev, dev);
-   if (retval) {
-   SMSC_WARN(pdata, probe

[PATCH 1/2] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0

[Jeremy Linton]

By default, mdiobus_alloc() sets the PHY's to polling mode, but a
pointer size memcpy means that couple IRQs (depending
on 32-bit or 64-bit kernels) end up being overwritten with
a value of 0. This means that PHY_POLL is disabled and results
in unpredictable behavior depending on the PHYs location on the mdio
bus. Remove that memcpy and the now unused phy_irq member to force
SMSC911x PHY's into polling mode 100% of the time.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..b5ab5e1 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -116,7 +116,6 @@ struct smsc911x_data {
 
struct phy_device *phy_dev;
struct mii_bus *mii_bus;
-   int phy_irq[PHY_MAX_ADDR];
unsigned int using_extphy;
int last_duplex;
int last_carrier;
@@ -1073,7 +1072,6 @@ static int smsc911x_mii_init(struct platform_device *pdev,
pdata->mii_bus->priv = pdata;
pdata->mii_bus->read = smsc911x_mii_read;
pdata->mii_bus->write = smsc911x_mii_write;
-   memcpy(pdata->mii_bus->irq, pdata->phy_irq, sizeof(pdata->mii_bus));
 
pdata->mii_bus->parent = >dev;
 
-- 
2.5.5

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 04:56 PM, Sergei Shtylyov wrote:

On 06/15/2016 12:53 AM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things
seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)

I'm using the device tree on my board.


Ok, I'm back on the machine, this is what mine says without that
patch.

SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver
[SMSC
LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0)


Hum, that's unexpected... things are probably more complex that I
thought. Do you have extra patches to this driver by changce?


No, the initial kernel where the problem was discovered is
4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the
driver
with the same effect.

Although, now that I'm looking closer at phy_irq, I'm curious how it
works for
anyone else...


Does anything change when you comment out that memcpy()? It
shouldn't probably...


Well that should change the irq to PHY_POLL by default rather than
the 0's
in the structure, which may be a better patch.


It shouldn't due to the wrong size. It should only overwrite IRQ and
index 0, unless I'm mistaken.


	Oh, sizeof(pointer)==8 on arm64, yah in the arm32 case you dodge the 
bullet.
	I think the memcpy removal solves the problem, i'm also going to test 
moving the mii_probe and will post an updated patch.


Thanks!

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 05:26 PM, Andrew Lunn wrote:

This was DT as well with a recent fedora/NetworkManager. It
actually seems to be timing related to how fast the device gets
configured after the initial phy probe. There is something like a 1
second window or so where it will work, but if network manager takes
longer than that, the link state drops and cannot be brought back up
unless the cable is pulled, replugged while the netdevice is being
restarted.


Ah!

There is another bug in the driver. The phy is connected to the netdev
after calling register_netdev(). You are supposed to do it before,
because the interface is usable, and can be used, directly after the
register.

Move the call to smsc911x_mii_init() before the register_netdev().


Yah, I buy that, and will move it an see what happens.

	But it doesn't solve the problem of the module use count being bumped 
in the probe rather than the ndo_open(). The users of 
phy_connect_direct() seem to be split between using it in the probe, and 
using it in the ndo_open (pxa168, and ax88796 for two examples of using 
it in the open).

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 04:42 PM, Sergei Shtylyov wrote:

On 06/15/2016 12:40 AM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things
seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)

I'm using the device tree on my board.


Ok, I'm back on the machine, this is what mine says without that patch.

SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver
[SMSC
LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0)


Hum, that's unexpected... things are probably more complex that I
thought. Do you have extra patches to this driver by changce?


No, the initial kernel where the problem was discovered is
4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the
driver
with the same effect.


Although, now that I'm looking closer at phy_irq, I'm curious how it
works for
anyone else...


Does anything change when you comment out that memcpy()? It
shouldn't probably...


	Well that should change the irq to PHY_POLL by default rather than the 
0's in the structure, which may be a better patch.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 04:34 PM, Sergei Shtylyov wrote:

On 06/15/2016 12:29 AM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)

I'm using the device tree on my board.


Ok, I'm back on the machine, this is what mine says without that patch.



SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver [SMSC
LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0)


Hum, that's unexpected... things are probably more complex that I
thought. Do you have extra patches to this driver by changce?


No, the initial kernel where the problem was discovered is
4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the 
driver with the same effect.



Although, now that I'm looking closer at phy_irq, I'm curious how it 
works for anyone else...

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:

On 06/14/2016 07:16 PM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)

I'm using the device tree on my board.


Ok, I'm back on the machine, this is what mine says without that patch.



SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver [SMSC 
LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0)

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:

On 06/14/2016 07:16 PM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


	AFAIK, I think that its set when the device is configured as a platform 
device, or there is an external phy/interrupt setup in DT. I might be 
wrong on that..







internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)

I'm using the device tree on my board.

MBR, Sergei

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:

On 06/14/2016 07:16 PM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)



BTW, the phy that is causing the problem is the one
labeled "SMSC LAN911x Internal PHY" in phy/smsc.c.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:

On 06/14/2016 07:16 PM, Jeremy Linton wrote:


If the interrupt configuration isn't set and we are using the


It's never set, judging by the driver code.


internal phy, then we need to poll the phy to reliably detect
phy state changes.


What address your internal PHY is at? Mine is at 1, and things seem
to work reliably after probing:

SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700]
(mii_bus:phy_addr=1800.etherne:01, irq=-1)


Looks like your phy ends up polling (-1==IRQ_POLL)...



I'm using the device tree on my board.


	This was DT as well with a recent fedora/NetworkManager. It actually 
seems to be timing related to how fast the device gets configured after 
the initial phy probe. There is something like a 1 second window or so 
where it will work, but if network manager takes longer than that, the 
link state drops and cannot be brought back up unless the cable is 
pulled, replugged while the netdevice is being restarted.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 01:42 PM, Andrew Lunn wrote:

On Tue, Jun 14, 2016 at 11:16:02AM -0500, Jeremy Linton wrote:

If the interrupt configuration isn't set and we are using the
internal phy, then we need to poll the phy to reliably detect
phy state changes.


Hi Jeremy
 Why does the external phy not have the exact same problem?


Hello,

It may... but I've only got a fairly limited hardware testing setup, and 
this driver is used across a pretty broad set of hardware configured in 
various ways. So, I'm trying to limit the scope of things I might break. 
Initially I thought it was added to avoid a QEMU issue, but now I don't 
think that is the case.


So, if 0 can't be an interrupt why not update phy_interrupt_is_valid() 
to check for it? That would fix the problem too...

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 02:49 PM, Sergei Shtylyov wrote:

On 06/14/2016 10:27 PM, Sergei Shtylyov wrote:


If the interrupt configuration isn't set and we are using the
internal phy, then we need to poll the phy to reliably detect
phy state changes.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..369dc7d 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct
net_device *dev)
 return -ENODEV;
 }

+if ((!phydev->irq) && (!pdata->using_extphy))


   Inner parens aren't needed at all.


   Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we
should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()...


And looking at that array, I doubt it's really useful for
anything... And the memcpy() there seems buggy as well -- it copies just
4 bytes of this array to 'pdata->mii_bus->irq'. I do care about this
driver, so might be a good idea to clean it up a bit...


	The use of phy_connect_direct() in the driver probe is incorrect, and 
keeps the driver from being unloaded.


	Also, some portion of smsc's can deliver mii state change interrupts 
via the smsc interrupt, but that code is no longer in this driver.


	I suspect a portion of the problem, besides all the strange hardware 
configurations this driver supports are the emulated hardware like QEMU 
that also uses it.

[PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

If the interrupt configuration isn't set and we are using the
internal phy, then we need to poll the phy to reliably detect
phy state changes.

Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..369dc7d 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
return -ENODEV;
}
 
+   if ((!phydev->irq) && (!pdata->using_extphy))
+   phydev->irq = PHY_POLL;
+
SMSC_TRACE(pdata, probe, "PHY: addr %d, phy_id 0x%08X",
   phydev->mdio.addr, phydev->phy_id);
 
-- 
2.5.5

Re: [PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Jeremy Linton]

On 09/09/2015 11:10 AM, Marc Zyngier wrote:

Jeremy,

I can see two issues here: we have a screaming interrupt, and
we seem to corrupt some workqueue.

How did you get this to work? Firmware release?


Marc,

I'm responding because its been a month or so since my last response, 
and I haven't forgotten about this issue.


First, any custom tianocore build (*) should work. The required changes 
have been in the last few linaro snapshots as well 
(http://snapshots.linaro.org/components/kernel/linaro-edk2/, currently 
at 40) but I personally haven't had a lot of luck with the prebuilt 
images due to problems unrelated to this change. Others may have more luck.


* For those that don't know, tianno core is at:

https://github.com/tianocore/edk2.git
Use the master branch

After setting the environment variables/dependencies appropriately:

make -f ArmPlatformPkg/ArmJunoPkg/Makefile all

Will create a functional ACPI firmware image for all recent kernels, 
including ACPI/PCIe ones.


Thanks for everyone's patience on this,






--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Jeremy Linton]

Marc,

|FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a
|horrible death:

Sorry about the delay, I didn't see this message.



|How did you get this to work? Firmware release?

Yes, it's a firmware problem with the ACPI tables. It is likely you need _DSD 
changes for the SMSC. If your trying to run juno with ACPI there are a few 
other ACPI table changes you will need beyond what was in the last linaro 
release. I wanted to just provide a direct link to the correct firmware in this 
response, but it seems the required version is sort of up in the air at the 
moment. Be assured I will really push on this next week, and will provide a 
follow up here.

Originally, the lack of DSD wasn't a problem, but as this patch evolved  the 
fact that it consolidates a number of configuration code paths balancing that 
got tricky, and ACPI machines with "bad" tables were the victims (rather than 
breaking something I can't control).

In the meantime, if this is blocking anyone I can provide instructions on how 
to update the required ACPI tables.

Thanks,



-- IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered 
in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, 
Registered in England & Wales, Company No:  2548782

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] device property: Don't overwrite addr when failing in device_get_mac_address

[Jeremy Linton]

On 09/03/2015 05:59 PM, Julien Grall wrote:

The function device_get_mac_address is trying different property names
in order to get the mac address. To check the return value, the variable
addr (which contain the buffer pass by the caller) will be re-used. This
means that if the previous property is not found, the next property will
be read using a NULL buffer.


Thanks for catching that! I checked the OF version to see if it has the 
same problem, but of course it doesn't because I added the logic to pass 
the buffer into the routine.


Reviewed-by: Jeremy Linton <jeremy.lin...@arm.com>



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next v3 1/2] device property: Return -ENXIO if there is no suitable FW interface

[Jeremy Linton]

On 08/26/2015 10:27 PM, Guenter Roeck wrote:

Return -ENXIO if device property array access functions don't find
a suitable firmware interface.

This lets drivers decide if they should use available platform data
instead.


Works fine on an ACPI based ARM system.

Thanks, for taking care of this.

Tested-by: Jeremy Linton jeremy.lin...@arm.com




--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] smsc911x: Ignore error return from device_get_phy_mode()

[Jeremy Linton]

On 08/26/2015 01:49 PM, Guenter Roeck wrote:

Check the return value from device_property_read_u32() to see if there
is a suitable firmware interface to read the data, and abort if not.
The function should return -ENXIO in that case; however, it returns
-ENODATA. Check for both.

Fixes: 62ee783bf1f8 (smsc911x: Fix crash seen if neither ACPI nor OF is configured 
or used)
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
Needs testing. RFC because I am not sure if the -ENODATA check is acceptable.


I'm not really sure about it myself. I can think of cases where it might 
cause problems. That said it does work in an ACPI environment with or 
without the _DSD block. If the DSD/property isn't set, obviously the 
device doesn't configure (but it doesn't crash either) so that is good 
and an overall improvement for ACPI.


Also, I personally might have hoisted the reg-io-width ahead of the 
device_get_phy_mode() and removed the phy checks, but I don't imagine 
there is much functional difference at this point.


Tested-by: Jeremy Linton jeremy.lin...@arm.com




--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Jeremy Linton]

On 08/26/2015 12:04 PM, Tony Lindgren wrote:

* Guenter Roeck li...@roeck-us.net [150817 13:48]:

Commit 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT) makes

Looks like this change makes at least omap boards using smsc911x
fail with -22 for me in Linux next.

Do any of the the device tree configured smsc911x devices actually
have a phy configured?


Tony,

	Looks like all the ones in the kernel boot/dts directory have a phy 
including the omap3-lilly except for the ste-snowball.dts.


Do you have smsc,force-internal-phy set instead?

Thanks,



--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Small cleanups for smsc and device property

[Jeremy Linton]

These patches are against net-next.

This patch set adds a length check to device_get_mac_addr() before
calling is_valid_ether_addr(), it also removes an unisssary dev==null
check.

The remainder is updates to the comments.

Jeremy Linton (2):
  device property: Add ETH_ALEN check, update comments.
  smsc911x: Remove dev==NULL check.

 drivers/base/property.c  | 21 +
 drivers/net/ethernet/smsc/smsc911x.c |  3 ---
 2 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] device property: Add ETH_ALEN check, update comments.

[Jeremy Linton]

This patch adds MAC address length check back into
the device_get_mac_addr() function before calling
is_valid_ether_addr() similar to the way the OF
routine does it.

Update the comments for the two new functions.

Signed-off-by: Jeremy Linton jeremy.lin...@arm.com
---
 drivers/base/property.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 2e8cd14..4c20828 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -537,7 +537,7 @@ bool device_dma_is_coherent(struct device *dev)
 EXPORT_SYMBOL_GPL(device_dma_is_coherent);
 
 /**
- * device_get_phy_mode - Get phy mode for given device_node
+ * device_get_phy_mode - Get phy mode for given device
  * @dev:   Pointer to the given device
  *
  * The function gets phy interface string from property 'phy-mode' or
@@ -570,13 +570,18 @@ static void *device_get_mac_addr(struct device *dev,
 {
int ret = device_property_read_u8_array(dev, name, addr, alen);
 
-   if (ret == 0  is_valid_ether_addr(addr))
+   if (ret == 0  alen == ETH_ALEN  is_valid_ether_addr(addr))
return addr;
return NULL;
 }
 
 /**
- * Search the device tree for the best MAC address to use.  'mac-address' is
+ * device_get_mac_address - Get the MAC for a given device
+ * @dev:   Pointer to the device
+ * @addr:  Address of buffer to store the MAC in
+ * @alen:  Length of the buffer pointed to by addr, should be ETH_ALEN
+ *
+ * Search the firmware node for the best MAC address to use.  'mac-address' is
  * checked first, because that is supposed to contain to most recent MAC
  * address. If that isn't set, then 'local-mac-address' is checked next,
  * because that is the default address.  If that isn't set, then the obsolete
@@ -587,11 +592,11 @@ static void *device_get_mac_addr(struct device *dev,
  * MAC address.
  *
  * All-zero MAC addresses are rejected, because those could be properties that
- * exist in the device tree, but were not set by U-Boot.  For example, the
- * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
- * addresses.  Some older U-Boots only initialized 'local-mac-address'.  In
- * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
- * but is all zeros.
+ * exist in the firmware tables, but were not updated by the firmware.  For
+ * example, the DTS could define 'mac-address' and 'local-mac-address', with
+ * zero MAC addresses.  Some older U-Boots only initialized 
'local-mac-address'.
+ * In this case, the real MAC is in 'local-mac-address', and 'mac-address'
+ * exists but is all zeros.
 */
 void *device_get_mac_address(struct device *dev, char *addr, int alen)
 {
-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] smsc911x: Remove dev==NULL check.

[Jeremy Linton]

The dev==NULL check in smsc911x_probe_config is useless
and isn't providing any additional protection. If a fwnode
doesn't exist then an appropriate error should be returned
by device_get_phy_mode() covering the original case
of a missing of/fwnode.

Signed-off-by: Jeremy Linton jeremy.lin...@arm.com
---
 drivers/net/ethernet/smsc/smsc911x.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 34f9768..6eef325 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2370,9 +2370,6 @@ static int smsc911x_probe_config(struct 
smsc911x_platform_config *config,
int phy_interface;
u32 width = 0;
 
-   if (!dev)
-   return -ENODEV;
-
phy_interface = device_get_phy_mode(dev);
if (phy_interface  0)
return phy_interface;
-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH] xgene mac/phy cleanup

[Jeremy Linton]

This patch converts the xgene driver to use some common ACPI/DT agnostic
functions for retrieving MAC and PHY settings. I don't have a way to test
them at the moment, so if someone can verify they work that would be great.

BTW: These patches are against net-next.

Thanks,

Jeremy Linton (1):
  net: xgene Remove xgene specific phy and MAC lookup functions

 drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 38 ++--
 1 file changed, 2 insertions(+), 36 deletions(-)

-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] net: xgene Remove xgene specific phy and MAC lookup functions

[Jeremy Linton]

Convert the xgene_get_mac_address to device_get_mac_address(), and
xgene_get_phy_mode() to device_get_phy_mode().

Signed-off-by: Jeremy Linton jeremy.lin...@arm.com
---
 drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 38 ++--
 1 file changed, 2 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c 
b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 299eb43..4f68d19 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -905,40 +905,6 @@ static int xgene_get_port_id_dt(struct device *dev, struct 
xgene_enet_pdata *pda
return ret;
 }
 
-static int xgene_get_mac_address(struct device *dev,
-unsigned char *addr)
-{
-   int ret;
-
-   ret = device_property_read_u8_array(dev, local-mac-address, addr, 6);
-   if (ret)
-   ret = device_property_read_u8_array(dev, mac-address,
-   addr, 6);
-   if (ret)
-   return -ENODEV;
-
-   return ETH_ALEN;
-}
-
-static int xgene_get_phy_mode(struct device *dev)
-{
-   int i, ret;
-   char *modestr;
-
-   ret = device_property_read_string(dev, phy-connection-type,
- (const char **)modestr);
-   if (ret)
-   ret = device_property_read_string(dev, phy-mode,
- (const char **)modestr);
-   if (ret)
-   return -ENODEV;
-
-   for (i = 0; i  PHY_INTERFACE_MODE_MAX; i++) {
-   if (!strcasecmp(modestr, phy_modes(i)))
-   return i;
-   }
-   return -ENODEV;
-}
 
 static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
 {
@@ -998,12 +964,12 @@ static int xgene_enet_get_resources(struct 
xgene_enet_pdata *pdata)
if (ret)
return ret;
 
-   if (xgene_get_mac_address(dev, ndev-dev_addr) != ETH_ALEN)
+   if (!device_get_mac_address(dev, ndev-dev_addr, ETH_ALEN))
eth_hw_addr_random(ndev);
 
memcpy(ndev-perm_addr, ndev-dev_addr, ndev-addr_len);
 
-   pdata-phy_mode = xgene_get_phy_mode(dev);
+   pdata-phy_mode = device_get_phy_mode(dev);
if (pdata-phy_mode  0) {
dev_err(dev, Unable to get phy-connection-type\n);
return pdata-phy_mode;
-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Jeremy Linton]

On 08/17/2015 05:14 PM, Guenter Roeck wrote:

One nitpick I noticed after sending my patch: dev can never be NULL in
smsc911x_probe_config(), so it does not really make sense to check if it is 
NULL.


No, it doesn't... it should really be something like

if (dev_fwnode(dev))

But dev_fwnode is static inline in property.c, and i'm pretty sure that 
still isn't 100% correct.The best plan might just be to remove the 
check, and abort on failure to find the phy property per your patch.






--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Jeremy Linton]

On 08/17/2015 03:45 PM, Guenter Roeck wrote:

Commit 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT) makes
the call to smsc911x_probe_config() unconditional, and no longer fails if
there is no device node. device_get_phy_mode() is called unconditionally,
and if there is no phy node configured returns an error code. This error
code is assigned to phy_interface, and interpreted elsewhere in the code
as valid phy mode. This in turn causes qemu to crash when running a
variant of realview_pb_defconfig.


Thanks for catching that! I have a couple other minor cleanups per the 
reviewers.



Jeremy

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Jeremy Linton]

On 08/12/2015 05:13 PM, Florian Fainelli wrote:

On 12/08/15 15:06, Jeremy Linton wrote:

+
+static void *device_get_mac_addr(struct device *dev,
+const char *name, char *addr,
+int alen)
+{
+   int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+   if (ret == 0  is_valid_ether_addr(addr))
+   return addr;
+   return NULL;


The DT counterpart has an additional check on the properly length to be
ETH_ALEN, you might want to have such check here for consistency and
correctness.


I will add it back,

Thanks,



--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Jeremy Linton]

Hello Robin,

On 08/13/2015 06:57 AM, Robin Murphy wrote:

+static void *device_get_mac_addr(struct device *dev,
+const char *name, char *addr,
+int alen)
+{
+   int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+   if (ret == 0  is_valid_ether_addr(addr))
+   return addr;
+   return NULL;
+}


Not sure I understand the logic here - return the same thing we were
given if we updated it, or null if we didn't. It's only indicating
success/failure (the caller can perfectly well cast its own buffer to a
void * if it needs to), so why wouldn't you just return a normal int
error code?



	No particular reason, other than initially I was trying to keep the 
function as similar as possible to the one in of_net. AKA copy paste 
job. I can convert the return types, but I was trying for a simple 
function rename. That way the users of the of version could be converted 
with relative ease, and the drivers which invented their own version of 
these functions could be changed to use this instead. Of course, that 
plan took a blow, when I added the addr/alen parameters.


Same thing applies for the other function.




--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Jeremy Linton]

Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.

Signed-off-by: Jeremy Linton jeremy.lin...@arm.com
---
 drivers/net/ethernet/smsc/smsc911x.c | 48 +---
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 959aeea..0f21aa3 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -59,7 +59,9 @@
 #include linux/of_device.h
 #include linux/of_gpio.h
 #include linux/of_net.h
+#include linux/acpi.h
 #include linux/pm_runtime.h
+#include linux/property.h
 
 #include smsc911x.h
 
@@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops = 
{
.tx_writefifo = smsc911x_tx_writefifo_shift,
 };
 
-#ifdef CONFIG_OF
-static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config,
-   struct device_node *np)
+static int smsc911x_probe_config(struct smsc911x_platform_config *config,
+struct device *dev)
 {
-   const char *mac;
u32 width = 0;
 
-   if (!np)
+   if (!dev)
return -ENODEV;
 
-   config-phy_interface = of_get_phy_mode(np);
+   config-phy_interface = device_get_phy_mode(dev);
 
-   mac = of_get_mac_address(np);
-   if (mac)
-   memcpy(config-mac, mac, ETH_ALEN);
+   device_get_mac_address(dev, config-mac, ETH_ALEN);
 
-   of_property_read_u32(np, reg-shift, config-shift);
+   device_property_read_u32(dev, reg-shift, config-shift);
 
-   of_property_read_u32(np, reg-io-width, width);
+   device_property_read_u32(dev, reg-io-width, width);
if (width == 4)
config-flags |= SMSC911X_USE_32BIT;
else
config-flags |= SMSC911X_USE_16BIT;
 
-   if (of_get_property(np, smsc,irq-active-high, NULL))
+   if (device_property_present(dev, smsc,irq-active-high))
config-irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH;
 
-   if (of_get_property(np, smsc,irq-push-pull, NULL))
+   if (device_property_present(dev, smsc,irq-push-pull))
config-irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL;
 
-   if (of_get_property(np, smsc,force-internal-phy, NULL))
+   if (device_property_present(dev, smsc,force-internal-phy))
config-flags |= SMSC911X_FORCE_INTERNAL_PHY;
 
-   if (of_get_property(np, smsc,force-external-phy, NULL))
+   if (device_property_present(dev, smsc,force-external-phy))
config-flags |= SMSC911X_FORCE_EXTERNAL_PHY;
 
-   if (of_get_property(np, smsc,save-mac-address, NULL))
+   if (device_property_present(dev, smsc,save-mac-address))
config-flags |= SMSC911X_SAVE_MAC_ADDRESS;
 
return 0;
 }
-#else
-static inline int smsc911x_probe_config_dt(
-   struct smsc911x_platform_config *config,
-   struct device_node *np)
-{
-   return -ENODEV;
-}
-#endif /* CONFIG_OF */
 
 static int smsc911x_drv_probe(struct platform_device *pdev)
 {
-   struct device_node *np = pdev-dev.of_node;
struct net_device *dev;
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(pdev-dev);
@@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
goto out_disable_resources;
}
 
-   retval = smsc911x_probe_config_dt(pdata-config, np);
+   retval = smsc911x_probe_config(pdata-config, pdev-dev);
if (retval  config) {
/* copy config parameters across to pdata */
memcpy(pdata-config, config, sizeof(pdata-config));
@@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, smsc911x_dt_ids);
 #endif
 
+static const struct acpi_device_id smsc911x_acpi_match[] = {
+   { ARMH9118, 0 },
+   { }
+};
+MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match);
+
 static struct platform_driver smsc911x_driver = {
.probe = smsc911x_drv_probe,
.remove = smsc911x_drv_remove,
@@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = {
.name   = SMSC_CHIPNAME,
.pm = SMSC911X_PM_OPS,
.of_match_table = of_match_ptr(smsc911x_dt_ids),
+   .acpi_match_table = ACPI_PTR(smsc911x_acpi_match),
},
 };
 
-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Enable smsc911x for use with ACPI

[Jeremy Linton]

This set of patches enables the front Ethernet port on the
ARM Juno development platform when used with an ACPI enabled kernel.

These patches covert the of_property* calls in the driver to the
DT/ACPI agnostic device_property* calls, and add the arm hardware
id to the acpi_match_table.

To support the above changes I copied a couple routines from
of_net into the properties.c file, and modified them to
be ACPI/DT agnostic. I'm not 100% sure this is the correct location
for these functions. But I think they are required to avoid having
a dozen different implementations scattered across assorted Ethernet
adapters that are being enabled to use ACPI properties.

Jeremy Linton (2):
  Add a matching set of device_ functions for determining mac/phy
  Convert smsc911x to use ACPI as well as DT

 drivers/base/property.c  | 73 
 drivers/net/ethernet/smsc/smsc911x.c | 48 +++-
 include/linux/property.h |  4 ++
 3 files changed, 99 insertions(+), 26 deletions(-)

-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Jeremy Linton]

OF has some helper functions for parsing MAC and PHY settings.
In cases where the platform is providing this information rather
than the device itself, there needs to be similar functions for ACPI.

These functions are slightly modified versions of the ones in
of_net which can use information provided via DT or ACPI.

Signed-off-by: Jeremy Linton jeremy.lin...@arm.com
---
 drivers/base/property.c  | 73 
 include/linux/property.h |  4 +++
 2 files changed, 77 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index f3f6d16..2e8cd14 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -16,6 +16,8 @@
 #include linux/of.h
 #include linux/of_address.h
 #include linux/property.h
+#include linux/etherdevice.h
+#include linux/phy.h
 
 /**
  * device_add_property_set - Add a collection of properties to a device object.
@@ -533,3 +535,74 @@ bool device_dma_is_coherent(struct device *dev)
return coherent;
 }
 EXPORT_SYMBOL_GPL(device_dma_is_coherent);
+
+/**
+ * device_get_phy_mode - Get phy mode for given device_node
+ * @dev:   Pointer to the given device
+ *
+ * The function gets phy interface string from property 'phy-mode' or
+ * 'phy-connection-type', and return its index in phy_modes table, or errno in
+ * error case.
+ */
+int device_get_phy_mode(struct device *dev)
+{
+   const char *pm;
+   int err, i;
+
+   err = device_property_read_string(dev, phy-mode, pm);
+   if (err  0)
+   err = device_property_read_string(dev,
+ phy-connection-type, pm);
+   if (err  0)
+   return err;
+
+   for (i = 0; i  PHY_INTERFACE_MODE_MAX; i++)
+   if (!strcasecmp(pm, phy_modes(i)))
+   return i;
+
+   return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(device_get_phy_mode);
+
+static void *device_get_mac_addr(struct device *dev,
+const char *name, char *addr,
+int alen)
+{
+   int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+   if (ret == 0  is_valid_ether_addr(addr))
+   return addr;
+   return NULL;
+}
+
+/**
+ * Search the device tree for the best MAC address to use.  'mac-address' is
+ * checked first, because that is supposed to contain to most recent MAC
+ * address. If that isn't set, then 'local-mac-address' is checked next,
+ * because that is the default address.  If that isn't set, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree.
+ *
+ * Note that the 'address' property is supposed to contain a virtual address of
+ * the register set, but some DTS files have redefined that property to be the
+ * MAC address.
+ *
+ * All-zero MAC addresses are rejected, because those could be properties that
+ * exist in the device tree, but were not set by U-Boot.  For example, the
+ * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
+ * addresses.  Some older U-Boots only initialized 'local-mac-address'.  In
+ * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
+ * but is all zeros.
+*/
+void *device_get_mac_address(struct device *dev, char *addr, int alen)
+{
+   addr = device_get_mac_addr(dev, mac-address, addr, alen);
+   if (addr)
+   return addr;
+
+   addr = device_get_mac_addr(dev, local-mac-address, addr, alen);
+   if (addr)
+   return addr;
+
+   return device_get_mac_addr(dev, address, addr, alen);
+}
+EXPORT_SYMBOL(device_get_mac_address);
diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9..a59c6ee 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -166,4 +166,8 @@ void device_add_property_set(struct device *dev, struct 
property_set *pset);
 
 bool device_dma_is_coherent(struct device *dev);
 
+int device_get_phy_mode(struct device *dev);
+
+void *device_get_mac_address(struct device *dev, char *addr, int alen);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.4.3


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html