[PATCH 0/4] misc: hpilo: Do not claim on unsupported hardware

[PATCH 0/4] misc: hpilo: Do not claim on unsupported hardware

[Matt Hsiao]

Changes:
1) Add additional subsystem_vendor check for SSID 0x1979.
2) Instead of having explicit if statement to check device IDs,
   provide a pci_device_id table of devices to blacklist.
3) Add new patch to add SSID 0x0289 to the blacklist table.
4) Bump version to reflect above minor changes.

Matt Hsiao (4):
  misc: hpilo: Be more specific when ignoring the aux iLO
  misc: hpilo: Exclude unsupported device via blacklist
  misc: hpilo: Do not claim unsupported hardware
  misc: hpilo: Update driver version

 drivers/misc/hpilo.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] misc: hpilo: Be more specific when ignoring the aux iLO

[Matt Hsiao]

Be more specific with the subsystem_vendor id used before iLO5

Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
---
 drivers/misc/hpilo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index e9c9ef5..01c407a 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -763,8 +763,9 @@ static int ilo_probe(struct pci_dev *pdev,
 	int devnum, minor, start, error = 0;
 	struct ilo_hwinfo *ilo_hw;
 
-	/* Ignore subsystem_device = 0x1979 (set by BIOS)  */
-	if (pdev->subsystem_device == 0x1979)
+	/* Ignore auxiliary iLO device */
+	if (pdev->subsystem_vendor == PCI_VENDOR_ID_HP &&
+	    pdev->subsystem_device == 0x1979)
 		return 0;
 
 	if (max_ccb > MAX_CCB)
-- 
1.8.3.1

[PATCH 2/4] misc: hpilo: Exclude unsupported device via blacklist

[Matt Hsiao]

Instead of having explicit if statments excluding devices,
use a pci_device_id table of devices to blacklist.

Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
---
 drivers/misc/hpilo.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 01c407a..0224e50 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -29,6 +29,11 @@
 static unsigned int ilo_major;
 static unsigned int max_ccb = 16;
 static char ilo_hwdev[MAX_ILO_DEV];
+static const struct pci_device_id ilo_blacklist[] = {
+	/* auxiliary iLO */
+	{PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP, 0x1979)},
+	{}
+};
 
 static inline int get_entry_id(int entry)
 {
@@ -763,10 +768,10 @@ static int ilo_probe(struct pci_dev *pdev,
 	int devnum, minor, start, error = 0;
 	struct ilo_hwinfo *ilo_hw;
 
-	/* Ignore auxiliary iLO device */
-	if (pdev->subsystem_vendor == PCI_VENDOR_ID_HP &&
-	    pdev->subsystem_device == 0x1979)
-		return 0;
+	if (pci_match_id(ilo_blacklist, pdev)) {
+		dev_dbg(&pdev->dev, "Not supported on this device\n");
+		return -ENODEV;
+	}
 
 	if (max_ccb > MAX_CCB)
 		max_ccb = MAX_CCB;
-- 
1.8.3.1

Re: [PATCH 2/4] misc: hpilo: Exclude unsupported device via blacklist

[Greg KH]

On Thu, Feb 21, 2019 at 04:04:40PM +0800, Matt Hsiao wrote:
> Instead of having explicit if statments excluding devices,
> use a pci_device_id table of devices to blacklist.
> 
> Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
> ---
>  drivers/misc/hpilo.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> index 01c407a..0224e50 100644
> --- a/drivers/misc/hpilo.c
> +++ b/drivers/misc/hpilo.c
> @@ -29,6 +29,11 @@
>  static unsigned int ilo_major;
>  static unsigned int max_ccb = 16;
>  static char ilo_hwdev[MAX_ILO_DEV];
> +static const struct pci_device_id ilo_blacklist[] = {
> +	/* auxiliary iLO */
> +	{PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP, 0x1979)},
> +	{}
> +};
>  
>  static inline int get_entry_id(int entry)
>  {
> @@ -763,10 +768,10 @@ static int ilo_probe(struct pci_dev *pdev,
>  	int devnum, minor, start, error = 0;
>  	struct ilo_hwinfo *ilo_hw;
>  
> -	/* Ignore auxiliary iLO device */
> -	if (pdev->subsystem_vendor == PCI_VENDOR_ID_HP &&
> -	    pdev->subsystem_device == 0x1979)
> -		return 0;
> +	if (pci_match_id(ilo_blacklist, pdev)) {
> +		dev_dbg(&pdev->dev, "Not supported on this device\n");
> +		return -ENODEV;
> +	}

Why not just merge this into the previous patch?

And why do some devices need to be blacklisted, shouldn't there only be
a whitelist in the first place?  Do you need to tighten up your original
device ids?

thanks,

greg k-h

Re: [PATCH 2/4] misc: hpilo: Exclude unsupported device via blacklist

[Jerry Hoemann]

On Thu, Feb 21, 2019 at 09:33:55AM +0100, Greg KH wrote:
> On Thu, Feb 21, 2019 at 04:04:40PM +0800, Matt Hsiao wrote:

> > +static const struct pci_device_id ilo_blacklist[] = {
> > +	/* auxiliary iLO */
> > +	{PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP, 0x1979)},
> > +	{}
> > +};
> >  

...

> 
> And why do some devices need to be blacklisted, shouldn't there only be
> a whitelist in the first place?  Do you need to tighten up your original
> device ids?

Hi Greg,

I related the underlying reason for the black listing on another message
of this thread.  I can fill you in on why we've taken this approach to
white/black listing.

HPE hardware/firmware teams will put out minor updates to the iLO using
the same device info except for the subsystem device id.

The approach we've taken in both the hpilo and hpwdt drivers is
to claim based upon {Vendor, PC DevID, SubVendor}.

This allows old software to work on new hardware without patching.

As our primary way to support our customers is via distros, this patching
when it does happen requires us to not just submit a patch upstream, but
to then to have the patches back ported to multiple releases of multiple
distros.  This process takes many many months.

So far, the approach we've taken has worked fairly well as this is only
the second time in 10+ years that we've needed to blacklist an instance.

Hope this helps.

Jerry

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH 2/4] misc: hpilo: Exclude unsupported device via blacklist

[Greg KH]

On Thu, Feb 21, 2019 at 09:35:07PM -0700, Jerry Hoemann wrote:
> On Thu, Feb 21, 2019 at 09:33:55AM +0100, Greg KH wrote:
> > On Thu, Feb 21, 2019 at 04:04:40PM +0800, Matt Hsiao wrote:
> 
> > > +static const struct pci_device_id ilo_blacklist[] = {
> > > +	/* auxiliary iLO */
> > > +	{PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP, 0x1979)},
> > > +	{}
> > > +};
> > >  
> 
> ...
> 
> > 
> > And why do some devices need to be blacklisted, shouldn't there only be
> > a whitelist in the first place?  Do you need to tighten up your original
> > device ids?
> 
> Hi Greg,
> 
> I related the underlying reason for the black listing on another message
> of this thread.  I can fill you in on why we've taken this approach to
> white/black listing.
> 
> HPE hardware/firmware teams will put out minor updates to the iLO using
> the same device info except for the subsystem device id.
> 
> The approach we've taken in both the hpilo and hpwdt drivers is
> to claim based upon {Vendor, PC DevID, SubVendor}.
> 
> This allows old software to work on new hardware without patching.
> 
> As our primary way to support our customers is via distros, this patching
> when it does happen requires us to not just submit a patch upstream, but
> to then to have the patches back ported to multiple releases of multiple
> distros.  This process takes many many months.
> 
> So far, the approach we've taken has worked fairly well as this is only
> the second time in 10+ years that we've needed to blacklist an instance.

