linux-kernel - RE: [PATCH v2] i2c: cadence: Add system suspend and resume PM support

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <ZQ0PR01MB1160E73BE852925A270DBE95EB64A@ZQ0PR01MB1160.CHNPR01.prod.partner.outlook.cn>
Date: Sun, 7 Jan 2024 23:47:17 +0000
From: JiSheng Teoh <jisheng.teoh@...rfivetech.com>
To: "andi.shyti@...nel.org" <andi.shyti@...nel.org>
CC: JiSheng Teoh <jisheng.teoh@...rfivetech.com>, Leyfoon Tan
	<leyfoon.tan@...rfivetech.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "michal.simek@....com" <michal.simek@....com>
Subject: RE: [PATCH v2] i2c: cadence: Add system suspend and resume PM support

Hi Andi,

> On Sun, 10 Dec 2023 12:54:12 +0100
> Andi Shyti <andi.shyti@...nel.org> wrote:
> 
> > Hi Ji Sheng,
> >
> > [...]
> >
> > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) {
> > > >
> > > > I am not really understanding what you are trying to do here:
> > > >
> > > > > +	struct cdns_i2c *xi2c = dev_get_drvdata(dev);
> > > > > +	int err;
> > > > > +
> > > > > +	err = cdns_i2c_runtime_resume(dev);
> > > >
> > > > First you try to resume...
> > > >
> > > > > +	if (err)
> > > > > +		return err;
> > > > > +
> > > > > +	if (pm_runtime_status_suspended(dev)) {
> > > >
> > > > ... then you check if you are suspended ...
> > >
> > > This serves as a check and balance to ensure that when the system
> > > resumes with device in runtime suspend state, we disable the clock
> > > enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced
> > > clock reference count for subsequent runtime resume transition.
> > > Similar implementation can be found in this commit:
> > > https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab
> > > 10dd5ccaea393
> > >
> >
> > OK, this is done purely for clock balancing, but then, I still don't
> > understand the case. I expect the clock counter to be unbalanced when
> > you suspend (because is moving towards '0').
> >
> > While, if you check if the clock is unbalanced when resuming, it means
> > that the clock had a negative counter (which is impossible because the
> > clock counter is unsigned).
> >
> > If there is any unbalancing at this stage, then I recommend you to
> > check what has happened previously.
> >
> > ... Or is there anything I am missing?
> >
> > Thanks,
> > Andi
> 
> You are right, the clock counter will move towards 0 during system suspend.
> Conversely during system resume, the clock counter is incremented to 1 early on in cdns_i2c_runtime_resume(). So the clock counter
> is not negative to start with.
> At this point of time, clock counter is 1. If the device is in runtime suspend, we decrement the clock counter back to 0, so the
> subsequent runtime resume could increment it back to 1. In a sense, balancing the clock counter.
> Please help correct me if I've got it wrong.
> 
Just to check on the status of this patch. Let me know if the above statement makes sense.
> >
> > > > > +		err = cdns_i2c_runtime_suspend(dev);
> > > >
> > > > ... and suspend again? Shouldn't this be _resume()?
> > > >
> > > > Thanks,
> > > > Andi
> > > >
> > > > > +		if (err)
> > > > > +			return err;
> > > > > +	}
> > > > > +
> > > > > +	i2c_mark_adapter_resumed(&xi2c->adap);
> > > > > +
> > > > > +	return 0;
> > > > > +}
>