| 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: <d8b785ff14e4c0b12aa73a6ebb2be6466157cc1c.camel@posteo.de>
Date: Thu, 09 Oct 2025 11:07:01 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Damien Le Moal <dlemoal@...nel.org>, 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, Markus Probst <markus.probst@...teo.de>
Subject: Re: [PATCH 2/2] Power on ata ports defined in ACPI before probing
ports
On Mon, 2025-10-06 at 10:09 +0900, Damien Le Moal wrote:
> 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.
I don't think this can be avoided while still having proper log
messages.
In the first hunk the power state will be set on a ata_device, on the
second hunk on a ata_port.
>
> > + }
> > +}
> > +
> > /**
> > * 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);
>
Thanks
- Markus Probst
Powered by blists - more mailing lists