[PATCH v1 1/2] nvme-pci: Use single IRQ vector for old Apple models

[PATCH v1 1/2] nvme-pci: Use single IRQ vector for old Apple models

[Andy Shevchenko]

People reported that old Apple machines are not working properly
if the non-first IRQ vector is in use.

Set quirk for that models to limit IRQ to use first vector only.

Based on original patch by GitHub user npx001.

Link: https://github.com/Dunedan/mbp-2016-linux/issues/9
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Leif Liddy <leif.liddy@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/nvme/host/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index da392b50f73e..2c1c64a12024 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3096,7 +3096,8 @@ static const struct pci_device_id nvme_id_table[] = {
 		.driver_data = NVME_QUIRK_NO_DEEPEST_PS |
 				NVME_QUIRK_IGNORE_DEV_SUBNQN, },
 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001),
+		.driver_data = NVME_QUIRK_SINGLE_VECTOR },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
 		.driver_data = NVME_QUIRK_SINGLE_VECTOR |
-- 
2.25.0


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

[PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Andy Shevchenko]

Like for Intel devices use PCI_VDEVICE() macro to describe
Apple devices.

Besides that it's a luck that we got devices enumerated when
they are listed after class matching entry. So, move them above
and leave class matching entry the last in the ID table.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Leif Liddy <leif.liddy@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/nvme/host/pci.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 2c1c64a12024..b02951b19990 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3095,14 +3095,17 @@ static const struct pci_device_id nvme_id_table[] = {
 	{ PCI_DEVICE(0x1cc1, 0x8201),   /* ADATA SX8200PNP 512GB */
 		.driver_data = NVME_QUIRK_NO_DEEPEST_PS |
 				NVME_QUIRK_IGNORE_DEV_SUBNQN, },
-	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001),
-		.driver_data = NVME_QUIRK_SINGLE_VECTOR },
-	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
+
+	/* Apple devices */
+	{ PCI_VDEVICE(APPLE, 0x2001),
+		.driver_data = NVME_QUIRK_SINGLE_VECTOR, },
+	{ PCI_VDEVICE(APPLE, 0x2003), },
+	{ PCI_VDEVICE(APPLE, 0x2005),
 		.driver_data = NVME_QUIRK_SINGLE_VECTOR |
 				NVME_QUIRK_128_BYTES_SQES |
-				NVME_QUIRK_SHARED_TAGS },
+				NVME_QUIRK_SHARED_TAGS, },
+
+	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, nvme_id_table);
-- 
2.25.0


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

Re: [PATCH v1 1/2] nvme-pci: Use single IRQ vector for old Apple models

[Christoph Hellwig]

On Wed, Feb 12, 2020 at 12:32:18PM +0200, Andy Shevchenko wrote:
> People reported that old Apple machines are not working properly
> if the non-first IRQ vector is in use.
> 
> Set quirk for that models to limit IRQ to use first vector only.
> 
> Based on original patch by GitHub user npx001.

Shouldn't that be a reported by?  And do we have a real name / email
address?

The patch itself looks fine.

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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Christoph Hellwig]

On Wed, Feb 12, 2020 at 12:32:19PM +0200, Andy Shevchenko wrote:
> Like for Intel devices use PCI_VDEVICE() macro to describe
> Apple devices.

This actually makes the code longer and adds the antipattern of macros
that use string pasting on their argument, and this breaking grep-ability
badly, so NAK.

> Besides that it's a luck that we got devices enumerated when
> they are listed after class matching entry. So, move them above
> and leave class matching entry the last in the ID table.

It isn't by luck.  If the Apple devices were using the proper NVMe
class ID we'd never hit the apple specific entries, but the actually
use a different class code (the nvme one in big endian IIRC).  That
being said I'm all for having the class match last.

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

Re: [PATCH v1 1/2] nvme-pci: Use single IRQ vector for old Apple models

[Andy Shevchenko]

On Wed, Feb 12, 2020 at 06:37:09PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 12, 2020 at 12:32:18PM +0200, Andy Shevchenko wrote:
> > People reported that old Apple machines are not working properly
> > if the non-first IRQ vector is in use.
> > 
> > Set quirk for that models to limit IRQ to use first vector only.
> > 
> > Based on original patch by GitHub user npx001.
> 
> Shouldn't that be a reported by?  And do we have a real name / email
> address?

