Message-ID: <43EF6F61.6000407@drzeus.cx>
Date: Sun, 12 Feb 2006 18:24:49 +0100
From: Pierre Ossman <drzeus@drzeus.cx>
User-Agent: Thunderbird 1.5 (X11/20060119)
MIME-Version: 1.0
To: Sergey Vlasov <vsu@altlinux.ru>
CC: rmk+lkml@arm.linux.org.uk, linux-kernel@vger.kernel.org, jgarzik@pobox.com
Subject: Re: [PATCH 2/2] [MMC] Secure Digital Host Controller Interface driver
References: <20060211001523.10315.34499.stgit@poseidon.drzeus.cx>	<20060211001525.10315.30769.stgit@poseidon.drzeus.cx> <20060212201407.70761172.vsu@altlinux.ru>
In-Reply-To: <20060212201407.70761172.vsu@altlinux.ru>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1416
Lines: 55

Sergey Vlasov wrote:

> The interrupt handler can be called immediately after request_irq()
> completes (even if you are sure that the device itself cannot generate
> interrupts at this point, the interrupt line can be shared).  And
> host->lock is not yet initialized - oops...
> 

Ah. I'll fix that.

> 
> The same problem as with request_irq(), just from the other side - until
> free_irq() returns, you may still get calls to your interrupt handler,
> and host->ioaddr is already unmapped - oops again.
> 

Ditto.

>> +
>> +	mmc_free_host(mmc);
>> +}
>> +
>> +static int __devinit sdhci_probe(struct pci_dev *pdev,
>> +	const struct pci_device_id *ent)
>> +{
>> +	int ret, i;
>> +	u8 slots;
>> +	struct sdhci_chip *chip;
>> +
>> +	BUG_ON(pdev == NULL);
>> +	BUG_ON(ent == NULL);
> 
> IMHO these BUG_ON() calls are overkill.
> 

I prefer BUG_ON():s since they print a line number. A page fault induced
oops just gives me a byte offset into the compiled function.

> [...]
>> +typedef struct sdhci_host *sdhci_host_p;
> 
> The general policy seems to be "typedefs are evil"...

Fair enough. It didn't get much usage anyway.


Thanks for the code review. :)

Rgds
Pierre
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/