On Thursday 29 December 2005 03:40, Anderson Lizardo wrote: Something that sort of caught my eye while looking at this (I generally don't post here so I'm bitting the bullet and hoping I don't screw up), it seems that this is an experimental driver, but doesn't contain any sort of uniquely seperated verbose debug information. Let me try and narrow that down: > +int mmc_key_instantiate(struct key *key, const void *data, size_t datalen) > +{ > + struct mmc_key_payload *mpayload; > + struct device *dev; > + struct mmc_card *card; > + int ret; > + > + ret = -EINVAL; > + if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data) > + goto error; Right here something about the data being passed to the function is invalid. > + ret = key_payload_reserve(key, datalen); > + if (ret < 0) > + goto error; > + > + ret = -ENOMEM; > + mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL); > + if (!mpayload) > + goto error; Unable to allocate mpayload structure, or something of the like. > + /* attach the data */ > + mpayload->datalen = datalen; > + memcpy(mpayload->data, data, datalen); > + rcu_assign_pointer(key->payload.data, mpayload); > + > + ret = -EINVAL; > + dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable); > + if (!dev) > + goto error; Unable to locate device. > + card = dev_to_mmc_card(dev); > + if (mmc_card_locked(card)) { > + ret = mmc_lock_unlock(card, key, MMC_LOCK_MODE_UNLOCK); > + mmc_remove_card(card); > + mmc_register_card(card); > + } else > + ret = mmc_lock_unlock(card, key, MMC_LOCK_MODE_SET_PWD); > + if (ret) > + ret = -EKEYREJECTED; Key was rejected, though I suppose EKEYREJECTED pretty much states that. [snip snip] > + > +/* > + * update a mmc key > + * - the key's semaphore is write-locked > + */ > +int mmc_key_update(struct key *key, const void *data, size_t datalen) > +{ > + struct mmc_key_payload *mpayload, *zap; > + struct device *dev; > + struct mmc_card *card; > + int ret; > + > + ret = -EINVAL; > + if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data) > + goto error; See above about invalid data > + /* construct a replacement payload */ > + ret = -ENOMEM; > + mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL); > + if (!mpayload) > + goto error; This code almost seemed similiar to mmc_key_instantiate.. I almost wonder if the code could be consolidated into a single function with some sort of update conditional code. With that the debug information wouldn't be duplicated. so snip > +#ifdef CONFIG_MMC_PASSWORDS > + else { > + ret = register_key_type(&mmc_key_type); > + if (ret) { Something about the registration failing. > + class_unregister(&mmc_host_class); > + bus_unregister(&mmc_bus_type); > + } > + } > +#endif > } > return ret; > } > @@ -345,6 +501,9 @@ static void __exit mmc_exit(void) > { > class_unregister(&mmc_host_class); > bus_unregister(&mmc_bus_type); > +#ifdef CONFIG_MMC_PASSWORDS > + unregister_key_type(&mmc_key_type); > +#endif > } > > module_init(mmc_init); That was mainly it. The verbose debug information is more of a "this would be nice" sort of thing. Just from a user's perspective of debuggin experimental drivers, this sort of thing is always nice. The code duplication in mmc_key_instantiate/update still catches my eye though, there may be a functional code flow to this that I'm not aware of, so again I bite the bullet. Best hope that I don't make a fool of myself :). Chris White
Attachment:
pgpWiBkHN2KXs.pgp
Description: PGP signature
- References:
- [patch 0/5] Add MMC password protection (lock/unlock) support V2
- From: Anderson Lizardo <[email protected]>
- [patch 3/5] Add MMC password protection (lock/unlock) support V2
- From: Anderson Lizardo <[email protected]>
- [patch 0/5] Add MMC password protection (lock/unlock) support V2
- Prev by Date: Re: [patch 1/3] mutex subsystem: trylock
- Next by Date: Re: [PATCH] ipw2200 stack reduction
- Previous by thread: [patch 3/5] Add MMC password protection (lock/unlock) support V2
- Next by thread: [patch 4/5] Add MMC password protection (lock/unlock) support V2
- Index(es):