[PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn

[Joe Perches]

There are ~4300 uses of pr_warn and ~250 uses of the older
pr_warning in the kernel source tree.

Make the use of pr_warn consistent across all kernel files.

This excludes all files in tools/ as there is a separate
define pr_warning for that directory tree and pr_warn is
not used in tools/.

Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing.

Miscellanea:

o Coalesce formats and realign arguments

Some files not compiled - no cross-compilers

Joe Perches (35):
  alpha: Convert remaining uses of pr_warning to pr_warn
  ARM: ep93xx: Convert remaining uses of pr_warning to pr_warn
  arm64: Convert remaining uses of pr_warning to pr_warn
  arch/blackfin: Convert remaining uses of pr_warning to pr_warn
  ia64: Convert remaining use of pr_warning to pr_warn
  powerpc: Convert remaining uses of pr_warning to pr_warn
  sh: Convert remaining uses of pr_warning to pr_warn
  sparc: Convert remaining use of pr_warning to pr_warn
  x86: Convert remaining uses of pr_warning to pr_warn
  drivers/acpi: Convert remaining uses of pr_warning to pr_warn
  block/drbd: Convert remaining uses of pr_warning to pr_warn
  gdrom: Convert remaining uses of pr_warning to pr_warn
  drivers/char: Convert remaining use of pr_warning to pr_warn
  clocksource: Convert remaining use of pr_warning to pr_warn
  drivers/crypto: Convert remaining uses of pr_warning to pr_warn
  fmc: Convert remaining use of pr_warning to pr_warn
  drivers/gpu: Convert remaining uses of pr_warning to pr_warn
  drivers/ide: Convert remaining uses of pr_warning to pr_warn
  drivers/input: Convert remaining uses of pr_warning to pr_warn
  drivers/isdn: Convert remaining uses of pr_warning to pr_warn
  drivers/macintosh: Convert remaining uses of pr_warning to pr_warn
  drivers/media: Convert remaining use of pr_warning to pr_warn
  drivers/mfd: Convert remaining uses of pr_warning to pr_warn
  drivers/mtd: Convert remaining uses of pr_warning to pr_warn
  drivers/of: Convert remaining uses of pr_warning to pr_warn
  drivers/oprofile: Convert remaining uses of pr_warning to pr_warn
  drivers/platform: Convert remaining uses of pr_warning to pr_warn
  drivers/rapidio: Convert remaining use of pr_warning to pr_warn
  drivers/scsi: Convert remaining use of pr_warning to pr_warn
  drivers/sh: Convert remaining use of pr_warning to pr_warn
  drivers/tty: Convert remaining uses of pr_warning to pr_warn
  drivers/video: Convert remaining uses of pr_warning to pr_warn
  kernel/trace: Convert remaining uses of pr_warning to pr_warn
  lib: Convert remaining uses of pr_warning to pr_warn
  sound/soc: Convert remaining uses of pr_warning to pr_warn

 arch/alpha/kernel/perf_event.c |  4 +-
 arch/arm/mach-ep93xx/core.c|  4 +-
 arch/arm64/include/asm/syscall.h   |  8 ++--
 arch/arm64/kernel/hw_breakpoint.c  |  8 ++--
 arch/arm64/kernel/smp.c|  4 +-
 arch/blackfin/kernel/nmi.c |  2 +-
 arch/blackfin/kernel/ptrace.c  |  2 +-
 arch/blackfin/mach-bf533/boards/stamp.c|  2 +-
 arch/blackfin/mach-bf537/boards/cm_bf537e.c|  2 +-
 arch/blackfin/mach-bf537/boards/cm_bf537u.c|  2 +-
 arch/blackfin/mach-bf537/boards/stamp.c|  2 +-
 arch/blackfin/mach-bf537/boards/tcm_bf537.c|  2 +-
 arch/blackfin/mach-bf561/boards/cm_bf561.c |  2 +-
 arch/blackfin/mach-bf561/boards/ezkit.c|  2 +-
 arch/blackfin/mm/isram-driver.c|  4 +-
 arch/ia64/kernel/setup.c   |  6 +--
 arch/powerpc/kernel/pci-common.c   |  4 +-
 arch/powerpc/mm/init_64.c  |  5 +--
 arch/powerpc/mm/mem.c  |  3 +-
 arch/powerpc/platforms/512x/mpc512x_shared.c   |  4 +-
 arch/powerpc/platforms/85xx/socrates_fpga_pic.c|  7 ++--
 arch/powerpc/platforms/86xx/mpc86xx_hpcn.c |  2 +-
 arch/powerpc/platforms/pasemi/dma_lib.c|  4 +-
 arch/powerpc/platforms/powernv/opal.c  |  8 ++--
 arch/powerpc/platforms/powernv/pci-ioda.c  | 10 ++---
 arch/powerpc/platforms/ps3/device-init.c   | 14 +++
 arch/powerpc/platforms/ps3/mm.c|  4 +-
 arch/powerpc/platforms/ps3/os-area.c   |  2 +-
 arch/powerpc/platforms/pseries/iommu.c |  8 ++--
 arch/powerpc/platforms/pseries/setup.c |  4 +-
 arch/powerpc/sysdev/fsl_pci.c  |  9 ++---
 arch/powerpc/sysdev/mpic.c | 10 ++---
 arch/powerpc/sysdev/xics/icp-native.c  | 10 ++---
 arch/powerpc/sysdev/xics/ics-opal.c|  4 +-
 arch/powerpc/sysdev/xics/ics-rtas.c|  4 +-
 arch/powerpc/sysdev/xics/xics-common.c |  8 ++--
 arch/sh/boards/mach-sdk7786/nmi.c  |  2 +-
 arch/sh/drivers/pci/fixups-sdk7786.c   |  2 +-
 arch/sh/kernel/io_trapped.c

Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Hannes Reinecke]

On 02/16/2017 05:09 PM, Bart Van Assche wrote:
> On Thu, 2017-02-16 at 16:12 +0100, Hannes Reinecke wrote:
>> +struct scsi_device *scsi_device_from_queue(struct request_queue *q)
>> +{
>> +struct scsi_device *sdev = NULL;
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(q->queue_lock, flags);
>> +if (q->mq_ops) {
>> +if (q->mq_ops == _mq_ops)
>> +sdev = q->queuedata;
>> +} else if (q->request_fn == scsi_request_fn)
>> +sdev = q->queuedata;
>> +if (!sdev || !get_device(>sdev_gendev))
>> +sdev = NULL;
>> +spin_unlock_irqrestore(q->queue_lock, flags);
>> +
>> +return sdev;
>> +}
> 
> Hello Hannes,
> 
> Do we need to take the queue lock? Neither q->mq_ops nor q->request_fn are
> modified after a block device has been created. q->queuedata is not modified
> by any SCSI driver after it has been set. And since the caller of
> scsi_device_from_queue() has to guarantee that the queue does not disappear
> while this function is in progress, the queue lock does not have to be held
> around the get_device() call either. Otherwise this patch looks fine to me.
> 
Well, we did for the original code path, so I assumed that it'd be
prudent to do so.
And it's hardly time-critical anyway.
But yes, I guess we could do without the lock.

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)

[PATCH 29/35] drivers/scsi: Convert remaining use of pr_warning to pr_warn

[Joe Perches]

To enable eventual removal of pr_warning

This makes pr_warn use consistent for drivers/scsi

Prior to this patch, there was 1 use of pr_warning and
96 uses of pr_warn in drivers/scsi

Signed-off-by: Joe Perches 
---
 drivers/scsi/a3000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/a3000.c b/drivers/scsi/a3000.c
index e6375b4de79e..2be0f577abbf 100644
--- a/drivers/scsi/a3000.c
+++ b/drivers/scsi/a3000.c
@@ -38,7 +38,7 @@ static irqreturn_t a3000_intr(int irq, void *data)
spin_unlock_irqrestore(instance->host_lock, flags);
return IRQ_HANDLED;
}
-   pr_warning("Non-serviced A3000 SCSI-interrupt? ISTR = %02x\n", status);
+   pr_warn("Non-serviced A3000 SCSI-interrupt? ISTR = %02x\n", status);
return IRQ_NONE;
 }
 
-- 
2.10.0.rc2.1.g053435c

[PATCH] cxlflash: Enable PCI device ID for future IBM CXL Flash AFU

[Uma Krishnan]

From: "Matthew R. Ochs" 

Add support for a future IBM Coherent Accelerator (CXL) flash
AFU with an ID of 0x0624.

Signed-off-by: Matthew R. Ochs 
Signed-off-by: Uma Krishnan 
---
 drivers/scsi/cxlflash/main.c | 4 
 drivers/scsi/cxlflash/main.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 7069639..3061d80 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -2259,6 +2259,8 @@ static struct dev_dependent_vals dev_corsa_vals = { 
CXLFLASH_MAX_SECTORS,
0ULL };
 static struct dev_dependent_vals dev_flash_gt_vals = { CXLFLASH_MAX_SECTORS,
CXLFLASH_NOTIFY_SHUTDOWN };
+static struct dev_dependent_vals dev_briard_vals = { CXLFLASH_MAX_SECTORS,
+   CXLFLASH_NOTIFY_SHUTDOWN };
 
 /*
  * PCI device binding table
@@ -2268,6 +2270,8 @@ static struct pci_device_id cxlflash_pci_table[] = {
 PCI_ANY_ID, PCI_ANY_ID, 0, 0, (kernel_ulong_t)_corsa_vals},
{PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_FLASH_GT,
 PCI_ANY_ID, PCI_ANY_ID, 0, 0, (kernel_ulong_t)_flash_gt_vals},
+   {PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_BRIARD,
+PCI_ANY_ID, PCI_ANY_ID, 0, 0, (kernel_ulong_t)_briard_vals},
{}
 };
 
diff --git a/drivers/scsi/cxlflash/main.h b/drivers/scsi/cxlflash/main.h
index e43545c..0be2261 100644
--- a/drivers/scsi/cxlflash/main.h
+++ b/drivers/scsi/cxlflash/main.h
@@ -25,6 +25,7 @@
 
 #define PCI_DEVICE_ID_IBM_CORSA0x04F0
 #define PCI_DEVICE_ID_IBM_FLASH_GT 0x0600
+#define PCI_DEVICE_ID_IBM_BRIARD   0x0624
 
 /* Since there is only one target, make it 0 */
 #define CXLFLASH_TARGET0
-- 
2.1.0

[PATCH V2 12/15] aacraid: Reorder Adapter status check

[Raghava Aditya Renukunta]

The driver currently checks the SELF_TEST_FAILED first and then
KERNEL_PANIC next. Under error conditions(boot code failure) both
SELF_TEST_FAILED and KERNEL_PANIC can be set at the same time.

The driver has the capability to reset the controller on an KERNEL_PANIC,
but not on SELF_TEST_FAILED.

Fixed by first checking KERNEL_PANIC and then the others.

Cc: sta...@vger.kernel.org
Fixes: e8b12f0fb835223752 ([SCSI] aacraid: Add new code for PMC-Sierra's SRC 
base controller family)
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
Corrected stray , in commit message

 drivers/scsi/aacraid/src.c | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index 9b11e1a..71aaabd 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -437,16 +437,23 @@ static int aac_src_check_health(struct aac_dev *dev)
u32 status = src_readl(dev, MUnit.OMR);
 
/*
+*  Check to see if the board panic'd.
+*/
+   if (unlikely(status & KERNEL_PANIC))
+   goto err_blink;
+
+   /*
 *  Check to see if the board failed any self tests.
 */
if (unlikely(status & SELF_TEST_FAILED))
-   return -1;
+   goto err_out;
 
/*
-*  Check to see if the board panic'd.
+*  Check to see if the board failed any self tests.
 */
-   if (unlikely(status & KERNEL_PANIC))
-   return (status >> 16) & 0xFF;
+   if (unlikely(status & MONITOR_PANIC))
+   goto err_out;
+
/*
 *  Wait for the adapter to be up and running.
 */
@@ -456,6 +463,12 @@ static int aac_src_check_health(struct aac_dev *dev)
 *  Everything is OK
 */
return 0;
+
+err_out:
+   return -1;
+
+err_blink:
+   return (status > 16) & 0xFF;
 }
 
 static inline u32 aac_get_vector(struct aac_dev *dev)
-- 
2.7.4

[PATCH V2 10/15] aacraid: Decrease adapter health check interval

[Raghava Aditya Renukunta]

Currently driver checks the health status of the adapter once every 24
hours. When that happens the driver becomes dependent on the kernel to
figure out if the  adapter is misbehaving. This might take some time
(when the adapter is idle). The driver currently has support to
restart/recover the controller when it fails, and decreasing the time
interval will help.

