linux-kernel - Re: [PATCH v2 18/26] MMC: Add JZ4740 mmc driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 19 Jun 2010 15:46:25 +0100
From:	Matt Fleming <matt@...sole-pimps.org>
To:	Lars-Peter Clausen <lars@...afoo.de>
Cc:	linux-mips@...ux-mips.org, linux-kernel@...r.kernel.org,
	Lars-Peter Clausen <lars@...afoo.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-mmc@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>
Subject: Re: [PATCH v2 18/26] MMC: Add JZ4740 mmc driver

On Sat, 19 Jun 2010 07:08:23 +0200, Lars-Peter Clausen <lars@...afoo.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/