Messages in this thread | | | From | Guan-Yu Lin <> | Date | Mon, 26 Feb 2024 17:15:00 +0800 | Subject | Re: [PATCH v3] PM / core: conditionally skip system pm in device/driver model |
| |
On Fri, Feb 23, 2024 at 11:18 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Feb 23, 2024 at 02:38:29PM +0000, Guan-Yu Lin wrote: > > In systems with a main processor and a co-processor, asynchronous > > controller management can lead to conflicts. One example is the main > > processor attempting to suspend a device while the co-processor is > > actively using it. To address this, we introduce a new sysfs entry > > called "conditional_skip". This entry allows the system to selectively > > skip certain device power management state transitions. To use this > > feature, set the value in "conditional_skip" to indicate the type of > > state transition you want to avoid. Please review /Documentation/ABI/ > > testing/sysfs-devices-power for more detailed information. > > ... > > > +static ssize_t conditional_skip_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t n) > > +{ > > > + int ret; > > > + if (kstrtoint(buf, 0, &ret)) > > Why is it int? It seems like flags, should not be unsigned as u32 or so? >
The ".event" member in struct pm_message is an int, but the values assigned to it are used like bit flags (e.g. PM_EVENT_FREEZE=0x1, PM_EVENT_SUSPEND=0x2, PM_EVENT_HIBERNATE=0x4). Is this an intentional design choice? We might need to change the design accordingly.
> > + return -EINVAL; > > Do not shadow the real error code without justification. >
Thanks for suggesting the desired implementation. I'll refactor it in the next version.
> > + ret &= (PM_EVENT_FREEZE|PM_EVENT_SUSPEND|PM_EVENT_HIBERNATE); > > + > > + dev->power.conditional_skip_pm = ret; > > + > > + return n; > > +} > > > + > > Redundant blank line. >
Thanks for the heads-up.
> > +static DEVICE_ATTR_RW(conditional_skip); > > -- > With Best Regards, > Andy Shevchenko > >
| |