libata: fix device iteration bugs
PATCH 1/4 libata: fix device iteration bugs
There were several places where only enabled devices should be iterated over but device enabledness wasn't checked.
* IDENTIFY data 40 wire check in cable_is_40wire()
* xfer_mode/ncq_enabled saving in ata_scsi_error()
* DUBIOUS_XFER handling in ata_set_mode()
While at it, reformat comment in cable_is_40wire().
---
Jeff, this four patch series fixes iteration bugs and improves iterators and fix ata_port_detach() related bugs. The iterator change is a bit big for #upstream-fixes but it's fairly straight forward and I verified them in most combinations, so it should be pretty safe.
Thanks.
drivers/ata/libata-core.c | 18 ++++++++---------- drivers/ata/libata-eh.c | 9 +++++++++ 2 files changed, 17 insertions(+), 10 deletions(-)
drivers/ata/libata-core.c +8 -10
@@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_poif (ap->cbl ==ATA_CBL_PATA40_SHORT )return 0 ;/* If the controller doesn't know we scan - -- Note: We look for all 40 wire detects at this point. -Any 80 wire detect is taken to be 80 wire cable -because -- In many setups only the one drive (slave if present) -will give a valid detect -- If you have a non detect capable drive you don't -want it to colour the choice -*/ +* +* - Note: We look for all 40 wire detects at this point. Any +* 80 wire detect is taken to be 80 wire cable because - In +* many setups only the one drive (slave if present) will +* give a valid detect - If you have a non detect capable +* drive you don't want it to colour the choice +*/ ata_port_for_each_link (link, ap) {ata_link_for_each_dev (dev, link) { -if (!ata_is_40wire (dev)) +if (ata_dev_enabled (dev) && !ata_is_40wire (dev))return 0 ; } }
drivers/ata/libata-eh.c +9 -0
@@ -603,6 +603,9 @@ void ata_scsi_error(struct Scsi_Host *hoata_link_for_each_dev (dev, link) {int devno = dev->devno; +if (!ata_dev_enabled (dev)) +continue ; + ehc->saved_xfer_mode[devno] = dev->xfer_mode;if (ata_ncq_enabled (dev)) ehc->saved_ncq_enabled |=1 << devno;
@@ -2790,6 +2793,9 @@ int ata_set_mode(struct ata_link *link,/* if data transfer is verified, clear DUBIOUS_XFER on ering top */ ata_link_for_each_dev (dev, link) { +if (!ata_dev_enabled (dev)) +continue ; +if (!(dev->flags &ATA_DFLAG_DUBIOUS_XFER )) {struct ata_ering_entry *ent;
@@ -2811,6 +2817,9 @@ int ata_set_mode(struct ata_link *link,u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno)); +if (!ata_dev_enabled (dev)) +continue ; +if (dev->xfer_mode != saved_xfer_mode ||ata_ncq_enabled (dev) != saved_ncq) dev->flags |=ATA_DFLAG_DUBIOUS_XFER ;
PATCH 2/4 libata: beef up iterators
There currently are the following loop constructs.
* __ata_port_for_each_link() for all available links
* ata_port_for_each_link() for edge links
* ata_link_for_each_dev() for all devices
* ata_link_for_each_dev_reverse() for all devices in reverse order
Now there's a need for loop construct which is similar to __ata_port_for_each_link() but iterates over PMP links before the host link. Instead of adding another one with long name, do the following cleanup.
* Implement and export ata_link_next() and ata_dev_next() which take @mode parameter and can be used to build custom loop.
* Implement ata_for_each_link() and ata_for_each_dev() which take looping mode explicitly.
The following iteration modes are implemented.
* ATA_LITER_EDGE : loop over edge links
* ATA_LITER_HOST_FIRST : loop over all links, host link first
* ATA_LITER_PMP_FIRST : loop over all links, PMP links first
* ATA_DITER_ENABLED : loop over enabled devices
* ATA_DITER_ENABLED_REVERSE : loop over enabled devices in reverse order
* ATA_DITER_ALL : loop over all devices
* ATA_DITER_ALL_REVERSE : loop over all devices in reverse order
This change removes exlicit device enabledness checks from many loops and makes it clear which ones are iterated over in which direction.
--- drivers/ata/ahci.c | 8 - drivers/ata/ata_generic.c | 5 - drivers/ata/libata-acpi.c | 19 +--- drivers/ata/libata-core.c | 183 ++++++++++++++++++++++++++++--------------- drivers/ata/libata-eh.c | 84 ++++++++----------- drivers/ata/libata-pmp.c | 22 ++--- drivers/ata/libata-scsi.c | 22 ++--- drivers/ata/pata_it821x.c | 34 +++---- drivers/ata/pata_ixp4xx_cf.c | 14 +-- drivers/ata/pata_legacy.c | 17 +-- drivers/ata/pata_pdc2027x.c | 27 ++---- drivers/ata/pata_platform.c | 14 +-- drivers/ata/pata_rz1000.c | 16 +-- drivers/ata/sata_sil.c | 2 include/linux/libata.h | 50 +++++++---- 15 files changed, 279 insertions(+), 238 deletions(-)
drivers/ata/libata-core.c +122 -61
@@ -163,43 +163,119 @@ MODULE_LICENSE("GPL");MODULE_VERSION (DRV_VERSION ); -/* -* Iterator helpers. Don't use directly. +/** +* ata_link_next - link iteration helper +* @link: the previous link, NULL to start +* @ap: ATA port containing links to iterate +* @mode: iteration mode, one of ATA_LITER_* +* +* LOCKING: +* Host lock or EH context. * -* LOCKING: -* Host lock or EH context. +* RETURNS: +* Pointer to the next link. */ -struct ata_link *__ata_port_next_link (struct ata_port *ap, -struct ata_link *link,bool dev_only) +struct ata_link *ata_link_next (struct ata_link *link,struct ata_port *ap, +enum ata_link_iter_mode mode) { +BUG_ON (mode !=ATA_LITER_EDGE && + mode !=ATA_LITER_PMP_FIRST && mode !=ATA_LITER_HOST_FIRST ); +/* NULL link indicates start of iteration */ -if (!link) { -if (dev_only &&sata_pmp_attached (ap)) -return ap->pmp_link; -return &ap->link; - } +if (!link) +switch (mode) { +case ATA_LITER_EDGE : +case ATA_LITER_PMP_FIRST : +if (sata_pmp_attached (ap)) +return ap->pmp_link; +/* fall through */ +case ATA_LITER_HOST_FIRST : +return &ap->link; + } -/* we just iterated over the host master link, what's next? */ -if (link == &ap->link) { -if (!sata_pmp_attached (ap)) { -if (unlikely (ap->slave_link) && !dev_only) +/* we just iterated over the host link, what's next? */ +if (link == &ap->link) +switch (mode) { +case ATA_LITER_HOST_FIRST : +if (sata_pmp_attached (ap)) +return ap->pmp_link; +/* fall through */ +case ATA_LITER_PMP_FIRST : +if (unlikely (ap->slave_link))return ap->slave_link; +/* fall through */ +case ATA_LITER_EDGE :return NULL ; } -return ap->pmp_link; - }/* slave_link excludes PMP */ if (unlikely (link == ap->slave_link))return NULL ; -/* iterate to the next PMP link */ +/* we were over a PMP link */ if (++link < ap->pmp_link + ap->nr_pmp_links)return link; + +if (mode ==ATA_LITER_PMP_FIRST ) +return &ap->link; +return NULL ; }/** +* ata_dev_next - device iteration helper +* @dev: the previous device, NULL to start +* @link: ATA link containing devices to iterate +* @mode: iteration mode, one of ATA_DITER_* +* +* LOCKING: +* Host lock or EH context. +* +* RETURNS: +* Pointer to the next device. +*/ +struct ata_device *ata_dev_next (struct ata_device *dev,struct ata_link *link, +enum ata_dev_iter_mode mode) +{ +BUG_ON (mode !=ATA_DITER_ENABLED && mode !=ATA_DITER_ENABLED_REVERSE && + mode !=ATA_DITER_ALL && mode !=ATA_DITER_ALL_REVERSE ); + +/* NULL dev indicates start of iteration */ +if (!dev) +switch (mode) { +case ATA_DITER_ENABLED : +case ATA_DITER_ALL : + dev = link->device; + goto check; +case ATA_DITER_ENABLED_REVERSE : +case ATA_DITER_ALL_REVERSE : + dev = link->device +ata_link_max_devices (link) -1 ; + goto check; + } + + next: +/* move to the next one */ +switch (mode) { +case ATA_DITER_ENABLED : +case ATA_DITER_ALL : +if (++dev < link->device +ata_link_max_devices (link)) + goto check; +return NULL ; +case ATA_DITER_ENABLED_REVERSE : +case ATA_DITER_ALL_REVERSE : +if (--dev >= link->device) + goto check; +return NULL ; + } + + check: +if ((mode ==ATA_DITER_ENABLED || mode ==ATA_DITER_ENABLED_REVERSE ) && + !ata_dev_enabled (dev)) + goto next; +return dev; +} + +/** * ata_dev_phys_link - find physical link for a device * @dev: ATA device to look up physical link for *
@@ -1107,8 +1183,8 @@ static void ata_lpm_enable(struct ata_hofor (i =0 ; i < host->n_ports; i++) { ap = host->ports[i]; -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ALL )ata_dev_disable_pm (dev); } }
@@ -2584,11 +2660,11 @@ int ata_bus_probe(struct ata_port *ap)ata_port_probe (ap); -ata_link_for_each_dev (dev, &ap->link) +ata_for_each_dev (dev, &ap->link,ALL ) tries[dev->devno] =ATA_PROBE_MAX_TRIES ; retry: -ata_link_for_each_dev (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ALL ) {/* If we issue an SRST then an ATA drive (not ATAPI) * may change configuration and be in PIO0 timing. If * we do a hard reset (or are coming from power on)
@@ -2610,7 +2686,7 @@ int ata_bus_probe(struct ata_port *ap)/* reset and determine device classes */ ap->ops->phy_reset (ap); -ata_link_for_each_dev (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ALL ) {if (!(ap->flags &ATA_FLAG_DISABLED ) && dev->class !=ATA_DEV_UNKNOWN ) classes[dev->devno] = dev->class;
@@ -2626,7 +2702,7 @@ int ata_bus_probe(struct ata_port *ap)specific sequence bass-ackwards so that PDIAG- is released by the slave device */ -ata_link_for_each_dev_reverse (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ALL_REVERSE ) {if (tries[dev->devno]) dev->class = classes[dev->devno];
@@ -2643,24 +2719,19 @@ int ata_bus_probe(struct ata_port *ap)if (ap->ops->cable_detect) ap->cbl = ap->ops->cable_detect (ap); -/* We may have SATA bridge glue hiding here irrespective of the -reported cable types and sensed types */ -ata_link_for_each_dev (dev, &ap->link) { -if (!ata_dev_enabled (dev)) -continue ; -/* SATA drives indicate we have a bridge. We don't know which -end of the link the bridge is which is a problem */ +/* We may have SATA bridge glue hiding here irrespective of +* the reported cable types and sensed types. When SATA +* drives indicate we have a bridge, we don't know which end +* of the link the bridge is which is a problem. +*/ +ata_for_each_dev (dev, &ap->link,ENABLED )if (ata_id_is_sata (dev->id)) ap->cbl =ATA_CBL_SATA ; - }/* After the identify sequence we can now set up the devices. We do this in the normal order so that the user doesn't get confused */ -ata_link_for_each_dev (dev, &ap->link) { -if (!ata_dev_enabled (dev)) -continue ; - +ata_for_each_dev (dev, &ap->link,ENABLED ) { ap->link.eh_context.i.flags |=ATA_EHI_PRINTINFO ; rc =ata_dev_configure (dev); ap->link.eh_context.i.flags &= ~ATA_EHI_PRINTINFO ;
@@ -2673,9 +2744,8 @@ int ata_bus_probe(struct ata_port *ap)if (rc) goto fail; -ata_link_for_each_dev (dev, &ap->link) -if (ata_dev_enabled (dev)) -return 0 ; +ata_for_each_dev (dev, &ap->link,ENABLED ) +return 0 ;/* no device present, disable port */ ata_port_disable (ap);
@@ -3321,13 +3391,10 @@ int ata_do_set_mode(struct ata_link *linint rc =0 , used_dma =0 , found =0 ;/* step 1: calculate xfer_mask */ -ata_link_for_each_dev (dev, link) { +ata_for_each_dev (dev, link,ENABLED ) {unsigned long pio_mask, dma_mask;unsigned int mode_mask; -if (!ata_dev_enabled (dev)) -continue ; - mode_mask =ATA_DMA_MASK_ATA ;if (dev->class ==ATA_DEV_ATAPI ) mode_mask =ATA_DMA_MASK_ATAPI ;
@@ -3356,10 +3423,7 @@ int ata_do_set_mode(struct ata_link *lin goto out;/* step 2: always set host PIO timings */ -ata_link_for_each_dev (dev, link) { -if (!ata_dev_enabled (dev)) -continue ; - +ata_for_each_dev (dev, link,ENABLED ) {if (dev->pio_mode ==0xff ) {ata_dev_printk (dev,KERN_WARNING ,"no PIO support\n" ); rc = -EINVAL ;
@@ -3373,8 +3437,8 @@ int ata_do_set_mode(struct ata_link *lin }/* step 3: set host DMA timings */ -ata_link_for_each_dev (dev, link) { -if (!ata_dev_enabled (dev) || !ata_dma_enabled (dev)) +ata_for_each_dev (dev, link,ENABLED ) { +if (!ata_dma_enabled (dev))continue ; dev->xfer_mode = dev->dma_mode;
@@ -3384,11 +3448,7 @@ int ata_do_set_mode(struct ata_link *lin }/* step 4: update devices' xfer mode */ -ata_link_for_each_dev (dev, link) { -/* don't update suspended devices' xfer mode */ -if (!ata_dev_enabled (dev)) -continue ; - +ata_for_each_dev (dev, link,ENABLED ) { rc =ata_dev_set_mode (dev);if (rc) goto out;
@@ -4176,9 +4236,9 @@ static int cable_is_40wire(struct ata_po* give a valid detect - If you have a non detect capable * drive you don't want it to colour the choice */ -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev) && !ata_is_40wire (dev)) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) { +if (!ata_is_40wire (dev))return 0 ; } }
@@ -5130,7 +5190,7 @@ static int ata_host_request_pm(struct at } ap->pflags |=ATA_PFLAG_PM_PENDING ; -__ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,HOST_FIRST ) { link->eh_info.action |= action; link->eh_info.flags |= ehi_flags; }
@@ -5975,9 +6035,9 @@ static void ata_port_detach(struct ata_p/* EH is now guaranteed to see UNLOADING - EH context belongs * to us. Restore SControl and disable all existing devices. */ -__ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,HOST_FIRST ) {sata_scr_write (link,SCR_CONTROL , link->saved_scontrol); -ata_link_for_each_dev (dev, link) +ata_for_each_dev (dev, link,ALL )ata_dev_disable (dev); }
@@ -6440,7 +6500,8 @@ EXPORT_SYMBOL_GPL(ata_base_port_ops);EXPORT_SYMBOL_GPL (sata_port_ops);EXPORT_SYMBOL_GPL (ata_dummy_port_ops);EXPORT_SYMBOL_GPL (ata_dummy_port_info); -EXPORT_SYMBOL_GPL (__ata_port_next_link); +EXPORT_SYMBOL_GPL (ata_link_next); +EXPORT_SYMBOL_GPL (ata_dev_next);EXPORT_SYMBOL_GPL (ata_std_bios_param);EXPORT_SYMBOL_GPL (ata_host_init);EXPORT_SYMBOL_GPL (ata_host_alloc);
drivers/ata/libata-eh.c +37 -47
286 lines not on this page · raw message
include/linux/libata.h +30 -20
56 lines not on this page · raw message
drivers/ata/ahci.c +4 -4
@@ -1105,14 +1105,14 @@ static void ahci_start_port(struct ata_p/* turn on LEDs */ if (ap->flags &ATA_FLAG_EM ) { -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) { emp = &pp->em_priv[link->pmp];ahci_transmit_led_message (ap, emp->led_state,4 ); } }if (ap->flags &ATA_FLAG_SW_ACTIVITY ) -ata_port_for_each_link (link, ap) +ata_for_each_link (link, ap,EDGE )ahci_init_sw_activity (link); }
@@ -1344,7 +1344,7 @@ static ssize_t ahci_led_show(struct ata_struct ahci_em_priv *emp;int rc =0 ; -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) { emp = &pp->em_priv[link->pmp]; rc +=sprintf (buf,"%lx\n" , emp->led_state); }
@@ -1924,7 +1924,7 @@ static void ahci_error_intr(struct ata_pu32 serror;/* determine active link */ -ata_port_for_each_link (link, ap) +ata_for_each_link (link, ap,EDGE )if (ata_link_active (link))break ;if (!link)
drivers/ata/ata_generic.c +1 -4
@@ -57,10 +57,7 @@ static int generic_set_mode(struct ata_lif (pdev->vendor ==PCI_VENDOR_ID_CENATEK ) dma_enabled =0xFF ; -ata_link_for_each_dev (dev, link) { -if (!ata_dev_enabled (dev)) -continue ; - +ata_for_each_dev (dev, link,ENABLED ) {/* We don't really care */ dev->pio_mode =XFER_PIO_0 ; dev->dma_mode =XFER_MW_DMA_0 ;
drivers/ata/libata-acpi.c +8 -11
@@ -89,7 +89,7 @@ void ata_acpi_associate_sata_port(struct ap->link.device->acpi_handle =NULL ; -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {acpi_integer adr =SATA_ADR (ap->port_no, link->pmp); link->device->acpi_handle =
@@ -144,8 +144,8 @@ static void ata_acpi_detach_device(strucstruct ata_link *tlink;struct ata_device *tdev; -ata_port_for_each_link (tlink, ap) -ata_link_for_each_dev (tdev, tlink) +ata_for_each_link (tlink, ap,EDGE ) +ata_for_each_dev (tdev, tlink,ALL ) tdev->flags |=ATA_DFLAG_DETACH ; }
@@ -627,12 +627,9 @@ int ata_acpi_cbl_80wire(struct ata_port {struct ata_device *dev; -ata_link_for_each_dev (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ENABLED ) {unsigned long xfer_mask, udma_mask; -if (!ata_dev_enabled (dev)) -continue ; - xfer_mask =ata_acpi_gtm_xfermask (dev, gtm);ata_unpack_xfermask (xfer_mask,NULL ,NULL , &udma_mask);
@@ -932,7 +929,7 @@ void ata_acpi_on_resume(struct ata_port* use values set by _STM. Cache _GTF result and * schedule _GTF. */ -ata_link_for_each_dev (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ALL ) {ata_acpi_clear_gtf (dev);if (ata_dev_enabled (dev) &&ata_dev_get_GTF (dev,NULL ) >=0 )
@@ -943,7 +940,7 @@ void ata_acpi_on_resume(struct ata_port* there's no reason to evaluate IDE _GTF early * without _STM. Clear cache and schedule _GTF. */ -ata_link_for_each_dev (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ALL ) {ata_acpi_clear_gtf (dev);if (ata_dev_enabled (dev)) dev->flags |=ATA_DFLAG_ACPI_PENDING ;
@@ -971,8 +968,8 @@ void ata_acpi_set_state(struct ata_portif (state.event ==PM_EVENT_ON )acpi_bus_set_power (ap->acpi_handle,ACPI_STATE_D0 ); -ata_link_for_each_dev (dev, &ap->link) { -if (dev->acpi_handle &&ata_dev_enabled (dev)) +ata_for_each_dev (dev, &ap->link,ENABLED ) { +if (dev->acpi_handle)acpi_bus_set_power (dev->acpi_handle, state.event ==PM_EVENT_ON ?ACPI_STATE_D0 :ACPI_STATE_D3 );
drivers/ata/libata-pmp.c +11 -11
@@ -321,7 +321,7 @@ static void sata_pmp_quirks(struct ata_pif (vendor ==0x1095 && devid ==0x3726 ) {/* sil3726 quirks */ -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {/* Class code report is unreliable and SRST * times out under certain configurations. */
@@ -336,7 +336,7 @@ static void sata_pmp_quirks(struct ata_p } }else if (vendor ==0x1095 && devid ==0x4723 ) {/* sil4723 quirks */ -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {/* class code report is unreliable */ if (link->pmp <2 ) link->flags |=ATA_LFLAG_ASSUME_ATA ;
@@ -348,7 +348,7 @@ static void sata_pmp_quirks(struct ata_p } }else if (vendor ==0x1095 && devid ==0x4726 ) {/* sil4726 quirks */ -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {/* Class code report is unreliable and SRST * times out under certain configurations. * Config device can be at port 0 or 5 and
@@ -450,7 +450,7 @@ int sata_pmp_attach(struct ata_device *dif (ap->ops->pmp_attach) ap->ops->pmp_attach (ap); -ata_port_for_each_link (tlink, ap) +ata_for_each_link (tlink, ap,EDGE )sata_link_init_spd (tlink);ata_acpi_associate_sata_port (ap);
@@ -487,7 +487,7 @@ static void sata_pmp_detach(struct ata_dif (ap->ops->pmp_detach) ap->ops->pmp_detach (ap); -ata_port_for_each_link (tlink, ap) +ata_for_each_link (tlink, ap,EDGE )ata_eh_detach_dev (tlink->device);spin_lock_irqsave (ap->lock, flags);
@@ -700,7 +700,7 @@ static int sata_pmp_eh_recover_pmp(struc }/* PMP is reset, SErrors cannot be trusted, scan all */ -ata_port_for_each_link (tlink, ap) { +ata_for_each_link (tlink, ap,EDGE ) {struct ata_eh_context *ehc = &tlink->eh_context; ehc->i.probe_mask |=ATA_ALL_DEVICES ;
@@ -768,7 +768,7 @@ static int sata_pmp_eh_handle_disabled_lspin_lock_irqsave (ap->lock, flags); -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {if (!(link->flags &ATA_LFLAG_DISABLED ))continue ;
@@ -852,7 +852,7 @@ static int sata_pmp_eh_recover(struct atint cnt, rc; pmp_tries =ATA_EH_PMP_TRIES ; -ata_port_for_each_link (link, ap) +ata_for_each_link (link, ap,EDGE ) link_tries[link->pmp] =ATA_EH_PMP_LINK_TRIES ; retry:
@@ -861,7 +861,7 @@ static int sata_pmp_eh_recover(struct at rc =ata_eh_recover (ap, ops->prereset, ops->softreset, ops->hardreset, ops->postreset,NULL );if (rc) { -ata_link_for_each_dev (dev, &ap->link) +ata_for_each_dev (dev, &ap->link,ALL )ata_dev_disable (dev);return rc; }
@@ -870,7 +870,7 @@ static int sata_pmp_eh_recover(struct atreturn 0 ;/* new PMP online */ -ata_port_for_each_link (link, ap) +ata_for_each_link (link, ap,EDGE ) link_tries[link->pmp] =ATA_EH_PMP_LINK_TRIES ;/* fall through */
@@ -942,7 +942,7 @@ static int sata_pmp_eh_recover(struct at } cnt =0 ; -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {if (!(gscr_error & (1 << link->pmp)))continue ;
drivers/ata/libata-scsi.c +11 -11
@@ -3228,12 +3228,12 @@ void ata_scsi_scan_host(struct ata_portreturn ; repeat: -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) {struct scsi_device *sdev;int channel =0 , id =0 ; -if (!ata_dev_enabled (dev) || dev->sdev) +if (dev->sdev)continue ;if (ata_is_host_link (link))
@@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port* failure occurred, scan would have failed silently. Check * whether all devices are attached. */ -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev) && !dev->sdev) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) { +if (!dev->sdev) goto exit_loop; } }
@@ -3380,7 +3380,7 @@ static void ata_scsi_handle_link_detach(struct ata_port *ap = link->ap;struct ata_device *dev; -ata_link_for_each_dev (dev, link) { +ata_for_each_dev (dev, link,ALL ) {unsigned long flags;if (!(dev->flags &ATA_DFLAG_DETACHED ))
@@ -3495,7 +3495,7 @@ static int ata_scsi_user_scan(struct Scsif (devno ==SCAN_WILD_CARD ) {struct ata_link *link; -ata_port_for_each_link (link, ap) { +ata_for_each_link (link, ap,EDGE ) {struct ata_eh_info *ehi = &link->eh_info; ehi->probe_mask |=ATA_ALL_DEVICES ; ehi->action |=ATA_EH_RESET ;
@@ -3543,11 +3543,11 @@ void ata_scsi_dev_rescan(struct work_strspin_lock_irqsave (ap->lock, flags); -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) {struct scsi_device *sdev = dev->sdev; -if (!ata_dev_enabled (dev) || !sdev) +if (!sdev)continue ;if (scsi_device_get (sdev))continue ;
drivers/ata/pata_it821x.c +16 -18
@@ -465,24 +465,22 @@ static int it821x_smart_set_mode(struct {struct ata_device *dev; -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev)) { -/* We don't really care */ - dev->pio_mode =XFER_PIO_0 ; - dev->dma_mode =XFER_MW_DMA_0 ; -/* We do need the right mode information for DMA or PIO -and this comes from the current configuration flags */ -if (ata_id_has_dma (dev->id)) { -ata_dev_printk (dev,KERN_INFO ,"configured for DMA\n" ); - dev->xfer_mode =XFER_MW_DMA_0 ; - dev->xfer_shift =ATA_SHIFT_MWDMA ; - dev->flags &= ~ATA_DFLAG_PIO ; - }else { -ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); - dev->xfer_mode =XFER_PIO_0 ; - dev->xfer_shift =ATA_SHIFT_PIO ; - dev->flags |=ATA_DFLAG_PIO ; - } +ata_for_each_dev (dev, link,ENABLED ) { +/* We don't really care */ + dev->pio_mode =XFER_PIO_0 ; + dev->dma_mode =XFER_MW_DMA_0 ; +/* We do need the right mode information for DMA or PIO +and this comes from the current configuration flags */ +if (ata_id_has_dma (dev->id)) { +ata_dev_printk (dev,KERN_INFO ,"configured for DMA\n" ); + dev->xfer_mode =XFER_MW_DMA_0 ; + dev->xfer_shift =ATA_SHIFT_MWDMA ; + dev->flags &= ~ATA_DFLAG_PIO ; + }else { +ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); + dev->xfer_mode =XFER_PIO_0 ; + dev->xfer_shift =ATA_SHIFT_PIO ; + dev->flags |=ATA_DFLAG_PIO ; } }return 0 ;
drivers/ata/pata_ixp4xx_cf.c +6 -8
@@ -30,14 +30,12 @@ static int ixp4xx_set_mode(struct ata_li {struct ata_device *dev; -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev)) { -ata_dev_printk (dev,KERN_INFO ,"configured for PIO0\n" ); - dev->pio_mode =XFER_PIO_0 ; - dev->xfer_mode =XFER_PIO_0 ; - dev->xfer_shift =ATA_SHIFT_PIO ; - dev->flags |=ATA_DFLAG_PIO ; - } +ata_for_each_dev (dev, link,ENABLED ) { +ata_dev_printk (dev,KERN_INFO ,"configured for PIO0\n" ); + dev->pio_mode =XFER_PIO_0 ; + dev->xfer_mode =XFER_PIO_0 ; + dev->xfer_shift =ATA_SHIFT_PIO ; + dev->flags |=ATA_DFLAG_PIO ; }return 0 ; }
drivers/ata/pata_legacy.c +7 -10
@@ -194,15 +194,12 @@ static int legacy_set_mode(struct ata_li {struct ata_device *dev; -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev)) { -ata_dev_printk (dev,KERN_INFO , -"configured for PIO\n" ); - dev->pio_mode =XFER_PIO_0 ; - dev->xfer_mode =XFER_PIO_0 ; - dev->xfer_shift =ATA_SHIFT_PIO ; - dev->flags |=ATA_DFLAG_PIO ; - } +ata_for_each_dev (dev, link,ENABLED ) { +ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); + dev->pio_mode =XFER_PIO_0 ; + dev->xfer_mode =XFER_PIO_0 ; + dev->xfer_shift =ATA_SHIFT_PIO ; + dev->flags |=ATA_DFLAG_PIO ; }return 0 ; }
@@ -1028,7 +1025,7 @@ static __init int legacy_init_one(struct/* Nothing found means we drop the port as its probably not there */ ret = -ENODEV ; -ata_link_for_each_dev (dev, &ap->link) { +ata_for_each_dev (dev, &ap->link,ALL ) {if (!ata_dev_absent (dev)) { legacy_host[probe->slot] = host; ld->platform_dev = pdev;
drivers/ata/pata_pdc2027x.c +12 -15
@@ -406,23 +406,20 @@ static int pdc2027x_set_mode(struct ata_if (rc <0 )return rc; -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev)) { +ata_for_each_dev (dev, link,ENABLED ) { +pdc2027x_set_piomode (ap, dev); -pdc2027x_set_piomode (ap, dev); +/* +* Enable prefetch if the device support PIO only. +*/ +if (dev->xfer_shift ==ATA_SHIFT_PIO ) { +u32 ctcr1 =ioread32 (dev_mmio (ap, dev,PDC_CTCR1 )); + ctcr1 |= (1 <<25 ); +iowrite32 (ctcr1,dev_mmio (ap, dev,PDC_CTCR1 )); -/* -* Enable prefetch if the device support PIO only. -*/ -if (dev->xfer_shift ==ATA_SHIFT_PIO ) { -u32 ctcr1 =ioread32 (dev_mmio (ap, dev,PDC_CTCR1 )); - ctcr1 |= (1 <<25 ); -iowrite32 (ctcr1,dev_mmio (ap, dev,PDC_CTCR1 )); - -PDPRINTK ("Turn on prefetch\n" ); - }else { -pdc2027x_set_dmamode (ap, dev); - } +PDPRINTK ("Turn on prefetch\n" ); + }else { +pdc2027x_set_dmamode (ap, dev); } }return 0 ;
drivers/ata/pata_platform.c +6 -8
@@ -34,14 +34,12 @@ static int pata_platform_set_mode(struct {struct ata_device *dev; -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev)) { -/* We don't really care */ - dev->pio_mode = dev->xfer_mode =XFER_PIO_0 ; - dev->xfer_shift =ATA_SHIFT_PIO ; - dev->flags |=ATA_DFLAG_PIO ; -ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); - } +ata_for_each_dev (dev, link,ENABLED ) { +/* We don't really care */ + dev->pio_mode = dev->xfer_mode =XFER_PIO_0 ; + dev->xfer_shift =ATA_SHIFT_PIO ; + dev->flags |=ATA_DFLAG_PIO ; +ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); }return 0 ; }
drivers/ata/pata_rz1000.c +7 -9
@@ -38,15 +38,13 @@ static int rz1000_set_mode(struct ata_li {struct ata_device *dev; -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev)) { -/* We don't really care */ - dev->pio_mode =XFER_PIO_0 ; - dev->xfer_mode =XFER_PIO_0 ; - dev->xfer_shift =ATA_SHIFT_PIO ; - dev->flags |=ATA_DFLAG_PIO ; -ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); - } +ata_for_each_dev (dev, link,ENABLED ) { +/* We don't really care */ + dev->pio_mode =XFER_PIO_0 ; + dev->xfer_mode =XFER_PIO_0 ; + dev->xfer_shift =ATA_SHIFT_PIO ; + dev->flags |=ATA_DFLAG_PIO ; +ata_dev_printk (dev,KERN_INFO ,"configured for PIO\n" ); }return 0 ; }
drivers/ata/sata_sil.c +1 -1
@@ -278,7 +278,7 @@ static int sil_set_mode(struct ata_linkif (rc)return rc; -ata_link_for_each_dev (dev, link) { +ata_for_each_dev (dev, link,ALL ) {if (!ata_dev_enabled (dev)) dev_mode[dev->devno] =0 ;/* PIO0/1/2 */ else if (dev->flags &ATA_DFLAG_PIO )
PATCH 3/4 libata: when restoring SControl during detach do the PMP links first
When restoring SControl during detach, PMP links should be handled first as changing SControl of the host link can affect SCR access of PMP links.
--- drivers/ata/libata-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ata/libata-core.c +1 -1
@@ -6035,7 +6035,7 @@ static void ata_port_detach(struct ata_p/* EH is now guaranteed to see UNLOADING - EH context belongs * to us. Restore SControl and disable all existing devices. */ -ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_link (link, ap,PMP_FIRST ) {sata_scr_write (link,SCR_CONTROL , link->saved_scontrol);ata_for_each_dev (dev, link,ALL )ata_dev_disable (dev);
PATCH 4/4 libata: perform port detach in EH
ata_port_detach() first made sure EH saw ATA_PFLAG_UNLOADING and then assumed EH context belongs to it and performed detach operation itself. However, UNLOADING doesn't disable all of EH and this could lead to problems including triggering WARN_ON()'s in EH path.
This patch makes port detach behave more like other EH actions such that ata_port_detach() requests EH to detach and waits for completion.
--- drivers/ata/libata-core.c | 23 ++++------------------- drivers/ata/libata-eh.c | 32 +++++++++++++++++++++++++++++++- include/linux/libata.h | 3 ++- 3 files changed, 37 insertions(+), 21 deletions(-)
drivers/ata/libata-eh.c +31 -1
@@ -491,6 +491,31 @@ enum blk_eh_timer_return ata_scsi_timed_return ret; } +static void ata_eh_unload (struct ata_port *ap) +{ +struct ata_link *link; +struct ata_device *dev; +unsigned long flags; + +/* Restore SControl IPM and SPD for the next driver and +* disable attached devices. +*/ +ata_for_each_link (link, ap,PMP_FIRST ) { +sata_scr_write (link,SCR_CONTROL , link->saved_scontrol &0xff0 ); +ata_for_each_dev (dev, link,ALL ) +ata_dev_disable (dev); + } + +/* freeze and set UNLOADED */ +spin_lock_irqsave (ap->lock, flags); + +ata_port_freeze (ap);/* won't be thawed */ + ap->pflags &= ~ATA_PFLAG_EH_PENDING ;/* clear pending from freeze */ + ap->pflags |=ATA_PFLAG_UNLOADED ; + +spin_unlock_irqrestore (ap->lock, flags); +} +/** * ata_scsi_error - SCSI layer error handler callback * @host: SCSI host on which error occurred
@@ -621,8 +646,13 @@ void ata_scsi_error(struct Scsi_Host *ho/* invoke EH, skip if unloading or suspended */ if (!(ap->pflags & (ATA_PFLAG_UNLOADING |ATA_PFLAG_SUSPENDED ))) ap->ops->error_handler (ap); -else +else { +/* if unloading, commence suicide */ +if ((ap->pflags &ATA_PFLAG_UNLOADING ) && + !(ap->pflags &ATA_PFLAG_UNLOADED )) +ata_eh_unload (ap);ata_eh_finish (ap); + }/* process port suspend request */ ata_eh_handle_port_suspend (ap);
include/linux/libata.h +2 -1
@@ -213,10 +213,11 @@ enum {ATA_PFLAG_FROZEN = (1 <<2 ),/* port is frozen */ ATA_PFLAG_RECOVERED = (1 <<3 ),/* recovery action performed */ ATA_PFLAG_LOADING = (1 <<4 ),/* boot/loading probe */ -ATA_PFLAG_UNLOADING = (1 <<5 ),/* module is unloading */ ATA_PFLAG_SCSI_HOTPLUG = (1 <<6 ),/* SCSI hotplug scheduled */ ATA_PFLAG_INITIALIZING = (1 <<7 ),/* being initialized, don't touch */ ATA_PFLAG_RESETTING = (1 <<8 ),/* reset in progress */ +ATA_PFLAG_UNLOADING = (1 <<9 ),/* driver is being unloaded */ +ATA_PFLAG_UNLOADED = (1 <<10 ),/* driver is unloaded */ ATA_PFLAG_SUSPENDED = (1 <<17 ),/* port is suspended (power) */ ATA_PFLAG_PM_PENDING = (1 << 18), /* PM operation pending */
drivers/ata/libata-core.c +4 -19
@@ -6019,8 +6019,6 @@ int ata_host_activate(struct ata_host *hstatic void ata_port_detach (struct ata_port *ap) {unsigned long flags; -struct ata_link *link; -struct ata_device *dev;if (!ap->ops->error_handler) goto skip_eh;
@@ -6028,28 +6026,15 @@ static void ata_port_detach(struct ata_p/* tell EH we're leaving & flush EH */ spin_lock_irqsave (ap->lock, flags); ap->pflags |=ATA_PFLAG_UNLOADING ; +ata_port_schedule_eh (ap);spin_unlock_irqrestore (ap->lock, flags); +/* wait till EH commits suicide */ ata_port_wait_eh (ap); -/* EH is now guaranteed to see UNLOADING - EH context belongs -* to us. Restore SControl and disable all existing devices. -*/ -ata_for_each_link (link, ap,PMP_FIRST ) { -sata_scr_write (link,SCR_CONTROL , link->saved_scontrol); -ata_for_each_dev (dev, link,ALL ) -ata_dev_disable (dev); - } - -/* Final freeze & EH. All in-flight commands are aborted. EH -* will be skipped and retrials will be terminated with bad -* target. -*/ -spin_lock_irqsave (ap->lock, flags); -ata_port_freeze (ap);/* won't be thawed */ -spin_unlock_irqrestore (ap->lock, flags); +/* it better be dead now */ +WARN_ON (!(ap->pflags &ATA_PFLAG_UNLOADED )); -ata_port_wait_eh (ap);cancel_rearming_delayed_work (&ap->hotplug_task); skip_eh:
Re: [PATCH #upstream-fixes 4/4] libata: perform port detach in EH
Quoted 8 lines · Tejun Heo
ata_port_detach()first made sure EH sawATA_PFLAG_UNLOADINGand then assumed EH context belongs to it and performed detach operation itself. However, UNLOADING doesn't disable all of EH and this could lead to problems including triggering WARN_ON()'s in EH path.This patch makes port detach behave more like other EH actions such that
ata_port_detach()requests EH to detach and waits for completion.
Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/libata-core.c | 23 ++++------------------- drivers/ata/libata-eh.c | 32 +++++++++++++++++++++++++++++++- include/linux/libata.h | 3 ++- 3 files changed, 37 insertions(+), 21 deletions(-)
dropped patches 3-4 as well, due to dropping patch #2
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
In reply to Tejun HeoQuoted 33 lines · Tejun Heo
There currently are the following loop constructs.
*
__ata_port_for_each_link()for all available links
*ata_port_for_each_link()for edge links
*ata_link_for_each_dev()for all devices
*ata_link_for_each_dev_reverse()for all devices in reverse orderNow there's a need for loop construct which is similar to
__ata_port_for_each_link()but iterates over PMP links before the host link. Instead of adding another one with long name, do the following cleanup.* Implement and export
ata_link_next()andata_dev_next()which take @mode parameter and can be used to build custom loop.
* Implementata_for_each_link()andata_for_each_dev()which take looping mode explicitly.The following iteration modes are implemented.
*
ATA_LITER_EDGE: loop over edge links
*ATA_LITER_HOST_FIRST: loop over all links, host link first
*ATA_LITER_PMP_FIRST: loop over all links, PMP links first*
ATA_DITER_ENABLED: loop over enabled devices
*ATA_DITER_ENABLED_REVERSE: loop over enabled devices in reverse order
*ATA_DITER_ALL: loop over all devices
*ATA_DITER_ALL_REVERSE: loop over all devices in reverse orderThis change removes exlicit device enabledness checks from many loops and makes it clear which ones are iterated over in which direction.
Signed-off-by Tejun Heo <[email protected]>---
[...]
Quoted 4 lines
Index:
work/drivers/ata/libata-core.c
===================================================================drivers/ata/libata-core.c +0
-0
[...]
drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187
Quoted 8 lines
drivers/ata/libata-core.c +2
-2@@ -1107,8 +1183,8 @@ static void ata_lpm_enable(struct ata_hofor (i =0 ; i < host->n_ports; i++) { ap = host->ports[i]; -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ALL )
Where did these short forms (EDGE, ALL) spring from? Does this code even compile?
[...]
Quoted 4 lines
Index:
work/drivers/ata/libata-eh.c
===================================================================drivers/ata/libata-eh.c +0
-0
[...]
Quoted 13 lines
drivers/ata/libata-eh.c +2
-3@@ -2880,9 +2871,8 @@ static int ata_link_nr_enabled(struct atstruct ata_device *dev;int cnt =0 ; -ata_link_for_each_dev (dev, link) -if (ata_dev_enabled (dev)) - cnt++; +ata_for_each_dev (dev, link,ENABLED ) + cnt++;return cnt; }
drivers/ata/libata-eh.c static int ata_link_nr_vacant(struct ata · line 2887
Quoted 8 lines
drivers/ata/libata-eh.c +1
-1@@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct atastruct ata_device *dev;int cnt =0 ; -ata_link_for_each_dev (dev, link) +ata_for_each_dev (dev, link,ALL )if (dev->class ==ATA_DEV_UNKNOWN ) cnt++;return cnt;
What about making the two above (ata_link_nr_*()) static inline while you are at it? Or is this another one of those cases where the compiler knows best anyway?
[...]
Quoted 4 lines
Index:
work/drivers/ata/libata-scsi.c
===================================================================drivers/ata/libata-scsi.c +0
-0
[...]
drivers/ata/libata-scsi.c void ata_scsi_scan_host(struct ata_port · line 3262
Quoted 13 lines
drivers/ata/libata-scsi.c +3
-3@@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port* failure occurred, scan would have failed silently. Check * whether all devices are attached. */ -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev) && !dev->sdev) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) { +if (!dev->sdev) goto exit_loop; } }
Getting rid of those braces would make things even cleaner in my opinion. (That's my nit picking since Sergei picked on that comment formatting issue shortly before I was going to ;-) ).
Regards,
Elias
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
Hello,
Quoted 7 lines · Elias Oltmanns
drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187
Quoted 4 lines
drivers/ata/libata-core.c +2
-2-ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ALL )Where did these short forms (EDGE, ALL) spring from? Does this code even compile?
+#define ata_for_each_link(link, ap, mode) \
+ for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
+ (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
+
+#define ata_for_each_dev(dev, link, mode) \
+ for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
+ (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
Quoted 4 lines
Quoted 4 lines
drivers/ata/libata-eh.c +0
-0@@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct atastruct ata_device *dev;int cnt =0 ;
Quoted 9 lines
drivers/ata/libata-eh.c static int ata_link_nr_vacant(struct ata · line 2887
Quoted 5 lines
drivers/ata/libata-eh.c +1
-1-ata_link_for_each_dev (dev, link) +ata_for_each_dev (dev, link,ALL )if (dev->class ==ATA_DEV_UNKNOWN ) cnt++;return cnt;What about making the two above (ata_link_nr_*()) static inline while you are at it? Or is this another one of those cases where the compiler knows best anyway?
Yeah, it's a static function used only in single place. Should be pretty straight forward for the compiler and even if it fails to notice it, it doesn't hurt at all.
Quoted 15 lines
[...]
Quoted 4 lines
Index:
work/drivers/ata/libata-scsi.c
===================================================================drivers/ata/libata-scsi.c +0
-0[...]
Quoted 9 lines
drivers/ata/libata-scsi.c +2
-3@@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port* failure occurred, scan would have failed silently. Check * whether all devices are attached. */ -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev) && !dev->sdev) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) {
Quoted 8 lines
drivers/ata/libata-scsi.c void ata_scsi_scan_host(struct ata_port · line 3262
Quoted 4 lines
drivers/ata/libata-scsi.c +1
-0+if (!dev->sdev) goto exit_loop; } }Getting rid of those braces would make things even cleaner in my opinion. (That's my nit picking since Sergei picked on that comment formatting issue shortly before I was going to ;-) ).
Yeah, maybe. I don't know. The reason why I added braces there in the first place was because emacs has some problem dealing with nested non-standard looping constructs.
Thanks.
Signature · 1 line
tejun
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
Quoted 11 lines · Tejun Heo
Hello,
Quoted 7 lines · Elias Oltmanns
drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187
Quoted 4 lines
drivers/ata/libata-core.c +2
-2-ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ALL )Where did these short forms (EDGE, ALL) spring from? Does this code even compile?
include/linux/libata.h static inline int ata_link_active(struct · line 1313
Quoted 7 lines
include/linux/libata.h +7
-0+#define ata_for_each_link (link, ap, mode) \ + for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \ + (link) = ata_link_next((link), (ap), ATA_LITER_##mode)) + +#define ata_for_each_dev (dev, link, mode) \ + for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \ + (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
Sorry, I should have been more explicit. I was referring to EDGE and ALL as opposed to ATA_LITER_EDGE and ATA_DITER_ALL. Unless I've missed something, the former aren't defined anywhere in your patch.
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
drivers/ata/libata-eh.c +0
-0@@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct atastruct ata_device *dev;int cnt =0 ;Quoted 9 lines
drivers/ata/libata-eh.c static int ata_link_nr_vacant(struct ata · line 2887
Quoted 5 lines
drivers/ata/libata-eh.c +1
-1-ata_link_for_each_dev (dev, link) +ata_for_each_dev (dev, link,ALL )if (dev->class ==ATA_DEV_UNKNOWN ) cnt++;return cnt;What about making the two above (ata_link_nr_*()) static inline while you are at it? Or is this another one of those cases where the compiler knows best anyway?
Quoted 3 lines
Yeah, it's a static function used only in single place. Should be pretty straight forward for the compiler and even if it fails to notice it, it doesn't hurt at all.
I should have guessed, I suppose. Probably just was too lazy to check, sorry.
Quoted 25 lines
Quoted 16 lines
[...]
Quoted 4 lines
Index:
work/drivers/ata/libata-scsi.c
===================================================================drivers/ata/libata-scsi.c +0
-0[...]
Quoted 9 lines
drivers/ata/libata-scsi.c +2
-3@@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port* failure occurred, scan would have failed silently. Check * whether all devices are attached. */ -ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) { -if (ata_dev_enabled (dev) && !dev->sdev) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ENABLED ) {Quoted 8 lines
drivers/ata/libata-scsi.c void ata_scsi_scan_host(struct ata_port · line 3262
Quoted 4 lines
drivers/ata/libata-scsi.c +1
-0+if (!dev->sdev) goto exit_loop; } }Getting rid of those braces would make things even cleaner in my opinion. (That's my nit picking since Sergei picked on that comment formatting issue shortly before I was going to ;-) ).
Quoted 3 lines
Yeah, maybe. I don't know. The reason why I added braces there in the first place was because emacs has some problem dealing with nested non-standard looping constructs.
Right, I seem to remember having trouble with that myself. So I'm all in favour of keeping them after all ;-).
Regards,
Elias
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
Quoted 10 lines · Elias Oltmanns
Quoted 9 lines · Tejun Heo
Hello,
Quoted 6 lines · Elias Oltmanns
drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187
Quoted 4 lines
drivers/ata/libata-core.c +2
-2-ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ALL )Where did these short forms (EDGE, ALL) spring from? Does this code even compile?
Quoted 11 lines
include/linux/libata.h static inline int ata_link_active(struct · line 1313
Quoted 7 lines
include/linux/libata.h +7
-0+#define ata_for_each_link (link, ap, mode) \ + for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \ + (link) = ata_link_next((link), (ap), ATA_LITER_##mode)) + +#define ata_for_each_dev (dev, link, mode) \ + for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \ + (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))Sorry, I should have been more explicit. I was referring to EDGE and ALL as opposed to
ATA_LITER_EDGEandATA_DITER_ALL. Unless I've missed something, the former aren't defined anywhere in your patch.
I seriously can't be more explicit. Please take a shower and read my reply again, especially, the "ATA_[LD]ITER_##mode" part. :-)
Signature · 1 line
tejun
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
Quoted 24 lines · Tejun Heo
Quoted 11 lines · Elias Oltmanns
Quoted 9 lines · Tejun Heo
Hello,
Quoted 6 lines · Elias Oltmanns
drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187
Quoted 4 lines
drivers/ata/libata-core.c +2
-2-ata_port_for_each_link (link, ap) { -ata_link_for_each_dev (dev, link) +ata_for_each_link (link, ap,EDGE ) { +ata_for_each_dev (dev, link,ALL )Where did these short forms (EDGE, ALL) spring from? Does this code even compile?
Quoted 11 lines
include/linux/libata.h static inline int ata_link_active(struct · line 1313
Quoted 7 lines
include/linux/libata.h +7
-0+#define ata_for_each_link (link, ap, mode) \ + for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \ + (link) = ata_link_next((link), (ap), ATA_LITER_##mode)) + +#define ata_for_each_dev (dev, link, mode) \ + for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \ + (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))Sorry, I should have been more explicit. I was referring to EDGE and ALL as opposed to
ATA_LITER_EDGEandATA_DITER_ALL. Unless I've missed something, the former aren't defined anywhere in your patch.
Quoted 2 lines
I seriously can't be more explicit. Please take a shower and read my reply again, especially, the "ATA_[LD]ITER_##mode" part. :-)
Oh dear, I really need to drop that idea that a macro does what I think it does. Sorry for the noise.
Regards,
Elias
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
In reply to Tejun HeoQuoted 41 lines · Tejun Heo
There currently are the following loop constructs.
*
__ata_port_for_each_link()for all available links
*ata_port_for_each_link()for edge links
*ata_link_for_each_dev()for all devices
*ata_link_for_each_dev_reverse()for all devices in reverse orderNow there's a need for loop construct which is similar to
__ata_port_for_each_link()but iterates over PMP links before the host link. Instead of adding another one with long name, do the following cleanup.* Implement and export
ata_link_next()andata_dev_next()which take @mode parameter and can be used to build custom loop.
* Implementata_for_each_link()andata_for_each_dev()which take looping mode explicitly.The following iteration modes are implemented.
*
ATA_LITER_EDGE: loop over edge links
*ATA_LITER_HOST_FIRST: loop over all links, host link first
*ATA_LITER_PMP_FIRST: loop over all links, PMP links first*
ATA_DITER_ENABLED: loop over enabled devices
*ATA_DITER_ENABLED_REVERSE: loop over enabled devices in reverse order
*ATA_DITER_ALL: loop over all devices
*ATA_DITER_ALL_REVERSE: loop over all devices in reverse orderThis change removes exlicit device enabledness checks from many loops and makes it clear which ones are iterated over in which direction.
Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/ahci.c | 8 - drivers/ata/ata_generic.c | 5 - drivers/ata/libata-acpi.c | 19 +--- drivers/ata/libata-core.c | 183 ++++++++++++++++++++++++++++--------------- drivers/ata/libata-eh.c | 84 ++++++++----------- drivers/ata/libata-pmp.c | 22 ++--- drivers/ata/libata-scsi.c | 22 ++--- drivers/ata/pata_it821x.c | 34 +++----
Quoted 8 lines
drivers/ata/pata_ixp4xx_cf.c | 14 +-- drivers/ata/pata_legacy.c | 17 +-- drivers/ata/pata_pdc2027x.c | 27 ++---- drivers/ata/pata_platform.c | 14 +-- drivers/ata/pata_rz1000.c | 16 +-- drivers/ata/sata_sil.c | 2 include/linux/libata.h | 50 +++++++---- 15 files changed, 279 insertions(+), 238 deletions(-)
ACK... for 2.6.29. I think this is too much change for -rc2. It's a good idea though.
Also, it could use more documentation than just the commit message... something that programmers can easily refer to (header file comment?)
Jeff
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
Quoted 52 lines · Jeff Garzik
Quoted 42 lines · Tejun Heo
There currently are the following loop constructs.
*
__ata_port_for_each_link()for all available links
*ata_port_for_each_link()for edge links
*ata_link_for_each_dev()for all devices
*ata_link_for_each_dev_reverse()for all devices in reverse orderNow there's a need for loop construct which is similar to
__ata_port_for_each_link()but iterates over PMP links before the host link. Instead of adding another one with long name, do the following cleanup.* Implement and export
ata_link_next()andata_dev_next()which take @mode parameter and can be used to build custom loop.
* Implementata_for_each_link()andata_for_each_dev()which take looping mode explicitly.The following iteration modes are implemented.
*
ATA_LITER_EDGE: loop over edge links
*ATA_LITER_HOST_FIRST: loop over all links, host link first
*ATA_LITER_PMP_FIRST: loop over all links, PMP links first*
ATA_DITER_ENABLED: loop over enabled devices
*ATA_DITER_ENABLED_REVERSE: loop over enabled devices in reverse order
*ATA_DITER_ALL: loop over all devices
*ATA_DITER_ALL_REVERSE: loop over all devices in reverse orderThis change removes exlicit device enabledness checks from many loops and makes it clear which ones are iterated over in which direction.
Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/ahci.c | 8 - drivers/ata/ata_generic.c | 5 - drivers/ata/libata-acpi.c | 19 +--- drivers/ata/libata-core.c | 183 ++++++++++++++++++++++++++++--------------- drivers/ata/libata-eh.c | 84 ++++++++----------- drivers/ata/libata-pmp.c | 22 ++--- drivers/ata/libata-scsi.c | 22 ++--- drivers/ata/pata_it821x.c | 34 +++----Quoted 8 lines
drivers/ata/pata_ixp4xx_cf.c | 14 +-- drivers/ata/pata_legacy.c | 17 +-- drivers/ata/pata_pdc2027x.c | 27 ++---- drivers/ata/pata_platform.c | 14 +-- drivers/ata/pata_rz1000.c | 16 +-- drivers/ata/sata_sil.c | 2 include/linux/libata.h | 50 +++++++---- 15 files changed, 279 insertions(+), 238 deletions(-)
Quoted 3 lines
ACK... for 2.6.29. I think this is too much change for -rc2. It's a good idea though.
Also, it could use more documentation than just the commit message... something that programmers can easily refer to (header file comment?)
..
Yeah. Especially a reminder of what "LITER" and "DITER" stand for! :)
Cheers
Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators
Quoted 53 lines · Mark Lord
Quoted 52 lines · Jeff Garzik
Quoted 50 lines · Tejun Heo
There currently are the following loop constructs.
*
__ata_port_for_each_link()for all available links
*ata_port_for_each_link()for edge links
*ata_link_for_each_dev()for all devices
*ata_link_for_each_dev_reverse()for all devices in reverse orderNow there's a need for loop construct which is similar to
__ata_port_for_each_link()but iterates over PMP links before the host link. Instead of adding another one with long name, do the following cleanup.* Implement and export
ata_link_next()andata_dev_next()which take @mode parameter and can be used to build custom loop.
* Implementata_for_each_link()andata_for_each_dev()which take looping mode explicitly.The following iteration modes are implemented.
*
ATA_LITER_EDGE: loop over edge links
*ATA_LITER_HOST_FIRST: loop over all links, host link first
*ATA_LITER_PMP_FIRST: loop over all links, PMP links first*
ATA_DITER_ENABLED: loop over enabled devices
*ATA_DITER_ENABLED_REVERSE: loop over enabled devices in reverse order
*ATA_DITER_ALL: loop over all devices
*ATA_DITER_ALL_REVERSE: loop over all devices in reverse orderThis change removes exlicit device enabledness checks from many loops and makes it clear which ones are iterated over in which direction.
Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/ahci.c | 8 - drivers/ata/ata_generic.c | 5 - drivers/ata/libata-acpi.c | 19 +--- drivers/ata/libata-core.c | 183 ++++++++++++++++++++++++++++--------------- drivers/ata/libata-eh.c | 84 ++++++++----------- drivers/ata/libata-pmp.c | 22 ++--- drivers/ata/libata-scsi.c | 22 ++--- drivers/ata/pata_it821x.c | 34 +++---- drivers/ata/pata_ixp4xx_cf.c | 14 +-- drivers/ata/pata_legacy.c | 17 +-- drivers/ata/pata_pdc2027x.c | 27 ++---- drivers/ata/pata_platform.c | 14 +-- drivers/ata/pata_rz1000.c | 16 +-- drivers/ata/sata_sil.c | 2 include/linux/libata.h | 50 +++++++---- 15 files changed, 279 insertions(+), 238 deletions(-)
Quoted 6 lines
Quoted 3 lines
ACK... for 2.6.29. I think this is too much change for -rc2. It's a good idea though.
Also, it could use more documentation than just the commit message... something that programmers can easily refer to (header file comment?)
..
Yeah. Especially a reminder of what "LITER" and "DITER" stand for! :)
Well, DITER stands for device iteration, and LITER stands for an amount of petrol... <grin>
Jeff
Hello.
Quoted 11 lines · Tejun Heo
There were several places where only enabled devices should be iterated over but device enabledness wasn't checked.
* IDENTIFY data 40 wire check in
cable_is_40wire()
* xfer_mode/ncq_enabled saving inata_scsi_error()
*DUBIOUS_XFERhandling inata_set_mode()While at it, reformat comment in
cable_is_40wire().Signed-off-by Tejun Heo <[email protected]>
More nitpicking... :-)
Quoted 15 lines
Index:
work/drivers/ata/libata-core.c
===================================================================drivers/ata/libata-core.c +0
-7@@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_poif (ap->cbl ==ATA_CBL_PATA40_SHORT )return 0 ;/* If the controller doesn't know we scan - -- Note: We look for all 40 wire detects at this point. -Any 80 wire detect is taken to be 80 wire cable -because -- In many setups only the one drive (slave if present) -will give a valid detect -- If you have a non detect capable drive you don't
drivers/ata/libata-core.c static int cable_is_40wire(struct ata_po · line 4179
Quoted 10 lines
drivers/ata/libata-core.c +7
-2-want it to colour the choice -*/ +* +* - Note: We look for all 40 wire detects at this point. Any +* 80 wire detect is taken to be 80 wire cable because - In +* many setups only the one drive (slave if present) will +* give a valid detect - If you have a non detect capable +* drive you don't want it to colour the choice +*/
The comment formatting now is worse than it was... I'd drop hyphen
before "Note:" and align the bullets under this note (also doing s/If/if/).
MBR, Sergei
Quoted 38 lines · Sergei Shtylyov
Hello.
Quoted 11 lines · Tejun Heo
There were several places where only enabled devices should be iterated over but device enabledness wasn't checked.
* IDENTIFY data 40 wire check in
cable_is_40wire()
* xfer_mode/ncq_enabled saving inata_scsi_error()
*DUBIOUS_XFERhandling inata_set_mode()While at it, reformat comment in
cable_is_40wire().Signed-off-by Tejun Heo <[email protected]>More nitpicking... :-)
Quoted 20 lines
Index:
work/drivers/ata/libata-core.c
===================================================================drivers/ata/libata-core.c +3
-9@@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_poif (ap->cbl ==ATA_CBL_PATA40_SHORT )return 0 ;/* If the controller doesn't know we scan - -- Note: We look for all 40 wire detects at this point. -Any 80 wire detect is taken to be 80 wire cable -because -- In many setups only the one drive (slave if present) -will give a valid detect -- If you have a non detect capable drive you don't -want it to colour the choice -*/ +* +* - Note: We look for all 40 wire detects at this point. Any +* 80 wire detect is taken to be 80 wire cable because - In
Quoted 8 lines
drivers/ata/libata-core.c static int cable_is_40wire(struct ata_po
Quoted 5 lines
drivers/ata/libata-core.c +4
-0+* many setups only the one drive (slave if present) will +* give a valid detect - If you have a non detect capable +* drive you don't want it to colour the choice +*/ The comment formatting now is worse than it was... I'd drop hyphen
before "Note:" and align the bullets under this note (also doing s/If/if/).
Yeah, somehow I missed those bullets altogether. Will regenerate the patch. Thanks.
Signature · 1 line
tejun
PATCH 1/4 libata: fix device iteration bugs
There were several places where only enabled devices should be iterated over but device enabledness wasn't checked.
* IDENTIFY data 40 wire check in cable_is_40wire()
* xfer_mode/ncq_enabled saving in ata_scsi_error()
* DUBIOUS_XFER handling in ata_set_mode()
While at it, reformat comments in cable_is_40wire().
---
Comments reformatted as per Sergei's comment.
drivers/ata/libata-core.c | 32 ++++++++++++++++++-------------- drivers/ata/libata-eh.c | 9 +++++++++ 2 files changed, 27 insertions(+), 14 deletions(-)
drivers/ata/libata-core.c +18 -14
@@ -4158,29 +4158,33 @@ static int cable_is_40wire(struct ata_postruct ata_link *link;struct ata_device *dev; -/* If the controller thinks we are 40 wire, we are */ +/* If the controller thinks we are 40 wire, we are. */ if (ap->cbl ==ATA_CBL_PATA40 )return 1 ; -/* If the controller thinks we are 80 wire, we are */ + +/* If the controller thinks we are 80 wire, we are. */ if (ap->cbl ==ATA_CBL_PATA80 || ap->cbl ==ATA_CBL_SATA )return 0 ; -/* If the system is known to be 40 wire short cable (eg laptop), -then we allow 80 wire modes even if the drive isn't sure */ + +/* If the system is known to be 40 wire short cable (eg +* laptop), then we allow 80 wire modes even if the drive +* isn't sure. +*/ if (ap->cbl ==ATA_CBL_PATA40_SHORT )return 0 ; -/* If the controller doesn't know we scan -- Note: We look for all 40 wire detects at this point. -Any 80 wire detect is taken to be 80 wire cable -because -- In many setups only the one drive (slave if present) -will give a valid detect -- If you have a non detect capable drive you don't -want it to colour the choice -*/ +/* If the controller doesn't know, we scan. +* +* Note: We look for all 40 wire detects at this point. Any +* 80 wire detect is taken to be 80 wire cable because +* - in many setups only the one drive (slave if present) will +* give a valid detect +* - if you have a non detect capable drive you don't want it +* to colour the choice +*/ ata_port_for_each_link (link, ap) {ata_link_for_each_dev (dev, link) { -if (!ata_is_40wire (dev)) +if (ata_dev_enabled (dev) && !ata_is_40wire (dev))return 0 ; } }
drivers/ata/libata-eh.c +9 -0
@@ -603,6 +603,9 @@ void ata_scsi_error(struct Scsi_Host *hoata_link_for_each_dev (dev, link) {int devno = dev->devno; +if (!ata_dev_enabled (dev)) +continue ; + ehc->saved_xfer_mode[devno] = dev->xfer_mode;if (ata_ncq_enabled (dev)) ehc->saved_ncq_enabled |=1 << devno;
@@ -2790,6 +2793,9 @@ int ata_set_mode(struct ata_link *link,/* if data transfer is verified, clear DUBIOUS_XFER on ering top */ ata_link_for_each_dev (dev, link) { +if (!ata_dev_enabled (dev)) +continue ; +if (!(dev->flags &ATA_DFLAG_DUBIOUS_XFER )) {struct ata_ering_entry *ent;
@@ -2811,6 +2817,9 @@ int ata_set_mode(struct ata_link *link,u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno)); +if (!ata_dev_enabled (dev)) +continue ; +if (dev->xfer_mode != saved_xfer_mode ||ata_ncq_enabled (dev) != saved_ncq) dev->flags |=ATA_DFLAG_DUBIOUS_XFER ;
Jeff Garzik
AppliedQuoted 9 lines · Tejun Heo
There were several places where only enabled devices should be iterated over but device enabledness wasn't checked.
* IDENTIFY data 40 wire check in
cable_is_40wire()
* xfer_mode/ncq_enabled saving inata_scsi_error()
*DUBIOUS_XFERhandling inata_set_mode()While at it, reformat comments in
cable_is_40wire().
Quoted 7 lines
Signed-off-by Tejun Heo <[email protected]>---
Comments reformatted as per Sergei's comment.drivers/ata/libata-core.c | 32 ++++++++++++++++++-------------- drivers/ata/libata-eh.c | 9 +++++++++ 2 files changed, 27 insertions(+), 14 deletions(-)
applied