| 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
| ||
|
Message-ID: <20090613123249.28304386@mjolnir.ossman.eu>
Date: Sat, 13 Jun 2009 12:32:49 +0200
From: Pierre Ossman <pierre@...man.eu>
To: dpervushin@...eddedalley.com
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH, RFC] Freescale STMP SD/MMC driver
On Fri, 05 Jun 2009 00:28:12 +0400
dmitry pervushin <dpervushin@...eddedalley.com> wrote:
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/highmem.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/completion.h>
> +#include <linux/mmc/mmc.h>
Including this header should never be needed in a host driver. It's a
sign you've violated the layering somewhere.
> +
> +#define CLOCKRATE_MIN 400000
> +#define CLOCKRATE_MAX 48000000
> +
This maximum frequency will never be used as you haven't indicated what
kind of highspeed timing the controller supports (MMC and/or SD).
> +/* Return read only state of card */
> +static int stmp3xxx_mmc_get_ro(struct mmc_host *mmc)
> +{
> + struct stmp3xxx_mmc_host *host = mmc_priv(mmc);
> + struct stmp3xxxmmc_platform_data *pdata = host->dev->platform_data;
> +
> + if (pdata && pdata->get_wp)
> + return pdata->get_wp();
> +
> + return 0;
> +}
You should return -ENOSYS when you do not know the state.
> +/*
> + * Check for MMC command errors
> + * Returns error code or zerro if no errors
> + */
> +static inline int stmp3xxx_mmc_cmd_error(u32 status)
> +{
> + int err = 0;
> +
> + if (status & BM_SSP_STATUS_TIMEOUT)
> + err = -ETIMEDOUT;
> + else if (status & BM_SSP_STATUS_RESP_TIMEOUT)
> + err = -ETIMEDOUT;
> + else if (status & BM_SSP_STATUS_RESP_CRC_ERR)
> + err = -EILSEQ;
> + else if (status & BM_SSP_STATUS_RESP_ERR)
> + err = -EIO;
When does the controller signal RESP_ERR?
> +/* Copy data between sg list and dma buffer */
> +static unsigned int stmp3xxx_sg_dma_copy(struct stmp3xxx_mmc_host *host,
> + unsigned int size, int to_dma)
Why do you need to copy to the DMA buffer? Can't the device DMA
directly to/from where we want?
> +{
> + struct mmc_data *data = host->cmd->data;
> + unsigned int copy_size, bytes_copied = 0;
> + struct scatterlist *sg;
> + char *dmabuf = host->dma_buf;
> + char *sgbuf;
> + int len, i;
> +
> + sg = data->sg;
> + len = data->sg_len;
> +
> + /*
> + * Just loop through all entries. Size might not
> + * be the entire list though so make sure that
> + * we do not transfer too much.
> + */
> + for (i = 0; i < len; i++) {
sg lists are not simply arrays anymore so you have to use the helpers
(sg_next and such).
> + dev_dbg(host->dev, "ADTC command:\n"
> + "response: %d, ignore crc: %d\n"
> + "data list: %u, blocksz: %u, blocks: %u, timeout: %uns %uclks, "
> + "flags: 0x%x\n", resp, ignore_crc, cmd->data->sg_len,
> + cmd->data->blksz, cmd->data->blocks, cmd->data->timeout_ns,
> + cmd->data->timeout_clks, cmd->data->flags);
This is all generic stuff that is already printed by the MMC core.
> + BUG_ON((data_size % 8) > 0);
This is not a bug. You need to check for this case and handle it
properly (failing it with EINVAL if the hardware is completely
incapable of supporting the request).
> + /*
> + * We need to set the hardware register to the logarithm to base 2 of
> + * the block size.
> + */
And what if the block size isn't a power of two?
> + if (cmd->opcode == MMC_STOP_TRANSMISSION)
> + ssp_cmd0 |= BM_SSP_CMD0_APPEND_8CYC;
I'm not sure what you're doing here. There should be a 8 cycle pause
after every request, not just this opcode.
> + cmd->error = stmp3xxx_mmc_cmd_error(host->status);
Are you putting data errors in here as well? Because I don't see you
setting data->error anywhere.
> +/* Begin sedning a command to the card */
> +static void stmp3xxx_mmc_start_cmd(struct stmp3xxx_mmc_host *host,
> + struct mmc_command *cmd)
> +{
> + dev_dbg(host->dev, "MMC command:\n"
> + "type: 0x%x opcode: %u, arg: %u, flags 0x%x retries: %u\n",
> + mmc_cmd_type(cmd), cmd->opcode, cmd->arg, cmd->flags,
> + cmd->retries);
This is also printed by the core.
> +/* Configure card */
> +static void stmp3xxx_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct stmp3xxx_mmc_host *host = mmc_priv(mmc);
> + struct stmp3xxxmmc_platform_data *pdata;
> +
> + dev_dbg(host->dev, "MMC set ios:\n"
> + "Clock %u, vdd %u, bus_mode %u, chip_select %u, "
> + "power mode %u, bus_width %u\n", ios->clock, ios->vdd,
> + ios->bus_mode, ios->chip_select, ios->power_mode,
> + ios->bus_width);
Also printed by the core.
> + /* Set minimal clock rate */
> + host->clk = clk_get(NULL, "ssp");
> + if (IS_ERR(host->clk)) {
> + err = PTR_ERR(host->clk);
> + dev_err(dev, "clocks initialization failed\n");
> + goto out_clk;
> + }
> +
> + clk_enable(host->clk);
> + stmp3xxx_set_sclk_speed(host, CLOCKRATE_MIN);
> +
> + /* Reset MMC block */
> + stmp3xxx_mmc_reset(host);
I'm not entirely sure what the end result is here, but you really
shouldn't enable the clock until the MMC core tells you to.
Rgds
--
-- Pierre Ossman
WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.
Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)
Powered by blists - more mailing lists