| Message ID | 48c332c9c688796b7ea157046b8feb68a11e60d4.1473130123.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 06, 2016 at 10:55:11AM +0800, Baolin Wang wrote: > In order to clean up the mmc_erase() function and do some optimization > for erase size alignment, factor out the guts of erase size alignment > into mmc_align_erase_size() function. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > Tested-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > drivers/mmc/core/core.c | 60 +++++++++++++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 23 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 7d7209d..5f93eef 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2202,6 +2202,37 @@ out: > return err; > } > > +static unsigned int mmc_align_erase_size(struct mmc_card *card, > + unsigned int *from, > + unsigned int *to, > + unsigned int nr) > +{ > + unsigned int from_new = *from, nr_new = nr, rem; > + > + rem = from_new % card->erase_size; > + if (rem) { > + rem = card->erase_size - rem; > + from_new += rem; > + if (nr_new > rem) > + nr_new -= rem; > + else > + return 0; > + } > + > + rem = nr_new % card->erase_size; > + if (rem) > + nr_new -= rem; > + > + if (nr_new == 0) > + return 0; > + > + /* 'from' and 'to' are inclusive */ > + *to = from_new + nr_new - 1; > + *from = from_new; > + > + return nr_new; > +} > + > /** > * mmc_erase - erase sectors. > * @card: card to erase > @@ -2234,31 +2265,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, > } > > if (arg == MMC_ERASE_ARG) { > - rem = from % card->erase_size; > - if (rem) { > - rem = card->erase_size - rem; > - from += rem; > - if (nr > rem) > - nr -= rem; > - else > - return 0; > - } > - rem = nr % card->erase_size; > - if (rem) > - nr -= rem; > + nr = mmc_align_erase_size(card, &from, &to, nr); > + if (nr == 0) > + return 0; > + } else { > + /* 'from' and 'to' are inclusive */ > + to -= 1; > } > > - if (nr == 0) > - return 0; > - > - to = from + nr; > - > - if (to <= from) > - return -EINVAL; Hmm, this is swallowing -EINVAL behaviour i.e., now possibly violating protocol? (this may easily be ok - haven't done an extensive review - but since the commit has that characteristic change, the commit message should contain that detail) Thanks for the cleanup work & HTH, Andreas Mohr -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andreas, On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> wrote: > On Tue, Sep 06, 2016 at 10:55:11AM +0800, Baolin Wang wrote: >> In order to clean up the mmc_erase() function and do some optimization >> for erase size alignment, factor out the guts of erase size alignment >> into mmc_align_erase_size() function. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> Tested-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> drivers/mmc/core/core.c | 60 +++++++++++++++++++++++++++++------------------ >> 1 file changed, 37 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 7d7209d..5f93eef 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -2202,6 +2202,37 @@ out: >> return err; >> } >> >> +static unsigned int mmc_align_erase_size(struct mmc_card *card, >> + unsigned int *from, >> + unsigned int *to, >> + unsigned int nr) >> +{ >> + unsigned int from_new = *from, nr_new = nr, rem; >> + >> + rem = from_new % card->erase_size; >> + if (rem) { >> + rem = card->erase_size - rem; >> + from_new += rem; >> + if (nr_new > rem) >> + nr_new -= rem; >> + else >> + return 0; >> + } >> + >> + rem = nr_new % card->erase_size; >> + if (rem) >> + nr_new -= rem; >> + >> + if (nr_new == 0) >> + return 0; >> + >> + /* 'from' and 'to' are inclusive */ >> + *to = from_new + nr_new - 1; >> + *from = from_new; >> + >> + return nr_new; >> +} >> + >> /** >> * mmc_erase - erase sectors. >> * @card: card to erase >> @@ -2234,31 +2265,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, >> } >> >> if (arg == MMC_ERASE_ARG) { >> - rem = from % card->erase_size; >> - if (rem) { >> - rem = card->erase_size - rem; >> - from += rem; >> - if (nr > rem) >> - nr -= rem; >> - else >> - return 0; >> - } >> - rem = nr % card->erase_size; >> - if (rem) >> - nr -= rem; >> + nr = mmc_align_erase_size(card, &from, &to, nr); >> + if (nr == 0) >> + return 0; >> + } else { >> + /* 'from' and 'to' are inclusive */ >> + to -= 1; >> } >> >> - if (nr == 0) >> - return 0; >> - >> - to = from + nr; >> - >> - if (to <= from) >> - return -EINVAL; > > Hmm, this is swallowing -EINVAL behaviour > i.e., now possibly violating protocol? I didn't see what situation will make variable 'to' is less than 'from' since I think variable 'nr' is always larger than 0, right? If so, we should remove this useless checking. Thanks. > > (this may easily be ok - haven't done an extensive review - > but since the commit has that characteristic change, > the commit message should contain that detail) > > Thanks for the cleanup work & HTH, > > Andreas Mohr
On Tue, Sep 06, 2016 at 02:26:06PM +0800, Baolin Wang wrote: > On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> wrote: > >> - to = from + nr; > >> - > >> - if (to <= from) > >> - return -EINVAL; > > > > Hmm, this is swallowing -EINVAL behaviour > > i.e., now possibly violating protocol? > > I didn't see what situation will make variable 'to' is less than > 'from' since I think variable 'nr' is always larger than 0, right? If > so, we should remove this useless checking. Thanks. Hmm, indeed, since all participating variables are unsigned, the existing calculation should never hit this. However, one could argue that this is an additional safeguard against implementation source getting modified in a way that will suddenly result in this pathologic case becoming true (where a -EINVAL bailout surely will then pinpoint things much more visibly for some users, as opposed to potential data corruption or some such). I have seen another change > - if (nr == 0) > - return 0; where it gets moved out of common path and into MMC_ERASE_ARG-specific branch (likely because the subsequent common-path conditional of nr > rem is deemed sufficient). This seems to be again a change where a simple yet crucial device geometry calculation post-condition (either to > from, or nr > 0) is then not verified specifically/separately. Ultimately, I am not sure whether or not these (post-)conditions should be verified in their most basic, simple form, as an extra/separate verification step. HTH, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/09/2016 9:26 a.m., Baolin Wang wrote: > Hi Andreas, > > On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> wrote: >> On Tue, Sep 06, 2016 at 10:55:11AM +0800, Baolin Wang wrote: >>> In order to clean up the mmc_erase() function and do some optimization >>> for erase size alignment, factor out the guts of erase size alignment >>> into mmc_align_erase_size() function. >>> >>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>> Tested-by: Shawn Lin <shawn.lin@rock-chips.com> >>> --- >>> drivers/mmc/core/core.c | 60 +++++++++++++++++++++++++++++------------------ >>> 1 file changed, 37 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>> index 7d7209d..5f93eef 100644 >>> --- a/drivers/mmc/core/core.c >>> +++ b/drivers/mmc/core/core.c >>> @@ -2202,6 +2202,37 @@ out: >>> return err; >>> } >>> >>> +static unsigned int mmc_align_erase_size(struct mmc_card *card, >>> + unsigned int *from, >>> + unsigned int *to, >>> + unsigned int nr) >>> +{ >>> + unsigned int from_new = *from, nr_new = nr, rem; >>> + >>> + rem = from_new % card->erase_size; >>> + if (rem) { >>> + rem = card->erase_size - rem; >>> + from_new += rem; >>> + if (nr_new > rem) >>> + nr_new -= rem; >>> + else >>> + return 0; >>> + } >>> + >>> + rem = nr_new % card->erase_size; >>> + if (rem) >>> + nr_new -= rem; >>> + >>> + if (nr_new == 0) >>> + return 0; >>> + >>> + /* 'from' and 'to' are inclusive */ >>> + *to = from_new + nr_new - 1; >>> + *from = from_new; >>> + >>> + return nr_new; >>> +} >>> + >>> /** >>> * mmc_erase - erase sectors. >>> * @card: card to erase >>> @@ -2234,31 +2265,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, >>> } >>> >>> if (arg == MMC_ERASE_ARG) { >>> - rem = from % card->erase_size; >>> - if (rem) { >>> - rem = card->erase_size - rem; >>> - from += rem; >>> - if (nr > rem) >>> - nr -= rem; >>> - else >>> - return 0; >>> - } >>> - rem = nr % card->erase_size; >>> - if (rem) >>> - nr -= rem; >>> + nr = mmc_align_erase_size(card, &from, &to, nr); >>> + if (nr == 0) >>> + return 0; >>> + } else { >>> + /* 'from' and 'to' are inclusive */ >>> + to -= 1; >>> } >>> >>> - if (nr == 0) >>> - return 0; >>> - >>> - to = from + nr; >>> - >>> - if (to <= from) >>> - return -EINVAL; >> >> Hmm, this is swallowing -EINVAL behaviour >> i.e., now possibly violating protocol? > > I didn't see what situation will make variable 'to' is less than > 'from' since I think variable 'nr' is always larger than 0, right? If > so, we should remove this useless checking. Thanks. It is checking overflows. > >> >> (this may easily be ok - haven't done an extensive review - >> but since the commit has that characteristic change, >> the commit message should contain that detail) >> >> Thanks for the cleanup work & HTH, >> >> Andreas Mohr > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 September 2016 at 15:19, Andreas Mohr <andi@lisas.de> wrote: > On Tue, Sep 06, 2016 at 02:26:06PM +0800, Baolin Wang wrote: >> On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> wrote: >> >> - to = from + nr; >> >> - >> >> - if (to <= from) >> >> - return -EINVAL; >> > >> > Hmm, this is swallowing -EINVAL behaviour >> > i.e., now possibly violating protocol? >> >> I didn't see what situation will make variable 'to' is less than >> 'from' since I think variable 'nr' is always larger than 0, right? If >> so, we should remove this useless checking. Thanks. > > Hmm, indeed, since all participating variables are unsigned, > the existing calculation should never hit this. > However, one could argue that this is an additional safeguard > against implementation source getting modified in a way > that will suddenly result in this pathologic case becoming true > (where a -EINVAL bailout surely will then pinpoint things > much more visibly for some users, > as opposed to potential data corruption or some such). OK. I'll add this checking. > > > > I have seen another change > >> - if (nr == 0) >> - return 0; > > where it gets moved out of common path > and into MMC_ERASE_ARG-specific branch > (likely because the subsequent common-path conditional of > nr > rem > is deemed sufficient). > > This seems to be again a change > where a simple yet crucial > device geometry calculation post-condition > (either to > from, or nr > 0) > is then not verified specifically/separately. > > Ultimately, I am not sure whether or not > these (post-)conditions should be verified > in their most basic, simple form, > as an extra/separate verification step. After some investigation, I think we add this checking is more safer. Thanks for your comments.
On 6 September 2016 at 15:52, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 6/09/2016 9:26 a.m., Baolin Wang wrote: >> >> Hi Andreas, >> >> On 6 September 2016 at 12:34, Andreas Mohr <andi@lisas.de> wrote: >>> >>> On Tue, Sep 06, 2016 at 10:55:11AM +0800, Baolin Wang wrote: >>>> >>>> In order to clean up the mmc_erase() function and do some optimization >>>> for erase size alignment, factor out the guts of erase size alignment >>>> into mmc_align_erase_size() function. >>>> >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>>> Tested-by: Shawn Lin <shawn.lin@rock-chips.com> >>>> --- >>>> drivers/mmc/core/core.c | 60 >>>> +++++++++++++++++++++++++++++------------------ >>>> 1 file changed, 37 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>>> index 7d7209d..5f93eef 100644 >>>> --- a/drivers/mmc/core/core.c >>>> +++ b/drivers/mmc/core/core.c >>>> @@ -2202,6 +2202,37 @@ out: >>>> return err; >>>> } >>>> >>>> +static unsigned int mmc_align_erase_size(struct mmc_card *card, >>>> + unsigned int *from, >>>> + unsigned int *to, >>>> + unsigned int nr) >>>> +{ >>>> + unsigned int from_new = *from, nr_new = nr, rem; >>>> + >>>> + rem = from_new % card->erase_size; >>>> + if (rem) { >>>> + rem = card->erase_size - rem; >>>> + from_new += rem; >>>> + if (nr_new > rem) >>>> + nr_new -= rem; >>>> + else >>>> + return 0; >>>> + } >>>> + >>>> + rem = nr_new % card->erase_size; >>>> + if (rem) >>>> + nr_new -= rem; >>>> + >>>> + if (nr_new == 0) >>>> + return 0; >>>> + >>>> + /* 'from' and 'to' are inclusive */ >>>> + *to = from_new + nr_new - 1; >>>> + *from = from_new; >>>> + >>>> + return nr_new; >>>> +} >>>> + >>>> /** >>>> * mmc_erase - erase sectors. >>>> * @card: card to erase >>>> @@ -2234,31 +2265,14 @@ int mmc_erase(struct mmc_card *card, unsigned >>>> int from, unsigned int nr, >>>> } >>>> >>>> if (arg == MMC_ERASE_ARG) { >>>> - rem = from % card->erase_size; >>>> - if (rem) { >>>> - rem = card->erase_size - rem; >>>> - from += rem; >>>> - if (nr > rem) >>>> - nr -= rem; >>>> - else >>>> - return 0; >>>> - } >>>> - rem = nr % card->erase_size; >>>> - if (rem) >>>> - nr -= rem; >>>> + nr = mmc_align_erase_size(card, &from, &to, nr); >>>> + if (nr == 0) >>>> + return 0; >>>> + } else { >>>> + /* 'from' and 'to' are inclusive */ >>>> + to -= 1; >>>> } >>>> >>>> - if (nr == 0) >>>> - return 0; >>>> - >>>> - to = from + nr; >>>> - >>>> - if (to <= from) >>>> - return -EINVAL; >>> >>> >>> Hmm, this is swallowing -EINVAL behaviour >>> i.e., now possibly violating protocol? >> >> >> I didn't see what situation will make variable 'to' is less than >> 'from' since I think variable 'nr' is always larger than 0, right? If >> so, we should remove this useless checking. Thanks. > > > It is checking overflows. Yes, you are right, my mistake. I will add this checking in next version. > >> >>> >>> (this may easily be ok - haven't done an extensive review - >>> but since the commit has that characteristic change, >>> the commit message should contain that detail) >>> >>> Thanks for the cleanup work & HTH, >>> >>> Andreas Mohr >> >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 7d7209d..5f93eef 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2202,6 +2202,37 @@ out: return err; } +static unsigned int mmc_align_erase_size(struct mmc_card *card, + unsigned int *from, + unsigned int *to, + unsigned int nr) +{ + unsigned int from_new = *from, nr_new = nr, rem; + + rem = from_new % card->erase_size; + if (rem) { + rem = card->erase_size - rem; + from_new += rem; + if (nr_new > rem) + nr_new -= rem; + else + return 0; + } + + rem = nr_new % card->erase_size; + if (rem) + nr_new -= rem; + + if (nr_new == 0) + return 0; + + /* 'from' and 'to' are inclusive */ + *to = from_new + nr_new - 1; + *from = from_new; + + return nr_new; +} + /** * mmc_erase - erase sectors. * @card: card to erase @@ -2234,31 +2265,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr, } if (arg == MMC_ERASE_ARG) { - rem = from % card->erase_size; - if (rem) { - rem = card->erase_size - rem; - from += rem; - if (nr > rem) - nr -= rem; - else - return 0; - } - rem = nr % card->erase_size; - if (rem) - nr -= rem; + nr = mmc_align_erase_size(card, &from, &to, nr); + if (nr == 0) + return 0; + } else { + /* 'from' and 'to' are inclusive */ + to -= 1; } - if (nr == 0) - return 0; - - to = from + nr; - - if (to <= from) - return -EINVAL; - - /* 'from' and 'to' are inclusive */ - to -= 1; - /* * Special case where only one erase-group fits in the timeout budget: * If the region crosses an erase-group boundary on this particular