Re: [PATCH 3/7] libata: separate out ata_dev_read_id()

Re: [PATCH 3/7] libata: separate out ata_dev_read_id()

[Jeff Garzik]

Tejun Heo wrote:
> Separate out ata_dev_read_id() from ata_dev_identify().  This is the
> first half of splitting ata_dev_identify().  ata_dev_read_id() will
> also be used for revalidation.  This patch does not make any behavior
> change.
> 
> ata_dev_read_id() doesn't modify any of libata-internal data
> structures.  It simply reads IDENTIFY page and returns error code on
> failure.  INIT_DEV_PARAMS and EDD wrong class code are also handled by
> this function.
> 
> Re-reading IDENTIFY after INIT_DEV_PARAMS is performed by jumping to
> retry: instead of calling ata_dev_reread_id().  This is done because
> 1. there's retry label anyway 2. ata_dev_reread_id() cannot be used
> anywhere else so there's no reason to keep it.
> 
> This function is probably the place to set transfer mode to PIO0
> before IDENTIFY.  However, reset -> identify -> init_dev_params order
> should be kept for pre-ATA4 devices so we cannot set transfer mode
> before IDENTIFY for them.  How do we know if a device is post-ATA4
> before IDENTIFY?
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |  219 +++++++++++++++++++++++++++-----------------
>  1 files changed, 135 insertions(+), 84 deletions(-)
> 
> 44fd0f6a248de6216af9ee63ee1ccb2bb438bf7b
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index a435454..6de78d1 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -903,42 +903,37 @@ unsigned int ata_pio_need_iordy(const st
>  }
>  
>  /**
> - *	ata_dev_identify - obtain IDENTIFY x DEVICE page
> - *	@ap: port on which device we wish to probe resides
> - *	@device: device bus address, starting at zero
> - *
> - *	Following bus reset, we issue the IDENTIFY [PACKET] DEVICE
> - *	command, and read back the 512-byte device information page.
> - *	The device information page is fed to us via the standard
> - *	PIO-IN protocol, but we hand-code it here. (TODO: investigate
> - *	using standard PIO-IN paths)
> - *
> - *	After reading the device information page, we use several
> - *	bits of information from it to initialize data structures
> - *	that will be used during the lifetime of the ata_device.
> - *	Other data from the info page is used to disqualify certain
> - *	older ATA devices we do not wish to support.
> + *	ata_dev_read_id - Read ID data from the specified device
> + *	@ap: port on which target device resides
> + *	@dev: target device
> + *	@p_class: pointer to class of the target device (may be changed)
> + *	@post_reset: is this read ID post-reset?
> + *	@p_id: read IDENTIFY page (newly allocated)
> + *
> + *	Read ID data from the specified device.  ATA_CMD_ID_ATA is
> + *	performed on ATA devices and ATA_CMD_ID_ATAPI on ATAPI
> + *	devices.  This function also takes care of EDD signature
> + *	misreporting (to be removed once EDD support is gone) and
> + *	issues ATA_CMD_INIT_DEV_PARAMS for pre-ATA4 drives.
>   *
>   *	LOCKING:
> - *	Inherited from caller.  Some functions called by this function
> - *	obtain the host_set lock.
> + *	Kernel thread context (may sleep)
> + *
> + *	RETURNS:
> + *	0 on success, -errno otherwise.
>   */
> -
> -static void ata_dev_identify(struct ata_port *ap, unsigned int device)
> +static int ata_dev_read_id(struct ata_port *ap, struct ata_device *dev,
> +			   unsigned int *p_class, int post_reset, u16 **p_id)
>  {
> -	struct ata_device *dev = &ap->device[device];
> -	unsigned int major_version;
> -	unsigned long xfer_modes;
> +	unsigned int class = *p_class;
>  	unsigned int using_edd;
>  	struct ata_taskfile tf;
> -	unsigned int err_mask;
> -	int i, rc;
> +	unsigned int err_mask = 0;
> +	u16 *id;
> +	const char *reason;
> +	int rc;
>  
> -	if (!ata_dev_present(dev)) {
> -		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
> -			ap->id, device);
> -		return;
> -	}
> +	DPRINTK("ENTER, host %u, dev %u\n", ap->id, dev->devno);
>  
>  	if (ap->ops->probe_reset ||
>  	    ap->flags & (ATA_FLAG_SRST | ATA_FLAG_SATA_RESET))
> @@ -946,34 +941,40 @@ static void ata_dev_identify(struct ata_
>  	else
>  		using_edd = 1;
>  
> -	DPRINTK("ENTER, host %u, dev %u\n", ap->id, device);
> -
> -	WARN_ON(dev->class != ATA_DEV_ATA && dev->class != ATA_DEV_ATAPI &&
> -		dev->class != ATA_DEV_NONE);
> +	ata_dev_select(ap, dev->devno, 1, 1); /* select device 0/1 */
>  
> -	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
> -
> -	dev->id = kmalloc(sizeof(dev->id[0]) * ATA_ID_WORDS, GFP_KERNEL);
> -	if (dev->id == NULL)
> +	id = kmalloc(sizeof(id[0]) * ATA_ID_WORDS, GFP_KERNEL);
> +	if (id == NULL) {
> +		rc = -ENOMEM;
> +		reason = "out of memory";
>  		goto err_out;
> +	}
>  
> -retry:
> -	ata_tf_init(ap, &tf, device);
> + retry:
> +	ata_tf_init(ap, &tf, dev->devno);
>  
> -	if (dev->class == ATA_DEV_ATA) {
> +	switch (class) {
> +	case ATA_DEV_ATA:
>  		tf.command = ATA_CMD_ID_ATA;
> -		DPRINTK("do ATA identify\n");
> -	} else {
> +		break;
> +	case ATA_DEV_ATAPI:
>  		tf.command = ATA_CMD_ID_ATAPI;
> -		DPRINTK("do ATAPI identify\n");
> +		break;
> +	default:
> +		rc = -ENODEV;
> +		reason = "unsupported class";
> +		goto err_out;
>  	}
>  
>  	tf.protocol = ATA_PROT_PIO;
>  
>  	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
> -				     dev->id, sizeof(dev->id[0]) * ATA_ID_WORDS);
> +				     id, sizeof(id[0]) * ATA_ID_WORDS);
>  
>  	if (err_mask) {
> +		rc = -EIO;
> +		reason = "I/O error";
> +
>  		if (err_mask & ~AC_ERR_DEV)
>  			goto err_out;
>  
> @@ -988,25 +989,106 @@ retry:
>  		 * ATA software reset (SRST, the default) does not appear
>  		 * to have this problem.
>  		 */
> -		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
> +		if ((using_edd) && (class == ATA_DEV_ATA)) {
>  			u8 err = tf.feature;
>  			if (err & ATA_ABORTED) {
> -				dev->class = ATA_DEV_ATAPI;
> +				class = ATA_DEV_ATAPI;
>  				goto retry;
>  			}
>  		}
>  		goto err_out;
>  	}
>  
> -	swap_buf_le16(dev->id, ATA_ID_WORDS);
> +	swap_buf_le16(id, ATA_ID_WORDS);
>  
>  	/* print device capabilities */
>  	printk(KERN_DEBUG "ata%u: dev %u cfg "
>  	       "49:%04x 82:%04x 83:%04x 84:%04x 85:%04x 86:%04x 87:%04x 88:%04x\n",
> -	       ap->id, device, dev->id[49],
> -	       dev->id[82], dev->id[83], dev->id[84],
> -	       dev->id[85], dev->id[86], dev->id[87],
> -	       dev->id[88]);
> +	       ap->id, dev->devno,
> +	       id[49], id[82], id[83], id[84], id[85], id[86], id[87], id[88]);
> +
> +	/* sanity check */
> +	if ((class == ATA_DEV_ATA) != ata_id_is_ata(id)) {
> +		rc = -EINVAL;
> +		reason = "device reports illegal type";
> +		goto err_out;
> +	}
> +
> +	if (post_reset && class == ATA_DEV_ATA) {
> +		/*
> +		 * The exact sequence expected by certain pre-ATA4 drives is:
> +		 * SRST RESET
> +		 * IDENTIFY
> +		 * INITIALIZE DEVICE PARAMETERS
> +		 * anything else..
> +		 * Some drives were very specific about that exact sequence.
> +		 */
> +		if (ata_id_major_version(id) < 4 || !ata_id_has_lba(id)) {
> +			err_mask = ata_dev_init_params(ap, dev);
> +			if (err_mask) {
> +				rc = -EIO;
> +				reason = "INIT_DEV_PARAMS failed";
> +				goto err_out;
> +			}
> +
> +			/* current CHS translation info (id[53-58]) might be
> +			 * changed. reread the identify device info.
> +			 */
> +			post_reset = 0;
> +			goto retry;
> +		}
> +	}
> +
> +	*p_class = class;
> +	*p_id = id;
> +	return 0;

