SDIO Simplified Specification V2.00 states that it is strongly recommended
that the host executes either a power reset or issues a CMD52 (I/O Reset) to
re-initialize an I/O only card or the I/O portion of a combo card.
Additionally, the CMD52 must be issued first because it cannot be issued
after a CMD0.
With this patch the Nintendo Wii SDIO-based WLAN card is detected after a
system reset, without requiring a complete system powercycle.
Signed-off-by: Albert Herranz <[email protected]>
---
v1
- reworded commit message
- CC'd akpm as suggested by mb
drivers/mmc/core/core.c | 1 +
drivers/mmc/core/sdio_ops.c | 36 ++++++++++++++++++++++++++++++------
drivers/mmc/core/sdio_ops.h | 1 +
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d84c880..c768f70 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -890,6 +890,7 @@ void mmc_rescan(struct work_struct *work)
mmc_claim_host(host);
mmc_power_up(host);
+ sdio_go_idle(host);
mmc_go_idle(host);
mmc_send_if_cond(host, host->ocr_avail);
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 4eb7825..14d3204 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -67,13 +67,13 @@ int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
return err;
}
-int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
- unsigned addr, u8 in, u8* out)
+static int mmc_io_rw_direct_host(struct mmc_host *host, int write, unsigned fn,
+ unsigned addr, u8 in, u8 *out)
{
struct mmc_command cmd;
int err;
- BUG_ON(!card);
+ BUG_ON(!host);
BUG_ON(fn > 7);
/* sanity check */
@@ -90,11 +90,11 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
cmd.arg |= in;
cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_AC;
- err = mmc_wait_for_cmd(card->host, &cmd, 0);
+ err = mmc_wait_for_cmd(host, &cmd, 0);
if (err)
return err;
- if (mmc_host_is_spi(card->host)) {
+ if (mmc_host_is_spi(host)) {
/* host driver already reported errors */
} else {
if (cmd.resp[0] & R5_ERROR)
@@ -106,7 +106,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
}
if (out) {
- if (mmc_host_is_spi(card->host))
+ if (mmc_host_is_spi(host))
*out = (cmd.resp[0] >> 8) & 0xFF;
else
*out = cmd.resp[0] & 0xFF;
@@ -115,6 +115,13 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
return 0;
}
+int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
+ unsigned addr, u8 in, u8 *out)
+{
+ BUG_ON(!card);
+ return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out);
+}
+
int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz)
{
@@ -182,3 +189,20 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
return 0;
}
+int sdio_go_idle(struct mmc_host *host)
+{
+ int ret;
+ u8 abort;
+
+ /* SDIO Simplified Specification V2.0, 4.4 Reset for SDIO */
+
+ ret = mmc_io_rw_direct_host(host, 0, 0, SDIO_CCCR_ABORT, 0, &abort);
+ if (ret)
+ abort = 0x08;
+ else
+ abort |= 0x08;
+
+ ret = mmc_io_rw_direct_host(host, 1, 0, SDIO_CCCR_ABORT, abort, NULL);
+ return ret;
+}
+
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index e2e74b0..9b546c7 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -17,6 +17,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
unsigned addr, u8 in, u8* out);
int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
+int sdio_go_idle(struct mmc_host *host);
#endif
--
1.6.0.4
On Thu, 10 Sep 2009 14:56:41 +0200
Albert Herranz <[email protected]> wrote:
> SDIO Simplified Specification V2.00 states that it is strongly recommended
> that the host executes either a power reset or issues a CMD52 (I/O Reset) to
> re-initialize an I/O only card or the I/O portion of a combo card.
> Additionally, the CMD52 must be issued first because it cannot be issued
> after a CMD0.
>
> With this patch the Nintendo Wii SDIO-based WLAN card is detected after a
> system reset, without requiring a complete system powercycle.
>
> Signed-off-by: Albert Herranz <[email protected]>
> ---
I had problems with cards already in the idle state crashing when I
sent them a reset. I suggest a big round of testing before merging this.
> @@ -182,3 +189,20 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
> return 0;
> }
>
> +int sdio_go_idle(struct mmc_host *host)
> +{
It might be clearer if you use the terminology from the spec, so either
sdio_abort() or sdio_reset().
Rgds
--
-- Pierre Ossman
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.
Some manufacturers provide vendor information in non-vendor specific CIS
tuples. For example, Broadcom uses an Extended Function tuple to provide
the MAC address on some of their network cards, as in the case of the
Nintendo Wii WLAN daughter card.
This patch allows passing correct tuples unknown to the SDIO core to
a matching SDIO driver instead of rejecting them and failing.
Signed-off-by: Albert Herranz <[email protected]>
---
v1
- fixed typo in commit message
- CC'd akpm as suggested by mb
- required by commit 4ea602e183ca20a7577ebe253323d0e5d0f9847 in net-next-2.6
drivers/mmc/core/sdio_cis.c | 46 +++++++++++++++++++++++-------------------
1 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index 963f293..87934ac 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -123,8 +123,9 @@ static int cistpl_funce_func(struct sdio_func *func,
vsn = func->card->cccr.sdio_vsn;
min_size = (vsn == SDIO_SDIO_REV_1_00) ? 28 : 42;
+ /* let the SDIO driver take care of unknown tuples */
if (size < min_size || buf[0] != 1)
- return -EINVAL;
+ return -EILSEQ;
/* TPLFE_MAX_BLK_SIZE */
func->max_blksize = buf[12] | (buf[13] << 8);
@@ -154,13 +155,7 @@ static int cistpl_funce(struct mmc_card *card, struct sdio_func *func,
else
ret = cistpl_funce_common(card, buf, size);
- if (ret) {
- printk(KERN_ERR "%s: bad CISTPL_FUNCE size %u "
- "type %u\n", mmc_hostname(card->host), size, buf[0]);
- return ret;
- }
-
- return 0;
+ return ret;
}
typedef int (tpl_parse_t)(struct mmc_card *, struct sdio_func *,
@@ -253,21 +248,12 @@ static int sdio_read_cis(struct mmc_card *card, struct sdio_func *func)
for (i = 0; i < ARRAY_SIZE(cis_tpl_list); i++)
if (cis_tpl_list[i].code == tpl_code)
break;
- if (i >= ARRAY_SIZE(cis_tpl_list)) {
- /* this tuple is unknown to the core */
- this->next = NULL;
- this->code = tpl_code;
- this->size = tpl_link;
- *prev = this;
- prev = &this->next;
- printk(KERN_DEBUG
- "%s: queuing CIS tuple 0x%02x length %u\n",
- mmc_hostname(card->host), tpl_code, tpl_link);
- } else {
+ if (i < ARRAY_SIZE(cis_tpl_list)) {
const struct cis_tpl *tpl = cis_tpl_list + i;
if (tpl_link < tpl->min_size) {
printk(KERN_ERR
- "%s: bad CIS tuple 0x%02x (length = %u, expected >= %u)\n",
+ "%s: bad CIS tuple 0x%02x"
+ " (length = %u, expected >= %u)\n",
mmc_hostname(card->host),
tpl_code, tpl_link, tpl->min_size);
ret = -EINVAL;
@@ -275,7 +261,25 @@ static int sdio_read_cis(struct mmc_card *card, struct sdio_func *func)
ret = tpl->parse(card, func,
this->data, tpl_link);
}
- kfree(this);
+ /* already successfully parsed, not needed anymore */
+ if (!ret)
+ kfree(this);
+ } else {
+ /* unknown tuple */
+ ret = -EILSEQ;
+ }
+
+ if (ret == -EILSEQ) {
+ /* this tuple is unknown to the core */
+ this->next = NULL;
+ this->code = tpl_code;
+ this->size = tpl_link;
+ *prev = this;
+ prev = &this->next;
+ pr_debug("%s: queuing CIS tuple 0x%02x length %u\n",
+ mmc_hostname(card->host), tpl_code, tpl_link);
+ /* keep on analyzing tuples */
+ ret = 0;
}
ptr += tpl_link;
--
1.6.0.4
--- El vie, 11/9/09, Pierre Ossman <[email protected]> escribi?:
> The description for this patch should be made clearer. The title
> suggests it adds functionality that's already in place. It should be
> something along the lines of "Also pass malformed tuples to
> card drivers".
Hi Pierre,
Thanks for your patch review.
I didn't want to use "malformed" in the first place. I used "unknown" as "unknown to the SDIO core". The SDIO core in Linux only knows about FUNCE tuples of type 1 (with a sane length) as described in the SDIO Simplified Spec V2.00.
I think we just have a language issue here, but if you prefer the "malformed" wording I'm ok with that.
> In the sake of sanity, you might want to add this behaviour to all
> parsers, not just the FUNCE one.
I didn't find an application for the other parsers yet, so I tried to stick to the strictly necessary and just did the FUNCE one which has a direct application for Broadcom cards.
>
> I'm also unclear on how this is supposed to work. What does the
> broadcom tuple look like? This patch looks like it will silence a lot
> of legitimate warnings, and possibly pollute the card structures with
> bogus data.
The contents of the Broadcom FUNCE (type 0x22) tuple that contains the MAC address of a card looks like the following (tuple size 8):
04 06 00 1d bc 62 79 fd
^^ ^^ ^^^^^^^^^^^^^^^^^
| | |
| | +--- MAC address
| +--------------------- length (6 bytes for a ethernet MAC address)
+------------------------ type 4 (CISTPL_FUNCE_LAN_NODE_ID)
If you prefer it, instead of passing al "unknown" (or "malformed") FUNCE tuples to SDIO drivers, I can let through only a subset of whitelisted FUNCE types (starting with type 4).
>
> > diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
> > index 963f293..87934ac 100644
> > --- a/drivers/mmc/core/sdio_cis.c
> > +++ b/drivers/mmc/core/sdio_cis.c
> > @@ -123,8 +123,9 @@ static int cistpl_funce_func(struct sdio_func *func,
> >? ??? vsn = func->card->cccr.sdio_vsn;
> >? ??? min_size = (vsn == SDIO_SDIO_REV_1_00) ? 28 : 42;
> >?
> > +??? /* let the SDIO driver take care of unknown tuples */
> >? ??? if (size < min_size || buf[0] != 1)
>
> Misleading comment, the tuple is not unknown.
>
Same language issue described before.
> > -??? ??? return -EINVAL;
> > +??? ??? return -EILSEQ;
> >?
>
> What does this change improve?
-EILSEQ is used to indicate that the tuple was not parsed by the SDIO core and should be passed to the SDIO driver via the SDIO func tuple list.
>
> >? ??? /* TPLFE_MAX_BLK_SIZE */
> >? ??? func->max_blksize =
> buf[12] | (buf[13] << 8);
> > @@ -154,13 +155,7 @@ static int cistpl_funce(struct mmc_card *card, struct sdio_func *func,
> >? ??? else
> >? ??? ??? ret = cistpl_funce_common(card, buf, size);
> >?
> > -??? if (ret) {
> > -??? ??? printk(KERN_ERR "%s: bad CISTPL_FUNCE size %u "
> > -??? ??? ? ? ???"type %u\n", mmc_hostname(card->host), size, buf[0]);
> > -??? ??? return ret;
> > -??? }
> > -
> > -??? return 0;
> > +??? return ret;
> >? }
> >?
> >? typedef int (tpl_parse_t)(struct mmc_card *, struct sdio_func *,
>
> Silencing a legitimate error.
>
Yes, I see your point.
I think we can keep this code but prevent displaying the error if ret == -EILSEQ (i.e. the tuple is "unknown"/"malformed" BUT should be passed to the SDIO driver for parsing).
> > +??? ??? if (ret == -EILSEQ) {
> > +??? ???
> ??? /* this tuple is unknown to the core */
>
> Misleading comment, the tuple might be known but malformed.
Same languange issue again.
>
> Rgds
> --
> ? ???-- Pierre Ossman
Thanks a lot for your comments,
Albert
--- El vie, 11/9/09, Andrew Morton <[email protected]> escribi?:
> > Some manufacturers provide vendor information in
> non-vendor specific CIS
> > tuples. For example, Broadcom uses an Extended
> Function tuple to provide
> > the MAC address on some of their network cards, as in
> the case of the
> > Nintendo Wii WLAN daughter card.
> >
> > This patch allows passing correct tuples unknown to
> the SDIO core to
> > a matching SDIO driver instead of rejecting them and
> failing.
> >
>
> This looks leaky to me.
>
Hi Andrew, thanks for the review.
I hope I can clarify a bit what the patch does in the next comments.
>
> : ??? ??? if (i < ARRAY_SIZE(cis_tpl_list)) {
> : ??? ??? ??? const struct cis_tpl *tpl = cis_tpl_list + i;
> : ??? ??? ??? if (tpl_link < tpl->min_size) {
> : ??? ??? ??? ??? printk(KERN_ERR
> : ??? ??? ??? ??? ? ? ???"%s: bad CIS tuple 0x%02x"
> : ??? ??? ??? ??? ? ? ???" (length = %u, expected >= %u)\n",
> : ??? ??? ??? ???mmc_hostname(card->host),
> : ??? ??? ??? ??? ? ? ???tpl_code, tpl_link, tpl->min_size);
> : ??? ??? ??? ??? ret = -EINVAL;
>
> ret == -EINVAL
>
At this point ret is not -EINVAL. If it was -EINVAL the code would have had exit at this snipped before:
if (ret) {
kfree(this);
break;
}
> : ??? ??? ??? } else if (tpl->parse) {
> : ??? ??? ??? ??? ret = tpl->parse(card, func,
> : ??? ??? ????????this->data, tpl_link);
> : ??? ??? ??? }
> : ??? ??? ??? /* already successfully parsed, not needed anymore */
> : ??? ??? ??? if (!ret)
> : ??? ??? ??? ??? kfree(this);
>
> `this' doesn't get freed
>
Yes, that's the whole point of the patch. It must be freed _only_ if the SDIO core parsed it. If the SDIO core cannot parse it then it gets passed to the SDIO driver via the SDIO func struct tuple list (later).
> : ??? ??? } else {
> : ??? ??? ???/* unknown tuple */
> : ??? ??? ???ret = -EILSEQ;
> : ??? ??? }
> :
> : ??? ??? if (ret == -EILSEQ) {
>
> `this' doesn't get remembered.
>
When ret is -EILSEQ `this' is linked to the SDIO func tuple list (later).
> : ??? ??? ???/* this tuple is unknown to the core */
> : ??? ??? ???this->next = NULL;
> : ??? ??? ???this->code = tpl_code;
> : ??? ??? ???this->size = tpl_link;
> : ??? ??? ???*prev = this;
> : ??? ??? ???prev = &this->next;
> : ??? ??? ???pr_debug("%s: queuing CIS tuple 0x%02x length %u\n",
> : ??? ??? ??? ?????mmc_hostname(card->host), tpl_code, tpl_link);
> : ??? ??? ???/* keep on analyzing tuples */
> : ??? ??? ???ret = 0;
> : ??? ??? }
> :
> : ??? ??? ptr += tpl_link;
>
> `this' leaks.
>
`this' doesn't leak. `this' has been linked to the SDIO func tuple list in:
*prev = this;
> : ??? } while (!ret);
>
> Please check?
>
Thanks a lot for you comments,
Albert
On Thu, 10 Sep 2009 14:56:42 +0200 Albert Herranz <[email protected]> wrote:
> Some manufacturers provide vendor information in non-vendor specific CIS
> tuples. For example, Broadcom uses an Extended Function tuple to provide
> the MAC address on some of their network cards, as in the case of the
> Nintendo Wii WLAN daughter card.
>
> This patch allows passing correct tuples unknown to the SDIO core to
> a matching SDIO driver instead of rejecting them and failing.
>
This looks leaky to me.
: if (i < ARRAY_SIZE(cis_tpl_list)) {
: const struct cis_tpl *tpl = cis_tpl_list + i;
: if (tpl_link < tpl->min_size) {
: printk(KERN_ERR
: "%s: bad CIS tuple 0x%02x"
: " (length = %u, expected >= %u)\n",
: mmc_hostname(card->host),
: tpl_code, tpl_link, tpl->min_size);
: ret = -EINVAL;
ret == -EINVAL
: } else if (tpl->parse) {
: ret = tpl->parse(card, func,
: this->data, tpl_link);
: }
: /* already successfully parsed, not needed anymore */
: if (!ret)
: kfree(this);
`this' doesn't get freed
: } else {
: /* unknown tuple */
: ret = -EILSEQ;
: }
:
: if (ret == -EILSEQ) {
`this' doesn't get remembered.
: /* this tuple is unknown to the core */
: this->next = NULL;
: this->code = tpl_code;
: this->size = tpl_link;
: *prev = this;
: prev = &this->next;
: pr_debug("%s: queuing CIS tuple 0x%02x length %u\n",
: mmc_hostname(card->host), tpl_code, tpl_link);
: /* keep on analyzing tuples */
: ret = 0;
: }
:
: ptr += tpl_link;
`this' leaks.
: } while (!ret);
Please check?
On Fri, 11 Sep 2009 07:52:11 +0000 (GMT) Albert Herranz <[email protected]> wrote:
> --- El vie, 11/9/09, Andrew Morton <[email protected]> escribi__:
> > > Some manufacturers provide vendor information in
> > non-vendor specific CIS
> > > tuples. For example, Broadcom uses an Extended
> > Function tuple to provide
> > > the MAC address on some of their network cards, as in
> > the case of the
> > > Nintendo Wii WLAN daughter card.
> > >
> > > This patch allows passing correct tuples unknown to
> > the SDIO core to
> > > a matching SDIO driver instead of rejecting them and
> > failing.
> > >
> >
> > This looks leaky to me.
> >
>
> Hi Andrew, thanks for the review.
> I hope I can clarify a bit what the patch does in the next comments.
>
> >
> > : ______ ______ if (i < ARRAY_SIZE(cis_tpl_list)) {
argh, now yahoo mail is converting tabs to 0xa0's as well. Despair.
> > : ______ ______ ______ const struct cis_tpl *tpl = cis_tpl_list + i;
> > : ______ ______ ______ if (tpl_link < tpl->min_size) {
> > : ______ ______ ______ ______ printk(KERN_ERR
> > : ______ ______ ______ ______ __ __ ______"%s: bad CIS tuple 0x%02x"
> > : ______ ______ ______ ______ __ __ ______" (length = %u, expected >= %u)\n",
> > : ______ ______ ______ ______mmc_hostname(card->host),
> > : ______ ______ ______ ______ __ __ ______tpl_code, tpl_link, tpl->min_size);
> > : ______ ______ ______ ______ ret = -EINVAL;
> >
> > ret == -EINVAL
> >
>
> At this point ret is not -EINVAL.
Yes it is. We just did
ret = -EINVAL;
If that assignment happens, we leak `this'.
--- El vie, 11/9/09, Andrew Morton <[email protected]> escribi?:
> > >
> > > ret == -EINVAL
> > >
> >
> > At this point ret is not -EINVAL.
>
> Yes it is.? We just did
>
> ??? ret = -EINVAL;
>
>
> If that assignment happens, we leak `this'.
>
Hi Andrew,
I misunderstood you. I thought that you were trying to imply on your original comment that retval was _already_ -EINVAL at that point.
Now I see the issue. `this' should be freed if successfully parsed (!ret) or if invalid and not going to be passed to a SDIO driver (ret == -EINVAL).
Thanks for catching that. I'll send an updated patch.
Cheers,
Albert
On Thu, 10 Sep 2009 14:56:42 +0200
Albert Herranz <[email protected]> wrote:
> Some manufacturers provide vendor information in non-vendor specific CIS
> tuples. For example, Broadcom uses an Extended Function tuple to provide
> the MAC address on some of their network cards, as in the case of the
> Nintendo Wii WLAN daughter card.
>
> This patch allows passing correct tuples unknown to the SDIO core to
> a matching SDIO driver instead of rejecting them and failing.
>
> Signed-off-by: Albert Herranz <[email protected]>
> ---
The description for this patch should be made clearer. The title
suggests it adds functionality that's already in place. It should be
something along the lines of "Also pass malformed tuples to card
drivers".
In the sake of sanity, you might want to add this behaviour to all
parsers, not just the FUNCE one.
I'm also unclear on how this is supposed to work. What does the
broadcom tuple look like? This patch looks like it will silence a lot
of legitimate warnings, and possibly pollute the card structures with
bogus data.
> diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
> index 963f293..87934ac 100644
> --- a/drivers/mmc/core/sdio_cis.c
> +++ b/drivers/mmc/core/sdio_cis.c
> @@ -123,8 +123,9 @@ static int cistpl_funce_func(struct sdio_func *func,
> vsn = func->card->cccr.sdio_vsn;
> min_size = (vsn == SDIO_SDIO_REV_1_00) ? 28 : 42;
>
> + /* let the SDIO driver take care of unknown tuples */
> if (size < min_size || buf[0] != 1)
Misleading comment, the tuple is not unknown.
> - return -EINVAL;
> + return -EILSEQ;
>
What does this change improve?
> /* TPLFE_MAX_BLK_SIZE */
> func->max_blksize = buf[12] | (buf[13] << 8);
> @@ -154,13 +155,7 @@ static int cistpl_funce(struct mmc_card *card, struct sdio_func *func,
> else
> ret = cistpl_funce_common(card, buf, size);
>
> - if (ret) {
> - printk(KERN_ERR "%s: bad CISTPL_FUNCE size %u "
> - "type %u\n", mmc_hostname(card->host), size, buf[0]);
> - return ret;
> - }
> -
> - return 0;
> + return ret;
> }
>
> typedef int (tpl_parse_t)(struct mmc_card *, struct sdio_func *,
Silencing a legitimate error.
> + if (ret == -EILSEQ) {
> + /* this tuple is unknown to the core */
Misleading comment, the tuple might be known but malformed.
Rgds
--
-- Pierre Ossman
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.