[PATCH v3 0/2] Support power resources defined in acpi on ata

[PATCH v3 0/2] Support power resources defined in acpi on ata

[Markus Probst]

This series adds support for power resources defined in acpi on ata
ports/devices. A device can define a power resource in an ata port/device,
which then gets powered on right before the port is probed. This can be
useful for devices, which have sata power connectors that are:
  a: powered down by default
  b: can be individually powered on
like in some synology nas devices. If thats the case it will be assumed,
that the power resource won't survive reboots and therefore the disk will
be stopped.

Changes since v2:
- improved commit messages
- addressed warning from kernel test robot

Changes since v1:
- improved commit messages
- addressed style issues (too long lines and docs)
- removed ata_dev_manage_restart() and ata_port_set_power_state()
  methods
- improved log messages in ata_acpi_port_set_power_state


Markus Probst (2):
  scsi: sd: Add manage_restart device attribute to scsi_disk
  ata: Use ACPI methods to power on disks

 drivers/ata/libata-acpi.c  | 71 ++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-core.c  |  2 ++
 drivers/ata/libata-scsi.c  |  1 +
 drivers/ata/libata.h       |  4 +++
 drivers/scsi/sd.c          | 35 ++++++++++++++++++-
 include/scsi/scsi_device.h |  6 ++++
 6 files changed, 118 insertions(+), 1 deletion(-)

-- 
2.49.1

[PATCH v3 1/2] scsi: sd: Add manage_restart device attribute to scsi_disk

[Markus Probst]

In addition to the already existing manage_shutdown,
manage_system_start_stop and manage_runtime_start_stop device
scsi_disk attributes, add manage_restart, which allows the high-level
device driver (sd) to manage the device power state for SYSTEM_RESTART if set to 1.

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;
+
+	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;
+	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.
+	 */
+	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.
-- 
2.49.1

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

[Niklas Cassel]

On Fri, Oct 10, 2025 at 10:38:34PM +0000, Markus Probst wrote:
> In addition to the already existing manage_shutdown,
> manage_system_start_stop and manage_runtime_start_stop device
> scsi_disk attributes, add manage_restart, which allows the high-level
> device driver (sd) to manage the device power state for SYSTEM_RESTART if set to 1.

The commit message explains what is done, but does not answer the question:
"why?"

I can read patch 2/2 and understand why, but considering that this is a patch
for SCSI and not ATA, I think that you need to provide some context here, so
that someone doing a git log drivers/scsi/ will actually have any idea of why
this was done.


Kind regards,
Niklas

[PATCH v3 2/2] ata: Use ACPI methods to power on disks

[Markus Probst]

Some embedded devices have the ability to control whether power is
provided to the disks via the sata power connector or not. If power
resources are defined on ata ports / devices in ACPI, we should try to set
the power state to D0 before probing the disk to ensure that any power
supply or power gate that may exist is providing power to the disk.

An example for such devices would be newer synology nas devices. Every
disk slot has its own sata power connector. Whether the connector is
providing power is controlled via an gpio, which is *off by default*.
Also the disk loses power on reboots.

Add a new function, ata_acpi_dev_manage_restart(), that will be used to
determine if a disk should be stopped before restarting the system. If a
usable ACPI power resource has been found, it is assumed that the disk
will lose power after a restart and should be stopped to avoid a power
failure. Also add a new function, ata_acpi_port_set_power_state(), that
will be used to power on the sata power connector if usable ACPI power
resources on the associated ata port are found. It will be called right
before probing the port, therefore the disk will be powered on just in
time.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/ata/libata-acpi.c | 71 +++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-core.c |  2 ++
 drivers/ata/libata-scsi.c |  1 +
 drivers/ata/libata.h      |  4 +++
 4 files changed, 78 insertions(+)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index f2140fc06ba0..4a72a98b922c 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -245,6 +245,77 @@ void ata_acpi_bind_dev(struct ata_device *dev)
 				   ata_acpi_dev_uevent);
 }
 
