RE: [PATCH] [SCSI] bfa: fix missing unlock on error in bfad_iocmd_cfg_trunk()
-Original Message- From: Wei Yongjun [mailto:weiyj...@gmail.com] Sent: Friday, December 20, 2013 8:21 AM To: Anil Gurumurthy; Vijaya Mohan Guvva; jbottom...@parallels.com Cc: yongjun_...@trendmicro.com.cn; linux-scsi@vger.kernel.org Subject: [PATCH] [SCSI] bfa: fix missing unlock on error in bfad_iocmd_cfg_trunk() From: Wei Yongjun yongjun_...@trendmicro.com.cn Add the missing unlock before return from function bfad_iocmd_cfg_trunk() in the error handling case. Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn --- drivers/scsi/bfa/bfad_bsg.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Looks fine, thanks, Vijay Acked-by: Vijaya Mohan Guvva vmo...@brocade.com -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bfa: Fix smatch warnings
From: Vijaya Mohan Guvva vgu...@redhat.com
Fixed following smatch warnings in bfa.
drivers/scsi/bfa/bfa_ioc.c:3882 bfa_sfp_show_comp() error: memcpy()
'des' too small (64 vs 248)
drivers/scsi/bfa/bfa_ioc.c:6859 bfa_flash_status_read() warn: unsigned
'status' is never less than zero.
drivers/scsi/bfa/bfa_ioc.c:6881 bfa_flash_status_read() warn: unsigned
'status' is never less than zero.
drivers/scsi/bfa/bfa_ioc.c:6917 bfa_flash_read_start() warn: unsigned
'status' is never less than zero.
drivers/scsi/bfa/bfa_ioc.c:7043 bfa_flash_raw_read() warn: unsigned
'status' is never less than zero.
Signed-off-by: Vijaya Mohan Guvva vmo...@brocade.com
---
drivers/scsi/bfa/bfa_ioc.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 65180e1..7e256ef 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -3878,7 +3878,7 @@ bfa_sfp_show_comp(struct bfa_sfp_s *sfp, struct
bfi_mbmsg_s *msg)
bfa_trc(sfp, sfp-data_valid);
if (sfp-data_valid) {
u32 size = sizeof(struct sfp_mem_s);
- u8 *des = (u8 *) (sfp-sfpmem-srlid_base);
+ u8 *des = (u8 *) (sfp-sfpmem);
memcpy(des, sfp-dbuf_kva, size);
}
/*
@@ -6851,7 +6851,7 @@ static u32
bfa_flash_status_read(void __iomem *pci_bar)
{
union bfa_flash_dev_status_reg_udev_status;
- u32 status;
+ int status;
u32 ret_status;
int i;
@@ -6899,7 +6899,7 @@ static u32
bfa_flash_read_start(void __iomem *pci_bar, u32 offset, u32 len,
char *buf)
{
- u32 status;
+ int status;
/*
* len must be mutiple of 4 and not exceeding fifo size
@@ -7021,7 +7021,8 @@ bfa_status_t
bfa_flash_raw_read(void __iomem *pci_bar, u32 offset, char *buf,
u32 len)
{
- u32 n, status;
+ u32 n;
+ int status;
u32 off, l, s, residue, fifo_sz;
residue = len;
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] aacraid: kdump fix
This patch fixes kernel panic issue while booting into the kdump kernel.
We have triggered crash and kdump vmcore was successful. No issues seen while
booting into the OS.
Signed-off-by: Mahesh Rajashekhara mahesh.rajashekh...@pmcs.com
---
drivers/scsi/aacraid/aacraid.h |2 +-
drivers/scsi/aacraid/rx.c |5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 9323d05..eaaf870 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -12,7 +12,7 @@
**/
#ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 30200
+# define AAC_DRIVER_BUILD 30300
# define AAC_DRIVER_BRANCH -ms
#endif
#define MAXIMUM_NUM_CONTAINERS 32
diff --git a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
index dada38a..9f2d88f 100644
--- a/drivers/scsi/aacraid/rx.c
+++ b/drivers/scsi/aacraid/rx.c
@@ -500,13 +500,14 @@ static int aac_rx_restart_adapter(struct aac_dev *dev,
int bled)
if (bled (bled != -ETIMEDOUT))
return -EINVAL;
}
- if (bled || (var == 0x3803000F)) { /* USE_OTHER_METHOD */
+ if (bled (var == 0x3803000F)) { /* USE_OTHER_METHOD */
rx_writel(dev, MUnit.reserved2, 3);
msleep(5000); /* Delay 5 seconds */
var = 0x0001;
}
- if (var != 0x0001)
+ if (bled (var != 0x0001))
return -EINVAL;
+ ssleep(5);
if (rx_readl(dev, MUnit.OMRx[0]) KERNEL_PANIC)
return -ENODEV;
if (startup_timeout 300)
--
1.7.7.3
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pm80xx: Spinlock fix
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00 2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave instead
of spin_unlock_irq
why the change to global lock_flags? I'm not a spinlock expert, but is this
safe?
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
- unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003
#define SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004
@@ -425,10 +424,10 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N (N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
- spin_lock_irqsave(pm8001_ha-lock, flags);
+ spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
- spin_unlock_irqrestore(pm8001_ha-lock, flags);
+ spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
- spin_unlock_irqrestore(pm8001_ha-lock, flags);
+ spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -466,7 +465,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/*set the shifted destination address to 0x0 to avoid error operation */
pm8001_bar4_shift(pm8001_ha, 0x0);
- spin_unlock_irqrestore(pm8001_ha-lock, flags);
+ spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -481,7 +480,6 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
u32 offset;
u32 value;
u32 i;
- unsigned long flags;
#define OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR 0x0003
#define OPEN_RETRY_INTERVAL_PHY_4_7_SHIFT_ADDR 0x0004
@@ -490,11 +488,11 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
#define OPEN_RETRY_INTERVAL_REG_MASK 0x
value = interval OPEN_RETRY_INTERVAL_REG_MASK;
- spin_lock_irqsave(pm8001_ha-lock, flags);
+ spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
/* shift bar and set the OPEN_REJECT(RETRY) interval time of PHY 0 -3.*/
if (-1 == pm8001_bar4_shift(pm8001_ha,
OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR)) {
- spin_unlock_irqrestore(pm8001_ha-lock, flags);
+ spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 0; i 4; i++) {
@@ -504,7 +502,7 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
if (-1 == pm8001_bar4_shift(pm8001_ha,
OPEN_RETRY_INTERVAL_PHY_4_7_SHIFT_ADDR)) {
- spin_unlock_irqrestore(pm8001_ha-lock, flags);
+ spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -513,7 +511,7 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
}
/*set the shifted destination address to 0x0 to avoid error operation */
pm8001_bar4_shift(pm8001_ha, 0x0);
- spin_unlock_irqrestore(pm8001_ha-lock, flags);
+ spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -768,11 +766,11 @@ static u32 soft_reset_ready_check(struct
Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00 2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave instead
of spin_unlock_irq
why the change to global lock_flags? I'm not a spinlock expert, but is this
safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if
that is the cause.
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003
#define SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004
@@ -425,10 +424,10 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N (N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -466,7 +465,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/*set the shifted destination address to 0x0 to avoid error operation */
pm8001_bar4_shift(pm8001_ha, 0x0);
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -481,7 +480,6 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
u32 offset;
u32 value;
u32 i;
-unsigned long flags;
#define OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR 0x0003
#define OPEN_RETRY_INTERVAL_PHY_4_7_SHIFT_ADDR 0x0004
@@ -490,11 +488,11 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
#define OPEN_RETRY_INTERVAL_REG_MASK 0x
value = interval OPEN_RETRY_INTERVAL_REG_MASK;
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
/* shift bar and set the OPEN_REJECT(RETRY) interval time of PHY 0 -3.*/
if (-1 == pm8001_bar4_shift(pm8001_ha,
OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 0; i 4; i++) {
@@ -504,7 +502,7 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
if (-1 == pm8001_bar4_shift(pm8001_ha,
OPEN_RETRY_INTERVAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -513,7 +511,7 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
}
/*set the shifted destination address to 0x0 to avoid error operation */
[Bug 36742] Fusion MPT2SAS driver fails to suspend to S3 incorrectly, suspend to disk works.
https://bugzilla.kernel.org/show_bug.cgi?id=36742 Alan a...@lxorguk.ukuu.org.uk changed: What|Removed |Added Status|NEW |RESOLVED CC||a...@lxorguk.ukuu.org.uk Resolution|--- |OBSOLETE -- You are receiving this mail because: You are watching the assignee of the bug. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] pm80xx: Spinlock fix
-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com]
Sent: Monday, December 23, 2013 7:03 PM
To: Tomas Henzl; Viswas G
Cc: linux-scsi@vger.kernel.org; jason.seb...@gmail.com;
jbottom...@parallels.com; Vasanthalakshmi Tharmarajan; Suresh Thiagarajan
Subject: Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00
2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave
instead of spin_unlock_irq why the change to global lock_flags? I'm not a
spinlock expert, but is this safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if that is
the cause.
Suresh: We could not reproduce this issue and Jason(in CC) also could not
reproduce it for now. spin_lock_irqsave and spin_unlock_irqrestore were called
from multiple functions. Earlier flags was declared as a local variable
wherever spinlock was used. This was not correct since irq information was
saved in one function's flag which is a local to that function and restored in
other function where again flags was declared as local variable to that
function. So the data stored in flags while irq save was not used while
restoring. Since we have lock per card, we are associating flag also per card
for that lock.
-Suresh
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -425,10 +424,10
@@ static void mpi_set_phys_g3_with_ssc(struct pm8001_hba_info *pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N (N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -466,7 +465,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/*set the shifted destination address to 0x0 to avoid error operation */
pm8001_bar4_shift(pm8001_ha, 0x0);
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -481,7 +480,6 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
u32 offset;
u32 value;
u32 i;
-unsigned long flags;
#define OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
OPEN_RETRY_INTERVAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -490,11 +488,11
@@ static void mpi_set_open_retry_interval_reg(struct pm8001_hba_info
*pm8001_ha, #define OPEN_RETRY_INTERVAL_REG_MASK 0x
value = interval OPEN_RETRY_INTERVAL_REG_MASK;
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
/* shift bar and set the OPEN_REJECT(RETRY) interval time of PHY 0
Re: status of block-integrity
On Mon, Dec 23, 2013 at 08:35:22AM -0500, Martin K. Petersen wrote:
Christoph == Christoph Hellwig h...@infradead.org writes:
Christoph We have the block integrity code to support DIF/DIX in the
Christoph the tree for about 5 and a half years, and we still don't
Christoph have a single consumer of it.
What do you mean? If you have a DIX-capable HBA (lpfc, qla2xxx, zfcp)
then integrity protection is active from the block layer down. The only
code that's not currently being exercised are the tag interleaving
functions. I was hoping the FS people would use them for back pointers
but nobody seemed to bite.
With single consumer of it I obviously meant the various symbols for the
consumer side as well as the application tag support.
Patch to remove the dead code below:
---
From: Christoph Hellwig h...@lst.de
Subject: [PATCH] block: remove dead on arrival integrity code
Signed-off-by: Christoph Hellwig h...@lst.de
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 03cf717..0b14db7 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -194,7 +194,6 @@ int blk_integrity_merge_rq(struct request_queue *q, struct
request *req,
return 0;
}
-EXPORT_SYMBOL(blk_integrity_merge_rq);
int blk_integrity_merge_bio(struct request_queue *q, struct request *req,
struct bio *bio)
@@ -214,7 +213,6 @@ int blk_integrity_merge_bio(struct request_queue *q, struct
request *req,
return 0;
}
-EXPORT_SYMBOL(blk_integrity_merge_bio);
struct integrity_sysfs_entry {
struct attribute attr;
@@ -414,8 +412,6 @@ int blk_integrity_register(struct gendisk *disk, struct
blk_integrity *template)
bi-generate_fn = template-generate_fn;
bi-verify_fn = template-verify_fn;
bi-tuple_size = template-tuple_size;
- bi-set_tag_fn = template-set_tag_fn;
- bi-get_tag_fn = template-get_tag_fn;
bi-tag_size = template-tag_size;
} else
bi-name = bi_unsupported_name;
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 6174ca4..e32035a 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -128,39 +128,10 @@ static int sd_dif_type1_verify_ip(struct
blk_integrity_exchg *bix)
return sd_dif_type1_verify(bix, sd_dif_ip_fn);
}
-/*
- * Functions for interleaving and deinterleaving application tags
- */
-static void sd_dif_type1_set_tag(void *prot, void *tag_buf, unsigned int
sectors)
-{
- struct sd_dif_tuple *sdt = prot;
- u8 *tag = tag_buf;
- unsigned int i, j;
-
- for (i = 0, j = 0 ; i sectors ; i++, j += 2, sdt++) {
- sdt-app_tag = tag[j] 8 | tag[j+1];
- BUG_ON(sdt-app_tag == 0x);
- }
-}
-
-static void sd_dif_type1_get_tag(void *prot, void *tag_buf, unsigned int
sectors)
-{
- struct sd_dif_tuple *sdt = prot;
- u8 *tag = tag_buf;
- unsigned int i, j;
-
- for (i = 0, j = 0 ; i sectors ; i++, j += 2, sdt++) {
- tag[j] = (sdt-app_tag 0xff00) 8;
- tag[j+1] = sdt-app_tag 0xff;
- }
-}
-
static struct blk_integrity dif_type1_integrity_crc = {
.name = T10-DIF-TYPE1-CRC,
.generate_fn= sd_dif_type1_generate_crc,
.verify_fn = sd_dif_type1_verify_crc,
- .get_tag_fn = sd_dif_type1_get_tag,
- .set_tag_fn = sd_dif_type1_set_tag,
.tuple_size = sizeof(struct sd_dif_tuple),
.tag_size = 0,
};
@@ -169,8 +140,6 @@ static struct blk_integrity dif_type1_integrity_ip = {
.name = T10-DIF-TYPE1-IP,
.generate_fn= sd_dif_type1_generate_ip,
.verify_fn = sd_dif_type1_verify_ip,
- .get_tag_fn = sd_dif_type1_get_tag,
- .set_tag_fn = sd_dif_type1_set_tag,
.tuple_size = sizeof(struct sd_dif_tuple),
.tag_size = 0,
};
@@ -245,42 +214,10 @@ static int sd_dif_type3_verify_ip(struct
blk_integrity_exchg *bix)
return sd_dif_type3_verify(bix, sd_dif_ip_fn);
}
-static void sd_dif_type3_set_tag(void *prot, void *tag_buf, unsigned int
sectors)
-{
- struct sd_dif_tuple *sdt = prot;
- u8 *tag = tag_buf;
- unsigned int i, j;
-
- for (i = 0, j = 0 ; i sectors ; i++, j += 6, sdt++) {
- sdt-app_tag = tag[j] 8 | tag[j+1];
- sdt-ref_tag = tag[j+2] 24 | tag[j+3] 16 |
- tag[j+4] 8 | tag[j+5];
- }
-}
-
-static void sd_dif_type3_get_tag(void *prot, void *tag_buf, unsigned int
sectors)
-{
- struct sd_dif_tuple *sdt = prot;
- u8 *tag = tag_buf;
- unsigned int i, j;
-
- for (i = 0, j = 0 ; i sectors ; i++, j += 2, sdt++) {
- tag[j] = (sdt-app_tag 0xff00) 8;
- tag[j+1] = sdt-app_tag 0xff;
-
Re: status of block-integrity
Christoph == Christoph Hellwig h...@infradead.org writes: Christoph We have the block integrity code to support DIF/DIX in the Christoph the tree for about 5 and a half years, and we still don't Christoph have a single consumer of it. What do you mean? If you have a DIX-capable HBA (lpfc, qla2xxx, zfcp) then integrity protection is active from the block layer down. The only code that's not currently being exercised are the tag interleaving functions. I was hoping the FS people would use them for back pointers but nobody seemed to bite. Christoph Given that we'll have a lot of work to do in this area with Christoph block multiqueue I think it's time to either kill it off for Christoph good or make sure we can actually use and test it. I don't understand why multiqueue would require a lot of work? It's just an extra scatterlist per request. And obviously, if there's anything that needs to be done in this area I'll be happy to do so... -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug 42765] mptscsih driver issues task aborts during high write utilization
https://bugzilla.kernel.org/show_bug.cgi?id=42765 Alan a...@lxorguk.ukuu.org.uk changed: What|Removed |Added Resolution|--- |OBSOLETE Status|NEW |RESOLVED CC||a...@lxorguk.ukuu.org.uk --- Comment #3 from Alan a...@lxorguk.ukuu.org.uk --- If this is still present in modern kernels please update the bug -- You are receiving this mail because: You are watching the assignee of the bug. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pm80xx: Spinlock fix
Why is this considered dangerous? I put some thought into it and
couldn't find any obvious reason why it wouldn't work, but I also
couldn't find any other drivers that work this way. Is there a
particular reason to avoid doing it this way?
On Mon, Dec 23, 2013 at 8:45 AM, Suresh Thiagarajan
suresh.thiagara...@pmcs.com wrote:
-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com]
Sent: Monday, December 23, 2013 7:03 PM
To: Tomas Henzl; Viswas G
Cc: linux-scsi@vger.kernel.org; jason.seb...@gmail.com;
jbottom...@parallels.com; Vasanthalakshmi Tharmarajan; Suresh Thiagarajan
Subject: Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00
2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave
instead of spin_unlock_irq why the change to global lock_flags? I'm not a
spinlock expert, but is this safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if that
is the cause.
Suresh: We could not reproduce this issue and Jason(in CC) also could not
reproduce it for now. spin_lock_irqsave and spin_unlock_irqrestore were
called from multiple functions. Earlier flags was declared as a local
variable wherever spinlock was used. This was not correct since irq
information was saved in one function's flag which is a local to that
function and restored in other function where again flags was declared as
local variable to that function. So the data stored in flags while irq save
was not used while restoring. Since we have lock per card, we are associating
flag also per card for that lock.
-Suresh
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -425,10 +424,10
@@ static void mpi_set_phys_g3_with_ssc(struct pm8001_hba_info *pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N (N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -466,7 +465,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/*set the shifted destination address to 0x0 to avoid error operation
*/
pm8001_bar4_shift(pm8001_ha, 0x0);
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -481,7 +480,6 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
u32 offset;
u32 value;
u32 i;
-unsigned long flags;
#define OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
OPEN_RETRY_INTERVAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -490,11 +488,11
@@ static void
Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 03:55 PM, Jason Seba wrote:
Why is this considered dangerous? I put some thought into it and
couldn't find any obvious reason why it wouldn't work, but I also
couldn't find any other drivers that work this way. Is there a
particular reason to avoid doing it this way?
If you use global flags, you may change interrupt state depends on context.
On Mon, Dec 23, 2013 at 8:45 AM, Suresh Thiagarajan
suresh.thiagara...@pmcs.com wrote:
-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com]
Sent: Monday, December 23, 2013 7:03 PM
To: Tomas Henzl; Viswas G
Cc: linux-scsi@vger.kernel.org; jason.seb...@gmail.com;
jbottom...@parallels.com; Vasanthalakshmi Tharmarajan; Suresh Thiagarajan
Subject: Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00
2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave
instead of spin_unlock_irq why the change to global lock_flags? I'm not a
spinlock expert, but is this safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if that
is the cause.
Suresh: We could not reproduce this issue and Jason(in CC) also could not
reproduce it for now. spin_lock_irqsave and spin_unlock_irqrestore were
called from multiple functions. Earlier flags was declared as a local
variable wherever spinlock was used. This was not correct since irq
information was saved in one function's flag which is a local to that
function and restored in other function where again flags was declared as
local variable to that function. So the data stored in flags while irq save
was not used while restoring. Since we have lock per card, we are
associating flag also per card for that lock.
-Suresh
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -425,10 +424,10
@@ static void mpi_set_phys_g3_with_ssc(struct pm8001_hba_info *pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N (N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -466,7 +465,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/*set the shifted destination address to 0x0 to avoid error operation
*/
pm8001_bar4_shift(pm8001_ha, 0x0);
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock, pm8001_ha-lock_flags);
return;
}
@@ -481,7 +480,6 @@ static void mpi_set_open_retry_interval_reg(struct
pm8001_hba_info *pm8001_ha,
u32 offset;
u32 value;
u32 i;
-unsigned long flags;
#define OPEN_RETRY_INTERVAL_PHY_0_3_SHIFT_ADDR
Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled
Nicholas,
On Sat, 2013-12-14 at 10:51 -0600, Dr. Greg Wettstein wrote:
On Dec 12, 11:45am, Nicholas A. Bellinger wrote:
What I would prefer myself is to have a single set of target drivers
that works for both LIO and SCST. That would not only make both projects
easier to maintain but also would expose all target drivers to wider
testing. However, since the interface between target core and target
drivers is very different for LIO and for SCST reaching this goal would
be a challenging project by itself.
After spending 18+ months on qla2xxx target code, and getting it
into a state that it could withstand public scrutiny for mainline
acceptance, I have no interest in making changes to mainline driver
code for supporting someone else's out-of-tree stuff.
I believe, to be fair, that if it took 18 months to get the qla2x00t
code ported into the kernel that it would have taken even longer to
get fibre-channel support for the in-kernel target stack if the
process would have started from scratch. The kernel implementation
stands very heavily on engineering work which Vlad, ID7 and others put
into the target code.
Secondary to implementing the sqatgt driver I have been through every
line of both Vlad's code and your derivative work. It is a straight
forward exercise to follow both pieces of work side by side.
So despite all the hostility and rancor which surrounds the two major
Linux SCSI target implementations, lets concede that the in-kernel
fibre-channel driver benefited a great deal from the efforts of the
SCST community. This coming from someone who arranged for a small
amount of financial support for some of the code which is currently
residing in the in-kernel Qlogic target driver.
We don't add interfaces into mainline drive code to support
out-of-tree projects, because quite frankly, it gives less incentive
for the out-of-tree holdouts to use, improve and contribute to
mainline target code.
But the kernel does provide services to external users through a
binary API supplied by exported symbols. Your work on the Qlogic
fibre-channel target driver includes all but one small function needed
to allow SCST to use the in kernel driver.
To Nicholas' credit, the engineering work done on the qla2x00t driver,
as a component of porting it into the kernel, provides almost all of
the resources needed for a straight forward integration of SCST.
Having anticipated this debate we focused on designing the sqatgt
interface driver so it would integrate cleanly with the architectural
model selected for the kernel.
Bart, if you could review the in-kernel Qlogic driver you will find
the the tcm_qla2xxx.c module interfaces the Qlogic driver with the
in-kernel SCSI target engine. Similar functionality is provided by
the scst_qla2xxx.c in the sqatgt driver for SCST. So there is a
reasonably straight forward and standardized approach for getting both
projects on a common driver infrastructure, which as you note benefits
everyone.
The only missing piece of the puzzle is an exported function to
enumerate the target interfaces which are available. Since there are
exported functions to enable/disable interfaces, handle session
management and a variety of other functions it doesn't seem
unreasonable to provide a method of enumerating which interfaces are
available for target mode.
So Nicholas in the spirit of everyone working for the common good of
the storage community I'm hoping you will support our proposal of the
following patch to Andrew Vasquez and the Qlogic maintainers:
---
+void
+qlt_get_target_list(void (*callback)(struct scsi_qla_host *))
+{
+ struct qla_tgt *tgt;
+
+mutex_lock(qla_tgt_mutex);
+list_for_each_entry(tgt, qla_tgt_glist, tgt_list_entry) {
+ (*callback)(tgt-vha);
+ }
+mutex_unlock(qla_tgt_mutex);
+
+ return;
+}
+EXPORT_SYMBOL(qlt_get_target_list);
---
Which provides the only missing piece needed for SCST to live happily
on as a modular extension to the kernel. With this in place we can
all focus on more pressing issues, such as why Steve Magnani can't get
two ports on the same ISP chip to run in different modes.
I have code from QLogic that appears to resolve the issue with which I
started this thread, namely crummy initiator performance when the same
physical port is configured for dual initiator and target mode. QLogic's
code appears to be a fusion of the mainline qla2xxx driver and SCST,
which makes it taxonomically close to Greg's sqatgt proposal.
Once I figure out which portions of QLogic's code resolve the issue I
would be happy to post them as patches to mainline. But I can't justify
the effort to do that if Greg's proposal is DOA, since I wouldn't be
able to make use of the
[PATCH] megaraid_sas: Quirk mmio hook for 1078 MR controller
This patch has fix for LSI Gen-1 MR controller issue which only pop-up on few systems and it is not generic. On few system, MR 1078 MR controller is not working if mmio decoding is off. This patch proposed early quirck entry for Device id 0x1000/0x0411 to enable mmio. Signed-off-by: Kashyap Desai kashyap.de...@lsi.com --- diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index f6c31fa..a20fe41 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -43,6 +43,10 @@ static void quirk_mmio_always_on(struct pci_dev *dev) } DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on); +/* LSI controller device id 0x0411 has some issues if mmio_always_on is not set. + * It pop-up only on few specific system, where decoding disable is not working. + */ +DECLARE_PCI_FIXUP_EARLY(0x1000, 0x0411, quirk_mmio_always_on); /* The Mellanox Tavor device gives false positive parity errors * Mark this device with a broken_parity_status, to allow -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 04:06 PM, Jack Wang wrote:
On 12/23/2013 03:55 PM, Jason Seba wrote:
Why is this considered dangerous? I put some thought into it and
couldn't find any obvious reason why it wouldn't work, but I also
couldn't find any other drivers that work this way. Is there a
particular reason to avoid doing it this way?
If you use global flags, you may change interrupt state depends on context.
The problem could show up when different threads try to store different content
to the flags.
On Mon, Dec 23, 2013 at 8:45 AM, Suresh Thiagarajan
suresh.thiagara...@pmcs.com wrote:
-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com]
Sent: Monday, December 23, 2013 7:03 PM
To: Tomas Henzl; Viswas G
Cc: linux-scsi@vger.kernel.org; jason.seb...@gmail.com;
jbottom...@parallels.com; Vasanthalakshmi Tharmarajan; Suresh Thiagarajan
Subject: Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00
2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave
instead of spin_unlock_irq why the change to global lock_flags? I'm not a
spinlock expert, but is this safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if
that is the cause.
Suresh: We could not reproduce this issue and Jason(in CC) also could not
reproduce it for now. spin_lock_irqsave and spin_unlock_irqrestore were
called from multiple functions. Earlier flags was declared as a local
variable wherever spinlock was used. This was not correct since irq
information was saved in one function's flag which is a local to that
function and restored in other function where again flags was declared as
local variable to that function. So the data stored in flags while irq save
was not used while restoring. Since we have lock per card, we are
associating flag also per card for that lock.
Thanks, it took me a while until I have found an example - you need to unlock
mpi_ssp_event and the lock was taken elsewhere, so I understand it better now.
You know your code much better - are you sure that no other thread will be
started
which could change the value of lock_flags?
Tomas
-Suresh
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -425,10 +424,10
@@ static void mpi_set_phys_g3_with_ssc(struct pm8001_hba_info *pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N
(N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
for (i = 4; i 8; i++) {
@@ -466,7 +465,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/*set the shifted destination address to 0x0 to avoid
Re: [PATCH] pm80xx: Spinlock fix
Wouldn't the contents of the global flags value be protected by the
spinlock itself? Or is that making a dangerous assumption about the
particulars of how spinlocks work on various platforms?
On Mon, Dec 23, 2013 at 10:28 AM, Tomas Henzl the...@redhat.com wrote:
On 12/23/2013 04:06 PM, Jack Wang wrote:
On 12/23/2013 03:55 PM, Jason Seba wrote:
Why is this considered dangerous? I put some thought into it and
couldn't find any obvious reason why it wouldn't work, but I also
couldn't find any other drivers that work this way. Is there a
particular reason to avoid doing it this way?
If you use global flags, you may change interrupt state depends on context.
The problem could show up when different threads try to store different
content to the flags.
On Mon, Dec 23, 2013 at 8:45 AM, Suresh Thiagarajan
suresh.thiagara...@pmcs.com wrote:
-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com]
Sent: Monday, December 23, 2013 7:03 PM
To: Tomas Henzl; Viswas G
Cc: linux-scsi@vger.kernel.org; jason.seb...@gmail.com;
jbottom...@parallels.com; Vasanthalakshmi Tharmarajan; Suresh Thiagarajan
Subject: Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00
2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave
instead of spin_unlock_irq why the change to global lock_flags? I'm not
a spinlock expert, but is this safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if
that is the cause.
Suresh: We could not reproduce this issue and Jason(in CC) also could not
reproduce it for now. spin_lock_irqsave and spin_unlock_irqrestore were
called from multiple functions. Earlier flags was declared as a local
variable wherever spinlock was used. This was not correct since irq
information was saved in one function's flag which is a local to that
function and restored in other function where again flags was declared as
local variable to that function. So the data stored in flags while irq
save was not used while restoring. Since we have lock per card, we are
associating flag also per card for that lock.
Thanks, it took me a while until I have found an example - you need to unlock
mpi_ssp_event and the lock was taken elsewhere, so I understand it better now.
You know your code much better - are you sure that no other thread will be
started
which could change the value of lock_flags?
Tomas
-Suresh
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -425,10 +424,10
@@ static void mpi_set_phys_g3_with_ssc(struct pm8001_hba_info
*pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N
(N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+
Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 04:33 PM, Jason Seba wrote:
Wouldn't the contents of the global flags value be protected by the
spinlock itself? Or is that making a dangerous assumption about the
particulars of how spinlocks work on various platforms?
I'm not an expert but I think, that the spinlock starts by storing the 'flags'
and continues with acquiring that actual spin
On Mon, Dec 23, 2013 at 10:28 AM, Tomas Henzl the...@redhat.com wrote:
On 12/23/2013 04:06 PM, Jack Wang wrote:
On 12/23/2013 03:55 PM, Jason Seba wrote:
Why is this considered dangerous? I put some thought into it and
couldn't find any obvious reason why it wouldn't work, but I also
couldn't find any other drivers that work this way. Is there a
particular reason to avoid doing it this way?
If you use global flags, you may change interrupt state depends on context.
The problem could show up when different threads try to store different
content to the flags.
On Mon, Dec 23, 2013 at 8:45 AM, Suresh Thiagarajan
suresh.thiagara...@pmcs.com wrote:
-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com]
Sent: Monday, December 23, 2013 7:03 PM
To: Tomas Henzl; Viswas G
Cc: linux-scsi@vger.kernel.org; jason.seb...@gmail.com;
jbottom...@parallels.com; Vasanthalakshmi Tharmarajan; Suresh Thiagarajan
Subject: Re: [PATCH] pm80xx: Spinlock fix
On 12/23/2013 02:07 PM, Tomas Henzl wrote:
On 12/18/2013 12:28 PM, Viswas G wrote:
From 9338d4bc92b23b4c283f9bd6812646ab74866a40 Mon Sep 17 00:00:00
2001
From: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Date: Mon, 16 Dec 2013 21:15:20 +0530
Subject: [PATCH] pm80xx: Spinlock fix
spin_unlock was used instead of spin_unlock_irqrestore. To fix this
lock_flags per-adapter is added and used across all the places where
pm8001_ha-lock is used.
I think this could have been fixed but just using spin_unlock_irqsave
instead of spin_unlock_irq why the change to global lock_flags? I'm not
a spinlock expert, but is this safe?
Agree with Tomas, it's dangerous to change to global lock_flags.
Have you reproduce the bug reported from Jason, and verify the patch?
I think better just to change the spin_lock_irq to spin_lock_irqsave if
that is the cause.
Suresh: We could not reproduce this issue and Jason(in CC) also could not
reproduce it for now. spin_lock_irqsave and spin_unlock_irqrestore were
called from multiple functions. Earlier flags was declared as a local
variable wherever spinlock was used. This was not correct since irq
information was saved in one function's flag which is a local to that
function and restored in other function where again flags was declared as
local variable to that function. So the data stored in flags while irq
save was not used while restoring. Since we have lock per card, we are
associating flag also per card for that lock.
Thanks, it took me a while until I have found an example - you need to unlock
mpi_ssp_event and the lock was taken elsewhere, so I understand it better
now.
You know your code much better - are you sure that no other thread will be
started
which could change the value of lock_flags?
Tomas
-Suresh
Jack
Reported-by: Jason Seba jason.seb...@gmail.com
Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com
Signed-off-by: Viswas G viswa...@pmcs.com
---
drivers/scsi/pm8001/pm8001_hwi.c | 158
++
drivers/scsi/pm8001/pm8001_sas.c | 50 ++--
drivers/scsi/pm8001/pm8001_sas.h |1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 69 ++---
4 files changed, 160 insertions(+), 118 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c
b/drivers/scsi/pm8001/pm8001_hwi.c
index 0a1296a..3901c40 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -411,7 +411,6 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
u32 SSCbit)
{
u32 value, offset, i;
-unsigned long flags;
#define SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR 0x0003 #define
SAS2_SETTINGS_LOCAL_PHY_4_7_SHIFT_ADDR 0x0004 @@ -425,10 +424,10
@@ static void mpi_set_phys_g3_with_ssc(struct pm8001_hba_info
*pm8001_ha,
* Using shifted destination address 0x3_:0x1074 + 0x4000*N
(N=0:3)
* Using shifted destination address 0x4_:0x1074 + 0x4000*(N-4)
(N=4:7)
*/
-spin_lock_irqsave(pm8001_ha-lock, flags);
+spin_lock_irqsave(pm8001_ha-lock, pm8001_ha-lock_flags);
if (-1 == pm8001_bar4_shift(pm8001_ha,
SAS2_SETTINGS_LOCAL_PHY_0_3_SHIFT_ADDR)) {
-spin_unlock_irqrestore(pm8001_ha-lock, flags);
+spin_unlock_irqrestore(pm8001_ha-lock,
pm8001_ha-lock_flags);
return;
}
@@ -439,7 +438,7 @@ static void mpi_set_phys_g3_with_ssc(struct
pm8001_hba_info *pm8001_ha,
/* shift membase 3 for SAS2_SETTINGS_LOCAL_PHY 4 - 7 */
if (-1 ==
Re: [PATCH] pm80xx: Spinlock fix
On 12/23, Tomas Henzl wrote: On 12/23/2013 04:06 PM, Jack Wang wrote: On 12/23/2013 03:55 PM, Jason Seba wrote: Why is this considered dangerous? I put some thought into it and couldn't find any obvious reason why it wouldn't work, but I also couldn't find any other drivers that work this way. Is there a particular reason to avoid doing it this way? If you use global flags, you may change interrupt state depends on context. The problem could show up when different threads try to store different content to the flags. Agreed. I have no idea if the patch is right or not, but at least the changelog should clearly explain that only one thread can do spin_lock_irqsave(x-lock, x-lock_flags) at any time, otherwise the patch (and the code) _looks_ wrong even if it is correct. And if we can't use a local unsigned long flags because _unlock can happen in another function, imho this should be mentioned in the changelog as well. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pm80xx: Spinlock fix
On 12/23, Jason Seba wrote:
Wouldn't the contents of the global flags value be protected by the
spinlock itself?
This can be even true because nowadays spin_lock_irqsave() writes to
flags after it takes the lock, and _irqrestore works gets the copy
of flags before it releases the lock.
Still this doesn't look safe and afaik this is not documented. Although
I have to admit that after I actually looked at the current implementation
I think this should work.
Perhaps we should ask the maintainers upstream? Even if this works, I am
not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave()
can be changed as, say
#define spin_lock_irqsave(lock, flags) \
do {\
local_irq_save(flags); \
spin_lock(lock);\
} while (0)
(and iirc it was defined this way a long ago). In this case flags is
obviously not protected.
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)
On 12/23, Oleg Nesterov wrote:
Perhaps we should ask the maintainers upstream? Even if this works, I am
not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave()
can be changed as, say
#define spin_lock_irqsave(lock, flags) \
do {\
local_irq_save(flags); \
spin_lock(lock);\
} while (0)
(and iirc it was defined this way a long ago). In this case flags is
obviously not protected.
Yes, lets ask the maintainers.
In short, is this code
spinlock_t LOCK;
unsigned long FLAGS;
void my_lock(void)
{
spin_lock_irqsave(LOCK, FLAGS);
}
void my_unlock(void)
{
spin_unlock_irqrestore(LOCK, FLAGS);
}
correct or not?
Initially I thought that this is obviously wrong, irqsave/irqrestore
assume that flags is owned by the caller, not by the lock. And iirc
this was certainly wrong in the past.
But when I look at spinlock.c it seems that this code can actually work.
_irqsave() writes to FLAGS after it takes the lock, and _irqrestore()
has a copy of FLAGS before it drops this lock.
And it turns out, some users assume this should work, for example
arch/arm/mach-omap2/powerdomain.c:
pwrdm_lock() and pwrdm_unlock()
drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c:
brcmf_fws_lock() and brcmf_fws_unlock()
seem to do exactly this. Plus the pending patch for drivers/scsi/pm8001/.
So is it documented somewhere that this sequence is correct, or the code
above should be changed even if it happens to work?
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] megaraid_sas: Quirk mmio hook for 1078 MR controller
On Mon, Dec 23, 2013 at 8:13 AM, kashyap.de...@lsi.com wrote: This patch has fix for LSI Gen-1 MR controller issue which only pop-up on few systems and it is not generic. On few system, MR 1078 MR controller is not working if mmio decoding is off. This patch proposed early quirck entry for Device id 0x1000/0x0411 to enable mmio. Signed-off-by: Kashyap Desai kashyap.de...@lsi.com --- diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index f6c31fa..a20fe41 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -43,6 +43,10 @@ static void quirk_mmio_always_on(struct pci_dev *dev) } DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on); +/* LSI controller device id 0x0411 has some issues if mmio_always_on is not set. + * It pop-up only on few specific system, where decoding disable is not working. + */ +DECLARE_PCI_FIXUP_EARLY(0x1000, 0x0411, quirk_mmio_always_on); What is the actual problem that happens without this quirk? Is there some hardware defect in the MR 1078 that makes it fail when we disable decoding? Disabling decoding is important because it prevents conflicts with other devices while sizing and updating BARs, so I don't want to use this quirk unless it's really necessary. Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)
On Mon, Dec 23, 2013 at 9:27 AM, Oleg Nesterov o...@redhat.com wrote:
In short, is this code
spinlock_t LOCK;
unsigned long FLAGS;
void my_lock(void)
{
spin_lock_irqsave(LOCK, FLAGS);
}
void my_unlock(void)
{
spin_unlock_irqrestore(LOCK, FLAGS);
}
correct or not?
Hell no. flags needs to be a thread-private variable, or at least
protected some way (ie the above could work if everything is inside a
bigger lock, to serialize access to FLAGS).
Linus
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)
On 12/23, Linus Torvalds wrote:
On Mon, Dec 23, 2013 at 9:27 AM, Oleg Nesterov o...@redhat.com wrote:
In short, is this code
spinlock_t LOCK;
unsigned long FLAGS;
void my_lock(void)
{
spin_lock_irqsave(LOCK, FLAGS);
}
void my_unlock(void)
{
spin_unlock_irqrestore(LOCK, FLAGS);
}
correct or not?
Hell no. flags needs to be a thread-private variable, or at least
protected some way (ie the above could work if everything is inside a
bigger lock, to serialize access to FLAGS).
This was my understanding (although, once again, it seems to me this can
suprisingly work with the current implementation).
However, the code above already has the users. Do you think it makes
sense to add something like
void spinlock_irqsave_careful(spinlock_t *lock, unsigned long *flags)
{
unsigned long _flags;
spinlock_irqsave(lock, _flags);
*flags = flags;
}
void spinlock_irqrestore_careful(spinlock_t *lock, unsigned long *flags)
{
unsigned long _flags = *flags;
spinlock_irqrestore(lock, _flags);
}
into include/linux/spinlock.h ?
Oleg.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)
* Oleg Nesterov o...@redhat.com wrote:
On 12/23, Oleg Nesterov wrote:
Perhaps we should ask the maintainers upstream? Even if this works, I am
not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave()
can be changed as, say
#define spin_lock_irqsave(lock, flags) \
do {\
local_irq_save(flags); \
spin_lock(lock);\
} while (0)
(and iirc it was defined this way a long ago). In this case flags is
obviously not protected.
Yes, lets ask the maintainers.
In short, is this code
spinlock_t LOCK;
unsigned long FLAGS;
void my_lock(void)
{
spin_lock_irqsave(LOCK, FLAGS);
}
void my_unlock(void)
{
spin_unlock_irqrestore(LOCK, FLAGS);
}
correct or not?
Initially I thought that this is obviously wrong, irqsave/irqrestore
assume that flags is owned by the caller, not by the lock. And
iirc this was certainly wrong in the past.
But when I look at spinlock.c it seems that this code can actually
work. _irqsave() writes to FLAGS after it takes the lock, and
_irqrestore() has a copy of FLAGS before it drops this lock.
I don't think that's true: if it was then the lock would not be
irqsave, a hardware-irq could come in after the lock has been taken
and before flags are saved+disabled.
So AFAICS this is an unsafe pattern, beyond being ugly as hell.
Thanks,
Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)
On 12/23, Ingo Molnar wrote: * Oleg Nesterov o...@redhat.com wrote: Initially I thought that this is obviously wrong, irqsave/irqrestore assume that flags is owned by the caller, not by the lock. And iirc this was certainly wrong in the past. But when I look at spinlock.c it seems that this code can actually work. _irqsave() writes to FLAGS after it takes the lock, and _irqrestore() has a copy of FLAGS before it drops this lock. I don't think that's true: if it was then the lock would not be irqsave, a hardware-irq could come in after the lock has been taken and before flags are saved+disabled. I do agree that this pattern is not safe, that is why I decided to ask. But, unless I missed something, with the current implementation spin_lock_irqsave(lock, global_flags) does: unsigned long local_flags; local_irq_save(local_flags); spin_lock(lock); global_flags = local_flags; so the access to global_flags is actually serialized by lock. So AFAICS this is an unsafe pattern, beyond being ugly as hell. Yes, I think the same. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug 8221] RACE: Lock is expected before calling ips_removeq_scb_head, but in some call chains not held!
https://bugzilla.kernel.org/show_bug.cgi?id=8221 Alan a...@lxorguk.ukuu.org.uk changed: What|Removed |Added Status|NEW |RESOLVED CC||a...@lxorguk.ukuu.org.uk Resolution|--- |OBSOLETE Regression|--- |No -- You are receiving this mail because: You are watching the assignee of the bug. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: spinlock_irqsave() flags (Was: pm80xx: Spinlock fix)
On Mon, Dec 23, 2013 at 10:24 AM, Oleg Nesterov o...@redhat.com wrote: However, the code above already has the users. Do you think it makes sense to add something like No. I think it makes sense to put a big warning on any users you find, and fart in the general direction of any developer who did that broken pattern. Because code like that is obviously crap. Linus -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
scsi-mq WIP updated to v3.13-rc3
Hi Folks, Just a heads up that scsi-mq alpha code has been updated to v3.13-rc3 using the freshly upstreamed blk-mq logic. The working branch is available here: git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git scsi-mq The changelog since the last v3.12-rc3 rev includes: - Use Scsi_Host to check for can_queue + cmd_per_lun - Add explicit struct scsi_cmnd memset in scsi_mq_alloc_queue() - Add support for cmd-prot_sdb pre-allocation - Add support for cmd-mq_prot_sgl pre-allocation - Honor request_queue-dma_drain_size in scsi_mq_queue_rq (Jianpeng Ma) - Drop unnecessary QUEUE_FLAG_NONROT in scsi_mq_alloc_queue (Jianpeng Ma) - Add bio_integrity setup to blk_mq_make_request - Pre-allocation of sg tables based on host sg_tablesize (Jianpeng Ma) - Pre-allocation of protection sg tables based on host sg_tablesize - Updates for DIF prot_sdb-table.sgl pre-allocation in scsi_init_io - Acquire q-sysfs_lock for elevator_init (Fengguang) - Only reference scsi_cmd_to_driver with valid req-rq_disk in scsi_finish_command() Also, basic DIF functionality is now working! Here's a quick snippet of DIF TYPE1 in action using scsi_debug dif=1 dix=1: [ 34.331249] scsi_debug_init: dif_storep 16777216 bytes @ c90001459000 [ 34.338588] scsi_debug: host protection DIF1 DIX1 [ 34.339490] scsi6 : scsi_debug, version 1.82 [20100324], dev_size_mb=1024, opts=0x0 [ 34.341026] Calling blk_mq_init_queue: scsi_mq_ops: 8163e760, queue_depth: 64, cmd_size: 304 SCSI cmd_size: 0 [ 34.343031] blk-mq: CPU - queue map [ 34.343689] CPU 0 - Queue 0 [ 34.344322] CPU 1 - Queue 0 [ 34.344869] CPU 2 - Queue 0 [ 34.345422] CPU 3 - Queue 0 [ 34.346005] Performing sc map setup on q: 88003018 hctx: 880034e3de00 i: 0 [ 34.348146] scsi_mq_alloc_queue() complete [ 34.349393] scsi 6:0:0:0: Direct-Access Linuxscsi_debug 0004 PQ: 0 ANSI: 5 [ 34.351013] sd 6:0:0:0: Attached scsi generic sg1 type 0 [ 34.352067] sd 6:0:0:0: [sda] Enabling DIF Type 1 protection [ 34.352075] sd 6:0:0:0: [sda] 2097152 512-byte logical blocks: (1.07 GB/1.00 GiB) [ 34.360115] sd 6:0:0:0: [sda] Write Protect is off [ 34.363006] sd 6:0:0:0: [sda] Mode Sense: 73 00 10 08 [ 34.372176] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 34.416255] sda: unknown partition table [ 34.419315] sd 6:0:0:0: [sda] Enabling DIX T10-DIF-TYPE1-CRC protection [ 34.423194] sd 6:0:0:0: [sda] DIF application tag size 2 [ 34.444172] sd 6:0:0:0: [sda] Attached SCSI disk [ 68.554153] scsi_mq: sc: 880030198568 SCSI CDB: : 0x00 [ 68.554164] scsi_mq: scsi_bufflen: 0 rq-cmd_len: 6 [ 68.554178] scsi_mq_end_request: sc: 880030198568 sc-tag: 48 error: 0 [ 68.554243] Entering sd_dif_type1_generate [ 68.554264] TYPE1 Generate: sector: 0 sdt-guard_tag: 0xdfcc sdt-app_tag: 0x sdt-ref_tag: 0 [ 68.554281] TYPE1 Generate: sector: 1 sdt-guard_tag: 0xd693 sdt-app_tag: 0x sdt-ref_tag: 1 [ 68.554297] TYPE1 Generate: sector: 2 sdt-guard_tag: 0xd8ea sdt-app_tag: 0x sdt-ref_tag: 2 [ 68.554309] TYPE1 Generate: sector: 3 sdt-guard_tag: 0x64a3 sdt-app_tag: 0x sdt-ref_tag: 3 [ 68.554322] TYPE1 Generate: sector: 4 sdt-guard_tag: 0xd194 sdt-app_tag: 0x sdt-ref_tag: 4 [ 68.554334] TYPE1 Generate: sector: 5 sdt-guard_tag: 0x0c30 sdt-app_tag: 0x sdt-ref_tag: 5 [ 68.554346] TYPE1 Generate: sector: 6 sdt-guard_tag: 0xcbff sdt-app_tag: 0x sdt-ref_tag: 6 [ 68.554359] TYPE1 Generate: sector: 7 sdt-guard_tag: 0x5c4e sdt-app_tag: 0x sdt-ref_tag: 7 [ 68.554384] scsi_mq: sc: 880030198568 SCSI CDB: : 0x2a [ 68.554389] scsi_mq: scsi_bufflen: 4096 rq-cmd_len: 16 [ 68.554396] Enering prot_verify_write sector: 8 ei_lba: 0 [ 68.554402] WRITE sector: 0 sdt-guard_tag: 0xdfcc sdt-app_tag: 0x sdt-ref_tag: 0 [ 68.554414] WRITE sector: 1 sdt-guard_tag: 0xd693 sdt-app_tag: 0x sdt-ref_tag: 1 [ 68.554427] WRITE sector: 2 sdt-guard_tag: 0xd8ea sdt-app_tag: 0x sdt-ref_tag: 2 [ 68.554439] WRITE sector: 3 sdt-guard_tag: 0x64a3 sdt-app_tag: 0x sdt-ref_tag: 3 [ 68.554451] WRITE sector: 4 sdt-guard_tag: 0xd194 sdt-app_tag: 0x sdt-ref_tag: 4 [ 68.554463] WRITE sector: 5 sdt-guard_tag: 0x0c30 sdt-app_tag: 0x sdt-ref_tag: 5 [ 68.554475] WRITE sector: 6 sdt-guard_tag: 0xcbff sdt-app_tag: 0x sdt-ref_tag: 6 [ 68.554488] WRITE sector: 7 sdt-guard_tag: 0x5c4e sdt-app_tag: 0x sdt-ref_tag: 7 SNIP [ 68.572734] scsi_mq: sc: 880030198568 SCSI CDB: : 0x28 [ 68.572741] scsi_mq: scsi_bufflen: 4096 rq-cmd_len: 16 [ 68.572750] Entering prot_verify_read: sector: 8 ei_lba: 0 [ 68.572758] READ sector: 0 sdt-guard_tag: 0xdfcc sdt-app_tag: 0x sdt-ref_tag: 0 [ 68.572774] READ sector: 1 sdt-guard_tag: 0xd693 sdt-app_tag: 0x sdt-ref_tag: 1 [ 68.572786] READ sector: 2 sdt-guard_tag: 0xd8ea sdt-app_tag: 0x sdt-ref_tag: 2 [
Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled
On Mon, 2013-12-23 at 09:13 -0600, Steve Magnani wrote:
Nicholas,
On Sat, 2013-12-14 at 10:51 -0600, Dr. Greg Wettstein wrote:
On Dec 12, 11:45am, Nicholas A. Bellinger wrote:
SNIP
We don't add interfaces into mainline drive code to support
out-of-tree projects, because quite frankly, it gives less incentive
for the out-of-tree holdouts to use, improve and contribute to
mainline target code.
But the kernel does provide services to external users through a
binary API supplied by exported symbols. Your work on the Qlogic
fibre-channel target driver includes all but one small function needed
to allow SCST to use the in kernel driver.
To Nicholas' credit, the engineering work done on the qla2x00t driver,
as a component of porting it into the kernel, provides almost all of
the resources needed for a straight forward integration of SCST.
Having anticipated this debate we focused on designing the sqatgt
interface driver so it would integrate cleanly with the architectural
model selected for the kernel.
Bart, if you could review the in-kernel Qlogic driver you will find
the the tcm_qla2xxx.c module interfaces the Qlogic driver with the
in-kernel SCSI target engine. Similar functionality is provided by
the scst_qla2xxx.c in the sqatgt driver for SCST. So there is a
reasonably straight forward and standardized approach for getting both
projects on a common driver infrastructure, which as you note benefits
everyone.
The only missing piece of the puzzle is an exported function to
enumerate the target interfaces which are available. Since there are
exported functions to enable/disable interfaces, handle session
management and a variety of other functions it doesn't seem
unreasonable to provide a method of enumerating which interfaces are
available for target mode.
So Nicholas in the spirit of everyone working for the common good of
the storage community I'm hoping you will support our proposal of the
following patch to Andrew Vasquez and the Qlogic maintainers:
---
+void
+qlt_get_target_list(void (*callback)(struct scsi_qla_host *))
+{
+ struct qla_tgt *tgt;
+
+mutex_lock(qla_tgt_mutex);
+list_for_each_entry(tgt, qla_tgt_glist, tgt_list_entry) {
+ (*callback)(tgt-vha);
+ }
+mutex_unlock(qla_tgt_mutex);
+
+ return;
+}
+EXPORT_SYMBOL(qlt_get_target_list);
---
Which provides the only missing piece needed for SCST to live happily
on as a modular extension to the kernel. With this in place we can
all focus on more pressing issues, such as why Steve Magnani can't get
two ports on the same ISP chip to run in different modes.
I have code from QLogic that appears to resolve the issue with which I
started this thread, namely crummy initiator performance when the same
physical port is configured for dual initiator and target mode. QLogic's
code appears to be a fusion of the mainline qla2xxx driver and SCST,
which makes it taxonomically close to Greg's sqatgt proposal.
Glad to hear that you where able to resolve the issue on your setup.
However, as you've noticed mixed mode operation is currently unsupported
in mainline qla_target.c code, due to the fact that there is not yet a
clean method to transition between target + initiator mode for a given
qla2xxx port.
All of the methods for doing this in the original out-of-tree code where
terribly ugly hacks (and continue to be), and hence where left-out for
the mainline merge.
Once I figure out which portions of QLogic's code resolve the issue I
would be happy to post them as patches to mainline. But I can't justify
the effort to do that if Greg's proposal is DOA, since I wouldn't be
able to make use of the resulting patched code.
My issue with Greg's proposal is the additional of new interfaces
specific to out-of-tree code, that no mainline code utilizes. To
repeat, we do not add interfaces that are specific to out-of-tree code,
regardless of what subsystem / driver they reside.
So what needs to happen with Greg's effort is to convert to using
existing interfaces for qla2xxx port registration that tcm_qla2xxx is
already using, eg: qlt_lport_register() + qlt_lport_deregister(), and
make them work for his use-case.
They already provide what is required for an a mainline or out-of-tree
target stack to function, and yes, even ensure that multiple target
stacks can play nicely with qla_target.c logic at the same time.
As an engineer with a problem to solve and a deadline, I don't much care
whether the name of the solution is LIO or SCST. But it is a necessity
of my project that the target implementation reside in userland. AFAIK
LIO does not support this, nor plan to.
Not true. The patches for target_core_user.c where posted
Re: [PATCH] drivers: target: target_core_mod: use div64_u64_rem() instead of operator '%' for u64
On 12/23/2013 02:51 PM, Nicholas A. Bellinger wrote:
On Sun, 2013-12-22 at 17:17 +0800, Chen Gang wrote:
On 12/22/2013 10:56 AM, Nicholas A. Bellinger wrote:
Hi Chen,
On Sat, 2013-12-21 at 10:08 +0800, Chen Gang wrote:
In kernel, need use div64_u64_rem() instead of operator '%' for u64, or
can not pass compiling (with allmodconfig under metag):
MODPOST 2909 modules
ERROR: __umoddi3 [drivers/target/target_core_mod.ko] undefined!
Also need u64 type cast for u32 variable multiply u32 variable, or will
cause type overflow issue.
Signed-off-by: Chen Gang gang.chen.5...@gmail.com
---
drivers/target/target_core_alua.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
FYI, this unsigned long long division in core_alua_state_lba_dependent()
was fixed for 32-bit in linux-next = 12192013 code.
OK, thanks.
The related fix patch changed start_lba = lba % ... to start_lba =
lba / ..., and also assumed segment_size * segment_mult is still
within u32 (can not cause type over flow).
I guess the original author already knew about them, and intended to do
like that (if not, please let me know, thanks).
Sorry, your correct that the original code is using modulo division to
calculate start_lba.
Oh, that's all right, (in fact, don't need sorry), I am not quite
familiar with the details, so need related member help check it. :-)
Hannes, can you please double check this below..?
Please help check when have time, thanks.
Thank you,
--nab
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index dc0d399..ff2aadc 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -489,7 +489,8 @@ static inline int core_alua_state_lba_dependent(
u64 first_lba = map-lba_map_first_lba;
if (segment_mult) {
- start_lba = lba % (segment_size * segment_mult);
+ u64 tmp = (u64)segment_size * segment_mult;
+ div64_u64_rem(lba, tmp, start_lba);
last_lba = first_lba + segment_size - 1;
if (start_lba = first_lba
start_lba = last_lba) {
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
bio_integrity_verify() bug causing READ verify to be silently skipped
Hi Martin Co,
So after playing with the mainline DIF client against an initial WIP
target DIF support patch, I've started hitting a bug in
bio_integrity_verify() that causes READ verify logic to be silently
skipped for both WIP target and existing scsi_debug DIF code.
The issue is with the scsi_end_request() - blk_end_request() completion
path, where eventually blk_update_request() - req_bio_endio() -
bio_advance() is called to increment bio-bi_idx to a non-zero value.
Given that bio_integrity_verify() is using bio_for_each_segment(), the
loop starts from the updated bio-bi_idx, and not a zero value, which
ends up skipping individual bio segment calls to bi-verify_fn().
The following patch changes bio_integrity_verify() to use
bio_for_each_segment_all() instead of bio_for_each_segment() to ensure
that the segment walk always starts from zero, regardless of the current
bio-bi_idx value after bio_advance().
Note this bug has been observed with v3.13-rc3 code, and I haven't yet
looked back to figure out when this bug was first introduced.. Any
ideas..?
Interestingly enough, the scsi-mq alpha code does not suffer from this
bug, as blk_end_request() is never called from scsi_mq_end_request() -
blk_mq_end_io() completion path code.
Thank you,
--nab
From 32242942edca095e8dd126cb1408f2842340773e Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger n...@linux-iscsi.org
Date: Tue, 24 Dec 2013 04:00:24 +
Subject: [PATCH] bio-integrity: Fix bio_integrity_verify segment start bug
This patch addresses a bug in bio_integrity_verify() code that has
been causing DIF READ verify operations to be silently skipped.
The issue is that bio-bi_idx will have been incremented within
bio_advance() code in the normal blk_update_request() -
req_bio_endio() completion path, and bio_integrity_verify() is
using bio_for_each_segment() which starts the bio segment walk
at the current bio-bi_idx.
So instead use bio_for_each_segment_all() to always start the bio
segment walk from zero, regardless of the current bio-bi_idx
value after bio_advance() has been called.
Cc: Martin K. Petersen martin.peter...@oracle.com
Cc: Jens Axboe ax...@kernel.dk
Cc: Christoph Hellwig h...@lst.de
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
---
fs/bio-integrity.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index fc60b31..f1d5cde 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -450,7 +450,7 @@ static int bio_integrity_verify(struct bio *bio)
bix.disk_name = bio-bi_bdev-bd_disk-disk_name;
bix.sector_size = bi-sector_size;
- bio_for_each_segment(bv, bio, i) {
+ bio_for_each_segment_all(bv, bio, i) {
void *kaddr = kmap_atomic(bv-bv_page);
bix.data_buf = kaddr + bv-bv_offset;
bix.data_size = bv-bv_len;
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
