Prev: PROBLEM: SIS7019 stops recording after 42 min
Next: [PATCH v3] alsa: ASoC: Add JZ4740 codec driver
From: Matt Fleming on 19 Jun 2010 10:50
On Sat, 19 Jun 2010 07:08:23 +0200, Lars-Peter Clausen <lars(a)metafoo.de> wrote:
> This patch adds support for the mmc controller on JZ4740 SoCs.
>

Hey Lars-Peter,

I had a quick look over this patch and it looks OK. Just a few comments.

> +static void jz4740_mmc_timeout(unsigned long data)
> +{
> + struct jz4740_mmc_host *host = (struct jz4740_mmc_host *)data;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + if (!host->waiting) {
> + spin_unlock_irqrestore(&host->lock, flags);
> + return;
> + }
> +
> + host->waiting = 0;
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + host->req->cmd->error = -ETIMEDOUT;
> + jz4740_mmc_request_done(host);
> +}
> +

Taking a spinlock and disabling interrupts seems like too much overhead
to simply test and clear a bit. Wouldn't it be better to implement this
with test_and_clear_bit(), which on MIPS will likely be implemented with
ll/sc instructions? It's particularly important to keep this
low-overhead since this bit is modified in the interrupt handler.

> +static void jz4740_mmc_request_done(struct jz4740_mmc_host *host)
> +{
> + struct mmc_request *req;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + req = host->req;
> + host->req = NULL;
> + host->waiting = 0;
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + if (!unlikely(req))
> + return;
> +
> + mmc_request_done(host->mmc, req);
> +}
> +

Am I right in thinking that this spinlock guards against the interrupt
handler and the timeout function running at the same time? So it's not
really possible to drop the spinlock from here?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Lars-Peter Clausen on 19 Jun 2010 11:40
Hi

Matt Fleming wrote:
> On Sat, 19 Jun 2010 07:08:23 +0200, Lars-Peter Clausen
<lars(a)metafoo.de> wrote:
>> This patch adds support for the mmc controller on JZ4740 SoCs.
>>
>
> Hey Lars-Peter,
>
> I had a quick look over this patch and it looks OK. Just a few comments.
>
>> +static void jz4740_mmc_timeout(unsigned long data)
>> +{
>> + struct jz4740_mmc_host *host = (struct jz4740_mmc_host *)data;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + if (!host->waiting) {
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + return;
>> + }
>> +
>> + host->waiting = 0;
>> +
>> + spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + host->req->cmd->error = -ETIMEDOUT;
>> + jz4740_mmc_request_done(host);
>> +}
>> +
>
> Taking a spinlock and disabling interrupts seems like too much overhead
> to simply test and clear a bit. Wouldn't it be better to implement this
> with test_and_clear_bit(), which on MIPS will likely be implemented with
> ll/sc instructions? It's particularly important to keep this
> low-overhead since this bit is modified in the interrupt handler.
>
Sounds like a good idea :)
>> +static void jz4740_mmc_request_done(struct jz4740_mmc_host *host)
>> +{
>> + struct mmc_request *req;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + req = host->req;
>> + host->req = NULL;
>> + host->waiting = 0;
>> + spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + if (!unlikely(req))
>> + return;
>> +
>> + mmc_request_done(host->mmc, req);
>> +}
>> +
>
> Am I right in thinking that this spinlock guards against the interrupt
> handler and the timeout function running at the same time? So it's not
> really possible to drop the spinlock from here?
>
Yes, at least that is what it was meant for. But it was there before
the waiting bit and right now I can not construct any code paths that
could lead to jz4740_mmc_request_done from two paths at the same time.
The timer wont call it if the waiting bit is not set and the irq
handler won't wake the threaded irq handler if the waiting bit is not
set. I'll think a bit more about it and eventually drop the spinlock here.
Thanks for your review :)

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Matt Fleming on 29 Jun 2010 16:20
On Mon, 28 Jun 2010 03:20:41 +0200, Lars-Peter Clausen <lars(a)metafoo.de> wrote:
> This patch adds support for the mmc controller on JZ4740 SoCs.
>
> Signed-off-by: Lars-Peter Clausen <lars(a)metafoo.de>
> Cc: Andrew Morton <akpm(a)linux-foundation.org>
> Cc: Matt Fleming <matt(a)console-pimps.org>
> Cc: linux-mmc(a)vger.kernel.org
>
> ---
> Changes since v1
> - Do not request IRQ with IRQF_DISABLED since it is a noop now
> - Use a generous slack for the timeout timer. It does not need to be accurate.
> Changes since v2
> - Use sg_mapping_to iterate over sg elements in mmc read and write functions
> - Use bitops instead of a spinlock and a variable for testing whether a request
> has been finished.
> - Rework irq and timeout handling in order to get rid of locking in hot paths

