linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"Martin K . Petersen" <martin.petersen@oracle.com>
Subject: Re: [PATCH v2 1/1] libata: libata: add ATA_HORKAGE_NO_NCQ_TRIM for Samsung 860 and 870 SSDs
Date: Fri, 27 Aug 2021 06:04:09 +0000	[thread overview]
Message-ID: <CH2PR04MB70784A9A507CD9F3EA07986BE7C89@CH2PR04MB7078.namprd04.prod.outlook.com> (raw)
In-Reply-To: <20210827053344.15087-2-hpa@redhat.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

  reply	other threads:[~2021-08-27  6:04 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-27  5:33 [PATCH v2 0/1] " Kate Hsuan
2021-08-27  5:33 ` [PATCH v2 1/1] libata: " Kate Hsuan
2021-08-27  6:04   ` Damien Le Moal [this message]
2021-08-27  6:56   ` Greg KH
2021-08-27 10:36   ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CH2PR04MB70784A9A507CD9F3EA07986BE7C89@CH2PR04MB7078.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=hdegoede@redhat.com \
    --cc=hpa@redhat.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=stable@vger.kernel.org \
    --subject='Re: [PATCH v2 1/1] libata: libata: add ATA_HORKAGE_NO_NCQ_TRIM for Samsung 860 and 870 SSDs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).