Re: [PATCH v2 1/1] libata: libata: add ATA_HORKAGE_NO_NCQ_TRIM for Samsung 860 and 870 SSDs

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/08/27 14:34, Kate Hsuan wrote:
> A flag ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL is added to disable NCQ
> on AMD/MAREL/ASMEDIA chipsets.
> 
> Samsung 860/870 SSD are disabled to use NCQ trim functions but it will
> lead to performace drop. From the bugzilla, we could realize the issues
> only appears on those chipset mentioned above. So this flag could be
> used to only disable NCQ on specific chipsets.
> 
> Fixes: ca6bfcb2f6d9 ("libata: Enable queued TRIM for Samsung SSD 860")
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=203475
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

Since this is a v2, you should not keep the review tag here.

> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---

This is a v2 patch, so please add the changelog from v1 here.

But I think that v1 introduced ATA_HORKAGE_NO_NCQ_TRIM. Yet, here you introduce
a completely different flag on top of the patch that introduced
ATA_HORKAGE_NO_NCQ_TRIM. So this patch is not version 2 of the previous one. It
is a completely different patch. Unless I missed v1 on the list...


>  drivers/ata/libata-core.c | 37 ++++++++++++++++++++++++++++++++-----
>  include/linux/libata.h    |  3 +++
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index cc459ce90018..50f635669dd4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2119,6 +2119,8 @@ static inline u8 ata_dev_knobble(struct ata_device *dev)
>  static void ata_dev_config_ncq_send_recv(struct ata_device *dev)
>  {
>  	struct ata_port *ap = dev->link->ap;
> +	struct device *parent = NULL;
> +	struct pci_dev *pcidev = NULL;
>  	unsigned int err_mask;
>  
>  	if (!ata_log_supported(dev, ATA_LOG_NCQ_SEND_RECV)) {
> @@ -2138,9 +2140,32 @@ static void ata_dev_config_ncq_send_recv(struct ata_device *dev)
>  		memcpy(cmds, ap->sector_buf, ATA_LOG_NCQ_SEND_RECV_SIZE);
>  
>  		if (dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM) {
> -			ata_dev_dbg(dev, "disabling queued TRIM support\n");
> -			cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &=
> -				~ATA_LOG_NCQ_SEND_RECV_DSM_TRIM;
> +			if (dev->horkage & ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL)
> +			{

Please follow the kernel coding style: do not break line before "{". This
comment applies to all your modifications below too.

> +				// get parent device for the controller vendor ID
> +				for(parent = dev->tdev.parent; parent != NULL; parent = parent->parent)
> +				{
> +					if(dev_is_pci(parent))
> +					{
> +						pcidev = to_pci_dev(parent);
> +						if (pcidev->vendor == PCI_VENDOR_ID_MARVELL ||
> +							pcidev->vendor == PCI_VENDOR_ID_AMD 	||
> +							pcidev->vendor == PCI_VENDOR_ID_ASMEDIA )

Please align the conditions.

> +						{
> +							ata_dev_dbg(dev, "Disable NCQ -> vendor ID %x product ID %x\n", 
> +												pcidev->vendor, pcidev->device);

Weird alignment here too. Move the arguments aligned with "dev" at the beginning
of the line.

> +							cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &=
> +								~ATA_LOG_NCQ_SEND_RECV_DSM_TRIM;
> +						}
> +						break;
> +					}
> +				}
> +			}else
> +			{
> +				ata_dev_dbg(dev, "disabling queued TRIM support\n");
> +				cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &=
> +					~ATA_LOG_NCQ_SEND_RECV_DSM_TRIM;
> +			}
>  		}
>  	}
>  }
> @@ -3951,9 +3976,11 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
>  	{ "Samsung SSD 850*",		NULL,	ATA_HORKAGE_NO_NCQ_TRIM |
>  						ATA_HORKAGE_ZERO_AFTER_TRIM, },
>  	{ "Samsung SSD 860*",           NULL,   ATA_HORKAGE_NO_NCQ_TRIM |
> -						ATA_HORKAGE_ZERO_AFTER_TRIM, },
> +						ATA_HORKAGE_ZERO_AFTER_TRIM |
> +						ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL, },

You named your flag ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL but you are
applying it to a Samsung device. This is rather confusing. I do not think you
need to have the vendor in the flag name, e.g. ATA_HORKAGE_NO_NCQ. Whatever
device in ata_device_blacklist has the flag will have NCQ disabled.

>  	{ "Samsung SSD 870*",           NULL,   ATA_HORKAGE_NO_NCQ_TRIM |
> -						ATA_HORKAGE_ZERO_AFTER_TRIM, },
> +						ATA_HORKAGE_ZERO_AFTER_TRIM |
> +						ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL, },

If you disable NCQ entirely for this device, then what is the point of keeping
ATA_HORKAGE_NO_NCQ_TRIM ? TRIM commands will not be NCQ anymore, similarly to
all other commands.

>  	{ "FCCT*M500*",			NULL,	ATA_HORKAGE_NO_NCQ_TRIM |
>  						ATA_HORKAGE_ZERO_AFTER_TRIM, },
>  
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 3fcd24236793..ec17f1f3fbf6 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -422,6 +422,9 @@ enum {
>  	ATA_HORKAGE_NOTRIM	= (1 << 24),	/* don't use TRIM */
>  	ATA_HORKAGE_MAX_SEC_1024 = (1 << 25),	/* Limit max sects to 1024 */
>  	ATA_HORKAGE_MAX_TRIM_128M = (1 << 26),	/* Limit max trim size to 128M */
> +	ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL = (1 << 27), /*Disable NCQ only on 
> +							ASMeida, AMD, and Marvell 
> +							Chipset*/

See above. You do not need to have the vendor name in the flag name.

>  
>  	 /* DMA mask for user DMA control: User visible values; DO NOT
>  	    renumber */
> 


-- 
Damien Le Moal
Western Digital Research