From: Marek Vasut <marex@denx.de>
To: Austin Boyle <boyle.austin@gmail.com>
Cc: Angus Clark <angus.clark@st.com>,
Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
Gerlando Falauto <gerlando.falauto@keymile.com>,
Brian Norris <computersforpeace@gmail.com>,
David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH v2] mtd: m25p80: Calculate flash block protect bits based on number of sectors
Date: Sun, 13 Apr 2014 19:24:03 +0200 [thread overview]
Message-ID: <201404131924.03967.marex@denx.de> (raw)
In-Reply-To: <alpine.DEB.2.02.1404062115570.19515@austin-Satellite-L510>
On Sunday, April 06, 2014 at 01:19:59 PM, Austin Boyle wrote:
[...]
> @@ -752,24 +773,18 @@ static int m25p80_lock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len)
>
> status_old = read_sr(flash);
>
> - if (offset < flash->mtd.size-(flash->mtd.size/2))
> - status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/4))
> - status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> - else if (offset < flash->mtd.size-(flash->mtd.size/8))
> - status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/16))
> - status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> - else if (offset < flash->mtd.size-(flash->mtd.size/32))
> - status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/64))
> - status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> - else
> - status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> + for (lock_bits = 1; lock_bits < 7; lock_bits++) {
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
> + if (offset >= protected_area_start)
> + break;
> + }
> +
> + status_new = (status_old & ~SR_BP_BIT_MASK) |
> + ((lock_bits << SR_BP_BIT_OFFSET) & SR_BP_BIT_MASK);
This portion of code looks like a duplicate of ...
> /* Only modify protection if it will not unlock other areas */
> - if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) >
> - (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> + if ((status_new & SR_BP_BIT_MASK) > (status_old & SR_BP_BIT_MASK)) {
> write_enable(flash);
> if (write_sr(flash, status_new) < 0) {
> res = 1;
> @@ -786,6 +801,8 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len) struct m25p *flash = mtd_to_m25p(mtd);
> uint32_t offset = ofs;
> uint8_t status_old, status_new;
> + uint8_t lock_bits;
> + uint32_t protected_area_start;
> int res = 0;
>
> mutex_lock(&flash->lock);
> @@ -797,24 +814,19 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len)
>
> status_old = read_sr(flash);
>
> - if (offset+len > flash->mtd.size-(flash->mtd.size/64))
> - status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0);
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/32))
> - status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/16))
> - status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/8))
> - status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/4))
> - status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/2))
> - status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> - else
> - status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> + for (lock_bits = 1; lock_bits < 7; lock_bits++) {
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
> + if (offset+len >= protected_area_start)
> + break;
> + }
> + lock_bits--;
> +
> + status_new = (status_old & ~SR_BP_BIT_MASK) |
> + ((lock_bits << SR_BP_BIT_OFFSET) & SR_BP_BIT_MASK);
... this here (if you don't consider the lock_bits--) . Why don't we move this
into a separate function to avoid the duplication?
> /* Only modify protection if it will not lock other areas */
> - if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) <
> - (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> + if ((status_new & SR_BP_BIT_MASK) < (status_old & SR_BP_BIT_MASK)) {
> write_enable(flash);
> if (write_sr(flash, status_new) < 0) {
> res = 1;
> @@ -826,6 +838,39 @@ err: mutex_unlock(&flash->lock);
> return res;
> }
>
> +static int m25p80_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t
> len) +{
> + struct m25p *flash = mtd_to_m25p(mtd);
> + uint32_t offset = ofs;
> + uint8_t status;
> + uint8_t lock_bits;
> + uint32_t protected_area_start;
> + int res;
> +
> + mutex_lock(&flash->lock);
> + /* Wait until finished previous command */
> + if (wait_till_ready(flash)) {
> + mutex_unlock(&flash->lock);
> + return -EBUSY;
> + }
> + status = read_sr(flash);
> + mutex_unlock(&flash->lock);
> +
> + lock_bits = ((status & SR_BP_BIT_MASK) >> SR_BP_BIT_OFFSET);
Excessive () , the outer ones are not needed.
> +
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
You might want to introduce some "get_protected_area_size()" here, since this is
the third time I see this call.
Otherwise nice, thanks for cleaning this mess up, it really helps !
WARNING: multiple messages have this Message-ID (diff)
From: Marek Vasut <marex@denx.de>
To: Austin Boyle <boyle.austin@gmail.com>
Cc: Gerlando Falauto <gerlando.falauto@keymile.com>,
Brian Norris <computersforpeace@gmail.com>,
linux-mtd@lists.infradead.org,
David Woodhouse <dwmw2@infradead.org>,
linux-kernel@vger.kernel.org,
Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
Angus Clark <angus.clark@st.com>
Subject: Re: [PATCH v2] mtd: m25p80: Calculate flash block protect bits based on number of sectors
Date: Sun, 13 Apr 2014 19:24:03 +0200 [thread overview]
Message-ID: <201404131924.03967.marex@denx.de> (raw)
In-Reply-To: <alpine.DEB.2.02.1404062115570.19515@austin-Satellite-L510>
On Sunday, April 06, 2014 at 01:19:59 PM, Austin Boyle wrote:
[...]
> @@ -752,24 +773,18 @@ static int m25p80_lock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len)
>
> status_old = read_sr(flash);
>
> - if (offset < flash->mtd.size-(flash->mtd.size/2))
> - status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/4))
> - status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> - else if (offset < flash->mtd.size-(flash->mtd.size/8))
> - status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/16))
> - status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> - else if (offset < flash->mtd.size-(flash->mtd.size/32))
> - status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/64))
> - status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> - else
> - status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> + for (lock_bits = 1; lock_bits < 7; lock_bits++) {
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
> + if (offset >= protected_area_start)
> + break;
> + }
> +
> + status_new = (status_old & ~SR_BP_BIT_MASK) |
> + ((lock_bits << SR_BP_BIT_OFFSET) & SR_BP_BIT_MASK);
This portion of code looks like a duplicate of ...
> /* Only modify protection if it will not unlock other areas */
> - if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) >
> - (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> + if ((status_new & SR_BP_BIT_MASK) > (status_old & SR_BP_BIT_MASK)) {
> write_enable(flash);
> if (write_sr(flash, status_new) < 0) {
> res = 1;
> @@ -786,6 +801,8 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len) struct m25p *flash = mtd_to_m25p(mtd);
> uint32_t offset = ofs;
> uint8_t status_old, status_new;
> + uint8_t lock_bits;
> + uint32_t protected_area_start;
> int res = 0;
>
> mutex_lock(&flash->lock);
> @@ -797,24 +814,19 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len)
>
> status_old = read_sr(flash);
>
> - if (offset+len > flash->mtd.size-(flash->mtd.size/64))
> - status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0);
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/32))
> - status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/16))
> - status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/8))
> - status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/4))
> - status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/2))
> - status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> - else
> - status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> + for (lock_bits = 1; lock_bits < 7; lock_bits++) {
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
> + if (offset+len >= protected_area_start)
> + break;
> + }
> + lock_bits--;
> +
> + status_new = (status_old & ~SR_BP_BIT_MASK) |
> + ((lock_bits << SR_BP_BIT_OFFSET) & SR_BP_BIT_MASK);
... this here (if you don't consider the lock_bits--) . Why don't we move this
into a separate function to avoid the duplication?
> /* Only modify protection if it will not lock other areas */
> - if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) <
> - (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> + if ((status_new & SR_BP_BIT_MASK) < (status_old & SR_BP_BIT_MASK)) {
> write_enable(flash);
> if (write_sr(flash, status_new) < 0) {
> res = 1;
> @@ -826,6 +838,39 @@ err: mutex_unlock(&flash->lock);
> return res;
> }
>
> +static int m25p80_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t
> len) +{
> + struct m25p *flash = mtd_to_m25p(mtd);
> + uint32_t offset = ofs;
> + uint8_t status;
> + uint8_t lock_bits;
> + uint32_t protected_area_start;
> + int res;
> +
> + mutex_lock(&flash->lock);
> + /* Wait until finished previous command */
> + if (wait_till_ready(flash)) {
> + mutex_unlock(&flash->lock);
> + return -EBUSY;
> + }
> + status = read_sr(flash);
> + mutex_unlock(&flash->lock);
> +
> + lock_bits = ((status & SR_BP_BIT_MASK) >> SR_BP_BIT_OFFSET);
Excessive () , the outer ones are not needed.
> +
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
You might want to introduce some "get_protected_area_size()" here, since this is
the third time I see this call.
Otherwise nice, thanks for cleaning this mess up, it really helps !
next prev parent reply other threads:[~2014-04-13 22:02 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-06 11:19 [PATCH v2] mtd: m25p80: Calculate flash block protect bits based on number of sectors Austin Boyle
2014-04-06 11:19 ` Austin Boyle
2014-04-13 17:24 ` Marek Vasut [this message]
2014-04-13 17:24 ` Marek Vasut
2014-04-16 12:26 ` Austin Boyle
2014-04-16 12:30 ` Marek Vasut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201404131924.03967.marex@denx.de \
--to=marex@denx.de \
--cc=angus.clark@st.com \
--cc=artem.bityutskiy@linux.intel.com \
--cc=boyle.austin@gmail.com \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=gerlando.falauto@keymile.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.