[PATCH v2] mpt3sas: Display message on Configurable secure HBA

[Sreekanth Reddy]

Display below warning message only up on detection of
Configurable secure type controllers.

"HBA is in Configurable Secure mode"

v2 change set:
Replaced dev_warn() with dev_info() function while
displaying above message.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 104413e..5b9806d 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -10366,6 +10366,10 @@ static void pcie_device_make_active(struct 
MPT3SAS_ADAPTER *ioc,
ioc->id = mpt3_ids++;
sprintf(ioc->driver_name, "%s", MPT3SAS_DRIVER_NAME);
switch (pdev->device) {
+   case MPI26_MFGPAGE_DEVID_CFG_SEC_3816:
+   case MPI26_MFGPAGE_DEVID_CFG_SEC_3916:
+   dev_info(>dev,
+   "HBA is in Configurable Secure mode\n");
case MPI26_MFGPAGE_DEVID_SAS3508:
case MPI26_MFGPAGE_DEVID_SAS3508_1:
case MPI26_MFGPAGE_DEVID_SAS3408:
@@ -10373,9 +10377,6 @@ static void pcie_device_make_active(struct 
MPT3SAS_ADAPTER *ioc,
case MPI26_MFGPAGE_DEVID_SAS3516_1:
case MPI26_MFGPAGE_DEVID_SAS3416:
case MPI26_MFGPAGE_DEVID_SAS3616:
-   case MPI26_MFGPAGE_DEVID_CFG_SEC_3816:
-   case MPI26_MFGPAGE_DEVID_CFG_SEC_3916:
-   ioc_warn(ioc, "HBA is in Configurable Secure mode\n");
case MPI26_MFGPAGE_DEVID_HARD_SEC_3816:
case MPI26_MFGPAGE_DEVID_HARD_SEC_3916:
ioc->is_gen35_ioc = 1;
-- 
1.8.3.1

[PATCH] mpt3sas: Display message on Configurable secure HBA

[Sreekanth Reddy]

Display below warning message only up on detection of
Configurable secure type controllers.

"HBA is in Configurable Secure mode"

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 104413e..f73dadc 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -10366,6 +10366,10 @@ static void pcie_device_make_active(struct 
MPT3SAS_ADAPTER *ioc,
ioc->id = mpt3_ids++;
sprintf(ioc->driver_name, "%s", MPT3SAS_DRIVER_NAME);
switch (pdev->device) {
+   case MPI26_MFGPAGE_DEVID_CFG_SEC_3816:
+   case MPI26_MFGPAGE_DEVID_CFG_SEC_3916:
+   dev_warn(>dev,
+   "HBA is in Configurable Secure mode\n");
case MPI26_MFGPAGE_DEVID_SAS3508:
case MPI26_MFGPAGE_DEVID_SAS3508_1:
case MPI26_MFGPAGE_DEVID_SAS3408:
@@ -10373,9 +10377,6 @@ static void pcie_device_make_active(struct 
MPT3SAS_ADAPTER *ioc,
case MPI26_MFGPAGE_DEVID_SAS3516_1:
case MPI26_MFGPAGE_DEVID_SAS3416:
case MPI26_MFGPAGE_DEVID_SAS3616:
-   case MPI26_MFGPAGE_DEVID_CFG_SEC_3816:
-   case MPI26_MFGPAGE_DEVID_CFG_SEC_3916:
-   ioc_warn(ioc, "HBA is in Configurable Secure mode\n");
case MPI26_MFGPAGE_DEVID_HARD_SEC_3816:
case MPI26_MFGPAGE_DEVID_HARD_SEC_3916:
ioc->is_gen35_ioc = 1;
-- 
1.8.3.1

RE: [PATCH v4 1/6] mpt3sas: Introduce mpt3sas_base_pci_device_is_available

[Sreekanth Reddy]

HI Bjorn,

Please provide your valuable suggestion/reply here.

Thank you,
Sreekanth

-Original Message-
From: Suganath Prabu Subramani
[mailto:suganath-prabu.subram...@broadcom.com]
Sent: Monday, October 8, 2018 12:15 PM
To: helg...@kernel.org
Cc: lu...@wunner.de; linux-scsi@vger.kernel.org; linux-...@vger.kernel.org;
Andy Shevchenko; Sathya Prakash; Sreekanth Reddy;
linux-ker...@vger.kernel.org; b...@kernel.crashing.org; rus...@russell.cc;
sbobr...@linux.ibm.com; Oliver
Subject: Re: [PATCH v4 1/6] mpt3sas: Introduce
mpt3sas_base_pci_device_is_available

On Tue, Oct 2, 2018 at 7:34 PM Bjorn Helgaas  wrote:
>
> On Mon, Oct 01, 2018 at 03:40:51PM -0500, Bjorn Helgaas wrote:
> > I think the names "pci_device_is_present()" and
> > "mpt3sas_base_pci_device_is_available()" contribute to the problem
> > because they make promises that can't be kept -- all we can say is
> > that the device *was* present, but we know whether it is *still*
> > present.

Bjorn,

In the patch we are using '!' (i.e. not operation) of
pci_device_is_present(),
which is logically same as pci_device_is absent, and it is
same for mpt3sas_base_pci_device_is_available().
My understanding is that, you want us to rename these functions for
better readability
Is that correct ?


> Oops, I meant "we DON'T know whether it is still present."
>
> > I think it would be better if the interfaces were something
> > like "pci_device_is_absent()" because that gives a result we can rely
> > on.  If that returns true, we know the device is definitely gone.
> >
> > Bjorn

Re: [PATCH v2 1/6] mpt3sas: Introduce mpt3sas_base_pci_device_is_available

[Sreekanth Reddy]

On Thu, Sep 20, 2018 at 4:37 PM Christoph Hellwig  wrote:
>
> > +u8
> > +mpt3sas_base_pci_device_is_available(struct MPT3SAS_ADAPTER *ioc)
> > +{
> > + if (ioc->pci_error_recovery ||
> > +  (!pci_device_is_present(ioc->pdev)))
> > + return 0;
> > +
> > + return 1;
> > +}
>
> This should simplify be:
>
> bool mpt3sas_base_pci_device_is_available(struct MPT3SAS_ADAPTER *ioc)
> {
> return !ioc->pci_error_recovery && pci_device_is_present(ioc->pdev);
> }
>
> probably even as an inline..

Thank you. we will make this change and also declare this function as inline.

Thanks,
Sreekanth

Re: [Patch v1 0/7] mpt3sas: Hot-Plug Surprise removal support on IOC.

[Sreekanth Reddy]

On Wed, Sep 5, 2018 at 1:08 PM, Lukas Wunner  wrote:
> On Wed, Sep 05, 2018 at 11:45:45AM +0530, Sreekanth Reddy wrote:
>> On Tue, Sep 4, 2018 at 3:12 PM, Lukas Wunner  wrote:
>> > Many scsi drivers call pci_channel_offline() to detect inaccessibility
>> > of the device due to a PCI error:
>> > https://elixir.bootlin.com/linux/v4.19-rc2/ident/pci_channel_offline
>> >
>> > A patch is pending such that surprise removal can also be queried
>> > with that same function:
>> > https://www.spinics.net/lists/linux-pci/msg75722.html
>>
>> Lukas, thanks for pointing to this pci_dev_is_disconnected() API. So
>> we can use this API directly instead of reading the vendor Id and
>> checking for all one's once this patch get accepted?
>
> Yes, except pci_dev_is_disconnected() is private to the PCI core,
> but dev->error_state and pci_channel_offline() is public.


As Busch suggested, we will use pci_device_is_present() API.
We have tested HBA hot-unplug operations with this API and the things
are working fine.

>
>
>> I have one more instance where still we need this poll kthread, i.e
>> during the device probe time we have some commands which has more
>> timeout value (e.g. 300 seconds), so if user has unplugged this device
>> just after sending this more time-out valued command then driver has
>> to wait until this time-out value expires. i.e. this device is still
>> visible in lspci output until this 300 seconds timeout value expires
>> even though device is unplugged. if we have a poll kthread (which will
>> poll for every one second) then driver can early detect the unplugged
>> state and can terminate the outstanding commands and hence probe
>> operation can be completed quickly.
>
> The only instances I can see in your driver where it waits for 300 s
> is in _base_diag_reset(), which does an msleep(256) in a loop for up
> to 300 s, and scsih_scan_finished(), which is called in a loop with an
> msleep(10) by do_scsi_scan_host().
>
> Any harm in simply checking for removal of the device in those loops
> and bailing out if so?  Instead of the poll kthread to achieve the same?
we can do for this port enable request but still we have other
requests where we don't have this type of loops and used
wait_for_completion_timeout () API where we can't bailout the request
in-between and we have to wait until the timeout value expires. For
these types of request terminating it though watchdog thread will be
simple.

[PATCH 2/7] mpt3sas: Add HBA hot plug watchdog thread:
https://www.spinics.net/lists/linux-scsi/msg122963.html

In the above patch and in the worker thread function we are just
setting ioc->remove_host flag,
Apart from resetting this flag we have to terminate all the
outstanding commands at the HBA level and that is missing in the above
patch. we will post new set of patches.

We feel that this watch dog thread is needed, as we need to gracefully
terminate all the outstanding command at the HBA level apart from stop
receiving the new commands. With watch dog thread we can easily
terminate all the outstanding commands.

>
>
>> Also whether we need to wait for below patches get accepted? so that
>> we can post the new set of patches accommodating according to your
>> suggestions,
>> https://www.spinics.net/lists/linux-pci/msg75722.html
>> https://www.spinics.net/lists/linux-pci/msg75438.html
>
> I can't tell you whether and when those patches get accepted, that's
> for Bjorn Helgaas to decide.  Also, what does get accepted might differ.
> E.g. the first patch uses the existing pci_channel_io_perm_failure
> state for removal.  There was debate whether to introduce a new state
> for removed devices to avoid overloading the existing state, which is
> used for error recovery.
>
> All I can recommend is to follow linux-pci, test the patches that have
> already been brought forward to ascertain they fulfil your needs,
> and generally participate in the debate so that your use cases are
> covered.
>
> Thanks,
>
> Lukas

Re: [Patch v1 0/7] mpt3sas: Hot-Plug Surprise removal support on IOC.

[Sreekanth Reddy]

l


On Tue, Sep 4, 2018 at 3:12 PM, Lukas Wunner  wrote:
> On Tue, Sep 04, 2018 at 11:19:04AM +0530, Sreekanth Reddy wrote:
>> On Fri, Aug 31, 2018 at 2:25 PM, Lukas Wunner  wrote:
>> > * Just reading the vendor ID may not be sufficient to detect unplug,
>> >   it may also read as "all ones" if the link is down due to error
>> >   recovery by DPC.
>>
>> So, is their any other way to detect pci device unplug apart from
>> reading the vendor ID, I mean we have check any other flags, etc?
>
> Many scsi drivers call pci_channel_offline() to detect inaccessibility
> of the device due to a PCI error:
> https://elixir.bootlin.com/linux/v4.19-rc2/ident/pci_channel_offline
>
> A patch is pending such that surprise removal can also be queried
> with that same function:
> https://www.spinics.net/lists/linux-pci/msg75722.html

Lukas, thanks for pointing to this pci_dev_is_disconnected() API. So
we can use this API directly instead of reading the vendor Id and
checking for all one's once this patch get accepted?

>
>
>> > * I don't see why you need to poll for the device's removal from a
>> >   watchdog thread.  pciehp will invoke your driver's ->remove hook
>> >   once the device is gone.
>>
>> If we have some three to four PCI devices and all pci devices are hot
>> unplugged simultaneously, then we observed that driver's-remove hook
>> is called sequentially. So it takes some time to call fourth PCI
>> device driver's->remove hook. so during this time we want all the
>> outstanding commands to be gracefully terminated and hence we added
>> this watchdog thread to quickly detect the hba unplug and take
>> necessary steps such as gracefully terminate the outstanding IOs and
>> stop receiving further IOs on it. At later time when PCI subsystem
>> calls driver's-remove hook then driver can quickly release the
>> resources allocated for this unplugged device.
>
> pciehp does the following as soon as the surprise removal event is handled:
>
> pci_dev_set_disconnected(dev, NULL);
> if (pci_has_subordinate(dev))
> pci_walk_bus(dev->subordinate, pci_dev_set_disconnected, 
> NULL);
>
> Thus, once the above-linked patch lands, you'll be able to detect
> surprise removal by calling pci_channel_offline() or checking
> pdev->error_state == pci_channel_io_perm_failure, obviating the
> need to poll for surprise removal from a kthread.
>
> There's another patch pending to move pci_walk_bus() out of the
> pci_lock_rescan_remove() so that it runs even earlier, without
> pointlessly waiting for a lock:
> https://www.spinics.net/lists/linux-pci/msg75438.html

Yes due to this pci_lock_rescan_remove() lock, we observed that
driver's->remove hook called sequentially even though multiple HBA
devices are hot unplugged simultaneously.

I have one more instance where still we need this poll kthread, i.e
during the device probe time we have some commands which has more
timeout value (e.g. 300 seconds), so if user has unplugged this device
just after sending this more time-out valued command then driver has
to wait until this time-out value expires. i.e. this device is still
visible in lspci output until this 300 seconds timeout value expires
even though device is unplugged. if we have a poll kthread (which will
poll for every one second) then driver can early detect the unplugged
state and can terminate the outstanding commands and hence probe
operation can be completed quickly.


Also whether we need to wait for below patches get accepted? so that
we can post the new set of patches accommodating according to your
suggestions,
https://www.spinics.net/lists/linux-pci/msg75722.html
https://www.spinics.net/lists/linux-pci/msg75438.html

>
> Thanks,
>
> Lukas

Re: [Patch v1 0/7] mpt3sas: Hot-Plug Surprise removal support on IOC.

[Sreekanth Reddy]

On Fri, Aug 31, 2018 at 2:25 PM, Lukas Wunner  wrote:
> [cc += linux-pci, benh]
>
> On Fri, Aug 31, 2018 at 7:37 AM Suganath Prabu S 
>  wrote:
>> Posting below set of patches to support PCIe Hot Plug surprise removal,
>> and few defect fixes.
>
> Please cross-post to linux-pci in the future.
>
>
> Regarding [PATCH 1/7] mpt3sas: Introduce mpt3sas_base_pci_device_is_unplugged:
> https://www.spinics.net/lists/linux-scsi/msg122962.html
>
> * mpt3sas_base_pci_device_is_unplugged() is a duplication of the existing
>   pci_device_is_present().

Thanks for pointing this pci_device_is_present() API, we will replace
mpt3sas_base_pci_device_is_unplugged() with  pci_device_is_present().

>
> * Just reading the vendor ID may not be sufficient to detect unplug,
>   it may also read as "all ones" if the link is down due to error
>   recovery by DPC.

So, is their any other way to detect pci device unplug apart from
reading the vendor ID, I mean we have check any other flags, etc?

>
>
> Regarding [PATCH 2/7] mpt3sas: Add HBA hot plug watchdog thread:
> https://www.spinics.net/lists/linux-scsi/msg122963.html
>
> * I don't see why you need to poll for the device's removal from a
>   watchdog thread.  pciehp will invoke your driver's ->remove hook
>   once the device is gone.

If we have some three to four PCI devices and all pci devices are hot
unplugged simultaneously, then we observed that driver's-remove hook
is called sequentially. So it takes some time to call fourth PCI
device driver's->remove hook. so during this time we want all the
outstanding commands to be gracefully terminated and hence we added
this watchdog thread to quickly detect the hba unplug and take
necessary steps such as gracefully terminate the outstanding IOs and
stop receiving further IOs on it. At later time when PCI subsystem
calls driver's-remove hook then driver can quickly release the
resources allocated for this unplugged device.

>
> * A recent discussion initiated by Benjamin Herrenschmidt came to the
>   conclusion that device removal should be treated as a type of
>   error state (either pci_channel_io_perm_failure or another, newly
>   introduced state).  It will then be possible to detect the device's
>   inaccessibility with pci_channel_offline().  Please help work towards
>   such a future solution in the PCI core instead of solutions localized
>   to a single device driver.  Sorry, the discussion was lengthy, it is
>   available here:
>   https://www.spinics.net/lists/linux-pci/msg75425.html

Oh great, sure. We have very limited knowledge on PCI subsystem but we
try our best in future to provide solutions in the PCI core.

>
> Thanks,
>
> Lukas

Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Wed, Aug 8, 2018 at 1:27 AM, Bart Van Assche  wrote:
> On Tue, 2018-08-07 at 21:46 +0530, Sreekanth Reddy wrote:
>> [Sreekanth] In the patch description I mentioned that I have done a
>> manual mistake and I am correcting with this patch.
>>
>> Hope I have answered all of your quires.
>
> Not yet unfortunately. Why do you consider the current implementation a
> wrong?

In function mpt3sas_base_clear_st(),

st->cb_idx = 0xFF;
st->direct_io = 0;
st->smid = 0;
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);

In the above code we can see that smid is reset to zero before
resetting the chain_offset to zero in chain_lookup[] table for
corresponding IO request entry (i.e. smid -1 th entry). So in the
above code driver is logically resetting the chain_offset to zero for
the -1th entry of the chain_lookup[] table for all the IO requests,
which I am say it wrong. I am saying that above code should be like
below,

st->cb_idx = 0xFF;
st->direct_io = 0;
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
st->smid = 0;

I.e. reset the smid variable to zero only after resetting the
chain_offset for the corresponding IO request (i.e. for smid -1 th
entry) of the chain_lookup[] table.

> Why is the patch at the start of this e-mail thread considered a
> fix? Which behavior changes due to this patch? If this patch is a bug fix,
> how can the bug be triggered and why is this patch considered the right way
> to fix that bug?

See while posting the patch "mpt3sas: Fix calltrace observed while
running IO & reset" in hurry I made a manual mistake that I added
"st->smid = 0" line before "atomic_set(>chain_lookup[st->smid -
1].chain_offset, 0);" line in mpt3sas_base_clear_st() function instead
I should have added "st->smid = 0" line after the atomic set line.
Which I am correcting my mistake with this patch.

Thanks,
Sreekanth

>
> Thanks,
>
> Bart.
>

Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Tue, Aug 7, 2018 at 8:27 PM, Bart Van Assche  wrote:
> On Tue, 2018-08-07 at 20:03 +0530, Sreekanth Reddy wrote:
>> The main intention of this patch to reset the smid to zero after
>> resetting the corresponding smid entry's chain_offset to zero. While
>> posting "mpt3sas: Fix calltrace observed while running IO & reset"
>> patch I have done manual mistake that I reset the smid to zero before
>> resetting the chain_offset which is wrong. Due to which driver
>> always's reset the chain_offset of zero th  entry of chain_lookup[]
>> table which is wrong and hence I am correcting it with this patch.
>>
>> After that you asked me to add memory barriers and hence I have added
>> memory barriers in subsequent versions of the patch.
>
> Hello Sreekanth,
>
> Please answer the questions I already asked you twice instead of sidestepping
> these.


Here I am copying your quires from your previous mails and I am
replaying inline..


In mpt3sas_base_get_smid_scsiio() I found the following code:

  struct scsiio_tracker *request = scsi_cmd_priv(scmd);
  u16 smid = scmd->request->tag + 1;
  request->smid = smid;

That confirms what you wrote about smid being unique for each I/O request.

However, this also raises new questions. As far as I can see all code
in the I/O path that accesses the chain_lookup[] array uses index smid
- 1.

[Sreekanth] Our IOC firmware always requires smid to be >=1, zero is
illegal smid for the IOC firmware. Hence while framing MPT SCSI IO
request driver always make sure that smid to be >=1, so it uses smid
as tag + 1.
where as chain_lookup[] starts with index zero, So driver is accessing
this chain_lookup[] with smid-1 index.


The patch at the start of this e-mail thread modifies two functions, namely

mpt3sas_remove_dead_ioc_func() and mpt3sas_base_clear_st(). Since the
chain_lookup[] array is indexed with the smid and hence contains
request- private data, how is it even possible that these functions
race against each other?
[Sreekanth] This patch modifies in two functions namely
'_base_get_chain_buffer_tracker()' & 'mpt3sas_base_clear_st()' not in
the mpt3sas_remove_dead_ioc_func(). Their is no race between
mpt3sas_remove_dead_ioc_func() & mpt3sas_base_clear_st(). Also their
is no race between _base_get_chain_buffer_tracker() &
mpt3sas_base_clear_st(), since for a particular IO
_base_get_chain_buffer_tracker() is called during IO submission time
for getting required chain buffer and mpt3sas_base_clear_st() is
called to free the used chain buffers in the IO completion path. Also
we have pre-allocated required chain buffers for each IO request and
hence no need to main any list and their is no race between these two
functions.

 Is there perhaps code that accesses the chain_lookup[] array and that
is called from another context than the I/O completion path?
[Sreekanth] No, it is called only from IO submission time.


Is the patch at the start of this e-mail thread the result of a
theoretical concern or of a real failure? In the latter case, which
sequence of actions triggered the failure? The answer to this question
should already have been mentioned in the description of v1 of this
patch
[Sreekanth] The main intention of this patch to reset the smid to zero
after resetting the corresponding smid entry's chain_offset to zero in
chain_lookup[]. While posting "mpt3sas: Fix calltrace observed while
running IO & reset" patch I have done manual mistake that I reset the
smid to zero before resetting the chain_offset which is wrong. Due to
which driver always's reset the chain_offset of zero th  entry of
chain_lookup[] table which is wrong and we may see that scmd's will be
returned from scsih_qcmd() with "host busy" status due to
unavailability of chain buffers (as driver is resetting the wrong
smid's i.e. zero th smid entry's chain_offset instead of correct smid
entry's chain_offset) and IO's will be in hung state .  So, I am
correcting it with this patch.
After that you asked me to add memory barriers and hence I have added
memory barriers in subsequent versions of the patch.


How can this patch be necessary if there are no races between I/O
submission and I/O completion?
[Sreekanth] As I explained above in this patch I am resetting the smid
variable to zero only after resetting the smid's chain_offset in
chain_lookup[] table. Due to manual mistake earlier I am resetting the
smid variable to zero before resetting the correct smid entry's
chain_offset in the chain_lookup[] table, which is wrong and I am
correcting it with this patch.

 Changing the order of two memory writes only makes sense if there is
a race condition involved. Since the block layer allocates a request
tag before scsih_qcmd() is called and since mpt3sas_base_free_smid()
is called before scmd->scsi_done(scmd) in _scsih_io_done(), the
request tag i

Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Tue, Aug 7, 2018 at 7:29 PM, Bart Van Assche  wrote:
> On Tue, 2018-08-07 at 12:10 +0530, Sreekanth Reddy wrote:
>> This function _base_get_chain_buffer_tracker() is called only during
>> the IO submission time and mpt3sas_base_clear_st() is called during IO
>> completion time and hence I don't see any race condition here.
>>
>> Currently this patch is very much needed, so please consider this
>> patch. May be we can start new email thread to discuss on the possible
>> race conditions you are observing and I can clear your quires or I can
>> fix them as on we can find the real race conditions.
>
> Hello Sreekanth,
>
> How can this patch be necessary if there are no races between I/O submission
> and I/O completion? Changing the order of two memory writes only makes sense
> if there is a race condition involved. Since the block layer allocates a
> request tag before scsih_qcmd() is called and since mpt3sas_base_free_smid()
> is called before scmd->scsi_done(scmd) in _scsih_io_done(), the request tag
> is freed after the smid has been freed when I/O completes in a normal way.
> So there shouldn't be a race condition in that scenario.
>
> By the way, you have not answered my question about why development of this
> patch started. Does this patch address a theoretical concern or a real bug?
> In the latter case, which scenario triggers the addressed bug? If you want
> this patch to go upstream you should be able to explain why you think it is
> useful, and that description should be included in the patch description.

Bart,

The main intention of this patch to reset the smid to zero after
resetting the corresponding smid entry's chain_offset to zero. While
posting "mpt3sas: Fix calltrace observed while running IO & reset"
patch I have done manual mistake that I reset the smid to zero before
resetting the chain_offset which is wrong. Due to which driver
always's reset the chain_offset of zero th  entry of chain_lookup[]
table which is wrong and hence I am correcting it with this patch.

After that you asked me to add memory barriers and hence I have added
memory barriers in subsequent versions of the patch.

Thanks,
Sreekanth


>
> Thanks,
>
> Bart.
>

Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Mon, Aug 6, 2018 at 11:41 PM, Bart Van Assche  wrote:
> On Sat, 2018-08-04 at 18:56 +0530, Sreekanth Reddy wrote:
>> No Bart, their is no race condition here. Since chain lookup table
>> entry is uniquely accessed using smid value. And this smid (which is
>> scmd->request->tag +1) is unique for each IO request. And
>> _base_get_chain_buffer_tracker() function is called only during IO
>> submission time (i.e. before submitting the IO to the IOC firmware)
>> and mpt3sas_base_clear_st() function is called in the ISR (i.e during
>> IO completion) path. So for a particular IO these two functions called
>> at two different instances of time.
>
> Hello Sreekanth,
>
> In mpt3sas_base_get_smid_scsiio() I found the following code:
>
> struct scsiio_tracker *request = scsi_cmd_priv(scmd);
> u16 smid = scmd->request->tag + 1;
> request->smid = smid;
>
> That confirms what you wrote about smid being unique for each I/O request.
> However, this also raises new questions. As far as I can see all code in
> the I/O path that accesses the chain_lookup[] array uses index smid - 1.

Our IOC firmware always requires smid to be >=1, zero is illegal smid
for the IOC firmware. Hence while framing MPT SCSI IO request driver
always make sure that smid to be >=1, so it uses smid as tag + 1.
where as chain_lookup[] starts with index zero, So driver is accessing
this chain_lookup[] with smid-1 index.


> The patch at the start of this e-mail thread modifies two functions, namely
> mpt3sas_remove_dead_ioc_func() and mpt3sas_base_clear_st().

This patch modifies in two functions namely
'_base_get_chain_buffer_tracker()' & 'mpt3sas_base_clear_st()' not in
the mpt3sas_remove_dead_ioc_func(), not sure why 'git format-patch' is
showing 'mpt3sas_remove_dead_ioc_func' function name in the patch.

This function _base_get_chain_buffer_tracker() is called only during
the IO submission time and mpt3sas_base_clear_st() is called during IO
completion time and hence I don't see any race condition here.

Currently this patch is very much needed, so please consider this
patch. May be we can start new email thread to discuss on the possible
race conditions you are observing and I can clear your quires or I can
fix them as on we can find the real race conditions.

Thanks,
Sreekanth


 Since the
> chain_lookup[] array is indexed with the smid and hence contains request-
> private data, how is it even possible that these functions race against
> each other? Is there perhaps code that accesses the chain_lookup[] array
> and that is called from another context than the I/O completion path?
>
> Is the patch at the start of this e-mail thread the result of a theoretical
> concern or of a real failure? In the latter case, which sequence of actions
> triggered the failure? The answer to this question should already have been
> mentioned in the description of v1 of this patch.
>
> Thanks,
>
> Bart.

Re: [PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Fri, Aug 3, 2018 at 10:02 PM, Bart Van Assche  wrote:
> On Fri, 2018-08-03 at 12:16 -0400, Sreekanth Reddy wrote:
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> index 902610d..2c5a5b4 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> @@ -1702,6 +1702,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>>   return NULL;
>>
>>   chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset];
>> +
>> + /*
>> +  * Added memory barrier to make sure that correct chain tracker
>> +  * is retrieved before incrementing the smid pool's chain_offset
>> +  * value in chain lookup table.
>> +  */
>> + smp_mb();
>>   atomic_inc(>chain_lookup[smid - 1].chain_offset);
>>   return chain_req;
>>  }
>> @@ -3283,8 +3290,15 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER 
>> *ioc,
>>   return;
>>   st->cb_idx = 0xFF;
>>   st->direct_io = 0;
>> - st->smid = 0;
>>   atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
>> +
>> + /*
>> +  * Added memory barrier to make sure that smid is set to zero
>> +  * only after resetting corresponding smid pool's chain_offset to zero
>> +  * in chain lookup table.
>> +  */
>> + smp_mb();
>> + st->smid = 0;
>>  }
>
> Thanks for having addressed previous review comments. Hence:
>
> Reviewed-by: Bart Van Assche 
>
> However, I'm not yet convinced that this patch is sufficient to fix the race
> between _base_get_chain_buffer_tracker() and mpt3sas_base_clear_st(). Can e.g.
> the atomic_set() that resets chain_offset occur after it has been read by
> _base_get_chain_buffer_tracker() and before that function increments the
> chain_offset member variable?
>

No Bart, their is no race condition here. Since chain lookup table
entry is uniquely accessed using smid value. And this smid (which is
scmd->request->tag +1) is unique for each IO request. And
_base_get_chain_buffer_tracker() function is called only during IO
submission time (i.e. before submitting the IO to the IOC firmware)
and mpt3sas_base_clear_st() function is called in the ISR (i.e during
IO completion) path. So for a particular IO these two functions called
at two different instances of time.

Thanks,
Sreekanth

> Thanks,
>
> Bart.
>

[PATCH v3] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

In mpt3sas_base_clear_st() function smid value is reset in
wrong line, i.e. driver should reset smid value to zero after decrementing
chain_offset counter in chain_lookup table but in current code, driver is
resetting smid value before decrementing the chain_offset counter. Which we
are correcting with this patch.

v1 changelog:
Added memory barriers before & after atomic_set() API.

v2 changelog:
Added proper comments to describe the need of using
smp_mb__before_atomic() & smp_mb__after_atomic() APIs
in the driver before calling these APIs.

v3 changelog:
Replaced smp_mb__before_atomic() & smp_mb__after_atomic() APIs
with smp_mb() APIs.

Used smp_mb() API in mpt3sas_base_clear_st() to make sure that
smid is reset to zero only after corresponding smid pool's
chain_offset in chain lookup table is reset to zero.

Used smp_mb() API in _base_get_chain_buffer_tracker()
to make sure that proper chain tracker is retrieved from the
corresponding smid's pool from chain table before incrementing
smid pool's chain offset value.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 902610d..2c5a5b4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1702,6 +1702,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
return NULL;
 
chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset];
+
+   /*
+* Added memory barrier to make sure that correct chain tracker
+* is retrieved before incrementing the smid pool's chain_offset
+* value in chain lookup table.
+*/
+   smp_mb();
atomic_inc(>chain_lookup[smid - 1].chain_offset);
return chain_req;
 }
@@ -3283,8 +3290,15 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
return;
st->cb_idx = 0xFF;
st->direct_io = 0;
-   st->smid = 0;
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
+
+   /*
+* Added memory barrier to make sure that smid is set to zero
+* only after resetting corresponding smid pool's chain_offset to zero
+* in chain lookup table.
+*/
+   smp_mb();
+   st->smid = 0;
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH v1 RESEND] mpt3sas: Swap I/O memory read value back to cpu endianness

[Sreekanth Reddy]

On Fri, Aug 3, 2018 at 3:07 PM, Andy Shevchenko
 wrote:
> On Thu, 2018-08-02 at 16:16 -0400, Martin K. Petersen wrote:
>> Andy,
>>
>> Please review the changes Sreekanth made to address your feedback.
>
> From my point of view they look good.
>
> I assume Sreekanth tested them on real hardware and as I remember davem@
> actually the one who possesses sparc machine with this hardware.

Yes I have tested this patch changes on SPARC64 system and it is working fine.

Thanks,
Sreekanth

