From 7ebd8c5acad5f8ca41f37b36dc62570e1fa13d8b Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Thu, 18 Jul 2024 16:59:06 +0900 Subject: ata: libata: Use QUIRK instead of HORKAGE According to Wiktionary, the verb "hork" is computing slang defined as "To foul up; to be occupied with difficulty, tangle, or unpleasantness; to be broken" (https://en.wiktionary.org/wiki/hork#Verb). libata uses this with the term "horkage" to refer to broken device features. Given that this term is not widely used and its meaning unknown to many, rename it to the more commonly used term "quirk", similar to many other places in the kernel. The renaming done is: 1) Rename all ATA_HORKAGE_XXX flags to ATA_QUIRK_XXX 2) Rename struct ata_device horkage field to quirks 3) Rename struct ata_blacklist_entry to struct ata_dev_quirks_entry. The array of these structures defining quirks for known devices is renamed __ata_dev_quirks. 4) The functions ata_dev_blacklisted() and ata_force_horkage() are renamed to ata_dev_quirks() and ata_force_quirks() respectively. 5) All the force_horkage_xxx() macros are renamed to force_quirk_xxx() And while at it, make sure that the type "unsigned int" is used consistantly for quirk flags variables and data structure fields. Signed-off-by: Damien Le Moal Reviewed-by: Niklas Cassel Reviewed-by: Igor Pylypiv --- drivers/ata/libata-scsi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'drivers/ata/libata-scsi.c') diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index d6f5e25e1ed8..3a442f564b0d 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2083,7 +2083,7 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf) if (ata_id_has_trim(args->id)) { u64 max_blocks = 65535 * ATA_MAX_TRIM_RNUM; - if (dev->horkage & ATA_HORKAGE_MAX_TRIM_128M) + if (dev->quirks & ATA_QUIRK_MAX_TRIM_128M) max_blocks = 128 << (20 - SECTOR_SHIFT); put_unaligned_be64(max_blocks, &rbuf[36]); @@ -2561,11 +2561,11 @@ static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf) rbuf[15] = lowest_aligned; if (ata_id_has_trim(args->id) && - !(dev->horkage & ATA_HORKAGE_NOTRIM)) { + !(dev->quirks & ATA_QUIRK_NOTRIM)) { rbuf[14] |= 0x80; /* LBPME */ if (ata_id_has_zero_after_trim(args->id) && - dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM) { + dev->quirks & ATA_QUIRK_ZERO_AFTER_TRIM) { ata_dev_info(dev, "Enabling discard_zeroes_data\n"); rbuf[14] |= 0x40; /* LBPRZ */ } @@ -3229,8 +3229,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) } scsi_16_lba_len(cdb, &block, &n_block); - if (!unmap || - (dev->horkage & ATA_HORKAGE_NOTRIM) || + if (!unmap || (dev->quirks & ATA_QUIRK_NOTRIM) || !ata_id_has_trim(dev->id)) { fp = 1; bp = 3; -- cgit v1.2.3 From a16951510fae8fa9b934673404c4fc990d124ccd Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Thu, 5 Sep 2024 11:00:59 +0900 Subject: ata: libata-scsi: Improve ata_scsi_handle_link_detach() A struct ata_device flags should always be set and cleared with the device port locked. Testing for a flag should thus also be done while holding the device port lock. In accordance to this principle, modify ata_scsi_handle_link_detach() to test and clear the ATA_DFLAG_DETACHED flag while holding the device port lock. Signed-off-by: Damien Le Moal Reviewed-by: Niklas Cassel --- drivers/ata/libata-scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'drivers/ata/libata-scsi.c') diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 3a442f564b0d..6bd7ab27fcbb 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -4605,10 +4605,12 @@ static void ata_scsi_handle_link_detach(struct ata_link *link) ata_for_each_dev(dev, link, ALL) { unsigned long flags; - if (!(dev->flags & ATA_DFLAG_DETACHED)) + spin_lock_irqsave(ap->lock, flags); + if (!(dev->flags & ATA_DFLAG_DETACHED)) { + spin_unlock_irqrestore(ap->lock, flags); continue; + } - spin_lock_irqsave(ap->lock, flags); dev->flags &= ~ATA_DFLAG_DETACHED; spin_unlock_irqrestore(ap->lock, flags); -- cgit v1.2.3 From 5f8319c4b3ec4e8fdc7f7bf61f47f985e1a6f074 Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Thu, 5 Sep 2024 10:51:19 +0900 Subject: ata: libata: Introduce ata_dev_free_resources Introduce the function ata_dev_free_resources() to free the resources allocated to support a device features. For now, this function is reduced to calling zpodd_exit() for devices that have this feature enabled. ata_dev_free_resources() is called from ata_eh_dev_disable() as this function is always called for all devices attached to a port that is being detached and for devices that are being disabled due to being removed (detached) from the system or due to errors. With this change, the call to zpodd_exit() done in ata_port_detach() and ata_scsi_handle_link_detach() are removed as these functions remove all devices attached to the link or port using libata EH, thus resulting in ata_eh_dev_disable() being called and the zpodd_exit() function being executed. Signed-off-by: Damien Le Moal Reviewed-by: Niklas Cassel --- drivers/ata/libata-core.c | 27 +++++++++++++++++++-------- drivers/ata/libata-eh.c | 5 ++++- drivers/ata/libata-scsi.c | 3 --- drivers/ata/libata.h | 1 + 4 files changed, 24 insertions(+), 12 deletions(-) (limited to 'drivers/ata/libata-scsi.c') diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 32325a1c07af..bfd452b0d46d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5976,6 +5976,21 @@ int ata_host_activate(struct ata_host *host, int irq, } EXPORT_SYMBOL_GPL(ata_host_activate); +/** + * ata_dev_free_resources - Free a device resources + * @dev: Target ATA device + * + * Free resources allocated to support a device features. + * + * LOCKING: + * Kernel thread context (may sleep). + */ +void ata_dev_free_resources(struct ata_device *dev) +{ + if (zpodd_dev_enabled(dev)) + zpodd_exit(dev); +} + /** * ata_port_detach - Detach ATA port in preparation of device removal * @ap: ATA port to be detached @@ -6030,19 +6045,15 @@ static void ata_port_detach(struct ata_port *ap) cancel_delayed_work_sync(&ap->hotplug_task); cancel_delayed_work_sync(&ap->scsi_rescan_task); - /* clean up zpodd on port removal */ - ata_for_each_link(link, ap, HOST_FIRST) { - ata_for_each_dev(dev, link, ALL) { - if (zpodd_dev_enabled(dev)) - zpodd_exit(dev); - } - } + /* Delete port multiplier link transport devices */ if (ap->pmp_link) { int i; + for (i = 0; i < SATA_PMP_MAX_PORTS; i++) ata_tlink_delete(&ap->pmp_link[i]); } - /* remove the associated SCSI host */ + + /* Remove the associated SCSI host */ scsi_remove_host(ap->scsi_host); ata_tport_delete(ap); } diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index ed535e1b4225..364828b8a22d 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -500,10 +500,13 @@ static void ata_eh_dev_disable(struct ata_device *dev) ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO0 | ATA_DNXFER_QUIET); dev->class++; - /* From now till the next successful probe, ering is used to + /* + * From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info. */ ata_ering_clear(&dev->ering); + + ata_dev_free_resources(dev); } static void ata_eh_unload(struct ata_port *ap) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 6bd7ab27fcbb..4fb45565c142 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -4614,9 +4614,6 @@ static void ata_scsi_handle_link_detach(struct ata_link *link) dev->flags &= ~ATA_DFLAG_DETACHED; spin_unlock_irqrestore(ap->lock, flags); - if (zpodd_dev_enabled(dev)) - zpodd_exit(dev); - ata_scsi_remove_dev(dev); } } diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 2a9d1bbf2482..927d77bde7ef 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -71,6 +71,7 @@ extern bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, bool set_active); extern void ata_dev_power_set_standby(struct ata_device *dev); extern void ata_dev_power_set_active(struct ata_device *dev); +void ata_dev_free_resources(struct ata_device *dev); extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel); extern unsigned int ata_dev_set_feature(struct ata_device *dev, u8 subcmd, u8 action); -- cgit v1.2.3 From 602bcf212637633d537ee74bf39c6bc5722efb9b Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Wed, 17 Jul 2024 17:55:31 +0900 Subject: ata: libata: Improve CDL resource management The ncq_sense_buf buffer field of struct ata_port is allocated and used only for devices that support the Command Duration Limits (CDL) feature. However, the cdl buffer of struct ata_device, which is used to cache the command duration limits log page for devices supporting CDL is always allocated as part of struct ata_device, which is wasteful of memory for devices that do not support this feature. Clean this up by defining both buffers as part of the new ata_cdl structure and allocating this structure only for devices that support the CDL feature. This new structure is attached to struct ata_device using the cdl pointer. The functions ata_dev_init_cdl_resources() and ata_dev_cleanup_cdl_resources() are defined to manage this new structure allocation, initialization and freeing when a port is removed or a device disabled. ata_dev_init_cdl_resources() is called from ata_dev_config_cdl() only for devices that support CDL. ata_dev_cleanup_cdl_resources() is called from ata_dev_free_resources() to free the ata_cdl structure when a device is being disabled by EH. Note that the name of the former cdl log buffer of struct ata_device is changed to desc_log_buf to make it clearer that it is a buffer for the limit descriptors log page. This change reduces the size of struct ata_device, thus reducing memory usage for ATA devices that do not support the CDL feature. Signed-off-by: Damien Le Moal Reviewed-by: Niklas Cassel --- drivers/ata/libata-core.c | 61 ++++++++++++++++++++++++++++------------------- drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 ++++++++++++---- 5 files changed, 57 insertions(+), 30 deletions(-) (limited to 'drivers/ata/libata-scsi.c') diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index bfd452b0d46d..082179c38e1b 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2464,12 +2464,41 @@ static void ata_dev_config_trusted(struct ata_device *dev) dev->flags |= ATA_DFLAG_TRUSTED; } +void ata_dev_cleanup_cdl_resources(struct ata_device *dev) +{ + kfree(dev->cdl); + dev->cdl = NULL; +} + +static int ata_dev_init_cdl_resources(struct ata_device *dev) +{ + struct ata_cdl *cdl = dev->cdl; + unsigned int err_mask; + + if (!cdl) { + cdl = kzalloc(sizeof(*cdl), GFP_KERNEL); + if (!cdl) + return -ENOMEM; + dev->cdl = cdl; + } + + err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, cdl->desc_log_buf, + ATA_LOG_CDL_SIZE / ATA_SECT_SIZE); + if (err_mask) { + ata_dev_warn(dev, "Read Command Duration Limits log failed\n"); + ata_dev_cleanup_cdl_resources(dev); + return -EIO; + } + + return 0; +} + static void ata_dev_config_cdl(struct ata_device *dev) { - struct ata_port *ap = dev->link->ap; unsigned int err_mask; bool cdl_enabled; u64 val; + int ret; if (ata_id_major_version(dev->id) < 11) goto not_supported; @@ -2564,37 +2593,20 @@ static void ata_dev_config_cdl(struct ata_device *dev) } } - /* - * Allocate a buffer to handle reading the sense data for successful - * NCQ Commands log page for commands using a CDL with one of the limit - * policy set to 0xD (successful completion with sense data available - * bit set). - */ - if (!ap->ncq_sense_buf) { - ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL); - if (!ap->ncq_sense_buf) - goto not_supported; - } - - /* - * Command duration limits is supported: cache the CDL log page 18h - * (command duration descriptors). - */ - err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, dev->sector_buf, 1); - if (err_mask) { - ata_dev_warn(dev, "Read Command Duration Limits log failed\n"); + /* CDL is supported: allocate and initialize needed resources. */ + ret = ata_dev_init_cdl_resources(dev); + if (ret) { + ata_dev_warn(dev, "Initialize CDL resources failed\n"); goto not_supported; } - memcpy(dev->cdl, dev->sector_buf, ATA_LOG_CDL_SIZE); dev->flags |= ATA_DFLAG_CDL; return; not_supported: dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED); - kfree(ap->ncq_sense_buf); - ap->ncq_sense_buf = NULL; + ata_dev_cleanup_cdl_resources(dev); } static int ata_dev_config_lba(struct ata_device *dev) @@ -5451,7 +5463,6 @@ void ata_port_free(struct ata_port *ap) kfree(ap->pmp_link); kfree(ap->slave_link); - kfree(ap->ncq_sense_buf); ida_free(&ata_ida, ap->print_id); kfree(ap); } @@ -5989,6 +6000,8 @@ void ata_dev_free_resources(struct ata_device *dev) { if (zpodd_dev_enabled(dev)) zpodd_exit(dev); + + ata_dev_cleanup_cdl_resources(dev); } /** diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 498430db86f7..c8b119a06bb2 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) { struct ata_device *dev = link->device; struct ata_port *ap = dev->link->ap; - u8 *buf = ap->ncq_sense_buf; + u8 *buf = dev->cdl->ncq_sense_log_buf; struct ata_queued_cmd *qc; unsigned int err_mask, tag; u8 *sense, sk = 0, asc = 0, ascq = 0; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 4fb45565c142..9f75d26808bf 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2248,7 +2248,7 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf) static unsigned int ata_msense_control_spgt2(struct ata_device *dev, u8 *buf, u8 spg) { - u8 *b, *cdl = dev->cdl, *desc; + u8 *b, *cdl = dev->cdl->desc_log_buf, *desc; u32 policy; int i; diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 927d77bde7ef..0337be4faec7 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -90,6 +90,7 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg); extern const char *sata_spd_string(unsigned int spd); extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log, u8 page, void *buf, unsigned int sectors); +void ata_dev_cleanup_cdl_resources(struct ata_device *dev); #define to_ata_port(d) container_of(d, struct ata_port, tdev) diff --git a/include/linux/libata.h b/include/linux/libata.h index aac38dcd2230..9b4a6ff03235 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -700,6 +700,21 @@ struct ata_cpr_log { struct ata_cpr cpr[] __counted_by(nr_cpr); }; +struct ata_cdl { + /* + * Buffer to cache the CDL log page 18h (command duration descriptors) + * for SCSI-ATA translation. + */ + u8 desc_log_buf[ATA_LOG_CDL_SIZE]; + + /* + * Buffer to handle reading the sense data for successful NCQ Commands + * log page for commands using a CDL with one of the limits policy set + * to 0xD (successful completion with sense data available bit set). + */ + u8 ncq_sense_log_buf[ATA_LOG_SENSE_NCQ_SIZE]; +}; + struct ata_device { struct ata_link *link; unsigned int devno; /* 0 or 1 */ @@ -762,8 +777,8 @@ struct ata_device { /* Concurrent positioning ranges */ struct ata_cpr_log *cpr_log; - /* Command Duration Limits log support */ - u8 cdl[ATA_LOG_CDL_SIZE]; + /* Command Duration Limits support */ + struct ata_cdl *cdl; /* error history */ int spdn_cnt; @@ -917,8 +932,6 @@ struct ata_port { #ifdef CONFIG_ATA_ACPI struct ata_acpi_gtm __acpi_init_gtm; /* use ata_acpi_init_gtm() */ #endif - /* owned by EH */ - u8 *ncq_sense_buf; }; /* The following initializer overrides a method to NULL whether one of -- cgit v1.2.3 From e5dd410acb34c7341a0a93b429dcf3dabf9e3323 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Mon, 9 Sep 2024 17:42:38 +0200 Subject: ata: libata: Clear DID_TIME_OUT for ATA PT commands with sense data When ata_qc_complete() schedules a command for EH using ata_qc_schedule_eh(), blk_abort_request() will be called, which leads to req->q->mq_ops->timeout() / scsi_timeout() being called. scsi_timeout(), if the LLDD has no abort handler (libata has no abort handler), will set host byte to DID_TIME_OUT, and then call scsi_eh_scmd_add() to add the command to EH. Thus, when commands first enter libata's EH strategy_handler, all the commands that have been added to EH will have DID_TIME_OUT set. libata has its own flag (AC_ERR_TIMEOUT), that it sets for commands that have not received a completion at the time of entering EH. Thus, libata doesn't really care about DID_TIME_OUT at all, and currently clears the host byte at the end of EH, in ata_scsi_qc_complete(), before scsi_eh_finish_cmd() is called. However, this clearing in ata_scsi_qc_complete() is currently only done for commands that are not ATA passthrough commands. Since the host byte is visible in the completion that we return to user space for ATA passthrough commands, for ATA passthrough commands that got completed via EH (commands with sense data), the user will incorrectly see: ATA pass-through(16): transport error: Host_status=0x03 [DID_TIME_OUT] Fix this by moving the clearing of the host byte (which is currently only done for commands that are not ATA passthrough commands) from ata_scsi_qc_complete() to the start of EH (regardless if the command is ATA passthrough or not). While at it, use the proper helper function to clear the host byte, rather than open coding the clearing. This will make sure that we: -Correctly clear DID_TIME_OUT for both ATA passthrough commands and commands that are not ATA passthrough commands. -Do not needlessly clear the host byte for commands that did not go via EH. ata_scsi_qc_complete() is called both for commands that are completed normally (without going via EH), and for commands that went via EH, however, only commands that went via EH will have DID_TIME_OUT set. Fixes: 24aeebbf8ea9 ("scsi: ata: libata: Change ata_eh_request_sense() to not set CHECK_CONDITION") Reported-by: Igor Pylypiv Closes: https://lore.kernel.org/linux-ide/ZttIN8He8TOZ7Lct@google.com/ Signed-off-by: Niklas Cassel Tested-by: Igor Pylypiv Reviewed-by: Hannes Reinecke Signed-off-by: Damien Le Moal --- drivers/ata/libata-eh.c | 8 ++++++++ drivers/ata/libata-scsi.c | 3 --- 2 files changed, 8 insertions(+), 3 deletions(-) (limited to 'drivers/ata/libata-scsi.c') diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 364828b8a22d..3f0144e7dc80 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -633,6 +633,14 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) { struct ata_queued_cmd *qc; + /* + * If the scmd was added to EH, via ata_qc_schedule_eh() -> + * scsi_timeout() -> scsi_eh_scmd_add(), scsi_timeout() will + * have set DID_TIME_OUT (since libata does not have an abort + * handler). Thus, to clear DID_TIME_OUT, clear the host byte. + */ + set_host_byte(scmd, DID_OK); + ata_qc_for_each_raw(ap, qc, i) { if (qc->flags & ATA_QCFLAG_ACTIVE && qc->scsicmd == scmd) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 9f75d26808bf..061fe63497bf 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1680,9 +1680,6 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION); } else if (is_error && !have_sense) { ata_gen_ata_sense(qc); - } else { - /* Keep the SCSI ML and status byte, clear host byte. */ - cmd->result &= 0x0000ffff; } ata_qc_done(qc); -- cgit v1.2.3