If I would know who is behind that...
I'll post a link to this patch to ask people test / tag it.

> The patch itself looks fine.

Thanks for review.

-- 
With Best Regards,
Andy Shevchenko



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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Andy Shevchenko]

On Wed, Feb 12, 2020 at 06:39:01PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 12, 2020 at 12:32:19PM +0200, Andy Shevchenko wrote:
> > Like for Intel devices use PCI_VDEVICE() macro to describe
> > Apple devices.
> 
> This actually makes the code longer

I didn't get this. How? The code is for sure shorter.

> and adds the antipattern of macros
> that use string pasting on their argument,

Anti-pattern to what? Can you elaborate a bit more? Sounds like I missed some
very basic thing.

> 	and this breaking grep-ability > badly, so NAK.

This like a mantra people are telling, but looks like simple they didn't try.
What grep issue do you see in this case (I would agree if we talk about device
ID, though)?

Are you going to remove PCI_VDEVICE() completely? Are you going to be
consistent with the rest in this driver?

This patch (first part of it) is to bring some consistency to this table. I
don't see how it may thing worse than they are already.

> > Besides that it's a luck that we got devices enumerated when
> > they are listed after class matching entry. So, move them above
> > and leave class matching entry the last in the ID table.
> 
> It isn't by luck.  If the Apple devices were using the proper NVMe
> class ID we'd never hit the apple specific entries, but the actually
> use a different class code (the nvme one in big endian IIRC).  That
> being said I'm all for having the class match last.

Nobody prevents Apple to fix this and re-use old ID. So, under luck we may
consider Apple's negligence to the standards.

-- 
With Best Regards,
Andy Shevchenko



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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Keith Busch]

On Wed, Feb 12, 2020 at 10:34:18PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 12, 2020 at 06:39:01PM +0100, Christoph Hellwig wrote:
> > On Wed, Feb 12, 2020 at 12:32:19PM +0200, Andy Shevchenko wrote:
> > > Like for Intel devices use PCI_VDEVICE() macro to describe
> > > Apple devices.
> > 
> > This actually makes the code longer
> 
> I didn't get this. How? The code is for sure shorter.

What metric are we using? The diff stat says longer.

  drivers/nvme/host/pci.c | 15 +++++++++------
  1 file changed, 9 insertions(+), 6 deletions(-)

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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Andy Shevchenko]

On Thu, Feb 13, 2020 at 05:40:00AM +0900, Keith Busch wrote:
> On Wed, Feb 12, 2020 at 10:34:18PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2020 at 06:39:01PM +0100, Christoph Hellwig wrote:
> > > On Wed, Feb 12, 2020 at 12:32:19PM +0200, Andy Shevchenko wrote:
> > > > Like for Intel devices use PCI_VDEVICE() macro to describe
> > > > Apple devices.
> > > 
> > > This actually makes the code longer
> > 
> > I didn't get this. How? The code is for sure shorter.
> 
> What metric are we using? The diff stat says longer.

I actually read *the code*. Blank lines and comments are being added for better
grouping.

Since you cut off the patch itself I can't simple point out what exactly has
been done.

>   drivers/nvme/host/pci.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)

-- 
With Best Regards,
Andy Shevchenko



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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Keith Busch]

On Thu, Feb 13, 2020 at 10:52:15AM +0200, Andy Shevchenko wrote:
> On Thu, Feb 13, 2020 at 05:40:00AM +0900, Keith Busch wrote:
> > On Wed, Feb 12, 2020 at 10:34:18PM +0200, Andy Shevchenko wrote:
> > > On Wed, Feb 12, 2020 at 06:39:01PM +0100, Christoph Hellwig wrote:
> > > > On Wed, Feb 12, 2020 at 12:32:19PM +0200, Andy Shevchenko wrote:
> > > > > Like for Intel devices use PCI_VDEVICE() macro to describe
> > > > > Apple devices.
> > > > 
> > > > This actually makes the code longer
> > > 
> > > I didn't get this. How? The code is for sure shorter.
> > 
> > What metric are we using? The diff stat says longer.
> 
> I actually read *the code*. Blank lines and comments are being added for better
> grouping.
> 
> Since you cut off the patch itself I can't simple point out what exactly has
> been done.

