[PATCH 0/4] libata: misc frozen port cleanups

[PATCH 0/4] libata: misc frozen port cleanups

[Niklas Cassel]

Hello there,

This series adds a new ata_port_is_frozen() helper function,
and makes use of it in ata and libsas.

Additionally, improve ata_read_log_page() to avoid a futile
retry while the port is frozen.

Kind regards,
Niklas


Niklas Cassel (4):
  ata: add ata_port_is_frozen() helper
  ata: make use of ata_port_is_frozen() helper
  scsi: libsas: make use of ata_port_is_frozen() helper
  ata: libata-core: do not retry reading the log on timeout

 drivers/ata/libahci.c         |  6 +++---
 drivers/ata/libata-acpi.c     |  4 ++--
 drivers/ata/libata-core.c     |  7 ++++---
 drivers/ata/libata-eh.c       | 21 ++++++++++-----------
 drivers/ata/libata-sata.c     |  2 +-
 drivers/ata/libata-scsi.c     |  2 +-
 drivers/ata/sata_nv.c         |  2 +-
 drivers/ata/sata_promise.c    |  2 +-
 drivers/ata/sata_sx4.c        |  2 +-
 drivers/scsi/libsas/sas_ata.c |  2 +-
 include/linux/libata.h        |  5 +++++
 11 files changed, 30 insertions(+), 25 deletions(-)

-- 
2.37.3

[PATCH 1/4] ata: add ata_port_is_frozen() helper

[Niklas Cassel]

At the request of the libata maintainer, introduce a ata_port_is_frozen()
helper function.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 include/linux/libata.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index a505cfb92ab3..d5ac52654b42 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1043,6 +1043,11 @@ static inline int ata_port_is_dummy(struct ata_port *ap)
 	return ap->ops == &ata_dummy_port_ops;
 }
 
+static inline bool ata_port_is_frozen(const struct ata_port *ap)
+{
+	return ap->pflags & ATA_PFLAG_FROZEN;
+}
+
 extern int ata_std_prereset(struct ata_link *link, unsigned long deadline);
 extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
 				int (*check_ready)(struct ata_link *link));
-- 
2.37.3

Re: [PATCH 1/4] ata: add ata_port_is_frozen() helper

[John Garry]

On 07/10/2022 14:23, Niklas Cassel wrote:
> At the request of the libata maintainer, introduce a ata_port_is_frozen()
> helper function.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   include/linux/libata.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index a505cfb92ab3..d5ac52654b42 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1043,6 +1043,11 @@ static inline int ata_port_is_dummy(struct ata_port *ap)
>   	return ap->ops == &ata_dummy_port_ops;
>   }

Hi Niklas,

 >
 > +static inline bool ata_port_is_frozen(const struct ata_port *ap)

The majority of libata APIs don't use const in this way, so I think that 
consistency is better.

Indeed, this is not const data which you're pointing at, so maybe it's 
better to be honest with the compiler. And since this is inlined, could 
the compiler optimise out multiple reads on ap->flags in a caller 
function since we tell it it's const?

Thanks,
John

> +{
> +	return ap->pflags & ATA_PFLAG_FROZEN;
> +}
> +
>   extern int ata_std_prereset(struct ata_link *link, unsigned long deadline);
>   extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
>   				int (*check_ready)(struct ata_link *link));

Re: [PATCH 1/4] ata: add ata_port_is_frozen() helper

[Niklas Cassel]

On Mon, Oct 10, 2022 at 08:00:09AM +0100, John Garry wrote:
> On 07/10/2022 14:23, Niklas Cassel wrote:
> > At the request of the libata maintainer, introduce a ata_port_is_frozen()
> > helper function.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >   include/linux/libata.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index a505cfb92ab3..d5ac52654b42 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1043,6 +1043,11 @@ static inline int ata_port_is_dummy(struct ata_port *ap)
> >   	return ap->ops == &ata_dummy_port_ops;
> >   }
> 
> Hi Niklas,

Hello John,

> 
> >
> > +static inline bool ata_port_is_frozen(const struct ata_port *ap)
> 
> The majority of libata APIs don't use const in this way, so I think that
> consistency is better.

Well, right now there is no consistency :)