Ok, that's fine, but you should put that information in the changelog
text so that we understand what is going on here.

thanks,

greg k-h

[PATCH 3/4] misc: hpilo: Do not claim unsupported hardware

[Matt Hsiao]

Do not claim when SSID 0x0289 as the iLO features
are not enabled/validated by the firmware.

Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
---
 drivers/misc/hpilo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 0224e50..927309b 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -32,6 +32,8 @@
 static const struct pci_device_id ilo_blacklist[] = {
 	/* auxiliary iLO */
 	{PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP, 0x1979)},
+	/* CL */
+	{PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP_3PAR, 0x0289)},
 	{}
 };
 
-- 
1.8.3.1

Re: [PATCH 3/4] misc: hpilo: Do not claim unsupported hardware

[Greg KH]

On Thu, Feb 21, 2019 at 04:04:41PM +0800, Matt Hsiao wrote:
> Do not claim when SSID 0x0289 as the iLO features
> are not enabled/validated by the firmware.

Can you put more information here, like _what_ hardware is not being
supported anymore?  As it is, this has nothing to do with "validation by
the firmware", you are just deciding to not support a device that
previously was supported by this driver.  As such you should provide
more information why you are taking away functionality.

thnaks,

greg k-h

Re: [PATCH 3/4] misc: hpilo: Do not claim unsupported hardware