>
>>
>> > Swap the I/O memory read value back to cpu endianness before storing
>> > it
>> > in a data structures which are defined in the MPI headers where
>> > u8 components are not defined in the endianness order.
>> >
>> > In this area from day one mpt3sas driver is using le32_to_cpu() &
>> > cpu_to_le32() APIs. But in the patch cf6bf9710c
>> > (mpt3sas: Bug fix for big endian systems) we have removed these APIs
>> > before reading I/O memory which we should haven't done it. So
>> > in this patch I am correcting it by adding these APIs back
>> > before accessing I/O memory.
>> >
>> > v1: Changelog:
>> > Replaced writeq API with __raw_writeq() & mmiowb() APIs.
>>
>>
>
> --
> Andy Shevchenko 
> Intel Finland Oy

[PATCH v2] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

In mpt3sas_base_clear_st() function smid value is reseted in
wrong line, i.e. driver should reset smid value to zero after decrementing
chain_offset counter in chain_lookup table but in current code, driver is
resetting smid value before decrementing the chain_offset counter. which we
are correcting with this patch.

v1 changelog:
Added memory barriers before & after atomic_set() API.

v2 changelog:
Added proper comments to describe the need of using
smp_mb__before_atomic() & smp_mb__after_atomic() APIs
in the driver before calling these APIs.

Used smp_mb__after_atomic() API in mpt3sas_base_clear_st() to
make sure that smid is reset to zero only after corresponding
smid pool's chain_offset in chain lookup table is reset to zero.

Used smp_mb__before_atomic() API in _base_get_chain_buffer_tracker()
to make sure that proper chain tracker is retrieved from the
corresponding smid's pool from chain table before incrementing
smid pool's chain offset value.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 902610d..94f7236 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1702,6 +1702,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
return NULL;
 
chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset];
+   /* Added memory barrier to make sure that correct chain tracker
+* is retrieved before incrementing the smid pool's chain_offset
+* value in chain lookup table.
+*/
+   smp_mb__before_atomic();
atomic_inc(>chain_lookup[smid - 1].chain_offset);
return chain_req;
 }
@@ -3283,8 +3288,13 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
return;
st->cb_idx = 0xFF;
st->direct_io = 0;
-   st->smid = 0;
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
+   /* Added memory barrier to make sure that smid is set to zero
+* only after resetting corresponding smid pool's chain_offset to zero
+* in chain lookup table.
+*/
+   smp_mb__after_atomic();
+   st->smid = 0;
 }
 
 /**
-- 
1.8.3.1

[PATCH v1 RESEND] mpt3sas: Swap I/O memory read value back to cpu endianness

[Sreekanth Reddy]

Swap the I/O memory read value back to cpu endianness before storing it
in a data structures which are defined in the MPI headers where
u8 components are not defined in the endianness order.

In this area from day one mpt3sas driver is using le32_to_cpu() &
cpu_to_le32() APIs. But in the patch cf6bf9710c
(mpt3sas: Bug fix for big endian systems) we have removed these APIs
before reading I/O memory which we should haven't done it. So
in this patch I am correcting it by adding these APIs back
before accessing I/O memory.

v1: Changelog:
Replaced writeq API with __raw_writeq() & mmiowb() APIs.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 94359d8..c75e88a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3350,11 +3350,10 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
spinlock_t *writeq_lock)
 {
unsigned long flags;
-   __u64 data_out = b;
 
spin_lock_irqsave(writeq_lock, flags);
-   writel((u32)(data_out), addr);
-   writel((u32)(data_out >> 32), (addr + 4));
+   __raw_writel((u32)(b), addr);
+   __raw_writel((u32)(b >> 32), (addr + 4));
mmiowb();
spin_unlock_irqrestore(writeq_lock, flags);
 }
@@ -3374,7 +3373,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
 static inline void
 _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)
 {
-   writeq(b, addr);
+   __raw_writeq(b, addr);
+   mmiowb();
 }
 #else
 static inline void
@@ -5275,7 +5275,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
 
/* send message 32-bits at a time */
for (i = 0, failed = 0; i < request_bytes/4 && !failed; i++) {
-   writel((u32)(request[i]), >chip->Doorbell);
+   writel(cpu_to_le32(request[i]), >chip->Doorbell);
if ((_base_wait_for_doorbell_ack(ioc, 5)))
failed = 1;
}
@@ -5296,7 +5296,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
}
 
/* read the first two 16-bits, it gives the total length of the reply */
-   reply[0] = (u16)(readl(>chip->Doorbell)
+   reply[0] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
if ((_base_wait_for_doorbell_int(ioc, 5))) {
@@ -5305,7 +5305,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
ioc->name, __LINE__);
return -EFAULT;
}
-   reply[1] = (u16)(readl(>chip->Doorbell)
+   reply[1] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
 
@@ -5319,7 +5319,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
if (i >=  reply_bytes/2) /* overflow case */
readl(>chip->Doorbell);
else
-   reply[i] = (u16)(readl(>chip->Doorbell)
+   reply[i] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
}
-- 
1.8.3.1

[PATCH v1] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

In mpt3sas_base_clear_st() function smid value is reseted in wrong line,
i.e. driver should reset smid value to zero after decrementing chain_offset
counter in chain_lookup table but in current code, driver is resetting smid
value before decrementing the chain_offset counter. which we are correcting
with this patch.

v1 changelog:
Added memory barriers before & after atomic_set() API.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 902610d..94359d8 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
return NULL;
 
chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset];
+   /* Adding memory barrier before atomic operation. */
+   smp_mb__before_atomic();
atomic_inc(>chain_lookup[smid - 1].chain_offset);
return chain_req;
 }
@@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
return;
st->cb_idx = 0xFF;
st->direct_io = 0;
-   st->smid = 0;
+   /* Adding memory barrier before atomic operation. */
+   smp_mb__before_atomic();
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
+   /* Adding memory barrier after atomic operation. */
+   smp_mb__after_atomic();
+   st->smid = 0;
 }
 
 /**
-- 
1.8.3.1

[PATCH v1] mpt3sas: Swap I/O memory read value back to cpu endianness

[Sreekanth Reddy]

Swap the I/O memory read value back to cpu endianness before storing it
in a data structures which are defined in the MPI headers where
u8 components are not defined in the endianness order.

In this area from day one mpt3sas driver is using le32_to_cpu() &
cpu_to_le32() APIs. But in the patch cf6bf9710c
(mpt3sas: Bug fix for big endian systems) we have removed these APIs
before reading I/O memory which we should haven't done it. So
in this patch I am correcting it by adding these APIs back
before accessing I/O memory.

v1: Changelog:
Replaced writeq API with __raw_writeq() & mmiowb() APIs.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 94359d8..c75e88a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3350,11 +3350,10 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
spinlock_t *writeq_lock)
 {
unsigned long flags;
-   __u64 data_out = b;
 
spin_lock_irqsave(writeq_lock, flags);
-   writel((u32)(data_out), addr);
-   writel((u32)(data_out >> 32), (addr + 4));
+   __raw_writel((u32)(b), addr);
+   __raw_writel((u32)(b >> 32), (addr + 4));
mmiowb();
spin_unlock_irqrestore(writeq_lock, flags);
 }
@@ -3374,7 +3373,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
 static inline void
 _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)
 {
-   writeq(b, addr);
+   __raw_writeq(b, addr);
+   mmiowb();
 }
 #else
 static inline void
@@ -5275,7 +5275,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
 
/* send message 32-bits at a time */
for (i = 0, failed = 0; i < request_bytes/4 && !failed; i++) {
-   writel((u32)(request[i]), >chip->Doorbell);
+   writel(cpu_to_le32(request[i]), >chip->Doorbell);
if ((_base_wait_for_doorbell_ack(ioc, 5)))
failed = 1;
}
@@ -5296,7 +5296,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
}
 
/* read the first two 16-bits, it gives the total length of the reply */
-   reply[0] = (u16)(readl(>chip->Doorbell)
+   reply[0] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
if ((_base_wait_for_doorbell_int(ioc, 5))) {
@@ -5305,7 +5305,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
ioc->name, __LINE__);
return -EFAULT;
}
-   reply[1] = (u16)(readl(>chip->Doorbell)
+   reply[1] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
 
@@ -5319,7 +5319,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
if (i >=  reply_bytes/2) /* overflow case */
readl(>chip->Doorbell);
else
-   reply[i] = (u16)(readl(>chip->Doorbell)
+   reply[i] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
}
-- 
1.8.3.1

Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Tue, Jul 24, 2018 at 10:37 AM, Sreekanth Reddy
 wrote:
> Any update on this patch!.

Just Ping once again. Any update on this patch.

> Thanks,
> Sreekanth
>
> On Fri, Jul 20, 2018 at 5:56 PM, Sreekanth Reddy
>  wrote:
>> In mpt3sas_base_clear_st() function smid value is reseted in wrong line,
>> i.e. driver should reset smid value to zero after decrementing chain_offset
>> counter in chain_lookup table but in current code, driver is resetting smid
>> value before decrementing the chain_offset counter. which we are correcting
>> with this patch.
>>
>> v1 changelog:
>> Added memory barriers before & after atomic_set() API.
>>
>> Signed-off-by: Sreekanth Reddy 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> index 902610d..94359d8 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>> return NULL;
>>
>> chain_req = >chain_lookup[smid - 
>> 1].chains_per_smid[chain_offset];
>> +   /* Adding memory barrier before atomic operation. */
>> +   smp_mb__before_atomic();
>> atomic_inc(>chain_lookup[smid - 1].chain_offset);
>> return chain_req;
>>  }
>> @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER 
>> *ioc,
>> return;
>> st->cb_idx = 0xFF;
>> st->direct_io = 0;
>> -   st->smid = 0;
>> +   /* Adding memory barrier before atomic operation. */
>> +   smp_mb__before_atomic();
>> atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
>> +   /* Adding memory barrier after atomic operation. */
>> +   smp_mb__after_atomic();
>> +   st->smid = 0;
>>  }
>>
>>  /**
>> --
>> 1.8.3.1
>>

Re: [PATCH] mpt3sas: Swap I/O memory read value back to cpu endianness

[Sreekanth Reddy]

On Wed, Jul 25, 2018 at 8:32 PM, Andy Shevchenko
 wrote:
> On Wed, Jul 25, 2018 at 12:42 PM, Sreekanth Reddy
>  wrote:
>> Swap the I/O memory read value back to cpu endianness before storing it
>> in a data structures which are defined in the MPI headers where
>> u8 components are not defined in the endianness order.
>>
>> In this area from day one mpt3sas driver is using le32_to_cpu() &
>> cpu_to_le32() APIs. But in the patch cf6bf9710c
>> (mpt3sas: Bug fix for big endian systems) we have removed these APIs
>> before reading I/O memory which we should haven't done it. So
>> in this patch I am correcting it by adding these APIs back
>> before accessing I/O memory.
>
>> -   __u64 data_out = b;
>> +   __u64 data_out = cpu_to_le64(b);
>>
>> spin_lock_irqsave(writeq_lock, flags);
>> writel((u32)(data_out), addr);
>
> Wouldn't be the same as
>
> __raw_writel(data_out >> 0, addr);
> __raw_writel(data_out >> 32, addr + 4);
> mmiowb();
>
> ?

Yes, I can replace the writel() APIs with __raw_writel() with mmiowb()
memory barrier. Hoping this doesn't create any other side effects.

I will post new patch with this change tomorrow after doing some
testing in this area.

>
>>  _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)
>>  {
>> -   writeq(b, addr);
>> +   writeq(cpu_to_le64(b), addr);
>
> Similar here
> __raw_writeq(b, addr);
> mmiowb();
>
>
>> -   writel((u32)(request[i]), >chip->Doorbell);
>> +   writel(cpu_to_le32(request[i]), >chip->Doorbell);
>
> This kind of endianess play (including above) should make sparse unhappy.
>
> Did you run it with
>
> C=1 CF=-D__CHECK_ENDIAN__
>
> ?

Yes I run it on 4.18 kernel and I don't see any error or warning for
these lines.

Thanks,
Sreekanth

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

Any update on this patch!.

Thanks,
Sreekanth

On Fri, Jul 20, 2018 at 5:56 PM, Sreekanth Reddy
 wrote:
> In mpt3sas_base_clear_st() function smid value is reseted in wrong line,
> i.e. driver should reset smid value to zero after decrementing chain_offset
> counter in chain_lookup table but in current code, driver is resetting smid
> value before decrementing the chain_offset counter. which we are correcting
> with this patch.
>
> v1 changelog:
> Added memory barriers before & after atomic_set() API.
>
> Signed-off-by: Sreekanth Reddy 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 902610d..94359d8 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> return NULL;
>
> chain_req = >chain_lookup[smid - 
> 1].chains_per_smid[chain_offset];
> +   /* Adding memory barrier before atomic operation. */
> +   smp_mb__before_atomic();
> atomic_inc(>chain_lookup[smid - 1].chain_offset);
> return chain_req;
>  }
> @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
> return;
> st->cb_idx = 0xFF;
> st->direct_io = 0;
> -   st->smid = 0;
> +   /* Adding memory barrier before atomic operation. */
> +   smp_mb__before_atomic();
> atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
> +   /* Adding memory barrier after atomic operation. */
> +   smp_mb__after_atomic();
> +   st->smid = 0;
>  }
>
>  /**
> --
> 1.8.3.1
>

[PATCH] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

In mpt3sas_base_clear_st() function smid value is reseted in wrong line,
i.e. driver should reset smid value to zero after decrementing chain_offset
counter in chain_lookup table but in current code, driver is resetting smid
value before decrementing the chain_offset counter. which we are correcting
with this patch.

v1 changelog:
Added memory barriers before & after atomic_set() API.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 902610d..94359d8 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
return NULL;
 
chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset];
+   /* Adding memory barrier before atomic operation. */
+   smp_mb__before_atomic();
atomic_inc(>chain_lookup[smid - 1].chain_offset);
return chain_req;
 }
@@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
return;
st->cb_idx = 0xFF;
st->direct_io = 0;
-   st->smid = 0;
+   /* Adding memory barrier before atomic operation. */
+   smp_mb__before_atomic();
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
+   /* Adding memory barrier after atomic operation. */
+   smp_mb__after_atomic();
+   st->smid = 0;
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

On Wed, Jul 18, 2018 at 7:38 PM, Bart Van Assche  wrote:
> On Wed, 2018-07-18 at 01:22 -0400, Sreekanth Reddy wrote:
>> In mpt3sas_base_clear_st() function smid value is reseted in wrong line,
>> i.e. driver should reset smid value to zero after decrementing chain_offset
>> counter in chain_lookup table but in current code, driver is resetting smid
>> value before decrementing the chain_offset counter. which we are correcting
>> with this patch.
>>
>> Signed-off-by: Sreekanth Reddy 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> index 902610d..94b939b 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
>>   return;
>>   st->cb_idx = 0xFF;
>>   st->direct_io = 0;
>> - st->smid = 0;
>>   atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
>> + st->smid = 0;
>>  }
>
> How can this patch be correct without memory barrier between the atomic set 
> and the
> st->smid assignment?

Yes I agree that I need to add memory barrier around atomic_set() API.
I was not aware of this before.

Is it fine to use smp_mb__before_atomic() API before atomic_set() &
smp_mb__after_atomic() API after atomic_set?

Also in other places in driver, we are using atomic_inc() &
atomic_dec() APIs without memory barrier, I need to add memory barrier
their too. So is it fine to post separate patch for adding memory
barrier around atomic_xyz() APIs? if yes then please consider this
patch and I will post separate patch for adding memory barriers around
the atomic_xyz() APIs.

Thanks,
Sreekanth

>
> Bart.
>
>

[PATCH] mpt3sas: correct reset of smid while clearing scsi tracker

[Sreekanth Reddy]

In mpt3sas_base_clear_st() function smid value is reseted in wrong line,
i.e. driver should reset smid value to zero after decrementing chain_offset
counter in chain_lookup table but in current code, driver is resetting smid
value before decrementing the chain_offset counter. which we are correcting
with this patch.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 902610d..94b939b 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
return;
st->cb_idx = 0xFF;
st->direct_io = 0;
-   st->smid = 0;
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
+   st->smid = 0;
 }
 
 /**
-- 
1.8.3.1

[PATCH][RESEND v1]mpt3sas: Fix calltrace observed while running IO & reset

[Sreekanth Reddy]

 [] scsi_softirq_done+0x132/0x160
 [] blk_done_softirq+0x96/0xc0
 [] __do_softirq+0xf5/0x280
 [] call_softirq+0x1c/0x30
 [] do_softirq+0x65/0xa0
 [] irq_exit+0x105/0x110
 [] smp_apic_timer_interrupt+0x48/0x60
 [] apic_timer_interrupt+0x162/0x170
 
 [] ? scsi_done+0x21/0x60
 [] ? delay_tsc+0x38/0x60
 [] __const_udelay+0x2d/0x30
 [] _base_handshake_req_reply_wait+0x8e/0x4a0 [mpt3sas]
 [] _base_get_ioc_facts+0x123/0x590 [mpt3sas]
 [] ? _base_diag_reset+0x238/0x340 [mpt3sas]
 [] mpt3sas_base_hard_reset_handler+0x1f3/0x420 [mpt3sas]
 [] _ctl_ioctl_main.isra.12+0x11b9/0x1200 [mpt3sas]
 [] ? xfs_file_aio_write+0x155/0x1b0 [xfs]
 [] ? do_sync_write+0x93/0xe0
 [] _ctl_ioctl+0x1a/0x20 [mpt3sas]
 [] do_vfs_ioctl+0x350/0x560
 [] ? __sb_end_write+0x31/0x60
 [] SyS_ioctl+0xa1/0xc0
 [] ? system_call_after_swapgs+0xa2/0x146
 [] system_call_fastpath+0x1c/0x21
 [] ? system_call_after_swapgs+0xae/0x146
Code: 83 c3 10 4c 89 e2 4c 89 ee e8 8d 21 04 00 48 8b 03 48 85 c0 75 e5 41 f6 
44 24 4a 10 74 ad 4c 89 e6 4c 89 ef e8 b2 42 00 00 eb a0 <0f> 0b 0f 1f 40 00 66 
2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90
RIP  [] blk_requeue_request+0x90/0xa0
 RSP 

As a part of host reset operation, driver will flushout all IOs
outstanding at driver level with "DID_RESET" result.
To find which are all commands outstanding at the driver level,
driver loops with smid starting from one to HBA queue depth and
calls mpt3sas_scsih_scsi_lookup_get() to get scmd as shown below

 for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
if (!scmd)
continue;

But in mpt3sas_scsih_scsi_lookup_get() function, driver returns some
scsi cmnds which are not outstanding at the driver level (possibly request
is constructed at block layer since QUEUE_FLAG_QUIESCED is not set. Even if
driver uses scsi_block_requests and scsi_unblock_requests, issue still
persists as they will be just blocking further IO from scsi layer and
not from block layer) and these commands are flushed with
DID_RESET host bytes thus resulting into above kernel BUG.

This issue got introduced from below patch changes,
scsi: mpt3sas: lockless command submission (commit id dbec4c9040).

To fix this issue, we have modified the mpt3sas_scsih_scsi_lookup_get()
to check for smid equals to zero (note: whenever any scsi cmnd is
processing at the driver level then smid for that scsi cmnd will be
non-zero, always it starts from one) before it returns the scmd pointer to
the caller. If smid is zero then this function returns scmd pointer as
NULL and driver won't flushout those scsi cmnds at driver level with
DID_RESET host byte thus this issue will not be observed.

Signed-off-by: Sreekanth Reddy 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c  | 1 +
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 569392d..902610d 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3283,6 +3283,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
return;
st->cb_idx = 0xFF;
st->direct_io = 0;
+   st->smid = 0;
atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0);
 }
 
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b8d131a..f3d7270 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -1489,7 +1489,7 @@ struct scsi_cmnd *
scmd = scsi_host_find_tag(ioc->shost, unique_tag);
if (scmd) {
st = scsi_cmd_priv(scmd);
-   if (st->cb_idx == 0xFF)
+   if (st->cb_idx == 0xFF || st->smid == 0)
scmd = NULL;
}
}
-- 
1.8.3.1

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Tue, Jul 10, 2018 at 10:58 PM, Sreekanth Reddy
 wrote:
> On Mon, Jul 9, 2018 at 5:14 PM, Sreekanth Reddy
>  wrote:
>> On Thu, Jun 28, 2018 at 10:56 AM, Sreekanth Reddy
>>  wrote:
>>> On Thu, Jun 28, 2018 at 3:48 AM, Bart Van Assche  
>>> wrote:
>>>> On 06/24/18 23:10, Sreekanth Reddy wrote:
>>>>>
>>>>> Before calling scsi_internal_device_block_nowait() API; driver sets
>>>>> sas_device_priv_data->block flag as one. And in the scsih_qcmd()
>>>>> driver checks for this flag as shown below and return the commands
>>>>> with host busy status.
>>>>>
>>>>> } else if (sas_target_priv_data->tm_busy ||
>>>>>  sas_device_priv_data->block)
>>>>>  return SCSI_MLQUEUE_DEVICE_BUSY;
>>>>
>>>>
>>>> That's exactly the kind of construct that should occur in the SCSI core or
>>>> block layer core and not in a SCSI LLD. Additionally, as explained before,
>>>> the construct you described above is racy.
>>>
>>> Can you please elaborate more in details about the racy condition
>>> which you think?
>>> Also if at all is their any racy condition here then we are ready to
>>> work on it separately,
>>> So please consider the fix which we have posted.
>>>
>>> Thanks,
>>> Sreekanth
>>>
>>>
>>>
>>>
>>>>
>>>> Bart.
>>
>>
>> Any update regarding this patch?
>
> Pinging once again! please consider this patch fix. this is a very
> simple fix for the issue reported and doesn't have any side effort.


Any update on acceptance of this patch.

This is a very simple fix for the current issue  which is described in
the patch header, please consider this patch.

We will also review complete driver code path once again and we are
ready to fix if any racy conditions found while reviewing in coming
days.

Thanks,
Sreekanth

Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch

[Sreekanth Reddy]

On Wed, Jul 11, 2018 at 8:32 AM, Martin K. Petersen
 wrote:
>
>> Since this commit is buried a bit deep in the pile, please submit an
>> incremental patch that addresses Dave's problem.

This patch "Fix for regression caused due to cf6bf9710c" addresses the
Dave's problem.

>
> I obviously missed the first email of this thread. Sigh.

Whether I need to re-send this same patch?

>
> --
> Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Mon, Jul 9, 2018 at 5:14 PM, Sreekanth Reddy
 wrote:
> On Thu, Jun 28, 2018 at 10:56 AM, Sreekanth Reddy
>  wrote:
>> On Thu, Jun 28, 2018 at 3:48 AM, Bart Van Assche  
>> wrote:
>>> On 06/24/18 23:10, Sreekanth Reddy wrote:
>>>>
>>>> Before calling scsi_internal_device_block_nowait() API; driver sets
>>>> sas_device_priv_data->block flag as one. And in the scsih_qcmd()
>>>> driver checks for this flag as shown below and return the commands
>>>> with host busy status.
>>>>
>>>> } else if (sas_target_priv_data->tm_busy ||
>>>>  sas_device_priv_data->block)
>>>>  return SCSI_MLQUEUE_DEVICE_BUSY;
>>>
>>>
>>> That's exactly the kind of construct that should occur in the SCSI core or
>>> block layer core and not in a SCSI LLD. Additionally, as explained before,
>>> the construct you described above is racy.
>>
>> Can you please elaborate more in details about the racy condition
>> which you think?
>> Also if at all is their any racy condition here then we are ready to
>> work on it separately,
>> So please consider the fix which we have posted.
>>
>> Thanks,
>> Sreekanth
>>
>>
>>
>>
>>>
>>> Bart.
>
>
> Any update regarding this patch?

Pinging once again! please consider this patch fix. this is a very
simple fix for the issue reported and doesn't have any side effort.

Regards,
Sreekanth

>
> Thanks,
> Sreekanth

Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch

[Sreekanth Reddy]

On Tue, Jul 10, 2018 at 2:38 AM, David Miller  wrote:
> From: Sreekanth Reddy 
> Date: Mon, 9 Jul 2018 17:12:51 +0530
>
>> This is a shared structure between the host drivers and HBA device.
>> HBA Firmware sends the information though this structures which are
>> defined in the MPI headers. Now we can't change the order of these u8
>> objects.
>>
>> typedef struct _MPI2_DEFAULT_REPLY {
>> U16 FunctionDependent1; /*0x00 */
>> U8 MsgLength;   /*0x02 */
>> U8 Function;/*0x03 */
>> U16 FunctionDependent2; /*0x04 */
>
> When a big-endian cpu stores a 16-bit or 32-bit value into a piece of
> "cpu memory", which is what you are doing before interpreting the u8
> values, you must define the u8 components in the endianness order
> they will be stored in.
>
> Alternatively, you must extract the 8-bit values by hand using
> shifts and masks in a local variable.
>
> The huge problem is that you are mixing and matching I/O memory
> accessors which do endianness swaps for you, and normal cpu
> loads and stores into a data structure in cpu memory, then
> reading smaller sized components back and expects this all to
> work out properly.
>
> In fact, if you want to keep the structure definition above
> as-is, you should swap the I/O memory read value _BACK_ to
> cpu endianness before storing it.
>
> And in fact that is what the code is doing now, which you broke by
> trying to "fix" it.
>
> Meanwhile, this discussion has been drawn out way way way too long,
> and the fact is that your change broke big endian instead of fixing
> any real problem whatsoever.  You did not test your change on any real
> big endian system.
>
> Therefore, as requested from the very beginning, would you please
> revert this change which broke big endian and for which you do not
> have a satisfactory resolution for at this time.

I am trying to revert patch commit cf6bf9710c (scsi: mpt3sas: Bug fix
for big endian systems) but revert operation is not happening
smoothly, it may be due to dependency with some other patch changes.

Anyway in this patch we have reverted those hunks of cf6bf9710c patch
which are problematic other hunks of cf6bf9710c patch are good. Can't
this patch be considered? or shall I resend this patch with
description(i.e. patch header) change.

Thanks,
Sreekanth

>
> This is how regressions are handled, if a fix cannot be procured in a
> short amount of time, we revert.
>
> Thank you.

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Thu, Jun 28, 2018 at 10:56 AM, Sreekanth Reddy
 wrote:
> On Thu, Jun 28, 2018 at 3:48 AM, Bart Van Assche  
> wrote:
>> On 06/24/18 23:10, Sreekanth Reddy wrote:
>>>
>>> Before calling scsi_internal_device_block_nowait() API; driver sets
>>> sas_device_priv_data->block flag as one. And in the scsih_qcmd()
>>> driver checks for this flag as shown below and return the commands
>>> with host busy status.
>>>
>>> } else if (sas_target_priv_data->tm_busy ||
>>>  sas_device_priv_data->block)
>>>  return SCSI_MLQUEUE_DEVICE_BUSY;
>>
>>
>> That's exactly the kind of construct that should occur in the SCSI core or
>> block layer core and not in a SCSI LLD. Additionally, as explained before,
>> the construct you described above is racy.
>
> Can you please elaborate more in details about the racy condition
> which you think?
> Also if at all is their any racy condition here then we are ready to
> work on it separately,
> So please consider the fix which we have posted.
>
> Thanks,
> Sreekanth
>
>
>
>
>>
>> Bart.


Any update regarding this patch?

Thanks,
Sreekanth

Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch

[Sreekanth Reddy]

On Wed, Jul 4, 2018 at 5:52 PM, David Miller  wrote:
> From: Sreekanth Reddy 
> Date: Wed, 4 Jul 2018 16:54:05 +0530
>
>>
>> Also I tried replacing readl() API with readw()API (as HBA FW will
>> send 16 bit data at a time) as shown below and still I see same issue,
>>
>> MPI2DefaultReply_t *default_reply = (MPI2DefaultReply_t *)reply;
>> u16 reply1;
>> reply1 = readw(>chip->Doorbell);
>> reply[1] = reply1;
>>
>> printk("LSI debug.. 0x%x, 0x%x, 0x%x \n", reply1, reply[1]);
>> writel(0, >chip->HostInterruptStatus);
>>
>> printk("LSI MsgLength :%d\n", default_reply->MsgLength);
>>
>> And I got below output where message length is wrong, when I execute
>> above code on SPARC64 machine,
>
> It's the ordering of the u8 objects in the structure that is the problem,
> on big endian they need to be swapped.

David,

This is a shared structure between the host drivers and HBA device.
HBA Firmware sends the information though this structures which are
defined in the MPI headers. Now we can't change the order of these u8
objects.

typedef struct _MPI2_DEFAULT_REPLY {
U16 FunctionDependent1; /*0x00 */
U8 MsgLength;   /*0x02 */
U8 Function;/*0x03 */
U16 FunctionDependent2; /*0x04 */
U8 FunctionDependent3;  /*0x06 */
U8 MsgFlags;/*0x07 */
U8 VP_ID;   /*0x08 */
U8 VF_ID;   /*0x09 */
U16 Reserved1;  /*0x0A */
U16 FunctionDependent5; /*0x0C */
U16 IOCStatus;  /*0x0E */
U32 IOCLogInfo; /*0x10 */
} MPI2_DEFAULT_REPLY, *PTR_MPI2_DEFAULT_REPLY,
MPI2DefaultReply_t, *pMPI2DefaultReply_t;

Please let me know what I can do further.

Thanks,
Sreekanth

Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch

[Sreekanth Reddy]

On Tue, Jul 3, 2018 at 8:53 PM, James Bottomley
 wrote:
> On Tue, 2018-07-03 at 22:49 +0900, David Miller wrote:
>> From: Sreekanth Reddy 
>> Date: Tue, 3 Jul 2018 17:48:49 +0530
>>
>> > Any suggestion/update over my previous mail. I am using 4.13
>> kernel.
>>
>> I think the issue is that if you are reading a 32-bit word and then
>> interpreting it as a struct full of individual bytes, you have to
>> order the bytes in the structure appropriately for the cpu
>> endianness.
>
> This is undoubtedly it.  The point being if you read from a structure
> using readX, you have to read every element at its correct length for
> the endian swaps to work.  You can't do a readq on 2 32 bit words and
> expect the endianness to be correct (you'll find they come out in the
> wrong order).
>
> I think you're using a shared (device and cpu) memory mapped structured
> data with a doorbell register,

[Sreekanth] Yes it is correct, we are using a shared memory mapped
structured data with a doorbell register.
In this particular handshake function, HBA will send only 16 bit data
at a time. So driver has to read 16 bit data at a time in a loop until
complete reply message is read.


>  which is pretty identical to how the
> qla1280 does it.  We went through several iterations of fixing that
> driver for big endian but finally settled on putting __le annotations
> on all the structures and doing cpu_to_leX() swaps as we wrote them
> (and obviously leX_to_cpu() swaps to read them), meaning the structure
> in memory is always correct for the device.  Then we used a writeX to
> poke the doorbell and the device just picked up the correct
> information.
>
> The rule you want to be following is: memory mapped structure, you're
> responsible for annotation and swapping; readX/writeX to correctly
> sized data, the API will swap for you.
>
> So, can we just revert the original patch which is clearly now a
> regression and try to get this fixed in the merge window?
[Sreekanth] Yes we can revert the original patch.

>  I think the
> actual bug is simply you're missing __leX annotations on the shared
> memory mapped structure to fix sparse, but otherwise everything is
> working.

Actually our memory mapped structure variables are declared with
__leX annotations as shown below,

typedef struct _MPI2_DEFAULT_REPLY {
U16 FunctionDependent1; /*0x00 */
U8 MsgLength;   /*0x02 */
U8 Function;/*0x03 */
U16 FunctionDependent2; /*0x04 */
U8 FunctionDependent3;  /*0x06 */
U8 MsgFlags;/*0x07 */
U8 VP_ID;   /*0x08 */
U8 VF_ID;   /*0x09 */
U16 Reserved1;  /*0x0A */
U16 FunctionDependent5; /*0x0C */
U16 IOCStatus;  /*0x0E */
U32 IOCLogInfo; /*0x10 */
} MPI2_DEFAULT_REPLY, *PTR_MPI2_DEFAULT_REPLY,
MPI2DefaultReply_t, *pMPI2DefaultReply_t;

