lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CY5PR11MB621157335FFB1089F49CEF8B954A2@CY5PR11MB6211.namprd11.prod.outlook.com>
Date: Mon, 28 Oct 2024 15:01:56 +0000
From: "Gupta, Anshuman" <anshuman.gupta@...el.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@...el.com>, "Usyskin, Alexander"
	<alexander.usyskin@...el.com>, "Deak, Imre" <imre.deak@...el.com>
CC: Miquel Raynal <miquel.raynal@...tlin.com>, Richard Weinberger
	<richard@....at>, Vignesh Raghavendra <vigneshr@...com>, "De Marchi, Lucas"
	<lucas.demarchi@...el.com>, Thomas Hellström
	<thomas.hellstrom@...ux.intel.com>, Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
	Simona Vetter <simona@...ll.ch>, Jani Nikula <jani.nikula@...ux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>, Tvrtko Ursulin
	<tursulin@...ulin.net>, "Weil, Oren jer" <oren.jer.weil@...el.com>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations



> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@...el.com>
> Sent: Monday, October 28, 2024 8:27 PM
> To: Usyskin, Alexander <alexander.usyskin@...el.com>; Gupta, Anshuman
> <anshuman.gupta@...el.com>; Deak, Imre <imre.deak@...el.com>
> Cc: Miquel Raynal <miquel.raynal@...tlin.com>; Richard Weinberger
> <richard@....at>; Vignesh Raghavendra <vigneshr@...com>; De Marchi,
> Lucas <lucas.demarchi@...el.com>; Thomas Hellström
> <thomas.hellstrom@...ux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst@...ux.intel.com>; Maxime Ripard <mripard@...nel.org>;
> Thomas Zimmermann <tzimmermann@...e.de>; David Airlie
> <airlied@...il.com>; Simona Vetter <simona@...ll.ch>; Jani Nikula
> <jani.nikula@...ux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@...ux.intel.com>; Tvrtko Ursulin <tursulin@...ulin.net>;
> Weil, Oren jer <oren.jer.weil@...el.com>; linux-mtd@...ts.infradead.org; dri-
> devel@...ts.freedesktop.org; intel-gfx@...ts.freedesktop.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > Enable runtime PM in mtd driver to notify graphics driver that whole
> > card should be kept awake while nvm operations are performed through
> > this driver.
> >
> > CC: Lucas De Marchi <lucas.demarchi@...el.com>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> > ---
> >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > +++++++++++++++++++++++++-----
> >  1 file changed, 61 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > b/drivers/mtd/devices/mtd-intel-dg.c
> > index 4951438e8faf..3d53f35478e8 100644
> > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > @@ -15,11 +15,14 @@
> >  #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 500
> > +
> >  struct intel_dg_nvm {
> >  	struct kref refcnt;
> >  	struct mtd_info mtd;
> > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd,
> struct erase_info *info)
> >  	loff_t from;
> >  	size_t len;
> >  	size_t total_len;
> > +	int ret = 0;
> >
> >  	if (WARN_ON(!nvm))
> >  		return -EINVAL;
> > @@ -476,20 +480,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(mtd->dev.parent);
> > +	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;
> > +			goto out;
> >  		}
> >
> >  		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;
> > +			goto out;
> >  		}
> >
> >  		from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> @@
> > 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;
> > +			goto out;
> >  		}
> >
> >  		addr += len;
> >  		total_len -= len;
> >  	}
> >
> > -	return 0;
> > +out:
> > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > +	return ret;
> >  }
> >
> >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > size_t len, @@ -541,17 +557,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(mtd->dev.parent);
> > +	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(mtd->dev.parent);
> > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > +	return ret;
> >  }
> >
> >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > len, @@ -580,17 +604,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(mtd->dev.parent);
> > +	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(mtd->dev.parent);
> > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > +	return ret;
> >  }
> >
> >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6 +754,17
> > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> >  		n++;
> >  	}
> >
> > +	pm_runtime_enable(device);
> > +
> > +	pm_runtime_set_autosuspend_delay(device,
> INTEL_DG_NVM_RPM_TIMEOUT);
> > +	pm_runtime_use_autosuspend(device);
> 
> something looks strange here...
> on the functions above you get and put references for the parent device
> directly.
> But here you enable the rpm for this device.
> 
> If I remember correctly from some old experiments, the rpm is smart enough
> and wake up the parent before waking up the child. So I'm wondering if we
> should only do the child without the parent.
Agree parent can't runtime suspend ind if it has active child.
Let this driver have it's own get/put routine instead of parent.
Thanks,
Anshuman Gupta.
> 
> Cc: Imre Deak <imre.deak@...el.com>
> Cc: Anshuman Gupta <anshuman.gupta@...el.com>
> 
> > +
> > +	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)) {
> >  		dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> @@ 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:
> > +	pm_runtime_disable(device);
> >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> >  	return ret;
> >  }
> > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> auxiliary_device *aux_dev)
> >  	if (!nvm)
> >  		return;
> >
> > +	pm_runtime_disable(&aux_dev->dev);
> > +
> >  	mtd_device_unregister(&nvm->mtd);
> >
> >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > --
> > 2.43.0
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