This patch set comprises some one-liners to fix issues with repeated
loading and unloading of a modular mv643xx_eth driver.
First two patches take care of the periodic port statistic timer, that
updates statistics by reading port registers using add_timer/mod_timer.
Patch 1 moves timer re-schedule from mib_counters_update to the timer
callback. As mib_counters_update is also called from non-timer context,
this ensures the timer is reactivated from timer context only.
Patch 2 moves initial timer schedule from _probe() time to right before
the port is actually started as the corresponding del_timer_sync is at
_stop() time. This fixes a regression, where unloading the driver from a
non-started eth device can cause the timer to access deallocated mem.
Patch 3 adds an assignment of the ports device_node to the corresponding
self-created platform_device. This is required to allow fixups based on
the device_node's compatible string later. Actually, it is also a potential
regression because we already check compatible string for Kirkwood, but
does not (yet) rely on the fixup.
All patches are based on v3.12-rc3 and have been tested on Kirkwood-based
Seagate Dockstar.
Patches 1 and 2 can also possibly queued up for -stable.
Sebastian Hesselbarth (3):
net: mv643xx_eth: update statistics timer from timer context only
net: mv643xx_eth: fix orphaned statistics timer crash
net: mv643xx_eth: fix missing device_node for port devices
drivers/net/ethernet/marvell/mv643xx_eth.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
---
Cc: David Miller <[email protected]>
Cc: Lennert Buytenhek <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
--
1.7.10.4
DT-based mv643xx_eth probes and creates platform_devices for the
port devices on its own. To allow fixups for ports based on the
device_node, we need to set .of_node of the corresponding device
with the correct node.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Cc: David Miller <[email protected]>
Cc: Lennert Buytenhek <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/net/ethernet/marvell/mv643xx_eth.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 2364707..2c210ec 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2533,6 +2533,7 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
if (!ppdev)
return -ENOMEM;
ppdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+ ppdev->dev.of_node = pnp;
ret = platform_device_add_resources(ppdev, &res, 1);
if (ret)
--
1.7.10.4
The periodic statistics timer gets started at port _probe() time, but
is stopped on _stop() only. In a modular environment, this can cause
the timer to access already deallocated memory, if the module is unloaded
without starting the eth device. To fix this, we add the timer right
before the port is started, instead of at _probe() time.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Cc: David Miller <[email protected]>
Cc: Lennert Buytenhek <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/net/ethernet/marvell/mv643xx_eth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 44a87e4..2364707 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2235,6 +2235,7 @@ static int mv643xx_eth_open(struct net_device *dev)
mp->int_mask |= INT_TX_END_0 << i;
}
+ add_timer(&mp->mib_counters_timer);
port_start(mp);
wrlp(mp, INT_MASK_EXT, INT_EXT_LINK_PHY | INT_EXT_TX);
@@ -2914,7 +2915,6 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
mp->mib_counters_timer.data = (unsigned long)mp;
mp->mib_counters_timer.function = mib_counters_timer_wrapper;
mp->mib_counters_timer.expires = jiffies + 30 * HZ;
- add_timer(&mp->mib_counters_timer);
spin_lock_init(&mp->mib_counters_lock);
--
1.7.10.4
Each port driver installs a periodic timer to update port statistics
by calling mib_counters_update. As mib_counters_update is also called
from non-timer context, we should not reschedule the timer there but
rather move it to timer-only context.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Cc: David Miller <[email protected]>
Cc: Lennert Buytenhek <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/net/ethernet/marvell/mv643xx_eth.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 7fb5677..44a87e4 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1131,15 +1131,13 @@ static void mib_counters_update(struct mv643xx_eth_private *mp)
p->rx_discard += rdlp(mp, RX_DISCARD_FRAME_CNT);
p->rx_overrun += rdlp(mp, RX_OVERRUN_FRAME_CNT);
spin_unlock_bh(&mp->mib_counters_lock);
-
- mod_timer(&mp->mib_counters_timer, jiffies + 30 * HZ);
}
static void mib_counters_timer_wrapper(unsigned long _mp)
{
struct mv643xx_eth_private *mp = (void *)_mp;
-
mib_counters_update(mp);
+ mod_timer(&mp->mib_counters_timer, jiffies + 30 * HZ);
}
--
1.7.10.4
On Wed, Oct 02, 2013 at 12:57:20PM +0200, Sebastian Hesselbarth wrote:
> Each port driver installs a periodic timer to update port statistics
> by calling mib_counters_update. As mib_counters_update is also called
> from non-timer context, we should not reschedule the timer there but
> rather move it to timer-only context.
>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> Cc: David Miller <[email protected]>
> Cc: Lennert Buytenhek <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/ethernet/marvell/mv643xx_eth.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
Acked-by: Jason Cooper <[email protected]>
Introduced by:
4ff3495a mv643xx_eth: enforce frequent hardware statistics polling
which goes all the way back to v2.6.28
thx,
Jason.
On Wed, Oct 02, 2013 at 12:57:21PM +0200, Sebastian Hesselbarth wrote:
> The periodic statistics timer gets started at port _probe() time, but
> is stopped on _stop() only. In a modular environment, this can cause
> the timer to access already deallocated memory, if the module is unloaded
> without starting the eth device. To fix this, we add the timer right
> before the port is started, instead of at _probe() time.
>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> Cc: David Miller <[email protected]>
> Cc: Lennert Buytenhek <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/ethernet/marvell/mv643xx_eth.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Jason Cooper <[email protected]>
Introduced by:
4ff3495a mv643xx_eth: enforce frequent hardware statistics polling
which also goes all the way back to v2.6.28
thx,
Jason.
On Wed, Oct 02, 2013 at 12:57:22PM +0200, Sebastian Hesselbarth wrote:
> DT-based mv643xx_eth probes and creates platform_devices for the
> port devices on its own. To allow fixups for ports based on the
> device_node, we need to set .of_node of the corresponding device
> with the correct node.
>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> Cc: David Miller <[email protected]>
> Cc: Lennert Buytenhek <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/ethernet/marvell/mv643xx_eth.c | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Jason Cooper <[email protected]>
thx,
Jason.
From: Sebastian Hesselbarth <[email protected]>
Date: Wed, 2 Oct 2013 12:57:19 +0200
> This patch set comprises some one-liners to fix issues with repeated
> loading and unloading of a modular mv643xx_eth driver.
>
> First two patches take care of the periodic port statistic timer, that
> updates statistics by reading port registers using add_timer/mod_timer.
>
> Patch 1 moves timer re-schedule from mib_counters_update to the timer
> callback. As mib_counters_update is also called from non-timer context,
> this ensures the timer is reactivated from timer context only.
>
> Patch 2 moves initial timer schedule from _probe() time to right before
> the port is actually started as the corresponding del_timer_sync is at
> _stop() time. This fixes a regression, where unloading the driver from a
> non-started eth device can cause the timer to access deallocated mem.
>
> Patch 3 adds an assignment of the ports device_node to the corresponding
> self-created platform_device. This is required to allow fixups based on
> the device_node's compatible string later. Actually, it is also a potential
> regression because we already check compatible string for Kirkwood, but
> does not (yet) rely on the fixup.
>
> All patches are based on v3.12-rc3 and have been tested on Kirkwood-based
> Seagate Dockstar.
>
> Patches 1 and 2 can also possibly queued up for -stable.
Series applied, patch #1 and #2 queued up for -stable, thanks!
On Wed, Oct 02, 2013 at 12:57:19PM +0200, Sebastian Hesselbarth wrote:
> This patch set comprises some one-liners to fix issues with repeated
> loading and unloading of a modular mv643xx_eth driver.
>
> First two patches take care of the periodic port statistic timer, that
> updates statistics by reading port registers using add_timer/mod_timer.
>
> Patch 1 moves timer re-schedule from mib_counters_update to the timer
> callback. As mib_counters_update is also called from non-timer context,
> this ensures the timer is reactivated from timer context only.
>
> Patch 2 moves initial timer schedule from _probe() time to right before
> the port is actually started as the corresponding del_timer_sync is at
> _stop() time. This fixes a regression, where unloading the driver from a
> non-started eth device can cause the timer to access deallocated mem.
>
> Patch 3 adds an assignment of the ports device_node to the corresponding
> self-created platform_device. This is required to allow fixups based on
> the device_node's compatible string later. Actually, it is also a potential
> regression because we already check compatible string for Kirkwood, but
> does not (yet) rely on the fixup.
>
> All patches are based on v3.12-rc3 and have been tested on Kirkwood-based
> Seagate Dockstar.
>
> Patches 1 and 2 can also possibly queued up for -stable.
>
> Sebastian Hesselbarth (3):
> net: mv643xx_eth: update statistics timer from timer context only
> net: mv643xx_eth: fix orphaned statistics timer crash
> net: mv643xx_eth: fix missing device_node for port devices
>
> drivers/net/ethernet/marvell/mv643xx_eth.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> ---
> Cc: David Miller <[email protected]>
> Cc: Lennert Buytenhek <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Sorry for missing this series, somehow my filters failed to catch it.
Anyway, I would expect these patches to have my Reported-by, but I'm
glad to see you've solved all the issues.
I see David has already applied them all, but I'll re-run my tests to
check there aren't any more issues if I can find some time next week.
Thanks for taking care of it!
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
On 10/04/2013 06:53 PM, Ezequiel Garcia wrote:
> On Wed, Oct 02, 2013 at 12:57:19PM +0200, Sebastian Hesselbarth wrote:
>> This patch set comprises some one-liners to fix issues with repeated
>> loading and unloading of a modular mv643xx_eth driver.
>>
>> First two patches take care of the periodic port statistic timer, that
>> updates statistics by reading port registers using add_timer/mod_timer.
>>
>> Patch 1 moves timer re-schedule from mib_counters_update to the timer
>> callback. As mib_counters_update is also called from non-timer context,
>> this ensures the timer is reactivated from timer context only.
>>
>> Patch 2 moves initial timer schedule from _probe() time to right before
>> the port is actually started as the corresponding del_timer_sync is at
>> _stop() time. This fixes a regression, where unloading the driver from a
>> non-started eth device can cause the timer to access deallocated mem.
>>
>> Patch 3 adds an assignment of the ports device_node to the corresponding
>> self-created platform_device. This is required to allow fixups based on
>> the device_node's compatible string later. Actually, it is also a potential
>> regression because we already check compatible string for Kirkwood, but
>> does not (yet) rely on the fixup.
>>
>> All patches are based on v3.12-rc3 and have been tested on Kirkwood-based
>> Seagate Dockstar.
>>
>> Patches 1 and 2 can also possibly queued up for -stable.
>>
>> Sebastian Hesselbarth (3):
>> net: mv643xx_eth: update statistics timer from timer context only
>> net: mv643xx_eth: fix orphaned statistics timer crash
>> net: mv643xx_eth: fix missing device_node for port devices
>>
>> drivers/net/ethernet/marvell/mv643xx_eth.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> ---
>> Cc: David Miller <[email protected]>
>> Cc: Lennert Buytenhek <[email protected]>
>> Cc: Jason Cooper <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>
> Sorry for missing this series, somehow my filters failed to catch it.
>
> Anyway, I would expect these patches to have my Reported-by, but I'm
> glad to see you've solved all the issues.
True, thanks to Ezequiel for initially reporting the issues. I will
have to be more careful about the Reported-by next time.
> I see David has already applied them all, but I'll re-run my tests to
> check there aren't any more issues if I can find some time next week.
>
> Thanks for taking care of it!