Patchwork [RFC] Fix another namespace issue with devices assigned to classes

login
register
mail settings
Submitter Kay Sievers
Date June 7, 2010, 12:26 p.m.
Message ID <1275913563.1823.1.camel@yio.site>
Download mbox | patch
Permalink /patch/54849/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Kay Sievers - June 7, 2010, 12:26 p.m.
On Mon, Jun 7, 2010 at 13:41, Johannes Berg <johannes@sipsolutions.net> wrote:
> (mind you, I think we probably need to have the bus/driver assignment,
> but I wanted to try out your suggestion first)

Class device can never have a driver. And unregistered drivers can
not be used, what you try to do here. That all should just be removed or
properly registered with the core if needed.

> So I removed bus/driver assignment from the above code just to try it
> out, and got
>
> Device 'hwsim0' does not have a release() function, it is broken and
> must be fixed.

The driver core expects the resources of the device to be freed on
release. You can provide an empty release function because you do this
from a global list of devices.

> This has evolved far too much for me right now. Can we just apply the
> initial patch from Eric and be happier for a while? I can't justify
> spending this much time on it right now. Alternatively, you could look
> at hwsim too, since it's all virtual, nothing special is required, I do
> testing in a virtual machine ...
>
> current patch is at
> http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch

Here is something that seems to work for me.

Cheers,
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
Johannes Berg - June 7, 2010, 12:36 p.m.
On Mon, 2010-06-07 at 14:26 +0200, Kay Sievers wrote:
> On Mon, Jun 7, 2010 at 13:41, Johannes Berg <johannes@sipsolutions.net> wrote:
> > (mind you, I think we probably need to have the bus/driver assignment,
> > but I wanted to try out your suggestion first)
> 
> Class device can never have a driver. And unregistered drivers can
> not be used, what you try to do here. That all should just be removed or
> properly registered with the core if needed.

Yeah but it shouldn't influence the operation? Anyway I see from your
patch that I should have assigned the release function and that would've
helped I guess.

> > So I removed bus/driver assignment from the above code just to try it
> > out, and got
> >
> > Device 'hwsim0' does not have a release() function, it is broken and
> > must be fixed.
> 
> The driver core expects the resources of the device to be freed on
> release. You can provide an empty release function because you do this
> from a global list of devices.

Ok, makes sense.

> > This has evolved far too much for me right now. Can we just apply the
> > initial patch from Eric and be happier for a while? I can't justify
> > spending this much time on it right now. Alternatively, you could look
> > at hwsim too, since it's all virtual, nothing special is required, I do
> > testing in a virtual machine ...
> >
> > current patch is at
> > http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch
> 
> Here is something that seems to work for me.

I see you remove the driver. Does this mean that in sysfs these devices
wouldn't have a driver symlink? ISTR that we needed that for userspace,
but I'm not entirely sure, and I don't have all the relevant userspace
in my test setup.

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
Kay Sievers - June 7, 2010, 12:54 p.m.
On Mon, Jun 7, 2010 at 14:36, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2010-06-07 at 14:26 +0200, Kay Sievers wrote:
>> On Mon, Jun 7, 2010 at 13:41, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > (mind you, I think we probably need to have the bus/driver assignment,
>> > but I wanted to try out your suggestion first)
>>
>> Class device can never have a driver. And unregistered drivers can
>> not be used, what you try to do here. That all should just be removed or
>> properly registered with the core if needed.
>
> Yeah but it shouldn't influence the operation?

It does. You can not use "dead static" drivers. They need to be
properly registered, and can only work for bus devices. Class devices
are not allowed to have drivers.

This worked only because the core ignored the value you assigned
behind its back. :)

>> Here is something that seems to work for me.
>
> I see you remove the driver. Does this mean that in sysfs these devices
> wouldn't have a driver symlink? ISTR that we needed that for userspace,
> but I'm not entirely sure, and I don't have all the relevant userspace
> in my test setup.

Yeah, if you need the driver, you need to use bus device, as you do
now. The driver needs to be registered with the core, and the probe
function needs to tell to bind to the devices while registered. You
can not manually assign this.

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
Johannes Berg - June 8, 2010, 9:27 a.m.
On Mon, 2010-06-07 at 14:54 +0200, Kay Sievers wrote:

> > I see you remove the driver. Does this mean that in sysfs these devices
> > wouldn't have a driver symlink? ISTR that we needed that for userspace,
> > but I'm not entirely sure, and I don't have all the relevant userspace
> > in my test setup.
> 
> Yeah, if you need the driver, you need to use bus device, as you do
> now. The driver needs to be registered with the core, and the probe
> function needs to tell to bind to the devices while registered. You
> can not manually assign this.