Well, including it here would be irrelevant to "the code is for sure
shorter" when we can empirically measure *that* point. I'm not the one
who made this about long vs short in the first place, but if you want
to switch the discussion to the content, then great. I personally value
clarity over brevity anyway.

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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Christoph Hellwig]

On Wed, Feb 12, 2020 at 10:34:18PM +0200, Andy Shevchenko wrote:
> > This actually makes the code longer
> 
> I didn't get this. How? The code is for sure shorter.

Looks at the diffstat.

> 
> > and adds the antipattern of macros
> > that use string pasting on their argument,
> 
> Anti-pattern to what? Can you elaborate a bit more? Sounds like I missed some
> very basic thing.

> > 	and this breaking grep-ability > badly, so NAK.
> 
> This like a mantra people are telling, but looks like simple they didn't try.
> What grep issue do you see in this case (I would agree if we talk about device
> ID, though)?

If I want to grep for PCI_VENDOR_ID_APPLE I find the ids in the old code.
I won't find them in your new code.

> Are you going to remove PCI_VDEVICE() completely? Are you going to be
> consistent with the rest in this driver?

If you care strongly about using the same macro please send a patch to
remove it in nvme.  I certainly don't want the sane version switched
over to it.

> > It isn't by luck.  If the Apple devices were using the proper NVMe
> > class ID we'd never hit the apple specific entries, but the actually
> > use a different class code (the nvme one in big endian IIRC).  That
> > being said I'm all for having the class match last.
> 
> Nobody prevents Apple to fix this and re-use old ID. So, under luck we may
> consider Apple's negligence to the standards.

I'm just saying the current atch is not by luck.  As said in the previous
mail I'm fine with moving the class match to the end as it generally
is a good idea.  I just disagree with the "works by luck" statement for
the current entries.

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

Re: [PATCH v1 1/2] nvme-pci: Use single IRQ vector for old Apple models

[Keith Busch]

Thanks, applied for the next 5.6 pull request.

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

Re: [PATCH v1 2/2] nvme-pci: Convert to PCI_VDEVICE() macro for Apple devices

[Andy Shevchenko]

On Wed, Feb 19, 2020 at 04:06:25PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 12, 2020 at 10:34:18PM +0200, Andy Shevchenko wrote:
> > > This actually makes the code longer
> > 
> > I didn't get this. How? The code is for sure shorter.
> 
> Looks at the diffstat.

Yes, but let's calculate a real code (not all lines).
In terms of characters it's shorter, in terms of LOCs it's the same.
On top few comment / blank lines were added.

> > > and adds the antipattern of macros
> > > that use string pasting on their argument,
> > 
> > Anti-pattern to what? Can you elaborate a bit more? Sounds like I missed some
> > very basic thing.
> 
> > > 	and this breaking grep-ability > badly, so NAK.
> > 
> > This like a mantra people are telling, but looks like simple they didn't try.
> > What grep issue do you see in this case (I would agree if we talk about device
> > ID, though)?
> 
> If I want to grep for PCI_VENDOR_ID_APPLE I find the ids in the old code.
> I won't find them in your new code.

But it's matter of writing good enough RE, right?
What about, as rough approximation, `git grep -n 'PCI_[^,]\+APPLE'`?

> > Are you going to remove PCI_VDEVICE() completely? Are you going to be
> > consistent with the rest in this driver?
> 
> If you care strongly about using the same macro please send a patch to
> remove it in nvme.  I certainly don't want the sane version switched
> over to it.

Perhaps in the future, but no guarantees I find a time for that soon.

> > > It isn't by luck.  If the Apple devices were using the proper NVMe
> > > class ID we'd never hit the apple specific entries, but the actually
> > > use a different class code (the nvme one in big endian IIRC).  That
> > > being said I'm all for having the class match last.
> > 
> > Nobody prevents Apple to fix this and re-use old ID. So, under luck we may
> > consider Apple's negligence to the standards.
> 
> I'm just saying the current atch is not by luck.  As said in the previous
> mail I'm fine with moving the class match to the end as it generally
> is a good idea.  I just disagree with the "works by luck" statement for
> the current entries.

Thanks, I will send a new patch just for that.

-- 
With Best Regards,
Andy Shevchenko



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