| Message ID | 1275998127.1899.38.camel@yio.site |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote: > Ok, this needs testing. > > Please let me know, if that works for you so far. Will do. Just a question: none of this seems to pin the module? So can I be sure if I rmmod the module that the release function will still be around etc.? johannes -- 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 Tue, Jun 8, 2010 at 14:03, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote: > >> Ok, this needs testing. >> >> Please let me know, if that works for you so far. > > Will do. Just a question: none of this seems to pin the module? So can I > be sure if I rmmod the module that the release function will still be > around etc.? Oh, sorry, that's something very specific to network devices, that the module can be unloaded while the devices it has created are still in use. I am not sure right now what's needed to make this work in a single module. Kay -- 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 Tue, 2010-06-08 at 15:54 +0200, Kay Sievers wrote: > On Tue, Jun 8, 2010 at 14:03, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote: > > > >> Ok, this needs testing. > >> > >> Please let me know, if that works for you so far. > > > > Will do. Just a question: none of this seems to pin the module? So can I > > be sure if I rmmod the module that the release function will still be > > around etc.? > > Oh, sorry, that's something very specific to network devices, that the > module can be unloaded while the devices it has created are still in > use. I am not sure right now what's needed to make this work in a > single module. Well they will be unregistered and everything, but once all the netdevs are gone etc. the devices you create in the patch might stick around because somebody has them open in sysfs, and I see nothing that would pin the module in that case? johannes -- 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 Tue, Jun 8, 2010 at 16:06, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2010-06-08 at 15:54 +0200, Kay Sievers wrote: >> On Tue, Jun 8, 2010 at 14:03, Johannes Berg <johannes@sipsolutions.net> wrote: >> > On Tue, 2010-06-08 at 13:55 +0200, Kay Sievers wrote: >> > >> >> Ok, this needs testing. >> >> >> >> Please let me know, if that works for you so far. >> > >> > Will do. Just a question: none of this seems to pin the module? So can I >> > be sure if I rmmod the module that the release function will still be >> > around etc.? >> >> Oh, sorry, that's something very specific to network devices, that the >> module can be unloaded while the devices it has created are still in >> use. I am not sure right now what's needed to make this work in a >> single module. > > Well they will be unregistered and everything, but once all the netdevs > are gone etc. the devices you create in the patch might stick around > because somebody has them open in sysfs, and I see nothing that would > pin the module in that case? That's what I mean, I have no idea how to solve that with network devices. I don't think any other subsytem allows to unload the module, when devices, the module has created, are in use. With this patch, it is likely to fail, even without sysfs, just when the netif is still in up, and the module is unloaded. The current code uses the in-core class_create() logic, which was only meant for devices with a device node, and which is cleaned up by the core itself. That's why this issue never appeared before. But as said, I have no idea how to solve that with a single module. It might not work at all without moving stuff into the core. That people use device_create() with no major/minor might indicate that something else is needed here. :) Kay -- 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 Tue, 2010-06-08 at 16:21 +0200, Kay Sievers wrote: > > Well they will be unregistered and everything, but once all the netdevs > > are gone etc. the devices you create in the patch might stick around > > because somebody has them open in sysfs, and I see nothing that would > > pin the module in that case? > > That's what I mean, I have no idea how to solve that with network > devices. I don't think any other subsytem allows to unload the module, > when devices, the module has created, are in use. You're right ... the would only be unregistered from your release, which would happen after the module is long gone ... > The current code uses the in-core class_create() logic, which was only > meant for devices with a device node, and which is cleaned up by the > core itself. That's why this issue never appeared before. > > But as said, I have no idea how to solve that with a single module. It > might not work at all without moving stuff into the core. That people > use device_create() with no major/minor might indicate that something > else is needed here. :) So ... can we apply Eric's patch for now then? johannes -- 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 Tue, Jun 8, 2010 at 16:26, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2010-06-08 at 16:21 +0200, Kay Sievers wrote: > >> > Well they will be unregistered and everything, but once all the netdevs >> > are gone etc. the devices you create in the patch might stick around >> > because somebody has them open in sysfs, and I see nothing that would >> > pin the module in that case? >> >> That's what I mean, I have no idea how to solve that with network >> devices. I don't think any other subsytem allows to unload the module, >> when devices, the module has created, are in use. > > You're right ... the would only be unregistered from your release, which > would happen after the module is long gone ... > >> The current code uses the in-core class_create() logic, which was only >> meant for devices with a device node, and which is cleaned up by the >> core itself. That's why this issue never appeared before. >> >> But as said, I have no idea how to solve that with a single module. It >> might not work at all without moving stuff into the core. That people >> use device_create() with no major/minor might indicate that something >> else is needed here. :) > > So ... can we apply Eric's patch for now then? It might break other stuff we don't know about yet. Just like we did not know about what things hwsim is doing here. :) Eric's patch change the sysfs layout and insert directories into the device path, and stuff which hardcodes things, or accesses devices with ../<name> will break. I don't mind trying it, but I wouldn't be surprised if that needs to be reverted when issues caused by this appear. The hwsim issues are caused by the current hwsim code, by doing things it should not do. Class devices of different classes must never be stacked (the core should not allow that in the first place). Class devices must never have a driver assigned behind its back. Also device_create() should not be used for devices without a major/minor (but that seems to be done in several other places too). To fix the hwsim driver core interaction, core changes will probably be needed to allow network modules to be removed while their devices are active. That's something which seems not to work for bus devices currently. Kay -- 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 Tue, 2010-06-08 at 16:47 +0200, Kay Sievers wrote: > > So ... can we apply Eric's patch for now then? > > It might break other stuff we don't know about yet. Just like we did > not know about what things hwsim is doing here. :) True. > The hwsim issues are caused by the current hwsim code, by doing things > it should not do. Class devices of different classes must never be > stacked (the core should not allow that in the first place). Class > devices must never have a driver assigned behind its back. Also > device_create() should not be used for devices without a major/minor > (but that seems to be done in several other places too). Back when hwsim was written that would have been useful feedback to whoever did ... now, not so much. > To fix the hwsim driver core interaction, core changes will probably > be needed to allow network modules to be removed while their devices > are active. That's something which seems not to work for bus devices > currently. Well it just needs to pin the module refcount from the bus and/or device struct. That seems not too hard? johannes -- 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 Tue, Jun 8, 2010 at 17:06, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2010-06-08 at 16:47 +0200, Kay Sievers wrote: > >> > So ... can we apply Eric's patch for now then? >> >> It might break other stuff we don't know about yet. Just like we did >> not know about what things hwsim is doing here. :) > > True. > >> The hwsim issues are caused by the current hwsim code, by doing things >> it should not do. Class devices of different classes must never be >> stacked (the core should not allow that in the first place). Class >> devices must never have a driver assigned behind its back. Also >> device_create() should not be used for devices without a major/minor >> (but that seems to be done in several other places too). > > Back when hwsim was written that would have been useful feedback to > whoever did ... now, not so much. Well, it still needs to be fixed. It will break again, when stuff changes like now. >> To fix the hwsim driver core interaction, core changes will probably >> be needed to allow network modules to be removed while their devices >> are active. That's something which seems not to work for bus devices >> currently. > > Well it just needs to pin the module refcount from the bus and/or device > struct. That seems not too hard? How? If we ever get a reference of the module, it will not be able to be unloaded while stuff is active, right? Kay -- 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 Tue, 2010-06-08 at 18:26 +0200, Kay Sievers wrote: > >> To fix the hwsim driver core interaction, core changes will probably > >> be needed to allow network modules to be removed while their devices > >> are active. That's something which seems not to work for bus devices > >> currently. > > > > Well it just needs to pin the module refcount from the bus and/or device > > struct. That seems not too hard? > > How? If we ever get a reference of the module, it will not be able to > be unloaded while stuff is active, right? Hmm, true, but how does this work with class devices it has now? It seems to work fine now. It's because we have a generic release function there I guess? johannes -- 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 Tue, Jun 8, 2010 at 18:33, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2010-06-08 at 18:26 +0200, Kay Sievers wrote: > >> >> To fix the hwsim driver core interaction, core changes will probably >> >> be needed to allow network modules to be removed while their devices >> >> are active. That's something which seems not to work for bus devices >> >> currently. >> > >> > Well it just needs to pin the module refcount from the bus and/or device >> > struct. That seems not too hard? >> >> How? If we ever get a reference of the module, it will not be able to >> be unloaded while stuff is active, right? > > Hmm, true, but how does this work with class devices it has now? It > seems to work fine now. It's because we have a generic release function > there I guess? Because the code to cleanup is always built into the core, yes. Even the empty release function thing we can not do. :) That all works if you have two modules, like almost all buses have. That's what I meant, that we need to add stuff to the core to be able to cleanup bus devices internally too, if we use everything in a single module, which is also supposed to cleanup on unload, like the network devices like to do. Kay -- 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 Tue, 2010-06-08 at 18:39 +0200, Kay Sievers wrote: > That all works if you have two modules, like almost all buses have. > That's what I meant, that we need to add stuff to the core to be able > to cleanup bus devices internally too, if we use everything in a > single module, which is also supposed to cleanup on unload, like the > network devices like to do. Or some "wait for bus to be cleaned up" we can call in the module exit maybe? johannes -- 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, Jun 11, 2010 at 11:55, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2010-06-08 at 18:39 +0200, Kay Sievers wrote: > >> That all works if you have two modules, like almost all buses have. >> That's what I meant, that we need to add stuff to the core to be able >> to cleanup bus devices internally too, if we use everything in a >> single module, which is also supposed to cleanup on unload, like the >> network devices like to do. > > Or some "wait for bus to be cleaned up" we can call in the module exit > maybe? That would block the rmmod process until the resources are cleaned up, wouldn't it? The network devices can do this because the cleanup code is always compiled-in, for a module cleaning up itself, this is kind of complicated, isn't it? Kay -- 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 Mon, 2010-06-14 at 11:13 +0200, Kay Sievers wrote: > That would block the rmmod process until the resources are cleaned up, > wouldn't it? Yes, would that be so bad? > The network devices can do this because the cleanup code is always > compiled-in, for a module cleaning up itself, this is kind of > complicated, isn't it? It just needs a wait_for_bus_exit() function that the module calls in _exit? johannes -- 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 Mon, Jun 14, 2010 at 11:20, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2010-06-14 at 11:13 +0200, Kay Sievers wrote: > >> That would block the rmmod process until the resources are cleaned up, >> wouldn't it? > > Yes, would that be so bad? Sounds fine, if we can expect that nothing else can hold a reference and may block rmmod for a very long time with this logic. Sysfs access should be fine these days, Tejun changed this a while ago. > It just needs a wait_for_bus_exit() function that the module calls in > _exit? Sounds worth trying, to see if that works as expected. Kay -- 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/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 6f8cb3e..2543e74 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -194,7 +194,16 @@ static inline void hwsim_clear_sta_magic(struct ieee80211_sta *sta) sp->magic = 0; } -static struct class *hwsim_class; +static struct bus_type hwsim_bus = { + .name = "mac80211_hwsim", +}; + +static struct device_driver hwsim_driver = { + .name = "mac80211_hwsim", + .bus = &hwsim_bus, +}; + +static struct device *hwsim_virtual_parent; static struct net_device *hwsim_mon; /* global monitor netdev */ @@ -280,7 +289,7 @@ static struct list_head hwsim_radios; struct mac80211_hwsim_data { struct list_head list; struct ieee80211_hw *hw; - struct device *dev; + struct device dev; struct ieee80211_supported_band bands[2]; struct ieee80211_channel channels_2ghz[ARRAY_SIZE(hwsim_channels_2ghz)]; struct ieee80211_channel channels_5ghz[ARRAY_SIZE(hwsim_channels_5ghz)]; @@ -1063,22 +1072,13 @@ static void mac80211_hwsim_free(void) list_move(i, &tmplist); spin_unlock_bh(&hwsim_radio_lock); - list_for_each_entry_safe(data, tmpdata, &tmplist, list) { - debugfs_remove(data->debugfs_group); - debugfs_remove(data->debugfs_ps); - debugfs_remove(data->debugfs); - ieee80211_unregister_hw(data->hw); - device_unregister(data->dev); - ieee80211_free_hw(data->hw); - } - class_destroy(hwsim_class); + list_for_each_entry_safe(data, tmpdata, &tmplist, list) + device_unregister(&data->dev); + device_unregister(hwsim_virtual_parent); + driver_unregister(&hwsim_driver); + bus_unregister(&hwsim_bus); } - -static struct device_driver mac80211_hwsim_driver = { - .name = "mac80211_hwsim" -}; - static const struct net_device_ops hwsim_netdev_ops = { .ndo_start_xmit = hwsim_mon_xmit, .ndo_change_mtu = eth_change_mtu, @@ -1095,6 +1095,7 @@ static void hwsim_mon_setup(struct net_device *dev) dev->type = ARPHRD_IEEE80211_RADIOTAP; memset(dev->dev_addr, 0, ETH_ALEN); dev->dev_addr[0] = 0x12; + dev->dev.parent = hwsim_virtual_parent; } @@ -1231,6 +1232,17 @@ DEFINE_SIMPLE_ATTRIBUTE(hwsim_fops_group, hwsim_fops_group_read, hwsim_fops_group_write, "%llx\n"); +static void hwsim_dev_release(struct device* hwsim_dev) { + struct mac80211_hwsim_data *data = + container_of(hwsim_dev, struct mac80211_hwsim_data, dev); + + debugfs_remove(data->debugfs_group); + debugfs_remove(data->debugfs_ps); + debugfs_remove(data->debugfs); + ieee80211_unregister_hw(data->hw); + ieee80211_free_hw(data->hw); +} + static int __init init_mac80211_hwsim(void) { int i, err = 0; @@ -1251,9 +1263,22 @@ static int __init init_mac80211_hwsim(void) spin_lock_init(&hwsim_radio_lock); INIT_LIST_HEAD(&hwsim_radios); - hwsim_class = class_create(THIS_MODULE, "mac80211_hwsim"); - if (IS_ERR(hwsim_class)) - return PTR_ERR(hwsim_class); + err = bus_register(&hwsim_bus); + if (err) + return err; + + err = driver_register(&hwsim_driver); + if (err) { + bus_unregister(&hwsim_bus); + return err; + } + + hwsim_virtual_parent = root_device_register("mac80211_hwsim"); + if (IS_ERR(hwsim_virtual_parent)) { + driver_unregister(&hwsim_driver); + bus_unregister(&hwsim_bus); + return PTR_ERR(hwsim_virtual_parent); + } memset(addr, 0, ETH_ALEN); addr[0] = 0x02; @@ -1271,18 +1296,19 @@ static int __init init_mac80211_hwsim(void) data = hw->priv; data->hw = hw; - data->dev = device_create(hwsim_class, NULL, 0, hw, - "hwsim%d", i); - if (IS_ERR(data->dev)) { + dev_set_name(&data->dev, "hwsim%d", i); + data->dev.parent = hwsim_virtual_parent; + data->dev.bus = &hwsim_bus; + data->dev.release = hwsim_dev_release; + err = device_register(&data->dev); + if (err) { printk(KERN_DEBUG - "mac80211_hwsim: device_create " - "failed (%ld)\n", PTR_ERR(data->dev)); - err = -ENOMEM; + "mac80211_hwsim: device_register failed (%d)\n", + err); goto failed_drvdata; } - data->dev->driver = &mac80211_hwsim_driver; - SET_IEEE80211_DEV(hw, data->dev); + SET_IEEE80211_DEV(hw, &data->dev); addr[3] = i >> 8; addr[4] = i; memcpy(data->addresses[0].addr, addr, ETH_ALEN); @@ -1513,7 +1539,7 @@ failed_mon: return err; failed_hw: - device_unregister(data->dev); + device_unregister(&data->dev); failed_drvdata: ieee80211_free_hw(hw); failed: