Message ID | 984fd8eb53b742bd46e7b42605ae4e0ceaf5ba08.1474450295.git.dwalter@sigma-star.at |
---|---|
State | Accepted |
Headers | show |
On Wed, 21 Sep 2016 11:43:56 +0200 Daniel Walter <dwalter@sigma-star.at> wrote: > From: Richard Weinberger <richard@nod.at> > > If the master device has callbacks for _get/put_device() > and this MTD has slaves a get_mtd_device() call on paritions > will never issue the registered callbacks. > Fix this by propagating _get/put_device() down. Brian, can we have this one queued for 4.9? I can't take it in my tree if you want, but it's probably better if it's in the mtd tree. > > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/mtdpart.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index 1f13e32..ec852fa 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -317,6 +317,18 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs) > return res; > } > > +static int part_get_device(struct mtd_info *mtd) > +{ > + struct mtd_part *part = mtd_to_part(mtd); > + return part->master->_get_device(part->master); > +} > + > +static void part_put_device(struct mtd_info *mtd) > +{ > + struct mtd_part *part = mtd_to_part(mtd); > + part->master->_put_device(part->master); > +} > + > static int part_ooblayout_ecc(struct mtd_info *mtd, int section, > struct mtd_oob_region *oobregion) > { > @@ -463,6 +475,12 @@ static struct mtd_part *allocate_partition(struct mtd_info *master, > slave->mtd._block_isbad = part_block_isbad; > if (master->_block_markbad) > slave->mtd._block_markbad = part_block_markbad; > + > + if (master->_get_device) > + slave->mtd._get_device = part_get_device; > + if (master->_put_device) > + slave->mtd._put_device = part_put_device; > + > slave->mtd._erase = part_erase; > slave->master = master; > slave->offset = part->offset;
On Wed, Sep 21, 2016 at 12:15:31PM +0200, Boris Brezillon wrote: > On Wed, 21 Sep 2016 11:43:56 +0200 > Daniel Walter <dwalter@sigma-star.at> wrote: > > > From: Richard Weinberger <richard@nod.at> > > > > If the master device has callbacks for _get/put_device() > > and this MTD has slaves a get_mtd_device() call on paritions > > will never issue the registered callbacks. > > Fix this by propagating _get/put_device() down. > > Brian, can we have this one queued for 4.9? I can't take it in my tree > if you want, but it's probably better if it's in the mtd tree. Applied this patch to l2-mtd.git
On Wed, Sep 28, 2016 at 8:16 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Wed, Sep 21, 2016 at 12:15:31PM +0200, Boris Brezillon wrote: >> On Wed, 21 Sep 2016 11:43:56 +0200 >> Daniel Walter <dwalter@sigma-star.at> wrote: >> >> > From: Richard Weinberger <richard@nod.at> >> > >> > If the master device has callbacks for _get/put_device() >> > and this MTD has slaves a get_mtd_device() call on paritions >> > will never issue the registered callbacks. >> > Fix this by propagating _get/put_device() down. >> >> Brian, can we have this one queued for 4.9? I can't take it in my tree >> if you want, but it's probably better if it's in the mtd tree. > > Applied this patch to l2-mtd.git > I think this should also go into -stable.
On Wed, Dec 14, 2016 at 07:24:46PM +0000, Karl Beldan wrote: > On Wed, Sep 28, 2016 at 8:16 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Wed, Sep 21, 2016 at 12:15:31PM +0200, Boris Brezillon wrote: > >> On Wed, 21 Sep 2016 11:43:56 +0200 > >> Daniel Walter <dwalter@sigma-star.at> wrote: > >> > >> > From: Richard Weinberger <richard@nod.at> > >> > > >> > If the master device has callbacks for _get/put_device() > >> > and this MTD has slaves a get_mtd_device() call on paritions > >> > will never issue the registered callbacks. > >> > Fix this by propagating _get/put_device() down. > >> > >> Brian, can we have this one queued for 4.9? I can't take it in my tree > >> if you want, but it's probably better if it's in the mtd tree. > > > > Applied this patch to l2-mtd.git > > > > I think this should also go into -stable. Why? Do you have real use cases that are broken by this? I understand this is a problem, but I'm curious on how this satisfies the stable rules. Also, note that this isn't a regression; it's been broken forever and apparently no one noticed. IMO that raises the bar a bit (but not impossibly so) for -stable. Anyway, if we decide to do this, you'll also want to include the git hash and applicable kernel versions, per Option 2 [1]. Brian [1] Documentation/stable_kernel_rules.txt.
Hi! On 14.12.2016 22:09, Brian Norris wrote: > On Wed, Dec 14, 2016 at 07:24:46PM +0000, Karl Beldan wrote: >> On Wed, Sep 28, 2016 at 8:16 PM, Brian Norris >> <computersforpeace@gmail.com> wrote: >>> On Wed, Sep 21, 2016 at 12:15:31PM +0200, Boris Brezillon wrote: >>>> On Wed, 21 Sep 2016 11:43:56 +0200 >>>> Daniel Walter <dwalter@sigma-star.at> wrote: >>>> >>>>> From: Richard Weinberger <richard@nod.at> >>>>> >>>>> If the master device has callbacks for _get/put_device() >>>>> and this MTD has slaves a get_mtd_device() call on paritions >>>>> will never issue the registered callbacks. >>>>> Fix this by propagating _get/put_device() down. >>>> >>>> Brian, can we have this one queued for 4.9? I can't take it in my tree >>>> if you want, but it's probably better if it's in the mtd tree. >>> >>> Applied this patch to l2-mtd.git >>> >> >> I think this should also go into -stable. > > Why? Do you have real use cases that are broken by this? I understand > this is a problem, but I'm curious on how this satisfies the stable > rules. > > Also, note that this isn't a regression; it's been broken forever and > apparently no one noticed. IMO that raises the bar a bit (but not > impossibly so) for -stable. Yes. AFAICT you can only trigger it using my "new" nandsim which is not mainline so far. Thanks, //richard
On Wed, Dec 14, 2016 at 10:12:42PM +0100, Richard Weinberger wrote: > On 14.12.2016 22:09, Brian Norris wrote: > > Also, note that this isn't a regression; it's been broken forever and > > apparently no one noticed. IMO that raises the bar a bit (but not > > impossibly so) for -stable. > > Yes. AFAICT you can only trigger it using my "new" nandsim > which is not mainline so far. Ah, OK. So the only current _{get,put}_device() implementor in mainline is gluebi, so far. And it's OK for now to just have the master device be refcounted, and just rely on the partitions being removed before the master, right? In that case, no, this shouldn't go to -stable. Brian
On Wed, Dec 14, 2016 at 9:09 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Wed, Dec 14, 2016 at 07:24:46PM +0000, Karl Beldan wrote: >> On Wed, Sep 28, 2016 at 8:16 PM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> > On Wed, Sep 21, 2016 at 12:15:31PM +0200, Boris Brezillon wrote: >> >> On Wed, 21 Sep 2016 11:43:56 +0200 >> >> Daniel Walter <dwalter@sigma-star.at> wrote: >> >> >> >> > From: Richard Weinberger <richard@nod.at> >> >> > >> >> > If the master device has callbacks for _get/put_device() >> >> > and this MTD has slaves a get_mtd_device() call on paritions >> >> > will never issue the registered callbacks. >> >> > Fix this by propagating _get/put_device() down. >> >> >> >> Brian, can we have this one queued for 4.9? I can't take it in my tree >> >> if you want, but it's probably better if it's in the mtd tree. >> > >> > Applied this patch to l2-mtd.git >> > >> >> I think this should also go into -stable. > > Why? Do you have real use cases that are broken by this? I understand I do, some code adding partitions on a gluebi master. > this is a problem, but I'm curious on how this satisfies the stable > rules. > > Also, note that this isn't a regression; it's been broken forever and > apparently no one noticed. IMO that raises the bar a bit (but not > impossibly so) for -stable. > I just encountered the bug yesterday and yes it is obvious it has been broken forever. I don't have strong opinion about these things so no worries. Karl > Anyway, if we decide to do this, you'll also want to include the git > hash and applicable kernel versions, per Option 2 [1]. > > Brian > > [1] Documentation/stable_kernel_rules.txt.
On 15.12.2016 08:09, Karl Beldan wrote: >>> I think this should also go into -stable. >> >> Why? Do you have real use cases that are broken by this? I understand > > I do, some code adding partitions on a gluebi master. What exactly are you doing? >> this is a problem, but I'm curious on how this satisfies the stable >> rules. >> >> Also, note that this isn't a regression; it's been broken forever and >> apparently no one noticed. IMO that raises the bar a bit (but not >> impossibly so) for -stable. >> > > I just encountered the bug yesterday and yes it is obvious it has been > broken forever. > I don't have strong opinion about these things so no worries. If existing stuff is broken, and you can trigger it. Please let us know. Then it should go into -stable. Thanks, //richard
On Thu, Dec 15, 2016 at 08:51:06AM +0100, Richard Weinberger wrote: > On 15.12.2016 08:09, Karl Beldan wrote: > >>> I think this should also go into -stable. > >> > >> Why? Do you have real use cases that are broken by this? I understand > > > > I do, some code adding partitions on a gluebi master. > > What exactly are you doing? > > >> this is a problem, but I'm curious on how this satisfies the stable > >> rules. > >> > >> Also, note that this isn't a regression; it's been broken forever and > >> apparently no one noticed. IMO that raises the bar a bit (but not > >> impossibly so) for -stable. > >> > > > > I just encountered the bug yesterday and yes it is obvious it has been > > broken forever. > > I don't have strong opinion about these things so no worries. > > If existing stuff is broken, and you can trigger it. Please let us > know. Then it should go into -stable. > I thought that's what I already did. Anyways, it didn't require much imagination to come up with a script triggering the issue so here you are: #{{ modprobe nandsim modprobe gluebi ubiattach -p /dev/mtd1 -b 1 ubimkvol /dev/ubi0 -S 4 -N vol_0 mtdpart add /dev/mtd2 vol_0_0 0 0x1000 tail -F /dev/mtd2 & while :; do dd bs=1 count=1 if=/dev/mtd3 >/dev/null 2>&1 || break; done kill -9 %1 # Oops #}} Karl
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 1f13e32..ec852fa 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -317,6 +317,18 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs) return res; } +static int part_get_device(struct mtd_info *mtd) +{ + struct mtd_part *part = mtd_to_part(mtd); + return part->master->_get_device(part->master); +} + +static void part_put_device(struct mtd_info *mtd) +{ + struct mtd_part *part = mtd_to_part(mtd); + part->master->_put_device(part->master); +} + static int part_ooblayout_ecc(struct mtd_info *mtd, int section, struct mtd_oob_region *oobregion) { @@ -463,6 +475,12 @@ static struct mtd_part *allocate_partition(struct mtd_info *master, slave->mtd._block_isbad = part_block_isbad; if (master->_block_markbad) slave->mtd._block_markbad = part_block_markbad; + + if (master->_get_device) + slave->mtd._get_device = part_get_device; + if (master->_put_device) + slave->mtd._put_device = part_put_device; + slave->mtd._erase = part_erase; slave->master = master; slave->offset = part->offset;