On Tue, 2009-05-19 at 15:11 +0000, John Fastabend wrote: > Currently, the netdev module ref count is not decremented with module_put() > when the module is unloaded while fcoe instances are present. To fix this > removed reference count on netdev module completely and added functionality to > netdev event handling for NETDEV_UNREGISTER events. > > This allows fcoe to remove devices cleanly when the netdev module is unloaded > so we no longer need to hold a reference count for the netdev module. > > Signed-off-by: John Fastabend <[email protected]> > --- > > drivers/scsi/fcoe/fcoe.c | 94 > +++++++--------------------------------------- > 1 files changed, 14 insertions(+), 80 deletions(-) > > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c > index 03e1926..85e51e9 100644 > --- a/drivers/scsi/fcoe/fcoe.c > +++ b/drivers/scsi/fcoe/fcoe.c > @@ -371,14 +371,16 @@ static int fcoe_if_destroy(struct net_device *netdev) > fc_exch_mgr_free(lp->emp); > > /* Delete secondary MAC addresses */ > - rtnl_lock(); > - memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN); > - dev_unicast_delete(fc->real_dev, flogi_maddr, ETH_ALEN); > - if (!is_zero_ether_addr(fc->ctlr.data_src_addr)) > - dev_unicast_delete(fc->real_dev, > - fc->ctlr.data_src_addr, ETH_ALEN); > - dev_mc_delete(fc->real_dev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0); > - rtnl_unlock(); > + if (fc->real_dev->reg_state == NETREG_REGISTERED) {
John, Chris and I have been talking about this patch and I wanted to put the discussion out to the list to see if there were any thoughts on the topic. I don't really like this NETREG_REGISTERD check here. The reason it has to be done is because we cannot hold the rtnl_lock (next line in patch) if it's already held. That might happen because after this patch fcoe_if_destroy() could be called from fcoe_device_notification() when we get a NETDEV_UNREGISTER event. I think destroying the interface is correct if we get that event, but I wish we didn't have to have this check. My solution was to more broadly hold the rtnl_lock so that the "destroy" operation was completely atomic. What I'd do is make it so that the fcoe_if_destroy() routine requires the rtnl_lock, so we lock and unlock before and after calling it. We should probably do the same for "create" too. This would ensure that we're not trying to create/destroy the interface at the same time. It would also allow us to remove the NETREG_REGISTERED check because it's only there to prevent a deadlock and we'd be guaranteed that it would only be held once with this change. Thoughts? _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