Acked-by: Matt Fleming <matt(a)console-pimps.org>

Are you planning on maintaining this driver? If so, it'd be a good idea
to update MAINTAINERS.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 30 Jun 2010 17:00
On Mon, 28 Jun 2010 03:20:41 +0200
Lars-Peter Clausen <lars(a)metafoo.de> wrote:

> This patch adds support for the mmc controller on JZ4740 SoCs.
>
> Signed-off-by: Lars-Peter Clausen <lars(a)metafoo.de>
> Cc: Andrew Morton <akpm(a)linux-foundation.org>
> Cc: Matt Fleming <matt(a)console-pimps.org>
> Cc: linux-mmc(a)vger.kernel.org
>
> ...
>
> +#define JZ4740_MMC_MAX_TIMEOUT 10000000

That was a really big timeout. How long do 1e7 readw's take? Oh well.

>
> ...
>
> +static void jz4740_mmc_clock_disable(struct jz4740_mmc_host *host)
> +{
> + uint32_t status;
> +
> + writew(JZ_MMC_STRPCL_CLOCK_STOP, host->base + JZ_REG_MMC_STRPCL);
> + do {
> + status = readl(host->base + JZ_REG_MMC_STATUS);
> + } while (status & JZ_MMC_STATUS_CLK_EN);
> +}
> +
> +static void jz4740_mmc_reset(struct jz4740_mmc_host *host)
> +{
> + uint32_t status;
> +
> + writew(JZ_MMC_STRPCL_RESET, host->base + JZ_REG_MMC_STRPCL);
> + udelay(10);
> + do {
> + status = readl(host->base + JZ_REG_MMC_STATUS);
> + } while (status & JZ_MMC_STATUS_IS_RESETTING);
> +}

Maybe these should have a timeout too?

>
> ...
>
> +static inline unsigned int jz4740_mmc_wait_irq(struct jz4740_mmc_host *host,
> + unsigned int irq)
> +{
> + unsigned int timeout = JZ4740_MMC_MAX_TIMEOUT;
> + uint16_t status;
> +
> + do {
> + status = readw(host->base + JZ_REG_MMC_IREG);
> + } while (!(status & irq) && --timeout);
> +
> + return timeout;
> +}

This guy's too big to inline. Recent gcc's know that and they tend to
uninline such things behind your back anwyay.

>
> ...
>
> +struct jz4740_mmc_platform_data {
> + int gpio_power;
> + int gpio_card_detect;
> + int gpio_read_only;
> + unsigned card_detect_active_low:1;
> + unsigned read_only_active_low:1;
> + unsigned power_active_low:1;
> +
> + unsigned data_1bit:1;
> +};

The bitfields will all share the same word, so modification of one
field can race against modification of another field. Hence some form
of locking which covers *all* the bitfields is needed.

Is that a problem in this driver?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Lars-Peter Clausen on 1 Jul 2010 11:50
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Fleming wrote:
> On Mon, 28 Jun 2010 03:20:41 +0200, Lars-Peter Clausen <lars(a)metafoo.de> wrote:
>> This patch adds support for the mmc controller on JZ4740 SoCs.
>>
>> Signed-off-by: Lars-Peter Clausen <lars(a)metafoo.de>
>> Cc: Andrew Morton <akpm(a)linux-foundation.org>
>> Cc: Matt Fleming <matt(a)console-pimps.org>
>> Cc: linux-mmc(a)vger.kernel.org
>>
>> ---
>> Changes since v1
>> - Do not request IRQ with IRQF_DISABLED since it is a noop now
>> - Use a generous slack for the timeout timer. It does not need to be accurate.
>> Changes since v2
>> - Use sg_mapping_to iterate over sg elements in mmc read and write functions
>> - Use bitops instead of a spinlock and a variable for testing whether a request
>> has been finished.
>> - Rework irq and timeout handling in order to get rid of locking in hot paths
>
> Acked-by: Matt Fleming <matt(a)console-pimps.org>
>
> Are you planning on maintaining this driver? If so, it'd be a good idea
> to update MAINTAINERS.

Hi

Thanks for reviewing the patch.
I guess I should send a MAINTAINERS patch which adds entries for all of the JZ4740
drivers.

- - Lars
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkwsuJUACgkQBX4mSR26RiNHVgCfTq+tc2I1QBniqijyUjNDxPIX
GsEAn1xgPWz+L0uqHWthzJ+lMtFaUBtY
=nVt9
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: PROBLEM: SIS7019 stops recording after 42 min
Next: [PATCH v3] alsa: ASoC: Add JZ4740 codec driver