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
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?"...