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

[Pierre Ossman <pierre-vCPtPcF4ZGuHXe+LvDLADg@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]

On Thu, 10 Sep 2009 14:56:42 +0200
Albert Herranz <albert_herranz-mRCrAkd8dF0@public.gmane.org> 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 <albert_herranz-mRCrAkd8dF0@public.gmane.org>
> ---

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]