| Message ID | 1344637513-29383-1-git-send-email-Chunhe.Lan@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 10 August 2012, Chunhe Lan wrote: > +static inline void mmc_delay(unsigned int ms) > +{ > + if (ms < 1000 / HZ) { > + cond_resched(); > + mdelay(ms); > + } else { > + msleep(ms); > + } > +} I would actually question the point in this function to start with: The decision whether to call mdelay() or msleep() should only be based on whether you are allowed to sleep in the caller context. The idea of cond_resched(); mdelay(ms); sets off alarm bells, and I would always replace that with msleep(). Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 21 September 2012, Chunhe Lan wrote: > On 08/10/2012 09:27 AM, Arnd Bergmann wrote: > > On Friday 10 August 2012, Chunhe Lan wrote: > > > > cond_resched(); > > mdelay(ms); > > > > sets off alarm bells, and I would always replace that with msleep(). > I think that it does not replace with msleep(). > When the time of sleep is very short, program should not been scheduled > in the context. Because it expends the more time. > A time measured in miliseconds is never "very short" for the scheduler, a lot of things can happen during that time span. The code I quoted also does not care too much about accuracy, otherwise it would adapt the time in the mdelay based on whether the cond_resched() actually schedules to another thread. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/10/2012 09:27 AM, Arnd Bergmann wrote: > On Friday 10 August 2012, Chunhe Lan wrote: > >> +static inline void mmc_delay(unsigned int ms) >> +{ >> + if (ms < 1000 / HZ) { >> + cond_resched(); >> + mdelay(ms); >> + } else { >> + msleep(ms); >> + } >> +} > I would actually question the point in this function to start with: The > decision whether to call mdelay() or msleep() should only be based on > whether you are allowed to sleep in the caller context. The idea of > > > cond_resched(); > mdelay(ms); > > sets off alarm bells, and I would always replace that with msleep(). I think that it does not replace with msleep(). When the time of sleep is very short, program should not been scheduled in the context. Because it expends the more time. Thanks, Chunhe > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 24 September 2012, Chunhe Lan wrote: > OK. As you have mentioned, it would been modified to such: > > static inline void mmc_delay(unsigned int ms) > { > if (ms < 1000 / HZ) { > cond_resched(); > msleep(ms); > } else { > msleep(ms); > } > } This version would be rather broken, because it compares times in two different units (ms and jiffies), and because it does a cond_resched() directly before an msleep: both of which end up calling schedule() and being away for some time, cond_resched() for an unknown time, and msleep for a minimum time on top of that. > OR such: > > static inline void mmc_delay(unsigned int ms) > { > msleep(ms); > } That would be my preferred choice, unless someone has specific issues with this. > OR other code? Well, in principle, you could implement something like static inline void mmc_delay(unsigned int ms) { ktime_t end = ktime_add_us(ktime_get(), ms * 1000); while (1) { s64 remaining; cond_resched(); remaining = ktime_to_us(ktime_sub(end, ktime_get())); if (remaining < 0) break; udelay(min_t(u32, remaining, 100)); } } Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 24, 2012 at 8:17 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> static inline void mmc_delay(unsigned int ms) >> { >> msleep(ms); >> } > > That would be my preferred choice, unless someone has specific issues with this. If we're going to do that, then just get rid of mmc_delay and replace all calls to it with msleep(). Why bother with the inline function? There's nothing really MMC-specific about it.
On 09/21/2012 08:33 AM, Arnd Bergmann wrote: > On Friday 21 September 2012, Chunhe Lan wrote: >> On 08/10/2012 09:27 AM, Arnd Bergmann wrote: >>> On Friday 10 August 2012, Chunhe Lan wrote: >>> >>> cond_resched(); >>> mdelay(ms); >>> >>> sets off alarm bells, and I would always replace that with msleep(). >> I think that it does not replace with msleep(). >> When the time of sleep is very short, program should not been scheduled >> in the context. Because it expends the more time. >> > A time measured in miliseconds is never "very short" for the scheduler, > a lot of things can happen during that time span. The code I quoted > also does not care too much about accuracy, otherwise it would adapt > the time in the mdelay based on whether the cond_resched() actually > schedules to another thread. OK. As you have mentioned, it would been modified to such: static inline void mmc_delay(unsigned int ms) { if (ms < 1000 / HZ) { cond_resched(); msleep(ms); } else { msleep(ms); } } OR such: static inline void mmc_delay(unsigned int ms) { msleep(ms); } OR other code? Thanks, Chunhe > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index 3bdafbc..5f63d00 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 { @@ -46,16 +44,6 @@ void mmc_set_timing(struct mmc_host *host, unsigned int timing); void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type); void mmc_power_off(struct mmc_host *host); -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/include/linux/mmc/core.h b/include/linux/mmc/core.h index 1b431c7..7021658 100644 --- a/include/linux/mmc/core.h +++ b/include/linux/mmc/core.h @@ -10,6 +10,7 @@ #include <linux/interrupt.h> #include <linux/completion.h> +#include <linux/delay.h> struct request; struct mmc_data; @@ -192,6 +193,16 @@ static inline void mmc_claim_host(struct mmc_host *host) __mmc_claim_host(host, NULL); } +static inline void mmc_delay(unsigned int ms) +{ + if (ms < 1000 / HZ) { + cond_resched(); + mdelay(ms); + } else { + msleep(ms); + } +} + extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max); #endif /* LINUX_MMC_CORE_H */