Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Arnd Bergmann]

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

[PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Chunhe Lan]

Move mmc_delay() from drivers/mmc/core/core.h to
include/linux/mmc/core.h. So when other functions
call it with include syntax using <linux/mmc/core.h>
of absolute path rather than "../core/core.h" of
relative path.

Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
Cc: Chris Ball <cjb@laptop.org>
---
 drivers/mmc/core/core.h  |   12 ------------
 include/linux/mmc/core.h |   11 +++++++++++
 2 files changed, 11 insertions(+), 12 deletions(-)

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 */
-- 
1.7.6.5

Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Arnd Bergmann]

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

Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Chunhe Lan]

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
>

Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Arnd Bergmann]

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

Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Tabi Timur-B04825]

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 wit=
h 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.

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [PATCH v3 1/2] mmc: Move mmc_delay() to include/linux/mmc/core.h

[Chunhe Lan]

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
>