Messages in this thread | | | Date | Wed, 20 Oct 2021 11:13:47 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH v2 2/4] mtd: core: protect access to MTD devices while in suspend |
| |
On Wed, 20 Oct 2021 10:45:32 +0200 Sean Nyekjaer <sean@geanix.com> wrote:
> static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user) > @@ -1257,6 +1259,8 @@ int mtd_erase(struct mtd_info *mtd, sruct erase_info *instr) > > ledtrig_mtd_activity(); > > + mtd_start_access(master); > + > if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) { > adjinstr.addr = (loff_t)mtd_div_by_eb(instr->addr, mtd) * > master->erasesize; > @@ -1278,6 +1282,8 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) > } > } > > + mtd_end_access(master); > +
The section covered in mtd_erase() is too broad. Put the start/end calls around the ->_erase() call.
> return ret; > }
> @@ -1576,7 +1604,6 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) > ret_code = mtd_read_oob_std(mtd, from, ops); > > mtd_update_ecc_stats(mtd, master, &old_stats); > -
Unrelated line removal. Please drop this change.
> /* > * In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics > * similar to mtd->_read(), returning a non-negative integer > @@ -1615,7 +1642,9 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to, > if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) > return mtd_io_emulated_slc(mtd, to, false, ops); > > - return mtd_write_oob_std(mtd, to, ops); > + ret = mtd_write_oob_std(mtd, to, ops); > + > + return ret;
Ditto, you can keep the 'return mtd_write_oob_std(mtd, to, ops);' here.
> } > EXPORT_SYMBOL_GPL(mtd_write_oob);
> +static inline void mtd_start_access(struct mtd_info *master) > +{ > + WARN_ON_ONCE(master != mtd_get_master(master)); > + > + /* > + * Don't take the suspend_lock on devices that don't > + * implement the suspend hook. Otherwise, lockdep will > + * complain about nested locks when trying to suspend MTD > + * partitions or MTD devices created by gluebi which are > + * backed by real devices. > + */ > + if (!master->_suspend) > + return; > + > + /* > + * Wait until the device is resumed. Should we have a > + * non-blocking mode here? > + */ > + while (1) { > + down_read(&master->master.suspend_lock); > + if (!master->master.suspended) > + return; > + > + up_read(&master->master.suspend_lock); > + wait_event(master->master.resume_wq, master->master.suspended == 0);
Please keep the tests consistent:
wait_event(master->master.resume_wq, !master->master.suspended);
> + } > +} > +
| |