[PATCH] scsi: lpfc: Add shutdown method for kexec

[PATCH] scsi: lpfc: Add shutdown method for kexec

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

We see lpfc devices regularly fail during kexec. Fix this by adding
a shutdown method which mirrors the remove method.

Signed-off-by: Anton Blanchard <anton@samba.org>
---
 drivers/scsi/lpfc/lpfc_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 4776fd8..10f75ad 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -11447,6 +11447,7 @@ static struct pci_driver lpfc_driver = {
 	.id_table	= lpfc_id_table,
 	.probe		= lpfc_pci_probe_one,
 	.remove		= lpfc_pci_remove_one,
+	.shutdown	= lpfc_pci_remove_one,
 	.suspend        = lpfc_pci_suspend_one,
 	.resume		= lpfc_pci_resume_one,
 	.err_handler    = &lpfc_err_handler,
-- 
2.9.3

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Benjamin Herrenschmidt]

On Mon, 2017-02-13 at 08:49 +1100, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> We see lpfc devices regularly fail during kexec. Fix this by adding
> a shutdown method which mirrors the remove method.

Or instead finally do what I've been advocating for years (and even
sent patches for) which is to have kexec call remove instead of
shutdown.

Shutdown is and has *always* been the wrong thing to do.

> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>  drivers/scsi/lpfc/lpfc_init.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_init.c
> b/drivers/scsi/lpfc/lpfc_init.c
> index 4776fd8..10f75ad 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -11447,6 +11447,7 @@ static struct pci_driver lpfc_driver = {
>  	.id_table	= lpfc_id_table,
>  	.probe		= lpfc_pci_probe_one,
>  	.remove		= lpfc_pci_remove_one,
> +	.shutdown	= lpfc_pci_remove_one,
>  	.suspend        = lpfc_pci_suspend_one,
>  	.resume		= lpfc_pci_resume_one,
>  	.err_handler    = &lpfc_err_handler,

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Anton Blanchard]

Hi Ben,

On Mon, 13 Feb 2017 10:14:54 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Mon, 2017-02-13 at 08:49 +1100, Anton Blanchard wrote:
> > From: Anton Blanchard <anton@samba.org>
> > 
> > We see lpfc devices regularly fail during kexec. Fix this by adding
> > a shutdown method which mirrors the remove method.  
> 
> Or instead finally do what I've been advocating for years (and even
> sent patches for) which is to have kexec call remove instead of
> shutdown.
>
> Shutdown is and has *always* been the wrong thing to do.

Good point, at the very least we should call remove if shutdown doesn't
exist. Eric: could we make the changes Ben suggests?

Anton

> > Signed-off-by: Anton Blanchard <anton@samba.org>
> > ---
> >  drivers/scsi/lpfc/lpfc_init.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/scsi/lpfc/lpfc_init.c
> > b/drivers/scsi/lpfc/lpfc_init.c
> > index 4776fd8..10f75ad 100644
> > --- a/drivers/scsi/lpfc/lpfc_init.c
> > +++ b/drivers/scsi/lpfc/lpfc_init.c
> > @@ -11447,6 +11447,7 @@ static struct pci_driver lpfc_driver = {
> >  	.id_table	= lpfc_id_table,
> >  	.probe		= lpfc_pci_probe_one,
> >  	.remove		= lpfc_pci_remove_one,
> > +	.shutdown	= lpfc_pci_remove_one,
> >  	.suspend        = lpfc_pci_suspend_one,
> >  	.resume		= lpfc_pci_resume_one,
> >  	.err_handler    = &lpfc_err_handler,  
> 

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Eric W. Biederman]

Anton Blanchard <anton@samba.org> writes:

> Hi Ben,
>
> On Mon, 13 Feb 2017 10:14:54 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> On Mon, 2017-02-13 at 08:49 +1100, Anton Blanchard wrote:
>> > From: Anton Blanchard <anton@samba.org>
>> > 
>> > We see lpfc devices regularly fail during kexec. Fix this by adding
>> > a shutdown method which mirrors the remove method.  
>> 
>> Or instead finally do what I've been advocating for years (and even
>> sent patches for) which is to have kexec call remove instead of
>> shutdown.

Ben what happened to those patches.  I don't remember them?

>> Shutdown is and has *always* been the wrong thing to do.

I won't argue that.

> Good point, at the very least we should call remove if shutdown doesn't
> exist. Eric: could we make the changes Ben suggests?

