[PATCH 0/2] Support power resources in ata ports

Markus Probst posted 2 patches 2 months, 1 week ago
Only 1 patches received!
drivers/ata/libata-core.c  | 21 ++++++++++++++
drivers/ata/libata-scsi.c  |  1 +
drivers/ata/libata.h       |  4 +++
drivers/scsi/sd.c          | 35 +++++++++++++++++++++++-
include/linux/libata.h     |  1 +
include/scsi/scsi_device.h |  6 ++++
7 files changed, 135 insertions(+), 1 deletion(-)
[PATCH 0/2] Support power resources in ata ports
Posted by Markus Probst 2 months, 1 week ago
This series adds support for power resources defined in acpi for ata
ports. A device can define a power resource in an ata port, which then
gets turned on right before the port is probed. This can be useful for
devices, which have ata ports that are:
  a: powered down by default
  b: can be individually powered on
like in every synology nas device that supports a feature they call
"deep-sleep". If thats the case it will be assumed, that the power
resource won't survive reboots and therefore the disk will be stopped.

I will maybe extend it later to allow
  - setting a delay between the probes (embedded devices have limited
power and if the disks are all spin up at the same time it would cause
a power peak)
  - powering down the ata ports (removing power from the disks) while
the disk is spin down (saving power and possibly disk lifetime)


For details, look at the individual commit messages.

Markus Probst (2):
  Add manage_restart device attribute to scsi_disk
  Power on ata ports defined in ACPI before probing ports

 drivers/ata/libata-acpi.c  | 68
++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-core.c  | 21 ++++++++++++++
 drivers/ata/libata-scsi.c  |  1 +
 drivers/ata/libata.h       |  4 +++
 drivers/scsi/sd.c          | 35 +++++++++++++++++++++++-
 include/linux/libata.h     |  1 +
 include/scsi/scsi_device.h |  6 ++++
 7 files changed, 135 insertions(+), 1 deletion(-)
[PATCH v2 0/2] Support power resources defined in acpi on ata
Posted by Markus Probst 2 months, 1 week ago
This series adds support for power resources defined in acpi for 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 ata ports 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 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 ata ports

 drivers/ata/libata-acpi.c  | 70 ++++++++++++++++++++++++++++++++++++++
 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, 117 insertions(+), 1 deletion(-)

-- 
2.49.1
[PATCH v2 1/2] scsi: sd: Add manage_restart device attribute to scsi_disk
Posted by Markus Probst 2 months, 1 week ago
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
[PATCH v2 2/2] ata: Use ACPI methods to power on ata ports
Posted by Markus Probst 2 months, 1 week ago
Some embedded devices, including many Synology NAS devices, have the
ability to control whether a ATA port has power or not.

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 an ata port if usable ACPI power resources are
found. It will be called right before probing the port, therefore the port
will be powered on just in time.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index f2140fc06ba0..bba5ef49f055 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -245,6 +245,76 @@ void ata_acpi_bind_dev(struct ata_device *dev)
 				   ata_acpi_dev_uevent);
 }
 
+/**
+ * ata_acpi_dev_manage_restart - if the disk should be stopped (spin down) on
+ * system restart.
+ * @dev: target ATA device
+ *
+ * RETURNS:
+ * true if the disk should be stopped, otherwise false
+ */
+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
+ *
+ * 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 v2 2/2] ata: Use ACPI methods to power on ata ports
Posted by kernel test robot 2 months, 1 week ago
Hi Markus,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jejb-scsi/for-next]
[also build test WARNING on mkp-scsi/for-next linus/master v6.17 next-20251009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Probst/scsi-sd-Add-manage_restart-device-attribute-to-scsi_disk/20251010-111134
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/20251009112433.108643-3-markus.probst%40posteo.de
patch subject: [PATCH v2 2/2] ata: Use ACPI methods to power on ata ports
config: x86_64-randconfig-071-20251010 (https://download.01.org/0day-ci/archive/20251010/202510102322.yF4HEkAc-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251010/202510102322.yF4HEkAc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510102322.yF4HEkAc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/ata/libata-acpi.c:277 function parameter 'enable' not described in 'ata_acpi_port_set_power_state'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2 2/2] ata: Use ACPI methods to power on ata ports
Posted by Niklas Cassel 2 months, 1 week ago
On Thu, Oct 09, 2025 at 11:24:49AM +0000, Markus Probst wrote:
> Some embedded devices, including many Synology NAS devices, have the
> ability to control whether a ATA port has power or not.

In V1, you mentioned that it was to control the SATA power supply,
now you mention the ATA port. I am confused.

If it is for the ATA port, then SATA already has support for this,
using PxSCTL.

How does this ACPI way to control power interact with the regular
way to control power for a port using PxSCTL?


> 
> 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 an ata port if usable ACPI power resources are
> found. It will be called right before probing the port, therefore the port
> will be powered on just in time.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/ata/libata-acpi.c | 70 +++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-core.c |  2 ++
>  drivers/ata/libata-scsi.c |  1 +
>  drivers/ata/libata.h      |  4 +++
>  4 files changed, 77 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index f2140fc06ba0..bba5ef49f055 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -245,6 +245,76 @@ void ata_acpi_bind_dev(struct ata_device *dev)
>  				   ata_acpi_dev_uevent);
>  }
>  
> +/**
> + * ata_acpi_dev_manage_restart - if the disk should be stopped (spin down) on
> + * system restart.
> + * @dev: target ATA device
> + *
> + * RETURNS:
> + * true if the disk should be stopped, otherwise false
> + */
> +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.

