On Mon, Jan 09, 2006 at 06:16:02PM -0400, Anderson Briglia wrote:
> + dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable);
> + if (!dev)
> + goto error;
> + card = dev_to_mmc_card(dev);
> +
> + if (operation == KEY_OP_INSTANTIATE) { /* KEY_OP_INSTANTIATE */
> + 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);
I really don't like this - if the MMC card is not locked, we set a
password on it. If it's locked, we unlock it.
That's a potential race condition if you're trying to unlock a card
and the card is changed beneath you while you slept waiting for
memory - you end up setting that password on the new card.
It's far better to have separate "unlock this card" and "set a
password on this card" commands rather than trying to combine the
two operations.
Also, removing and re-registering a card is an offence. These
things are ref-counted, and mmc_remove_card() will drop the last
reference - so the memory associated with it will be freed. Then
you re-register it. Whoops.
If you merely want to try to attach a driver, use device_attach()
instead.
Also, what if you have multiple MMC cards? I have a board here
with two MMC slots. I'd rather not have it try to set the same
password on both devices.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
Russell King wrote:
>On Mon, Jan 09, 2006 at 06:16:02PM -0400, Anderson Briglia wrote:
>
>
>>+ dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable);
>>+ if (!dev)
>>+ goto error;
>>+ card = dev_to_mmc_card(dev);
>>+
>>+ if (operation == KEY_OP_INSTANTIATE) { /* KEY_OP_INSTANTIATE */
>>+ 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);
>>
>>
>
>I really don't like this - if the MMC card is not locked, we set a
>password on it. If it's locked, we unlock it.
>
>That's a potential race condition if you're trying to unlock a card
>and the card is changed beneath you while you slept waiting for
>memory - you end up setting that password on the new card.
>
>It's far better to have separate "unlock this card" and "set a
>password on this card" commands rather than trying to combine the
>two operations.
>
>
Ok.
>Also, removing and re-registering a card is an offence. These
>things are ref-counted, and mmc_remove_card() will drop the last
>reference - so the memory associated with it will be freed. Then
>you re-register it. Whoops.
>
>If you merely want to try to attach a driver, use device_attach()
>instead.
>
>
If we use device_attach(), the mmc_block driver is not informed about
the card's unlocking. I did some tests, using device_attach() instead of
those mmc functions and seems that the mmc_block driver tries to use a
invalid device reference. What do you suggest on this case?
>Also, what if you have multiple MMC cards? I have a board here
>with two MMC slots. I'd rather not have it try to set the same
>password on both devices.
>
>
Sorry, but this series of patches only support one mmc host. I'll update
the TODO section of the summary e-mail.
Anderson Briglia
INdT - Manaus
Anderson Briglia wrote:
> Russell King wrote:
>
>
>>On Mon, Jan 09, 2006 at 06:16:02PM -0400, Anderson Briglia wrote:
>>
>>
>>
>>>+ dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable);
>>>+ if (!dev)
>>>+ goto error;
>>>+ card = dev_to_mmc_card(dev);
>>>+
>>>+ if (operation == KEY_OP_INSTANTIATE) { /* KEY_OP_INSTANTIATE */
>>>+ 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);
>>>
>
>>Also, removing and re-registering a card is an offence. These
>>things are ref-counted, and mmc_remove_card() will drop the last
>>reference - so the memory associated with it will be freed. Then
>>you re-register it. Whoops.
>>
>>If you merely want to try to attach a driver, use device_attach()
>>instead.
>>
We changed the mmc_remove_card() and mmc_register_card() by device_release_driver() and
device_attach(), supposedly avoiding ref-counts issues.
Regards,
Anderson Briglia
INdT - Manaus
On Wed, Jan 11, 2006 at 09:58:03AM -0400, Anderson Briglia wrote:
> Anderson Briglia wrote:
> > Russell King wrote:
> >
> >
> >>On Mon, Jan 09, 2006 at 06:16:02PM -0400, Anderson Briglia wrote:
> >>
> >>
> >>
> >>>+ dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable);
> >>>+ if (!dev)
> >>>+ goto error;
> >>>+ card = dev_to_mmc_card(dev);
> >>>+
> >>>+ if (operation == KEY_OP_INSTANTIATE) { /* KEY_OP_INSTANTIATE */
> >>>+ 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);
> >>>
> >
> >>Also, removing and re-registering a card is an offence. These
> >>things are ref-counted, and mmc_remove_card() will drop the last
> >>reference - so the memory associated with it will be freed. Then
> >>you re-register it. Whoops.
> >>
> >>If you merely want to try to attach a driver, use device_attach()
> >>instead.
> >>
> We changed the mmc_remove_card() and mmc_register_card() by
> device_release_driver() and device_attach(), supposedly avoiding
> ref-counts issues.
As per my previous mail - I think this probably comes down to differences
between mainline and the omap tree. My suggestion should work fine
in mainline. I can only suspect that the OMAP tree is doing something
it shouldn't.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core