linux-kernel - Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line

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: <20080522181713.GA26918@polina.dev.rtsoft.ru>
Date:	Thu, 22 May 2008 22:17:13 +0400
From:	Anton Vorontsov <avorontsov@...mvista.com>
To:	Pierre Ossman <drzeus-mmc@...eus.cx>
Cc:	Kumar Gala <galak@...nel.crashing.org>,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Jochen Friedrich <jochen@...am.de>,
	Timur Tabi <timur@...escale.com>, linuxppc-dev@...abs.org,
	linux-kernel@...r.kernel.org,
	spi-devel-general@...ts.sourceforge.net
Subject: Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card
	detect line

On Wed, May 21, 2008 at 09:28:31PM +0200, Pierre Ossman wrote:
> On Wed, 21 May 2008 22:47:13 +0400
> Anton Vorontsov <avorontsov@...mvista.com> wrote:
> 
> > 
> > Calling get_cd() for every request smells like overhead, especially given
> > that that get_cd() could ask for GPIO status via relatively slow bus (like
> > I2C GPIO expanders). So, polling seems most reasonable solution here, no
> > need to call it very often.
> > 
> 
> Fair enough. You should probably add a comment about this somewhere so
> that people do not call get_cd() in the core request function and
> similar places. Place it so that both get_cd() and get_ro() are covered
> though, as it should be relevant for both.

I think this is applicable to the .set_ios() too.

[...]
> > +	if (host->ops->get_cd) {
> > +		int old_cd_status = host->cd_status;
> > +
> > +		host->cd_status = !!host->ops->get_cd(host);
> > +		if (!(old_cd_status ^ host->cd_status)) {
> > +			mmc_bus_put(host);
> > +			goto out;
> > +		}
> > +	}
> > +
> 
> This should only be done when there is no bus handler. Since we are
> polling, we might actually miss the user removing and reinserting the
> card. The only way to check for that is to poke the card and see if it
> is still alive. This also means you won't need that state variable.

Yeah, this makes sense. Indeed pretty easy to trigger [if poll interval
increased to 3 seconds, for example].

> Also, that second if clause seems fit for an obfuscation contest. ;)

cd_status was a bitfield, so I thought that bit operations would be
appropriate. :-)

> > p.s. Since mmc_host_ops no longer the same for every instance of
> > mmc_spi, struct mmc_host_ops can't be const and should be allocated
> > dynamically.
> 
> This can be solved by allowing get_cd() to return an error that will be
> treated as if get_cd() wasn't defined. -ENODEV seems suitable.

-ENOSYS (not implemented) sounds better for this purpose...

> (get_ro() needs the same treatment, but I haven't gotten around to
> that)

Ok. How about this version?

-- 
Anton Vorontsov
email: cbouatmailru@...il.com
irc://irc.freenode.net/bd2
--
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/