Reply.
I have a proposal for you in the tune of One Hundred & Five Million EUR, after successful transfer, we shall share in the ratio of forty for you and sixty for me. Please reply for specifics. Yours, Mr. Cham Tao Soon Chairman Audit Committee UOB Bank, Singapore -- 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 v3] [SCSI] scsi_debug: silence GCC warning
Building scsi_debug.o triggers a GCC warning:
drivers/scsi/scsi_debug.c: In function ‘dif_verify’:
drivers/scsi/scsi_debug.c:1755:3: warning: ‘csum’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
This warning was apparently introduced (in v3.11-rc1) by commit
beb40ea42b ("[SCSI] scsi_debug: reduce duplication between
prot_verify_read and prot_verify_write"). It is a false positive. But if
we transform the switch statement in dif_compute_csum() to a
straightforward if/else statement we supply GCC with enough information
to determine that csum will not be used uninitialized.
Signed-off-by: Paul Bolle
---
0) v2: much simpler, and doesn't change any behavior;
v3: mention commit bebe40ea42b, as Akinobu suggested.
1) Still only compile tested.
drivers/scsi/scsi_debug.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index cb4fefa..7565ec5 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1735,14 +1735,11 @@ static u16 dif_compute_csum(const void *buf, int len)
{
u16 csum;
- switch (scsi_debug_guard) {
- case 1:
+ if (scsi_debug_guard == 1)
csum = ip_compute_csum(buf, len);
- break;
- case 0:
+ else
csum = cpu_to_be16(crc_t10dif(buf, len));
- break;
- }
+
return csum;
}
--
1.8.1.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
RE: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations
On Thu, July 11, 2013, Sujit Reddy Thumma wrote:
> On 7/10/2013 7:01 PM, Seungwon Jeon wrote:
> > I'm not sure that BKOPS with runtime-pm associates.
> > Do you think it's helpful for power management?
> > How about hibernation scheme for runtime-pm?
> > I'm testing and I can introduce soon.
>
> Well, I am thinking on following approach when we introduce
> power management.
>
> ufshcd_runtime_suspend() {
> if (bkops_status >= NON_CRITICAL) { /* 0x1 */
> ufshcd_enable_auto_bkops();
> hibernate(); /* only the link and the device
> should be able to cary out bkops */
> } else {
> hibernate(); /* Link and the device for more savings */
> }
> }
>
> Let me know if this is okay.
I still consider whether BKOPS is proper behavior with runtime-pm or not.
How about this scenario? It seems more simple.
If we concern a response latency of transfer requests, BKOPS can be disabled by
default.
And then BKOPS can be enabled whenever device requests in some exception.
If you have any idea, let me know.
Thanks,
Seungwon Jeon
--
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 V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU
On Thu, July 11, 2013, Sujit Reddy Thumma wrote:
> On 7/10/2013 6:58 PM, Seungwon Jeon wrote:
> > On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
> >> As part of device initialization sequence, sending NOP OUT UPIU and
> >> waiting for NOP IN UPIU response is mandatory. This confirms that the
> >> device UFS Transport (UTP) layer is functional and the host can configure
> >> the device with further commands. Add support for sending NOP OUT UPIU to
> >> check the device connection path and test whether the UTP layer on the
> >> device side is functional during initialization.
> >>
> >> A tag is acquired from the SCSI tag map space in order to send the device
> >> management command. When the tag is acquired by internal command the scsi
> >> command is rejected with host busy flag in order to requeue the request.
> >> To avoid frequent collisions between internal commands and scsi commands
> >> the device management command tag is allocated in the opposite direction
> >> w.r.t block layer tag allocation.
> >>
> >> Signed-off-by: Sujit Reddy Thumma
> >> Signed-off-by: Dolev Raviv
> >> ---
> >> drivers/scsi/ufs/ufs.h| 43 +++-
> >> drivers/scsi/ufs/ufshcd.c | 596
> >> +
> >> drivers/scsi/ufs/ufshcd.h | 29 ++-
> >> 3 files changed, 552 insertions(+), 116 deletions(-)
> >>
> >> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> >> index 139bc06..14c0a4e 100644
> >> --- a/drivers/scsi/ufs/ufs.h
> >> +++ b/drivers/scsi/ufs/ufs.h
> >> @@ -36,10 +36,16 @@
> >> #ifndef _UFS_H
> >> #define _UFS_H
> >>
> >> +#include
> >> +#include
> >> +
> >> #define MAX_CDB_SIZE 16
> >> +#define GENERAL_UPIU_REQUEST_SIZE 32
> >> +#define UPIU_HEADER_DATA_SEGMENT_MAX_SIZE ((ALIGNED_UPIU_SIZE) - \
> >> + (GENERAL_UPIU_REQUEST_SIZE))
> >>
> >> #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
> >> - ((byte3 << 24) | (byte2 << 16) |\
> >> + cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
> >> (byte1 << 8) | (byte0))
> >>
> >> /*
> >> @@ -73,6 +79,7 @@ enum {
> >>UPIU_TRANSACTION_TASK_RSP = 0x24,
> >>UPIU_TRANSACTION_READY_XFER = 0x31,
> >>UPIU_TRANSACTION_QUERY_RSP = 0x36,
> >> + UPIU_TRANSACTION_REJECT_UPIU= 0x3F,
> >> };
> >>
> >> /* UPIU Read/Write flags */
> >> @@ -110,6 +117,12 @@ enum {
> >>UPIU_COMMAND_SET_TYPE_QUERY = 0x2,
> >> };
> >>
> >> +/* UTP Transfer Request Command Offset */
> >> +#define UPIU_COMMAND_TYPE_OFFSET 28
> >> +
> >> +/* Offset of the response code in the UPIU header */
> >> +#define UPIU_RSP_CODE_OFFSET 8
> >> +
> >> enum {
> >>MASK_SCSI_STATUS= 0xFF,
> >>MASK_TASK_RESPONSE = 0xFF00,
> >> @@ -138,26 +151,32 @@ struct utp_upiu_header {
> >>
> >> /**
> >>* struct utp_upiu_cmd - Command UPIU structure
> >> - * @header: UPIU header structure DW-0 to DW-2
> >>* @data_transfer_len: Data Transfer Length DW-3
> >>* @cdb: Command Descriptor Block CDB DW-4 to DW-7
> >>*/
> >> struct utp_upiu_cmd {
> >> - struct utp_upiu_header header;
> >>u32 exp_data_transfer_len;
> >>u8 cdb[MAX_CDB_SIZE];
> >> };
> >>
> >> /**
> >> - * struct utp_upiu_rsp - Response UPIU structure
> >> - * @header: UPIU header DW-0 to DW-2
> >> + * struct utp_upiu_req - general upiu request structure
> >> + * @header:UPIU header structure DW-0 to DW-2
> >> + * @sc: fields structure for scsi command DW-3 to DW-7
> >> + */
> >> +struct utp_upiu_req {
> >> + struct utp_upiu_header header;
> >> + struct utp_upiu_cmd sc;
> >> +};
> >> +
> >> +/**
> >> + * struct utp_cmd_rsp - Response UPIU structure
> >>* @residual_transfer_count: Residual transfer count DW-3
> >>* @reserved: Reserved double words DW-4 to DW-7
> >>* @sense_data_len: Sense data length DW-8 U16
> >>* @sense_data: Sense data field DW-8 to DW-12
> >>*/
> >> -struct utp_upiu_rsp {
> >> - struct utp_upiu_header header;
> >> +struct utp_cmd_rsp {
> >>u32 residual_transfer_count;
> >>u32 reserved[4];
> >>u16 sense_data_len;
> >> @@ -165,6 +184,16 @@ struct utp_upiu_rsp {
> >> };
> >>
> >> /**
> >> + * struct utp_upiu_rsp - general upiu response structure
> >> + * @header: UPIU header structure DW-0 to DW-2
> >> + * @sr: fields structure for scsi command DW-3 to DW-12
> >> + */
> >> +struct utp_upiu_rsp {
> >> + struct utp_upiu_header header;
> >> + struct utp_cmd_rsp sr;
> >> +};
> >> +
> >> +/**
> >>* struct utp_upiu_task_req - Task request UPIU structure
> >>* @header - UPIU header structure DW0 to DW-2
> >>* @input_param1: Input parameter 1 DW-3
> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> index b743bd6..3f482b6 100644
> >> --- a/drivers/scsi/ufs/ufshcd.c
> >> +++ b/drivers/scsi/ufs/ufshcd.c
> >> @@ -43,6 +43,11 @@
> >> /* UIC command timeout, unit: ms */
> >> #define UIC_CMD_TIMEOUT 500
> >>
> >> +/* NOP OUT retri
RE: [PATCH V3 2/2] scsi: ufs: Set fDeviceInit flag to initiate device initialization
Hi,
Sorry for the late response. I had a few days off.
On Thu, July 11, 2013, Dolev Raviv wrote:
> > On Tuesday, July 09, 2013, Sujit Reddy Thumma wrote:
> >> From: Dolev Raviv
> >> Allow UFS device to complete its initialization and accept
> >> SCSI commands by setting fDeviceInit flag. The device may take
> >> time for this operation and hence the host should poll until
> >> fDeviceInit flag is toggled to zero. This step is mandated by
> >> UFS device specification for device initialization completion.
> >> Signed-off-by: Dolev Raviv
> >> Signed-off-by: Sujit Reddy Thumma
> >> ---
> >> drivers/scsi/ufs/ufs.h| 88 +-
> >> drivers/scsi/ufs/ufshcd.c | 292
> >> -
> >> drivers/scsi/ufs/ufshcd.h | 14 ++
> >> drivers/scsi/ufs/ufshci.h |2 +-
> >> 4 files changed, 390 insertions(+), 6 deletions(-)
> >> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> >> index 14c0a4e..db5bde4 100644
> >> --- a/drivers/scsi/ufs/ufs.h
> >> +++ b/drivers/scsi/ufs/ufs.h
> >> @@ -43,6 +43,8 @@
> >> #define GENERAL_UPIU_REQUEST_SIZE 32
> >> #define UPIU_HEADER_DATA_SEGMENT_MAX_SIZE ((ALIGNED_UPIU_SIZE) - \
> >>(GENERAL_UPIU_REQUEST_SIZE))
> >> +#define QUERY_OSF_SIZE((GENERAL_UPIU_REQUEST_SIZE) - \
> >> + (sizeof(struct utp_upiu_header)))
> >> #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
> >>cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
> >> @@ -68,7 +70,7 @@ enum {
> >>UPIU_TRANSACTION_COMMAND= 0x01,
> >>UPIU_TRANSACTION_DATA_OUT = 0x02,
> >>UPIU_TRANSACTION_TASK_REQ = 0x04,
> >> - UPIU_TRANSACTION_QUERY_REQ = 0x26,
> >> + UPIU_TRANSACTION_QUERY_REQ = 0x16,
> >> };
> >> /* UTP UPIU Transaction Codes Target to Initiator */
> >> @@ -97,8 +99,19 @@ enum {
> >>UPIU_TASK_ATTR_ACA = 0x03,
> >> };
> >> -/* UTP QUERY Transaction Specific Fields OpCode */
> >> +/* UPIU Query request function */
> >> enum {
> >> + UPIU_QUERY_FUNC_STANDARD_READ_REQUEST = 0x01,
> >> + UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST = 0x81,
> >> +};
> >> +
> >> +/* Flag idn for Query Requests*/
> >> +enum flag_idn {
> >> + QUERY_FLAG_IDN_FDEVICEINIT = 0x01,
> >> +};
> >> +
> >> +/* UTP QUERY Transaction Specific Fields OpCode */
> >> +enum query_opcode {
> >>UPIU_QUERY_OPCODE_NOP = 0x0,
> >>UPIU_QUERY_OPCODE_READ_DESC = 0x1,
> >>UPIU_QUERY_OPCODE_WRITE_DESC= 0x2,
> >> @@ -110,6 +123,21 @@ enum {
> >>UPIU_QUERY_OPCODE_TOGGLE_FLAG = 0x8,
> >> };
> >> +/* Query response result code */
> >> +enum {
> >> + QUERY_RESULT_SUCCESS= 0x00,
> >> + QUERY_RESULT_NOT_READABLE = 0xF6,
> >> + QUERY_RESULT_NOT_WRITEABLE = 0xF7,
> >> + QUERY_RESULT_ALREADY_WRITTEN= 0xF8,
> >> + QUERY_RESULT_INVALID_LENGTH = 0xF9,
> >> + QUERY_RESULT_INVALID_VALUE = 0xFA,
> >> + QUERY_RESULT_INVALID_SELECTOR = 0xFB,
> >> + QUERY_RESULT_INVALID_INDEX = 0xFC,
> >> + QUERY_RESULT_INVALID_IDN= 0xFD,
> >> + QUERY_RESULT_INVALID_OPCODE = 0xFE,
> >> + QUERY_RESULT_GENERAL_FAILURE= 0xFF,
> >> +};
> >> +
> >> /* UTP Transfer Request Command Type (CT) */
> >> enum {
> >>UPIU_COMMAND_SET_TYPE_SCSI = 0x0,
> >> @@ -127,6 +155,7 @@ enum {
> >>MASK_SCSI_STATUS= 0xFF,
> >>MASK_TASK_RESPONSE = 0xFF00,
> >>MASK_RSP_UPIU_RESULT= 0x,
> >> + MASK_QUERY_DATA_SEG_LEN = 0x,
> >> };
> >> /* Task management service response */
> >> @@ -160,13 +189,40 @@ struct utp_upiu_cmd {
> >> };
> >> /**
> >> + * struct utp_upiu_query - upiu request buffer structure for
> >> + * query request.
> >> + * @opcode: command to perform B-0
> >> + * @idn: a value that indicates the particular type of data B-1 + *
> @index: Index to further identify data B-2
> >> + * @selector: Index to further identify data B-3
> >> + * @reserved_osf: spec reserved field B-4,5
> >> + * @length: number of descriptor bytes to read/write B-6,7
> >> + * @value: Attribute value to be written DW-5
> >> + * @reserved: spec reserved DW-6,7
> >> + */
> >> +struct utp_upiu_query {
> >> + u8 opcode;
> >> + u8 idn;
> >> + u8 index;
> >> + u8 selector;
> >> + u16 reserved_osf;
> >> + u16 length;
> >> + u32 value;
> >> + u32 reserved[2];
> >> +};
> >> +
> >> +/**
> >> * struct utp_upiu_req - general upiu request structure
> >> * @header:UPIU header structure DW-0 to DW-2
> >> * @sc: fields structure for scsi command DW-3 to DW-7
> >> + * @qr: fields structure for query request DW-3 to DW-7
> >> */
> >> struct utp_upiu_req {
> >>struct utp_upiu_header header;
> >> - struct utp_upiu_cmd sc;
> >> + union {
> >> + struct utp_upiu_cmd sc;
> >> + struct utp_upiu_query qr;
> >> + };
> >> };
> >> /**
> >> @@ -187,10 +243,14 @@ struct utp_
RE: mpt2sas,mpt3sas watchdog device removal
Hi Joe, >-Original Message- >From: Joe Lawrence [mailto:joe.lawre...@stratus.com] >Sent: Tuesday, July 16, 2013 8:52 PM >To: James Bottomley >Cc: Reddy, Sreekanth; linux-scsi@vger.kernel.org >Subject: Re: mpt2sas,mpt3sas watchdog device removal > >On Tue, 16 Jul 2013 16:03:38 +0400 >James Bottomley wrote: > >> On Tue, 2013-07-16 at 17:30 +0530, Reddy, Sreekanth wrote: >> > James, >> > >> > This patch seem to be fine. Please consider this patch. >> >> Where's the new version? The one that has all of this fixed: >> >> > Off list, Sreekanth from LSI tested and noticed a few issues with >> > this >> > patch: >> > >> > - mpt2sas_base_stop_watchdog is called twice: The call from >> >mpt2sas_base_detach is safe, but now unnecessary (as a call was >> >added earlier up in the PCI driver callbacks to ensure that the >> >watchdog was out of the way.) This second invocation can be >> > removed. >> > >> > - If the watchdog detects a bad IOC, the watchdog remains running: >> >The watchdog workqueue isn't cleaned up until >> >mpt2sas_base_stop_watchdog is called, so in the case that the >> >watchdog removes the device from SCSI topo, the workqueue will >> >remain unused until PCI .remove/.shutdown cleans it up. Perhaps a >> >single watchdog that iterates over all adapters would be simpler? >> > >> > Finally, if SCSI topo detachment is all that is interesting here, >> > would it make more sense to move the watchdog into the MPT "scsi" >> > code? I haven't looked at the code yet, but this might make an MPT >> > fusion patch easier (due to dependencies between its "scsi" and >> > "base" modules). > >This patch fizzled out in May as other work took priority. If LSI is still >interested in these changes, I can dust off my notes and test/rebase for the >3.11 series. It would be grate help if you post the updated patch which will accommodate the fixes for above comments. > >A few of the issues quoted above are easily fixed, however I remember >having an outstanding question of how to best clean up the driver's per device >watchdog workqueue: > >The way the MPT drivers are working right now is that the watchdog >workqueue function _base_fault_reset_work() initiates a PCI device removal >via kthread. The PCI callback kthread context then tears down the device and >cancel/flush/destroys the watchdog workqueue. > >This patch eliminated the kthread and its call into PCI API, simply detaching >from the SCSI midlayer. In my opinion, the kthread complicated device >removal and introduced potential races if the watchdog tried removing the >device at the same time an ordinary device removal request occurred. > >At the time, the best solution I had was to leave the unused workqueue >around until its PCI device was removed. > >Regards, > >-- Joe Regards, Sreekanth -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/4] [SCSI] sg: fix race condition in sg_open
There is a race when open sg with O_EXCL flag. Also a race may happen between sg_open and sg_remove. Changes from v3: * release o_sem in sg_release(), not in sg_remove_sfp(). * not set exclude with sfd_lock held. Vaughan Cao (4): [SCSI] sg: use rwsem to solve race during exclusive open [SCSI] sg: no need sg_open_exclusive_lock [SCSI] sg: checking sdp->detached isn't protected when open [SCSI] sg: push file descriptor list locking down to per-device locking drivers/scsi/sg.c | 186 ++ 1 file changed, 88 insertions(+), 98 deletions(-) -- 1.7.11.7 -- 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 v4 2/4] [SCSI] sg: no need sg_open_exclusive_lock
Open exclusive check is protected by o_sem, no need sg_open_exclusive_lock.
@exclude is used to record which type of rwsem we are holding.
Signed-off-by: Vaughan Cao
---
drivers/scsi/sg.c | 34 +-
1 file changed, 5 insertions(+), 29 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index edc395a..671b760 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -103,8 +103,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
static int sg_add(struct device *, struct class_interface *);
static void sg_remove(struct device *, struct class_interface *);
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
static DEFINE_IDR(sg_index_idr);
static DEFINE_RWLOCK(sg_index_lock); /* Also used to lock
file descriptor list
for device */
@@ -174,7 +172,6 @@ typedef struct sg_device { /* holds the state of each scsi
generic device */
struct list_head sfds;
struct rw_semaphore o_sem; /* exclude open should hold this rwsem
*/
volatile char detached; /* 0->attached, 1->detached pending removal */
- /* exclude protected by sg_open_exclusive_lock */
char exclude; /* opened for exclusive access */
char sgdebug; /* 0->off, 1->sense, 9->dump dev, 10-> all devs
*/
struct gendisk *disk;
@@ -224,27 +221,6 @@ static int sg_allow_access(struct file *filp, unsigned
char *cmd)
return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
}
-static int get_exclude(Sg_device *sdp)
-{
- unsigned long flags;
- int ret;
-
- spin_lock_irqsave(&sg_open_exclusive_lock, flags);
- ret = sdp->exclude;
- spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
- return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&sg_open_exclusive_lock, flags);
- sdp->exclude = val;
- spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
- return val;
-}
-
static int sfds_list_empty(Sg_device *sdp)
{
unsigned long flags;
@@ -317,7 +293,7 @@ sg_open(struct inode *inode, struct file *filp)
}
/* Since write lock is held, no need to check sfd_list */
if (flags & O_EXCL)
- set_exclude(sdp, 1);
+ sdp->exclude = 1; /* used by release lock */
if (sdp->detached) {
retval = -ENODEV;
@@ -339,7 +315,7 @@ sg_open(struct inode *inode, struct file *filp)
if (retval) {
sem_out:
if (flags & O_EXCL) {
- set_exclude(sdp, 0);/* undo if error */
+ sdp->exclude = 0; /* undo if error */
up_write(&sdp->o_sem);
} else
up_read(&sdp->o_sem);
@@ -366,8 +342,8 @@ sg_release(struct inode *inode, struct file *filp)
return -ENXIO;
SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
- excl = get_exclude(sdp);
- set_exclude(sdp, 0);
+ excl = sdp->exclude;
+ sdp->exclude = 0;
if (excl)
up_write(&sdp->o_sem);
else
@@ -2637,7 +2613,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s,
void *v)
scsidp->lun,
scsidp->host->hostt->emulated);
seq_printf(s, " sg_tablesize=%d excl=%d\n",
- sdp->sg_tablesize, get_exclude(sdp));
+ sdp->sg_tablesize, sdp->exclude);
sg_proc_debug_helper(s, sdp);
}
read_unlock_irqrestore(&sg_index_lock, iflags);
--
1.7.11.7
--
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 v4 3/4] [SCSI] sg: checking sdp->detached isn't protected when open
@detached is set under the protection of sg_index_lock. Without getting the
lock, new sfp will be added during sg removal and there is no chance for it
to be picked out. So check with sg_index_lock held in sg_add_sfp().
Signed-off-by: Vaughan Cao
---
drivers/scsi/sg.c | 26 ++
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 671b760..4d2a19f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -196,7 +196,7 @@ static void sg_remove_scat(Sg_scatter_hold * schp);
static void sg_build_reserve(Sg_fd * sfp, int req_size);
static void sg_link_reserve(Sg_fd * sfp, Sg_request * srp, int size);
static void sg_unlink_reserve(Sg_fd * sfp, Sg_request * srp);
-static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev);
+static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev, int * reason);
static void sg_remove_sfp(struct kref *);
static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id);
static Sg_request *sg_add_request(Sg_fd * sfp);
@@ -295,21 +295,14 @@ sg_open(struct inode *inode, struct file *filp)
if (flags & O_EXCL)
sdp->exclude = 1; /* used by release lock */
- if (sdp->detached) {
- retval = -ENODEV;
- goto sem_out;
- }
if (sfds_list_empty(sdp)) { /* no existing opens on this device */
sdp->sgdebug = 0;
q = sdp->device->request_queue;
sdp->sg_tablesize = queue_max_segments(q);
}
- if ((sfp = sg_add_sfp(sdp, dev)))
- filp->private_data = sfp;
- else {
- retval = -ENOMEM;
+ if (!(sfp = sg_add_sfp(sdp, dev, &retval)))
goto sem_out;
- }
+ filp->private_data = sfp;
retval = 0;
if (retval) {
@@ -2047,15 +2040,18 @@ sg_remove_request(Sg_fd * sfp, Sg_request * srp)
}
static Sg_fd *
-sg_add_sfp(Sg_device * sdp, int dev)
+sg_add_sfp(Sg_device * sdp, int dev, int * reason)
{
Sg_fd *sfp;
unsigned long iflags;
int bufflen;
sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
- if (!sfp)
+ if (!sfp) {
+ if (reason)
+ *reason = -ENOMEM;
return NULL;
+ }
init_waitqueue_head(&sfp->read_wait);
rwlock_init(&sfp->rq_list_lock);
@@ -2070,6 +2066,12 @@ sg_add_sfp(Sg_device * sdp, int dev)
sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
sfp->parentdp = sdp;
write_lock_irqsave(&sg_index_lock, iflags);
+ if (sdp->detached) {
+ write_unlock_irqrestore(&sg_index_lock, iflags);
+ if (reason)
+ *reason = -ENODEV;
+ return NULL;
+ }
list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
write_unlock_irqrestore(&sg_index_lock, iflags);
SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
--
1.7.11.7
--
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 v4 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking
Push file descriptor list locking down to per-device locking. Let sg_index_lock
only protect device lookup.
sdp->detached is also set and checked with this lock held.
Signed-off-by: Vaughan Cao
---
drivers/scsi/sg.c | 61 ++-
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 4d2a19f..3ea7c09 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -104,8 +104,7 @@ static int sg_add(struct device *, struct class_interface
*);
static void sg_remove(struct device *, struct class_interface *);
static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock); /* Also used to lock
- file descriptor list
for device */
+static DEFINE_RWLOCK(sg_index_lock);
static struct class_interface sg_interface = {
.add_dev= sg_add,
@@ -142,8 +141,7 @@ typedef struct sg_request { /* SG_MAX_QUEUE requests
outstanding per file */
} Sg_request;
typedef struct sg_fd { /* holds the state of a file descriptor */
- /* sfd_siblings is protected by sg_index_lock */
- struct list_head sfd_siblings;
+ struct list_head sfd_siblings; /* protected by sfd_lock of device */
struct sg_device *parentdp; /* owning device */
wait_queue_head_t read_wait;/* queue read until command done */
rwlock_t rq_list_lock; /* protect access to list in req_arr */
@@ -168,7 +166,7 @@ typedef struct sg_device { /* holds the state of each scsi
generic device */
struct scsi_device *device;
int sg_tablesize; /* adapter's max scatter-gather table size */
u32 index; /* device index number */
- /* sfds is protected by sg_index_lock */
+ spinlock_t sfd_lock;/* protect file descriptor list for device */
struct list_head sfds;
struct rw_semaphore o_sem; /* exclude open should hold this rwsem
*/
volatile char detached; /* 0->attached, 1->detached pending removal */
@@ -226,9 +224,9 @@ static int sfds_list_empty(Sg_device *sdp)
unsigned long flags;
int ret;
- read_lock_irqsave(&sg_index_lock, flags);
+ spin_lock_irqsave(&sdp->sfd_lock, flags);
ret = list_empty(&sdp->sfds);
- read_unlock_irqrestore(&sg_index_lock, flags);
+ spin_unlock_irqrestore(&sdp->sfd_lock, flags);
return ret;
}
@@ -1391,6 +1389,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct
scsi_device *scsidp)
disk->first_minor = k;
sdp->disk = disk;
sdp->device = scsidp;
+ spin_lock_init(&sdp->sfd_lock);
INIT_LIST_HEAD(&sdp->sfds);
init_rwsem(&sdp->o_sem);
sdp->sg_tablesize = queue_max_segments(q);
@@ -1533,11 +1532,13 @@ static void sg_remove(struct device *cl_dev, struct
class_interface *cl_intf)
/* Need a write lock to set sdp->detached. */
write_lock_irqsave(&sg_index_lock, iflags);
+ spin_lock(&sdp->sfd_lock);
sdp->detached = 1;
list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
wake_up_interruptible(&sfp->read_wait);
kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
}
+ spin_unlock(&sdp->sfd_lock);
write_unlock_irqrestore(&sg_index_lock, iflags);
sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2065,15 +2066,15 @@ sg_add_sfp(Sg_device * sdp, int dev, int * reason)
sfp->cmd_q = SG_DEF_COMMAND_Q;
sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
sfp->parentdp = sdp;
- write_lock_irqsave(&sg_index_lock, iflags);
+ spin_lock_irqsave(&sdp->sfd_lock, iflags);
if (sdp->detached) {
- write_unlock_irqrestore(&sg_index_lock, iflags);
+ spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
if (reason)
*reason = -ENODEV;
return NULL;
}
list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
- write_unlock_irqrestore(&sg_index_lock, iflags);
+ spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
if (unlikely(sg_big_buff != def_reserved_size))
sg_big_buff = def_reserved_size;
@@ -2123,9 +2124,9 @@ static void sg_remove_sfp(struct kref *kref)
struct sg_device *sdp = sfp->parentdp;
unsigned long iflags;
- write_lock_irqsave(&sg_index_lock, iflags);
+ spin_lock_irqsave(&sdp->sfd_lock, iflags);
list_del(&sfp->sfd_siblings);
- write_unlock_irqrestore(&sg_index_lock, iflags);
+ spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
schedule_work(&sfp->ew.work);
@@ -2516,7 +2517,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s,
void *v)
return 0;
}
-/* must be called while holding sg_index_l
[PATCH v4 1/4] [SCSI] sg: use rwsem to solve race during exclusive open
A race condition may happen if two threads are both trying to open the same sg
with O_EXCL simultaneously. It's possible that they both find fsds list is
empty and get_exclude(sdp) returns 0, then they both call set_exclude() and
break out from wait_event_interruptible and resume open.
Now use rwsem to protect this process. Exclusive open gets write lock and
others get read lock. The lock will be held until file descriptor is closed.
This also leads 'exclude' only a status rather than a check mark.
Signed-off-by: Vaughan Cao
---
drivers/scsi/sg.c | 77 ++-
1 file changed, 42 insertions(+), 35 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 25b5455..edc395a 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -168,11 +168,11 @@ typedef struct sg_fd {/* holds the state of a
file descriptor */
typedef struct sg_device { /* holds the state of each scsi generic device */
struct scsi_device *device;
- wait_queue_head_t o_excl_wait; /* queue open() when O_EXCL in use */
int sg_tablesize; /* adapter's max scatter-gather table size */
u32 index; /* device index number */
/* sfds is protected by sg_index_lock */
struct list_head sfds;
+ struct rw_semaphore o_sem; /* exclude open should hold this rwsem
*/
volatile char detached; /* 0->attached, 1->detached pending removal */
/* exclude protected by sg_open_exclusive_lock */
char exclude; /* opened for exclusive access */
@@ -293,35 +293,35 @@ sg_open(struct inode *inode, struct file *filp)
goto error_out;
}
- if (flags & O_EXCL) {
- if (O_RDONLY == (flags & O_ACCMODE)) {
- retval = -EPERM; /* Can't lock it with read only access
*/
- goto error_out;
- }
- if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
- retval = -EBUSY;
- goto error_out;
- }
- res = wait_event_interruptible(sdp->o_excl_wait,
- ((!sfds_list_empty(sdp) ||
get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
- if (res) {
- retval = res; /* -ERESTARTSYS because signal hit
process */
- goto error_out;
- }
- } else if (get_exclude(sdp)) { /* some other fd has an exclusive lock
on dev */
- if (flags & O_NONBLOCK) {
- retval = -EBUSY;
- goto error_out;
- }
- res = wait_event_interruptible(sdp->o_excl_wait,
!get_exclude(sdp));
- if (res) {
- retval = res; /* -ERESTARTSYS because signal hit
process */
- goto error_out;
+ if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+ retval = -EPERM; /* Can't lock it with read only access */
+ goto error_out;
+ }
+ if (flags & O_NONBLOCK) {
+ if (flags & O_EXCL) {
+ if (!down_write_trylock(&sdp->o_sem)) {
+ retval = -EBUSY;
+ goto error_out;
+ }
+ } else {
+ if (!down_read_trylock(&sdp->o_sem)) {
+ retval = -EBUSY;
+ goto error_out;
+ }
}
+ } else {
+ if (flags & O_EXCL)
+ down_write(&sdp->o_sem);
+ else
+ down_read(&sdp->o_sem);
}
+ /* Since write lock is held, no need to check sfd_list */
+ if (flags & O_EXCL)
+ set_exclude(sdp, 1);
+
if (sdp->detached) {
retval = -ENODEV;
- goto error_out;
+ goto sem_out;
}
if (sfds_list_empty(sdp)) { /* no existing opens on this device */
sdp->sgdebug = 0;
@@ -331,16 +331,19 @@ sg_open(struct inode *inode, struct file *filp)
if ((sfp = sg_add_sfp(sdp, dev)))
filp->private_data = sfp;
else {
- if (flags & O_EXCL) {
- set_exclude(sdp, 0);/* undo if error */
- wake_up_interruptible(&sdp->o_excl_wait);
- }
retval = -ENOMEM;
- goto error_out;
+ goto sem_out;
}
retval = 0;
-error_out:
+
if (retval) {
+sem_out:
+ if (flags & O_EXCL) {
+ set_exclude(sdp, 0);/* undo if error */
+ up_write(&sdp->o_sem);
+ } else
+ up_read(&sdp->o_sem);
+error_out:
scsi_autopm_put_device(sdp->device);
sdp_put:
Re: [PATCH RESEND 0/1] AHCI: Optimize interrupt processing
On Tue, Jul 16, 2013 at 02:38:03PM -0700, Nicholas A. Bellinger wrote: > [7.927818] scsi_execute(): Calling blk_mq_free_request >>> > [7.927826] scsi 0:0:0:0: Direct-Access ATA ST9500530NS CC03 > PQ: 0 ANSI: 5 > > OK, so INQUIRY response payload is looking as expected here. Yep. It is not on the top of my head, but I remember something like INQUIRYs are emulated and thus do not have payload. > [7.927960] sd 0:0:0:0: [sda] Sector size 0 reported, assuming 512. > [7.927964] sd 0:0:0:0: [sda] 1 512-byte logical blocks: (512 B/512 B) > [7.927965] sd 0:0:0:0: [sda] 0-byte physical blocks > > Strange.. READ_CAPACITY appears to be returning a payload as zeros..? Yep. Because blk_execute_rq() does not put the proper callback and data do not get copied from sg's to bounce buffer. That is why I tried to use blk_mq_execute_rq() instead. Once I do that, data start getting read and booting stops elsewhere. Of course, I was suspecting that change alone is not valid and wondered about the status of scsi-mq in the first place, and if more changes are coming. So I it turns out "req->errors + req->resid_len" issue (you described earlier) needs to be addressed before going forward with libata (only?). > Not sure why yet some control CDBs are getting back the expected > payload, while others are returning zeros.. Bio buffers do not get updated from callback. > Also, looking at the included stack back-trace: [...] Thanks a lot for these and other your comments, Nicholas! -- Regards, Alexander Gordeev agord...@redhat.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
Re: [PATCH 6/7] qla4xxx: Export more firmware info in sysfs
On 07/08/2013 06:33 AM, adheer.chandravan...@qlogic.com wrote:
> static ssize_t
> @@ -181,8 +179,8 @@ qla4xxx_iscsi_version_show(struct device *dev, struct
> device_attribute *attr,
> char *buf)
> {
> struct scsi_qla_host *ha = to_qla_host(class_to_shost(dev));
> - return snprintf(buf, PAGE_SIZE, "%d.%02d\n", ha->iscsi_major,
> - ha->iscsi_minor);
> + return snprintf(buf, PAGE_SIZE, "%d.%02d\n", ha->fw_info.iscsi_major,
> + ha->fw_info.iscsi_minor);
> }
Is this the same as the iscsi max and min from the iscsi spec? If so, it
should be a iscsi host setting.
--
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 V3 1/2] scsi: ufs: Add support for host assisted background operations
> >I'm not sure that BKOPS with runtime-pm associates.
> >Do you think it's helpful for power management?
> >How about hibernation scheme for runtime-pm?
> >I'm testing and I can introduce soon.
>
>Well, I am thinking on following approach when we introduce
>power management.
>
>ufshcd_runtime_suspend() {
>if (bkops_status >= NON_CRITICAL) { /* 0x1 */
>ufshcd_enable_auto_bkops();
>hibernate(); /* only the link and the device
>should be able to cary out bkops */
>} else {
>hibernate(); /* Link and the device for more savings */
>}
>}
>
>Let me know if this is okay.
I still consider whether BKOPS is proper behavior with runtime-pm or not.
The BKOPS is something that host allows the card to carry out
when the host knows it is idle and not expecting back to back requests.
Runtime PM idle is the only way to know whether the device is
idle (unless we want to reinvent the wheel to detect the idleness of
host and trigger bkops). There was a discussion on this in MMC mailing
list as well, and folks have agreed to move idle time bkops to runtime
PM (http://thread.gmane.org/gmane.linux.kernel.mmc/19444/)
How about this scenario? It seems more simple.
If we concern a response latency of transfer requests, BKOPS can be disabled by
default.
And then BKOPS can be enabled whenever device requests in some exception.
If you have any idea, let me know.
Exceptions are raised only when the device is in critical need for
bkops. Also the spec. recommends, host should ensure that the device
doesn't go into such states.
With your suggestion, when we disable bkops, the exception is raised and
we enable bkops after which there is no way to disable it again?
--
Regards,
Sujit
--
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 V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU
+ * ufshcd_wait_for_register - wait for register value to change
+ * @hba - per-adapter interface
+ * @reg - mmio register offset
+ * @mask - mask to apply to read register value
+ * @val - wait condition
+ * @interval_us - polling interval in microsecs
+ * @timeout_ms - timeout in millisecs
+ *
+ * Returns final register value after iteration
+ */
+static u32 ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
+ u32 val, unsigned long interval_us, unsigned long timeout_ms)
I feel like this function's role is to wait for clearing register (specially,
doorbells).
If you really don't intend to apply other all register, I think it would better
to change the
function name.
ex> ufshcd_wait_for_clear_doorbell or ufshcd_wait_for_clear_reg?
Although, this API is introduced in this patch and used only for
clearing the doorbell, I have intentionally made it generic to avoid
duplication of wait condition code in future (not just clearing but
also for setting a bit). For example, when we write to HCE.ENABLE we
wait for it turn to 1.
And if you like it, it could be more simple like below
static bool ufshcd_wait_for_clear_reg(struct ufs_hba *hba, u32 reg, u32 mask,
unsigned long interval_us,
unsigned int timeout_ms)
{
unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
Jiffies on 100Hz systems have minimum granularity of 10ms. So we really
wait for more 10ms even though the timeout_ms < 10ms.
Yes. Real timeout depends on system.
But normally actual wait will be maintained until 'ufshcd_readl' is done.
Timeout is valid for failure case. If we don't need to apply a strict timeout
value, it's not bad.
Hmm.. make sense. Will take care in the next patchset.
/* wakeup within 50us of expiry */
const unsigned int expiry = 50;
while (ufshcd_readl(hba, reg) & mask) {
usleep_range(interval_us, interval_us + expiry);
if (time_after(jiffies, timeout)) {
if (ufshcd_readl(hba, reg) & mask)
return false;
I really want the caller to decide on what to do after the timeout.
This helps in error handling cases where we try to clear off the entire
door-bell register but only a few of the bits are cleared after timeout.
I don't know what we can do with the report of the partial success on clearing
bit.
It's just failure case. Isn't enough with false/true?
The point is, if a bit can't be cleared it can be regarded as a
permanent failure (only for that request), otherwise, it can be
retried with the same tag value.
else
return true;
}
}
return true;
}
+{
+ u32 tmp;
+ ktime_t start;
+ unsigned long diff;
+
+ tmp = ufshcd_readl(hba, reg);
+
+ if ((val & mask) != val) {
+ dev_err(hba->dev, "%s: Invalid wait condition 0x%x\n",
+ __func__, val);
+ goto out;
+ }
+
+ start = ktime_get();
+ while ((tmp & mask) != val) {
+ /* wakeup within 50us of expiry */
+ usleep_range(interval_us, interval_us + 50);
+ tmp = ufshcd_readl(hba, reg);
+ diff = ktime_to_ms(ktime_sub(ktime_get(), start));
+ if (diff > timeout_ms) {
+ tmp = ufshcd_readl(hba, reg);
+ break;
+ }
+ }
+out:
+ return tmp;
+}
+
..
+static int
+ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
+{
+ int err = 0;
+ unsigned long flags;
+ u32 reg;
+ u32 mask = 1 << tag;
+
+ /* clear outstanding transaction before retry */
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ ufshcd_utrl_clear(hba, tag);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ /*
+* wait for for h/w to clear corresponding bit in door-bell.
+* max. wait is 1 sec.
+*/
+ reg = ufshcd_wait_for_register(hba,
+ REG_UTP_TRANSFER_REQ_DOOR_BELL,
+ mask, 0, 1000, 1000);
4th argument should be (~mask) instead of '0', right?
True, but not really for this implementation. It breaks the check in
in wait_for_register -
if ((val & mask) != val)
dev_err(...);
Hmm, it seems complicated to use.
Ok. Is right the following about val as 4th argument?
- clear: val should be '0' regardless corresponding bit.
- set: val should be same with mask.
If the related comment is added, it will be helpful.
Thinking again it looks like it is complicated. How about changing
the check to -
val = val & mask; /* ignore the bits we don't intend to wait on */
while (ufshcd_readl() & mask != val) {
sleep
}
Usage will be ~mask for clearing the bits, mask for setting the bits
in
