libata: reorganize ata_dev_identify()
PATCH 1/1 libata: reorganize ata_dev_identify()
Hello, Jeff. Hello, Albert.
This patchset is the third part of divided 'reorganize configuration and implement revalidation' patchset[1]. The first[2] and second[3] parts made into #upstream lately.
This patchset is against
the current upstream (254a5c45f8e53775a1e1dd7498a8dcd665341f1e)
+ rename ata_dev_id_[c_]string() patch [4]
Note: Jeff, I generated patches over the rename patch as the patch
make naming not only shorter but also more consistent. If you don't like the rename, I'll regenerate this patchset without it.
This patchset concentrates on reorganizing ata_dev_identify() into ata_dev_read_id() and ata_dev_configure() such that those functions can be also be used for revalidation and other purposes.
Also, both functions don't directly take devices or ports offline. They just report failures to upper layers and determining whether to take devices/ports offline is the responsibility of upper layer driving logic.
Other than failure case handling, reorganized configuration code should be functionality-wise identical to ata_dev_identify().
Thanks.
--
tejun
[1] http://article.gmane.org/gmane.linux.ide/7632
[2] http://article.gmane.org/gmane.linux.ide/7977
[3] http://article.gmane.org/gmane.linux.ide/7981
[4] http://article.gmane.org/gmane.linux.ide/7997
PATCH 2/7 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 | 11 +++++++++-- include/linux/libata.h | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-)
437d923fd2bb8020b4a5cdaa8080bcf9f20698cd
drivers/scsi/libata-core.c +9 -2
@@ -953,6 +953,10 @@ static void ata_dev_identify(struct ata_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 ) + goto err_out; + retry:ata_tf_init (ap, &tf, device);
@@ -967,7 +971,7 @@ retry: tf.protocol =ATA_PROT_PIO ; err_mask =ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , - dev->id,sizeof (dev->id)); + dev->id,sizeof (dev->id[0 ]) *ATA_ID_WORDS );if (err_mask) {if (err_mask & ~AC_ERR_DEV )
@@ -2515,7 +2519,7 @@ static void ata_dev_reread_id(struct ata tf.protocol =ATA_PROT_PIO ;if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , - dev->id,sizeof (dev->id))) + dev->id,sizeof (dev->id[0 ]) *ATA_ID_WORDS )) goto err_out;swap_buf_le16 (dev->id,ATA_ID_WORDS );
@@ -4722,11 +4726,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.1.5
PATCH 4/7 libata: kill ata_dev_reread_id()
In reply to Tejun HeoKill now-unused ata_dev_reread_id().
---
drivers/scsi/libata-core.c | 42 ------------------------------------------ 1 files changed, 0 insertions(+), 42 deletions(-)
4436e50d21e0dcfaa26f54dc798b9d23c4d71d11
drivers/scsi/libata-core.c +0 -42
@@ -61,7 +61,6 @@#include "libata.h" -static void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev);static unsigned int ata_dev_init_params (struct ata_port *ap,struct ata_device *dev);static void ata_set_mode (struct ata_port *ap);
@@ -2545,47 +2544,6 @@ static void ata_dev_set_xfermode(struct }/** -* ata_dev_reread_id - Reread the device identify device info -* @ap: port where the device is -* @dev: device to reread the identify device info -* -* LOCKING: -*/ - -static void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) -{ -struct ata_taskfile tf; - -ata_tf_init (ap, &tf, dev->devno); - -if (dev->class ==ATA_DEV_ATA ) { - tf.command =ATA_CMD_ID_ATA ; -DPRINTK ("do ATA identify\n" ); - }else { - tf.command =ATA_CMD_ID_ATAPI ; -DPRINTK ("do ATAPI identify\n" ); - } - - tf.flags |=ATA_TFLAG_DEVICE ; - tf.protocol =ATA_PROT_PIO ; - -if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , - dev->id,sizeof (dev->id[0 ]) *ATA_ID_WORDS )) - goto err_out; - -swap_buf_le16 (dev->id,ATA_ID_WORDS ); - -ata_dump_id (dev->id); - -DPRINTK ("EXIT\n" ); - -return ; -err_out: -printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id); -ata_port_disable (ap); -} - -/** * ata_dev_init_params - Issue INIT DEV PARAMS command * @ap: Port associated with device @dev * @dev: Device to which command will be sent
Signature · 1 line
1.1.5
Re: [PATCH 4/7] libata: kill ata_dev_reread_id()
Quoted 3 lines · Tejun Heo
Kill now-unused
ata_dev_reread_id().Signed-off-by Tejun Heo <[email protected]>
ACK
PATCH 5/7 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 | 76 ++++++++++++++++++++++---------------------- 1 files changed, 38 insertions(+), 38 deletions(-)
8d98269f37ee0ef92dc4a0822d250acdf75a4cf3
drivers/scsi/libata-core.c +38 -38
@@ -1050,44 +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); - - 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
@@ -1096,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; }
@@ -1120,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 */
@@ -1142,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,
@@ -1158,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)); }
@@ -1175,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; }
@@ -1258,11 +1246,23 @@ 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 ; + +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.1.5
Re: [PATCH 5/7] libata: separate out ata_dev_configure()
Quoted 11 lines · Tejun Heo
Separate out
ata_dev_configure()fromata_dev_identify()such thatata_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 killsata_dev_identify()and inlines the logic intoata_bus_probe().Signed-off-by Tejun Heo <[email protected]>
ACK
PATCH 6/7 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(-)
74bf76a028e19e725d788e332486ce0b2dfcca36
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
@@ -1261,7 +1249,6 @@ static int ata_bus_probe(struct ata_portcontinue ; } -ata_dev_config (ap, i); found =1 ; }
@@ -4959,7 +4946,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.1.5
Re: [PATCH 6/7] libata: fold ata_dev_config() into ata_dev_configure()
Quoted 4 lines · Tejun Heo
ata_dev_config()needs to be done everytime a device is configured. Fold it intoata_dev_configure().Signed-off-by Tejun Heo <[email protected]>
ACK
PATCH 7/7 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(-)
04a9356ddbbfafba7ec9b6b71db30ccad2190e02
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 ;
@@ -1252,7 +1257,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);
@@ -1263,7 +1268,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.1.5
Re: [PATCH 7/7] libata: reorganize ata_bus_probe()
Quoted 56 lines · Tejun Heo
Now 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.Signed-off-by Tejun Heo <[email protected]>---
drivers/scsi/libata-core.c | 46 ++++++++++++++++++++++++-------------------- 1 files changed, 25 insertions(+), 21 deletions(-)
04a9356ddbbfafba7ec9b6b71db30ccad2190e02drivers/scsi/libata-core.c +13
-17@@ -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);
drivers/scsi/libata-core.c err_out_nosup: · line 1236
Quoted 10 lines
drivers/scsi/libata-core.c +8
-2-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); + }
the legacy phy_reset code path appears to call ata_port_probe() a second time. otherwise, seems OK.
Jeff
Re: [PATCH 7/7] libata: reorganize ata_bus_probe()
Quoted 11 lines · Jeff Garzik
Quoted 6 lines · Tejun Heo
Now 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.Signed-off-by Tejun Heo <[email protected]>the legacy
phy_resetcode path appears to callata_port_probe()a second time. otherwise, seems OK.
Hello, Jeff.
That is intentional. ->phy_reset disables port if something goes wrong during reset while the new model just offlines affected devices. The code there emulates new behavior by turning off all devices and enabling the port if the port gets disabled during ->phy_reset.
Signature · 1 line
tejun