Re: [PATCH] mmc: mmci: Do not release spinlock in request_end
Jon Medhurst (Tixy) wrote:
On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
The patch mmc: core: move -request() call from atomic context,
is the reason to why this change is possible. This simplifies the
error handling code execution path quite a lot and potentially also
fixes some error handling hang problems.
Signed-off-by: Ulf Hansson ulf.hans...@stericsson.com
This doesn't look right:
void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
{
if (err cmd-retries) {
host-ops-request(host, mrq);
This is NOT how it looks at mmc-next. You need to test with Adrian
Hunters patch which the commit refers two.
Linux next for mmc is available at:
git://dev.laptop.org/users/cjb/mmc mmc-next
So, not dropping the spinlock results in calling the request function
with the spinlock held - and as the request function then goes on to
lock the spinlock, we will deadlock.
Indeed, deadlock behaviour at this point is what I see with this patch
on a Versatile Express board running 3.0-rc9.
--
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 1/4] mmc: mmci: Bugfix in pio read for small packets
On 10/07/2011 09:11 PM, Russell King - ARM Linux wrote:
But first, you need to fix your code so you're only reading 32-bit
quantities from the FIFO register.
Hi Russel, what to you think of doing it this way instead:
/*
* SDIO especially may want to send something that is
* not divisible by 4 (as opposed to card sectors
* etc), and the FIFO only accept full 32-bit reads.
*/
if (count 4) {
unsigned char buf[4];
readsl(base + MMCIFIFO, buf, 1);
memcpy(ptr, buf, count);
}
else
readsl(base + MMCIFIFO, ptr, count 2);
This makes sure we only access the FIFO in a 32 bit way, and the only
overhead for the standard case (count = 4) is the if clause. I have
verified this to work with our WLAN driver.
Best Regards
Stefan Nilsson
--
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: mmci: Do not release spinlock in request_end
On Fri, Oct 14, 2011 at 09:37:51AM +0200, Ulf Hansson wrote:
Jon Medhurst (Tixy) wrote:
On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
The patch mmc: core: move -request() call from atomic context,
is the reason to why this change is possible. This simplifies the
error handling code execution path quite a lot and potentially also
fixes some error handling hang problems.
Signed-off-by: Ulf Hansson ulf.hans...@stericsson.com
This doesn't look right:
void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
{
if (err cmd-retries) {
host-ops-request(host, mrq);
This is NOT how it looks at mmc-next. You need to test with Adrian
Hunters patch which the commit refers two.
In that case, how can I take the patch to mmci if it depends on something
in another tree?
--
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: mmci: Do not release spinlock in request_end
Russell King - ARM Linux wrote:
On Fri, Oct 14, 2011 at 09:37:51AM +0200, Ulf Hansson wrote:
Jon Medhurst (Tixy) wrote:
On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
The patch mmc: core: move -request() call from atomic context,
is the reason to why this change is possible. This simplifies the
error handling code execution path quite a lot and potentially also
fixes some error handling hang problems.
Signed-off-by: Ulf Hansson ulf.hans...@stericsson.com
This doesn't look right:
void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
{
if (err cmd-retries) {
host-ops-request(host, mrq);
This is NOT how it looks at mmc-next. You need to test with Adrian
Hunters patch which the commit refers two.
In that case, how can I take the patch to mmci if it depends on something
in another tree?
I don't know. But how do you update your tree from next normally? I
believe the problem is more related to that the mmc-next tree is now on
a temporary git. If you do not update your tree how shall we be able to
continue with integration of new patches that depends on mmc patches
from next?
BR
Uffe
--
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: mmci: Do not release spinlock in request_end
On Fri, Oct 14, 2011 at 09:51:44AM +0200, Ulf Hansson wrote: Russell King - ARM Linux wrote: In that case, how can I take the patch to mmci if it depends on something in another tree? I don't know. But how do you update your tree from next normally? I don't - no one in their right mind does. linux-next is not a tree which should ever be pulled into any other git tree to base work upon - it's completely unstable and you can't rely on any commit in there remaining. The merge commits between each tree are also specific to linux-next only. Linus will also refuse to pull any tree which has linux-next merged into it. I believe the problem is more related to that the mmc-next tree is now on a temporary git. Where a tree is hosted has no influence on whether its 'temporary' or not. If you do not update your tree how shall we be able to continue with integration of new patches that depends on mmc patches from next? That's what I'm saying - I can't take the patch as it introduces bugs if I do. It's better to either wait until after the next merge window, or we have to sort out merging it via the mmc tree instead. -- 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 1/4] mmc: mmci: Bugfix in pio read for small packets
So if they apply, they are independent AFAICT. 7108/1 and 7109/1, have real dependencies. Otherwise there none. Ok, I assume that those depend on the first two patches. So, I tried applying 7110/1 .. 7112/1 but the first rejects because it doesn't have the non-power-of-2 support patch applied (7107/1). And it seems sensible that 7107/1 depends on 7106/1 (the PIO patch) which I believe to be a problem. I see the problem. The patches from the patch serie Clarify code paths for how to modify the power register are independent from this SDIO serie. Although the patches has been based upon the patches from the SDIO serie, which is as you say the reason to why they not apply cleanly. I will upload a second version of the serie Clarify code paths for how to modify the power register which is NOT based on the SDIO serie. Then when can discuss each serie separately. Therefore, I don't think any of these can be applied without the initial PIO patch. One thing I haven't yet mentioned is about the non-power-of-2 support - surely this can only be supported if blksz_datactrl16 is set? If so, shouldn't it key off that? I don't see how it could otherwise support non-power of 2 block sizes with just a log2 of the block size programmed into the data control register. Your observation is correct, but at the same time do we really need to have such a dependency check in the code? The hardware setup is handled in the variant struct completely, so we believe that should be enough. -- 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: mmci: Do not release spinlock in request_end
If you do not update your tree how shall we be able to continue with integration of new patches that depends on mmc patches from next? That's what I'm saying - I can't take the patch as it introduces bugs if I do. It's better to either wait until after the next merge window, or we have to sort out merging it via the mmc tree instead. Thanks, now I really get the problem! :-) From mmci host driver perspective this will then be kind of complicated since it sometimes will depend on the patches on the mmc framework. Now we have the spinlock patch, but I believe this will happen soon again. So how can we handle to merge it via mmc tree instead? Will you be able to Ack patches for mmci so Chris Ball can merge them for the mmc tree instead? Of course I only mean patches that really has a dependency, the rest can be handled as is I believe. BR Uffe -- 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
[PATCH V5 0/3] ARM: SAMSUNG: Add support for sdhci clock lookup using generic names
This patchset adds support for sdhci controller clock lookup using generic names. With this patchset, there will be no need to pass clock names in sdhci platform data. This patchset depends on the patchset: Add a common macro for creating struct clk_lookup entries. mmc: sdhci-s3c: add default controller configuration. V2 Changes: - Added HCLK instance for HSMMC instance 1 as suggested by Heiko. - Rebased on UART clkdev patches. So these patches should be applied after UART clkdev patches are applied. V3 Changes: - Removed double registration of hsmmc1 hclk from s3c2416 as suggested by Heiko V4 Changes: - Changed the devname for bus clocks in Exynos4 to s3c-sdhci. from exynos4-sdhci. as suggested by Sylwester. V5 Changes: - Rebased the patches on latest For-next branch of Kgene's tree after S3C2416: Enable armdiv and armclk patch by Heiko. Rajeshwari Shinde (3): SDHCI: S3C: Use generic clock names for sdhci bus clock options ARM: SAMSUNG: Remove SDHCI bus clocks from platform data ARM: SAMSUNG: Add lookup of sdhci-s3c clocks using generic names arch/arm/mach-exynos4/Makefile |1 - arch/arm/mach-exynos4/clock.c | 88 +-- arch/arm/mach-exynos4/setup-sdhci.c| 22 arch/arm/mach-s3c2416/Makefile |1 - arch/arm/mach-s3c2416/clock.c | 68 ++- arch/arm/mach-s3c2416/setup-sdhci.c| 24 arch/arm/mach-s3c64xx/Makefile |1 - arch/arm/mach-s3c64xx/clock.c | 126 + arch/arm/mach-s3c64xx/setup-sdhci.c| 24 arch/arm/mach-s5pc100/Makefile |1 - arch/arm/mach-s5pc100/clock.c | 130 +- arch/arm/mach-s5pc100/setup-sdhci.c| 23 arch/arm/mach-s5pv210/Makefile |1 - arch/arm/mach-s5pv210/clock.c | 167 +--- arch/arm/mach-s5pv210/setup-sdhci.c| 22 arch/arm/plat-s3c24xx/s3c2443-clock.c | 16 ++- arch/arm/plat-samsung/include/plat/sdhci.h | 31 - drivers/mmc/host/sdhci-s3c.c |6 +- 18 files changed, 361 insertions(+), 391 deletions(-) delete mode 100644 arch/arm/mach-exynos4/setup-sdhci.c delete mode 100644 arch/arm/mach-s3c2416/setup-sdhci.c delete mode 100644 arch/arm/mach-s3c64xx/setup-sdhci.c delete mode 100644 arch/arm/mach-s5pc100/setup-sdhci.c delete mode 100644 arch/arm/mach-s5pv210/setup-sdhci.c -- 1.7.4.4 -- 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
[PATCH V5 1/3] SDHCI: S3C: Use generic clock names for sdhci bus clock options
This patch modifies the driver to stop depending on the clock names
being passed from the platform and switch over to bus clock lookup
using generic clock names.
Signed-off-by: Rajeshwari Shinde rajeshwar...@samsung.com
---
drivers/mmc/host/sdhci-s3c.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 82709b6..a5fde87 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -435,14 +435,12 @@ static int __devinit sdhci_s3c_probe(struct
platform_device *pdev)
for (clks = 0, ptr = 0; ptr MAX_BUS_CLK; ptr++) {
struct clk *clk;
- char *name = pdata-clocks[ptr];
+ char name[14];
- if (name == NULL)
- continue;
+ sprintf(name, mmc_busclk.%d, ptr);
clk = clk_get(dev, name);
if (IS_ERR(clk)) {
- dev_err(dev, failed to get clock %s\n, name);
continue;
}
--
1.7.4.4
--
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
[PATCH V5 2/3] ARM: SAMSUNG: Remove SDHCI bus clocks from platform data
The bus clocks previously sent through platform data to SDHCI controller
are removed.
Signed-off-by: Rajeshwari Shinde rajeshwar...@samsung.com
---
arch/arm/mach-exynos4/Makefile |1 -
arch/arm/mach-exynos4/setup-sdhci.c| 22 ---
arch/arm/mach-s3c2416/Makefile |1 -
arch/arm/mach-s3c2416/setup-sdhci.c| 24 -
arch/arm/mach-s3c64xx/Makefile |1 -
arch/arm/mach-s3c64xx/setup-sdhci.c| 24 -
arch/arm/mach-s5pc100/Makefile |1 -
arch/arm/mach-s5pc100/setup-sdhci.c| 23
arch/arm/mach-s5pv210/Makefile |1 -
arch/arm/mach-s5pv210/setup-sdhci.c| 22 ---
arch/arm/plat-samsung/include/plat/sdhci.h | 31
11 files changed, 0 insertions(+), 151 deletions(-)
delete mode 100644 arch/arm/mach-exynos4/setup-sdhci.c
delete mode 100644 arch/arm/mach-s3c2416/setup-sdhci.c
delete mode 100644 arch/arm/mach-s3c64xx/setup-sdhci.c
delete mode 100644 arch/arm/mach-s5pc100/setup-sdhci.c
delete mode 100644 arch/arm/mach-s5pv210/setup-sdhci.c
diff --git a/arch/arm/mach-exynos4/Makefile b/arch/arm/mach-exynos4/Makefile
index 2bb18f4..c47aae3 100644
--- a/arch/arm/mach-exynos4/Makefile
+++ b/arch/arm/mach-exynos4/Makefile
@@ -55,7 +55,6 @@ obj-$(CONFIG_EXYNOS4_SETUP_I2C5) += setup-i2c5.o
obj-$(CONFIG_EXYNOS4_SETUP_I2C6) += setup-i2c6.o
obj-$(CONFIG_EXYNOS4_SETUP_I2C7) += setup-i2c7.o
obj-$(CONFIG_EXYNOS4_SETUP_KEYPAD) += setup-keypad.o
-obj-$(CONFIG_EXYNOS4_SETUP_SDHCI) += setup-sdhci.o
obj-$(CONFIG_EXYNOS4_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
obj-$(CONFIG_EXYNOS4_SETUP_USB_PHY)+= setup-usb-phy.o
diff --git a/arch/arm/mach-exynos4/setup-sdhci.c
b/arch/arm/mach-exynos4/setup-sdhci.c
deleted file mode 100644
index 92937b4..000
--- a/arch/arm/mach-exynos4/setup-sdhci.c
+++ /dev/null
@@ -1,22 +0,0 @@
-/* linux/arch/arm/mach-exynos4/setup-sdhci.c
- *
- * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
- * http://www.samsung.com
- *
- * EXYNOS4 - Helper functions for settign up SDHCI device(s) (HSMMC)
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
-*/
-
-#include linux/types.h
-
-/* clock sources for the mmc bus clock, order as for the ctrl2[5..4] */
-
-char *exynos4_hsmmc_clksrcs[4] = {
- [0] = NULL,
- [1] = NULL,
- [2] = sclk_mmc, /* mmc_bus */
- [3] = NULL,
-};
diff --git a/arch/arm/mach-s3c2416/Makefile b/arch/arm/mach-s3c2416/Makefile
index 7b805b2..ca0cd22 100644
--- a/arch/arm/mach-s3c2416/Makefile
+++ b/arch/arm/mach-s3c2416/Makefile
@@ -15,7 +15,6 @@ obj-$(CONFIG_S3C2416_PM) += pm.o
#obj-$(CONFIG_S3C2416_DMA) += dma.o
# Device setup
-obj-$(CONFIG_S3C2416_SETUP_SDHCI) += setup-sdhci.o
obj-$(CONFIG_S3C2416_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
# Machine support
diff --git a/arch/arm/mach-s3c2416/setup-sdhci.c
b/arch/arm/mach-s3c2416/setup-sdhci.c
deleted file mode 100644
index cee5395..000
--- a/arch/arm/mach-s3c2416/setup-sdhci.c
+++ /dev/null
@@ -1,24 +0,0 @@
-/* linux/arch/arm/mach-s3c2416/setup-sdhci.c
- *
- * Copyright 2010 Promwad Innovation Company
- * Yauhen Kharuzhy yauhen.kharu...@promwad.com
- *
- * S3C2416 - Helper functions for settign up SDHCI device(s) (HSMMC)
- *
- * Based on mach-s3c64xx/setup-sdhci.c
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
-*/
-
-#include linux/types.h
-
-/* clock sources for the mmc bus clock, order as for the ctrl2[5..4] */
-
-char *s3c2416_hsmmc_clksrcs[4] = {
- [0] = hsmmc,
- [1] = hsmmc,
- [2] = hsmmc-if,
- /* [3] = 48m, - note not successfully used yet */
-};
diff --git a/arch/arm/mach-s3c64xx/Makefile b/arch/arm/mach-s3c64xx/Makefile
index cfc0b99..e32093c 100644
--- a/arch/arm/mach-s3c64xx/Makefile
+++ b/arch/arm/mach-s3c64xx/Makefile
@@ -32,7 +32,6 @@ obj-$(CONFIG_S3C64XX_SETUP_I2C0) += setup-i2c0.o
obj-$(CONFIG_S3C64XX_SETUP_I2C1) += setup-i2c1.o
obj-$(CONFIG_S3C64XX_SETUP_IDE) += setup-ide.o
obj-$(CONFIG_S3C64XX_SETUP_KEYPAD) += setup-keypad.o
-obj-$(CONFIG_S3C64XX_SETUP_SDHCI) += setup-sdhci.o
obj-$(CONFIG_S3C64XX_SETUP_FB_24BPP) += setup-fb-24bpp.o
obj-$(CONFIG_S3C64XX_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
diff --git a/arch/arm/mach-s3c64xx/setup-sdhci.c
b/arch/arm/mach-s3c64xx/setup-sdhci.c
deleted file mode 100644
index c75a71b..000
--- a/arch/arm/mach-s3c64xx/setup-sdhci.c
+++ /dev/null
@@ -1,24 +0,0 @@
-/* linux/arch/arm/mach-s3c64xx/setup-sdhci.c
- *
- * Copyright 2008 Simtec Electronics
- * Copyright 2008 Simtec Electronics
- * Ben Dooks b...@simtec.co.uk
- *
Re: [PATCH V2 11/16] mmc: omap_hsmmc: ensure pbias configuration is always done
Hi, On Thu, Sep 29 2011, T Krishnamoorthy, Balaji wrote: On Fri, May 6, 2011 at 2:44 PM, Adrian Hunter adrian.hun...@nokia.com wrote: Go through the driver's set_power() functions rather than calling regulator_enable/disable() directly because otherwise pbias configuration for MMC1 is not done. Hi Chris, Are you OK to queue this patch as bug fix. Rest of the patches of this series is either merged or not needed. Should I rebase and repost this alone ? FWIW: Acked-by: Balaji T K balaj...@ti.com Thanks, I've pushed this to mmc-next for 3.2 now. - Chris. -- Chris Ball c...@laptop.org http://printf.net/ One Laptop Per Child -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Thu, 13 Oct 2011, Grant Likely wrote: For the deferred case; here is an example of the additional constraint. Consider the following hierarchy: -A +-B | +-C | +-D | +-E +-F +-G dpm_list could be ordered in at least the following ways (depending on exactly when devices get registered). There are many permutation, but children are always be listed after its direct parent. 1) ABECDFG (breadth first) 2) AEBFGCD (breadth first) 3) ABCDEFG (depth first) 4) AEFGBCD (depth first) Now, assume that device B depends on device F, and also assume that there is no way either to express that in the hierarchy or even for the constraint to be known at device registration time (the is exactly the situation we're dealing with on embedded platforms). Only the driver for B knows that it needs a resource provided by F's driver. So, the situation becomes that the ordering of dpm_list must now also be sorted so that non-tree dependencies are also accounted for. Of the list above, only sort order 4 meets the new constraint. The question then becomes, how can the dpm_list get resorted dynamically at runtime to ensure that the new constraints are always met without breaking old ones. Here are some options I can think of: 1) When a deferred probe succeeds, move the deferred device to the end of the dpm_list. Then the new sort order might be one of: 1) AECDFGB 2) AEFGCDB 3) ACDEFGB 4) AEFGCDB However, that approach breaks the guarantee that children appear after their parents (C D appear before B in all cases above) How can a device acquire children before it has a driver? 2a) When a deferred probe succeeds, move the deferred device and it's entire child hierarchy to the end of the list to become one of: This is the same as the above, under the reasonable assumption that devices without drivers can't have any children. Or to be more carefully precise, that a device with children won't need to defer a probe. We can check that easily enough: Fail a deferral if the device already has children. 2b) alternately, when *any* probe succeeds, move the deferred device and its children to the end of the list. This continues to maintain the parent-child relationship, and it ensures that the dpm_list is always also sorted in probe-order (devices bound to drivers will collect at the end of the list). We do not want to move entire hierarchies. And in fact, even in 1 or 2a, we would have to do the move from _within_ the deferred probe routine, not afterward, to make sure that it occurs before any children are added (and after all the prerequisite devices have been registered). 3) Add devices to dpm_list after successful probe instead of at device_add time. Potential problem: this means that only devices with drivers actually get suspend/resume calls. I don't know nearly enough about the PM subsystem, but I suspect that this is a problem. Yes, this is no good. 4) ignore parent-child relationships entirely and just move devices to the end of the list on successful probe. If it probed successfully, then it can be successfully suspended regardless of whether it has any children. That breaks the parent-child ordering, but guarantees the probe order ordering. Any devices without drivers will end up collecting at the beginning of the list, and will be suspended after all the devices with drivers. Problem: Same as with 3, I suspect that even devices without drivers need to process PM suspend, which makes this option unworkable. ... For my money, I think that option 2b shows the most promise despite the potential O(N^2) complexity. There may be a better algorithm for doing the runtime reordering that isn't O(N^2) that I haven't thought of. Having a list that is strongly ordered both on hierarchy *and* probe order I think is the right thing to do, even without deferred probe support. The answer is 1, but do the move at the appropriate time within the probe routine. Alan Stern -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Thu, 13 Oct 2011, Grant Likely wrote: However it's worth pointing out right at the start that we already have device_pm_move_before(), device_pm_move_after(), and device_pm_move_last(). They are intended specifically to provide drivers with a way of making sure that dpm_list is in the right order -- people have been aware of these issues for some time. I saw those. I also notice that they are only used by device_move() when reparenting a device (which is another wrinkle I hadn't though about). Reparenting a device becomes problematic if the probe order is also represented in the list. If device_move() gets called, then any implicit probe-order sorting for that device would be lost. I also notice that device_move disregards any children when moving a device, which could also be a problem. Although it looks like the only users of device_move are: drivers/media/video/pvrusb2/pvrusb2-v4l2.c drivers/s390/cio/device.c net/bluetooth/hci_sysfs.c net/bluetooth/rfcomm/tty.c So it may not be significant to adapt. I trust that very little will be needed. I haven't checked the existing callers, but it's reasonable to require that a device being moved not have any children. Alan Stern -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Fri, Oct 14, 2011 at 9:37 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 13 Oct 2011, Grant Likely wrote: For the deferred case; here is an example of the additional constraint. Consider the following hierarchy: -A +-B | +-C | +-D | +-E +-F +-G dpm_list could be ordered in at least the following ways (depending on exactly when devices get registered). There are many permutation, but children are always be listed after its direct parent. 1) ABECDFG (breadth first) 2) AEBFGCD (breadth first) 3) ABCDEFG (depth first) 4) AEFGBCD (depth first) Now, assume that device B depends on device F, and also assume that there is no way either to express that in the hierarchy or even for the constraint to be known at device registration time (the is exactly the situation we're dealing with on embedded platforms). Only the driver for B knows that it needs a resource provided by F's driver. So, the situation becomes that the ordering of dpm_list must now also be sorted so that non-tree dependencies are also accounted for. Of the list above, only sort order 4 meets the new constraint. The question then becomes, how can the dpm_list get resorted dynamically at runtime to ensure that the new constraints are always met without breaking old ones. Here are some options I can think of: 1) When a deferred probe succeeds, move the deferred device to the end of the dpm_list. Then the new sort order might be one of: 1) AECDFGB 2) AEFGCDB 3) ACDEFGB 4) AEFGCDB However, that approach breaks the guarantee that children appear after their parents (C D appear before B in all cases above) How can a device acquire children before it has a driver? There are a few potential situations in embedded systems (or at least nothing currently prevents it) where platform setup code constructs a device hierarchy without the aid of device drivers, and it is still possible for a parent device to be attached to a driver. IIUC, SPARC creates an entire hierarchy of platform_devices from all the nodes in the OpenFirmware device tree, and any of those devices can be bound to a driver. I don't like that approach, but at the very least it needs to be guarded against. On Fri, Oct 14, 2011 at 9:39 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 13 Oct 2011, Grant Likely wrote: I saw those. I also notice that they are only used by device_move() when reparenting a device (which is another wrinkle I hadn't though about). Reparenting a device becomes problematic if the probe order is also represented in the list. If device_move() gets called, then any implicit probe-order sorting for that device would be lost. I also notice that device_move disregards any children when moving a device, which could also be a problem. Although it looks like the only users of device_move are: drivers/media/video/pvrusb2/pvrusb2-v4l2.c drivers/s390/cio/device.c net/bluetooth/hci_sysfs.c net/bluetooth/rfcomm/tty.c So it may not be significant to adapt. I trust that very little will be needed. I haven't checked the existing callers, but it's reasonable to require that a device being moved not have any children. Yes, that does indeed simplify the issue considerably. Let's do that. So, for this patch, there will be two bits added. First, don't allow deferral on devices with children, and second a successful probe (deferred or otherwise) should always move a device to the end of the dap_list if it doesn't have children to guarantee that the list order satisfies both the hierarchical and probe order. Manjunath, do you think you can prototype this? g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Fri, 14 Oct 2011, Grant Likely wrote: How can a device acquire children before it has a driver? There are a few potential situations in embedded systems (or at least nothing currently prevents it) where platform setup code constructs a device hierarchy without the aid of device drivers, and it is still possible for a parent device to be attached to a driver. IIUC, SPARC creates an entire hierarchy of platform_devices from all the nodes in the OpenFirmware device tree, and any of those devices can be bound to a driver. I don't like that approach, but at the very least it needs to be guarded against. Do these devices ever require deferred probes? On Fri, Oct 14, 2011 at 9:39 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 13 Oct 2011, Grant Likely wrote: I saw those. �I also notice that they are only used by device_move() when reparenting a device (which is another wrinkle I hadn't though about). �Reparenting a device becomes problematic if the probe order is also represented in the list. �If device_move() gets called, then any implicit probe-order sorting for that device would be lost. I also notice that device_move disregards any children when moving a device, which could also be a problem. Although it looks like the only users of device_move are: drivers/media/video/pvrusb2/pvrusb2-v4l2.c drivers/s390/cio/device.c net/bluetooth/hci_sysfs.c net/bluetooth/rfcomm/tty.c So it may not be significant to adapt. I trust that very little will be needed. �I haven't checked the existing callers, but it's reasonable to require that a device being moved not have any children. Yes, that does indeed simplify the issue considerably. Let's do that. So, for this patch, there will be two bits added. First, don't allow deferral on devices with children, and second a successful probe (deferred or otherwise) should always move a device to the end of the dap_list if it doesn't have children to guarantee that the list order satisfies both the hierarchical and probe order. Manjunath, do you think you can prototype this? I don't think the second part needs to be quite so invasive. Certainly drivers that never defer probes shouldn't require anything to be moved. A deferred probe _should_ move the device to the end of the list. But this needs to happen in the probe routine itself (after it has verified that all the other required devices are present and before it has registered any children) to prevent races. It can't be done in a central location. Alan Stern -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Fri, Oct 14, 2011 at 10:33 AM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 14 Oct 2011, Grant Likely wrote: How can a device acquire children before it has a driver? There are a few potential situations in embedded systems (or at least nothing currently prevents it) where platform setup code constructs a device hierarchy without the aid of device drivers, and it is still possible for a parent device to be attached to a driver. IIUC, SPARC creates an entire hierarchy of platform_devices from all the nodes in the OpenFirmware device tree, and any of those devices can be bound to a driver. I don't like that approach, but at the very least it needs to be guarded against. Do these devices ever require deferred probes? Yes, they very well might. However, I'm happy with the limitation that only leaf devices can take advantage of probe deferral. On Fri, Oct 14, 2011 at 9:39 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 13 Oct 2011, Grant Likely wrote: Yes, that does indeed simplify the issue considerably. Let's do that. So, for this patch, there will be two bits added. First, don't allow deferral on devices with children, and second a successful probe (deferred or otherwise) should always move a device to the end of the dap_list if it doesn't have children to guarantee that the list order satisfies both the hierarchical and probe order. Manjunath, do you think you can prototype this? I don't think the second part needs to be quite so invasive. Certainly drivers that never defer probes shouldn't require anything to be moved. Think about that a minute. Consider a dpm_list of devices: abcDefGh Now assume that D has an implicit dependency on G. If D gets probed first, then it will be deferred until G gets probed and then D will get retried and moved to the end of the list resulting in: abcefGhD Everything is good now for the order that things need to be suspended in. Now lets assume that the drivers get linked into the kernel in a different order (or the modules get loaded in a different order) and G gets probed first, followed by D. No deferral occurred and so no reordering occurs, resulting in: abcDefGh (unchanged) But now suspend is broken because D depends on G, but G will be suspended before D. This looks and smells like a bug to me. In fact, even without using probe deferral it looks like a bug because the dap_list isn't taking into account the fact that there are very likely to be implicit dependencies between devices that are not represented in the device hierarchy (maybe not common in PCs, but certainly is the case for embedded). But, it is also easy to solve by ensuring the dap_list is also probe-order sorted. A deferred probe _should_ move the device to the end of the list. But this needs to happen in the probe routine itself (after it has verified that all the other required devices are present and before it has registered any children) to prevent races. It can't be done in a central location. I'm really concerned about drivers having to implement this and not getting it correct; particularly when moving a device to the end of the list is cheap, and it shouldn't be a problem to do the move unconditionally after a driver has been matches, but before probe() is called. We may be able to keep watch to make sure that drivers using probe deferral are also reordering themselves, but that does nothing for the cases described above where the link order of the drivers determines probe order, not the dap_list order. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Fri, 14 Oct 2011, Grant Likely wrote: I don't think the second part needs to be quite so invasive. Certainly drivers that never defer probes shouldn't require anything to be moved. Think about that a minute. Consider a dpm_list of devices: abcDefGh Now assume that D has an implicit dependency on G. If D gets probed first, then it will be deferred until G gets probed and then D will get retried and moved to the end of the list resulting in: abcefGhD Everything is good now for the order that things need to be suspended in. Now lets assume that the drivers get linked into the kernel in a different order (or the modules get loaded in a different order) and G gets probed first, followed by D. No deferral occurred and so no reordering occurs, resulting in: abcDefGh (unchanged) But now suspend is broken because D depends on G, but G will be suspended before D. However D sometimes does defer probes. Therefore the device always needs to be moved, even though this particular probe wasn't deferred. This looks and smells like a bug to me. In fact, even without using probe deferral it looks like a bug because the dap_list isn't taking into account the fact that there are very likely to be implicit dependencies between devices that are not represented in the device hierarchy (maybe not common in PCs, but certainly is the case for embedded). But, it is also easy to solve by ensuring the dap_list is also probe-order sorted. A deferred probe _should_ move the device to the end of the list. �But this needs to happen in the probe routine itself (after it has verified that all the other required devices are present and before it has registered any children) to prevent races. �It can't be done in a central location. I'm really concerned about drivers having to implement this and not getting it correct; particularly when moving a device to the end of the list is cheap, and it shouldn't be a problem to do the move unconditionally after a driver has been matches, but before probe() is called. But that's too early. What if D gets moved to the end of the list, then G gets added to the list and probed, and then D's probe succeeds? And after the probe returns is too late, because by then there may well be child devices. We may be able to keep watch to make sure that drivers using probe deferral are also reordering themselves, but that does nothing for the cases described above where the link order of the drivers determines probe order, not the dap_list order. Devices need to be moved whenever they have any external dependencies, regardless of the particular order they get probed in. Alan Stern -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Fri, Oct 14, 2011 at 11:33 AM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 14 Oct 2011, Grant Likely wrote: I don't think the second part needs to be quite so invasive. Certainly drivers that never defer probes shouldn't require anything to be moved. Think about that a minute. Consider a dpm_list of devices: abcDefGh Now assume that D has an implicit dependency on G. If D gets probed first, then it will be deferred until G gets probed and then D will get retried and moved to the end of the list resulting in: abcefGhD Everything is good now for the order that things need to be suspended in. Now lets assume that the drivers get linked into the kernel in a different order (or the modules get loaded in a different order) and G gets probed first, followed by D. No deferral occurred and so no reordering occurs, resulting in: abcDefGh (unchanged) But now suspend is broken because D depends on G, but G will be suspended before D. However D sometimes does defer probes. Therefore the device always needs to be moved, even though this particular probe wasn't deferred. Yes, that's part of my point. This looks and smells like a bug to me. In fact, even without using probe deferral it looks like a bug because the dap_list isn't taking into account the fact that there are very likely to be implicit dependencies between devices that are not represented in the device hierarchy (maybe not common in PCs, but certainly is the case for embedded). But, it is also easy to solve by ensuring the dap_list is also probe-order sorted. A deferred probe _should_ move the device to the end of the list. But this needs to happen in the probe routine itself (after it has verified that all the other required devices are present and before it has registered any children) to prevent races. It can't be done in a central location. I'm really concerned about drivers having to implement this and not getting it correct; particularly when moving a device to the end of the list is cheap, and it shouldn't be a problem to do the move unconditionally after a driver has been matches, but before probe() is called. But that's too early. What if D gets moved to the end of the list, then G gets added to the list and probed, and then D's probe succeeds? So the argument is that if really_probe() called dpm_move_last() immediately before the dev-bus-probe()/drv-probe() call then there is a race condition that G could get both registered and probed before D's probe() starts using G's resources. And so, the list would still have G after D which is in the wrong order. Do I understand correctly? And after the probe returns is too late, because by then there may well be child devices. Agreed, after probe returns is definitely too late. We may be able to keep watch to make sure that drivers using probe deferral are also reordering themselves, but that does nothing for the cases described above where the link order of the drivers determines probe order, not the dap_list order. Devices need to be moved whenever they have any external dependencies, regardless of the particular order they get probed in. I suspect this gets messy in a hurry, but perhaps it is worth trying. So, any device that makes use of a PHY, GPIO line, codec, etc will need to call dpm_move_last() before adding child devices, correct? I'd be much happier to find a way to do this in core code though. And there is still a potential race condition here. For example, if G is in the middle of it's probe routine, and D gets probed between G registering GPIOs and calling dpm_move_last(), then we're in the same boat again. I think the window for this race can be considered to be of the same magnitude as the moved to early race described above. I need to think about this more... g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- 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 2/5] drivercore: Add driver probe deferral mechanism
On Fri, 14 Oct 2011, Grant Likely wrote: However D sometimes does defer probes. �Therefore the device always needs to be moved, even though this particular probe wasn't deferred. Yes, that's part of my point. Okay, then we agree. :-) So the argument is that if really_probe() called dpm_move_last() immediately before the dev-bus-probe()/drv-probe() call then there is a race condition that G could get both registered and probed before D's probe() starts using G's resources. And so, the list would still have G after D which is in the wrong order. Do I understand correctly? Exactly so. Devices need to be moved whenever they have any external dependencies, regardless of the particular order they get probed in. I suspect this gets messy in a hurry, but perhaps it is worth trying. So, any device that makes use of a PHY, GPIO line, codec, etc will need to call dpm_move_last() before adding child devices, correct? Pretty much, yes. Unless the driver somehow knows that it will become sufficiently idle at an early suspend stage (like the prepare callback) that it doesn't need to use the PHY, GPIO, codec, etc. I'd be much happier to find a way to do this in core code though. And there is still a potential race condition here. For example, if G is in the middle of it's probe routine, and D gets probed between G registering GPIOs and calling dpm_move_last(), then we're in the same boat again. Of course, this means that G must call dpm_move_last() _before_ registering its GPIOs. So the overall flow of a probe routine is simple enough: 1. Check that all the resources you need are available. 2. If not, defer your probe. If yes, call dpm_move_last(). 3. Finish the probe, including registration of resources that will be available to other drivers (such as child devices). I think the window for this race can be considered to be of the same magnitude as the moved to early race described above. I need to think about this more... Alan Stern -- 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 2/5] drivercore: Add driver probe deferral mechanism
On 10/14/2011 10:20 AM, Grant Likely wrote: On Fri, Oct 14, 2011 at 10:33 AM, Alan Sternst...@rowland.harvard.edu wrote: On Fri, 14 Oct 2011, Grant Likely wrote: How can a device acquire children before it has a driver? There are a few potential situations in embedded systems (or at least nothing currently prevents it) where platform setup code constructs a device hierarchy without the aid of device drivers, and it is still possible for a parent device to be attached to a driver. IIUC, SPARC creates an entire hierarchy of platform_devices from all the nodes in the OpenFirmware device tree, and any of those devices can be bound to a driver. I don't like that approach, but at the very least it needs to be guarded against. Do these devices ever require deferred probes? Yes, they very well might. However, I'm happy with the limitation that only leaf devices can take advantage of probe deferral. I have: I2C-Bus-A +--Mux-I2C (controlled by parent I2C-Bus-A) +---I2C-Bus-1 | +--GPIO-Expander-A | +---I2C-Bus-2 +--GPIO-Expander-B These all have a parent/child relationship so no deferral is needed, so far so good. Then this: MDIO-Bus-A +---Mux-MDIO (controlled by GPIO-Expander-A) +---MDIO-Bus-1 | +---MDIO-Bus-2 +---PHY-1 | +---PHY-2 In this case the driver for Mux-MDIO needs to be deferred until *both* MDIO-Bus-A's driver *and* GPIO-Expander-B's driver are loaded. A perfect use case for the patch. Would you consider Mux-MDIO to be a 'leaf device'? If not, then I have real problems with 'the limitation that only leaf devices can take advantage of probe deferral' David Daney -- 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
