[PATCH] nvme: associate stripe size quirk with D4512

[PATCH] nvme: associate stripe size quirk with D4512

[David Fugate]

Intel's D4512 dual-port SSD is based on the older Intel P4500 SSDs
whose development predated the NOIOB feature. Based on a customer
request, the D4512's device ID was changed from the P4500s'. This
patch associates D4512's device ID with the stripe size quirk to
improve its performance.

Signed-off-by: David Fugate <david.fugate@linux.intel.com>
---
 drivers/nvme/host/pci.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5577c8cbb3e0..80afc8b3344d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3070,16 +3070,19 @@ static const struct pci_error_handlers nvme_err_handler = {
 };
 
 static const struct pci_device_id nvme_id_table[] = {
-	{ PCI_VDEVICE(INTEL, 0x0953),
+	{ PCI_VDEVICE(INTEL, 0x0953),	/* Intel 750/P3500/P3600/P3700 */
 		.driver_data = NVME_QUIRK_STRIPE_SIZE |
 				NVME_QUIRK_DEALLOCATE_ZEROES, },
-	{ PCI_VDEVICE(INTEL, 0x0a53),
+	{ PCI_VDEVICE(INTEL, 0x0a53),	/* Intel P3520 */
 		.driver_data = NVME_QUIRK_STRIPE_SIZE |
 				NVME_QUIRK_DEALLOCATE_ZEROES, },
-	{ PCI_VDEVICE(INTEL, 0x0a54),
+	{ PCI_VDEVICE(INTEL, 0x0a54),	/* Intel P4500/P4600 */
 		.driver_data = NVME_QUIRK_STRIPE_SIZE |
 				NVME_QUIRK_DEALLOCATE_ZEROES, },
-	{ PCI_VDEVICE(INTEL, 0x0a55),
+	{ PCI_VDEVICE(INTEL, 0x0a55),	/* Dell Express Flash P4600 */
+		.driver_data = NVME_QUIRK_STRIPE_SIZE |
+				NVME_QUIRK_DEALLOCATE_ZEROES, },
+	{ PCI_VDEVICE(INTEL, 0x0d54),	/* Intel D4512 */
 		.driver_data = NVME_QUIRK_STRIPE_SIZE |
 				NVME_QUIRK_DEALLOCATE_ZEROES, },
 	{ PCI_VDEVICE(INTEL, 0xf1a5),	/* Intel 600P/P3100 */
-- 
2.17.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[Christoph Hellwig]

On Wed, Jun 10, 2020 at 09:38:36PM -0600, David Fugate wrote:
> Intel's D4512 dual-port SSD is based on the older Intel P4500 SSDs
> whose development predated the NOIOB feature. Based on a customer
> request, the D4512's device ID was changed from the P4500s'. This
> patch associates D4512's device ID with the stripe size quirk to
> improve its performance.

NAK.  We've been told Intel forever that we need a standard quirk,
and we actually do have way to expose this information in Identify now.
Just kick your firmware engineers in the but to set the trivial field
in Identify.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[David Fugate]

On Thu, 2020-06-11 at 07:41 +0200, Christoph Hellwig wrote:
> On Wed, Jun 10, 2020 at 09:38:36PM -0600, David Fugate wrote:
> > Intel's D4512 dual-port SSD is based on the older Intel P4500 SSDs
> > whose development predated the NOIOB feature. Based on a customer
> > request, the D4512's device ID was changed from the P4500s'. This
> > patch associates D4512's device ID with the stripe size quirk to
> > improve its performance.
> 
> NAK.  We've been told Intel forever that we need a standard quirk,
> and we actually do have way to expose this information in Identify
> now.
> Just kick your firmware engineers in the but to set the trivial field
> in Identify.

Thanks for the feedback Christoph. Our FW engineer for this product is
currently on vacation, but I'll relay your suggestion to see if it's
viable.

In the meantime, I'm hearing your rejection for this is simply Intel
standardized the driver-assisted striping feature as NOIOB and failed
to realize it in any Intel product. Other than this, was there a
technical reason for rejecting this patch?

Thanks again!


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[Christoph Hellwig]

On Fri, Jun 12, 2020 at 08:43:15AM -0600, David Fugate wrote:
> On Thu, 2020-06-11 at 07:41 +0200, Christoph Hellwig wrote:
> > On Wed, Jun 10, 2020 at 09:38:36PM -0600, David Fugate wrote:
> > > Intel's D4512 dual-port SSD is based on the older Intel P4500 SSDs
> > > whose development predated the NOIOB feature. Based on a customer
> > > request, the D4512's device ID was changed from the P4500s'. This
> > > patch associates D4512's device ID with the stripe size quirk to
> > > improve its performance.
> > 
> > NAK.  We've been told Intel forever that we need a standard quirk,
> > and we actually do have way to expose this information in Identify
> > now.
> > Just kick your firmware engineers in the but to set the trivial field
> > in Identify.
> 
> Thanks for the feedback Christoph. Our FW engineer for this product is
> currently on vacation, but I'll relay your suggestion to see if it's
> viable.
> 
> In the meantime, I'm hearing your rejection for this is simply Intel
> standardized the driver-assisted striping feature as NOIOB and failed
> to realize it in any Intel product. Other than this, was there a
> technical reason for rejecting this patch?

If you look back in the archives we have been unhappy with this quirk for
years and thus helped to standardize it.  So we very much expect Intel to
use the standard way to any new product instead of adding more and more
quirks.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[David Fugate]

On Mon, 2020-06-15 at 09:20 +0200, Christoph Hellwig wrote:
> On Fri, Jun 12, 2020 at 08:43:15AM -0600, David Fugate wrote:
> > On Thu, 2020-06-11 at 07:41 +0200, Christoph Hellwig wrote:
> > > On Wed, Jun 10, 2020 at 09:38:36PM -0600, David Fugate wrote:
> > > > Intel's D4512 dual-port SSD is based on the older Intel P4500
> > > > SSDs
> > > > whose development predated the NOIOB feature. Based on a
> > > > customer
> > > > request, the D4512's device ID was changed from the P4500s'.
> > > > This
> > > > patch associates D4512's device ID with the stripe size quirk
> > > > to
> > > > improve its performance.
> > > 
> > > NAK.  We've been told Intel forever that we need a standard
> > > quirk,
> > > and we actually do have way to expose this information in
> > > Identify
> > > now.
> > > Just kick your firmware engineers in the but to set the trivial
> > > field
> > > in Identify.
> > 
> > Thanks for the feedback Christoph. Our FW engineer for this product
> > is
> > currently on vacation, but I'll relay your suggestion to see if
> > it's
> > viable.
> > 
> > In the meantime, I'm hearing your rejection for this is simply
> > Intel
> > standardized the driver-assisted striping feature as NOIOB and
> > failed
> > to realize it in any Intel product. Other than this, was there a
> > technical reason for rejecting this patch?
> 
> If you look back in the archives we have been unhappy with this quirk
> for
> years and thus helped to standardize it.  So we very much expect
> Intel to
> use the standard way to any new product instead of adding more and
> more
> quirks.

That's fair feedback. Did hear from our D4512 team there are concerns
with this drive advertising itself as NVMe 1.2 compliant while also
implementing aspects of NVMe 1.3 such as NOIOB. Of course the Linux
NVMe driver does the right thing in terms of ignoring the drive-
reported spec compliance value in favor of feature detection. Can't be
certain that's the case with all other OSes and closed-source drivers
in particular. No final word from that FW team yet. In the meantime,
I've submitted a separate patch to document marketing names of Intel
drives requiring these quirks.

BTW - Intel's recently announced P5500/P5600 product line implements
NOIOB per the spec. It's just these old controllers we're having
problems with.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[Keith Busch]

On Thu, Jul 02, 2020 at 03:16:04PM -0600, David Fugate wrote:
> Did hear from our D4512 team there are concerns with this drive
> advertising itself as NVMe 1.2 compliant while also implementing
> aspects of NVMe 1.3 such as NOIOB. Of course the Linux NVMe driver
> does the right thing in terms of ignoring the drive-reported spec
> compliance value in favor of feature detection. Can't be certain
> that's the case with all other OSes and closed-source drivers
> in particular.

I don't understand this concern. If a driver fails to detect a feature,
then they're in the exact same situation as if the controller didn't
implement it.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[David Fugate]

On Thu, 2020-07-02 at 14:48 -0700, Keith Busch wrote:
> On Thu, Jul 02, 2020 at 03:16:04PM -0600, David Fugate wrote:
> > Did hear from our D4512 team there are concerns with this drive
> > advertising itself as NVMe 1.2 compliant while also implementing
> > aspects of NVMe 1.3 such as NOIOB. Of course the Linux NVMe driver
> > does the right thing in terms of ignoring the drive-reported spec
> > compliance value in favor of feature detection. Can't be certain
> > that's the case with all other OSes and closed-source drivers
> > in particular.
> 
> I don't understand this concern. If a driver fails to detect a
> feature,
> then they're in the exact same situation as if the controller didn't
> implement it.

Imagine a less-enlightened NVMe driver which guards enablement of
features based *both* on the NVMe spec version the controller reports
as well as feature detection. In such cases, it wouldn't matter if a
drive implemented NOIOB (NVMe 1.3) if the controller advertises itself
as 1.2. E.g., same general type of version-checking logic as
implemented by nvme_report_ns_ids, but expanded to something that
actually *is* detectable as a feature. Linux does the right thing as
well as Intel-authored NVMe drivers I'm familiar with. Can't say this
for closed-source drivers though, and that was the concern given.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: associate stripe size quirk with D4512

[Keith Busch]

On Thu, Jul 02, 2020 at 04:26:21PM -0600, David Fugate wrote:
> On Thu, 2020-07-02 at 14:48 -0700, Keith Busch wrote:
> > On Thu, Jul 02, 2020 at 03:16:04PM -0600, David Fugate wrote:
> > > Did hear from our D4512 team there are concerns with this drive
> > > advertising itself as NVMe 1.2 compliant while also implementing
> > > aspects of NVMe 1.3 such as NOIOB. Of course the Linux NVMe driver
> > > does the right thing in terms of ignoring the drive-reported spec
> > > compliance value in favor of feature detection. Can't be certain
> > > that's the case with all other OSes and closed-source drivers
> > > in particular.
> > 
> > I don't understand this concern. If a driver fails to detect a
> > feature,
> > then they're in the exact same situation as if the controller didn't
> > implement it.
> 
> Imagine a less-enlightened NVMe driver which guards enablement of
> features based *both* on the NVMe spec version the controller reports
> as well as feature detection. In such cases, it wouldn't matter if a
> drive implemented NOIOB (NVMe 1.3) if the controller advertises itself
> as 1.2. 

That's exactly my point. It doesn't matter for these theoretical drivers
whether you implement the feature or not. Meanwhile, 5 years worth of
existing drivers can readily use it. You stand to gain something by
implementing it, and nothing to lose.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme