Re: [PATCH 2/3] PM: add sysfs attributes for additional suspend/resume metrics

[Joshua Crofts]

On Tue, 10 Nov 2026 13:58:34 -0500
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Tue, Nov 10, 2026 at 01:54:05PM -0500, Greg Kroah-Hartman wrote:
> > On Tue, Nov 10, 2026 at 07:50:20PM +0100, Joshua Crofts wrote:  
> > > On Tue, 10 Nov 2026 12:29:11 -0500
> > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > 
> > > ...
> > >   
> > > > > +static ssize_t runtime_resume_count_show(struct device *dev,
> > > > > +					 struct device_attribute *attr,
> > > > > +					 char *buf)
> > > > > +{
> > > > > +	u64 count;
> > > > > +
> > > > > +	spin_lock_irq(&dev->power.lock);
> > > > > +	count = dev->power.resume_count;
> > > > > +	spin_unlock_irq(&dev->power.lock);    
> > > > 
> > > > Why is a lock needed at all?  The value isn't going to "smear", it will
> > > > change right after the lock anyway.  
> > > 
> > > On 64-bit systems an aligned u64 read is atomic, but on 32-bit systems it
> > > gets split into two 32-bit loads. Without locking, the read can tear
> > > between the upper and lower halves.  
> > 
> > Are you sure?  ANd if so, is that so bad?  The value is "best effort" as
> > it can, and will, change right after reading it.  

Fair enough, I agree this is "best effort", so no problem on my end with
dropping the locks. 

> And even more relevant, what specific 32bit system are you using that
> this new api is going to be relying on?

I'm not using any 32-bit system, this issue was flagged by a local Sashiko
run and at the time it made sense to me.

-- 
Kind regards,
Joshua Crofts