So I spoke to Jouni and he says that he doesn't remember needing the
driver, so I guess we don't care. Can I get you to submit that patch
properly? Send it to linux-wireless@vger.kernel.org please.

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
Kay Sievers - June 8, 2010, 9:30 a.m.
On Tue, Jun 8, 2010 at 11:27, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2010-06-07 at 14:54 +0200, Kay Sievers wrote:
>
>> > I see you remove the driver. Does this mean that in sysfs these devices
>> > wouldn't have a driver symlink? ISTR that we needed that for userspace,
>> > but I'm not entirely sure, and I don't have all the relevant userspace
>> > in my test setup.
>>
>> Yeah, if you need the driver, you need to use bus device, as you do
>> now. The driver needs to be registered with the core, and the probe
>> function needs to tell to bind to the devices while registered. You
>> can not manually assign this.
>
> So I spoke to Jouni and he says that he doesn't remember needing the
> driver, so I guess we don't care. Can I get you to submit that patch
> properly? Send it to linux-wireless@vger.kernel.org please.

Sure, you mean the patch that changes the hwsim code, or the one I
talked about to properly create a virtual bus parent?

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
Johannes Berg - June 8, 2010, 9:45 a.m.
On Tue, 2010-06-08 at 11:30 +0200, Kay Sievers wrote:
> On Tue, Jun 8, 2010 at 11:27, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2010-06-07 at 14:54 +0200, Kay Sievers wrote:
> >
> >> > I see you remove the driver. Does this mean that in sysfs these devices
> >> > wouldn't have a driver symlink? ISTR that we needed that for userspace,
> >> > but I'm not entirely sure, and I don't have all the relevant userspace
> >> > in my test setup.
> >>
> >> Yeah, if you need the driver, you need to use bus device, as you do
> >> now. The driver needs to be registered with the core, and the probe
> >> function needs to tell to bind to the devices while registered. You
> >> can not manually assign this.
> >
> > So I spoke to Jouni and he says that he doesn't remember needing the
> > driver, so I guess we don't care. Can I get you to submit that patch
> > properly? Send it to linux-wireless@vger.kernel.org please.
> 
> Sure, you mean the patch that changes the hwsim code, or the one I
> talked about to properly create a virtual bus parent?

The hwsim one you pasted earlier.

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

Patch

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 6f8cb3e..d210ce6 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -194,7 +194,13 @@  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 hwsim_parent = {
+	.init_name = "mac80211_hwsim",
+};
 
 static struct net_device *hwsim_mon; /* global monitor netdev */
 
@@ -1071,14 +1077,10 @@  static void mac80211_hwsim_free(void)
 		device_unregister(data->dev);
 		ieee80211_free_hw(data->hw);
 	}
-	class_destroy(hwsim_class);
+	device_unregister(&hwsim_parent);
+	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,
@@ -1231,6 +1233,9 @@  DEFINE_SIMPLE_ATTRIBUTE(hwsim_fops_group,
 			hwsim_fops_group_read, hwsim_fops_group_write,
 			"%llx\n");
 
+/* all devices are kept in out own list and the ressources are freed on exit */
+static void hwsim_dev_release(struct device* dev) {}
+
 static int __init init_mac80211_hwsim(void)
 {
 	int i, err = 0;
@@ -1251,9 +1256,15 @@  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 = device_register(&hwsim_parent);
+	if (err) {
+		bus_unregister(&hwsim_bus);
+		return err;
+	}
 
 	memset(addr, 0, ETH_ALEN);
 	addr[0] = 0x02;
@@ -1271,16 +1282,24 @@  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)) {
-			printk(KERN_DEBUG
-			       "mac80211_hwsim: device_create "
-			       "failed (%ld)\n", PTR_ERR(data->dev));
+		data->dev = kzalloc(sizeof(struct device), GFP_KERNEL);
+		if (!data->dev) {
 			err = -ENOMEM;
 			goto failed_drvdata;
 		}
-		data->dev->driver = &mac80211_hwsim_driver;
+
+		dev_set_name(data->dev, "hwsim%d", i);
+		data->dev->parent = &hwsim_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_register failed (%d)\n",
+			       err);
+			goto failed_drvdata;
+		}
 
 		SET_IEEE80211_DEV(hw, data->dev);
 		addr[3] = i >> 8;