From: Haavard Skinnemoen <[email protected]>
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 <[email protected]>
---
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
From: Haavard Skinnemoen <[email protected]>
Export all the fields in struct mmc_ios under /sys/kernel/debug/<host>/ios
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, 26 Jun 2008 13:09:47 +0200
Haavard Skinnemoen <[email protected]> wrote:
> From: Haavard Skinnemoen <[email protected]>
>
> 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 <[email protected]>
> ---
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.
Pierre Ossman <[email protected]> wrote:
> On Thu, 26 Jun 2008 13:09:47 +0200
> Haavard Skinnemoen <[email protected]> wrote:
>
> > From: Haavard Skinnemoen <[email protected]>
> >
> > 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 <[email protected]>
> > ---
>
> 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
On Thu, 26 Jun 2008 13:09:48 +0200
Haavard Skinnemoen <[email protected]> wrote:
> From: Haavard Skinnemoen <[email protected]>
>
> Export all the fields in struct mmc_ios under /sys/kernel/debug/<host>/ios
>
> Signed-off-by: Haavard Skinnemoen <[email protected]>
> ---
> 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.
On Thu, 26 Jun 2008 13:09:49 +0200
Haavard Skinnemoen <[email protected]> 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 <[email protected]>
> ---
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.
On Sat, 28 Jun 2008 15:37:04 +0200
Haavard Skinnemoen <[email protected]> wrote:
> Pierre Ossman <[email protected]> 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.
Pierre Ossman <[email protected]> wrote:
> On Thu, 26 Jun 2008 13:09:48 +0200
> Haavard Skinnemoen <[email protected]> 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
Pierre Ossman <[email protected]> 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
Pierre Ossman <[email protected]> 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
On Sat, 28 Jun 2008 15:47:00 +0200
Haavard Skinnemoen <[email protected]> wrote:
> Pierre Ossman <[email protected]> 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.
On Sat, 28 Jun 2008 15:48:44 +0200
Haavard Skinnemoen <[email protected]> wrote:
> Pierre Ossman <[email protected]> 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.
Pierre Ossman <[email protected]> wrote:
> On Sat, 28 Jun 2008 15:47:00 +0200
> Haavard Skinnemoen <[email protected]> wrote:
>
> > Pierre Ossman <[email protected]> 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
Pierre Ossman <[email protected]> wrote:
> On Sat, 28 Jun 2008 15:48:44 +0200
> Haavard Skinnemoen <[email protected]> wrote:
>
> > Pierre Ossman <[email protected]> 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
Pierre Ossman <[email protected]> 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
Haavard Skinnemoen <[email protected]> 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
On Sat, 28 Jun 2008 16:41:46 +0200
Haavard Skinnemoen <[email protected]> wrote:
> Haavard Skinnemoen <[email protected]> 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.
On Sat, 28 Jun 2008 16:15:24 +0200
Haavard Skinnemoen <[email protected]> wrote:
> Pierre Ossman <[email protected]> wrote:
> > On Sat, 28 Jun 2008 15:48:44 +0200
> > Haavard Skinnemoen <[email protected]> wrote:
> >
> > > Pierre Ossman <[email protected]> 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.
On Sat, Jun 28, 2008 at 04:07:52PM +0200, Haavard Skinnemoen wrote:
> Pierre Ossman <[email protected]> wrote:
> > On Sat, 28 Jun 2008 15:47:00 +0200
> > Haavard Skinnemoen <[email protected]> wrote:
> >
> > > Pierre Ossman <[email protected]> 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
Pierre Ossman <[email protected]> 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
Greg KH <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
On Sat, 28 Jun 2008 17:36:37 +0200
Haavard Skinnemoen <[email protected]> wrote:
> Pierre Ossman <[email protected]> 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.
Haavard Skinnemoen <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
On Sat, Jun 28, 2008 at 06:37:36PM +0200, Haavard Skinnemoen wrote:
> Haavard Skinnemoen <[email protected]> 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 <[email protected]>
> 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
On Sat, 28 Jun 2008 09:43:43 -0700
Greg KH <[email protected]> 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