[Jerry Hoemann]

On Thu, Feb 21, 2019 at 09:35:15AM +0100, Greg KH wrote:
> On Thu, Feb 21, 2019 at 04:04:41PM +0800, Matt Hsiao wrote:
> > Do not claim when SSID 0x0289 as the iLO features
> > are not enabled/validated by the firmware.
> 
> Can you put more information here, like _what_ hardware is not being
> supported anymore?  As it is, this has nothing to do with "validation by
> the firmware", you are just deciding to not support a device that
> previously was supported by this driver.  As such you should provide
> more information why you are taking away functionality.

Hi Greg,

The SSID 0x0289 correspond to the recent CL2600/CL2800 servers.  These servers
leveraged Proliant hardware but are targeted to a different market segment
with different requirements.  They also come with a different firmware base.

Based upon the targeted market needs, these server de-featured certain
aspects of Proliants with this being one.

As a result, the linux hpilo driver still claims the hardware but
is not functional.

We decided to blacklist the driver to reduce confusion to customers.

Matt will update the documentation in the second version of the patch set.

Hope this helps.

Take care,

Jerry

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

[PATCH 4/4] misc: hpilo: Update driver version

[Matt Hsiao]

Bump version number to reflect recent minor changes.

Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
---
 drivers/misc/hpilo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 927309b..4ef6802 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -913,7 +913,7 @@ static void __exit ilo_exit(void)
 	class_destroy(ilo_class);
 }
 
-MODULE_VERSION("1.5.0");
+MODULE_VERSION("1.5.1");
 MODULE_ALIAS(ILO_NAME);
 MODULE_DESCRIPTION(ILO_NAME);
 MODULE_AUTHOR("David Altobelli <david.altobelli@hpe.com>");
-- 
1.8.3.1

Re: [PATCH 4/4] misc: hpilo: Update driver version

[Greg KH]

On Thu, Feb 21, 2019 at 04:04:42PM +0800, Matt Hsiao wrote:
> Bump version number to reflect recent minor changes.
> 
> Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
> ---
>  drivers/misc/hpilo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> index 927309b..4ef6802 100644
> --- a/drivers/misc/hpilo.c
> +++ b/drivers/misc/hpilo.c
> @@ -913,7 +913,7 @@ static void __exit ilo_exit(void)
>  	class_destroy(ilo_class);
>  }
>  
> -MODULE_VERSION("1.5.0");
> +MODULE_VERSION("1.5.1");

This line means nothing, it should just be removed entirely.  The
"version" of the driver is the kernel version itself.

Want me to drop it?

thanks,

greg k-h

Re: [PATCH 4/4] misc: hpilo: Update driver version

[Jerry Hoemann]

On Thu, Feb 21, 2019 at 09:32:56AM +0100, Greg KH wrote:
...
> >  
> > -MODULE_VERSION("1.5.0");
> > +MODULE_VERSION("1.5.1");
> 
> This line means nothing, it should just be removed entirely.  The
> "version" of the driver is the kernel version itself.

Hi Greg,

This doesn't hold when we do driver updates.

Our primary means of supporting Linux to our customers is via our
distro partners.  While we prefer to use in distro drivers, HPE does
from time to time deliver driver updates via the "Service Pack for
Proliants" -- The SPP. 

