linux-kernel - Re: [Patch 1/1 v5] via-sdmmc: VIA MSP card reader driver support

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:	Fri, 10 Apr 2009 20:35:20 +0200
From:	Pierre Ossman <drzeus-mmc@...eus.cx>
To:	JosephChan@....com.tw
Cc:	linux-kernel@...r.kernel.org, arnd@...db.de, ben-linux@...ff.org
Subject: Re: [Patch 1/1 v5] via-sdmmc: VIA MSP card reader driver support

On Wed, 1 Apr 2009 10:24:02 +0800
JosephChan@....com.tw wrote:

> Hi all,
> 
> The following patch provides VIA MSP SD/MMC card reader driver support.
> This patch is based on 2.6.29 kernel.
> Thanks to Pierre's suggestions.
> Please feel free to contact me if any problem. Thanks in advanced.
> 
> In addition, the SPEC is available in VX800 Programming guide (Device 13 Function 0) on http://linux.via.com.tw
> 

Looking good. There's just a few minor things. See below.

> +
> +nodata:
> +	if (cmd->opcode == MMC_STOP_TRANSMISSION)
> +		cmdctrl |= VIA_CRDR_SDCTRL_STOP;
> +

I didn't check this part properly the last time. I'm assuming you're
telling the hardware that this is a command that terminates a data
transmission? If so, you cannot check the opcode like that. There might
be other stop commands.

What you need to do is to either pass another parameter to your
function when you're calling it with data->stop, or simply have a test
in there if cmd == data->stop (or mrq->stop, whichever is most
convenient).

> +
> +	if (ios->clock >= 40000000)
> +		clock = PCI_CLK_48M;
> +	else if (ios->clock >= 30000000)
> +		clock = PCI_CLK_33M;
> +	else if (ios->clock >= 20000000)
> +		clock = PCI_CLK_24M;
> +	else if (ios->clock >= 15000000)
> +		clock = PCI_CLK_16M;
> +	else if (ios->clock >= 10000000)
> +		clock = PCI_CLK_12M;
> +	else if (ios->clock >=  5000000)
> +		clock = PCI_CLK_8M;
> +	else
> +		clock = PCI_CLK_375K;
> +

This looks a bit strange. Why are you not comparing with the clock
frequency you'll be setting?

> +
> +	mmc->f_min = VIA_CRDR_MIN_CLOCK;
> +	mmc->f_max = VIA_CRDR_MAX_CLOCK;
> +	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
> +	mmc->caps = MMC_CAP_4_BIT_DATA;

You also need to set the bits stating if the controller supports the
high-speed SD and/or MMC mode, otherwise the core won't go over 25/20
MHz.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org
  TigerVNC, core developer          http://www.tigervnc.org

  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.
--
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/