and

typedef u8 U8;
typedef __le16 U16;
typedef __le32 U32;
typedef __le64 U64 __attribute__ ((aligned(4)));

Also I tried replacing readl() API with readw()API (as HBA FW will
send 16 bit data at a time) as shown below and still I see same issue,

MPI2DefaultReply_t *default_reply = (MPI2DefaultReply_t *)reply;
u16 reply1;
reply1 = readw(>chip->Doorbell);
reply[1] = reply1;

printk("LSI debug.. 0x%x, 0x%x, 0x%x \n", reply1, reply[1]);
writel(0, >chip->HostInterruptStatus);

printk("LSI MsgLength :%d\n", default_reply->MsgLength);

And I got below output where message length is wrong, when I execute
above code on SPARC64 machine,

LSI debug.. 0x311, 0x311
LSI MsgLength :3

Thanks,
Sreekanth

>
> James
>

Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch

[Sreekanth Reddy]

Hi,

Any suggestion/update over my previous mail. I am using 4.13 kernel.

Thanks,
Sreekanth

On Sat, Jun 30, 2018 at 12:34 AM, Sreekanth Reddy
 wrote:
> Hi All,
>
> Here is the issue which we are observing when driver don't use
> le16_to_cpu() in below code snippet on Sparc64 machine when driver is
> reading 2 bytes of data which is posted by HBA firmware,
>
> u32 reply1;
> reply1 = readl(>chip->Doorbell);
> reply[1] = (reply1 & MPI2_DOORBELL_DATA_MASK);
>
> printk("LSI debug.. 0x%x, 0x%x, 0x%x \n", reply1, reply[1]);
> writel(0, >chip->HostInterruptStatus);
>
> printk("LSI MsgLength :%d\n", default_reply->MsgLength);
>
> When I execute above code I got below output on Sparc64 machine,
>
> LSI debug.. 0x1c000311, 0x311
> LSI MsgLength :3
>
> When I execute same code in x86 machine then I got below output,
>
> LSI debug.. 0x1c000311, 0x311
> LSI MsgLength :17
>
> Correct message (Here I am referring IOCFacts message) Length is 17
> words. But on Sparc64 machine we got message length as 3 words which
> is wrong.
>
> Here is data structure of default reply message,
>
> typedef struct _MPI2_DEFAULT_REPLY {
> U16 FunctionDependent1; /*0x00 */
> U8 MsgLength;   /*0x02 */
> U8 Function;/*0x03 */
> U16 FunctionDependent2; /*0x04 */
> U8 FunctionDependent3;  /*0x06 */
> U8 MsgFlags;/*0x07 */
> U8 VP_ID;   /*0x08 */
> U8 VF_ID;   /*0x09 */
> U16 Reserved1;  /*0x0A */
> U16 FunctionDependent5; /*0x0C */
> U16 IOCStatus;  /*0x0E */
> U32 IOCLogInfo; /*0x10 */
> } MPI2_DEFAULT_REPLY, *PTR_MPI2_DEFAULT_REPLY,
> MPI2DefaultReply_t, *pMPI2DefaultReply_t;
>
> Until host reads correct number of reply words, IOC won't clear
> Doorbel Used bit and hence we see below error message while loading
> the driver and IOC initialization fails.
>
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_get_ioc_facts
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_wait_for_iocstate
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: doorbell is in use (line=5241)
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_get_ioc_facts:
> handshake failed (r=-14)
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: mpt3sas_base_free_resources
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_make_ioc_ready
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: mpt3sas_base_unmap_resources
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_release_memory_pools
> Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: failure at
> /home/chaitra/mpt3sas_with_sparse_patch/mpt3sas_scsih.c:10776/_scsih_probe()
>
>
> Thanks,
> Sreekanth
>
> On Sat, Jun 30, 2018 at 12:06 AM, Andy Shevchenko
>  wrote:
>> On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley
>>  wrote:
>>> On Fri, 2018-06-29 at 10:58 -0400, Chaitra P B wrote:
>>>>  "scsi: mpt3sas: Bug fix for big endian systems"
>>>>
>>>> Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc"
>>>> was posted to fix sparse warnings. While posting this patch it was
>>>> assumed that readl() & writel() APIs internally calls le32_to_cpu() &
>>>> cpu_to_le32() APIs respectively. Looks like it is not true for all
>>>> architecture
>>>
>>> Just a minute, it damn well should be.  The definition of readl/writel
>>> is barriers and little endian (you can see this in asm-generic/io.h).
>>>
>>> Which architecture is getting this wrong?  Because it sounds like
>>> that's what we need to fix rather than doing something like this in all
>>> drivers.
>>>
>>> Sparc (and parisc) definitely do the little endian thing, so if this
>>> code is what it takes to get them working again, it looks like you're
>>> double swapping somewhere.  I really think cf6bf9710c needs to be
>>> reverted and you should look again at your sparse warnings.  Since the
>>> driver is using the non-raw readX/writeX it should be cpu endian
>>> internally which cf6bf9710c upsets.
>>
>> And we definitely won't see the constructions like
>> writeq(cpu_to_le64()) in the code, because it's weird.
>> If I get it correctly it's equivalent to __raw_writeq().
>>
>> --
>> With Best Regards,
>> Andy Shevchenko

Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch

[Sreekanth Reddy]

Hi All,

Here is the issue which we are observing when driver don't use
le16_to_cpu() in below code snippet on Sparc64 machine when driver is
reading 2 bytes of data which is posted by HBA firmware,

u32 reply1;
reply1 = readl(>chip->Doorbell);
reply[1] = (reply1 & MPI2_DOORBELL_DATA_MASK);

printk("LSI debug.. 0x%x, 0x%x, 0x%x \n", reply1, reply[1]);
writel(0, >chip->HostInterruptStatus);

printk("LSI MsgLength :%d\n", default_reply->MsgLength);

When I execute above code I got below output on Sparc64 machine,

LSI debug.. 0x1c000311, 0x311
LSI MsgLength :3

When I execute same code in x86 machine then I got below output,

LSI debug.. 0x1c000311, 0x311
LSI MsgLength :17

Correct message (Here I am referring IOCFacts message) Length is 17
words. But on Sparc64 machine we got message length as 3 words which
is wrong.

Here is data structure of default reply message,

typedef struct _MPI2_DEFAULT_REPLY {
U16 FunctionDependent1; /*0x00 */
U8 MsgLength;   /*0x02 */
U8 Function;/*0x03 */
U16 FunctionDependent2; /*0x04 */
U8 FunctionDependent3;  /*0x06 */
U8 MsgFlags;/*0x07 */
U8 VP_ID;   /*0x08 */
U8 VF_ID;   /*0x09 */
U16 Reserved1;  /*0x0A */
U16 FunctionDependent5; /*0x0C */
U16 IOCStatus;  /*0x0E */
U32 IOCLogInfo; /*0x10 */
} MPI2_DEFAULT_REPLY, *PTR_MPI2_DEFAULT_REPLY,
MPI2DefaultReply_t, *pMPI2DefaultReply_t;

Until host reads correct number of reply words, IOC won't clear
Doorbel Used bit and hence we see below error message while loading
the driver and IOC initialization fails.

Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_get_ioc_facts
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_wait_for_iocstate
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: doorbell is in use (line=5241)
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_get_ioc_facts:
handshake failed (r=-14)
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: mpt3sas_base_free_resources
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_make_ioc_ready
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: mpt3sas_base_unmap_resources
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_release_memory_pools
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: failure at
/home/chaitra/mpt3sas_with_sparse_patch/mpt3sas_scsih.c:10776/_scsih_probe()


Thanks,
Sreekanth

On Sat, Jun 30, 2018 at 12:06 AM, Andy Shevchenko
 wrote:
> On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley
>  wrote:
>> On Fri, 2018-06-29 at 10:58 -0400, Chaitra P B wrote:
>>>  "scsi: mpt3sas: Bug fix for big endian systems"
>>>
>>> Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc"
>>> was posted to fix sparse warnings. While posting this patch it was
>>> assumed that readl() & writel() APIs internally calls le32_to_cpu() &
>>> cpu_to_le32() APIs respectively. Looks like it is not true for all
>>> architecture
>>
>> Just a minute, it damn well should be.  The definition of readl/writel
>> is barriers and little endian (you can see this in asm-generic/io.h).
>>
>> Which architecture is getting this wrong?  Because it sounds like
>> that's what we need to fix rather than doing something like this in all
>> drivers.
>>
>> Sparc (and parisc) definitely do the little endian thing, so if this
>> code is what it takes to get them working again, it looks like you're
>> double swapping somewhere.  I really think cf6bf9710c needs to be
>> reverted and you should look again at your sparse warnings.  Since the
>> driver is using the non-raw readX/writeX it should be cpu endian
>> internally which cf6bf9710c upsets.
>
> And we definitely won't see the constructions like
> writeq(cpu_to_le64()) in the code, because it's weird.
> If I get it correctly it's equivalent to __raw_writeq().
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Thu, Jun 28, 2018 at 3:48 AM, Bart Van Assche  wrote:
> On 06/24/18 23:10, Sreekanth Reddy wrote:
>>
>> Before calling scsi_internal_device_block_nowait() API; driver sets
>> sas_device_priv_data->block flag as one. And in the scsih_qcmd()
>> driver checks for this flag as shown below and return the commands
>> with host busy status.
>>
>> } else if (sas_target_priv_data->tm_busy ||
>>  sas_device_priv_data->block)
>>  return SCSI_MLQUEUE_DEVICE_BUSY;
>
>
> That's exactly the kind of construct that should occur in the SCSI core or
> block layer core and not in a SCSI LLD. Additionally, as explained before,
> the construct you described above is racy.

Can you please elaborate more in details about the racy condition
which you think?
Also if at all is their any racy condition here then we are ready to
work on it separately,
So please consider the fix which we have posted.

Thanks,
Sreekanth




>
> Bart.

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Sat, Jun 23, 2018 at 2:56 AM, Bart Van Assche  wrote:
> On 06/22/18 09:38, Sreekanth Reddy wrote:
>>
>> In driver's .resume() callback function, driver is doing IOC reset
>> operation. And as per your suggestion we tried using
>> scsi_internal_device_block_nowait() to block the all the devices
>> attached to the HBA before going for IOC reset operation. During
>> system resume time as drives will be in quiesce state and calling
>> scsi_internal_device_block_nowait() return with error status -22.
>>
>> So you suggested us to try using lock_system_sleep() API before
>> calling scsi_internal_device_block_nowait(), so that we don't get -22
>> return status when we call scsi_internal_device_block_nowait() API
>> during resume time. We tried the same and we see system get hang
>> during hibernation. Please correct me if I am wrong or if I have
>> wrongly understood your suggestions.
>>
>> I feel that the fix which have posted is the better fix then using
>> scsi_internal_device_block_nowait()/scsi_internal_device_unblock_nowait()
>> APIs.
>
>
> Hello Sreekanth,
>
> It seems like there is a misunderstanding: what I proposed was to use
> lock_system_sleep() to delay hibernation until unlock_system_sleep() has
> been called. I had not realized that the mpt3sas driver can call
> scsi_internal_device_block_nowait() during system resume.
>
> There is another issue with the scsi_internal_device_block_nowait() calls by
> the mpt3sas driver: callers like _scsih_sas_broadcast_primitive_event() seem
> to assume that all scsih_qcmd() calls have finished as soon as
> scsi_internal_device_block_nowait() has returned. However, that assumption
> is not correct, especially when using scsi-mq.
>

Hello Bart,

Before calling scsi_internal_device_block_nowait() API; driver sets
sas_device_priv_data->block flag as one. And in the scsih_qcmd()
driver checks for this flag as shown below and return the commands
with host busy status.

} else if (sas_target_priv_data->tm_busy ||
sas_device_priv_data->block)
return SCSI_MLQUEUE_DEVICE_BUSY;

 Also as a part of processing the braodcast primitive event driver
issues the task query TM to determine where the IO command is and will
only abort those IOs (by issuing task abort TM) which are queued in
the target devices. Hope I have clarified the issue which you have
pointed out.

> If the patch "mpt3sas: Fix calltrace observed while running IO & host reset"
> would be allowed upstream, will the issues mentioned above ever be
> addressed?
If their are any issues we are always avilable to address them.


Thanks,
Sreekanth

>
> Bart.

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Fri, Jun 22, 2018 at 8:22 PM, Bart Van Assche  wrote:
> On 06/21/18 22:35, Sreekanth Reddy wrote:
>>
>> No, lock_system_sleep() is not inserted in the interrupt context. we
>> have inserted it in .resume() call back function just before issuing
>> the IOC reset.
>
>
> That's the wrong place to insert a lock_system_sleep() call. Please have a
> look at drivers/scsi/scsi_transport_spi.c for an example of how to use that
> function.
>

I am totally confused.

In driver's .resume() callback function, driver is doing IOC reset
operation. And as per your suggestion we tried using
scsi_internal_device_block_nowait() to block the all the devices
attached to the HBA before going for IOC reset operation. During
system resume time as drives will be in quiesce state and calling
scsi_internal_device_block_nowait() return with error status -22.

So you suggested us to try using lock_system_sleep() API before
calling scsi_internal_device_block_nowait(), so that we don't get -22
return status when we call scsi_internal_device_block_nowait() API
during resume time. We tried the same and we see system get hang
during hibernation. Please correct me if I am wrong or if I have
wrongly understood your suggestions.

I feel that the fix which have posted is the better fix then using
scsi_internal_device_block_nowait()/scsi_internal_device_unblock_nowait()
APIs.

Thanks,
Sreekanth



> Thanks,
>
> Bart.

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Thu, Jun 21, 2018 at 7:49 PM, Bart Van Assche  wrote:
> On Thu, 2018-06-21 at 15:41 +0530, Sreekanth Reddy wrote:
>> Bart, we tried using lock_system_sleep() before calling IOC reset
>> operation in .resume() callback function and unlock_system_sleep()
>> after the IOC reset. With this code change we see system is going to
>> hang state during hibernation and we just see below messages,
>>
>> [  625.788598] PM: hibernation entry
>> Jun 21 05:37:33 localhost kernel: PM: hibernation entry
>> [  627.428159] PM: Syncing filesystems ...
>> Jun 21 05:37:34 localhost kernel: PM: Syncing filesystems ...
>> [  628.756119] PM: done.
>> [  628.758707] Freezing user space processes ... (elapsed 0.001 seconds) 
>> done.
>> [  628.768340] OOM killer disabled.
>> [  628.772010] PM: Preallocating image memory... done (allocated 197704 
>> pages)
>> [  632.554470] PM: Allocated 790816 kbytes in 3.77 seconds (209.76 MB/s)
>> [  632.561664] Freezing remaining freezable tasks ... (elapsed 0.002
>> seconds) done.
>> [  632.572269] Suspending console(s) (use no_console_suspend to debug)
>>
>>
>> The fix which we have posted looks simple and we don't see any side
>> effects of it.
>> We have done complete regression testing on our fix and we don't see
>> any issue with it. So please consider our fix which have posted.
>
> scsi_internal_device_block_nowait() can be called by the mpt3sas driver from
> interrupt context. lock_system_sleep() locks a mutex and hence must not be
> called from interrupt context. Does the above mean that (a) a call to
> lock_system_sleep() was inserted in an interrupt handler and

No, lock_system_sleep() is not inserted in the interrupt context. we
have inserted it in .resume() call back function just before issuing
the IOC reset.

(b) that the
> above test was performed with a kernel in which lockdep was disabled?

No, lockdep is enabled in the kernel.


~Sreekanth
>
> Bart.
>
>
>

Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset

[Sreekanth Reddy]

On Wed, Jun 20, 2018 at 11:43 PM, Bart Van Assche
 wrote:
> On Wed, 2018-06-20 at 09:18 +0530, Chaitra Basappa wrote:
>> We have tried with calling scsi_internal_device_block_nowait() API before
>> doing IOC reset (i.e. host reset) and called
>> scsi_internal_device_unblock_nowait() after performing IOC reset.
>> We have tested this code change with various test cases such as
>> adding/removing target drives or expanders during diag reset with and
>> without IOs and at high level we see all are working but we observe below
>> error messages while performing hibernation operation,
>>
>> sd 1:0:0:0: device_block, handle(0x0028)
>> BRCM Debug: sdev->sdev_state: 5 before device_block_nowait
>> BRCM Debug: sdev->sdev_state: 5 after_device_block_nowait
>> sd 1:0:0:0: device_block failed with return(-22) for handle(0x0028)
>> .
>> .
>> sd 0:0:0:0: device_unblock and setting to running, handle(0x0028)
>> sd 0:0:0:0: device_unblock failed with return(-22) for handle(0x0028)
>> performing a block followed by an unblock
>> sd 0:0:0:0: retried device_block failed with return(-22) for handle(0x0028)
>> sd 0:0:0:0: retried device_unblock failed with return(-22) for
>> handle(0x0028)
>>
>> We are observing these messages during of system resume time, during which
>> driver issues IOC reset operation in the .resume() callback function.
>> In the above error messages we see that drives are in SDEV_QUIESCE state.
>> When drives are SDEV_QUIESCE state then moving these drives to
>> SDEV_BLOCK state is not allowed and hence we observe above error messages.
>>
>> SDEV_QUIESCE state means that Device quiescent. No block commands will be
>> accepted, only specials (which originate in the midlayer).
>
> Neither scsi_internal_device_block_nowait() nor
> scsi_internal_device_unblock_nowait() should ever have been changed from
> static into exported functions. But that's another discussion. Regarding the
> adverse interaction of scsi_internal_device_block_nowait() and
> scsi_internal_device_unblock_nowait() with the power management code, have
> you considered to surround code that blocks and unblocks SCSI devices with
> lock_system_sleep() / unlock_system_sleep() to avoid that these functions
> fail with error code -22?
>

Bart, we tried using lock_system_sleep() before calling IOC reset
operation in .resume() callback function and unlock_system_sleep()
after the IOC reset. With this code change we see system is going to
hang state during hibernation and we just see below messages,

[  625.788598] PM: hibernation entry
Jun 21 05:37:33 localhost kernel: PM: hibernation entry
[  627.428159] PM: Syncing filesystems ...
Jun 21 05:37:34 localhost kernel: PM: Syncing filesystems ...
[  628.756119] PM: done.
[  628.758707] Freezing user space processes ... (elapsed 0.001 seconds) done.
[  628.768340] OOM killer disabled.
[  628.772010] PM: Preallocating image memory... done (allocated 197704 pages)
[  632.554470] PM: Allocated 790816 kbytes in 3.77 seconds (209.76 MB/s)
[  632.561664] Freezing remaining freezable tasks ... (elapsed 0.002
seconds) done.
[  632.572269] Suspending console(s) (use no_console_suspend to debug)


The fix which we have posted looks simple and we don't see any side
effects of it.
We have done complete regression testing on our fix and we don't see
any issue with it. So please consider our fix which have posted.

Thanks,
Sreekanth


> Thanks,
>
> Bart.
>
>
>

RE: [PATCH v3 00/14] mpt3sas: Enhancements and Defect fixes.

[Sreekanth Reddy]

Hi,

Any update on this patch set.   

Thanks,
Sreekanth

-Original Message-
From: Chaitra P B [mailto:chaitra.basa...@broadcom.com]
Sent: Tuesday, April 24, 2018 2:58 PM
To: linux-scsi@vger.kernel.org
Cc: sathya.prak...@broadcom.com; sreekanth.re...@broadcom.com;
suganath-prabu.subram...@broadcom.com; Chaitra P B
Subject: [PATCH v3 00/14] mpt3sas: Enhancements and Defect fixes.

Chaitra P B (14):
  mpt3sas: Bug fix for big endian systems.
  mpt3sas: Pre-allocate RDPQ Array at driver boot time.
  mpt3sas: Lockless access for chain buffers.
  mpt3sas: Optimize I/O memory consumption in driver.
  mpt3sas: Enhanced handling of Sense Buffer.
  mpt3sas: Added support for SAS Device Discovery Error Event.
  mpt3sas: Increase event log buffer to support 24 port HBA's.
  mpt3sas: Allow processing of events during driver unload.
  mpt3sas: Cache enclosure pages during enclosure add.
  mpt3sas: Report Firmware Package Version from HBA Driver.
  mpt3sas: Update MPI Headers
  mpt3sas: For NVME device, issue a protocol level reset instead of
hot reset and use TM timeout value exposed in PCIe Device Page  2.
  mpt3sas: fix possible memory leak.
  mpt3sas: Update driver version "25.100.00.00"

 drivers/scsi/mpt3sas/mpi/mpi2.h  |   9 +-
 drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h |  30 +-
 drivers/scsi/mpt3sas/mpi/mpi2_init.h |   2 +-
 drivers/scsi/mpt3sas/mpi/mpi2_ioc.h  |   7 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c  | 475
+++---
 drivers/scsi/mpt3sas/mpt3sas_base.h  |  60 +++-
 drivers/scsi/mpt3sas/mpt3sas_ctl.c   |  33 ++-
 drivers/scsi/mpt3sas/mpt3sas_ctl.h   |   2 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 491
+--
 drivers/scsi/mpt3sas/mpt3sas_warpdrive.c |   3 +-
 10 files changed, 816 insertions(+), 296 deletions(-)

-- 
1.8.3.1

Re: [PATCH v2 01/14] mpt3sas: Bug fix for big endian systems.

[Sreekanth Reddy]

On Wed, May 2, 2018 at 9:21 AM, Martin K. Petersen
 wrote:
