[v4,2/3] mmc: core: Factor out the alignment of erase size
diff mbox

Message ID 48c332c9c688796b7ea157046b8feb68a11e60d4.1473130123.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

(Exiting) Baolin Wang Sept. 6, 2016, 2:55 a.m. UTC
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(-)

Comments

Andreas Mohr Sept. 6, 2016, 4:34 a.m. UTC | #1
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
(Exiting) Baolin Wang Sept. 6, 2016, 6:26 a.m. UTC | #2
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
Andreas Mohr Sept. 6, 2016, 7:19 a.m. UTC | #3
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
Adrian Hunter Sept. 6, 2016, 7:52 a.m. UTC | #4
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
(Exiting) Baolin Wang Sept. 6, 2016, 8:25 a.m. UTC | #5
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.
(Exiting) Baolin Wang Sept. 6, 2016, 8:27 a.m. UTC | #6
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

Patch
diff mbox

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