mmc: block: Remove error check of hw_reset on reset
PATCH v3 1/2 mmc: block: Remove error check of hw_reset on reset
Before switching back to the right partition in mmc_blk_reset there used to be a check if hw_reset was even supported. This return value was removed, so there is no reason to check. Furthermore ensure part_curr is not falsely set to a valid value on reset or partition switch error.
---
-v3: Ensure invalid part_curr on error
-v2: Do not attempt to switch partitions if reset failed
drivers/mmc/core/block.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
drivers/mmc/core/block.c +14 -15
@@ -134,6 +134,7 @@ struct mmc_blk_data {* track of the current selected device partition. */ unsigned int part_curr; +#define MMC_BLK_PART_INVALID UINT_MAX/* Unknown partition active */ int area_type;/* debugfs files (only in main mmc_blk_data) */
@@ -991,29 +992,27 @@ static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, int type) {int err; +struct mmc_blk_data *main_md =dev_get_drvdata (&host->card->dev); +int part_err;if (md->reset_done & type)return -EEXIST ; md->reset_done |= type; + main_md->part_curr =MMC_BLK_PART_INVALID ; err =mmc_hw_reset (host->card); +if (err) +return err;/* Ensure we switch back to the correct partition */ -if (err) { -struct mmc_blk_data *main_md = -dev_get_drvdata (&host->card->dev); -int part_err; - - main_md->part_curr = main_md->part_type; - part_err =mmc_blk_part_switch (host->card, md->part_type); -if (part_err) { -/* -* We have failed to get back into the correct -* partition, so we need to abort the whole request. -*/ -return -ENODEV ; - } + part_err =mmc_blk_part_switch (host->card, md->part_type); +if (part_err) { +/* +* We have failed to get back into the correct +* partition, so we need to abort the whole request. +*/ +return -ENODEV ; } -return err; +return 0 ; }static inline void mmc_blk_reset_success (struct mmc_blk_data *md,int type)
Signature · 4 lines
2.37.3 Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
Quoted 11 lines · Christian Löhle
Before switching back to the right partition in
mmc_blk_resetthere used to be a check ifhw_resetwas even supported. This return value was removed, so there is no reason to check. Furthermore ensurepart_curris not falsely set to a valid value on reset or partition switch error.Fixes fefdd3c91e0a ("mmc: core: Drop superfluous validations in mmc_hw|sw_reset()")Signed-off-by Christian Loehle <[email protected]>
This looks good to me, however I am awaiting and ack from Adrian before applying, to make sure we are all aligned with the approach.
Kind regards
Uffe
Quoted 68 lines
---
(quoted diff) +0
-2-v3: Ensure invalid part_curr on error -v2: Do not attempt to switch partitions if reset failed drivers/mmc/core/block.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)drivers/mmc/core/block.c +14
-16@@ -134,6 +134,7 @@ struct mmc_blk_data {* track of the current selected device partition. */ unsigned int part_curr; +#define MMC_BLK_PART_INVALID UINT_MAX/* Unknown partition active */ int area_type;/* debugfs files (only in main mmc_blk_data) */ @@ -991,29 +992,27 @@ static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, int type) {int err; +struct mmc_blk_data *main_md =dev_get_drvdata (&host->card->dev); +int part_err;if (md->reset_done & type)return -EEXIST ; md->reset_done |= type; + main_md->part_curr =MMC_BLK_PART_INVALID ; err =mmc_hw_reset (host->card); +if (err) +return err;/* Ensure we switch back to the correct partition */ -if (err) { -struct mmc_blk_data *main_md = -dev_get_drvdata (&host->card->dev); -int part_err; - - main_md->part_curr = main_md->part_type; - part_err =mmc_blk_part_switch (host->card, md->part_type); -if (part_err) { -/* -* We have failed to get back into the correct -* partition, so we need to abort the whole request. -*/ -return -ENODEV ; - } + part_err =mmc_blk_part_switch (host->card, md->part_type); +if (part_err) { +/* +* We have failed to get back into the correct +* partition, so we need to abort the whole request. +*/ +return -ENODEV ; } -return err; +return 0 ; }static inline void mmc_blk_reset_success (struct mmc_blk_data *md,int type) --2.37.3
Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
Quoted 31 lines · Christian Löhle
Before switching back to the right partition in
mmc_blk_resetthere used to be a check ifhw_resetwas even supported. This return value was removed, so there is no reason to check. Furthermore ensurepart_curris not falsely set to a valid value on reset or partition switch error.Fixes fefdd3c91e0a ("mmc: core: Drop superfluous validations in mmc_hw|sw_reset()")Signed-off-by Christian Loehle <[email protected]>---
(quoted diff) +0
-2-v3: Ensure invalid part_curr on error -v2: Do not attempt to switch partitions if reset failed drivers/mmc/core/block.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)drivers/mmc/core/block.c +1
-0@@ -134,6 +134,7 @@ struct mmc_blk_data {* track of the current selected device partition. */ unsigned int part_curr; +#define MMC_BLK_PART_INVALID UINT_MAX/* Unknown partition active */ int area_type;/* debugfs files (only in main mmc_blk_data) */ @@ -991,29 +992,27 @@ static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, int type)
drivers/mmc/core/block.c static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, · line 1002
Quoted 10 lines
drivers/mmc/core/block.c +3
-0{int err; +struct mmc_blk_data *main_md =dev_get_drvdata (&host->card->dev); +int part_err;if (md->reset_done & type)return -EEXIST ; md->reset_done |= type; + main_md->part_curr =MMC_BLK_PART_INVALID ;
This forces a partition switch even if it is not necessary.
drivers/mmc/core/block.c static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, · line 1003
Quoted 1 line
drivers/mmc/core/block.c +0
-0err =mmc_hw_reset (host->card);
This would be better:
/* * A successful reset will leave the card in the main partition, but * upon failure it might not be, so set it to MMC_BLK_PART_INVALID * in that case. */ main_md->part_curr = err ? MMC_BLK_PART_INVALID : main_md->part_type;
drivers/mmc/core/block.c static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, · line 1005
Quoted 2 lines
drivers/mmc/core/block.c +2
-0+if (err) +return err;
There was a time when mmc requests would be retried directly without going back through the block layer. I don't think that can happen anymore after a failed reset. However, if you are going to skip the partition switch, the commit message needs to explain that you have checked the code paths and we never retry a request directly after a failed reset. Also a comment is needed to say the same.
Quoted 7 lines
drivers/mmc/core/block.c +0
-6/* Ensure we switch back to the correct partition */ -if (err) { -struct mmc_blk_data *main_md = -dev_get_drvdata (&host->card->dev); -int part_err; - - main_md->part_curr = main_md->part_type;
drivers/mmc/core/block.c static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, · line 1008
Quoted 10 lines
drivers/mmc/core/block.c +2
-8- part_err =mmc_blk_part_switch (host->card, md->part_type); -if (part_err) { -/* -* We have failed to get back into the correct -* partition, so we need to abort the whole request. -*/ -return -ENODEV ; - } + part_err =mmc_blk_part_switch (host->card, md->part_type); +if (part_err) {
Don't really need part_err. i.e. could be
if (mmc_blk_part_switch(host->card, md->part_type) {Quoted 11 lines
drivers/mmc/core/block.c +6
-1+/* +* We have failed to get back into the correct +* partition, so we need to abort the whole request. +*/ +return -ENODEV ; } -return err; +return 0 ; }static inline void mmc_blk_reset_success (struct mmc_blk_data *md,int type)
Quoted 38 lines · Adrian Hunter
Quoted 36 lines · Christian Löhle
Before switching back to the right partition in
mmc_blk_resetthere used to be a check ifhw_resetwas even supported. This return value was removed, so there is no reason to check. Furthermore ensurepart_curris not falsely set to a valid value on reset or partition switch error.Fixes fefdd3c91e0a ("mmc: core: Drop superfluous validations in mmc_hw|sw_reset()")Signed-off-by Christian Loehle <[email protected]>---
(quoted diff) +0
-2-v3: Ensure invalid part_curr on error -v2: Do not attempt to switch partitions if reset failed drivers/mmc/core/block.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)drivers/mmc/core/block.c +3
-0@@ -134,6 +134,7 @@ struct mmc_blk_data {* track of the current selected device partition. */ unsigned int part_curr; +#define MMC_BLK_PART_INVALID UINT_MAX/* Unknown partition active */ int area_type;/* debugfs files (only in main mmc_blk_data) */ @@ -991,29 +992,27 @@ static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, int type) {int err; +struct mmc_blk_data *main_md =dev_get_drvdata (&host->card->dev); +int part_err;
Quoted 7 lines
drivers/mmc/core/block.c static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, · line 1002
Quoted 5 lines
drivers/mmc/core/block.c +1
-0if (md->reset_done & type)return -EEXIST ; md->reset_done |= type; + main_md->part_curr =MMC_BLK_PART_INVALID ;This forces a partition switch even if it is not necessary.
Right, very good point!
Quoted 12 lines
drivers/mmc/core/block.c static int mmc_blk_reset(struct mmc_blk_data *md, struct mmc_host *host, · line 1003
Quoted 2 lines
drivers/mmc/core/block.c +0
-0err =mmc_hw_reset (host->card);This would be better:
/*
* A successful reset will leave the card in the main partition, but
* upon failure it might not be, so set it toMMC_BLK_PART_INVALID
* in that case.
*/
main_md->part_curr= err ?MMC_BLK_PART_INVALID:main_md->part_type;
Yes! And the comment is valuable too!
[...]
Kind regards
Uffe