| Message ID | 1409700683-25393-2-git-send-email-pab@pabigot.com |
|---|---|
| State | Superseded |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
On 09/02/2014 05:31 PM, Peter A. Bigot wrote: > Though it might be expected to do so, mmc_switch_part() does not change > the part_num field of the device on which the partition has been > changed. As such, checking to see whether the partition is already the > target partition will fail to correctly restore the original > configuration in cases like env_mmc which rely on this behavior to > avoid having to preserve the pre-switch partition number outside the > device structure. > diff --git a/common/env_mmc.c b/common/env_mmc.c > - if (part != mmc->part_num) { > - ret = mmc_switch_part(dev, part); > - if (ret) > - puts("MMC partition switch failed\n"); > - } > + ret = mmc_switch_part(dev, part); > + if (ret) > + puts("MMC partition switch failed\n"); I'm not sure this is correct, but it might be. I believe the if() is present to avoid any attempt to call mmc_switch_part() on a device that doesn't have HW partitions (in which case, both part and mmc->part_num should already be 0), since such an attempt might print an error message. If you don't observe any error message printed after this change, then perhaps this patch is fine.
On 09/03/2014 10:46 AM, Stephen Warren wrote: > On 09/02/2014 05:31 PM, Peter A. Bigot wrote: >> Though it might be expected to do so, mmc_switch_part() does not change >> the part_num field of the device on which the partition has been >> changed. As such, checking to see whether the partition is already the >> target partition will fail to correctly restore the original >> configuration in cases like env_mmc which rely on this behavior to >> avoid having to preserve the pre-switch partition number outside the >> device structure. > >> diff --git a/common/env_mmc.c b/common/env_mmc.c > >> - if (part != mmc->part_num) { >> - ret = mmc_switch_part(dev, part); >> - if (ret) >> - puts("MMC partition switch failed\n"); >> - } >> + ret = mmc_switch_part(dev, part); >> + if (ret) >> + puts("MMC partition switch failed\n"); > > I'm not sure this is correct, but it might be. > > I believe the if() is present to avoid any attempt to call > mmc_switch_part() on a device that doesn't have HW partitions (in > which case, both part and mmc->part_num should already be 0), since > such an attempt might print an error message. That could be true. The patch that added the feature didn't provide that information. In my case, the device does have HW partitions. > If you don't observe any error message printed after this change, then > perhaps this patch is fine. It does work in my environment, but would not retain the behavior you describe. The existing code is still wrong, but the error is elsewhere: I'll provide a new patch to supersede this one. Peter
diff --git a/common/env_mmc.c b/common/env_mmc.c index a7621a8..9556296 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -78,11 +78,9 @@ static int mmc_set_env_part(struct mmc *mmc) dev = 0; #endif - if (part != mmc->part_num) { - ret = mmc_switch_part(dev, part); - if (ret) - puts("MMC partition switch failed\n"); - } + ret = mmc_switch_part(dev, part); + if (ret) + puts("MMC partition switch failed\n"); return ret; } @@ -113,8 +111,7 @@ static void fini_mmc_for_env(struct mmc *mmc) #ifdef CONFIG_SPL_BUILD dev = 0; #endif - if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) - mmc_switch_part(dev, mmc->part_num); + mmc_switch_part(dev, mmc->part_num); #endif }
Though it might be expected to do so, mmc_switch_part() does not change the part_num field of the device on which the partition has been changed. As such, checking to see whether the partition is already the target partition will fail to correctly restore the original configuration in cases like env_mmc which rely on this behavior to avoid having to preserve the pre-switch partition number outside the device structure. Signed-off-by: Peter A. Bigot <pab@pabigot.com> --- common/env_mmc.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)