>
> Hi Chaitra,
>
>>>  for (i = 0; i < ioc->combined_reply_index_count; i++) {
>>> -ioc->replyPostRegisterIndex[i] = (resource_size_t
>> *)
>>> - ((u8 *)>chip->Doorbell +
>>> +ioc->replyPostRegisterIndex[i] =
>>> +(volatile void __iomem *)
>>> + ((u8 __force *)>chip->Doorbell +
>>>   MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
>>>   (i *
>> MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET));
>>
>> Why the double type casts? You've already changed replyPostRegisterIndex
>> to be 'volatile void __iomem **' in the header file. So why not:
>>
>> ioc->replyPostRegisterIndex[i] =
>> >chip->Doorbell +
>> MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
>> i * 
>> MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET;
>
> You didn't address my question about why the type casts were required in
> the first place? I get cautious when I see nested casting...
>

Martin,

In v3 patch,we have removed this nested casting and just used (u8
__force) to fix the sparse warning.

~ Sreekanth


>> Also looks like ioc->reply_post_host_index handling a few lines further
>> down could lose the type casts.
>
> See above.
>
> --
> Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] block: ratelimite pr_err on IO path

[Sreekanth Reddy]

On Mon, Apr 16, 2018 at 1:46 PM, Jinpu Wang  wrote:
> On Fri, Apr 13, 2018 at 6:59 PM, Martin K. Petersen
>  wrote:
>>
>> Jinpu,
>>
>> [CC:ed the mpt3sas maintainers]
>>
>> The ratelimit patch is just an attempt to treat the symptom, not the
>> cause.
> Agree. If we can fix the root cause, it will be great.
>>
>>> Thanks for asking, we updated mpt3sas driver which enables DIX support
>>> (prot_mask=0x7f), all disks are SATA SSDs, no DIF support.
>>> After reboot, kernel reports the IO errors from all the drives behind
>>> HBA, seems for almost every read IO, which turns the system unusable:
>>> [   13.079375] sda: ref tag error at location 0 (rcvd 143196159)
>>> [   13.079989] sda: ref tag error at location 937702912 (rcvd 143196159)
>>> [   13.080233] sda: ref tag error at location 937703072 (rcvd 143196159)
>>> [   13.080407] sda: ref tag error at location 0 (rcvd 143196159)
>>> [   13.080594] sda: ref tag error at location 8 (rcvd 143196159)
>>
>> That sounds like a bug in the mpt3sas driver or firmware. I guess the
>> HBA could conceivably be operating a SATA device as DIX Type 0 and strip
>> the PI on the drive side. But that doesn't seem to be a particularly
>> useful mode of operation.
>>
>> Jinpu: Which firmware are you running? Also, please send us the output
>> of:
>>
>> sg_readcap -l /dev/sda
>> sg_inq -x /dev/sda
>> sg_vpd /dev/sda
>>
> Disks are INTEL SSDSC2BX48, directly attached to HBA.
> LSISAS3008: FWVersion(13.00.00.00), ChipRevision(0x02), 
> BiosVersion(08.11.00.00)
> mpt3sas_cm2: Protocol=(Initiator,Target),
> Capabilities=(TLR,EEDP,Snapshot Buffer,Diag Trace Buffer,Task Set
> Full,NCQ)
>
> jwang@x:~$ sudo sg_vpd /dev/sdz
> Supported VPD pages VPD page:
>   Supported VPD pages [sv]
>   Unit serial number [sn]
>   Device identification [di]
>   Mode page policy [mpp]
>   ATA information (SAT) [ai]
>   Block limits (SBC) [bl]
>   Block device characteristics (SBC) [bdc]
>   Logical block provisioning (SBC) [lbpv]
> jwang@x:~$ sudo sg_inq -x /dev/sdz
> VPD INQUIRY: extended INQUIRY data page
> inquiry: field in cdb illegal (page not supported)
> jwang@x:~$ sudo sg_readcap -l /dev/sdz
> Read Capacity results:
>Protection: prot_en=0, p_type=0, p_i_exponent=0
>Logical block provisioning: lbpme=1, lbprz=1
>Last logical block address=937703087 (0x37e436af), Number of
> logical blocks=937703088
>Logical block length=512 bytes
>Logical blocks per physical block exponent=3 [so physical block
> length=4096 bytes]
>Lowest aligned logical block address=0
> Hence:
>Device size: 480103981056 bytes, 457862.8 MiB, 480.10 GB
>
>
>> Broadcom: How is DIX supposed to work for SATA drives behind an mpt3sas
>> controller?

[Sreekanth] Current Upstream mpt3sas driver doesn't have DIX support
capabilities,
it supports only DIF feature.


Thanks,
Sreekanth
>>
>> --
>> Martin K. Petersen  Oracle Linux Engineering
>
>
> Thanks!
>
> --
> Jack Wang
> Linux Kernel Developer
>
> ProfitBricks GmbH
> Greifswalder Str. 207
> D - 10405 Berlin

RE: [PATCH v1 14/15] mpt3sas: fix possible memory leak.

[Sreekanth Reddy]

Yes Sasha, We like to have this patch included in a stable tree.

Thanks,
Sreekanth

-Original Message-
From: Sasha Levin [mailto:alexander.le...@microsoft.com]
Sent: Tuesday, April 10, 2018 7:19 PM
To: Sasha Levin; Chaitra P B; linux-scsi@vger.kernel.org
Cc: sathya.prak...@broadcom.com; sreekanth.re...@broadcom.com;
sta...@vger.kernel.org
Subject: Re: [PATCH v1 14/15] mpt3sas: fix possible memory leak.

Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined to
be a high probability candidate for -stable trees. (score: 12.9808)

The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33,
v4.9.93, v4.4.127.

v4.16.1: Build OK!
v4.15.16: Build OK!
v4.14.33: Build OK!
v4.9.93: Build OK!
v4.4.127: Build OK!

Please let us know if you'd like to have this patch included in a stable
tree.

--
Thanks,
Sasha

RE: [PATCH v1 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.

[Sreekanth Reddy]

Yes Sasha, We like to have this patch included in a stable tree.

Thanks,
Sreekanth

-Original Message-
From: Sasha Levin [mailto:alexander.le...@microsoft.com]
Sent: Tuesday, April 10, 2018 7:19 PM
To: Sasha Levin; Chaitra P B; linux-scsi@vger.kernel.org
Cc: sathya.prak...@broadcom.com; sreekanth.re...@broadcom.com;
sta...@vger.kernel.org
Subject: Re: [PATCH v1 03/15] mpt3sas: Add sanity checks for scsi tracker
before accessing it.

Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined to
be a high probability candidate for -stable trees. (score: 28.7865)

The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33,
v4.9.93, v4.4.127.

v4.16.1: Build OK!
v4.15.16: Failed to apply! Possible dependencies:
dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.14.33: Failed to apply! Possible dependencies:
dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.9.93: Failed to apply! Possible dependencies:
dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.4.127: Failed to apply! Possible dependencies:
dbec4c9040ed ("scsi: mpt3sas: lockless command submission")


Please let us know if you'd like to have this patch included in a stable
tree.

--
Thanks,
Sasha

Re: [PATCH 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.

[Sreekanth Reddy]

On Tue, Apr 3, 2018 at 9:26 PM, Bart Van Assche <bart.vanass...@wdc.com> wrote:
> On Tue, 2018-04-03 at 10:11 +0530, Sreekanth Reddy wrote:
>> [SR] No, driver calls _scsih_flush_running_cmds during Host reset time
>> and during host reset time driver will set 'ioc->shost_recovery' flag.
>> So in the scsih_qcmd() driver will return the incoming SCSI cmds with
>> "SCSI_MLQUEUE_HOST_BUSY" whenever 'ioc->shost_recovery' flag is set as
>> shown below,
>>
>> /* host recovery or link resets sent via IOCTLs */
>> if (ioc->shost_recovery || ioc->ioc_link_reset_in_progress)
>> return SCSI_MLQUEUE_HOST_BUSY;
>
> The ioc->shost_recovery flag is involved in at least the following race:
> * From one context a SCSI command is submitted and scsih_qcmd() gets called.
> * At the same time sg_reset is invoked from a shell and triggers a call to
>   scsih_host_reset(). That function in turn will call
>   mpt3sas_base_hard_reset_handler().
>
> I think this scenario can cause ioc->shost_recovery to be set by the mpt3sas
> driver after it has been checked by the scsih_qcmd() function.

Then these outstanding commands will be get flush by the driver in
_scsih_flush_running_cmds() function which we call as a part of host
reset operation.

>
> Anyway, let's get back to patch 03/15 at the start of this e-mail thread.
> That patch looks to me like an incomplete attempt to work around a race
> condition in the mpt3sas driver. I don't expect that anyone will trust that
> patch without further explanation. Which race condition does that patch
> address? And why do the mpt3sas maintainers believe that this patch is
> sufficient to address that race condition?

Sure Bart, we will add proper description with the information which I
explained in this mail thread on how this patch will fix below issue,

During Host reset operation time driver will
flush out all the outstanding IO's to the SML in below function,

static void
_scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
{
struct scsi_cmnd *scmd;
struct scsiio_tracker *st;
u16 smid;
int count = 0;

[SR] Here driver is looping starting from smid one to HBA queue depth.
for (smid = 1; smid <= ioc->scsiio_depth; smid++) {

[SR] Some times it is possible that scsi_cmnd might have created at
SML but it might not be issued to the driver as host reset operation
is going on, So here we will get non-NULL scmd.
scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
if (!scmd)
continue;
count++;
_scsih_set_satl_pending(scmd, false);
[SR] Here we are trying to get the scsi tracker 'st' for the above
scmd (which is not received by the driver) and so fields of this 'st'
are uninitialized.
st = scsi_cmd_priv(scmd);
[SR] And here we are trying to clear the scsi tracker 'st' which is
not yet all initialized by the driver, in other terms we are trying to
flush out the scmd command which is not yet all received by the
driver.
mpt3sas_base_clear_st(ioc, st);
scsi_dma_unmap(scmd);
if (ioc->pci_error_recovery || ioc->remove_host)
scmd->result = DID_NO_CONNECT << 16;
else
scmd->result = DID_RESET << 16;
scmd->scsi_done(scmd);
}
dtmprintk(ioc, pr_info(MPT3SAS_FMT "completing %d cmds\n",
ioc->name, count));
}

>
> Thanks,
>
> Bart.
>

Re: [PATCH 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.

[Sreekanth Reddy]

On Mon, Apr 2, 2018 at 8:55 PM, Bart Van Assche <bart.vanass...@wdc.com> wrote:
> On Mon, 2018-04-02 at 11:53 +0530, Sreekanth Reddy wrote:
>> On Fri, Mar 30, 2018 at 5:29 PM, Christoph Hellwig <h...@infradead.org> 
>> wrote:
>> > On Fri, Mar 30, 2018 at 03:07:12PM +0530, Chaitra P B wrote:
>> > > Check scsi tracker for NULL before accessing it.
>> > > And in some places there are possibilities for getting valid st
>> > > but still other fields are not set.
>> >
>> > Please explain how that could ever happen.  You should never see
>> > a scsi_cmnd without the device pointer.
>>
>> Chris,
>>
>> Here is one example, During Host reset operation time driver will
>> flush out all the outstanding IO's to the SML in below function,
>>
>> static void
>> _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
>> {
>> struct scsi_cmnd *scmd;
>> struct scsiio_tracker *st;
>> u16 smid;
>> int count = 0;
>>
>> [SR] Here driver is looping starting from smid one to HBA queue depth.
>> for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
>>
>> [SR] Some times it is possible that scsi_cmnd might have created at
>> SML but it might not be issued to the driver as host reset operation
>> is going on, So here we will get non-NULL scmd.
>> scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
>> if (!scmd)
>> continue;
>> count++;
>> _scsih_set_satl_pending(scmd, false);
>> [SR] Here we are trying to get the scsi tracker 'st' for the above
>> scmd (which is not received by the driver) and so fields of this 'st'
>> are uninitialized.
>> st = scsi_cmd_priv(scmd);
>> [SR] And here we are trying to clear the scsi tracker 'st' which is
>> not yet all initialized by the driver, in other terms we are trying to
>> flush out the scmd command which is not yet all received by the
>> driver.
>> mpt3sas_base_clear_st(ioc, st);
>> scsi_dma_unmap(scmd);
>> if (ioc->pci_error_recovery || ioc->remove_host)
>> scmd->result = DID_NO_CONNECT << 16;
>> else
>> scmd->result = DID_RESET << 16;
>> scmd->scsi_done(scmd);
>> }
>> dtmprintk(ioc, pr_info(MPT3SAS_FMT "completing %d cmds\n",
>> ioc->name, count));
>> }
>
> Hello Sreekanth,
>
> From mpt3sas_scsih_scsi_lookup_get():
>
> st = scsi_cmd_priv(scmd);
> if (st->cb_idx == 0xFF)
> scmd = NULL;
>
> From mpt3sas_base_get_smid(), mpt3sas_base_get_smid_scsiio() and
> mpt3sas_base_get_smid_hpr():
>
> request->cb_idx = cb_idx;
>
> Can _scsih_flush_running_cmds() be executed concurrently with scsih_qcmd()?

[SR] No, driver calls _scsih_flush_running_cmds during Host reset time
and during host reset time driver will set 'ioc->shost_recovery' flag.
So in the scsih_qcmd() driver will return the incoming SCSI cmds with
"SCSI_MLQUEUE_HOST_BUSY" whenever 'ioc->shost_recovery' flag is set as
shown below,

/* host recovery or link resets sent via IOCTLs */
if (ioc->shost_recovery || ioc->ioc_link_reset_in_progress)
return SCSI_MLQUEUE_HOST_BUSY;

> In other words, can it happen that mpt3sas_scsih_scsi_lookup_get() sees that
> st->cb_idx == 0xff just before scsih_qcmd() assigns a value to .cb_idx? Can
> this cause _scsih_flush_running_cmds() to skip commands that it shouldn't
> skip?
[SR] No, it won't happen. as I explained above during host reset time
driver return the incoming SCSI commands with host busy status and
_scsih_flush_running_cmds called during the host reset time.

>
> Can it happen that _scsih_flush_running_cmds() calls .scsi_done() for a SCSI
> command just after scsih_qcmd() transferred control for that command to the
> firmware? Can that cause .scsi_done() to be called twice for the same command?
[SR] No, while _scsih_flush_running_cmds() function is getting
executed no SCSI commands are issued to the firmware.

>
> Thanks,
>
> Bart.
>
>

Re: [PATCH 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.

[Sreekanth Reddy]

On Fri, Mar 30, 2018 at 5:29 PM, Christoph Hellwig  wrote:
> On Fri, Mar 30, 2018 at 03:07:12PM +0530, Chaitra P B wrote:
>> Check scsi tracker for NULL before accessing it.
>> And in some places there are possibilities for getting valid st
>> but still other fields are not set.
>
> Please explain how that could ever happen.  You should never see
> a scsi_cmnd without the device pointer.

Chris,

Here is one example, During Host reset operation time driver will
flush out all the outstanding IO's to the SML in below function,

static void
_scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
{
struct scsi_cmnd *scmd;
struct scsiio_tracker *st;
u16 smid;
int count = 0;

[SR] Here driver is looping starting from smid one to HBA queue depth.
for (smid = 1; smid <= ioc->scsiio_depth; smid++) {

[SR] Some times it is possible that scsi_cmnd might have created at
SML but it might not be issued to the driver as host reset operation
is going on, So here we will get non-NULL scmd.
scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
if (!scmd)
continue;
count++;
_scsih_set_satl_pending(scmd, false);
[SR] Here we are trying to get the scsi tracker 'st' for the above
scmd (which is not received by the driver) and so fields of this 'st'
are uninitialized.
st = scsi_cmd_priv(scmd);
[SR] And here we are trying to clear the scsi tracker 'st' which is
not yet all initialized by the driver, in other terms we are trying to
flush out the scmd command which is not yet all received by the
driver.
mpt3sas_base_clear_st(ioc, st);
scsi_dma_unmap(scmd);
if (ioc->pci_error_recovery || ioc->remove_host)
scmd->result = DID_NO_CONNECT << 16;
else
scmd->result = DID_RESET << 16;
scmd->scsi_done(scmd);
}
dtmprintk(ioc, pr_info(MPT3SAS_FMT "completing %d cmds\n",
ioc->name, count));
}

Thanks,
Sreekanth

RE: [PATCH v3 0/2] scsi: mpt3sas: prevent oops in the shutdown/unload path

[Sreekanth Reddy]

-Original Message-
From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
Sent: Wednesday, March 7, 2018 6:48 AM
To: Mauricio Faria de Oliveira
Cc: Martin K. Petersen; j...@linux.vnet.ibm.com;
linux-scsi@vger.kernel.org; bart.vanass...@wdc.com;
sreekanth.re...@broadcom.com; dougm...@linux.vnet.ibm.com
Subject: Re: [PATCH v3 0/2] scsi: mpt3sas: prevent oops in the
shutdown/unload path


Mauricio Faria,

> Are you OK with / would ack this patchset for stable (v4.14+) ?
>
> I have the backports ready and can submit if you are OK with it.

I am OK.

Sreekanth?

-- 
Martin K. Petersen  Oracle Linux Engineering

I am also OK for it.

Thanks,
Sreekant

RE: [PATCH] scsi: mpt3sas: clarify mmio pointer types

[Sreekanth Reddy]

-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: Thursday, March 1, 2018 6:37 PM
To: Suganath Prabu S; Sathya Prakash; Chaitra P B; James E.J. Bottomley;
Martin K. Petersen
Cc: Arnd Bergmann; Hannes Reinecke; Sreekanth Reddy;
mpt-fusionlinux@broadcom.com; linux-scsi@vger.kernel.org;
linux-ker...@vger.kernel.org
Subject: [PATCH] scsi: mpt3sas: clarify mmio pointer types

The newly added code mixes up phys_addr_t/resource_size_t with dma_addr_t
and void pointers, as seen from these compiler warning:

drivers/scsi/mpt3sas/mpt3sas_base.c: In function '_base_get_chain_phys':
drivers/scsi/mpt3sas/mpt3sas_base.c:235:21: error: cast to pointer from
integer of different size [-Werror=int-to-pointer-cast]
  base_chain_phys  = (void *)ioc->chip_phys + MPI_FRAME_START_OFFSET +
 ^
drivers/scsi/mpt3sas/mpt3sas_base.c: In function '_clone_sg_entries':
drivers/scsi/mpt3sas/mpt3sas_base.c:427:20: error: cast from pointer to
integer of different size [-Werror=pointer-to-int-cast]
sgel->Address = (dma_addr_t)dst_addr_phys;
^
drivers/scsi/mpt3sas/mpt3sas_base.c:438:7: error: cast from pointer to
integer of different size [-Werror=pointer-to-int-cast]
   (dma_addr_t)buff_ptr_phys;
   ^
drivers/scsi/mpt3sas/mpt3sas_base.c:444:10: error: cast from pointer to
integer of different size [-Werror=pointer-to-int-cast]
  (dma_addr_t)buff_ptr_phys;

Both dma_addr_t and phys_addr_t may be wider than a pointer, so we must
avoid the conversion to pointer types. This also helps readability.

A second problem is treating MMIO addresses from a 'struct resource'
as addresses that can be used for DMA on that device. In almost all cases,
those are the same, but on some of the more obscure architectures, PCI
memory address 0 is mapped into the CPU address space at a nonzero offset.
I don't have a good fix for that, so I'm adding a comment here, plus a
WARN_ON() that triggers whenever the phys_addr_t number is outside of the
low 32-bit address space and causes a straight overflow when assigned to
the 32-bit sgel->Address.

Fixes: 182ac784b41f ("scsi: mpt3sas: Introduce Base function for
cloning.")
Signed-off-by: Arnd Bergmann <a...@arndb.de>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 42
-
 drivers/scsi/mpt3sas/mpt3sas_base.h |  2 +-
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 9f2434e59b40..61f93a134956 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -225,14 +225,14 @@ _base_get_chain(struct MPT3SAS_ADAPTER *ioc, u16
smid,
  *
  * @Return - Physical chain address.
  */
-static inline void *
+static inline phys_addr_t
 _base_get_chain_phys(struct MPT3SAS_ADAPTER *ioc, u16 smid,
u8 sge_chain_count)
 {
-   void *base_chain_phys, *chain_phys;
+   phys_addr_t base_chain_phys, chain_phys;
u16 cmd_credit = ioc->facts.RequestCredit + 1;

-   base_chain_phys  = (void *)ioc->chip_phys + MPI_FRAME_START_OFFSET
+
+   base_chain_phys  = ioc->chip_phys + MPI_FRAME_START_OFFSET +
(cmd_credit * ioc->request_sz) +
REPLY_FREE_POOL_SIZE;
chain_phys = base_chain_phys + (smid * ioc->facts.MaxChainDepth *
@@ -272,11 +272,11 @@ _base_get_buffer_bar0(struct MPT3SAS_ADAPTER *ioc,
u16 smid)
  *
  * @Returns - Pointer to buffer location in BAR0.
  */
-static void *
+static phys_addr_t
 _base_get_buffer_phys_bar0(struct MPT3SAS_ADAPTER *ioc, u16 smid)  {
u16 cmd_credit = ioc->facts.RequestCredit + 1;
-   void *chain_end_phys = _base_get_chain_phys(ioc,
+   phys_addr_t chain_end_phys = _base_get_chain_phys(ioc,
cmd_credit + 1,
ioc->facts.MaxChainDepth);
return chain_end_phys + (smid * 64 * 1024); @@ -330,11 +330,12 @@
static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
bool is_write = 0;
u16 i = 0;
void __iomem *buffer_iomem;
-   void  *buffer_iomem_phys;
+   phys_addr_t buffer_iomem_phys;
void __iomem *buff_ptr;
-   void *buff_ptr_phys;
+   phys_addr_t buff_ptr_phys;
void __iomem *dst_chain_addr[MCPU_MAX_CHAINS_PER_IO];
-   void *src_chain_addr[MCPU_MAX_CHAINS_PER_IO], *dst_addr_phys;
+   void *src_chain_addr[MCPU_MAX_CHAINS_PER_IO];
+   phys_addr_t dst_addr_phys;
MPI2RequestHeader_t *request_hdr;
struct scsi_cmnd *scmd;
struct scatterlist *sg_scmd = NULL;
@@ -391,6 +392,7 @@ static void _clone_sg_entries(struct MPT3SAS_ADAPTER
*ioc,

buff_ptr = buffer_iomem;
buff_ptr_phys = buffer_iomem_phys;
+   WARN_ON(buff_ptr_phys > U32_MAX);

if (sgel->FlagsLength &
(MPI2_SGE_FLAGS_HOST_TO_IOC <<
MPI2_SGE_FLAGS_SHIFT)) @@ -421,10 +423,1

RE: [PATCH] mpt3sas: Do not mark fw_event workqueue as WQ_MEM_RECLAIM

[Sreekanth Reddy]

-Original Message-
From: Hannes Reinecke [mailto:h...@suse.de]
Sent: Monday, February 26, 2018 7:56 PM
To: Martin K. Petersen
Cc: Christoph Hellwig; James Bottomley; linux-scsi@vger.kernel.org; Hannes
Reinecke; Sreekanth Reddy; Suganath Prabu Subramani; Hannes Reinecke
Subject: [PATCH] mpt3sas: Do not mark fw_event workqueue as WQ_MEM_RECLAIM

The firmware event workqueue should not be marked as WQ_MEM_RECLAIM as
it's doesn't need to make forward progress under memory pressure.
In the current state it will result in a deadlock if the device had been
forcefully removed.

Cc: Sreekanth Reddy <sreekanth.re...@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subram...@broadcom.com>
Signed-off-by: Hannes Reinecke <h...@suse.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 74fca184dba9..b1d3218a7ad7 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -10547,7 +10547,7 @@ _scsih_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
snprintf(ioc->firmware_event_name,
sizeof(ioc->firmware_event_name),
"fw_event_%s%d", ioc->driver_name, ioc->id);
ioc->firmware_event_thread = alloc_ordered_workqueue(
-   ioc->firmware_event_name, WQ_MEM_RECLAIM);
+   ioc->firmware_event_name, 0);
if (!ioc->firmware_event_thread) {
pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
ioc->name, __FILE__, __LINE__, __func__);
--
2.12.3

Acked-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>

Thanks,
Sreekanth

RE: [PATCH v2 1/2] scsi: mpt3sas: fix oops in error handlers after shutdown/unload

[Sreekanth Reddy]

-Original Message-
From: Mauricio Faria de Oliveira [mailto:mauri...@linux.vnet.ibm.com]
Sent: Saturday, February 17, 2018 4:10 AM
To: linux-scsi@vger.kernel.org; bart.vanass...@wdc.com;
sreekanth.re...@broadcom.com
Cc: sathya.prak...@broadcom.com; chaitra.basa...@broadcom.com;
suganath-prabu.subram...@broadcom.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com; dougm...@linux.vnet.ibm.com
Subject: [PATCH v2 1/2] scsi: mpt3sas: fix oops in error handlers after
shutdown/unload

This patch adds checks for 'ioc->remove_host' in the SCSI error handlers,
so not to access pointers/resources potentially freed in the PCI
shutdown/module unload path.  The error handlers may be invoked after
shutdown/unload, depending on other components.

This problem was observed with kexec on a system with a mpt3sas based
adapter and an infiniband adapter which takes long enough to shutdown:

The mpt3sas driver finished shutting down / disabled interrupt handling,
thus some commands have not finished and timed out.

Since the system was still running (waiting for the infiniband adapter to
shutdown), the scsi error handler for task abort of mpt3sas was invoked,
and hit an oops -- either in scsih_abort() because 'ioc->scsi_lookup' was
NULL (without commit dbec4c90
"scsi: mpt3sas: lockless command submission"), or later up in
scsih_host_reset() (with or without that commit), because it eventually
called mpt3sas_base_get_iocstate().

After commit dbec4c90, the oops in scsih_abort() does not occur anymore
(_scsih_scsi_lookup_find_by_scmd() is no longer called), but that commit
is too big and out of the scope of linux-stable, where this patch might
help, so still go for the changes.

Also, this might help to prevent similar errors in the future, in case
code changes and possibly tries to access freed stuff.

Note the fix in scsih_host_reset() is still important anyway.

Signed-off-by: Mauricio Faria de Oliveira <mauri...@linux.vnet.ibm.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 74fca18..5ab3caf 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2835,7 +2835,8 @@ int mpt3sas_scsih_issue_locked_tm(struct
MPT3SAS_ADAPTER *ioc, u16 handle,
_scsih_tm_display_info(ioc, scmd);

sas_device_priv_data = scmd->device->hostdata;
-   if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
+   if (!sas_device_priv_data || !sas_device_priv_data->sas_target ||
+   ioc->remove_host) {
sdev_printk(KERN_INFO, scmd->device,
"device been deleted! scmd(%p)\n", scmd);
scmd->result = DID_NO_CONNECT << 16;
@@ -2898,7 +2899,8 @@ int mpt3sas_scsih_issue_locked_tm(struct
MPT3SAS_ADAPTER *ioc, u16 handle,
_scsih_tm_display_info(ioc, scmd);

sas_device_priv_data = scmd->device->hostdata;
-   if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
+   if (!sas_device_priv_data || !sas_device_priv_data->sas_target ||
+   ioc->remove_host) {
sdev_printk(KERN_INFO, scmd->device,
"device been deleted! scmd(%p)\n", scmd);
scmd->result = DID_NO_CONNECT << 16;
@@ -2961,7 +2963,8 @@ int mpt3sas_scsih_issue_locked_tm(struct
MPT3SAS_ADAPTER *ioc, u16 handle,
_scsih_tm_display_info(ioc, scmd);

sas_device_priv_data = scmd->device->hostdata;
-   if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
+   if (!sas_device_priv_data || !sas_device_priv_data->sas_target ||
+   ioc->remove_host) {
starget_printk(KERN_INFO, starget, "target been deleted!
scmd(%p)\n",
scmd);
scmd->result = DID_NO_CONNECT << 16;
@@ -3019,7 +3022,7 @@ int mpt3sas_scsih_issue_locked_tm(struct
MPT3SAS_ADAPTER *ioc, u16 handle,
ioc->name, scmd);
scsi_print_command(scmd);

-   if (ioc->is_driver_loading) {
+   if (ioc->is_driver_loading || ioc->remove_host) {
    pr_info(MPT3SAS_FMT "Blocking the host reset\n",
ioc->name);
r = FAILED;
--
1.8.3.1


Acked-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>

Thanks,
Sreekanth

RE: [PATCH v2] scsi: mpt3sas: fix oops in error handlers after shutdown/unload

[Sreekanth Reddy]

Mauricio,

During the shutdown time, I don't want the outstanding IOs to timeout due to
disabling of interrupts and go the TM path. So I wanted to clear out all the
Outstanding IOs in the shutdown path itself instead of clearing them in TM
path.

But if there are already some TMs are outstanding while initiating the
shutdown operation then your patch looks good to me.

May be can you include my set of changes along with your solution?

Thanks,
Sreekanth

-Original Message-
From: Mauricio Faria de Oliveira [mailto:mauri...@linux.vnet.ibm.com]
Sent: Wednesday, February 14, 2018 9:05 PM
To: Sreekanth Reddy; linux-scsi@vger.kernel.org; bart.vanass...@wdc.com
Cc: Sathya Prakash Veerichetty; Chaitra Basappa; Suganath Prabu Subramani;
j...@linux.vnet.ibm.com; martin.peter...@oracle.com;
dougm...@linux.vnet.ibm.com
Subject: Re: [PATCH v2] scsi: mpt3sas: fix oops in error handlers after
shutdown/unload

Hi Sreenkanth,

Thanks for reviewing.

On 02/12/2018 04:28 AM, Sreekanth Reddy wrote:
> Instead of returning the scmd with DID_NO_CONNECT in TM path, can we
> wait for some time (10 seconds) in shutdown/unload path for the
> outstanding commands to complete and even then [if] the scmds are
> outstanding then return all the outstanding scmds with DID_NO_CONNECT
> in the shutdown/unload path itself as shown below,

Apparently there is a problem with that.

That is not enough for TM commands; it's only for SCSI IO commands.

The TM commands use the high-priority queue, and SCSI IO commands use the
SCSI IO queue.  But this patch only waits for and return commands in the
latter:

 > +mpt3sas_wait_for_commands_to_complete(ioc);
 > +_scsih_flush_running_cmds(ioc);

They only loop up until 'ioc->scsiio_depth', but TM commands get SMIDs above
that [see mpt3sas_base_free_smid() and mpt3sas_scsih_issue_tm()].

So, there's an exposure for abort/reset/other TM commands. For example, the
SCSI IO commands finish quickly in wait_for_commands_to_complete(), then the
shutdown function continues, disables interrupts, and release
pointers/memory.  Then, an outstanding abort/reset command is finished, its
completion interrupt is not serviced, and the SCSI EH for it kicks
in/continues, and so it escalates up, and hit that oops in host reset.

Is there any problem with the patch that I proposed?

It seems okay to have another check in error path (not hot/performance
path), and that check is already used in many other points in the code, for
the same reasons (exit early before the code attempts to use stuff that
might be released).

Thanks again,

--
Mauricio Faria de Oliveira
IBM Linux Technology Center

RE: [PATCH v2] scsi: mpt3sas: fix oops in error handlers after shutdown/unload

[Sreekanth Reddy]

Mauricio,

Instead of returning the scmd with DID_NO_CONNECT in TM path, can we wait
for some time (10 seconds) in shutdown/unload path for the outstanding
commands to complete and even then the scmds are outstanding then return
all the outstanding scmds with DID_NO_CONNECT in the shutdown/unload path
itself as shown below,


diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 13d6e4e..f62ce50 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6294,14 +6294,14 @@ _base_reset_handler(struct MPT3SAS_ADAPTER *ioc,
int reset_phase)
 }

 /**
- * _wait_for_commands_to_complete - reset controller
+ * mpt3sas_wait_for_commands_to_complete - reset controller
  * @ioc: Pointer to MPT_ADAPTER structure
  *
  * This function is waiting 10s for all pending commands to complete
  * prior to putting controller in reset.
  */
-static void
-_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc)
+void
+mpt3sas_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc)
 {
u32 ioc_state;

@@ -6374,7 +6374,7 @@ mpt3sas_base_hard_reset_handler(struct
MPT3SAS_ADAPTER *ioc,
is_fault = 1;
}
_base_reset_handler(ioc, MPT3_IOC_PRE_RESET);
-   _wait_for_commands_to_complete(ioc);
+   mpt3sas_wait_for_commands_to_complete(ioc);
_base_mask_interrupts(ioc);
r = _base_make_ioc_ready(ioc, type);
if (r)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 789bc42..99ccf83 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1433,6 +1433,9 @@ void mpt3sas_base_update_missing_delay(struct
MPT3SAS_ADAPTER *ioc,

 int mpt3sas_port_enable(struct MPT3SAS_ADAPTER *ioc);

+void
+mpt3sas_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc);
+

 /* scsih shared API */
 struct scsi_cmnd *mpt3sas_scsih_scsi_lookup_get(struct MPT3SAS_ADAPTER
*ioc,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 74fca18..458709e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4453,7 +4453,7 @@ _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER
*ioc)
st = scsi_cmd_priv(scmd);
mpt3sas_base_clear_st(ioc, st);
scsi_dma_unmap(scmd);
-   if (ioc->pci_error_recovery)
+   if (ioc->pci_error_recovery || ioc->remove_host)
scmd->result = DID_NO_CONNECT << 16;
else
scmd->result = DID_RESET << 16;
@@ -9739,6 +9739,10 @@ static void scsih_remove(struct pci_dev *pdev)
unsigned long flags;

ioc->remove_host = 1;
+
+   mpt3sas_wait_for_commands_to_complete(ioc);
+   _scsih_flush_running_cmds(ioc);
+
_scsih_fw_event_cleanup_queue(ioc);

spin_lock_irqsave(>fw_event_lock, flags);
@@ -9815,6 +9819,10 @@ scsih_shutdown(struct pci_dev *pdev)
unsigned long flags;

ioc->remove_host = 1;
+
+   mpt3sas_wait_for_commands_to_complete(ioc);
+   _scsih_flush_running_cmds(ioc);
+
_scsih_fw_event_cleanup_queue(ioc);

spin_lock_irqsave(>fw_event_lock, flags);
---

-Original Message-
From: linux-scsi-ow...@vger.kernel.org
[mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Mauricio Faria de
Oliveira
Sent: Friday, February 2, 2018 3:46 AM
To: linux-scsi@vger.kernel.org; bart.vanass...@wdc.com
Cc: sathya.prak...@broadcom.com; chaitra.basa...@broadcom.com;
suganath-prabu.subram...@broadcom.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com; dougm...@linux.vnet.ibm.com
Subject: [PATCH v2] scsi: mpt3sas: fix oops in error handlers after
shutdown/unload

This patch adds checks for 'ioc->remove_host' in the SCSI error handlers,
so not to access pointers/resources potentially freed in the PCI
shutdown/module unload path.  The error handlers may be invoked after
shutdown/unload, depending on other components.

This problem was observed with kexec on a system with a mpt3sas based
adapter and an infiniband adapter which takes long enough to shutdown:

The mpt3sas driver finished shutting down / disabled interrupt handling,
thus some commands have not finished and timed out.

Since the system was still running (waiting for the infiniband adapter to
shutdown), the scsi error handler for task abort of mpt3sas was invoked,
and hit an oops -- either in scsih_abort() because 'ioc->scsi_lookup' was
NULL (without commit dbec4c9040ed
("scsi: mpt3sas: lockless command submission")), or later up in
scsih_host_reset() (with or without that commit), because it eventually
called mpt3sas_base_get_iocstate().

After that commit, the oops in scsih_abort() does not occur anymore
(_scsih_scsi_lookup_find_by_scmd() is no longer called), but that commit
is too big and out of the scope of linux-stable, where this patch might
help, so still go for the changes.

[PATCH 01/10] mpt3sas: Processing of Cable Exception events

[Sreekanth Reddy]

Earlier Active Cable Exception event with reason code
"Cable Degraded (0x02))" was added only for Active Cable,
Now this event is extended to Passive cable too.
So re-arranged display message accordingly.

Also added Cable Exception Event even for SAS3008 & SAS3108 HBAs
(i.e. MPI 2.5 spec supporting HBAs) earlier this event was
enabled only for MPI 2.6 spec supporting HBA devices.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c  |  5 ++---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 20 +++-
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 870..844e29c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -655,7 +655,7 @@ _base_display_event_data(struct MPT3SAS_ADAPTER *ioc,
desc = "Temperature Threshold";
break;
case MPI2_EVENT_ACTIVE_CABLE_EXCEPTION:
-   desc = "Active cable exception";
+   desc = "Cable Event";
break;
}
 
@@ -5517,8 +5517,7 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
_base_unmask_events(ioc, MPI2_EVENT_IR_OPERATION_STATUS);
_base_unmask_events(ioc, MPI2_EVENT_LOG_ENTRY_ADDED);
_base_unmask_events(ioc, MPI2_EVENT_TEMP_THRESHOLD);
-   if (ioc->hba_mpi_version_belonged == MPI26_VERSION)
-   _base_unmask_events(ioc, MPI2_EVENT_ACTIVE_CABLE_EXCEPTION);
+   _base_unmask_events(ioc, MPI2_EVENT_ACTIVE_CABLE_EXCEPTION);
 
r = _base_make_ioc_operational(ioc);
if (r)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 22998cb..9594166 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -8056,19 +8056,21 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER 
*ioc, u8 msix_index,
(Mpi26EventDataActiveCableExcept_t *) mpi_reply->EventData;
switch (ActiveCableEventData->ReasonCode) {
case MPI26_EVENT_ACTIVE_CABLE_INSUFFICIENT_POWER:
-   pr_notice(MPT3SAS_FMT "Receptacle ID %d: This active 
cable"
- " requires %d mW of power\n", ioc->name,
-ActiveCableEventData->ReceptacleID,
+   pr_notice(MPT3SAS_FMT
+   "Currently an active cable with ReceptacleID %d\n",
+   ioc->name, ActiveCableEventData->ReceptacleID);
+   pr_notice("cannot be powered and devices connected\n");
+   pr_notice("to this active cable will not be seen\n");
+   pr_notice("This active cable require %d mW of power\n",
 ActiveCableEventData->ActiveCablePowerRequirement);
-   pr_notice(MPT3SAS_FMT "Receptacle ID %d: Devices 
connected"
- " to this active cable will not be seen\n",
-ioc->name, ActiveCableEventData->ReceptacleID);
break;
 
case MPI26_EVENT_ACTIVE_CABLE_DEGRADED:
-   pr_notice(MPT3SAS_FMT "ReceptacleID %d: This cable",
-   ioc->name, ActiveCableEventData->ReceptacleID);
-   pr_notice(" is not running at an optimal speed(12 
Gb/s)\n");
+   pr_notice(MPT3SAS_FMT
+   "Currently a cable with ReceptacleID %d\n",
+   ioc->name, ActiveCableEventData->ReceptacleID);
+   pr_notice(
+   "is not running at optimal speed(12 Gb/s rate)\n");
break;
}
 
-- 
2.4.3

[PATCH 00/10] [SCSI] mpt3sas: Phase15 driver enhancements and fixes

[Sreekanth Reddy]

Phase15 driver enhancements and fixes.

Sreekanth Reddy (10):
  mpt3sas: Processing of Cable Exception events
  mpt3sas: Fixed memory leaks in driver
  mpt3sas: Reduce memory footprints in kdump kernel
  mpt3sas: Fix removal and addition of vSES device during host reset
  mpt3sas: Fix IO error occurs on pulling out a drive from RAID1 volume
created on two SATA drive
  mpt3sas: Updated MPI headers to v2.00.48
  mpt3sas: Display chassis slot information of the drive
  mpt3sas: Fix possibility of using invalid Enclosure Handles for SAS
device after host reset
  mpt3sas: Adding support for SAS3616 HBA device
  mpt3sas: Bump mpt3sas driver version to v16.100.00.00

 drivers/scsi/mpt3sas/mpi/mpi2.h  |  43 ++-
 drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 564 +--
 drivers/scsi/mpt3sas/mpi/mpi2_init.h |  11 +-
 drivers/scsi/mpt3sas/mpi/mpi2_ioc.h  | 282 +-
 drivers/scsi/mpt3sas/mpi/mpi2_pci.h  | 112 +++
 drivers/scsi/mpt3sas/mpi/mpi2_tool.h |  14 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c  |  19 +-
 drivers/scsi/mpt3sas/mpt3sas_base.h  |  10 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 383 ++--
 9 files changed, 1241 insertions(+), 197 deletions(-)
 create mode 100644 drivers/scsi/mpt3sas/mpi/mpi2_pci.h

-- 
2.4.3

[PATCH 04/10] mpt3sas: Fix removal and addition of vSES device during host reset

[Sreekanth Reddy]

For Dev Handles who value is less than hba's phys count number
 driver will return HBA sas address value as a sas address.
So for Virtual SES device also driver was returning HBA sas address instead
 of Virtual SES sas address. So now updated the driver to return
 Virtual SES's sas address for Virtual SES device instead of
 HBA's sas address.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 600e8ef..cc78ce4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -406,11 +406,6 @@ _scsih_get_sas_address(struct MPT3SAS_ADAPTER *ioc, u16 
handle,
 
*sas_address = 0;
 
-   if (handle <= ioc->sas_hba.num_phys) {
-   *sas_address = ioc->sas_hba.sas_address;
-   return 0;
-   }
-
if ((mpt3sas_config_get_sas_device_pg0(ioc, _reply, _device_pg0,
MPI2_SAS_DEVICE_PGAD_FORM_HANDLE, handle))) {
pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n", ioc->name,
@@ -420,7 +415,15 @@ _scsih_get_sas_address(struct MPT3SAS_ADAPTER *ioc, u16 
handle,
 
ioc_status = le16_to_cpu(mpi_reply.IOCStatus) & MPI2_IOCSTATUS_MASK;
if (ioc_status == MPI2_IOCSTATUS_SUCCESS) {
-   *sas_address = le64_to_cpu(sas_device_pg0.SASAddress);
+   /* For HBA vSES don't return hba sas address instead return
+* vSES's sas address.
+*/
+   if ((handle <= ioc->sas_hba.num_phys) &&
+  (!(le32_to_cpu(sas_device_pg0.DeviceInfo) &
+  MPI2_SAS_DEVICE_INFO_SEP)))
+   *sas_address = ioc->sas_hba.sas_address;
+   else
+   *sas_address = le64_to_cpu(sas_device_pg0.SASAddress);
return 0;
}
 
-- 
2.4.3

[PATCH 03/10] mpt3sas: Reduce memory footprints in kdump kernel

[Sreekanth Reddy]

To reduce the memory footprints of the driver in kdump kernel,
 we have made below driver setting when system boots in to kdump kernel,

1. Used single MSI-x vector.
2. Disable RDPQ mode.
3. Set sg_table_size to 32 by default.
4) Set SCSI IO Queue depth to 200.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 14 +++---
 drivers/scsi/mpt3sas/mpt3sas_base.h |  2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 844e29c..11c6afe 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1990,7 +1990,7 @@ _base_enable_msix(struct MPT3SAS_ADAPTER *ioc)
  ioc->cpu_count, max_msix_vectors);
 
if (!ioc->rdpq_array_enable && max_msix_vectors == -1)
-   local_max_msix_vectors = 8;
+   local_max_msix_vectors = (reset_devices) ? 1 : 8;
else
local_max_msix_vectors = max_msix_vectors;
 
@@ -3308,6 +3308,11 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
sg_tablesize = MPT3SAS_SG_DEPTH;
}
 
+   /* max sgl entries <= MPT_KDUMP_MIN_PHYS_SEGMENTS in KDUMP mode */
+   if (reset_devices)
+   sg_tablesize = min_t(unsigned short, sg_tablesize,
+  MPT_KDUMP_MIN_PHYS_SEGMENTS);
+
if (sg_tablesize < MPT_MIN_PHYS_SEGMENTS)
sg_tablesize = MPT_MIN_PHYS_SEGMENTS;
else if (sg_tablesize > MPT_MAX_PHYS_SEGMENTS) {
@@ -3340,7 +3345,10 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
ioc->internal_depth, facts->RequestCredit);
if (max_request_credit > MAX_HBA_QUEUE_DEPTH)
max_request_credit =  MAX_HBA_QUEUE_DEPTH;
-   } else
+   } else if (reset_devices)
+   max_request_credit = min_t(u16, facts->RequestCredit,
+   (MPT3SAS_KDUMP_SCSI_IO_DEPTH + ioc->internal_depth));
+   else
max_request_credit = min_t(u16, facts->RequestCredit,
MAX_HBA_QUEUE_DEPTH);
 
@@ -4446,7 +4454,7 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
if ((facts->IOCCapabilities & MPI2_IOCFACTS_CAPABILITY_INTEGRATED_RAID))
ioc->ir_firmware = 1;
if ((facts->IOCCapabilities &
- MPI2_IOCFACTS_CAPABILITY_RDPQ_ARRAY_CAPABLE))
+ MPI2_IOCFACTS_CAPABILITY_RDPQ_ARRAY_CAPABLE) && (!reset_devices))
ioc->rdpq_array_capable = 1;
if (facts->IOCCapabilities & MPI26_IOCFACTS_CAPABILITY_ATOMIC_REQ)
ioc->atomic_desc_capable = 1;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h 
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index a77bb7d..95ee1c6 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -92,6 +92,7 @@
  */
 #define MPT_MAX_PHYS_SEGMENTS  SG_CHUNK_SIZE
 #define MPT_MIN_PHYS_SEGMENTS  16
+#define MPT_KDUMP_MIN_PHYS_SEGMENTS32
 
 #ifdef CONFIG_SCSI_MPT3SAS_MAX_SGE
 #define MPT3SAS_SG_DEPTH   CONFIG_SCSI_MPT3SAS_MAX_SGE
@@ -111,6 +112,7 @@
 #define MPT3SAS_SATA_QUEUE_DEPTH   32
 #define MPT3SAS_SAS_QUEUE_DEPTH254
 #define MPT3SAS_RAID_QUEUE_DEPTH   128
+#define MPT3SAS_KDUMP_SCSI_IO_DEPTH200
 
 #define MPT3SAS_RAID_MAX_SECTORS   8192
 
-- 
2.4.3

[PATCH 02/10] mpt3sas: Fixed memory leaks in driver

[Sreekanth Reddy]

Fixed below memory leak in driver,

* While removing Expander devices - we are removing expander
 device entry from the list before freeing it's child devices,
 so while freeing child device we are finding its parent device
 node as NULL and so we are not freeing the child device's
 allocated data structures.
 Updated the driver to remove the expander device from the list
 only after freeing all its child devices.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 9594166..600e8ef 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -5274,8 +5274,6 @@ mpt3sas_expander_remove(struct MPT3SAS_ADAPTER *ioc, u64 
sas_address)
spin_lock_irqsave(>sas_node_lock, flags);
sas_expander = mpt3sas_scsih_expander_find_by_sas_address(ioc,
sas_address);
-   if (sas_expander)
-   list_del(_expander->list);
spin_unlock_irqrestore(>sas_node_lock, flags);
if (sas_expander)
_scsih_expander_node_remove(ioc, sas_expander);
@@ -7476,7 +7474,6 @@ _scsih_remove_unresponding_sas_devices(struct 
MPT3SAS_ADAPTER *ioc)
spin_unlock_irqrestore(>sas_node_lock, flags);
list_for_each_entry_safe(sas_expander, sas_expander_next, _list,
list) {
-   list_del(_expander->list);
_scsih_expander_node_remove(ioc, sas_expander);
}
 
@@ -8102,7 +8099,6 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, 
u8 msix_index,
  * _scsih_expander_node_remove - removing expander device from list.
  * @ioc: per adapter object
  * @sas_expander: the sas_device object
- * Context: Calling function should acquire ioc->sas_node_lock.
  *
  * Removing object and freeing associated memory from the
  * ioc->sas_expander_list.
@@ -8114,6 +8110,7 @@ _scsih_expander_node_remove(struct MPT3SAS_ADAPTER *ioc,
struct _sas_node *sas_expander)
 {
struct _sas_port *mpt3sas_port, *next;
+   unsigned long flags;
 
/* remove sibling ports attached to this expander */
list_for_each_entry_safe(mpt3sas_port, next,
@@ -8141,6 +8138,10 @@ _scsih_expander_node_remove(struct MPT3SAS_ADAPTER *ioc,
sas_expander->handle, (unsigned long long)
sas_expander->sas_address);
 
+   spin_lock_irqsave(>sas_node_lock, flags);
+   list_del(_expander->list);
+   spin_unlock_irqrestore(>sas_node_lock, flags);
+
kfree(sas_expander->phy);
kfree(sas_expander);
 }
-- 
2.4.3

[PATCH 05/10] mpt3sas: Fix IO error occurs on pulling out a drive from RAID1 volume created on two SATA drive

[Sreekanth Reddy]

Whenever IO for raid volume fails with IOCStatus
 "MPI2_IOCSTATUS_SCSI_IOC_TERMINATED"and SCSIStatus equal to
 "(MPI2_SCSI_STATE_TERMINATED | MPI2_SCSI_STATE_NO_SCSI_STATUS)"
 then return the IO to SML with "DID_RESET"
 (i.e. retry the IO infinite times) host bytes.

Earlier driver is returning the IO with "DID_SOFT_ERROR"
 that reties the IO quickly for five times but still
 firmware needed some more time and hence IOs were failing.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index cc78ce4..fa948ab 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4807,6 +4807,11 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 
msix_index, u32 reply)
} else if (log_info == VIRTUAL_IO_FAILED_RETRY) {
scmd->result = DID_RESET << 16;
break;
+   } else if ((scmd->device->channel == RAID_CHANNEL) &&
+  (scsi_state == (MPI2_SCSI_STATE_TERMINATED |
+  MPI2_SCSI_STATE_NO_SCSI_STATUS))) {
+   scmd->result = DID_RESET << 16;
+   break;
}
scmd->result = DID_SOFT_ERROR << 16;
break;
-- 
2.4.3

[PATCH 07/10] mpt3sas: Display chassis slot information of the drive

[Sreekanth Reddy]

Display chassis slot information along with other drive location parameters
 such as slot number and connector name in the logs; if chassis slot
 validity bit is set in 'SAS Enclosure Page 0' while adding the drive.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.h  |   4 +
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 266 ++-
 2 files changed, 143 insertions(+), 127 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h 
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 95ee1c6..75d90f2 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -469,6 +469,8 @@ struct _internal_cmd {
  * @pfa_led_on: flag for PFA LED status
  * @pend_sas_rphy_add: flag to check if device is in sas_rphy_add()
  * addition routine.
+ * @chassis_slot: chassis slot
+ * @is_chassis_slot_valid: chassis slot valid or not
  */
 struct _sas_device {
struct list_head list;
@@ -491,6 +493,8 @@ struct _sas_device {
u8  pfa_led_on;
u8  pend_sas_rphy_add;
u8  enclosure_level;
+   u8  chassis_slot;
+   u8  is_chassis_slot_valid;
u8  connector_name[5];
struct kref refcount;
 };
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index fa948ab..17b934b 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -653,6 +653,69 @@ mpt3sas_get_sdev_by_handle(struct MPT3SAS_ADAPTER *ioc, 
u16 handle)
 }
 
 /**
+ * _scsih_display_enclosure_chassis_info - display device location info
+ * @ioc: per adapter object
+ * @sas_device: per sas device object
+ * @sdev: scsi device struct
+ * @starget: scsi target struct
+ *
+ * Returns nothing.
+ */
+static void
+_scsih_display_enclosure_chassis_info(struct MPT3SAS_ADAPTER *ioc,
+   struct _sas_device *sas_device, struct scsi_device *sdev,
+   struct scsi_target *starget)
+{
+   if (sdev) {
+   if (sas_device->enclosure_handle != 0)
+   sdev_printk(KERN_INFO, sdev,
+   "enclosure logical id (0x%016llx), slot(%d) \n",
+   (unsigned long long)
+   sas_device->enclosure_logical_id,
+   sas_device->slot);
+   if (sas_device->connector_name[0] != '\0')
+   sdev_printk(KERN_INFO, sdev,
+   "enclosure level(0x%04x), connector name( %s)\n",
+   sas_device->enclosure_level,
+   sas_device->connector_name);
+   if (sas_device->is_chassis_slot_valid)
+   sdev_printk(KERN_INFO, sdev, "chassis slot(0x%04x)\n",
+   sas_device->chassis_slot);
+   } else if (starget) {
+   if (sas_device->enclosure_handle != 0)
+   starget_printk(KERN_INFO, starget,
+   "enclosure logical id(0x%016llx), slot(%d) \n",
+   (unsigned long long)
+   sas_device->enclosure_logical_id,
+   sas_device->slot);
+   if (sas_device->connector_name[0] != '\0')
+   starget_printk(KERN_INFO, starget,
+   "enclosure level(0x%04x), connector name( %s)\n",
+   sas_device->enclosure_level,
+   sas_device->connector_name);
+   if (sas_device->is_chassis_slot_valid)
+   starget_printk(KERN_INFO, starget,
+   "chassis slot(0x%04x)\n",
+   sas_device->chassis_slot);
+   } else {
+   if (sas_device->enclosure_handle != 0)
+   pr_info(MPT3SAS_FMT
+   "enclosure logical id(0x%016llx), slot(%d) \n",
+   ioc->name, (unsigned long long)
+   sas_device->enclosure_logical_id,
+   sas_device->slot);
+   if (sas_device->connector_name[0] != '\0')
+   pr_info(MPT3SAS_FMT
+   "enclosure level(0x%04x), connector name( %s)\n",
+   ioc->name, sas_device->enclosure_level,
+   sas_device->connector_name);
+   if (sas_device->is_chassis_slot_valid)
+   pr_info(MPT3SAS_FMT "chassis slot(0x%04x)\n",
+   ioc->name, sas_device->chassis_slot);
+   }
+}
+
+/**
  * _scsih_sas_device_remove - remove sas_device from list.
  * @ioc: per adapter object
  * @sas_device: the sas_device object
@@ -673,17 +736,7 @@ _scsih_sas_device_remove(s

[PATCH 06/10] mpt3sas: Updated MPI headers to v2.00.48

[Sreekanth Reddy]

Updated MPI headers to v2.00.48

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpi/mpi2.h  |  43 ++-
 drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 564 +--
 drivers/scsi/mpt3sas/mpi/mpi2_init.h |  11 +-
 drivers/scsi/mpt3sas/mpi/mpi2_ioc.h  | 282 +-
 drivers/scsi/mpt3sas/mpi/mpi2_pci.h  | 112 +++
 drivers/scsi/mpt3sas/mpi/mpi2_tool.h |  14 +-
 6 files changed, 992 insertions(+), 34 deletions(-)
 create mode 100644 drivers/scsi/mpt3sas/mpi/mpi2_pci.h

diff --git a/drivers/scsi/mpt3sas/mpi/mpi2.h b/drivers/scsi/mpt3sas/mpi/mpi2.h
index a9a659f..bc59058 100644
--- a/drivers/scsi/mpt3sas/mpi/mpi2.h
+++ b/drivers/scsi/mpt3sas/mpi/mpi2.h
@@ -8,7 +8,7 @@
  * scatter/gather formats.
  * Creation Date:  June 21, 2006
  *
- * mpi2.h Version:  02.00.42
+ * mpi2.h Version:  02.00.48
  *
  * NOTE: Names (typedefs, defines, etc.) beginning with an MPI25 or Mpi25
  *   prefix are for use only on MPI v2.5 products, and must not be used
@@ -103,6 +103,16 @@
  * 08-25-15  02.00.40  Bumped MPI2_HEADER_VERSION_UNIT.
  * 12-15-15  02.00.41  Bumped MPI_HEADER_VERSION_UNIT
  * 01-01-16  02.00.42  Bumped MPI_HEADER_VERSION_UNIT
+ * 04-05-16  02.00.43  Modified  MPI26_DIAG_BOOT_DEVICE_SELECT defines
+ * to be unique within first 32 characters.
+ * Removed AHCI support.
+ * Removed SOP support.
+ * Bumped MPI2_HEADER_VERSION_UNIT.
+ * 04-10-16  02.00.44  Bumped MPI2_HEADER_VERSION_UNIT.
+ * 07-06-16  02.00.45  Bumped MPI2_HEADER_VERSION_UNIT.
+ * 09-02-16  02.00.46  Bumped MPI2_HEADER_VERSION_UNIT.
+ * 11-23-16  02.00.47  Bumped MPI2_HEADER_VERSION_UNIT.
+ * 02-03-17  02.00.48  Bumped MPI2_HEADER_VERSION_UNIT.
  * --
  */
 
@@ -142,7 +152,7 @@
 #define MPI2_VERSION_02_06 (0x0206)
 
 /*Unit and Dev versioning for this MPI header set */
-#define MPI2_HEADER_VERSION_UNIT(0x2A)
+#define MPI2_HEADER_VERSION_UNIT(0x30)
 #define MPI2_HEADER_VERSION_DEV (0x00)
 #define MPI2_HEADER_VERSION_UNIT_MASK   (0xFF00)
 #define MPI2_HEADER_VERSION_UNIT_SHIFT  (8)
@@ -249,6 +259,12 @@ typedef volatile struct _MPI2_SYSTEM_INTERFACE_REGS {
 #define MPI2_DIAG_BOOT_DEVICE_SELECT_DEFAULT(0x)
 #define MPI2_DIAG_BOOT_DEVICE_SELECT_HCDW   (0x0800)
 
+/* Defines for V7A/V7R HostDiagnostic Register */
+#define MPI26_DIAG_BOOT_DEVICE_SEL_64FLASH  (0x)
+#define MPI26_DIAG_BOOT_DEVICE_SEL_64HCDW   (0x0800)
+#define MPI26_DIAG_BOOT_DEVICE_SEL_32FLASH  (0x1000)
+#define MPI26_DIAG_BOOT_DEVICE_SEL_32HCDW   (0x1800)
+
 #define MPI2_DIAG_CLEAR_FLASH_BAD_SIG   (0x0400)
 #define MPI2_DIAG_FORCE_HCB_ON_RESET(0x0200)
 #define MPI2_DIAG_HCB_MODE  (0x0100)
@@ -367,6 +383,7 @@ typedef struct _MPI2_DEFAULT_REQUEST_DESCRIPTOR {
 #define MPI2_REQ_DESCRIPT_FLAGS_DEFAULT_TYPE(0x08)
 #define MPI2_REQ_DESCRIPT_FLAGS_RAID_ACCELERATOR(0x0A)
 #define MPI25_REQ_DESCRIPT_FLAGS_FAST_PATH_SCSI_IO  (0x0C)
+#define MPI26_REQ_DESCRIPT_FLAGS_PCIE_ENCAPSULATED  (0x10)
 
 #define MPI2_REQ_DESCRIPT_FLAGS_IOC_FIFO_MARKER (0x01)
 
@@ -425,6 +442,13 @@ typedef MPI2_SCSI_IO_REQUEST_DESCRIPTOR
Mpi25FastPathSCSIIORequestDescriptor_t,
*pMpi25FastPathSCSIIORequestDescriptor_t;
 
+/*PCIe Encapsulated Request Descriptor */
+typedef MPI2_SCSI_IO_REQUEST_DESCRIPTOR
+   MPI26_PCIE_ENCAPSULATED_REQUEST_DESCRIPTOR,
+   *PTR_MPI26_PCIE_ENCAPSULATED_REQUEST_DESCRIPTOR,
+   Mpi26PCIeEncapsulatedRequestDescriptor_t,
+   *pMpi26PCIeEncapsulatedRequestDescriptor_t;
+
 /*union of Request Descriptors */
 typedef union _MPI2_REQUEST_DESCRIPTOR_UNION {
MPI2_DEFAULT_REQUEST_DESCRIPTOR Default;
@@ -433,6 +457,7 @@ typedef union _MPI2_REQUEST_DESCRIPTOR_UNION {
MPI2_SCSI_TARGET_REQUEST_DESCRIPTOR SCSITarget;
MPI2_RAID_ACCEL_REQUEST_DESCRIPTOR RAIDAccelerator;
MPI25_FP_SCSI_IO_REQUEST_DESCRIPTOR FastPathSCSIIO;
+   MPI26_PCIE_ENCAPSULATED_REQUEST_DESCRIPTOR PCIeEncapsulated;
U64 Words;
 } MPI2_REQUEST_DESCRIPTOR_UNION,
*PTR_MPI2_REQUEST_DESCRIPTOR_UNION,
@@ -450,6 +475,7 @@ typedef union _MPI2_REQUEST_DESCRIPTOR_UNION {
  *  Atomic SCSI Target Request Descriptor
  *  Atomic RAID Accelerator Request Descriptor
  *  Atomic Fast Path SCSI IO Request Descriptor
+ *  Atomic PCIe Encapsulated Request Descriptor
  */
 
 /*Atomic Request Descriptor */
@@ -487,6 +513,7 @@ typedef struct _MPI2_DEFAULT_REPLY_DESCRIPTOR {
 #define MPI2_RPY_DESCRIPT_FLAGS_TARGET_COMMAND_BUFFER   (0x03)
 #define MPI2_RPY_DESCRIPT_FLAGS_RAID_ACCELERATOR_SUCCESS(0x05)
 #define MPI25_RPY_DESCRIPT_FLAGS_FAST_PATH_SCSI_IO_SUCCESS  (0x06)
+#

[PATCH 09/10] mpt3sas: Adding support for SAS3616 HBA device

[Sreekanth Reddy]

Adding PNP ID of Mercator i.e. SAS3616 HBA device.
Its device ID is 0xD1 and vendor ID is 0x1000.

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b819914..3f640ba 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -8808,6 +8808,7 @@ _scsih_determine_hba_mpi_version(struct pci_dev *pdev)
case MPI26_MFGPAGE_DEVID_SAS3516:
case MPI26_MFGPAGE_DEVID_SAS3516_1:
case MPI26_MFGPAGE_DEVID_SAS3416:
+   case MPI26_MFGPAGE_DEVID_SAS3616:
return MPI26_VERSION;
}
return 0;
@@ -8885,6 +8886,7 @@ _scsih_probe(struct pci_dev *pdev, const struct 
pci_device_id *id)
case MPI26_MFGPAGE_DEVID_SAS3516:
case MPI26_MFGPAGE_DEVID_SAS3516_1:
case MPI26_MFGPAGE_DEVID_SAS3416:
+   case MPI26_MFGPAGE_DEVID_SAS3616:
ioc->is_gen35_ioc = 1;
break;
default:
@@ -9341,6 +9343,9 @@ static const struct pci_device_id mpt3sas_pci_table[] = {
PCI_ANY_ID, PCI_ANY_ID },
{ MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_SAS3416,
PCI_ANY_ID, PCI_ANY_ID },
+   /* Mercator ~ 3616*/
+   { MPI2_MFGPAGE_VENDORID_LSI, MPI26_MFGPAGE_DEVID_SAS3616,
+   PCI_ANY_ID, PCI_ANY_ID },
{0} /* Terminating entry */
 };
 MODULE_DEVICE_TABLE(pci, mpt3sas_pci_table);
-- 
2.4.3

[PATCH 08/10] mpt3sas: Fix possibility of using invalid Enclosure Handles for SAS device after host reset

[Sreekanth Reddy]

Enclosure handles are not updated after host reset.
As a result, driver device structure is holding previously
 assigned enclosure handle which is different from the
 enclosure handle populated in the corresponding device page.

Modified the driver to update devices enclosure handles after
 host reset to current value, by referring the enclosure handles
 from corresponding device pages

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 117 ---
 1 file changed, 81 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 17b934b..b819914 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -5383,6 +5383,52 @@ _scsih_check_access_status(struct MPT3SAS_ADAPTER *ioc, 
u64 sas_address,
 }
 
 /**
+ * _scsih_get_enclosure_logicalid_chassis_slot - get device's
+ * EnclosureLogicalID and ChassisSlot information.
+ * @ioc: per adapter object
+ * @sas_device_pg0: SAS device page0
+ * @sas_device: per sas device object
+ *
+ * Returns nothing.
+ */
+static void
+_scsih_get_enclosure_logicalid_chassis_slot(struct MPT3SAS_ADAPTER *ioc,
+   Mpi2SasDevicePage0_t *sas_device_pg0, struct _sas_device *sas_device)
+{
+   Mpi2ConfigReply_t mpi_reply;
+   Mpi2SasEnclosurePage0_t enclosure_pg0;
+
+   if (!sas_device_pg0 || !sas_device)
+   return;
+
+   sas_device->enclosure_handle =
+   le16_to_cpu(sas_device_pg0->EnclosureHandle);
+   sas_device->is_chassis_slot_valid = 0;
+
+   if (!le16_to_cpu(sas_device_pg0->EnclosureHandle))
+   return;
+
+   if (mpt3sas_config_get_enclosure_pg0(ioc, _reply,
+   _pg0, MPI2_SAS_ENCLOS_PGAD_FORM_HANDLE,
+   le16_to_cpu(sas_device_pg0->EnclosureHandle))) {
+   pr_err(MPT3SAS_FMT
+   "Enclosure Pg0 read failed for handle(0x%04x)\n",
+   ioc->name, le16_to_cpu(sas_device_pg0->EnclosureHandle));
+   return;
+   }
+
+   sas_device->enclosure_logical_id =
+   le64_to_cpu(enclosure_pg0.EnclosureLogicalID);
+
+   if (le16_to_cpu(enclosure_pg0.Flags) &
+   MPI2_SAS_ENCLS0_FLAGS_CHASSIS_SLOT_VALID) {
+   sas_device->is_chassis_slot_valid = 1;
+   sas_device->chassis_slot = enclosure_pg0.ChassisSlot;
+   }
+}
+
+
+/**
  * _scsih_check_device - checking device responsiveness
  * @ioc: per adapter object
  * @parent_sas_address: sas address of parent expander or sas host
@@ -5398,7 +5444,6 @@ _scsih_check_device(struct MPT3SAS_ADAPTER *ioc,
 {
Mpi2ConfigReply_t mpi_reply;
Mpi2SasDevicePage0_t sas_device_pg0;
-   Mpi2SasEnclosurePage0_t enclosure_pg0;
struct _sas_device *sas_device;
u32 ioc_status;
unsigned long flags;
@@ -5407,7 +5452,6 @@ _scsih_check_device(struct MPT3SAS_ADAPTER *ioc,
struct MPT3SAS_TARGET *sas_target_priv_data;
u32 device_info;
 
-
if ((mpt3sas_config_get_sas_device_pg0(ioc, _reply, _device_pg0,
MPI2_SAS_DEVICE_PGAD_FORM_HANDLE, handle)))
return;
@@ -5454,18 +5498,9 @@ _scsih_check_device(struct MPT3SAS_ADAPTER *ioc,
sas_device->enclosure_level = 0;
sas_device->connector_name[0] = '\0';
}
-   sas_device->is_chassis_slot_valid = 0;
-   if (sas_device->enclosure_handle &&
-   !(mpt3sas_config_get_enclosure_pg0(ioc, _reply,
-   _pg0, MPI2_SAS_ENCLOS_PGAD_FORM_HANDLE,
-   sas_device->enclosure_handle))) {
-   if (le16_to_cpu(enclosure_pg0.Flags) &
-   MPI2_SAS_ENCLS0_FLAGS_CHASSIS_SLOT_VALID) {
-   sas_device->is_chassis_slot_valid = 1;
-   sas_device->chassis_slot =
-   enclosure_pg0.ChassisSlot;
-   }
-   }
+
+   _scsih_get_enclosure_logicalid_chassis_slot(ioc,
+   _device_pg0, sas_device);
}
 
/* check if device is present */
@@ -7088,10 +7123,8 @@ Mpi2SasDevicePage0_t *sas_device_pg0)
 {
struct MPT3SAS_TARGET *sas_target_priv_data = NULL;
struct scsi_target *starget;
-   struct _sas_device *sas_device;
+   struct _sas_device *sas_device = NULL;
unsigned long flags;
-   Mpi2SasEnclosurePage0_t enclosure_pg0;
-   Mpi2ConfigReply_t mpi_reply;
 
spin_lock_irqsave(>sas_device_lock, flags);
list_for_each_entry(sas_device, >sas_device_list, list) {
@@ -7131,18 +7164,8 @@ Mpi2SasDevicePage0_t *sas_device_pg0)
sas_device->connector_name[0] = '\0

[PATCH 10/10] mpt3sas: Bump mpt3sas driver version to v16.100.00.00

[Sreekanth Reddy]

Bump mpt3sas driver version to v16.100.00.00

Signed-off-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h 
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 75d90f2..0fe3969 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -73,8 +73,8 @@
 #define MPT3SAS_DRIVER_NAME"mpt3sas"
 #define MPT3SAS_AUTHOR "Avago Technologies <mpt-fusionlinux@avagotech.com>"
 #define MPT3SAS_DESCRIPTION"LSI MPT Fusion SAS 3.0 Device Driver"
-#define MPT3SAS_DRIVER_VERSION "15.100.00.00"
-#define MPT3SAS_MAJOR_VERSION  15
+#define MPT3SAS_DRIVER_VERSION "16.100.00.00"
+#define MPT3SAS_MAJOR_VERSION  16
 #define MPT3SAS_MINOR_VERSION  100
 #define MPT3SAS_BUILD_VERSION  0
 #define MPT3SAS_RELEASE_VERSION00
-- 
2.4.3

Re: [PATCH v2 00/13] mpt3sas driver NVMe support:

[Sreekanth Reddy]

On Tue, Aug 8, 2017 at 9:34 AM, Keith Busch  wrote:
> On Mon, Aug 07, 2017 at 08:45:25AM -0700, James Bottomley wrote:
>> On Mon, 2017-08-07 at 20:01 +0530, Kashyap Desai wrote:
>> >
>> > We have to attempt this use case and see how it behaves. I have not
>> > tried this, so not sure if things are really bad or just some tuning
>> > may be helpful. I will revert back to you on this.
>> >
>> > I understood request as -  We need some udev rules to be working well
>> > for *same* NVME drives if it is behind  or native .
>> > Example - If user has OS installed on NVME drive which is behind
>> >  driver as SCSI disk should be able to boot if he/she hooked
>> > same NVME drive which is detected by native  driver (and vice
>> > versa.)
>>
>> It's not just the udev rules, it's the tools as well; possibly things
>> like that nvme-cli toolkit Intel is doing.
>
> It looks like they can make existing nvme tooling work with little
> effort if they have the driver implement NVME_IOCTL_ADMIN_COMMAND, and

Precisely, I was thinking on the same line and you clarified. I will
spend sometime on this and get back to you.

> then have their driver build the MPI NVMe Encapsulated Request from that.

Hi Everyone,

Thanks for the discussion and feedback.  We have noted this (i.e. will
Encapsulate NVME_IOCTL_ADMIN_CMD received from nvme cli in to MPI NVMe
Encapsulated Request message and will issue this request to firmware)
as our next to-do item and I will post possible options (this may need
some discussion with our FW group, so need time to get back with all
the details).

We will be posting next version of patch set i.e. v3, which will a
accommodate below changes suggested by Hannes and Martin over v2 patch
set.

1. In the MPI header files, we have reformatted headers to have type
and variable on one line as suggested by Hannes,
2. As suggested by Martin, started using blk_queue_virt_boundary() API
for NVMe drives and simplified the PRP formation.
3. Removed 'TODO' comments

Thanks,
Sreekanth

Re: [PATCH v2 09/13] mpt3sas: scan and add nvme device after controller reset

[Sreekanth Reddy]

On Thu, Aug 3, 2017 at 12:10 PM, Hannes Reinecke  wrote:
> On 07/14/2017 03:22 PM, Suganath Prabu S wrote:
>> After Controller reset, Scan and add nvme device back to the topology.
>>
>> Signed-off-by: Chaitra P B 
>> Signed-off-by: Suganath Prabu S 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_scsih.c |  194 
>> +-
>>  1 files changed, 190 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> index c5a131f..e3e803c 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> @@ -4869,6 +4869,7 @@ _scsih_scsi_ioc_info(struct MPT3SAS_ADAPTER *ioc, 
>> struct scsi_cmnd *scmd,
>>   char *desc_scsi_state = ioc->tmp_string;
>>   u32 log_info = le32_to_cpu(mpi_reply->IOCLogInfo);
>>   struct _sas_device *sas_device = NULL;
>> + struct _pcie_device *pcie_device = NULL;
>>   struct scsi_target *starget = scmd->device->sdev_target;
>>   struct MPT3SAS_TARGET *priv_target = starget->hostdata;
>>   char *device_str = NULL;
>> @@ -5001,6 +5002,28 @@ _scsih_scsi_ioc_info(struct MPT3SAS_ADAPTER *ioc, 
>> struct scsi_cmnd *scmd,
>>   if (priv_target->flags & MPT_TARGET_FLAGS_VOLUME) {
>>   pr_warn(MPT3SAS_FMT "\t%s wwid(0x%016llx)\n", ioc->name,
>>   device_str, (unsigned long long)priv_target->sas_address);
>> + } else if (priv_target->flags & MPT_TARGET_FLAGS_PCIE_DEVICE) {
>> + pcie_device = mpt3sas_get_pdev_from_target(ioc, priv_target);
>> + if (pcie_device) {
>> + pr_info(MPT3SAS_FMT "\twwid(0x%016llx), port(%d)\n",
>> + ioc->name,
>> + (unsigned long long)pcie_device->wwid,
>> + pcie_device->port_num);
>> + if (pcie_device->enclosure_handle != 0)
>> + pr_info(MPT3SAS_FMT
>> + "\tenclosure logical id(0x%016llx), "
>> + "slot(%d)\n", ioc->name,
>> + (unsigned long long)
>> + pcie_device->enclosure_logical_id,
>> + pcie_device->slot);
>> + if (pcie_device->connector_name[0])
>> + pr_info(MPT3SAS_FMT
>> + "\tenclosure level(0x%04x),"
>> + "connector name( %s)\n",
>> + ioc->name, pcie_device->enclosure_level,
>> + pcie_device->connector_name);
>> + pcie_device_put(pcie_device);
>> + }
>>   } else {
>>   sas_device = mpt3sas_get_sdev_from_target(ioc, priv_target);
>>   if (sas_device) {
>> @@ -5047,11 +5070,10 @@ _scsih_scsi_ioc_info(struct MPT3SAS_ADAPTER *ioc, 
>> struct scsi_cmnd *scmd,
>>   struct sense_info data;
>>   _scsih_normalize_sense(scmd->sense_buffer, );
>>   pr_warn(MPT3SAS_FMT
>> - "\t[sense_key,asc,ascq]: [0x%02x,0x%02x,0x%02x], 
>> count(%d)\n",
>> - ioc->name, data.skey,
>> - data.asc, data.ascq, le32_to_cpu(mpi_reply->SenseCount));
>> +   "\t[sense_key,asc,ascq]: [0x%02x,0x%02x,0x%02x], 
>> count(%d)\n",
>> +   ioc->name, data.skey,
>> +   data.asc, data.ascq, le32_to_cpu(mpi_reply->SenseCount));
>>   }
>> -
>>   if (scsi_state & MPI2_SCSI_STATE_RESPONSE_INFO_VALID) {
>>   response_info = le32_to_cpu(mpi_reply->ResponseInfo);
>>   response_bytes = (u8 *)_info;
>> @@ -8512,6 +8534,130 @@ _scsih_search_responding_sas_devices(struct 
>> MPT3SAS_ADAPTER *ioc)
>>  }
>>
>>  /**
>> + * _scsih_mark_responding_pcie_device - mark a pcie_device as responding
>> + * @ioc: per adapter object
>> + * @pcie_device_pg0: PCIe Device page 0
>> + *
>> + * After host reset, find out whether devices are still responding.
>> + * Used in _scsih_remove_unresponding_devices.
>> + *
>> + * Return nothing.
>> + */
>> +static void
>> +_scsih_mark_responding_pcie_device(struct MPT3SAS_ADAPTER *ioc,
>> + Mpi26PCIeDevicePage0_t *pcie_device_pg0)
>> +{
>> + struct MPT3SAS_TARGET *sas_target_priv_data = NULL;
>> + struct scsi_target *starget;
>> + struct _pcie_device *pcie_device;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(>pcie_device_lock, flags);
>> + list_for_each_entry(pcie_device, >pcie_device_list, list) {
>> + if ((pcie_device->wwid == pcie_device_pg0->WWID) &&
>> + (pcie_device->slot == pcie_device_pg0->Slot)) {
>> + pcie_device->responding = 1;
>> + starget = pcie_device->starget;
>> + if (starget && 

Re: [PATCH v2 02/13] mpt3sas: Add nvme device support in slave alloc, target alloc and probe

[Sreekanth Reddy]

On Thu, Aug 3, 2017 at 11:57 AM, Hannes Reinecke  wrote:
> On 07/14/2017 03:22 PM, Suganath Prabu S wrote:
>> 1) Added support for probing pcie device and adding NVMe drives to
>> SML and driver's internal list pcie_device_list.
>>
>> 2) Added support for determing NVMe as boot device.
>>
>> 3) Added nvme device support for call back functions scan_finished
>> target_alloc,slave_alloc,target destroy and slave destroy.
>>
>>  a) During scan, pcie devices are probed and added to SML to drivers
>> internal list.
>>
>>  b) target_alloc & slave alloc API's allocates resources for
>> (MPT3SAS_TARGET & MPT3SAS_DEVICE) private datas and holds
>> information like handle, target_id etc.
>>
>>  c) slave_destroy & target_destroy are called when driver unregisters
>> or removes device. Also frees allocated resources and info.
>>
>> Signed-off-by: Chaitra P B 
>> Signed-off-by: Suganath Prabu S 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  110 -
>>  drivers/scsi/mpt3sas/mpt3sas_scsih.c |  431 
>> +++---
>>  2 files changed, 507 insertions(+), 34 deletions(-)
>>
> Have you considered using 'scan_start()/scan_finished()' SCSI midlayer
> callbacks here?
> Seeing that you are enumerating the devices internally already that
> should give you better control about the scanning process.

Yes mpt3sas driver has defined scan_start() & scan_finished()
callbacks function. In the scan_start() callback function driver will
issue Port Enable request to the HBA Firmware to enable the HBA ports,
so the driver can start receiving drives (SAS/SATA/NVMe) discovery
events from firmware and add the discovered drives to the respective
drive type list. (e.g. SAS & SATA drives are added sas_device_list and
NVMe drives are added to pcie_device_list) . Once the driver receives
the Port Enable completion reply message from firmware then in the
scan_finished() callback function driver will register the SAS & SATA
devices added in the sas_device_list to SCSI Transport Layer by using
sas_rphy_add() API and NVMe devices added in the pcie_device_list are
directly added to SML by using scsi_add_device() API with channel
number set to two.

Thanks,
Sreekanth
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes ReineckeTeamlead Storage & Networking
> h...@suse.de   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH v2 08/13] mpt3sas: Set NVMe device queue depth as 128

[Sreekanth Reddy]

On Thu, Aug 3, 2017 at 12:09 PM, Hannes Reinecke  wrote:
> On 07/14/2017 03:22 PM, Suganath Prabu S wrote:
>> Sets nvme device queue depth, name and displays device capabilities
>>
>> Signed-off-by: Chaitra P B 
>> Signed-off-by: Suganath Prabu S 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.h  |2 +-
>>  drivers/scsi/mpt3sas/mpt3sas_scsih.c |   40 
>> ++
>>  2 files changed, 41 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.h
>> index 0a8187e..b7855c8 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
>> @@ -115,7 +115,7 @@
>>
>>  #define MPT3SAS_RAID_MAX_SECTORS 8192
>>  #define MPT3SAS_HOST_PAGE_SIZE_4K12
>> -
>> +#define MPT3SAS_NVME_QUEUE_DEPTH 128
>>  #define MPT_NAME_LENGTH  32  /* generic length of 
>> strings */
>>  #define MPT_STRING_LENGTH64
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> index 1dd9674..c5a131f 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> @@ -2290,6 +2290,7 @@ scsih_slave_configure(struct scsi_device *sdev)
>>   struct MPT3SAS_DEVICE *sas_device_priv_data;
>>   struct MPT3SAS_TARGET *sas_target_priv_data;
>>   struct _sas_device *sas_device;
>> + struct _pcie_device *pcie_device;
>>   struct _raid_device *raid_device;
>>   unsigned long flags;
>>   int qdepth;
>> @@ -2420,6 +2421,45 @@ scsih_slave_configure(struct scsi_device *sdev)
>>   }
>>   }
>>
>> + /* PCIe handling */
>> + if (sas_target_priv_data->flags & MPT_TARGET_FLAGS_PCIE_DEVICE) {
>> + spin_lock_irqsave(>pcie_device_lock, flags);
>> + pcie_device = __mpt3sas_get_pdev_by_wwid(ioc,
>> + sas_device_priv_data->sas_target->sas_address);
>> + if (!pcie_device) {
>> + spin_unlock_irqrestore(>pcie_device_lock, flags);
>> + dfailprintk(ioc, pr_warn(MPT3SAS_FMT
>> + "failure at %s:%d/%s()!\n", ioc->name, 
>> __FILE__,
>> + __LINE__, __func__));
>> + return 1;
>> + }
>> +
>> + /*TODO-right Queue Depth?*/
>> + qdepth = MPT3SAS_NVME_QUEUE_DEPTH;
>> + ds = "NVMe";
>> + /*TODO-Add device name when defined*/
>> + sdev_printk(KERN_INFO, sdev,
>> + "%s: handle(0x%04x), wwid(0x%016llx), port(%d)\n",
>> + ds, handle, (unsigned long long)pcie_device->wwid,
>> + pcie_device->port_num);
>> + if (pcie_device->enclosure_handle != 0)
>> + sdev_printk(KERN_INFO, sdev,
>> + "%s: enclosure logical id(0x%016llx), slot(%d)\n",
>> + ds,
>> + (unsigned long long)pcie_device->enclosure_logical_id,
>> + pcie_device->slot);
>> + if (pcie_device->connector_name[0] != '\0')
>> + sdev_printk(KERN_INFO, sdev,
>> + "%s: enclosure level(0x%04x),"
>> + "connector name( %s)\n", ds,
>> + pcie_device->enclosure_level,
>> + pcie_device->connector_name);
>> + pcie_device_put(pcie_device);
>> + spin_unlock_irqrestore(>pcie_device_lock, flags);
>> + scsih_change_queue_depth(sdev, qdepth);
>> + return 0;
>> + }
>> +
>>   spin_lock_irqsave(>sas_device_lock, flags);
>>   sas_device = __mpt3sas_get_sdev_by_addr(ioc,
>>  sas_device_priv_data->sas_target->sas_address);
>>
> Well; what are these TODOs doing here?
> If you know things are not correct, why not doing them correctly?
> If you cannot do them correctly, why?

Hannes,

These TODOs comments are added during initial development phase. We
will remove these TODO comments.

Thanks,
Sreekanth

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes ReineckeTeamlead Storage & Networking
> h...@suse.de   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH v2 01/13] mpt3sas: Update MPI Header

[Sreekanth Reddy]

On Thu, Aug 3, 2017 at 1:25 PM, Johannes Thumshirn  wrote:
> On Thu, Aug 03, 2017 at 08:24:59AM +0200, Hannes Reinecke wrote:
>> > +   U32
>> > +   IOCLogInfo; /*0x10 */
>> > +   U16
>> > +   ErrorResponseCount; /*0x14 */
>> > +   U16
>> > +   Reserved4;  /*0x16 */
>> > +} MPI26_NVME_ENCAPSULATED_ERROR_REPLY,
>> > +   *PTR_MPI26_NVME_ENCAPSULATED_ERROR_REPLY,
>> > +   Mpi26NVMeEncapsulatedErrorReply_t,
>> > +   *pMpi26NVMeEncapsulatedErrorReply_t;
>> > +
>> > +
>> > +#endif
>> > +
>> > +
>> Very odd indentation.
>> Please reformat to have type and variable on one line.
>
> And use Linux types and no CamelCase and no typedefs (especially not for
> pointers), you're not on Windows here.

We will reformat to have type and variable on one line as suggested by Hannes.

But it will be going have a huge change in the driver, if we want to
remove the CamelCase and typedefs. From legacy days onwards mpi
headers has the CamelCase type variable names and the same variables
are used in the code in large number of places. Also our MPI headers
are common across the OS platforms and so maintaining mpi headers
updates will be more difficult.

Thanks,
Sreekanth

>
> Thanks,
> Johannes
>
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

mpt3sas: driver got task abort request just after it's .shutdown() function is invoked

[Sreekanth Reddy]

Hi All,

I am using 4.9 kernel and I am observing NULL pointer deference type
kernel panic in the below scenario,

* Hotplug (i.e. hot add) the HBA (with a set of drives attached to it)
to the system just few seconds before issuing "poweroff" command.

* Observed that during drives discovery process; for some of the
drives "MODE SENSE" command got timed out.

* And as "poweroff" command issued, so kernel as called driver's
shutdown() callback function and driver has cleaned up all the HBA
resources (such as IRQ's, memory pools etc).

* But as the "MODE SENSE" command got timed out, so SCSI EH thread has
invoked driver's .eh_abort_handler() callback function, but by this
time driver has already cleaned up the resources and so it leads to
kernel panic when it trying to access one of these resources.

I was assuming that kernel should not call driver's shutdown()
callback function until all the outstanding IOs count reaches to zero
(i.e. kernel should call the driver's shutdown() functions only after
clearing up all the outstanding IOs). Please correct me if I am wrong,
and please suggest better way to handle these types of issues.

Thanks,
Sreekanth


Here I am copying the required driver logs,

[  429.995550] mpt3sas :b6:00.0: enabling device (0100 -> 0102)
[  429.995629] mpt3sas_cm3: 64 BIT PCI BUS DMA ADDRESSING SUPPORTED,
total mem (131164804 kB)
[  429.995636] mpt3sas_cm3: _base_get_ioc_facts
[  429.995636] mpt3sas_cm3: _base_wait_for_iocstate
[  430.109107] offset:data
[  430.109107] [0x00]:03110206
[  430.109108] [0x04]:2e00
[  430.109108] [0x08]:
[  430.109108] [0x0c]:
[  430.109109] [0x10]:
[  430.109109] [0x14]:60010080
[  430.109110] [0x18]:22281700
[  430.109110] [0x1c]:0007a85c
[  430.109110] [0x20]:0e00
[  430.109111] [0x24]:00100040
[  430.109111] [0x28]:0180001b
[  430.109111] [0x2c]:00b100b0
[  430.109112] [0x30]:006c0003
[  430.109112] [0x34]:0020ffe0
[  430.109112] [0x38]:00800263
[  430.109113] [0x3c]:0019
[  430.109113] mpt3sas_cm3: IOC Number : 0
[  430.109114] mpt3sas_cm3: hba queue depth(5888), max chains per io(128)
[  430.109114] mpt3sas_cm3: request frame size(256), reply frame size(128)
[  430.109130] mpt3sas_cm3: msix is supported, vector_count(96)
[  430.109131] mpt3sas_cm3: MSI-X vectors supported: 96, no of cores:
16, max_msix_vectors: -1
[  430.109497] mpt3sas3-msix0: PCI-MSI-X enabled: IRQ 76
[  430.109497] mpt3sas3-msix1: PCI-MSI-X enabled: IRQ 77
[  430.109498] mpt3sas3-msix2: PCI-MSI-X enabled: IRQ 78
[  430.109498] mpt3sas3-msix3: PCI-MSI-X enabled: IRQ 79
[  430.109499] mpt3sas3-msix4: PCI-MSI-X enabled: IRQ 80
[  430.109499] mpt3sas3-msix5: PCI-MSI-X enabled: IRQ 81
[  430.109499] mpt3sas3-msix6: PCI-MSI-X enabled: IRQ 82
[  430.109500] mpt3sas3-msix7: PCI-MSI-X enabled: IRQ 83
[  430.109500] mpt3sas3-msix8: PCI-MSI-X enabled: IRQ 84
[  430.109500] mpt3sas3-msix9: PCI-MSI-X enabled: IRQ 85
[  430.109501] mpt3sas3-msix10: PCI-MSI-X enabled: IRQ 86
[  430.109501] mpt3sas3-msix11: PCI-MSI-X enabled: IRQ 87
[  430.109502] mpt3sas3-msix12: PCI-MSI-X enabled: IRQ 88
[  430.109502] mpt3sas3-msix13: PCI-MSI-X enabled: IRQ 89
[  430.109502] mpt3sas3-msix14: PCI-MSI-X enabled: IRQ 90
[  430.109503] mpt3sas3-msix15: PCI-MSI-X enabled: IRQ 91
[  430.109504] mpt3sas_cm3: iomem(0xfb80),
mapped(0xc9000e89), size(65536)
[  430.109504] mpt3sas_cm3: ioport(0xc000), size(256)
[  430.109531] mpt3sas_cm3: _base_get_ioc_facts
[  430.109531] mpt3sas_cm3: _base_wait_for_iocstate
[  430.222840] offset:data
[  430.222840] [0x00]:03110206
[  430.222841] [0x04]:2e00
[  430.222841] [0x08]:
[  430.222842] [0x0c]:
[  430.222842] [0x10]:
[  430.222842] [0x14]:60010080
[  430.222843] [0x18]:22281700
[  430.222843] [0x1c]:0007a85c
[  430.222844] [0x20]:0e00
[  430.222844] [0x24]:00100040
[  430.222844] [0x28]:0180001b
[  430.222845] [0x2c]:00b100b0
[  430.222845] [0x30]:006c0003
[  430.222846] [0x34]:0020ffe0
[  430.222846] [0x38]:00800263
[  430.222846] [0x3c]:0019
[  430.222847] mpt3sas_cm3: IOC Number : 0
[  430.222847] mpt3sas_cm3: hba queue depth(5888), max chains per io(128)
[  430.222848] mpt3sas_cm3: request frame size(256), reply frame size(128)
[  430.222848] mpt3sas_cm3: _base_make_ioc_ready
[  430.222850] mpt3sas_cm3: _base_get_port_facts
[  430.276713] offset:data
[  430.276714] [0x00]:0507
[  430.276714] [0x04]:
[  430.276714] [0x08]:
[  430.276715] [0x0c]:
[  430.276715] [0x10]:
[  430.276716] [0x14]:3000
[  430.276716] [0x18]:00d8
[  430.276716] mpt3sas_cm3: _base_allocate_memory_pools
[  430.276717] mpt3sas_cm3: scatter gather: sge_in_main_msg(8),
sge_per_chain(15), sge_per_io(128), chains_per_io(9)
[  430.276718] mpt3sas_cm3: scsi host: can_queue depth (5772)
[  

Re: [RESEND][PATCH 07/10][SCSI]mpt2sas: Added Reply Descriptor Post Queue (RDPQ) Array support

[Sreekanth Reddy]

On Thu, Jul 24, 2014 at 1:16 AM, Martin K. Petersen
<martin.peter...@oracle.com> wrote:
>>>>>> "Sreekanth" == Sreekanth Reddy <sreekanth.re...@avagotech.com> writes:
>
> Sreekanth,
>
> Sreekanth> 2. As per MPI Spec, each set of 8 reply descriptor post
> Sreekanth> queues must have the same value for the upper 32-bits of
> Sreekanth> their memory address. So allocated set of eight queues in a
> Sreekanth> single pool and added a new function is_MSB_are_same() to
> Sreekanth> check whether higher 32 bits of this pool memory address are
> Sreekanth> same or not. If this functions returns zero then we are
> Sreekanth> saving these pools in the bad_reply_post_pool list. then
> Sreekanth> releasing these pools once we get the required memory pools.
>
> Why don't you just set pci_set_consistent_dma_mask() to DMA_BIT_MASK(32)
> before you allocate the queue entries?

Martin,

I am taking out this old mail to find out is their any other better
way to make sure that allocated DMA pool doesn't cross particular
boundary line (in our case all upper 32 bits of this pool should be
same, i.e. all the buffer from the pool should be within the 4GB
boundary).

We need to satisfy this condition on those system where 32 bit dma
consistent mask is not supported and it only supports 64 bit dma
consistent mask. So on these system we can't set
pci_set_consistent_dma_mask() to DMA_BIT_MASK(32).

Thanks,
Sreekanth

>
> --
> Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

[Sreekanth Reddy]

On Thu, Apr 20, 2017 at 7:58 AM, Martin K. Petersen
 wrote:
> Sinan Kaya  writes:
>
>> Due to relaxed ordering requirements on multiple architectures,
>> drivers are required to use wmb/rmb/mb combinations when they need to
>> guarantee observability between the memory and the HW.
>>
>> The mpt3sas driver is already using wmb() for this purpose.  However,
>> it issues a writel following wmb(). writel() function on arm/arm64
>> arhictectures have an embedded wmb() call inside.

[Sreekanth] Whether same thing applicable for SPARC & POWER
architectures. If yes then we are fine with this patch changes.

>>
>> This results in unnecessary performance loss and code duplication.
>>
>> writel already guarantees ordering for both cpu and bus. we don't need
>> additional wmb()
>
> Broadcom folks, please review!
>
> --
> Martin K. Petersen  Oracle Linux Engineering

Re: Getting "Wrong diagnostic page; asked for 7 got 0" error message on HBA's virtual SES device

[Sreekanth Reddy]

Hi,

Any Update?

Thanks,
Sreekanth

On Mon, Mar 13, 2017 at 12:13 PM, Sreekanth Reddy
<sreekanth.re...@broadcom.com> wrote:
> Hi,
>
> Our LSI(Broadcom) SAS3.5 HBA device's support virtual SES device.
>
> Whenever we load the mpt3sas driver then we are observing below error message,
>
> "Wrong diagnostic page; asked for 7 got 0"
>
> Our virtual SES device doesn't support Diagnostic page 7, it supports
> only below diagnostic pages,
>
> • 00h List of Supported Diagnostic Pages
> • 01h SES Configuration Diagnostic Page
> • 02h SES Enclosure Control/Enclosure Status Diagnostic Page
> • 0Ah SES Additional Element Status Diagnostic Page
>
> Page Code 07 is Element Descriptor Diagnostic Page
> Per SES3 spec (see table 10) this page is optional and not supported
> by our internal VSES.
>
> But 'ses' kernel module is issuing a RECEIVE_DIAGNOSTIC command for
> diagnostic page 7 without checking whether target device supports this
> page or not (i.e. without looking into 00h page) as shown below,
>
> result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
> if (result)
> goto simple_populate;
>
> As the virtual SES devices doesn't support this page 7, so it has
> failed this RECEIVE_DIAGNOSTIC command with illegal request sense key
> as shown below,
>
> ses 11:0:0:0: tag#0 Send: scmd 0x883fee898000
> ses 11:0:0:0: tag#0 CDB: Receive Diagnostic 1c 01 07 00 20 00
> ses 11:0:0:0: tag#0 CDB: Receive Diagnostic 1c 01 07 00 20 00
> mpt3sas_cm1:sas_address(0x510600b012345600), phy(16)
> mpt3sas_cm1:enclosure_logical_id(0x500605b012345600),slot(16)
> mpt3sas_cm1:enclosure level(0x), connector name( )
> mpt3sas_cm1:handle(0x0011), ioc_status(success)(0x), smid(1)
> mpt3sas_cm1:request_len(32), underflow(0), resid(32)
> mpt3sas_cm1:tag(0), transfer_count(0), sc->result(0x0002)
> mpt3sas_cm1:scsi_status(check condition)(0x02),
> scsi_state(autosense valid )(0x01)
> mpt3sas_cm1:[sense_key,asc,ascq]: [0x05,0x26,0x00], count(18)
> mpt3sas_cm1: log_info(0x31200205): originator(PL), code(0x20), 
> sub_code(0x0205)
> ses 11:0:0:0: tag#0 Done: SUCCESS Result: hostbyte=DID_TARGET_FAILURE
> driverbyte=DRIVER_OK
> ses 11:0:0:0: tag#0 CDB: Receive Diagnostic 1c 01 07 00 20 00
> ses 11:0:0:0: tag#0 Sense Key : Illegal Request [current]
> ses 11:0:0:0: tag#0 Add. Sense: Invalid field in parameter list
> ses 11:0:0:0: tag#0 scsi host busy 1 failed 0
> ses 11:0:0:0: Notifying upper driver of completion (result 812)
> ses 11:0:0:0: tag#0 0 sectors total, 32 bytes done.
> ses 11:0:0:0: Wrong diagnostic page; asked for 7 got 0
>
> As the command is failed with illegal request sense key, so the buffer
> used In function 'ses_recv_diag' will be not updated and so it will
> have all zero's. so the page_code will be zero and we observe below
> wrong error message even though vSES device has failed the command
> with illegal request error message and it has not returned any wrong
> diagnostic page.
>
> sdev_printk(KERN_ERR, sdev,
> "Wrong diagnostic page; asked for %d got %u\n",
> page_code, recv_page_code);
>
> Thanks,
> Sreekanth

Getting "Wrong diagnostic page; asked for 7 got 0" error message on HBA's virtual SES device

[Sreekanth Reddy]

Hi,

Our LSI(Broadcom) SAS3.5 HBA device's support virtual SES device.

Whenever we load the mpt3sas driver then we are observing below error message,

"Wrong diagnostic page; asked for 7 got 0"

Our virtual SES device doesn't support Diagnostic page 7, it supports
only below diagnostic pages,

• 00h List of Supported Diagnostic Pages
• 01h SES Configuration Diagnostic Page
• 02h SES Enclosure Control/Enclosure Status Diagnostic Page
• 0Ah SES Additional Element Status Diagnostic Page

Page Code 07 is Element Descriptor Diagnostic Page
Per SES3 spec (see table 10) this page is optional and not supported
by our internal VSES.

But 'ses' kernel module is issuing a RECEIVE_DIAGNOSTIC command for
diagnostic page 7 without checking whether target device supports this
page or not (i.e. without looking into 00h page) as shown below,

result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
if (result)
goto simple_populate;

As the virtual SES devices doesn't support this page 7, so it has
failed this RECEIVE_DIAGNOSTIC command with illegal request sense key
as shown below,

ses 11:0:0:0: tag#0 Send: scmd 0x883fee898000
ses 11:0:0:0: tag#0 CDB: Receive Diagnostic 1c 01 07 00 20 00
ses 11:0:0:0: tag#0 CDB: Receive Diagnostic 1c 01 07 00 20 00
mpt3sas_cm1:sas_address(0x510600b012345600), phy(16)
mpt3sas_cm1:enclosure_logical_id(0x500605b012345600),slot(16)
mpt3sas_cm1:enclosure level(0x), connector name( )
mpt3sas_cm1:handle(0x0011), ioc_status(success)(0x), smid(1)
mpt3sas_cm1:request_len(32), underflow(0), resid(32)
mpt3sas_cm1:tag(0), transfer_count(0), sc->result(0x0002)
mpt3sas_cm1:scsi_status(check condition)(0x02),
scsi_state(autosense valid )(0x01)
mpt3sas_cm1:[sense_key,asc,ascq]: [0x05,0x26,0x00], count(18)
mpt3sas_cm1: log_info(0x31200205): originator(PL), code(0x20), sub_code(0x0205)
ses 11:0:0:0: tag#0 Done: SUCCESS Result: hostbyte=DID_TARGET_FAILURE
driverbyte=DRIVER_OK
ses 11:0:0:0: tag#0 CDB: Receive Diagnostic 1c 01 07 00 20 00
ses 11:0:0:0: tag#0 Sense Key : Illegal Request [current]
ses 11:0:0:0: tag#0 Add. Sense: Invalid field in parameter list
ses 11:0:0:0: tag#0 scsi host busy 1 failed 0
ses 11:0:0:0: Notifying upper driver of completion (result 812)
ses 11:0:0:0: tag#0 0 sectors total, 32 bytes done.
ses 11:0:0:0: Wrong diagnostic page; asked for 7 got 0

As the command is failed with illegal request sense key, so the buffer
used In function 'ses_recv_diag' will be not updated and so it will
have all zero's. so the page_code will be zero and we observe below
wrong error message even though vSES device has failed the command
with illegal request error message and it has not returned any wrong
diagnostic page.

sdev_printk(KERN_ERR, sdev,
"Wrong diagnostic page; asked for %d got %u\n",
page_code, recv_page_code);

Thanks,
Sreekanth

Re: [PATCHv4 09/12] mpt3sas: simplify task management functions

[Sreekanth Reddy]

I feel that using these flags are not working as expected. From the
driver's prospective it should return status of the TM based on
whether it has cleared reference of the timed out IO in the driver or
not (i.e. if it is successfully able to clear the reference (i.e.
cleared from scsi lookup) of the timed out IO from driver, firmware
then return success status otherwise return failure status). It should
not check for it's above layer reference.

On Wed, Feb 22, 2017 at 4:01 PM, Hannes Reinecke  wrote:
> One can simply check 'target_busy' or 'device_busy' when figuring
> out if there are outstanding commands; no need to painstakingly
> counting them by hand.
>
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 88 
> +++-
>  1 file changed, 7 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 1c45fb3..e0cb35d 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -1133,74 +1133,6 @@ struct _sas_node *
>  }
>
>  /**
> - * _scsih_scsi_lookup_find_by_target - search for matching channel:id
> - * @ioc: per adapter object
> - * @id: target id
> - * @channel: channel
> - * Context: This function will acquire ioc->scsi_lookup_lock.
> - *
> - * This will search for a matching channel:id in the scsi_lookup array,
> - * returning 1 if found.
> - */
> -static u8
> -_scsih_scsi_lookup_find_by_target(struct MPT3SAS_ADAPTER *ioc, int id,
> -   int channel)
> -{
> -   u8 found;
> -   unsigned long   flags;
> -   int i;
> -
> -   spin_lock_irqsave(>scsi_lookup_lock, flags);
> -   found = 0;
> -   for (i = 0 ; i < ioc->scsiio_depth; i++) {
> -   if (ioc->scsi_lookup[i].scmd &&
> -   (ioc->scsi_lookup[i].scmd->device->id == id &&
> -   ioc->scsi_lookup[i].scmd->device->channel == channel)) {
> -   found = 1;
> -   goto out;
> -   }
> -   }
> - out:
> -   spin_unlock_irqrestore(>scsi_lookup_lock, flags);
> -   return found;
> -}
> -
> -/**
> - * _scsih_scsi_lookup_find_by_lun - search for matching channel:id:lun
> - * @ioc: per adapter object
> - * @id: target id
> - * @lun: lun number
> - * @channel: channel
> - * Context: This function will acquire ioc->scsi_lookup_lock.
> - *
> - * This will search for a matching channel:id:lun in the scsi_lookup array,
> - * returning 1 if found.
> - */
> -static u8
> -_scsih_scsi_lookup_find_by_lun(struct MPT3SAS_ADAPTER *ioc, int id,
> -   unsigned int lun, int channel)
> -{
> -   u8 found;
> -   unsigned long   flags;
> -   int i;
> -
> -   spin_lock_irqsave(>scsi_lookup_lock, flags);
> -   found = 0;
> -   for (i = 0 ; i < ioc->scsiio_depth; i++) {
> -   if (ioc->scsi_lookup[i].scmd &&
> -   (ioc->scsi_lookup[i].scmd->device->id == id &&
> -   ioc->scsi_lookup[i].scmd->device->channel == channel &&
> -   ioc->scsi_lookup[i].scmd->device->lun == lun)) {
> -   found = 1;
> -   goto out;
> -   }
> -   }
> - out:
> -   spin_unlock_irqrestore(>scsi_lookup_lock, flags);
> -   return found;
> -}
> -
> -/**
>   * scsih_change_queue_depth - setting device queue depth
>   * @sdev: scsi device struct
>   * @qdepth: requested queue depth
> @@ -2339,19 +2271,9 @@ struct _sas_node *
> rc = FAILED;
> break;
>
> -   case MPI2_SCSITASKMGMT_TASKTYPE_TARGET_RESET:
> -   if (_scsih_scsi_lookup_find_by_target(ioc, id, channel))
> -   rc = FAILED;
> -   else
> -   rc = SUCCESS;
> -   break;
> case MPI2_SCSITASKMGMT_TASKTYPE_ABRT_TASK_SET:
> case MPI2_SCSITASKMGMT_TASKTYPE_LOGICAL_UNIT_RESET:
> -   if (_scsih_scsi_lookup_find_by_lun(ioc, id, lun, channel))
> -   rc = FAILED;
> -   else
> -   rc = SUCCESS;
> -   break;
> +   case MPI2_SCSITASKMGMT_TASKTYPE_TARGET_RESET:
> case MPI2_SCSITASKMGMT_TASKTYPE_QUERY_TASK:
> rc = SUCCESS;
> break;
> @@ -2554,7 +2476,9 @@ int mpt3sas_scsih_issue_locked_tm(struct 
> MPT3SAS_ADAPTER *ioc, u16 handle,
> r = mpt3sas_scsih_issue_locked_tm(ioc, handle, scmd->device->channel,
> scmd->device->id, scmd->device->lun,
> MPI2_SCSITASKMGMT_TASKTYPE_LOGICAL_UNIT_RESET, 0, 30);
> -
> +   /* Check for busy commands after reset */
> +   if (r == SUCCESS && atomic_read(>device->device_busy))
> +   r = FAILED;

For an experiment, I have mocked driver to return 'task abort' with
fail status even though it was able to abort the IO. I have done 

Re: [PATCHv4 12/12] mpt3sas: lockless command submission

[Sreekanth Reddy]

On Wed, Feb 22, 2017 at 4:01 PM, Hannes Reinecke  wrote:
> Instead of holding 'struct scsiio_tracker' in its own pool we
> can embed it into the payload of the scsi command. This allows
> us to get rid of the lock when submitting and receiving commands
> and streamline operations.
>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c  | 120 
> +--
>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  23 +++---
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c   |  17 +++--
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 118 +-
>  drivers/scsi/mpt3sas/mpt3sas_warpdrive.c |  35 +
>  5 files changed, 105 insertions(+), 208 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 169d185..966a775 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -863,12 +863,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>  }
>
>  struct scsiio_tracker *
> -mpt3sas_get_st_from_smid(struct MPT3SAS_ADAPTER *ioc, u16 smid)
> +_get_st_from_smid(struct MPT3SAS_ADAPTER *ioc, u16 smid)

Can we rename the name of this function as  _base_get_st_from_smid(),
Since we always try to follow below notations,
 * prefix with "_" if the function is used
within this file other wise prefix with "mpt3sas_" if this
function is shared between more than one file.

>  {
> +   struct scsi_cmnd *cmd;
> +
> if (WARN_ON(!smid) ||
> WARN_ON(smid >= ioc->hi_priority_smid))
> return NULL;
> -   return >scsi_lookup[smid - 1];
> +
> +   cmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
> +   if (cmd)
> +   return scsi_cmd_priv(cmd);
> +
> +   return NULL;
>  }
>
>  /**
> @@ -889,7 +896,7 @@ struct scsiio_tracker *
> struct scsiio_tracker *st;
>
> if (smid < ctl_smid) {
> -   st = mpt3sas_get_st_from_smid(ioc, smid);
> +   st = _get_st_from_smid(ioc, smid);
> if (st)
> cb_idx = st->cb_idx;

I think, here we can safely assign "cb_idx"  as "ioc->scsi_io_cb_idx",
no need to get it from scsiio_tracker. since all the smid's below
ctl_smid will we used for SCSI IO's recieved from scsih_qcmd.

> } else if (smid == ctl_smid)
> @@ -1276,15 +1283,16 @@ struct scsiio_tracker *
>  /**
>   * _base_get_chain_buffer_tracker - obtain chain tracker
>   * @ioc: per adapter object
> - * @smid: smid associated to an IO request
> + * @scmd: SCSI commands of the IO request
>   *
>   * Returns chain tracker(from ioc->free_chain_list)
>   */
>  static struct chain_tracker *
> -_base_get_chain_buffer_tracker(struct MPT3SAS_ADAPTER *ioc, u16 smid)
> +_base_get_chain_buffer_tracker(struct MPT3SAS_ADAPTER *ioc,
> +  struct scsi_cmnd *scmd)
>  {
> struct chain_tracker *chain_req;
> -   struct scsiio_tracker *st;
> +   struct scsiio_tracker *st = scsi_cmd_priv(scmd);
> unsigned long flags;
>
> spin_lock_irqsave(>scsi_lookup_lock, flags);
> @@ -1297,9 +1305,7 @@ struct scsiio_tracker *
> chain_req = list_entry(ioc->free_chain_list.next,
> struct chain_tracker, tracker_list);
> list_del_init(_req->tracker_list);
> -   st = mpt3sas_get_st_from_smid(ioc, smid);
> -   if (st)
> -   list_add_tail(_req->tracker_list, >chain_list);
> +   list_add_tail(_req->tracker_list, >chain_list);
> spin_unlock_irqrestore(>scsi_lookup_lock, flags);

Still we have this lock in the IO path. May be we can remove this lock
too by allocating "ioc->ioc->chains_needed_per_io * shost->can_queue"
number of chain buffers and we can acces the required chain buffer
using scmd's tag pluse chain_offset_per_io coressponding to that
repective tag.

May be we will post a separate patch for this after some time.

> return chain_req;
>  }
> @@ -1485,7 +1491,7 @@ struct scsiio_tracker *
>
> /* initializing the chain flags and pointers */
> chain_flags = MPI2_SGE_FLAGS_CHAIN_ELEMENT << MPI2_SGE_FLAGS_SHIFT;
> -   chain_req = _base_get_chain_buffer_tracker(ioc, smid);
> +   chain_req = _base_get_chain_buffer_tracker(ioc, scmd);
> if (!chain_req)
> return -1;
> chain = chain_req->chain_buffer;
> @@ -1525,7 +1531,7 @@ struct scsiio_tracker *
> sges_in_segment--;
> }
>
> -   chain_req = _base_get_chain_buffer_tracker(ioc, smid);
> +   chain_req = _base_get_chain_buffer_tracker(ioc, scmd);
> if (!chain_req)
> return -1;
> chain = chain_req->chain_buffer;
> @@ -1619,7 +1625,7 @@ struct scsiio_tracker *
> }
>
> /* initializing the pointers */
> -   chain_req = 

Re: [PATCHv4 11/12] mpt3sas: simplify _wait_for_commands_to_complete()

[Sreekanth Reddy]

On Wed, Feb 22, 2017 at 4:01 PM, Hannes Reinecke  wrote:
> Use 'host_busy' instead of counting outstanding commands by hand.
>
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 14 --
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index e6aafa5..169d185 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2408,9 +2408,9 @@ struct scsiio_tracker *
>  * See _wait_for_commands_to_complete() call with regards to this 
> code.
>  */
> if (ioc->shost_recovery && ioc->pending_io_count) {
> -   if (ioc->pending_io_count == 1)
> +   ioc->pending_io_count = atomic_read(>shost->host_busy);

This won't consider the scsi IO issued from ioctl path. If that scsi
io is still outstanding then here we will return without waiting for
it to complete.

> +   if (ioc->pending_io_count == 0)
> wake_up(>reset_wq);
> -   ioc->pending_io_count--;
> }
>  }
>
> @@ -5687,15 +5687,13 @@ struct scsiio_tracker *
>   * _wait_for_commands_to_complete - reset controller
>   * @ioc: Pointer to MPT_ADAPTER structure
>   *
> - * This function waiting(3s) for all pending commands to complete
> + * This function is waiting 10s for all pending commands to complete
>   * prior to putting controller in reset.
>   */
>  static void
>  _wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc)
>  {
> u32 ioc_state;
> -   unsigned long flags;
> -   u16 i;
>
> ioc->pending_io_count = 0;
>
> @@ -5704,11 +5702,7 @@ struct scsiio_tracker *
> return;
>
> /* pending command count */
> -   spin_lock_irqsave(>scsi_lookup_lock, flags);
> -   for (i = 0; i < ioc->scsiio_depth; i++)
> -   if (ioc->scsi_lookup[i].cb_idx != 0xFF)
> -   ioc->pending_io_count++;
> -   spin_unlock_irqrestore(>scsi_lookup_lock, flags);
> +   ioc->pending_io_count = atomic_read(>shost->host_busy);
>
> if (!ioc->pending_io_count)
> return;
> --
> 1.8.5.6
>

Re: [PATCHv3 01/10] mpt3sas: switch to pci_alloc_irq_vectors

[Sreekanth Reddy]

On Thu, Feb 23, 2017 at 6:22 AM, Martin K. Petersen
 wrote:
>> "Christoph" == Christoph Hellwig  writes:
>
> Christoph> Martin, can we get at least this patch still in for 4.11?
> Christoph> I'd really like to see as many intance of the old MSI-X
> Christoph> allocation and IRQ affinity mess dead as soon as possible.
>
> Me too. I'll queue it up for 4.11 unless Broadcom objects. And then
> we'll do the rest for 4.12.

This patch looks good. Please consider this patch for 4.11.

We need some time to review remaining patches as it has lot code
refinement and we need to cover some basic test cases.

Thanks,
Sreekanth

>
> --
> Martin K. Petersen  Oracle Linux Engineering

Re: [PATCHv3 07/10] mpt3sas: check command status before attempting abort

[Sreekanth Reddy]

On Tue, Feb 21, 2017 at 5:57 PM, Hannes Reinecke  wrote:
> When attempting a command abort we should check the command status
> prior to sending the abort; the command might've been completed
> already.

I think we are already taking care of this. Before calling
'mpt3sas_scsih_issue_tm()' function in 'scsih_abort()' we are already
verifying whether the scmd exists in the driver's scsi_lookup table or
not. if exits then we are issuing the task abort TM to Firmware
otherwise we will return success status from scsih_abort() without
issuing task abort TM to Firmware as shown below,

smid = _scsih_scsi_lookup_find_by_scmd(ioc, scmd);
if (!smid) {
scmd->result = DID_RESET << 16;
r = SUCCESS;
goto out;
}

Thanks,
Sreekanth

>
> Signed-off-by: Hannes Reinecke 
> Reviewed-by: Christoph Hellwig 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 6bc9291..1c45fb3 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -2261,6 +2261,14 @@ struct _sas_node *
> return (!rc) ? SUCCESS : FAILED;
> }
>
> +   if (type == MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK) {
> +   scsi_lookup = mpt3sas_get_st_from_smid(ioc, smid_task);
> +   if (!scsi_lookup)
> +   return FAILED;
> +   if (scsi_lookup->cb_idx == 0xFF)
> +   return SUCCESS;
> +   }
> +
> smid = mpt3sas_base_get_smid_hpr(ioc, ioc->tm_cb_idx);
> if (!smid) {
> pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
> @@ -2268,9 +2276,6 @@ struct _sas_node *
> return FAILED;
> }
>
> -   if (type == MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK)
> -   scsi_lookup = mpt3sas_get_st_from_smid(ioc, smid_task);
> -
> dtmprintk(ioc, pr_info(MPT3SAS_FMT
> "sending tm: handle(0x%04x), task_type(0x%02x), smid(%d)\n",
> ioc->name, handle, type, smid_task));
> --
> 1.8.5.6
>

Re: [PATCH 07/10] mpt3sas: use hi-priority queue for TMFs

[Sreekanth Reddy]

On Thu, Feb 16, 2017 at 3:44 PM, Hannes Reinecke <h...@suse.de> wrote:
> On 02/16/2017 11:09 AM, Sreekanth Reddy wrote:
>> On Tue, Jan 31, 2017 at 2:55 PM, Hannes Reinecke <h...@suse.de> wrote:
>>> When sending a TMF via the ioctl interface we should be using
>>> the hi-priority queue instead of the scsi queue to be consistent
>>> with overall TMF usage.
>>>
>>> Signed-off-by: Hannes Reinecke <h...@suse.com>
>>> ---
>>>  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
>>> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>>> index 95f0f24..e952175 100644
>>> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>>> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>>> @@ -720,7 +720,7 @@ enum block_state {
>>> }
>>> } else {
>>>
>>> -   smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, 
>>> NULL);
>>> +   smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);
>>
>> This else part is not for TM path, It is for IO path. For the TM
>> request we are already using smid from hpr queue, as shown below
>>
>> if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {
>> smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);
>> if (!smid) {
>> pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
>> ioc->name, __func__);
>> ret = -EAGAIN;
>> goto out;
>> }
>> } else {
>>
>> smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, 
>> NULL);
>>
> Yes, I know.
> But the current method of inserting a SCSI command completely goes
> against blk-mq request usage; with blk-mq the tags are managed with the
> block layer, so you need to integrate with that to get a correct tag.
>
> Is this _really_ a crucial functionality? Can't we just drop it and use
> sg/bsg for this?
> It would make my life _so_ much easier; the alternative would be to
> either implement 'real' reserved command handling or rewriting the above
> code-path in terms of 'struct request', packing and unpacking as we go.
> Neither is very appealing.

I think it is better to take out one request frame from can_queue
count and reserve it for this ioctl scsi io. Since we allow only one
ioctl command at a time.

Thanks,
Sreekanth

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes ReineckeTeamlead Storage & Networking
> h...@suse.de   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH 07/10] mpt3sas: use hi-priority queue for TMFs

[Sreekanth Reddy]

On Tue, Jan 31, 2017 at 2:55 PM, Hannes Reinecke  wrote:
> When sending a TMF via the ioctl interface we should be using
> the hi-priority queue instead of the scsi queue to be consistent
> with overall TMF usage.
>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 95f0f24..e952175 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -720,7 +720,7 @@ enum block_state {
> }
> } else {
>
> -   smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, 
> NULL);
> +   smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);

This else part is not for TM path, It is for IO path. For the TM
request we are already using smid from hpr queue, as shown below

if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {
smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);
if (!smid) {
pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
ioc->name, __func__);
ret = -EAGAIN;
goto out;
}
} else {

smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);

Thanks,
Sreekanth

> if (!smid) {
> pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
> ioc->name, __func__);
> --
> 1.8.5.6
>

Re: [PATCH 05/10] mpt3sas: open-code _scsih_scsi_lookup_get()

[Sreekanth Reddy]

On Tue, Jan 31, 2017 at 2:55 PM, Hannes Reinecke  wrote:
> Just a wrapper around the scsi lookup array and only used
> in one place, so open-code it.

I think this whole series of patches created over 14.101.00.00 version
driver not on the latest 15.100.00.00 driver. So this patch will
conflict with 15.100.00.00 version driver i.e. with commit
459325c466d278d3c9f51ddc9bb544c014136fd1(mpt3sas: Fix for Crusader to
achieve product targets with SAS devices).

Thanks,
Sreekanth

>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 +--
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index b5c966e..979f95a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -1061,19 +1061,6 @@ struct _sas_node *
>  }
>
>  /**
> - * _scsih_scsi_lookup_get - returns scmd entry
> - * @ioc: per adapter object
> - * @smid: system request message index
> - *
> - * Returns the smid stored scmd pointer.
> - */
> -static struct scsi_cmnd *
> -_scsih_scsi_lookup_get(struct MPT3SAS_ADAPTER *ioc, u16 smid)
> -{
> -   return ioc->scsi_lookup[smid - 1].scmd;
> -}
> -
> -/**
>   * _scsih_scsi_lookup_get_clear - returns scmd entry
>   * @ioc: per adapter object
>   * @smid: system request message index
> @@ -6101,7 +6088,7 @@ static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)
> for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
> if (ioc->shost_recovery)
> goto out;
> -   scmd = _scsih_scsi_lookup_get(ioc, smid);
> +   scmd = ioc->scsi_lookup[smid - 1].scmd;
> if (!scmd)
> continue;
> sdev = scmd->device;
> --
> 1.8.5.6
>

Re: [PATCH 04/10] mpt3sas: separate out _base_recovery_check()

[Sreekanth Reddy]

On Tue, Jan 31, 2017 at 2:55 PM, Hannes Reinecke  wrote:
> No functional change.
>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 120b317..dd14596 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2366,6 +2366,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>  }
>
>  static void
> +_base_recovery_check(struct MPT3SAS_ADAPTER *ioc)
> +{
> +   /*
> +* See _wait_for_commands_to_complete() call with regards to this 
> code.
> +*/
> +   if (ioc->shost_recovery && ioc->pending_io_count) {
> +   if (ioc->pending_io_count == 1)
> +   wake_up(>reset_wq);
> +   ioc->pending_io_count = 0;

I think here we should reduce the pending_io_count by one instead of
assigning it to zero in-order to have same behavior as before.

Thanks,
Sreekanth

> +   }
> +}
> +
> +static void
>  _dechain_st(struct MPT3SAS_ADAPTER *ioc, struct scsiio_tracker *st)
>  {
> struct chain_tracker *chain_req;
> @@ -2402,15 +2415,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> list_add(>scsi_lookup[i].tracker_list, >free_list);
> spin_unlock_irqrestore(>scsi_lookup_lock, flags);
>
> -   /*
> -* See _wait_for_commands_to_complete() call with regards
> -* to this code.
> -*/
> -   if (ioc->shost_recovery && ioc->pending_io_count) {
> -   if (ioc->pending_io_count == 1)
> -   wake_up(>reset_wq);
> -   ioc->pending_io_count--;
> -   }
> +   _base_recovery_check(ioc);
> return;
> } else if (smid < ioc->internal_smid) {
> /* hi-priority */
> --
> 1.8.5.6
>

Re: [PATCH 01/10] mpt3sas: switch to pci_alloc_irq_vectors

[Sreekanth Reddy]

On Tue, Jan 31, 2017 at 2:55 PM, Hannes Reinecke  wrote:
> Cleanup the MSI-X handling allowing us to use
> the PCI-layer provided vector allocation.
>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 100 
> +---
>  drivers/scsi/mpt3sas/mpt3sas_base.h |   2 -
>  2 files changed, 46 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index f00ef88..0185a8d 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -1129,7 +1129,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> /* TMs are on msix_index == 0 */
> if (reply_q->msix_index == 0)
> continue;
> -   synchronize_irq(reply_q->vector);
> +   synchronize_irq(pci_irq_vector(ioc->pdev, 
> reply_q->msix_index));
> }
>  }
>
> @@ -1818,11 +1818,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>
> list_for_each_entry_safe(reply_q, next, >reply_queue_list, list) 
> {
> list_del(_q->list);
> -   if (smp_affinity_enable) {
> -   irq_set_affinity_hint(reply_q->vector, NULL);
> -   free_cpumask_var(reply_q->affinity_hint);
> -   }
> -   free_irq(reply_q->vector, reply_q);
> +   free_irq(pci_irq_vector(ioc->pdev, reply_q->msix_index),
> +reply_q);
> kfree(reply_q);
> }
>  }
> @@ -1831,13 +1828,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>   * _base_request_irq - request irq
>   * @ioc: per adapter object
>   * @index: msix index into vector table
> - * @vector: irq vector
>   *
>   * Inserting respective reply_queue into the list.
>   */
>  static int
> -_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index, u32 vector)
> +_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index)
>  {
> +   struct pci_dev *pdev = ioc->pdev;
> struct adapter_reply_queue *reply_q;
> int r;
>
> @@ -1849,14 +1846,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> }
> reply_q->ioc = ioc;
> reply_q->msix_index = index;
> -   reply_q->vector = vector;
> -
> -   if (smp_affinity_enable) {
> -   if (!zalloc_cpumask_var(_q->affinity_hint, GFP_KERNEL)) 
> {
> -   kfree(reply_q);
> -   return -ENOMEM;
> -   }
> -   }
>
> atomic_set(_q->busy, 0);
> if (ioc->msix_enable)
> @@ -1865,12 +1854,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> else
> snprintf(reply_q->name, MPT_NAME_LENGTH, "%s%d",
> ioc->driver_name, ioc->id);
> -   r = request_irq(vector, _base_interrupt, IRQF_SHARED, reply_q->name,
> -   reply_q);
> +   r = request_irq(pci_irq_vector(pdev, index), _base_interrupt,
> +   IRQF_SHARED, reply_q->name, reply_q);
> if (r) {
> pr_err(MPT3SAS_FMT "unable to allocate interrupt %d!\n",
> -   reply_q->name, vector);
> -   free_cpumask_var(reply_q->affinity_hint);
> +  reply_q->name, pci_irq_vector(pdev, index));
> kfree(reply_q);
> return -EBUSY;
> }
> @@ -1906,6 +1894,21 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> if (!nr_msix)
> return;
>
> +   if (smp_affinity_enable) {
> +   list_for_each_entry(reply_q, >reply_queue_list, list) {
> +   const cpumask_t *mask = 
> pci_irq_get_affinity(ioc->pdev,
> +   reply_q->msix_index);
> +   if (!mask) {
> +   pr_warn(MPT3SAS_FMT "no affinity for msi 
> %x\n",
> +   ioc->name, reply_q->msix_index);
> +   continue;
> +   }
> +
> +   for_each_cpu(cpu, mask)
> +   ioc->cpu_msix_table[cpu] = 
> reply_q->msix_index;
> +   }
> +   return;
> +   }
> cpu = cpumask_first(cpu_online_mask);
>
> list_for_each_entry(reply_q, >reply_queue_list, list) {
> @@ -1919,18 +1922,9 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> group++;
>
> for (i = 0 ; i < group ; i++) {
> -   ioc->cpu_msix_table[cpu] = index;
> -   if (smp_affinity_enable)
> -   cpumask_or(reply_q->affinity_hint,
> -  reply_q->affinity_hint, get_cpu_mask(cpu));
> +   ioc->cpu_msix_table[cpu] = reply_q->msix_index;
> cpu = cpumask_next(cpu, cpu_online_mask);
> 

Re: [PATCH 00/10] mpt3sas: full mq support

[Sreekanth Reddy]

On Mon, Feb 13, 2017 at 6:41 PM, Hannes Reinecke <h...@suse.de> wrote:
> On 02/13/2017 07:15 AM, Sreekanth Reddy wrote:
>> On Fri, Feb 10, 2017 at 12:29 PM, Hannes Reinecke <h...@suse.de> wrote:
>>> On 02/10/2017 05:43 AM, Sreekanth Reddy wrote:
>>>> On Thu, Feb 9, 2017 at 6:42 PM, Hannes Reinecke <h...@suse.de> wrote:
>>>>> On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:
>>> [ .. ]
>>>>>>
>>>>>>
>>>>>> Hannes,
>>>>>>
>>>>>> I have created a md raid0 with 4 SAS SSD drives using below command,
>>>>>> #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh
>>>>>> /dev/sdi /dev/sdj
>>>>>>
>>>>>> And here is 'mdadm --detail /dev/md0' command output,
>>>>>> --
>>>>>> /dev/md0:
>>>>>> Version : 1.2
>>>>>>   Creation Time : Thu Feb  9 14:38:47 2017
>>>>>>  Raid Level : raid0
>>>>>>  Array Size : 780918784 (744.74 GiB 799.66 GB)
>>>>>>Raid Devices : 4
>>>>>>   Total Devices : 4
>>>>>> Persistence : Superblock is persistent
>>>>>>
>>>>>> Update Time : Thu Feb  9 14:38:47 2017
>>>>>>   State : clean
>>>>>>  Active Devices : 4
>>>>>> Working Devices : 4
>>>>>>  Failed Devices : 0
>>>>>>   Spare Devices : 0
>>>>>>
>>>>>>  Chunk Size : 512K
>>>>>>
>>>>>>Name : host_name
>>>>>>UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
>>>>>>  Events : 0
>>>>>>
>>>>>> Number   Major   Minor   RaidDevice State
>>>>>>0   8   960  active sync   /dev/sdg
>>>>>>1   8  1121  active sync   /dev/sdh
>>>>>>2   8  1442  active sync   /dev/sdj
>>>>>>3   8  1283  active sync   /dev/sdi
>>>>>> --
>>>>>>
>>>>>> Then I have used below fio profile to run 4K sequence read operations
>>>>>> with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my
>>>>>> system has two numa node and each with 12 cpus).
>>>>>> -
>>>>>> global]
>>>>>> ioengine=libaio
>>>>>> group_reporting
>>>>>> direct=1
>>>>>> rw=read
>>>>>> bs=4k
>>>>>> allow_mounted_write=0
>>>>>> iodepth=128
>>>>>> runtime=150s
>>>>>>
>>>>>> [job1]
>>>>>> filename=/dev/md0
>>>>>> -
>>>>>>
>>>>>> Here are the fio results when nr_hw_queues=1 (i.e. single request
>>>>>> queue) with various number of job counts
>>>>>> 1JOB 4k read  : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec
>>>>>> 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec
>>>>>> 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec
>>>>>> 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec
>>>>>>
>>>>>> Here are the fio results when nr_hw_queues=24 (i.e. multiple request
>>>>>> queue) with various number of job counts
>>>>>> 1JOB 4k read   : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
>>>>>> 2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec
>>>>>> 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec
>>>>>> 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec
>>>>>>
>>>>>> Here we can see that on less number of jobs count, single request
>>>>>> queue (nr_hw_queues=1) is giving more IOPs than multi request
>>>>>> queues(nr_hw_queues=24).
>>>>>>
>&g

