[PATCH] mtd: intel-dg: wake card on operations

[PATCH] mtd: intel-dg: wake card on operations

[Alexander Usyskin]

Enable runtime PM in mtd driver to notify parent graphics driver
that whole card should be kept awake while nvm operations are
performed through this driver.

CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/mtd/devices/mtd_intel_dg.c | 72 +++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/devices/mtd_intel_dg.c b/drivers/mtd/devices/mtd_intel_dg.c
index b438ee5aacc3..9c8d1405219c 100644
--- a/drivers/mtd/devices/mtd_intel_dg.c
+++ b/drivers/mtd/devices/mtd_intel_dg.c
@@ -15,14 +15,18 @@
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/pm_runtime.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/sizes.h>
 #include <linux/types.h>
 
+#define INTEL_DG_NVM_RPM_TIMEOUT_MS 500
+
 struct intel_dg_nvm {
 	struct kref refcnt;
 	struct mtd_info mtd;
+	struct device *dev;
 	struct mutex lock; /* region access lock */
 	void __iomem *base;
 	void __iomem *base2;
@@ -421,6 +425,8 @@ static int intel_dg_nvm_init(struct intel_dg_nvm *nvm, struct device *device,
 	unsigned int i, n;
 	int ret;
 
+	nvm->dev = device;
+
 	/* clean error register, previous errors are ignored */
 	idg_nvm_error(nvm);
 
@@ -494,6 +500,7 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
 	size_t total_len;
 	unsigned int idx;
 	ssize_t bytes;
+	int ret = 0;
 	loff_t from;
 	size_t len;
 	u8 region;
@@ -512,20 +519,28 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
 	total_len = info->len;
 	addr = info->addr;
 
+	ret = pm_runtime_resume_and_get(nvm->dev);
+	if (ret < 0) {
+		dev_err(&mtd->dev, "rpm: get failed %d\n", ret);
+		return ret;
+	}
+
 	guard(mutex)(&nvm->lock);
 
 	while (total_len > 0) {
 		if (!IS_ALIGNED(addr, SZ_4K) || !IS_ALIGNED(total_len, SZ_4K)) {
 			dev_err(&mtd->dev, "unaligned erase %llx %zx\n", addr, total_len);
 			info->fail_addr = addr;
-			return -ERANGE;
+			ret = -ERANGE;
+			break;
 		}
 
 		idx = idg_nvm_get_region(nvm, addr);
 		if (idx >= nvm->nregions) {
 			dev_err(&mtd->dev, "out of range");
 			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
-			return -ERANGE;
+			ret = -ERANGE;
+			break;
 		}
 
 		from = addr - nvm->regions[idx].offset;
@@ -541,14 +556,17 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
 		if (bytes < 0) {
 			dev_dbg(&mtd->dev, "erase failed with %zd\n", bytes);
 			info->fail_addr += nvm->regions[idx].offset;
-			return bytes;
+			ret = bytes;
+			break;
 		}
 
 		addr += len;
 		total_len -= len;
 	}
 
-	return 0;
+	pm_runtime_mark_last_busy(nvm->dev);
+	pm_runtime_put_autosuspend(nvm->dev);
+	return ret;
 }
 
 static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
@@ -577,17 +595,25 @@ static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (len > nvm->regions[idx].size - from)
 		len = nvm->regions[idx].size - from;
 
+	ret = pm_runtime_resume_and_get(nvm->dev);
+	if (ret < 0) {
+		dev_err(&mtd->dev, "rpm: get failed %zd\n", ret);
+		return ret;
+	}
+
 	guard(mutex)(&nvm->lock);
 
 	ret = idg_read(nvm, region, from, len, buf);
 	if (ret < 0) {
 		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
-		return ret;
+	} else {
+		*retlen = ret;
+		ret = 0;
 	}
 
-	*retlen = ret;
-
-	return 0;
+	pm_runtime_mark_last_busy(nvm->dev);
+	pm_runtime_put_autosuspend(nvm->dev);
+	return ret;
 }
 
 static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
@@ -616,17 +642,25 @@ static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (len > nvm->regions[idx].size - to)
 		len = nvm->regions[idx].size - to;
 
+	ret = pm_runtime_resume_and_get(nvm->dev);
+	if (ret < 0) {
+		dev_err(&mtd->dev, "rpm: get failed %zd\n", ret);
+		return ret;
+	}
+
 	guard(mutex)(&nvm->lock);
 
 	ret = idg_write(nvm, region, to, len, buf);
 	if (ret < 0) {
 		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
-		return ret;
+	} else {
+		*retlen = ret;
+		ret = 0;
 	}
 
-	*retlen = ret;
-
-	return 0;
+	pm_runtime_mark_last_busy(nvm->dev);
+	pm_runtime_put_autosuspend(nvm->dev);
+	return ret;
 }
 
 static void intel_dg_nvm_release(struct kref *kref)
@@ -753,6 +787,17 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 	}
 	nvm->nregions = n; /* in case where kasprintf fail */
 
