Re: [patch 0/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

Re: [patch 0/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> Hi folks,
>
>   

Hi Carlos,

This is very nice work and it is something that should be in the kernel.

Unfortunately, I won't have time to look at this until the end of the
week. So just hang tight, I haven't overlooked you. :)

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 0/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Russell King]

On Mon, Oct 23, 2006 at 08:03:31AM +0200, Pierre Ossman wrote:
> Carlos Aguiar wrote:
> > Hi folks,
> 
> Hi Carlos,
> 
> This is very nice work and it is something that should be in the kernel.
> 
> Unfortunately, I won't have time to look at this until the end of the
> week. So just hang tight, I haven't overlooked you. :)

Just make sure that it checks for MMC_CAP_BYTEBLOCK - if that flag isn't
set, the host can't do non-power of two transfers, so probably password
support has to be refused on such hosts.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 1/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> When a card is locked, only commands from the "basic" and "lock card" classes
> are accepted. To be able to use the other commands, the card must be unlocked
> first.
>
> This patch prevents the block driver from trying to run privileged class
> commands on locked MMC cards, which will fail anyway.
>   

Incorrect commit message. It stops driver probes (all of them).

> 20 11:41:54.000000000 -0400
> @@ -85,6 +85,7 @@ struct mmc_host {
>  	unsigned long		caps;		/* Host capabilities */
>  
>  #define MMC_CAP_4_BIT_DATA	(1 << 0)	/* Can the host do 4 bit transfers */
> +#define MMC_CAP_LOCK_UNLOCK	(1 << 1)	/* Host password support capability */
>  
>  	/* host specific block data */
>  	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
>
>   

You  need to rebase your patch set on a more recent kernel. This won't
apply cleanly.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 2/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> +int mmc_key_instantiate(struct key *key, const void *data, size_t datalen)
>   

static

> +int mmc_key_match(const struct key *key, const void *description)
>   

static

> +void mmc_key_destroy(struct key *key)
>   

static

> @@ -335,6 +403,15 @@ static int __init mmc_init(void)
>  		ret = class_register(&mmc_host_class);
>  		if (ret)
>  			bus_unregister(&mmc_bus_type);
> +#ifdef	CONFIG_MMC_PASSWORDS
> +		else {
> +			ret = register_key_type(&mmc_key_type);
> +			if (ret) {
> +				class_unregister(&mmc_host_class);
> +				bus_unregister(&mmc_bus_type);
> +			}
> +		}
> +#endif
>  	}
>  	return ret;
>  }
>   

We're starting to get a bit of code duplication here. Perhaps an error
handling section at the end of the function would be better.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 3/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> @@ -1071,10 +1074,10 @@ static void mmc_check_cards(struct mmc_h
>  		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>  
>  		err = mmc_wait_for_cmd(host, &cmd, CMD_RETRIES);
> -		if (err == MMC_ERR_NONE)
> +		if (err != MMC_ERR_NONE) {
> +			mmc_card_set_dead(card);
>  			continue;
> -
> -		mmc_card_set_dead(card);
> +		}
>  	}
>  }
>  
>   

This seems like a pointless change.

> @@ -1160,6 +1163,139 @@ static void mmc_setup(struct mmc_host *h
>  		mmc_read_scrs(host);
>  }
>  
> +/* Calculate the minimal blksz_bits to hold x bytes. */
> +static inline int blksz_bits(unsigned x)
> +{
> +	return fls(x-1);
> +}
> +
>   

blksz_bits is gone, so this is superfluous. Note Russell's comment about
looking at the MMC_CAP_BYTEBLOCK capability, something you do not do
right now.

