Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Greg KH]

> From: Jeff Dike <jdike@addtoit.com>, Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> Define a release method for the ubd and network driver so that sysfs doesn't
> complain when one is removed via:

What?  No.  The kernel is complaining for a reason, don't try to
out-smart it.

> 
> host $ uml_mconsole <umid> remove <dev>
> 
> Done by Jeff around January for ubd only, later lost, then restored in his tree
> - however I'm merging it now since there's no reason to leave this here.
> 
> We don't need to do any cleanup in the new added method, because when hot-unplug
> is done by uml_mconsole we already handle cleanup in mconsole infrastructure,
> i.e.  mc_device->remove (net_remove/ubd_remove), which is also the calling
> method.

Huh?  You have 2 different release functions for the same object?  And
how do you know which one is correct?  That does not sound right at all.

Please fix this correctly.

thanks,

greg k-h

Re: [uml-devel] Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Blaisorblade]

On Wednesday 18 January 2006 02:12, Greg KH wrote:
> > From: Jeff Dike <jdike@addtoit.com>, Paolo 'Blaisorblade' Giarrusso
> > <blaisorblade@yahoo.it>
> >
> > Define a release method for the ubd and network driver so that sysfs
> > doesn't complain when one is removed via:

> What?  No.  The kernel is complaining for a reason, don't try to
> out-smart it.

I'm not trying to ignore the warning.

> > host $ uml_mconsole <umid> remove <dev>

> > Done by Jeff around January for ubd only, later lost, then restored in
> > his tree - however I'm merging it now since there's no reason to leave
> > this here.

> > We don't need to do any cleanup in the new added method, because when
> > hot-unplug is done by uml_mconsole we already handle cleanup in mconsole
> > infrastructure, i.e.  mc_device->remove (net_remove/ubd_remove), which is
> > also the calling method.

> Huh?  You have 2 different release functions for the same object?

Not sure which ones you refer. net_remove and ubd_remove are for different 
devices; mc_device->remove and the sysfs release are in different layers and 
for different things.

> And 
> how do you know which one is correct?  That does not sound right at all.

No, but since we don't have plugs, we get configuration requests to remove 
devices.

And currently, since this existed in already 2.4, the handler of the 
configuration request(mc_device->remove) does all the work to shutdown the 
interface, and then calls into the kobject stuff (with 
platform_device_unregister()); and this complains for the missing ->remove.

If it's better, we may try to move things inside the sysfs ->remove event, 
with care because different stuff has been done.

However, there are a lot of things to look at here... is there anything in 
ubd_remove() smelling awful? Because we have a crash which is caused by us 
harming sysfs data structures, which triggers on ubd_remove().

Bug report:
http://marc.theaimsgroup.com/?l=user-mode-linux-devel&m=113537479514679&w=2

investigations:
http://marc.theaimsgroup.com/?l=user-mode-linux-devel&m=113538805406552&w=2

> Please fix this correctly.

-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it

Re: [uml-devel] Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Greg KH]

On Wed, Jan 18, 2006 at 12:53:54PM +0100, Blaisorblade wrote:
> On Wednesday 18 January 2006 02:12, Greg KH wrote:
> > > From: Jeff Dike <jdike@addtoit.com>, Paolo 'Blaisorblade' Giarrusso
> > > <blaisorblade@yahoo.it>
> > >
> > > Define a release method for the ubd and network driver so that sysfs
> > > doesn't complain when one is removed via:
> 
> > What?  No.  The kernel is complaining for a reason, don't try to
> > out-smart it.
> 
> I'm not trying to ignore the warning.

By providing a release function that does nothing, all you are doing is
shutting the kernel up.  You are not doing anything to "fix" the real
problem at all.

> > > host $ uml_mconsole <umid> remove <dev>
> 
> > > Done by Jeff around January for ubd only, later lost, then restored in
> > > his tree - however I'm merging it now since there's no reason to leave
> > > this here.
> 
> > > We don't need to do any cleanup in the new added method, because when
> > > hot-unplug is done by uml_mconsole we already handle cleanup in mconsole
> > > infrastructure, i.e.  mc_device->remove (net_remove/ubd_remove), which is
> > > also the calling method.
> 
> > Huh?  You have 2 different release functions for the same object?
> 
> Not sure which ones you refer. net_remove and ubd_remove are for different 
> devices; mc_device->remove and the sysfs release are in different layers and 
> for different things.

Ok, but you are still adding 2 empty release functions.

Please do not do that, if you think you need this, your code logic is
wrong.

As for your specific bug report, sorry, I really don't know.

good luck,

greg k-h

Re: [uml-devel] Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Blaisorblade]

On Wednesday 18 January 2006 18:02, Greg KH wrote:
> On Wed, Jan 18, 2006 at 12:53:54PM +0100, Blaisorblade wrote:

