Patchwork [v8,05/11] libata-eh: allow defer in ata_exec_internal

login
register
mail settings
Submitter Aaron Lu
Date Oct. 29, 2012, 9:01 a.m.
Message ID <1351501298-3716-6-git-send-email-aaron.lu@intel.com>
Download mbox | patch
Permalink /patch/194909/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Aaron Lu - Oct. 29, 2012, 9:01 a.m.
ata_exec_internal will preempt the ata link's active_tag and ata port's
qc_active flags, this is OK for error recovery, but if normal code path
wants to use ata_exec_internal, there is a problem: we need to check if
it is OK to issue a new command with the help of port_ops->defer.

In ZPODD, I'll need to find out the loading mechanism of the ODD by
issuing a GET_CONFIGURATION command. And this command may very well
race with commands issued from SCSI layer. So instead of preempt the
current command, defer the new command if it's not OK to issue it, as
it is always wrong to issue a non-NCQ command when there is command(s)
in processing.

So ata_exec_internal is modified to check if it is in eh recovery
environment, and if yes, act as before; if not, check if this command
should be defered with the help of port_ops->defer.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-core.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)
Tejun Heo - Oct. 29, 2012, 3:20 p.m.
On Mon, Oct 29, 2012 at 05:01:32PM +0800, Aaron Lu wrote:
> ata_exec_internal will preempt the ata link's active_tag and ata port's
> qc_active flags, this is OK for error recovery, but if normal code path
> wants to use ata_exec_internal, there is a problem: we need to check if
> it is OK to issue a new command with the help of port_ops->defer.
> 
> In ZPODD, I'll need to find out the loading mechanism of the ODD by
> issuing a GET_CONFIGURATION command. And this command may very well
> race with commands issued from SCSI layer. So instead of preempt the
> current command, defer the new command if it's not OK to issue it, as
> it is always wrong to issue a non-NCQ command when there is command(s)
> in processing.

Why not do the discovery from EH?

Thanks.
Aaron Lu - Oct. 30, 2012, 3 a.m.
On 10/29/2012 11:20 PM, Tejun Heo wrote:
> On Mon, Oct 29, 2012 at 05:01:32PM +0800, Aaron Lu wrote:
>> ata_exec_internal will preempt the ata link's active_tag and ata port's
>> qc_active flags, this is OK for error recovery, but if normal code path
>> wants to use ata_exec_internal, there is a problem: we need to check if
>> it is OK to issue a new command with the help of port_ops->defer.
>>
>> In ZPODD, I'll need to find out the loading mechanism of the ODD by
>> issuing a GET_CONFIGURATION command. And this command may very well
>> race with commands issued from SCSI layer. So instead of preempt the
>> current command, defer the new command if it's not OK to issue it, as
>> it is always wrong to issue a non-NCQ command when there is command(s)
>> in processing.
> 
> Why not do the discovery from EH?

Do you mean set a device level EH flag and then schedule EH to do the
discovery?

Thanks,
Aaron

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Oct. 30, 2012, 3:01 a.m.
Hello,

On Tue, Oct 30, 2012 at 11:00:16AM +0800, Aaron Lu wrote:
> > Why not do the discovery from EH?
> 
> Do you mean set a device level EH flag and then schedule EH to do the
> discovery?

Yeah, something like that or why can't it be done while probing the
device in the first place?

Thanks.
Aaron Lu - Oct. 30, 2012, 3:09 a.m.
On 10/30/2012 11:01 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 30, 2012 at 11:00:16AM +0800, Aaron Lu wrote:
>>> Why not do the discovery from EH?
>>
>> Do you mean set a device level EH flag and then schedule EH to do the
>> discovery?
> 
> Yeah, something like that or why can't it be done while probing the
> device in the first place?

I'm not aware of a place to store such ODD specific information when
probing the device.

