[PATCH v4 1/3] mmc: core: Remove some redundant validations in mmc_erase() function

[PATCH v4 1/3] mmc: core: Remove some redundant validations in mmc_erase() function

[Baolin Wang]

Before issuing mmc_erase() function, users always have checked if it can
erase with mmc_can_erase/trim/discard() function, thus remove the redundant
erase checking in mmc_erase() function.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes since v3:
 - Split into 3 separate patches.
 - Add test tag by Shawn.

Changes since v2:
 - Add nr checking and other optimization in mmc_erase() function.

Changes since v1:
 - Add the alignment if card->erase_size is not power of 2.
---
 drivers/mmc/core/core.c |    7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e55cde6..7d7209d 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2217,13 +2217,6 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
 	unsigned int rem, to = from + nr;
 	int err;
 
-	if (!(card->host->caps & MMC_CAP_ERASE) ||
-	    !(card->csd.cmdclass & CCC_ERASE))
-		return -EOPNOTSUPP;
-
-	if (!card->erase_size)
-		return -EOPNOTSUPP;
-
 	if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
 		return -EOPNOTSUPP;
 
-- 
1.7.9.5

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

[Baolin Wang]

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;
-
-	/* '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
-- 
1.7.9.5

Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[Andreas Mohr]

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

Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[Baolin Wang]

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



-- 
Baolin.wang
Best Regards

Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[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

Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[Baolin Wang]

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.

-- 
Baolin.wang
Best Regards

Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[Adrian Hunter]

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
>
>
>

Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size

[Baolin Wang]

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



-- 
Baolin.wang
Best Regards

[PATCH v4 3/3] mmc: core: Optimize the mmc erase size alignment

[Baolin Wang]

In most cases the 'card->erase_size' is power of 2, then the round_up/down()
function is more efficient than '%' operation when the 'card->erase_size' is
power of 2.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/mmc/core/core.c |   34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5f93eef..e014949 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2209,19 +2209,37 @@ static unsigned int mmc_align_erase_size(struct mmc_card *card,
 {
 	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;
+	/*
+	 * When the 'card->erase_size' is power of 2, we can use round_up/down()
+	 * to align the erase size efficiently.
+	 */
+	if (is_power_of_2(card->erase_size)) {
+		unsigned int temp = from_new;
+
+		from_new = round_up(temp, card->erase_size);
+		rem = from_new - temp;
+
 		if (nr_new > rem)
 			nr_new -= rem;
 		else
 			return 0;
-	}
 
-	rem = nr_new % card->erase_size;
-	if (rem)
-		nr_new -= rem;
+		nr_new = round_down(nr_new, card->erase_size);
+	} else {
+		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;
-- 
1.7.9.5

Re: [PATCH v4 1/3] mmc: core: Remove some redundant validations in mmc_erase() function

[Ulf Hansson]

On 6 September 2016 at 04:55, Baolin Wang <baolin.wang@linaro.org> wrote:
> Before issuing mmc_erase() function, users always have checked if it can
> erase with mmc_can_erase/trim/discard() function, thus remove the redundant
> erase checking in mmc_erase() function.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> Changes since v3:
>  - Split into 3 separate patches.
>  - Add test tag by Shawn.
>
> Changes since v2:
>  - Add nr checking and other optimization in mmc_erase() function.
>
> Changes since v1:
>  - Add the alignment if card->erase_size is not power of 2.
> ---
>  drivers/mmc/core/core.c |    7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e55cde6..7d7209d 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2217,13 +2217,6 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
>         unsigned int rem, to = from + nr;
>         int err;
>
> -       if (!(card->host->caps & MMC_CAP_ERASE) ||
> -           !(card->csd.cmdclass & CCC_ERASE))
> -               return -EOPNOTSUPP;
> -
> -       if (!card->erase_size)
> -               return -EOPNOTSUPP;
> -

Could we postpone this until after a clean-up-series of the mmc erase functions?

Until the function remains an exported API, I think it should keep
doing this validations.

>         if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
>                 return -EOPNOTSUPP;
>
> --
> 1.7.9.5
>

Kind regards
Uffe

Re: [PATCH v4 1/3] mmc: core: Remove some redundant validations in mmc_erase() function

[Baolin Wang]

On 6 September 2016 at 20:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 September 2016 at 04:55, Baolin Wang <baolin.wang@linaro.org> wrote:
>> Before issuing mmc_erase() function, users always have checked if it can
>> erase with mmc_can_erase/trim/discard() function, thus remove the redundant
>> erase checking in mmc_erase() function.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>> Changes since v3:
>>  - Split into 3 separate patches.
>>  - Add test tag by Shawn.
>>
>> Changes since v2:
>>  - Add nr checking and other optimization in mmc_erase() function.
>>
>> Changes since v1:
>>  - Add the alignment if card->erase_size is not power of 2.
>> ---
>>  drivers/mmc/core/core.c |    7 -------
>>  1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index e55cde6..7d7209d 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2217,13 +2217,6 @@ int mmc_erase(struct mmc_card *card, unsigned int from, unsigned int nr,
>>         unsigned int rem, to = from + nr;
>>         int err;
>>
>> -       if (!(card->host->caps & MMC_CAP_ERASE) ||
>> -           !(card->csd.cmdclass & CCC_ERASE))
>> -               return -EOPNOTSUPP;
>> -
>> -       if (!card->erase_size)
>> -               return -EOPNOTSUPP;
>> -
>
> Could we postpone this until after a clean-up-series of the mmc erase functions?
>
> Until the function remains an exported API, I think it should keep
> doing this validations.

OK. That's reasonable.

>
>>         if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
>>                 return -EOPNOTSUPP;
>>
>> --
>> 1.7.9.5
>>
>
> Kind regards
> Uffe



-- 
Baolin.wang
Best Regards