linux-kernel - Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

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:	Sun, 8 Feb 2009 21:50:20 +0100
From:	Pierre Ossman <drzeus@...eus.cx>
To:	Anton Vorontsov <avorontsov@...mvista.com>
Cc:	Ben Dooks <ben-linux@...ff.org>, Arnd Bergmann <arnd@...db.de>,
	Kumar Gala <galak@...nel.crashing.org>,
	Liu Dave <DaveLiu@...escale.com>, sdhci-devel@...t.drzeus.cx,
	linux-kernel@...r.kernel.org, linuxppc-dev@...abs.org
Subject: Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory
 accessors

On Fri, 6 Feb 2009 21:06:45 +0300
Anton Vorontsov <avorontsov@...mvista.com> wrote:

> Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
> read{l,b,w}).
> 
> With this patch drivers may change memory accessors, so that we can
> support hosts with "weird" IO memory access requirments.
> 
> For example, in "FSL eSDHC" SDHCI hardware all registers are 32 bit
> width, with big-endian addressing. That is, readb(0x2f) should turn
> into readb(0x2c), and readw(0x2c) should be translated to
> le16_to_cpu(readw(0x2e)).
> 
> Signed-off-by: Anton Vorontsov <avorontsov@...mvista.com>
> ---

I was hoping we wouldn't have to do a lot of magic in the accessors
since the spec is rather clear on the register interface. :/

Let's see if I've understood this correctly.

1. The CPU is big-endian but the register are little-endian (as the
spec requires). I was under the impression that the read*/write*
accessor handled any endian conversion between the bus and the cpu? How
do e.g. PCI work on Sparc?

2. Register access must be done 32 bits at a time. Now this is just
broken and might cause big problems as some registers cannot just be
read and written back to. OTOH you refer to readw() in your example,
not readl(). What's the deal here?

> +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
> +{
> +	host->writel(host, val, reg);
> +}

Having to override these are worst case scenario as far as I'm
concerned, so I'd prefer something like:

if (!host->ops->writel)
	writel(host->ioaddr + reg, val);
else
	host->ops->writel(host, val, reg);

and maybe even a likely() up there.

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" (198 bytes)