On Thu, 19 Feb 2009 17:26:26 +0200
Jorg Schummer <[email protected]> wrote:
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index df6ce4a..cd2e29f 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -740,6 +740,22 @@ void mmc_rescan(struct work_struct *work)
>
> mmc_bus_get(host);
>
> + /* if there is a card registered */
> + if (host->bus_ops != NULL) {
> +
> + if (host->bus_ops->detect && !host->bus_dead) {
> +
> + /* check whether the card is still present */
> + host->bus_ops->detect(host);
> +
> + /* release the bus and update bus status in case
> + the card was removed */
> + mmc_bus_put(host);
> + mmc_bus_get(host);
> + }
> + }
> +
> + /* if there is no card registered */
> if (host->bus_ops == NULL) {
> /*
> * Only we can add a new handler, so it's safe to
Perhaps it's more clear if you grab the lock for the first section,
release it after and then regrab it for the second section. A bit less
efficient, but I don't think that will be a problem in practice.
> @@ -789,12 +805,8 @@ void mmc_rescan(struct work_struct *work)
>
> mmc_release_host(host);
> mmc_power_off(host);
> - } else {
> - if (host->bus_ops->detect && !host->bus_dead)
> - host->bus_ops->detect(host);
> -
> + } else
> mmc_bus_put(host);
> - }
> out:
> if (host->caps & MMC_CAP_NEEDS_POLL)
> mmc_schedule_delayed_work(&host->detect, HZ);
The else section got a bit small here with that code removed. Perhaps
we should instead have:
if (host->bus_ops != NULL) {
mmc_bus_put(host);
goto out;
}
With some adjusted comments to make the flow clear.
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.
Hello,
thanks for your reply.
On Mon, 2009-03-02 at 21:09 +0100, ext Pierre Ossman wrote:
> On Thu, 19 Feb 2009 17:26:26 +0200
> Jorg Schummer <[email protected]> wrote:
>
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index df6ce4a..cd2e29f 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -740,6 +740,22 @@ void mmc_rescan(struct work_struct *work)
> >
> > mmc_bus_get(host);
> >
> > + /* if there is a card registered */
> > + if (host->bus_ops != NULL) {
> > +
> > + if (host->bus_ops->detect && !host->bus_dead) {
> > +
> > + /* check whether the card is still present */
> > + host->bus_ops->detect(host);
> > +
> > + /* release the bus and update bus status in case
> > + the card was removed */
> > + mmc_bus_put(host);
> > + mmc_bus_get(host);
> > + }
> > + }
> > +
> > + /* if there is no card registered */
> > if (host->bus_ops == NULL) {
> > /*
> > * Only we can add a new handler, so it's safe to
>
> Perhaps it's more clear if you grab the lock for the first section,
> release it after and then regrab it for the second section. A bit less
> efficient, but I don't think that will be a problem in practice.
Yes, you're probably right, I should take read- and maintainability into
account there. Will change that.
>
> > @@ -789,12 +805,8 @@ void mmc_rescan(struct work_struct *work)
> >
> > mmc_release_host(host);
> > mmc_power_off(host);
> > - } else {
> > - if (host->bus_ops->detect && !host->bus_dead)
> > - host->bus_ops->detect(host);
> > -
> > + } else
> > mmc_bus_put(host);
> > - }
> > out:
> > if (host->caps & MMC_CAP_NEEDS_POLL)
> > mmc_schedule_delayed_work(&host->detect, HZ);
>
> The else section got a bit small here with that code removed. Perhaps
> we should instead have:
>
> if (host->bus_ops != NULL) {
> mmc_bus_put(host);
> goto out;
> }
I guess you meant
else {
mmc_bus_put(host);
goto out;
}
since it would be the else clause of
if (host->bus_ops == NULL)
Also: Are you sure "goto out" should be added in the else-branch? (Since
it's not present in the end of the corresponding if-branch either, and
is technically not needed.)
Regards,
Jörg
On Tue, 03 Mar 2009 09:24:19 +0200
Jörg Schummer <[email protected]> wrote:
> >
> > The else section got a bit small here with that code removed. Perhaps
> > we should instead have:
> >
> > if (host->bus_ops != NULL) {
> > mmc_bus_put(host);
> > goto out;
> > }
>
> I guess you meant
>
> else {
> mmc_bus_put(host);
> goto out;
> }
>
> since it would be the else clause of
>
> if (host->bus_ops == NULL)
>
No, I meant to put it further up and unindent the detection code.
Basically you'll be reversing the if-else blocks, but using a goto
instead of an else.
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.
With this patch, mmc_rescan can detect the removal of an mmc card and
the insertion of (possibly another) card in the same run. This means
that a card change can be detected without having to call
mmc_detect_change multiple times.
This change generalises the core such that it can be easily used by
hosts which provide a mechanism to detect only the presence of a card
reader cover, which has to be taken off in order to insert a card. Other
hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
a card is removed and when a card is inserted, so it is sufficient for
them if mmc_rescan handles only one event at a time. "Cover detect"
hosts, however, only receive events about the cover status. This means
that between 2 subsequent events, both a card removal and a card
insertion can occur. In this case, the pre-patch version of mmc_rescan
would only detect the removal of the previous card but not the insertion
of the new card.
Signed-off-by: Jorg Schummer <[email protected]>
---
drivers/mmc/core/core.c | 99 ++++++++++++++++++++++++++---------------------
1 files changed, 55 insertions(+), 44 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index df6ce4a..5970719 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -740,61 +740,72 @@ void mmc_rescan(struct work_struct *work)
mmc_bus_get(host);
- if (host->bus_ops == NULL) {
- /*
- * Only we can add a new handler, so it's safe to
- * release the lock here.
- */
+ /* if there is a card registered, check whether it is still present */
+ if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
+ host->bus_ops->detect(host);
+
+ mmc_bus_put(host);
+
+
+ mmc_bus_get(host);
+
+ /* if there still is a card present, stop here */
+ if (host->bus_ops != NULL) {
mmc_bus_put(host);
+ goto out;
+ }
- if (host->ops->get_cd && host->ops->get_cd(host) == 0)
- goto out;
+ /* detect a newly inserted card */
- mmc_claim_host(host);
+ /*
+ * Only we can add a new handler, so it's safe to
+ * release the lock here.
+ */
+ mmc_bus_put(host);
- mmc_power_up(host);
- mmc_go_idle(host);
+ if (host->ops->get_cd && host->ops->get_cd(host) == 0)
+ goto out;
- mmc_send_if_cond(host, host->ocr_avail);
+ mmc_claim_host(host);
- /*
- * First we search for SDIO...
- */
- err = mmc_send_io_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_sdio(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ mmc_power_up(host);
+ mmc_go_idle(host);
- /*
- * ...then normal SD...
- */
- err = mmc_send_app_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_sd(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ mmc_send_if_cond(host, host->ocr_avail);
- /*
- * ...and finally MMC.
- */
- err = mmc_send_op_cond(host, 0, &ocr);
- if (!err) {
- if (mmc_attach_mmc(host, ocr))
- mmc_power_off(host);
- goto out;
- }
+ /*
+ * First we search for SDIO...
+ */
+ err = mmc_send_io_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sdio(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
- mmc_release_host(host);
- mmc_power_off(host);
- } else {
- if (host->bus_ops->detect && !host->bus_dead)
- host->bus_ops->detect(host);
+ /*
+ * ...then normal SD...
+ */
+ err = mmc_send_app_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_sd(host, ocr))
+ mmc_power_off(host);
+ goto out;
+ }
- mmc_bus_put(host);
+ /*
+ * ...and finally MMC.
+ */
+ err = mmc_send_op_cond(host, 0, &ocr);
+ if (!err) {
+ if (mmc_attach_mmc(host, ocr))
+ mmc_power_off(host);
+ goto out;
}
+
+ mmc_release_host(host);
+ mmc_power_off(host);
+
out:
if (host->caps & MMC_CAP_NEEDS_POLL)
mmc_schedule_delayed_work(&host->detect, HZ);
--
1.5.4.3
On Tue, 31 Mar 2009 17:51:21 +0300
Jorg Schummer <[email protected]> wrote:
> With this patch, mmc_rescan can detect the removal of an mmc card and
> the insertion of (possibly another) card in the same run. This means
> that a card change can be detected without having to call
> mmc_detect_change multiple times.
>
> This change generalises the core such that it can be easily used by
> hosts which provide a mechanism to detect only the presence of a card
> reader cover, which has to be taken off in order to insert a card. Other
> hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
> a card is removed and when a card is inserted, so it is sufficient for
> them if mmc_rescan handles only one event at a time. "Cover detect"
> hosts, however, only receive events about the cover status. This means
> that between 2 subsequent events, both a card removal and a card
> insertion can occur. In this case, the pre-patch version of mmc_rescan
> would only detect the removal of the previous card but not the insertion
> of the new card.
>
> Signed-off-by: Jorg Schummer <[email protected]>
> ---
Queued, thanks.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org
TigerVNC, core developer http://www.tigervnc.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.
On Fri, 2009-04-10 at 18:39 +0200, ext Pierre Ossman wrote:
> On Tue, 31 Mar 2009 17:51:21 +0300
> Jorg Schummer <[email protected]> wrote:
>
> > With this patch, mmc_rescan can detect the removal of an mmc card and
> > the insertion of (possibly another) card in the same run. This means
> > that a card change can be detected without having to call
> > mmc_detect_change multiple times.
> >
> > This change generalises the core such that it can be easily used by
> > hosts which provide a mechanism to detect only the presence of a card
> > reader cover, which has to be taken off in order to insert a card. Other
> > hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
> > a card is removed and when a card is inserted, so it is sufficient for
> > them if mmc_rescan handles only one event at a time. "Cover detect"
> > hosts, however, only receive events about the cover status. This means
> > that between 2 subsequent events, both a card removal and a card
> > insertion can occur. In this case, the pre-patch version of mmc_rescan
> > would only detect the removal of the previous card but not the insertion
> > of the new card.
> >
> > Signed-off-by: Jorg Schummer <[email protected]>
> > ---
>
> Queued, thanks.
Hi Pierre,
is there any news about this patch?
Regards,
Jörg
On Wed, 03 Jun 2009 12:47:48 +0300
Jörg Schummer <[email protected]> wrote:
>
> Hi Pierre,
>
> is there any news about this patch?
>
It's in my -next branch and will be submitted upstream in the next
merge window.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org
TigerVNC, core developer http://www.tigervnc.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.