From e8866e26f5e8ec551eec73dca1c30d458f9dca86 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 11 Apr 2025 07:59:09 +0200 Subject: ata: libata-core: Simplify ata_print_version_once Use dev_dbg_once() instead of open-coding the once functionality. Signed-off-by: Heiner Kallweit Signed-off-by: Damien Le Moal --- drivers/ata/libata-core.c | 6 ------ 1 file changed, 6 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 773799cfd443..79b20da0a256 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6682,12 +6682,6 @@ const struct ata_port_info ata_dummy_port_info = { }; EXPORT_SYMBOL_GPL(ata_dummy_port_info); -void ata_print_version(const struct device *dev, const char *version) -{ - dev_printk(KERN_DEBUG, dev, "version %s\n", version); -} -EXPORT_SYMBOL(ata_print_version); - EXPORT_TRACEPOINT_SYMBOL_GPL(ata_tf_load); EXPORT_TRACEPOINT_SYMBOL_GPL(ata_exec_command); EXPORT_TRACEPOINT_SYMBOL_GPL(ata_bmdma_setup); -- cgit v1.2.3 From f54464458d34141911047258090f25c67204cda4 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Wed, 16 Apr 2025 11:31:30 +0200 Subject: ata: libata-sata: Simplify sense_valid fetching While the SENSE DATA VALID field in the ACS-6 specification is 47 bits, we are currently only fetching 32 bits, because these are the only bits that we care about (these bits represent the tags (which can be 0-31)). Thus, replace the existing logic with a simple get_unaligned_le32(). While at it, change the type of sense_valid to u32. Signed-off-by: Niklas Cassel Reviewed-by: Hannes Reinecke Reviewed-by: Igor Pylypiv Signed-off-by: Damien Le Moal --- drivers/ata/libata-sata.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 2e4463d3a356..89d3b784706b 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1509,9 +1509,10 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) struct ata_queued_cmd *qc; unsigned int err_mask, tag; u8 *sense, sk = 0, asc = 0, ascq = 0; - u64 sense_valid, val; u16 extended_sense; bool aux_icc_valid; + u32 sense_valid; + u64 val; int ret = 0; err_mask = ata_read_log_page(dev, ATA_LOG_SENSE_NCQ, 0, buf, 2); @@ -1529,8 +1530,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) return -EIO; } - sense_valid = (u64)buf[8] | ((u64)buf[9] << 8) | - ((u64)buf[10] << 16) | ((u64)buf[11] << 24); + sense_valid = get_unaligned_le32(&buf[8]); extended_sense = get_unaligned_le16(&buf[14]); aux_icc_valid = extended_sense & BIT(15); @@ -1545,7 +1545,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) * If the command does not have any sense data, clear ATA_SENSE. * Keep ATA_QCFLAG_EH_SUCCESS_CMD so that command is finished. */ - if (!(sense_valid & (1ULL << tag))) { + if (!(sense_valid & (1 << tag))) { qc->result_tf.status &= ~ATA_SENSE; continue; } -- cgit v1.2.3 From ecd9ecc75d150b82a832eb7aaa9b7b983fea5271 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Wed, 16 Apr 2025 11:31:31 +0200 Subject: ata: libata-sata: Use BIT() macro to convert tag to bit field The BIT() macro is commonly used in the kernel. Make use of it when converting a tag, fetched from the Successful NCQ Commands log or the NCQ Command Error log, to a bit field. This makes the code easier to read. Suggested-by: Igor Pylypiv Signed-off-by: Niklas Cassel Reviewed-by: Hannes Reinecke Reviewed-by: Igor Pylypiv Signed-off-by: Damien Le Moal --- drivers/ata/libata-sata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 89d3b784706b..4e3034af285e 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1545,7 +1545,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) * If the command does not have any sense data, clear ATA_SENSE. * Keep ATA_QCFLAG_EH_SUCCESS_CMD so that command is finished. */ - if (!(sense_valid & (1 << tag))) { + if (!(sense_valid & BIT(tag))) { qc->result_tf.status &= ~ATA_SENSE; continue; } @@ -1634,7 +1634,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link) return; } - if (!(link->sactive & (1 << tag))) { + if (!(link->sactive & BIT(tag))) { ata_link_err(link, "log page 10h reported inactive tag %d\n", tag); return; -- cgit v1.2.3 From 23a8e0df49b851ed1ad12f87c52d113be8a6b6e2 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Tue, 22 Apr 2025 10:09:40 +0100 Subject: ata: sata_sx4: Fix spelling mistake "parttern" -> "pattern" There are spelling mistakes in arrays test_parttern1 and test_parttern2. Fix them. Signed-off-by: Colin Ian King Signed-off-by: Damien Le Moal --- drivers/ata/sata_sx4.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c index c3042eca6332..f7f5131af937 100644 --- a/drivers/ata/sata_sx4.c +++ b/drivers/ata/sata_sx4.c @@ -1301,32 +1301,32 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host) } if (dimm_test) { - u8 test_parttern1[40] = + u8 test_pattern1[40] = {0x55,0xAA,'P','r','o','m','i','s','e',' ', 'N','o','t',' ','Y','e','t',' ', 'D','e','f','i','n','e','d',' ', '1','.','1','0', '9','8','0','3','1','6','1','2',0,0}; - u8 test_parttern2[40] = {0}; + u8 test_pattern2[40] = {0}; - pdc20621_put_to_dimm(host, test_parttern2, 0x10040, 40); - pdc20621_put_to_dimm(host, test_parttern2, 0x40, 40); + pdc20621_put_to_dimm(host, test_pattern2, 0x10040, 40); + pdc20621_put_to_dimm(host, test_pattern2, 0x40, 40); - pdc20621_put_to_dimm(host, test_parttern1, 0x10040, 40); - pdc20621_get_from_dimm(host, test_parttern2, 0x40, 40); - dev_info(host->dev, "DIMM test pattern 1: %x, %x, %s\n", test_parttern2[0], - test_parttern2[1], &(test_parttern2[2])); - pdc20621_get_from_dimm(host, test_parttern2, 0x10040, + pdc20621_put_to_dimm(host, test_pattern1, 0x10040, 40); + pdc20621_get_from_dimm(host, test_pattern2, 0x40, 40); + dev_info(host->dev, "DIMM test pattern 1: %x, %x, %s\n", test_pattern2[0], + test_pattern2[1], &(test_pattern2[2])); + pdc20621_get_from_dimm(host, test_pattern2, 0x10040, 40); dev_info(host->dev, "DIMM test pattern 2: %x, %x, %s\n", - test_parttern2[0], - test_parttern2[1], &(test_parttern2[2])); + test_pattern2[0], + test_pattern2[1], &(test_pattern2[2])); - pdc20621_put_to_dimm(host, test_parttern1, 0x40, 40); - pdc20621_get_from_dimm(host, test_parttern2, 0x40, 40); + pdc20621_put_to_dimm(host, test_pattern1, 0x40, 40); + pdc20621_get_from_dimm(host, test_pattern2, 0x40, 40); dev_info(host->dev, "DIMM test pattern 3: %x, %x, %s\n", - test_parttern2[0], - test_parttern2[1], &(test_parttern2[2])); + test_pattern2[0], + test_pattern2[1], &(test_pattern2[2])); } /* ECC initiliazation. */ -- cgit v1.2.3 From 11533932f5c506f66281a147ff8469b97c108ab4 Mon Sep 17 00:00:00 2001 From: Igor Pylypiv Date: Tue, 22 Apr 2025 10:21:23 -0700 Subject: ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT For ATA PASS-THROUGH + fixed format sense data + NCQ autosense the INFORMATION sense data field is being written twice: - 1st write: (redundant) scsi_set_sense_information() sets the INFORMATION field to ATA LBA. This is incorrect for ATA PASS-THROUGH. - 2nd write: (correct) ata_scsi_set_passthru_sense_fields() sets the INFORMATION field to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec. There is no user-visible issue because second write overwrites the incorrect data from the first write. This patch eliminates the reduntant write by moving the INFORMATION sense data field population logic to ata_scsi_qc_complete(). Signed-off-by: Igor Pylypiv Reviewed-by: Niklas Cassel Signed-off-by: Damien Le Moal --- drivers/ata/libata-sata.c | 2 -- drivers/ata/libata-scsi.c | 31 ++++++++++++++----------------- drivers/ata/libata.h | 3 --- 3 files changed, 14 insertions(+), 22 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 4e3034af285e..cb46ce276bb1 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1659,8 +1659,6 @@ void ata_eh_analyze_ncq_error(struct ata_link *link) if (ata_scsi_sense_is_valid(sense_key, asc, ascq)) { ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc, ascq); - ata_scsi_set_sense_information(dev, qc->scsicmd, - &qc->result_tf); qc->flags |= ATA_QCFLAG_SENSE_VALID; } } diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 2796c0da8257..ef117a0bc248 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -216,17 +216,21 @@ void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd, scsi_build_sense(cmd, d_sense, sk, asc, ascq); } -void ata_scsi_set_sense_information(struct ata_device *dev, - struct scsi_cmnd *cmd, - const struct ata_taskfile *tf) +static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc) { u64 information; - information = ata_tf_read_block(tf, dev); + if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) { + ata_dev_dbg(qc->dev, + "missing result TF: can't set INFORMATION sense field\n"); + return; + } + + information = ata_tf_read_block(&qc->result_tf, qc->dev); if (information == U64_MAX) return; - scsi_set_sense_information(cmd->sense_buffer, + scsi_set_sense_information(qc->scsicmd->sense_buffer, SCSI_SENSE_BUFFERSIZE, information); } @@ -971,8 +975,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) * ata_gen_ata_sense - generate a SCSI fixed sense block * @qc: Command that we are erroring out * - * Generate sense block for a failed ATA command @qc. Descriptor - * format is used to accommodate LBA48 block address. + * Generate sense block for a failed ATA command @qc. * * LOCKING: * None. @@ -982,8 +985,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) struct ata_device *dev = qc->dev; struct scsi_cmnd *cmd = qc->scsicmd; struct ata_taskfile *tf = &qc->result_tf; - unsigned char *sb = cmd->sense_buffer; - u64 block; u8 sense_key, asc, ascq; if (ata_dev_disabled(dev)) { @@ -1014,12 +1015,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0); return; } - - block = ata_tf_read_block(&qc->result_tf, dev); - if (block == U64_MAX) - return; - - scsi_set_sense_information(sb, SCSI_SENSE_BUFFERSIZE, block); } void ata_scsi_sdev_config(struct scsi_device *sdev) @@ -1679,8 +1674,10 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) ata_scsi_set_passthru_sense_fields(qc); if (is_ck_cond_request) set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION); - } else if (is_error && !have_sense) { - ata_gen_ata_sense(qc); + } else if (is_error) { + if (!have_sense) + ata_gen_ata_sense(qc); + ata_scsi_set_sense_information(qc); } ata_qc_done(qc); diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 0337be4faec7..ce5c628fa6fd 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -141,9 +141,6 @@ extern int ata_scsi_offline_dev(struct ata_device *dev); extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq); extern void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq); -extern void ata_scsi_set_sense_information(struct ata_device *dev, - struct scsi_cmnd *cmd, - const struct ata_taskfile *tf); extern void ata_scsi_media_change_notify(struct ata_device *dev); extern void ata_scsi_hotplug(struct work_struct *work); extern void ata_scsi_dev_rescan(struct work_struct *work); -- cgit v1.2.3 From 381d43b26282377a7e2f7ddfdd0147ad72353621 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Thu, 15 May 2025 15:56:23 +0200 Subject: ata: libata-eh: Update DIPM comments to reflect reality The comments describing which LPM policies that has DIPM enabled predates the introduction of the LPM policies ATA_LPM_MIN_POWER_WITH_PARTIAL and ATA_LPM_MED_POWER_WITH_DIPM. Update the DIPM comments to reflect reality. Also remove the sentence that claims that "Order device and link configurations such that the host always allows DIPM requests." This comment is written before 24e0e61db3cb ("ata: libata: disallow dev-initiated LPM transitions to unsupported states"). Even though the set_lpm() call is done before enabling DIPM, the host will not always allow DIPM requests. For all LPM polcies where DIPM is enabled, only DIPM requests to LPM states that are supported by the HBA will be allowed. Signed-off-by: Niklas Cassel Signed-off-by: Damien Le Moal --- drivers/ata/libata-eh.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index b990c1ee0b12..f39756a26751 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3443,10 +3443,9 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, return 0; /* - * DIPM is enabled only for MIN_POWER as some devices - * misbehave when the host NACKs transition to SLUMBER. Order - * device and link configurations such that the host always - * allows DIPM requests. + * DIPM is enabled only for ATA_LPM_MIN_POWER, + * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as + * some devices misbehave when the host NACKs transition to SLUMBER. */ ata_for_each_dev(dev, link, ENABLED) { bool hipm = ata_id_has_hipm(dev->id); @@ -3505,7 +3504,11 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, if (ap && ap->slave_link) ap->slave_link->lpm_policy = policy; - /* host config updated, enable DIPM if transitioning to MIN_POWER */ + /* + * Host config updated, enable DIPM if transitioning to + * ATA_LPM_MIN_POWER, ATA_LPM_MIN_POWER_WITH_PARTIAL, or + * ATA_LPM_MED_POWER_WITH_DIPM. + */ ata_for_each_dev(dev, link, ENABLED) { if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && !no_dipm && ata_id_has_dipm(dev->id)) { -- cgit v1.2.3 From 62eef53ab5ede2dba18ce4c5e7d031e05ab74025 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Thu, 15 May 2025 15:56:24 +0200 Subject: ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE link->lpm_policy is initialized to ATA_LPM_UNKNOWN in ata_eh_reset(). ata_eh_set_lpm() is then only called if link->lpm_policy != ap->target_lpm_policy (after reset) and then only if link->lpm_policy > ATA_LPM_MAX_POWER (before revalidation). This means that ata_eh_set_lpm() is currently never called with policy == ATA_LPM_UNKNOWN. Add a WARN_ON_ONCE so that it is more obvious from reading the code that this function is never called with policy == ATA_LPM_UNKNOWN. Signed-off-by: Niklas Cassel Signed-off-by: Damien Le Moal --- drivers/ata/libata-eh.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index f39756a26751..89b7b2139a16 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3442,6 +3442,13 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm)) return 0; + /* + * This function currently assumes that it will never be supplied policy + * ATA_LPM_UNKNOWN. + */ + if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN)) + return 0; + /* * DIPM is enabled only for ATA_LPM_MIN_POWER, * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as -- cgit v1.2.3 From 22cfba10dbfb3b4ded7038a543b74110d6d2de85 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Thu, 15 May 2025 15:56:25 +0200 Subject: ata: libata-eh: Rename hipm and dipm variables Rename the hipm and dipm variables to have a clearer name. Also fold in the usage of no_dipm, as that is required in order to give the dipm variable a more descriptive name. No functional change. Signed-off-by: Niklas Cassel Signed-off-by: Damien Le Moal --- drivers/ata/libata-eh.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 89b7b2139a16..dcb449edd315 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3455,22 +3455,23 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, * some devices misbehave when the host NACKs transition to SLUMBER. */ ata_for_each_dev(dev, link, ENABLED) { - bool hipm = ata_id_has_hipm(dev->id); - bool dipm = ata_id_has_dipm(dev->id) && !no_dipm; + bool dev_has_hipm = ata_id_has_hipm(dev->id); + bool dev_has_dipm = ata_id_has_dipm(dev->id); /* find the first enabled and LPM enabled devices */ if (!link_dev) link_dev = dev; - if (!lpm_dev && (hipm || dipm)) + if (!lpm_dev && (dev_has_hipm || (dev_has_dipm && !no_dipm))) lpm_dev = dev; hints &= ~ATA_LPM_EMPTY; - if (!hipm) + if (!dev_has_hipm) hints &= ~ATA_LPM_HIPM; /* disable DIPM before changing link config */ - if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dipm) { + if (policy < ATA_LPM_MED_POWER_WITH_DIPM && + (dev_has_dipm && !no_dipm)) { err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_DISABLE, SATA_DIPM); if (err_mask && err_mask != AC_ERR_DEV) { @@ -3517,8 +3518,10 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, * ATA_LPM_MED_POWER_WITH_DIPM. */ ata_for_each_dev(dev, link, ENABLED) { + bool dev_has_dipm = ata_id_has_dipm(dev->id); + if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && !no_dipm && - ata_id_has_dipm(dev->id)) { + dev_has_dipm) { err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE, SATA_DIPM); if (err_mask && err_mask != AC_ERR_DEV) { -- cgit v1.2.3 From 6d915e2812b3faae71d54b914f6351a562204b79 Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Thu, 15 May 2025 15:56:26 +0200 Subject: ata: libata-eh: Rename no_dipm variable to be more clear Rename the no_dipm variable to host_has_dipm, by inverting the expression, and and also having a clearer name. No functional change. Signed-off-by: Niklas Cassel Signed-off-by: Damien Le Moal --- drivers/ata/libata-eh.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index dcb449edd315..0e36a7806cff 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3432,7 +3432,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, struct ata_eh_context *ehc = &link->eh_context; struct ata_device *dev, *link_dev = NULL, *lpm_dev = NULL; enum ata_lpm_policy old_policy = link->lpm_policy; - bool no_dipm = link->ap->flags & ATA_FLAG_NO_DIPM; + bool host_has_dipm = !(link->ap->flags & ATA_FLAG_NO_DIPM); unsigned int hints = ATA_LPM_EMPTY | ATA_LPM_HIPM; unsigned int err_mask; int rc; @@ -3462,7 +3462,8 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, if (!link_dev) link_dev = dev; - if (!lpm_dev && (dev_has_hipm || (dev_has_dipm && !no_dipm))) + if (!lpm_dev && + (dev_has_hipm || (dev_has_dipm && host_has_dipm))) lpm_dev = dev; hints &= ~ATA_LPM_EMPTY; @@ -3471,7 +3472,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, /* disable DIPM before changing link config */ if (policy < ATA_LPM_MED_POWER_WITH_DIPM && - (dev_has_dipm && !no_dipm)) { + (dev_has_dipm && host_has_dipm)) { err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_DISABLE, SATA_DIPM); if (err_mask && err_mask != AC_ERR_DEV) { @@ -3520,7 +3521,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, ata_for_each_dev(dev, link, ENABLED) { bool dev_has_dipm = ata_id_has_dipm(dev->id); - if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && !no_dipm && + if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && host_has_dipm && dev_has_dipm) { err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE, SATA_DIPM); -- cgit v1.2.3 From a374cfbf609017f77a985357656be07a2da22c5f Mon Sep 17 00:00:00 2001 From: Niklas Cassel Date: Thu, 15 May 2025 15:56:27 +0200 Subject: ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM states Currently, it is possible that LPM is enabled while calling the set_lpm() callback. The current code performs a SET FEATURES command to disable DIPM if policy < ATA_LPM_MED_POWER_WITH_DIPM, this means that it will currently disable DIPM for policies: ATA_LPM_UNKNOWN, ATA_LPM_MAX_POWER, ATA_LPM_MED_POWER (but not for policy ATA_LPM_MED_POWER_WITH_DIPM). The code called after calling the set_lpm() callback will later perform a SET FEATURES command to enable DIPM, if policy >= ATA_LPM_MED_POWER_WITH_DIPM. As we can see DIPM will not be disabled before calling set_lpm() if the LPM policy is: ATA_LPM_MED_POWER_WITH_DIPM, ATA_LPM_MIN_POWER_WITH_PARTIAL, or ATA_LPM_MIN_POWER. Make sure that we always disable DIPM before calling the set_lpm() callback. This is because the set_lpm() callback is the function (for AHCI) that sets the proper bits in PxSCTL.IPM, reflecting the support of the HBA. PxSCTL.IPM controls the LPM states that the device is allowed to enter. If the device tries to enter a state disabled by PxSCTL.IPM, the host will NAK the transition. If we do not disable DIPM before modifying PxSCTL.IPM, it is possible that DIPM will try (and will be allowed to) enter a LPM state that the HBA does not support (since we have not yet written PxSCTL.IPM, the HBA wasn't able to NAK the transition). While at it, remove the guard of host support for DIPM around the disabling of DIPM. While it makes sense to take host support for DIPM into account when enabling DIPM, it makes zero sense to take host support into account when disabling DIPM. If the host does not support DIPM, that is an even bigger reason why DIPM should be disabled on the device side. Signed-off-by: Niklas Cassel Signed-off-by: Damien Le Moal --- drivers/ata/libata-eh.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/ata') diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 0e36a7806cff..c11d8e634bf7 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3471,8 +3471,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, hints &= ~ATA_LPM_HIPM; /* disable DIPM before changing link config */ - if (policy < ATA_LPM_MED_POWER_WITH_DIPM && - (dev_has_dipm && host_has_dipm)) { + if (dev_has_dipm) { err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_DISABLE, SATA_DIPM); if (err_mask && err_mask != AC_ERR_DEV) { -- cgit v1.2.3