Re: [PATCH 00/10] mpt3sas: full mq support

[Sreekanth Reddy]

On Fri, Feb 10, 2017 at 12:29 PM, Hannes Reinecke <h...@suse.de> wrote:
> On 02/10/2017 05:43 AM, Sreekanth Reddy wrote:
>> On Thu, Feb 9, 2017 at 6:42 PM, Hannes Reinecke <h...@suse.de> wrote:
>>> On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:
> [ .. ]
>>>>
>>>>
>>>> Hannes,
>>>>
>>>> I have created a md raid0 with 4 SAS SSD drives using below command,
>>>> #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh
>>>> /dev/sdi /dev/sdj
>>>>
>>>> And here is 'mdadm --detail /dev/md0' command output,
>>>> --
>>>> /dev/md0:
>>>> Version : 1.2
>>>>   Creation Time : Thu Feb  9 14:38:47 2017
>>>>  Raid Level : raid0
>>>>  Array Size : 780918784 (744.74 GiB 799.66 GB)
>>>>Raid Devices : 4
>>>>   Total Devices : 4
>>>> Persistence : Superblock is persistent
>>>>
>>>> Update Time : Thu Feb  9 14:38:47 2017
>>>>   State : clean
>>>>  Active Devices : 4
>>>> Working Devices : 4
>>>>  Failed Devices : 0
>>>>   Spare Devices : 0
>>>>
>>>>  Chunk Size : 512K
>>>>
>>>>Name : host_name
>>>>UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
>>>>  Events : 0
>>>>
>>>> Number   Major   Minor   RaidDevice State
>>>>0   8   960  active sync   /dev/sdg
>>>>1   8  1121  active sync   /dev/sdh
>>>>2   8  1442  active sync   /dev/sdj
>>>>3   8  1283  active sync   /dev/sdi
>>>> --
>>>>
>>>> Then I have used below fio profile to run 4K sequence read operations
>>>> with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my
>>>> system has two numa node and each with 12 cpus).
>>>> -
>>>> global]
>>>> ioengine=libaio
>>>> group_reporting
>>>> direct=1
>>>> rw=read
>>>> bs=4k
>>>> allow_mounted_write=0
>>>> iodepth=128
>>>> runtime=150s
>>>>
>>>> [job1]
>>>> filename=/dev/md0
>>>> -
>>>>
>>>> Here are the fio results when nr_hw_queues=1 (i.e. single request
>>>> queue) with various number of job counts
>>>> 1JOB 4k read  : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec
>>>> 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec
>>>> 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec
>>>> 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec
>>>>
>>>> Here are the fio results when nr_hw_queues=24 (i.e. multiple request
>>>> queue) with various number of job counts
>>>> 1JOB 4k read   : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
>>>> 2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec
>>>> 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec
>>>> 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec
>>>>
>>>> Here we can see that on less number of jobs count, single request
>>>> queue (nr_hw_queues=1) is giving more IOPs than multi request
>>>> queues(nr_hw_queues=24).
>>>>
>>>> Can you please share your fio profile, so that I can try same thing on
>>>> my system.
>>>>
>>> Have you tried with the latest git update from Jens for-4.11/block (or
>>> for-4.11/next) branch?
>>
>> I am using below git repo,
>>
>> https://git.kernel.org/cgit/linux/kernel/git/mkp/scsi.git/log/?h=4.11/scsi-queue
>>
>> Today I will try with Jens for-4.11/block.
>>
> By all means, do.
>
>>> I've found that using the mq-deadline scheduler has a noticeable
>>> performance boost.
>>>
>>> The fio job I'm using is essentially the same; you just should make sure
>>> to specify a 'numjob=' statement in there.
>>> Otherwi

