[1/5] MTD: mtdram: properly handle the phys argument in the point method

Message ID 20171011032621.26979-2-nicolas.pitre@linaro.org
State Superseded
Headers show
Series
  • unconfuse get_unmapped_area and point/unpoint driver methods
Related show

Commit Message

Nicolas Pitre Oct. 11, 2017, 3:26 a.m.
When the phys pointer is non null, the point method is expected to return
the physical address for the pointed area. In the case of the mtdram
driver we have to retrieve the physical address for the corresponding
vmalloc area. However, there is no guarantee that the vmalloc area is
made of physically contiguous pages. In that case we simply limit retlen
to the actually contiguous pages.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 drivers/mtd/devices/mtdram.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Richard Weinberger Oct. 11, 2017, 8:47 p.m. | #1
On Wed, Oct 11, 2017 at 5:26 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> When the phys pointer is non null, the point method is expected to return
> the physical address for the pointed area. In the case of the mtdram
> driver we have to retrieve the physical address for the corresponding
> vmalloc area. However, there is no guarantee that the vmalloc area is
> made of physically contiguous pages. In that case we simply limit retlen
> to the actually contiguous pages.
>
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  drivers/mtd/devices/mtdram.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> index cbd8547d7a..7dbee8e62f 100644
> --- a/drivers/mtd/devices/mtdram.c
> +++ b/drivers/mtd/devices/mtdram.c
> @@ -13,6 +13,7 @@
>  #include <linux/slab.h>
>  #include <linux/ioport.h>
>  #include <linux/vmalloc.h>
> +#include <linux/mm.h>
>  #include <linux/init.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/mtdram.h>
> @@ -69,6 +70,27 @@ static int ram_point(struct mtd_info *mtd, loff_t from, size_t len,
>  {
>         *virt = mtd->priv + from;
>         *retlen = len;
> +
> +       if (phys) {
> +               /* limit retlen to the number of contiguous physical pages */
> +               unsigned long page_ofs = offset_in_page(*virt);
> +               void *addr = *virt - page_ofs;
> +               unsigned long pfn1, pfn0 = vmalloc_to_pfn(addr);
> +
> +               *phys = __pfn_to_phys(pfn0) + page_ofs;
> +               len += page_ofs;
> +               while (len > PAGE_SIZE) {
> +                       len -= PAGE_SIZE;
> +                       addr += PAGE_SIZE;
> +                       pfn0++;
> +                       pfn1 = vmalloc_to_pfn(addr);
> +                       if (pfn1 != pfn0) {
> +                               *retlen = *virt - addr;
> +                               break;
> +                       }
> +               }
> +       }
> +
>         return 0;
>  }

Reviewed-by: Richard Weinberger <richard@nod.at>
Nicolas Pitre Oct. 11, 2017, 9:32 p.m. | #2
On Wed, 11 Oct 2017, Richard Weinberger wrote:

> On Wed, Oct 11, 2017 at 5:26 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > When the phys pointer is non null, the point method is expected to return
> > the physical address for the pointed area. In the case of the mtdram
> > driver we have to retrieve the physical address for the corresponding
> > vmalloc area. However, there is no guarantee that the vmalloc area is
> > made of physically contiguous pages. In that case we simply limit retlen
> > to the actually contiguous pages.
> >
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > ---
> >  drivers/mtd/devices/mtdram.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> > index cbd8547d7a..7dbee8e62f 100644
> > --- a/drivers/mtd/devices/mtdram.c
> > +++ b/drivers/mtd/devices/mtdram.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/ioport.h>
> >  #include <linux/vmalloc.h>
> > +#include <linux/mm.h>
> >  #include <linux/init.h>
> >  #include <linux/mtd/mtd.h>
> >  #include <linux/mtd/mtdram.h>
> > @@ -69,6 +70,27 @@ static int ram_point(struct mtd_info *mtd, loff_t from, size_t len,
> >  {
> >         *virt = mtd->priv + from;
> >         *retlen = len;
> > +
> > +       if (phys) {
> > +               /* limit retlen to the number of contiguous physical pages */
> > +               unsigned long page_ofs = offset_in_page(*virt);
> > +               void *addr = *virt - page_ofs;
> > +               unsigned long pfn1, pfn0 = vmalloc_to_pfn(addr);
> > +
> > +               *phys = __pfn_to_phys(pfn0) + page_ofs;
> > +               len += page_ofs;
> > +               while (len > PAGE_SIZE) {
> > +                       len -= PAGE_SIZE;
> > +                       addr += PAGE_SIZE;
> > +                       pfn0++;
> > +                       pfn1 = vmalloc_to_pfn(addr);
> > +                       if (pfn1 != pfn0) {
> > +                               *retlen = *virt - addr;

In fact the above should be: *retlen = addr - *virt;

Let me know if I should repost.


Nicolas
Richard Weinberger Oct. 11, 2017, 9:40 p.m. | #3
On Wed, Oct 11, 2017 at 11:32 PM, Nicolas Pitre
<nicolas.pitre@linaro.org> wrote:
> In fact the above should be: *retlen = addr - *virt;

Hmm, yes.

While we are here, how did you test that code?

> Let me know if I should repost.

Yes, please. But wait a few days, hopefully we get more reviews.
Nicolas Pitre Oct. 11, 2017, 9:53 p.m. | #4
On Wed, 11 Oct 2017, Richard Weinberger wrote:

> On Wed, Oct 11, 2017 at 11:32 PM, Nicolas Pitre
> <nicolas.pitre@linaro.org> wrote:
> > In fact the above should be: *retlen = addr - *virt;
> 
> Hmm, yes.
> 
> While we are here, how did you test that code?

I didn't actually manage to trigger it as physical pages were always 
contiguous in my testing. So this is from inspection i.e. I just re-read 
the code from your reply.

> > Let me know if I should repost.
> 
> Yes, please. But wait a few days, hopefully we get more reviews.

Sure.


Nicolas

Patch

diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
index cbd8547d7a..7dbee8e62f 100644
--- a/drivers/mtd/devices/mtdram.c
+++ b/drivers/mtd/devices/mtdram.c
@@ -13,6 +13,7 @@ 
 #include <linux/slab.h>
 #include <linux/ioport.h>
 #include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/mtdram.h>
@@ -69,6 +70,27 @@  static int ram_point(struct mtd_info *mtd, loff_t from, size_t len,
 {
 	*virt = mtd->priv + from;
 	*retlen = len;
+
+	if (phys) {
+		/* limit retlen to the number of contiguous physical pages */
+		unsigned long page_ofs = offset_in_page(*virt);
+		void *addr = *virt - page_ofs;
+		unsigned long pfn1, pfn0 = vmalloc_to_pfn(addr);
+
+		*phys = __pfn_to_phys(pfn0) + page_ofs;
+		len += page_ofs;
+		while (len > PAGE_SIZE) {
+			len -= PAGE_SIZE;
+			addr += PAGE_SIZE;
+			pfn0++;
+			pfn1 = vmalloc_to_pfn(addr);
+			if (pfn1 != pfn0) {
+				*retlen = *virt - addr;
+				break;
+			}
+		}
+	}
+
 	return 0;
 }