+	devm_pm_runtime_enable(device);
+
+	pm_runtime_set_autosuspend_delay(device, INTEL_DG_NVM_RPM_TIMEOUT_MS);
+	pm_runtime_use_autosuspend(device);
+
+	ret = pm_runtime_resume_and_get(device);
+	if (ret < 0) {
+		dev_err(device, "rpm: get failed %d\n", ret);
+		goto err_norpm;
+	}
+
 	nvm->base = devm_ioremap_resource(device, &invm->bar);
 	if (IS_ERR(nvm->base)) {
 		ret = PTR_ERR(nvm->base);
@@ -781,9 +826,12 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 
 	dev_set_drvdata(&aux_dev->dev, nvm);
 
+	pm_runtime_put(device);
 	return 0;
 
 err:
+	pm_runtime_put(device);
+err_norpm:
 	kref_put(&nvm->refcnt, intel_dg_nvm_release);
 	return ret;
 }
-- 
2.43.0

Re: [PATCH] mtd: intel-dg: wake card on operations

[Andy Shevchenko]

On Sun, Oct 19, 2025 at 06:01:45PM +0300, Alexander Usyskin wrote:
> Enable runtime PM in mtd driver to notify parent graphics driver
> that whole card should be kept awake while nvm operations are
> performed through this driver.

...

> +	pm_runtime_mark_last_busy(nvm->dev);
> +	pm_runtime_put_autosuspend(nvm->dev);

Please, drop the second (duplicate) call.

...

> +	devm_pm_runtime_enable(device);

Please, justify why this code is good without error checking. Before doing that
think for a moment for the cases when devm_*() might be developed in the future
and return something interesting (if not yet).

...

>  err:
> +	pm_runtime_put(device);
> +err_norpm:
>  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
>  	return ret;

Mixing devm with non-devm usually lead to hard to catch bugs in the error paths
/ remove stages with ordering of cleaning resources up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mtd: intel-dg: wake card on operations

[Andy Shevchenko]

On Mon, Oct 20, 2025 at 01:09:10PM +0300, Andy Shevchenko wrote:
> On Sun, Oct 19, 2025 at 06:01:45PM +0300, Alexander Usyskin wrote:
> > Enable runtime PM in mtd driver to notify parent graphics driver
> > that whole card should be kept awake while nvm operations are
> > performed through this driver.

Ah, and perhaps a bit elaboration why graphics card needs that?

...

> > +	pm_runtime_mark_last_busy(nvm->dev);
> > +	pm_runtime_put_autosuspend(nvm->dev);
> 
> Please, drop the second (duplicate) call.

...

> > +	devm_pm_runtime_enable(device);
> 
> Please, justify why this code is good without error checking. Before doing that
> think for a moment for the cases when devm_*() might be developed in the future
> and return something interesting (if not yet).

...

> >  err:
> > +	pm_runtime_put(device);
> > +err_norpm:
> >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> >  	return ret;
> 
> Mixing devm with non-devm usually lead to hard to catch bugs in the error paths
> / remove stages with ordering of cleaning resources up.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

> Subject: Re: [PATCH] mtd: intel-dg: wake card on operations
> 
> On Mon, Oct 20, 2025 at 01:09:10PM +0300, Andy Shevchenko wrote:
> > On Sun, Oct 19, 2025 at 06:01:45PM +0300, Alexander Usyskin wrote:
> > > Enable runtime PM in mtd driver to notify parent graphics driver
> > > that whole card should be kept awake while nvm operations are
> > > performed through this driver.
> 
> Ah, and perhaps a bit elaboration why graphics card needs that?
> 
The memory power will be cut when whole card is powered down.
Intel DG card does not have separate power control for persistent memory.
Will add this in v2.

> ...
> 
> > > +	pm_runtime_mark_last_busy(nvm->dev);
> > > +	pm_runtime_put_autosuspend(nvm->dev);
> >
> > Please, drop the second (duplicate) call.

Missed the patch where pm_runtime_put_autosuspend includes pm_runtime_mark_last_busy.
Will update in V2, thx

> 
> ...
> 
> > > +	devm_pm_runtime_enable(device);
> >
> > Please, justify why this code is good without error checking. Before doing
> that
> > think for a moment for the cases when devm_*() might be developed in the
> future
> > and return something interesting (if not yet).
> 

We should not fail the probe because of runtime  pm enablement failure, I suppose.
There are other ways to keep card awake.
The pm_runtime_* functions work without runtime_enable but have no effect.
Thus, we can ignore failure here.

> ...
> 
> > >  err:
> > > +	pm_runtime_put(device);
> > > +err_norpm:
> > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > >  	return ret;
> >
> > Mixing devm with non-devm usually lead to hard to catch bugs in the error
> paths
> > / remove stages with ordering of cleaning resources up.
> 

I see that this pattern is reasonably common in drivers.
There can't be devm wrappers for pm_runtime_get/put and these functions works
regardless of enable status.

> --
> With Best Regards,
> Andy Shevchenko
> 

