[PATCH] mtd: Allocate bdi objects dynamically

[PATCH] mtd: Allocate bdi objects dynamically

[Sandeep Jain]

From: Steve Longerbeam <steve_longerbeam@mentor.com>

The MTD backing dev info objects mtd_bdi was statically allocated.
So when MTD is built as a loadable module, this object fall in the
vmalloc address space.

The problem with that, is that the BDI APIs use wake_up_bit(), which calls
virt_to_page() to retrieve the memory zone of the page containing the
wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
highmem addresses.

Fix this by allocating the BDI objects dynamically with kmalloc. The
objects now fall in the logical address space so that BDI APIs will
work in all cases (mtd builtin or module).

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
---
 drivers/mtd/mtdcore.c |   30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e3936b8..9015b94 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -47,8 +47,7 @@
 
 #include "mtdcore.h"
 
-static struct backing_dev_info mtd_bdi = {
-};
+static struct backing_dev_info *mtd_bdi;
 
 #ifdef CONFIG_PM_SLEEP
 
@@ -397,7 +396,7 @@ int add_mtd_device(struct mtd_info *mtd)
 	if (WARN_ONCE(mtd->backing_dev_info, "MTD already registered\n"))
 		return -EEXIST;
 
-	mtd->backing_dev_info = &mtd_bdi;
+	mtd->backing_dev_info = mtd_bdi;
 
 	BUG_ON(mtd->writesize == 0);
 	mutex_lock(&mtd_table_mutex);
@@ -1668,18 +1667,20 @@ static const struct file_operations mtd_proc_ops = {
 /*====================================================================*/
 /* Init code */
 
-static int __init mtd_bdi_init(struct backing_dev_info *bdi, const char *name)
+static struct backing_dev_info * __init mtd_bdi_init(char *name)
 {
+	struct backing_dev_info *bdi;
 	int ret;
 
-	ret = bdi_init(bdi);
-	if (!ret)
-		ret = bdi_register(bdi, NULL, "%s", name);
+	bdi = kzalloc(sizeof(*bdi), GFP_KERNEL);
+	if (!bdi)
+		return ERR_PTR(-ENOMEM);
 
+	ret = bdi_setup_and_register(bdi, name);
 	if (ret)
-		bdi_destroy(bdi);
+		kfree(bdi);
 
-	return ret;
+	return ret ? ERR_PTR(ret) : bdi;
 }
 
 static struct proc_dir_entry *proc_mtd;
@@ -1692,9 +1693,11 @@ static int __init init_mtd(void)
 	if (ret)
 		goto err_reg;
 
-	ret = mtd_bdi_init(&mtd_bdi, "mtd");
-	if (ret)
+	mtd_bdi = mtd_bdi_init("mtd");
+	if (IS_ERR(mtd_bdi)) {
+		ret = PTR_ERR(mtd_bdi);
 		goto err_bdi;
+	}
 
 	proc_mtd = proc_create("mtd", 0, NULL, &mtd_proc_ops);
 
@@ -1707,6 +1710,8 @@ static int __init init_mtd(void)
 out_procfs:
 	if (proc_mtd)
 		remove_proc_entry("mtd", NULL);
+	bdi_destroy(mtd_bdi);
+	kfree(mtd_bdi);
 err_bdi:
 	class_unregister(&mtd_class);
 err_reg:
@@ -1720,7 +1725,8 @@ static void __exit cleanup_mtd(void)
 	if (proc_mtd)
 		remove_proc_entry("mtd", NULL);
 	class_unregister(&mtd_class);
-	bdi_destroy(&mtd_bdi);
+	bdi_destroy(mtd_bdi);
+	kfree(mtd_bdi);
 	idr_destroy(&mtd_idr);
 }
 
-- 
1.7.9.5

[PATCH v1 0/2] mtd: lock module while used

[Sandeep Jain]

*** BLURB HERE ***

Jim Baxter (1):
  mtd: Add hooks to call get and put on mtd devices.

Vladimir Zapolskiy (1):
  mtd: m25p80: lock module while used

 drivers/mtd/devices/m25p80.c |   15 +++++++++++++++
 drivers/mtd/mtdpart.c        |   18 ++++++++++++++++++
 2 files changed, 33 insertions(+)

-- 
1.7.9.5

[PATCH v1 1/2] mtd: m25p80: lock module while used

[Sandeep Jain]

From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

The change controls module users counter, which prevents to get
accidental oops on module unload while it is in use by mtd subsystem:

