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@xxxxxxxxxx>
> Reviewed-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
Since this is a v2, you should not keep the review tag here.
> Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
> ---
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
