[PATCH 1/3] mmc: Export internal host state through debugfs

[PATCH 1/3] mmc: Export internal host state through debugfs

[Haavard Skinnemoen]

From: Haavard Skinnemoen <hskinnemoen@atmel.com>

This adds a new config option, MMC_DEBUG_FS which will, when enabled,
create a few files under /sys/kernel/debug containing information
about an mmc host's internal state.

Host drivers can add additional files and directories under the host's
root directory by passing the debugfs_root field in struct mmc_host as
the 'parent' parameter to debugfs_create_*.

Unfinished: No files are actually created yet.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/mmc/Kconfig      |   12 ++++++++++++
 drivers/mmc/core/host.c  |   39 +++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |    3 +++
 3 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index c0b41e8..434ee2d 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -18,6 +18,18 @@ config MMC_DEBUG
 	  This is an option for use by developers; most people should
 	  say N here.  This enables MMC core and driver debugging.
 
+config MMC_DEBUG_FS
+	bool "Debugging information files in debugfs"
+	depends on MMC && DEBUG_FS
+	help
+	  Enable this option to export some of the internal MMC state
+	  through files under /sys/kernel/debug/. This may help
+	  troubleshooting a buggy driver or when you're bringing up a
+	  driver on a new board.
+
+	  This debug option is relatively unintrusive, but most people
+	  don't need this. If in doubt, say N.
+
 if MMC
 
 source "drivers/mmc/core/Kconfig"
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 1d795c5..93da502 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -22,6 +22,41 @@
 #include "core.h"
 #include "host.h"
 
+#ifdef CONFIG_MMC_DEBUG_FS
+#include <linux/debugfs.h>
+
+static void mmc_add_host_debugfs(struct mmc_host *host)
+{
+	struct dentry *root;
+
+	root = debugfs_create_dir(mmc_hostname(host), NULL);
+	if (IS_ERR(root) || !root)
+		goto err_root;
+	host->debugfs_root = root;
+
+	return;
+
+err_root:
+	dev_err(&host->class_dev, "failed to initialize debugfs\n");
+}
+
+static void mmc_remove_host_debugfs(struct mmc_host *host)
+{
+	debugfs_remove(host->debugfs_root);
+}
+
+#else
+static inline void mmc_add_host_debugfs(struct mmc_host *host)
+{
+
+}
+
+static inline void mmc_remove_host_debugfs(struct mmc_host *host)
+{
+
+}
+#endif
+
 #define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
 static void mmc_host_classdev_release(struct device *dev)
@@ -127,6 +162,8 @@ int mmc_add_host(struct mmc_host *host)
 	if (err)
 		return err;
 
+	mmc_add_host_debugfs(host);
+
 	mmc_start_host(host);
 
 	return 0;