- - 
Thanks,
Sasha

Re: [PATCH] mtd: intel-dg: wake card on operations

[Andy Shevchenko]

On Tue, Oct 21, 2025 at 12:51:30PM +0000, Usyskin, Alexander wrote:
> > On Mon, Oct 20, 2025 at 01:09:10PM +0300, Andy Shevchenko wrote:
> > > On Sun, Oct 19, 2025 at 06:01:45PM +0300, Alexander Usyskin wrote:

...

> > > > +	devm_pm_runtime_enable(device);
> > >
> > > Please, justify why this code is good without error checking. Before doing
> > that
> > > think for a moment for the cases when devm_*() might be developed in the
> > future
> > > and return something interesting (if not yet).
> 
> We should not fail the probe because of runtime  pm enablement failure, I suppose.
> There are other ways to keep card awake.
> The pm_runtime_* functions work without runtime_enable but have no effect.
> Thus, we can ignore failure here.

Using devm_*() in such a case is misleading. It incorporates errors from
different layers and ignoring both is odd.

I would suggest to avoid using devm_*() in this case and put a comment on
the ignored PM errors (however, personally I think this approach is wrong).

...

> > > >  err:
> > > > +	pm_runtime_put(device);
> > > > +err_norpm:
> > > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > >  	return ret;
> > >
> > > Mixing devm with non-devm usually lead to hard to catch bugs in the error
> > paths
> > > / remove stages with ordering of cleaning resources up.
> 
> I see that this pattern is reasonably common in drivers.
> There can't be devm wrappers for pm_runtime_get/put and these functions works
> regardless of enable status.

It can be wrapped to become a managed resource, but the problem is that you are
ignoring errors from it, which I consider a bit incorrect.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mtd: intel-dg: wake card on operations

[Lucas De Marchi]

On Tue, Oct 21, 2025 at 05:39:34PM +0300, Andy Shevchenko wrote:
>On Tue, Oct 21, 2025 at 12:51:30PM +0000, Usyskin, Alexander wrote:
>> > On Mon, Oct 20, 2025 at 01:09:10PM +0300, Andy Shevchenko wrote:
>> > > On Sun, Oct 19, 2025 at 06:01:45PM +0300, Alexander Usyskin wrote:
>
>...
>
>> > > > +	devm_pm_runtime_enable(device);
>> > >
>> > > Please, justify why this code is good without error checking. Before doing
>> > that
>> > > think for a moment for the cases when devm_*() might be developed in the
>> > future
>> > > and return something interesting (if not yet).
>>
>> We should not fail the probe because of runtime  pm enablement failure, I suppose.

not really

>> There are other ways to keep card awake.
>> The pm_runtime_* functions work without runtime_enable but have no effect.
>> Thus, we can ignore failure here.
>
>Using devm_*() in such a case is misleading. It incorporates errors from
>different layers and ignoring both is odd.
>
>I would suggest to avoid using devm_*() in this case and put a comment on
>the ignored PM errors (however, personally I think this approach is wrong).

Agreed. We should not silently continue on error. Fix the cause of the
error intead. If it's something that can be disabled in
runtime/configure time, and it doesn't return success, handle that
specific error code.

If there's a reason to ignore the error, it should be intentional.

Lucas De Marchi

RE: [PATCH] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

> Subject: Re: [PATCH] mtd: intel-dg: wake card on operations
> 
> On Tue, Oct 21, 2025 at 05:39:34PM +0300, Andy Shevchenko wrote:
> >On Tue, Oct 21, 2025 at 12:51:30PM +0000, Usyskin, Alexander wrote:
> >> > On Mon, Oct 20, 2025 at 01:09:10PM +0300, Andy Shevchenko wrote:
> >> > > On Sun, Oct 19, 2025 at 06:01:45PM +0300, Alexander Usyskin wrote:
> >
> >...
> >
> >> > > > +	devm_pm_runtime_enable(device);
> >> > >
> >> > > Please, justify why this code is good without error checking. Before
> doing
> >> > that
> >> > > think for a moment for the cases when devm_*() might be developed
> in the
> >> > future
> >> > > and return something interesting (if not yet).
> >>
> >> We should not fail the probe because of runtime  pm enablement failure, I
> suppose.
> 
> not really
> 
> >> There are other ways to keep card awake.
> >> The pm_runtime_* functions work without runtime_enable but have no
> effect.
> >> Thus, we can ignore failure here.
> >
> >Using devm_*() in such a case is misleading. It incorporates errors from
> >different layers and ignoring both is odd.
> >
> >I would suggest to avoid using devm_*() in this case and put a comment on
> >the ignored PM errors (however, personally I think this approach is wrong).
> 
> Agreed. We should not silently continue on error. Fix the cause of the
> error intead. If it's something that can be disabled in
> runtime/configure time, and it doesn't return success, handle that
> specific error code.
> 
> If there's a reason to ignore the error, it should be intentional.
> 
> Lucas De Marchi

Ok, will check and fail the probe on this api failure.


- - 
Thanks,
Sasha