Re: [PATCH 0/2] mpt2sas,mpt3sas - PCI master abort fixups

[Joe Lawrence <joe.lawrence@stratus.com>]

On 04/12/2015 08:54 PM, James Bottomley wrote:
> On Sun, 2015-04-12 at 20:11 -0400, Joe Lawrence wrote:
>> On 12/30/2014 09:07 AM, Joe Lawrence wrote:
>>> A colleague noticed that the mpt2 and mpt3sas drivers do not correctly
>>> check the PCI master abort pattern in _base_wait_for_doorbell_ack.  This
>>> pattern should be checked *prior* to any valid bit patterns, which would
>>> always return true since a PCI read on master abort sets all bits high.
>>>
>>> The second patch adds similar checking to _base_wait_for_doorbell_int and
>>> _base_wait_for_doorbell_not_used to avoid potentially long loops around
>>> PCI reads.
>>>
>>> Joe Lawrence (2):
>>>   mpt2sas,mpt3sas: correct master-abort checking in doorbell ack
>>>   mpt2sas,mpt3sas: additional master abort checks
>>>
>>>  drivers/scsi/mpt2sas/mpt2sas_base.c |   17 ++++++++++++-----
>>>  drivers/scsi/mpt3sas/mpt3sas_base.c |   17 ++++++++++++-----
>>>  2 files changed, 24 insertions(+), 10 deletions(-)
>>>
>>
>> Avago ping?
>>
>> This one was pretty straightforward: check 0xFFFFFFFF *before* any
>> individual bit(s), i.e. before reading the doorbell register.
> 
> OK, Joe, explain why this patch is important: what problems could result
> from it not being present?  If you convince everyone then no more mpt2/3
> sas patches until this is at least commented on and a plan of action
> proposed.

Hi James,

As currently coded:  If the PCI read returns a master abort,
_base_wait_for_doorbell_ack will loop until it exhausts its timeout (up
to 15 seconds).  Other parts of the driver, like the periodic watchdog
or EEH, may detect a similar problem before such a long time and cleanup
the mess.  However, complete device removal may be stalled until whoever
called _base_wait_for_doorbell_ack is satisfied that it has finished.

This behavior is not really a bug, but feels like one in the making.
Should additional code be introduced, copy/pasted, etc. it may not do
what was intended.

For future reference, would a repost have been more appropriate?  This
changeset was so small that I figured a status ping would have sufficed.

Regards,

-- Joe