Message ID | 56321DCD.3070702@baylibre.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Hi Neil, [auto build test ERROR on net/master -- if it's inappropriate base, please suggest rules for selecting the more suitable base] url: https://github.com/0day-ci/linux/commits/Neil-Armstrong/Further-fix-for-dsa-unbinding/20151029-212633 config: x86_64-randconfig-x010-201543 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 Note: the linux-review/Neil-Armstrong/Further-fix-for-dsa-unbinding/20151029-212633 HEAD 3c957c00de125eeffe9f50eda7cf94a98855f615 builds fine. It only hurts bisectibility. All errors (new ones prefixed by >>): net/dsa/dsa.c: In function 'dsa_switch_destroy': >> net/dsa/dsa.c:459:18: error: 'p' undeclared (first use in this function) phy_disconnect(p->phy); ^ net/dsa/dsa.c:459:18: note: each undeclared identifier is reported only once for each function it appears in vim +/p +459 net/dsa/dsa.c 453 454 if (!ds->ports[port]) 455 continue; 456 457 netif_carrier_off(ds->ports[port]); 458 unregister_netdev(ds->ports[port]); > 459 phy_disconnect(p->phy); 460 free_netdev(ds->ports[port]); 461 } 462 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Oct 29, 2015 at 02:23:25PM +0100, Neil Armstrong wrote: > Add missing netif_carrier_off and phy_disconnect calls to the > dsa_switch_destroy function to make sure the netdev and phy > ressources are clean before complete removal. > > Signed-off-by: Frode Isaksen <fisaksen@baylibre.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > net/dsa/dsa.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index 597a462..11452e4 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -454,7 +454,9 @@ static void dsa_switch_destroy(struct dsa_switch *ds) > if (!ds->ports[port]) > continue; > > + netif_carrier_off(ds->ports[port]); > unregister_netdev(ds->ports[port]); > + phy_disconnect(p->phy); > free_netdev(ds->ports[port]); > } Once you make it actually compile.... I'm not sure this is safe. The loop above this one has just destroyed some phys, and now you are potentially disconnecting a phy you just destroyed, causing it to be accessed? I would suggest you first fix the ordering in dsa_switch_destroy() and then add the missing netif_carrier_off() and phy_disconnect(). Thanks 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 03:00 PM, Andrew Lunn wrote: > On Thu, Oct 29, 2015 at 02:23:25PM +0100, Neil Armstrong wrote: >> >> + netif_carrier_off(ds->ports[port]); >> unregister_netdev(ds->ports[port]); >> + phy_disconnect(p->phy); >> free_netdev(ds->ports[port]); >> } > > Once you make it actually compile.... > > I'm not sure this is safe. The loop above this one has just destroyed > some phys, and now you are potentially disconnecting a phy you just > destroyed, causing it to be accessed? > > I would suggest you first fix the ordering in dsa_switch_destroy() > and then add the missing netif_carrier_off() and phy_disconnect(). > > Thanks > Andrew > Yes, you are right, I will submit a cleaned up version. I forgot the fixed phy case actually. Thanks, 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
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 597a462..11452e4 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -454,7 +454,9 @@ static void dsa_switch_destroy(struct dsa_switch *ds) if (!ds->ports[port]) continue; + netif_carrier_off(ds->ports[port]); unregister_netdev(ds->ports[port]); + phy_disconnect(p->phy); free_netdev(ds->ports[port]); }