An SPP driver update can supply an updated module without modifying
the underlying base kernel version.  Because of this, the underlying
kernel version number doesn't always imply the version of a module
being used.

Even when we use only in kernel drivers, we also have the case where
our distro partners will back port newer driver versions to an older
distro release.  This gives the situation where an older kernel
version can have a newer module than a newer kernel version for
that distro.

We have found that having module version bumped when modifying the
driver helps us identify the version module actually being used.

Take care,

Jerry

> 
> Want me to drop it?

No. please keep it.  :)

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH 4/4] misc: hpilo: Update driver version

[Greg KH]

On Thu, Feb 21, 2019 at 09:11:11PM -0700, Jerry Hoemann wrote:
> On Thu, Feb 21, 2019 at 09:32:56AM +0100, Greg KH wrote:
> ...
> > >  
> > > -MODULE_VERSION("1.5.0");
> > > +MODULE_VERSION("1.5.1");
> > 
> > This line means nothing, it should just be removed entirely.  The
> > "version" of the driver is the kernel version itself.
> 
> Hi Greg,
> 
> This doesn't hold when we do driver updates.

That doesn't matter to the in-kernel code.

> Our primary means of supporting Linux to our customers is via our
> distro partners.  While we prefer to use in distro drivers, HPE does
> from time to time deliver driver updates via the "Service Pack for
> Proliants" -- The SPP. 

That's fine, but again, does not matter to the in-kernel driver at all.

> An SPP driver update can supply an updated module without modifying
> the underlying base kernel version.  Because of this, the underlying
> kernel version number doesn't always imply the version of a module
> being used.
> 
> Even when we use only in kernel drivers, we also have the case where
> our distro partners will back port newer driver versions to an older
> distro release.  This gives the situation where an older kernel
> version can have a newer module than a newer kernel version for
> that distro.
> 
> We have found that having module version bumped when modifying the
> driver helps us identify the version module actually being used.

What happens when your driver gets backports in stable kernel updates
and those updates get merged into distro kernels?  You now have a
version that means something you do not think it means.

I understand that in your viewpoint, your driver's version means
something.  But in reality, it's only the kernel's version that means
something because your driver is just part of the overall kernel, it
does not stand alone.

Your driver is simple enough that the version number doesn't really
change often and the code doesn't either, so you haven't hit these
issues yet, but for others that have tried to manage a "which version is
this driver" when dealing with backports, stable kernels, out-of-tree
drivers, and the like, it really is meaningless.

For this reason, this macro has been removed from many subsystems
already, I forgot about "misc", I might as well sweep it and do it
there too.

If you have an out-of-tree version that you provide to distros, you are
of course free to have the "version" in there if you like, but again,
for the in-kernel version of this code, it does not matter at all.

thanks,

greg k-h

Re: [PATCH 4/4] misc: hpilo: Update driver version

[Jerry Hoemann]

On Fri, Feb 22, 2019 at 07:49:28AM +0100, Greg KH wrote:
> On Thu, Feb 21, 2019 at 09:11:11PM -0700, Jerry Hoemann wrote:
> 
> > Our primary means of supporting Linux to our customers is via our
> > distro partners.  While we prefer to use in distro drivers, HPE does
> > from time to time deliver driver updates via the "Service Pack for
> > Proliants" -- The SPP. 
> 
> That's fine, but again, does not matter to the in-kernel driver at all.

Your point?  No one claimed that changing the version number
of the module changes its functionality.  We're changing the driver
version number to reflect that the driver's functionality changed.

We do this to help determine the version running on a system
in the event we have problems.  It's a support issue.

> I understand that in your viewpoint, your driver's version means
> something.  But in reality, it's only the kernel's version that means
> something because your driver is just part of the overall kernel, it
> does not stand alone.

I never claimed a driver stood alone.  jeezz.

When you say kernel "version", are you trying to say that the version
string printed by the kernel determines the source of the drivers?
(I ask, because I have heard other maintainers make this claim.)

The kernel version string only reliably determines the base kernel build.
Modules can be unloaded and replaced by totally new versions drastically
different from the version that existed at the time of the base kernel build.

