| Message ID | 20200813193400.105603-1-william.gray@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [BIONIC/linux-raspi2] mtd: nand: bcm2835-smi-nand: Pass a nand_chip object to nand_release() | expand |
Looks good, thanks. I would just change the title from: "mtd: nand: bcm2835-smi-nand: Pass a nand_chip object to nand_release()" to "UBUNTU: SAUCE: mtd: nand: bcm2835-smi-nand: Pass a nand_chip object to nand_release()" On 2020-08-13 15:34:00 , William Breathitt Gray wrote: > BugLink: https://bugs.launchpad.net/bugs/1886710 > > The nand_release() function takes a nand_chip object instead of a > mtd_info one. > > Signed-off-by: William Breathitt Gray <william.gray@canonical.com> > --- > drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c > index c4826ea1c2ba..4962cc743439 100644 > --- a/drivers/mtd/nand/bcm2835_smi_nand.c > +++ b/drivers/mtd/nand/bcm2835_smi_nand.c > @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) > if (!ret) > return 0; > > - nand_release(mtd); > + nand_release(mtd_to_nand(mtd)); > return -EINVAL; > } > > @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) > { > struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); > > - nand_release(&host->mtd); > + nand_release(mtd_to_nand(&host->mtd)); > > return 0; > } > -- > 2.25.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2020-08-13 15:34:00 , William Breathitt Gray wrote: > BugLink: https://bugs.launchpad.net/bugs/1886710 > > The nand_release() function takes a nand_chip object instead of a > mtd_info one. > > Signed-off-by: William Breathitt Gray <william.gray@canonical.com> Acked-by: Kelsey Skunberg <kelsey.skunberg@canonical.com> > --- > drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c > index c4826ea1c2ba..4962cc743439 100644 > --- a/drivers/mtd/nand/bcm2835_smi_nand.c > +++ b/drivers/mtd/nand/bcm2835_smi_nand.c > @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) > if (!ret) > return 0; > > - nand_release(mtd); > + nand_release(mtd_to_nand(mtd)); > return -EINVAL; > } > > @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) > { > struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); > > - nand_release(&host->mtd); > + nand_release(mtd_to_nand(&host->mtd)); > > return 0; > } > -- > 2.25.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Applied to bionic/raspi2 master-next. Thank you! -Kelsey On 2020-08-13 15:34:00 , William Breathitt Gray wrote: > BugLink: https://bugs.launchpad.net/bugs/1886710 > > The nand_release() function takes a nand_chip object instead of a > mtd_info one. > > Signed-off-by: William Breathitt Gray <william.gray@canonical.com> > --- > drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c > index c4826ea1c2ba..4962cc743439 100644 > --- a/drivers/mtd/nand/bcm2835_smi_nand.c > +++ b/drivers/mtd/nand/bcm2835_smi_nand.c > @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) > if (!ret) > return 0; > > - nand_release(mtd); > + nand_release(mtd_to_nand(mtd)); > return -EINVAL; > } > > @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) > { > struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); > > - nand_release(&host->mtd); > + nand_release(mtd_to_nand(&host->mtd)); > > return 0; > } > -- > 2.25.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 13.08.20 21:34, William Breathitt Gray wrote: > BugLink: https://bugs.launchpad.net/bugs/1886710 > > The nand_release() function takes a nand_chip object instead of a > mtd_info one. > > Signed-off-by: William Breathitt Gray <william.gray@canonical.com> > --- Wait a bit, let me understand this. This is not a normal change. If I understand this right, there was a stable patch which was applied without backport and missed in the test builds? In that case I would rather like to have the subject something like "UBUNTU: SAUCE: Fix argument to nand_release()" and refer to the stable patch in a "Fixes:" line in the sob area. Also explaining how this ended up being needed and why there is no upstream pick/backport. Right now this all looks more like a random patch without reason. And we also should apply this to the primary tree if it is wrong there. Though maybe it is something that is a clash between the upstream code and the vendor tree backport. In any case one wants to have more background info because in half a year nobody remembers. -Stefan > drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c > index c4826ea1c2ba..4962cc743439 100644 > --- a/drivers/mtd/nand/bcm2835_smi_nand.c > +++ b/drivers/mtd/nand/bcm2835_smi_nand.c > @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) > if (!ret) > return 0; > > - nand_release(mtd); > + nand_release(mtd_to_nand(mtd)); > return -EINVAL; > } > > @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) > { > struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); > > - nand_release(&host->mtd); > + nand_release(mtd_to_nand(&host->mtd)); > > return 0; > } >
On Fri, Aug 14, 2020 at 12:38 AM Stefan Bader <stefan.bader@canonical.com> wrote: > > On 13.08.20 21:34, William Breathitt Gray wrote: > > BugLink: https://bugs.launchpad.net/bugs/1886710 > > > > The nand_release() function takes a nand_chip object instead of a > > mtd_info one. > > > > Signed-off-by: William Breathitt Gray <william.gray@canonical.com> > > --- > > Wait a bit, let me understand this. This is not a normal change. If I understand > this right, there was a stable patch which was applied without backport and > missed in the test builds? In that case I would rather like to have the subject > something like "UBUNTU: SAUCE: Fix argument to nand_release()" and refer to the > stable patch in a "Fixes:" line in the sob area. Also explaining how this ended > up being needed and why there is no upstream pick/backport. > Right now this all looks more like a random patch without reason. And we also > should apply this to the primary tree if it is wrong there. Though maybe it is > something that is a clash between the upstream code and the vendor tree > backport. In any case one wants to have more background info because in half a > year nobody remembers. > > -Stefan Morning Stefan, I at least included the "UBUNTU: SAUCE:" when applying, I should have noted that in the applied email. I do agree this should have had more details added to the description.. sorry for not pushing for that. You can find the conversation around this patch in rake, though I'll see if I can sum it up at least. This patch is specific to B/raspi2 and is more of an addition to an upstream stable patch vs a fix. this upstream patch: 59ac276f2227 ("mtd: rawnand: Pass a nand_chip object to nand_release()") landed in B/linux without problems. When it applied to B/raspi2, the patch didn't make the same needed changes to the smi nand driver which appears to be specific to B/raspi2. You can find the patch that introduces smi nand into B/raspi2 here: c8dad9a4bb72 ("Add SMI NAND driver") William's patch extends the changes from the upstream patch to the smi nand driver. Without this patch B/raspi2 build test failed and it was confirmed to pass before applying. I realize this information isn't as useful here as it would have been in the commit message. From here I imagine it would be best to get an updated commit log onto the patch? Sorry again - I'll be more careful with what details to expect in patches in the future while reviewing. -Kelsey > > > drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c > > index c4826ea1c2ba..4962cc743439 100644 > > --- a/drivers/mtd/nand/bcm2835_smi_nand.c > > +++ b/drivers/mtd/nand/bcm2835_smi_nand.c > > @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) > > if (!ret) > > return 0; > > > > - nand_release(mtd); > > + nand_release(mtd_to_nand(mtd)); > > return -EINVAL; > > } > > > > @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) > > { > > struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); > > > > - nand_release(&host->mtd); > > + nand_release(mtd_to_nand(&host->mtd)); > > > > return 0; > > } > > > >
Probably too late, but... > Looks good, thanks. I would just change the title from: That's not a 'would', it's a 'must'. Patches not originating from upstream need to be prefixed with 'UBUNTU: SAUCE:' > "mtd: nand: bcm2835-smi-nand: Pass a nand_chip object to nand_release()" > > to > > "UBUNTU: SAUCE: mtd: nand: bcm2835-smi-nand: Pass a nand_chip object to nand_release()" > > > > > > > On 2020-08-13 15:34:00 , William Breathitt Gray wrote: > > BugLink: https://bugs.launchpad.net/bugs/1886710 > > > > The nand_release() function takes a nand_chip object instead of a > > mtd_info one. Hrm. Why? When was this introduced? How did we end up in this situation? That needs to be spelled out in the commit message otherwise we'll have no idea what's going on. Needless to say that nobody will remember why this is needed in a few weeks from now. Rather provide too much information in the commit message than none at all ;-) ...Juerg > > Signed-off-by: William Breathitt Gray <william.gray@canonical.com> > > --- > > drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c > > index c4826ea1c2ba..4962cc743439 100644 > > --- a/drivers/mtd/nand/bcm2835_smi_nand.c > > +++ b/drivers/mtd/nand/bcm2835_smi_nand.c > > @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) > > if (!ret) > > return 0; > > > > - nand_release(mtd); > > + nand_release(mtd_to_nand(mtd)); > > return -EINVAL; > > } > > > > @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) > > { > > struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); > > > > - nand_release(&host->mtd); > > + nand_release(mtd_to_nand(&host->mtd)); > > > > return 0; > > } > > -- > > 2.25.1 > > > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team >
diff --git a/drivers/mtd/nand/bcm2835_smi_nand.c b/drivers/mtd/nand/bcm2835_smi_nand.c index c4826ea1c2ba..4962cc743439 100644 --- a/drivers/mtd/nand/bcm2835_smi_nand.c +++ b/drivers/mtd/nand/bcm2835_smi_nand.c @@ -222,7 +222,7 @@ static int bcm2835_smi_nand_probe(struct platform_device *pdev) if (!ret) return 0; - nand_release(mtd); + nand_release(mtd_to_nand(mtd)); return -EINVAL; } @@ -230,7 +230,7 @@ static int bcm2835_smi_nand_remove(struct platform_device *pdev) { struct bcm2835_smi_nand_host *host = platform_get_drvdata(pdev); - nand_release(&host->mtd); + nand_release(mtd_to_nand(&host->mtd)); return 0; }
BugLink: https://bugs.launchpad.net/bugs/1886710 The nand_release() function takes a nand_chip object instead of a mtd_info one. Signed-off-by: William Breathitt Gray <william.gray@canonical.com> --- drivers/mtd/nand/bcm2835_smi_nand.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)