linux-kernel - Re: RFC: Driver for CB710/720 memory card reader (MMC part)

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: <20081004215122.0626cd7b@mjolnir.drzeus.cx>
Date:	Sat, 4 Oct 2008 21:51:22 +0200
From:	Pierre Ossman <drzeus-mmc@...eus.cx>
To:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:	linux-kernel@...r.kernel.org, Alex Dubov <oakad@...oo.com>
Subject: Re: RFC: Driver for CB710/720 memory card reader (MMC part) - v2

On Thu, 25 Sep 2008 08:29:24 +0200
Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:

> 
> This should be almost ready. mmc-test passes up to a point where it
> tries non-sector-size writes to the card - I didn't have time to look
> into this further, yet.
> 

If you can't figure it out, just make sure the driver fails the
requests with -EINVAL.

> > > +/* sg-to-PIO buffer */
> [and its API]
> > Why this complex system? Can't you use the handlers the kernel already
> > provides? You also get a lot of special handling with those, e.g.
> > highmem.
> 
> I looked at linux/lib/scatterlist.c and found nothing really useful. If
> there were some preconditions always met for scatterlists - like that block
> does not span multiple pages, or at least 16-byte block don't, then most
> of this code can go away. It looks complicated, because it implements
> a special iterator interface that always returns multiple-of-16-byte blocks
> for reading or writing. This makes the PIO loops very simple and fast
> (the bounce_buffer case was never triggered by mmc-block in my tests).
> 

The buffers leave no alignment guarantees unfortunately. 

Have a look at the current sdhci.c PIO routines though. It uses the sg
iterator helpers and keeps track of four byte chunks (as opposed to the
16 byte ones you need).

> > > +static void cb710_mmc_enable_irq(struct cb710_chip *chip, int enable)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&chip->irq_lock, flags);
> > > +	__cb710_mmc_enable_irq(chip, enable);
> > > +	spin_unlock_irqrestore(&chip->irq_lock, flags);
> > > +}
> > This is a fairly useless wrapper. Look over how it is called instead.
> 
> Can you expand on this? Right now irq handler doesn't call
> __cb710_mmc_enable_irq() but modifies registers itself. This will
> change eventually.
> 

Just require that the lock must be held by the caller when invoking
cb710_mmc_enable_irq(). It's usually easier to keep track of locks by
keeping the lock handling at entry points into the driver.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.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/