Re: [PATCH 1/2] Add manage_restart device attribute to scsi_disk

[Markus Probst <markus.probst@posteo.de>]

On Mon, 2025-10-06 at 09:58 +0900, Damien Le Moal wrote:
> On 10/6/25 04:06, Markus Probst wrote:
> > there is already manage_shutdown, manage_system_start_stop and
> > manage_runtime_start_stop device attributes that allows the high-
> > level
> > device driver (sd) manage the device power state, expect for the
> > system_state SYSTEM_RESTART. With this device attribute, it is
> > possible to
> > let the high-level device driver (sd) manage the device power state
> > for
> > SYSTEM_RESTART too.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  drivers/scsi/sd.c          | 35
> > ++++++++++++++++++++++++++++++++++-
> >  include/scsi/scsi_device.h |  6 ++++++
> >  2 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 5b8668accf8e..a3e9c2e9d9f4 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -318,6 +318,36 @@ static ssize_t manage_shutdown_store(struct
> > device *dev,
> >  }
> >  static DEVICE_ATTR_RW(manage_shutdown);
> >  
> > +static ssize_t manage_restart_show(struct device *dev,
> > +				   struct device_attribute *attr,
> > char *buf)
> > +{
> > +	struct scsi_disk *sdkp = to_scsi_disk(dev);
> > +	struct scsi_device *sdp = sdkp->device;
> 
> sdp is not really needed.

Then it would be inconsistent with manage_shutdown_*,
manage_runtime_start_stop_*, manage_system_start_stop_*, there it has
the sdp variable declared.

> 
> > +
> > +	return sysfs_emit(buf, "%u\n", sdp->manage_restart);
> > +}
> > +
> > +
> > +static ssize_t manage_restart_store(struct device *dev,
> > +				    struct device_attribute *attr,
> > +				    const char *buf, size_t count)
> > +{
> > +	struct scsi_disk *sdkp = to_scsi_disk(dev);
> > +	struct scsi_device *sdp = sdkp->device;
> 
> Same here.
> 
> > +	bool v;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EACCES;
> > +
> > +	if (kstrtobool(buf, &v))
> > +		return -EINVAL;
> > +
> > +	sdp->manage_restart = v;
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(manage_restart);
> > +
> >  static ssize_t
> >  allow_restart_show(struct device *dev, struct device_attribute
> > *attr, char *buf)
> >  {
> > @@ -654,6 +684,7 @@ static struct attribute *sd_disk_attrs[] = {
> >  	&dev_attr_manage_system_start_stop.attr,
> >  	&dev_attr_manage_runtime_start_stop.attr,
> >  	&dev_attr_manage_shutdown.attr,
> > +	&dev_attr_manage_restart.attr,
> >  	&dev_attr_protection_type.attr,
> >  	&dev_attr_protection_mode.attr,
> >  	&dev_attr_app_tag_own.attr,
> > @@ -4175,7 +4206,9 @@ static void sd_shutdown(struct device *dev)
> >  	    (system_state == SYSTEM_POWER_OFF &&
> >  	     sdkp->device->manage_shutdown) ||
> >  	    (system_state == SYSTEM_RUNNING &&
> > -	     sdkp->device->manage_runtime_start_stop)) {
> > +	     sdkp->device->manage_runtime_start_stop) ||
> > +	    (system_state == SYSTEM_RESTART &&
> > +	     sdkp->device->manage_restart)) {
> >  		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
> >  		sd_start_stop_device(sdkp, 0);
> >  	}
> > diff --git a/include/scsi/scsi_device.h
> > b/include/scsi/scsi_device.h
> > index 6d6500148c4b..c7e657ac8b6d 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -178,6 +178,12 @@ struct scsi_device {
> >  	 */
> >  	unsigned manage_shutdown:1;
> >  
> > +	/*
> > +	 * If true, let the high-level device driver (sd) manage
> > the device
> > +	 * power state for system restart (reboot) operations.
> 
> What about cold boot ? Same is needed, no ?
Not sure what you mean with cold boot here.

> The name "manage_restart" is a bit
> confusing since we already have manage_system_start_stop,
> manage_runtime_start_stop and manage_shutdown. I do not have a better
> suggestion
> though.
In that case, we could remove the DEVICE_ATTR_RW(manage_restart), so it
would not be exposed to userspace yet and can later by changed without
breaking userspace?
> 
> > +	 */
> > +	unsigned manage_restart:1;
> > +
> >  	/*
> >  	 * If set and if the device is runtime suspended, ask the
> > high-level
> >  	 * device driver (sd) to force a runtime resume of the
> > device.
> 

Thanks
- Markus Probst