Patchwork WARNINGs because of ide-proc handling

login
register
mail settings
Submitter David Miller
Date Sept. 3, 2010, 2:35 p.m.
Message ID <20100903.073503.191401190.davem@davemloft.net>
Download mbox | patch
Permalink /patch/63685/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

David Miller - Sept. 3, 2010, 2:35 p.m.
From: David Miller <davem@davemloft.net>
Date: Fri, 03 Sep 2010 05:34:37 -0700 (PDT)

> From: Wolfram Sang <w.sang@pengutronix.de>
> Date: Fri, 3 Sep 2010 07:53:28 +0200
> 
>> 
>>> Thanks for your analysis and suggested patch, I'll take a look at this.
>> 
>> Thanks for your help. If we can't find something, I'll just do a bisect this
>> weekend.
> 
> Frankly, at this point, I'm beginning suspect that the procfs stuff
> simply never warned at some point in the past and that the IDE
> code has had this problem for a while.

I just confirmed that the procfs code only started warning on removal
of non-present entries this past March, so basically the IDE code has
silently had this problem of not actually creating any of it's procfs
nodes for some time now.

Wolfram I'll look into your patch some more and integrate it, thanks.

commit 12bac0d9f4dbf3445a0319beee848d15fa32775e
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Fri Mar 5 13:44:00 2010 -0800

    proc: warn on non-existing proc entries
    
    * warn if creation goes on to non-existent directory
    * warn if removal goes on from non-existing directory
    * warn if non-existing proc entry is removed
    
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Cc: Al Viro <viro@zeniv.linux.org.uk>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang - Sept. 3, 2010, 2:47 p.m.
On Fri, Sep 03, 2010 at 07:35:03AM -0700, David Miller wrote:

> >>> Thanks for your analysis and suggested patch, I'll take a look at this.
> >> 
> >> Thanks for your help. If we can't find something, I'll just do a bisect this
> >> weekend.
> > 
> > Frankly, at this point, I'm beginning suspect that the procfs stuff
> > simply never warned at some point in the past and that the IDE
> > code has had this problem for a while.
> 
> I just confirmed that the procfs code only started warning on removal
> of non-present entries this past March, so basically the IDE code has
> silently had this problem of not actually creating any of it's procfs
> nodes for some time now.
> 
> Wolfram I'll look into your patch some more and integrate it, thanks.

I am still puzzled why my hard-disk does have these entries then. I am
away from that laptop currently, but will check later this evening.

Regards,

   Wolfram
Wolfram Sang - Sept. 3, 2010, 6:59 p.m.
On Fri, Sep 03, 2010 at 07:35:03AM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Fri, 03 Sep 2010 05:34:37 -0700 (PDT)
> 
> > From: Wolfram Sang <w.sang@pengutronix.de>
> > Date: Fri, 3 Sep 2010 07:53:28 +0200
> > 
> >> 
> >>> Thanks for your analysis and suggested patch, I'll take a look at this.
> >> 
> >> Thanks for your help. If we can't find something, I'll just do a bisect this
> >> weekend.
> > 
> > Frankly, at this point, I'm beginning suspect that the procfs stuff
> > simply never warned at some point in the past and that the IDE
> > code has had this problem for a while.
> 
> I just confirmed that the procfs code only started warning on removal
> of non-present entries this past March, so basically the IDE code has
> silently had this problem of not actually creating any of it's procfs
> nodes for some time now.

Confirming. With 2.6.33, I don't get the warnings, but neither get the
procfs-entries.

Checking my debug-output again, I assume that the hard-disk has the entries,
because it is the first user of ide-gd, so that driver still needs to be
requested. Maybe this is enough delay for the directories to appear? Wild
guess...

> Wolfram I'll look into your patch some more and integrate it, thanks.

Just in case you need a SoB:

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Thanks,

   Wolfram
David Miller - Sept. 14, 2010, 4:17 a.m.
From: Wolfram Sang <w.sang@pengutronix.de>
Date: Fri, 3 Sep 2010 20:59:34 +0200

> Checking my debug-output again, I assume that the hard-disk has the entries,
> because it is the first user of ide-gd, so that driver still needs to be
> requested. Maybe this is enough delay for the directories to appear? Wild
> guess...

The issue has to do with the order of module initialization.

I've never seen this since on machines where IDE is actually used I
tend to build everything statically into the tree.

In such a configuration, the ide-cd, ide-gd, etc. drivers come at the
end of the link, based upon the ordering listed in the file
drivers/ide/Makefile.

Therefore, their probe functions are invoked last, long after core IDE
and the individual IDE host controller drivers register.

The individual IDE host controller probes will therefore happen first.

As we discussed, things work if ide_proc_port_register_devices()
executes before ide_proc_register_driver() is called.

And this is satisfied by the "all of IDE built-in" configuration
because the event sequence is:

1) IDE host controller driver registers, probes and finds ideX
2) ide_host_register(ideX, ...)
3) ide_proc_port_register_devices(ideX)

... some time passes ...

4) ide-gd driver registers
5) generic_ide_probe(hdX)
6) ide_gd_probe()
7) disk_ops->setup()
8) ide_disk_setup()
9) ide_proc_register_driver(hdX)

etc.

At ide_host_register() time, hwif_register_devices() created all of
the IDE devices on the ide_bus_type bus, but since ide-gd et al.  were
not registered yet, these device instances don't get matched and
probed at hwif_register_devices() time.

However, if the various IDE core driver bits are modules and are
loaded already, then the ide-cs driver subsequently loads, or an
ide-cs device hot plugs, then we get into the situation you've
discovered where we invoke the PROCFS init functions in the wrong
order.

Anyways, I just wanted to let you know that I figured out why it does
work in some circumstances, and I will now go back and do a final
audit of your patch before applying it.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" 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/fs/proc/generic.c b/fs/proc/generic.c
index ce2d954..08f4d71 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -312,8 +312,10 @@  static int __xlate_proc_name(const char *name, struct proc_dir_entry **ret,
 			if (proc_match(len, cp, de))
 				break;
 		}
-		if (!de)
+		if (!de) {
+			WARN(1, "name '%s'\n", name);
 			return -ENOENT;
+		}
 		cp += len + 1;
 	}
 	*residual = cp;
@@ -818,8 +820,10 @@  void remove_proc_entry(const char *name, struct proc_dir_entry *parent)
 		}
 	}
 	spin_unlock(&proc_subdir_lock);
-	if (!de)
+	if (!de) {
+		WARN(1, "name '%s'\n", name);
 		return;
+	}
 
 	spin_lock(&de->pde_unload_lock);
 	/*