[PATCH v3 3/4] mtd: Stop directly calling master ->_xxx() hooks from mtdpart code
Peter Pan
peterpansjtu at gmail.com
Wed Jan 3 18:06:03 PST 2018
Boris,
On Fri, Dec 22, 2017 at 4:37 PM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> On Fri, 22 Dec 2017 13:40:26 +0800
> Peter Pan <peterpansjtu at gmail.com> wrote:
>
>> Hi Boris,
>>
>> On Fri, Dec 15, 2017 at 8:39 PM, Boris Brezillon
>> <boris.brezillon at free-electrons.com> wrote:
>> > The MTD layer provides several wrappers around mtd->_xxx() hooks. Call
>> > these wrappers instead of directly dereferencing the associated ->_xxx()
>> > pointer.
>> >
>> > This change has been motivated by another rework letting the core
>> > handle the case where ->_read/write_oob() are implemented but not
>> > ->_read/write(). In this case, we want mtd_read/write() to fall back to
>> > ->_read/write_oob() when ->_read/write() are NULL. The problem is,
>> > mtdpart is directly calling the ->_xxx() instead of using the wrappers,
>> > thus leading to a NULL pointer exception.
>> >
>> > Even though we only need to do the change for part_read/write(), going
>> > through those wrappers for all kind of part -> master operation
>> > propagation is a good thing, because other wrappers might become
>> > smarter over time, and the duplicated check overhead (parameters will
>> > be checked at the partition and master level instead of only at the
>> > partition level) should be negligible.
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
>> > ---
>> > Changes in v3:
>> > - unconditionally assign part wrappers as suggested by Brian
>> >
>> > Changes in v2:
>> > - new patch needed to fix a NULL pointer dereference BUG
>> > ---
>> > drivers/mtd/mtdpart.c | 141 +++++++++++++++++++-------------------------------
>> > 1 file changed, 53 insertions(+), 88 deletions(-)
>> >
>> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> > index be088bccd593..e83c9d870b11 100644
>> > --- a/drivers/mtd/mtdpart.c
>> > +++ b/drivers/mtd/mtdpart.c
>> > @@ -74,8 +74,7 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>> > int res;
>> >
>>
>> This is not about your modification. But shouldn't we add check to prevent
>> part_read/write/write_oob from accessing past the end of partition?
>> There is a check in part_read_oob() only.
>
> You should not call part_xxx() directly, and mtd_read/write{_oob}()
> should already check that. If that's not the case, we should fix them.
>
> Can you give a bit more details about what is wrong?
Sorry for late reply. My failure is due to failed to assign mtd->oobavail.
We don't have problem in OOB ops checking.
But since we already check it in mtd_read/write_oob(), shouln't we
remove the check in part_read/write_oob() ?
Thanks
Peter Pan
>
> Thanks,
>
> Boris
>
>>
>> Thanks
>> Peter Pan
>>
>> > stats = part->parent->ecc_stats;
>> > - res = part->parent->_read(part->parent, from + part->offset, len,
>> > - retlen, buf);
>> > + res = mtd_read(part->parent, from + part->offset, len, retlen, buf);
>> > if (unlikely(mtd_is_eccerr(res)))
>> > mtd->ecc_stats.failed +=
>> > part->parent->ecc_stats.failed - stats.failed;
>> > @@ -90,15 +89,15 @@ static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> >
>> > - return part->parent->_point(part->parent, from + part->offset, len,
>> > - retlen, virt, phys);
>> > + return mtd_point(part->parent, from + part->offset, len, retlen, virt,
>> > + phys);
>> > }
>> >
>> > static int part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> >
>> > - return part->parent->_unpoint(part->parent, from + part->offset, len);
>> > + return mtd_unpoint(part->parent, from + part->offset, len);
>> > }
>> >
>> > static int part_read_oob(struct mtd_info *mtd, loff_t from,
>> > @@ -126,7 +125,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>> > return -EINVAL;
>> > }
>> >
>> > - res = part->parent->_read_oob(part->parent, from + part->offset, ops);
>> > + res = mtd_read_oob(part->parent, from + part->offset, ops);
>> > if (unlikely(res)) {
>> > if (mtd_is_bitflip(res))
>> > mtd->ecc_stats.corrected++;
>> > @@ -140,48 +139,43 @@ static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
>> > size_t len, size_t *retlen, u_char *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_read_user_prot_reg(part->parent, from, len,
>> > - retlen, buf);
>> > + return mtd_read_user_prot_reg(part->parent, from, len, retlen, buf);
>> > }
>> >
>> > static int part_get_user_prot_info(struct mtd_info *mtd, size_t len,
>> > size_t *retlen, struct otp_info *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_get_user_prot_info(part->parent, len, retlen,
>> > - buf);
>> > + return mtd_get_user_prot_info(part->parent, len, retlen, buf);
>> > }
>> >
>> > static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
>> > size_t len, size_t *retlen, u_char *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_read_fact_prot_reg(part->parent, from, len,
>> > - retlen, buf);
>> > + return mtd_read_fact_prot_reg(part->parent, from, len, retlen, buf);
>> > }
>> >
>> > static int part_get_fact_prot_info(struct mtd_info *mtd, size_t len,
>> > size_t *retlen, struct otp_info *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_get_fact_prot_info(part->parent, len, retlen,
>> > - buf);
>> > + return mtd_get_fact_prot_info(part->parent, len, retlen, buf);
>> > }
>> >
>> > static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
>> > size_t *retlen, const u_char *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_write(part->parent, to + part->offset, len,
>> > - retlen, buf);
>> > + return mtd_write(part->parent, to + part->offset, len, retlen, buf);
>> > }
>> >
>> > static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
>> > size_t *retlen, const u_char *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_panic_write(part->parent, to + part->offset, len,
>> > - retlen, buf);
>> > + return mtd_panic_write(part->parent, to + part->offset, len, retlen,
>> > + buf);
>> > }
>> >
>> > static int part_write_oob(struct mtd_info *mtd, loff_t to,
>> > @@ -193,30 +187,29 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to,
>> > return -EINVAL;
>> > if (ops->datbuf && to + ops->len > mtd->size)
>> > return -EINVAL;
>> > - return part->parent->_write_oob(part->parent, to + part->offset, ops);
>> > + return mtd_write_oob(part->parent, to + part->offset, ops);
>> > }
>> >
>> > static int part_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>> > size_t len, size_t *retlen, u_char *buf)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_write_user_prot_reg(part->parent, from, len,
>> > - retlen, buf);
>> > + return mtd_write_user_prot_reg(part->parent, from, len, retlen, buf);
>> > }
>> >
>> > static int part_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
>> > size_t len)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_lock_user_prot_reg(part->parent, from, len);
>> > + return mtd_lock_user_prot_reg(part->parent, from, len);
>> > }
>> >
>> > static int part_writev(struct mtd_info *mtd, const struct kvec *vecs,
>> > unsigned long count, loff_t to, size_t *retlen)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_writev(part->parent, vecs, count,
>> > - to + part->offset, retlen);
>> > + return mtd_writev(part->parent, vecs, count, to + part->offset,
>> > + retlen);
>> > }
>> >
>> > static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
>> > @@ -225,7 +218,7 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
>> > int ret;
>> >
>> > instr->addr += part->offset;
>> > - ret = part->parent->_erase(part->parent, instr);
>> > + ret = mtd_erase(part->parent, instr);
>> > if (ret) {
>> > if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
>> > instr->fail_addr -= part->offset;
>> > @@ -251,51 +244,51 @@ EXPORT_SYMBOL_GPL(mtd_erase_callback);
>> > static int part_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_lock(part->parent, ofs + part->offset, len);
>> > + return mtd_lock(part->parent, ofs + part->offset, len);
>> > }
>> >
>> > static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_unlock(part->parent, ofs + part->offset, len);
>> > + return mtd_unlock(part->parent, ofs + part->offset, len);
>> > }
>> >
>> > static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_is_locked(part->parent, ofs + part->offset, len);
>> > + return mtd_is_locked(part->parent, ofs + part->offset, len);
>> > }
>> >
>> > static void part_sync(struct mtd_info *mtd)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - part->parent->_sync(part->parent);
>> > + mtd_sync(part->parent);
>> > }
>> >
>> > static int part_suspend(struct mtd_info *mtd)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_suspend(part->parent);
>> > + return mtd_suspend(part->parent);
>> > }
>> >
>> > static void part_resume(struct mtd_info *mtd)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - part->parent->_resume(part->parent);
>> > + mtd_resume(part->parent);
>> > }
>> >
>> > static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > ofs += part->offset;
>> > - return part->parent->_block_isreserved(part->parent, ofs);
>> > + return mtd_block_isreserved(part->parent, ofs);
>> > }
>> >
>> > static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > ofs += part->offset;
>> > - return part->parent->_block_isbad(part->parent, ofs);
>> > + return mtd_block_isbad(part->parent, ofs);
>> > }
>> >
>> > static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
>> > @@ -304,7 +297,7 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
>> > int res;
>> >
>> > ofs += part->offset;
>> > - res = part->parent->_block_markbad(part->parent, ofs);
>> > + res = mtd_block_markbad(part->parent, ofs);
>> > if (!res)
>> > mtd->ecc_stats.badblocks++;
>> > return res;
>> > @@ -313,13 +306,13 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
>> > static int part_get_device(struct mtd_info *mtd)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - return part->parent->_get_device(part->parent);
>> > + return __get_mtd_device(part->parent);
>> > }
>> >
>> > static void part_put_device(struct mtd_info *mtd)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> > - part->parent->_put_device(part->parent);
>> > + __put_mtd_device(part->parent);
>> > }
>> >
>> > static int part_ooblayout_ecc(struct mtd_info *mtd, int section,
>> > @@ -347,8 +340,7 @@ static int part_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
>> > {
>> > struct mtd_part *part = mtd_to_part(mtd);
>> >
>> > - return part->parent->_max_bad_blocks(part->parent,
>> > - ofs + part->offset, len);
>> > + return mtd_max_bad_blocks(part->parent, ofs + part->offset, len);
>> > }
>> >
>> > static inline void free_partition(struct mtd_part *p)
>> > @@ -437,59 +429,32 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent,
>> >
>> > slave->mtd._read = part_read;
>> > slave->mtd._write = part_write;
>> > -
>> > - if (parent->_panic_write)
>> > - slave->mtd._panic_write = part_panic_write;
>> > -
>> > - if (parent->_point && parent->_unpoint) {
>> > - slave->mtd._point = part_point;
>> > - slave->mtd._unpoint = part_unpoint;
>> > - }
>> > -
>> > - if (parent->_read_oob)
>> > - slave->mtd._read_oob = part_read_oob;
>> > - if (parent->_write_oob)
>> > - slave->mtd._write_oob = part_write_oob;
>> > - if (parent->_read_user_prot_reg)
>> > - slave->mtd._read_user_prot_reg = part_read_user_prot_reg;
>> > - if (parent->_read_fact_prot_reg)
>> > - slave->mtd._read_fact_prot_reg = part_read_fact_prot_reg;
>> > - if (parent->_write_user_prot_reg)
>> > - slave->mtd._write_user_prot_reg = part_write_user_prot_reg;
>> > - if (parent->_lock_user_prot_reg)
>> > - slave->mtd._lock_user_prot_reg = part_lock_user_prot_reg;
>> > - if (parent->_get_user_prot_info)
>> > - slave->mtd._get_user_prot_info = part_get_user_prot_info;
>> > - if (parent->_get_fact_prot_info)
>> > - slave->mtd._get_fact_prot_info = part_get_fact_prot_info;
>> > - if (parent->_sync)
>> > - slave->mtd._sync = part_sync;
>> > - if (!partno && !parent->dev.class && parent->_suspend &&
>> > - parent->_resume) {
>> > + slave->mtd._panic_write = part_panic_write;
>> > + slave->mtd._point = part_point;
>> > + slave->mtd._unpoint = part_unpoint;
>> > + slave->mtd._read_oob = part_read_oob;
>> > + slave->mtd._write_oob = part_write_oob;
>> > + slave->mtd._read_user_prot_reg = part_read_user_prot_reg;
>> > + slave->mtd._read_fact_prot_reg = part_read_fact_prot_reg;
>> > + slave->mtd._write_user_prot_reg = part_write_user_prot_reg;
>> > + slave->mtd._lock_user_prot_reg = part_lock_user_prot_reg;
>> > + slave->mtd._get_user_prot_info = part_get_user_prot_info;
>> > + slave->mtd._get_fact_prot_info = part_get_fact_prot_info;
>> > + slave->mtd._sync = part_sync;
>> > + if (!partno && !parent->dev.class) {
>> > slave->mtd._suspend = part_suspend;
>> > slave->mtd._resume = part_resume;
>> > }
>> > - if (parent->_writev)
>> > - slave->mtd._writev = part_writev;
>> > - if (parent->_lock)
>> > - slave->mtd._lock = part_lock;
>> > - if (parent->_unlock)
>> > - slave->mtd._unlock = part_unlock;
>> > - if (parent->_is_locked)
>> > - slave->mtd._is_locked = part_is_locked;
>> > - if (parent->_block_isreserved)
>> > - slave->mtd._block_isreserved = part_block_isreserved;
>> > - if (parent->_block_isbad)
>> > - slave->mtd._block_isbad = part_block_isbad;
>> > - if (parent->_block_markbad)
>> > - slave->mtd._block_markbad = part_block_markbad;
>> > - if (parent->_max_bad_blocks)
>> > - slave->mtd._max_bad_blocks = part_max_bad_blocks;
>> > -
>> > - if (parent->_get_device)
>> > - slave->mtd._get_device = part_get_device;
>> > - if (parent->_put_device)
>> > - slave->mtd._put_device = part_put_device;
>> > + slave->mtd._writev = part_writev;
>> > + slave->mtd._lock = part_lock;
>> > + slave->mtd._unlock = part_unlock;
>> > + slave->mtd._is_locked = part_is_locked;
>> > + slave->mtd._block_isreserved = part_block_isreserved;
>> > + slave->mtd._block_isbad = part_block_isbad;
>> > + slave->mtd._block_markbad = part_block_markbad;
>> > + slave->mtd._max_bad_blocks = part_max_bad_blocks;
>> > + slave->mtd._get_device = part_get_device;
>> > + slave->mtd._put_device = part_put_device;
>> >
>> > slave->mtd._erase = part_erase;
>> > slave->parent = parent;
>> > --
>> > 2.11.0
>> >
>
More information about the linux-mtd
mailing list