The delivery of drivers updates independent of base kernel was old
practice when I started Unix development 30 years ago.  It was not unique
to HPE then or now.  I don't see it stopping.

So while Linux delivers drivers built to a baseline kernel build, we
cannot rely that the bits being used on a system still reflect that initial
install.  We can't just assume a driver version.  And without knowing
the driver version, it makes support more difficult.

If you're trying to be profound, the "version" of the OS running is
more than just the base kernel.  That is only the beginning.
We then have to consider the modules loaded and the order that they're
loaded.  This sequence is unbounded as modules can be repeatedly loaded
and unloaded.  When you know that, then you know the "version" of the kernel
running.

But that isn't what the kernel version string gives you.  So why have it/print it?
By your reasoning it's meaningless. It should be tossed.  We have it because
it gives us info, but it is only a start.


-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH 4/4] misc: hpilo: Update driver version

[Greg KH]

On Mon, Feb 25, 2019 at 01:28:05AM -0700, Jerry Hoemann wrote:
> On Fri, Feb 22, 2019 at 07:49:28AM +0100, Greg KH wrote:
> > On Thu, Feb 21, 2019 at 09:11:11PM -0700, Jerry Hoemann wrote:
> > 
> > > Our primary means of supporting Linux to our customers is via our
> > > distro partners.  While we prefer to use in distro drivers, HPE does
> > > from time to time deliver driver updates via the "Service Pack for
> > > Proliants" -- The SPP. 
> > 
> > That's fine, but again, does not matter to the in-kernel driver at all.
> 
> Your point?  No one claimed that changing the version number
> of the module changes its functionality.  We're changing the driver
> version number to reflect that the driver's functionality changed.
> 
> We do this to help determine the version running on a system
> in the event we have problems.  It's a support issue.

You last touched that version number in 2016, despite there being
changes made to the driver in the _years_ since then.  So if someone
called in and said, "I have a problem with version "1.5.0", you really
have no idea what that code is.

That shows to me that the version field in the driver means nothing, so
it should be removed.

> > I understand that in your viewpoint, your driver's version means
> > something.  But in reality, it's only the kernel's version that means
> > something because your driver is just part of the overall kernel, it
> > does not stand alone.
> 
> I never claimed a driver stood alone.  jeezz.
> 
> When you say kernel "version", are you trying to say that the version
> string printed by the kernel determines the source of the drivers?
> (I ask, because I have heard other maintainers make this claim.)

Yes.

> The kernel version string only reliably determines the base kernel build.
> Modules can be unloaded and replaced by totally new versions drastically
> different from the version that existed at the time of the base kernel build.

Sure, and if you do that you are on your own, feel free to put what ever
string you want in your external module source code.

Note, you will know that this is an "external" module, the kernel does
tell you that you did this, it is not silent at all.

> The delivery of drivers updates independent of base kernel was old
> practice when I started Unix development 30 years ago.  It was not unique
> to HPE then or now.  I don't see it stopping.

The old model of detaching drivers from the operating system is not at
play here.  When you had different distribution channels, trying to have
a version number made sense to try to get a grip on what is running
where.  That's not the case with Linux, and hasn't been for the past 20+
years.

Linux is distributed as a "whole", kernel+drivers, they are directly
tied together and are one body of work.  Yes, you can have external
modules, but that is not the normal method of operation and one I could
care less about here.  All I care about is that our tree works properly.

And as such, I will continue to state that an individual driver version
number means nothing, as it is the actual version of the kernel itself
that actually means something.

For drivers that actually have active development (unlike this one), it
is very simple to see how the module version gets out of whack.  Take
one patch that goes into the main kernel tree, and have that backported
to the stable and LTS kernels, and all of a sudden your "version number"
means nothing, as the version number of the stable kernel's copy of the
driver did not change.  And you really can't bump it to a different
number from the main version, so what do you do?  Try to come up with
some other intermediate number?  That's not ok as you can't keep up with
that numbering scheme as all that really matters is the kernel version
number itself!

Anyway, as this driver is obviously not under development at all, and
the changes that have happened since 2016 never caused you to change the
number, I'm not going to take this patch, sorry.

greg k-h