| Message ID | 003d34d1643242488b533dc14f69830f@hyperstone.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2,1/2] mmc: block: Remove error check of hw_reset on reset | expand |
On 7/10/22 18:42, Christian Löhle wrote: > 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. > > Fixes: fefdd3c91e0a ("mmc: core: Drop superfluous validations in mmc_hw|sw_reset()") > Cc: stable@vger.kernel.org > > Signed-off-by: Christian Loehle <cloehle@hyperstone.com> > --- > -v2: Do not attempt to switch partitions if reset failed > > drivers/mmc/core/block.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index ce89611a136e..8db72cba2bbe 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -991,29 +991,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; > err = mmc_hw_reset(host->card); > + if (err) > + return err; This could be a potential source of data corruption. There is no guarantee that a subsequent I/O will fail just because the reset failed. Reading / writing the wrong partition would be disastrous, so we should always try to get back to the correct partition. I haven't looked at the possibility of just flagging the partition as invalid - need to be sure any subsequent I/O attempts still go through a path that switches the partition. > /* 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; > - } > + 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; > } > - return err; > + return 0; > } > > static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type)
> >-----Original Message----- >From: Adrian Hunter <adrian.hunter@intel.com> >Sent: Samstag, 8. Oktober 2022 10:38 >To: Christian Löhle <CLoehle@hyperstone.com>; Ulf Hansson <ulf.hansson@linaro.org>; Linux MMC List <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org >Subject: Re: [PATCHv2 1/2] mmc: block: Remove error check of hw_reset on reset > >On 7/10/22 18:42, Christian Löhle wrote: >> 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. >> >> Fixes: fefdd3c91e0a ("mmc: core: Drop superfluous validations in >> mmc_hw|sw_reset()") >> Cc: stable@vger.kernel.org >> >> Signed-off-by: Christian Loehle <cloehle@hyperstone.com> >> --- >> -v2: Do not attempt to switch partitions if reset failed >> >> drivers/mmc/core/block.c | 28 +++++++++++++--------------- >> 1 file changed, 13 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index >> ce89611a136e..8db72cba2bbe 100644 >> --- a/drivers/mmc/core/block.c >> +++ b/drivers/mmc/core/block.c >> @@ -991,29 +991,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; >> err = mmc_hw_reset(host->card); >> + if (err) >> + return err; > > This could be a potential source of data corruption. > > There is no guarantee that a subsequent I/O will fail just because the reset failed. Reading / writing the wrong partition would be disastrous, so we should always try to get back to the correct partition. > > I haven't looked at the possibility of just flagging the partition as invalid - need to be sure any subsequent I/O attempts still go through a path that switches the partition. I can see where youre coming from, but similarly a failing mmc_blk_part_switch doesn't imply all subsequent IO will fail. Flagging the partition as invalid can be seen as rendering the system to a potentially useless state, which is a bit overboard for e.g. one CRC7 failure on the switch. Not sure yet what the ideal behavior is, but either way I would go with v1 1/2 and v2 2/2 for now? That already fixes imo the most relevant potential data corruptions. (successful reset -> no switch) Then we can come up with a a good handling for mmc_blk_reset or even around mmc_blk_part_switch. Or what do you suggest? Regards, Christian Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz Managing Director: Dr. Jan Peter Berns. Commercial register of local courts: Freiburg HRB381782
On 10/10/22 11:08, Christian Löhle wrote: > >> >> -----Original Message----- >> From: Adrian Hunter <adrian.hunter@intel.com> >> Sent: Samstag, 8. Oktober 2022 10:38 >> To: Christian Löhle <CLoehle@hyperstone.com>; Ulf Hansson <ulf.hansson@linaro.org>; Linux MMC List <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org >> Subject: Re: [PATCHv2 1/2] mmc: block: Remove error check of hw_reset on reset >> >> On 7/10/22 18:42, Christian Löhle wrote: >>> 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. >>> >>> Fixes: fefdd3c91e0a ("mmc: core: Drop superfluous validations in >>> mmc_hw|sw_reset()") >>> Cc: stable@vger.kernel.org >>> >>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com> >>> --- >>> -v2: Do not attempt to switch partitions if reset failed >>> >>> drivers/mmc/core/block.c | 28 +++++++++++++--------------- >>> 1 file changed, 13 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index >>> ce89611a136e..8db72cba2bbe 100644 >>> --- a/drivers/mmc/core/block.c >>> +++ b/drivers/mmc/core/block.c >>> @@ -991,29 +991,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; >>> err = mmc_hw_reset(host->card); >>> + if (err) >>> + return err; >> >> This could be a potential source of data corruption. >> >> There is no guarantee that a subsequent I/O will fail just because the reset failed. Reading / writing the wrong partition would be disastrous, so we should always try to get back to the correct partition. >> >> I haven't looked at the possibility of just flagging the partition as invalid - need to be sure any subsequent I/O attempts still go through a path that switches the partition. > > I can see where youre coming from, but similarly a failing mmc_blk_part_switch doesn't imply all subsequent IO will fail. > Flagging the partition as invalid can be seen as rendering the system to a potentially useless state, which is a bit overboard for e.g. one CRC7 failure on the switch. I wasn't clear sorry. I meant setting main_md->part_curr to a value that doesn't match any partition, thereby forcing the next I/O to switch partition first. > Not sure yet what the ideal behavior is, but either way I would go with v1 1/2 and v2 2/2 for now? That already fixes imo the most relevant potential data corruptions. (successful reset -> no switch) > Then we can come up with a a good handling for mmc_blk_reset or even around mmc_blk_part_switch. > Or what do you suggest? What about what I just described above.
On Mon, 10 Oct 2022 at 14:36, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 10/10/22 11:08, Christian Löhle wrote: > > > >> > >> -----Original Message----- > >> From: Adrian Hunter <adrian.hunter@intel.com> > >> Sent: Samstag, 8. Oktober 2022 10:38 > >> To: Christian Löhle <CLoehle@hyperstone.com>; Ulf Hansson <ulf.hansson@linaro.org>; Linux MMC List <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCHv2 1/2] mmc: block: Remove error check of hw_reset on reset > >> > >> On 7/10/22 18:42, Christian Löhle wrote: > >>> 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. > >>> > >>> Fixes: fefdd3c91e0a ("mmc: core: Drop superfluous validations in > >>> mmc_hw|sw_reset()") > >>> Cc: stable@vger.kernel.org > >>> > >>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com> > >>> --- > >>> -v2: Do not attempt to switch partitions if reset failed > >>> > >>> drivers/mmc/core/block.c | 28 +++++++++++++--------------- > >>> 1 file changed, 13 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index > >>> ce89611a136e..8db72cba2bbe 100644 > >>> --- a/drivers/mmc/core/block.c > >>> +++ b/drivers/mmc/core/block.c > >>> @@ -991,29 +991,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; > >>> err = mmc_hw_reset(host->card); > >>> + if (err) > >>> + return err; > >> > >> This could be a potential source of data corruption. > >> > >> There is no guarantee that a subsequent I/O will fail just because the reset failed. Reading / writing the wrong partition would be disastrous, so we should always try to get back to the correct partition. > >> > >> I haven't looked at the possibility of just flagging the partition as invalid - need to be sure any subsequent I/O attempts still go through a path that switches the partition. > > > > I can see where youre coming from, but similarly a failing mmc_blk_part_switch doesn't imply all subsequent IO will fail. > > Flagging the partition as invalid can be seen as rendering the system to a potentially useless state, which is a bit overboard for e.g. one CRC7 failure on the switch. > > I wasn't clear sorry. I meant setting main_md->part_curr to a value > that doesn't match any partition, thereby forcing the next I/O to > switch partition first. If I understand correctly, you are suggesting to ignore the return code from mmc_hw_reset() and then always try to switch to the correct partition. If we end up failing to switch the partition, then we should set an invalid value in main_md->part_curr and return an error code? > > > Not sure yet what the ideal behavior is, but either way I would go with v1 1/2 and v2 2/2 for now? That already fixes imo the most relevant potential data corruptions. (successful reset -> no switch) > > Then we can come up with a a good handling for mmc_blk_reset or even around mmc_blk_part_switch. > > Or what do you suggest? > > What about what I just described above. > It seems reasonable to me (assuming I have understood correctly). One additional thing though. I would appreciate some comments in the code, so it becomes clear of what goes on. Kind regards Uffe
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index ce89611a136e..8db72cba2bbe 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -991,29 +991,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; 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; - } + 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; } - return err; + return 0; } static inline void mmc_blk_reset_success(struct mmc_blk_data *md, int type)
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. Fixes: fefdd3c91e0a ("mmc: core: Drop superfluous validations in mmc_hw|sw_reset()") Cc: stable@vger.kernel.org Signed-off-by: Christian Loehle <cloehle@hyperstone.com> --- -v2: Do not attempt to switch partitions if reset failed drivers/mmc/core/block.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)