> As for your specific bug report, sorry, I really don't know.

I know it's not something easy to find, but nor me nor Jeff master deeply the 
kobject API. And we're not short of work.

Is there anybody who while reviewing various drivers could review our ones on 
this aspect (the same way people like you use to review and fix bugs all over 
the tree)?

I know this may seem unkind, but it's a "specialization thing" - 
don't let an engineer heal somebody nor a doctor build a house, and don't 
request them doing such things - defer the thing to more competent people, if 
possible.

Alternatively, documentation is accepted. Is LDD v3 up-to-date enough? Guess 
not.

A question: looking at ide driver I saw that they don't use the 
platform_device API, because they're already in the block category.

Could we be causing trouble by using both block API (with gendisk stuff) and 
platform_device? Can platform_device

Alternatively, is drivers/input/serio/i8042.c a likely correct API example?
-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

		
___________________________________ 
Yahoo! Messenger with Voice: chiama da PC a telefono a tariffe esclusive 
http://it.messenger.yahoo.com

[PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Paolo 'Blaisorblade' Giarrusso]

From: Jeff Dike <jdike@addtoit.com>, Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Define a release method for the ubd and network driver so that sysfs doesn't
complain when one is removed via:

host $ uml_mconsole <umid> remove <dev>

Done by Jeff around January for ubd only, later lost, then restored in his tree
- however I'm merging it now since there's no reason to leave this here.

We don't need to do any cleanup in the new added method, because when hot-unplug
is done by uml_mconsole we already handle cleanup in mconsole infrastructure,
i.e.  mc_device->remove (net_remove/ubd_remove), which is also the calling
method.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/net_kern.c |    6 ++++++
 arch/um/drivers/ubd_kern.c |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c
index fb1f9fb..5b8c64e 100644
--- a/arch/um/drivers/net_kern.c
+++ b/arch/um/drivers/net_kern.c
@@ -276,6 +276,11 @@ static struct platform_driver uml_net_dr
 };
 static int driver_registered;
 
+/* Sysfs requires a ->release when mconsole hot-unplug is done */
+static void dummy_device_release(struct device *dev)
+{
+}
+
 static int eth_configure(int n, void *init, char *mac,
 			 struct transport *transport)
 {
@@ -324,6 +329,7 @@ static int eth_configure(int n, void *in
 	}
 	device->pdev.id = n;
 	device->pdev.name = DRIVER_NAME;
+	device->pdev.dev.release = dummy_device_release;
 	platform_device_register(&device->pdev);
 	SET_NETDEV_DEV(dev,&device->pdev.dev);
 
diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 7696f8d..49dda56 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -617,6 +617,11 @@ static int ubd_open_dev(struct ubd *dev)
 	return(err);
 }
 
+/* Sysfs requires a ->release when mconsole hot-unplug is done */
+static void dummy_device_release(struct device *dev)
+{
+}
+
 static int ubd_new_disk(int major, u64 size, int unit,
 			struct gendisk **disk_out)
 			
@@ -652,6 +657,7 @@ static int ubd_new_disk(int major, u64 s
 	if (major == MAJOR_NR) {
 		ubd_dev[unit].pdev.id   = unit;
 		ubd_dev[unit].pdev.name = DRIVER_NAME;
+		ubd_dev[unit].pdev.dev.release = dummy_device_release;
 		platform_device_register(&ubd_dev[unit].pdev);
 		disk->driverfs_dev = &ubd_dev[unit].pdev.dev;
 	}

Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Jeff Dike]

On Wed, Jan 18, 2006 at 01:19:21AM +0100, Paolo 'Blaisorblade' Giarrusso wrote:
> Done by Jeff around January for ubd only, later lost, then restored in his tree
> - however I'm merging it now since there's no reason to leave this here.

The original was dinged by hch for covering over a problem without really
fixing it.

We need to think about this one some more.

				Jeff

Re: [PATCH 1/9] uml: avoid sysfs warning on hot-unplug

[Blaisorblade]

On Wednesday 18 January 2006 03:53, Jeff Dike wrote:
> On Wed, Jan 18, 2006 at 01:19:21AM +0100, Paolo 'Blaisorblade' Giarrusso 
wrote:
> > Done by Jeff around January for ubd only, later lost, then restored in
> > his tree - however I'm merging it now since there's no reason to leave
> > this here.
>
> The original was dinged by hch for covering over a problem without really
> fixing it.
>
> We need to think about this one some more.

About this, have you reported upstream the "crash with remove ubd2 and ubd3" 
thing caused by wrong refcounts? Greg and friends tend to fix various drivers 
all around, I guess it's the right policy to ask them to fix this too.

-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

		
___________________________________ 
Yahoo! Messenger with Voice: chiama da PC a telefono a tariffe esclusive 
http://it.messenger.yahoo.com