I think you should order this patch before the dev->id[0] reworked patch.

In particular, "p_id = id" appears to stomp on the existing dev->id 
allocation.

	Jeff

Re: [PATCH 3/7] libata: separate out ata_dev_read_id()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Separate out ata_dev_read_id() from ata_dev_identify().  This is the
>> first half of splitting ata_dev_identify().  ata_dev_read_id() will
>> also be used for revalidation.  This patch does not make any behavior
>> change.
>>
>> ata_dev_read_id() doesn't modify any of libata-internal data
>> structures.  It simply reads IDENTIFY page and returns error code on
>> failure.  INIT_DEV_PARAMS and EDD wrong class code are also handled by
>> this function.
>>
>> Re-reading IDENTIFY after INIT_DEV_PARAMS is performed by jumping to
>> retry: instead of calling ata_dev_reread_id().  This is done because
>> 1. there's retry label anyway 2. ata_dev_reread_id() cannot be used
>> anywhere else so there's no reason to keep it.
>>
>> This function is probably the place to set transfer mode to PIO0
>> before IDENTIFY.  However, reset -> identify -> init_dev_params order
>> should be kept for pre-ATA4 devices so we cannot set transfer mode
>> before IDENTIFY for them.  How do we know if a device is post-ATA4
>> before IDENTIFY?
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
> 
> I think you should order this patch before the dev->id[0] reworked patch.

Will do.

> In particular, "p_id = id" appears to stomp on the existing dev->id 
> allocation.

Hmmm... ata_dev_read_id() is responsible for allocating id array and the 
caller is responsible for making sure what p_id points to is 
deallocated. e.g. ata_dev_revalidate() frees dev->id before passing it 
to ata_dev_read_id().  Seems problemetic?

-- 
tejun