Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Kate Hsuan <hpa@redhat.com>, Jens Axboe <axboe@kernel.dk>,
        Hans de Goede <hdegoede@redhat.com>
CC:     "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung
 860 and 870 SSD.
Thread-Topic: [PATCH v3 1/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung
 860 and 870 SSD.
Thread-Index: AQHXna11sYaFfjER/Uyhl8ugEqYihw==
Date:   Mon, 30 Aug 2021 21:58:04 +0000
Message-ID: <DM6PR04MB708110D5731E3A6BCEA8A2DEE7CB9@DM6PR04MB7081.namprd04.prod.outlook.com>
References: <20210830144253.289542-1-hpa@redhat.com>
 <20210830144253.289542-2-hpa@redhat.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Il0D+DDu63JTH66Y7tQQcDXlTI9ibS/P1cKeVUjH4fN0xz6Ae+J4TsKdDJPW?=
 =?us-ascii?Q?CC70/JX5ZKGGo+HsxrCrlnSw2C7krOQLB01ZvHEjzHLq6P7MAQ0EugtvQZ2l?=
 =?us-ascii?Q?R7arLUGpbzAgeAUXnsZxu+tmNpDG2AEZiz29DRrXhuhtIMSTSlouXAeutDBv?=
 =?us-ascii?Q?9/y2+pY+eCAjSisWNY+xy8aHRoq3lUMsiZ+vSD6PlSWzRYjWMWwaj2u6Q54D?=
 =?us-ascii?Q?I721gBgsOv8vRrfJdRB7RUWYhdaEfQ1vjZCsPtdMkFje4ANLllsXJ6gNGbjO?=
 =?us-ascii?Q?NH5xHiMwpGWl7p/ylDJTwkivMNwbASE1SPj2F0uzS4DkjOlSUg/xg2pfOBxy?=
 =?us-ascii?Q?i0CcOMaFpAqMDR5Y6IXMjyPVjPeaLaPne6M8Xw/nKy3eLaUv5bvCKPXmbXJ/?=
 =?us-ascii?Q?7RjPAKTv8BYaQ12pKy/J/ABXC74TmgHlSZqJPJrJfyG+MebKpzyxHbf9MBjV?=
 =?us-ascii?Q?+q6oijVx6VotNNglRb2Vhfn0tHiV2Iosig3rG8Cp1HXFoCBXzSS2vUFcnCFq?=
 =?us-ascii?Q?ercR+JDUNcsrG31vCA3dzJR8oeHnAqv7Rfua/OmE+GlXE29or2CuDSWpOjTP?=
 =?us-ascii?Q?nCsf/roxhevi3oMIvY0TDL+S9a8uu93vuMbVisSlWk1fM2wd34vT3/d3/T8z?=
 =?us-ascii?Q?Y9+Sh0wFvCYRtvsVewXE/gi7Z6Zt9bFY4BJAP8A5m037aahtTVd8i4TfOmVF?=
 =?us-ascii?Q?JO+iuPIWoFmaFHrh8wVJd3l68eeVnYsSrCJl4eHpMj9C9TQnlkzx/MNLubsW?=
 =?us-ascii?Q?tRhuubcgyBy7J2NRunoDQEvOBvyv+y+F8rrn1fVTRgwrUdF3xRzIDuJHSrxm?=
 =?us-ascii?Q?zy/ASQMOtztS5u6mEKmjU2sPFYEflHD+sd8gKLjj79N0Rzj2Tb3+FSuOhQTK?=
 =?us-ascii?Q?6p1NBU7SRNI6lPzeNnvhC1yxbcbP9Y/kv/yWTETZVZy4QwkGO6ZA4OJQO8qv?=
 =?us-ascii?Q?FCCzwiOIgAW+e1Q8TZ40HGKccRShKVBi7TeI24F/HasbBXsh5QJ8pz4hllC8?=
 =?us-ascii?Q?+CbIzdSaiRfpems3R+YYj1bCuCU9YuBteeHFPJrmhZv2DELk7cg9LdF2fxlD?=
 =?us-ascii?Q?gmpWGTikmnyKxJTYOgveoCJ8NV7Ag1+yBTp1RpOeM5R8dO64FdbHHDHziwkV?=
 =?us-ascii?Q?3DtYAXjfK/ICmxD2b+UCDnLtdJULZEVaHdXywagUZXYOX+GRF0Qskxaaxcmt?=
 =?us-ascii?Q?meSaByc540TP07lWe31FwJpVkHRTDHKDZEj2/jER0rL5lBG1uamAfUJ27CEa?=
 =?us-ascii?Q?0FHNkuDlQtWYXo85QTgonJFgrE7HSjAqqgpr/CFVW1JhkWzczbk0zpJ9lsy4?=
 =?us-ascii?Q?C74qO8O1vHApamSNb9RD6/UDi9trLY5/dxjLRUBNcirH07TkCNWfNBtBYFo5?=
 =?us-ascii?Q?ECgCQmLMmkNXpE96ckT2zfH3sVmgnZDyvwWLoaFJFmzh4Tj94A=3D=3D?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR04MB7081.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 76bf6306-8a47-4c2c-74ed-08d96c013e6a
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Aug 2021 21:58:04.6402
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Kfhie4wUtqICpCkG55rJEeAsE8HYNdhI8bds3pT9SJbuhw0VSdS6lkezOndw6lf9C15sQP1iA9uo7RerkeIXog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5370
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021/08/30 23:43, Kate Hsuan wrote:=0A=
> Many users are reporting that the Samsung 860 and 870 SSD are having=0A=
> various issues when combined with AMD SATA controllers and only=0A=
> completely disabling NCQ helps to avoid these issues.=0A=
> =0A=
> Entire disabling NCQ for Samsugn 860/870 SSD will cause I/O performance=
=0A=
> drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to=0A=
=0A=
With "Entire disabling NCQ...", did you mean "Always disabling NCQ" ?=0A=
If I understand this issue correctly, the explanation should be something l=
ike:=0A=
=0A=
Always disabling NCQ for Samsung 860/870 SSDs regardless of the host SATA=
=0A=
adapter vendor will cause I/O performance degradation with well behaved=0A=
adapters. To limit the performance impact to AMD adapters, introduce the=0A=
ATA_HORKAGE_NO_NCQ_ON_AMD flag to force disable NCQ only for these adapters=
.=0A=
=0A=
> used to perform an additional check for these SSDs. If it finds it's=0A=
> parent ATA controller is AMD, the NCQ will be disabled. Otherwise, the=0A=
> NCQ is kept to enable.=0A=
> =0A=
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=3D201693=0A=
> Signed-off-by: Kate Hsuan <hpa@redhat.com>=0A=
> ---=0A=
>  drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--=0A=
>  include/linux/libata.h    |  1 +=0A=
>  2 files changed, 23 insertions(+), 2 deletions(-)=0A=
> =0A=
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c=0A=
> index c861c93d1e84..36c62f758b73 100644=0A=
> --- a/drivers/ata/libata-core.c=0A=
> +++ b/drivers/ata/libata-core.c=0A=
> @@ -2190,6 +2190,8 @@ static int ata_dev_config_ncq(struct ata_device *de=
v,=0A=
>  			       char *desc, size_t desc_sz)=0A=
>  {=0A=
>  	struct ata_port *ap =3D dev->link->ap;=0A=
> +	struct pci_dev *pcidev =3D NULL;=0A=
> +	struct device *parent_dev =3D NULL;=0A=
>  	int hdepth =3D 0, ddepth =3D ata_id_queue_depth(dev->id);=0A=
>  	unsigned int err_mask;=0A=
>  	char *aa_desc =3D "";=0A=
> @@ -2204,6 +2206,22 @@ static int ata_dev_config_ncq(struct ata_device *d=
ev,=0A=
>  		snprintf(desc, desc_sz, "NCQ (not used)");=0A=
>  		return 0;=0A=
>  	}=0A=
> +=0A=
> +	if (dev->horkage & ATA_HORKAGE_NONCQ_ON_AMD) {=0A=
> +		for (parent_dev =3D dev->tdev.parent; parent_dev !=3D NULL;=0A=
> +		    parent_dev =3D parent_dev->parent) {=0A=
> +			if (dev_is_pci(parent_dev)) {=0A=
> +				pcidev =3D to_pci_dev(parent_dev);=0A=
> +				if (pcidev->vendor =3D=3D PCI_VENDOR_ID_AMD) {=0A=
> +					snprintf(desc, desc_sz,=0A=
> +						 "NCQ (not used)");=0A=
> +					return 0;=0A=
> +				}=0A=
> +			break;=0A=
> +			}=0A=
> +		}=0A=
=0A=
It would be really nice to move this hunk into a small helper, something li=
ke:=0A=
=0A=
static bool ata_dev_check_adapter(struct ata_device *dev,=0A=
				  unsigned short vendor_id)=0A=
=0A=
The "if" code block then becomes:=0A=
=0A=
	if ((dev->horkage & ATA_HORKAGE_NONCQ_ON_AMD) &&=0A=
	    ata_dev_check_adapter(dev, PCI_VENDOR_ID_AMD)) {=0A=
		snprintf(desc, desc_sz, "NCQ (not used)");=0A=
		return 0;=0A=
	}=0A=
=0A=
=0A=
> +	}=0A=
> +=0A=
>  	if (ap->flags & ATA_FLAG_NCQ) {=0A=
>  		hdepth =3D min(ap->scsi_host->can_queue, ATA_MAX_QUEUE);=0A=
>  		dev->flags |=3D ATA_DFLAG_NCQ;=0A=
> @@ -3971,9 +3989,11 @@ static const struct ata_blacklist_entry ata_device=
_blacklist [] =3D {=0A=
>  	{ "Samsung SSD 850*",		NULL,	ATA_HORKAGE_NO_NCQ_TRIM |=0A=
>  						ATA_HORKAGE_ZERO_AFTER_TRIM, },=0A=
>  	{ "Samsung SSD 860*",           NULL,   ATA_HORKAGE_NO_NCQ_TRIM |=0A=
> -						ATA_HORKAGE_ZERO_AFTER_TRIM, },=0A=
> +						ATA_HORKAGE_ZERO_AFTER_TRIM |=0A=
> +						ATA_HORKAGE_NONCQ_ON_AMD, },=0A=
>  	{ "Samsung SSD 870*",           NULL,   ATA_HORKAGE_NO_NCQ_TRIM |=0A=
> -						ATA_HORKAGE_ZERO_AFTER_TRIM, },=0A=
> +						ATA_HORKAGE_ZERO_AFTER_TRIM |=0A=
> +						ATA_HORKAGE_NONCQ_ON_AMD, },=0A=
>  	{ "FCCT*M500*",			NULL,	ATA_HORKAGE_NO_NCQ_TRIM |=0A=
>  						ATA_HORKAGE_ZERO_AFTER_TRIM, },=0A=
>  =0A=
> diff --git a/include/linux/libata.h b/include/linux/libata.h=0A=
> index 860e63f5667b..42e16114e91f 100644=0A=
> --- a/include/linux/libata.h=0A=
> +++ b/include/linux/libata.h=0A=
> @@ -426,6 +426,7 @@ enum {=0A=
>  	ATA_HORKAGE_NOTRIM	=3D (1 << 24),	/* don't use TRIM */=0A=
>  	ATA_HORKAGE_MAX_SEC_1024 =3D (1 << 25),	/* Limit max sects to 1024 */=
=0A=
>  	ATA_HORKAGE_MAX_TRIM_128M =3D (1 << 26),	/* Limit max trim size to 128M=
 */=0A=
> +	ATA_HORKAGE_NONCQ_ON_AMD =3D (1 << 27),	/* Disable NCQ on AMD chipset *=
/=0A=
=0A=
Please add a "_" after "NO", similarly to ATA_HORKAGE_NO_NCQ_TRIM:=0A=
=0A=
	ATA_HORKAGE_NO_NCQ_ON_AMD =3D (1 << 27),	/* Disable NCQ on AMD chipset */=
=0A=
=0A=
>  =0A=
>  	 /* DMA mask for user DMA control: User visible values; DO NOT=0A=
>  	    renumber */=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=