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]

Message-ID: <20090221165757.22747648@mjolnir.ossman.eu>
Date:	Sat, 21 Feb 2009 16:57:57 +0100
From:	Pierre Ossman <drzeus@...eus.cx>
To:	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, 13 Feb 2009 17:40:39 +0300
Anton Vorontsov <avorontsov@...mvista.com> wrote:

> 
> No, on eSDHC the registers are big-endian, 32-bit width, with, for
> example, two 16-bit "logical" registers packed into it.
> 
> That is,
> 
>  0x4  0x5 0x6  0x7
> |~~~~~~~~:~~~~~~~~|
> | BLKCNT : BLKSZ  |
> |________:________|
>  31              0
> 
> ( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
>   that you swapped bytes in this 32 bit register... then the registers
>   and their byte addresses will look normal. )
> 
> So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):
> 
> - We'll read BLKCNT, while we wanted BLKSZ. This is because the
>   address bits should be translated before we try word or byte
>   reads/writes.
> - On powerpc read{l,w}() convert the read value from little-endian
>   to big-endian byte order, which is wrong for our case (the
>   register is big-endian already).
> 
> That means that we have to convert address, but we don't want to
> convert the result of read/write ops.
> 

*cries*

Now this is just incredibly horrible. Why the hell did they try to use
the sdhci interface and then do stupid things like this?

> > > +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
> 
> Hm. It's not a worst case scenario, it's a normal scenario for
> eSDHC. Why should we treat eSDHC as a second-class citizen?
> 

Because it's complete and utter crap. Freescale has completely ignored
the basic register interface requirements of the SDHCI spec. Treating
eSDHC as a second-class citizen is generous IMO.

> > 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);
> 
> Surely the overhead isn't measurable... but why we purposely make
> things worse?
> 

We can most likely do some micro-optimisation do make the compare part
cheaper, but the point was to avoid a function call for all the
properly implemented controllers out there. We could have a flag so
that it only has to check host->flags, which will most likely be in the
cache anyway.

Overhead for eSDHC is not a concern in my book, what is interesting is
how much this change slows things down for other controllers.

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)