* [PATCH v3 0/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. @ 2021-08-30 14:42 Kate Hsuan 2021-08-30 14:42 ` [PATCH v3 1/1] " Kate Hsuan 2021-08-30 21:45 ` [PATCH v3 0/1] " Damien Le Moal 0 siblings, 2 replies; 6+ messages in thread From: Kate Hsuan @ 2021-08-30 14:42 UTC (permalink / raw) To: Jens Axboe, Hans de Goede, Damien Le Moal Cc: linux-ide, linux-kernel, Kate Hsuan Many users reported the issue when running the system with Samsung 860, 870 SSD, and AMD chipset. Therefore, completely disabling the NCQ can avoid this issue. Entire disabling NCQ for Samsung 860/870 SSD will cause I/O performance drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to used to perform an additional check for these SSDs. If it finds its parent ATA controller is AMD, the NCQ will be disabled. Otherwise, the NCQ is kept to enable. Changes since v3 * Modified the flag from ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL to ATA_HORKAGE_NONCQ_ON_AMD. * Modified and fixed the code to completely disable NCQ on AMD controller. Kate Hsuan (1): libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. drivers/ata/libata-core.c | 24 ++++++++++++++++++++++-- include/linux/libata.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. 2021-08-30 14:42 [PATCH v3 0/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD Kate Hsuan @ 2021-08-30 14:42 ` Kate Hsuan 2021-08-30 15:05 ` Hans de Goede 2021-08-30 21:58 ` Damien Le Moal 2021-08-30 21:45 ` [PATCH v3 0/1] " Damien Le Moal 1 sibling, 2 replies; 6+ messages in thread From: Kate Hsuan @ 2021-08-30 14:42 UTC (permalink / raw) To: Jens Axboe, Hans de Goede, Damien Le Moal Cc: linux-ide, linux-kernel, Kate Hsuan Many users are reporting that the Samsung 860 and 870 SSD are having various issues when combined with AMD SATA controllers and only completely disabling NCQ helps to avoid these issues. Entire disabling NCQ for Samsugn 860/870 SSD will cause I/O performance drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to used to perform an additional check for these SSDs. If it finds it's parent ATA controller is AMD, the NCQ will be disabled. Otherwise, the NCQ is kept to enable. BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201693 Signed-off-by: Kate Hsuan <hpa@redhat.com> --- drivers/ata/libata-core.c | 24 ++++++++++++++++++++++-- include/linux/libata.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index c861c93d1e84..36c62f758b73 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2190,6 +2190,8 @@ static int ata_dev_config_ncq(struct ata_device *dev, char *desc, size_t desc_sz) { struct ata_port *ap = dev->link->ap; + struct pci_dev *pcidev = NULL; + struct device *parent_dev = NULL; int hdepth = 0, ddepth = ata_id_queue_depth(dev->id); unsigned int err_mask; char *aa_desc = ""; @@ -2204,6 +2206,22 @@ static int ata_dev_config_ncq(struct ata_device *dev, snprintf(desc, desc_sz, "NCQ (not used)"); return 0; } + + if (dev->horkage & ATA_HORKAGE_NONCQ_ON_AMD) { + for (parent_dev = dev->tdev.parent; parent_dev != NULL; + parent_dev = parent_dev->parent) { + if (dev_is_pci(parent_dev)) { + pcidev = to_pci_dev(parent_dev); + if (pcidev->vendor == PCI_VENDOR_ID_AMD) { + snprintf(desc, desc_sz, + "NCQ (not used)"); + return 0; + } + break; + } + } + } + if (ap->flags & ATA_FLAG_NCQ) { hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE); dev->flags |= ATA_DFLAG_NCQ; @@ -3971,9 +3989,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_AMD, }, { "Samsung SSD 870*", NULL, ATA_HORKAGE_NO_NCQ_TRIM | - ATA_HORKAGE_ZERO_AFTER_TRIM, }, + ATA_HORKAGE_ZERO_AFTER_TRIM | + ATA_HORKAGE_NONCQ_ON_AMD, }, { "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 860e63f5667b..42e16114e91f 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -426,6 +426,7 @@ 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_AMD = (1 << 27), /* Disable NCQ on AMD chipset */ /* DMA mask for user DMA control: User visible values; DO NOT renumber */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. 2021-08-30 14:42 ` [PATCH v3 1/1] " Kate Hsuan @ 2021-08-30 15:05 ` Hans de Goede 2021-08-30 21:58 ` Damien Le Moal 1 sibling, 0 replies; 6+ messages in thread From: Hans de Goede @ 2021-08-30 15:05 UTC (permalink / raw) To: Kate Hsuan, Jens Axboe, Damien Le Moal; +Cc: linux-ide, linux-kernel Hi, On 8/30/21 4:42 PM, Kate Hsuan wrote: > Many users are reporting that the Samsung 860 and 870 SSD are having > various issues when combined with AMD SATA controllers and only > completely disabling NCQ helps to avoid these issues. > > Entire disabling NCQ for Samsugn 860/870 SSD will cause I/O performance > drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to > used to perform an additional check for these SSDs. If it finds it's > parent ATA controller is AMD, the NCQ will be disabled. Otherwise, the > NCQ is kept to enable. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201693 > Signed-off-by: Kate Hsuan <hpa@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/ata/libata-core.c | 24 ++++++++++++++++++++++-- > include/linux/libata.h | 1 + > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index c861c93d1e84..36c62f758b73 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2190,6 +2190,8 @@ static int ata_dev_config_ncq(struct ata_device *dev, > char *desc, size_t desc_sz) > { > struct ata_port *ap = dev->link->ap; > + struct pci_dev *pcidev = NULL; > + struct device *parent_dev = NULL; > int hdepth = 0, ddepth = ata_id_queue_depth(dev->id); > unsigned int err_mask; > char *aa_desc = ""; > @@ -2204,6 +2206,22 @@ static int ata_dev_config_ncq(struct ata_device *dev, > snprintf(desc, desc_sz, "NCQ (not used)"); > return 0; > } > + > + if (dev->horkage & ATA_HORKAGE_NONCQ_ON_AMD) { > + for (parent_dev = dev->tdev.parent; parent_dev != NULL; > + parent_dev = parent_dev->parent) { > + if (dev_is_pci(parent_dev)) { > + pcidev = to_pci_dev(parent_dev); > + if (pcidev->vendor == PCI_VENDOR_ID_AMD) { > + snprintf(desc, desc_sz, > + "NCQ (not used)"); > + return 0; > + } > + break; > + } > + } > + } > + > if (ap->flags & ATA_FLAG_NCQ) { > hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE); > dev->flags |= ATA_DFLAG_NCQ; > @@ -3971,9 +3989,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_AMD, }, > { "Samsung SSD 870*", NULL, ATA_HORKAGE_NO_NCQ_TRIM | > - ATA_HORKAGE_ZERO_AFTER_TRIM, }, > + ATA_HORKAGE_ZERO_AFTER_TRIM | > + ATA_HORKAGE_NONCQ_ON_AMD, }, > { "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 860e63f5667b..42e16114e91f 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -426,6 +426,7 @@ 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_AMD = (1 << 27), /* Disable NCQ on AMD chipset */ > > /* DMA mask for user DMA control: User visible values; DO NOT > renumber */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. 2021-08-30 14:42 ` [PATCH v3 1/1] " Kate Hsuan 2021-08-30 15:05 ` Hans de Goede @ 2021-08-30 21:58 ` Damien Le Moal 1 sibling, 0 replies; 6+ messages in thread From: Damien Le Moal @ 2021-08-30 21:58 UTC (permalink / raw) To: Kate Hsuan, Jens Axboe, Hans de Goede; +Cc: linux-ide, linux-kernel On 2021/08/30 23:43, Kate Hsuan wrote: > Many users are reporting that the Samsung 860 and 870 SSD are having > various issues when combined with AMD SATA controllers and only > completely disabling NCQ helps to avoid these issues. > > Entire disabling NCQ for Samsugn 860/870 SSD will cause I/O performance > drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to With "Entire disabling NCQ...", did you mean "Always disabling NCQ" ? If I understand this issue correctly, the explanation should be something like: Always disabling NCQ for Samsung 860/870 SSDs regardless of the host SATA adapter vendor will cause I/O performance degradation with well behaved adapters. To limit the performance impact to AMD adapters, introduce the ATA_HORKAGE_NO_NCQ_ON_AMD flag to force disable NCQ only for these adapters. > used to perform an additional check for these SSDs. If it finds it's > parent ATA controller is AMD, the NCQ will be disabled. Otherwise, the > NCQ is kept to enable. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=201693 > Signed-off-by: Kate Hsuan <hpa@redhat.com> > --- > drivers/ata/libata-core.c | 24 ++++++++++++++++++++++-- > include/linux/libata.h | 1 + > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index c861c93d1e84..36c62f758b73 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2190,6 +2190,8 @@ static int ata_dev_config_ncq(struct ata_device *dev, > char *desc, size_t desc_sz) > { > struct ata_port *ap = dev->link->ap; > + struct pci_dev *pcidev = NULL; > + struct device *parent_dev = NULL; > int hdepth = 0, ddepth = ata_id_queue_depth(dev->id); > unsigned int err_mask; > char *aa_desc = ""; > @@ -2204,6 +2206,22 @@ static int ata_dev_config_ncq(struct ata_device *dev, > snprintf(desc, desc_sz, "NCQ (not used)"); > return 0; > } > + > + if (dev->horkage & ATA_HORKAGE_NONCQ_ON_AMD) { > + for (parent_dev = dev->tdev.parent; parent_dev != NULL; > + parent_dev = parent_dev->parent) { > + if (dev_is_pci(parent_dev)) { > + pcidev = to_pci_dev(parent_dev); > + if (pcidev->vendor == PCI_VENDOR_ID_AMD) { > + snprintf(desc, desc_sz, > + "NCQ (not used)"); > + return 0; > + } > + break; > + } > + } It would be really nice to move this hunk into a small helper, something like: static bool ata_dev_check_adapter(struct ata_device *dev, unsigned short vendor_id) The "if" code block then becomes: if ((dev->horkage & ATA_HORKAGE_NONCQ_ON_AMD) && ata_dev_check_adapter(dev, PCI_VENDOR_ID_AMD)) { snprintf(desc, desc_sz, "NCQ (not used)"); return 0; } > + } > + > if (ap->flags & ATA_FLAG_NCQ) { > hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE); > dev->flags |= ATA_DFLAG_NCQ; > @@ -3971,9 +3989,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_AMD, }, > { "Samsung SSD 870*", NULL, ATA_HORKAGE_NO_NCQ_TRIM | > - ATA_HORKAGE_ZERO_AFTER_TRIM, }, > + ATA_HORKAGE_ZERO_AFTER_TRIM | > + ATA_HORKAGE_NONCQ_ON_AMD, }, > { "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 860e63f5667b..42e16114e91f 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -426,6 +426,7 @@ 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_AMD = (1 << 27), /* Disable NCQ on AMD chipset */ Please add a "_" after "NO", similarly to ATA_HORKAGE_NO_NCQ_TRIM: ATA_HORKAGE_NO_NCQ_ON_AMD = (1 << 27), /* Disable NCQ on AMD chipset */ > > /* DMA mask for user DMA control: User visible values; DO NOT > renumber */ > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. 2021-08-30 14:42 [PATCH v3 0/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD Kate Hsuan 2021-08-30 14:42 ` [PATCH v3 1/1] " Kate Hsuan @ 2021-08-30 21:45 ` Damien Le Moal 2021-08-31 7:38 ` Hans de Goede 1 sibling, 1 reply; 6+ messages in thread From: Damien Le Moal @ 2021-08-30 21:45 UTC (permalink / raw) To: Kate Hsuan, Jens Axboe, Hans de Goede; +Cc: linux-ide, linux-kernel On 2021/08/30 23:43, Kate Hsuan wrote: > Many users reported the issue when running the system with Samsung 860, > 870 SSD, and AMD chipset. Therefore, completely disabling the NCQ can > avoid this issue. > > Entire disabling NCQ for Samsung 860/870 SSD will cause I/O performance > drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to > used to perform an additional check for these SSDs. If it finds its parent > ATA controller is AMD, the NCQ will be disabled. Otherwise, the NCQ is kept > to enable. For a single patch, generally, a cover letter is not needed. Especially so in this case since your cover letter message is the same as the patch commit message. > > Changes since v3 > * Modified the flag from ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL to > ATA_HORKAGE_NONCQ_ON_AMD. > * Modified and fixed the code to completely disable NCQ on AMD controller. Technically, this is a v2 right ? Also, by "completely", did you mean "always" ? (see patch comments). > > > Kate Hsuan (1): > libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. > > drivers/ata/libata-core.c | 24 ++++++++++++++++++++++-- > include/linux/libata.h | 1 + > 2 files changed, 23 insertions(+), 2 deletions(-) > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD. 2021-08-30 21:45 ` [PATCH v3 0/1] " Damien Le Moal @ 2021-08-31 7:38 ` Hans de Goede 0 siblings, 0 replies; 6+ messages in thread From: Hans de Goede @ 2021-08-31 7:38 UTC (permalink / raw) To: Damien Le Moal, Kate Hsuan, Jens Axboe; +Cc: linux-ide, linux-kernel Hi, On 8/30/21 11:45 PM, Damien Le Moal wrote: > On 2021/08/30 23:43, Kate Hsuan wrote: >> Many users reported the issue when running the system with Samsung 860, >> 870 SSD, and AMD chipset. Therefore, completely disabling the NCQ can >> avoid this issue. >> >> Entire disabling NCQ for Samsung 860/870 SSD will cause I/O performance >> drop. In this case, a flag ATA_HORKAGE_NONCQ_ON_AMD is introduced to >> used to perform an additional check for these SSDs. If it finds its parent >> ATA controller is AMD, the NCQ will be disabled. Otherwise, the NCQ is kept >> to enable. > > For a single patch, generally, a cover letter is not needed. Especially so in > this case since your cover letter message is the same as the patch commit message. > >> >> Changes since v3 >> * Modified the flag from ATA_HORKAGE_NONCQ_ON_ASMEDIA_AMD_MARVELL to >> ATA_HORKAGE_NONCQ_ON_AMD. >> * Modified and fixed the code to completely disable NCQ on AMD controller. > > Technically, this is a v2 right ? Also, by "completely", did you mean "always" ? > (see patch comments). Right, as mentioned in my reply to the v1 which was accidentally labelled v2 I suggested to just make this v3 to avoid confusion, otherwise we would have 2 v2-s. Regards, Hans ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-31 7:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-30 14:42 [PATCH v3 0/1] libata: Add ATA_HORKAGE_NONCQ_ON_AMD for Samsung 860 and 870 SSD Kate Hsuan 2021-08-30 14:42 ` [PATCH v3 1/1] " Kate Hsuan 2021-08-30 15:05 ` Hans de Goede 2021-08-30 21:58 ` Damien Le Moal 2021-08-30 21:45 ` [PATCH v3 0/1] " Damien Le Moal 2021-08-31 7:38 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).