Fixed by decreasing check interval from 24 hours to 1 minute

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/aachba.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 98d4ffd..3ede50f 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -311,7 +311,7 @@ module_param(update_interval, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(update_interval, "Interval in seconds between time sync"
" updates issued to adapter.");
 
-int check_interval = 24 * 60 * 60;
+int check_interval = 60;
 module_param(check_interval, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(check_interval, "Interval in seconds between adapter health"
" checks.");
-- 
2.7.4

[PATCH V2 15/15] aacraid: Update driver version

[Raghava Aditya Renukunta]

Updated driver version to 50792

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/aacraid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 622fd69..d036a80 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -97,7 +97,7 @@ enum {
 #definePMC_GLOBAL_INT_BIT0 0x0001
 
 #ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 50740
+# define AAC_DRIVER_BUILD 50792
 # define AAC_DRIVER_BRANCH "-custom"
 #endif
 #define MAXIMUM_NUM_CONTAINERS 32
-- 
2.7.4

[PATCH V2 13/15] aacraid: Save adapter fib log before an IOP reset

[Raghava Aditya Renukunta]

Currently  the adapter firmware does not save outstanding I/O's log
information  when an IOP reset is triggered. This is problematic when
trying to root cause and debug issues.

Fixed by adding sync command to trigger I/O log file save in the adapter
firmware before issuing an IOP reset.

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/aachba.c  |  4 
 drivers/scsi/aacraid/aacraid.h |  6 ++
 drivers/scsi/aacraid/src.c | 17 +
 3 files changed, 27 insertions(+)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 3ede50f..e3e93de 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -294,6 +294,10 @@ MODULE_PARM_DESC(aif_timeout, "The duration of time in 
seconds to wait for"
"deregistering them. This is typically adjusted for heavily burdened"
" systems.");
 
+int aac_fib_dump;
+module_param(aac_fib_dump, int, 0644);
+MODULE_PARM_DESC(aac_fib_dump, "Dump controller fibs prior to IOP_RESET 0=off, 
1=on");
+
 int numacb = -1;
 module_param(numacb, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(numacb, "Request a limit to the number of adapter control"
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 9281e72..622fd69 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1444,6 +1444,10 @@ struct aac_supplement_adapter_info
 #define AAC_OPTION_VARIABLE_BLOCK_SIZE cpu_to_le32(0x0004)
 /* 240 simple volume support */
 #define AAC_OPTION_SUPPORTED_240_VOLUMES cpu_to_le32(0x1000)
+/*
+ * Supports FIB dump sync command send prior to IOP_RESET
+ */
+#define AAC_OPTION_SUPPORTED3_IOP_RESET_FIB_DUMP   cpu_to_le32(0x4000)
 #define AAC_SIS_VERSION_V3 3
 #define AAC_SIS_SLOT_UNKNOWN   0xFF
 
@@ -2483,6 +2487,7 @@ struct aac_hba_info {
 #define GET_DRIVER_BUFFER_PROPERTIES   0x0023
 #define RCV_TEMP_READINGS  0x0025
 #define GET_COMM_PREFERRED_SETTINGS0x0026
+#define IOP_RESET_FW_FIB_DUMP  0x0034
 #define IOP_RESET  0x1000
 #define IOP_RESET_ALWAYS   0x1001
 #define RE_INIT_ADAPTER0x00ee
@@ -2686,4 +2691,5 @@ extern int aac_commit;
 extern int update_interval;
 extern int check_interval;
 extern int aac_check_reset;
+extern int aac_fib_dump;
 #endif
diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index 71aaabd..2e5338d 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -679,10 +679,27 @@ void aac_set_intx_mode(struct aac_dev *dev)
}
 }
 
+static void aac_dump_fw_fib_iop_reset(struct aac_dev *dev)
+{
+   __le32 supported_options3;
+
+   if (!aac_fib_dump)
+   return;
+
+   supported_options3  = dev->supplement_adapter_info.supported_options3;
+   if (!(supported_options3 & AAC_OPTION_SUPPORTED3_IOP_RESET_FIB_DUMP))
+   return;
+
+   aac_adapter_sync_cmd(dev, IOP_RESET_FW_FIB_DUMP,
+   0, 0, 0,  0, 0, 0, NULL, NULL, NULL, NULL, NULL);
+}
+
 static void aac_send_iop_reset(struct aac_dev *dev, int bled)
 {
u32 var, reset_mask;
 
+   aac_dump_fw_fib_iop_reset(dev);
+
bled = aac_adapter_sync_cmd(dev, IOP_RESET_ALWAYS,
0, 0, 0, 0, 0, 0, ,
_mask, NULL, NULL, NULL);
-- 
2.7.4

[PATCH V2 09/15] aacraid: Reload offlined drives after controller reset

[Raghava Aditya Renukunta]

During the IOP reset stress testing, it was found that the drives can be
marked offline when the adapter controller crashes and IO's are running
in parallel. When the controller  does come back from the reset, the drive
that is marked offline is not exposed.

Fixed by removing and adding drives that are marked offline. In addition
invoke a scsi host bus rescan to capture any additional configuration
changes.

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/commsup.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index eb4d8cf..1f716c0 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1637,11 +1637,29 @@ static int _aac_reset_adapter(struct aac_dev *aac, int 
forced, u8 reset_type)
command->SCp.phase = AAC_OWNER_ERROR_HANDLER;
command->scsi_done(command);
}
+   /*
+* Any Device that was already marked offline needs to be cleaned up
+*/
+   __shost_for_each_device(dev, host) {
+   if (!scsi_device_online(dev)) {
+   sdev_printk(KERN_INFO, dev, "Removing offline 
device\n");
+   scsi_remove_device(dev);
+   scsi_device_put(dev);
+   }
+   }
retval = 0;
 
 out:
aac->in_reset = 0;
scsi_unblock_requests(host);
+   /*
+* Issue bus rescan to catch any configuration that might have
+* occurred
+*/
+   if (!retval) {
+   dev_info(>pdev->dev, "Issuing bus rescan\n");
+   scsi_scan_host(host);
+   }
if (jafo) {
spin_lock_irq(host->host_lock);
}
-- 
2.7.4

[PATCH V2 14/15] aacraid: Fix a potential spinlock double unlock bug

[Raghava Aditya Renukunta]

The driver does not unlock the reply  queue spin lock after handling SMART
adapter events. Instead it might attempt to unlock an already unlocked
spin lock.

Fixed by making sure the driver locks the spin lock before freeing it.

Thank you dan for finding this issue out.

Fixes: 6223a39fe6fbbeef (scsi: aacraid: Added support for hotplug)
Reported-by: Dan Carpenter 
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/commsup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 1f716c0..a2ea70d 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -2215,7 +2215,7 @@ static void aac_process_events(struct aac_dev *dev)
/* Thor AIF */
aac_handle_sa_aif(dev, fib);
aac_fib_adapter_complete(fib, (u16)sizeof(u32));
-   continue;
+   goto free_fib;
}
/*
 *  We will process the FIB here or pass it to a
-- 
2.7.4

[PATCH V2 11/15] aacraid: Skip IOP reset on controller panic(SMART Family)

[Raghava Aditya Renukunta]

When the SMART family of controller panic (KERNEL_PANIC) , they do not
honor IOP resets. So better to skip it and directly perform a IWBR reset.

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
Removed unnecessary brackets and fixed a spelling mistake

 drivers/scsi/aacraid/src.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index b23c818..9b11e1a 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -714,6 +714,12 @@ static int aac_src_restart_adapter(struct aac_dev *dev, 
int bled, u8 reset_type)
pr_err("%s%d: adapter kernel panic'd %x.\n",
dev->name, dev->id, bled);
 
+   /*
+* When there is a BlinkLED, IOP_RESET has not effect
+*/
+   if (bled >= 2 && dev->sa_firmware && reset_type & HW_IOP_RESET)
+   reset_type &= ~HW_IOP_RESET;
+
dev->a_ops.adapter_enable_int = aac_src_disable_interrupt;
 
switch (reset_type) {
-- 
2.7.4

[PATCH V2 02/15] aacraid: Use correct channel number for raw srb

[Raghava Aditya Renukunta]

The channel being used for raw srb commands is retrieved from the utility
sent fibs and is converted into physical channel id. The driver does not
need to to do this since the management utility sends the correct channel
id in the first place and in addition the driver sets inaccurate
information in the cmd sent to the firmware and gets an invalid response.

Fixed by using channel id from srb command.

Cc: sta...@vger.kernel.org
Fixes: 423400e64d377c0 ("scsi: aacraid: Include HBA direct interface")
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/commctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 614842a..f6afd50 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -580,7 +580,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void 
__user * arg)
goto cleanup;
}
 
-   chn = aac_logical_to_phys(user_srbcmd->channel);
+   chn = user_srbcmd->channel;
if (chn < AAC_MAX_BUSES && user_srbcmd->id < AAC_MAX_TARGETS &&
dev->hba_map[chn][user_srbcmd->id].devtype ==
AAC_DEVTYPE_NATIVE_RAW) {
-- 
2.7.4

[PATCH V2 04/15] aacraid: Prevent E3 lockup when deleting units

[Raghava Aditya Renukunta]

Arrconf management utility at times sends fibs with AdapterProcessed set
in its fibs. This causes the controller to panic and lockup.

Fixed by failing the commands that have AdapterProcessed set in its flag.

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/commsup.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index a8dd4b5..e221321 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -527,6 +527,10 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned 
long size,
 
if (!(hw_fib->header.XferState & cpu_to_le32(HostOwned)))
return -EBUSY;
+
+   if (hw_fib->header.XferState & cpu_to_le32(AdapterProcessed))
+   return -EINVAL;
+
/*
 *  There are 5 cases with the wait and response requested flags.
 *  The only invalid cases are if the caller requests to wait and
-- 
2.7.4

[PATCH V2 05/15] aacraid: Fix memory leak in fib init path

[Raghava Aditya Renukunta]

aac_fib_map_free frees misaligned fib dma memory, additionally it does not
free up the whole memory.

Fixed by changing the  code to free up the correct and full memory
allocation.

Cc: sta...@vger.kernel.org
Fixes: e8b12f0fb835223 ([SCSI] aacraid: Add new code for PMC-Sierra's SRC based 
controller family)
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 

---
Changes in V2:
Refactored memory free code to make it easier to understand

 drivers/scsi/aacraid/commsup.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index e221321..c10954b 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -95,12 +95,20 @@ static int fib_map_alloc(struct aac_dev *dev)
 
 void aac_fib_map_free(struct aac_dev *dev)
 {
-   if (dev->hw_fib_va && dev->max_cmd_size) {
-   pci_free_consistent(dev->pdev,
-   (dev->max_cmd_size *
-   (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
-   dev->hw_fib_va, dev->hw_fib_pa);
-   }
+   size_t alloc_size;
+   size_t fib_size;
+   int num_fibs;
+
+   if(!dev->hw_fib_va || !dev->max_cmd_size)
+   return;
+
+   num_fibs = dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB;
+   fib_size = dev->max_fib_size + sizeof(struct aac_fib_xporthdr);
+   alloc_size = fib_size * num_fibs + ALIGN32 - 1;
+
+   pci_free_consistent(dev->pdev, alloc_size, dev->hw_fib_va,
+   dev->hw_fib_pa);
+
dev->hw_fib_va = NULL;
dev->hw_fib_pa = 0;
 }
@@ -153,22 +161,20 @@ int aac_fib_setup(struct aac_dev * dev)
if (i<0)
return -ENOMEM;
 
-   /* 32 byte alignment for PMC */
-   hw_fib_pa = (dev->hw_fib_pa + (ALIGN32 - 1)) & ~(ALIGN32 - 1);
-   dev->hw_fib_va = (struct hw_fib *)((unsigned char *)dev->hw_fib_va +
-   (hw_fib_pa - dev->hw_fib_pa));
-   dev->hw_fib_pa = hw_fib_pa;
memset(dev->hw_fib_va, 0,
(dev->max_cmd_size + sizeof(struct aac_fib_xporthdr)) *
(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB));
 
+   /* 32 byte alignment for PMC */
+   hw_fib_pa = (dev->hw_fib_pa + (ALIGN32 - 1)) & ~(ALIGN32 - 1);
+   hw_fib= (struct hw_fib *)((unsigned char *)dev->hw_fib_va +
+   (hw_fib_pa - dev->hw_fib_pa));
+
/* add Xport header */
-   dev->hw_fib_va = (struct hw_fib *)((unsigned char *)dev->hw_fib_va +
+   hw_fib = (struct hw_fib *)((unsigned char *)hw_fib +
sizeof(struct aac_fib_xporthdr));
-   dev->hw_fib_pa += sizeof(struct aac_fib_xporthdr);
+   hw_fib_pa += sizeof(struct aac_fib_xporthdr);
 
-   hw_fib = dev->hw_fib_va;
-   hw_fib_pa = dev->hw_fib_pa;
/*
 *  Initialise the fibs
 */
-- 
2.7.4

[PATCH V2 03/15] aacraid: Fix for excessive prints on EEH

[Raghava Aditya Renukunta]

This issue showed up on a kdump debug(single CPU on powerkvm), when EEH
errors rendered the adapter unusable. The driver correctly detected the
issue and attempted to restart the controller, in doing so the driver
attempted to read the status registers of the controller. This triggered
additional eeh errors which continued for a good 6 minutes.

Fixed by returning without waiting when EEH error is reported.

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 

---
Changes for V2:
Refactored code to remove CONFIG_EEH macros

 drivers/scsi/aacraid/commsup.c | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 56090f5..a8dd4b5 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -461,6 +461,35 @@ int aac_queue_get(struct aac_dev * dev, u32 * index, u32 
qid, struct hw_fib * hw
return 0;
 }
 
+#ifdef CONFIG_EEH
+static inline int aac_check_eeh_failure(struct aac_dev *dev)
+{
+   /* Check for an EEH failure for the given
+* device node. Function eeh_dev_check_failure()
+* returns 0 if there has not been an EEH error
+* otherwise returns a non-zero value.
+*
+* Need to be called before any PCI operation,
+* i.e.,before aac_adapter_check_health()
+*/
+   struct eeh_dev *edev = pci_dev_to_eeh_dev(dev->pdev);
+
+   if (eeh_dev_check_failure(edev)) {
+   /* The EEH mechanisms will handle this
+* error and reset the device if
+* necessary.
+*/
+   return 1;
+   }
+   return 0;
+}
+#else
+static inline int aac_check_eeh_failure(struct aac_dev *dev)
+{
+   return 0;
+}
+#endif
+
 /*
  * Define the highest level of host to adapter communication routines.
  * These routines will support host to adapter FS commuication. These
@@ -496,7 +525,6 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned 
long size,
unsigned long mflags = 0;
unsigned long sflags = 0;
 
-
if (!(hw_fib->header.XferState & cpu_to_le32(HostOwned)))
return -EBUSY;
/*
@@ -662,6 +690,10 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned 
long size,
}
return -ETIMEDOUT;
}
+
+   if (aac_check_eeh_failure(dev))
+   return -EFAULT;
+
if ((blink = aac_adapter_check_health(dev)) > 
0) {
if (wait == -1) {
printk(KERN_ERR "aacraid: 
aac_fib_send: adapter blinkLED 0x%x.\n"
@@ -755,7 +787,12 @@ int aac_hba_send(u8 command, struct fib *fibptr, 
fib_callback callback,
FIB_COUNTER_INCREMENT(aac_config.NativeSent);
 
if (wait) {
+
spin_unlock_irqrestore(>event_lock, flags);
+
+   if (aac_check_eeh_failure(dev))
+   return -EFAULT;
+
/* Only set for first known interruptable command */
if (down_interruptible(>event_wait)) {
fibptr->done = 2;
-- 
2.7.4

[PATCH V2 06/15] aacraid: Added sysfs for driver version

[Raghava Aditya Renukunta]

Added support to retrieve driver version from a new sysfs variable called
driver_version. It makes it easier for the user to figure out the driver
version that is currently running.

Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 

---
Changes in V2:
None

 drivers/scsi/aacraid/linit.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index ab4f1e7..df02784 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1131,6 +1131,13 @@ static ssize_t aac_show_bios_version(struct device 
*device,
return len;
 }
 
+static ssize_t aac_show_driver_version(struct device *device,
+   struct device_attribute *attr,
+   char *buf)
+{
+   return snprintf(buf, PAGE_SIZE, "%s\n", aac_driver_version);
+}
+
 static ssize_t aac_show_serial_number(struct device *device,
   struct device_attribute *attr, char *buf)
 {
@@ -1241,6 +1248,13 @@ static struct device_attribute aac_bios_version = {
},
.show = aac_show_bios_version,
 };
+static struct device_attribute aac_lld_version = {
+   .attr = {
+   .name = "driver_version",
+   .mode = 0444,
+   },
+   .show = aac_show_driver_version,
+};
 static struct device_attribute aac_serial_number = {
.attr = {
.name = "serial_number",
@@ -1278,6 +1292,7 @@ static struct device_attribute *aac_attrs[] = {
_kernel_version,
_monitor_version,
_bios_version,
+   _lld_version,
_serial_number,
_max_channel,
_max_id,
-- 
2.7.4

[PATCH V2 00/15] aacraid: Fixes and enhancements for arc family

[Raghava Aditya Renukunta]

This patch set contains issue fixes, enhancements and other misc changes.

The majority of the fixes are a direct outcome of testing and work done on the
adapter reset mechanism. Initially it just had IOP reset and then was augmented
with IWBR soft hardware resets in the previous patch set. The reset mechanism
is triggered in 2 paths, one is from the eh handler from the kernel and the
other is from the driver's internal periodic health checkup.

Changes in V2:
Withdrew PATCH 10 (aacraid: Terminate kthread on controller fw assert) in lieu
of reworking and submitting it in this patchset.
Refactored memory leak fix patch
Refactored EEH patch to remove macro
Misc grammar fixes

Raghava Aditya Renukunta (15):
 [SCSI] aacraid: Fix camel case
 [SCSI] aacraid: Use correct channel number for raw srb
 [SCSI] aacraid: Fix for excessive prints on EEH
 [SCSI] aacraid: Prevent E3 lockup when deleting units
 [SCSI] aacraid: Fix memory leak in fib init path
 [SCSI] aacraid: Added sysfs for driver version
 [SCSI] aacraid: Fix sync fibs time out on controller reset
 [SCSI] aacraid: Skip wellness sync on controller failure
 [SCSI] aacraid: Reload offlined drives after controller reset
 [SCSI] aacraid: Decrease adapter health check  interval
 [SCSI] aacraid: Skip IOP reset on controller panic(SMART Family)
 [SCSI] aacraid: Reorder Adapter status check
 [SCSI] aacraid: Save adapter fib log before an IOP reset
 [SCSI] aacraid: Fix a potential spinlock double unlock bug
 [SCSI] aacraid: Update driver version

 drivers/scsi/aacraid/aachba.c   |  59 --
 drivers/scsi/aacraid/aacraid.h  | 107 +---
 drivers/scsi/aacraid/commctrl.c |   2 +-
 drivers/scsi/aacraid/comminit.c |   2 +-
 drivers/scsi/aacraid/commsup.c  | 107 
 drivers/scsi/aacraid/linit.c|  47 --
 drivers/scsi/aacraid/rx.c   |   2 +-
 drivers/scsi/aacraid/src.c  |  48 +++---
 8 files changed, 254 insertions(+), 120 deletions(-)

-- 
2.7.4

[PATCH V2 01/15] aacraid: Fix camel case

[Raghava Aditya Renukunta]

Replaced camel case with snake case for init supported options.

Suggested-by: Johannes Thumshirn 
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/aachba.c  | 53 ---
 drivers/scsi/aacraid/aacraid.h | 98 +-
 drivers/scsi/aacraid/commsup.c |  6 +--
 drivers/scsi/aacraid/linit.c   | 32 +++---
 drivers/scsi/aacraid/rx.c  |  2 +-
 drivers/scsi/aacraid/src.c |  2 +-
 6 files changed, 100 insertions(+), 93 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 907f1e8..98d4ffd 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -483,7 +483,7 @@ int aac_get_containers(struct aac_dev *dev)
if (status >= 0) {
dresp = (struct aac_get_container_count_resp *)fib_data(fibptr);
maximum_num_containers = 
le32_to_cpu(dresp->ContainerSwitchEntries);
-   if (fibptr->dev->supplement_adapter_info.SupportedOptions2 &
+   if (fibptr->dev->supplement_adapter_info.supported_options2 &
AAC_OPTION_SUPPORTED_240_VOLUMES) {
maximum_num_containers =
le32_to_cpu(dresp->MaxSimpleVolumes);
@@ -639,13 +639,16 @@ static void _aac_probe_container2(void * context, struct 
fib * fibptr)
fsa_dev_ptr = fibptr->dev->fsa_dev;
if (fsa_dev_ptr) {
struct aac_mount * dresp = (struct aac_mount *) 
fib_data(fibptr);
+   __le32 sup_options2;
+
fsa_dev_ptr += scmd_id(scsicmd);
+   sup_options2 =
+   fibptr->dev->supplement_adapter_info.supported_options2;
 
if ((le32_to_cpu(dresp->status) == ST_OK) &&
(le32_to_cpu(dresp->mnt[0].vol) != CT_NONE) &&
(le32_to_cpu(dresp->mnt[0].state) != FSCS_HIDDEN)) {
-   if 
(!(fibptr->dev->supplement_adapter_info.SupportedOptions2 &
-   AAC_OPTION_VARIABLE_BLOCK_SIZE)) {
+   if (!(sup_options2 & AAC_OPTION_VARIABLE_BLOCK_SIZE)) {
dresp->mnt[0].fileinfo.bdevinfo.block_size = 
0x200;
fsa_dev_ptr->block_size = 0x200;
} else {
@@ -688,7 +691,7 @@ static void _aac_probe_container1(void * context, struct 
fib * fibptr)
int status;
 
dresp = (struct aac_mount *) fib_data(fibptr);
-   if (!(fibptr->dev->supplement_adapter_info.SupportedOptions2 &
+   if (!(fibptr->dev->supplement_adapter_info.supported_options2 &
AAC_OPTION_VARIABLE_BLOCK_SIZE))
dresp->mnt[0].capacityhigh = 0;
if ((le32_to_cpu(dresp->status) != ST_OK) ||
@@ -705,7 +708,7 @@ static void _aac_probe_container1(void * context, struct 
fib * fibptr)
 
dinfo = (struct aac_query_mount *)fib_data(fibptr);
 
-   if (fibptr->dev->supplement_adapter_info.SupportedOptions2 &
+   if (fibptr->dev->supplement_adapter_info.supported_options2 &
AAC_OPTION_VARIABLE_BLOCK_SIZE)
dinfo->command = cpu_to_le32(VM_NameServeAllBlk);
else
@@ -745,7 +748,7 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, 
int (*callback)(stru
 
dinfo = (struct aac_query_mount *)fib_data(fibptr);
 
-   if (fibptr->dev->supplement_adapter_info.SupportedOptions2 &
+   if (fibptr->dev->supplement_adapter_info.supported_options2 &
AAC_OPTION_VARIABLE_BLOCK_SIZE)
dinfo->command = cpu_to_le32(VM_NameServeAllBlk);
else
@@ -896,12 +899,14 @@ char * get_container_type(unsigned tindex)
 static void setinqstr(struct aac_dev *dev, void *data, int tindex)
 {
struct scsi_inq *str;
+   struct aac_supplement_adapter_info *sup_adap_info;
 
+   sup_adap_info = >supplement_adapter_info;
str = (struct scsi_inq *)(data); /* cast data to scsi inq block */
memset(str, ' ', sizeof(*str));
 
-   if (dev->supplement_adapter_info.AdapterTypeText[0]) {
-   char * cp = dev->supplement_adapter_info.AdapterTypeText;
+   if (sup_adap_info->adapter_type_text[0]) {
+   char *cp = sup_adap_info->adapter_type_text;
int c;
if ((cp[0] == 'A') && (cp[1] == 'O') && (cp[2] == 'C'))
inqstrcpy("SMC", str->vid);
@@ -911,8 +916,7 @@ static void setinqstr(struct aac_dev *dev, void *data, int 
tindex)
++cp;
c = *cp;
*cp = '\0';
-   inqstrcpy (dev->supplement_adapter_info.AdapterTypeText,
-  str->vid);
+ 

[PATCH V2 07/15] aacraid: Fix sync fibs time out on controller reset

[Raghava Aditya Renukunta]

After controller shutdown, all sync fibs time out due to not knowing
about the switch to INT-x mode

Fixed by replacing aac_src_access_devreg() to aac_set_intx_mode() call.

Cc: sta...@vger.kernel.org
Fixes: 495c021767bd78c998 (aacraid: MSI-x support)
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/aacraid.h  | 1 +
 drivers/scsi/aacraid/comminit.c | 2 +-
 drivers/scsi/aacraid/src.c  | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index b5a2c87..9281e72 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -2639,6 +2639,7 @@ void aac_hba_callback(void *context, struct fib *fibptr);
 #define fib_data(fibctx) ((void *)(fibctx)->hw_fib_va->data)
 struct aac_dev *aac_init_adapter(struct aac_dev *dev);
 void aac_src_access_devreg(struct aac_dev *dev, int mode);
+void aac_set_intx_mode(struct aac_dev *dev);
 int aac_get_config_status(struct aac_dev *dev, int commit_flag);
 int aac_get_containers(struct aac_dev *dev);
 int aac_scsi_cmd(struct scsi_cmnd *cmd);
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index d0c7724..cd3456e 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -326,7 +326,7 @@ int aac_send_shutdown(struct aac_dev * dev)
 dev->pdev->device == PMC_DEVICE_S8 ||
 dev->pdev->device == PMC_DEVICE_S9) &&
 dev->msi_enabled)
-   aac_src_access_devreg(dev, AAC_ENABLE_INTX);
+   aac_set_intx_mode(dev);
return status;
 }
 
diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index c17b060..b23c818 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -657,7 +657,7 @@ static int aac_srcv_ioremap(struct aac_dev *dev, u32 size)
return 0;
 }
 
-static void aac_set_intx_mode(struct aac_dev *dev)
+void aac_set_intx_mode(struct aac_dev *dev)
 {
if (dev->msi_enabled) {
aac_src_access_devreg(dev, AAC_ENABLE_INTX);
-- 
2.7.4

[PATCH V2 08/15] aacraid: Skip wellness sync on controller failure

[Raghava Aditya Renukunta]

aac_command_thread checks on the health of controller periodically,
using aac_check_health. If the status is an error state KERNEL_PANIC or
anything else. The driver will attempt to restart the adapter, but the
response is not checked in aac_command_thread. This allows the periodic
sync to go thru and lead the driver to a hung state.

Fixed by terminating the periodic loop(intended per original design),
if the controller is not restored to a healthy state.

Cc: sta...@vger.kernel.org
Fixes: 3d77d8404478353358 (scsi: aacraid: Added support for periodic wellness 
sync)
Signed-off-by: Raghava Aditya Renukunta 
Reviewed-by: David Carroll 
Reviewed-by: Johannes Thumshirn 

---
Changes in V2:
None

 drivers/scsi/aacraid/commsup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index c10954b..eb4d8cf 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -2481,7 +2481,7 @@ int aac_command_thread(void *data)
 
/* Don't even try to talk to adapter if its sick */
ret = aac_check_health(dev);
-   if (!dev->queues)
+   if (ret || !dev->queues)
break;
next_check_jiffies = jiffies
   + ((long)(unsigned)check_interval)
-- 
2.7.4

[PATCH 1/2] Support Pegasus 3 product

[Charles Chiou]

From: Charles 

Pegasus series is a RAID support product by using Thunderbolt technology.
The newest product, Pegasus 3(P3) is support Thunderbolt 3 technology with 
another chip.

1.Change driver version.
2.Add P3 VID, DID and define it's device address.
3.P3 use msi interrupt, so stex_request_irq P3 type enable msi.
4.For hibernation, use msi_lock in stex_ss_handshake to prevent msi register 
write again when handshaking.
5.P3 doesn't need read() as flush.
6.In stex_ss_intr & stex_abort, P3 only clear interrupt register when getting 
vendor defined interrupt.

Signed-off-by: Charles.Chiou 
Signed-off-by: Paul.Lyu 
---
 drivers/scsi/stex.c | 262 ++--
 1 file changed, 195 insertions(+), 67 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 5b23175..e177dfe 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -38,9 +38,9 @@
 #include 
 
 #define DRV_NAME "stex"
-#define ST_DRIVER_VERSION  "5.00..01"
-#define ST_VER_MAJOR   5
-#define ST_VER_MINOR   00
+#define ST_DRIVER_VERSION  "6.02..01"
+#define ST_VER_MAJOR   6
+#define ST_VER_MINOR   02
 #define ST_OEM 
 #define ST_BUILD_VER   01
 
@@ -64,6 +64,13 @@ enum {
YI2H_INT_C  = 0xa0,
YH2I_REQ= 0xc0,
YH2I_REQ_HI = 0xc4,
+   PSCRATCH0   = 0xb0,
+   PSCRATCH1   = 0xb4,
+   PSCRATCH2   = 0xb8,
+   PSCRATCH3   = 0xbc,
+   PSCRATCH4   = 0xc8,
+   MAILBOX_BASE= 0x1000,
+   MAILBOX_HNDSHK_STS  = 0x0,
 
/* MU register value */
MU_INBOUND_DOORBELL_HANDSHAKE   = (1 << 0),
@@ -87,7 +94,7 @@ enum {
MU_STATE_STOP   = 5,
MU_STATE_NOCONNECT  = 6,
 
-   MU_MAX_DELAY= 120,
+   MU_MAX_DELAY= 50,
MU_HANDSHAKE_SIGNATURE  = 0x5555,
MU_HANDSHAKE_SIGNATURE_HALF = 0x5a5a,
MU_HARD_RESET_WAIT  = 3,
@@ -135,6 +142,7 @@ enum {
st_yosemite = 2,
st_seq  = 3,
st_yel  = 4,
+   st_P3   = 5,
 
PASSTHRU_REQ_TYPE   = 0x0001,
PASSTHRU_REQ_NO_WAKEUP  = 0x0100,
@@ -339,6 +347,7 @@ struct st_hba {
u16 rq_size;
u16 sts_count;
u8  supports_pm;
+   int msi_lock;
 };
 
 struct st_card_info {
@@ -540,11 +549,15 @@ static void stex_controller_info(struct st_hba *hba, 
struct st_ccb *ccb)
 
++hba->req_head;
hba->req_head %= hba->rq_count+1;
-
-   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
-   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
-   writel(addr, hba->mmio_base + YH2I_REQ);
-   readl(hba->mmio_base + YH2I_REQ); /* flush */
+   if (hba->cardtype == st_P3) {
+   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
+   writel(addr, hba->mmio_base + YH2I_REQ);
+   } else {
+   writel((addr >> 16) >> 16, hba->mmio_base + YH2I_REQ_HI);
+   readl(hba->mmio_base + YH2I_REQ_HI); /* flush */
+   writel(addr, hba->mmio_base + YH2I_REQ);
+   readl(hba->mmio_base + YH2I_REQ); /* flush */
+   }
 }
 
 static void return_abnormal_state(struct st_hba *hba, int status)
@@ -974,15 +987,31 @@ static irqreturn_t stex_ss_intr(int irq, void *__hba)
 
spin_lock_irqsave(hba->host->host_lock, flags);
 
-   data = readl(base + YI2H_INT);
-   if (data && data != 0x) {
-   /* clear the interrupt */
-   writel(data, base + YI2H_INT_C);
-   stex_ss_mu_intr(hba);
-   spin_unlock_irqrestore(hba->host->host_lock, flags);
-   if (unlikely(data & SS_I2H_REQUEST_RESET))
-   queue_work(hba->work_q, >reset_work);
-   return IRQ_HANDLED;
+   if (hba->cardtype == st_yel) {
+   data = readl(base + YI2H_INT);
+   if (data && data != 0x) {
+   /* clear the interrupt */
+   writel(data, base + YI2H_INT_C);
+   stex_ss_mu_intr(hba);
+   spin_unlock_irqrestore(hba->host->host_lock, flags);
+   if (unlikely(data & SS_I2H_REQUEST_RESET))
+   queue_work(hba->work_q, >reset_work);
+   return 

[PATCH 2/2] Add S6 support

[Charles Chiou]

From: Charles 

1.Add reboot notifier and register it in stex_probe for all supported device.
2.For all supported device in restart flow, we get a callback from notifier and 
set S6flag for stex_shutdown & stex_hba_stop to send restart command to FW.

Signed-off-by: Charles.Chiou 
Signed-off-by: Paul.Lyu 
---
 drivers/scsi/stex.c | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index e177dfe..0751561 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -362,6 +363,12 @@ struct st_card_info {
u16 sts_count;
 };
 
+int S6flag;
+static int stex_halt(struct notifier_block *nb, ulong event, void *buf);
+static struct notifier_block stex_notifier = {
+   stex_halt, NULL, 0
+};
+
 static int msi;
 module_param(msi, int, 0);
 MODULE_PARM_DESC(msi, "Enable Message Signaled Interrupts(0=off, 1=on)");
@@ -1673,6 +1680,9 @@ static int stex_probe(struct pci_dev *pdev, const struct 
pci_device_id *id)
 
pci_set_master(pdev);
 
+   S6flag = 0;
+   register_reboot_notifier(_notifier);
+
host = scsi_host_alloc(_template, sizeof(struct st_hba));
 
if (!host) {
@@ -1947,15 +1957,20 @@ static void stex_remove(struct pci_dev *pdev)
scsi_host_put(hba->host);
 
pci_disable_device(pdev);
+
+   unregister_reboot_notifier(_notifier);
 }
 
 static void stex_shutdown(struct pci_dev *pdev)
 {
struct st_hba *hba = pci_get_drvdata(pdev);
 
-   if (hba->supports_pm == 0)
+   if (hba->supports_pm == 0) {
stex_hba_stop(hba, ST_IGNORED);
-   else
+   } else if (hba->supports_pm == 1 && S6flag) {
+   unregister_reboot_notifier(_notifier);
+   stex_hba_stop(hba, ST_S6);
+   } else
stex_hba_stop(hba, ST_S5);
 }
 
@@ -1992,6 +2007,12 @@ static int stex_resume(struct pci_dev *pdev)
stex_handshake(hba);
return 0;
 }
+
+static int stex_halt(struct notifier_block *nb, unsigned long event, void *buf)
+{
+   S6flag = 1;
+   return NOTIFY_OK;
+}
 MODULE_DEVICE_TABLE(pci, stex_pci_tbl);
 
 static struct pci_driver stex_pci_driver = {
-- 
1.9.1

[PATCH v5] sd: Check for unaligned partial completion

[Damien Le Moal]

Commit f2e767bb5d6e ("mpt3sas: Force request partial completion alignment")
was not considering the case of REQ_TYPE_FS commands not operating on
logical block size units (e.g. REQ_OP_ZONE_REPORT and its 64B aligned
partial replies). This could result is incorrectly retrying (forever)
those commands.

Move the partial completion alignement check of mpt3sas to a generic
implementation in sd_done so that the check ignores REQ_TYPE_FS
requests with special payload size handling (REQ_OP_DISCARD,
REQ_OP_WRITE_SAME, REQ_OP_ZONE_REPORT and REQ_OP_ZONE_RESET). For the
remaining REQ_OP_FLUSH, REQ_OP_READ and REQ_OP_WRITE, we only need to
check the resid alignment, correct it if necessary and then correct
good_bytes. Note that in this case, good_bytes will always initially be 0
or aligned on the device logical block size, so correcting resid alignment
will always result in good_bytes also being properly aligned.

Signed-off-by: Damien Le Moal 
Fixes: f2e767bb5d6e ("mpt3sas: Force request partial completion alignment")

---

Changes from v4:
- As suggested by Bart, make sure resid does not exceed good_bytes
- Use sd_printk for message instead of going through scsi log

Changes from v3:
- Moved check to initial switch-case so that commands with special payload
  handling are ignored.

Changes from v2:
- Fixed good_bytes calculation after correction of unaligned resid
  It should be good_bytes=scsi_buflen() - resid, and not good_bytes-=resid

 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 ---
 drivers/scsi/sd.c| 17 +
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 0b5b423..1961535 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4658,7 +4658,6 @@ _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;
 
mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
@@ -4717,20 +4716,6 @@ _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;
-   if (unlikely(scmd->request->cmd_type == REQ_TYPE_FS && sector_sz &&
-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 = round_down(xfer_cnt, 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);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1f5d92a..525b162 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1790,6 +1790,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 {
int result = SCpnt->result;
unsigned int good_bytes = result ? 0 : scsi_bufflen(SCpnt);
+   unsigned int sector_size = SCpnt->device->sector_size;
+   unsigned int resid;
struct scsi_sense_hdr sshdr;
struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
struct request *req = SCpnt->request;
@@ -1820,6 +1822,21 @@ static int sd_done(struct scsi_cmnd *SCpnt)
scsi_set_resid(SCpnt, blk_rq_bytes(req));
}
break;
+   default:
+   /*
+* In case of bogus fw or device, we could end up having
+* an unaligned partial completion. Check this here and force
+* alignment.
+*/
+   resid = scsi_get_resid(SCpnt);
+   if (resid & (sector_size - 1)) {
+   sd_printk(KERN_INFO, sdkp,
+   "Unaligned partial completion (resid=%u, 
sector_sz=%u)\n",
+   resid, sector_size);
+   resid = min(good_bytes, round_up(resid, sector_size));
+   good_bytes -= resid;
+   scsi_set_resid(SCpnt, resid);
+   }
}
 
if (result) {
-- 
2.9.3

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Ondrej Zary]

On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
[...]
> Are you trying to figure out which commands are going to disconnect during
> a transfer? This is really a function of the firmware in the target; there
> are no good heuristics AFAICT, so the PDMA algorithm has to be robust.
> mac_scsi has to cope with this too.
>
> Does the problem go away when you assign no IRQ? When instance->irq ==
> NO_IRQ, the core driver will inhibit disconnect privileges.

Yes, it seems to run fine with IRQ/disconnect disabled.
With IRQ enabled, "dd if=/dev/sr0 of=anything" stops after a while. I get 
gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.

When I enable interrupts during PDMA (like the removed UNSAFE macro did), the 
problems go away. I see an IRQ after each pread call.
(had to disable "no 53C80 gated irq after transfer" and "no end dma signal" 
messages to reduce log spam)

-- 
Ondrej Zary

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Keith Busch]

On Thu, Feb 16, 2017 at 01:21:29PM -0500, Mike Snitzer wrote:
> Then undeprecate them.  Decisions like marking a path checker deprecated
> were _not_ made with NVMe in mind.  They must predate NVMe.
> 
> multipath-tools has tables that specify all the defaults for a given
> target backend.  NVMe will just be yet another.  Yes some user _could_
> shoot themselves in the foot by overriding the proper configuration but
> since when are we motivated by _not_ giving users the power to hang
> themselves?
> 
> As for configurability (chosing between N valid configs/settings): At
> some point the user will want one behaviour vs another.  Thinking
> otherwise is just naive.  Think error timeouts, etc.  Any multipath
> kernel implementation (which dm-multipath is BTW) will eventually find
> itself at a crossroads where the underlying fabric could be tweaked in
> different ways.  Thinking you can just hardcode these attributes and
> settings is foolish.

Roger that, and I absolutely want to see this work with the existing
framework.

I just think it'd be easier for everyone if multipath were more like
the generic block layer, in that devices are surfaced with configurable
policies without userspace telling it which to use. The kernel knowing
safe defaults for a particular device is probably the more common case,
and userspace can still tune them as needed. Of course, I accept you're
in a better position to know if this is folly.

RE: [PATCH 10/16] aacraid: Terminate kthread on controller fw assert

[Raghava Aditya Renukunta]

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Thursday, February 16, 2017 1:31 AM
> To: Raghava Aditya Renukunta
> ; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com; linux-scsi@vger.kernel.org
> Cc: Dave Carroll ; Gana Sridaran
> ; Scott Benesh
> ; dan.carpen...@oracle.com
> Subject: Re: [PATCH 10/16] aacraid: Terminate kthread on controller fw
> assert
> 
> EXTERNAL EMAIL
> 
> 
> On 02/15/2017 11:22 PM, Raghava Aditya Renukunta wrote:
> >>
> >> This look a bit scary. Can't the kthread be converted to a workqueue so
> >> we could call cancel_work_sync()?
> >
> > Could you please elaborate on the reasons why this fix is scary?
> > I understand that killing a thread is not standard (for any reason),
> > and if there are other nuanced issues I would like to understand them.
> 
> I'm actually concerned that this could have all kinds of side effects.
> But this is just a gut feeling. I see some drm drivers are doing the
> same, so it might be possible, but IMHO this is not a good design.
> 
> And IIRC kthreads do have more downsides (i.e. CPU hotplugging and
> issues with kernel live patching).
> 
> I think most kthreads (haven't looked too close to the aacraid kthread I
> must admit, but I'll be doing so) can be converted to either workqueues
> or timers (or a combination of both).
> 
> Thanks,
> Johannes

Makes sense,  and I agree. With that being said I will withdraw this patch
and resend it out in different patch series once we  rework aac_command_thread
into a work queue/timers.

Regards,
Raghava Aditya


> --
> 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

RE: [PATCH 06/16] aacraid: Added sysfs for driver version

[Raghava Aditya Renukunta]

> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Wednesday, February 15, 2017 11:43 PM
> To: Raghava Aditya Renukunta
> ; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com; linux-scsi@vger.kernel.org
> Cc: Dave Carroll ; Gana Sridaran
> ; Scott Benesh
> ; dan.carpen...@oracle.com
> Subject: Re: [PATCH 06/16] aacraid: Added sysfs for driver version
> 
> EXTERNAL EMAIL
> 
> 
> On 02/15/2017 07:12 PM, Raghava Aditya Renukunta wrote:
> >
> > I agree , but it makes it easier to get the driver version when I am
> developing
> > and I don't know which driver version is currently loaded
> >
> > In addition internally our test automation suites use this information as
> > opposed to modinfo to get the driver version running.
> 
> OK then.
> 
> Speaking of test automation, is there something you may be able to share?
> 
> Thanks,
> Johannes

Well still in the very initial phases, so nothing concrete yet.

The plan is to compile the source and install it on various kernels for testing
 and modinfo  will not be able to retrieve the version info.  

Additionally it seems to be a  cleaner method when compared to modinfo.

Regards,
Raghava Aditya

> --
> 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

Re: [PATCH v4] sd: Check for unaligned partial completion

[Guilherme G. Piccoli]

On 15/02/2017 05:06, Ram Pai wrote:
> On Wed, Feb 15, 2017 at 03:48:52PM +0900, Damien Le Moal wrote:
>> Christoph,
>>
>> On 2/15/17 15:34, Christoph Hellwig wrote:
>>> this looks reasonable, but we should ask Guilherme and Ram to confirm
>>> it fixes their originally reported issue.  I've added them to Cc.
>>
>> Thank you.
>>
>> Guilherme, Ram,
>>
>> Please test ! The original fix breaks the zoned block device support
>> that was newly added to 4.10. So we need a fix for that and your issue
>> combined before Linus releases 4.10 stable this weekend.
> 
> Yes logically it looks correct and should fix our issue. 
> 
> I doubt Guilherme has access to the same adapter firmware that exposed
> the original problem. We will probably have to induce the erroneous
> behavior in the driver to reproduce the issue and verify the fix.

Ram, I do have access but the environment changed a lot, including
disks, setup of the distributed filesystem and tests...and so I wasn't
able to reproduce the issue anymore.

Since the logic of the new approach looks correct, I'm ok with the patch.

Thanks,


Guilherme

> 
> Will let you know,
> RP
> 

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Mike Snitzer]

On Thu, Feb 16 2017 at  1:07pm -0500,
Keith Busch  wrote:

> On Thu, Feb 16, 2017 at 05:37:41PM +, Bart Van Assche wrote:
> > On Thu, 2017-02-16 at 12:38 -0500, Keith Busch wrote:
> > > Maybe I'm not seeing the bigger picture. Is there some part to multipath
> > > that the kernel is not in a better position to handle?
> > 
> > Does this mean that the code to parse /etc/multipath.conf will be moved into
> > the kernel? Or will we lose the ability to configure the policies that
> > /etc/multipath.conf allows to configure?
> 
> No, I'm just considering the settings for a device that won't work
> at all if multipath.conf is wrong. For example, the uuid attributes,
> path priority, or path checker. These can't be considered configurable
> policies if all but one of them are invalid for a specific device type.
> 
> It shouldn't even be an option to let a user select TUR path checker
> for NVMe, and the only checkers multipath-tools provide that even make
> sense for NVMe are deprecated.

Then undeprecate them.  Decisions like marking a path checker deprecated
were _not_ made with NVMe in mind.  They must predate NVMe.

multipath-tools has tables that specify all the defaults for a given
target backend.  NVMe will just be yet another.  Yes some user _could_
shoot themselves in the foot by overriding the proper configuration but
since when are we motivated by _not_ giving users the power to hang
themselves?

As for configurability (chosing between N valid configs/settings): At
some point the user will want one behaviour vs another.  Thinking
otherwise is just naive.  Think error timeouts, etc.  Any multipath
kernel implementation (which dm-multipath is BTW) will eventually find
itself at a crossroads where the underlying fabric could be tweaked in
different ways.  Thinking you can just hardcode these attributes and
settings is foolish.

Re: [bug report] scsi: ufs-qcom: dump additional testbus registers

[Subhash Jadavani]

On 2017-02-13 23:58, Dan Carpenter wrote:

Hello Venkat Gopalakrishnan,

The patch 9c46b8676271: "scsi: ufs-qcom: dump additional testbus
registers" from Feb 3, 2017, leads to the following static checker
warning:

drivers/scsi/ufs/ufs-qcom.c:1531 ufs_qcom_testbus_cfg_is_ok()
warn: impossible condition '(host->testbus.select_minor > 255) =>
(0-255 > 255)'

drivers/scsi/ufs/ufs-qcom.c
  1517  static bool ufs_qcom_testbus_cfg_is_ok(struct ufs_qcom_host 
*host)

  1518  {
  1519  if (host->testbus.select_major >= TSTBUS_MAX) {
  1520  dev_err(host->hba->dev,
  1521  "%s: UFS_CFG1[TEST_BUS_SEL} may not
equal 0x%05X\n",
  1522  __func__, host->testbus.select_major);
  1523  return false;
  1524  }
  1525
  1526  /*
  1527   * Not performing check for each individual 
select_major

  1528   * mappings of select_minor, since there is no harm in
  1529   * configuring a non-existent select_minor
  1530   */
  1531  if (host->testbus.select_minor > 0xFF) {
^

It might make sense to keep this check.  I don't know.  But it's
confusing that 0xFF is a magic number.  Better to make it a define.


Yes, i agree, this check is redundant. I will post a fix to remove this.



  1532  dev_err(host->hba->dev,
  1533  "%s: 0x%05X is not a legal testbus 
option\n",

  1534  __func__, host->testbus.select_minor);
  1535  return false;
  1536  }
  1537
  1538  return true;
  1539  }

regards,
dan carpenter


--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Sagi Grimberg]

I'm fine with the path selectors getting moved out; maybe it'll
encourage new path selectors to be developed.

But there will need to be some userspace interface stood up to support
your native NVMe multipathing (you may not think it needed but think in
time there will be a need to configure _something_).  That is the
fragmentation I'm referring to.


I guess one config option that we'd need is multibus vs. failover
which are used per use-case.

I'd have to say that having something much simpler than multipath-tools
does sound appealing.

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Keith Busch]

On Thu, Feb 16, 2017 at 05:37:41PM +, Bart Van Assche wrote:
> On Thu, 2017-02-16 at 12:38 -0500, Keith Busch wrote:
> > Maybe I'm not seeing the bigger picture. Is there some part to multipath
> > that the kernel is not in a better position to handle?
> 
> Does this mean that the code to parse /etc/multipath.conf will be moved into
> the kernel? Or will we lose the ability to configure the policies that
> /etc/multipath.conf allows to configure?

No, I'm just considering the settings for a device that won't work
at all if multipath.conf is wrong. For example, the uuid attributes,
path priority, or path checker. These can't be considered configurable
policies if all but one of them are invalid for a specific device type.

It shouldn't even be an option to let a user select TUR path checker
for NVMe, and the only checkers multipath-tools provide that even make
sense for NVMe are deprecated.

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Bart Van Assche]

On Thu, 2017-02-16 at 12:38 -0500, Keith Busch wrote:
> Maybe I'm not seeing the bigger picture. Is there some part to multipath
> that the kernel is not in a better position to handle?

Does this mean that the code to parse /etc/multipath.conf will be moved into
the kernel? Or will we lose the ability to configure the policies that
/etc/multipath.conf allows to configure?

Bart.

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Keith Busch]

On Thu, Feb 16, 2017 at 10:13:37AM -0500, Mike Snitzer wrote:
> On Thu, Feb 16 2017 at  9:26am -0500,
> Christoph Hellwig  wrote:
>
> > just a little new code in the block layer, and a move of the path
> > selectors from dm to the block layer.  I would not call this
> > fragmentation.
> 
> I'm fine with the path selectors getting moved out; maybe it'll
> encourage new path selectors to be developed.
> 
> But there will need to be some userspace interface stood up to support
> your native NVMe multipathing (you may not think it needed but think in
> time there will be a need to configure _something_).  That is the
> fragmentation I'm referring to.

I'm not sure what Christoph's proposal looks like, but I have to agree
that multipath support directly in the kernel without requiring user
space to setup the mpath block device is easier for everyone. The only
NVMe specific part, though, just needs to be how it reports unique
identifiers to the multipath layer.

Maybe I'm not seeing the bigger picture. Is there some part to multipath
that the kernel is not in a better position to handle?

Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Keith Busch]

On Thu, Feb 16, 2017 at 12:05:19PM -0500, Keith Busch wrote:
> On Thu, Feb 16, 2017 at 04:12:23PM +0100, Hannes Reinecke wrote:
> > The device handler needs to check if a given queue belongs to
> > a scsi device; only then does it make sense to attach a device
> > handler.
> > 
> > Signed-off-by: Hannes Reinecke 
> 
> The thing I don't like is that this still has dm-mpath directly calling
> into scsi. I don't think dm-mpath has any business calling protocol
> specific APIs, and doesn't help other protocols with similar needs.
> 
> Can we solve this with an indirection layer instead? 
> 
> (untested; just checking if this direction is preferable)

Eh osrry, I accidently included unrelated stuff in the previous. Here's
what I meant to send:

---
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 3570bcb..28310a2 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -847,7 +847,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, 
struct path_selector *ps
 
if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, >flags)) {
 retain:
-   attached_handler_name = scsi_dh_attached_handler_name(q, 
GFP_KERNEL);
+   attached_handler_name = blk_dh_attached_handler_name(q, 
GFP_KERNEL);
if (attached_handler_name) {
/*
 * Clear any hw_handler_params associated with a
@@ -870,7 +870,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, 
struct path_selector *ps
}
 
if (m->hw_handler_name) {
-   r = scsi_dh_attach(q, m->hw_handler_name);
+   r = blk_dh_attach(q, m->hw_handler_name);
if (r == -EBUSY) {
char b[BDEVNAME_SIZE];
 
@@ -885,7 +885,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, 
struct path_selector *ps
}
 
if (m->hw_handler_params) {
-   r = scsi_dh_set_params(q, m->hw_handler_params);
+   r = blk_dh_set_params(q, m->hw_handler_params);
if (r < 0) {
ti->error = "unable to set hardware "
"handler parameters";
@@ -1526,7 +1526,7 @@ static void activate_path(struct work_struct *work)
struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
 
if (pgpath->is_active && !blk_queue_dying(q))
-   scsi_dh_activate(q, pg_init_done, pgpath);
+   blk_dh_activate(q, pg_init_done, pgpath);
else
pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9e1e14..e23c7d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2028,6 +2028,13 @@ static u64 scsi_calculate_bounce_limit(struct Scsi_Host 
*shost)
return bounce_limit;
 }
 
+static struct dh_ops scsi_dh_ops = {
+   .dh_activate = scsi_dh_activate,
+   .dh_attach = scsi_dh_attach,
+   .dh_attached_handler_name = scsi_dh_attached_handler_name,
+   .dh_set_params = scsi_dh_set_params,
+};
+
 static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 {
struct device *dev = shost->dma_dev;
@@ -2062,6 +2069,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, 
struct request_queue *q)
 * blk_queue_update_dma_alignment() later.
 */
blk_queue_dma_alignment(q, 0x03);
+
+   q->dh_ops = _dh_ops;
 }
 
 struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1ca8e8f..5899768 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -378,6 +378,14 @@ static inline int blkdev_reset_zones_ioctl(struct 
block_device *bdev,
 
 #endif /* CONFIG_BLK_DEV_ZONED */
 
+typedef void (*activate_complete)(void *, int);
+struct dh_ops {
+   int (*dh_activate)(struct request_queue *, activate_complete, void *);
+   int (*dh_attach)(struct request_queue *, const char *);
+   const char *(*dh_attached_handler_name)(struct request_queue *, gfp_t);
+   int (*dh_set_params)(struct request_queue *, const char *);
+};
+
 struct request_queue {
/*
 * Together with queue_head for cacheline sharing
@@ -408,6 +416,7 @@ struct request_queue {
lld_busy_fn *lld_busy_fn;
 
struct blk_mq_ops   *mq_ops;
+   struct dh_ops   *dh_ops;
 
unsigned int*mq_map;
 
@@ -572,6 +581,35 @@ struct request_queue {
boolmq_sysfs_init_done;
 };
 
+static inline int blk_dh_activate(struct request_queue *q, activate_complete 
fn, void *data)
+{
+   if (q->dh_ops && q->dh_ops->dh_activate)
+   return q->dh_ops->dh_activate(q, fn, data);
+   fn(data, 0);
+   return 0;
+}
+
+static inline int blk_dh_attach(struct request_queue *q, const char *name)
+{
+   

Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Keith Busch]

On Thu, Feb 16, 2017 at 04:12:23PM +0100, Hannes Reinecke wrote:
> The device handler needs to check if a given queue belongs to
> a scsi device; only then does it make sense to attach a device
> handler.
> 
> Signed-off-by: Hannes Reinecke 

The thing I don't like is that this still has dm-mpath directly calling
into scsi. I don't think dm-mpath has any business calling protocol
specific APIs, and doesn't help other protocols with similar needs.

Can we solve this with an indirection layer instead? 

(untested; just checking if this direction is preferable)
---
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 3570bcb..28310a2 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -847,7 +847,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, 
struct path_selector *ps
 
if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, >flags)) {
 retain:
-   attached_handler_name = scsi_dh_attached_handler_name(q, 
GFP_KERNEL);
+   attached_handler_name = blk_dh_attached_handler_name(q, 
GFP_KERNEL);
if (attached_handler_name) {
/*
 * Clear any hw_handler_params associated with a
@@ -870,7 +870,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, 
struct path_selector *ps
}
 
if (m->hw_handler_name) {
-   r = scsi_dh_attach(q, m->hw_handler_name);
+   r = blk_dh_attach(q, m->hw_handler_name);
if (r == -EBUSY) {
char b[BDEVNAME_SIZE];
 
@@ -885,7 +885,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, 
struct path_selector *ps
}
 
if (m->hw_handler_params) {
-   r = scsi_dh_set_params(q, m->hw_handler_params);
+   r = blk_dh_set_params(q, m->hw_handler_params);
if (r < 0) {
ti->error = "unable to set hardware "
"handler parameters";
@@ -1526,7 +1526,7 @@ static void activate_path(struct work_struct *work)
struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
 
if (pgpath->is_active && !blk_queue_dying(q))
-   scsi_dh_activate(q, pg_init_done, pgpath);
+   blk_dh_activate(q, pg_init_done, pgpath);
else
pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
 }
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 06d2ed4..49a061a 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -107,7 +107,7 @@ static int dpc_probe(struct pcie_device *dev)
int status;
u16 ctl, cap;
 
-   dpc = kzalloc(>device, sizeof(*dpc), GFP_KERNEL);
+   dpc = kzalloc(sizeof(*dpc), GFP_KERNEL);
if (!dpc)
return -ENOMEM;
 
@@ -116,7 +116,7 @@ static int dpc_probe(struct pcie_device *dev)
INIT_WORK(>work, interrupt_event_handler);
set_service_data(dev, dpc);
 
-   status = request_irq(>device, dev->irq, dpc_irq, IRQF_SHARED,
+   status = request_irq(dev->irq, dpc_irq, IRQF_SHARED,
  "pcie-dpc", dpc);
if (status) {
dev_warn(>device, "request IRQ%d failed: %d\n", dev->irq,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9e1e14..e23c7d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2028,6 +2028,13 @@ static u64 scsi_calculate_bounce_limit(struct Scsi_Host 
*shost)
return bounce_limit;
 }
 
+static struct dh_ops scsi_dh_ops = {
+   .dh_activate = scsi_dh_activate,
+   .dh_attach = scsi_dh_attach,
+   .dh_attached_handler_name = scsi_dh_attached_handler_name,
+   .dh_set_params = scsi_dh_set_params,
+};
+
 static void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 {
struct device *dev = shost->dma_dev;
@@ -2062,6 +2069,8 @@ static void __scsi_init_queue(struct Scsi_Host *shost, 
struct request_queue *q)
 * blk_queue_update_dma_alignment() later.
 */
blk_queue_dma_alignment(q, 0x03);
+
+   q->dh_ops = _dh_ops;
 }
 
 struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1ca8e8f..5899768 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -378,6 +378,14 @@ static inline int blkdev_reset_zones_ioctl(struct 
block_device *bdev,
 
 #endif /* CONFIG_BLK_DEV_ZONED */
 
+typedef void (*activate_complete)(void *, int);
+struct dh_ops {
+   int (*dh_activate)(struct request_queue *, activate_complete, void *);
+   int (*dh_attach)(struct request_queue *, const char *);
+   const char *(*dh_attached_handler_name)(struct request_queue *, gfp_t);
+   int (*dh_set_params)(struct request_queue *, const char *);
+};
+
 struct request_queue {
/*
 * Together with 

Re: [PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Bart Van Assche]

On Thu, 2017-02-16 at 16:12 +0100, Hannes Reinecke wrote:
> +struct scsi_device *scsi_device_from_queue(struct request_queue *q)
> +{
> + struct scsi_device *sdev = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + if (q->mq_ops) {
> + if (q->mq_ops == _mq_ops)
> + sdev = q->queuedata;
> + } else if (q->request_fn == scsi_request_fn)
> + sdev = q->queuedata;
> + if (!sdev || !get_device(>sdev_gendev))
> + sdev = NULL;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + return sdev;
> +}

Hello Hannes,

Do we need to take the queue lock? Neither q->mq_ops nor q->request_fn are
modified after a block device has been created. q->queuedata is not modified
by any SCSI driver after it has been set. And since the caller of
scsi_device_from_queue() has to guarantee that the queue does not disappear
while this function is in progress, the queue lock does not have to be held
around the get_device() call either. Otherwise this patch looks fine to me.

Bart.

Re: hch's native NVMe multipathing [was: Re: [PATCH 1/2] Don't blacklist nvme]

[Mike Snitzer]

On Thu, Feb 16 2017 at  9:26am -0500,
Christoph Hellwig  wrote:

> On Wed, Feb 15, 2017 at 09:53:57PM -0500, Mike Snitzer wrote:
> > going to LSF/MM?).  Yet you're expecting to just drop it into the tree
> > without a care in the world about the implications.
> 
> I am planning to post it for comments, and then plan to "drop it in the
> tree" exactly because I think of the implications.
> 
> Keith did that

Not following what you're saying Keith did.  Please feel free to
clarify.

But we definitely need to devise a way for NVMe to inform DM multipath
(and vice-versa): hands off this device.  Awkward details to work
through to be sure...

> But once we already do the discovery of the path
> relations in the transport (e.g scsi_dh) we can just move the path
> selectors (for which I'm reusing the DM code anyway btw) and the
> bouncing of I/O to the block layer and cut out the middle man.

The middle man is useful if it can support all transports.  If it only
supports some then yeah the utility is certainly reduced.

> The main reason is that things will just work (TM) instead of having
> to carry around additional userspace to configure a an unneded
> additional device layer that just causes confusion.  Beyond the
> scsi_dh equivalent there is basically no new code in nvme, 

I'm going to look at removing any scsi_dh code from DM multipath
(someone already proposed removing the 'retain_attached_hw_handler'
feature).  Not much point having anything in DM multipath now that scsi
discovery has the ability to auto-attach the right scsi_dh via scsi_dh's
.match hook.  As a side-effect it will fix Keith's scsi_dh crash (when
operating on NVMe request_queue).

My hope is that your NVMe equivalent for scsi_dh will "just work" (TM)
like scsi_dh auto-attach does.  There isn't a finished ALUA equivalent
standard for NVMe so I'd imagine at this point you have a single device
handler for NVMe to do error translation?

Anyway, the scsi_dh equivalent for NVMe is welcomed news!

> just a little new code in the block layer, and a move of the path
> selectors from dm to the block layer.  I would not call this
> fragmentation.

I'm fine with the path selectors getting moved out; maybe it'll
encourage new path selectors to be developed.

But there will need to be some userspace interface stood up to support
your native NVMe multipathing (you may not think it needed but think in
time there will be a need to configure _something_).  That is the
fragmentation I'm referring to.

> Anyway, there is very little point in having an abstract discussion
> here, I'll try to get the code ready ASAP, although until the end of
> next week I'm really pressed with other deadlines.

OK.

FYI, I never wanted to have an abstract discussion.  We need a real nuts
and bolts discussion.  Happy to have it play out on the lists. 

I'm not violently opposed to your native NVMe multipathing -- especially
from a reference implementation point of view.  I think that in practice
it'll  keep DM multipath honest -- help drive scalability improvements, etc.
If over time the native NVMe multipathing _is_ the preferred multipathing
solution for NVme, then so be it.  It'll be on merits.. as it should be.

But I'm sure you're well aware that I and Red Hat and our partners have
a vested interest in providing a single multipath stack that "just
works" for all appropriate storage.

[PATCHv2] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Hannes Reinecke]

The device handler needs to check if a given queue belongs to
a scsi device; only then does it make sense to attach a device
handler.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/scsi_dh.c | 22 --
 drivers/scsi/scsi_lib.c| 26 ++
 include/scsi/scsi_device.h |  1 +
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index b8d3b97..84addee 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -219,20 +219,6 @@ int scsi_unregister_device_handler(struct 
scsi_device_handler *scsi_dh)
 }
 EXPORT_SYMBOL_GPL(scsi_unregister_device_handler);
 
-static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
-{
-   struct scsi_device *sdev;
-   unsigned long flags;
-
-   spin_lock_irqsave(q->queue_lock, flags);
-   sdev = q->queuedata;
-   if (!sdev || !get_device(>sdev_gendev))
-   sdev = NULL;
-   spin_unlock_irqrestore(q->queue_lock, flags);
-
-   return sdev;
-}
-
 /*
  * scsi_dh_activate - activate the path associated with the scsi_device
  *  corresponding to the given request queue.
@@ -251,7 +237,7 @@ int scsi_dh_activate(struct request_queue *q, 
activate_complete fn, void *data)
struct scsi_device *sdev;
int err = SCSI_DH_NOSYS;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev) {
if (fn)
fn(data, err);
@@ -298,7 +284,7 @@ int scsi_dh_set_params(struct request_queue *q, const char 
*params)
struct scsi_device *sdev;
int err = -SCSI_DH_NOSYS;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev)
return err;
 
@@ -321,7 +307,7 @@ int scsi_dh_attach(struct request_queue *q, const char 
*name)
struct scsi_device_handler *scsi_dh;
int err = 0;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev)
return -ENODEV;
 
@@ -359,7 +345,7 @@ const char *scsi_dh_attached_handler_name(struct 
request_queue *q, gfp_t gfp)
struct scsi_device *sdev;
const char *handler_name = NULL;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev)
return NULL;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9d6aed5..1a81ef9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2145,6 +2145,32 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
blk_mq_free_tag_set(>tag_set);
 }
 
+/**
+ * scsi_device_from_queue - return sdev associated with a request_queue
+ * @q: The request queue to return the sdev from
+ *
+ * Return the sdev associated with a request queue or NULL if the
+ * request_queue does not reference a SCSI device.
+ */
+struct scsi_device *scsi_device_from_queue(struct request_queue *q)
+{
+   struct scsi_device *sdev = NULL;
+   unsigned long flags;
+
+   spin_lock_irqsave(q->queue_lock, flags);
+   if (q->mq_ops) {
+   if (q->mq_ops == _mq_ops)
+   sdev = q->queuedata;
+   } else if (q->request_fn == scsi_request_fn)
+   sdev = q->queuedata;
+   if (!sdev || !get_device(>sdev_gendev))
+   sdev = NULL;
+   spin_unlock_irqrestore(q->queue_lock, flags);
+
+   return sdev;
+}
+EXPORT_SYMBOL_GPL(scsi_device_from_queue);
+
 /*
  * Function:scsi_block_requests()
  *
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..be41c76 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -315,6 +315,7 @@ extern int scsi_add_device(struct Scsi_Host *host, uint 
channel,
 extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh);
 void scsi_attach_vpd(struct scsi_device *sdev);
 
+extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-- 
1.8.5.6

Re: [PATCH] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Hannes Reinecke]

On 02/16/2017 04:07 PM, Hannes Reinecke wrote:
> The device handler needs to check if a given queue belongs to
> a scsi device; only then does it make sense to attach a device
> handler.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/scsi_dh.c | 22 --
>  drivers/scsi/scsi_lib.c| 24 
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
> index b8d3b97..84addee 100644
> --- a/drivers/scsi/scsi_dh.c
> +++ b/drivers/scsi/scsi_dh.c
> @@ -219,20 +219,6 @@ int scsi_unregister_device_handler(struct 
> scsi_device_handler *scsi_dh)
>  }
>  EXPORT_SYMBOL_GPL(scsi_unregister_device_handler);
>  
> -static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
> -{
> - struct scsi_device *sdev;
> - unsigned long flags;
> -
> - spin_lock_irqsave(q->queue_lock, flags);
> - sdev = q->queuedata;
> - if (!sdev || !get_device(>sdev_gendev))
> - sdev = NULL;
> - spin_unlock_irqrestore(q->queue_lock, flags);
> -
> - return sdev;
> -}
> -
>  /*
>   * scsi_dh_activate - activate the path associated with the scsi_device
>   *  corresponding to the given request queue.
> @@ -251,7 +237,7 @@ int scsi_dh_activate(struct request_queue *q, 
> activate_complete fn, void *data)
>   struct scsi_device *sdev;
>   int err = SCSI_DH_NOSYS;
>  
> - sdev = get_sdev_from_queue(q);
> + sdev = scsi_device_from_queue(q);
>   if (!sdev) {
>   if (fn)
>   fn(data, err);
> @@ -298,7 +284,7 @@ int scsi_dh_set_params(struct request_queue *q, const 
> char *params)
>   struct scsi_device *sdev;
>   int err = -SCSI_DH_NOSYS;
>  
> - sdev = get_sdev_from_queue(q);
> + sdev = scsi_device_from_queue(q);
>   if (!sdev)
>   return err;
>  
> @@ -321,7 +307,7 @@ int scsi_dh_attach(struct request_queue *q, const char 
> *name)
>   struct scsi_device_handler *scsi_dh;
>   int err = 0;
>  
> - sdev = get_sdev_from_queue(q);
> + sdev = scsi_device_from_queue(q);
>   if (!sdev)
>   return -ENODEV;
>  
> @@ -359,7 +345,7 @@ const char *scsi_dh_attached_handler_name(struct 
> request_queue *q, gfp_t gfp)
>   struct scsi_device *sdev;
>   const char *handler_name = NULL;
>  
> - sdev = get_sdev_from_queue(q);
> + sdev = scsi_device_from_queue(q);
>   if (!sdev)
>   return NULL;
>  
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 9d6aed5..9c732d3 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2145,6 +2145,30 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>   blk_mq_free_tag_set(>tag_set);
>  }
>  
> +/**
> + * scsi_device_from_queue - return sdev associated with a request_queue
> + * @q: The request queue to return the sdev from
> + *
> + * Return the sdev associated with a request queue or NULL if the
> + * request_queue does not reference a SCSI device.
> + */
> +struct scsi_device *scsi_device_from_queue(struct request_queue *q)
> +{
> + struct scsi_device *sdev = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + if (q->mq_ops) {
> + if (q->mq_ops == _mq_ops)
> + sdev = q->queuedata;
> + } else if (q->request_fn == scsi_request_fn)
> + sdev = q->queuedata;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + return sdev;
> +}
> +EXPORT_SYMBOL_GPL(scsi_device_from_queue);
> +
>  /*
>   * Function:scsi_block_requests()
>   *
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 8990e58..be41c76 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -315,6 +315,7 @@ extern int scsi_add_device(struct Scsi_Host *host, uint 
> channel,
>  extern int scsi_unregister_device_handler(struct scsi_device_handler 
> *scsi_dh);
>  void scsi_attach_vpd(struct scsi_device *sdev);
>  
> +extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
>  extern int scsi_device_get(struct scsi_device *);
>  extern void scsi_device_put(struct scsi_device *);
>  extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
> 
fsck. Forgot the 'get_device' thingie.

Will be reposting shortly.

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)

[PATCH] scsi: use 'scsi_device_from_queue()' for scsi_dh

[Hannes Reinecke]

The device handler needs to check if a given queue belongs to
a scsi device; only then does it make sense to attach a device
handler.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/scsi_dh.c | 22 --
 drivers/scsi/scsi_lib.c| 24 
 include/scsi/scsi_device.h |  1 +
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index b8d3b97..84addee 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -219,20 +219,6 @@ int scsi_unregister_device_handler(struct 
scsi_device_handler *scsi_dh)
 }
 EXPORT_SYMBOL_GPL(scsi_unregister_device_handler);
 
-static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
-{
-   struct scsi_device *sdev;
-   unsigned long flags;
-
-   spin_lock_irqsave(q->queue_lock, flags);
-   sdev = q->queuedata;
-   if (!sdev || !get_device(>sdev_gendev))
-   sdev = NULL;
-   spin_unlock_irqrestore(q->queue_lock, flags);
-
-   return sdev;
-}
-
 /*
  * scsi_dh_activate - activate the path associated with the scsi_device
  *  corresponding to the given request queue.
@@ -251,7 +237,7 @@ int scsi_dh_activate(struct request_queue *q, 
activate_complete fn, void *data)
struct scsi_device *sdev;
int err = SCSI_DH_NOSYS;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev) {
if (fn)
fn(data, err);
@@ -298,7 +284,7 @@ int scsi_dh_set_params(struct request_queue *q, const char 
*params)
struct scsi_device *sdev;
int err = -SCSI_DH_NOSYS;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev)
return err;
 
@@ -321,7 +307,7 @@ int scsi_dh_attach(struct request_queue *q, const char 
*name)
struct scsi_device_handler *scsi_dh;
int err = 0;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev)
return -ENODEV;
 
@@ -359,7 +345,7 @@ const char *scsi_dh_attached_handler_name(struct 
request_queue *q, gfp_t gfp)
struct scsi_device *sdev;
const char *handler_name = NULL;
 
-   sdev = get_sdev_from_queue(q);
+   sdev = scsi_device_from_queue(q);
if (!sdev)
return NULL;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9d6aed5..9c732d3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2145,6 +2145,30 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
blk_mq_free_tag_set(>tag_set);
 }
 
+/**
+ * scsi_device_from_queue - return sdev associated with a request_queue
+ * @q: The request queue to return the sdev from
+ *
+ * Return the sdev associated with a request queue or NULL if the
+ * request_queue does not reference a SCSI device.
+ */
+struct scsi_device *scsi_device_from_queue(struct request_queue *q)
+{
+   struct scsi_device *sdev = NULL;
+   unsigned long flags;
+
+   spin_lock_irqsave(q->queue_lock, flags);
+   if (q->mq_ops) {
+   if (q->mq_ops == _mq_ops)
+   sdev = q->queuedata;
+   } else if (q->request_fn == scsi_request_fn)
+   sdev = q->queuedata;
+   spin_unlock_irqrestore(q->queue_lock, flags);
+
+   return sdev;
+}
+EXPORT_SYMBOL_GPL(scsi_device_from_queue);
+
 /*
  * Function:scsi_block_requests()
  *
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..be41c76 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -315,6 +315,7 @@ extern int scsi_add_device(struct Scsi_Host *host, uint 
channel,
 extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh);
 void scsi_attach_vpd(struct scsi_device *sdev);
 
+extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-- 
1.8.5.6

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Hannes Reinecke]

On 02/16/2017 03:38 PM, h...@lst.de wrote:
> On Thu, Feb 16, 2017 at 02:30:34PM +, Bart Van Assche wrote:
>>> sdev = q->queuedata;
>>> -   if (!sdev || !get_device(>sdev_gendev))
>>> +   if (!sdev ||
>>> +   !scsi_is_sdev_device(>sdev_gendev) ||
>>> +   !get_device(>sdev_gendev))
>>> sdev = NULL;
>>> spin_unlock_irqrestore(q->queue_lock, flags);
>>
>> Hello Hannes,
>>
>> Sorry but this approach looks wrong to me. A block driver can store any data
>> in .queuedata, even data that would cause the scsi_is_sdev_device() function
>> to crash.
> 
> Yes, I fear you're right.  I guess we need to call into the scsi_dh_*
> functions through an indirection mediated by the block layer.
> 
Okay, will be tooling it up.

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] scsi_dh: only attach to SCSI devices

[h...@lst.de]

On Thu, Feb 16, 2017 at 02:30:34PM +, Bart Van Assche wrote:
> > sdev = q->queuedata;
> > -   if (!sdev || !get_device(>sdev_gendev))
> > +   if (!sdev ||
> > +   !scsi_is_sdev_device(>sdev_gendev) ||
> > +   !get_device(>sdev_gendev))
> > sdev = NULL;
> > spin_unlock_irqrestore(q->queue_lock, flags);
> 
> Hello Hannes,
> 
> Sorry but this approach looks wrong to me. A block driver can store any data
> in .queuedata, even data that would cause the scsi_is_sdev_device() function
> to crash.

Yes, I fear you're right.  I guess we need to call into the scsi_dh_*
functions through an indirection mediated by the block layer.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Bart Van Assche]

On Thu, 2017-02-16 at 15:32 +0100, Christoph Hellwig wrote:
> On Thu, Feb 16, 2017 at 08:33:14AM +0100, Hannes Reinecke wrote:
> > Any device might be setting a queuedata structure, so we need to
> > check if the queuedata really belongs to a SCSI device before
> > proceeding.
> 
> Can you add a comment next to the scsi_is_sdev_device call explaining
> it?  The thing is a bit of a hack, but probably the best we can do for
> now.

A much better approach would be to change the type of the argument of
get_sdev_from_queue().

Bart.

Re: sense handling improvements

[Christoph Hellwig]

On Wed, Feb 15, 2017 at 10:42:56PM -0500, Martin K. Petersen wrote:
> Christoph> The first patch prevents any possibily of reusing stale sense
> Christoph> codes in sense headers, and is a bug fix that we should
> Christoph> probably get into the block tree ASAP.
> 
> Christoph> The rest cleans up handling of the parsed sense data and
> Christoph> could go in either through the block tree, or a SCSI branch
> Christoph> on top of the block tree.
> 
> I can bring them in after Linus' initial block pull.

That would be great.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Christoph Hellwig]

On Thu, Feb 16, 2017 at 08:33:14AM +0100, Hannes Reinecke wrote:
> Any device might be setting a queuedata structure, so we need to
> check if the queuedata really belongs to a SCSI device before
> proceeding.

Can you add a comment next to the scsi_is_sdev_device call explaining
it?  The thing is a bit of a hack, but probably the best we can do for
now.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Bart Van Assche]

On Thu, 2017-02-16 at 08:33 +0100, Hannes Reinecke wrote:
> Any device might be setting a queuedata structure, so we need to
> check if the queuedata really belongs to a SCSI device before
> proceeding.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/scsi_dh.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
> index b8d3b97..da104ed 100644
> --- a/drivers/scsi/scsi_dh.c
> +++ b/drivers/scsi/scsi_dh.c
> @@ -226,7 +226,9 @@ static struct scsi_device *get_sdev_from_queue(struct 
> request_queue *q)
>  
>   spin_lock_irqsave(q->queue_lock, flags);
>   sdev = q->queuedata;
> - if (!sdev || !get_device(>sdev_gendev))
> + if (!sdev ||
> + !scsi_is_sdev_device(>sdev_gendev) ||
> + !get_device(>sdev_gendev))
>   sdev = NULL;
>   spin_unlock_irqrestore(q->queue_lock, flags);

Hello Hannes,

Sorry but this approach looks wrong to me. A block driver can store any data
in .queuedata, even data that would cause the scsi_is_sdev_device() function
to crash.

Bart.

Re: [PATCH RFC] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

Hi James,

have you noticed this patch?


Dne 7.2.2017 v 15:08 Maurizio Lombardi napsal(a):
> With multipath, it may happen that the same device is passed
> to enclosure_add_device() multiple times and that the enclosure_add_links()
> function fails to create the symlinks because the device's sysfs
> directory entry is still NULL.
> In this case, the links will never be created because all the subsequent
> calls to enclosure_add_device() will immediately fail with EEXIST.
> 
> This patch modifies the code so the driver will detect this condition
> and will retry to create the symlinks when enclosure_add_device() is called.
> 
> Signed-off-by: Maurizio Lombardi 
> ---
>  drivers/misc/enclosure.c  | 16 ++--
>  include/linux/enclosure.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 65fed71..a856c98 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev, 
> int component,
>struct device *dev)
>  {
>   struct enclosure_component *cdev;
> + int error;
>  
>   if (!edev || component >= edev->components)
>   return -EINVAL;
>  
>   cdev = >component[component];
>  
> - if (cdev->dev == dev)
> + if (cdev->dev == dev) {
> + if (!cdev->links_created) {
> + error = enclosure_add_links(cdev);
> + if (!error)
> + cdev->links_created = 1;
> + }
>   return -EEXIST;
> + }
>  
>   if (cdev->dev)
>   enclosure_remove_links(cdev);
>  
>   put_device(cdev->dev);
>   cdev->dev = get_device(dev);
> - return enclosure_add_links(cdev);
> + error = enclosure_add_links(cdev);
> + if (!error)
> + cdev->links_created = 1;
> + else
> + cdev->links_created = 0;
> + return error;
>  }
>  EXPORT_SYMBOL_GPL(enclosure_add_device);
>  
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index a4cf57c..c3bdc4c 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -97,6 +97,7 @@ struct enclosure_component {
>   struct device cdev;
>   struct device *dev;
>   enum enclosure_component_type type;
> + int links_created;
>   int number;
>   int fault;
>   int active;
> 

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

[Kashyap Desai]

> > - Later we can explore if nr_hw_queue more than one really add benefit.
> > From current limited testing, I don't see major performance boost if
> > we have nr_hw_queue more than one.
> >
> Well, the _actual_ code to support mq is rather trivial, and really serves
> as a
> good testbed for scsi-mq.
> I would prefer to leave it in, and disable it via a module parameter.

I am thinking as adding extra code for more than one nr_hw_queue will add
maintenance overhead and support. Especially IO error handling code become
complex with nr_hw_queues > 1 case.  If we really like to see performance
boost, we should attempt and bare other side effect.

For time being we should drop this nr_hw_queue > 1 support is what I choose
(not even module parameter base).

>
> But in either case, I can rebase the patches to leave any notions of
> 'nr_hw_queues' to patch 8 for implementing full mq support.

Thanks Hannes. It was just heads up...We are not sure when we can submit
upcoming patch set from Broadcom. May be we can syncup with you offline in
case any rebase requires.

>
> And we need to discuss how to handle MPI2_FUNCTION_SCSI_IO_REQUEST;
> the current method doesn't work with blk-mq.
> I really would like to see that go, especially as sg/bsg supports the same
> functionality ...
>

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

[Hannes Reinecke]

On 02/16/2017 11:23 AM, Sreekanth Reddy wrote:
> On Thu, Feb 16, 2017 at 3:44 PM, Hannes Reinecke  wrote:
>> On 02/16/2017 11:09 AM, Sreekanth Reddy wrote:
>>> 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);
>>>
>> 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.
> 
Ok, that makes things easier. Will be updating the code.

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 Thu, Feb 16, 2017 at 3:44 PM, Hannes Reinecke  wrote:
> On 02/16/2017 11:09 AM, Sreekanth Reddy wrote:
>> 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);
>>
> 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 00/10] mpt3sas: full mq support

[Hannes Reinecke]

On 02/16/2017 10:48 AM, Kashyap Desai wrote:
>> -Original Message-
>> From: Hannes Reinecke [mailto:h...@suse.de]
>> Sent: Wednesday, February 15, 2017 3:35 PM
>> To: Kashyap Desai; Sreekanth Reddy
>> Cc: Christoph Hellwig; Martin K. Petersen; James Bottomley; linux-
>> s...@vger.kernel.org; Sathya Prakash Veerichetty; PDL-MPT-FUSIONLINUX
>> Subject: Re: [PATCH 00/10] mpt3sas: full mq support
>>
>> On 02/15/2017 10:18 AM, Kashyap Desai wrote:


 Hannes,

 Result I have posted last time is with merge operation enabled in
 block layer. If I disable merge operation then I don't see much
 improvement with multiple hw request queues. Here is the result,

 fio results when nr_hw_queues=1,
 4k read when numjobs=24: io=248387MB, bw=1655.1MB/s, iops=423905,
 runt=150003msec

 fio results when nr_hw_queues=24,
 4k read when numjobs=24: io=263904MB, bw=1759.4MB/s, iops=450393,
 runt=150001msec
>>>
>>> Hannes -
>>>
>>>  I worked with Sreekanth and also understand pros/cons of Patch #10.
>>> " [PATCH 10/10] mpt3sas: scsi-mq interrupt steering"
>>>
>>> In above patch, can_queue of HBA is divided based on logic CPU, it
>>> means we want to mimic as if mpt3sas HBA support multi queue
>>> distributing actual resources which is single Submission H/W Queue.
>>> This approach badly impact many performance areas.
>>>
>>> nr_hw_queues = 1 is what I observe as best performance approach since
>>> it never throttle IO if sdev->queue_depth is set to HBA queue depth.
>>> In case of nr_hw_queues = "CPUs" throttle IO at SCSI level since we
>>> never allow more than "updated can_queue" in LLD.
>>>
>> True.
>> And this was actually one of the things I wanted to demonstrate with this
>> patchset :-) ATM blk-mq really works best when having a distinct tag space
>> per port/device. As soon as the hardware provides a _shared_ tag space you
>> end up with tag starvation issues as blk-mq only allows you to do a static
>> split of the available tagspace.
>> While this patchset demonstrates that the HBA itself _does_ benefit from
>> using block-mq (especially on highly parallel loads), it also demonstrates
>> that
>> _block-mq_ has issues with singlethreaded loads on this HBA (or, rather,
>> type of HBA, as I doubt this issue is affecting mpt3sas only).
>>
>>> Below code bring actual HBA can_queue very low ( Ea on 96 logical core
>>> CPU new can_queue goes to 42, if HBA queue depth is 4K). It means we
>>> will see lots of IO throttling in scsi mid layer due to
>>> shost->can_queue reach the limit very soon if you have  jobs with
>> higher QD.
>>>
>>> if (ioc->shost->nr_hw_queues > 1) {
>>> ioc->shost->nr_hw_queues = ioc->msix_vector_count;
>>> ioc->shost->can_queue /= ioc->msix_vector_count;
>>> }
>>> I observe negative performance if I have 8 SSD drives attached to
>>> Ventura (latest IT controller). 16 fio jobs at QD=128 gives ~1600K
>>> IOPs and the moment I switch to nr_hw_queues = "CPUs", it gave hardly
>>> ~850K IOPs. This is mainly because of host_busy stuck at very low ~169
>>> on
>> my setup.
>>>
>> Which actually might be an issue with the way scsi is hooked into blk-mq.
>> The SCSI stack is using 'can_queue' as a check for 'host_busy', ie if the
>> host is
>> capable of accepting more commands.
>> As we're limiting can_queue (to get the per-queue command depth
>> correctly) we should be using the _overall_ command depth for the
>> can_queue value itself to make the host_busy check work correctly.
>>
>> I've attached a patch for that; can you test if it makes a difference?
> Hannes -
> Attached patch works fine for me. FYI -  We need to set device queue depth
> to can_queue as we are currently not doing in mpt3sas driver.
> 
> With attached patch when I tried, I see ~2-3% improvement running multiple
> jobs. Single job profile no difference.
> 
> So looks like we are good to reach performance with single nr_hw_queues.
> 
Whee, cool.

> We have some patches to be send so want to know how to rebase this patch
> series as few patches coming from Broadcom. Can we consider below as plan ?
> 
Sure, can do.

> - Patches from 1-7 will be reposted. Also Sreekanth will complete review on
> existing patch 1-7.
> - We need blk_tag support only for nr_hw_queue = 1.
> 
> With that say, we will have many code changes/function without "
> shost_use_blk_mq" check and assume it is single nr_hw_queue supported
>  driver.
> 
> Ea - Below function can be simplify - just refer tag from scmd->request and
> don't need check of shost_use_blk_mq + nr_hw_queue etc..
> 
> u16
> mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
> struct scsi_cmnd *scmd)
> {
> struct scsiio_tracker *request;
> u16 smid = scmd->request->tag +  1;
>  ...
>  return request->smid;
> }
> 
> - Later we can explore if nr_hw_queue more than one really add benefit.
> From current limited testing, I don't see major performance boost 

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

[Hannes Reinecke]

On 02/16/2017 11:09 AM, Sreekanth Reddy wrote:
> 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);
> 
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.

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()

[Hannes Reinecke]

On 02/16/2017 10:59 AM, Sreekanth Reddy wrote:
> 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).
> 
Correct, I just noticed it.
Will be rebasing it.

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 04/10] mpt3sas: separate out _base_recovery_check()

[Hannes Reinecke]

On 02/16/2017 10:53 AM, Sreekanth Reddy wrote:
> 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.
> 
Ouch, indeed. My fault.

Will be fixing it up.

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 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 00/10] mpt3sas: full mq support

[Kashyap Desai]

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Wednesday, February 15, 2017 3:35 PM
> To: Kashyap Desai; Sreekanth Reddy
> Cc: Christoph Hellwig; Martin K. Petersen; James Bottomley; linux-
> s...@vger.kernel.org; Sathya Prakash Veerichetty; PDL-MPT-FUSIONLINUX
> Subject: Re: [PATCH 00/10] mpt3sas: full mq support
>
> On 02/15/2017 10:18 AM, Kashyap Desai wrote:
> >>
> >>
> >> Hannes,
> >>
> >> Result I have posted last time is with merge operation enabled in
> >> block layer. If I disable merge operation then I don't see much
> >> improvement with multiple hw request queues. Here is the result,
> >>
> >> fio results when nr_hw_queues=1,
> >> 4k read when numjobs=24: io=248387MB, bw=1655.1MB/s, iops=423905,
> >> runt=150003msec
> >>
> >> fio results when nr_hw_queues=24,
> >> 4k read when numjobs=24: io=263904MB, bw=1759.4MB/s, iops=450393,
> >> runt=150001msec
> >
> > Hannes -
> >
> >  I worked with Sreekanth and also understand pros/cons of Patch #10.
> > " [PATCH 10/10] mpt3sas: scsi-mq interrupt steering"
> >
> > In above patch, can_queue of HBA is divided based on logic CPU, it
> > means we want to mimic as if mpt3sas HBA support multi queue
> > distributing actual resources which is single Submission H/W Queue.
> > This approach badly impact many performance areas.
> >
> > nr_hw_queues = 1 is what I observe as best performance approach since
> > it never throttle IO if sdev->queue_depth is set to HBA queue depth.
> > In case of nr_hw_queues = "CPUs" throttle IO at SCSI level since we
> > never allow more than "updated can_queue" in LLD.
> >
> True.
> And this was actually one of the things I wanted to demonstrate with this
> patchset :-) ATM blk-mq really works best when having a distinct tag space
> per port/device. As soon as the hardware provides a _shared_ tag space you
> end up with tag starvation issues as blk-mq only allows you to do a static
> split of the available tagspace.
> While this patchset demonstrates that the HBA itself _does_ benefit from
> using block-mq (especially on highly parallel loads), it also demonstrates
> that
> _block-mq_ has issues with singlethreaded loads on this HBA (or, rather,
> type of HBA, as I doubt this issue is affecting mpt3sas only).
>
> > Below code bring actual HBA can_queue very low ( Ea on 96 logical core
> > CPU new can_queue goes to 42, if HBA queue depth is 4K). It means we
> > will see lots of IO throttling in scsi mid layer due to
> > shost->can_queue reach the limit very soon if you have  jobs with
> higher QD.
> >
> > if (ioc->shost->nr_hw_queues > 1) {
> > ioc->shost->nr_hw_queues = ioc->msix_vector_count;
> > ioc->shost->can_queue /= ioc->msix_vector_count;
> > }
> > I observe negative performance if I have 8 SSD drives attached to
> > Ventura (latest IT controller). 16 fio jobs at QD=128 gives ~1600K
> > IOPs and the moment I switch to nr_hw_queues = "CPUs", it gave hardly
> > ~850K IOPs. This is mainly because of host_busy stuck at very low ~169
> > on
> my setup.
> >
> Which actually might be an issue with the way scsi is hooked into blk-mq.
> The SCSI stack is using 'can_queue' as a check for 'host_busy', ie if the
> host is
> capable of accepting more commands.
> As we're limiting can_queue (to get the per-queue command depth
> correctly) we should be using the _overall_ command depth for the
> can_queue value itself to make the host_busy check work correctly.
>
> I've attached a patch for that; can you test if it makes a difference?
Hannes -
Attached patch works fine for me. FYI -  We need to set device queue depth
to can_queue as we are currently not doing in mpt3sas driver.

With attached patch when I tried, I see ~2-3% improvement running multiple
jobs. Single job profile no difference.

So looks like we are good to reach performance with single nr_hw_queues.

We have some patches to be send so want to know how to rebase this patch
series as few patches coming from Broadcom. Can we consider below as plan ?

- Patches from 1-7 will be reposted. Also Sreekanth will complete review on
existing patch 1-7.
- We need blk_tag support only for nr_hw_queue = 1.

With that say, we will have many code changes/function without "
shost_use_blk_mq" check and assume it is single nr_hw_queue supported
 driver.

Ea - Below function can be simplify - just refer tag from scmd->request and
don't need check of shost_use_blk_mq + nr_hw_queue etc..

u16
mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
struct scsi_cmnd *scmd)
{
struct scsiio_tracker *request;
u16 smid = scmd->request->tag +  1;
 ...
 return request->smid;
}

- Later we can explore if nr_hw_queue more than one really add benefit.
>From current limited testing, I don't see major performance boost if we have
nr_hw_queue more than one.

>
> > May be as Sreekanth mentioned, performance improvement you have
> > observed is due to nomerges=2 is not set and OS will attempt 

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 10/16] aacraid: Terminate kthread on controller fw assert

[Johannes Thumshirn]

On 02/15/2017 11:22 PM, Raghava Aditya Renukunta wrote:
>>
>> This look a bit scary. Can't the kthread be converted to a workqueue so
>> we could call cancel_work_sync()?
> 
> Could you please elaborate on the reasons why this fix is scary?
> I understand that killing a thread is not standard (for any reason), 
> and if there are other nuanced issues I would like to understand them.

I'm actually concerned that this could have all kinds of side effects.
But this is just a gut feeling. I see some drm drivers are doing the
same, so it might be possible, but IMHO this is not a good design.

And IIRC kthreads do have more downsides (i.e. CPU hotplugging and
issues with kernel live patching).

I think most kthreads (haven't looked too close to the aacraid kthread I
must admit, but I'll be doing so) can be converted to either workqueues
or timers (or a combination of both).

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