| Message ID | 56321DA1.4060304@baylibre.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Oct 29, 2015 at 02:22:41PM +0100, Neil Armstrong wrote: > 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 <fisaksen@baylibre.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > include/net/dsa.h | 3 +-- > net/dsa/dsa.c | 28 ++++++++-------------------- > 2 files changed, 9 insertions(+), 22 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 98ccbde..d4d13f7 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -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. > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index 1eba07f..aeb6a7c 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -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); 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/29/2015 02:51 PM, Andrew Lunn wrote: > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct. Thursday 29 (44) 03:40 PM, Andrew Lunn wrote: > > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/29/2015 04:28 PM, Andrew Lunn wrote: >> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 30, 2015 at 04:44:13PM +0100, Neil Armstrong wrote: > On 10/29/2015 04:28 PM, Andrew Lunn wrote: > >> 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. Great, always nice to see unneeded code removed. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/dsa.h b/include/net/dsa.h index 98ccbde..d4d13f7 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -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. diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 1eba07f..aeb6a7c 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -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];