Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1477034 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 05:00 +0200
    Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Andreas Mohr <andi@lisas.de> - 2016-09-06 06:40 +0200
      Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 08:30 +0200
        Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Andreas Mohr <andi@lisas.de> - 2016-09-06 09:20 +0200
          Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 10:30 +0200
        Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Adrian Hunter <adrian.hunter@intel.com> - 2016-09-06 10:00 +0200
          Re: [PATCH v4 2/3] mmc: core: Factor out the alignment of erase size Baolin Wang <baolin.wang@linaro.org> - 2016-09-06 10:30 +0200

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

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

[toc] | [next] | [standalone]

#1477066

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

[toc] | [prev] | [next] | [standalone]

#1477097

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

[toc] | [prev] | [next] | [standalone]

#1477111

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

[toc] | [prev] | [next] | [standalone]

#1477156

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

[toc] | [prev] | [next] | [standalone]

#1477139

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

[toc] | [prev] | [next] | [standalone]

#1477153

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web