@@ -146,6 +183,8 @@ void mmc_remove_host(struct mmc_host *host)
 {
 	mmc_stop_host(host);
 
+	mmc_remove_host_debugfs(host);
+
 	device_del(&host->class_dev);
 
 	led_trigger_unregister_simple(host->led);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 7ab962f..31002e7 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -134,6 +134,9 @@ struct mmc_host {
 #ifdef CONFIG_LEDS_TRIGGERS
 	struct led_trigger	*led;		/* activity led */
 #endif
+#ifdef CONFIG_MMC_DEBUG_FS
+	struct dentry		*debugfs_root;
+#endif
 
 	unsigned long		private[0] ____cacheline_aligned;
 };
-- 
1.5.5.4

[PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

From: Haavard Skinnemoen <hskinnemoen@atmel.com>

Export all the fields in struct mmc_ios under /sys/kernel/debug/<host>/ios

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/mmc/core/host.c  |   55 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |   13 +++++++++++
 2 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 93da502..2977f26 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -25,6 +25,54 @@
 #ifdef CONFIG_MMC_DEBUG_FS
 #include <linux/debugfs.h>
 
+static void mmc_remove_ios_debugfs(struct mmc_ios *ios)
+{
+	if (ios->dbg_root) {
+		debugfs_remove(ios->dbg_clock);
+		debugfs_remove(ios->dbg_vdd);
+		debugfs_remove(ios->dbg_bus_mode);
+		debugfs_remove(ios->dbg_chip_select);
+		debugfs_remove(ios->dbg_power_mode);
+		debugfs_remove(ios->dbg_bus_width);
+		debugfs_remove(ios->dbg_timing);
+		debugfs_remove(ios->dbg_root);
+		ios->dbg_root = NULL;
+	}
+}
+
+static int mmc_add_ios_debugfs(struct mmc_ios *ios, struct dentry *parent)
+{
+	struct dentry *dir;
+
+	dir = debugfs_create_dir("ios", parent);
+	if (!dir)
+		return -EBUSY;	/* or whatever */
+	ios->dbg_root = dir;
+
+	ios->dbg_clock = debugfs_create_u32("clock", 0400, dir, &ios->clock);
+	ios->dbg_vdd = debugfs_create_u16("vdd", 0400, dir, &ios->vdd);
+	ios->dbg_bus_mode = debugfs_create_u8("bus_mode", 0400, dir,
+					      &ios->bus_mode);
+	ios->dbg_chip_select = debugfs_create_u8("chip_select", 0400, dir,
+						 &ios->chip_select);
+	ios->dbg_power_mode = debugfs_create_u8("power_mode", 0400, dir,
+						&ios->power_mode);
+	ios->dbg_bus_width = debugfs_create_u8("bus_width", 0400, dir,
+					       &ios->bus_width);
+	ios->dbg_timing = debugfs_create_u8("timing", 0400, dir, &ios->timing);
+
+	if (!ios->dbg_clock || !ios->dbg_vdd || !ios->dbg_bus_mode
+			|| !ios->dbg_chip_select || !ios->dbg_power_mode
+			|| !ios->dbg_bus_width || !ios->dbg_timing)
+		goto err;
+
+	return 0;
+
+err:
+	mmc_remove_ios_debugfs(ios);
+	return -EBUSY;
+}
+
 static void mmc_add_host_debugfs(struct mmc_host *host)
 {
 	struct dentry *root;
@@ -34,14 +82,21 @@ static void mmc_add_host_debugfs(struct mmc_host *host)
 		goto err_root;
 	host->debugfs_root = root;
 
+	if (mmc_add_ios_debugfs(&host->ios, root))
+		goto err_ios;
+
 	return;
 
+err_ios:
+	debugfs_remove(root);
+	host->debugfs_root = NULL;
 err_root:
 	dev_err(&host->class_dev, "failed to initialize debugfs\n");
 }
 
 static void mmc_remove_host_debugfs(struct mmc_host *host)
 {
+	mmc_remove_ios_debugfs(&host->ios);
 	debugfs_remove(host->debugfs_root);
 }
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 31002e7..1ba2723 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -47,6 +47,17 @@ struct mmc_ios {
 #define MMC_TIMING_LEGACY	0
 #define MMC_TIMING_MMC_HS	1
 #define MMC_TIMING_SD_HS	2
+
+#ifdef CONFIG_MMC_DEBUG_FS
+	struct dentry		*dbg_root;
+	struct dentry		*dbg_clock;
+	struct dentry		*dbg_vdd;
+	struct dentry		*dbg_bus_mode;
+	struct dentry		*dbg_chip_select;
+	struct dentry		*dbg_power_mode;
+	struct dentry		*dbg_bus_width;
+	struct dentry		*dbg_timing;
+#endif
 };
 
 struct mmc_host_ops {
@@ -136,6 +147,8 @@ struct mmc_host {
 #endif
 #ifdef CONFIG_MMC_DEBUG_FS
 	struct dentry		*debugfs_root;
+	struct dentry		*debugfs_ios;
+	struct dentry		*debugfs_ios_clock;
 #endif
 
 	unsigned long		private[0] ____cacheline_aligned;
-- 
1.5.5.4

[PATCH 3/3] mmc: Add per-card debugfs support

[Haavard Skinnemoen]

For each card successfully added to the bus, create a subdirectory under
the host's debugfs root with information about the card.

At the moment, only a single file is added to the card directory:
"status". Reading this file will ask the card about its current status
and return it. This can be useful if the card just refuses to respond to
any commands, which might indicate that the card state is not what the
MMC core thinks it is (due to a missing stop command, for example.)

Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 drivers/mmc/core/bus.c   |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/card.h |    4 ++
 2 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index fd95b18..04978c5 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -24,6 +24,73 @@
 #define dev_to_mmc_card(d)	container_of(d, struct mmc_card, dev)
 #define to_mmc_driver(d)	container_of(d, struct mmc_driver, drv)
 
+#ifdef CONFIG_MMC_DEBUG_FS
+#include <linux/debugfs.h>
+#include "mmc_ops.h"
+
+static int mmc_dbg_card_status_get(void *data, u64 *val)
+{
+	struct mmc_card	*card = data;
+	u32		status;
+	int		ret;
+
+	mmc_claim_host(card->host);
+
+	ret = mmc_send_status(data, &status);
+	if (!ret)
+		*val = status;
+
+	mmc_release_host(card->host);
+
+	return ret;
+}
+DEFINE_SIMPLE_ATTRIBUTE(mmc_dbg_card_status_fops, mmc_dbg_card_status_get,
+		NULL, "%08llx\n");
+
+static void mmc_add_card_debugfs(struct mmc_card *card)
+{
+	struct mmc_host	*host = card->host;
+	struct dentry	*root;
+
+	if (!host->debugfs_root)
+		return;
+
+	root = debugfs_create_dir(mmc_card_id(card), host->debugfs_root);
+	if (IS_ERR(root) || !root)
+		goto err_root;
+	card->dbg_root = root;
+
+	card->dbg_status = debugfs_create_file("status", 0400, root, card,
+				&mmc_dbg_card_status_fops);
+	if (!card->dbg_status)
+		goto err_status;
+
+	return;
+
+err_status:
+	debugfs_remove(root);
+	card->dbg_root = root;
+err_root:
+	dev_err(&card->dev, "failed to initialize debugfs\n");
+}
+
+static void mmc_remove_card_debugfs(struct mmc_card *card)
+{
+	debugfs_remove(card->dbg_status);
+	debugfs_remove(card->dbg_root);
+}
+#else
+static inline void mmc_add_card_debugfs(struct mmc_card *card)
+{
+
+}
+
+static inline void mmc_remove_card_debugfs(struct mmc_card *card)
+{
+
+}
+#endif
+
 static ssize_t mmc_type_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
@@ -252,6 +319,8 @@ int mmc_add_card(struct mmc_card *card)
 	if (ret)
 		return ret;
 
+	mmc_add_card_debugfs(card);
+
 	mmc_card_set_present(card);
 
 	return 0;
@@ -263,6 +332,8 @@ int mmc_add_card(struct mmc_card *card)
  */
 void mmc_remove_card(struct mmc_card *card)
 {
+	mmc_remove_card_debugfs(card);
+
 	if (mmc_card_present(card)) {
 		if (mmc_host_is_spi(card->host)) {
 			printk(KERN_INFO "%s: SPI card removed\n",
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 0d508ac..3d2bff8 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -111,6 +111,10 @@ struct mmc_card {
 	unsigned		num_info;	/* number of info strings */
 	const char		**info;		/* info strings */
 	struct sdio_func_tuple	*tuples;	/* unknown common tuples */
+#ifdef CONFIG_MMC_DEBUG_FS
+	struct dentry		*dbg_root;
+	struct dentry		*dbg_status;
+#endif
 };
 
 #define mmc_card_mmc(c)		((c)->type == MMC_TYPE_MMC)
-- 
1.5.5.4

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Pierre Ossman]

On Thu, 26 Jun 2008 13:09:49 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> For each card successfully added to the bus, create a subdirectory under
> the host's debugfs root with information about the card.
> 
> At the moment, only a single file is added to the card directory:
> "status". Reading this file will ask the card about its current status
> and return it. This can be useful if the card just refuses to respond to
> any commands, which might indicate that the card state is not what the
> MMC core thinks it is (due to a missing stop command, for example.)
> 
> Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> ---

The status command doesn't work on SDIO cards, so this seems like the
wrong place for it.

> +#else
> +static inline void mmc_add_card_debugfs(struct mmc_card *card)
> +{
> +
> +}
> +
> +static inline void mmc_remove_card_debugfs(struct mmc_card *card)
> +{
> +
> +}
> +#endif
> +

ifdef the calls here as well.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> The status command doesn't work on SDIO cards, so this seems like the
> wrong place for it.

Where do you want it then?

Btw, /sys/class/mmc_host/mmc0/<card>/test doesn't work on SDIO cards
either:

mmc0: Starting tests of card mmc0:0001...
mmc0: Test case 1. Basic write (no data verification)...
mmc0: Result: ERROR (-110)
mmc0: Test case 2. Basic read (no data verification)...
mmc0: Result: ERROR (-110)
mmc0: Test case 3. Basic write (with data verification)...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 4. Basic read (with data verification)...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 5. Multi-block write...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 6. Multi-block read...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 7. Power of two block writes...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 8. Power of two block reads...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 9. Weird sized block writes...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 10. Weird sized block reads...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 11. Badly aligned write...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 12. Badly aligned read...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 13. Badly aligned multi-block write...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 14. Badly aligned multi-block read...
mmc0: Result: Prepare stage failed! (-110)
mmc0: Test case 15. Correct xfer_size at write (start failure)...
mmc0: Result: ERROR (-110)
mmc0: Test case 16. Correct xfer_size at read (start failure)...
mmc0: Result: ERROR (-110)
mmc0: Test case 17. Correct xfer_size at write (midway failure)...
mmc0: Result: ERROR (-110)
mmc0: Test case 18. Correct xfer_size at read (midway failure)...
mmc0: Result: ERROR (-110)
mmc0: Tests completed.

Haavard

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Pierre Ossman]

On Sat, 28 Jun 2008 15:48:44 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > The status command doesn't work on SDIO cards, so this seems like the
> > wrong place for it.
> 
> Where do you want it then?
> 

drivers/mmc/core/mmc.c seems like the correct place (and some coupling
from sd.c as well). See if you can do something that's similar to how
sysfs nodes are handled by the bus handlers.

> Btw, /sys/class/mmc_host/mmc0/<card>/test doesn't work on SDIO cards
> either:
> 

Ooops. It shouldn't be binding to non-storage cards. I'll make sure to
have that fixed.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> On Sat, 28 Jun 2008 15:48:44 +0200
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> 
> > Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > > The status command doesn't work on SDIO cards, so this seems like the
> > > wrong place for it.
> > 
> > Where do you want it then?
> > 
> 
> drivers/mmc/core/mmc.c seems like the correct place (and some coupling
> from sd.c as well). See if you can do something that's similar to how
> sysfs nodes are handled by the bus handlers.

Hmm...I thought card.c seemed like a good place for card-specific debug
information. Even though this particular attribute isn't relevant to
some types of cards, is that a reason to create the whole directory
elsewhere and add complicated dependencies between files?

How about I check card->type and just not create the "status" attribute
for SDIO cards?

Haavard

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Pierre Ossman]

On Sat, 28 Jun 2008 16:15:24 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > On Sat, 28 Jun 2008 15:48:44 +0200
> > Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> > 
> > > Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > > > The status command doesn't work on SDIO cards, so this seems like the
> > > > wrong place for it.
> > > 
> > > Where do you want it then?
> > > 
> > 
> > drivers/mmc/core/mmc.c seems like the correct place (and some coupling
> > from sd.c as well). See if you can do something that's similar to how
> > sysfs nodes are handled by the bus handlers.
> 
> Hmm...I thought card.c seemed like a good place for card-specific debug
> information. Even though this particular attribute isn't relevant to
> some types of cards, is that a reason to create the whole directory
> elsewhere and add complicated dependencies between files?
> 

The directory might be suitable there, just not the "status" file. The
MMC code used to be a horrible mess of "if":s, "but":s and "when":s in
order to handle the crappy details of MMC vs SD. I'd like to avoid
going back to that nightmare as much as possible. The layering can
never be perfect, but right now it's at least just core.c that needs to
know about the different systems.

An alternative to sticking it into mmc.c is to create a debugfs.c that
contains all the uglyness. Debugging code isn't quite as important to
keep crystal clear.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > Hmm...I thought card.c seemed like a good place for card-specific debug

That should be bus.c of course...

> > information. Even though this particular attribute isn't relevant to
> > some types of cards, is that a reason to create the whole directory
> > elsewhere and add complicated dependencies between files?
> >   
> 
> The directory might be suitable there, just not the "status" file. The
> MMC code used to be a horrible mess of "if":s, "but":s and "when":s in
> order to handle the crappy details of MMC vs SD. I'd like to avoid
> going back to that nightmare as much as possible. The layering can
> never be perfect, but right now it's at least just core.c that needs to
> know about the different systems.
> 
> An alternative to sticking it into mmc.c is to create a debugfs.c that
> contains all the uglyness. Debugging code isn't quite as important to
> keep crystal clear.

But if we're going to rely on gcc optimizing the whole thing away when
debugfs is disabled, we must keep these buggers in the same file as its
caller...

Though I suppose an extra function call that does nothing at host and
card registration time isn't a very high price to pay for cleaner code.

Haavard

Re: [PATCH 3/3] mmc: Add per-card debugfs support

[Pierre Ossman]

On Sat, 28 Jun 2008 17:36:37 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > 
> > An alternative to sticking it into mmc.c is to create a debugfs.c that
> > contains all the uglyness. Debugging code isn't quite as important to
> > keep crystal clear.
> 
> But if we're going to rely on gcc optimizing the whole thing away when
> debugfs is disabled, we must keep these buggers in the same file as its
> caller...
> 
> Though I suppose an extra function call that does nothing at host and
> card registration time isn't a very high price to pay for cleaner code.
> 

This is in the device addition/removal code as well. Hardly a hotpath.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Pierre Ossman]

On Thu, 26 Jun 2008 13:09:48 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> From: Haavard Skinnemoen <hskinnemoen@atmel.com>
> 
> Export all the fields in struct mmc_ios under /sys/kernel/debug/<host>/ios
> 
> Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
> ---
>  drivers/mmc/core/host.c  |   55 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h |   13 +++++++++++
>  2 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 93da502..2977f26 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -25,6 +25,54 @@
>  #ifdef CONFIG_MMC_DEBUG_FS
>  #include <linux/debugfs.h>
>  
> +static void mmc_remove_ios_debugfs(struct mmc_ios *ios)
> +{
> +	if (ios->dbg_root) {

You can save a bit of indentation if you do:

	if (!ios->dbg_root)
		return;

> +static int mmc_add_ios_debugfs(struct mmc_ios *ios, struct dentry *parent)
> +{
> +	struct dentry *dir;
> +
> +	dir = debugfs_create_dir("ios", parent);
> +	if (!dir)
> +		return -EBUSY;	/* or whatever */

Is it undefined what a NULL return means here? I would have expected an
ERRPTR or ENOMEM.

> +#ifdef CONFIG_MMC_DEBUG_FS
> +	struct dentry		*dbg_root;
> +	struct dentry		*dbg_clock;
> +	struct dentry		*dbg_vdd;
> +	struct dentry		*dbg_bus_mode;
> +	struct dentry		*dbg_chip_select;
> +	struct dentry		*dbg_power_mode;
> +	struct dentry		*dbg_bus_width;
> +	struct dentry		*dbg_timing;
> +#endif

Can't we use debugfs' own bookkeeping to keep track of them? Saves us a
lot of noise in these structures.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> On Thu, 26 Jun 2008 13:09:48 +0200
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> > +static void mmc_remove_ios_debugfs(struct mmc_ios *ios)
> > +{
> > +	if (ios->dbg_root) {
> 
> You can save a bit of indentation if you do:
> 
> 	if (!ios->dbg_root)
> 		return;

Good point.

> > +static int mmc_add_ios_debugfs(struct mmc_ios *ios, struct dentry *parent)
> > +{
> > +	struct dentry *dir;
> > +
> > +	dir = debugfs_create_dir("ios", parent);
> > +	if (!dir)
> > +		return -EBUSY;	/* or whatever */
> 
> Is it undefined what a NULL return means here? I would have expected an
> ERRPTR or ENOMEM.

Yeah...debugfs_create_dir() may return an ERRPTR only if debugfs is
disabled. All other errors return NULL.

We wouldn't be here unless the caller managed to create the host
directory, so checking IS_ERR() is redundant. Maybe ENOMEM would make
more sense, but I could also be that the directory already exists,
hence EBUSY is appropriate. We really don't know as long as debugfs
keeps us in the dark...

> > +#ifdef CONFIG_MMC_DEBUG_FS
> > +	struct dentry		*dbg_root;
> > +	struct dentry		*dbg_clock;
> > +	struct dentry		*dbg_vdd;
> > +	struct dentry		*dbg_bus_mode;
> > +	struct dentry		*dbg_chip_select;
> > +	struct dentry		*dbg_power_mode;
> > +	struct dentry		*dbg_bus_width;
> > +	struct dentry		*dbg_timing;
> > +#endif
> 
> Can't we use debugfs' own bookkeeping to keep track of them? Saves us a
> lot of noise in these structures.

You mean d_subdirs in struct dentry? I guess we could do that...though
I was sort of trying not to dig too deply into VFS internals...

Haavard

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Pierre Ossman]

On Sat, 28 Jun 2008 15:47:00 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > 
> > Can't we use debugfs' own bookkeeping to keep track of them? Saves us a
> > lot of noise in these structures.
> 
> You mean d_subdirs in struct dentry? I guess we could do that...though
> I was sort of trying not to dig too deply into VFS internals...
> 

I'm a complete noob when it comes to debugfs. There isn't some
recursive delete function that can be used on the "ios" node?

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> On Sat, 28 Jun 2008 15:47:00 +0200
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> 
> > Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > > 
> > > Can't we use debugfs' own bookkeeping to keep track of them? Saves us a
> > > lot of noise in these structures.
> > 
> > You mean d_subdirs in struct dentry? I guess we could do that...though
> > I was sort of trying not to dig too deply into VFS internals...
> > 
> 
> I'm a complete noob when it comes to debugfs. There isn't some
> recursive delete function that can be used on the "ios" node?

I don't think so, but I think it would be very useful. I sometimes
forget to remove an entry so that I have to reboot in order to get my
driver's debugfs interface going again..

Greg, do you have any suggestions?

Haavard

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Greg KH]

On Sat, Jun 28, 2008 at 04:07:52PM +0200, Haavard Skinnemoen wrote:
> Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > On Sat, 28 Jun 2008 15:47:00 +0200
> > Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> > 
> > > Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > > > 
> > > > Can't we use debugfs' own bookkeeping to keep track of them? Saves us a
> > > > lot of noise in these structures.
> > > 
> > > You mean d_subdirs in struct dentry? I guess we could do that...though
> > > I was sort of trying not to dig too deply into VFS internals...
> > > 
> > 
> > I'm a complete noob when it comes to debugfs. There isn't some
> > recursive delete function that can be used on the "ios" node?
> 
> I don't think so, but I think it would be very useful. I sometimes
> forget to remove an entry so that I have to reboot in order to get my
> driver's debugfs interface going again..
> 
> Greg, do you have any suggestions?

Someone can add a "debugfs_rm_dentry_and_children" function if they so
desire to handle this kind of thing if they want to :)

thanks,

greg k-h

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

Greg KH <greg@kroah.com> wrote:
> Someone can add a "debugfs_rm_dentry_and_children" function if they so
> desire to handle this kind of thing if they want to :)

Ok, I'll try. I'm not an FS developer, so I'm sure the below patch is
horribly broken in some subtle way, but it does appear to work at least
the few times I tried it...

Lockdep indeed confirms that something is broken:

=============================================
[ INFO: possible recursive locking detected ]
2.6.26-rc8 #34
---------------------------------------------
rmmod/396 is trying to acquire lock:
 (&sb->s_type->i_mutex_key#2){--..}, at: [<900a4ab6>] debugfs_remove_recursive+0x2e/0xc0

but task is already holding lock:
 (&sb->s_type->i_mutex_key#2){--..}, at: [<900a4aa8>] debugfs_remove_recursive+0x20/0xc0

other info that might help us debug this:
1 lock held by rmmod/396:
 #0:  (&sb->s_type->i_mutex_key#2){--..}, at: [<900a4aa8>] debugfs_remove_recursive+0x20/0xc0

stack backtrace:
Call trace:
 [<90017018>] dump_stack+0x18/0x20
 [<900327ac>] __lock_acquire+0x6a0/0x964
 [<9003306c>] lock_acquire+0x38/0x4c
 [<90170130>] mutex_lock_nested+0x7c/0x18c
 [<900a4ab6>] debugfs_remove_recursive+0x2e/0xc0
 [<c0861c26>] mmc_remove_host+0x12/0x34 [mmc_core]
 [<c0857222>] atmci_remove+0x4a/0xd4 [atmel_mci]
 [<900d478a>] platform_drv_remove+0x10/0x12
 [<900d3cfc>] __device_release_driver+0x48/0x64
 [<900d3d74>] driver_detach+0x5c/0x7c
 [<900d34be>] bus_remove_driver+0x5a/0x70
 [<900d4054>] driver_unregister+0x24/0x28
 [<900d488e>] platform_driver_unregister+0xa/0xc
 [<c08571ce>] atmci_exit+0xa/0x14 [atmel_mci]
 [<90038086>] sys_delete_module+0x116/0x15c
 [<90013132>] syscall_return+0x0/0x12

But I don't know exactly what. Isn't it ok to take the inode lock in
parent -> child order?

Haavard

>From 5b8c84b71456de1cfc37910c40c974e3459f39cf Mon Sep 17 00:00:00 2001
From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Sat, 28 Jun 2008 17:54:15 +0200
Subject: [PATCH] debugfs: Implement debugfs_remove_recursive()

debugfs_remove_recursive() will remove a dentry and all its children.
Drivers can use this to zap their whole debugfs tree so that they don't
need to keep track of every single debugfs dentry they created.

It may fail to remove the whole tree in certain cases:

sh-3.2# rmmod atmel-mci < /sys/kernel/debug/mmc0/ios/clock
mmc0: card b368 removed
atmel_mci atmel_mci.0: Lost dma0chan1, falling back to PIO
sh-3.2# ls /sys/kernel/debug/mmc0/
ios

But I'm not sure if that case can be handled in any sane manner.

Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 fs/debugfs/inode.c      |  114 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/debugfs.h |    4 ++
 2 files changed, 101 insertions(+), 17 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index e9602d8..6234322 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -309,6 +309,31 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_symlink);
 
+static void __debugfs_remove(struct dentry *dentry, struct dentry *parent)
+{
+	int ret = 0;
+
+	if (debugfs_positive(dentry)) {
+		if (dentry->d_inode) {
+			dget(dentry);
+			switch (dentry->d_inode->i_mode & S_IFMT) {
+			case S_IFDIR:
+				ret = simple_rmdir(parent->d_inode, dentry);
+				break;
+			case S_IFLNK:
+				kfree(dentry->d_inode->i_private);
+				/* fall through */
+			default:
+				simple_unlink(parent->d_inode, dentry);
+				break;
+			}
+			if (!ret)
+				d_delete(dentry);
+			dput(dentry);
+		}
+	}
+}
+
 /**
  * debugfs_remove - removes a file or directory from the debugfs filesystem
  * @dentry: a pointer to a the dentry of the file or directory to be
@@ -325,7 +350,6 @@ EXPORT_SYMBOL_GPL(debugfs_create_symlink);
 void debugfs_remove(struct dentry *dentry)
 {
 	struct dentry *parent;
-	int ret = 0;
 	
 	if (!dentry)
 		return;
@@ -335,29 +359,85 @@ void debugfs_remove(struct dentry *dentry)
 		return;
 
 	mutex_lock(&parent->d_inode->i_mutex);
-	if (debugfs_positive(dentry)) {
-		if (dentry->d_inode) {
-			dget(dentry);
-			switch (dentry->d_inode->i_mode & S_IFMT) {
-			case S_IFDIR:
-				ret = simple_rmdir(parent->d_inode, dentry);
-				break;
-			case S_IFLNK:
-				kfree(dentry->d_inode->i_private);
-				/* fall through */
-			default:
-				simple_unlink(parent->d_inode, dentry);
+	__debugfs_remove(dentry, parent);
+	mutex_unlock(&parent->d_inode->i_mutex);
+	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+}
+EXPORT_SYMBOL_GPL(debugfs_remove);
+
+/**
+ * debugfs_remove_recursive - recursively removes a directory
+ * @dentry: a pointer to a the dentry of the directory to be removed.
+ *
+ * This function recursively removes a directory tree in debugfs that
+ * was previously created with a call to another debugfs function
+ * (like debugfs_create_file() or variants thereof.)
+ *
+ * This function is required to be called in order for the file to be
+ * removed, no automatic cleanup of files will happen when a module is
+ * removed, you are responsible here.
+ */
+void debugfs_remove_recursive(struct dentry *dentry)
+{
+	struct dentry *child;
+	struct dentry *parent;
+
+	if (!dentry)
+		return;
+
+	parent = dentry->d_parent;
+	if (!parent || !parent->d_inode)
+		return;
+
+	mutex_lock(&parent->d_inode->i_mutex);
+	mutex_lock(&dentry->d_inode->i_mutex);
+	parent = dentry;
+
+	while (1) {
+		/*
+		 * When all dentries under "parent" has been removed,
+		 * walk up the tree until we reach our starting point.
+		 */
+		if (list_empty(&parent->d_subdirs)) {
+			mutex_unlock(&parent->d_inode->i_mutex);
+			if (parent == dentry)
 				break;
+			parent = parent->d_parent;
+		}
+		child = list_entry(parent->d_subdirs.next, struct dentry,
+				d_u.d_child);
+
+		/*
+		 * If "child" isn't empty, walk down the tree and
+		 * remove all its descendants first.
+		 */
+		if (!list_empty(&child->d_subdirs)) {
+			parent = child;
+			mutex_lock(&parent->d_inode->i_mutex);
+			continue;
+		}
+		__debugfs_remove(child, parent);
+		if (parent->d_subdirs.next == &child->d_u.d_child) {
+			/*
+			 * Avoid infinite loop if we fail to remove
+			 * one dentry.
+			 */
+			while (parent != dentry) {
+				mutex_unlock(&parent->d_inode->i_mutex);
+				parent = parent->d_parent;
 			}
-			if (!ret)
-				d_delete(dentry);
-			dput(dentry);
+			mutex_unlock(&parent->d_inode->i_mutex);
+			break;
 		}
+		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 	}
+
+	parent = dentry->d_parent;
+	__debugfs_remove(dentry, parent);
 	mutex_unlock(&parent->d_inode->i_mutex);
 	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 }
-EXPORT_SYMBOL_GPL(debugfs_remove);
+EXPORT_SYMBOL_GPL(debugfs_remove_recursive);
 
 /**
  * debugfs_rename - rename a file/directory in the debugfs filesystem
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 7266124..b67b375 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -42,6 +42,7 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 				      const char *dest);
 
 void debugfs_remove(struct dentry *dentry);
+void debugfs_remove_recursive(struct dentry *dentry);
 
 struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, const char *new_name);
@@ -99,6 +100,9 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
 static inline void debugfs_remove(struct dentry *dentry)
 { }
 
+static inline void debugfs_remove_recursive(struct dentry *dentry)
+{ }
+
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, char *new_name)
 {
-- 
1.5.5.4

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> Ok, I'll try. I'm not an FS developer, so I'm sure the below patch is
> horribly broken in some subtle way, but it does appear to work at least
> the few times I tried it...
> 
> Lockdep indeed confirms that something is broken:

Another try. This time I'm taking only one lock at a time, so lockdep
is happy. Is it still safe?

Haavard

>From ee72de0df9a471eb7fbab7827606a2b9a6afe19c Mon Sep 17 00:00:00 2001
From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Sat, 28 Jun 2008 17:54:15 +0200
Subject: [PATCH] debugfs: Implement debugfs_remove_recursive()

debugfs_remove_recursive() will remove a dentry and all its children.
Drivers can use this to zap their whole debugfs tree so that they don't
need to keep track of every single debugfs dentry they created.

It may fail to remove the whole tree in certain cases:

sh-3.2# rmmod atmel-mci < /sys/kernel/debug/mmc0/ios/clock
mmc0: card b368 removed
atmel_mci atmel_mci.0: Lost dma0chan1, falling back to PIO
sh-3.2# ls /sys/kernel/debug/mmc0/
ios

But I'm not sure if that case can be handled in any sane manner.

Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 fs/debugfs/inode.c      |  114 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/debugfs.h |    4 ++
 2 files changed, 100 insertions(+), 18 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index e9602d8..08e28c9 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -309,6 +309,31 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_symlink);
 
+static void __debugfs_remove(struct dentry *dentry, struct dentry *parent)
+{
+	int ret = 0;
+
+	if (debugfs_positive(dentry)) {
+		if (dentry->d_inode) {
+			dget(dentry);
+			switch (dentry->d_inode->i_mode & S_IFMT) {
+			case S_IFDIR:
+				ret = simple_rmdir(parent->d_inode, dentry);
+				break;
+			case S_IFLNK:
+				kfree(dentry->d_inode->i_private);
+				/* fall through */
+			default:
+				simple_unlink(parent->d_inode, dentry);
+				break;
+			}
+			if (!ret)
+				d_delete(dentry);
+			dput(dentry);
+		}
+	}
+}
+
 /**
  * debugfs_remove - removes a file or directory from the debugfs filesystem
  * @dentry: a pointer to a the dentry of the file or directory to be
@@ -325,7 +350,6 @@ EXPORT_SYMBOL_GPL(debugfs_create_symlink);
 void debugfs_remove(struct dentry *dentry)
 {
 	struct dentry *parent;
-	int ret = 0;
 	
 	if (!dentry)
 		return;
@@ -335,29 +359,83 @@ void debugfs_remove(struct dentry *dentry)
 		return;
 
 	mutex_lock(&parent->d_inode->i_mutex);
-	if (debugfs_positive(dentry)) {
-		if (dentry->d_inode) {
-			dget(dentry);
-			switch (dentry->d_inode->i_mode & S_IFMT) {
-			case S_IFDIR:
-				ret = simple_rmdir(parent->d_inode, dentry);
-				break;
-			case S_IFLNK:
-				kfree(dentry->d_inode->i_private);
-				/* fall through */
-			default:
-				simple_unlink(parent->d_inode, dentry);
+	__debugfs_remove(dentry, parent);
+	mutex_unlock(&parent->d_inode->i_mutex);
+	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+}
+EXPORT_SYMBOL_GPL(debugfs_remove);
+
+/**
+ * debugfs_remove_recursive - recursively removes a directory
+ * @dentry: a pointer to a the dentry of the directory to be removed.
+ *
+ * This function recursively removes a directory tree in debugfs that
+ * was previously created with a call to another debugfs function
+ * (like debugfs_create_file() or variants thereof.)
+ *
+ * This function is required to be called in order for the file to be
+ * removed, no automatic cleanup of files will happen when a module is
+ * removed, you are responsible here.
+ */
+void debugfs_remove_recursive(struct dentry *dentry)
+{
+	struct dentry *child;
+	struct dentry *parent;
+
+	if (!dentry)
+		return;
+
+	parent = dentry->d_parent;
+	if (!parent || !parent->d_inode)
+		return;
+
+	parent = dentry;
+	mutex_lock(&parent->d_inode->i_mutex);
+
+	while (1) {
+		/*
+		 * When all dentries under "parent" has been removed,
+		 * walk up the tree until we reach our starting point.
+		 */
+		if (list_empty(&parent->d_subdirs)) {
+			mutex_unlock(&parent->d_inode->i_mutex);
+			if (parent == dentry)
 				break;
-			}
-			if (!ret)
-				d_delete(dentry);
-			dput(dentry);
+			parent = parent->d_parent;
+			mutex_lock(&parent->d_inode->i_mutex);
+		}
+		child = list_entry(parent->d_subdirs.next, struct dentry,
+				d_u.d_child);
+
+		/*
+		 * If "child" isn't empty, walk down the tree and
+		 * remove all its descendants first.
+		 */
+		if (!list_empty(&child->d_subdirs)) {
+			mutex_unlock(&parent->d_inode->i_mutex);
+			parent = child;
+			mutex_lock(&parent->d_inode->i_mutex);
+			continue;
 		}
+		__debugfs_remove(child, parent);
+		if (parent->d_subdirs.next == &child->d_u.d_child) {
+			/*
+			 * Avoid infinite loop if we fail to remove
+			 * one dentry.
+			 */
+			mutex_unlock(&parent->d_inode->i_mutex);
+			break;
+		}
+		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 	}
+
+	parent = dentry->d_parent;
+	mutex_lock(&parent->d_inode->i_mutex);
+	__debugfs_remove(dentry, parent);
 	mutex_unlock(&parent->d_inode->i_mutex);
 	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 }
