Re: [PATCH] sdio: pass unknown cis tuples to sdio drivers

[Albert Herranz <albert_herranz-mRCrAkd8dF0@public.gmane.org>]

--- El vie, 11/9/09, Pierre Ossman <pierre-vCPtPcF4ZGuHXe+LvDLADg@public.gmane.org> 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



      
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html