Re: [PATCH 00/10] mpt3sas: full mq support

[Sreekanth Reddy]

On Thu, Feb 9, 2017 at 6:42 PM, Hannes Reinecke <h...@suse.de> wrote:
> On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:
>> On Wed, Feb 1, 2017 at 1:13 PM, Hannes Reinecke <h...@suse.de> wrote:
>>>
>>> On 02/01/2017 08:07 AM, Kashyap Desai wrote:
>>>>>
>>>>> -Original Message-
>>>>> From: Hannes Reinecke [mailto:h...@suse.de]
>>>>> Sent: Wednesday, February 01, 2017 12:21 PM
>>>>> To: Kashyap Desai; Christoph Hellwig
>>>>> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org;
>>>>> Sathya
>>>>> Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
>>>>> Subject: Re: [PATCH 00/10] mpt3sas: full mq support
>>>>>
>>>>> On 01/31/2017 06:54 PM, Kashyap Desai wrote:
>>>>>>>
>>>>>>> -Original Message-
>>>>>>> From: Hannes Reinecke [mailto:h...@suse.de]
>>>>>>> Sent: Tuesday, January 31, 2017 4:47 PM
>>>>>>> To: Christoph Hellwig
>>>>>>> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org;
>>>>>>
>>>>>> Sathya
>>>>>>>
>>>>>>> Prakash; Kashyap Desai; mpt-fusionlinux@broadcom.com
>>>>>>> Subject: Re: [PATCH 00/10] mpt3sas: full mq support
>>>>>>>
>>>>>>> On 01/31/2017 11:02 AM, Christoph Hellwig wrote:
>>>>>>>>
>>>>>>>> On Tue, Jan 31, 2017 at 10:25:50AM +0100, Hannes Reinecke wrote:
>>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> this is a patchset to enable full multiqueue support for the
>>>>>>>>> mpt3sas
>>>>>>>
>>>>>>> driver.
>>>>>>>>>
>>>>>>>>> While the HBA only has a single mailbox register for submitting
>>>>>>>>> commands, it does have individual receive queues per MSI-X
>>>>>>>>> interrupt and as such does benefit from converting it to full
>>>>>>>>> multiqueue
>>>>>>
>>>>>> support.
>>>>>>>>
>>>>>>>>
>>>>>>>> Explanation and numbers on why this would be beneficial, please.
>>>>>>>> We should not need multiple submissions queues for a single register
>>>>>>>> to benefit from multiple completion queues.
>>>>>>>>
>>>>>>> Well, the actual throughput very strongly depends on the blk-mq-sched
>>>>>>> patches from Jens.
>>>>>>> As this is barely finished I didn't post any numbers yet.
>>>>>>>
>>>>>>> However:
>>>>>>> With multiqueue support:
>>>>>>> 4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=
>>>>>
>>>>> 60021msec
>>>>>>>
>>>>>>> With scsi-mq on 1 queue:
>>>>>>> 4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec
>>>>>>> So yes, there _is_ a benefit.
>>
>>
>> Hannes,
>>
>> I have created a md raid0 with 4 SAS SSD drives using below command,
>> #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh
>> /dev/sdi /dev/sdj
>>
>> And here is 'mdadm --detail /dev/md0' command output,
>> --
>> /dev/md0:
>> Version : 1.2
>>   Creation Time : Thu Feb  9 14:38:47 2017
>>  Raid Level : raid0
>>  Array Size : 780918784 (744.74 GiB 799.66 GB)
>>Raid Devices : 4
>>   Total Devices : 4
>> Persistence : Superblock is persistent
>>
>> Update Time : Thu Feb  9 14:38:47 2017
>>   State : clean
>>  Active Devices : 4
>> Working Devices : 4
>>  Failed Devices : 0
>>   Spare Devices : 0
>>
>>  Chunk Size : 512K
>>
>>Name : host_name
>>UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
>>  Events : 0
>>
>> Number   Major   Minor   RaidDevice State
>>0   8   960  active sync   /dev/sdg
>>1   8  1121  active sync   /dev/sdh
>>2

