WARNINGs because of ide-proc handling

WARNINGs because of ide-proc handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

Hello,

I get the following warning (and some similar ones) on my laptop running
2.6.36-rc3 when removing a CF card via pcmcia:

[ 1087.602129] WARNING: at fs/proc/generic.c:816 remove_proc_entry+0x185/0x1d0()
[ 1087.602137] Hardware name: Amilo M14255
[ 1087.602142] name 'cache'
...

It turned out that none of the entries in ide_disk_proc[] gets created when
inserting the card, so trying to remove them triggers this warning. Adding
some debug printouts

========

diff --git a/drivers/ide/ide-proc.c b/drivers/ide/ide-proc.c
index a3133d7..5016a54 100644
--- a/drivers/ide/ide-proc.c
+++ b/drivers/ide/ide-proc.c
@@ -650,6 +651,7 @@ void ide_proc_register_driver(ide_drive_t *drive, struct ide_driver *driver)
        drive->settings = driver->proc_devsets(drive);
        mutex_unlock(&ide_setting_mtx);
 
+       printk(KERN_INFO "*** wsa: %s - %p - %d\n", driver->proc_entries(drive)->name, drive->proc, (drive->media == ide_disk));
        ide_add_proc_entries(drive->proc, driver->proc_entries(drive), drive);
 }
 
========

results in:

[    9.040157] *** wsa: cache - (null) - 1

This shows that the path needed for those proc-entries does not exist at this
moment (pointer is null). So the callchain for creating all the proc-entries
seems to be disordered? The entries are present for my hard-disk, though.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: WARNINGs because of ide-proc handling

[David Miller]

From: Wolfram Sang <w.sang@pengutronix.de>
Date: Tue, 31 Aug 2010 22:56:05 +0200

> Hello,
> 
> I get the following warning (and some similar ones) on my laptop running
> 2.6.36-rc3 when removing a CF card via pcmcia:
> 
> [ 1087.602129] WARNING: at fs/proc/generic.c:816 remove_proc_entry+0x185/0x1d0()
> [ 1087.602137] Hardware name: Amilo M14255
> [ 1087.602142] name 'cache'
> ...
> 
> It turned out that none of the entries in ide_disk_proc[] gets created when
> inserting the card, so trying to remove them triggers this warning. Adding
> some debug printouts

Hmmm, at some point ide_host_add() or ide_host_register() should be invoked,
which should create the drive->proc and hwif->proc PROCFS directories.

What driver is your pcmcia IDE card using?

Re: WARNINGs because of ide-proc handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]

On Wed, Sep 01, 2010 at 07:33:32PM -0700, David Miller wrote:

Adding Bartlomiej to CC, maybe he has something to add :)

> From: Wolfram Sang <w.sang@pengutronix.de>
> Date: Tue, 31 Aug 2010 22:56:05 +0200
> 
> > Hello,
> > 
> > I get the following warning (and some similar ones) on my laptop running
> > 2.6.36-rc3 when removing a CF card via pcmcia:
> > 
> > [ 1087.602129] WARNING: at fs/proc/generic.c:816 remove_proc_entry+0x185/0x1d0()
> > [ 1087.602137] Hardware name: Amilo M14255
> > [ 1087.602142] name 'cache'
> > ...
> > 
> > It turned out that none of the entries in ide_disk_proc[] gets created when
> > inserting the card, so trying to remove them triggers this warning. Adding
> > some debug printouts
> 
> Hmmm, at some point ide_host_add() or ide_host_register() should be invoked,
> which should create the drive->proc and hwif->proc PROCFS directories.
> 
> What driver is your pcmcia IDE card using?

ide-cs.

I did some more debugging and have a function_graph-trace which proves the
following callchain:

ide_host_add -> ... -> ide_host_register

This calls _first_ hwif_register_devices() and only _later_
ide_proc_register_port() and ide_proc_port_register_devices() which are
responsible for creating the needed directories. In hwif_register_devices(), a
device gets created and immediately attached to the driver, ide-gd in this
case. The probe-function calls disk_ops->setup() which wants to create the
proc-entries, but the directories are not there yet. This patch helps,
combining two loops and moving hwif_register_devices() to the end:

diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 4c3d1bf..068cef0 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -1448,19 +1448,13 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d,
 		if (hwif == NULL)
 			continue;
 
-		if (hwif->present)
-			hwif_register_devices(hwif);
-	}
-
-	ide_host_for_each_port(i, hwif, host) {
-		if (hwif == NULL)
-			continue;
-
 		ide_sysfs_register_port(hwif);
 		ide_proc_register_port(hwif);
 
-		if (hwif->present)
+		if (hwif->present) {
 			ide_proc_port_register_devices(hwif);
+			hwif_register_devices(hwif);
+		}
 	}
 
 	return j ? 0 : -1;

Dunno, if it is correct, though. The strange thing is that the code is like
this for two years and, surprisingly, I don't see the problem with 2.6.33.
Also, the proc-entry for my hard-disk is fine. I can't see any suspicious
commits in the ide-directory except maybe the BKL pushdown had a side-effect
regarding the serialization of the sequence? Ah, too late today to seriously
think about such issues...

Thanks for the input,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: WARNINGs because of ide-proc handling

[David Miller]

From: Wolfram Sang <w.sang@pengutronix.de>
Date: Thu, 2 Sep 2010 23:28:39 +0200

> Dunno, if it is correct, though. The strange thing is that the code is like
> this for two years and, surprisingly, I don't see the problem with 2.6.33.
> Also, the proc-entry for my hard-disk is fine. I can't see any suspicious
> commits in the ide-directory except maybe the BKL pushdown had a side-effect
> regarding the serialization of the sequence? Ah, too late today to seriously
> think about such issues...

I don't think it's the BKL pushdown, it seems it would be due to something
else to me.

Thanks for your analysis and suggested patch, I'll take a look at this.

Re: WARNINGs because of ide-proc handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]


> 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.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: WARNINGs because of ide-proc handling

[David Miller]

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.

Re: WARNINGs because of ide-proc handling

[Bartlomiej Zolnierkiewicz]

On Friday 03 September 2010 02:34:37 pm David Miller wrote:
> 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.

JFYI deprecated procfs stuff has been long gone from atang tree..

Thanks.
--
Bartlomiej Zolnierkiewicz

Re: WARNINGs because of ide-proc handling

[David Miller]

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>

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);
 	/*

Re: WARNINGs because of ide-proc handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: WARNINGs because of ide-proc handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: WARNINGs because of ide-proc handling

[David Miller]

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.