-EXPORT_SYMBOL_GPL(debugfs_remove);
+EXPORT_SYMBOL_GPL(debugfs_remove_recursive);
 
 /**
  * debugfs_rename - rename a file/directory in the debugfs filesystem
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 7266124..b67b375 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -42,6 +42,7 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 				      const char *dest);
 
 void debugfs_remove(struct dentry *dentry);
+void debugfs_remove_recursive(struct dentry *dentry);
 
 struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, const char *new_name);
@@ -99,6 +100,9 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
 static inline void debugfs_remove(struct dentry *dentry)
 { }
 
+static inline void debugfs_remove_recursive(struct dentry *dentry)
+{ }
+
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, char *new_name)
 {
-- 
1.5.5.4

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Greg KH]

On Sat, Jun 28, 2008 at 06:37:36PM +0200, Haavard Skinnemoen wrote:
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> > Ok, I'll try. I'm not an FS developer, so I'm sure the below patch is
> > horribly broken in some subtle way, but it does appear to work at least
> > the few times I tried it...
> > 
> > Lockdep indeed confirms that something is broken:
> 
> Another try. This time I'm taking only one lock at a time, so lockdep
> is happy. Is it still safe?

At first glance, this looks sane, thanks a lot.

> From ee72de0df9a471eb7fbab7827606a2b9a6afe19c Mon Sep 17 00:00:00 2001
> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> Date: Sat, 28 Jun 2008 17:54:15 +0200
> Subject: [PATCH] debugfs: Implement debugfs_remove_recursive()
> 
> debugfs_remove_recursive() will remove a dentry and all its children.
> Drivers can use this to zap their whole debugfs tree so that they don't
> need to keep track of every single debugfs dentry they created.
> 
> It may fail to remove the whole tree in certain cases:
> 
> sh-3.2# rmmod atmel-mci < /sys/kernel/debug/mmc0/ios/clock
> mmc0: card b368 removed
> atmel_mci atmel_mci.0: Lost dma0chan1, falling back to PIO
> sh-3.2# ls /sys/kernel/debug/mmc0/
> ios
> 
> But I'm not sure if that case can be handled in any sane manner.

I think we have always been failing this case as I know securityfs also
has this same issue, and the code base is pretty much identical.

So don't worry that this patch caused this issue.

I'll queue it up unless there are any other objections to it.

thanks,

greg k-h

Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

On Sat, 28 Jun 2008 09:43:43 -0700
Greg KH <greg@kroah.com> wrote:

> > sh-3.2# rmmod atmel-mci < /sys/kernel/debug/mmc0/ios/clock
> > mmc0: card b368 removed
> > atmel_mci atmel_mci.0: Lost dma0chan1, falling back to PIO
> > sh-3.2# ls /sys/kernel/debug/mmc0/
> > ios
> > 
> > But I'm not sure if that case can be handled in any sane manner.  
> 
> I think we have always been failing this case as I know securityfs also
> has this same issue, and the code base is pretty much identical.
> 
> So don't worry that this patch caused this issue.

Yeah, I'm pretty sure this issue was there before too, when the driver
kept track of things. I just never tested it.

The main reason I tested it now was to make sure it didn't hang, and it
didn't.

> I'll queue it up unless there are any other objections to it.

Thanks.

Haavard

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Pierre Ossman]

On Thu, 26 Jun 2008 13:09:47 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> From: Haavard Skinnemoen <hskinnemoen@atmel.com>
> 
> This adds a new config option, MMC_DEBUG_FS which will, when enabled,
> create a few files under /sys/kernel/debug containing information
> about an mmc host's internal state.
> 
> Host drivers can add additional files and directories under the host's
> root directory by passing the debugfs_root field in struct mmc_host as
> the 'parent' parameter to debugfs_create_*.
> 
> Unfinished: No files are actually created yet.
> 
> Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
> ---

Is there any point to having a separate option for this? Can't we just
include it if DEBUG_FS is defined?

Otherwise the system looks ok to me, assuming that everyone is of the
opinion that debugfs is _not_ a stable API.

> +
> +#else
> +static inline void mmc_add_host_debugfs(struct mmc_host *host)
> +{
> +
> +}
> +
> +static inline void mmc_remove_host_debugfs(struct mmc_host *host)
> +{
> +
> +}
> +#endif
> +

In the other places we have conditional function, it is done by
ifdef:ing the calls instead. There are just two of them, so it should
be less mess.


-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> On Thu, 26 Jun 2008 13:09:47 +0200
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> 
> > From: Haavard Skinnemoen <hskinnemoen@atmel.com>
> > 
> > This adds a new config option, MMC_DEBUG_FS which will, when enabled,
> > create a few files under /sys/kernel/debug containing information
> > about an mmc host's internal state.
> > 
> > Host drivers can add additional files and directories under the host's
> > root directory by passing the debugfs_root field in struct mmc_host as
> > the 'parent' parameter to debugfs_create_*.
> > 
> > Unfinished: No files are actually created yet.
> > 
> > Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
> > ---
> 
> Is there any point to having a separate option for this? Can't we just
> include it if DEBUG_FS is defined?

Sure. It doesn't introduce any additional overhead, just a bit of extra
code.

> Otherwise the system looks ok to me, assuming that everyone is of the
> opinion that debugfs is _not_ a stable API.

That's my assumption at least. Debugfs is just a place where you can
throw random stuff that may help you debugging stuff.

> > +
> > +#else
> > +static inline void mmc_add_host_debugfs(struct mmc_host *host)
> > +{
> > +
> > +}
> > +
> > +static inline void mmc_remove_host_debugfs(struct mmc_host *host)
> > +{
> > +
> > +}
> > +#endif
> > +
> 
> In the other places we have conditional function, it is done by
> ifdef:ing the calls instead. There are just two of them, so it should
> be less mess.

Hmm. You mean it's better with three #ifdefs than one?

Haavard

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Pierre Ossman]

On Sat, 28 Jun 2008 15:37:04 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > 
> > In the other places we have conditional function, it is done by
> > ifdef:ing the calls instead. There are just two of them, so it should
> > be less mess.
> 
> Hmm. You mean it's better with three #ifdefs than one?
> 

Fewer lines though. Those dummy function stick out like a sore thumb.
It's a decent trade-off when you have multiple callers, but here we have
just the one.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> Fewer lines though. Those dummy function stick out like a sore thumb.
> It's a decent trade-off when you have multiple callers, but here we have
> just the one.

Ok, I'll change it.

Haavard

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> > Hmm. You mean it's better with three #ifdefs than one?
> >   
> 
> Fewer lines though. Those dummy function stick out like a sore thumb.
> It's a decent trade-off when you have multiple callers, but here we have
> just the one.

Actually, if debugfs is disabled, debugfs_create_dir() and friends are
inline functions which return ERR_PTR(-ENODEV). So if we just remove
the whole #ifdef altogether, gcc should be smart enough to optimize
away the whole thing if debugfs isn't enabled...

I'll give it a try.

Haavard

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Haavard Skinnemoen]

Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> Actually, if debugfs is disabled, debugfs_create_dir() and friends are
> inline functions which return ERR_PTR(-ENODEV). So if we just remove
> the whole #ifdef altogether, gcc should be smart enough to optimize
> away the whole thing if debugfs isn't enabled...
> 
> I'll give it a try.

Yeah, it does seem to work, but it means we'll have to put at least
debugfs_root into struct mmc_host unconditionally. Assuming we find a
way to not put all the other stuff in there, is that acceptable?

Haavard

Re: [PATCH 1/3] mmc: Export internal host state through debugfs

[Pierre Ossman]

On Sat, 28 Jun 2008 16:41:46 +0200
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:

> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> > Actually, if debugfs is disabled, debugfs_create_dir() and friends are
> > inline functions which return ERR_PTR(-ENODEV). So if we just remove
> > the whole #ifdef altogether, gcc should be smart enough to optimize
> > away the whole thing if debugfs isn't enabled...
> > 
> > I'll give it a try.
> 
> Yeah, it does seem to work, but it means we'll have to put at least
> debugfs_root into struct mmc_host unconditionally. Assuming we find a
> way to not put all the other stuff in there, is that acceptable?
> 

Sounds good.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.