| Message ID | 1416835236-25185-4-git-send-email-richard@nod.at |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, 2014-11-24 at 14:20 +0100, Richard Weinberger wrote: > ...otherwise the deferred work might run after datastructures > got freed and corrupt memory. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/ubi/wl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c > index 7f135df..cb2e571 100644 > --- a/drivers/mtd/ubi/wl.c > +++ b/drivers/mtd/ubi/wl.c > @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) > void ubi_wl_close(struct ubi_device *ubi) > { > dbg_wl("close the WL sub-system"); > +#ifdef CONFIG_MTD_UBI_FASTMAP > + flush_work(&ubi->fm_work); > +#endif If you are using the work infrastructure implemented in wl.c, then fastmap work should be no different to any other work. And we do flush all works in 'shutdown_work()'. The fastmap work should be flushed there too. I think we discussed this already - there should be one single queue of works, managed by the same set of functions, all flushed in the same place, one-by-one... Obviously, there is some misunderstanding. This looks like lack of separation and misuse of layering. I am missing explanations why I am wrong...
Am 27.11.2014 um 16:38 schrieb Artem Bityutskiy: > On Mon, 2014-11-24 at 14:20 +0100, Richard Weinberger wrote: >> ...otherwise the deferred work might run after datastructures >> got freed and corrupt memory. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> drivers/mtd/ubi/wl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >> index 7f135df..cb2e571 100644 >> --- a/drivers/mtd/ubi/wl.c >> +++ b/drivers/mtd/ubi/wl.c >> @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) >> void ubi_wl_close(struct ubi_device *ubi) >> { >> dbg_wl("close the WL sub-system"); >> +#ifdef CONFIG_MTD_UBI_FASTMAP >> + flush_work(&ubi->fm_work); >> +#endif > > If you are using the work infrastructure implemented in wl.c, then > fastmap work should be no different to any other work. And we do flush > all works in 'shutdown_work()'. The fastmap work should be flushed there > too. > > I think we discussed this already - there should be one single queue of > works, managed by the same set of functions, all flushed in the same > place, one-by-one... > > Obviously, there is some misunderstanding. This looks like lack of > separation and misuse of layering. I am missing explanations why I am > wrong... So you want me to use the UBI WL background thread for the scheduled fastmap work? I didn't do it that way because you said more than once that fastmap is fastmap and WL is WL. Therefore I've separated it. Thanks, //richard
On Thu, 2014-11-27 at 17:08 +0100, Richard Weinberger wrote: > > Obviously, there is some misunderstanding. This looks like lack of > > separation and misuse of layering. I am missing explanations why I am > > wrong... > > So you want me to use the UBI WL background thread for the scheduled fastmap work? No. It is more like either use it or do not use it. > I didn't do it that way because you said more than once that fastmap is fastmap and > WL is WL. Therefore I've separated it. And "separated" meaning adding this code to wl.c? +#ifdef CONFIG_MTD_UBI_FASTMAP + flush_work(&ubi->fm_work); +#endif Could it be separated some more then?
Am 27.11.2014 um 17:29 schrieb Artem Bityutskiy: > On Thu, 2014-11-27 at 17:08 +0100, Richard Weinberger wrote: >>> Obviously, there is some misunderstanding. This looks like lack of >>> separation and misuse of layering. I am missing explanations why I am >>> wrong... >> >> So you want me to use the UBI WL background thread for the scheduled fastmap work? > > No. It is more like either use it or do not use it. Sorry, I don't understand. What do you want to do to? >> I didn't do it that way because you said more than once that fastmap is fastmap and >> WL is WL. Therefore I've separated it. > > And "separated" meaning adding this code to wl.c? > > +#ifdef CONFIG_MTD_UBI_FASTMAP > + flush_work(&ubi->fm_work); > +#endif > > Could it be separated some more then? > Of course, commit "UBI: Move fastmap specific functions out of wl.c" does. But this commit is *bugfix* commit. Thanks, //richard
On Thu, 2014-11-27 at 17:35 +0100, Richard Weinberger wrote: > Am 27.11.2014 um 17:29 schrieb Artem Bityutskiy: > > On Thu, 2014-11-27 at 17:08 +0100, Richard Weinberger wrote: > >>> Obviously, there is some misunderstanding. This looks like lack of > >>> separation and misuse of layering. I am missing explanations why I am > >>> wrong... > >> > >> So you want me to use the UBI WL background thread for the scheduled fastmap work? > > > > No. It is more like either use it or do not use it. > > Sorry, I don't understand. > What do you want to do to? Just keep the code structured. I am just asking questions and trying to to analyze your patches. If at some point I would like you to do something specific, I clearly state this. In this case I was complaining about fastmap specifics in an unrelated file, so basically the wish is to have it go away. How exactly - not specified, up to you :-) Or, this means just telling me why it is this way, justify. When I was working with this code, I did give people specific suggestions, line-by-line. Now I am more doing more of a sanity check, looking after the bigger picture. I understand that this is not a picture of an ideal maintainer, and I am not anymore an ideal maintainer for this stuff (I think I used to, though), simply because of lack of time. Doing the best effort job now. > >> I didn't do it that way because you said more than once that fastmap is fastmap and > >> WL is WL. Therefore I've separated it. > > > > And "separated" meaning adding this code to wl.c? > > > > +#ifdef CONFIG_MTD_UBI_FASTMAP > > + flush_work(&ubi->fm_work); > > +#endif > > > > Could it be separated some more then? > > > > Of course, commit "UBI: Move fastmap specific functions out of wl.c" does. I did not see it in this series. So you could tell this earlier, not after 2 e-mail exchanges. Do not assume I remember the details of our previous discussion. Assume I forgot everything :-) > But this commit is *bugfix* commit. I thought adding an close function to fastmap.c is a simple task.
Am 27.11.2014 um 17:47 schrieb Artem Bityutskiy: > On Thu, 2014-11-27 at 17:35 +0100, Richard Weinberger wrote: >> Am 27.11.2014 um 17:29 schrieb Artem Bityutskiy: >>> On Thu, 2014-11-27 at 17:08 +0100, Richard Weinberger wrote: >>>>> Obviously, there is some misunderstanding. This looks like lack of >>>>> separation and misuse of layering. I am missing explanations why I am >>>>> wrong... >>>> >>>> So you want me to use the UBI WL background thread for the scheduled fastmap work? >>> >>> No. It is more like either use it or do not use it. >> >> Sorry, I don't understand. >> What do you want to do to? > > Just keep the code structured. I am just asking questions and trying to > to analyze your patches. If at some point I would like you to do > something specific, I clearly state this. In this case I was complaining > about fastmap specifics in an unrelated file, so basically the wish is > to have it go away. How exactly - not specified, up to you :-) Or, this > means just telling me why it is this way, justify. > > When I was working with this code, I did give people specific > suggestions, line-by-line. Now I am more doing more of a sanity check, > looking after the bigger picture. > > I understand that this is not a picture of an ideal maintainer, and I am > not anymore an ideal maintainer for this stuff (I think I used to, > though), simply because of lack of time. Doing the best effort job now. This is perfectly fine. I'm trying hard to keep your job as easy as possible. >>>> I didn't do it that way because you said more than once that fastmap is fastmap and >>>> WL is WL. Therefore I've separated it. >>> >>> And "separated" meaning adding this code to wl.c? >>> >>> +#ifdef CONFIG_MTD_UBI_FASTMAP >>> + flush_work(&ubi->fm_work); >>> +#endif >>> >>> Could it be separated some more then? >>> >> >> Of course, commit "UBI: Move fastmap specific functions out of wl.c" does. > > I did not see it in this series. So you could tell this earlier, not > after 2 e-mail exchanges. Do not assume I remember the details of our > previous discussion. Assume I forgot everything :-) You did not see it in that series because you asked me to split it. The massive clean stuff comes after the fixes. This is the branch where I keep the whole series. https://git.kernel.org/cgit/linux/kernel/git/rw/misc.git/log/?h=fastmap_upgrade2 Right now you've seen 6 out of 40 patches. Maybe I'll change a few commits before submitting them. I have some new ideas for more cleanups. :) >> But this commit is *bugfix* commit. > > I thought adding an close function to fastmap.c is a simple task. As I said, later in the series I clean up a lot. Thanks, //richard
On 11/27/2014 5:38 PM, Artem Bityutskiy wrote: > On Mon, 2014-11-24 at 14:20 +0100, Richard Weinberger wrote: >> ...otherwise the deferred work might run after datastructures >> got freed and corrupt memory. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> drivers/mtd/ubi/wl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >> index 7f135df..cb2e571 100644 >> --- a/drivers/mtd/ubi/wl.c >> +++ b/drivers/mtd/ubi/wl.c >> @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) >> void ubi_wl_close(struct ubi_device *ubi) >> { >> dbg_wl("close the WL sub-system"); >> +#ifdef CONFIG_MTD_UBI_FASTMAP >> + flush_work(&ubi->fm_work); >> +#endif > > If you are using the work infrastructure implemented in wl.c, then > fastmap work should be no different to any other work. And we do flush > all works in 'shutdown_work()'. The fastmap work should be flushed there > too. I tend to agree with Artem. Could you please move it to shutdown_work()? I'm asking because I do want to pick up all the bug fixes patches, but not going to pick up the "cleaning up" patches at this point. > > I think we discussed this already - there should be one single queue of > works, managed by the same set of functions, all flushed in the same > place, one-by-one... > > Obviously, there is some misunderstanding. This looks like lack of > separation and misuse of layering. I am missing explanations why I am > wrong... > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > Thanks, Tanya Brokhman
Am 04.12.2014 um 17:44 schrieb Tanya Brokhman: > On 11/27/2014 5:38 PM, Artem Bityutskiy wrote: >> On Mon, 2014-11-24 at 14:20 +0100, Richard Weinberger wrote: >>> ...otherwise the deferred work might run after datastructures >>> got freed and corrupt memory. >>> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >>> --- >>> drivers/mtd/ubi/wl.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>> index 7f135df..cb2e571 100644 >>> --- a/drivers/mtd/ubi/wl.c >>> +++ b/drivers/mtd/ubi/wl.c >>> @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) >>> void ubi_wl_close(struct ubi_device *ubi) >>> { >>> dbg_wl("close the WL sub-system"); >>> +#ifdef CONFIG_MTD_UBI_FASTMAP >>> + flush_work(&ubi->fm_work); >>> +#endif >> >> If you are using the work infrastructure implemented in wl.c, then >> fastmap work should be no different to any other work. And we do flush >> all works in 'shutdown_work()'. The fastmap work should be flushed there >> too. > > I tend to agree with Artem. Could you please move it to shutdown_work()? I'm asking because I do want to pick up all the bug fixes patches, but not going to pick up the "cleaning > up" patches at this point. Okay. You have overruled me. :-) Thanks, //richard
On Mon, Nov 24, 2014 at 02:20:33PM +0100, Richard Weinberger wrote: > ...otherwise the deferred work might run after datastructures > got freed and corrupt memory. > > Signed-off-by: Richard Weinberger <richard@nod.at> After moving it to shutdown_work(): Reviewed-by: Guido MartÃnez <guido@vanguardiasur.com.ar> > --- > drivers/mtd/ubi/wl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c > index 7f135df..cb2e571 100644 > --- a/drivers/mtd/ubi/wl.c > +++ b/drivers/mtd/ubi/wl.c > @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) > void ubi_wl_close(struct ubi_device *ubi) > { > dbg_wl("close the WL sub-system"); > +#ifdef CONFIG_MTD_UBI_FASTMAP > + flush_work(&ubi->fm_work); > +#endif > shutdown_work(ubi); > protection_queue_destroy(ubi); > tree_destroy(&ubi->used); > -- > 1.8.4.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 11/24/2014 10:20 AM, Richard Weinberger wrote: > ...otherwise the deferred work might run after datastructures > got freed and corrupt memory. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/ubi/wl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c > index 7f135df..cb2e571 100644 > --- a/drivers/mtd/ubi/wl.c > +++ b/drivers/mtd/ubi/wl.c > @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) > void ubi_wl_close(struct ubi_device *ubi) > { > dbg_wl("close the WL sub-system"); > +#ifdef CONFIG_MTD_UBI_FASTMAP > + flush_work(&ubi->fm_work); > +#endif > shutdown_work(ubi); > protection_queue_destroy(ubi); > tree_destroy(&ubi->used); > IMHO, it's best to avoid nasty ifdefs like this (there are lots of ways of getting it cleaner). But I guess it's not a big deal.
Am 09.01.2015 um 22:32 schrieb Ezequiel Garcia: > On 11/24/2014 10:20 AM, Richard Weinberger wrote: >> ...otherwise the deferred work might run after datastructures >> got freed and corrupt memory. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> drivers/mtd/ubi/wl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >> index 7f135df..cb2e571 100644 >> --- a/drivers/mtd/ubi/wl.c >> +++ b/drivers/mtd/ubi/wl.c >> @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) >> void ubi_wl_close(struct ubi_device *ubi) >> { >> dbg_wl("close the WL sub-system"); >> +#ifdef CONFIG_MTD_UBI_FASTMAP >> + flush_work(&ubi->fm_work); >> +#endif >> shutdown_work(ubi); >> protection_queue_destroy(ubi); >> tree_destroy(&ubi->used); >> > > IMHO, it's best to avoid nasty ifdefs like this (there are lots of ways > of getting it cleaner). But I guess it's not a big deal. I agree that's why I've cleaned up the vast majority of all ifdefs in a later cleanup commit. My original plan was to have first pure bug fixes and then cleanups to make backporting of my patches easy. Thanks, //richard
On 01/09/2015 06:37 PM, Richard Weinberger wrote: > Am 09.01.2015 um 22:32 schrieb Ezequiel Garcia: >> On 11/24/2014 10:20 AM, Richard Weinberger wrote: >>> ...otherwise the deferred work might run after datastructures >>> got freed and corrupt memory. >>> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >>> --- >>> drivers/mtd/ubi/wl.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>> index 7f135df..cb2e571 100644 >>> --- a/drivers/mtd/ubi/wl.c >>> +++ b/drivers/mtd/ubi/wl.c >>> @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) >>> void ubi_wl_close(struct ubi_device *ubi) >>> { >>> dbg_wl("close the WL sub-system"); >>> +#ifdef CONFIG_MTD_UBI_FASTMAP >>> + flush_work(&ubi->fm_work); >>> +#endif >>> shutdown_work(ubi); >>> protection_queue_destroy(ubi); >>> tree_destroy(&ubi->used); >>> >> >> IMHO, it's best to avoid nasty ifdefs like this (there are lots of ways >> of getting it cleaner). But I guess it's not a big deal. > > I agree that's why I've cleaned up the vast majority of all ifdefs in a later cleanup > commit. My original plan was to have first pure bug fixes and then cleanups to make > backporting of my patches easy. > Ah, yes, backporting is a good point.
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 7f135df..cb2e571 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -2041,6 +2041,9 @@ static void protection_queue_destroy(struct ubi_device *ubi) void ubi_wl_close(struct ubi_device *ubi) { dbg_wl("close the WL sub-system"); +#ifdef CONFIG_MTD_UBI_FASTMAP + flush_work(&ubi->fm_work); +#endif shutdown_work(ubi); protection_queue_destroy(ubi); tree_destroy(&ubi->used);
...otherwise the deferred work might run after datastructures got freed and corrupt memory. Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/wl.c | 3 +++ 1 file changed, 3 insertions(+)