% dd if=/dev/mtd0 of=/dev/null &
% rmmod m25p80

Removing MTD device #0 (spi32766.0) with use count 1
Unable to handle kernel paging request at virtual address 7f4fb7f8
pgd = bd094000
[7f4fb7f8] *pgd=4cb66811, *pte=00000000, *ppte=00000000
Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
---
 drivers/mtd/devices/m25p80.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd..2eb1530 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -185,6 +185,19 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return ret;
 }
 
+static void m25p80_put(struct mtd_info *mtd)
+{
+	module_put(THIS_MODULE);
+}
+
+static int m25p80_get(struct mtd_info *mtd)
+{
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	return 0;
+}
+
 /*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
@@ -212,6 +225,8 @@ static int m25p_probe(struct spi_device *spi)
 	nor->write = m25p80_write;
 	nor->write_reg = m25p80_write_reg;
 	nor->read_reg = m25p80_read_reg;
+	nor->mtd._put_device = m25p80_put;
+	nor->mtd._get_device = m25p80_get;
 
 	nor->dev = &spi->dev;
 	spi_nor_set_flash_node(nor, spi->dev.of_node);
-- 
1.7.9.5

[PATCH v1 2/2] mtd: Add hooks to call get and put on mtd devices.

[Sandeep Jain]

From: Jim Baxter <jim_baxter@mentor.com>

Add hooks to mtdpart to call _get_device and _put_device,
this allows the mtd devices to perform internal
reference counting if required.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
---
 drivers/mtd/mtdpart.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1f13e32..adbb1fd 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -338,6 +338,20 @@ static const struct mtd_ooblayout_ops part_ooblayout_ops = {
 	.free = part_ooblayout_free,
 };
 
+static int part_get_device(struct mtd_info *mtd)
+{
+	struct mtd_part *part = mtd_to_part(mtd);
+
+	return part->master->_get_device(part->master);
+}
+
+static void part_put_device(struct mtd_info *mtd)
+{
+	struct mtd_part *part = mtd_to_part(mtd);
+
+	part->master->_put_device(part->master);
+}
+
 static inline void free_partition(struct mtd_part *p)
 {
 	kfree(p->mtd.name);
@@ -463,6 +477,10 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 		slave->mtd._block_isbad = part_block_isbad;
 	if (master->_block_markbad)
 		slave->mtd._block_markbad = part_block_markbad;
+	if (master->_get_device)
+		slave->mtd._get_device = part_get_device;
+	if (master->_put_device)
+		slave->mtd._put_device = part_put_device;
 	slave->mtd._erase = part_erase;
 	slave->master = master;
 	slave->offset = part->offset;
-- 
1.7.9.5

Re: [PATCH v1 2/2] mtd: Add hooks to call get and put on mtd devices.

[Richard Weinberger]

Hi!

On Thu, Aug 4, 2016 at 4:16 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
> From: Jim Baxter <jim_baxter@mentor.com>
>
> Add hooks to mtdpart to call _get_device and _put_device,
> this allows the mtd devices to perform internal
> reference counting if required.
>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>

I sent the same patch some time ago:
https://patchwork.ozlabs.org/patch/644422/

> ---
>  drivers/mtd/mtdpart.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 1f13e32..adbb1fd 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -338,6 +338,20 @@ static const struct mtd_ooblayout_ops part_ooblayout_ops = {
>         .free = part_ooblayout_free,
>  };
>
> +static int part_get_device(struct mtd_info *mtd)
> +{
> +       struct mtd_part *part = mtd_to_part(mtd);
> +
> +       return part->master->_get_device(part->master);
> +}
> +
> +static void part_put_device(struct mtd_info *mtd)
> +{
> +       struct mtd_part *part = mtd_to_part(mtd);
> +
> +       part->master->_put_device(part->master);
> +}
> +
>  static inline void free_partition(struct mtd_part *p)
>  {
>         kfree(p->mtd.name);
> @@ -463,6 +477,10 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>                 slave->mtd._block_isbad = part_block_isbad;
>         if (master->_block_markbad)
>                 slave->mtd._block_markbad = part_block_markbad;
> +       if (master->_get_device)
> +               slave->mtd._get_device = part_get_device;
> +       if (master->_put_device)
> +               slave->mtd._put_device = part_put_device;
>         slave->mtd._erase = part_erase;
>         slave->master = master;
>         slave->offset = part->offset;
> --
> 1.7.9.5
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Thanks,
//richard

Re: [PATCH v1 2/2] mtd: Add hooks to call get and put on mtd devices.

[Baxter, Jim]

> Hi!
> 
> On Thu, Aug 4, 2016 at 4:16 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
>> From: Jim Baxter <jim_baxter@mentor.com>
>>
>> Add hooks to mtdpart to call _get_device and _put_device,
>> this allows the mtd devices to perform internal
>> reference counting if required.
>>
>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
> 
> I sent the same patch some time ago:
> https://patchwork.ozlabs.org/patch/644422/
> 

Hi Richard,

Thank you for letting us know.

Thanks,
Jim

Re: [PATCH v1 2/2] mtd: Add hooks to call get and put on mtd devices.

[Richard Weinberger]

Am 05.08.2016 um 13:54 schrieb Baxter, Jim:
>> On Thu, Aug 4, 2016 at 4:16 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
>>> From: Jim Baxter <jim_baxter@mentor.com>
>>>
>>> Add hooks to mtdpart to call _get_device and _put_device,
>>> this allows the mtd devices to perform internal
>>> reference counting if required.
>>>
>>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
>>
>> I sent the same patch some time ago:
>> https://patchwork.ozlabs.org/patch/644422/

Brian, can you please pickup my patch for 4.8-rcX?
The patch fixes a real problem and another patch from
Mentor folks depends on it.

Thanks,
//richard

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Richard Weinberger]

On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>
> The MTD backing dev info objects mtd_bdi was statically allocated.
> So when MTD is built as a loadable module, this object fall in the
> vmalloc address space.
>
> The problem with that, is that the BDI APIs use wake_up_bit(), which calls
> virt_to_page() to retrieve the memory zone of the page containing the
> wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
> highmem addresses.
>
> Fix this by allocating the BDI objects dynamically with kmalloc. The
> objects now fall in the logical address space so that BDI APIs will
> work in all cases (mtd builtin or module).
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>

Reviewed-by: Richard Weinberger <richard@nod.at>

-- 
Thanks,
//richard

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Sandeep Jain]

Dear Maintainers,
  This patch is reviewed by Richard.
Requesting for Maintainer's attention for patch merge.

Thanks & Regards,
Sandeep Jain

On Sat, Sep 17, 2016 at 04:41:47PM +0200, Richard Weinberger wrote:
> On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
> > From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >
> > The MTD backing dev info objects mtd_bdi was statically allocated.
> > So when MTD is built as a loadable module, this object fall in the
> > vmalloc address space.
> >
> > The problem with that, is that the BDI APIs use wake_up_bit(), which calls
> > virt_to_page() to retrieve the memory zone of the page containing the
> > wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
> > highmem addresses.
> >
> > Fix this by allocating the BDI objects dynamically with kmalloc. The
> > objects now fall in the logical address space so that BDI APIs will
> > work in all cases (mtd builtin or module).
> >
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> > Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
> 
> Reviewed-by: Richard Weinberger <richard@nod.at>
> 
> -- 
> Thanks,
> //richard

Re: [PATCH v1 1/2] mtd: m25p80: lock module while used

[Sandeep Jain]

Dear Maintainers,
	Requesting for your attention for patch review/merge.

Thanks & Regards,
Sandeep Jain

On Thu, Aug 04, 2016 at 07:46:33PM +0530, Sandeep Jain wrote:
> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
> The change controls module users counter, which prevents to get
> accidental oops on module unload while it is in use by mtd subsystem:
> 
> % dd if=/dev/mtd0 of=/dev/null &
> % rmmod m25p80
> 
> Removing MTD device #0 (spi32766.0) with use count 1
> Unable to handle kernel paging request at virtual address 7f4fb7f8
> pgd = bd094000
> [7f4fb7f8] *pgd=4cb66811, *pte=00000000, *ppte=00000000
> Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
> ---
>  drivers/mtd/devices/m25p80.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd..2eb1530 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -185,6 +185,19 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>  	return ret;
>  }
>  
> +static void m25p80_put(struct mtd_info *mtd)
> +{
> +	module_put(THIS_MODULE);
> +}
> +
> +static int m25p80_get(struct mtd_info *mtd)
> +{
> +	if (!try_module_get(THIS_MODULE))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>  /*
>   * board specific setup should have ensured the SPI clock used here
>   * matches what the READ command supports, at least until this driver
> @@ -212,6 +225,8 @@ static int m25p_probe(struct spi_device *spi)
>  	nor->write = m25p80_write;
>  	nor->write_reg = m25p80_write_reg;
>  	nor->read_reg = m25p80_read_reg;
> +	nor->mtd._put_device = m25p80_put;
> +	nor->mtd._get_device = m25p80_get;
>  
>  	nor->dev = &spi->dev;
>  	spi_nor_set_flash_node(nor, spi->dev.of_node);
> -- 
> 1.7.9.5
> 

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Marek Vasut]

On 11/03/2016 12:34 PM, Sandeep Jain wrote:
> Dear Maintainers,
>   This patch is reviewed by Richard.
> Requesting for Maintainer's attention for patch merge.
> 
> Thanks & Regards,
> Sandeep Jain
> 
> On Sat, Sep 17, 2016 at 04:41:47PM +0200, Richard Weinberger wrote:
>> On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>
>>> The MTD backing dev info objects mtd_bdi was statically allocated.
>>> So when MTD is built as a loadable module, this object fall in the
>>> vmalloc address space.
>>>
>>> The problem with that, is that the BDI APIs use wake_up_bit(), which calls
>>> virt_to_page() to retrieve the memory zone of the page containing the
>>> wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
>>> highmem addresses.
>>>
>>> Fix this by allocating the BDI objects dynamically with kmalloc. The
>>> objects now fall in the logical address space so that BDI APIs will
>>> work in all cases (mtd builtin or module).
>>>
>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
>>
>> Reviewed-by: Richard Weinberger <richard@nod.at>

I don't see any obvious problem either:
Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

Re: [PATCH v1 1/2] mtd: m25p80: lock module while used

[Marek Vasut]

On 11/03/2016 12:39 PM, Sandeep Jain wrote:
> Dear Maintainers,
> 	Requesting for your attention for patch review/merge.
> 
> Thanks & Regards,
> Sandeep Jain
> 
> On Thu, Aug 04, 2016 at 07:46:33PM +0530, Sandeep Jain wrote:
>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>
>> The change controls module users counter, which prevents to get
>> accidental oops on module unload while it is in use by mtd subsystem:
>>
>> % dd if=/dev/mtd0 of=/dev/null &
>> % rmmod m25p80
>>
>> Removing MTD device #0 (spi32766.0) with use count 1
>> Unable to handle kernel paging request at virtual address 7f4fb7f8
>> pgd = bd094000
>> [7f4fb7f8] *pgd=4cb66811, *pte=00000000, *ppte=00000000
>> Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
>> ---
>>  drivers/mtd/devices/m25p80.c |   15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 9cf7fcd..2eb1530 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -185,6 +185,19 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>  	return ret;
>>  }
>>  
>> +static void m25p80_put(struct mtd_info *mtd)
>> +{
>> +	module_put(THIS_MODULE);
>> +}
>> +
>> +static int m25p80_get(struct mtd_info *mtd)
>> +{
>> +	if (!try_module_get(THIS_MODULE))
>> +		return -ENODEV;
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * board specific setup should have ensured the SPI clock used here
>>   * matches what the READ command supports, at least until this driver
>> @@ -212,6 +225,8 @@ static int m25p_probe(struct spi_device *spi)
>>  	nor->write = m25p80_write;
>>  	nor->write_reg = m25p80_write_reg;
>>  	nor->read_reg = m25p80_read_reg;
>> +	nor->mtd._put_device = m25p80_put;
>> +	nor->mtd._get_device = m25p80_get;
>>  
>>  	nor->dev = &spi->dev;
>>  	spi_nor_set_flash_node(nor, spi->dev.of_node);

This makes me ponder how many other drivers suffer from this issue and
whether you shouldn't fix this in the core code instead. What do you think?

-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Sandeep Jain]

Dear Maintainers,
   This patch is already reviewed twice with no issues.
Requesting your attention for patch merge.

Thanks & Regards,
Sandeep Jain

On Sat, Nov 05, 2016 at 08:22:31AM +0100, Marek Vasut wrote:
> On 11/03/2016 12:34 PM, Sandeep Jain wrote:
> > Dear Maintainers,
> >   This patch is reviewed by Richard.
> > Requesting for Maintainer's attention for patch merge.
> > 
> > Thanks & Regards,
> > Sandeep Jain
> > 
> > On Sat, Sep 17, 2016 at 04:41:47PM +0200, Richard Weinberger wrote:
> >> On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
> >>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>
> >>> The MTD backing dev info objects mtd_bdi was statically allocated.
> >>> So when MTD is built as a loadable module, this object fall in the
> >>> vmalloc address space.
> >>>
> >>> The problem with that, is that the BDI APIs use wake_up_bit(), which calls
> >>> virt_to_page() to retrieve the memory zone of the page containing the
> >>> wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
> >>> highmem addresses.
> >>>
> >>> Fix this by allocating the BDI objects dynamically with kmalloc. The
> >>> objects now fall in the logical address space so that BDI APIs will
> >>> work in all cases (mtd builtin or module).
> >>>
> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> >>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
> >>
> >> Reviewed-by: Richard Weinberger <richard@nod.at>
> 
> I don't see any obvious problem either:
> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
> 
> -- 
> Best regards,
> Marek Vasut

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Marek Vasut]

On 11/29/2016 09:12 AM, Sandeep Jain wrote:
> Dear Maintainers,
>    This patch is already reviewed twice with no issues.
> Requesting your attention for patch merge.
> 
> Thanks & Regards,
> Sandeep Jain
> 
> On Sat, Nov 05, 2016 at 08:22:31AM +0100, Marek Vasut wrote:
>> On 11/03/2016 12:34 PM, Sandeep Jain wrote:
>>> Dear Maintainers,
>>>   This patch is reviewed by Richard.
>>> Requesting for Maintainer's attention for patch merge.
>>>
>>> Thanks & Regards,
>>> Sandeep Jain
>>>
>>> On Sat, Sep 17, 2016 at 04:41:47PM +0200, Richard Weinberger wrote:
>>>> On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>
>>>>> The MTD backing dev info objects mtd_bdi was statically allocated.
>>>>> So when MTD is built as a loadable module, this object fall in the
>>>>> vmalloc address space.
>>>>>
>>>>> The problem with that, is that the BDI APIs use wake_up_bit(), which calls
>>>>> virt_to_page() to retrieve the memory zone of the page containing the
>>>>> wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
>>>>> highmem addresses.
>>>>>
>>>>> Fix this by allocating the BDI objects dynamically with kmalloc. The
>>>>> objects now fall in the logical address space so that BDI APIs will
>>>>> work in all cases (mtd builtin or module).
>>>>>
>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>>>>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
>>>>
>>>> Reviewed-by: Richard Weinberger <richard@nod.at>
>>
>> I don't see any obvious problem either:
>> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

Bump?

-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Brian Norris]

On Thu, Dec 01, 2016 at 04:39:23PM +0100, Marek Vasut wrote:
> On 11/29/2016 09:12 AM, Sandeep Jain wrote:
> > On Sat, Nov 05, 2016 at 08:22:31AM +0100, Marek Vasut wrote:
> >> On 11/03/2016 12:34 PM, Sandeep Jain wrote:
> >>> On Sat, Sep 17, 2016 at 04:41:47PM +0200, Richard Weinberger wrote:
> >>>> On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
> >>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>>
> >>>>> The MTD backing dev info objects mtd_bdi was statically allocated.
> >>>>> So when MTD is built as a loadable module, this object fall in the
> >>>>> vmalloc address space.
> >>>>>
> >>>>> The problem with that, is that the BDI APIs use wake_up_bit(), which calls
> >>>>> virt_to_page() to retrieve the memory zone of the page containing the
> >>>>> wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
> >>>>> highmem addresses.
> >>>>>
> >>>>> Fix this by allocating the BDI objects dynamically with kmalloc. The
> >>>>> objects now fall in the logical address space so that BDI APIs will
> >>>>> work in all cases (mtd builtin or module).
> >>>>>
> >>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> >>>>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
> >>>>
> >>>> Reviewed-by: Richard Weinberger <richard@nod.at>
> >>
> >> I don't see any obvious problem either:
> >> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
> 
> Bump?

Applied to l2-mtd.git.

Re: [PATCH] mtd: Allocate bdi objects dynamically

[Marek Vasut]

On 12/01/2016 07:14 PM, Brian Norris wrote:
> On Thu, Dec 01, 2016 at 04:39:23PM +0100, Marek Vasut wrote:
>> On 11/29/2016 09:12 AM, Sandeep Jain wrote:
>>> On Sat, Nov 05, 2016 at 08:22:31AM +0100, Marek Vasut wrote:
>>>> On 11/03/2016 12:34 PM, Sandeep Jain wrote:
>>>>> On Sat, Sep 17, 2016 at 04:41:47PM +0200, Richard Weinberger wrote:
>>>>>> On Thu, Aug 4, 2016 at 4:01 PM, Sandeep Jain <Sandeep_Jain@mentor.com> wrote:
>>>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>
>>>>>>> The MTD backing dev info objects mtd_bdi was statically allocated.
>>>>>>> So when MTD is built as a loadable module, this object fall in the
>>>>>>> vmalloc address space.
>>>>>>>
>>>>>>> The problem with that, is that the BDI APIs use wake_up_bit(), which calls
>>>>>>> virt_to_page() to retrieve the memory zone of the page containing the
>>>>>>> wait_queue to wake up, and virt_to_page() is not valid for vmalloc or
>>>>>>> highmem addresses.
>>>>>>>
>>>>>>> Fix this by allocating the BDI objects dynamically with kmalloc. The
>>>>>>> objects now fall in the logical address space so that BDI APIs will
>>>>>>> work in all cases (mtd builtin or module).
>>>>>>>
>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>>>>>>> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
>>>>>>
>>>>>> Reviewed-by: Richard Weinberger <richard@nod.at>
>>>>
>>>> I don't see any obvious problem either:
>>>> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
>>
>> Bump?
> 
> Applied to l2-mtd.git.
> 
Thanks !

-- 
Best regards,
Marek Vasut

Re: [PATCH v1 1/2] mtd: m25p80: lock module while used

[Brian Norris]

Hi,

On Sat, Nov 05, 2016 at 08:24:24AM +0100, Marek Vasut wrote:
> On 11/03/2016 12:39 PM, Sandeep Jain wrote:
> > On Thu, Aug 04, 2016 at 07:46:33PM +0530, Sandeep Jain wrote:
> >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >>
> >> The change controls module users counter, which prevents to get
> >> accidental oops on module unload while it is in use by mtd subsystem:
> >>
> >> % dd if=/dev/mtd0 of=/dev/null &
> >> % rmmod m25p80
> >>
> >> Removing MTD device #0 (spi32766.0) with use count 1
> >> Unable to handle kernel paging request at virtual address 7f4fb7f8
> >> pgd = bd094000
> >> [7f4fb7f8] *pgd=4cb66811, *pte=00000000, *ppte=00000000
> >> Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
> >>
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> Signed-off-by: Sandeep Jain <Sandeep_Jain@mentor.com>
> >> ---
> >>  drivers/mtd/devices/m25p80.c |   15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> >> index 9cf7fcd..2eb1530 100644
> >> --- a/drivers/mtd/devices/m25p80.c
> >> +++ b/drivers/mtd/devices/m25p80.c
> >> @@ -185,6 +185,19 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
> >>  	return ret;
> >>  }
> >>  
> >> +static void m25p80_put(struct mtd_info *mtd)
> >> +{
> >> +	module_put(THIS_MODULE);
> >> +}
> >> +
> >> +static int m25p80_get(struct mtd_info *mtd)
> >> +{
> >> +	if (!try_module_get(THIS_MODULE))
> >> +		return -ENODEV;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /*
> >>   * board specific setup should have ensured the SPI clock used here
> >>   * matches what the READ command supports, at least until this driver
> >> @@ -212,6 +225,8 @@ static int m25p_probe(struct spi_device *spi)
> >>  	nor->write = m25p80_write;
> >>  	nor->write_reg = m25p80_write_reg;
> >>  	nor->read_reg = m25p80_read_reg;
> >> +	nor->mtd._put_device = m25p80_put;
> >> +	nor->mtd._get_device = m25p80_get;
> >>  
> >>  	nor->dev = &spi->dev;
> >>  	spi_nor_set_flash_node(nor, spi->dev.of_node);
> 
> This makes me ponder how many other drivers suffer from this issue and
> whether you shouldn't fix this in the core code instead. What do you think?

I'm a bit confused; the owner is already set as mtd->owner
(spi_register_driver() assigns the driver.owner, and the MTD core code
finds it via mtd->dev.parent), and I think we grab the appropriate
references. But I wouldn't be surprised if there was a bug lurking in
there somewhere still. Certainly the removal/cleanup logic might still
have some issues.

But I also notice that your supposed test case actually works just fine
for me:

# dd if=/dev/mtd0 of=/dev/null bs=2M & rmmod m25p80
[1] 8781
rmmod: ERROR: Module m25p80 is in use

Maybe this has already been fixed in the meantime?

And anyway, if there is a problem like this, I expect we'll want to
handle it in the core code, as Marek suggested.

Brian