Definitely.  That was the original design of the kexec interface
but people were worried about calling remove during reboot might
introduce regressions on the reboot functionality.  So shutdown was
added to be remove without the cleaning up the kernel data structures.

I am all for changing the core to call remove.  That seems to be a more
tested code path (because remove tends to be part of the development
path for modules) and thus much more likely to work in practice.

The challenge with changes in this area is that when the infrastructure
is changed for everyone someone needs to baby sit it until all of the
unexpected issues are resolved.  I was hoping a couple of years ago that
Ben could be that person.

Eric

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Benjamin Herrenschmidt]

On Mon, 2017-02-13 at 13:21 +1300, Eric W. Biederman wrote:
> > Good point, at the very least we should call remove if shutdown doesn't
> > exist. Eric: could we make the changes Ben suggests?
> 
> Definitely.  That was the original design of the kexec interface
> but people were worried about calling remove during reboot might
> introduce regressions on the reboot functionality.  So shutdown was
> added to be remove without the cleaning up the kernel data structures.

Right. Part of the problem was also that remove was dependent on CONFIG_HOTPLUG
though that's no longer the case anymore.

The problem is that a bunch of drivers either don't have a shutdown or
worse, have one that actually shuts the HW down rather than "idle" it
which puts it in a state that the new kernel can't deal with.

> I am all for changing the core to call remove.  That seems to be a more
> tested code path (because remove tends to be part of the development
> path for modules) and thus much more likely to work in practice.
> 
> The challenge with changes in this area is that when the infrastructure
> is changed for everyone someone needs to baby sit it until all of the
> unexpected issues are resolved.  I was hoping a couple of years ago that
> Ben could be that person.

Correct. And I lack time. But since we are a platform that uses kexec
daily as our main booting mechanism we should probably be the ones
driving that.

I had a patch at least to fallback to remove in absence of shutdown
which I can try to dig out.

We can add a config option to make it do the other way around that
we should start testing internally. I'm sure we will find 1 or 2
regressions as drivers do weird things.

Cheers,
Ben.

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Brian King]

On 02/12/2017 07:01 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2017-02-13 at 13:21 +1300, Eric W. Biederman wrote:
>>> Good point, at the very least we should call remove if shutdown doesn't
>>> exist. Eric: could we make the changes Ben suggests?
>>
>> Definitely.  That was the original design of the kexec interface
>> but people were worried about calling remove during reboot might
>> introduce regressions on the reboot functionality.  So shutdown was
>> added to be remove without the cleaning up the kernel data structures.
> 
> Right. Part of the problem was also that remove was dependent on CONFIG_HOTPLUG
> though that's no longer the case anymore.
> 
> The problem is that a bunch of drivers either don't have a shutdown or
> worse, have one that actually shuts the HW down rather than "idle" it
> which puts it in a state that the new kernel can't deal with.

If we do transition to use remove rather than shutdown, I think we want
some way for a device driver to know whether we are doing kexec or not.
A RAID adapter with a write cache is going to want to flush its write
cache on a PCI hotplug remove, but for a kexec, its going to want to skip
that so the kexec is faster. Today, since kexec looks like a reboot,
rather than a shutdown, we can skip the flush on a reboot, since its
technically not needed there either.

-Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Benjamin Herrenschmidt]

On Mon, 2017-02-13 at 15:57 -0600, Brian King wrote:
> If we do transition to use remove rather than shutdown, I think we
> want
> some way for a device driver to know whether we are doing kexec or
> not.
> A RAID adapter with a write cache is going to want to flush its write
> cache on a PCI hotplug remove, but for a kexec, its going to want to
> skip
> that so the kexec is faster. Today, since kexec looks like a reboot,
> rather than a shutdown, we can skip the flush on a reboot, since its
> technically not needed there either.

What happens if a non-flushed adapter gets a PERST ?

I wouldn't trust that 'don't have to flush' magic ...

Cheers,
Ben.

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Eric W. Biederman]

Brian King <brking@linux.vnet.ibm.com> writes:

> On 02/12/2017 07:01 PM, Benjamin Herrenschmidt wrote:
>> On Mon, 2017-02-13 at 13:21 +1300, Eric W. Biederman wrote:
>>>> Good point, at the very least we should call remove if shutdown doesn't
>>>> exist. Eric: could we make the changes Ben suggests?
>>>
>>> Definitely.  That was the original design of the kexec interface
>>> but people were worried about calling remove during reboot might
>>> introduce regressions on the reboot functionality.  So shutdown was
>>> added to be remove without the cleaning up the kernel data structures.
>> 
>> Right. Part of the problem was also that remove was dependent on CONFIG_HOTPLUG
>> though that's no longer the case anymore.
>> 
>> The problem is that a bunch of drivers either don't have a shutdown or
>> worse, have one that actually shuts the HW down rather than "idle" it
>> which puts it in a state that the new kernel can't deal with.
>
> If we do transition to use remove rather than shutdown, I think we want
> some way for a device driver to know whether we are doing kexec or not.
> A RAID adapter with a write cache is going to want to flush its write
> cache on a PCI hotplug remove, but for a kexec, its going to want to skip
> that so the kexec is faster. Today, since kexec looks like a reboot,
> rather than a shutdown, we can skip the flush on a reboot, since its
> technically not needed there either.

This doesn't make any sense.

With a PCI hotplug remove you can't do anything because by the time you
get the event the hardware is gone.  I think there are optional
interlocks but nothing as good as the old 3.5" floppy disk button, or
the button on CD drives.

The only difference ever that should exist between shutdown and remove
is do you clean up kernel data structures.  The shutdown method is
allowed to skip the cleanup up kernel data structures that the remove
method needs to make.

Assuming the kernel does not have corrupted data structures I don't see
any practical reason for distinguishing between the two.  There may be
some real world gotchas we have to deal with but semantically I don't
see any concerns.

Eric

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Benjamin Herrenschmidt]

On Tue, 2017-02-14 at 15:45 +1300, Eric W. Biederman wrote:
> The only difference ever that should exist between shutdown and remove
> is do you clean up kernel data structures.  The shutdown method is
> allowed to skip the cleanup up kernel data structures that the remove
> method needs to make.
> 
> Assuming the kernel does not have corrupted data structures I don't see
> any practical reason for distinguishing between the two.  There may be
> some real world gotchas we have to deal with but semantically I don't
> see any concerns.

We had historical additions to shutdown actually shutting things down,
like spinning platters down to park disk heads, switching devices
into D3 on some systems, etc... that remove never had (and we don't
want for kexec).

Cheers,
Ben.

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Brian King]

On 02/13/2017 08:04 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2017-02-13 at 15:57 -0600, Brian King wrote:
>> If we do transition to use remove rather than shutdown, I think we
>> want
>> some way for a device driver to know whether we are doing kexec or
>> not.
>> A RAID adapter with a write cache is going to want to flush its write
>> cache on a PCI hotplug remove, but for a kexec, its going to want to
>> skip
>> that so the kexec is faster. Today, since kexec looks like a reboot,
>> rather than a shutdown, we can skip the flush on a reboot, since its
>> technically not needed there either.
> 
> What happens if a non-flushed adapter gets a PERST ?

It depends on the adapter, so it really needs to be a policy in the device
driver. For any adapter that has a non volatile write cache, data must preserved,
otherwise the write cache is not truly non volatile. For adapters with a volatile
write cache, then its subject to the adapter hardware. For ipr adapters
that have a volatile cache, they do guarantee the data is preserved across
a PERST.

> I wouldn't trust that 'don't have to flush' magic ...

I really don't think we want to force RAID adapters that have gigabytes of
*non volatile* write cache to flush their cache when we are merely performing
a kexec. This can take several minutes in the worst case scenarios.

-Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Eric W. Biederman]

Brian King <brking@linux.vnet.ibm.com> writes:

> On 02/13/2017 08:04 PM, Benjamin Herrenschmidt wrote:
>> On Mon, 2017-02-13 at 15:57 -0600, Brian King wrote:
>>> If we do transition to use remove rather than shutdown, I think we
>>> want
>>> some way for a device driver to know whether we are doing kexec or
>>> not.
>>> A RAID adapter with a write cache is going to want to flush its write
>>> cache on a PCI hotplug remove, but for a kexec, its going to want to
>>> skip
>>> that so the kexec is faster. Today, since kexec looks like a reboot,
>>> rather than a shutdown, we can skip the flush on a reboot, since its
>>> technically not needed there either.
>> 
>> What happens if a non-flushed adapter gets a PERST ?
>
> It depends on the adapter, so it really needs to be a policy in the device
> driver. For any adapter that has a non volatile write cache, data must preserved,
> otherwise the write cache is not truly non volatile. For adapters with a volatile
> write cache, then its subject to the adapter hardware. For ipr adapters
> that have a volatile cache, they do guarantee the data is preserved across
> a PERST.
>
>> I wouldn't trust that 'don't have to flush' magic ...
>
> I really don't think we want to force RAID adapters that have gigabytes of
> *non volatile* write cache to flush their cache when we are merely performing
> a kexec. This can take several minutes in the worst case scenarios.


