RE: [PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set

["Limonciello, Mario" <Mario.Limonciello@xxxxxxx>]

[Public]



> -----Original Message-----
> From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Sent: Monday, November 15, 2021 19:41
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
> ide@xxxxxxxxxxxxxxx>; Shah, Nehal-bakulchandra <Nehal-
> bakulchandra.Shah@xxxxxxx>
> Subject: Re: [PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set
> 
> On 2021/11/13 5:15, Mario Limonciello wrote:
> > The StorageD3Enable _DSD is used for the vendor to indicate that the disk
> > should be opted into or out of a different behavior based upon the platform
> > design.
> >
> > For AMD's Renoir and Green Sardine platforms it's important that any
> > attached SATA storage has transitioned into DevSlp when s2idle is used.
> >
> > If the disk is left in active/partial/slumber, then the system is not able
> > to resume properly.
> >
> > When the StorageD3Enable _DSD is detected, check the system is using s2idle
> > and DevSlp is enabled and if so explicitly wait long enough for the disk to
> > enter DevSlp.
> >
> > Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@xxxxxxx>
> > BugLink:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cmario.li
> monciello%40amd.com%7Cb8d8f7b189a8471d2f0408d9a8a224ff%7C3dd8961f
> e4884e608e11a82d994e183d%7C0%7C0%7C637726236637477756%7CUnknow
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C3000&amp;sdata=NPnRafAYSFrIRlDAlM2K6WDzk9Mr3
> Rnn65n5wREiUZA%3D&amp;reserved=0
> > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.mi
> crosoft.com%2Fen-us%2Fwindows-hardware%2Fdesign%2Fcomponent-
> guidelines%2Fpower-management-for-storage-hardware-devices-
> intro&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7Cb8d8f7b189a
> 8471d2f0408d9a8a224ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0
> %7C637726236637487711%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata
> =qTZOAUQFzKnXn9i6r9noM17YPvfZ3kRtAztZjtrdxvU%3D&amp;reserved=0
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> >  drivers/ata/libahci.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > index 28430c093a7f..15c293da30ca 100644
> > --- a/drivers/ata/libahci.c
> > +++ b/drivers/ata/libahci.c
> > @@ -2322,6 +2322,18 @@ int ahci_port_resume(struct ata_port *ap)
> >  }
> >  EXPORT_SYMBOL_GPL(ahci_port_resume);
> >
> > +static void ahci_handle_s2idle(struct ata_port *ap)
> > +{
> > +	void __iomem *port_mmio = ahci_port_base(ap);
> > +	u32 devslp;
> > +
> > +	if (pm_suspend_via_firmware())
> > +		return;
> > +	devslp = readl(port_mmio + PORT_DEVSLP);
> > +	if ((devslp & PORT_DEVSLP_ADSE))
> > +		ata_msleep(ap, devslp_idle_timeout);
> > +}
> 
> This generates a warning at compilation if CONFIG_PM is not enabled. This
> function must be under the #ifdef CONFIG_PM below.
> I can fix this up, or you can resend a v2 if you prefer.

Thanks for checking the detail and the offer to fix on your end.
That's fine with me.

Thanks!

> 
> > +
> >  #ifdef CONFIG_PM
> >  static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
> >  {
> > @@ -2336,6 +2348,9 @@ static int ahci_port_suspend(struct ata_port *ap,
> pm_message_t mesg)
> >  		ata_port_freeze(ap);
> >  	}
> >
> > +	if (acpi_storage_d3(ap->host->dev))
> > +		ahci_handle_s2idle(ap);
> > +
> >  	ahci_rpm_put_port(ap);
> >  	return rc;
> >  }
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research