mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v4 0/3] Support power resources defined in acpi on ata
@ 2025-10-23 16:52 Markus Probst
  2025-10-23 16:52 ` [PATCH v4 1/3] scsi: sd: Add manage_restart device attribute to scsi_disk Markus Probst
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Markus Probst @ 2025-10-23 16:52 UTC (permalink / raw)
  To: Damien Le Moal, Niklas Cassel, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-ide, linux-scsi, linux-kernel, 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 v3:
- rename function from "ata_port_set_power_state"
  to "ata_acpi_port_power_on" and remove enable argument
- split "ata_acpi_port_power_on" and "ata_acpi_dev_manage_restart" into
  two commits
- improved commit messages
- improved comments (style and new comments)

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 (3):
  scsi: sd: Add manage_restart device attribute to scsi_disk
  ata: stop disk on restart if ACPI power resources are found
  ata: Use ACPI methods to power on disks

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

-- 
2.51.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v4 1/3] scsi: sd: Add manage_restart device attribute to scsi_disk
  2025-10-23 16:52 [PATCH v4 0/3] Support power resources defined in acpi on ata Markus Probst
@ 2025-10-23 16:52 ` Markus Probst
  2025-10-23 16:52 ` [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found Markus Probst
  2025-10-23 16:52 ` [PATCH v4 3/3] ata: Use ACPI methods to power on disks Markus Probst
  2 siblings, 0 replies; 6+ messages in thread
From: Markus Probst @ 2025-10-23 16:52 UTC (permalink / raw)
  To: Damien Le Moal, Niklas Cassel, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-ide, linux-scsi, linux-kernel, 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.

This attribute is necessary for the following commit "ata: stop disk on
restart if ACPI power resources are found" to avoid a potential disk power
failure in the case the SATA power connector does not retain the power
state after a restart.

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 0252d3f6bed1..2ea244d6f998 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,
@@ -4177,7 +4208,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.51.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found
  2025-10-23 16:52 [PATCH v4 0/3] Support power resources defined in acpi on ata Markus Probst
  2025-10-23 16:52 ` [PATCH v4 1/3] scsi: sd: Add manage_restart device attribute to scsi_disk Markus Probst
@ 2025-10-23 16:52 ` Markus Probst
  2025-10-23 20:49   ` Damien Le Moal
  2025-10-23 16:52 ` [PATCH v4 3/3] ata: Use ACPI methods to power on disks Markus Probst
  2 siblings, 1 reply; 6+ messages in thread
From: Markus Probst @ 2025-10-23 16:52 UTC (permalink / raw)
  To: Damien Le Moal, Niklas Cassel, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-ide, linux-scsi, linux-kernel, Markus Probst

Some embedded devices have the ability to control whether power is
provided to the disks via the SATA power connector or not. ACPI power
resources are usually off by default, thus making it unclear if the
specific power resource will retain its state after a restart. If power
resources are defined on ATA ports / devices in ACPI, we should stop the
disk on SYSTEM_RESTART, to ensure the disk will not lose power while
active.

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.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index f2140fc06ba0..f05c247d25bc 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -245,6 +245,34 @@ 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, we assume the disk loses power on
+	 * reboot.
+	 */
+
+	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
+	 * `ata_device` instead of the `ata_port`.
+	 */
+	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_dissociate - dissociate ATA host from ACPI objects
  * @host: target ATA host
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b43a3196e2be..026122bb6f2f 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..af08bb9b40d0 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -130,6 +130,7 @@ 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 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 +141,7 @@ 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 bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
 #endif
 
 /* libata-scsi.c */
-- 
2.51.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v4 3/3] ata: Use ACPI methods to power on disks
  2025-10-23 16:52 [PATCH v4 0/3] Support power resources defined in acpi on ata Markus Probst
  2025-10-23 16:52 ` [PATCH v4 1/3] scsi: sd: Add manage_restart device attribute to scsi_disk Markus Probst
  2025-10-23 16:52 ` [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found Markus Probst
@ 2025-10-23 16:52 ` Markus Probst
  2025-10-23 20:56   ` Damien Le Moal
  2 siblings, 1 reply; 6+ messages in thread
From: Markus Probst @ 2025-10-23 16:52 UTC (permalink / raw)
  To: Damien Le Moal, Niklas Cassel, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-ide, linux-scsi, linux-kernel, 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_port_power_on(), that will be used to power
on the SATA power connector if usable ACPI power resources on the
associated ATA port / device 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 | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-core.c |  2 ++
 drivers/ata/libata.h      |  2 ++
 3 files changed, 44 insertions(+)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index f05c247d25bc..9d6177420e82 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -273,6 +273,46 @@ bool ata_acpi_dev_manage_restart(struct ata_device *dev)
 	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
 }
 