+/**
+ * ata_acpi_dev_manage_restart - if the disk should be stopped (spun down) on
+ * system restart.
+ * @dev: target ATA device
+ *
+ * RETURNS:
+ * 1 if the disk should be stopped, otherwise 0
+ */
+bool ata_acpi_dev_manage_restart(struct ata_device *dev)
+{
+	// If the device is power manageable and we assume the disk loses power
+	// on reboot.
+	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA) {
+		if (!is_acpi_device_node(dev->tdev.fwnode))
+			return 0;
+		return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
+	}
+
+	if (!is_acpi_device_node(dev->link->ap->tdev.fwnode))
+		return 0;
+	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
+}
+
+/**
+ * ata_acpi_port_set_power_state - set the power state of the ata port
+ * @ap: target ATA port
+ * @enable: power state to be set
+ *
+ * This function is called at the beginning of ata_port_probe.
+ */
+void ata_acpi_port_set_power_state(struct ata_port *ap, bool enable)
+{
+	acpi_handle handle;
+	unsigned char state;
+	int i;
+
+	if (libata_noacpi)
+		return;
+
+	if (enable)
+		state = ACPI_STATE_D0;
+	else
+		state = ACPI_STATE_D3_COLD;
+
+	if (ap->flags & ATA_FLAG_ACPI_SATA) {
+		for (i = 0; i < ATA_MAX_DEVICES; i++) {
+			if (!is_acpi_device_node(ap->link.device[i].tdev.fwnode))
+				continue;
+			handle = ACPI_HANDLE(&ap->link.device[i].tdev);
+			if (!acpi_bus_power_manageable(handle))
+				continue;
+			if (!acpi_bus_set_power(handle, state))
+				ata_dev_dbg(&ap->link.device[i], "acpi: power was %s\n",
+					     enable ? "en" : "dis");
+			else
+				ata_dev_err(&ap->link.device[i], "acpi: failed to set power state\n");
+		}
+		return;
+	}
+	if (!is_acpi_device_node(ap->tdev.fwnode))
+		return;
+	handle = ACPI_HANDLE(&ap->tdev);
+	if (!acpi_bus_power_manageable(handle))
+		return;
+
+	if (!acpi_bus_set_power(handle, state))
+		ata_port_dbg(ap, "acpi: power %sabled\n", enable ? "en" : "dis");
+	else
+		ata_port_err(ap, "acpi: failed to set power state\n");
+}
+
 /**
  * ata_acpi_dissociate - dissociate ATA host from ACPI objects
  * @host: target ATA host
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ff53f5f029b4..ee8b504596b2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5904,6 +5904,8 @@ void ata_port_probe(struct ata_port *ap)
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned long flags;
 
+	ata_acpi_port_set_power_state(ap, true);
+
 	/* kick EH for boot probing */
 	spin_lock_irqsave(ap->lock, flags);
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2ded5e476d6e..12dd305afe26 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1095,6 +1095,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
 		 */
 		sdev->manage_runtime_start_stop = 1;
 		sdev->manage_shutdown = 1;
+		sdev->manage_restart = ata_acpi_dev_manage_restart(dev);
 		sdev->force_runtime_start_on_system_start = 1;
 	}
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index e5b977a8d3e1..c6daa2b1d15f 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -130,6 +130,8 @@ extern void ata_acpi_on_disable(struct ata_device *dev);
 extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 extern void ata_acpi_bind_port(struct ata_port *ap);
 extern void ata_acpi_bind_dev(struct ata_device *dev);
+extern void ata_acpi_port_set_power_state(struct ata_port *ap, bool enable);
+extern bool ata_acpi_dev_manage_restart(struct ata_device *dev);
 extern acpi_handle ata_dev_acpi_handle(struct ata_device *dev);
 #else
 static inline void ata_acpi_dissociate(struct ata_host *host) { }
@@ -140,6 +142,8 @@ static inline void ata_acpi_set_state(struct ata_port *ap,
 				      pm_message_t state) { }
 static inline void ata_acpi_bind_port(struct ata_port *ap) {}
 static inline void ata_acpi_bind_dev(struct ata_device *dev) {}
+static inline void ata_acpi_port_set_power_state(struct ata_port *ap, bool enable) {}
+static inline bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
 #endif
 
 /* libata-scsi.c */
-- 
2.49.1

Re: [PATCH v3 2/2] ata: Use ACPI methods to power on disks

[Niklas Cassel]

On Fri, Oct 10, 2025 at 10:38:35PM +0000, Markus Probst wrote:
> Some embedded devices have the ability to control whether power is
> provided to the disks via the sata power connector or not. If power
> resources are defined on ata ports / devices in ACPI, we should try to set
> the power state to D0 before probing the disk to ensure that any power
> supply or power gate that may exist is providing power to the disk.
> 
> An example for such devices would be newer synology nas devices. Every
> disk slot has its own sata power connector. Whether the connector is
> providing power is controlled via an gpio, which is *off by default*.
> Also the disk loses power on reboots.
> 
> Add a new function, ata_acpi_dev_manage_restart(), that will be used to
> determine if a disk should be stopped before restarting the system. If a
> usable ACPI power resource has been found, it is assumed that the disk
> will lose power after a restart and should be stopped to avoid a power
> failure. Also add a new function, ata_acpi_port_set_power_state(), that
> will be used to power on the sata power connector if usable ACPI power
> resources on the associated ata port are found. It will be called right
> before probing the port, therefore the disk will be powered on just in
> time.

