| Message ID | 20240111115123.1258422-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Power management fixes | expand |
On 1/11/24 2:51 PM, Damien Le Moal wrote: > This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a. > > Several users have signaled issues with commit fd3a6837d8e1 ("ata: > libata-core: Fix ata_pci_shutdown_one()") which causes failure of the > system SoC to go to a low power state. The reason for this problem > is not well understood but given that this patch is harmless with the > improvements to ata_dev_power_set_standby(), restore it to allow system > lower power state transitions. > > For regular system shutdown, ata_dev_power_set_standby() will be > executed twice: once the scsi device is removed and another when > ata_pci_shutdown_one() executes and EH completes unloading the devices. > Make the second call to ata_dev_power_set_standby() do nothing by using > ata_dev_power_is_active() and return if the device is already in > standby. > > Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-core.c | 75 +++++++++++++++++++++++---------------- > 1 file changed, 45 insertions(+), 30 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index d9f80f4f70f5..20a366942626 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, > return true; > } > > +static bool ata_dev_power_is_active(struct ata_device *dev) > +{ > + struct ata_taskfile tf; > + unsigned int err_mask; > + > + ata_tf_init(dev, &tf); > + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile regs but the device/head reg. Material for a fix, I guess... :-) > + tf.protocol = ATA_PROT_NODATA; > + tf.command = ATA_CMD_CHK_POWER; > + [...] MBR, Sergey
On 1/12/24 03:10, Sergei Shtylyov wrote: > On 1/11/24 2:51 PM, Damien Le Moal wrote: > >> This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a. >> >> Several users have signaled issues with commit fd3a6837d8e1 ("ata: >> libata-core: Fix ata_pci_shutdown_one()") which causes failure of the >> system SoC to go to a low power state. The reason for this problem >> is not well understood but given that this patch is harmless with the >> improvements to ata_dev_power_set_standby(), restore it to allow system >> lower power state transitions. >> >> For regular system shutdown, ata_dev_power_set_standby() will be >> executed twice: once the scsi device is removed and another when >> ata_pci_shutdown_one() executes and EH completes unloading the devices. >> Make the second call to ata_dev_power_set_standby() do nothing by using >> ata_dev_power_is_active() and return if the device is already in >> standby. >> >> Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> drivers/ata/libata-core.c | 75 +++++++++++++++++++++++---------------- >> 1 file changed, 45 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index d9f80f4f70f5..20a366942626 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, >> return true; >> } >> >> +static bool ata_dev_power_is_active(struct ata_device *dev) >> +{ >> + struct ata_taskfile tf; >> + unsigned int err_mask; >> + >> + ata_tf_init(dev, &tf); >> + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; > > Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile > regs but the device/head reg. Material for a fix, I guess... :-) Good point. I will look into it. This code was moved from libata-scsi translation, so it has been like this for a very long time... > >> + tf.protocol = ATA_PROT_NODATA; >> + tf.command = ATA_CMD_CHK_POWER; >> + > [...] > > MBR, Sergey
Hello Sergei, On Thu, Jan 11, 2024 at 09:10:43PM +0300, Sergei Shtylyov wrote: > On 1/11/24 2:51 PM, Damien Le Moal wrote: > > > This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a. > > > > Several users have signaled issues with commit fd3a6837d8e1 ("ata: > > libata-core: Fix ata_pci_shutdown_one()") which causes failure of the > > system SoC to go to a low power state. The reason for this problem > > is not well understood but given that this patch is harmless with the > > improvements to ata_dev_power_set_standby(), restore it to allow system > > lower power state transitions. > > > > For regular system shutdown, ata_dev_power_set_standby() will be > > executed twice: once the scsi device is removed and another when > > ata_pci_shutdown_one() executes and EH completes unloading the devices. > > Make the second call to ata_dev_power_set_standby() do nothing by using > > ata_dev_power_is_active() and return if the device is already in > > standby. > > > > Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > > --- > > drivers/ata/libata-core.c | 75 +++++++++++++++++++++++---------------- > > 1 file changed, 45 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > > index d9f80f4f70f5..20a366942626 100644 > > --- a/drivers/ata/libata-core.c > > +++ b/drivers/ata/libata-core.c > > @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, > > return true; > > } > > > > +static bool ata_dev_power_is_active(struct ata_device *dev) > > +{ > > + struct ata_taskfile tf; > > + unsigned int err_mask; > > + > > + ata_tf_init(dev, &tf); > > + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; > > Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile > regs but the device/head reg. Material for a fix, I guess... :-) > > > + tf.protocol = ATA_PROT_NODATA; > > + tf.command = ATA_CMD_CHK_POWER; > > + > [...] Looking at the definition of the flag: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76 "enable r/w to nsect/lba regs" This function does read from the nsect reg: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-core.c?h=v6.8-rc5#n2069 So I would prefer to keep it as it. Basically the whole motto for libata right now: "If it ain't broke, don't fix it." Sure, if we look at: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n343 it seems that flags ATA_TFLAG_ISADDR, ATA_TFLAG_LBA48, and ATA_TFLAG_DEVICE is used to "guard" if these regs should be written to the TF. However, if we look at: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n392 is seems that only flag ATA_TFLAG_LBA48 is used to "guard" if the regs should be read from the TF. So it looks like the intention was that these flags should be used to guard if certain TF regs should be written or read, but in reality, only some of the flags are actually for guarding reads. (While all of the flags are used for guarding writes.) Personally, I don't really see the point of using flags to guard writes to the host controller. Why would we want to skip writing certain TF regs? The struct ata_taskfile should be zero-initialized before filling it with a command, so why not always write all TF regs and remove these flags? Anyway, why touch it now and risk introducing regressions on some old PATA hardware? Kind regards, Niklas
On 2/19/24 6:29 PM, Niklas Cassel wrote: [...] >>> This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a. >>> >>> Several users have signaled issues with commit fd3a6837d8e1 ("ata: >>> libata-core: Fix ata_pci_shutdown_one()") which causes failure of the >>> system SoC to go to a low power state. The reason for this problem >>> is not well understood but given that this patch is harmless with the >>> improvements to ata_dev_power_set_standby(), restore it to allow system >>> lower power state transitions. >>> >>> For regular system shutdown, ata_dev_power_set_standby() will be >>> executed twice: once the scsi device is removed and another when >>> ata_pci_shutdown_one() executes and EH completes unloading the devices. >>> Make the second call to ata_dev_power_set_standby() do nothing by using >>> ata_dev_power_is_active() and return if the device is already in >>> standby. >>> >>> Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>> --- >>> drivers/ata/libata-core.c | 75 +++++++++++++++++++++++---------------- >>> 1 file changed, 45 insertions(+), 30 deletions(-) >>> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index d9f80f4f70f5..20a366942626 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c >>> @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, >>> return true; >>> } >>> >>> +static bool ata_dev_power_is_active(struct ata_device *dev) >>> +{ >>> + struct ata_taskfile tf; >>> + unsigned int err_mask; >>> + >>> + ata_tf_init(dev, &tf); >>> + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; >> >> Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile >> regs but the device/head reg. Material for a fix, I guess... :-) >> >>> + tf.protocol = ATA_PROT_NODATA; >>> + tf.command = ATA_CMD_CHK_POWER; >>> + >> [...] > > Looking at the definition of the flag: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76 > > "enable r/w to nsect/lba regs" I'm afraid this comment doesn't reflect the reality in its r/w part -- if you look at e.g. ata_sff_tf_read(), you'll see that it always reads all the legacy taskfile and only checks ATA_TFLAG_LBA48 in order to determine whether it should read the HOBs as well... > This function does read from the nsect reg: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-core.c?h=v6.8-rc5#n2069 > > So I would prefer to keep it as it. IMO, it doesn't make much sense -- unless you assume that the device could leave that reg unset as a result of this command... > Basically the whole motto for libata right now: > "If it ain't broke, don't fix it." Do you realize that each taskfile reg access takes e.g. 900-990 ns on the Intel PIIX/ICH (the part # was 82371/82801) IDE controllers (with 33 MHz PCI bus)? Luckily, we just have to write (almost?) whole taskfile on the read/write commands anyway... > Sure, if we look at: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n343 > > it seems that flags ATA_TFLAG_ISADDR, ATA_TFLAG_LBA48, and ATA_TFLAG_DEVICE > is used to "guard" if these regs should be written to the TF. > > > However, if we look at: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n392 > > is seems that only flag ATA_TFLAG_LBA48 is used to "guard" if the regs should > be read from the TF. Luckily, we have to read back the whole taskfile only on the read/write errors... > So it looks like the intention was that these flags should be used > to guard if certain TF regs should be written or read, but in reality, > only some of the flags are actually for guarding reads. > (While all of the flags are used for guarding writes.) So you're seeing that inconsistency (I mentioned) yourself... :-) > Personally, I don't really see the point of using flags to guard writes > to the host controller. Why would we want to skip writing certain TF regs? > > The struct ata_taskfile should be zero-initialized before filling it with TBH, I generally hate how libata implemented the taskfile accessors and I hate how *struct* ata_taskfile looks too... :-) > a command, so why not always write all TF regs and remove these flags? To stop wasting a good microsecond per a reg R/W cycle, perhaps? :-) Anyway, the ATA standards clearly describe what the regs are used by each command and what to expect on a normal/erratic command completion... In drivers/ide/ we finanally ended up with 8-bit reg validity flags, each bit corresponding to an individual taskfile reg: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=60f85019c6c8c1aebf3485a313e0da094bc95d07 > Anyway, why touch it now and risk introducing regressions on some old PATA > hardware? Do you realize that drivers/ide/ wasn't writing out the whole taskfile when issuing this particular command since: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d364c7f50b3bb6dc77259974038567b821e2cf0a If there were regressions, we would have seen them a long time ago, no? :-) > Kind regards, > Niklas MBR, Sergey
Hello Sergey, On Sat, Feb 24, 2024 at 12:04:46AM +0300, Sergey Shtylyov wrote: > On 2/19/24 6:29 PM, Niklas Cassel wrote: > [...] > > >>> This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a. > >>> > >>> Several users have signaled issues with commit fd3a6837d8e1 ("ata: > >>> libata-core: Fix ata_pci_shutdown_one()") which causes failure of the > >>> system SoC to go to a low power state. The reason for this problem > >>> is not well understood but given that this patch is harmless with the > >>> improvements to ata_dev_power_set_standby(), restore it to allow system > >>> lower power state transitions. > >>> > >>> For regular system shutdown, ata_dev_power_set_standby() will be > >>> executed twice: once the scsi device is removed and another when > >>> ata_pci_shutdown_one() executes and EH completes unloading the devices. > >>> Make the second call to ata_dev_power_set_standby() do nothing by using > >>> ata_dev_power_is_active() and return if the device is already in > >>> standby. > >>> > >>> Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") > >>> Cc: stable@vger.kernel.org > >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >>> --- > >>> drivers/ata/libata-core.c | 75 +++++++++++++++++++++++---------------- > >>> 1 file changed, 45 insertions(+), 30 deletions(-) > >>> > >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > >>> index d9f80f4f70f5..20a366942626 100644 > >>> --- a/drivers/ata/libata-core.c > >>> +++ b/drivers/ata/libata-core.c > >>> @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, > >>> return true; > >>> } > >>> > >>> +static bool ata_dev_power_is_active(struct ata_device *dev) > >>> +{ > >>> + struct ata_taskfile tf; > >>> + unsigned int err_mask; > >>> + > >>> + ata_tf_init(dev, &tf); > >>> + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; > >> > >> Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile > >> regs but the device/head reg. Material for a fix, I guess... :-) > >> > >>> + tf.protocol = ATA_PROT_NODATA; > >>> + tf.command = ATA_CMD_CHK_POWER; > >>> + > >> [...] > > > > Looking at the definition of the flag: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76 > > > > "enable r/w to nsect/lba regs" > > I'm afraid this comment doesn't reflect the reality in its r/w part -- > if you look at e.g. ata_sff_tf_read(), you'll see that it always reads > all the legacy taskfile and only checks ATA_TFLAG_LBA48 in order to > determine whether it should read the HOBs as well... Considering that you have experience with cleaning up a similar mess in drivers/ide, patches for drivers/ata which fixes the comment and the functions needlessly setting the ATA_TFLAG_DEVICE flag is more than welcome. From a quick look, there appears to be quite a few functions (in addition to ata_dev_power_is_active()) which needlessly sets this flag. Kind regards, Niklas
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d9f80f4f70f5..20a366942626 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, return true; } +static bool ata_dev_power_is_active(struct ata_device *dev) +{ + struct ata_taskfile tf; + unsigned int err_mask; + + ata_tf_init(dev, &tf); + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; + tf.protocol = ATA_PROT_NODATA; + tf.command = ATA_CMD_CHK_POWER; + + err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0); + if (err_mask) { + ata_dev_err(dev, "Check power mode failed (err_mask=0x%x)\n", + err_mask); + /* + * Assume we are in standby mode so that we always force a + * spinup in ata_dev_power_set_active(). + */ + return false; + } + + ata_dev_dbg(dev, "Power mode: 0x%02x\n", tf.nsect); + + /* Active or idle */ + return tf.nsect == 0xff; +} + /** * ata_dev_power_set_standby - Set a device power mode to standby * @dev: target device @@ -2017,8 +2044,9 @@ void ata_dev_power_set_standby(struct ata_device *dev) struct ata_taskfile tf; unsigned int err_mask; - /* If the device is already sleeping, do nothing. */ - if (dev->flags & ATA_DFLAG_SLEEPING) + /* If the device is already sleeping or in standby, do nothing. */ + if ((dev->flags & ATA_DFLAG_SLEEPING) || + !ata_dev_power_is_active(dev)) return; /* @@ -2046,33 +2074,6 @@ void ata_dev_power_set_standby(struct ata_device *dev) err_mask); } -static bool ata_dev_power_is_active(struct ata_device *dev) -{ - struct ata_taskfile tf; - unsigned int err_mask; - - ata_tf_init(dev, &tf); - tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR; - tf.protocol = ATA_PROT_NODATA; - tf.command = ATA_CMD_CHK_POWER; - - err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0); - if (err_mask) { - ata_dev_err(dev, "Check power mode failed (err_mask=0x%x)\n", - err_mask); - /* - * Assume we are in standby mode so that we always force a - * spinup in ata_dev_power_set_active(). - */ - return false; - } - - ata_dev_dbg(dev, "Power mode: 0x%02x\n", tf.nsect); - - /* Active or idle */ - return tf.nsect == 0xff; -} - /** * ata_dev_power_set_active - Set a device power mode to active * @dev: target device @@ -6184,10 +6185,24 @@ 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++) { - struct ata_port *ap = host->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); ap->pflags |= ATA_PFLAG_FROZEN;
This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a. Several users have signaled issues with commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions. For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby. Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-core.c | 75 +++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 30 deletions(-)