+/**
+ * ata_acpi_port_power_on - set the power state of the ata port to D0
+ * @ap: target ATA port
+ *
+ * This function is called at the beginning of ata_port_probe.
+ */
+void ata_acpi_port_power_on(struct ata_port *ap)
+{
+	acpi_handle handle;
+	int i;
+
+	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
+	 * `ata_device` instead of the `ata_port`.
+	 */
+	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, ACPI_STATE_D0))
+				ata_dev_dbg(&ap->link.device[i], "acpi: power on\n");
+			else
+				ata_dev_err(&ap->link.device[i], "acpi: failed to 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, ACPI_STATE_D0))
+		ata_port_dbg(ap, "acpi: power on\n");
+	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 2a210719c4ce..a6813ced3ec2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5901,6 +5901,8 @@ void ata_port_probe(struct ata_port *ap)
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned long flags;
 
+	ata_acpi_port_power_on(ap);
+
 	/* kick EH for boot probing */
 	spin_lock_irqsave(ap->lock, flags);
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index af08bb9b40d0..0e7ecac73680 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -130,6 +130,7 @@ 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_power_on(struct ata_port *ap);
 extern bool ata_acpi_dev_manage_restart(struct ata_device *dev);
 extern acpi_handle ata_dev_acpi_handle(struct ata_device *dev);
 #else
@@ -141,6 +142,7 @@ 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_power_on(struct ata_port *ap) {}
 static inline bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
 #endif
 
-- 
2.51.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found
  2025-10-23 16:52 ` [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found Markus Probst
@ 2025-10-23 20:49   ` Damien Le Moal
  0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2025-10-23 20:49 UTC (permalink / raw)
  To: Markus Probst, Niklas Cassel, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-ide, linux-scsi, linux-kernel

On 2025/10/23 9:52, 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. ACPI power
> resources are usually off by default, thus making it unclear if the
> specific power resource will retain its state after a restart. If power
> resources are defined on ATA ports / devices in ACPI, we should stop the
> disk on SYSTEM_RESTART, to ensure the disk will not lose power while
> active.
> 
> 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.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/ata/libata-acpi.c | 28 ++++++++++++++++++++++++++++
>  drivers/ata/libata-scsi.c |  1 +
>  drivers/ata/libata.h      |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index f2140fc06ba0..f05c247d25bc 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -245,6 +245,34 @@ 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.

Please align "system restart" to the "if" above.

> + * @dev: target ATA device
> + *
> + * RETURNS:
> + * 1 if the disk should be stopped, otherwise 0

s/1/true
s/0/false

And please terminate sentences with a period.

> + */
> +bool ata_acpi_dev_manage_restart(struct ata_device *dev)
> +{
> +	/* If the device is power manageable, we assume the disk loses power on
> +	 * reboot.
> +	 */

And then ? This seems incomplete...
Also please adhere to the normal multi-line comment style by starting the
multi-line comment block with "/*".

	/*
	 * If the device is power manageable, we assume the disk loses power on
	 * reboot.
	 */

> +
> +	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
> +	 * `ata_device` instead of the `ata_port`.
> +	 */

No need for all the quote marks here.

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

			return false;

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

		return false;

> +	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
> +}

Overall, I think the following is cleaner and simpler as it doe not repeat code:

bool ata_acpi_dev_manage_restart(struct ata_device *dev)
{
	struct device *tdev;

	/*
	 * If ATA_FLAG_ACPI_SATA is set, the acpi fwnode is attached to the
	 * ATA device instead of the port.
	 */
	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA)
		tdev = &dev->tdev;
	else
		tdev = &dev->link->ap->tdev;

	if (!is_acpi_device_node(tdev->fwnode))
		return false;

	return acpi_bus_power_manageable(ACPI_HANDLE(tdev));
}

