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

Markus Probst
2025-10-06T12:11:49Z

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