On Wed, 19 Dec 2007 15:22:13 +0100
Harald Welte <laforge@openmoko.org> wrote:

> Hi!
> 
> This is a MMC/SD driver for the Samsung S3C24xx SD/MMC controller, originally
> developed years ago by Thomas Kleffel <tk@maintech.de>.
> 
> Due to time constraints, he had no time to further maintain the driver
> and follow the mainline Linux changes in the SD/MMC stack.
> 
> With his authorization, I have taken over the task of making it
> compliant to the current mainline SD/MMC API and take care of the
> mainline kernel merge.
> 
> So please advise on whatever changes you deem neccessary and I'll try to
> do my best to incorporate them for a hopefully not-too-distant mainline
> merge.
> 
> After a potential kernel inclusion, we would co-maintain the driver.
> 
> Acked-by: Thomas Kleffel <tk@maintech.de>
> Signed-off-by: Harald Welte <laforge@openmoko.org>
> 
> ---

Well, my biggest beef with this driver is the excessive debug code. I did a cleanup in an older version of the driver, but it shouldn't be to difficult to do the same for this one. I've included my patch for reference.

> Index: linux-2.6/drivers/mmc/host/s3cmci.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/drivers/mmc/host/s3cmci.c

> +#include <linux/mmc/mmc.h>

This is always a warning sign. It should never be needed in a host driver.


> +
> +static inline int get_data_buffer(struct s3cmci_host *host,
> +				  u32 *words, u32 **pointer)
> +{
> +	struct scatterlist *sg;
> +
> +	if (host->pio_active == XFER_NONE)
> +		return -EINVAL;
> +
> +	if ((!host->mrq) || (!host->mrq->data))
> +		return -EINVAL;
> +
> +	if (host->pio_sgptr >= host->mrq->data->sg_len) {
> +		dbg(host, dbg_debug, "no more buffers (%i/%i)\n",
> +		      host->pio_sgptr, host->mrq->data->sg_len);
> +		return -EBUSY;
> +	}
> +	sg = &host->mrq->data->sg[host->pio_sgptr];
> +
> +	*words = sg->length >> 2;
> +	*pointer = page_address(sg_page(sg)) + sg->offset;
> +

The length might not be a multiple of four. And it might also be completely unaligned. Make sure you can either handle such requests, or fail them with -EINVAL.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org