| Message ID | 20190411220056.19109-5-martin.blumenstingl@googlemail.com |
|---|---|
| State | Accepted |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | meson-nand: small code improvements | expand |
On 2019/4/12 6:00, Martin Blumenstingl wrote: > Documentation/scheduler/completion.txt states: > Calling init_completion() on the same completion object twice is > most likely a bug as it re-initializes the queue to an empty queue and > enqueued tasks could get "lost" - use reinit_completion() in that case, > but be aware of other races. > > Initialize nfc->completion in meson_nfc_probe using init_completion and > change the call in meson_nfc_queue_rb to reinit_completion so the logic > matches what the documentation suggests. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/mtd/nand/raw/meson_nand.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > index 57cc4bd3f665..ea57ddcec41e 100644 > --- a/drivers/mtd/nand/raw/meson_nand.c > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > cfg |= NFC_RB_IRQ_EN; > writel(cfg, nfc->reg_base + NFC_REG_CFG); > > - init_completion(&nfc->completion); > + reinit_completion(&nfc->completion); Tested-by:Liang Yang <liang.yang@amlogic.com> Acked-by: Liang Yang <liang.yang@amlogic.com> > > /* use the max erase time as the maximum clock for waiting R/B */ > cmd = NFC_CMD_RB | NFC_CMD_RB_INT > @@ -1380,6 +1380,7 @@ static int meson_nfc_probe(struct platform_device *pdev) > > nand_controller_init(&nfc->controller); > INIT_LIST_HEAD(&nfc->chips); > + init_completion(&nfc->completion); Tested-by:Liang Yang <liang.yang@amlogic.com> Acked-by: Liang Yang <liang.yang@amlogic.com> > > nfc->dev = dev; > >
Hi Liang, On Mon, Apr 15, 2019 at 8:04 AM Liang Yang <liang.yang@amlogic.com> wrote: > > > On 2019/4/12 6:00, Martin Blumenstingl wrote: > > Documentation/scheduler/completion.txt states: > > Calling init_completion() on the same completion object twice is > > most likely a bug as it re-initializes the queue to an empty queue and > > enqueued tasks could get "lost" - use reinit_completion() in that case, > > but be aware of other races. > > > > Initialize nfc->completion in meson_nfc_probe using init_completion and > > change the call in meson_nfc_queue_rb to reinit_completion so the logic > > matches what the documentation suggests. > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > --- > > drivers/mtd/nand/raw/meson_nand.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > > index 57cc4bd3f665..ea57ddcec41e 100644 > > --- a/drivers/mtd/nand/raw/meson_nand.c > > +++ b/drivers/mtd/nand/raw/meson_nand.c > > @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > > cfg |= NFC_RB_IRQ_EN; > > writel(cfg, nfc->reg_base + NFC_REG_CFG); > > > > - init_completion(&nfc->completion); > > + reinit_completion(&nfc->completion); > Tested-by:Liang Yang <liang.yang@amlogic.com> > Acked-by: Liang Yang <liang.yang@amlogic.com> thank you for reviewing and testing my patches! [...] > Tested-by:Liang Yang <liang.yang@amlogic.com> > Acked-by: Liang Yang <liang.yang@amlogic.com> please consider the following note for future code-reviews: most maintainers take the patch from patchwork and apply it to their git tree. however, patchwork is not smart enough to detect when the same Tested-by/Acked-by is sent multiple times. this results in the same Tested-by/Acked-by being listed multiple times in the final commit: [0] what I do instead is to reply with one set of Tested-by/Acked-by (below the author's Signed-off-by) which is then valid for the whole patch. There's no problem to have Tested-by and Acked-by at the same time, the issue only shows up if you send Acked-by (or any other tag) for the same patch multiple times. Have a great day! Regards, Martin [0] https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?h=nand/next&id=39e01956e2f70ff9f0e97db1a69c9847aa1d5d8b
Hi Martin, On 2019/4/19 3:44, Martin Blumenstingl wrote: > Hi Liang, > > On Mon, Apr 15, 2019 at 8:04 AM Liang Yang <liang.yang@amlogic.com> wrote: >> >> >> On 2019/4/12 6:00, Martin Blumenstingl wrote: >>> Documentation/scheduler/completion.txt states: >>> Calling init_completion() on the same completion object twice is >>> most likely a bug as it re-initializes the queue to an empty queue and >>> enqueued tasks could get "lost" - use reinit_completion() in that case, >>> but be aware of other races. >>> >>> Initialize nfc->completion in meson_nfc_probe using init_completion and >>> change the call in meson_nfc_queue_rb to reinit_completion so the logic >>> matches what the documentation suggests. >>> >>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >>> --- >>> drivers/mtd/nand/raw/meson_nand.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>> index 57cc4bd3f665..ea57ddcec41e 100644 >>> --- a/drivers/mtd/nand/raw/meson_nand.c >>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>> @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) >>> cfg |= NFC_RB_IRQ_EN; >>> writel(cfg, nfc->reg_base + NFC_REG_CFG); >>> >>> - init_completion(&nfc->completion); >>> + reinit_completion(&nfc->completion); >> Tested-by:Liang Yang <liang.yang@amlogic.com> >> Acked-by: Liang Yang <liang.yang@amlogic.com> > thank you for reviewing and testing my patches! > > [...] >> Tested-by:Liang Yang <liang.yang@amlogic.com> >> Acked-by: Liang Yang <liang.yang@amlogic.com> > please consider the following note for future code-reviews: > most maintainers take the patch from patchwork and apply it to their git tree. > however, patchwork is not smart enough to detect when the same > Tested-by/Acked-by is sent multiple times. > this results in the same Tested-by/Acked-by being listed multiple > times in the final commit: [0] > > what I do instead is to reply with one set of Tested-by/Acked-by > (below the author's Signed-off-by) which is then valid for the whole > patch. > There's no problem to have Tested-by and Acked-by at the same time, > the issue only shows up if you send Acked-by (or any other tag) for > the same patch multiple times. > Thanks. Well, I known about it now. > > Have a great day! > Regards, > Martin > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?h=nand/next&id=39e01956e2f70ff9f0e97db1a69c9847aa1d5d8b > > . >
Hi Martin, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote on Thu, 18 Apr 2019 21:44:05 +0200: > Hi Liang, > > On Mon, Apr 15, 2019 at 8:04 AM Liang Yang <liang.yang@amlogic.com> wrote: > > > > > > On 2019/4/12 6:00, Martin Blumenstingl wrote: > > > Documentation/scheduler/completion.txt states: > > > Calling init_completion() on the same completion object twice is > > > most likely a bug as it re-initializes the queue to an empty queue and > > > enqueued tasks could get "lost" - use reinit_completion() in that case, > > > but be aware of other races. > > > > > > Initialize nfc->completion in meson_nfc_probe using init_completion and > > > change the call in meson_nfc_queue_rb to reinit_completion so the logic > > > matches what the documentation suggests. > > > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > --- > > > drivers/mtd/nand/raw/meson_nand.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > > > index 57cc4bd3f665..ea57ddcec41e 100644 > > > --- a/drivers/mtd/nand/raw/meson_nand.c > > > +++ b/drivers/mtd/nand/raw/meson_nand.c > > > @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > > > cfg |= NFC_RB_IRQ_EN; > > > writel(cfg, nfc->reg_base + NFC_REG_CFG); > > > > > > - init_completion(&nfc->completion); > > > + reinit_completion(&nfc->completion); > > Tested-by:Liang Yang <liang.yang@amlogic.com> > > Acked-by: Liang Yang <liang.yang@amlogic.com> > thank you for reviewing and testing my patches! > > [...] > > Tested-by:Liang Yang <liang.yang@amlogic.com> > > Acked-by: Liang Yang <liang.yang@amlogic.com> > please consider the following note for future code-reviews: > most maintainers take the patch from patchwork and apply it to their git tree. > however, patchwork is not smart enough to detect when the same > Tested-by/Acked-by is sent multiple times. > this results in the same Tested-by/Acked-by being listed multiple > times in the final commit: [0] > > what I do instead is to reply with one set of Tested-by/Acked-by > (below the author's Signed-off-by) which is then valid for the whole > patch. > There's no problem to have Tested-by and Acked-by at the same time, > the issue only shows up if you send Acked-by (or any other tag) for > the same patch multiple times. Crap, I did not noticed that. Thanks for pointing it. I don't have time right now to fix it and send a new PR, I'll see in one week. Thanks, Miquèl
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index 57cc4bd3f665..ea57ddcec41e 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -400,7 +400,7 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) cfg |= NFC_RB_IRQ_EN; writel(cfg, nfc->reg_base + NFC_REG_CFG); - init_completion(&nfc->completion); + reinit_completion(&nfc->completion); /* use the max erase time as the maximum clock for waiting R/B */ cmd = NFC_CMD_RB | NFC_CMD_RB_INT @@ -1380,6 +1380,7 @@ static int meson_nfc_probe(struct platform_device *pdev) nand_controller_init(&nfc->controller); INIT_LIST_HEAD(&nfc->chips); + init_completion(&nfc->completion); nfc->dev = dev;
Documentation/scheduler/completion.txt states: Calling init_completion() on the same completion object twice is most likely a bug as it re-initializes the queue to an empty queue and enqueued tasks could get "lost" - use reinit_completion() in that case, but be aware of other races. Initialize nfc->completion in meson_nfc_probe using init_completion and change the call in meson_nfc_queue_rb to reinit_completion so the logic matches what the documentation suggests. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/mtd/nand/raw/meson_nand.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)