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]

Date:	Sat, 21 Feb 2009 13:46:03 +0100
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) - v3

On Sun, 1 Feb 2009 19:54:35 +0100
Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:

> > > +/* helper functions */
> > > +
> > > +static inline void cb710_set_irq_handler(struct cb710_slot *slot,
> > > +	cb710_irq_handler_t handler)
> > > +{
> > > +	rcu_assign_pointer(slot->irq_handler, handler);
> > > +	synchronize_rcu();	/* sync to IRQ handler */
> > > +}
> > Why RCU:s? This smells of premature optimisation.
> 
> I thought this as the easiest way to assure that on return from
> cb710_set_irq_handler() there's no interrupt handler running using the
> old slot->irq_handler value.
> 

A spinlock will suffice fine for that.

> > > +/* per-MMC-reader structure */
> > > +struct cb710_mmc_reader {
> > > +	struct tasklet_struct finish_req_tasklet;
> > > +	struct mmc_request *mrq;
> > > +	spinlock_t irq_lock;
> > > +	unsigned char last_power_mode;
> > > +#ifdef VERBOSE_DEBUG
> > > +	spinlock_t serialization_lock;
> > > +	unsigned char active_req, active_ios;
> > > +#endif
> > > +};
> > I couldn't find VERBOSE_DEBUG defined somewhere. For ease of use with
> > future testers, you should connect it to some Kconfig option.
> 
> This is mostly related to veryfying some assumptions on the MMC core
> infrastructure. I can remove this altogether if the calls to driver
> ->request() and ->set_ios() are really guaranteed to be serialized.
> 

They are. Changing device settings during an ongoing request is very
undefined.

> > > +static int cb710_resume(struct pci_dev *pdev)
> > > +{
> > > +	pci_set_power_state(pdev, PCI_D0);
> > > +	pci_restore_state(pdev);
> > > +	return pcim_enable_device(pdev);
> > > +}
> > Free/restore interrupt?
> 
> Hmm. I checked couple random drivers in kernel tree and none (including
> MMC host drivers) are touching interrupt allocation in suspend/resume.
> 

Check sdhci.c, it frees the irq and re-requests it during suspend. This
is generally needed when you share interrupts as you need to avoid
trying to handle interrupts before your device has been resumed.

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/