Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1288319 > unrolled thread

[PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-10 09:10 +0100
    Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Brian Norris <computersforpeace@gmail.com> - 2015-12-11 01:50 +0100
      Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-11 15:00 +0100
        Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Dan Carpenter <dan.carpenter@oracle.com> - 2015-12-11 15:50 +0100
          Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-11 16:20 +0100
      Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-11 23:10 +0100
        Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Brian Norris <computersforpeace@gmail.com> - 2015-12-11 23:20 +0100
    [PATCH v5 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-11 15:10 +0100
      Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-11 16:20 +0100
        Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Dinh Nguyen <dinh.linux@gmail.com> - 2015-12-11 18:00 +0100
          Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-11 18:10 +0100
            Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release()  call in denali_remove() Dinh Nguyen <dinh.linux@gmail.com> - 2015-12-11 18:30 +0100

#1288319 — [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Unregister the NAND device from the NAND subsystem when removing a denali
NAND controller, otherwise the MTD attached to the NAND device is still
exposed by the MTD layer, and accesses to this device will likely crash
the system.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: <stable@vger.kernel.org> #3.8+
Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
---
 drivers/mtd/nand/denali.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 67eb2be..8feece3 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1622,6 +1622,7 @@ EXPORT_SYMBOL(denali_init);
 /* driver exit point */
 void denali_remove(struct denali_nand_info *denali)
 {
+	nand_release(&denali->mtd);
 	denali_irq_cleanup(denali->irq, denali);
 	dma_unmap_single(denali->dev, denali->buf.dma_buf,
 			 denali->mtd.writesize + denali->mtd.oobsize,
-- 
2.1.4

--
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/

[toc] | [next] | [standalone]

#1289034 — Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> Unregister the NAND device from the NAND subsystem when removing a denali
> NAND controller, otherwise the MTD attached to the NAND device is still
> exposed by the MTD layer, and accesses to this device will likely crash
> the system.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: <stable@vger.kernel.org> #3.8+

Does this follow these rules, from
Documentation/stable_kernel_rules.txt?

 - It must be obviously correct and tested.

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).

> Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
> ---
>  drivers/mtd/nand/denali.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 67eb2be..8feece3 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1622,6 +1622,7 @@ EXPORT_SYMBOL(denali_init);
>  /* driver exit point */
>  void denali_remove(struct denali_nand_info *denali)
>  {
> +	nand_release(&denali->mtd);
>  	denali_irq_cleanup(denali->irq, denali);
>  	dma_unmap_single(denali->dev, denali->buf.dma_buf,
>  			 denali->mtd.writesize + denali->mtd.oobsize,

It feels a bit odd to allow usage of MTD fields after it has been
unregistered. Maybe precompute this before the nand_release()?

Brian
--
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/

[toc] | [prev] | [next] | [standalone]

#1289592 — Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Hi Brian,

On Thu, 10 Dec 2015 16:40:08 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> > Unregister the NAND device from the NAND subsystem when removing a denali
> > NAND controller, otherwise the MTD attached to the NAND device is still
> > exposed by the MTD layer, and accesses to this device will likely crash
> > the system.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: <stable@vger.kernel.org> #3.8+
> 
> Does this follow these rules, from
> Documentation/stable_kernel_rules.txt?
> 
>  - It must be obviously correct and tested.
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).

As you wish, I'll remove those Cc and Fixes tags, or just drop the
patch if you think it's useless...
I just noticed the bug while reworking this series, and thought it
would be useful to fix it, but I honestly don't care if it's applied
or not (I don't use this platform).

> 
> > Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
> > ---
> >  drivers/mtd/nand/denali.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> > index 67eb2be..8feece3 100644
> > --- a/drivers/mtd/nand/denali.c
> > +++ b/drivers/mtd/nand/denali.c
> > @@ -1622,6 +1622,7 @@ EXPORT_SYMBOL(denali_init);
> >  /* driver exit point */
> >  void denali_remove(struct denali_nand_info *denali)
> >  {
> > +	nand_release(&denali->mtd);
> >  	denali_irq_cleanup(denali->irq, denali);
> >  	dma_unmap_single(denali->dev, denali->buf.dma_buf,
> >  			 denali->mtd.writesize + denali->mtd.oobsize,
> 
> It feels a bit odd to allow usage of MTD fields after it has been
> unregistered. Maybe precompute this before the nand_release()?

nand_realease() is not releasing the mtd instance or re-initialazing
its field, so it should be safe, but I agree that pre-computing the DMA
buffer size is more future-proof.

I'll fix that, send a v5 and let you decide whether it's needed or not.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1289642 — Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

On Fri, Dec 11, 2015 at 02:53:20PM +0100, Boris Brezillon wrote:
> Hi Brian,
> 
> On Thu, 10 Dec 2015 16:40:08 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> > > Unregister the NAND device from the NAND subsystem when removing a denali
> > > NAND controller, otherwise the MTD attached to the NAND device is still
> > > exposed by the MTD layer, and accesses to this device will likely crash
> > > the system.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Cc: <stable@vger.kernel.org> #3.8+
> > 
> > Does this follow these rules, from
> > Documentation/stable_kernel_rules.txt?
> > 
> >  - It must be obviously correct and tested.
> > 
> >  - It must fix a real bug that bothers people (not a, "This could be a
> >    problem..." type thing).
> 
> As you wish, I'll remove those Cc and Fixes tags, or just drop the
> patch if you think it's useless...

The fixes tag is a separate thing from CCing stable.  It's useful on by
itself.  I always put the person who wrote the original patch in the To:
header so they can review and comment if I have made a mistake.

regards,
dan carpenter

--
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/

[toc] | [prev] | [next] | [standalone]

#1289674 — Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Hi Dan,

On Fri, 11 Dec 2015 17:39:47 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Fri, Dec 11, 2015 at 02:53:20PM +0100, Boris Brezillon wrote:
> > Hi Brian,
> > 
> > On Thu, 10 Dec 2015 16:40:08 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> > 
> > > On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> > > > Unregister the NAND device from the NAND subsystem when removing a denali
> > > > NAND controller, otherwise the MTD attached to the NAND device is still
> > > > exposed by the MTD layer, and accesses to this device will likely crash
> > > > the system.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > Cc: <stable@vger.kernel.org> #3.8+
> > > 
> > > Does this follow these rules, from
> > > Documentation/stable_kernel_rules.txt?
> > > 
> > >  - It must be obviously correct and tested.
> > > 
> > >  - It must fix a real bug that bothers people (not a, "This could be a
> > >    problem..." type thing).
> > 
> > As you wish, I'll remove those Cc and Fixes tags, or just drop the
> > patch if you think it's useless...
> 
> The fixes tag is a separate thing from CCing stable.  It's useful on by
> itself.  I always put the person who wrote the original patch in the To:
> header so they can review and comment if I have made a mistake.

Noted. I added back the Fixes tag and added Dinh Nguyen (the commit
author) in the loop.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1289945 — Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Hi Brian,

On Thu, 10 Dec 2015 16:40:08 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> > Unregister the NAND device from the NAND subsystem when removing a denali
> > NAND controller, otherwise the MTD attached to the NAND device is still
> > exposed by the MTD layer, and accesses to this device will likely crash
> > the system.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: <stable@vger.kernel.org> #3.8+
> 
> Does this follow these rules, from
> Documentation/stable_kernel_rules.txt?
> 
>  - It must be obviously correct and tested.
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).

Sorry to bring the "stable or not stable (that is the question :-))"
debate back, but after thinking a bit more about the implications of
this missing nand_release() call, I think it is worth backporting the
fix to all stable kernels.
The reason is, it can potentially introduce a security hole, because if
the mtd device is not unregister but the underlying mtd object is freed
and the kernel reuses the same memory region for a different object,
the MTD layer will possibly call one of the mtd->_method() function,
and this field might point to another completely different function.

You'll say that denali devices are probably never removed and this is
the reason why people have never seen this problem before, which would
be a good reason to not bother backporting the patch.
But, given that the driver can be compiled as a module (the user can
possibly load/unload it, which will in turn create/destroy the
NAND/MTD device), and that the denali controller can be exposed through
a PCI bus (which, AFAIK is hotpluggable), I really think this fix
should be sent to stable.

Best Regards,

Boris

> 
> > Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
> > ---
> >  drivers/mtd/nand/denali.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> > index 67eb2be..8feece3 100644
> > --- a/drivers/mtd/nand/denali.c
> > +++ b/drivers/mtd/nand/denali.c
> > @@ -1622,6 +1622,7 @@ EXPORT_SYMBOL(denali_init);
> >  /* driver exit point */
> >  void denali_remove(struct denali_nand_info *denali)
> >  {
> > +	nand_release(&denali->mtd);
> >  	denali_irq_cleanup(denali->irq, denali);
> >  	dma_unmap_single(denali->dev, denali->buf.dma_buf,
> >  			 denali->mtd.writesize + denali->mtd.oobsize,
> 
> It feels a bit odd to allow usage of MTD fields after it has been
> unregistered. Maybe precompute this before the nand_release()?
> 
> Brian



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1289946 — Re: [PATCH v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Hi Boris,

On Fri, Dec 11, 2015 at 11:03:05PM +0100, Boris Brezillon wrote:
> On Thu, 10 Dec 2015 16:40:08 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> > On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> > > Unregister the NAND device from the NAND subsystem when removing a denali
> > > NAND controller, otherwise the MTD attached to the NAND device is still
> > > exposed by the MTD layer, and accesses to this device will likely crash
> > > the system.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Cc: <stable@vger.kernel.org> #3.8+
> > 
> > Does this follow these rules, from
> > Documentation/stable_kernel_rules.txt?
> > 
> >  - It must be obviously correct and tested.
> > 
> >  - It must fix a real bug that bothers people (not a, "This could be a
> >    problem..." type thing).
> 
> Sorry to bring the "stable or not stable (that is the question :-))"
> debate back, but after thinking a bit more about the implications of
> this missing nand_release() call, I think it is worth backporting the
> fix to all stable kernels.
> The reason is, it can potentially introduce a security hole, because if
> the mtd device is not unregister but the underlying mtd object is freed
> and the kernel reuses the same memory region for a different object,
> the MTD layer will possibly call one of the mtd->_method() function,
> and this field might point to another completely different function.
> 
> You'll say that denali devices are probably never removed and this is
> the reason why people have never seen this problem before, which would
> be a good reason to not bother backporting the patch.
> But, given that the driver can be compiled as a module (the user can
> possibly load/unload it, which will in turn create/destroy the
> NAND/MTD device), and that the denali controller can be exposed through
> a PCI bus (which, AFAIK is hotpluggable), I really think this fix
> should be sent to stable.

That's all well and good, but still nobody has told me they've tested
this.

I've pushed your v5 (+ comments, + ack) to l2-mtd.git. If it gets
testing and this request is made again at that point, we can easily send
it to stable after it hits Linus' tree. See option 2 in
Documentation/stable_kernel_rules.txt. You can even send the email
yourself, just CC me and anyone else relevant. I'll ack it if it's been
tested.

Regards,
Brian
--
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/

[toc] | [prev] | [next] | [standalone]

#1289602 — [PATCH v5 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Unregister the NAND device from the NAND subsystem when removing a denali
NAND controller, otherwise the MTD attached to the NAND device is still
exposed by the MTD layer, and accesses to this device will likely crash
the system.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Changes since v4:
- remove Cc stable and fixes tags
- calculate the dma buffer size before calling nand_release()

 drivers/mtd/nand/denali.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 67eb2be..fdfea05 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1622,9 +1622,11 @@ EXPORT_SYMBOL(denali_init);
 /* driver exit point */
 void denali_remove(struct denali_nand_info *denali)
 {
+	int bufsize = denali->mtd.writesize + denali->mtd.oobsize;
+
+	nand_release(&denali->mtd);
 	denali_irq_cleanup(denali->irq, denali);
-	dma_unmap_single(denali->dev, denali->buf.dma_buf,
-			 denali->mtd.writesize + denali->mtd.oobsize,
+	dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize,
 			 DMA_BIDIRECTIONAL);
 }
 EXPORT_SYMBOL(denali_remove);
-- 
2.1.4

--
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/

[toc] | [prev] | [next] | [standalone]

#1289669 — Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

+ Dinh (who made commit 2a0a288ec258)

Also added back the Fixes tag.

On Fri, 11 Dec 2015 15:02:34 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Unregister the NAND device from the NAND subsystem when removing a denali
> NAND controller, otherwise the MTD attached to the NAND device is still
> exposed by the MTD layer, and accesses to this device will likely crash
> the system.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")

> ---
> Changes since v4:
> - remove Cc stable and fixes tags
> - calculate the dma buffer size before calling nand_release()
> 
>  drivers/mtd/nand/denali.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 67eb2be..fdfea05 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1622,9 +1622,11 @@ EXPORT_SYMBOL(denali_init);
>  /* driver exit point */
>  void denali_remove(struct denali_nand_info *denali)
>  {
> +	int bufsize = denali->mtd.writesize + denali->mtd.oobsize;
> +
> +	nand_release(&denali->mtd);
>  	denali_irq_cleanup(denali->irq, denali);
> -	dma_unmap_single(denali->dev, denali->buf.dma_buf,
> -			 denali->mtd.writesize + denali->mtd.oobsize,
> +	dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize,
>  			 DMA_BIDIRECTIONAL);
>  }
>  EXPORT_SYMBOL(denali_remove);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1289733 — Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Hi Boris,

On Fri, Dec 11, 2015 at 9:10 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> + Dinh (who made commit 2a0a288ec258)
>
> Also added back the Fixes tag.
>
> On Fri, 11 Dec 2015 15:02:34 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>
>> Unregister the NAND device from the NAND subsystem when removing a denali
>> NAND controller, otherwise the MTD attached to the NAND device is still
>> exposed by the MTD layer, and accesses to this device will likely crash
>> the system.
>>
>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>
> Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
>
>> ---
>> Changes since v4:
>> - remove Cc stable and fixes tags
>> - calculate the dma buffer size before calling nand_release()
>>
>>  drivers/mtd/nand/denali.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> index 67eb2be..fdfea05 100644
>> --- a/drivers/mtd/nand/denali.c
>> +++ b/drivers/mtd/nand/denali.c
>> @@ -1622,9 +1622,11 @@ EXPORT_SYMBOL(denali_init);
>>  /* driver exit point */
>>  void denali_remove(struct denali_nand_info *denali)
>>  {
>> +     int bufsize = denali->mtd.writesize + denali->mtd.oobsize;
>> +
>> +     nand_release(&denali->mtd);
>>       denali_irq_cleanup(denali->irq, denali);
>> -     dma_unmap_single(denali->dev, denali->buf.dma_buf,
>> -                      denali->mtd.writesize + denali->mtd.oobsize,
>> +     dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize,
>>                        DMA_BIDIRECTIONAL);

Not sure what is the need to add bufsize here, but the commit message
doesn't reflect the change.

Dinh
--
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/

[toc] | [prev] | [next] | [standalone]

#1289744 — Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

Hi Dinh,

On Fri, 11 Dec 2015 10:50:21 -0600
Dinh Nguyen <dinh.linux@gmail.com> wrote:

> Hi Boris,
> 
> On Fri, Dec 11, 2015 at 9:10 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > + Dinh (who made commit 2a0a288ec258)
> >
> > Also added back the Fixes tag.
> >
> > On Fri, 11 Dec 2015 15:02:34 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >
> >> Unregister the NAND device from the NAND subsystem when removing a denali
> >> NAND controller, otherwise the MTD attached to the NAND device is still
> >> exposed by the MTD layer, and accesses to this device will likely crash
> >> the system.
> >>
> >> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >
> > Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
> >
> >> ---
> >> Changes since v4:
> >> - remove Cc stable and fixes tags
> >> - calculate the dma buffer size before calling nand_release()
> >>
> >>  drivers/mtd/nand/denali.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> >> index 67eb2be..fdfea05 100644
> >> --- a/drivers/mtd/nand/denali.c
> >> +++ b/drivers/mtd/nand/denali.c
> >> @@ -1622,9 +1622,11 @@ EXPORT_SYMBOL(denali_init);
> >>  /* driver exit point */
> >>  void denali_remove(struct denali_nand_info *denali)
> >>  {
> >> +     int bufsize = denali->mtd.writesize + denali->mtd.oobsize;
> >> +
> >> +     nand_release(&denali->mtd);
> >>       denali_irq_cleanup(denali->irq, denali);
> >> -     dma_unmap_single(denali->dev, denali->buf.dma_buf,
> >> -                      denali->mtd.writesize + denali->mtd.oobsize,
> >> +     dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize,
> >>                        DMA_BIDIRECTIONAL);
> 
> Not sure what is the need to add bufsize here, but the commit message
> doesn't reflect the change.

You were not in Cc of the first version (my fault), but Brian pointed
that the mtd fields could be in an unknown state after the
nand_release() call (this is currently not the case, but it change in
the future). The idea is to pre-compute the DMA buffer size before
releasing the mtd/nand device to prevent any future issues.

I don't think it is worth mentioning this in the commit message,
because these are just implementation details, but I can add the
following comment before the bufsize declaration:

	/*
	 * Pre-compute DMA buffer size to avoid any problems in case
	 * nand_release() ever changes in a way that mtd->writesize and
	 * mtd->oobsize are not reliable after this call.
	 */

What do you think?

Best Regards,

Boris

> 
> Dinh



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1289761 — Re: [PATCH v5 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

On Fri, Dec 11, 2015 at 11:08 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Dinh,
>
> On Fri, 11 Dec 2015 10:50:21 -0600
> Dinh Nguyen <dinh.linux@gmail.com> wrote:
>
>> Hi Boris,
>>
>> On Fri, Dec 11, 2015 at 9:10 AM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > + Dinh (who made commit 2a0a288ec258)
>> >
>> > Also added back the Fixes tag.
>> >
>> > On Fri, 11 Dec 2015 15:02:34 +0100
>> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> >
>> >> Unregister the NAND device from the NAND subsystem when removing a denali
>> >> NAND controller, otherwise the MTD attached to the NAND device is still
>> >> exposed by the MTD layer, and accesses to this device will likely crash
>> >> the system.
>> >>
>> >> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> >
>> > Fixes: 2a0a288ec258 ("mtd: denali: split the generic driver and PCI layer")
>> >
>> >> ---
>> >> Changes since v4:
>> >> - remove Cc stable and fixes tags
>> >> - calculate the dma buffer size before calling nand_release()
>> >>
>> >>  drivers/mtd/nand/denali.c | 6 ++++--
>> >>  1 file changed, 4 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> >> index 67eb2be..fdfea05 100644
>> >> --- a/drivers/mtd/nand/denali.c
>> >> +++ b/drivers/mtd/nand/denali.c
>> >> @@ -1622,9 +1622,11 @@ EXPORT_SYMBOL(denali_init);
>> >>  /* driver exit point */
>> >>  void denali_remove(struct denali_nand_info *denali)
>> >>  {
>> >> +     int bufsize = denali->mtd.writesize + denali->mtd.oobsize;
>> >> +
>> >> +     nand_release(&denali->mtd);
>> >>       denali_irq_cleanup(denali->irq, denali);
>> >> -     dma_unmap_single(denali->dev, denali->buf.dma_buf,
>> >> -                      denali->mtd.writesize + denali->mtd.oobsize,
>> >> +     dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize,
>> >>                        DMA_BIDIRECTIONAL);
>>
>> Not sure what is the need to add bufsize here, but the commit message
>> doesn't reflect the change.
>
> You were not in Cc of the first version (my fault), but Brian pointed
> that the mtd fields could be in an unknown state after the
> nand_release() call (this is currently not the case, but it change in
> the future). The idea is to pre-compute the DMA buffer size before
> releasing the mtd/nand device to prevent any future issues.
>
> I don't think it is worth mentioning this in the commit message,
> because these are just implementation details, but I can add the
> following comment before the bufsize declaration:
>
>         /*
>          * Pre-compute DMA buffer size to avoid any problems in case
>          * nand_release() ever changes in a way that mtd->writesize and
>          * mtd->oobsize are not reliable after this call.
>          */
>
> What do you think?
>

Ah, perfect! With the updated comment,

Acked-by: Dinh Nguyen <dinguyen@opensource.altera.com>

Thanks,
Dinh
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web