> +
>  /**
>   * ata_acpi_dissociate - dissociate ATA host from ACPI objects
>   * @host: target ATA host
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b43a3196e2be..026122bb6f2f 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..af08bb9b40d0 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -130,6 +130,7 @@ 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 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 +141,7 @@ 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 bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
>  #endif
>  
>  /* libata-scsi.c */


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 3/3] ata: Use ACPI methods to power on disks
  2025-10-23 16:52 ` [PATCH v4 3/3] ata: Use ACPI methods to power on disks Markus Probst
@ 2025-10-23 20:56   ` Damien Le Moal
  0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2025-10-23 20:56 UTC (permalink / raw)
  To: Markus Probst, Niklas Cassel, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-ide, linux-scsi, linux-kernel

On 2025/10/23 9:52, 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_port_power_on(), that will be used to power
> on the SATA power connector if usable ACPI power resources on the
> associated ATA port / device 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 | 40 +++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-core.c |  2 ++
>  drivers/ata/libata.h      |  2 ++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index f05c247d25bc..9d6177420e82 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -273,6 +273,46 @@ bool ata_acpi_dev_manage_restart(struct ata_device *dev)
>  	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
>  }
>  
> +/**
> + * ata_acpi_port_power_on - set the power state of the ata port to D0
> + * @ap: target ATA port
> + *
> + * This function is called at the beginning of ata_port_probe.
> + */
> +void ata_acpi_port_power_on(struct ata_port *ap)
> +{
> +	acpi_handle handle;
> +	int i;
> +
> +	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
> +	 * `ata_device` instead of the `ata_port`.
> +	 */
> +	if (ap->flags & ATA_FLAG_ACPI_SATA) {
> +		for (i = 0; i < ATA_MAX_DEVICES; i++) {

Adding:

			struct device *tdev = &ap->link.device[i].tdev;

will simplify the code a little below, and make the lines shorter (they are too
long).
> +			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, ACPI_STATE_D0))
> +				ata_dev_dbg(&ap->link.device[i], "acpi: power on\n");

Long line. Please split.
But is this debug message really needed ?

> +			else
> +				ata_dev_err(&ap->link.device[i], "acpi: failed to power state\n");

Very long line. Please split.

> +		}
> +		return;
> +	}

A blank line here would be nice.

> +	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, ACPI_STATE_D0))
> +		ata_port_dbg(ap, "acpi: power on\n");
> +	else
> +		ata_port_err(ap, "acpi: failed to set power state\n");

"to set power state" is not very clear. Which state ? Please rephrase this
message. Same remark for the same message above in the for loop

> +}
> +
>  /**
>   * 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 2a210719c4ce..a6813ced3ec2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5901,6 +5901,8 @@ void ata_port_probe(struct ata_port *ap)
>  	struct ata_eh_info *ehi = &ap->link.eh_info;
>  	unsigned long flags;
>  
> +	ata_acpi_port_power_on(ap);
> +
>  	/* kick EH for boot probing */
>  	spin_lock_irqsave(ap->lock, flags);
>  
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index af08bb9b40d0..0e7ecac73680 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -130,6 +130,7 @@ 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_power_on(struct ata_port *ap);
>  extern bool ata_acpi_dev_manage_restart(struct ata_device *dev);
>  extern acpi_handle ata_dev_acpi_handle(struct ata_device *dev);
>  #else
> @@ -141,6 +142,7 @@ 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_power_on(struct ata_port *ap) {}
>  static inline bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
>  #endif
>  


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-10-23 20:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-10-23 16:52 [PATCH v4 0/3] Support power resources defined in acpi on ata Markus Probst
2025-10-23 16:52 ` [PATCH v4 1/3] scsi: sd: Add manage_restart device attribute to scsi_disk Markus Probst
2025-10-23 16:52 ` [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found Markus Probst
2025-10-23 20:49   ` Damien Le Moal
2025-10-23 16:52 ` [PATCH v4 3/3] ata: Use ACPI methods to power on disks Markus Probst
2025-10-23 20:56   ` Damien Le Moal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®