From patchwork Wed Apr 16 08:42:36 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Damien Le Moal X-Patchwork-Id: 881795 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9DD5B16D9C2; Wed, 16 Apr 2025 08:43:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744793007; cv=none; b=Ltq0Bm3ZNR3ubuRg5vDQmd7BupHLRq/Ni83/EcJ7qfTVFoZD9JeJfkq9S/8vBuftE6RPMH0WvVR/BmYb9/j696LRuxXAtFNULMZPIfhMNMrl2l8+epE7y+8r6dHvkdOx0pSyt9RZcErm8eMxDYJ+07AKwp3f2lPpFWGHwA+TRg4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744793007; c=relaxed/simple; bh=KwlgcXLBhIHGmpxNvfzwJ3uUqjR5Xd4tQVjp6spfW70=; h=From:To:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=SgYvy6YzgVV1W5opRgLHBi14e5nAu8YnhLbpnvthi9UN7qgxWczXi1sIeAUSbPGuR5+EbCLVHABITXp1uMUfa1XDTfQy2xK2Bk+ZErKYQFwuk799nYYoAVKqOADbs3yovqBMx/E84TG5I5heuIJHAnTHedhjiq9pkcpNR/K9l2I= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ZAc2IfLM; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ZAc2IfLM" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8A1D4C4CEE9; Wed, 16 Apr 2025 08:43:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1744793007; bh=KwlgcXLBhIHGmpxNvfzwJ3uUqjR5Xd4tQVjp6spfW70=; h=From:To:Subject:Date:In-Reply-To:References:From; b=ZAc2IfLMyaYNcif6p1K4oPtCzRRmlkwG/wfbRkUR/M210aG6ZzUqRBvk1HbX8anOX ApFr/cQtnLKZIRuuaSa4yRhCVPxTX3jRCsuhEedoOqiyxtLn5VFk9fZT7PpuwqTy6E XSD0SSXv5WBuuimgInPtj6GSJfCx8JN5YEo/boyAnXioXwW9B9mY8ST4Y+gkOuCrby F9P55X0SIP0GT5y/SE6WBAY+HuzF+7EX+TZC/r4OnDau19gW0MyaJdyaydvOwTK0h2 nnIgYBO9A/8u5klhgrh1FRg2yVSLEJxXRWBRxH8zmFv8YtyUkjFTPTfaYnMv7Qa3mZ 1zUurgd8b8rwA== From: Damien Le Moal To: linux-ide@vger.kernel.org, Niklas Cassel , linux-scsi@vger.kernel.org, "Martin K . Petersen" Subject: [PATCH 1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature() Date: Wed, 16 Apr 2025 17:42:36 +0900 Message-ID: <20250416084238.258169-2-dlemoal@kernel.org> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250416084238.258169-1-dlemoal@kernel.org> References: <20250416084238.258169-1-dlemoal@kernel.org> Precedence: bulk X-Mailing-List: linux-scsi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 For the ATA features subpage of the control mode page, the T10 SAT-6 specifications state that: For a MODE SENSE command, the SATL shall return the CDL_CTRL field value that was last set by an application client. However, the function ata_msense_control_ata_feature() always sets the CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and T2B pages. This is thus incorrect and the value 0x02 must be reported only after the user enables the CDL feature, which is indicated with the ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the CDL_CTRL field of the ATA feature subpage of the control mode page must report a value of 0x00. Fix ata_msense_control_ata_feature() to report the correct values for the CDL_CTRL field, according to the enable/disable state of the device CDL feature. Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal Reviewed-by: Igor Pylypiv --- drivers/ata/libata-scsi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 2796c0da8257..e6c652b8a541 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev, */ put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]); - if (dev->flags & ATA_DFLAG_CDL) - buf[4] = 0x02; /* Support T2A and T2B pages */ + if ((dev->flags & ATA_DFLAG_CDL) && + (dev->flags & ATA_DFLAG_CDL_ENABLED)) + buf[4] = 0x02; /* T2A and T2B pages enabled */ else buf[4] = 0; From patchwork Wed Apr 16 08:42:37 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Damien Le Moal X-Patchwork-Id: 882764 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 766D516D9C2; Wed, 16 Apr 2025 08:43:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744793008; cv=none; b=mgYHVKws/qePMeajUQPBL9RifA0MEgj+B+3gQB3+qIKN6e+SnqdqxhuBgD6+xcnZ5UxFOr9vBf6THbB4c8KhSa80g/1DMLcldUXJvKwU+nUK1b+nbYGzJ6wSVHdg6vFVDPQCdlsMhMj4OTLUEgLX5sCF4ATrtK/zcwih6/Rnzu8= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744793008; c=relaxed/simple; bh=TPzmugDmNNfyBq9wQg/xVIAEkeYNh6rd45Qp3mXyhf0=; h=From:To:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=ODJio7zwWZ7KTs64tIpUZJwmN5Qe/DPt9v8fboKCH2n7vm7SqbMOBRpgRhL0TC9KlpfHd8bLFoLwrAlCp+pg4gaKJs9YRL0VxaxV5YE0sOtFZjsFPUBsMmQYdLmc7PyzfccvJRSP4hhF+mdMx47CE4bOvFGMB1qQLm1PnX+k0iU= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=mAultcUC; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="mAultcUC" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5E384C4CEEE; Wed, 16 Apr 2025 08:43:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1744793007; bh=TPzmugDmNNfyBq9wQg/xVIAEkeYNh6rd45Qp3mXyhf0=; h=From:To:Subject:Date:In-Reply-To:References:From; b=mAultcUCa2OKDceDdfWh9ipu+bXq38zjHrRsVEtwdjvS+GEqyAvN8dyXUllAQhnNK 0J+aquiVWVlTln5agDVgOG23WGe4LPu9jQXP2VYomNef+7WojbCwY4YSlDFmnMOCCr 0rjw2bVFUEMKD5Z0YUdsRw90XlQE/H97GKFo5agHRZjxIHBo9IQlirRVf68NJVCeAv YppheyljiGh97OfyRumfwvFiggAH7vjze+39wYnq+rTlOyVSkGnUi7a8kPqvc4pCSm pkMIaptZwSlQWUNVOzwp16HguZtiYxO+3olKD7+K8VRpzJJfh/U64ZnYA1Yw42B208 KfHJkusjok0Ug== From: Damien Le Moal To: linux-ide@vger.kernel.org, Niklas Cassel , linux-scsi@vger.kernel.org, "Martin K . Petersen" Subject: [PATCH 2/3] ata: libata-scsi: Improve CDL control Date: Wed, 16 Apr 2025 17:42:37 +0900 Message-ID: <20250416084238.258169-3-dlemoal@kernel.org> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250416084238.258169-1-dlemoal@kernel.org> References: <20250416084238.258169-1-dlemoal@kernel.org> Precedence: bulk X-Mailing-List: linux-scsi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 With ATA devices supporting the CDL feature, using CDL requires that the feature be enabled with a SET FEATURES command. This command is issued as the translated command for the MODE SELECT command issued by scsi_cdl_enable() when the user enables CDL through the device cdl_enable sysfs attribute. Currently, ata_mselect_control_ata_feature() always translates a MODE SELECT command for the ATA features subpage of the control mode page to a SET FEATURES command to enable or disable CDL based on the cdl_ctrl field. However, there is no need to issue the SET FEATURES command if: 1) The MODE SELECT command requests disabling CDL and CDL is already disabled. 2) The MODE SELECT command requests enabling CDL and CDL is already enabled. Fix ata_mselect_control_ata_feature() to issue the SET FEATURES command only when necessary. Since enabling CDL also implies a reset of the CDL statistics log page, avoiding useless CDL enable operations also avoids clearing the CDL statistics log. Also add debug messages to clearly signal when CDL is being enabled or disabled using a SET FEATURES command. Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal --- drivers/ata/libata-scsi.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index e6c652b8a541..6c73a8066381 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -3910,17 +3910,27 @@ static unsigned int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc, /* Check cdl_ctrl */ switch (buf[0] & 0x03) { case 0: - /* Disable CDL */ + /* Disable CDL if it is enabled */ + if (!(dev->flags & ATA_DFLAG_CDL_ENABLED)) + return 0; + ata_dev_dbg(dev, "Disabling CDL\n"); cdl_action = 0; dev->flags &= ~ATA_DFLAG_CDL_ENABLED; break; case 0x02: - /* Enable CDL T2A/T2B: NCQ priority must be disabled */ + /* + * Enable CDL T2A/T2B if not already enabled. Since this is + * mutually exclusive with NCQ priority, allow this only if NCQ + * priority is disabled. + */ + if (dev->flags & ATA_DFLAG_CDL_ENABLED) + return 0; if (dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED) { ata_dev_err(dev, "NCQ priority must be disabled to enable CDL\n"); return -EINVAL; } + ata_dev_dbg(dev, "Enabling CDL\n"); cdl_action = 1; dev->flags |= ATA_DFLAG_CDL_ENABLED; break; From patchwork Wed Apr 16 08:42:38 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Damien Le Moal X-Patchwork-Id: 881794 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4EC8216D9C2; Wed, 16 Apr 2025 08:43:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744793009; cv=none; b=lcQ4O+eYhjoZcfdg/f8p8z+MlqHqybsOZ8R8GWHtH+F2qrsLuJCQle/5ChCTKZkZGuLyjOTV+2+zTdnR3JPrvculyMtLuFHSgeGYar2opaMfUGj2cnMiBnhdX9mn1OCsoAsTzgiKUTXvGH+JBHAq/ILxVTXhAlrICVAUSGAEDa0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744793009; c=relaxed/simple; bh=lOyMiD9UoD5MQZTrgDpJRPHt2vdkigYv1J/uj0DvuAA=; h=From:To:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=cw6l/3uFvvcguZ5Dfbp7HcNOy0FMFjWRhx+7asB3HZuhskuxN2iCAZIUrCiLMjV3ZmMazIIASIJ75vmxL1Zz2nG/trQDWDj8GufUBeI0BBQHwy6EgCa71PuRUznQ1KM2o+OH2lqQf6QLAeTzuHxpBmJ72DbVcH1KmPd33nfdO0A= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Xp0uctuS; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Xp0uctuS" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3AA76C4CEE9; Wed, 16 Apr 2025 08:43:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1744793008; bh=lOyMiD9UoD5MQZTrgDpJRPHt2vdkigYv1J/uj0DvuAA=; h=From:To:Subject:Date:In-Reply-To:References:From; b=Xp0uctuSNYSXMK2/1inUMPZSdK2vCMQQ+gcqMXQYJq5rVFHnLh2cY5vgLl4wcYCHl NWkrb56FYKPlRmuxkbihQ/RDPwg4eG2xkT8aMprVKZhBSSZvON7FGZmATI5z82QscG STkmJisGD3VfIXreqHD9a3uZLQGdlhVh2cuojO3E2h+UMK7BEuIZrlao69ZD3wIXP9 3VY54bcSl1HLK4AZ9x+j/j6ctIKQMEnETEdZTmFON439jdst0iIZBY/Dm2z+lxKzh+ Q6J0AyboyKHrXgQi1JofS42g5SjHRm8YQC1pFWp0bXb7p8uNJcCL5K/EIjAK0bPW0L sl0eOjlBJPZtQ== From: Damien Le Moal To: linux-ide@vger.kernel.org, Niklas Cassel , linux-scsi@vger.kernel.org, "Martin K . Petersen" Subject: [PATCH 3/3] scsi: Improve CDL control Date: Wed, 16 Apr 2025 17:42:38 +0900 Message-ID: <20250416084238.258169-4-dlemoal@kernel.org> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250416084238.258169-1-dlemoal@kernel.org> References: <20250416084238.258169-1-dlemoal@kernel.org> Precedence: bulk X-Mailing-List: linux-scsi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 With ATA devices supporting the CDL feature, using CDL requires that the feature be enabled with a SET FEATURES command. This command is issued as the translated command for the MODE SELECT command issued by scsi_cdl_enable() when the user enables CDL through the device cdl_enable sysfs attribute. However, the implementation of scsi_cdl_enable() always issues a MODE SELECT command for ATA devices when the enable argument is true, even if CDL is already enabled on the device. While this does not cause any issue with using CDL descriptors with read/write commands (the CDL feature will be enabled on the drive), issuing the MODE SELECT command even when the device CDL feature is already enabled will cause a reset of the ATA device CDL statistics log page (as defined in ACS, any CDL enable action must reset the device statistics). Avoid this needless actions (and the implied statistics log page reset) by modifying scsi_cdl_enable() to issue the MODE SELECT command to enable CDL if and only if CDL is not reported as already enabled on the device. And while at it, simplify the initialization of the is_ata boolean variable and move the declaration of the scsi mode data and sense header variables to within the scope of ATA device handling. Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal Reviewed-by: Igor Pylypiv --- drivers/scsi/scsi.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 53daf923ad8e..518a252eb6aa 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -707,26 +707,23 @@ void scsi_cdl_check(struct scsi_device *sdev) */ int scsi_cdl_enable(struct scsi_device *sdev, bool enable) { - struct scsi_mode_data data; - struct scsi_sense_hdr sshdr; - struct scsi_vpd *vpd; - bool is_ata = false; char buf[64]; + bool is_ata; int ret; if (!sdev->cdl_supported) return -EOPNOTSUPP; rcu_read_lock(); - vpd = rcu_dereference(sdev->vpd_pg89); - if (vpd) - is_ata = true; + is_ata = rcu_dereference(sdev->vpd_pg89); rcu_read_unlock(); /* * For ATA devices, CDL needs to be enabled with a SET FEATURES command. */ if (is_ata) { + struct scsi_mode_data data; + struct scsi_sense_hdr sshdr; char *buf_data; int len; @@ -735,16 +732,30 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable) if (ret) return -EINVAL; - /* Enable CDL using the ATA feature page */ + /* Enable or disable CDL using the ATA feature page */ len = min_t(size_t, sizeof(buf), data.length - data.header_length - data.block_descriptor_length); buf_data = buf + data.header_length + data.block_descriptor_length; - if (enable) - buf_data[4] = 0x02; - else - buf_data[4] = 0; + + /* + * If we want to enable CDL and CDL is already enabled on the + * device, do nothing. This avoids needlessly resetting the CDL + * statistics on the device as that is implied by the CDL enable + * action. Similar to this, there is no need to do anything if + * we want to disable CDL and CDL is already disabled. + */ + if (enable) { + if ((buf_data[4] & 0x03) == 0x02) + goto out; + buf_data[4] &= ~0x03; + buf_data[4] |= 0x02; + } else { + if ((buf_data[4] & 0x03) == 0x00) + goto out; + buf_data[4] &= ~0x03; + } ret = scsi_mode_select(sdev, 1, 0, buf_data, len, 5 * HZ, 3, &data, &sshdr); @@ -756,6 +767,7 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable) } } +out: sdev->cdl_enable = enable; return 0;