[PATCH] ata: libata-core: Fix ata_pci_shutdown_one()

[PATCH] ata: libata-core: Fix ata_pci_shutdown_one()

[Damien Le Moal]

Commit 5b6fba546da2 ("ata: libata-core: Detach a port devices on
shutdown") modified the function ata_pci_shutdown_one() to stop
(suspend) devices attached to the ports of a PCI AHCI adapter to ensure
that drives are spun down before shutting down a system. However, this
is done only for PCI adapters and not for other types of adapters. This
limitation was addressed with commit 24eca2dce0f8 ("scsi: sd: Introduce
manage_shutdown device flag"). With this, all ATA disks are spun down on
system shutdown.

This reverts commit 5b6fba546da2 as the change introduced is now
useless.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6fb4e8dc8c3c..09ed67772fae 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6180,24 +6180,10 @@ EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 void ata_pci_shutdown_one(struct pci_dev *pdev)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	struct ata_port *ap;
-	unsigned long flags;
 	int i;
 
-	/* Tell EH to disable all devices */
-	for (i = 0; i < host->n_ports; i++) {
-		ap = host->ports[i];
-		spin_lock_irqsave(ap->lock, flags);
-		ap->pflags |= ATA_PFLAG_UNLOADING;
-		ata_port_schedule_eh(ap);
-		spin_unlock_irqrestore(ap->lock, flags);
-	}
-
 	for (i = 0; i < host->n_ports; i++) {
-		ap = host->ports[i];
-
-		/* Wait for EH to complete before freezing the port */
-		ata_port_wait_eh(ap);
+		struct ata_port *ap = host->ports[i];
 
 		ap->pflags |= ATA_PFLAG_FROZEN;
 
-- 
2.41.0

Re: [PATCH] ata: libata-core: Fix ata_pci_shutdown_one()

[Sergey Shtylyov]

Hello!

On 11/6/23 7:16 AM, Damien Le Moal wrote:

> Commit 5b6fba546da2 ("ata: libata-core: Detach a port devices on
> shutdown") modified the function ata_pci_shutdown_one() to stop
> (suspend) devices attached to the ports of a PCI AHCI adapter to ensure
> that drives are spun down before shutting down a system. However, this
> is done only for PCI adapters and not for other types of adapters. This
> limitation was addressed with commit 24eca2dce0f8 ("scsi: sd: Introduce
> manage_shutdown device flag"). With this, all ATA disks are spun down on
> system shutdown.
> 
> This reverts commit 5b6fba546da2 as the change introduced is now
> useless.

   You didn't use 'git revert'?

> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
[...]

MBR, Sergey

Re: [PATCH] ata: libata-core: Fix ata_pci_shutdown_one()

[Damien Le Moal]

On 11/6/23 17:47, Sergey Shtylyov wrote:
> Hello!
> 
> On 11/6/23 7:16 AM, Damien Le Moal wrote:
> 
>> Commit 5b6fba546da2 ("ata: libata-core: Detach a port devices on
>> shutdown") modified the function ata_pci_shutdown_one() to stop
>> (suspend) devices attached to the ports of a PCI AHCI adapter to ensure
>> that drives are spun down before shutting down a system. However, this
>> is done only for PCI adapters and not for other types of adapters. This
>> limitation was addressed with commit 24eca2dce0f8 ("scsi: sd: Introduce
>> manage_shutdown device flag"). With this, all ATA disks are spun down on
>> system shutdown.
>>
>> This reverts commit 5b6fba546da2 as the change introduced is now
>> useless.
> 
>    You didn't use 'git revert'?

I did, but I wrote a proper commit message.

> 
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [...]
> 
> MBR, Sergey

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: libata-core: Fix ata_pci_shutdown_one()

[Niklas Cassel]

On Mon, Nov 06, 2023 at 06:02:04PM +0900, Damien Le Moal wrote:
> On 11/6/23 17:47, Sergey Shtylyov wrote:
> > Hello!
> > 
> > On 11/6/23 7:16 AM, Damien Le Moal wrote:
> > 
> >> Commit 5b6fba546da2 ("ata: libata-core: Detach a port devices on
> >> shutdown") modified the function ata_pci_shutdown_one() to stop
> >> (suspend) devices attached to the ports of a PCI AHCI adapter to ensure
> >> that drives are spun down before shutting down a system. However, this
> >> is done only for PCI adapters and not for other types of adapters. This
> >> limitation was addressed with commit 24eca2dce0f8 ("scsi: sd: Introduce
> >> manage_shutdown device flag"). With this, all ATA disks are spun down on
> >> system shutdown.
> >>
> >> This reverts commit 5b6fba546da2 as the change introduced is now
> >> useless.
> > 
> >    You didn't use 'git revert'?
> 
> I did, but I wrote a proper commit message.

I think Sergey's question was because you didn't keep the automatically
generated one liner will the full SHA1 as the first line in your summary
(which is then followed by the reason for the revert), see e.g.:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac2263b588dffd3a1efd7ed0b156ea6c5aea200d


You have the line as the as final line in the summary, and shortened it
to not have the full SHA1.

Personally, I think I prefer having the full SHA1 as the first line,
as you directly see that it is a revert (without needing the read to
the end).

But I think it is fine either way:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH] ata: libata-core: Fix ata_pci_shutdown_one()

[Damien Le Moal]

On 11/7/23 19:06, Niklas Cassel wrote:
> Personally, I think I prefer having the full SHA1 as the first line,
> as you directly see that it is a revert (without needing the read to
> the end).
> 
> But I think it is fine either way:
> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

I tweaked the commit message to keep the full commit ID, making clear it is a
"git revert". Thanks.

-- 
Damien Le Moal
Western Digital Research