Re: [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands

[Jeff Garzik <jgarzik@pobox.com>]

Albert Lee wrote:
> Patch 3/3:
>   check qc->err_mask for the internal commands.
> 
> Problem:
>   The results of internal commands issued by libata itself were not checked.
> 
>   For example, ata_dev_identify() only checks the device status (qc->tf.command).
>   However, checking the device status is not enough to detect all errors.
> 
> Changes:
>   - check qc->err_mask for the internal commands:
>     - ata_dev_identify()
>     - ata_dev_set_xfermode()
>     - ata_dev_reread_id()
>     - ata_dev_init_params() 
>     - atapi_request_sense()
> 
> The patch accesses qc (and ap) after the qc is completed.
> This is bad from the object life cycle point of view and might cause race.
> However, since libata has complete control over the hardware when libata issues the internal commands
> and it seems noboby else may issue command at the same time, 
> accessing qc after ata_qc_complete() looks to be ok here.
> 
> p.s. 
>      The patch above can also make ata_dev_identify() detect the
>      "phantom slave device" problem.
> 
>      The "phantom slave device" was seen in specific PATA master-only configuration.
>      The master device responds to some commands issued to the slave device, and
>      make a illusion that the slave device exists.
>      The device status register of the "phantom slave device" always read as zero.
>      So, using (qc->tf.command & ATA_ERR) cannot detect the error.
>      Checking qc->err_mask can make ata_dev_identify() detect such 
>      "phantom slave device" and exclude the phantom device.
> 
> For your review and advice, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

In general this seems like an OK direction to take, but I am a bit 
worried about the object lifetime issues that you mention.  It is IMO 
worth considering a better lifetime strategy at this point, before 
applying this [worthwhile] change.

	Jeff