* [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management @ 2011-02-13 5:39 Philip Rakity 2011-02-13 10:25 ` Pierre Tardy 0 siblings, 1 reply; 14+ messages in thread From: Philip Rakity @ 2011-02-13 5:39 UTC (permalink / raw) To: Pierre Tardy; +Cc: linux-mmc Pierre, I am preparing a patch to just diable to interrupt line to the SD controller so global interrupts are not locked out. This will allow mdelay to work correctly in the routines the set_ios() calls. The pm_ calls remove the lock before being called and restore it afterwards. Is this just to ensure interrupts are globally enabled ? If so, I can simplify my patch deleting the original spin_lock calls that surround the pm_ calls. Philip >From 782c474f1abe992a203ad76594db025414332e67 Mon Sep 17 00:00:00 2001 From: Philip Rakity <prakity@marvell.com> Date: Tue, 25 Jan 2011 16:04:42 -0800 Subject: [PATCH 1/2] sdhci: sdhci.c: Do not disable global interrupts while waiting in set_ios holding the spin_lock with all interrupts disabled is not good form when doing mdelay. set_ios calls routines that need to delay for power to stabalize or for chip issues when quirks are defined. disable only our interupt and spin_lock on the chip instead of spin_lock with all interrupts disabled. Signed-off-by: Philip Rakity <prakity@marvell.com> Signed-off-by: Mark F. Brown <markb@marvell.com> --- drivers/mmc/host/sdhci.c | 15 ++++++++------- 1 files changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 655617c..1a88303 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1161,13 +1161,13 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq) static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) { struct sdhci_host *host; - unsigned long flags; unsigned int lastclock; u8 ctrl; host = mmc_priv(mmc); - spin_lock_irqsave(&host->lock, flags); + disable_irq(host->irq); + spin_lock(&host->lock); if (host->flags & SDHCI_DEVICE_DEAD) goto out; @@ -1180,14 +1180,14 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) lastclock = host->iosclock; host->iosclock = ios->clock; if (lastclock == 0 && ios->clock != 0) { - spin_unlock_irqrestore(&host->lock, flags); + spin_unlock(&host->lock); pm_runtime_get_sync(host->mmc->parent); - spin_lock_irqsave(&host->lock, flags); + spin_lock(&host->lock); } else if (lastclock != 0 && ios->clock == 0) { - spin_unlock_irqrestore(&host->lock, flags); + spin_unlock(&host->lock); pm_runtime_mark_last_busy(host->mmc->parent); pm_runtime_put_autosuspend(host->mmc->parent); - spin_lock_irqsave(&host->lock, flags); + spin_lock(&host->lock); } /* no need to configure the rest.. */ if (host->iosclock == 0) @@ -1257,7 +1257,8 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) out: mmiowb(); - spin_unlock_irqrestore(&host->lock, flags); + spin_unlock(&host->lock); + enable_irq(host->irq); } static int sdhci_get_ro(struct mmc_host *mmc) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-13 5:39 [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management Philip Rakity @ 2011-02-13 10:25 ` Pierre Tardy 2011-02-13 17:57 ` Philip Rakity 0 siblings, 1 reply; 14+ messages in thread From: Pierre Tardy @ 2011-02-13 10:25 UTC (permalink / raw) To: Philip Rakity; +Cc: linux-mmc On Sun, Feb 13, 2011 at 6:39 AM, Philip Rakity <prakity@marvell.com> wrote: > > Pierre, > > I am preparing a patch to just diable to interrupt line to the SD controller so global interrupts are not locked out. I think I already have seen previous version of that patch earlier. It does not sound like a bad idea. However, disabling all interrupt is really cheap, and disabling only one is a bit more expensive. AFAIK, spin_lock and co are really meant for very short time locks. > This > will allow mdelay to work correctly in the routines the set_ios() calls. This sounds a little bit worse. mdelay is never good, as this is active wait, that is loosing tons of good cpu cycles. Do we really need those? Can we do the same thing asynchronous? > > The pm_ calls remove the lock before being called and restore it afterwards. Is this just to ensure interrupts are globally > enabled ? At least on my platform, pm_ calls cannot be done in atomic context, and sdhci runtime_pm flow will call set_ios() back, so you will re-enter the function. > If so, I can simplify my patch deleting the original spin_lock calls that surround the pm_ calls. No, you cannot safely remove the spin_lock calls here. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-13 10:25 ` Pierre Tardy @ 2011-02-13 17:57 ` Philip Rakity 2011-02-14 5:50 ` Philip Rakity 0 siblings, 1 reply; 14+ messages in thread From: Philip Rakity @ 2011-02-13 17:57 UTC (permalink / raw) To: Pierre Tardy; +Cc: linux-mmc On Feb 13, 2011, at 2:25 AM, Pierre Tardy wrote: > On Sun, Feb 13, 2011 at 6:39 AM, Philip Rakity <prakity@marvell.com> wrote: >> >> Pierre, >> >> I am preparing a patch to just diable to interrupt line to the SD controller so global interrupts are not locked out. > I think I already have seen previous version of that patch earlier. It > does not sound like a bad idea. However, disabling all interrupt is > really cheap, and disabling only one is a bit more expensive. AFAIK, > spin_lock and co are really meant for very short time locks. > >> This >> will allow mdelay to work correctly in the routines the set_ios() calls. > This sounds a little bit worse. mdelay is never good, as this is > active wait, that is loosing tons of good cpu cycles. do not need active waiting. Good point. > Do we really need those? Can we do the same thing asynchronous? This might be possible without a restructure to separate out all the sub-calls that are done in set_ios. Otherwise we would require a restructure of the code in core/ to not use set_ios but to call only the function they need via separate call in's. (for example setting the width or power) and the associated changes in all the drivers in host. Rather then do mdelay we could use: (code from core.h) static inline void mmc_delay(unsigned int ms) { if (ms < 1000 / HZ) { cond_resched(); mdelay(ms); } else { msleep(ms); } } Since the whole point of locking out the one interrupt line is to let everything else work this look attractive. -- Opintions Please -- > >> >> The pm_ calls remove the lock before being called and restore it afterwards. Is this just to ensure interrupts are globally >> enabled ? > At least on my platform, pm_ calls cannot be done in atomic context, > and sdhci runtime_pm flow will call set_ios() back, so you will > re-enter the function. ok > >> If so, I can simplify my patch deleting the original spin_lock calls that surround the pm_ calls. > No, you cannot safely remove the spin_lock calls here. ok ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-13 17:57 ` Philip Rakity @ 2011-02-14 5:50 ` Philip Rakity 2011-02-14 9:29 ` Pierre Tardy 0 siblings, 1 reply; 14+ messages in thread From: Philip Rakity @ 2011-02-14 5:50 UTC (permalink / raw) To: Pierre Tardy; +Cc: linux-mmc Pierre, are you comfortable with this ? Philip >From 85b200161edac97fc2817ad3f213795df99cd7e8 Mon Sep 17 00:00:00 2001 From: Philip Rakity <prakity@marvell.com> Date: Tue, 25 Jan 2011 16:04:42 -0800 Subject: [PATCH] sdhci: sdhci.c: Do not disable global interrupts while waiting in set_ios holding the spin_lock with all interrupts disabled is not good form when doing mdelay. set_ios calls routines that need to delay for power to stabalize or for chip issues when quirks are defined. disable only our interupt and spin_lock on the chip instead of spin_lock with all interrupts disabled. use mmc_delay (if possible) rather then mdelay to avoid active waits. sdhci_reset not changed. still uses mdelay as we do not know if interrupts have been disabled so we play "safe" Signed-off-by: Philip Rakity <prakity@marvell.com> --- drivers/mmc/core/core.h | 12 ------------ drivers/mmc/host/sdhci.c | 26 +++++++++++++++++--------- include/linux/mmc/host.h | 11 +++++++++++ 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index 20b1c08..146b6f6 100644 --- a/drivers/mmc/core/core.h +++ b/drivers/mmc/core/core.h @@ -11,8 +11,6 @@ #ifndef _MMC_CORE_CORE_H #define _MMC_CORE_CORE_H -#include <linux/delay.h> - #define MMC_CMD_RETRIES 3 struct mmc_bus_ops { @@ -43,16 +41,6 @@ void mmc_set_bus_width_ddr(struct mmc_host *host, unsigned int width, u32 mmc_select_voltage(struct mmc_host *host, u32 ocr); void mmc_set_timing(struct mmc_host *host, unsigned int timing); -static inline void mmc_delay(unsigned int ms) -{ - if (ms < 1000 / HZ) { - cond_resched(); - mdelay(ms); - } else { - msleep(ms); - } -} - void mmc_rescan(struct work_struct *work); void mmc_start_host(struct mmc_host *host); void mmc_stop_host(struct mmc_host *host); diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 655617c..2d7fbd1 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) return; } timeout--; - mdelay(1); + mmc_delay(1); } clk |= SDHCI_CLOCK_CARD_EN; @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) * can apply clock after applying power */ if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) - mdelay(10); + mmc_delay(10); } /*****************************************************************************\ @@ -1161,22 +1161,21 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq) static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) { struct sdhci_host *host; - unsigned long flags; unsigned int lastclock; + unsigned long flags; u8 ctrl; host = mmc_priv(mmc); - spin_lock_irqsave(&host->lock, flags); - if (host->flags & SDHCI_DEVICE_DEAD) - goto out; + return; /* * get/put runtime_pm usage counter at ios->clock transitions * We need to do it before any other chip access, as sdhci could * be power gated */ + spin_lock_irqsave(&host->lock, flags); lastclock = host->iosclock; host->iosclock = ios->clock; if (lastclock == 0 && ios->clock != 0) { @@ -1189,9 +1188,18 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) pm_runtime_put_autosuspend(host->mmc->parent); spin_lock_irqsave(&host->lock, flags); } + spin_unlock_irqrestore(&host->lock, flags); + /* no need to configure the rest.. */ if (host->iosclock == 0) - goto out; + return; + + /* disable local interrupts and lock structures so + * that mmc_delay can be scheduled. This cannot happen + * if interrupts are globally disabled + */ + disable_irq(host->irq); + spin_lock(&host->lock); /* * Reset the chip on each power off. @@ -1255,9 +1263,9 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) if(host->quirks & SDHCI_QUIRK_RESET_CMD_DATA_ON_IOS) sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA); -out: mmiowb(); - spin_unlock_irqrestore(&host->lock, flags); + spin_unlock(&host->lock); + enable_irq(host->irq); } static int sdhci_get_ro(struct mmc_host *mmc) diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index bcb793e..d9edc06 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -15,6 +15,7 @@ #include <linux/mmc/core.h> #include <linux/mmc/pm.h> +#include <linux/delay.h> struct mmc_ios { unsigned int clock; /* clock rate */ @@ -326,5 +327,15 @@ static inline int mmc_card_is_powered_resumed(struct mmc_host *host) return host->pm_flags & MMC_PM_KEEP_POWER; } +static inline void mmc_delay(unsigned int ms) +{ + if (ms < 1000 / HZ) { + cond_resched(); + mdelay(ms); + } else { + msleep(ms); + } +} + #endif -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 5:50 ` Philip Rakity @ 2011-02-14 9:29 ` Pierre Tardy 2011-02-14 9:55 ` Wolfram Sang ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Pierre Tardy @ 2011-02-14 9:29 UTC (permalink / raw) To: Philip Rakity; +Cc: linux-mmc On Mon, Feb 14, 2011 at 6:50 AM, Philip Rakity <prakity@marvell.com> wrote: > > Pierre, > > are you comfortable with this ? The mmc_delay() looks hackish at the first sight. Looks like Pierre Ossman was the author, he must have his reasons. It looks like the mdelay are not in the fastpath, so why bother. And, more important, you will do cond_resched while holding you spinlock, which is *bad*. What if the mmc stack will call you again from another thread? deadlock... > diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h > -static inline void mmc_delay(unsigned int ms) why do you want to remove it from here ? It is still used by the core code. > @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) > return; > } > timeout--; > - mdelay(1); > + mmc_delay(1); This looks like timeout code that never happen in normal condition. > @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) > * can apply clock after applying power > */ > if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) > - mdelay(10); > + mmc_delay(10); Do you need this quirk in your platform? Pierre ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 9:29 ` Pierre Tardy @ 2011-02-14 9:55 ` Wolfram Sang 2011-02-14 17:27 ` Philip Rakity 2011-02-14 18:34 ` Philip Rakity 2011-02-15 3:09 ` Philip Rakity 2 siblings, 1 reply; 14+ messages in thread From: Wolfram Sang @ 2011-02-14 9:55 UTC (permalink / raw) To: Pierre Tardy; +Cc: Philip Rakity, linux-mmc > It looks like the mdelay are not in the fastpath, so why bother. On embedded systems, we had audio streaming or CAN communication or similar running in parallel. Currently, those mdelays (there are a couple, sadly) produce significant latencies, e.g. when inserting/removing a card. The latter gets especially bad with BROKEN_CARD_DETECTION. The proper solution is probably to check the locking and see if it can be made fine-granular, I'd assume? Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 9:55 ` Wolfram Sang @ 2011-02-14 17:27 ` Philip Rakity 2011-02-15 9:13 ` Wolfram Sang 0 siblings, 1 reply; 14+ messages in thread From: Philip Rakity @ 2011-02-14 17:27 UTC (permalink / raw) To: Wolfram Sang; +Cc: Pierre Tardy, linux-mmc I wonder if I can remove the locking (spin_lock). The disable and enable irq should be enough. Thoughts .. Philip On Feb 14, 2011, at 1:55 AM, Wolfram Sang wrote: > >> It looks like the mdelay are not in the fastpath, so why bother. > > On embedded systems, we had audio streaming or CAN communication or similar > running in parallel. Currently, those mdelays (there are a couple, sadly) > produce significant latencies, e.g. when inserting/removing a card. The latter > gets especially bad with BROKEN_CARD_DETECTION. > > The proper solution is probably to check the locking and see if it can be made > fine-granular, I'd assume? > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 17:27 ` Philip Rakity @ 2011-02-15 9:13 ` Wolfram Sang 0 siblings, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2011-02-15 9:13 UTC (permalink / raw) To: Philip Rakity; +Cc: Pierre Tardy, linux-mmc On Mon, Feb 14, 2011 at 09:27:52AM -0800, Philip Rakity wrote: > > I wonder if I can remove the locking (spin_lock). The disable and enable irq should be enough. I discussed this shortly with Sascha yesterday and we agree the core serializes requests to the host (by means of mmc_claim_host and synchronus requests). Protecting only the interrupts works for drivers like pxa and mxc. Such locking might or might not be not enough for sdhci, but that would have to be carefully evaluated. Should be worth trying, though (unless we missed something). Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 9:29 ` Pierre Tardy 2011-02-14 9:55 ` Wolfram Sang @ 2011-02-14 18:34 ` Philip Rakity 2011-02-14 18:41 ` Tardy, Pierre 2011-02-15 3:09 ` Philip Rakity 2 siblings, 1 reply; 14+ messages in thread From: Philip Rakity @ 2011-02-14 18:34 UTC (permalink / raw) To: Pierre Tardy, Wolfram Sang; +Cc: linux-mmc On Feb 14, 2011, at 1:29 AM, Pierre Tardy wrote: > On Mon, Feb 14, 2011 at 6:50 AM, Philip Rakity <prakity@marvell.com> wrote: >> >> Pierre, >> >> are you comfortable with this ? > The mmc_delay() looks hackish at the first sight. Looks like Pierre > Ossman was the author, he must have his reasons. > > It looks like the mdelay are not in the fastpath, so why bother. > > And, more important, you will do cond_resched while holding you > spinlock, which is *bad*. > What if the mmc stack will call you again from another thread? deadlock... Assumptions -- Please Confirm ------------------------------------------- There is a one to one relationship between the calls in the core layer into the SD driver. Each thread is independent of the other. Each thread maps into exactly one register instance for the SD driver eg --> 2 threads cannot be active at the same time that use the same registers in the SD driver. Question ------------ If these assumptions are correct how can the deadlock happen ? host->lock is held on a per thread basis. ====== > > >> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h >> -static inline void mmc_delay(unsigned int ms) > why do you want to remove it from here ? > It is still used by the core code. moved to include/linux/mmc so okay for sdhci driver. Did not want to duplicate code. > > >> @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) >> return; >> } >> timeout--; >> - mdelay(1); >> + mmc_delay(1); > This looks like timeout code that never happen in normal condition. > > >> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) >> * can apply clock after applying power >> */ >> if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) >> - mdelay(10); >> + mmc_delay(10); > Do you need this quirk in your platform? No > > > Pierre ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 18:34 ` Philip Rakity @ 2011-02-14 18:41 ` Tardy, Pierre 2011-02-14 20:17 ` Philip Rakity ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Tardy, Pierre @ 2011-02-14 18:41 UTC (permalink / raw) To: Philip Rakity, Pierre Tardy, Wolfram Sang; +Cc: linux-mmc Philip, > > And, more important, you will do cond_resched while holding you > > spinlock, which is *bad*. > > What if the mmc stack will call you again from another thread? deadlock... > Assumptions -- Please Confirm > ------------------------------------------- No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like: BUG: scheduling while in atomic. > >> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) > >> * can apply clock after applying power > >> */ > >> if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) > >> - mdelay(10); > >> + mmc_delay(10); > > Do you need this quirk in your platform? > > No Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen. Pierre --------------------------------------------------------------------- Intel Corporation SAS (French simplified joint stock company) Registered headquarters: "Les Montalets"- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 18:41 ` Tardy, Pierre @ 2011-02-14 20:17 ` Philip Rakity 2011-02-14 20:44 ` Philip Rakity 2011-02-15 8:39 ` Wolfram Sang 2 siblings, 0 replies; 14+ messages in thread From: Philip Rakity @ 2011-02-14 20:17 UTC (permalink / raw) To: Tardy, Pierre; +Cc: Pierre Tardy, Wolfram Sang, linux-mmc According to Zhangfei Gao -- android crashes when we are code to handle dual data rate because of the mdelay of 5ms that is needed for the voltage to become stable. (see my ddr patch) I have never seen this problem with android. The dual data rate code is in another patch; it adds another call into set_ios to handle this. His comment was doing mdelay stopped the system from being scheduled since mdelay is done with interrupts disabled. It also stopped the system for keeping correct time. If doing mdelay breaks android then the original code (no ddr code) is wrong since mdelay should not be issued with interrupts disabled and depending on the quirks defined the delays can be very large. Wanted to handle his concern. On Feb 14, 2011, at 10:41 AM, Tardy, Pierre wrote: > Philip, >>> And, more important, you will do cond_resched while holding you >>> spinlock, which is *bad*. >>> What if the mmc stack will call you again from another thread? deadlock... > >> Assumptions -- Please Confirm >> ------------------------------------------- > No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like: > BUG: scheduling while in atomic. > > >>>> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) >>>> * can apply clock after applying power >>>> */ >>>> if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) >>>> - mdelay(10); >>>> + mmc_delay(10); >>> Do you need this quirk in your platform? >> >> No > Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen. > > Pierre > > --------------------------------------------------------------------- > Intel Corporation SAS (French simplified joint stock company) > Registered headquarters: "Les Montalets"- 2, rue de Paris, > 92196 Meudon Cedex, France > Registration Number: 302 456 199 R.C.S. NANTERRE > Capital: 4,572,000 Euros > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 18:41 ` Tardy, Pierre 2011-02-14 20:17 ` Philip Rakity @ 2011-02-14 20:44 ` Philip Rakity 2011-02-15 8:39 ` Wolfram Sang 2 siblings, 0 replies; 14+ messages in thread From: Philip Rakity @ 2011-02-14 20:44 UTC (permalink / raw) To: Tardy, Pierre; +Cc: linux-mmc Pierre, The spin_lock may not be needed if the same thread cannot be re-entered. That was the reason for the assumptions. Sorry about not being clear. Philip On Feb 14, 2011, at 10:41 AM, Tardy, Pierre wrote: > Philip, >>> And, more important, you will do cond_resched while holding you >>> spinlock, which is *bad*. >>> What if the mmc stack will call you again from another thread? deadlock... > >> Assumptions -- Please Confirm >> ------------------------------------------- > No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like: > BUG: scheduling while in atomic. > > >>>> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) >>>> * can apply clock after applying power >>>> */ >>>> if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) >>>> - mdelay(10); >>>> + mmc_delay(10); >>> Do you need this quirk in your platform? >> >> No > Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen. > > Pierre > > --------------------------------------------------------------------- > Intel Corporation SAS (French simplified joint stock company) > Registered headquarters: "Les Montalets"- 2, rue de Paris, > 92196 Meudon Cedex, France > Registration Number: 302 456 199 R.C.S. NANTERRE > Capital: 4,572,000 Euros > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 18:41 ` Tardy, Pierre 2011-02-14 20:17 ` Philip Rakity 2011-02-14 20:44 ` Philip Rakity @ 2011-02-15 8:39 ` Wolfram Sang 2 siblings, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2011-02-15 8:39 UTC (permalink / raw) To: Tardy, Pierre; +Cc: Philip Rakity, Pierre Tardy, linux-mmc On Mon, Feb 14, 2011 at 06:41:47PM +0000, Tardy, Pierre wrote: > Philip, > > > And, more important, you will do cond_resched while holding you > > > spinlock, which is *bad*. > > > What if the mmc stack will call you again from another thread? deadlock... > > > Assumptions -- Please Confirm > > ------------------------------------------- > No need to do assumptions, schedule while holding spinlocks is bad, your kernel will oops with something like: > BUG: scheduling while in atomic. I thought the assumptions were made in order to check where the spinlock is _really_ needed. Thinking in that direction is a good aproach IMO. > > >> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) > > >> * can apply clock after applying power > > >> */ > > >> if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) > > >> - mdelay(10); > > >> + mmc_delay(10); > > > Do you need this quirk in your platform? > > > > No > Then, you dont have any mdelay in your set_ios, and you are trying to optimize something that never happen. There are some more mdelays in error-conditions which could happen occasionally (see sdhci_send_command, spinlocked by sdhci_request for example). Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management 2011-02-14 9:29 ` Pierre Tardy 2011-02-14 9:55 ` Wolfram Sang 2011-02-14 18:34 ` Philip Rakity @ 2011-02-15 3:09 ` Philip Rakity 2 siblings, 0 replies; 14+ messages in thread From: Philip Rakity @ 2011-02-15 3:09 UTC (permalink / raw) To: Pierre Tardy; +Cc: linux-mmc On Feb 14, 2011, at 1:29 AM, Pierre Tardy wrote: > On Mon, Feb 14, 2011 at 6:50 AM, Philip Rakity <prakity@marvell.com> wrote: >> >> Pierre, >> >> are you comfortable with this ? > The mmc_delay() looks hackish at the first sight. Looks like Pierre > Ossman was the author, he must have his reasons. > > It looks like the mdelay are not in the fastpath, so why bother. > > And, more important, you will do cond_resched while holding you > spinlock, which is *bad*. > What if the mmc stack will call you again from another thread? deadlock... The original code used a spin_lock_irqsave and a spin_lock_irqrestore so if the code was called again the deadlock surely would have happened by now. > > >> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h >> -static inline void mmc_delay(unsigned int ms) > why do you want to remove it from here ? > It is still used by the core code. > > >> @@ -1043,7 +1043,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) >> return; >> } >> timeout--; >> - mdelay(1); >> + mmc_delay(1); > This looks like timeout code that never happen in normal condition. > > >> @@ -1108,7 +1108,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power) >> * can apply clock after applying power >> */ >> if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) >> - mdelay(10); >> + mmc_delay(10); > Do you need this quirk in your platform? > > > Pierre ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-02-15 9:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-02-13 5:39 [RFC} sdhci.c set_ios -- disable global interrupts and Question on Power Management Philip Rakity 2011-02-13 10:25 ` Pierre Tardy 2011-02-13 17:57 ` Philip Rakity 2011-02-14 5:50 ` Philip Rakity 2011-02-14 9:29 ` Pierre Tardy 2011-02-14 9:55 ` Wolfram Sang 2011-02-14 17:27 ` Philip Rakity 2011-02-15 9:13 ` Wolfram Sang 2011-02-14 18:34 ` Philip Rakity 2011-02-14 18:41 ` Tardy, Pierre 2011-02-14 20:17 ` Philip Rakity 2011-02-14 20:44 ` Philip Rakity 2011-02-15 8:39 ` Wolfram Sang 2011-02-15 3:09 ` Philip Rakity
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.