Re: [PATCH 00/10] mpt3sas: full mq support

[Sreekanth Reddy]

On Wed, Feb 1, 2017 at 1:13 PM, Hannes Reinecke <h...@suse.de> wrote:
>
> On 02/01/2017 08:07 AM, Kashyap Desai wrote:
>>>
>>> -Original Message-
>>> From: Hannes Reinecke [mailto:h...@suse.de]
>>> Sent: Wednesday, February 01, 2017 12:21 PM
>>> To: Kashyap Desai; Christoph Hellwig
>>> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org;
>>> Sathya
>>> Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
>>> Subject: Re: [PATCH 00/10] mpt3sas: full mq support
>>>
>>> On 01/31/2017 06:54 PM, Kashyap Desai wrote:
>>>>>
>>>>> -Original Message-
>>>>> From: Hannes Reinecke [mailto:h...@suse.de]
>>>>> Sent: Tuesday, January 31, 2017 4:47 PM
>>>>> To: Christoph Hellwig
>>>>> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org;
>>>>
>>>> Sathya
>>>>>
>>>>> Prakash; Kashyap Desai; mpt-fusionlinux@broadcom.com
>>>>> Subject: Re: [PATCH 00/10] mpt3sas: full mq support
>>>>>
>>>>> On 01/31/2017 11:02 AM, Christoph Hellwig wrote:
>>>>>>
>>>>>> On Tue, Jan 31, 2017 at 10:25:50AM +0100, Hannes Reinecke wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> this is a patchset to enable full multiqueue support for the
>>>>>>> mpt3sas
>>>>>
>>>>> driver.
>>>>>>>
>>>>>>> While the HBA only has a single mailbox register for submitting
>>>>>>> commands, it does have individual receive queues per MSI-X
>>>>>>> interrupt and as such does benefit from converting it to full
>>>>>>> multiqueue
>>>>
>>>> support.
>>>>>>
>>>>>>
>>>>>> Explanation and numbers on why this would be beneficial, please.
>>>>>> We should not need multiple submissions queues for a single register
>>>>>> to benefit from multiple completion queues.
>>>>>>
>>>>> Well, the actual throughput very strongly depends on the blk-mq-sched
>>>>> patches from Jens.
>>>>> As this is barely finished I didn't post any numbers yet.
>>>>>
>>>>> However:
>>>>> With multiqueue support:
>>>>> 4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=
>>>
>>> 60021msec
>>>>>
>>>>> With scsi-mq on 1 queue:
>>>>> 4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec
>>>>> So yes, there _is_ a benefit.


Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command,
#mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh
/dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--
/dev/md0:
Version : 1.2
  Creation Time : Thu Feb  9 14:38:47 2017
 Raid Level : raid0
 Array Size : 780918784 (744.74 GiB 799.66 GB)
   Raid Devices : 4
  Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb  9 14:38:47 2017
  State : clean
 Active Devices : 4
Working Devices : 4
 Failed Devices : 0
  Spare Devices : 0

 Chunk Size : 512K

   Name : host_name
   UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
 Events : 0

Number   Major   Minor   RaidDevice State
   0   8   960  active sync   /dev/sdg
   1   8  1121  active sync   /dev/sdh
   2   8  1442  active sync   /dev/sdj
   3   8  1283  active sync   /dev/sdi
--

Then I have used below fio profile to run 4K sequence read operations
with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my
system has two numa node and each with 12 cpus).
-
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-

Here are the fio results when nr_hw_queues=1 (i.e. single request
queue) with various number of job counts
1JOB 4k read  : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec
2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec
4JOBs 4k 

Re: [PATCH V2] mpt3sas: disable ASPM for MPI2 controllers

[Sreekanth Reddy]

On Wed, Dec 28, 2016 at 4:35 PM, ojab <o...@ojab.ru> wrote:
> MPI2 controllers sometimes got lost (i. e. disappears from
> /sys/bus/pci/devices) if ASMP is enabled.
>
> Signed-off-by: Slava Kardakov <o...@ojab.ru>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=60644

>From some of our system engineers team, I came to known that ASPM
needs to be disabled.
So this patch looks good. Please consider this patch as

Acked-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>

Thanks,
Sreekanth

> ---
> V2: use name in Signed-off-by
>
> Not sure if it's a complete fix, but at least I can't reproduce the issue
> locally with it applied.
>
> Also it's my first patch, so I've surely screwed up some formatting etc.
>
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index b5c966e..203651a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -51,6 +51,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -8734,6 +8735,8 @@ _scsih_probe(struct pci_dev *pdev, const struct 
> pci_device_id *id)
>
> switch (hba_mpi_version) {
> case MPI2_VERSION:
> +   pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S |
> +   PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_CLKPM);
> /* Use mpt2sas driver host template for SAS 2.0 HBA's */
> shost = scsi_host_alloc(_driver_template,
>   sizeof(struct MPT3SAS_ADAPTER));
> --
> 2.10.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] mpt3sas: disable ASPM for MPI2 controllers

[Sreekanth Reddy]

Hi Ojab,

Can I get output of 'lspci -vvv -s' for LSI SAS2 card?

On my Supermicro system, I have enabled ASPM in system BIOS but still
in lspci output I see this ASPM was disabled for my SAS2 HBA card
which has Phase19 firmware (i.e. 19.00.00.00) as shown below,

# lspci -vvv -s 83:00.0
83:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic
SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
Subsystem: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2
Physical Slot: 1-1
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR+ FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
SERR- 
Capabilities: [190 v1] #16
Capabilities: [148 v1] Alternative Routing-ID Interpretation (ARI)
ARICap:MFVC- ACS-, Next Function: 0
ARICtl:MFVC- ACS-, Function Group: 0
Kernel driver in use: mpt3sas
Kernel modules: mpt3sas

So, Can you also try once with Phase19 firmware.

Thanks,
Sreekanth

On Mon, Feb 6, 2017 at 2:22 PM, ojab  wrote:
> On 2017/01/30 10:58, ojab wrote:
>>
>> On 2017/01/23 15:59, ojab wrote:
>>>
>>> On 2016/12/28 11:05, ojab wrote:

 MPI2 controllers sometimes got lost (i. e. disappears from
 /sys/bus/pci/devices) if ASMP is enabled.

 Signed-off-by: Slava Kardakov 
 Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=60644
>>>
>>>
>>> Can I haz Reviewed-by?
>>>
>>
>> ping?
>>
>
> Humble ping.
>
> //wbr ojab

Re: [PATCH v2] mpt3sas: Force request partial completion alignment

[Sreekanth Reddy]

On Tue, Jan 24, 2017 at 9:17 PM, Guilherme G. Piccoli
<gpicc...@linux.vnet.ibm.com> wrote:
> From: Ram Pai <linux...@us.ibm.com>
>
> The firmware or device, possibly under a heavy I/O load, can return
> on a partial unaligned boundary. Scsi-ml expects these requests to be
> completed on an alignment boundary. Scsi-ml blindly requeues the I/O
> without checking the alignment boundary of the I/O request for the
> remaining bytes. This leads to errors, since devices cannot perform
> non-aligned read/write operations.
>
> This patch fixes the issue in the driver. It aligns unaligned
> completions of FS requests, by truncating them to the nearest
> alignment boundary.
>
> Reported-by: Mauricio Faria De Oliveira <mauri...@linux.vnet.ibm.com>
> Signed-off-by: Guilherme G. Piccoli <gpicc...@linux.vnet.ibm.com>
> Signed-off-by: Ram Pai <linux...@us.ibm.com>
> ---
> v1->v2:
>   * Improved printk, by showing some variables too [suggested by Sreekanth].

This patch looks good. Please consider this patch as

Acked-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>

>
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 17 +
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 75f3fce..e52c942 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -4657,6 +4657,8 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, 
> u8 msix_index, u32 reply)
> struct MPT3SAS_DEVICE *sas_device_priv_data;
> u32 response_code = 0;
> unsigned long flags;
> +   unsigned int sector_sz;
> +   struct request *req;
>
> mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
> scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
> @@ -4715,6 +4717,21 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, 
> u8 msix_index, u32 reply)
> }
>
> xfer_cnt = le32_to_cpu(mpi_reply->TransferCount);
> +
> +   /* In case of bogus fw or device, we could end up having
> +* unaligned partial completion. We can force alignment here,
> +* then scsi-ml does not need to handle this misbehavior.
> +*/
> +   sector_sz = scmd->device->sector_size;
> +   req = scmd->request;
> +   if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
> +   (xfer_cnt % sector_sz))) {
> +   sdev_printk(KERN_INFO, scmd->device,
> +   "unaligned partial completion avoided (xfer_cnt=%u, 
> sector_sz=%u)\n",
> +   xfer_cnt, sector_sz);
> +   xfer_cnt = (xfer_cnt / sector_sz) * sector_sz;
> +   }
> +
> scsi_set_resid(scmd, scsi_bufflen(scmd) - xfer_cnt);
> if (ioc_status & MPI2_IOCSTATUS_FLAG_LOG_INFO_AVAILABLE)
> log_info =  le32_to_cpu(mpi_reply->IOCLogInfo);
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mpt3sas: Force request partial completion alignment

[Sreekanth Reddy]

On Wed, Dec 28, 2016 at 6:21 PM, Guilherme G. Piccoli
 wrote:
> From: Ram Pai 
>
> The firmware or device, possibly under a heavy I/O load, can return
> on a partial unaligned boundary. Scsi-ml expects these requests to be
> completed on an alignment boundary. Scsi-ml blindly requeues the I/O
> without checking the alignment boundary of the I/O request for the
> remaining bytes. This leads to errors, since devices cannot perform
> non-aligned read/write operations.
>
> This patch fixes the issue in the driver. It aligns unaligned
> completions of FS requests, by truncating them to the nearest
> alignment boundary.
>
> Reported-by: Mauricio Faria De Oliveira 
> Signed-off-by: Guilherme G. Piccoli 
> Signed-off-by: Ram Pai 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index b5c966e..55332a3 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -4644,6 +4644,8 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, 
> u8 msix_index, u32 reply)
> struct MPT3SAS_DEVICE *sas_device_priv_data;
> u32 response_code = 0;
> unsigned long flags;
> +   unsigned int sector_sz;
> +   struct request *req;
>
> mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
> scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
> @@ -4703,6 +4705,20 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, 
> u8 msix_index, u32 reply)
> }
>
> xfer_cnt = le32_to_cpu(mpi_reply->TransferCount);
> +
> +   /* In case of bogus fw or device, we could end up having
> +* unaligned partial completion. We can force alignment here,
> +* then scsi-ml does not need to handle this misbehavior.
> +*/
> +   sector_sz = scmd->device->sector_size;
> +   req = scmd->request;
> +   if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
> +   (xfer_cnt % sector_sz))) {
> +   sdev_printk(KERN_INFO, scmd->device,
> +   "unaligned partial completion avoided\n");

[Sreekanth] Patch looks good. But can we print xfer_cnt & sector_sz
values along with above print.

Also if it is generic drive issue, then can we move this work around
to SCSI Mid Layer?

> +   xfer_cnt = (xfer_cnt / sector_sz) * sector_sz;
> +   }
> +
> scsi_set_resid(scmd, scsi_bufflen(scmd) - xfer_cnt);
> if (ioc_status & MPI2_IOCSTATUS_FLAG_LOG_INFO_AVAILABLE)
> log_info =  le32_to_cpu(mpi_reply->IOCLogInfo);
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] mpt3sas: Fix for Crusader to achieve product targets with SAS devices.

[Sreekanth Reddy]

On Fri, Jan 20, 2017 at 8:30 PM, Johannes Thumshirn  wrote:
> On Fri, Jan 20, 2017 at 08:12:11PM +0530, Chaitra P B wrote:
>> Small glitch/degraded performance in Crusader is improved with SAS
>> drives by removing unnecessary spinlocks while clearing scsi command
>> in drivers internal lookup table.
>>
>> Signed-off-by: Chaitra P B 
>> Signed-off-by: Suganath Prabu S 
>> ---
>
> Whats wrong with creating a __scsih_scsi_lookup_get_clear() and calling
> it with the scsi_lookup_lock held from _scsih_scsi_lookup_get_clear()
> instead of duplicating the code?
>
> It's quite common in the linux kernel to have two functions func and __func,
> where func is
>
> func()
> {
> spin_lock();
> __func();
> spin_unlock();
> }
>

Sure, We will resend this path with this change.

Thanks,
Sreekanth

> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] mpt3sas: Fix Firmware fault state 0x2100 during heavy 4K RR FIO stress test.

[Sreekanth Reddy]

On Fri, Jan 20, 2017 at 8:40 PM, Johannes Thumshirn  wrote:
> On Fri, Jan 20, 2017 at 08:12:12PM +0530, Chaitra P B wrote:
>> Due existence of loop in the IO path our HBA will receive heavy IOs and
>> also as driver is not updating the Reply Post Host Index frequently, So
>> there will be a high chance that our Firmware unable to find any free entry
>> in the Reply Post Descriptor Queue (i.e. Queue overflow occurs) and can
>> observe 0x2100 firmware fault.
>> So to fix this, we have defined a thresh hold value. After continuously
>> processing this thresh hold number of reply descriptors driver will update
>> the Reply Descriptor Host Index so that this thresh hold number of reply
>> descriptors entries will be freed and these entries will be available for
>> firmware and we won't observe this Firmware fault. We have defined this
>> threshold value as 1/3rd of the hba queue depth.
>>
>> Signed-off-by: Chaitra P B 
>> Signed-off-by: Suganath Prabu S 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.c |   19 +++
>>  1 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> index 722fab9..a3fe1fb 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> @@ -1040,6 +1040,25 @@ _base_interrupt(int irq, void *bus_id)
>>   reply_q->reply_post_free[reply_q->reply_post_host_index].
>>   Default.ReplyFlags & MPI2_RPY_DESCRIPT_FLAGS_TYPE_MASK;
>>   completed_cmds++;
>> + /* Update the reply post host index after continuously
>> +  * processing the threshold number of Reply Descriptors.
>> +  * So that FW can find enough entries to post the Reply
>> +  * Descriptors in the reply descriptor post queue.
>> +  */
>> + if (completed_cmds > ioc->hba_queue_depth/3) {
>> + if (ioc->combined_reply_queue) {
>> + writel(reply_q->reply_post_host_index |
>> + ((msix_index  & 7) <<
>> +  MPI2_RPHI_MSIX_INDEX_SHIFT),
>> + ioc->replyPostRegisterIndex[msix_index/8]);
>> + } else {
>> + writel(reply_q->reply_post_host_index |
>> + (msix_index <<
>> +  MPI2_RPHI_MSIX_INDEX_SHIFT),
>> + 
>> >chip->ReplyPostHostIndex);
>> + }
>> + completed_cmds = 1;
>> + }
>>   if (request_desript_type == MPI2_RPY_DESCRIPT_FLAGS_UNUSED)
>>   goto out;
>>   if (!reply_q->reply_post_host_index)
>
> Do I understand it correctly that you fill the HBA's internal queue up to a
> 3rd and then kick it to start processing?

No, driver will continuously process the reply descriptors from Reply
Descriptor Post Queue (RDPQ) but will update it's Host Index (tail
index) with the firmware after continuously processing 1/3rd of the
HBA queue depth number of descriptors instead of updating it's host
index only at after it see unused descriptor entry. So that firmware
can always get enough free descriptors entries to post reply
descriptors and won't see any 0x2100 fault which will occur if
firmware doesn't find any free descriptor entry in the  RDPQ queue.

Thanks,
Sreekanth

>
> Thanks,
> Johannes
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] mpt3sas: disable ASPM for MPI2 controllers

[Sreekanth Reddy]

On Tue, Jan 17, 2017 at 4:13 AM, ojab <o...@ojab.ru> wrote:
> On 2017/01/16 14:03, ojab wrote:
>>
>> On 2017/01/16 13:31, Sreekanth Reddy wrote:
>>>
>>> On Mon, Jan 16, 2017 at 6:15 PM, ojab <o...@ojab.ru> wrote:
>>>>
>>>> On 2017/01/16 12:36, Sreekanth Reddy wrote:
>>>>>
>>>>>
>>>>> Ojab,
>>>>>
>>>>> I am checking internally with our FW team and Architect to known
>>>>> whether they aware/observed any issues similar to this (since this
>>>>> issue is nothing to do with driver).
>>>>>
>>>>> Meanwhile is it possible to provide us the PCI config space and FW
>>>>> logs when issue occurs?
>>>>
>>>>
>>>>
>>>> AFAIU FW log is enabled when `logging_level=0x3f8` is passed to the
>>>> module,
>>>> right?
>>>
>>>
>>> This is a driver logging_level. Firmware logs can be collected using
>>> UART logs.
>>
>>
>> According to the user guide (from
>>
>> https://www.broadcom.com/products/storage/host-bus-adapters/sas-9217-8i#documentation)
>> my 9217-8i requires 1.8V UART which I don't have at hand, I'll try to do
>> something on the weekend.
>> Is anything needed except connecting UART to gather required info?
>
> …actually I have compatible USB UART here. What should be done to get FW
> log? I've tried to reboot with UART connected (9600N1) but got no output.

You have to use, 115200 instead of 9600 baud rate.

Thanks,
Sreekanth

>
> //wbr ojab
>
>
>>
>> Anyway, right now various MPI2 HBAs doesn't work with ASPM enabled, so
>> can this patch be applied in the meantime while we're debugging the
>> issue further? Even if this issue can be properly fixed by FW update,
>> this workaround will be needed for HBAs with affected FW versions.
>>
>> //wbr ojab
>>
>>
>>>
>>>> Where can I find info about PCI config space?
>>>
>>>
>>> You can get PCI config space info from lspci command.
>>>
>>>>
>>>> //wbr ojab
>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Sreekanth
>>>>>
>>>>> On Mon, Jan 16, 2017 at 5:37 PM, ojab <o...@ojab.ru> wrote:
>>>>>>
>>>>>>
>>>>>> On 2017/01/06 15:48, Sreekanth Reddy wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Jan 6, 2017 at 7:24 AM, Martin K. Petersen
>>>>>>> <martin.peter...@oracle.com> wrote:
>>>>>>>
>>>>>>> Matin, We need some time to review this patch. I will provide my
>>>>>>> review comments by end of next week.
>>>>>>>
>>>>>> ping?
>>>>>>
>>>>>> //wbr ojab
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sreekanth
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> linux-scsi" in
>>>>> the body of a message to majord...@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands

[Sreekanth Reddy]

mmand(scmd);
>
> -   /*
> -* Lock the device for any subsequent command until command is
> -* done.
> -*/
> -   if (ata_12_16_cmd(scmd))
> -   scsi_internal_device_block(scmd->device);
> -
> sas_device_priv_data = scmd->device->hostdata;
> if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
> scmd->result = DID_NO_CONNECT << 16;
> @@ -4083,6 +4083,19 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd 
> *scmd)
> return 0;
> }
>
> +   /*
> +* Bug work around for firmware SATL handling.  The loop
> +* is based on atomic operations and ensures consistency
> +* since we're lockless at this point
> +*/
> +   do {
> +   if (sas_device_priv_data->ata_command_pending) {
> +   scmd->result = SAM_STAT_BUSY;
> +   scmd->scsi_done(scmd);
> +   return 0;
> +   }
> +   } while (_scsih_set_satl_pending(scmd, true));
> +

[Sreekanth] Just for readability purpose, can use use "if (test_bit(0,
_device_priv_data->ata_command_pending)"
 instead of "if (sas_device_priv_data->ata_command_pending)".
Since while setting & clearing the bit we are using atomic bit
operations. I don't see any issue functionality wise.

> sas_target_priv_data = sas_device_priv_data->sas_target;
>
> /* invalid device handle */
> @@ -4650,8 +4663,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, 
> u8 msix_index, u32 reply)
> if (scmd == NULL)
> return 1;
>
> -   if (ata_12_16_cmd(scmd))
> -   scsi_internal_device_unblock(scmd->device, SDEV_RUNNING);
> +   _scsih_set_satl_pending(scmd, false);
>
> mpi_request = mpt3sas_base_get_msg_frame(ioc, smid);
>
> --
> 2.6.6
>

Tested this patch. It is working fine. Please consider this patch as

Acked-by: Sreekanth Reddy <sreekanth.re...@broadcom.com>

Thanks,
Sreekanth
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] mpt3sas: disable ASPM for MPI2 controllers

[Sreekanth Reddy]

On Mon, Jan 16, 2017 at 6:15 PM, ojab <o...@ojab.ru> wrote:
> On 2017/01/16 12:36, Sreekanth Reddy wrote:
>>
>> Ojab,
>>
>> I am checking internally with our FW team and Architect to known
>> whether they aware/observed any issues similar to this (since this
>> issue is nothing to do with driver).
>>
>> Meanwhile is it possible to provide us the PCI config space and FW
>> logs when issue occurs?
>
>
> AFAIU FW log is enabled when `logging_level=0x3f8` is passed to the module,
> right?

This is a driver logging_level. Firmware logs can be collected using UART logs.

> Where can I find info about PCI config space?

You can get PCI config space info from lspci command.

>
> //wbr ojab
>>
>>
>> Thanks,
>> Sreekanth
>>
>> On Mon, Jan 16, 2017 at 5:37 PM, ojab <o...@ojab.ru> wrote:
>>>
>>> On 2017/01/06 15:48, Sreekanth Reddy wrote:
>>>>
>>>>
>>>> On Fri, Jan 6, 2017 at 7:24 AM, Martin K. Petersen
>>>> <martin.peter...@oracle.com> wrote:
>>>>
>>>> Matin, We need some time to review this patch. I will provide my
>>>> review comments by end of next week.
>>>>
>>> ping?
>>>
>>> //wbr ojab
>>>>
>>>>
>>>> Thanks,
>>>> Sreekanth
>>>
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] mpt3sas: disable ASPM for MPI2 controllers

[Sreekanth Reddy]

Ojab,

I am checking internally with our FW team and Architect to known
whether they aware/observed any issues similar to this (since this
issue is nothing to do with driver).

Meanwhile is it possible to provide us the PCI config space and FW
logs when issue occurs?

Thanks,
Sreekanth

On Mon, Jan 16, 2017 at 5:37 PM, ojab <o...@ojab.ru> wrote:
> On 2017/01/06 15:48, Sreekanth Reddy wrote:
>>
>> On Fri, Jan 6, 2017 at 7:24 AM, Martin K. Petersen
>> <martin.peter...@oracle.com> wrote:
>>
>> Matin, We need some time to review this patch. I will provide my
>> review comments by end of next week.
>>
> ping?
>
> //wbr ojab
>>
>> Thanks,
>> Sreekanth
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] mpt3sas: switch to pci_alloc_irq_vectors

[Sreekanth Reddy]

On Thu, Jan 12, 2017 at 7:50 PM, Hannes Reinecke  wrote:
> Cleanup the MSI-X handling allowing us to use
> the PCI-layer provided vector allocation.
>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 98 
> +
>  drivers/scsi/mpt3sas/mpt3sas_base.h |  2 -
>  2 files changed, 45 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index f00ef88..f2914f4 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -1129,7 +1129,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> /* TMs are on msix_index == 0 */
> if (reply_q->msix_index == 0)
> continue;
> -   synchronize_irq(reply_q->vector);
> +   synchronize_irq(pci_irq_vector(ioc->pdev, 
> reply_q->msix_index));
> }
>  }
>
> @@ -1818,11 +1818,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>
> list_for_each_entry_safe(reply_q, next, >reply_queue_list, list) 
> {
> list_del(_q->list);
> -   if (smp_affinity_enable) {
> -   irq_set_affinity_hint(reply_q->vector, NULL);
> -   free_cpumask_var(reply_q->affinity_hint);
> -   }
> -   free_irq(reply_q->vector, reply_q);
> +   free_irq(pci_irq_vector(ioc->pdev, reply_q->msix_index),
> +reply_q);
> kfree(reply_q);
> }
>  }
> @@ -1831,13 +1828,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>   * _base_request_irq - request irq
>   * @ioc: per adapter object
>   * @index: msix index into vector table
> - * @vector: irq vector
>   *
>   * Inserting respective reply_queue into the list.
>   */
>  static int
> -_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index, u32 vector)
> +_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index)
>  {
> +   struct pci_dev *pdev = ioc->pdev;
> struct adapter_reply_queue *reply_q;
> int r;
>
> @@ -1849,14 +1846,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> }
> reply_q->ioc = ioc;
> reply_q->msix_index = index;
> -   reply_q->vector = vector;
> -
> -   if (smp_affinity_enable) {
> -   if (!zalloc_cpumask_var(_q->affinity_hint, GFP_KERNEL)) 
> {
> -   kfree(reply_q);
> -   return -ENOMEM;
> -   }
> -   }
>
> atomic_set(_q->busy, 0);
> if (ioc->msix_enable)
> @@ -1865,12 +1854,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> else
> snprintf(reply_q->name, MPT_NAME_LENGTH, "%s%d",
> ioc->driver_name, ioc->id);
> -   r = request_irq(vector, _base_interrupt, IRQF_SHARED, reply_q->name,
> -   reply_q);
> +   r = request_irq(pci_irq_vector(pdev, index), _base_interrupt,
> +   IRQF_SHARED, reply_q->name, reply_q);
> if (r) {
> pr_err(MPT3SAS_FMT "unable to allocate interrupt %d!\n",
> -   reply_q->name, vector);
> -   free_cpumask_var(reply_q->affinity_hint);
> +  reply_q->name, pci_irq_vector(pdev, index));
> kfree(reply_q);
> return -EBUSY;
> }
> @@ -1906,6 +1894,21 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> if (!nr_msix)
> return;
>
> +   if (smp_affinity_enable) {
> +   list_for_each_entry(reply_q, >reply_queue_list, list) {
> +   const cpumask_t *mask = 
> pci_irq_get_affinity(ioc->pdev,
> +   reply_q->msix_index);
> +   if (!mask) {
> +   pr_warn(MPT3SAS_FMT "no affinity for msi 
> %x\n",
> +   ioc->name, reply_q->msix_index);
> +   continue;
> +   }
> +
> +   for_each_cpu(cpu, mask)
> +   ioc->cpu_msix_table[cpu] = 
> reply_q->msix_index;
> +   }
> +   return;
> +   }
> cpu = cpumask_first(cpu_online_mask);
>
> list_for_each_entry(reply_q, >reply_queue_list, list) {
> @@ -1920,17 +1923,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>
> for (i = 0 ; i < group ; i++) {
> ioc->cpu_msix_table[cpu] = index;
> -   if (smp_affinity_enable)
> -   cpumask_or(reply_q->affinity_hint,
> -  reply_q->affinity_hint, get_cpu_mask(cpu));
> cpu = cpumask_next(cpu, cpu_online_mask);
> }
> -   if (smp_affinity_enable)
> -   if 

Re: [PATCHv2] mpt3sas: switch to pci_alloc_irq_vectors

[Sreekanth Reddy]

On Tue, Dec 20, 2016 at 6:57 PM, Hannes Reinecke  wrote:
> Cleanup the MSI-X handling allowing us to use
> the PCI-layer provided vector allocation.
>
> Signed-off-by: Hannes Reinecke 
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index a1a5ceb..75149f0 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -1129,7 +1129,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> /* TMs are on msix_index == 0 */
> if (reply_q->msix_index == 0)
> continue;
> -   synchronize_irq(reply_q->vector);
> +   synchronize_irq(pci_irq_vector(ioc->pdev, 
> reply_q->msix_index));
> }
>  }
>
> @@ -1818,11 +1818,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>
> list_for_each_entry_safe(reply_q, next, >reply_queue_list, list) 
> {
> list_del(_q->list);
> -   if (smp_affinity_enable) {
> -   irq_set_affinity_hint(reply_q->vector, NULL);
> -   free_cpumask_var(reply_q->affinity_hint);
> -   }
> -   free_irq(reply_q->vector, reply_q);
> +   free_irq(pci_irq_vector(ioc->pdev, reply_q->msix_index),
> +reply_q);
> kfree(reply_q);
> }
>  }
> @@ -1831,13 +1828,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>   * _base_request_irq - request irq
>   * @ioc: per adapter object
>   * @index: msix index into vector table
> - * @vector: irq vector
>   *
>   * Inserting respective reply_queue into the list.
>   */
>  static int
> -_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index, u32 vector)
> +_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index)
>  {
> +   struct pci_dev *pdev = ioc->pdev;
> struct adapter_reply_queue *reply_q;
> int r;
>
> @@ -1849,14 +1846,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> }
> reply_q->ioc = ioc;
> reply_q->msix_index = index;
> -   reply_q->vector = vector;
> -
> -   if (smp_affinity_enable) {
> -   if (!zalloc_cpumask_var(_q->affinity_hint, GFP_KERNEL)) 
> {
> -   kfree(reply_q);
> -   return -ENOMEM;
> -   }
> -   }
>
> atomic_set(_q->busy, 0);
> if (ioc->msix_enable)
> @@ -1865,12 +1854,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
> else
> snprintf(reply_q->name, MPT_NAME_LENGTH, "%s%d",
> ioc->driver_name, ioc->id);
> -   r = request_irq(vector, _base_interrupt, IRQF_SHARED, reply_q->name,
> -   reply_q);
> +   r = request_irq(pci_irq_vector(pdev, index), _base_interrupt,
> +   IRQF_SHARED, reply_q->name, reply_q);
> if (r) {
> pr_err(MPT3SAS_FMT "unable to allocate interrupt %d!\n",
> -   reply_q->name, vector);
> -   free_cpumask_var(reply_q->affinity_hint);
> +  reply_q->name, pci_irq_vector(pdev, index));
> kfree(reply_q);
> return -EBUSY;
> }
> @@ -1892,7 +1880,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>  static void
>  _base_assign_reply_queues(struct MPT3SAS_ADAPTER *ioc)
>  {
> -   unsigned int cpu, nr_cpus, nr_msix, index = 0;
> +   unsigned int cpu, nr_msix;
> struct adapter_reply_queue *reply_q;
>
> if (!_base_is_controller_msix_enabled(ioc))
> @@ -1900,38 +1888,21 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>
> memset(ioc->cpu_msix_table, 0, ioc->cpu_msix_table_sz);
>
> -   nr_cpus = num_online_cpus();
> nr_msix = ioc->reply_queue_count = min(ioc->reply_queue_count,
>ioc->facts.MaxMSIxVectors);
> if (!nr_msix)
> return;
> -
> -   cpu = cpumask_first(cpu_online_mask);
> -
> list_for_each_entry(reply_q, >reply_queue_list, list) {
> -
> -   unsigned int i, group = nr_cpus / nr_msix;
> -
> -   if (cpu >= nr_cpus)
> -   break;
> -
> -   if (index < nr_cpus % nr_msix)
> -   group++;
> -
> -   for (i = 0 ; i < group ; i++) {
> -   ioc->cpu_msix_table[cpu] = index;
> -   if (smp_affinity_enable)
> -   cpumask_or(reply_q->affinity_hint,
> -  reply_q->affinity_hint, get_cpu_mask(cpu));
> -   cpu = cpumask_next(cpu, cpu_online_mask);
> +   const cpumask_t *mask = pci_irq_get_affinity(ioc->pdev,
> + reply_q->msix_index);
> +   if (!mask) {
> +   pr_warn(MPT3SAS_FMT "no affinity for msi %x\n",
> +