Like Damien mentioned earlier, please no C++ style comments.


Kind regards,
Niklas
Re: [PATCH v2 2/2] ata: Use ACPI methods to power on ata ports
Posted by Markus Probst 2 months, 1 week ago
On Thu, 2025-10-09 at 13:59 +0200, Niklas Cassel wrote:
> On Thu, Oct 09, 2025 at 11:24:49AM +0000, Markus Probst wrote:
> > Some embedded devices, including many Synology NAS devices, have
> > the
> > ability to control whether a ATA port has power or not.
> 
> In V1, you mentioned that it was to control the SATA power supply,
> now you mention the ATA port. I am confused.
The power supply associated with the ata port (or to be more precise,
it controls the power gate to the SATA Power connector for the specific
disk, at least in the synology example). It sets the power state
defined in ACPI on the ata port. I might need to update the wording
used in the patches to make it more clear.

> 
> If it is for the ATA port, then SATA already has support for this,
> using PxSCTL.
> 
> How does this ACPI way to control power interact with the regular
> way to control power for a port using PxSCTL?
> 
> 
> > 
> > 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 an ata port if usable ACPI power resources
> > are
> > found. It will be called right before probing the port, therefore
> > the port
> > will be powered on just in time.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  drivers/ata/libata-acpi.c | 70
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/ata/libata-core.c |  2 ++
> >  drivers/ata/libata-scsi.c |  1 +
> >  drivers/ata/libata.h      |  4 +++
> >  4 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index f2140fc06ba0..bba5ef49f055 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -245,6 +245,76 @@ void ata_acpi_bind_dev(struct ata_device *dev)
> >  				   ata_acpi_dev_uevent);
> >  }
> >  
> > +/**
> > + * ata_acpi_dev_manage_restart - if the disk should be stopped
> > (spin down) on
> > + * system restart.
> > + * @dev: target ATA device
> > + *
> > + * RETURNS:
> > + * true if the disk should be stopped, otherwise false
> > + */
> > +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.
> 
> Like Damien mentioned earlier, please no C++ style comments.
> 
> 
> Kind regards,
> Niklas

Also for the questions sent to the v1 patch:

> Since this patch implements something similar to DevSleep, but
> rather,
> IIUC, for the SATA power itself?

Yes


> How is SATA power supplied tied to a port in ACPI? If you have a
desktop
> you have a PSU, and don't really know which supply is for which port.

It is not for desktop computers, but for embedded devices.


> So, considering how many ways we already have to disable/power off a
port,
> you might understand why I think it is extra important that you
document
> exactly how, and why we need yet another way to disable/power on/off
a port.

In this case,
- According to ACPI spec, if a device has a power resource defined it
has to be turned on before we are able send commands to the device
- Because there is hardware out there, that is perfectly capable of
running upstream linux, which doesn't use one of the methods you
mentioned for controlling the power. Same did apply for the USB VBus
power, despite there being "USB_PORT_FEAT_POWER" in the spec and it
also got in the kernel, see commit
f7ac7787ad361e31a7972e2854ed8dc2eedfac3b.

I will try to add a short version of the reason written above in the
commit message.


Thanks,
- Markus Probst
Re: [PATCH v2 2/2] ata: Use ACPI methods to power on ata ports
Posted by Niklas Cassel 2 months, 1 week ago
Also, when sending a new version, please avoid linking using In-Reply-To to
previous versions of the same series, as it creates an "unmanageable forest
of references in email clients", see:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#explicit-in-reply-to-headers


Kind regards,
Niklas