$ git grep "static inline" include/linux/libata.h | grep "(const struct"
include/linux/libata.h:static inline bool ata_port_is_frozen(const struct ata_port *ap)
include/linux/libata.h:static inline int ata_acpi_stm(const struct ata_port *ap,
include/linux/libata.h:static inline int ata_acpi_gtm(const struct ata_port *ap,
include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
include/linux/libata.h:static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
include/linux/libata.h:static inline unsigned int ata_dev_disabled(const struct ata_device *dev)
include/linux/libata.h:static inline unsigned int ata_dev_absent(const struct ata_device *dev)
include/linux/libata.h:static inline int ata_link_max_devices(const struct ata_link *link)
include/linux/libata.h:static inline int ata_try_flush_cache(const struct ata_device *dev)

There are 10 uses (9 without my addition) that uses a const struct pointer.

So since both are used in libata, I chose the one that seemed most correct.

> 
> Indeed, this is not const data which you're pointing at, so maybe it's
> better to be honest with the compiler. And since this is inlined, could the
> compiler optimise out multiple reads on ap->flags in a caller function since
> we tell it it's const?

"This is not const data which you're pointing at"

Well, according to 6.7.6.1 Pointer declarators in
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf

A "const struct *ptr" means that the contents of any object pointed to
cannot be modified through that pointer.


"And since this is inlined, could the compiler optimise out multiple reads
on ap->flags in a caller function since we tell it it's const?"

I'm far from a compiler expert, but because an optimising compiler is free
to inline whatever function it wants, not just functions marked inline,
I would assume that the compiler would "do the right thing" regardless if
a function is marked as inline or not.

Doing a:
git grep "static inline" include/ | grep "(const struct" | wc -l
2055

Makes me quite confident that this should be fine.
Sure, the data it points to might never change.

But seeing e.g.:
$ git grep "static inline" include/ | grep "empty(const struct"

Especially used in tcp and qdisc makes me even more confident that this
will work fine.

Looking at e.g. __dev_xmit_skb():
https://elixir.bootlin.com/linux/v6.0/source/net/core/dev.c#L3803
we can see that it uses nolock_qdisc_is_empty() multiple times within
the same function. So now I'm very confident that this will be fine :)


Kind regards,
Niklas

Re: [PATCH 1/4] ata: add ata_port_is_frozen() helper

[John Garry]

On 10/10/2022 11:17, Niklas Cassel wrote:

Hi Niklas,

> Well, right now there is no consistency:)
> 
> $ git grep "static inline" include/linux/libata.h | grep "(const struct"
> include/linux/libata.h:static inline bool ata_port_is_frozen(const struct ata_port *ap)
> include/linux/libata.h:static inline int ata_acpi_stm(const struct ata_port *ap,
> include/linux/libata.h:static inline int ata_acpi_gtm(const struct ata_port *ap,
> include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
> include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
> include/linux/libata.h:static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
> include/linux/libata.h:static inline unsigned int ata_dev_disabled(const struct ata_device *dev)
> include/linux/libata.h:static inline unsigned int ata_dev_absent(const struct ata_device *dev)
> include/linux/libata.h:static inline int ata_link_max_devices(const struct ata_link *link)
> include/linux/libata.h:static inline int ata_try_flush_cache(const struct ata_device *dev)
> 
> There are 10 uses (9 without my addition) that uses a const struct pointer.

I was just checking *ata_port, and based my judgement on that one.

> 
> So since both are used in libata, I chose the one that seemed most correct.
> 
>> Indeed, this is not const data which you're pointing at, so maybe it's
>> better to be honest with the compiler. And since this is inlined, could the
>> compiler optimise out multiple reads on ap->flags in a caller function since
>> we tell it it's const?
> "This is not const data which you're pointing at"
> 
> Well, according to 6.7.6.1 Pointer declarators in
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf
> 
> A "const struct *ptr" means that the contents of any object pointed to
> cannot be modified through that pointer.
> 

sure

> 
> "And since this is inlined, could the compiler optimise out multiple reads
> on ap->flags in a caller function since we tell it it's const?"
> 
> I'm far from a compiler expert, but because an optimising compiler is free
> to inline whatever function it wants, not just functions marked inline,
> I would assume that the compiler would "do the right thing" regardless if
> a function is marked as inline or not.
> 
> Doing a:
> git grep "static inline" include/ | grep "(const struct" | wc -l
> 2055
> 
> Makes me quite confident that this should be fine.
> Sure, the data it points to might never change.
> 
> But seeing e.g.:
> $ git grep "static inline" include/ | grep "empty(const struct"
> 
> Especially used in tcp and qdisc makes me even more confident that this
> will work fine.

yeah, I think it should be fine, as the compiler should treat 
ata_port_is_frozen() as self-contained and thus make no judgement 
optimizing out such reads when inlining.

> 
> Looking at e.g. __dev_xmit_skb():
> https://elixir.bootlin.com/linux/v6.0/source/net/core/dev.c#L3803
> we can see that it uses nolock_qdisc_is_empty() multiple times within
> the same function. So now I'm very confident that this will be fine:)

I'm still not inclined to add const specifier, but I'll leave that to 
Damien and you.

I think generally we could add const a lot more in the kernel codebase. 
But we don't, as if we need to change how we treat the data we point to, 
i.e. stop writing or start writing, then we need to start changing APIs, 
and that is not so welcome, and so generally omit it. The same goes for 
non-ptr functions args. And then programmers are often a bit lazy and 
over-confident too (to not bother using it) :).

Thanks,
John

Re: [PATCH 1/4] ata: add ata_port_is_frozen() helper

[Damien Le Moal]

On 2022/10/10 20:22, John Garry wrote:
> On 10/10/2022 11:17, Niklas Cassel wrote:
> 
> Hi Niklas,
> 
>> Well, right now there is no consistency:)
>>
>> $ git grep "static inline" include/linux/libata.h | grep "(const struct"
>> include/linux/libata.h:static inline bool ata_port_is_frozen(const struct ata_port *ap)
>> include/linux/libata.h:static inline int ata_acpi_stm(const struct ata_port *ap,
>> include/linux/libata.h:static inline int ata_acpi_gtm(const struct ata_port *ap,
>> include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
>> include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
>> include/linux/libata.h:static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
>> include/linux/libata.h:static inline unsigned int ata_dev_disabled(const struct ata_device *dev)
>> include/linux/libata.h:static inline unsigned int ata_dev_absent(const struct ata_device *dev)
>> include/linux/libata.h:static inline int ata_link_max_devices(const struct ata_link *link)
>> include/linux/libata.h:static inline int ata_try_flush_cache(const struct ata_device *dev)
>>
>> There are 10 uses (9 without my addition) that uses a const struct pointer.
> 
> I was just checking *ata_port, and based my judgement on that one.
> 
>>
>> So since both are used in libata, I chose the one that seemed most correct.
>>
>>> Indeed, this is not const data which you're pointing at, so maybe it's
>>> better to be honest with the compiler. And since this is inlined, could the
>>> compiler optimise out multiple reads on ap->flags in a caller function since
>>> we tell it it's const?
>> "This is not const data which you're pointing at"
>>
>> Well, according to 6.7.6.1 Pointer declarators in
>> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf
>>
>> A "const struct *ptr" means that the contents of any object pointed to
>> cannot be modified through that pointer.
>>
> 
> sure
> 
>>
>> "And since this is inlined, could the compiler optimise out multiple reads
>> on ap->flags in a caller function since we tell it it's const?"
>>
>> I'm far from a compiler expert, but because an optimising compiler is free
>> to inline whatever function it wants, not just functions marked inline,
>> I would assume that the compiler would "do the right thing" regardless if
>> a function is marked as inline or not.
>>
>> Doing a:
>> git grep "static inline" include/ | grep "(const struct" | wc -l
>> 2055
>>
>> Makes me quite confident that this should be fine.
>> Sure, the data it points to might never change.
>>
>> But seeing e.g.:
>> $ git grep "static inline" include/ | grep "empty(const struct"
>>
>> Especially used in tcp and qdisc makes me even more confident that this
>> will work fine.
> 
> yeah, I think it should be fine, as the compiler should treat 
> ata_port_is_frozen() as self-contained and thus make no judgement 
> optimizing out such reads when inlining.
> 
>>
>> Looking at e.g. __dev_xmit_skb():
>> https://elixir.bootlin.com/linux/v6.0/source/net/core/dev.c#L3803
>> we can see that it uses nolock_qdisc_is_empty() multiple times within
>> the same function. So now I'm very confident that this will be fine:)
> 
> I'm still not inclined to add const specifier, but I'll leave that to 
> Damien and you.

Given that this helper is clearly intends to only read the port flags, I am fine
with the const argument, even though I think this will not buy us anything from
the compiler given the simplicity of the function :)

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/4] ata: make use of ata_port_is_frozen() helper

[Niklas Cassel]

Clean up the code by making use of the newly introduced
ata_port_is_frozen() helper function.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libahci.c      |  6 +++---
 drivers/ata/libata-acpi.c  |  4 ++--
 drivers/ata/libata-core.c  |  4 ++--
 drivers/ata/libata-eh.c    | 21 ++++++++++-----------
 drivers/ata/libata-sata.c  |  2 +-
 drivers/ata/libata-scsi.c  |  2 +-
 drivers/ata/sata_nv.c      |  2 +-
 drivers/ata/sata_promise.c |  2 +-
 drivers/ata/sata_sx4.c     |  2 +-
 9 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 954386a2b500..399feb09d1ba 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2106,7 +2106,7 @@ void ahci_error_handler(struct ata_port *ap)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
 
-	if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
+	if (!ata_port_is_frozen(ap)) {
 		/* restart engine */
 		hpriv->stop_engine(ap);
 		hpriv->start_engine(ap);
@@ -2297,7 +2297,7 @@ static void ahci_pmp_attach(struct ata_port *ap)
 	 * Note that during initialization, the port is marked as
 	 * frozen since the irq handler is not yet registered.
 	 */
-	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+	if (!ata_port_is_frozen(ap))
 		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
@@ -2316,7 +2316,7 @@ static void ahci_pmp_detach(struct ata_port *ap)
 	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
 
 	/* see comment above in ahci_pmp_attach() */
-	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+	if (!ata_port_is_frozen(ap))
 		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 61b4ccf88bf1..d36e71f475ab 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -992,7 +992,7 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
 
  acpi_err:
 	/* ignore evaluation failure if we can continue safely */
-	if (rc == -EINVAL && !nr_executed && !(ap->pflags & ATA_PFLAG_FROZEN))
+	if (rc == -EINVAL && !nr_executed && !ata_port_is_frozen(ap))
 		return 0;
 
 	/* fail and let EH retry once more for unknown IO errors */
@@ -1007,7 +1007,7 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
 	/* We can safely continue if no _GTF command has been executed
 	 * and port is not frozen.
 	 */
-	if (!nr_executed && !(ap->pflags & ATA_PFLAG_FROZEN))
+	if (!nr_executed && !ata_port_is_frozen(ap))
 		return 0;
 
 	return rc;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 75b86913db1a..1cf326dd7c41 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1489,7 +1489,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
 	spin_lock_irqsave(ap->lock, flags);
 
 	/* no internal command while frozen */
-	if (ap->pflags & ATA_PFLAG_FROZEN) {
+	if (ata_port_is_frozen(ap)) {
 		spin_unlock_irqrestore(ap->lock, flags);
 		return AC_ERR_SYSTEM;
 	}
@@ -4717,7 +4717,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 			return;
 		}
 
-		WARN_ON_ONCE(ap->pflags & ATA_PFLAG_FROZEN);
+		WARN_ON_ONCE(ata_port_is_frozen(ap));
 
 		/* read result TF if requested */
 		if (qc->flags & ATA_QCFLAG_RESULT_TF)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 1b82283f4b49..00181e747932 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1406,7 +1406,7 @@ static void ata_eh_request_sense(struct ata_queued_cmd *qc)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
-	if (qc->ap->pflags & ATA_PFLAG_FROZEN) {
+	if (ata_port_is_frozen(qc->ap)) {
 		ata_dev_warn(dev, "sense data available but port frozen\n");
 		return;
 	}
@@ -1588,7 +1588,7 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
 		break;
 
 	case ATA_DEV_ATAPI:
-		if (!(qc->ap->pflags & ATA_PFLAG_FROZEN)) {
+		if (!ata_port_is_frozen(qc->ap)) {
 			tmp = atapi_eh_request_sense(qc->dev,
 						qc->scsicmd->sense_buffer,
 						qc->result_tf.error >> 4);
@@ -1995,7 +1995,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 		ehc->i.flags |= ATA_EHI_QUIET;
 
 	/* enforce default EH actions */
-	if (ap->pflags & ATA_PFLAG_FROZEN ||
+	if (ata_port_is_frozen(ap) ||
 	    all_err_mask & (AC_ERR_HSM | AC_ERR_TIMEOUT))
 		ehc->i.action |= ATA_EH_RESET;
 	else if (((eflags & ATA_EFLAG_IS_IO) && all_err_mask) ||
@@ -2237,7 +2237,7 @@ static void ata_eh_link_report(struct ata_link *link)
 		return;
 
 	frozen = "";
-	if (ap->pflags & ATA_PFLAG_FROZEN)
+	if (ata_port_is_frozen(ap))
 		frozen = " frozen";
 
 	if (ap->eh_tries < ATA_EH_MAX_TRIES)
@@ -2558,8 +2558,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		if (reset && !(ehc->i.action & ATA_EH_RESET)) {
 			ata_for_each_dev(dev, link, ALL)
 				classes[dev->devno] = ATA_DEV_NONE;
-			if ((ap->pflags & ATA_PFLAG_FROZEN) &&
-			    ata_is_host_link(link))
+			if (ata_port_is_frozen(ap) && ata_is_host_link(link))
 				ata_eh_thaw_port(ap);
 			rc = 0;
 			goto out;
@@ -2717,7 +2716,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	ap->pflags &= ~ATA_PFLAG_EH_PENDING;
 	spin_unlock_irqrestore(link->ap->lock, flags);
 
-	if (ap->pflags & ATA_PFLAG_FROZEN)
+	if (ata_port_is_frozen(ap))
 		ata_eh_thaw_port(ap);
 
 	/*
@@ -3224,7 +3223,7 @@ static int ata_eh_maybe_retry_flush(struct ata_device *dev)
 		if (err_mask & AC_ERR_DEV) {
 			qc->err_mask |= AC_ERR_DEV;
 			qc->result_tf = tf;
-			if (!(ap->pflags & ATA_PFLAG_FROZEN))
+			if (!ata_port_is_frozen(ap))
 				rc = 0;
 		}
 	}
@@ -3401,7 +3400,7 @@ static int ata_eh_skip_recovery(struct ata_link *link)
 		return 1;
 
 	/* thaw frozen port and recover failed devices */
-	if ((ap->pflags & ATA_PFLAG_FROZEN) || ata_link_nr_enabled(link))
+	if (ata_port_is_frozen(ap) || ata_link_nr_enabled(link))
 		return 0;
 
 	/* reset at least once if reset is requested */
@@ -3756,7 +3755,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 		if (dev)
 			ata_eh_handle_dev_fail(dev, rc);
 
-		if (ap->pflags & ATA_PFLAG_FROZEN) {
+		if (ata_port_is_frozen(ap)) {
 			/* PMP reset requires working host port.
 			 * Can't retry if it's frozen.
 			 */
@@ -3930,7 +3929,7 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
 	ap->pflags &= ~ATA_PFLAG_PM_PENDING;
 	if (rc == 0)
 		ap->pflags |= ATA_PFLAG_SUSPENDED;
-	else if (ap->pflags & ATA_PFLAG_FROZEN)
+	else if (ata_port_is_frozen(ap))
 		ata_port_schedule_eh(ap);
 
 	spin_unlock_irqrestore(ap->lock, flags);
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index eef57d101ed1..60009ac03a8f 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1420,7 +1420,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 	int tag, rc;
 
 	/* if frozen, we can't do much */
-	if (ap->pflags & ATA_PFLAG_FROZEN)
+	if (ata_port_is_frozen(ap))
 		return;
 
 	/* is it NCQ device error? */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f3c64e796423..ccacaa5db936 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -642,7 +642,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 	struct ata_queued_cmd *qc;
 	int tag;
 
-	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
+	if (unlikely(ata_port_is_frozen(ap)))
 		goto fail;
 
 	if (ap->flags & ATA_FLAG_SAS_HOST) {
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 7f14d0d31057..9b2d289e89e1 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -2185,7 +2185,7 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
 	if (!fis)
 		return;
 
-	if (ap->pflags & ATA_PFLAG_FROZEN)
+	if (ata_port_is_frozen(ap))
 		return;
 
 	if (fis & NV_SWNCQ_IRQ_HOTPLUG) {
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index b8465fef2ed2..9cd7d8b71361 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -817,7 +817,7 @@ static int pdc_sata_hardreset(struct ata_link *link, unsigned int *class,
 
 static void pdc_error_handler(struct ata_port *ap)
 {
-	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+	if (!ata_port_is_frozen(ap))
 		pdc_reset_port(ap);
 
 	ata_sff_error_handler(ap);
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index 6ceec59cb291..ab70cbc78f96 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -855,7 +855,7 @@ static int pdc_softreset(struct ata_link *link, unsigned int *class,
 
 static void pdc_error_handler(struct ata_port *ap)
 {
-	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+	if (!ata_port_is_frozen(ap))
 		pdc_reset_port(ap);
 
 	ata_sff_error_handler(ap);
-- 
2.37.3

Re: [PATCH 2/4] ata: make use of ata_port_is_frozen() helper

[Damien Le Moal]

On 10/7/22 22:23, Niklas Cassel wrote:
> Clean up the code by making use of the newly introduced
> ata_port_is_frozen() helper function.

Looks good, but I think this should be squashed with patch 1.

> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  drivers/ata/libahci.c      |  6 +++---
>  drivers/ata/libata-acpi.c  |  4 ++--
>  drivers/ata/libata-core.c  |  4 ++--
>  drivers/ata/libata-eh.c    | 21 ++++++++++-----------
>  drivers/ata/libata-sata.c  |  2 +-
>  drivers/ata/libata-scsi.c  |  2 +-
>  drivers/ata/sata_nv.c      |  2 +-
>  drivers/ata/sata_promise.c |  2 +-
>  drivers/ata/sata_sx4.c     |  2 +-
>  9 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 954386a2b500..399feb09d1ba 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2106,7 +2106,7 @@ void ahci_error_handler(struct ata_port *ap)
>  {
>  	struct ahci_host_priv *hpriv = ap->host->private_data;
>  
> -	if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
> +	if (!ata_port_is_frozen(ap)) {
>  		/* restart engine */
>  		hpriv->stop_engine(ap);
>  		hpriv->start_engine(ap);
> @@ -2297,7 +2297,7 @@ static void ahci_pmp_attach(struct ata_port *ap)
>  	 * Note that during initialization, the port is marked as
>  	 * frozen since the irq handler is not yet registered.
>  	 */
> -	if (!(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (!ata_port_is_frozen(ap))
>  		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
>  }
>  
> @@ -2316,7 +2316,7 @@ static void ahci_pmp_detach(struct ata_port *ap)
>  	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
>  
>  	/* see comment above in ahci_pmp_attach() */
> -	if (!(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (!ata_port_is_frozen(ap))
>  		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
>  }
>  
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 61b4ccf88bf1..d36e71f475ab 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -992,7 +992,7 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
>  
>   acpi_err:
>  	/* ignore evaluation failure if we can continue safely */
> -	if (rc == -EINVAL && !nr_executed && !(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (rc == -EINVAL && !nr_executed && !ata_port_is_frozen(ap))
>  		return 0;
>  
>  	/* fail and let EH retry once more for unknown IO errors */
> @@ -1007,7 +1007,7 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
>  	/* We can safely continue if no _GTF command has been executed
>  	 * and port is not frozen.
>  	 */
> -	if (!nr_executed && !(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (!nr_executed && !ata_port_is_frozen(ap))
>  		return 0;
>  
>  	return rc;
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 75b86913db1a..1cf326dd7c41 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1489,7 +1489,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
>  	spin_lock_irqsave(ap->lock, flags);
>  
>  	/* no internal command while frozen */
> -	if (ap->pflags & ATA_PFLAG_FROZEN) {
> +	if (ata_port_is_frozen(ap)) {
>  		spin_unlock_irqrestore(ap->lock, flags);
>  		return AC_ERR_SYSTEM;
>  	}
> @@ -4717,7 +4717,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
>  			return;
>  		}
>  
> -		WARN_ON_ONCE(ap->pflags & ATA_PFLAG_FROZEN);
> +		WARN_ON_ONCE(ata_port_is_frozen(ap));
>  
>  		/* read result TF if requested */
>  		if (qc->flags & ATA_QCFLAG_RESULT_TF)
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 1b82283f4b49..00181e747932 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1406,7 +1406,7 @@ static void ata_eh_request_sense(struct ata_queued_cmd *qc)
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
>  
> -	if (qc->ap->pflags & ATA_PFLAG_FROZEN) {
> +	if (ata_port_is_frozen(qc->ap)) {
>  		ata_dev_warn(dev, "sense data available but port frozen\n");
>  		return;
>  	}
> @@ -1588,7 +1588,7 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
>  		break;
>  
>  	case ATA_DEV_ATAPI:
> -		if (!(qc->ap->pflags & ATA_PFLAG_FROZEN)) {
> +		if (!ata_port_is_frozen(qc->ap)) {
>  			tmp = atapi_eh_request_sense(qc->dev,
>  						qc->scsicmd->sense_buffer,
>  						qc->result_tf.error >> 4);
> @@ -1995,7 +1995,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
>  		ehc->i.flags |= ATA_EHI_QUIET;
>  
>  	/* enforce default EH actions */
> -	if (ap->pflags & ATA_PFLAG_FROZEN ||
> +	if (ata_port_is_frozen(ap) ||
>  	    all_err_mask & (AC_ERR_HSM | AC_ERR_TIMEOUT))
>  		ehc->i.action |= ATA_EH_RESET;
>  	else if (((eflags & ATA_EFLAG_IS_IO) && all_err_mask) ||
> @@ -2237,7 +2237,7 @@ static void ata_eh_link_report(struct ata_link *link)
>  		return;
>  
>  	frozen = "";
> -	if (ap->pflags & ATA_PFLAG_FROZEN)
> +	if (ata_port_is_frozen(ap))
>  		frozen = " frozen";
>  
>  	if (ap->eh_tries < ATA_EH_MAX_TRIES)
> @@ -2558,8 +2558,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  		if (reset && !(ehc->i.action & ATA_EH_RESET)) {
>  			ata_for_each_dev(dev, link, ALL)
>  				classes[dev->devno] = ATA_DEV_NONE;
> -			if ((ap->pflags & ATA_PFLAG_FROZEN) &&
> -			    ata_is_host_link(link))
> +			if (ata_port_is_frozen(ap) && ata_is_host_link(link))
>  				ata_eh_thaw_port(ap);
>  			rc = 0;
>  			goto out;
> @@ -2717,7 +2716,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
>  	ap->pflags &= ~ATA_PFLAG_EH_PENDING;
>  	spin_unlock_irqrestore(link->ap->lock, flags);
>  
> -	if (ap->pflags & ATA_PFLAG_FROZEN)
> +	if (ata_port_is_frozen(ap))
>  		ata_eh_thaw_port(ap);
>  
>  	/*
> @@ -3224,7 +3223,7 @@ static int ata_eh_maybe_retry_flush(struct ata_device *dev)
>  		if (err_mask & AC_ERR_DEV) {
>  			qc->err_mask |= AC_ERR_DEV;
>  			qc->result_tf = tf;
> -			if (!(ap->pflags & ATA_PFLAG_FROZEN))
> +			if (!ata_port_is_frozen(ap))
>  				rc = 0;
>  		}
>  	}
> @@ -3401,7 +3400,7 @@ static int ata_eh_skip_recovery(struct ata_link *link)
>  		return 1;
>  
>  	/* thaw frozen port and recover failed devices */
> -	if ((ap->pflags & ATA_PFLAG_FROZEN) || ata_link_nr_enabled(link))
> +	if (ata_port_is_frozen(ap) || ata_link_nr_enabled(link))
>  		return 0;
>  
>  	/* reset at least once if reset is requested */
> @@ -3756,7 +3755,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  		if (dev)
>  			ata_eh_handle_dev_fail(dev, rc);
>  
> -		if (ap->pflags & ATA_PFLAG_FROZEN) {
> +		if (ata_port_is_frozen(ap)) {
>  			/* PMP reset requires working host port.
>  			 * Can't retry if it's frozen.
>  			 */
> @@ -3930,7 +3929,7 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
>  	ap->pflags &= ~ATA_PFLAG_PM_PENDING;
>  	if (rc == 0)
>  		ap->pflags |= ATA_PFLAG_SUSPENDED;
> -	else if (ap->pflags & ATA_PFLAG_FROZEN)
> +	else if (ata_port_is_frozen(ap))
>  		ata_port_schedule_eh(ap);
>  
>  	spin_unlock_irqrestore(ap->lock, flags);
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index eef57d101ed1..60009ac03a8f 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1420,7 +1420,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
>  	int tag, rc;
>  
>  	/* if frozen, we can't do much */
> -	if (ap->pflags & ATA_PFLAG_FROZEN)
> +	if (ata_port_is_frozen(ap))
>  		return;
>  
>  	/* is it NCQ device error? */
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index f3c64e796423..ccacaa5db936 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -642,7 +642,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>  	struct ata_queued_cmd *qc;
>  	int tag;
>  
> -	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (unlikely(ata_port_is_frozen(ap)))
>  		goto fail;
>  
>  	if (ap->flags & ATA_FLAG_SAS_HOST) {
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 7f14d0d31057..9b2d289e89e1 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -2185,7 +2185,7 @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
>  	if (!fis)
>  		return;
>  
> -	if (ap->pflags & ATA_PFLAG_FROZEN)
> +	if (ata_port_is_frozen(ap))
>  		return;
>  
>  	if (fis & NV_SWNCQ_IRQ_HOTPLUG) {
> diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
> index b8465fef2ed2..9cd7d8b71361 100644
> --- a/drivers/ata/sata_promise.c
> +++ b/drivers/ata/sata_promise.c
> @@ -817,7 +817,7 @@ static int pdc_sata_hardreset(struct ata_link *link, unsigned int *class,
>  
>  static void pdc_error_handler(struct ata_port *ap)
>  {
> -	if (!(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (!ata_port_is_frozen(ap))
>  		pdc_reset_port(ap);
>  
>  	ata_sff_error_handler(ap);
> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
> index 6ceec59cb291..ab70cbc78f96 100644
> --- a/drivers/ata/sata_sx4.c
> +++ b/drivers/ata/sata_sx4.c
> @@ -855,7 +855,7 @@ static int pdc_softreset(struct ata_link *link, unsigned int *class,
>  
>  static void pdc_error_handler(struct ata_port *ap)
>  {
> -	if (!(ap->pflags & ATA_PFLAG_FROZEN))
> +	if (!ata_port_is_frozen(ap))
>  		pdc_reset_port(ap);
>  
>  	ata_sff_error_handler(ap);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] ata: make use of ata_port_is_frozen() helper

[Niklas Cassel]

On Sat, Oct 08, 2022 at 07:31:00AM +0900, Damien Le Moal wrote:
> On 10/7/22 22:23, Niklas Cassel wrote:
> > Clean up the code by making use of the newly introduced
> > ata_port_is_frozen() helper function.
> 
> Looks good, but I think this should be squashed with patch 1.

I could, but by that same logic, shouldn't patch 3/4 also be squashed into
patch 1?


Kind regards,
Niklas

Re: [PATCH 2/4] ata: make use of ata_port_is_frozen() helper

[Damien Le Moal]

On 10/8/22 08:50, Niklas Cassel wrote:
> On Sat, Oct 08, 2022 at 07:31:00AM +0900, Damien Le Moal wrote:
>> On 10/7/22 22:23, Niklas Cassel wrote:
>>> Clean up the code by making use of the newly introduced
>>> ata_port_is_frozen() helper function.
>>
>> Looks good, but I think this should be squashed with patch 1.
> 
> I could, but by that same logic, shouldn't patch 3/4 also be squashed into
> patch 1?

Well, I didn't get patch 3 :)

Looking at it now, since patch 3 are changes for libsas, better keep it a
separate patch, and we can see with Martin which tree to use to queue it
(it probably should go through the ata tree).

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] ata: make use of ata_port_is_frozen() helper

[Niklas Cassel]

On Sat, Oct 08, 2022 at 08:56:16AM +0900, Damien Le Moal wrote:
> On 10/8/22 08:50, Niklas Cassel wrote:
> > On Sat, Oct 08, 2022 at 07:31:00AM +0900, Damien Le Moal wrote:
> >> On 10/7/22 22:23, Niklas Cassel wrote:
> >>> Clean up the code by making use of the newly introduced
> >>> ata_port_is_frozen() helper function.
> >>
> >> Looks good, but I think this should be squashed with patch 1.
> > 
> > I could, but by that same logic, shouldn't patch 3/4 also be squashed into
> > patch 1?
> 
> Well, I didn't get patch 3 :)
> 
> Looking at it now, since patch 3 are changes for libsas, better keep it a
> separate patch, and we can see with Martin which tree to use to queue it
> (it probably should go through the ata tree).

+Martin, so that he will see the message.


Kind regards,
Niklas

[PATCH 4/4] ata: libata-core: do not retry reading the log on timeout

[Niklas Cassel]

ata_read_log_page() first tries to read the log using READ LOG DMA EXT.
If that fails it will instead try to read the log using READ LOG EXT.

ata_exec_internal_sg() is synchronous, so it will wait for the command to
finish. If we actually got an error back from the device, it is correct
to retry. However, if the command timed out, ata_exec_internal_sg() will
freeze the port.

There is no point in retrying if the port is frozen, as
ata_exec_internal_sg() will return AC_ERR_SYSTEM on a frozen port,
without ever sending the command down to the drive.

Therefore, avoid retrying if the first command froze the port, as that
will result in a misleading AC_ERR_SYSTEM error print, instead of printing
the error that actually caused the port to be frozen (AC_ERR_TIMEOUT).

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1cf326dd7c41..6ae5787103e7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2000,7 +2000,8 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 	if (err_mask) {
 		if (dma) {
 			dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
-			goto retry;
+			if (!ata_port_is_frozen(dev->link->ap))
+				goto retry;
 		}
 		ata_dev_err(dev,
 			    "Read log 0x%02x page 0x%02x failed, Emask 0x%x\n",
-- 
2.37.3

Re: [PATCH 4/4] ata: libata-core: do not retry reading the log on timeout

[Damien Le Moal]

On 10/7/22 22:23, Niklas Cassel wrote:
> ata_read_log_page() first tries to read the log using READ LOG DMA EXT.
> If that fails it will instead try to read the log using READ LOG EXT.
> 
> ata_exec_internal_sg() is synchronous, so it will wait for the command to
> finish. If we actually got an error back from the device, it is correct
> to retry. However, if the command timed out, ata_exec_internal_sg() will
> freeze the port.
> 
> There is no point in retrying if the port is frozen, as
> ata_exec_internal_sg() will return AC_ERR_SYSTEM on a frozen port,
> without ever sending the command down to the drive.
> 
> Therefore, avoid retrying if the first command froze the port, as that
> will result in a misleading AC_ERR_SYSTEM error print, instead of printing
> the error that actually caused the port to be frozen (AC_ERR_TIMEOUT).

Beside ata_read_log_page(), are there any other path that do a retry after
ata_exec_internal_sg() fails ?

Another note: this is patch 4/4 but I did not get any patch 3/4...

> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  drivers/ata/libata-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1cf326dd7c41..6ae5787103e7 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2000,7 +2000,8 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	if (err_mask) {
>  		if (dma) {
>  			dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
> -			goto retry;
> +			if (!ata_port_is_frozen(dev->link->ap))
> +				goto retry;
>  		}
>  		ata_dev_err(dev,
>  			    "Read log 0x%02x page 0x%02x failed, Emask 0x%x\n",

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] ata: libata-core: do not retry reading the log on timeout

[Niklas Cassel]

On Sat, Oct 08, 2022 at 07:33:54AM +0900, Damien Le Moal wrote:
> On 10/7/22 22:23, Niklas Cassel wrote:
> > ata_read_log_page() first tries to read the log using READ LOG DMA EXT.
> > If that fails it will instead try to read the log using READ LOG EXT.
> > 
> > ata_exec_internal_sg() is synchronous, so it will wait for the command to
> > finish. If we actually got an error back from the device, it is correct
> > to retry. However, if the command timed out, ata_exec_internal_sg() will
> > freeze the port.
> > 
> > There is no point in retrying if the port is frozen, as
> > ata_exec_internal_sg() will return AC_ERR_SYSTEM on a frozen port,
> > without ever sending the command down to the drive.
> > 
> > Therefore, avoid retrying if the first command froze the port, as that
> > will result in a misleading AC_ERR_SYSTEM error print, instead of printing
> > the error that actually caused the port to be frozen (AC_ERR_TIMEOUT).
> 
> Beside ata_read_log_page(), are there any other path that do a retry after
> ata_exec_internal_sg() fails ?

Let me check and get back to you.

> 
> Another note: this is patch 4/4 but I did not get any patch 3/4...

It is here:
https://lore.kernel.org/linux-scsi/20221007132342.1590367-1-niklas.cassel@wdc.com/T/#t

My scripts probably didn't add you since you are not a maintainer or
reviewer for drivers/scsi. I should probably have added you manually.

> 
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  drivers/ata/libata-core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 1cf326dd7c41..6ae5787103e7 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -2000,7 +2000,8 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
> >  	if (err_mask) {
> >  		if (dma) {
> >  			dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
> > -			goto retry;
> > +			if (!ata_port_is_frozen(dev->link->ap))
> > +				goto retry;
> >  		}
> >  		ata_dev_err(dev,
> >  			    "Read log 0x%02x page 0x%02x failed, Emask 0x%x\n",
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

Re: [PATCH 4/4] ata: libata-core: do not retry reading the log on timeout

[Damien Le Moal]

On 10/8/22 08:47, Niklas Cassel wrote:
> On Sat, Oct 08, 2022 at 07:33:54AM +0900, Damien Le Moal wrote:
>> On 10/7/22 22:23, Niklas Cassel wrote:
>>> ata_read_log_page() first tries to read the log using READ LOG DMA EXT.
>>> If that fails it will instead try to read the log using READ LOG EXT.
>>>
>>> ata_exec_internal_sg() is synchronous, so it will wait for the command to
>>> finish. If we actually got an error back from the device, it is correct
>>> to retry. However, if the command timed out, ata_exec_internal_sg() will
>>> freeze the port.
>>>
>>> There is no point in retrying if the port is frozen, as
>>> ata_exec_internal_sg() will return AC_ERR_SYSTEM on a frozen port,
>>> without ever sending the command down to the drive.
>>>
>>> Therefore, avoid retrying if the first command froze the port, as that
>>> will result in a misleading AC_ERR_SYSTEM error print, instead of printing
>>> the error that actually caused the port to be frozen (AC_ERR_TIMEOUT).
>>
>> Beside ata_read_log_page(), are there any other path that do a retry after
>> ata_exec_internal_sg() fails ?
> 
> Let me check and get back to you.
> 
>>
>> Another note: this is patch 4/4 but I did not get any patch 3/4...
> 
> It is here:
> https://lore.kernel.org/linux-scsi/20221007132342.1590367-1-niklas.cassel@wdc.com/T/#t
> 
> My scripts probably didn't add you since you are not a maintainer or
> reviewer for drivers/scsi. I should probably have added you manually.

Yes please, always send full series.

> 
>>
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>  drivers/ata/libata-core.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 1cf326dd7c41..6ae5787103e7 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -2000,7 +2000,8 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>>>  	if (err_mask) {
>>>  		if (dma) {
>>>  			dev->horkage |= ATA_HORKAGE_NO_DMA_LOG;
>>> -			goto retry;
>>> +			if (!ata_port_is_frozen(dev->link->ap))
>>> +				goto retry;
>>>  		}
>>>  		ata_dev_err(dev,
>>>  			    "Read log 0x%02x page 0x%02x failed, Emask 0x%x\n",
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] ata: libata-core: do not retry reading the log on timeout

[Niklas Cassel]

On Sat, Oct 08, 2022 at 01:47:49AM +0200, Niklas Cassel wrote:
> On Sat, Oct 08, 2022 at 07:33:54AM +0900, Damien Le Moal wrote:
> > On 10/7/22 22:23, Niklas Cassel wrote:
> > > ata_read_log_page() first tries to read the log using READ LOG DMA EXT.
> > > If that fails it will instead try to read the log using READ LOG EXT.
> > > 
> > > ata_exec_internal_sg() is synchronous, so it will wait for the command to
> > > finish. If we actually got an error back from the device, it is correct
> > > to retry. However, if the command timed out, ata_exec_internal_sg() will
> > > freeze the port.
> > > 
> > > There is no point in retrying if the port is frozen, as
> > > ata_exec_internal_sg() will return AC_ERR_SYSTEM on a frozen port,
> > > without ever sending the command down to the drive.
> > > 
> > > Therefore, avoid retrying if the first command froze the port, as that
> > > will result in a misleading AC_ERR_SYSTEM error print, instead of printing
> > > the error that actually caused the port to be frozen (AC_ERR_TIMEOUT).
> > 
> > Beside ata_read_log_page(), are there any other path that do a retry after
> > ata_exec_internal_sg() fails ?
> 
> Let me check and get back to you.

Hello Damien,

I couldn't find any other caller of ata_exec_internal_sg() or
ata_exec_internal() where this should be needed.


Kind regards,
Niklas

Re: [PATCH 0/4] libata: misc frozen port cleanups

[Damien Le Moal]

On 10/7/22 22:23, Niklas Cassel wrote:
> Hello there,
> 
> This series adds a new ata_port_is_frozen() helper function,
> and makes use of it in ata and libsas.
> 
> Additionally, improve ata_read_log_page() to avoid a futile
> retry while the port is frozen.
> 
> Kind regards,
> Niklas
> 
> 
> Niklas Cassel (4):
>   ata: add ata_port_is_frozen() helper
>   ata: make use of ata_port_is_frozen() helper
>   scsi: libsas: make use of ata_port_is_frozen() helper
>   ata: libata-core: do not retry reading the log on timeout
> 
>  drivers/ata/libahci.c         |  6 +++---
>  drivers/ata/libata-acpi.c     |  4 ++--
>  drivers/ata/libata-core.c     |  7 ++++---
>  drivers/ata/libata-eh.c       | 21 ++++++++++-----------
>  drivers/ata/libata-sata.c     |  2 +-
>  drivers/ata/libata-scsi.c     |  2 +-
>  drivers/ata/sata_nv.c         |  2 +-
>  drivers/ata/sata_promise.c    |  2 +-
>  drivers/ata/sata_sx4.c        |  2 +-
>  drivers/scsi/libsas/sas_ata.c |  2 +-
>  include/linux/libata.h        |  5 +++++
>  11 files changed, 30 insertions(+), 25 deletions(-)
> 

Applied to for-6.2. Thanks !

-- 
Damien Le Moal
Western Digital Research