Re: [PATCH 2/2] [MMC] Secure Digital Host Controller Interface driver

[Pierre Ossman]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/