s/sata/SATA/
s/nas/NAS/
s/ata/ATA/ (except for function names of course)

Since this patch is basically doing two logical changes
1) Calling ata_acpi_dev_manage_restart() on restart, which calls
acpi_bus_power_manageable() to disable power on shutdown.

2) Calling ata_acpi_port_set_power_state() during ata_port_probe(),
to enable power.

Please also split this patch into two, so that we have one commit per
logical change. That would make things easier to understand, as your
commit message your just describe one behavior instead of two completely
different behaviors.


Your commit message mentions that you want to spin down the disk on
restart to avoid "avoid a power failure".

Is there a reason why you call acpi_bus_power_manageable() to spin down
the disk instead of the regular function: ata_dev_power_set_standby()
which spins down the disk?


> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/ata/libata-acpi.c | 71 +++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-core.c |  2 ++
>  drivers/ata/libata-scsi.c |  1 +
>  drivers/ata/libata.h      |  4 +++
>  4 files changed, 78 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index f2140fc06ba0..4a72a98b922c 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -245,6 +245,77 @@ void ata_acpi_bind_dev(struct ata_device *dev)
>  				   ata_acpi_dev_uevent);
>  }
>  
> +/**
> + * ata_acpi_dev_manage_restart - if the disk should be stopped (spun down) on
> + * system restart.
> + * @dev: target ATA device
> + *
> + * RETURNS:
> + * 1 if the disk should be stopped, otherwise 0
> + */
> +bool ata_acpi_dev_manage_restart(struct ata_device *dev)
> +{
> +	// If the device is power manageable and we assume the disk loses power
> +	// on reboot.

Please no C++ style comments.

Also "If the device is power manageable and we assume"
should this not be
"If the device is power manageable, we assume"

Because your commit message says:
"If a usable ACPI power resource has been found, it is assumed that the disk
will lose power after a restart"

so I think the word "and" here is wrong.


> +	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA) {
> +		if (!is_acpi_device_node(dev->tdev.fwnode))
> +			return 0;
> +		return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
> +	}
> +

Please add a commend here explaining the difference between the
two cases, because you call either:
return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
or
return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));

At least the difference is not obvious to me, from just looking at this
function.


> +	if (!is_acpi_device_node(dev->link->ap->tdev.fwnode))
> +		return 0;
> +	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
> +}
> +
> +/**
> + * ata_acpi_port_set_power_state - set the power state of the ata port
> + * @ap: target ATA port
> + * @enable: power state to be set
> + *
> + * This function is called at the beginning of ata_port_probe.
> + */
> +void ata_acpi_port_set_power_state(struct ata_port *ap, bool enable)

This function is never called with enable==false, so let's please remove
this parameter and rename the function to something like
ata_acpi_port_enable_power() or similar.
If someone a future patch ever wants to refactor this to also handle
disable, then that patch can also create a parameter for this function.
Otherwise we are just adding dead code.


Kind regards,
Niklas

Re: [PATCH v3 2/2] ata: Use ACPI methods to power on disks

[Markus Probst]

