Re: [PATCH] jffs2: Don't add summary entry when MTD write fails
Hello David:
> > + int ret;
> > +
> > + ret = mtd_writev(c->mtd, vecs, count, to, retlen);
> > +
> > if (!jffs2_is_writebuffered(c)) {
> > if (jffs2_sum_active()) {
> > int res;
> > +
> > + if (ret ||
> > + *retlen != iov_length((struct iovec *)
> vecs, count))
> > + return ret;
> > +
> > res = jffs2_sum_add_kvec(c, vecs, count,
> (uint32_t) to);
> > if (res) {
> > return res;
>
> OK... but perhaps we can dispense with the separate 'ret' and 'res'
> variables and the rats nest of conditions, and do something like:
>
>int ret;
>
>ret = mtd_writev(…);
>
>if (!ret && *retlen == iov_length(…) &&
>!jffs2_is_writebuffered(c) && jffs2_sum_active())
> ret = jffs2_sum_add_kvec(…);
>
>return ret;
While the logic is the same, will the compiler generate the same code?
When CONFIG_JFFS2_SUMMARY is not set, "if (jffs2_sum_active())" means "if
(0)", and I would assume that the compiler removes the whole clause, "if"
and all. However, I am not sure what happens with "if (!ret && whatever &&
0)".
That's why I was taking pains to keep the original control flow intact,
even if it's a rat's nest (it is). If I remember correctly,
jffs2_flash_direct_writev() is called quite often, and I didn't want
performance to suffer. I may be completely wrong here, of course, but then
why wasn't the original source code "if (!jffs2_is_writebuffered(c) &&
jffs2_sum_active())"?
Best regards,
Thomas
Re: [PATCH] jffs2: Don't add summary entry when MTD write fails
Hello David:
> > + int ret;
> > +
> > + ret = mtd_writev(c->mtd, vecs, count, to, retlen);
> > +
> > if (!jffs2_is_writebuffered(c)) {
> > if (jffs2_sum_active()) {
> > int res;
> > +
> > + if (ret ||
> > + *retlen != iov_length((struct iovec *)
> vecs, count))
> > + return ret;
> > +
> > res = jffs2_sum_add_kvec(c, vecs, count,
> (uint32_t) to);
> > if (res) {
> > return res;
>
> OK... but perhaps we can dispense with the separate 'ret' and 'res'
> variables and the rats nest of conditions, and do something like:
>
>int ret;
>
>ret = mtd_writev(…);
>
>if (!ret && *retlen == iov_length(…) &&
>!jffs2_is_writebuffered(c) && jffs2_sum_active())
> ret = jffs2_sum_add_kvec(…);
>
>return ret;
While the logic is the same, will the compiler generate the same code?
When CONFIG_JFFS2_SUMMARY is not set, "if (jffs2_sum_active())" means "if
(0)", and I would assume that the compiler removes the whole clause, "if"
and all. However, I am not sure what happens with "if (!ret && whatever &&
0)".
That's why I was taking pains to keep the original control flow intact,
even if it's a rat's nest (it is). If I remember correctly,
jffs2_flash_direct_writev() is called quite often, and I didn't want
performance to suffer. I may be completely wrong here, of course, but then
why wasn't the original source code "if (!jffs2_is_writebuffered(c) &&
jffs2_sum_active())"?
Best regards,
Thomas
Re: JFFS2 deadlock
Hello Joakim: > Can we get this upstream before next release? I don't think there > will much more > testing at this point. I would second this. Actually, I did some additional stress testing, but didn't see any problems. Best regards, Thomas
Re: JFFS2 deadlock
Hello Joakim: > Can we get this upstream before next release? I don't think there > will much more > testing at this point. I would second this. Actually, I did some additional stress testing, but didn't see any problems. Best regards, Thomas
Re: JFFS2 deadlock
Hello David: > > Please could you try what's in the tree at > > http://git.infradead.org/users/dwmw2/jffs2-fixes.git > Your patch looks much simpler, and I will definitely test it. It may > take a few days, though, as I have to unearth the test scripts, and > find a time slot for testing. Here is what I did (sorry for the wait, things were piling up): 1) Removed Deng Chao's patch from my kernel, added your patch "jffs2: Fix page lock / f->sem deadlock". I am still on linux-3.14, but jffs2 hasn't changed much since then, so this shouldn't make a difference. Added a printk() before mutex_unlock(>sem) to check if the prospective page was locked, i.e. if the deadlock situation actually occurs. 2) On my target system, started wangzaiwei's test (with some fixes), plus a loop copying a large file over and over (to get GC rolling, which increases the chance of a deadlock). 3) After 24 hours, the system was still alive, and the printk() had been hit 32 times. So yes, I am confident that your patch avoids the deadlock, and if that's good enough for you, please add my Tested-by:. However, I am going to run some more stress tests here just to check that there weren't any unexpected side effects. (Don't get me wrong -- I am sure the patch is fine, but for me it's a case of "once bitten, twice shy" ...) Best regards, Thomas Betker
Re: JFFS2 deadlock
Hello David: > > Please could you try what's in the tree at > > http://git.infradead.org/users/dwmw2/jffs2-fixes.git > Your patch looks much simpler, and I will definitely test it. It may > take a few days, though, as I have to unearth the test scripts, and > find a time slot for testing. Here is what I did (sorry for the wait, things were piling up): 1) Removed Deng Chao's patch from my kernel, added your patch "jffs2: Fix page lock / f->sem deadlock". I am still on linux-3.14, but jffs2 hasn't changed much since then, so this shouldn't make a difference. Added a printk() before mutex_unlock(>sem) to check if the prospective page was locked, i.e. if the deadlock situation actually occurs. 2) On my target system, started wangzaiwei's test (with some fixes), plus a loop copying a large file over and over (to get GC rolling, which increases the chance of a deadlock). 3) After 24 hours, the system was still alive, and the printk() had been hit 32 times. So yes, I am confident that your patch avoids the deadlock, and if that's good enough for you, please add my Tested-by:. However, I am going to run some more stress tests here just to check that there weren't any unexpected side effects. (Don't get me wrong -- I am sure the patch is fine, but for me it's a case of "once bitten, twice shy" ...) Best regards, Thomas Betker
Re: JFFS2 deadlock
Hello David: > > Subject: [PATCH] Revert "jffs2: Fix lock acquisition order bug in > > jffs2_write_begin" > > http://article.gmane.org/gmane.linux.drivers.mtd/62951 > > > > This is a patch revising my original patch, which I sent to linux-mtd on > > 10-Nov-2015. I didn't see a response yet, but it's one of the outstanding > > patches above. > > That looks necessary but not sufficient. I think we need this > (untested) patch on top of it, to ensure that we *always* take the page > lock before f->sem? > > Please could you try what's in the tree at > http://git.infradead.org/users/dwmw2/jffs2-fixes.git I have been using a variant of Deng Chao's patch here for a long time, so that one has been tested quite a bit: http://lists.infradead.org/pipermail/linux-mtd/2013-August/048352.html. The problem with that patch was that it modified mm/filemap.c and include/linux/pagemap.h, which we were not too happy about. Your patch looks much simpler, and I will definitely test it. It may take a few days, though, as I have to unearth the test scripts, and find a time slot for testing. Best regards, Thomas Betker
Re: JFFS2 deadlock
Hello David: > > Subject: [PATCH] Revert "jffs2: Fix lock acquisition order bug in > > jffs2_write_begin" > > http://article.gmane.org/gmane.linux.drivers.mtd/62951 > > > > This is a patch revising my original patch, which I sent to linux-mtd on > > 10-Nov-2015. I didn't see a response yet, but it's one of the outstanding > > patches above. > > That looks necessary but not sufficient. I think we need this > (untested) patch on top of it, to ensure that we *always* take the page > lock before f->sem? > > Please could you try what's in the tree at > http://git.infradead.org/users/dwmw2/jffs2-fixes.git I have been using a variant of Deng Chao's patch here for a long time, so that one has been tested quite a bit: http://lists.infradead.org/pipermail/linux-mtd/2013-August/048352.html. The problem with that patch was that it modified mm/filemap.c and include/linux/pagemap.h, which we were not too happy about. Your patch looks much simpler, and I will definitely test it. It may take a few days, though, as I have to unearth the test scripts, and find a time slot for testing. Best regards, Thomas Betker
Re: JFFS2 deadlock
Hello Brian: > No, I'm pretty sure this is not the first report. I think there have > even been patches. The problem is that JFFS2 is effectively > unmaintained, despite what MAINTAINERS has to say about it. > > Previous reports: > > Subject: Another JFFS2 deadlock, kernel 3.4.11 > http://thread.gmane.org/gmane.linux.drivers.mtd/62523 > > Subject: [JFFS2] Revision "jffs2: Fix lock acquisition order bug in > jffs2_write_begin" introduces another dead lock. > http://thread.gmane.org/gmane.linux.drivers.mtd/47986 > For reference: outstanding patches, waiting for a maintainer (I've been > keeping patchwork up-to-date, mostly, but I'm not touching JFFS2 myself, > for the most part): > http://patchwork.ozlabs.org/project/linux-mtd/list/?q=jffs2 Subject: [PATCH] Revert "jffs2: Fix lock acquisition order bug in jffs2_write_begin" http://article.gmane.org/gmane.linux.drivers.mtd/62951 This is a patch revising my original patch, which I sent to linux-mtd on 10-Nov-2015. I didn't see a response yet, but it's one of the outstanding patches above. Best regards, Thomas
Re: JFFS2 deadlock
Hello Brian: > No, I'm pretty sure this is not the first report. I think there have > even been patches. The problem is that JFFS2 is effectively > unmaintained, despite what MAINTAINERS has to say about it. > > Previous reports: > > Subject: Another JFFS2 deadlock, kernel 3.4.11 > http://thread.gmane.org/gmane.linux.drivers.mtd/62523 > > Subject: [JFFS2] Revision "jffs2: Fix lock acquisition order bug in > jffs2_write_begin" introduces another dead lock. > http://thread.gmane.org/gmane.linux.drivers.mtd/47986 > For reference: outstanding patches, waiting for a maintainer (I've been > keeping patchwork up-to-date, mostly, but I'm not touching JFFS2 myself, > for the most part): > http://patchwork.ozlabs.org/project/linux-mtd/list/?q=jffs2 Subject: [PATCH] Revert "jffs2: Fix lock acquisition order bug in jffs2_write_begin" http://article.gmane.org/gmane.linux.drivers.mtd/62951 This is a patch revising my original patch, which I sent to linux-mtd on 10-Nov-2015. I didn't see a response yet, but it's one of the outstanding patches above. Best regards, Thomas
[PATCH] jffs2: Don't add summary entry when MTD write fails
From: Thomas Betker
jffs2_flash_direct_writev() always invokes jffs2_sum_add_kvec(), even
if mtd_writev() fails. Usually, this results in an extra summary entry
pointing to dirty node space, which should be ignored -- it is a bit of
a waste, but harmless.
When mtd_writev() returns *retlen == 0, though, the node space is not
reserved as dirty, but re-used; the extra summary entry then points
into the space of the next node. After the erase block has been closed,
we get the following messages on remount:
jffs2: error: (79) jffs2_link_node_ref:
Adding new ref c3048d18 at (0x00ec5b88-0x00ec6bcc)
not immediately after previous (0x00ec5b88-0x00ec5b88)
...
jffs2: Checked all inodes but still 0x2088 bytes of unchecked space?
jffs2: No space for garbage collection. Aborting GC thread
The extra summary entries amount to "unchecked space", so that
jffs2_garbage_collect_pass() returns -ENOSPC. And without garbage
collection, the filesystem becomes unuseable over time as the erase
blocks fill up.
Fix this by skipping jffs2_sum_add_kvec() when the MTD write fails. We
don't need the summary entry anyway, and the behaviour matches that of
jffs2_flash_writev() in wbuf.c (with write buffering enabled).
Signed-off-by: Thomas Betker
---
fs/jffs2/writev.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/jffs2/writev.c b/fs/jffs2/writev.c
index a1bda9d..eec4197 100644
--- a/fs/jffs2/writev.c
+++ b/fs/jffs2/writev.c
@@ -16,9 +16,18 @@
int jffs2_flash_direct_writev(struct jffs2_sb_info *c, const struct kvec *vecs,
unsigned long count, loff_t to, size_t *retlen)
{
+ int ret;
+
+ ret = mtd_writev(c->mtd, vecs, count, to, retlen);
+
if (!jffs2_is_writebuffered(c)) {
if (jffs2_sum_active()) {
int res;
+
+ if (ret ||
+ *retlen != iov_length((struct iovec *) vecs, count))
+ return ret;
+
res = jffs2_sum_add_kvec(c, vecs, count, (uint32_t) to);
if (res) {
return res;
@@ -26,19 +35,23 @@ int jffs2_flash_direct_writev(struct jffs2_sb_info *c,
const struct kvec *vecs,
}
}
- return mtd_writev(c->mtd, vecs, count, to, retlen);
+ return ret;
}
int jffs2_flash_direct_write(struct jffs2_sb_info *c, loff_t ofs, size_t len,
size_t *retlen, const u_char *buf)
{
int ret;
+
ret = mtd_write(c->mtd, ofs, len, retlen, buf);
if (jffs2_sum_active()) {
struct kvec vecs[1];
int res;
+ if (ret || *retlen != len)
+ return ret;
+
vecs[0].iov_base = (unsigned char *) buf;
vecs[0].iov_len = len;
@@ -47,5 +60,6 @@ int jffs2_flash_direct_write(struct jffs2_sb_info *c, loff_t
ofs, size_t len,
return res;
}
}
+
return ret;
}
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] iio: adc: xilinx: Fix VREFN scale
From: Thomas Betker The scaling factor for VREFN is 3.0/4096 (not 1.0/4096), just as for VREFP. This is not immediately obvious from the specification (Xilinx UG480), but has been confirmed by Xilinx support. Suggested-by: Hartmut Knaack Signed-off-by: Thomas Betker --- drivers/iio/adc/xilinx-xadc-core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c index 0370624..02e636a 100644 --- a/drivers/iio/adc/xilinx-xadc-core.c +++ b/drivers/iio/adc/xilinx-xadc-core.c @@ -841,6 +841,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev, case XADC_REG_VCCINT: case XADC_REG_VCCAUX: case XADC_REG_VREFP: + case XADC_REG_VREFN: case XADC_REG_VCCBRAM: case XADC_REG_VCCPINT: case XADC_REG_VCCPAUX: -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] jffs2: Don't add summary entry when MTD write fails
From: Thomas Betker <thomas.bet...@rohde-schwarz.com>
jffs2_flash_direct_writev() always invokes jffs2_sum_add_kvec(), even
if mtd_writev() fails. Usually, this results in an extra summary entry
pointing to dirty node space, which should be ignored -- it is a bit of
a waste, but harmless.
When mtd_writev() returns *retlen == 0, though, the node space is not
reserved as dirty, but re-used; the extra summary entry then points
into the space of the next node. After the erase block has been closed,
we get the following messages on remount:
jffs2: error: (79) jffs2_link_node_ref:
Adding new ref c3048d18 at (0x00ec5b88-0x00ec6bcc)
not immediately after previous (0x00ec5b88-0x00ec5b88)
...
jffs2: Checked all inodes but still 0x2088 bytes of unchecked space?
jffs2: No space for garbage collection. Aborting GC thread
The extra summary entries amount to "unchecked space", so that
jffs2_garbage_collect_pass() returns -ENOSPC. And without garbage
collection, the filesystem becomes unuseable over time as the erase
blocks fill up.
Fix this by skipping jffs2_sum_add_kvec() when the MTD write fails. We
don't need the summary entry anyway, and the behaviour matches that of
jffs2_flash_writev() in wbuf.c (with write buffering enabled).
Signed-off-by: Thomas Betker <thomas.bet...@rohde-schwarz.com>
---
fs/jffs2/writev.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/jffs2/writev.c b/fs/jffs2/writev.c
index a1bda9d..eec4197 100644
--- a/fs/jffs2/writev.c
+++ b/fs/jffs2/writev.c
@@ -16,9 +16,18 @@
int jffs2_flash_direct_writev(struct jffs2_sb_info *c, const struct kvec *vecs,
unsigned long count, loff_t to, size_t *retlen)
{
+ int ret;
+
+ ret = mtd_writev(c->mtd, vecs, count, to, retlen);
+
if (!jffs2_is_writebuffered(c)) {
if (jffs2_sum_active()) {
int res;
+
+ if (ret ||
+ *retlen != iov_length((struct iovec *) vecs, count))
+ return ret;
+
res = jffs2_sum_add_kvec(c, vecs, count, (uint32_t) to);
if (res) {
return res;
@@ -26,19 +35,23 @@ int jffs2_flash_direct_writev(struct jffs2_sb_info *c,
const struct kvec *vecs,
}
}
- return mtd_writev(c->mtd, vecs, count, to, retlen);
+ return ret;
}
int jffs2_flash_direct_write(struct jffs2_sb_info *c, loff_t ofs, size_t len,
size_t *retlen, const u_char *buf)
{
int ret;
+
ret = mtd_write(c->mtd, ofs, len, retlen, buf);
if (jffs2_sum_active()) {
struct kvec vecs[1];
int res;
+ if (ret || *retlen != len)
+ return ret;
+
vecs[0].iov_base = (unsigned char *) buf;
vecs[0].iov_len = len;
@@ -47,5 +60,6 @@ int jffs2_flash_direct_write(struct jffs2_sb_info *c, loff_t
ofs, size_t len,
return res;
}
}
+
return ret;
}
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] iio: adc: xilinx: Fix VREFN scale
From: Thomas Betker <thomas.bet...@rohde-schwarz.com> The scaling factor for VREFN is 3.0/4096 (not 1.0/4096), just as for VREFP. This is not immediately obvious from the specification (Xilinx UG480), but has been confirmed by Xilinx support. Suggested-by: Hartmut Knaack <knaac...@gmx.de> Signed-off-by: Thomas Betker <thomas.bet...@rohde-schwarz.com> --- drivers/iio/adc/xilinx-xadc-core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c index 0370624..02e636a 100644 --- a/drivers/iio/adc/xilinx-xadc-core.c +++ b/drivers/iio/adc/xilinx-xadc-core.c @@ -841,6 +841,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev, case XADC_REG_VCCINT: case XADC_REG_VCCAUX: case XADC_REG_VREFP: + case XADC_REG_VREFN: case XADC_REG_VCCBRAM: case XADC_REG_VCCPINT: case XADC_REG_VCCPAUX: -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Revert "jffs2: Fix lock acquisition order bug in jffs2_write_begin"
From: Thomas Betker
This reverts commit 5ffd3412ae55
("jffs2: Fix lock acquisition order bug in jffs2_write_begin").
The commit modified jffs2_write_begin() to remove a deadlock with
jffs2_garbage_collect_live(), but this introduced new deadlocks found
by multiple users. page_lock() actually has to be called before
mutex_lock(>alloc_sem) or mutex_lock(>sem) because
jffs2_write_end() and jffs2_readpage() are called with the page locked,
and they acquire c->alloc_sem and f->sem, resp.
In other words, the lock order in jffs2_write_begin() was correct, and
it is the jffs2_garbage_collect_live() path that has to be changed.
Revert the commit to get rid of the new deadlocks, and to clear the way
for a better fix of the original deadlock.
Reported-by: Deng Chao
Reported-by: Ming Liu
Reported-by: wangzaiwei
Signed-off-by: Thomas Betker
Cc: sta...@vger.kernel.org
---
fs/jffs2/file.c | 39 ++-
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index f509f62..3361979 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -137,39 +137,33 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
struct page *pg;
struct inode *inode = mapping->host;
struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
- struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
- struct jffs2_raw_inode ri;
- uint32_t alloc_len = 0;
pgoff_t index = pos >> PAGE_CACHE_SHIFT;
uint32_t pageofs = index << PAGE_CACHE_SHIFT;
int ret = 0;
- jffs2_dbg(1, "%s()\n", __func__);
-
- if (pageofs > inode->i_size) {
- ret = jffs2_reserve_space(c, sizeof(ri), _len,
- ALLOC_NORMAL,
JFFS2_SUMMARY_INODE_SIZE);
- if (ret)
- return ret;
- }
-
- mutex_lock(>sem);
pg = grab_cache_page_write_begin(mapping, index, flags);
- if (!pg) {
- if (alloc_len)
- jffs2_complete_reservation(c);
- mutex_unlock(>sem);
+ if (!pg)
return -ENOMEM;
- }
*pagep = pg;
- if (alloc_len) {
+ jffs2_dbg(1, "%s()\n", __func__);
+
+ if (pageofs > inode->i_size) {
/* Make new hole frag from old EOF to new page */
+ struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
+ struct jffs2_raw_inode ri;
struct jffs2_full_dnode *fn;
+ uint32_t alloc_len;
jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current
EOF and new page\n",
(unsigned int)inode->i_size, pageofs);
+ ret = jffs2_reserve_space(c, sizeof(ri), _len,
+ ALLOC_NORMAL,
JFFS2_SUMMARY_INODE_SIZE);
+ if (ret)
+ goto out_page;
+
+ mutex_lock(>sem);
memset(, 0, sizeof(ri));
ri.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
@@ -196,6 +190,7 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
if (IS_ERR(fn)) {
ret = PTR_ERR(fn);
jffs2_complete_reservation(c);
+ mutex_unlock(>sem);
goto out_page;
}
ret = jffs2_add_full_dnode_to_inode(c, f, fn);
@@ -210,10 +205,12 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
jffs2_mark_node_obsolete(c, fn->raw);
jffs2_free_full_dnode(fn);
jffs2_complete_reservation(c);
+ mutex_unlock(>sem);
goto out_page;
}
jffs2_complete_reservation(c);
inode->i_size = pageofs;
+ mutex_unlock(>sem);
}
/*
@@ -222,18 +219,18 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
* case of a short-copy.
*/
if (!PageUptodate(pg)) {
+ mutex_lock(>sem);
ret = jffs2_do_readpage_nolock(inode, pg);
+ mutex_unlock(>sem);
if (ret)
goto out_page;
}
- mutex_unlock(>sem);
jffs2_dbg(1, "end write_begin(). pg->flags %lx\n", pg->flags);
return ret;
out_page:
unlock_page(pg);
page_cache_release(pg);
- mutex_unlock(>sem);
return ret;
}
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Revert "jffs2: Fix lock acquisition order bug in jffs2_write_begin"
From: Thomas Betker <thomas.bet...@rohde-schwarz.com>
This reverts commit 5ffd3412ae55
("jffs2: Fix lock acquisition order bug in jffs2_write_begin").
The commit modified jffs2_write_begin() to remove a deadlock with
jffs2_garbage_collect_live(), but this introduced new deadlocks found
by multiple users. page_lock() actually has to be called before
mutex_lock(>alloc_sem) or mutex_lock(>sem) because
jffs2_write_end() and jffs2_readpage() are called with the page locked,
and they acquire c->alloc_sem and f->sem, resp.
In other words, the lock order in jffs2_write_begin() was correct, and
it is the jffs2_garbage_collect_live() path that has to be changed.
Revert the commit to get rid of the new deadlocks, and to clear the way
for a better fix of the original deadlock.
Reported-by: Deng Chao <deng.ch...@zte.com.cn>
Reported-by: Ming Liu <liu.min...@gmail.com>
Reported-by: wangzaiwei <wangzai...@top-vision.cn>
Signed-off-by: Thomas Betker <thomas.bet...@rohde-schwarz.com>
Cc: sta...@vger.kernel.org
---
fs/jffs2/file.c | 39 ++-
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index f509f62..3361979 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -137,39 +137,33 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
struct page *pg;
struct inode *inode = mapping->host;
struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
- struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
- struct jffs2_raw_inode ri;
- uint32_t alloc_len = 0;
pgoff_t index = pos >> PAGE_CACHE_SHIFT;
uint32_t pageofs = index << PAGE_CACHE_SHIFT;
int ret = 0;
- jffs2_dbg(1, "%s()\n", __func__);
-
- if (pageofs > inode->i_size) {
- ret = jffs2_reserve_space(c, sizeof(ri), _len,
- ALLOC_NORMAL,
JFFS2_SUMMARY_INODE_SIZE);
- if (ret)
- return ret;
- }
-
- mutex_lock(>sem);
pg = grab_cache_page_write_begin(mapping, index, flags);
- if (!pg) {
- if (alloc_len)
- jffs2_complete_reservation(c);
- mutex_unlock(>sem);
+ if (!pg)
return -ENOMEM;
- }
*pagep = pg;
- if (alloc_len) {
+ jffs2_dbg(1, "%s()\n", __func__);
+
+ if (pageofs > inode->i_size) {
/* Make new hole frag from old EOF to new page */
+ struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
+ struct jffs2_raw_inode ri;
struct jffs2_full_dnode *fn;
+ uint32_t alloc_len;
jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current
EOF and new page\n",
(unsigned int)inode->i_size, pageofs);
+ ret = jffs2_reserve_space(c, sizeof(ri), _len,
+ ALLOC_NORMAL,
JFFS2_SUMMARY_INODE_SIZE);
+ if (ret)
+ goto out_page;
+
+ mutex_lock(>sem);
memset(, 0, sizeof(ri));
ri.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
@@ -196,6 +190,7 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
if (IS_ERR(fn)) {
ret = PTR_ERR(fn);
jffs2_complete_reservation(c);
+ mutex_unlock(>sem);
goto out_page;
}
ret = jffs2_add_full_dnode_to_inode(c, f, fn);
@@ -210,10 +205,12 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
jffs2_mark_node_obsolete(c, fn->raw);
jffs2_free_full_dnode(fn);
jffs2_complete_reservation(c);
+ mutex_unlock(>sem);
goto out_page;
}
jffs2_complete_reservation(c);
inode->i_size = pageofs;
+ mutex_unlock(>sem);
}
/*
@@ -222,18 +219,18 @@ static int jffs2_write_begin(struct file *filp, struct
address_space *mapping,
* case of a short-copy.
*/
if (!PageUptodate(pg)) {
+ mutex_lock(>sem);
ret = jffs2_do_readpage_nolock(inode, pg);
+ mutex_unlock(>sem);
if (ret)
goto out_page;
}
- mutex_unlock(>sem);
jffs2_dbg(1, "end write_begin(). pg->flags %lx\n", pg->flags);
return ret;
out_page:
unlock_page(pg);
page_cache_release(pg);
- mutex_unlock(>sem);
return ret;
}
--
2.6.3
--
To unsubscribe from th
Re: [RFC PATCH 0/2] spi: add dual parallel & stacked mode support in Zynq MPSoC GQSPI controller
Hello Ranjit: > What is dual parallel mode? > --- > ZynqMP GQSPI controller supports Dual Parallel mode with following > functionalities: > 1) Supporting two SPI flash memories operating in parallel. 8 I/O lines. > 2) Chip selects and clock are shared to both the flash devices > 3) This mode is targeted for faster read/write speed and also doubles the size > 4) Commands/data can be transmitted/received from both the devices(mirror), >or only upper or only lower flash memory devices. > 5) Data arrangement: >With stripe enabled, >Even bytes i.e. 0, 2, 4,... are transmitted on Lower Data Bus >Odd bytes i.e. 1, 3, 5,.. are transmitted on Upper Data Bus. In the dual-parallel configuration, odd and even _bits_ of each byte are distributed over the flash chips; I am assuming this works just as in Zynq QSPI (apparently, the TRM for ZynqMP isn't out yet). Striping seems to be a different mechanism, though. Can you explain it a bit more? Also, the wording seems to indicate that it belongs to dual-stacked rather than dual-parallel. > Suggestions on MTD layer support > > In order to add above two specified modes, we may required to get some > support from MTD layer. > > I'm trying to list the dependencies as follows: > 1) Support for two flashes > 2) Enable/Disable data stripe as and when required. > 3) May need to update read_sr() to get status of both flashes > 4) May also need to update read_fsr() to get status of both flashes > 5) Adjustment of offset value based on the parallel/stacked mode configuration > 6) Setting either parallel or stacked mode during the scan process. > 7) In case of stacked mode, is there a MTD concatenation support? In addition to 5), the MTD driver using a dual-parallel QSPI flash has to 5a) add padding at the start of data for unaligned addresses, 5b) add padding at the end of data for unaligned lengths. Best regards, Thomas Betker -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/2] spi: add dual parallel stacked mode support in Zynq MPSoC GQSPI controller
Hello Ranjit: What is dual parallel mode? --- ZynqMP GQSPI controller supports Dual Parallel mode with following functionalities: 1) Supporting two SPI flash memories operating in parallel. 8 I/O lines. 2) Chip selects and clock are shared to both the flash devices 3) This mode is targeted for faster read/write speed and also doubles the size 4) Commands/data can be transmitted/received from both the devices(mirror), or only upper or only lower flash memory devices. 5) Data arrangement: With stripe enabled, Even bytes i.e. 0, 2, 4,... are transmitted on Lower Data Bus Odd bytes i.e. 1, 3, 5,.. are transmitted on Upper Data Bus. In the dual-parallel configuration, odd and even _bits_ of each byte are distributed over the flash chips; I am assuming this works just as in Zynq QSPI (apparently, the TRM for ZynqMP isn't out yet). Striping seems to be a different mechanism, though. Can you explain it a bit more? Also, the wording seems to indicate that it belongs to dual-stacked rather than dual-parallel. Suggestions on MTD layer support In order to add above two specified modes, we may required to get some support from MTD layer. I'm trying to list the dependencies as follows: 1) Support for two flashes 2) Enable/Disable data stripe as and when required. 3) May need to update read_sr() to get status of both flashes 4) May also need to update read_fsr() to get status of both flashes 5) Adjustment of offset value based on the parallel/stacked mode configuration 6) Setting either parallel or stacked mode during the scan process. 7) In case of stacked mode, is there a MTD concatenation support? In addition to 5), the MTD driver using a dual-parallel QSPI flash has to 5a) add padding at the start of data for unaligned addresses, 5b) add padding at the end of data for unaligned lengths. Best regards, Thomas Betker -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)
Hello Catalin: > > I've been under the impression that this shouldn't be done in the kernel, > > but in the boot loader/firmware: > > > > https://lkml.org/lkml/2015/2/20/199 > > > > http://lists.denx.de/pipermail/u-boot/2015-March/207803.html > > If you can fix it in the boot loader or firmware even better. If it's > not doable, this patch will help (if Russell takes it): > > http://article.gmane.org/gmane.linux.ports.sh.devel/45685 Thanks for the info! Geert's patch looks good to me, and I sure hope it will be accepted. Best regards, Thomas Betker -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)
Hello Catalin: I've been under the impression that this shouldn't be done in the kernel, but in the boot loader/firmware: https://lkml.org/lkml/2015/2/20/199 http://lists.denx.de/pipermail/u-boot/2015-March/207803.html If you can fix it in the boot loader or firmware even better. If it's not doable, this patch will help (if Russell takes it): http://article.gmane.org/gmane.linux.ports.sh.devel/45685 Thanks for the info! Geert's patch looks good to me, and I sure hope it will be accepted. Best regards, Thomas Betker -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)
> >> This patch is based on the
> >> commit 1a8e41cd672f ("ARM: 6395/1: VExpress: Set bit 22 in the PL310
> >> (cache controller) AuxCtlr register")
> >
> >
> > I've been under the impression that this shouldn't be done in the
> > kernel, but in the boot loader/firmware:
> >
> > https://lkml.org/lkml/2015/2/20/199
> >
> > http://lists.denx.de/pipermail/u-boot/2015-March/207803.html
>
> Tegra, Exynos, sti still have this bit set.
In 4.1-rc3, the bit is set by berlin, exynos, nomadik, omap2, sti, tegra,
vexpress.
> Does that mean that they should be just removed because fix should be in
> bootloader?
>
> Anyway it is normal that bootloader stay on system untouched and only OS
> is updated. But OK - let's make this in bootloader if this is preferred
> solution.
So the plan is to update each and every Zynq bootloader for a problem we
have encountered in Linux (in our case, a problem we have been hunting for
months)? My u-boot doesn't even use the cache; it's disabled until the
kernel boots.
I do understand that the kernel should not overwrite hardwired settings
such as cache size, but shouldn't we at least allow to fix things that
definitely need to be fixed?
Best regards,
Thomas Betker
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: zynq: Set bit 22 in PL310 AuxCtrl register (6395/1)
This patch is based on the commit 1a8e41cd672f (ARM: 6395/1: VExpress: Set bit 22 in the PL310 (cache controller) AuxCtlr register) I've been under the impression that this shouldn't be done in the kernel, but in the boot loader/firmware: https://lkml.org/lkml/2015/2/20/199 http://lists.denx.de/pipermail/u-boot/2015-March/207803.html Tegra, Exynos, sti still have this bit set. In 4.1-rc3, the bit is set by berlin, exynos, nomadik, omap2, sti, tegra, vexpress. Does that mean that they should be just removed because fix should be in bootloader? Anyway it is normal that bootloader stay on system untouched and only OS is updated. But OK - let's make this in bootloader if this is preferred solution. So the plan is to update each and every Zynq bootloader for a problem we have encountered in Linux (in our case, a problem we have been hunting for months)? My u-boot doesn't even use the cache; it's disabled until the kernel boots. I do understand that the kernel should not overwrite hardwired settings such as cache size, but shouldn't we at least allow to fix things that definitely need to be fixed? Best regards, Thomas Betker -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/3] Using SPI NOR flah on sunxi.
Hello Michal: > I tried to connect a SPI NOR flash to my sunxi board and due to the current > sunxi SPI driver limitations it does not work. > > The SPI driver returns an error when more than 64 bytes are > transferred at once > due to lack of DMA support. Wouldn't it be easier to fix the SPI driver to handle transfers larger than 64 bytes, filling and draining the FIFO multiple times if neccessary? (As far as I can tell, most SPI drivers do this.) Just asking, Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/3] Using SPI NOR flah on sunxi.
Hello Michal: I tried to connect a SPI NOR flash to my sunxi board and due to the current sunxi SPI driver limitations it does not work. The SPI driver returns an error when more than 64 bytes are transferred at once due to lack of DMA support. Wouldn't it be easier to fix the SPI driver to handle transfers larger than 64 bytes, filling and draining the FIFO multiple times if neccessary? (As far as I can tell, most SPI drivers do this.) Just asking, Thomas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
