Re: [PATCH 1/2] ata: ahci: Add force LPM policy quirk for ASUS B1400CEAE

[Niklas Cassel <cassel@kernel.org>]

On Fri, Feb 02, 2024 at 04:49:00PM +0800, Jian-Hong Pan wrote:
> Niklas Cassel <cassel@kernel.org> 於 2024年2月1日 週四 下午11:01寫道：
> >
> > On Wed, Jan 31, 2024 at 11:43:59PM +0100, Niklas Cassel wrote:
> > > On Wed, Jan 31, 2024 at 07:08:12AM -0400, Daniel Drake wrote:
> >
> > (snip)
> >
> > > In libata we perform a reset of the port at boot, see:
> > > libata-sata.c:sata_link_hardreset()
> > > after writing to SControl, we call
> > > libata-core.c:ata_wait_ready() that will poll for the port being ready
> > > by calling the check_ready callback.
> > > For AHCI, this callback funcion is set to:
> > > libahci.c:ahci_check_ready().
> > >
> > > A reset should take the device out of deep power state and should be
> > > sufficient to establish a connection (and that also seems to be the
> > > case when not using Intel VMD).
> > >
> > > However, if you want to debug, I would start by adding prints to
> > > libata-sata.c:sata_link_hardreset()
> > > libata-core.c:ata_wait_ready()
> > > libahci.c:ahci_check_ready().
> >
> > FWIW, this will dump SStatus.DET every time the check_ready callback function
> > (ahci_check_ready()) is called:
> >
> >
> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > index 1a63200ea437..0467e150601e 100644
> > --- a/drivers/ata/libahci.c
> > +++ b/drivers/ata/libahci.c
> > @@ -1533,6 +1533,12 @@ int ahci_check_ready(struct ata_link *link)
> >  {
> >         void __iomem *port_mmio = ahci_port_base(link->ap);
> >         u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
> > +       u32 cur = 0;
> > +
> > +       sata_scr_read(link, SCR_STATUS, &cur);
> > +
> > +       ata_link_info(link, "BUSY ? %d (status: %#x) SStatus.DET: %#x\n",
> > +                     status & ATA_BUSY, status, cur & 0xf);
> >
> >         return ata_check_ready(status);
> >  }
> 
> I think I can join the test based on kernel v6.8-rc2, too.
> 
> The original ASUS B1400CEAE has only one NVMe SSD.  I prepare the
> patch ("ata: ahci: Add force LPM policy quirk for ASUS B1400CEAE") to
> fix the power consumption issue for s2idle with enabled VMD.
> 
> The patch is a quirk limiting ASUS B1400CEAE only, not generic for the
> SATA controller [8086:a0d3].  Then, I install another SATA HDD into
> ASUS B1400CEAE for test.  The SATA HDD shows up and works.
> 
> $ dmesg | grep SATA
> [    0.785120] ahci 10000:e0:17.0: AHCI 0001.0301 32 slots 1 ports 6
> Gbps 0x1 impl SATA mode
> [    0.785269] ata1: SATA max UDMA/133 abar m2048@0x76102000 port
> 0x76102100 irq 144 lpm-pol 3
> [    1.096684] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> 
> However, if I simply revert the commit 6210038aeaf4 ("ata: ahci:
> Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"") (fix the
> conflict, of course), then the SATA HDD disappears!!?  Both
> CONFIG_SATA_MOBILE_LPM_POLICY=3 and 0 can reproduce the issue.
> 
> $ dmesg | grep SATA
> [    0.783211] ahci 10000:e0:17.0: AHCI 0001.0301 32 slots 1 ports 6
> Gbps 0x1 impl SATA mode
> [    0.783399] ata1: SATA max UDMA/133 abar m2048@0x76102000 port
> 0x76102100 irq 144 lpm-pol 3
> [    1.096685] ata1: SATA link down (SStatus 4 SControl 300)
> 
> Here is the dmesg of reverting ("ata: ahci: Revert "ata: ahci: Add
> Tiger Lake UP{3,4} AHCI controller"")
> https://bugzilla.kernel.org/show_bug.cgi?id=217114#c27
> The code already includes the debug message in ahci_check_ready() from
> Niklas.  However, the dmesg does not show the "BUSY ? ..." from
> ahci_check_ready().
> 
> From these scenarios mentioned above, they all apply LPM policy to the
> SATA controller [8086:a0d3].  But, they apply LPM policy at different
> time:
> * The patch ("ata: ahci: Add force LPM policy quirk for ASUS
> B1400CEAE") applies LPM policy in early ahci_init_one(), which is the
> probe callback.
> * Reverting 6210038aeaf4 ("ata: ahci: Revert "ata: ahci: Add Tiger
> Lake UP{3,4} AHCI controller"") applies LPM policy via "ahci_pci_tbl"
> table.

I don't see why it should matter if we set the AHCI_HFLAG_USE_LPM_POLICY
flag using ahci_pci_tbl, or by your suggested quirk in ahci_init_one(),
as in both cases the flag will be set before ahci_init_one() calls
ahci_update_initial_lpm_policy().


Could it perhaps be that in order for libata to be able to detect your
drive, when VMD is enabled, we also need your patch
"PCI: vmd: enable PCI PM's L1 substates of remapped PCIe port and NVMe" ?


If that is not the case, and there actually is a difference between using
ahci_pci_tbl and your suggested quirk, then my next suggestion would be to
add prints to libata-sata.c:sata_link_scr_lpm(). That way you can dump the
exact SCR writes that are being done for the working case vs. the
non-working case. (Since I assume that there must be some difference.)


Kind regards,
Niklas