On Mon, 2025-10-13 at 10:20 +0200, Niklas Cassel wrote:
> On Fri, Oct 10, 2025 at 10:38:35PM +0000, Markus Probst wrote:
> > Some embedded devices have the ability to control whether power is
> > provided to the disks via the sata power connector or not. If power
> > resources are defined on ata ports / devices in ACPI, we should try
> > to set
> > the power state to D0 before probing the disk to ensure that any
> > power
> > supply or power gate that may exist is providing power to the disk.
> > 
> > An example for such devices would be newer synology nas devices.
> > Every
> > disk slot has its own sata power connector. Whether the connector
> > is
> > providing power is controlled via an gpio, which is *off by
> > default*.
> > Also the disk loses power on reboots.
> > 
> > Add a new function, ata_acpi_dev_manage_restart(), that will be
> > used to
> > determine if a disk should be stopped before restarting the system.
> > If a
> > usable ACPI power resource has been found, it is assumed that the
> > disk
> > will lose power after a restart and should be stopped to avoid a
> > power
> > failure. Also add a new function, ata_acpi_port_set_power_state(),
> > that
> > will be used to power on the sata power connector if usable ACPI
> > power
> > resources on the associated ata port are found. It will be called
> > right
> > before probing the port, therefore the disk will be powered on just
> > in
> > time.
> 
> s/sata/SATA/
> s/nas/NAS/
> s/ata/ATA/ (except for function names of course)
> 
> Since this patch is basically doing two logical changes
> 1) Calling ata_acpi_dev_manage_restart() on restart, which calls
> acpi_bus_power_manageable() to disable power on shutdown.
> 
> 2) Calling ata_acpi_port_set_power_state() during ata_port_probe(),
> to enable power.
> 
> Please also split this patch into two, so that we have one commit per
> logical change. That would make things easier to understand, as your
> commit message your just describe one behavior instead of two
> completely
> different behaviors.
> 
> 
> Your commit message mentions that you want to spin down the disk on
> restart to avoid "avoid a power failure".
> 
> Is there a reason why you call acpi_bus_power_manageable() to spin
> down
> the disk instead of the regular function: ata_dev_power_set_standby()
> which spins down the disk?
I don't use acpi_bus_power_manageable() to spin down the disk. It just
checks if there is a power resource present in acpi for the ata port /
device. If thats the case, scsi should spin the disk down on
SYSTEM_RESTART.
> 
> 
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  drivers/ata/libata-acpi.c | 71
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/ata/libata-core.c |  2 ++
> >  drivers/ata/libata-scsi.c |  1 +
> >  drivers/ata/libata.h      |  4 +++
> >  4 files changed, 78 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index f2140fc06ba0..4a72a98b922c 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -245,6 +245,77 @@ void ata_acpi_bind_dev(struct ata_device *dev)
> >  				   ata_acpi_dev_uevent);
> >  }
> >  
> > +/**
> > + * ata_acpi_dev_manage_restart - if the disk should be stopped
> > (spun down) on
> > + * system restart.
> > + * @dev: target ATA device
> > + *
> > + * RETURNS:
> > + * 1 if the disk should be stopped, otherwise 0
> > + */
> > +bool ata_acpi_dev_manage_restart(struct ata_device *dev)
> > +{
> > +	// If the device is power manageable and we assume the
> > disk loses power
> > +	// on reboot.
> 
> Please no C++ style comments.
> 
> Also "If the device is power manageable and we assume"
> should this not be
> "If the device is power manageable, we assume"
> 
> Because your commit message says:
> "If a usable ACPI power resource has been found, it is assumed that
> the disk
> will lose power after a restart"
> 
> so I think the word "and" here is wrong.
> 
> 
> > +	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA) {
> > +		if (!is_acpi_device_node(dev->tdev.fwnode))
> > +			return 0;
> > +		return acpi_bus_power_manageable(ACPI_HANDLE(&dev-
> > >tdev));
> > +	}
> > +
> 
> Please add a commend here explaining the difference between the
> two cases, because you call either:
> return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
> or
> return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
> 
> At least the difference is not obvious to me, from just looking at
> this
> function.
if ATA_FLAG_ACPI_SATA is set, the acpi fwnode with the power resources
is not set on the ata_port->tdev, but on the ata_device->tdev. See
ata_acpi_bind_port (return if ATA_FLAG_ACPI_SATA is set) and
ata_acpi_bind_dev (return if ATA_FLAG_ACPI_SATA is not set).
I will add a comment for this.

Thanks
- Markus Probst

> 
> 
> > +	if (!is_acpi_device_node(dev->link->ap->tdev.fwnode))
> > +		return 0;
> > +	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link-
> > >ap->tdev));
> > +}
> > +
> > +/**
> > + * ata_acpi_port_set_power_state - set the power state of the ata
> > port
> > + * @ap: target ATA port
> > + * @enable: power state to be set
> > + *
> > + * This function is called at the beginning of ata_port_probe.
> > + */
> > +void ata_acpi_port_set_power_state(struct ata_port *ap, bool
> > enable)
> 
> This function is never called with enable==false, so let's please
> remove
> this parameter and rename the function to something like
> ata_acpi_port_enable_power() or similar.
> If someone a future patch ever wants to refactor this to also handle
> disable, then that patch can also create a parameter for this
> function.
> Otherwise we are just adding dead code.
> 
> 
> Kind regards,
> Niklas