| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7ab0ef5c-cce7-44d7-a416-4963e24e0b17@kernel.org>
Date: Mon, 6 Oct 2025 10:09:00 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Markus Probst <markus.probst@...teo.de>, Niklas Cassel
<cassel@...nel.org>,
"James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>
Cc: linux-ide@...r.kernel.org, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Power on ata ports defined in ACPI before probing
ports
On 10/6/25 04:06, Markus Probst wrote:
> some devices (for example every synology nas with "deep sleep" support)
> have the ability to power on and off each ata port individually. This
> turns the specific port on, if power manageable in acpi, before probing
> it.
Please add "ata: " prefix to the commit titel and capitalize the first letter of
sentences.
The commit message above should also be improved. E.g. The "This" in "This turns
on..." is unclear (I am assuming you mean "this patch" ?).
>
> This can later be extended to power down the ata ports (removing power
> from a disk) while the disk is spin down.
>
> Signed-off-by: Markus Probst <markus.probst@...teo.de>
> ---
> drivers/ata/libata-acpi.c | 68 +++++++++++++++++++++++++++++++++++++++
> drivers/ata/libata-core.c | 21 ++++++++++++
> drivers/ata/libata-scsi.c | 1 +
> drivers/ata/libata.h | 4 +++
> include/linux/libata.h | 1 +
> 5 files changed, 95 insertions(+)
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index f2140fc06ba0..891b59bfe29f 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -245,6 +245,74 @@ 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.
Long line. Please split at 80 chars.
> + * @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.
This is not C++. Please use C style comments and split the long line.
> + if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA) {
> + if (!is_acpi_device_node(dev->tdev.fwnode))
> + return false;
> + return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
> + }
> +
> + if (!is_acpi_device_node(dev->link->ap->tdev.fwnode))
> + return false;
> + 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 begin of ata_port_probe.
...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_info(&ap->link.device[i], "acpi: power was set to %d\n",
> + enable);
> + else
> + ata_dev_info(&ap->link.device[i], "acpi: power failed to be set\n");
> + }
Add a return here to avoid the else.
> + } else {
> + 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_info(ap, "acpi: power was set to %d\n", enable);
ata_port_debug(ap, "acpi: power %sabled\n",
enable ? "en" : "dis");
> + else
> + ata_port_info(ap, "acpi: power failed to be set\n");
ata_port_err(ap, "acpi: failed to set power state\n");
Overall, this hunk is the same as what is done in the loop above. So maybe
create a helper function instead of repeating the same procedure.
> + }
> +}
> +
> /**
> * 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..a52b916af14c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5899,11 +5899,32 @@ void ata_host_init(struct ata_host *host, struct device *dev,
> }
> EXPORT_SYMBOL_GPL(ata_host_init);
>
> +static inline void ata_port_set_power_state(struct ata_port *ap, bool enable)
> +{
> + ata_acpi_port_set_power_state(ap, enable);
> + // Maybe add a way with device tree and regulators too?
Drop this comment please.
> +}
> +
> +/**
> + * ata_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_dev_manage_restart(struct ata_device *dev)
> +{
> + return ata_acpi_dev_manage_restart(dev);
> +}
> +EXPORT_SYMBOL_GPL(ata_dev_manage_restart);
Why the export ? libata-core and libata-scsi are compiled together as libata.ko.
The export should not be needed.
> +
> void ata_port_probe(struct ata_port *ap)
> {
> struct ata_eh_info *ehi = &ap->link.eh_info;
> unsigned long flags;
>
> + ata_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..52297d9e3dc2 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_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..28cb652d99bc 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 false; }
> #endif
>
> /* libata-scsi.c */
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 0620dd67369f..af5974e91e1d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1302,6 +1302,7 @@ extern int sata_link_debounce(struct ata_link *link,
> extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
> bool spm_wakeup);
> extern int ata_slave_link_init(struct ata_port *ap);
> +extern bool ata_dev_manage_restart(struct ata_device *dev);
> extern void ata_port_probe(struct ata_port *ap);
> extern struct ata_port *ata_port_alloc(struct ata_host *host);
> extern void ata_port_free(struct ata_port *ap);
--
Damien Le Moal
Western Digital Research
Powered by blists - more mailing lists