libata: reorganize ata_dev_identify(), take 3
PATCH 1/1 libata: reorganize ata_dev_identify(), take 3
Hello, Jeff.
This is the third take of reorganize-ata_dev_identify patchset. In the last take[1], two patches (%1 and %2) made into #upstream out of six patches. %2 contained an illegal kfree in ata_dev_read_id() which is left over of patch reordering. The bug was spotted by Randy Dunlap and patch was submitted[2].
This patchset contains the remaining four patches regenerated against the current #upstream[3] + fix-illegal-kfree patch[2].
Thanks.
--
tejun
[1] http://article.gmane.org/gmane.linux.ide/8176
[2] http://article.gmane.org/gmane.linux.ide/8324
[3] cccc65a3b60edaf721cdee5a14f68ba009341822
PATCH 1/4 libata: convert dev->id to pointer
Convert dev->id from array to pointer. This is to accomodate revalidation. During revalidation, both old and new IDENTIFY pages should be accessible and single ->id array doesn't cut it.
---
drivers/scsi/libata-core.c | 20 +++++++++++++++++--- include/linux/libata.h | 2 +- 2 files changed, 18 insertions(+), 4 deletions(-)
6ae458d0e6b0f04d5092e79a8e45a4c187037d27
drivers/scsi/libata-core.c +17 -3
@@ -907,7 +907,7 @@ unsigned int ata_pio_need_iordy(const st* @dev: target device * @p_class: pointer to class of the target device (may be changed) * @post_reset: is this read ID post-reset? -* @id: buffer to fill IDENTIFY page into +* @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
@@ -922,12 +922,13 @@ unsigned int ata_pio_need_iordy(const st* 0 on success, -errno otherwise. */ static int ata_dev_read_id (struct ata_port *ap,struct ata_device *dev, -unsigned int *p_class,int post_reset,u16 *id) +unsigned int *p_class,int post_reset,u16 **p_id) {unsigned int class = *p_class;unsigned int using_edd;struct ata_taskfile tf;unsigned int err_mask =0 ; +u16 *id;const char *reason;int rc;
@@ -941,6 +942,13 @@ static int ata_dev_read_id(struct ata_poata_dev_select (ap, dev->devno,1 ,1 );/* select device 0/1 */ + 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, dev->devno);
@@ -1031,11 +1039,13 @@ static int ata_dev_read_id(struct ata_po } *p_class = class; + *p_id = id;return 0 ; err_out:printk (KERN_WARNING "ata%u: dev %u failed to IDENTIFY (%s)\n" , ap->id, dev->devno, reason); +kfree (id);return rc; }
@@ -1075,7 +1085,8 @@ static void ata_dev_identify(struct ata_DPRINTK ("ENTER, host %u, dev %u\n" , ap->id, device); - rc =ata_dev_read_id (ap, dev, &dev->class,1 , dev->id); +WARN_ON (dev->id !=NULL ); + rc =ata_dev_read_id (ap, dev, &dev->class,1 , &dev->id);if (rc) goto err_out;
@@ -4728,11 +4739,14 @@ void ata_host_set_remove(struct ata_hostint ata_scsi_release (struct Scsi_Host *host) {struct ata_port *ap = (struct ata_port *) &host->hostdata[0 ]; +int i;DPRINTK ("ENTER\n" ); ap->ops->port_disable (ap);ata_host_remove (ap,0 ); +for (i =0 ; i <ATA_MAX_DEVICES ; i++) +kfree (ap->device[i].id);DPRINTK ("EXIT\n" );return 1 ;
include/linux/libata.h +1 -1
@@ -339,7 +339,7 @@ struct ata_device {unsigned long flags;/* ATA_DFLAG_xxx */ unsigned int class;/* ATA_DEV_xxx */ unsigned int devno;/* 0 or 1 */ -u16 id[ATA_ID_WORDS ];/* IDENTIFY xxx DEVICE data */ +u16 *id;/* IDENTIFY xxx DEVICE data */ u8 pio_mode;u8 dma_mode;u8 xfer_mode;
Signature · 1 line
1.2.1
Re: [PATCH 1/4] libata: convert dev->id to pointer
Quoted 5 lines · Tejun Heo
Convert
dev->idfrom array to pointer. This is to accomodate revalidation. During revalidation, both old and new IDENTIFY pages should be accessible and single ->id array doesn't cut it.Signed-off-by Tejun Heo <[email protected]>
applied 1-4
PATCH 2/4 libata: separate out ata_dev_configure()
In reply to Tejun HeoSeparate out ata_dev_configure() from ata_dev_identify() such that ata_dev_configure() only configures @dev according to passed in @id. The function now does not disable device on failure, it just returns appropirate error code.
As this change leaves ata_dev_identify() with only reading ID, calling configure and disabling devices according to the results, this patch also kills ata_dev_identify() and inlines the logic into ata_bus_probe().
---
drivers/scsi/libata-core.c | 78 ++++++++++++++++++++++---------------------- 1 files changed, 39 insertions(+), 39 deletions(-)
9e8f99c18ddb9615403cdeca438a2d3c26b012fb
drivers/scsi/libata-core.c +39 -39
@@ -1050,45 +1050,31 @@ static int ata_dev_read_id(struct ata_po }/** -* 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 +* ata_dev_configure - Configure the specified ATA/ATAPI device +* @ap: Port on which target device resides +* @dev: Target device to configure * -* 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. +* Configure @dev according to @dev->id. Generic and low-level +* driver specific fixups are also applied. * * 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_configure (struct ata_port *ap,struct ata_device *dev) { -struct ata_device *dev = &ap->device[device];unsigned long xfer_modes;int i, rc;if (!ata_dev_present (dev)) {DPRINTK ("ENTER/EXIT (host %u, dev %u) -- nodev\n" , - ap->id, device); -return ; + ap->id, dev->devno); +return 0 ; } -DPRINTK ("ENTER, host %u, dev %u\n" , ap->id, device); - -WARN_ON (dev->id !=NULL ); - rc =ata_dev_read_id (ap, dev, &dev->class,1 , &dev->id); -if (rc) - goto err_out; +DPRINTK ("ENTER, host %u, dev %u\n" , ap->id, dev->devno);/* * common ATA, ATAPI feature tests
@@ -1097,6 +1083,7 @@ static void ata_dev_identify(struct ata_/* we require DMA support (bits 8 of word 49) */ if (!ata_id_has_dma (dev->id)) {printk (KERN_DEBUG "ata%u: no dma\n" , ap->id); + rc = -EINVAL ; goto err_out_nosup; }
@@ -1121,12 +1108,12 @@ static void ata_dev_identify(struct ata_/* print device info to dmesg */ printk (KERN_INFO "ata%u: dev %u ATA-%d, max %s, %Lu sectors:%s\n" , - ap->id, device, + ap->id, dev->devno,ata_id_major_version (dev->id),ata_mode_string (xfer_modes), (unsigned long long )dev->n_sectors, dev->flags &ATA_DFLAG_LBA48 ?" LBA48" :" LBA" ); - }else { + }else {/* CHS */ /* Default translation */
@@ -1143,7 +1130,7 @@ static void ata_dev_identify(struct ata_/* print device info to dmesg */ printk(KERN_INFO "ata%u: dev %u ATA-%d, max %s, %Lu sectors: CHS %d/%d/%d\n", - ap->id, device, + ap->id, dev->devno, ata_id_major_version(dev->id), ata_mode_string(xfer_modes), (unsigned long long)dev->n_sectors,
@@ -1159,13 +1146,14 @@ static void ata_dev_identify(struct ata_ rc =atapi_cdb_len (dev->id);if ((rc <12 ) || (rc >ATAPI_CDB_LEN )) {printk (KERN_WARNING "ata%u: unsupported CDB len\n" , ap->id); + rc = -EINVAL ; goto err_out_nosup; } dev->cdb_len = (unsigned int ) rc;/* print device info to dmesg */ printk (KERN_INFO "ata%u: dev %u ATAPI, max %s\n" , - ap->id, device, + ap->id, dev->devno,ata_mode_string (xfer_modes)); }
@@ -1176,14 +1164,13 @@ static void ata_dev_identify(struct ata_ ap->device[i].cdb_len);DPRINTK ("EXIT, drv_stat = 0x%x\n" ,ata_chk_status (ap)); -return ; +return 0 ; err_out_nosup:printk (KERN_WARNING "ata%u: dev %u not supported, ignoring\n" , - ap->id, device); -err_out: - dev->class++;/* converts ATA_DEV_xxx into ATA_DEV_xxx_UNSUP */ + ap->id, dev->devno);DPRINTK ("EXIT, err\n" ); +return rc; }
@@ -1259,11 +1246,24 @@ static int ata_bus_probe(struct ata_port goto err_out;for (i =0 ; i <ATA_MAX_DEVICES ; i++) { -ata_dev_identify (ap, i); -if (ata_dev_present (&ap->device[i])) { - found =1 ; -ata_dev_config (ap,i); +struct ata_device *dev = &ap->device[i]; + +if (!ata_dev_present (dev)) +continue ; + +WARN_ON (dev->id !=NULL ); +if (ata_dev_read_id (ap, dev, &dev->class,1 , &dev->id)) { + dev->class =ATA_DEV_NONE ; +continue ; } + +if (ata_dev_configure (ap, dev)) { + dev->class++;/* disable device */ +continue ; + } + +ata_dev_config (ap, i); + found =1 ; }if ((!found) || (ap->flags &ATA_FLAG_PORT_DISABLED ))
Signature · 1 line
1.2.1
PATCH 3/4 libata: fold ata_dev_config() into ata_dev_configure()
In reply to Tejun Heoata_dev_config() needs to be done everytime a device is configured. Fold it into ata_dev_configure().
---
drivers/scsi/libata-core.c | 48 ++++++++++++++++---------------------------- include/linux/libata.h | 1 - 2 files changed, 17 insertions(+), 32 deletions(-)
8686b79624ba861834b6648a043c1ca95ea806dd
drivers/scsi/libata-core.c +17 -31
@@ -1049,6 +1049,12 @@ static int ata_dev_read_id(struct ata_poreturn rc; } +static inline u8 ata_dev_knobble (const struct ata_port *ap, +struct ata_device *dev) +{ +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); +} +/** * ata_dev_configure - Configure the specified ATA/ATAPI device * @ap: Port on which target device resides
@@ -1163,6 +1169,17 @@ static int ata_dev_configure(struct ata_ ap->host->max_cmd_len, ap->device[i].cdb_len); +/* limit bridge transfers to udma5, 200 sectors */ +if (ata_dev_knobble (ap, dev)) { +printk (KERN_INFO "ata%u(%u): applying bridge limits\n" , + ap->id, dev->devno); + ap->udma_mask &=ATA_UDMA5 ; + dev->max_sectors =ATA_MAX_SECTORS ; + } + +if (ap->ops->dev_config) + ap->ops->dev_config (ap, dev); +DPRINTK ("EXIT, drv_stat = 0x%x\n" ,ata_chk_status (ap));return 0 ;
@@ -1173,35 +1190,6 @@ err_out_nosup:return rc; } - -static inline u8 ata_dev_knobble (const struct ata_port *ap, -struct ata_device *dev) -{ -return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); -} - -/** -* ata_dev_config - Run device specific handlers & check for SATA->PATA bridges -* @ap: Bus -* @i: Device -* -* LOCKING: -*/ - -void ata_dev_config (struct ata_port *ap,unsigned int i) -{ -/* limit bridge transfers to udma5, 200 sectors */ -if (ata_dev_knobble (ap, &ap->device[i])) { -printk (KERN_INFO "ata%u(%u): applying bridge limits\n" , - ap->id, i); - ap->udma_mask &=ATA_UDMA5 ; - ap->device[i].max_sectors =ATA_MAX_SECTORS ; - } - -if (ap->ops->dev_config) - ap->ops->dev_config (ap, &ap->device[i]); -} -/** * ata_bus_probe - Reset and probe ATA bus * @ap: Bus to probe
@@ -1262,7 +1250,6 @@ static int ata_bus_probe(struct ata_portcontinue ; } -ata_dev_config (ap, i); found =1 ; }
@@ -4963,7 +4950,6 @@ EXPORT_SYMBOL_GPL(ata_host_intr);EXPORT_SYMBOL_GPL (ata_dev_classify);EXPORT_SYMBOL_GPL (ata_id_string);EXPORT_SYMBOL_GPL (ata_id_c_string); -EXPORT_SYMBOL_GPL (ata_dev_config);EXPORT_SYMBOL_GPL (ata_scsi_simulate);EXPORT_SYMBOL_GPL (ata_eh_qc_complete);EXPORT_SYMBOL_GPL (ata_eh_qc_retry);
include/linux/libata.h +0 -1
@@ -542,7 +542,6 @@ extern void ata_id_string(const u16 *id, unsigned int ofs, unsigned int len);extern void ata_id_c_string (const u16 *id,unsigned char *s,unsigned int ofs,unsigned int len); -extern void ata_dev_config (struct ata_port *ap,unsigned int i);extern void ata_bmdma_setup (struct ata_queued_cmd *qc);extern void ata_bmdma_start (struct ata_queued_cmd *qc);extern void ata_bmdma_stop (struct ata_queued_cmd *qc);
Signature · 1 line
1.2.1
PATCH 4/4 libata: reorganize ata_bus_probe()
In reply to Tejun HeoNow that reset and configure are converted such that they don't modify or disable libata core data structures, reorganize ata_bus_probe() to reflect this change.
---
drivers/scsi/libata-core.c | 46 ++++++++++++++++++++++++-------------------- 1 files changed, 25 insertions(+), 21 deletions(-)
fd8657c1907afa3fc13b55ef4c274580d413527a
drivers/scsi/libata-core.c +25 -21
@@ -1207,35 +1207,40 @@ err_out_nosup:static int ata_bus_probe (struct ata_port *ap) { -unsigned int i, found =0 ; +unsigned int classes[ATA_MAX_DEVICES ]; +unsigned int i, rc, found =0 ; -if (ap->ops->probe_reset) { -unsigned int classes[ATA_MAX_DEVICES ]; -int rc; - -ata_port_probe (ap); +ata_port_probe (ap); +/* reset */ +if (ap->ops->probe_reset) { rc = ap->ops->probe_reset (ap, classes); -if (rc ==0 ) { -for (i =0 ; i <ATA_MAX_DEVICES ; i++) { -if (classes[i] ==ATA_DEV_UNKNOWN ) - classes[i] =ATA_DEV_NONE ; - ap->device[i].class = classes[i]; - } - }else { -printk (KERN_ERR "ata%u: probe reset failed, " -"disabling port\n" , ap->id); -ata_port_disable (ap); +if (rc) { +printk ("ata%u: reset failed (errno=%d)\n" , ap->id, rc); +return rc; } - }else + +for (i =0 ; i <ATA_MAX_DEVICES ; i++) +if (classes[i] ==ATA_DEV_UNKNOWN ) + classes[i] =ATA_DEV_NONE ; + }else { ap->ops->phy_reset (ap); -if (ap->flags &ATA_FLAG_PORT_DISABLED ) - goto err_out; +for (i =0 ; i <ATA_MAX_DEVICES ; i++) { +if (!(ap->flags &ATA_FLAG_PORT_DISABLED )) + classes[i] = ap->device[i].class; +else + ap->device[i].class =ATA_DEV_UNKNOWN ; + } +ata_port_probe (ap); + } +/* read IDENTIFY page and configure devices */ for (i =0 ; i <ATA_MAX_DEVICES ; i++) {struct ata_device *dev = &ap->device[i]; + dev->class = classes[i]; +if (!ata_dev_present (dev))continue ;
@@ -1253,7 +1258,7 @@ static int ata_bus_probe(struct ata_port found =1 ; } -if ((!found) || (ap->flags &ATA_FLAG_PORT_DISABLED )) +if (!found) goto err_out_disable;ata_set_mode (ap);
@@ -1264,7 +1269,6 @@ static int ata_bus_probe(struct ata_port err_out_disable: ap->ops->port_disable (ap); -err_out:return -1 ; }
Signature · 1 line
1.2.1