It is very very simple.  kexec does not guarantee what comes after it.

An optimization that skips flushing because the hardware write cache can
be trusted is fine.  An optimization that skips flushing because some
later kernel will take care of it is not.

I really don't see any room in there for distinguishing between which
kind of shutdown and/or kexec we are doing.  Either the hardware handles
it or it doesn't.

If the hardware doesn't handle everything than we need to handle it in
software.

Eric

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Mauricio Faria de Oliveira]

On 02/12/2017 07:49 PM, Anton Blanchard wrote:
> We see lpfc devices regularly fail during kexec. Fix this by adding
> a shutdown method which mirrors the remove method.

Reviewed-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
Tested-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>

@mkp, @jejb: may you please flag this patch for stable?  Thank you.

This patch fixes a problem of some adapter ports going offline after
reboot on the following adapter, at least:

     [10df:f100] Saturn-X: LightPulse Fibre Channel Host Adapter

     # dmesg | grep lpfc
     ...
     [   61.676728] lpfc 0003:12:00.0: 6:0442 Adapter failed to init,
     mbxCmd x88 CONFIG_PORT, mbxStatus x0 Data: x0
     [   92.766717] lpfc 0003:12:00.0: 6:0442 Adapter failed to init,
     mbxCmd x88 CONFIG_PORT, mbxStatus x0 Data: x0
     [  123.856717] lpfc 0003:12:00.0: 6:0442 Adapter failed to init,
     mbxCmd x88 CONFIG_PORT, mbxStatus x0 Data: x0
     ...

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Martin K. Petersen]

>>>>> "Mauricio" == Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> writes:

Mauricio> On 02/12/2017 07:49 PM, Anton Blanchard wrote:
>> We see lpfc devices regularly fail during kexec. Fix this by adding a
>> shutdown method which mirrors the remove method.

Mauricio> @mkp, @jejb: may you please flag this patch for stable?  Thank
Mauricio> you.

I don't recall a consensus being reached on this patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Benjamin Herrenschmidt]

On Mon, 2017-03-06 at 22:46 -0500, Martin K. Petersen wrote:
> > > > > > "Mauricio" == Mauricio Faria de Oliveira <mauricfo@linux.vn
> > > > > > et.ibm.com> writes:
> 
> Mauricio> On 02/12/2017 07:49 PM, Anton Blanchard wrote:
> > > We see lpfc devices regularly fail during kexec. Fix this by
> > > adding a
> > > shutdown method which mirrors the remove method.
> 
> Mauricio> @mkp, @jejb: may you please flag this patch for
> stable?  Thank
> Mauricio> you.
> 
> I don't recall a consensus being reached on this patch.

What would be the opposition ? Without it kexec breaks. With it, it
works ...

Now we all seem to agree that kexec should be overhauled to not use
shutdown but instead unbind drivers, but that's a more long term
project that nobody yet had a chance to tackle.

In the meantime, some systems need a functioning kexec to boot.

Cheers,
Ben.

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Mauricio Faria de Oliveira]

Martin,

On 03/07/2017 02:24 AM, Benjamin Herrenschmidt wrote:

> On Mon, 2017-03-06 at 22:46 -0500, Martin K. Petersen wrote:
>> I don't recall a consensus being reached on this patch.

> What would be the opposition ? Without it kexec breaks. With it, it
> works ...

That is the argument I'd present/ask for consideration too.

I think I should have included this in the tested-by tag email, for
documentation/evidence: no regression observed in system shutdown path.

Thanks,

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH] scsi: lpfc: Add shutdown method for kexec

[Martin K. Petersen]

>>>>> "Mauricio" == Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> writes:

Mauricio> I think I should have included this in the tested-by tag
Mauricio> email, for documentation/evidence: no regression observed in
Mauricio> system shutdown path.

Applied to 4.11/scsi-fixes.

-- 
Martin K. Petersen	Oracle Linux Engineering