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@xxxxxxxxxxxxxxx
> > Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> > ---
> > 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
