shdma: (or tmio_mmc?) inconsistent lock state detected on ag5evm
Hello, Guennadi.
This was started as an evaluation of your patch
[PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts
disabled
But now I don't know where exactly the issue is. So, let me begin new thread.
On ag5evm(the board under arch/arm/mach-shmobile, that is SMP), both
tag v3.0 + patch above, and
tag v3.0 + cjb/mmc-next
with CONFIG_PROVE_LOCKING=y, inconsistent lock state is detected as text
attached at
the bottom.
Function itself(mount/read/write/umount) seems working without problem, so far.
As it happens on the spinlock at sh_dmae_interrupt(), it seems to have been
there since
2dc7 dmaengine: shdma: fix locking
which introduced the lock there.
I found deleting the spin_lock/unlock there makes it quiet. But that lock must
be the
important part of the patch. Actually, deleting it brings another locking issue
on
tmio_mmc, and it confuses me. I wonder what is the correct solution. I'm afraid
I can't
tell what is the critical object to be protected in shdma and tmio_mmc source
code...
Any suggestions are appreciated.
/yoshii
=
[ INFO: inconsistent lock state ]
3.0.0-00100-g82258ef-dirty #3643
-
inconsistent {HARDIRQ-ON-W} - {IN-HARDIRQ-W} usage.
swapper/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
((new_sh_chan-desc_lock)-rlock){?.-...}, at: [c0795fc4]
sh_dmae_interrupt+0x14/0x78
{HARDIRQ-ON-W} state was registered at:
[c0689b30] __lock_acquire+0x5c4/0xbb4
[c068a6f8] lock_acquire+0x60/0x74
[c088da7c] _raw_spin_lock_bh+0x3c/0x4c
[c0796edc] sh_dmae_alloc_chan_resources+0x1b0/0x298
[c0794ca8] dma_chan_get+0xd8/0x17c
[c0795560] __dma_request_channel+0x140/0x1e4
[c07e5850] tmio_mmc_request_dma+0x74/0x10c
[c0886b84] tmio_mmc_host_probe+0x208/0x284
[c0886d68] sh_mobile_sdhi_probe+0x168/0x28c
[c07bca4c] platform_drv_probe+0x18/0x1c
[c07bb5b8] driver_probe_device+0x7c/0x178
[c07bb748] __driver_attach+0x94/0x98
[c07badbc] bus_for_each_dev+0x60/0x8c
[c07ba5a4] bus_add_driver+0xa8/0x2a4
[c07bbd24] driver_register+0x78/0x18c
[c0633530] do_one_initcall+0x34/0x184
[c00083d8] kernel_init+0xa8/0x134
[c063a5a8] kernel_thread_exit+0x0/0x8
irq event stamp: 17556
hardirqs last enabled at (17553): [c063a64c] default_idle+0x24/0x2c
hardirqs last disabled at (17554): [c0639674] __irq_svc+0x34/0xa0
softirqs last enabled at (17556): [c065c878] irq_enter+0x78/0x7c
softirqs last disabled at (17555): [c065c86c] irq_enter+0x6c/0x7c
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock((new_sh_chan-desc_lock)-rlock);
Interrupt
lock((new_sh_chan-desc_lock)-rlock);
*** DEADLOCK ***
no locks held by swapper/0.
stack backtrace:
[c063f730] (unwind_backtrace+0x0/0xfc) from [c0686358]
(print_usage_bug+0x21c/0x2bc)
[c0686358] (print_usage_bug+0x21c/0x2bc) from [c0686908]
(mark_lock+0x510/0x740)
[c0686908] (mark_lock+0x510/0x740) from [c0689ba4]
(__lock_acquire+0x638/0xbb4)
[c0689ba4] (__lock_acquire+0x638/0xbb4) from [c068a6f8]
(lock_acquire+0x60/0x74)
[c068a6f8] (lock_acquire+0x60/0x74) from [c088d868]
(_raw_spin_lock+0x34/0x44)
[c088d868] (_raw_spin_lock+0x34/0x44) from [c0795fc4]
(sh_dmae_interrupt+0x14/0x78)
[c0795fc4] (sh_dmae_interrupt+0x14/0x78) from [c0698f50]
(handle_irq_event_percpu+0x54/0x184)
[c0698f50] (handle_irq_event_percpu+0x54/0x184) from [c06990bc]
(handle_irq_event+0x3c/0x5c)
[c06990bc] (handle_irq_event+0x3c/0x5c) from [c069b6e8]
(handle_fasteoi_irq+0x9c/0x104)
[c069b6e8] (handle_fasteoi_irq+0x9c/0x104) from [c0698eec]
(generic_handle_irq+0x28/0x30)
[c0698eec] (generic_handle_irq+0x28/0x30) from [c0633058]
(asm_do_IRQ+0x58/0xb8)
[c0633058] (asm_do_IRQ+0x58/0xb8) from [c0644acc]
(shmobile_handle_irq_gic+0xc/0x80)
Exception stack(0xc0951f70 to 0xc0951fb8)
1f60: 0001 4491 c0951fa8
1f80: c095 c0977604 c088f9c4 c0955b40 4000406a 412fc098
1fa0: 0001 c0951fb8 4490 c063a650 2013
[c0644acc] (shmobile_handle_irq_gic+0xc/0x80) from [4491] (0x4491)
mmc0: new high speed SD card at address b368
mmcblk0: mmc0:b368 SDC 489 MiB
mmcblk0: p1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 2/6] arm: davinci: Explicitly set channel controllers' default queues
On Sun, Jul 10, 2011 at 18:44:35, Ido Yariv wrote: Davinci platforms may define a default queue for each channel controller. If one is not defined, the default queue is set to EVENTQ_1. However, there's no way to distinguish between an unset default queue to one that is set to EVENTQ_0, as EVENTQ_0 = 0. Explicitly specify the default queue for all channel controllers on all Davinci platforms to EVENTQ_1, and don't overwrite it in the EDMA probe function. One exception is the DA850 board, for which EVENTQ_1 is not a valid option for its second channel controller. Use EVENTQ_0 instead for that channel controller. Signed-off-by: Ido Yariv i...@wizery.com Looks good to me. Will queue for v3.2/fixes BTW, Arnd has indicated a preference for ARM: davinci: prefix so I will make that change while applying. Thanks, Sekhar -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 3/6] arm: davinci: mmc: Add support for set_power callback
On Sun, Jul 10, 2011 at 18:44:36, Ido Yariv wrote: Some devices connected to the MMC bus are power controlled by external means. For instance, an SDIO device may be powered down/up by an external gpio line. In order to avoid toggling power from within the MMC host driver, add a set_power callback function, which will be called by set_ios upon powering down/up. Signed-off-by: Ido Yariv i...@wizery.com --- arch/arm/mach-davinci/include/mach/mmc.h |3 +++ drivers/mmc/host/davinci_mmc.c | 13 + 2 files changed, 16 insertions(+), 0 deletions(-) This looks good to me, but needs Ack/Sign-off from MMC maintainer. Please repost keeping him in CC. Thanks, Sekhar -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 5/6] arm: davinci: DA850: Add GPIO pinmux configuration for wl1271
On Sun, Jul 10, 2011 at 18:44:38, Ido Yariv wrote: The wl1271 daughter board makes use of a few GPIOs: GPIO6_9 is used for powering down/up the WLAN functionality. GPIO6_10 is used as an input IRQ line from the WLAN chip. Add the required pinmux configuration for these GPIOs. Signed-off-by: Ido Yariv i...@wizery.com 4/6 and 5/6 both look good, will queue for v3.2/features Thanks, Sekhar -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/6] arm: davinci: Explicitly set channel controllers' default queues
Hello. Nori, Sekhar wrote: Davinci platforms may define a default queue for each channel controller. If one is not defined, the default queue is set to EVENTQ_1. However, there's no way to distinguish between an unset default queue to one that is set to EVENTQ_0, as EVENTQ_0 = 0. Explicitly specify the default queue for all channel controllers on all Davinci platforms to EVENTQ_1, and don't overwrite it in the EDMA probe function. One exception is the DA850 board, for which EVENTQ_1 is not a valid option for its second channel controller. Use EVENTQ_0 instead for that channel controller. Signed-off-by: Ido Yariv i...@wizery.com Looks good to me. Will queue for v3.2/fixes Why wait for 3.2? If this is considered a fix, it should be applied to 3.1, no? WBR, Sergei -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] mmc: core: add random fault injection
2011/7/21 Per Forlin per.for...@linaro.org:
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Looks good to me.
@@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
I think the fail_attr should be defined for each mmc_host like make_it_fail
in struct mmc_host and debugfs entries should also be created in a
subdirectory of mmc host debugfs.
And I know that init_fault_attr_dentries() can only create a
subdirectory in debugfs root directory. But I have a patch which
support for creating it in arbitrary directory. Could you take a look
at this? (Note that this patch is based on mmotm and not yet tested)
From 79fdd83b2af932d6fd155ae918e20572e6784bb8 Mon Sep 17 00:00:00 2001
From: Akinobu Mita akinobu.m...@gmail.com
Date: Mon, 25 Jul 2011 23:51:20 +0900
Subject: [PATCH] fault-injection: support for creating debugfs entries in
arbitrary directory
Signed-off-by: Akinobu Mita akinobu.m...@gmail.com
---
Documentation/fault-injection/fault-injection.txt |3 +--
block/blk-core.c |6 --
block/blk-timeout.c |5 -
include/linux/fault-inject.h | 18 +-
lib/fault-inject.c| 20 +++-
mm/failslab.c | 14 +++---
mm/page_alloc.c | 13 +
7 files changed, 33 insertions(+), 46 deletions(-)
diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt
index 7be15e4..dda989e 100644
--- a/Documentation/fault-injection/fault-injection.txt
+++ b/Documentation/fault-injection/fault-injection.txt
@@ -143,8 +143,7 @@ o provide a way to configure fault attributes
failslab, fail_page_alloc, and fail_make_request use this way.
Helper functions:
- init_fault_attr_dentries(entries, attr, name);
- void cleanup_fault_attr_dentries(entries);
+ debugfs_create_fault_attr(name, parent, attr);
- module parameters
diff --git a/block/blk-core.c b/block/blk-core.c
index 07988b4..498c525 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1367,8 +1367,10 @@ static bool should_fail_request(struct hd_struct *part, unsigned int bytes)
static int __init fail_make_request_debugfs(void)
{
- return init_fault_attr_dentries(fail_make_request,
- fail_make_request);
+ struct dentry *dir = debugfs_create_fault_attr(fail_make_request,
+ NULL, fail_make_request);
+
+ return IS_ERR(dir) ? PTR_ERR(dir) : 0;
}
late_initcall(fail_make_request_debugfs);
diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 4f0c06c..6397e2e 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -28,7 +28,10 @@ int blk_should_fake_timeout(struct request_queue *q)
static int __init fail_io_timeout_debugfs(void)
{
- return init_fault_attr_dentries(fail_io_timeout, fail_io_timeout);
+ struct dentry *dir = debugfs_create_fault_attr(fail_io_timeout,
+ NULL, fail_io_timeout);
+
+ return IS_ERR(dir) ? PTR_ERR(dir) : 0;
}
late_initcall(fail_io_timeout_debugfs);
diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h
index a842db6..3f583df 100644
--- a/include/linux/fault-inject.h
+++ b/include/linux/fault-inject.h
@@ -25,10 +25,6 @@ struct fault_attr {
unsigned long reject_end;
unsigned long count;
-
-#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
- struct dentry *dir;
-#endif
};
#define FAULT_ATTR_INITIALIZER {\
@@ -45,19 +41,15 @@ bool should_fail(struct fault_attr *attr, ssize_t size);
#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
-int init_fault_attr_dentries(struct fault_attr *attr, const char *name);
-void cleanup_fault_attr_dentries(struct fault_attr *attr);
+struct dentry *debugfs_create_fault_attr(const char *name,
+ struct dentry *parent, struct fault_attr *attr);
#else /* CONFIG_FAULT_INJECTION_DEBUG_FS */
-static inline int init_fault_attr_dentries(struct fault_attr *attr,
- const char *name)
-{
- return -ENODEV;
-}
-
-static inline void cleanup_fault_attr_dentries(struct fault_attr *attr)
+static inline struct dentry *debugfs_create_fault_attr(const char *name,
+ struct dentry *parent, struct fault_attr *attr);
{
+ return ERR_PTR(-ENODEV);
}
#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index da61bb5..95399e7 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -192,21 +192,15 @@ static struct dentry *debugfs_create_atomic_t(const char *name, mode_t
RE: [PATCH v2 2/6] arm: davinci: Explicitly set channel controllers' default queues
Hi Sergei, On Mon, Jul 25, 2011 at 19:03:45, Sergei Shtylyov wrote: Hello. Nori, Sekhar wrote: Davinci platforms may define a default queue for each channel controller. If one is not defined, the default queue is set to EVENTQ_1. However, there's no way to distinguish between an unset default queue to one that is set to EVENTQ_0, as EVENTQ_0 = 0. Explicitly specify the default queue for all channel controllers on all Davinci platforms to EVENTQ_1, and don't overwrite it in the EDMA probe function. One exception is the DA850 board, for which EVENTQ_1 is not a valid option for its second channel controller. Use EVENTQ_0 instead for that channel controller. Signed-off-by: Ido Yariv i...@wizery.com Looks good to me. Will queue for v3.2/fixes Why wait for 3.2? If this is considered a fix, it should be applied to 3.1, no? 3.2/fixes just indicates it will be queued as a fix/cleanup for 3.2 so it will have higher priority for merge when compared to a new feature. This patch doesn't really fix any existing broken functionality. It corrects event queue configuration for EDMA CC1 on DA850 for which there are no current users in mainline. So, not sending for v3.1. Thanks, Sekhar -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 6/6] arm: davinci: DA850: Add wl1271/wlan support
On Sun, Jul 10, 2011 at 18:44:39, Ido Yariv wrote:
The wl1271 daughter card for AM18x EVMs is a combo wireless connectivity
add-on card, based on the LS Research TiWi module with Texas
Instruments' wl1271 solution.
It is a 4-wire, 1.8V, embedded SDIO WLAN device with an external IRQ
line and is power-controlled by a GPIO-based fixed regulator.
This patch adds support for the WLAN capabilities of this expansion
board.
Signed-off-by: Ido Yariv i...@wizery.com
---
diff --git a/arch/arm/mach-davinci/board-da850-evm.c
b/arch/arm/mach-davinci/board-da850-evm.c
index a7b41bf..2dae1a1 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -31,6 +31,8 @@
+#ifdef CONFIG_DA850_WL12XX
+
+static int da850_wl12xx_fref = WL12XX_REFCLOCK_38;
+
+static int __init setup_da850_wl12xx_fref(char *fref)
+{
+ if (!strcmp(fref, 19.2))
+ da850_wl12xx_fref = WL12XX_REFCLOCK_19;
+ else if (!strcmp(fref, 26))
+ da850_wl12xx_fref = WL12XX_REFCLOCK_26;
+ else if (!strcmp(fref, 38.4))
+ da850_wl12xx_fref = WL12XX_REFCLOCK_38;
+ else if (!strcmp(fref, 52))
+ da850_wl12xx_fref = WL12XX_REFCLOCK_52;
+ else if (!strcmp(fref, XTAL26))
+ da850_wl12xx_fref = WL12XX_REFCLOCK_26_XTAL;
+ else if (!strcmp(fref, XTAL38.4))
+ da850_wl12xx_fref = WL12XX_REFCLOCK_38_XTAL;
+ else
+ pr_info(da850_wl12xx_fref is invalid. Valid options:
+ 19.2, 26, 38.4, 52, XTAL26 or XTAL38.4\n);
+ return 0;
+}
+__setup(da850_wl12xx_fref=, setup_da850_wl12xx_fref);
Adding a new kernel parameter requires update to
Documentation/kernel-parameters.txt as well.
I am Ccing a couple of folks in case they have ideas on
whether there is a better way to pass this information
to the kernel. I assume there is no way to detect
this from hardware.
+static struct davinci_mmc_config da850_mmc_wl12xx_config = {
+ .get_ro = NULL,
+ .get_cd = NULL,
You can get rid of these NULL initializers.
+ .set_power = wl12xx_set_power,
+ .wires = 4,
+ .max_freq = 2500,
+ .caps = MMC_CAP_4_BIT_DATA | MMC_CAP_NONREMOVABLE |
+ MMC_CAP_POWER_OFF_CARD,
+ .version= MMC_CTLR_VERSION_2,
+};
+static void da850_wl12xx_init(void)
+{
+ int ret;
+
+ ret = davinci_cfg_reg_list(da850_evm_mmc_wl12xx_pins);
+ if (ret) {
+ pr_warning(da850_evm_init: wl12xx/mmc mux setup failed:
+ %d\n, ret);
+ return;
+ }
+
+ ret = da850_register_mmcsd1(da850_mmc_wl12xx_config);
+ if (ret) {
+ pr_warning(da850_evm_init: wl12xx/mmc registration failed:
+ %d\n, ret);
+ return;
+ }
+
+ ret = gpio_request_one(DA850_WLAN_EN, GPIOF_OUT_INIT_LOW, wl12xx_en);
+ if (ret) {
+ pr_err(Error initializing the wl12xx enable gpio: %d\n, ret);
+ return;
+ }
+
+ ret = gpio_request_one(DA850_WLAN_IRQ, GPIOF_IN, wl12xx_irq);
+ if (ret) {
+ pr_err(Error initializing the wl12xx irq gpio: %d\n, ret);
+ gpio_free(DA850_WLAN_EN);
+ return;
+ }
+
+ da850_wl12xx_wlan_data.irq = gpio_to_irq(DA850_WLAN_IRQ);
+ da850_wl12xx_wlan_data.board_ref_clock = da850_wl12xx_fref;
+
+ ret = wl12xx_set_platform_data(da850_wl12xx_wlan_data);
+ if (ret) {
+ pr_err(Error setting wl12xx data: %d\n, ret);
+ gpio_free(DA850_WLAN_IRQ);
+ gpio_free(DA850_WLAN_EN);
Why not just use the traditional goto based bail out
mechanism? You will avoid the multiple gpio_free() calls.
Thanks,
Sekhar
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] mmc: core: add random fault injection
On 25 July 2011 17:58, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/7/21 Per Forlin per.for...@linaro.org: This adds support to inject data errors after a completed host transfer. The mmc core will return error even though the host transfer is successful. This simple fault injection proved to be very useful to test the non-blocking error handling in the mmc_blk_issue_rw_rq(). Random faults can also test how the host driver handles pre_req() and post_req() in case of errors. Looks good to me. Thanks, @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) flush_workqueue(workqueue); } +#ifdef CONFIG_FAIL_MMC_REQUEST + +static DECLARE_FAULT_ATTR(fail_mmc_request); I think the fail_attr should be defined for each mmc_host like make_it_fail in struct mmc_host and debugfs entries should also be created in a subdirectory of mmc host debugfs. I looked at blk-core.c and used the same code here. Current code creates the entry under the debugfs root. This means one fail_attr for all hosts. I agree, it's more clean to move the fail_attr to the host-debugfs-entry which require the fail_attr to be stored same way as make_it_fail. And I know that init_fault_attr_dentries() can only create a subdirectory in debugfs root directory. But I have a patch which support for creating it in arbitrary directory. Could you take a look at this? (Note that this patch is based on mmotm and not yet tested) Thanks, I will check it out. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] mmc: core: add random fault injection
Hi Akinobu, On 25 July 2011 21:19, Per Forlin per.for...@linaro.org wrote: On 25 July 2011 17:58, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/7/21 Per Forlin per.for...@linaro.org: This adds support to inject data errors after a completed host transfer. The mmc core will return error even though the host transfer is successful. This simple fault injection proved to be very useful to test the non-blocking error handling in the mmc_blk_issue_rw_rq(). Random faults can also test how the host driver handles pre_req() and post_req() in case of errors. Looks good to me. Thanks, @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) flush_workqueue(workqueue); } +#ifdef CONFIG_FAIL_MMC_REQUEST + +static DECLARE_FAULT_ATTR(fail_mmc_request); I think the fail_attr should be defined for each mmc_host like make_it_fail in struct mmc_host and debugfs entries should also be created in a subdirectory of mmc host debugfs. I looked at blk-core.c and used the same code here. Current code creates the entry under the debugfs root. This means one fail_attr for all hosts. I agree, it's more clean to move the fail_attr to the host-debugfs-entry which require the fail_attr to be stored same way as make_it_fail. And I know that init_fault_attr_dentries() can only create a subdirectory in debugfs root directory. But I have a patch which support for creating it in arbitrary directory. Could you take a look at this? (Note that this patch is based on mmotm and not yet tested) I looked at your patch and it raised two questions. I can't use FAULT_ATTR_INITIALIZER since mmc_host is allocated on the heap. It looks like setup_fault_attr(attr, str) will fail if str is NULL. How can I initialise the fault_attrs if not stack allocated? About the boot param initialisation of fault attr. There can only be one fault_mmc_request boot param for the entire kernel but there is one fault_attr per host, and there may be many hosts. It would be convenient if setup_fault_attrs would take (attr, boot_param_name), look up boot_param_name and use that otherwise set default values. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: mmc_test: avoid stalled file in debugfs
On 22 July 2011 15:13, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
During card removal and inserting cycle the test file in the debugfs could be
stalled until the host driver removes it. Let's keep the file in the linked
list and destroy it when card is removed.
Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
Thanks for catching and fixing this mistake. The two debugfs files
test and testlist were created but only testlist was added to the
list. This patch takes care of that and adds both test and testlist to
the linked list, and makes the code cleaner too.
---
drivers/mmc/card/mmc_test.c | 56
+++
1 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 006a5e9..915a221 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -2900,7 +2900,7 @@ static const struct file_operations
mmc_test_fops_testlist = {
.release = single_release,
};
-static void mmc_test_free_file_test(struct mmc_card *card)
+static void mmc_test_free_dbgfs_file(struct mmc_card *card)
{
struct mmc_test_dbgfs_file *df, *dfs;
@@ -2917,34 +2917,21 @@ static void mmc_test_free_file_test(struct mmc_card
*card)
mutex_unlock(mmc_test_lock);
}
-static int mmc_test_register_file_test(struct mmc_card *card)
+static int __mmc_test_register_dbgfs_file(struct mmc_card *card,
+ const char *name, mode_t mode, const struct file_operations *fops)
{
struct dentry *file = NULL;
struct mmc_test_dbgfs_file *df;
- int ret = 0;
-
- mutex_lock(mmc_test_lock);
-
- if (card-debugfs_root)
- file = debugfs_create_file(test, S_IWUSR | S_IRUGO,
- card-debugfs_root, card, mmc_test_fops_test);
-
- if (IS_ERR_OR_NULL(file)) {
- dev_err(card-dev,
- Can't create test. Perhaps debugfs is disabled.\n);
- ret = -ENODEV;
- goto err;
- }
if (card-debugfs_root)
- file = debugfs_create_file(testlist, S_IRUGO,
- card-debugfs_root, card, mmc_test_fops_testlist);
+ file = debugfs_create_file(name, mode,
card-debugfs_root,card,
+ fops);
Move card to next line together with fops and indent.
+ file = debugfs_create_file(name, mode, card-debugfs_root,
+card, fops);
if (IS_ERR_OR_NULL(file)) {
dev_err(card-dev,
- Can't create testlist. Perhaps debugfs is
disabled.\n);
- ret = -ENODEV;
- goto err;
+ Can't create %s. Perhaps debugfs is disabled.\n,
+ name);
+ return -ENODEV;
}
df = kmalloc(sizeof(struct mmc_test_dbgfs_file), GFP_KERNEL);
@@ -2952,14 +2939,31 @@ static int mmc_test_register_file_test(struct
mmc_card *card)
debugfs_remove(file);
dev_err(card-dev,
Can't allocate memory for internal usage.\n);
- ret = -ENOMEM;
- goto err;
+ return -ENOMEM;
}
df-card = card;
df-file = file;
list_add(df-link, mmc_test_file_test);
+ return 0;
+}
+
+static int mmc_test_register_dbgfs_file(struct mmc_card *card)
+{
+ int ret;
+
+ mutex_lock(mmc_test_lock);
+
+ ret = __mmc_test_register_dbgfs_file(card, test, S_IWUSR | S_IRUGO,
+ mmc_test_fops_test);
+ if (ret)
+ goto err;
+
+ ret = __mmc_test_register_dbgfs_file(card, testlist, S_IRUGO,
+ mmc_test_fops_testlist);
+ if (ret)
+ goto err;
err:
mutex_unlock(mmc_test_lock);
@@ -2974,7 +2978,7 @@ static int mmc_test_probe(struct mmc_card *card)
if (!mmc_card_mmc(card) !mmc_card_sd(card))
return -ENODEV;
- ret = mmc_test_register_file_test(card);
+ ret = mmc_test_register_dbgfs_file(card);
if (ret)
return ret;
@@ -2986,7 +2990,7 @@ static int mmc_test_probe(struct mmc_card *card)
static void mmc_test_remove(struct mmc_card *card)
{
mmc_test_free_result(card);
- mmc_test_free_file_test(card);
+ mmc_test_free_dbgfs_file(card);
}
static struct mmc_driver mmc_driver = {
@@ -3006,7 +3010,7 @@ static void __exit mmc_test_exit(void)
{
/* Clear stalled data if card is still plugged */
mmc_test_free_result(NULL);
- mmc_test_free_file_test(NULL);
+ mmc_test_free_dbgfs_file(NULL);
mmc_unregister_driver(mmc_driver);
}
--
1.7.5.4
Acked-by: Per Forlin per.for...@linaro.org
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the
Re: [PATCH v3 2/3] mmc: core: add random fault injection
2011/7/26 Per Forlin per.for...@linaro.org:
And I know that init_fault_attr_dentries() can only create a
subdirectory in debugfs root directory. But I have a patch which
support for creating it in arbitrary directory. Could you take a look
at this? (Note that this patch is based on mmotm and not yet tested)
I looked at your patch and it raised two questions.
I can't use FAULT_ATTR_INITIALIZER since mmc_host is allocated on the
heap. It looks like setup_fault_attr(attr, str) will fail if str is
NULL. How can I initialise the fault_attrs if not stack allocated?
About the boot param initialisation of fault attr. There can only be
one fault_mmc_request boot param for the entire kernel but there is
one fault_attr per host, and there may be many hosts. It would be
convenient if setup_fault_attrs would take (attr, boot_param_name),
look up boot_param_name and use that otherwise set default values.
I think you can define one default fail_attr for boot time configuration
and copy it to per-host fail_attr in mmc_add_host_debugfs().
/* pseudo-code */
static DECLARE_FAULT_ATTR(default_mmc_fail_attr);
static int __init setup_fail_mmc_request(char *str)
{
return setup_fault_attr(default_mmc_fail_attr, str);
}
__setup(fail_mmc_request=, setup_fail_mmc_request);
...
void mmc_add_host_debugfs(struct mmc_host *host)
{
...
#ifdef CONFIG_FAIL_MMC_REQUEST
host-fail_attr = default_mmc_fail_attr;
if (!debugfs_create_fault_attr(fail_mmc_request,
root, host-fail_attr))
goto err_node;
#endif
...
}
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
