[PATCHSET] libata: implement and use ata_dev_revalidate(), take #2

[PATCHSET] libata: implement and use ata_dev_revalidate(), take #2

[Tejun Heo]

Hello, Jeff.

This is the second take of implement-and-use-ata_dev_revalidate
patchset.  In the last take[1], out of four patches, %1 got ACK'ed, %2
had some minor issues, %3 got no comment and %4 had some issue but got
resolved.

Changes from the last take are...

* in #2, @verbose is renamed to @print_info as suggested
* in #2, lba_desc change is described in the patch description

This patchset is against
the current #upstream[2]
+ kill-illegal-kfree patch[3]
+ reorganize-ata_dev_identify patchset[4]

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/8050
[2] cccc65a3b60edaf721cdee5a14f68ba009341822
[3] http://article.gmane.org/gmane.linux.ide/8324
[4] http://article.gmane.org/gmane.linux.ide/8412

[PATCH 4/4] libata: revalidate after transfer mode configuration

[Tejun Heo]

Revalidate device after transfer mode configuration.  This also makes
dev->id up-to-date.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

05f0163669d505077e04f114027bea39083320bc
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index d599e8f..d73109e 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1621,6 +1621,12 @@ static void ata_dev_set_mode(struct ata_
 	idx = ofs + dev->xfer_shift;
 	WARN_ON(idx >= ARRAY_SIZE(xfer_mode_str));
 
+	if (ata_dev_revalidate(ap, dev, 0)) {
+		printk(KERN_ERR "ata%u: failed to revalidate after set "
+		       "xfermode, disabled\n", ap->id);
+		ata_port_disable(ap);
+	}
+
 	DPRINTK("idx=%d xfer_shift=%u, xfer_mode=0x%x, base=0x%x, offset=%d\n",
 		idx, dev->xfer_shift, (int)dev->xfer_mode, (int)base, ofs);
 
-- 
1.2.1

Re: [PATCH 4/4] libata: revalidate after transfer mode configuration

[Jeff Garzik]

Tejun Heo wrote:
> Revalidate device after transfer mode configuration.  This also makes
> dev->id up-to-date.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK 1, 2 and 4.

[PATCH 3/4] libata: implement ata_dev_revalidate()

[Tejun Heo]

ata_dev_revalidate() re-reads IDENTIFY PAGE of the given device and
makes sure it's the same device as the configured one.  Once it's
verified that it's the same device, @dev is configured according to
newly read IDENTIFY PAGE.  Note that revalidation currently doesn't
invoke transfer mode reconfiguration.

Criteria for 'same device'

* same class (of course)
* same model string
* same serial string
* if ATA, same n_sectors (to catch geometry parameter changes)

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |  115 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h     |    2 +
 2 files changed, 117 insertions(+), 0 deletions(-)

5f157f520bce43bee189d18e0736065ce7997334
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 64e087b..d599e8f 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2341,6 +2341,120 @@ int ata_drive_probe_reset(struct ata_por
 	return rc;
 }
 
+/**
+ *	ata_dev_same_device - Determine whether new ID matches configured device
+ *	@ap: port on which the device to compare against resides
+ *	@dev: device to compare against
+ *	@new_class: class of the new device
+ *	@new_id: IDENTIFY page of the new device
+ *
+ *	Compare @new_class and @new_id against @dev and determine
+ *	whether @dev is the device indicated by @new_class and
+ *	@new_id.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	1 if @dev matches @new_class and @new_id, 0 otherwise.
+ */
+static int ata_dev_same_device(struct ata_port *ap, struct ata_device *dev,
+			       unsigned int new_class, const u16 *new_id)
+{
+	const u16 *old_id = dev->id;
+	unsigned char model[2][41], serial[2][21];
+	u64 new_n_sectors;
+
+	if (dev->class != new_class) {
+		printk(KERN_WARNING
+		       "ata%u: dev %u class mismatch %d != %d\n",
+		       ap->id, dev->devno, dev->class, new_class);
+		return 0;
+	}
+
+	ata_id_c_string(old_id, model[0], ATA_ID_PROD_OFS, sizeof(model[0]));
+	ata_id_c_string(new_id, model[1], ATA_ID_PROD_OFS, sizeof(model[1]));
+	ata_id_c_string(old_id, serial[0], ATA_ID_SERNO_OFS, sizeof(serial[0]));
+	ata_id_c_string(new_id, serial[1], ATA_ID_SERNO_OFS, sizeof(serial[1]));
+	new_n_sectors = ata_id_n_sectors(new_id);
+
+	if (strcmp(model[0], model[1])) {
+		printk(KERN_WARNING
+		       "ata%u: dev %u model number mismatch '%s' != '%s'\n",
+		       ap->id, dev->devno, model[0], model[1]);
+		return 0;
+	}
+
+	if (strcmp(serial[0], serial[1])) {
+		printk(KERN_WARNING
+		       "ata%u: dev %u serial number mismatch '%s' != '%s'\n",
+		       ap->id, dev->devno, serial[0], serial[1]);
+		return 0;
+	}
+
+	if (dev->class == ATA_DEV_ATA && dev->n_sectors != new_n_sectors) {
+		printk(KERN_WARNING
+		       "ata%u: dev %u n_sectors mismatch %llu != %llu\n",
+		       ap->id, dev->devno, (unsigned long long)dev->n_sectors,
+		       (unsigned long long)new_n_sectors);
+		return 0;
+	}
+
+	return 1;
+}
+
+/**
+ *	ata_dev_revalidate - Revalidate ATA device
+ *	@ap: port on which the device to revalidate resides
+ *	@dev: device to revalidate
+ *	@post_reset: is this revalidation after reset?
+ *
+ *	Re-read IDENTIFY page and make sure @dev is still attached to
+ *	the port.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, negative errno otherwise
+ */
+int ata_dev_revalidate(struct ata_port *ap, struct ata_device *dev,
+		       int post_reset)
+{
+	unsigned int class;
+	u16 *id;
+	int rc;
+
+	if (!ata_dev_present(dev))
+		return 0;
+
+	class = dev->class;
+	id = NULL;
+
+	/* allocate & read ID data */
+	rc = ata_dev_read_id(ap, dev, &class, post_reset, &id);
+	if (rc)
+		goto fail;
+
+	/* is the device still there? */
+	if (!ata_dev_same_device(ap, dev, class, id)) {
+		rc = -ENODEV;
+		goto fail;
+	}
+
+	kfree(dev->id);
+	dev->id = id;
+
+	/* configure device according to the new ID */
+	return ata_dev_configure(ap, dev, 0);
+
+ fail:
+	printk(KERN_ERR "ata%u: dev %u revalidation failed (errno=%d)\n",
+	       ap->id, dev->devno, rc);
+	kfree(id);
+	return rc;
+}
+
 static void ata_pr_blacklisted(const struct ata_port *ap,
 			       const struct ata_device *dev)
 {
@@ -4960,6 +5074,7 @@ EXPORT_SYMBOL_GPL(sata_std_hardreset);
 EXPORT_SYMBOL_GPL(ata_std_postreset);
 EXPORT_SYMBOL_GPL(ata_std_probe_reset);
 EXPORT_SYMBOL_GPL(ata_drive_probe_reset);
+EXPORT_SYMBOL_GPL(ata_dev_revalidate);
 EXPORT_SYMBOL_GPL(ata_port_disable);
 EXPORT_SYMBOL_GPL(ata_ratelimit);
 EXPORT_SYMBOL_GPL(ata_busy_sleep);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1aab218..1392d22 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -485,6 +485,8 @@ extern int ata_std_softreset(struct ata_
 extern int sata_std_hardreset(struct ata_port *ap, int verbose,
 			      unsigned int *class);
 extern void ata_std_postreset(struct ata_port *ap, unsigned int *classes);
+extern int ata_dev_revalidate(struct ata_port *ap, struct ata_device *dev,
+			      int post_reset);
 extern void ata_port_disable(struct ata_port *);
 extern void ata_std_ports(struct ata_ioports *ioaddr);
 #ifdef CONFIG_PCI
-- 
1.2.1

Re: [PATCH 3/4] libata: implement ata_dev_revalidate()

[Jeff Garzik]

Tejun Heo wrote:
> ata_dev_revalidate() re-reads IDENTIFY PAGE of the given device and
> makes sure it's the same device as the configured one.  Once it's
> verified that it's the same device, @dev is configured according to
> newly read IDENTIFY PAGE.  Note that revalidation currently doesn't
> invoke transfer mode reconfiguration.
> 
> Criteria for 'same device'
> 
> * same class (of course)
> * same model string
> * same serial string
> * if ATA, same n_sectors (to catch geometry parameter changes)
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |  115 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/libata.h     |    2 +
>  2 files changed, 117 insertions(+), 0 deletions(-)
> 
> 5f157f520bce43bee189d18e0736065ce7997334
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 64e087b..d599e8f 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -2341,6 +2341,120 @@ int ata_drive_probe_reset(struct ata_por
>  	return rc;
>  }
>  
> +/**
> + *	ata_dev_same_device - Determine whether new ID matches configured device
> + *	@ap: port on which the device to compare against resides
> + *	@dev: device to compare against
> + *	@new_class: class of the new device
> + *	@new_id: IDENTIFY page of the new device
> + *
> + *	Compare @new_class and @new_id against @dev and determine
> + *	whether @dev is the device indicated by @new_class and
> + *	@new_id.
> + *
> + *	LOCKING:
> + *	None.
> + *
> + *	RETURNS:
> + *	1 if @dev matches @new_class and @new_id, 0 otherwise.
> + */
> +static int ata_dev_same_device(struct ata_port *ap, struct ata_device *dev,
> +			       unsigned int new_class, const u16 *new_id)
> +{
> +	const u16 *old_id = dev->id;
> +	unsigned char model[2][41], serial[2][21];
> +	u64 new_n_sectors;
> +
> +	if (dev->class != new_class) {
> +		printk(KERN_WARNING
> +		       "ata%u: dev %u class mismatch %d != %d\n",
> +		       ap->id, dev->devno, dev->class, new_class);
> +		return 0;
> +	}
> +
> +	ata_id_c_string(old_id, model[0], ATA_ID_PROD_OFS, sizeof(model[0]));
> +	ata_id_c_string(new_id, model[1], ATA_ID_PROD_OFS, sizeof(model[1]));
> +	ata_id_c_string(old_id, serial[0], ATA_ID_SERNO_OFS, sizeof(serial[0]));
> +	ata_id_c_string(new_id, serial[1], ATA_ID_SERNO_OFS, sizeof(serial[1]));
> +	new_n_sectors = ata_id_n_sectors(new_id);
> +
> +	if (strcmp(model[0], model[1])) {
> +		printk(KERN_WARNING
> +		       "ata%u: dev %u model number mismatch '%s' != '%s'\n",
> +		       ap->id, dev->devno, model[0], model[1]);
> +		return 0;
> +	}
> +
> +	if (strcmp(serial[0], serial[1])) {
> +		printk(KERN_WARNING
> +		       "ata%u: dev %u serial number mismatch '%s' != '%s'\n",
> +		       ap->id, dev->devno, serial[0], serial[1]);
> +		return 0;
> +	}
> +
> +	if (dev->class == ATA_DEV_ATA && dev->n_sectors != new_n_sectors) {
> +		printk(KERN_WARNING
> +		       "ata%u: dev %u n_sectors mismatch %llu != %llu\n",
> +		       ap->id, dev->devno, (unsigned long long)dev->n_sectors,
> +		       (unsigned long long)new_n_sectors);
> +		return 0;
> +	}
> +
> +	return 1;

For the hotplug case, its inaccurate to call these warnings.


> + *	ata_dev_revalidate - Revalidate ATA device
> + *	@ap: port on which the device to revalidate resides
> + *	@dev: device to revalidate
> + *	@post_reset: is this revalidation after reset?
> + *
> + *	Re-read IDENTIFY page and make sure @dev is still attached to
> + *	the port.
> + *
> + *	LOCKING:
> + *	Kernel thread context (may sleep)
> + *
> + *	RETURNS:
> + *	0 on success, negative errno otherwise
> + */
> +int ata_dev_revalidate(struct ata_port *ap, struct ata_device *dev,
> +		       int post_reset)
> +{
> +	unsigned int class;
> +	u16 *id;
> +	int rc;
> +
> +	if (!ata_dev_present(dev))
> +		return 0;

I would think the proper return value for this case is -ENODEV.

Otherwise OK.

	Jeff

[PATCH 1/4] libata: re-initialize parameters before configuring

[Tejun Heo]

In ata_dev_configure(), reinitialize parameters before configuring.
This change is for revalidation and hotplug.  As ata_dev_configure()
can be entered multiple times, parameters need to be reinitialized.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

ce0cef100691a38180b32a74ce6ab7ca90b390f0
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 63ba1aa..17d113b 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1082,6 +1082,15 @@ static int ata_dev_configure(struct ata_
 
 	DPRINTK("ENTER, host %u, dev %u\n", ap->id, dev->devno);
 
+	/* initialize to-be-configured parameters */
+	dev->flags = 0;
+	dev->max_sectors = 0;
+	dev->cdb_len = 0;
+	dev->n_sectors = 0;
+	dev->cylinders = 0;
+	dev->heads = 0;
+	dev->sectors = 0;
+
 	/*
 	 * common ATA, ATAPI feature tests
 	 */
-- 
1.2.1