I'm currently storing the loading mech type in structure zpodd, which
gets created after the corresponding SCSI device gets created in
ata_scsi_scan_host, so at the probe time, the zpodd structure does not
exist yet. And the reason I create the zpodd sturcture this late is
that, it is only created when the ODD together the platform is ZPODD
capable, and to find out if this platform is ZPODD capbale, ACPI binding
has to occur first, and ACPI binding happens when SCSI device is added
to the device tree.

Thanks,
Aaron

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Oct. 31, 2012, 9:52 p.m.
Hello,

On Tue, Oct 30, 2012 at 11:09:21AM +0800, Aaron Lu wrote:
> I'm not aware of a place to store such ODD specific information when
> probing the device.

You can always add some fields. :)

> I'm currently storing the loading mech type in structure zpodd, which
> gets created after the corresponding SCSI device gets created in
> ata_scsi_scan_host, so at the probe time, the zpodd structure does not
> exist yet. And the reason I create the zpodd sturcture this late is
> that, it is only created when the ODD together the platform is ZPODD
> capable, and to find out if this platform is ZPODD capbale, ACPI binding
> has to occur first, and ACPI binding happens when SCSI device is added
> to the device tree.

Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
during probing.  It's a different one, I presume?

Thanks.
Aaron Lu - Nov. 1, 2012, 2:35 a.m.
On 11/01/2012 05:52 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 30, 2012 at 11:09:21AM +0800, Aaron Lu wrote:
>> I'm not aware of a place to store such ODD specific information when
>> probing the device.
> 
> You can always add some fields. :)

OK. My concern is that, such information is only useful to ZPODD
capable device+platforms, so checking this loading mechanism thing for
all ATAPI devices during probe time doesn't seem a good idea.

> 
>> I'm currently storing the loading mech type in structure zpodd, which
>> gets created after the corresponding SCSI device gets created in
>> ata_scsi_scan_host, so at the probe time, the zpodd structure does not
>> exist yet. And the reason I create the zpodd sturcture this late is
>> that, it is only created when the ODD together the platform is ZPODD
>> capable, and to find out if this platform is ZPODD capbale, ACPI binding
>> has to occur first, and ACPI binding happens when SCSI device is added
>> to the device tree.
> 
> Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
> during probing.  It's a different one, I presume?

Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
libata: bind the Linux device tree to the ACPI device tree
ACPI binding happens when SCSI devices are added to the device tree. The
ata port/device software structure does not have a acpi_handle field
anymore.

Thanks,
Aaron

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Nov. 1, 2012, 4:03 p.m.
Hello, Aaron.

On Thu, Nov 01, 2012 at 10:35:10AM +0800, Aaron Lu wrote:
> > You can always add some fields. :)
> 
> OK. My concern is that, such information is only useful to ZPODD
> capable device+platforms, so checking this loading mechanism thing for
> all ATAPI devices during probe time doesn't seem a good idea.

Hmmm.. but it's not like querying acpi is high cost or anything.
Maybe I'm missing something but if it can be simpler that way, please
do so by all means.  I don't care whether you add some extra fields or
some processing overhead during probing.  It doesn't really matter.

> > Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
> > during probing.  It's a different one, I presume?
> 
> Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
> libata: bind the Linux device tree to the ACPI device tree
> ACPI binding happens when SCSI devices are added to the device tree. The
> ata port/device software structure does not have a acpi_handle field
> anymore.

Please bear with me.  I haven't paid much attention to zpodd, so it
probably is my ignorance but at least the ATA <-> ACPI association
happens during probing by calling ata_acpi_on_devcfg() from
ata_dev_configure(), and it pretty much has to happen then because
_SDD/_GTF should be executed after hardreset which happens during
probing.  So, yeah, I'm confused.