> +/**
> + *	mmc_lock_unlock - send LOCK_UNLOCK command to a specific card.
> + *	@card: card to which the LOCK_UNLOCK command should be sent
> + *	@key: key containing the MMC password
> + *	@mode: LOCK_UNLOCK mode
> + *
> + */
> +int mmc_lock_unlock(struct mmc_card *card, struct key *key, int mode)
> +{
> +	struct mmc_request mrq;
> +	struct mmc_command cmd;
> +	struct mmc_data data;
> +	struct scatterlist sg;
> +	struct mmc_key_payload *mpayload;
> +	unsigned long erase_timeout;
> +	int err, data_size;
> +	u8 *data_buf;
> +
> +	mpayload = NULL;
> +	data_size = 1;
> +	if (mode != MMC_LOCK_MODE_ERASE) {
> +		mpayload = rcu_dereference(key->payload.data);
> +		data_size = 2 + mpayload->datalen;
> +	}
> +
> +	data_buf = kmalloc(data_size, GFP_KERNEL);
>   

For something that can be at most 34 bytes, a kmalloc seems excessive.
Put it on the stack. Just remember to have checks so we do not overflow.

> +	if (!data_buf)
> +		return -ENOMEM;
> +	memset(data_buf, 0, data_size);
> +
> +	data_buf[0] = mode;
> +	if (mode != MMC_LOCK_MODE_ERASE) {
> +		data_buf[1] = mpayload->datalen;
> +		memcpy(data_buf + 2, mpayload->data, mpayload->datalen);
> +	}
> +
> +	err = mmc_card_claim_host(card);
> +	if (err != MMC_ERR_NONE) {
> +		mmc_card_set_dead(card);
> +		goto out;
> +	}
>   

Locking should be done outside this function to avoid races.

Also, why mark the card as dead because you cannot select it? It might
just be a temporary failure.

> +
> +	memset(&cmd, 0, sizeof(struct mmc_command));
> +
> +	cmd.opcode = MMC_SET_BLOCKLEN;
> +	cmd.arg = data_size;
> +	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +	err = mmc_wait_for_cmd(card->host, &cmd, CMD_RETRIES);
> +	if (err != MMC_ERR_NONE) {
> +		mmc_card_set_dead(card);
> +		goto error;
> +	}
>   

Same here.

> +
> +	sg_init_one(&sg, data_buf, data_size);
> +	err = mmc_wait_for_req(card->host, &mrq);
> +	if (err != MMC_ERR_NONE) {
> +		if(err != MMC_ERR_INVALID)
> +			mmc_card_set_dead(card);
> +		goto error;
> +	}
>   

Dito.

> +
> +EXPORT_SYMBOL(mmc_lock_unlock);
>   

Why would anything but mmc_core need to use this?

>  
>  /**
>   *	mmc_detect_change - process change of state on a MMC socket
> Index: linux-2.6.18/include/linux/mmc/card.h
> ===================================================================
> --- linux-2.6.18.orig/include/linux/mmc/card.h	2006-10-20 12:37:42.000000000 -0400
> +++ linux-2.6.18/include/linux/mmc/card.h	2006-10-20 12:38:18.000000000 -0400
> @@ -117,4 +117,8 @@ static inline int mmc_card_claim_host(st
>  
>  #define mmc_card_release_host(c)	mmc_release_host((c)->host)
>  
> +struct key;
> +
> +extern int mmc_lock_unlock(struct mmc_card *card, struct key *key, int mode);
> +
>  #endif
>   

Which means this should go away (and put in drivers/mmc/mmc.h).

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 4/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> Patch to add the host MMC lock/unlock capability support for OMAP platform.
>   

Now what could possibly be the purpose of this? What hardware feature is
required to support these commands?

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 5/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> Implement MMC password force erase, remove password, change password,
> unlock card and assign password operations. It uses the sysfs mechanism
> to send commands to the MMC subsystem. 
>   

There are some indentation problems with this patch.

Also, what's the difference between "change" and "assign"? The code
seems to do the same thing.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 6/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[Pierre Ossman]

Carlos Aguiar wrote:
> Removes an unused function: mmc_omap_switch_callback() and
> changed IRQ comparison in omap.c
>   

This looks completely unrelated to password support.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: [patch 3/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

[David Brownell]

On Sunday 29 October 2006 1:49 am, Pierre Ossman wrote:

> > +	data_buf = kmalloc(data_size, GFP_KERNEL);
> >   
> 
> For something that can be at most 34 bytes, a kmalloc seems excessive.
> Put it on the stack. Just remember to have checks so we do not overflow.

It does seem excessive, but stack-allocated buffers are not guaranteed
to be DMA-safe.  See Documentation/DMA-mapping.txt right in the first
major section "What memory is DMA'able?"...