Re: [PATCH libata-dev-2.6 1/2] ata_dev_identify() check status fix

[Albert Lee <albertcc@tw.ibm.com>]

Jeff Garzik wrote:
> Could we not pass the Status value via struct ata_taskfile ?
> 
> 

Jeff,

I'm worried about:

1. struct ata_taskfile is part of struct ata_queued_cmd. The lifecycle of ata_queued_cmd is over
    after __ata_qc_complete() is called. So, when wait_for_completion() returns,
    qc has already been recycled for next use.

2. The following code is seen in ata_pio_error():
      ata_qc_complete(qc, drv_stat | ATA_ERR);
    It looks a good design of libata to me: ata_qc_complete() gets both qc and an additional status parameter.
The status paramerter is mixed of real hardware status and error found by the software.
Instead put the software error to qc->tf.status, maybe we could keep this good design and leave
struct ata_taskfile for the hardware only?


Albert
=====================
(The code in context.)

	spin_lock_irqsave(&ap->host_set->lock, flags);
	rc = ata_qc_issue(qc);
	spin_unlock_irqrestore(&ap->host_set->lock, flags);

	if (rc)
		goto err_out;
	else
		wait_for_completion(&wait);

	// When wait_for_completion() returns,
	// the life cycle of qc was over and
	// qc->tag == ATA_TAG_POISON; here.
	// Maybe we should not access qc here.
	status = qc->tf.status;

	if (status & ATA_ERR) {
		/*
		 * arg!  EDD works for all test cases, but seems to return
		 * the ATA signature for some ATAPI devices.  Until the
		 * reason for this is found and fixed, we fix up the mess
		 * here.  If IDENTIFY DEVICE returns command aborted
		 * (as ATAPI devices do), then we issue an
		 * IDENTIFY PACKET DEVICE.
		 *
		 * ATA software reset (SRST, the default) does not appear
		 * to have this problem.
		 */
		if ((using_edd) && (qc->tf.command == ATA_CMD_ID_ATA)) {
			u8 err = ata_chk_err(ap);
			if (err & ATA_ABORTED) {
				dev->class = ATA_DEV_ATAPI;
				//
				// We should call ata_qc_new_init()
				// to revive qc here?
				//
				qc->cursg = 0;
				qc->cursg_ofs = 0;
				qc->cursect = 0;
				qc->nsect = 1;
				goto retry;
			}
		}
		goto err_out;
	}