| 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
| ||
|
Date: Sun, 13 Mar 2022 15:47:07 +0000
From: "Usyskin, Alexander" <alexander.usyskin@...el.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@...el.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
David Airlie <airlied@...ux.ie>,
"Daniel Vetter" <daniel@...ll.ch>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Winkler, Tomas" <tomas.winkler@...el.com>,
"Lubart, Vitaly" <vitaly.lubart@...el.com>,
"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>
Subject: RE: [Intel-gfx] [PATCH v10 4/5] mei: gsc: add runtime pm handlers
> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@...el.com>
> Sent: Thursday, March 10, 2022 21:04
> To: Usyskin, Alexander <alexander.usyskin@...el.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Jani Nikula
> <jani.nikula@...ux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@...ux.intel.com>; David Airlie <airlied@...ux.ie>; Daniel
> Vetter <daniel@...ll.ch>; Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>;
> linux-kernel@...r.kernel.org; Winkler, Tomas <tomas.winkler@...el.com>;
> Lubart, Vitaly <vitaly.lubart@...el.com>; intel-gfx@...ts.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v10 4/5] mei: gsc: add runtime pm handlers
>
> On Tue, Mar 08, 2022 at 06:36:53PM +0200, Alexander Usyskin wrote:
> > From: Tomas Winkler <tomas.winkler@...el.com>
> >
> > Implement runtime handlers for mei-gsc, to track
> > idle state of the device properly.
> >
> > CC: Rodrigo Vivi <rodrigo.vivi@...el.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@...el.com>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> > ---
> > drivers/misc/mei/gsc-me.c | 67
> ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> > index cf427f6fdec9..dac482ddab51 100644
> > --- a/drivers/misc/mei/gsc-me.c
> > +++ b/drivers/misc/mei/gsc-me.c
> > @@ -152,7 +152,72 @@ static int __maybe_unused
> mei_gsc_pm_resume(struct device *device)
> > return 0;
> > }
> >
> > -static SIMPLE_DEV_PM_OPS(mei_gsc_pm_ops, mei_gsc_pm_suspend,
> mei_gsc_pm_resume);
> > +static int __maybe_unused mei_gsc_pm_runtime_idle(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > +
> > + if (!dev)
> > + return -ENODEV;
> > + if (mei_write_is_idle(dev))
> > + pm_runtime_autosuspend(device);
>
> This is not needed. The _idle() callback is called right before the
> autosuspend.
> so you just need to return -EBUSY if not idle.
>
It is taken from blueprint in pci-me.c
IIRC here we ask the autosuspend to kick in after DELAY,
not simply rejecting it unconditionally.
> But also I'm missing the call to enable the autosuspend and set the delay.
>
These calls are in the second patch in the series, at the end of probe.
> Is this flow really working and you are getting device suspended when not in
> use?
> (Maybe it is just my ignorance on other flow types here)
>
GSC low-power is guided by DG card, here we only signaling to parent (i915, I think)
that GSC is idle or that we need resume to perform the operations.
> > +
> > + return -EBUSY;
> > +}
> > +
> > +static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > + struct mei_me_hw *hw;
> > + int ret;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + mutex_lock(&dev->device_lock);
> > +
> > + if (mei_write_is_idle(dev)) {
> > + hw = to_me_hw(dev);
> > + hw->pg_state = MEI_PG_ON;
> > + ret = 0;
> > + } else {
> > + ret = -EAGAIN;
> > + }
>
> probably not needed this here... but it would be good if you use
> the runtime_pm{get,put} to protect your write operations as well...
>
We reuse big portions of mei and mei-me drivers and there
all needed runtime_pm calls are implemented.
The runtime pm callbacks are different as GSC do not have
actual HW registers to handle the low power states as CSME has.
> > +
> > + mutex_unlock(&dev->device_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused mei_gsc_pm_runtime_resume(struct device
> *device)
> > +{
> > + struct mei_device *dev = dev_get_drvdata(device);
> > + struct mei_me_hw *hw;
> > + irqreturn_t irq_ret;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + mutex_lock(&dev->device_lock);
> > +
> > + hw = to_me_hw(dev);
> > + hw->pg_state = MEI_PG_OFF;
> > +
> > + mutex_unlock(&dev->device_lock);
> > +
> > + irq_ret = mei_me_irq_thread_handler(1, dev);
> > + if (irq_ret != IRQ_HANDLED)
> > + dev_err(dev->dev, "thread handler fail %d\n", irq_ret);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mei_gsc_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mei_gsc_pm_suspend,
> > + mei_gsc_pm_resume)
> > + SET_RUNTIME_PM_OPS(mei_gsc_pm_runtime_suspend,
> > + mei_gsc_pm_runtime_resume,
> > + mei_gsc_pm_runtime_idle)
> > +};
> >
> > static const struct auxiliary_device_id mei_gsc_id_table[] = {
> > {
> > --
> > 2.32.0
> >
--
Thanks,
Sasha
Powered by blists - more mailing lists