Thanks.
Aaron Lu - Nov. 2, 2012, 12:43 a.m.
On 11/02/2012 12:03 AM, Tejun Heo wrote:
> Hello, Aaron.
> 
> On Thu, Nov 01, 2012 at 10:35:10AM +0800, Aaron Lu wrote:
>>> You can always add some fields. :)
>>
>> OK. My concern is that, such information is only useful to ZPODD
>> capable device+platforms, so checking this loading mechanism thing for
>> all ATAPI devices during probe time doesn't seem a good idea.
> 
> Hmmm.. but it's not like querying acpi is high cost or anything.
> Maybe I'm missing something but if it can be simpler that way, please
> do so by all means.  I don't care whether you add some extra fields or
> some processing overhead during probing.  It doesn't really matter.

OK, thanks for the suggestion.

> 
>>> Hmm... I see.  Which ACPI binding is it?  The ATA ACPI binding happens
>>> during probing.  It's a different one, I presume?
>>
>> Since commit 6b66d95895c149cbc04d4fac5a2f5477c543a8ae:
>> libata: bind the Linux device tree to the ACPI device tree
>> ACPI binding happens when SCSI devices are added to the device tree. The
>> ata port/device software structure does not have a acpi_handle field
>> anymore.
> 
> Please bear with me.  I haven't paid much attention to zpodd, so it
> probably is my ignorance but at least the ATA <-> ACPI association
> happens during probing by calling ata_acpi_on_devcfg() from
> ata_dev_configure(), and it pretty much has to happen then because
> _SDD/_GTF should be executed after hardreset which happens during
> probing.  So, yeah, I'm confused.

Oh, yes, ACPI handle can be retrieved any time by quering some ACPI
method. And during probe time, it is done that way, while the binding
happens much later... But the ACPI handle can indeed be fetched at that
time :-)
So all the conditions I need to test ZPODD support is ready during probe
time and I'll move the check loading mechanism code there, thanks for
your suggestion.

Thanks,
Aaron

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 611050d..95fb7b8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1557,6 +1557,7 @@  unsigned ata_exec_internal_sg(struct ata_device *dev,
 	unsigned long flags;
 	unsigned int err_mask;
 	int rc;
+	bool eh_in_recover;
 
 	spin_lock_irqsave(ap->lock, flags);
 
@@ -1588,14 +1589,21 @@  unsigned ata_exec_internal_sg(struct ata_device *dev,
 	qc->dev = dev;
 	ata_qc_reinit(qc);
 
-	preempted_tag = link->active_tag;
-	preempted_sactive = link->sactive;
-	preempted_qc_active = ap->qc_active;
-	preempted_nr_active_links = ap->nr_active_links;
-	link->active_tag = ATA_TAG_POISON;
-	link->sactive = 0;
-	ap->qc_active = 0;
-	ap->nr_active_links = 0;
+	eh_in_recover = ap->pflags & ATA_PFLAG_EH_IN_PROGRESS;
+	if (eh_in_recover) {
+		preempted_tag = link->active_tag;
+		preempted_sactive = link->sactive;
+		preempted_qc_active = ap->qc_active;
+		preempted_nr_active_links = ap->nr_active_links;
+		link->active_tag = ATA_TAG_POISON;
+		link->sactive = 0;
+		ap->qc_active = 0;
+		ap->nr_active_links = 0;
+	} else if (ap->ops->qc_defer && ap->ops->qc_defer(qc)) {
+		ata_qc_free(qc);
+		spin_unlock_irqrestore(ap->lock, flags);
+		return -EBUSY;
+	}
 
 	/* prepare & issue qc */
 	qc->tf = *tf;
@@ -1687,10 +1695,12 @@  unsigned ata_exec_internal_sg(struct ata_device *dev,
 	err_mask = qc->err_mask;
 
 	ata_qc_free(qc);
-	link->active_tag = preempted_tag;
-	link->sactive = preempted_sactive;
-	ap->qc_active = preempted_qc_active;
-	ap->nr_active_links = preempted_nr_active_links;
+	if (eh_in_recover) {
+		link->active_tag = preempted_tag;
+		link->sactive = preempted_sactive;
+		ap->qc_active = preempted_qc_active;
+		ap->nr_active_links = preempted_nr_active_links;
+	}
 
 	spin_unlock_irqrestore(ap->lock, flags);