From: Pierre Ossman <drzeus-mmc@drzeus.cx>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: linux-kernel@vger.kernel.org, Alex Dubov <oakad@yahoo.com>
Subject: Re: RFC: Driver for CB710/720 memory card reader (MMC part) - v3
Date: Sat, 21 Feb 2009 13:46:03 +0100 [thread overview]
Message-ID: <20090221134603.6bc3c385@mjolnir.ossman.eu> (raw)
In-Reply-To: <20090201185435.GA9165@rere.qmqm.pl>
On Sun, 1 Feb 2009 19:54:35 +0100
Michał Mirosław <mirq-linux@rere.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.
next prev parent reply other threads:[~2009-02-21 12:46 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-09-07 21:52 MMC host driver requirements Michał Mirosław
2008-09-09 7:18 ` Pierre Ossman
2008-09-09 9:06 ` Michał Mirosław
2008-09-11 20:13 ` Driver for CB710/720 memory card reader (MMC part) Michał Mirosław
2008-09-12 23:43 ` RFC: " Michał Mirosław
2008-09-20 11:00 ` Pierre Ossman
2008-09-25 6:29 ` RFC: Driver for CB710/720 memory card reader (MMC part) - v2 Michał Mirosław
2008-10-04 19:51 ` Pierre Ossman
2008-10-29 14:11 ` RFC: Driver for CB710/720 memory card reader (MMC part) - v3 Michał Mirosław
2008-11-14 21:06 ` Pierre Ossman
2009-02-01 18:54 ` Michał Mirosław
2009-02-21 12:46 ` Pierre Ossman [this message]
2009-05-08 15:16 ` [PATCH/RFC 2.6.29.2] Driver for CB710/720 memory card reader (MMC part) - v4 Michał Mirosław
2009-05-22 11:27 ` Pierre Ossman
2009-05-22 17:55 ` [PATCH 2.6.29.3] Driver for CB710/720 memory card reader (MMC part) - v5 Michał Mirosław
2009-05-22 18:33 ` [PATCH 2.6.29.4] Driver for CB710/720 memory card reader (MMC part) - v5 fixed Michał Mirosław
2009-05-27 20:13 ` Pierre Ossman
2009-06-04 10:24 ` Michał Mirosław
2009-06-05 9:26 ` Michał Mirosław
2009-06-13 10:39 ` Pierre Ossman
2009-05-22 19:04 ` [PATCH/RFC 2.6.29.2] Driver for CB710/720 memory card reader (MMC part) - v4 Bartlomiej Zolnierkiewicz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090221134603.6bc3c385@mjolnir.ossman.eu \
--to=drzeus-mmc@drzeus.cx \
--cc=linux-kernel@vger.kernel.org \
--cc=mirq-linux@rere.qmqm.pl \
--cc=oakad@yahoo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).