net: dsa: Use delayed work instead of timer+work for polling

PATCH Superseded Neil Armstrong 3 participants 8 messages Oct 29, 2015 – Oct 30, 2015

Subsystems (3)

Networking Network drivers Networking DSA

lore

Share & tools

lore

Neil Armstrong

author

2015-10-29 13:22 UTC #0 lore raw

PATCH v2 1/6 net: dsa: Use delayed work instead of timer+work for polling

Simplifies the code and avoids a crash when removing the module:
dsa dsa ethmv2 (unregistering): Link is Down device eth1 left promiscuous mode

 Unable to handle kernel paging request at virtual address bacc5cf6
 ...
 (run_timer_softirq) from [<c003e810>] (__do_softirq+0xcc/0x320)
 (__do_softirq) from [<c003ed40>] (irq_exit+0xac/0x10c)
 (irq_exit) from [<c007ec20>] (__handle_domain_irq+0x50/0xa8)

Signed-off-by Frode Isaksen <[email protected]>

Signed-off-by Neil Armstrong <[email protected]>

---
 include/net/dsa.h |  3 +--
 net/dsa/dsa.c     | 28 ++++++++--------------------
 2 files changed, 9 insertions(+), 22 deletions(-)

include/net/dsa.h +1 -2

index 98ccbde..d4d13f7 100644
@@ -112,8 +112,7 @@ struct dsa_switch_tree {
 	 * Link state polling.
 	 */
 	int			link_poll_needed;
-	struct work_struct	link_poll_work;
-	struct timer_list	link_poll_timer;
+	struct delayed_work	link_poll_work;
 
 	/*
 	 * Data for the individual switch chips.

net/dsa/dsa.c +8 ~~-20~~

index 1eba07f..aeb6a7c 100644
@@ -513,7 +513,7 @@ static void dsa_link_poll_work(struct work_struct *ugly)
 	struct dsa_switch_tree *dst;
 	int i;
 
-	dst = container_of(ugly, struct dsa_switch_tree, link_poll_work);
+	dst = container_of(ugly, struct dsa_switch_tree, link_poll_work.work);
 
 	for (i = 0; i < dst->pd->nr_chips; i++) {
 		struct dsa_switch *ds = dst->ds[i];

@@ -522,17 +522,9 @@ static void dsa_link_poll_work(struct work_struct *ugly)
 			ds->drv->poll_link(ds);
 	}
 
-	mod_timer(&dst->link_poll_timer, round_jiffies(jiffies + HZ));
+	schedule_delayed_work(&dst->link_poll_work, round_jiffies_relative(HZ));
 }
 
-static void dsa_link_poll_timer(unsigned long _dst)
-{
-	struct dsa_switch_tree *dst = (void *)_dst;
-
-	schedule_work(&dst->link_poll_work);
-}
-
-
 /* platform driver init and cleanup *****************************************/
 static int dev_is_class(struct device *dev, void *class)
 {

@@ -880,12 +872,8 @@ static int dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
 	dev->dsa_ptr = (void *)dst;
 
 	if (dst->link_poll_needed) {
-		INIT_WORK(&dst->link_poll_work, dsa_link_poll_work);
-		init_timer(&dst->link_poll_timer);
-		dst->link_poll_timer.data = (unsigned long)dst;
-		dst->link_poll_timer.function = dsa_link_poll_timer;
-		dst->link_poll_timer.expires = round_jiffies(jiffies + HZ);
-		add_timer(&dst->link_poll_timer);
+		INIT_DELAYED_WORK(&dst->link_poll_work, dsa_link_poll_work);
+		schedule_delayed_work(&dst->link_poll_work, round_jiffies_relative(HZ));
 	}
 
 	return 0;

@@ -954,10 +942,10 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 {
 	int i;
 
-	if (dst->link_poll_needed)
-		del_timer_sync(&dst->link_poll_timer);
-
-	flush_work(&dst->link_poll_work);
+	if (dst->link_poll_needed) {
+		cancel_delayed_work_sync(&dst->link_poll_work);
+		flush_delayed_work(&dst->link_poll_work);
+	}
 
 	for (i = 0; i < dst->pd->nr_chips; i++) {
 		struct dsa_switch *ds = dst->ds[i];

Signature · 1 line

1.9.1

Andrew Lunn

maintainer

2015-10-29 13:52 UTC #1 lore raw

Quoted 45 lines · Neil Armstrong

Simplifies the code and avoids a crash when removing the module:
dsa dsa ethmv2 (unregistering): Link is Down device eth1 left promiscuous mode

 Unable to handle kernel paging request at virtual address bacc5cf6
 ...
 (run_timer_softirq) from [<c003e810>] (__do_softirq+0xcc/0x320)
 (__do_softirq) from [<c003ed40>] (irq_exit+0xac/0x10c)
 (irq_exit) from [<c007ec20>] (__handle_domain_irq+0x50/0xa8)

Signed-off-by Frode Isaksen <[email protected]>

Signed-off-by Neil Armstrong <[email protected]>

---
 include/net/dsa.h |  3 +--
 net/dsa/dsa.c     | 28 ++++++++--------------------
 2 files changed, 9 insertions(+), 22 deletions(-)

include/net/dsa.h +1 -2

index 98ccbde..d4d13f7 100644
@@ -112,8 +112,7 @@ struct dsa_switch_tree {
 	 * Link state polling.
 	 */
 	int			link_poll_needed;
-	struct work_struct	link_poll_work;
-	struct timer_list	link_poll_timer;
+	struct delayed_work	link_poll_work;
 
 	/*
 	 * Data for the individual switch chips.

net/dsa/dsa.c +1 -1

index 1eba07f..aeb6a7c 100644
@@ -513,7 +513,7 @@ static void dsa_link_poll_work(struct work_struct *ugly)
 	struct dsa_switch_tree *dst;
 	int i;
 
-	dst = container_of(ugly, struct dsa_switch_tree, link_poll_work);
+	dst = container_of(ugly, struct dsa_switch_tree, link_poll_work.work);
 
 	for (i = 0; i < dst->pd->nr_chips; i++) {
 		struct dsa_switch *ds = dst->ds[i];

net/dsa/dsa.c static void dsa_link_poll_work(struct work_struct *ugly) · line 522

Quoted 1 line

net/dsa/dsa.c +0 -0

@@ -522,17 +522,9 @@ static void dsa_link_poll_work(struct work_struct *ugly)
 			ds->drv->poll_link(ds);

Hi Neil, Frode

I assume you have see:

http://permalink.gmane.org/gmane.linux.network/380777

which is now in net-next.

The only driver making use of poll_link is mv88e6060.c. The argument from removing it from the other mv88e6xxx drivers is that it is not needed. The phylib will be polling the phys and looking for state changes. mv88e6060.c implements phy_read() and phy_write() so it should also allow the use of phylib.

Could you try 6060 without .poll_link. If that works, we should be able to remove all this code, rather than fix it up.

Andrew

Neil Armstrong

author

2015-10-29 14:10 UTC #2 lore raw

Quoted 15 lines · Andrew Lunn

On Thu, Oct 29, 2015 at 02:22:41PM +0100, Neil Armstrong wrote:
Hi Neil, Frode
I assume you have see:
http://permalink.gmane.org/gmane.linux.network/380777
which is now in net-next.
The only driver making use of poll_link is mv88e6060.c. The argument from removing it from the other mv88e6xxx drivers is that it is not needed. The phylib will be polling the phys and looking for state changes. mv88e6060.c implements phy_read() and phy_write() so it should also allow the use of phylib.

Quoted 5 lines

Could you try 6060 without .poll_link. If that works, we should be able to remove all this code, rather than fix it up.
Andrew

Hi Andrew,

Thanks for the hint, I will test this, but while reviewing the datasheet, the port 5 has no PHY, so must only be used as cpu port.

I also have another patchset which fixes 2 setup registers writes and adds a shiny new mv88e6060.h based on mv88e6xxx.h.
I think the removal of the poll_link in mv88e6060 should be in this patchset.

Neil

Andrew Lunn

maintainer

2015-10-29 14:40 UTC #3 lore raw

Quoted 2 lines

Thanks for the hint, I will test this, but while reviewing the datasheet, the port 5 has no PHY, so must only be used as cpu port.

Not necessarily. All that is needed is an external phy. Linux does not care what MDIO bus that phy hangs off. It could be the same MDIO bus as the switch, or some other MDIO bus. It is likely that port 5 is the CPU port, but it does not have to be.

Quoted 2 lines

I also have another patchset which fixes 2 setup registers writes and adds a shiny new mv88e6060.h based on mv88e6xxx.h.

Great. I hope that will help determine if we can merge 6060 into 6xxx.

Andrew

Vivien Didelot

2015-10-29 15:02 UTC #4 lore raw

Quoted 7 lines · Andrew Lunn

Quoted 2 lines
Thanks for the hint, I will test this, but while reviewing the datasheet, the port 5 has no PHY, so must only be used as cpu port.
Not necessarily. All that is needed is an external phy. Linux does not care what MDIO bus that phy hangs off. It could be the same MDIO bus as the switch, or some other MDIO bus. It is likely that port 5 is the CPU port, but it does not have to be.

Indeed in my setup with a single 88E6352, we will soon add an external PHY there (or on port 6, doesn't matter) to make it a 6-port switch.

Quoted 4 lines

Quoted 2 lines
I also have another patchset which fixes 2 setup registers writes and adds a shiny new mv88e6060.h based on mv88e6xxx.h.
Great. I hope that will help determine if we can merge 6060 into 6xxx.

Is 6060 supported by the 6352 family? It doesn't share any mv88e6xxx code...

Thanks,
-v

Andrew Lunn

maintainer

2015-10-29 15:28 UTC #5 lore raw

Quoted 2 lines

Is 6060 supported by the 6352 family? It doesn't share any mv88e6xxx code...

It is a totally separate driver. Nothing shared at all.. It is an old device, and only does 10/100. It could be it is just too different to support via mv88e6xxx.

Andrew

Neil Armstrong

author

2015-10-30 15:44 UTC #6 lore raw

Quoted 9 lines · Andrew Lunn

Quoted 2 lines
Is 6060 supported by the 6352 family? It doesn't share any mv88e6xxx code...
It is a totally separate driver. Nothing shared at all.. It is an old device, and only does 10/100. It could be it is just too different to support via mv88e6xxx.
Andrew

Andrew,

The link detection still works without the polling for 6060, this will largely simplify the patchset.

I will include a link poll callback removal in my 6060 cleanup patchset.

Then I will include a poll_link removal in dsa.c instead of fixing it.

Neil

Andrew Lunn

maintainer

2015-10-30 16:30 UTC #7 lore raw

Quoted 11 lines · Neil Armstrong

Quoted 9 lines · Andrew Lunn
Quoted 2 lines
Is 6060 supported by the 6352 family? It doesn't share any mv88e6xxx code...
It is a totally separate driver. Nothing shared at all.. It is an old device, and only does 10/100. It could be it is just too different to support via mv88e6xxx.
Andrew

Quoted 7 lines

Andrew,
The link detection still works without the polling for 6060, this will largely simplify the patchset.
I will include a link poll callback removal in my 6060 cleanup patchset.
Then I will include a poll_link removal in dsa.c instead of fixing it.

Great, always nice to see unneeded code removed.

Andrew