Re: [PATCH RESEND 0/1] AHCI: Optimize interrupt processing
On Thu, 2013-07-18 at 18:03 -0700, Nicholas A. Bellinger wrote: On Thu, 2013-07-18 at 18:30 -0600, Jens Axboe wrote: On Thu, Jul 18 2013, Nicholas A. Bellinger wrote: On Thu, 2013-07-18 at 13:12 -0600, Mike Christie wrote: On 07/18/2013 12:51 PM, Nicholas A. Bellinger wrote: SNIP nod, just noticed the blk_queue_bounce() in blk_rq_map_kern(). Not sure why this doesn't seem to be doing what it's supposed to for libata just yet.. How are you make the request from the bio? It'd be pretty trivial to ensure that it gets bounced properly... blk_mq_execute_rq() assumes a fully complete request, so it wont bounce anything. From what I gather for REQ_TYPE_BLOCK_PC, scsi_execute() - blk_rq_map_kern() - blk_rq_append_bio() - blk_rq_bio_prep() is what does the request setup from the bios returned by bio_[copy,map]_kern() in blk_rq_map_kern() code. blk_queue_bounce() is called immediately after blk_rq_append_bio() here, which AFAICT looks like it's doing the correct thing for scsi-mq.. What is strange here is that libata-scsi.c CDB emulation code is doing the same stuff for both INQUIRY (that seems to be OK) and READ_CAPACITY (that is returning zeros), which makes me think that something else is going on.. Alexander, where you able to re-test using sdev-sdev_mq_reg.queue_depth = 1 in scsi_mq_alloc_queue()..? So after a bit more hacking tonight it appears the explicit setting of dma_alignment by libata (sdev-sector_size - 1) is what is making blk_rq_map_kern() invoke blk_copy_kern() instead of blk_map_kern(), and triggering this scsi-mq specific bug with libata. I'm able to confirm with QEMU IDE emulation the bug is occurring only after INQUIRY and before READ_CAPACITY, as reported by Alexander. Below is a quick hack to skip this setting in ata_scsi_dev_config() for blk-mq, and leaves the default dma_alignment=0x03 for REQ_TYPE_BLOCK_PC requests as initially set by scsi-core in scsi_init_request_queue(). Also included is the change for using queue_depth = min(SHT-can_queue, SHT-cmd_per_lun) during scsi-mq request_queue initialization, along with a very basic ata_piix conversion for testing purposes. With these three changes in place, I'm able to register a single 1GB ata_piix LUN using QEMU IDE emulation, and successfully run simple fio writeverify tests with blocksize=4k @ queue_depth=1. Obviously this is not a proper bugfix for unaligned blk_copy_kern() with scsi-mq + REQ_TYPE_BLOCK_PC case, but should be enough to at least get libata LUN scanning to work. Need to take a deeper look at the problem here.. Alexander, care to give this work-around a shot on your bare-metal setup using ata_piix..? --nab diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c index 9a8a674..ac05cd6 100644 --- a/drivers/ata/ata_piix.c +++ b/drivers/ata/ata_piix.c @@ -1066,6 +1066,8 @@ static u8 piix_vmw_bmdma_status(struct ata_port *ap) static struct scsi_host_template piix_sht = { ATA_BMDMA_SHT(DRV_NAME), + .scsi_mq= true, + .queuecommand_mq = ata_scsi_queuecmd, }; static struct ata_port_operations piix_sata_ops = { diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 0101af5..191bc15 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1144,7 +1144,11 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, sector_size=%u PAGE_SIZE, PIO may malfunction\n, sdev-sector_size); - blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + if (!q-mq_ops) { + blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + } else { + printk(Skipping dma_alignment for libata w/ scsi-mq\n); + } if (dev-flags ATA_DFLAG_AN) set_bit(SDEV_EVT_MEDIA_CHANGE, sdev-supported_events); diff --git a/drivers/scsi/scsi-mq.c b/drivers/scsi/scsi-mq.c index ca6ff67..81b2633 100644 --- a/drivers/scsi/scsi-mq.c +++ b/drivers/scsi/scsi-mq.c @@ -199,11 +199,11 @@ int scsi_mq_alloc_queue(struct Scsi_Host *sh, struct scsi_device *sdev) int i, j; sdev-sdev_mq_reg.ops = scsi_mq_ops; - sdev-sdev_mq_reg.queue_depth = sdev-queue_depth; + sdev-sdev_mq_reg.queue_depth = min((short)sh-hostt-can_queue, + sh-hostt-cmd_per_lun); sdev-sdev_mq_reg.cmd_size = sizeof(struct scsi_cmnd) + sh-hostt-cmd_size; sdev-sdev_mq_reg.numa_node = NUMA_NO_NODE; sdev-sdev_mq_reg.nr_hw_queues = 1; - sdev-sdev_mq_reg.queue_depth = 64; sdev-sdev_mq_reg.flags = BLK_MQ_F_SHOULD_MERGE; printk(Calling blk_mq_init_queue: scsi_mq_ops: %p, queue_depth: %d, -- 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] scsi: replace strict_strtoul() with kstrtoul()
The usage of strict_strtoul() is not preferred, because strict_strtoul() is obsolete. Thus, kstrtoul() should be used. Signed-off-by: Jingoo Han jg1@samsung.com --- drivers/scsi/pmcraid.c|6 -- drivers/scsi/scsi_sysfs.c |6 -- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c index 1eb7b028..8cd98e6 100644 --- a/drivers/scsi/pmcraid.c +++ b/drivers/scsi/pmcraid.c @@ -4203,9 +4203,11 @@ static ssize_t pmcraid_store_log_level( struct Scsi_Host *shost; struct pmcraid_instance *pinstance; unsigned long val; + int ret; - if (strict_strtoul(buf, 10, val)) - return -EINVAL; + ret = kstrtoul(buf, 10, val); + if (ret) + return ret; /* log-level should be from 0 to 2 */ if (val 2) return -EINVAL; diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 7e50061..a876e9b 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -819,9 +819,11 @@ sdev_store_queue_ramp_up_period(struct device *dev, { struct scsi_device *sdev = to_scsi_device(dev); unsigned long period; + int ret; - if (strict_strtoul(buf, 10, period)) - return -EINVAL; + ret = kstrtoul(buf, 10, period); + if (ret) + return ret; sdev-queue_ramp_up_period = msecs_to_jiffies(period); return period; -- 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
[PATCH] target: replace strict_strto*() with kstrto*()
The usage of strict_strtoul() and strict_strtoull() is not preferred, because strict_strtoul() and strict_strtoull() are obsolete. Thus, kstrtoul() and kstrtoull() should be used. Signed-off-by: Jingoo Han jg1@samsung.com --- drivers/target/iscsi/iscsi_target_configfs.c |8 ++--- drivers/target/iscsi/iscsi_target_login.c |2 +- drivers/target/iscsi/iscsi_target_parameters.c |2 +- drivers/target/target_core_alua.c | 32 +- drivers/target/target_core_configfs.c | 42 drivers/target/target_core_fabric_configfs.c | 16 ++--- drivers/target/target_core_file.c |4 +-- drivers/target/target_core_iblock.c|4 +-- drivers/target/tcm_fc/tfc_conf.c |6 +++- 9 files changed, 64 insertions(+), 52 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c index bbfd288..ddfa32c 100644 --- a/drivers/target/iscsi/iscsi_target_configfs.c +++ b/drivers/target/iscsi/iscsi_target_configfs.c @@ -265,9 +265,9 @@ static struct se_tpg_np *lio_target_call_addnptotpg( *port_str = '\0'; /* Terminate string for IP */ port_str++; /* Skip over : */ - ret = strict_strtoul(port_str, 0, port); + ret = kstrtoul(port_str, 0, port); if (ret 0) { - pr_err(strict_strtoul() failed for port_str: %d\n, ret); + pr_err(kstrtoul() failed for port_str: %d\n, ret); return ERR_PTR(ret); } sock_in6 = (struct sockaddr_in6 *)sockaddr; @@ -290,9 +290,9 @@ static struct se_tpg_np *lio_target_call_addnptotpg( *port_str = '\0'; /* Terminate string for IP */ port_str++; /* Skip over : */ - ret = strict_strtoul(port_str, 0, port); + ret = kstrtoul(port_str, 0, port); if (ret 0) { - pr_err(strict_strtoul() failed for port_str: %d\n, ret); + pr_err(kstrtoul() failed for port_str: %d\n, ret); return ERR_PTR(ret); } sock_in = (struct sockaddr_in *)sockaddr; diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 3402241..76cf1cd 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -428,7 +428,7 @@ static int iscsi_login_zero_tsih_s2( ISCSI_LOGIN_STATUS_NO_RESOURCES); return -1; } - rc = strict_strtoul(param-value, 0, mrdsl); + rc = kstrtoul(param-value, 0, mrdsl); if (rc 0) { iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, ISCSI_LOGIN_STATUS_NO_RESOURCES); diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c index 35fd643..034bd6d 100644 --- a/drivers/target/iscsi/iscsi_target_parameters.c +++ b/drivers/target/iscsi/iscsi_target_parameters.c @@ -1182,7 +1182,7 @@ static int iscsi_check_acceptor_state(struct iscsi_param *param, char *value, unsigned long long tmp; int rc; - rc = strict_strtoull(param-value, 0, tmp); + rc = kstrtoull(param-value, 0, tmp); if (rc 0) return -1; diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c index cbe48ab..5403186 100644 --- a/drivers/target/target_core_alua.c +++ b/drivers/target/target_core_alua.c @@ -1756,10 +1756,10 @@ ssize_t core_alua_store_access_type( unsigned long tmp; int ret; - ret = strict_strtoul(page, 0, tmp); + ret = kstrtoul(page, 0, tmp); if (ret 0) { pr_err(Unable to extract alua_access_type\n); - return -EINVAL; + return ret; } if ((tmp != 0) (tmp != 1) (tmp != 2) (tmp != 3)) { pr_err(Illegal value for alua_access_type: @@ -1794,10 +1794,10 @@ ssize_t core_alua_store_nonop_delay_msecs( unsigned long tmp; int ret; - ret = strict_strtoul(page, 0, tmp); + ret = kstrtoul(page, 0, tmp); if (ret 0) { pr_err(Unable to extract nonop_delay_msecs\n); - return -EINVAL; + return ret; } if (tmp ALUA_MAX_NONOP_DELAY_MSECS) { pr_err(Passed nonop_delay_msecs: %lu, exceeds @@ -1825,10 +1825,10 @@ ssize_t core_alua_store_trans_delay_msecs( unsigned long tmp; int ret; - ret = strict_strtoul(page, 0, tmp); + ret = kstrtoul(page, 0, tmp); if (ret 0) { pr_err(Unable to
[Bug 59601] commit 97dec564fd4948e0e560869c80b76e166ca2a83e breaks communication with XYRATEX disk shelves
https://bugzilla.kernel.org/show_bug.cgi?id=59601 --- Comment #11 from Saurav Kashyap saurav.kash...@qlogic.com --- Hi Jack, This patch http://marc.info/?l=linux-scsim=137365649318663w=2 is submitted to upstream. Please close this BZ. thanks, ~Saurav -- 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 2/2] qla2xxx: Properly set the tagging for commands.
This should go to stable also, added in to list. This fixes a BZ https://bugzilla.kernel.org/show_bug.cgi?id=59601 Thanks, ~Saurav -Original Message- From: Saurav Kashyap [mailto:saurav.kash...@qlogic.com] Sent: Saturday, July 13, 2013 12:18 AM To: jbottom...@parallels.com Cc: Giridhar Malavali; Saurav Kashyap; Andrew Vasquez; linux-scsi Subject: [PATCH 2/2] qla2xxx: Properly set the tagging for commands. Reported-by: Jack Hill jackh...@jackhill.us Signed-off-by: Saurav Kashyap saurav.kash...@qlogic.com Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com --- drivers/scsi/qla2xxx/qla_iocb.c | 11 +-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 42ef481..ef0a548 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -419,6 +419,8 @@ qla2x00_start_scsi(srb_t *sp) __constant_cpu_to_le16(CF_SIMPLE_TAG); break; } + } else { + cmd_pkt-control_flags = __constant_cpu_to_le16(CF_SIMPLE_TAG); } /* Load SCSI command packet. */ @@ -1307,11 +1309,11 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct cmd_type_crc_2 *cmd_pkt, fcp_cmnd-task_attribute = TSK_ORDERED; break; default: - fcp_cmnd-task_attribute = 0; + fcp_cmnd-task_attribute = TSK_SIMPLE; break; } } else { - fcp_cmnd-task_attribute = 0; + fcp_cmnd-task_attribute = TSK_SIMPLE; } cmd_pkt-fcp_rsp_dseg_len = 0; /* Let response come in status iocb */ @@ -1525,7 +1527,12 @@ qla24xx_start_scsi(srb_t *sp) case ORDERED_QUEUE_TAG: cmd_pkt-task = TSK_ORDERED; break; + default: + cmd_pkt-task = TSK_SIMPLE; + break; } + } else { + cmd_pkt-task = TSK_SIMPLE; } /* Load SCSI command packet. */ -- 1.7.7 attachment: winmail.dat
RE: [PATCH V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU
On Thu, July 18, 2013, Sujit Reddy Thumma wrote: + * 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
RE: [PATCH V3 2/2] scsi: ufs: Set fDeviceInit flag to initiate device initialization
On Thursday, July 18, 2013, Dolev Raviv wrote: 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 dra...@codeaurora.org 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 dra...@codeaurora.org Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- 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; +
RE: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations
On Thu, July 18, 2013, Sujit Reddy Thumma 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. 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/) It looks like different. eMMC cannot execute BKOPS itself unlike UFS. That's the way eMMC's host should trigger the BKOPS manually. 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? Yes, it's difficult to find proper time. Maybe, BKOPS can be disabled when request comes up. Thanks, Seungwon Jeon -- 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 -- 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/4] scsi: ufs: Fix broken task management command implementation
On Tue, July 09, 2013 Sujit Reddy Thumma wrote: Currently, sending Task Management (TM) command to the card might be broken in some scenarios as listed below: Problem: If there are more than 8 TM commands the implementation returns error to the caller. Fix: Wait for one of the slots to be emptied and send the command. Problem: Sometimes it is necessary for the caller to know the TM service response code to determine the task status. Fix: Propogate the service response to the caller. Problem: If the TM command times out no proper error recovery is implemented. Fix: Clear the command in the controller door-bell register, so that further commands for the same slot don't fail. Problem: While preparing the TM command descriptor, the task tag used should be unique across SCSI/NOP/QUERY/TM commands and not the task tag of the command which the TM command is trying to manage. Fix: Use a unique task tag instead of task tag of SCSI command. Problem: Since the TM command involves H/W communication, abruptly ending the request on kill interrupt signal might cause h/w malfunction. Fix: Wait for hardware completion interrupt with TASK_UNINTERRUPTIBLE set. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 177 ++--- drivers/scsi/ufs/ufshcd.h |8 ++- 2 files changed, 126 insertions(+), 59 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index af7d01d..a176421 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -53,6 +53,9 @@ /* Query request timeout */ #define QUERY_REQ_TIMEOUT 30 /* msec */ +/* Task management command timeout */ +#define TM_CMD_TIMEOUT 100 /* msecs */ + /* Expose the flag value from utp_upiu_query.value */ #define MASK_QUERY_UPIU_FLAG_LOC 0xFF @@ -190,13 +193,35 @@ ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp) /** * ufshcd_get_tm_free_slot - get a free slot for task management request * @hba: per adapter instance + * @free_slot: pointer to variable with available slot value * - * Returns maximum number of task management request slots in case of - * task management queue full or returns the free slot number + * Get a free tag and lock it until ufshcd_put_tm_slot() is called. + * Returns 0 if free slot is not available, else return 1 with tag value + * in @free_slot. */ -static inline int ufshcd_get_tm_free_slot(struct ufs_hba *hba) +static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot) +{ + int tag; + bool ret = false; + + if (!free_slot) + goto out; + + do { + tag = find_first_zero_bit(hba-tm_slots_in_use, hba-nutmrs); + if (tag = hba-nutmrs) + goto out; + } while (test_and_set_bit_lock(tag, hba-tm_slots_in_use)); + + *free_slot = tag; + ret = true; +out: + return ret; +} + +static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot) { - return find_first_zero_bit(hba-outstanding_tasks, hba-nutmrs); + clear_bit_unlock(slot, hba-tm_slots_in_use); } /** @@ -1778,10 +1803,11 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev) * ufshcd_task_req_compl - handle task management request completion * @hba: per adapter instance * @index: index of the completed request + * @resp: task management service response * - * Returns SUCCESS/FAILED + * Returns non-zero value on error, zero on success */ -static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index) +static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp) { struct utp_task_req_desc *task_req_descp; struct utp_upiu_task_rsp *task_rsp_upiup; @@ -1802,19 +1828,15 @@ static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index) task_req_descp[index].task_rsp_upiu; task_result = be32_to_cpu(task_rsp_upiup-header.dword_1); task_result = ((task_result MASK_TASK_RESPONSE) 8); - - if (task_result != UPIU_TASK_MANAGEMENT_FUNC_COMPL - task_result != UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) - task_result = FAILED; - else - task_result = SUCCESS; + if (resp) + *resp = (u8)task_result; } else { - task_result = FAILED; - dev_err(hba-dev, - trc: Invalid ocs = %x\n, ocs_value); + dev_err(hba-dev, %s: failed, ocs = 0x%x\n, + __func__, ocs_value); } spin_unlock_irqrestore(hba-host-host_lock, flags); - return task_result; + + return ocs_value; } /** @@ -2298,7 +2320,7 @@ static void ufshcd_tmc_handler(struct ufs_hba
RE: [PATCH V3 3/4] scsi: ufs: Fix device and host reset methods
On Tue, July 09, 2013, Sujit Reddy Thumma wrote: As of now SCSI initiated error handling is broken because, the reset APIs don't try to bring back the device initialized and ready for further transfers. In case of timeouts, the scsi error handler takes care of handling aborts and resets. Improve the error handling in such scenario by resetting the device and host and re-initializing them in proper manner. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 467 +++-- drivers/scsi/ufs/ufshcd.h |2 + 2 files changed, 411 insertions(+), 58 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 51ce096..b4c9910 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -69,9 +69,15 @@ enum { /* UFSHCD states */ enum { - UFSHCD_STATE_OPERATIONAL, UFSHCD_STATE_RESET, UFSHCD_STATE_ERROR, + UFSHCD_STATE_OPERATIONAL, +}; + +/* UFSHCD error handling flags */ +enum { + UFSHCD_EH_HOST_RESET_PENDING = (1 0), + UFSHCD_EH_DEVICE_RESET_PENDING = (1 1), }; /* Interrupt configuration options */ @@ -87,6 +93,22 @@ enum { INT_AGGR_CONFIG, }; +#define ufshcd_set_device_reset_pending(h) \ + (h-eh_flags |= UFSHCD_EH_DEVICE_RESET_PENDING) +#define ufshcd_set_host_reset_pending(h) \ + (h-eh_flags |= UFSHCD_EH_HOST_RESET_PENDING) +#define ufshcd_device_reset_pending(h) \ + (h-eh_flags UFSHCD_EH_DEVICE_RESET_PENDING) +#define ufshcd_host_reset_pending(h) \ + (h-eh_flags UFSHCD_EH_HOST_RESET_PENDING) +#define ufshcd_clear_device_reset_pending(h) \ + (h-eh_flags = ~UFSHCD_EH_DEVICE_RESET_PENDING) +#define ufshcd_clear_host_reset_pending(h) \ + (h-eh_flags = ~UFSHCD_EH_HOST_RESET_PENDING) + +static void ufshcd_tmc_handler(struct ufs_hba *hba); +static void ufshcd_async_scan(void *data, async_cookie_t cookie); + /* * ufshcd_wait_for_register - wait for register value to change * @hba - per-adapter interface @@ -851,9 +873,22 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) tag = cmd-request-tag; - if (hba-ufshcd_state != UFSHCD_STATE_OPERATIONAL) { + switch (hba-ufshcd_state) { Lock is no needed for ufshcd_state? + case UFSHCD_STATE_OPERATIONAL: + break; + case UFSHCD_STATE_RESET: err = SCSI_MLQUEUE_HOST_BUSY; goto out; + case UFSHCD_STATE_ERROR: + set_host_byte(cmd, DID_ERROR); + cmd-scsi_done(cmd); + goto out; + default: + dev_WARN_ONCE(hba-dev, 1, %s: invalid state %d\n, + __func__, hba-ufshcd_state); + set_host_byte(cmd, DID_BAD_TARGET); + cmd-scsi_done(cmd); + goto out; } /* acquire the tag to make sure device cmds don't use it */ @@ -1573,8 +1608,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba) if (hba-ufshcd_state == UFSHCD_STATE_RESET) scsi_unblock_requests(hba-host); - hba-ufshcd_state = UFSHCD_STATE_OPERATIONAL; - out: return err; } @@ -2273,6 +2306,106 @@ out: } /** + * ufshcd_utrl_is_rsr_enabled - check if run-stop register is enabled + * @hba: per-adapter instance + */ +static bool ufshcd_utrl_is_rsr_enabled(struct ufs_hba *hba) +{ + return ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_LIST_RUN_STOP) 0x1; +} + +/** + * ufshcd_utmrl_is_rsr_enabled - check if run-stop register is enabled + * @hba: per-adapter instance + */ +static bool ufshcd_utmrl_is_rsr_enabled(struct ufs_hba *hba) +{ + return ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_RUN_STOP) 0x1; +} + +/** + * ufshcd_complete_pending_tasks - complete outstanding tasks + * @hba: per adapter instance + * + * Abort in-progress task management commands and wakeup + * waiting threads. + * + * Returns non-zero error value when failed to clear all the commands. + */ +static int ufshcd_complete_pending_tasks(struct ufs_hba *hba) +{ + u32 reg; + int err = 0; + unsigned long flags; + + if (!hba-outstanding_tasks) + goto out; + + /* Clear UTMRL only when run-stop is enabled */ + if (ufshcd_utmrl_is_rsr_enabled(hba)) + ufshcd_writel(hba, ~hba-outstanding_tasks, + REG_UTP_TASK_REQ_LIST_CLEAR); + + /* poll for max. 1 sec to clear door bell register by h/w */ + reg = ufshcd_wait_for_register(hba, + REG_UTP_TASK_REQ_DOOR_BELL, + hba-outstanding_tasks, 0, 1000, 1000); + if (reg hba-outstanding_tasks) + err = -ETIMEDOUT; + + spin_lock_irqsave(hba-host-host_lock, flags); + /* complete commands that were cleared out */ + ufshcd_tmc_handler(hba); + spin_unlock_irqrestore(hba-host-host_lock, flags); +out:
RE: [PATCH V3 2/4] scsi: ufs: Fix hardware race conditions while aborting a command
On Tue, July 09, 2013, Sujit Reddy Thumma wrote: There is a possible race condition in the hardware when the abort command is issued to terminate the ongoing SCSI command as described below: - A bit in the door-bell register is set in the controller for a new SCSI command. - In some rare situations, before controller get a chance to issue the command to the device, the software issued an abort command. It's interesting. I wonder when we can meet this situation. Is it possible if SCSI mid layer should send abort command as soon as the transfer command is issued? AFAIK abort command is followed if one command has timed out. That means command have been already issued and no response? If you had some problem, could you share? - If the device recieves abort command first then it returns success receives because the command itself is not present. - Now if the controller commits the command to device it will be processed. - Software thinks that command is aborted and proceed while still the device is processing it. - The software, controller and device may go out of sync because of this race condition. To avoid this, query task presence in the device before sending abort task command so that after the abort operation, the command is guaranteed to be non-existent in both controller and the device. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 70 +++- 1 files changed, 55 insertions(+), 15 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index a176421..51ce096 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2550,6 +2550,12 @@ static int ufshcd_host_reset(struct scsi_cmnd *cmd) * ufshcd_abort - abort a specific command * @cmd: SCSI command pointer * + * Abort the pending command in device by sending UFS_ABORT_TASK task management + * command, and in host controller by clearing the door-bell register. There can + * be race between controller sending the command to the device while abort is + * issued. To avoid that, first issue UFS_QUERY_TASK to check if the command is + * really issued and then try to abort it. + * * Returns SUCCESS/FAILED */ static int ufshcd_abort(struct scsi_cmnd *cmd) @@ -2558,7 +2564,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) struct ufs_hba *hba; unsigned long flags; unsigned int tag; - int err; + int err = 0; + int poll_cnt; u8 resp; struct ufshcd_lrb *lrbp; @@ -2566,33 +2573,59 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) hba = shost_priv(host); tag = cmd-request-tag; - spin_lock_irqsave(host-host_lock, flags); + /* If command is already aborted/completed, return SUCCESS */ + if (!(test_bit(tag, hba-outstanding_reqs))) + goto out; - /* check if command is still pending */ - if (!(test_bit(tag, hba-outstanding_reqs))) { - err = FAILED; - spin_unlock_irqrestore(host-host_lock, flags); + lrbp = hba-lrb[tag]; + for (poll_cnt = 100; poll_cnt; poll_cnt--) { + err = ufshcd_issue_tm_cmd(hba, lrbp-lun, lrbp-task_tag, + UFS_QUERY_TASK, resp); + if (!err resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { + /* cmd pending in the device */ + break; + } else if (!err resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) { + u32 reg; + + /* + * cmd not pending in the device, check if it is + * in transition. + */ + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); + if (reg (1 tag)) { + /* sleep for max. 2ms to stabilize */ + usleep_range(1000, 2000); + continue; + } + /* command completed already */ + goto out; + } else { + if (!err) + err = resp; /* service response error */ + goto out; + } + } + + if (!poll_cnt) { + err = -EBUSY; goto out; } - spin_unlock_irqrestore(host-host_lock, flags); - lrbp = hba-lrb[tag]; err = ufshcd_issue_tm_cmd(hba, lrbp-lun, lrbp-task_tag, UFS_ABORT_TASK, resp); if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { - err = FAILED; + if (!err) + err = resp; /* service response error */ goto out; - } else { - err = SUCCESS; } + err = ufshcd_clear_cmd(hba, tag); + if (err) + goto out; +
RE: [PATCH V3 4/4] scsi: ufs: Improve UFS fatal error handling
On Tue, July 09, 2013, Sujit Reddy Thumma wrote: Error handling in UFS driver is broken and resets the host controller for fatal errors without re-initialization. Correct the fatal error handling sequence according to UFS Host Controller Interface (HCI) v1.1 specification. o Upon determining fatal error condition the host controller may hang forever until a reset is applied, so just retrying the command doesn't work without a reset. So, the reset is applied in the driver context in a separate work and SCSI mid-layer isn't informed until reset is applied. o Processed requests which are completed without error are reported to SCSI layer as successful and any pending commands that are not started yet or are not cause of the error are re-queued into scsi midlayer queue. For the command that caused error, host controller or device is reset and DID_ERROR is returned for command retry after applying reset. o SCSI is informed about the expected Unit-Attentioni exception from the Attention'i', typo. device for the immediate command after a reset so that the SCSI layer take necessary steps to establish communication with the device. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 349 +++- drivers/scsi/ufs/ufshcd.h |2 + drivers/scsi/ufs/ufshci.h | 19 ++- 3 files changed, 295 insertions(+), 75 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index b4c9910..2a3874f 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -80,6 +80,14 @@ enum { UFSHCD_EH_DEVICE_RESET_PENDING = (1 1), }; +/* UFSHCD UIC layer error flags */ +enum { + UFSHCD_UIC_DL_PA_INIT_ERROR = (1 0), /* Data link layer error */ + UFSHCD_UIC_NL_ERROR = (1 1), /* Network layer error */ + UFSHCD_UIC_TL_ERROR = (1 2), /* Transport Layer error */ + UFSHCD_UIC_DME_ERROR = (1 3), /* DME error */ +}; + /* Interrupt configuration options */ enum { UFSHCD_INT_DISABLE, @@ -108,6 +116,7 @@ enum { static void ufshcd_tmc_handler(struct ufs_hba *hba); static void ufshcd_async_scan(void *data, async_cookie_t cookie); +static int ufshcd_reset_and_restore(struct ufs_hba *hba); /* * ufshcd_wait_for_register - wait for register value to change @@ -1605,9 +1614,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba) goto out; } - if (hba-ufshcd_state == UFSHCD_STATE_RESET) - scsi_unblock_requests(hba-host); - out: return err; } @@ -1733,66 +1739,6 @@ static int ufshcd_validate_dev_connection(struct ufs_hba *hba) } /** - * ufshcd_do_reset - reset the host controller - * @hba: per adapter instance - * - * Returns SUCCESS/FAILED - */ -static int ufshcd_do_reset(struct ufs_hba *hba) -{ - struct ufshcd_lrb *lrbp; - unsigned long flags; - int tag; - - /* block commands from midlayer */ - scsi_block_requests(hba-host); - - spin_lock_irqsave(hba-host-host_lock, flags); - hba-ufshcd_state = UFSHCD_STATE_RESET; - - /* send controller to reset state */ - ufshcd_hba_stop(hba); - spin_unlock_irqrestore(hba-host-host_lock, flags); - - /* abort outstanding commands */ - for (tag = 0; tag hba-nutrs; tag++) { - if (test_bit(tag, hba-outstanding_reqs)) { - lrbp = hba-lrb[tag]; - if (lrbp-cmd) { - scsi_dma_unmap(lrbp-cmd); - lrbp-cmd-result = DID_RESET 16; - lrbp-cmd-scsi_done(lrbp-cmd); - lrbp-cmd = NULL; - clear_bit_unlock(tag, hba-lrb_in_use); - } - } - } - - /* complete device management command */ - if (hba-dev_cmd.complete) - complete(hba-dev_cmd.complete); - - /* clear outstanding request/task bit maps */ - hba-outstanding_reqs = 0; - hba-outstanding_tasks = 0; - - /* Host controller enable */ - if (ufshcd_hba_enable(hba)) { - dev_err(hba-dev, - Reset: Controller initialization failed\n); - return FAILED; - } - - if (ufshcd_link_startup(hba)) { - dev_err(hba-dev, - Reset: Link start-up failed\n); - return FAILED; - } - - return SUCCESS; -} - -/** * ufshcd_slave_alloc - handle initial SCSI device configurations * @sdev: pointer to SCSI device * @@ -1809,6 +1755,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev) sdev-use_10_for_ms = 1; scsi_set_tag_type(sdev, MSG_SIMPLE_TAG); + /* allow SCSI layer to restart the device in case of errors */ + sdev-allow_restart = 1; + /* * Inform SCSI Midlayer that the LUN queue depth
Re: [Ksummit-2013-discuss] [ATTEND] scsi-mq prototype discussion
On 07/17/2013 12:52 AM, James Bottomley wrote: On Tue, 2013-07-16 at 15:15 -0600, Jens Axboe wrote: On Tue, Jul 16 2013, Nicholas A. Bellinger wrote: On Sat, 2013-07-13 at 06:53 +, James Bottomley wrote: On Fri, 2013-07-12 at 12:52 +0200, Hannes Reinecke wrote: On 07/12/2013 03:33 AM, Nicholas A. Bellinger wrote: On Thu, 2013-07-11 at 18:02 -0700, Greg KH wrote: On Thu, Jul 11, 2013 at 05:23:32PM -0700, Nicholas A. Bellinger wrote: Drilling down the work items ahead of a real mainline push is high on priority list for discussion. The parties to be included in such a discussion are: - Jens Axboe (blk-mq author) - James Bottomley (scsi maintainer) - Christoph Hellwig (scsi) - Martin Petersen (scsi) - Tejun Heo (block + libata) - Hannes Reinecke (scsi error recovery) - Kent Overstreet (block, per-cpu ida) - Stephen Cameron (scsi-over-pcie driver) - Andrew Vasquez (qla2xxx LLD) - James Smart (lpfc LLD) Isn't this something that should have been discussed at the storage mini-summit a few months ago? The scsi-mq prototype, along with blk-mq (in it's current form) did not exist a few short months ago. ;) It seems very specific to one subsystem to be a kernel summit topic, don't you think? It's no more subsystem specific than half of the other proposals so far, and given it's reach across multiple subsystems (block, scsi, target), and the amount of off-list interest on the topic, I think it would make a good candidate for discussion. And it'll open up new approaches which previously were dismissed, like re-implementing multipathing on top of scsi-mq, giving us the single scsi device like other UNIX systems. Also I do think there's quite some synergy to be had, as with blk-mq we could nail each queue to a processor, which would eliminate the need for locking. Which could be useful for other subsystems, too. Lets start with discussing this on the list, please, and then see where we go from there ... Yes, the discussion is beginning to make it's way to the list. I've mostly been waiting for blk-mq to get a wider review before taking the early scsi-mq prototype driver to a larger public audience. Primarily, I'm now reaching out to the people most effected by existing scsi_request_fn() based performance limitations. Most of them have abandoned existing scsi_request_fn() based logic in favor of raw block make_request() based drivers, and are now estimating the amount of effort to move to an scsi-mq based approach. Regardless, as the prototype progresses over the next months, having a face-to-face discussion with the key parties in the room would be very helpful given the large amount of effort involved to actually make this type of generational shift in SCSI actually happen. There's a certain amount of overlap with the aio/O_DIRECT work as well. But if it's not a general session, could always be a BOF or something. I'll second the argument that most technical topics probably DO belong in a topic related workshop. But that leaves us with basically only process related topics at KS, I don't think it hurts to have a bit of tech meat on the bone too. At least I personally miss that part of KS from years gone by. Heh well, given that most of the block mq discussions at LSF have been you saying you really should get around to cleaning up and posting the code, you'll understand my wanting to see that happen first ... I suppose we could try to run a storage workshop within KS, but I think most of the mini summit slots have already gone. There's also plumbers if all slots are gone (I would say that, being biased and on the programme committee) Ric is running the storage and Filesystems MC http://www.linuxplumbersconf.org/2013/ocw/events/LPC2013/tracks/159 James And we still are looking for suggested topics - it would be great to have the multi-queue work at plumbers. You can post a proposal for it (or other topics) here: http://www.linuxplumbersconf.org/2013/ocw/events/LPC2013/proposals Ric -- 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 1/2] qla2xxx: Fix sparse warnings in qlafx00_fxdisc_iocb function.
On Fri, 2013-07-12 at 14:47 -0400, Saurav Kashyap wrote: Signed-off-by: Saurav Kashyap saurav.kash...@qlogic.com Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com This does not fix a bug ... therefore it's not really -rc fixes material. In early -rc this rule can be relaxed a bit ... but what's the reason to get this through the -rc tree? James -- 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 RESEND 0/1] AHCI: Optimize interrupt processing
On Thu, 2013-07-18 at 23:34 -0700, Nicholas A. Bellinger wrote: diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 0101af5..191bc15 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1144,7 +1144,11 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, sector_size=%u PAGE_SIZE, PIO may malfunction\n, sdev-sector_size); - blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + if (!q-mq_ops) { + blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + } else { + printk(Skipping dma_alignment for libata w/ scsi-mq\n); + } Amazingly enough there is a reason for the dma alignment, and it wasn't just to annoy you, so you can't blindly do this. The email thread is probably lost in the mists of time, but if I remember correctly the problem is that some ahci DMA controllers barf if the sector they're doing DMA on crosses a page boundary. Some are annoying enough to actually cause silent data corruption. You won't find every ahci DMA controller doing this, so the change will work for some, but it will be hard to identify those it won't work for until people start losing data. The correct fix, obviously, is to do the bio copy on the kernel path for unaligned data. It is OK to assume that REQ_TYPE_FS data is correctly aligned (because of the block to page alignment). James -- 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 0/7] qla4xxx: Updates for scsi misc branch
On 07/08/2013 06:33 AM, adheer.chandravan...@qlogic.com wrote: From: Adheer Chandravanshi adheer.chandravan...@qlogic.com James, Please apply the following patches to the scsi tree at your earliest convenience. Adheer Chandravanshi (7): qla4xxx: Allow removal of failed session using logout. qla4xxx: Use discovery_parent_idx instead of discovery_parent_type qla4xxx: Set IPv6 traffic class if device type is IPv6. qla4xxx: discovery_parent_idx can be shown without any check. qla4xxx: Only BIOS boot target entries should be at index 0 and 1. qla4xxx: Export more firmware info in sysfs qla4xxx: Update driver version to 5.03.00-k11 Ignore my comment on that one patch. Patches look ok. Reviewed-by: Mike Christie micha...@cs.wisc.edu -- 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/17/2013 11:44 AM, Mike Christie wrote: 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. Ignore this comment. -- 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 RESEND 0/1] AHCI: Optimize interrupt processing
On 07/18/2013 06:23 PM, Nicholas A. Bellinger wrote: Just saw this while trying out iscsi with the scsi-mq stuff :) Took at stab at this a while back, but ended getting distracted on other items. Do you have an initial conversion running yet..? Not running well :) Have a patch but I am debugging it now. I messed up something with the cmd_size stuff. -- 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 59601] commit 97dec564fd4948e0e560869c80b76e166ca2a83e breaks communication with XYRATEX disk shelves
https://bugzilla.kernel.org/show_bug.cgi?id=59601 Jack Hill jackh...@jackhill.us changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |CODE_FIX --- Comment #12 from Jack Hill jackh...@jackhill.us --- Closing this bug since Saurav has submitted the patch upstream. -- 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 2/2] qla2xxx: Properly set the tagging for commands.
On Fri, Jul 19, 2013 at 09:16:51AM +, Saurav Kashyap wrote: This should go to stable also, added in to list. This fixes a BZ https://bugzilla.kernel.org/show_bug.cgi?id=59601 Thanks, ~Saurav -Original Message- From: Saurav Kashyap [mailto:saurav.kash...@qlogic.com] Sent: Saturday, July 13, 2013 12:18 AM To: jbottom...@parallels.com Cc: Giridhar Malavali; Saurav Kashyap; Andrew Vasquez; linux-scsi Subject: [PATCH 2/2] qla2xxx: Properly set the tagging for commands. Reported-by: Jack Hill jackh...@jackhill.us Signed-off-by: Saurav Kashyap saurav.kash...@qlogic.com Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com --- drivers/scsi/qla2xxx/qla_iocb.c | 11 +-- 1 files changed, 9 insertions(+), 2 deletions(-) formletter This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly. /formletter -- 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/) It looks like different. eMMC cannot execute BKOPS itself unlike UFS. That's the way eMMC's host should trigger the BKOPS manually. I guess it is not much of a difference for UFS as far as we concern only about idle time bkops. In UFS one can still disallow the device to not carry out BKOPS and hence the case where UFS device also cannot execute BKOPS itself and a idle timer is needed to allow BKOPS sporadically so that device doesn't go into URGENT_BKOPS state. 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? Yes, it's difficult to find proper time. Maybe, BKOPS can be disabled when request comes up. In cases where there are back-to-back heavy data write requests then it is not proper to enable/disable BKOPS as soon as the new request comes up. It may happen that the card will then move from PERFORMANCE_IMPACT state to CRITICAL and ultimately start failing the requests. The point is that we should never get into state where we need URGENT_BKOPS. -- 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 2/4] scsi: ufs: Fix hardware race conditions while aborting a command
On 7/19/2013 7:26 PM, Seungwon Jeon wrote: On Tue, July 09, 2013, Sujit Reddy Thumma wrote: There is a possible race condition in the hardware when the abort command is issued to terminate the ongoing SCSI command as described below: - A bit in the door-bell register is set in the controller for a new SCSI command. - In some rare situations, before controller get a chance to issue the command to the device, the software issued an abort command. It's interesting. I wonder when we can meet this situation. Is it possible if SCSI mid layer should send abort command as soon as the transfer command is issued? AFAIK abort command is followed if one command has timed out. That means command have been already issued and no response? If you had some problem, could you share? You are right. This is a very rare case and probably don't happen at all until and unless the SCSI error handling is changed. We found it just by static analysis. Probably, at some point I may push a patch that tries to reduce the read latencies while aborting a large write transfer when I come across a UFS device that has command per LU as 1 :-) I guess this is good to have change. The path chosen is according to SCSI SAM-5 architecture specification, so I don't expect any issues here. - If the device recieves abort command first then it returns success receives because the command itself is not present. - Now if the controller commits the command to device it will be processed. - Software thinks that command is aborted and proceed while still the device is processing it. - The software, controller and device may go out of sync because of this race condition. To avoid this, query task presence in the device before sending abort task command so that after the abort operation, the command is guaranteed to be non-existent in both controller and the device. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 70 +++- 1 files changed, 55 insertions(+), 15 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index a176421..51ce096 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2550,6 +2550,12 @@ static int ufshcd_host_reset(struct scsi_cmnd *cmd) * ufshcd_abort - abort a specific command * @cmd: SCSI command pointer * + * Abort the pending command in device by sending UFS_ABORT_TASK task management + * command, and in host controller by clearing the door-bell register. There can + * be race between controller sending the command to the device while abort is + * issued. To avoid that, first issue UFS_QUERY_TASK to check if the command is + * really issued and then try to abort it. + * * Returns SUCCESS/FAILED */ static int ufshcd_abort(struct scsi_cmnd *cmd) @@ -2558,7 +2564,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) struct ufs_hba *hba; unsigned long flags; unsigned int tag; -int err; +int err = 0; +int poll_cnt; u8 resp; struct ufshcd_lrb *lrbp; @@ -2566,33 +2573,59 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) hba = shost_priv(host); tag = cmd-request-tag; -spin_lock_irqsave(host-host_lock, flags); +/* If command is already aborted/completed, return SUCCESS */ +if (!(test_bit(tag, hba-outstanding_reqs))) +goto out; -/* check if command is still pending */ -if (!(test_bit(tag, hba-outstanding_reqs))) { -err = FAILED; -spin_unlock_irqrestore(host-host_lock, flags); +lrbp = hba-lrb[tag]; +for (poll_cnt = 100; poll_cnt; poll_cnt--) { +err = ufshcd_issue_tm_cmd(hba, lrbp-lun, lrbp-task_tag, +UFS_QUERY_TASK, resp); +if (!err resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { +/* cmd pending in the device */ +break; +} else if (!err resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) { +u32 reg; + +/* + * cmd not pending in the device, check if it is + * in transition. + */ +reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); +if (reg (1 tag)) { +/* sleep for max. 2ms to stabilize */ +usleep_range(1000, 2000); +continue; +} +/* command completed already */ +goto out; +} else { +if (!err) +err = resp; /* service response error */ +goto out; +} +} + +if (!poll_cnt) { +err = -EBUSY; goto out; } -spin_unlock_irqrestore(host-host_lock,
Re: [PATCH V3 1/4] scsi: ufs: Fix broken task management command implementation
On 7/19/2013 7:26 PM, Seungwon Jeon wrote: On Tue, July 09, 2013 Sujit Reddy Thumma wrote: Currently, sending Task Management (TM) command to the card might be broken in some scenarios as listed below: Problem: If there are more than 8 TM commands the implementation returns error to the caller. Fix: Wait for one of the slots to be emptied and send the command. Problem: Sometimes it is necessary for the caller to know the TM service response code to determine the task status. Fix: Propogate the service response to the caller. Problem: If the TM command times out no proper error recovery is implemented. Fix: Clear the command in the controller door-bell register, so that further commands for the same slot don't fail. Problem: While preparing the TM command descriptor, the task tag used should be unique across SCSI/NOP/QUERY/TM commands and not the task tag of the command which the TM command is trying to manage. Fix: Use a unique task tag instead of task tag of SCSI command. Problem: Since the TM command involves H/W communication, abruptly ending the request on kill interrupt signal might cause h/w malfunction. Fix: Wait for hardware completion interrupt with TASK_UNINTERRUPTIBLE set. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 177 ++--- drivers/scsi/ufs/ufshcd.h |8 ++- 2 files changed, 126 insertions(+), 59 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index af7d01d..a176421 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -53,6 +53,9 @@ /* Query request timeout */ #define QUERY_REQ_TIMEOUT 30 /* msec */ +/* Task management command timeout */ +#define TM_CMD_TIMEOUT 100 /* msecs */ + /* Expose the flag value from utp_upiu_query.value */ #define MASK_QUERY_UPIU_FLAG_LOC 0xFF @@ -190,13 +193,35 @@ ufshcd_get_tmr_ocs(struct utp_task_req_desc *task_req_descp) /** * ufshcd_get_tm_free_slot - get a free slot for task management request * @hba: per adapter instance + * @free_slot: pointer to variable with available slot value * - * Returns maximum number of task management request slots in case of - * task management queue full or returns the free slot number + * Get a free tag and lock it until ufshcd_put_tm_slot() is called. + * Returns 0 if free slot is not available, else return 1 with tag value + * in @free_slot. */ -static inline int ufshcd_get_tm_free_slot(struct ufs_hba *hba) +static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot) +{ +int tag; +bool ret = false; + +if (!free_slot) +goto out; + +do { +tag = find_first_zero_bit(hba-tm_slots_in_use, hba-nutmrs); +if (tag = hba-nutmrs) +goto out; +} while (test_and_set_bit_lock(tag, hba-tm_slots_in_use)); + +*free_slot = tag; +ret = true; +out: +return ret; +} + +static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot) { -return find_first_zero_bit(hba-outstanding_tasks, hba-nutmrs); +clear_bit_unlock(slot, hba-tm_slots_in_use); } /** @@ -1778,10 +1803,11 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev) * ufshcd_task_req_compl - handle task management request completion * @hba: per adapter instance * @index: index of the completed request + * @resp: task management service response * - * Returns SUCCESS/FAILED + * Returns non-zero value on error, zero on success */ -static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index) +static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp) { struct utp_task_req_desc *task_req_descp; struct utp_upiu_task_rsp *task_rsp_upiup; @@ -1802,19 +1828,15 @@ static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index) task_req_descp[index].task_rsp_upiu; task_result = be32_to_cpu(task_rsp_upiup-header.dword_1); task_result = ((task_result MASK_TASK_RESPONSE) 8); - -if (task_result != UPIU_TASK_MANAGEMENT_FUNC_COMPL -task_result != UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) -task_result = FAILED; -else -task_result = SUCCESS; +if (resp) +*resp = (u8)task_result; } else { -task_result = FAILED; -dev_err(hba-dev, -trc: Invalid ocs = %x\n, ocs_value); +dev_err(hba-dev, %s: failed, ocs = 0x%x\n, +__func__, ocs_value); } spin_unlock_irqrestore(hba-host-host_lock, flags); -return task_result; + +return ocs_value; } /** @@ -2298,7 +2320,7 @@ static void
Re: [PATCH V3 3/4] scsi: ufs: Fix device and host reset methods
On 7/19/2013 7:27 PM, Seungwon Jeon wrote: On Tue, July 09, 2013, Sujit Reddy Thumma wrote: As of now SCSI initiated error handling is broken because, the reset APIs don't try to bring back the device initialized and ready for further transfers. In case of timeouts, the scsi error handler takes care of handling aborts and resets. Improve the error handling in such scenario by resetting the device and host and re-initializing them in proper manner. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 467 +++-- drivers/scsi/ufs/ufshcd.h |2 + 2 files changed, 411 insertions(+), 58 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 51ce096..b4c9910 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -69,9 +69,15 @@ enum { /* UFSHCD states */ enum { -UFSHCD_STATE_OPERATIONAL, UFSHCD_STATE_RESET, UFSHCD_STATE_ERROR, +UFSHCD_STATE_OPERATIONAL, +}; + +/* UFSHCD error handling flags */ +enum { +UFSHCD_EH_HOST_RESET_PENDING = (1 0), +UFSHCD_EH_DEVICE_RESET_PENDING = (1 1), }; /* Interrupt configuration options */ @@ -87,6 +93,22 @@ enum { INT_AGGR_CONFIG, }; +#define ufshcd_set_device_reset_pending(h) \ +(h-eh_flags |= UFSHCD_EH_DEVICE_RESET_PENDING) +#define ufshcd_set_host_reset_pending(h) \ +(h-eh_flags |= UFSHCD_EH_HOST_RESET_PENDING) +#define ufshcd_device_reset_pending(h) \ +(h-eh_flags UFSHCD_EH_DEVICE_RESET_PENDING) +#define ufshcd_host_reset_pending(h) \ +(h-eh_flags UFSHCD_EH_HOST_RESET_PENDING) +#define ufshcd_clear_device_reset_pending(h) \ +(h-eh_flags = ~UFSHCD_EH_DEVICE_RESET_PENDING) +#define ufshcd_clear_host_reset_pending(h) \ +(h-eh_flags = ~UFSHCD_EH_HOST_RESET_PENDING) + +static void ufshcd_tmc_handler(struct ufs_hba *hba); +static void ufshcd_async_scan(void *data, async_cookie_t cookie); + /* * ufshcd_wait_for_register - wait for register value to change * @hba - per-adapter interface @@ -851,9 +873,22 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) tag = cmd-request-tag; -if (hba-ufshcd_state != UFSHCD_STATE_OPERATIONAL) { +switch (hba-ufshcd_state) { Lock is no needed for ufshcd_state? +case UFSHCD_STATE_OPERATIONAL: +break; +case UFSHCD_STATE_RESET: err = SCSI_MLQUEUE_HOST_BUSY; goto out; +case UFSHCD_STATE_ERROR: +set_host_byte(cmd, DID_ERROR); +cmd-scsi_done(cmd); +goto out; +default: +dev_WARN_ONCE(hba-dev, 1, %s: invalid state %d\n, +__func__, hba-ufshcd_state); +set_host_byte(cmd, DID_BAD_TARGET); +cmd-scsi_done(cmd); +goto out; } /* acquire the tag to make sure device cmds don't use it */ @@ -1573,8 +1608,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba) if (hba-ufshcd_state == UFSHCD_STATE_RESET) scsi_unblock_requests(hba-host); -hba-ufshcd_state = UFSHCD_STATE_OPERATIONAL; - out: return err; } @@ -2273,6 +2306,106 @@ out: } /** + * ufshcd_utrl_is_rsr_enabled - check if run-stop register is enabled + * @hba: per-adapter instance + */ +static bool ufshcd_utrl_is_rsr_enabled(struct ufs_hba *hba) +{ +return ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_LIST_RUN_STOP) 0x1; +} + +/** + * ufshcd_utmrl_is_rsr_enabled - check if run-stop register is enabled + * @hba: per-adapter instance + */ +static bool ufshcd_utmrl_is_rsr_enabled(struct ufs_hba *hba) +{ +return ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_RUN_STOP) 0x1; +} + +/** + * ufshcd_complete_pending_tasks - complete outstanding tasks + * @hba: per adapter instance + * + * Abort in-progress task management commands and wakeup + * waiting threads. + * + * Returns non-zero error value when failed to clear all the commands. + */ +static int ufshcd_complete_pending_tasks(struct ufs_hba *hba) +{ +u32 reg; +int err = 0; +unsigned long flags; + +if (!hba-outstanding_tasks) +goto out; + +/* Clear UTMRL only when run-stop is enabled */ +if (ufshcd_utmrl_is_rsr_enabled(hba)) +ufshcd_writel(hba, ~hba-outstanding_tasks, +REG_UTP_TASK_REQ_LIST_CLEAR); + +/* poll for max. 1 sec to clear door bell register by h/w */ +reg = ufshcd_wait_for_register(hba, +REG_UTP_TASK_REQ_DOOR_BELL, +hba-outstanding_tasks, 0, 1000, 1000); +if (reg hba-outstanding_tasks) +err = -ETIMEDOUT; + +spin_lock_irqsave(hba-host-host_lock, flags); +/* complete commands that were cleared out */ +ufshcd_tmc_handler(hba); +spin_unlock_irqrestore(hba-host-host_lock, flags); +out: +if
Re: [PATCH V3 4/4] scsi: ufs: Improve UFS fatal error handling
On 7/19/2013 7:28 PM, Seungwon Jeon wrote: On Tue, July 09, 2013, Sujit Reddy Thumma wrote: Error handling in UFS driver is broken and resets the host controller for fatal errors without re-initialization. Correct the fatal error handling sequence according to UFS Host Controller Interface (HCI) v1.1 specification. o Upon determining fatal error condition the host controller may hang forever until a reset is applied, so just retrying the command doesn't work without a reset. So, the reset is applied in the driver context in a separate work and SCSI mid-layer isn't informed until reset is applied. o Processed requests which are completed without error are reported to SCSI layer as successful and any pending commands that are not started yet or are not cause of the error are re-queued into scsi midlayer queue. For the command that caused error, host controller or device is reset and DID_ERROR is returned for command retry after applying reset. o SCSI is informed about the expected Unit-Attentioni exception from the Attention'i', typo. Okay. device for the immediate command after a reset so that the SCSI layer take necessary steps to establish communication with the device. Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org --- drivers/scsi/ufs/ufshcd.c | 349 +++- drivers/scsi/ufs/ufshcd.h |2 + drivers/scsi/ufs/ufshci.h | 19 ++- 3 files changed, 295 insertions(+), 75 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index b4c9910..2a3874f 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -80,6 +80,14 @@ enum { UFSHCD_EH_DEVICE_RESET_PENDING = (1 1), }; +/* UFSHCD UIC layer error flags */ +enum { +UFSHCD_UIC_DL_PA_INIT_ERROR = (1 0), /* Data link layer error */ +UFSHCD_UIC_NL_ERROR = (1 1), /* Network layer error */ +UFSHCD_UIC_TL_ERROR = (1 2), /* Transport Layer error */ +UFSHCD_UIC_DME_ERROR = (1 3), /* DME error */ +}; + /* Interrupt configuration options */ enum { UFSHCD_INT_DISABLE, @@ -108,6 +116,7 @@ enum { static void ufshcd_tmc_handler(struct ufs_hba *hba); static void ufshcd_async_scan(void *data, async_cookie_t cookie); +static int ufshcd_reset_and_restore(struct ufs_hba *hba); /* * ufshcd_wait_for_register - wait for register value to change @@ -1605,9 +1614,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba) goto out; } -if (hba-ufshcd_state == UFSHCD_STATE_RESET) -scsi_unblock_requests(hba-host); - out: return err; } @@ -1733,66 +1739,6 @@ static int ufshcd_validate_dev_connection(struct ufs_hba *hba) } /** - * ufshcd_do_reset - reset the host controller - * @hba: per adapter instance - * - * Returns SUCCESS/FAILED - */ -static int ufshcd_do_reset(struct ufs_hba *hba) -{ -struct ufshcd_lrb *lrbp; -unsigned long flags; -int tag; - -/* block commands from midlayer */ -scsi_block_requests(hba-host); - -spin_lock_irqsave(hba-host-host_lock, flags); -hba-ufshcd_state = UFSHCD_STATE_RESET; - -/* send controller to reset state */ -ufshcd_hba_stop(hba); -spin_unlock_irqrestore(hba-host-host_lock, flags); - -/* abort outstanding commands */ -for (tag = 0; tag hba-nutrs; tag++) { -if (test_bit(tag, hba-outstanding_reqs)) { -lrbp = hba-lrb[tag]; -if (lrbp-cmd) { -scsi_dma_unmap(lrbp-cmd); -lrbp-cmd-result = DID_RESET 16; -lrbp-cmd-scsi_done(lrbp-cmd); -lrbp-cmd = NULL; -clear_bit_unlock(tag, hba-lrb_in_use); -} -} -} - -/* complete device management command */ -if (hba-dev_cmd.complete) -complete(hba-dev_cmd.complete); - -/* clear outstanding request/task bit maps */ -hba-outstanding_reqs = 0; -hba-outstanding_tasks = 0; - -/* Host controller enable */ -if (ufshcd_hba_enable(hba)) { -dev_err(hba-dev, -Reset: Controller initialization failed\n); -return FAILED; -} - -if (ufshcd_link_startup(hba)) { -dev_err(hba-dev, -Reset: Link start-up failed\n); -return FAILED; -} - -return SUCCESS; -} - -/** * ufshcd_slave_alloc - handle initial SCSI device configurations * @sdev: pointer to SCSI device * @@ -1809,6 +1755,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev) sdev-use_10_for_ms = 1; scsi_set_tag_type(sdev, MSG_SIMPLE_TAG); +/* allow SCSI layer to restart the device in case of errors */ +sdev-allow_restart = 1; + /* * Inform SCSI Midlayer that the
Re: [PATCH V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU
On 7/19/2013 7:17 PM, Seungwon Jeon wrote: On Thu, July 18, 2013, Sujit Reddy Thumma wrote: + * 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 !=
Re: [PATCH RESEND 0/1] AHCI: Optimize interrupt processing
On Fri, 2013-07-19 at 08:33 -0700, James Bottomley wrote: On Thu, 2013-07-18 at 23:34 -0700, Nicholas A. Bellinger wrote: diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 0101af5..191bc15 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1144,7 +1144,11 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, sector_size=%u PAGE_SIZE, PIO may malfunction\n, sdev-sector_size); - blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + if (!q-mq_ops) { + blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + } else { + printk(Skipping dma_alignment for libata w/ scsi-mq\n); + } Amazingly enough there is a reason for the dma alignment, and it wasn't just to annoy you, so you can't blindly do this. The email thread is probably lost in the mists of time, but if I remember correctly the problem is that some ahci DMA controllers barf if the sector they're doing DMA on crosses a page boundary. Some are annoying enough to actually cause silent data corruption. You won't find every ahci DMA controller doing this, so the change will work for some, but it will be hard to identify those it won't work for until people start losing data. Thanks for the extra background. So at least from what I gather thus far this shouldn't be an issue for initial testing with scsi-mq - libata w/ ata_piix. The correct fix, obviously, is to do the bio copy on the kernel path for unaligned data. It is OK to assume that REQ_TYPE_FS data is correctly aligned (because of the block to page alignment). Indeed. Looking into the bio_copy_kern() breakage next.. --nab -- 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 RESEND 0/1] AHCI: Optimize interrupt processing
On Fri, 2013-07-19 at 09:58 -0600, Mike Christie wrote: On 07/18/2013 06:23 PM, Nicholas A. Bellinger wrote: Just saw this while trying out iscsi with the scsi-mq stuff :) Took at stab at this a while back, but ended getting distracted on other items. Do you have an initial conversion running yet..? Not running well :) Have a patch but I am debugging it now. I messed up something with the cmd_size stuff. nod, looking forward to see ib_iser move beyond the existing scsi_request_fn() bottleneck. ;) Feel free to send me the WIP off-list if you'd like another pair of eyes. --nab -- 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: [Ksummit-2013-discuss] [ATTEND] scsi-mq prototype discussion
On Wed, 2013-07-17 at 04:52 +, James Bottomley wrote: On Tue, 2013-07-16 at 15:15 -0600, Jens Axboe wrote: On Tue, Jul 16 2013, Nicholas A. Bellinger wrote: On Sat, 2013-07-13 at 06:53 +, James Bottomley wrote: SNIP Lets start with discussing this on the list, please, and then see where we go from there ... Yes, the discussion is beginning to make it's way to the list. I've mostly been waiting for blk-mq to get a wider review before taking the early scsi-mq prototype driver to a larger public audience. Primarily, I'm now reaching out to the people most effected by existing scsi_request_fn() based performance limitations. Most of them have abandoned existing scsi_request_fn() based logic in favor of raw block make_request() based drivers, and are now estimating the amount of effort to move to an scsi-mq based approach. Regardless, as the prototype progresses over the next months, having a face-to-face discussion with the key parties in the room would be very helpful given the large amount of effort involved to actually make this type of generational shift in SCSI actually happen. There's a certain amount of overlap with the aio/O_DIRECT work as well. But if it's not a general session, could always be a BOF or something. I'll second the argument that most technical topics probably DO belong in a topic related workshop. But that leaves us with basically only process related topics at KS, I don't think it hurts to have a bit of tech meat on the bone too. At least I personally miss that part of KS from years gone by. Heh well, given that most of the block mq discussions at LSF have been you saying you really should get around to cleaning up and posting the code, you'll understand my wanting to see that happen first ... I suppose we could try to run a storage workshop within KS, but I think most of the mini summit slots have already gone. That would be great, given there is a reasonable level of interest from various parities, and the pain threshold for existing scsi small block random I/O performance is high.. When will we know if there is a workshop / mini summit slot available..? (CC'ing Mike Christie as well for open-iscsi/scsi-mq bits) There's also plumbers if all slots are gone (I would say that, being biased and on the programme committee) Ric is running the storage and Filesystems MC http://www.linuxplumbersconf.org/2013/ocw/events/LPC2013/tracks/159 FYI, I'm not trying to 'sell' scsi-mq to the larger community yet, but rather interested in getting the right scsi/block/LLD people in the same room at KS for an hour or two to discuss implementation details, given the scope of the effort involved. --nab -- 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: [Ksummit-2013-discuss] [ATTEND] scsi-mq prototype discussion
On Fri, 2013-07-19 at 14:22 -0700, Nicholas A. Bellinger wrote: On Wed, 2013-07-17 at 04:52 +, James Bottomley wrote: On Tue, 2013-07-16 at 15:15 -0600, Jens Axboe wrote: On Tue, Jul 16 2013, Nicholas A. Bellinger wrote: On Sat, 2013-07-13 at 06:53 +, James Bottomley wrote: SNIP Lets start with discussing this on the list, please, and then see where we go from there ... Yes, the discussion is beginning to make it's way to the list. I've mostly been waiting for blk-mq to get a wider review before taking the early scsi-mq prototype driver to a larger public audience. Primarily, I'm now reaching out to the people most effected by existing scsi_request_fn() based performance limitations. Most of them have abandoned existing scsi_request_fn() based logic in favor of raw block make_request() based drivers, and are now estimating the amount of effort to move to an scsi-mq based approach. Regardless, as the prototype progresses over the next months, having a face-to-face discussion with the key parties in the room would be very helpful given the large amount of effort involved to actually make this type of generational shift in SCSI actually happen. There's a certain amount of overlap with the aio/O_DIRECT work as well. But if it's not a general session, could always be a BOF or something. I'll second the argument that most technical topics probably DO belong in a topic related workshop. But that leaves us with basically only process related topics at KS, I don't think it hurts to have a bit of tech meat on the bone too. At least I personally miss that part of KS from years gone by. Heh well, given that most of the block mq discussions at LSF have been you saying you really should get around to cleaning up and posting the code, you'll understand my wanting to see that happen first ... I suppose we could try to run a storage workshop within KS, but I think most of the mini summit slots have already gone. That would be great, given there is a reasonable level of interest from various parities, and the pain threshold for existing scsi small block random I/O performance is high.. When will we know if there is a workshop / mini summit slot available..? (CC'ing Mike Christie as well for open-iscsi/scsi-mq bits) There's also plumbers if all slots are gone (I would say that, being biased and on the programme committee) Ric is running the storage and Filesystems MC http://www.linuxplumbersconf.org/2013/ocw/events/LPC2013/tracks/159 FYI, I'm not trying to 'sell' scsi-mq to the larger community yet, but rather interested in getting the right scsi/block/LLD people in the same room at KS for an hour or two to discuss implementation details, given the scope of the effort involved. Well, so that's why I think plumbers might be a better venue: we'll have more of the actual storage people there. Usually we get at most 2-3 storage people to KS compared to the 25 or so we usually have at LSF ... that makes KS not a very good venue for storage centric discussions. James -- 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: [Ksummit-2013-discuss] [ATTEND] scsi-mq prototype discussion
On Fri, 2013-07-19 at 21:46 +, James Bottomley wrote: On Fri, 2013-07-19 at 14:22 -0700, Nicholas A. Bellinger wrote: On Wed, 2013-07-17 at 04:52 +, James Bottomley wrote: On Tue, 2013-07-16 at 15:15 -0600, Jens Axboe wrote: On Tue, Jul 16 2013, Nicholas A. Bellinger wrote: On Sat, 2013-07-13 at 06:53 +, James Bottomley wrote: SNIP Lets start with discussing this on the list, please, and then see where we go from there ... Yes, the discussion is beginning to make it's way to the list. I've mostly been waiting for blk-mq to get a wider review before taking the early scsi-mq prototype driver to a larger public audience. Primarily, I'm now reaching out to the people most effected by existing scsi_request_fn() based performance limitations. Most of them have abandoned existing scsi_request_fn() based logic in favor of raw block make_request() based drivers, and are now estimating the amount of effort to move to an scsi-mq based approach. Regardless, as the prototype progresses over the next months, having a face-to-face discussion with the key parties in the room would be very helpful given the large amount of effort involved to actually make this type of generational shift in SCSI actually happen. There's a certain amount of overlap with the aio/O_DIRECT work as well. But if it's not a general session, could always be a BOF or something. I'll second the argument that most technical topics probably DO belong in a topic related workshop. But that leaves us with basically only process related topics at KS, I don't think it hurts to have a bit of tech meat on the bone too. At least I personally miss that part of KS from years gone by. Heh well, given that most of the block mq discussions at LSF have been you saying you really should get around to cleaning up and posting the code, you'll understand my wanting to see that happen first ... I suppose we could try to run a storage workshop within KS, but I think most of the mini summit slots have already gone. That would be great, given there is a reasonable level of interest from various parities, and the pain threshold for existing scsi small block random I/O performance is high.. When will we know if there is a workshop / mini summit slot available..? (CC'ing Mike Christie as well for open-iscsi/scsi-mq bits) There's also plumbers if all slots are gone (I would say that, being biased and on the programme committee) Ric is running the storage and Filesystems MC http://www.linuxplumbersconf.org/2013/ocw/events/LPC2013/tracks/159 FYI, I'm not trying to 'sell' scsi-mq to the larger community yet, but rather interested in getting the right scsi/block/LLD people in the same room at KS for an hour or two to discuss implementation details, given the scope of the effort involved. Well, so that's why I think plumbers might be a better venue: we'll have more of the actual storage people there. Usually we get at most 2-3 storage people to KS compared to the 25 or so we usually have at LSF ... that makes KS not a very good venue for storage centric discussions. The most relevant people for the discussion are Jens, Hannes, Christoph, Tejun, Martin, Mike, and you. I know these folks are regular attendees for KS, but typically not for plumbers, which is why I made this KS topic proposal in the first place. --nab -- 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 v4 1/4] [SCSI] sg: use rwsem to solve race during exclusive open
On Wed, 17 July 2013 23:34:03 +0800, Vaughan Cao wrote: Date: Wed, 17 Jul 2013 23:34:03 +0800 From: Vaughan Cao vaughan@oracle.com To: jo...@logfs.org Cc: dgilb...@interlog.com, jbottom...@parallels.com, linux-scsi@vger.kernel.org, linux-ker...@vger.kernel.org, vaughan@oracle.com Subject: [PATCH v4 1/4] [SCSI] sg: use rwsem to solve race during exclusive open X-Mailer: git-send-email 1.7.11.7 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 vaughan@oracle.com Reviewed-by: Joern Engel jo...@logfs.org Jörn -- When you close your hand, you own nothing. When you open it up, you own the whole world. -- Li Mu Bai in Tiger Dragon -- 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 v4 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking
On Wed, 17 July 2013 23:34:06 +0800, Vaughan Cao wrote: Date: Wed, 17 Jul 2013 23:34:06 +0800 From: Vaughan Cao vaughan@oracle.com To: jo...@logfs.org Cc: dgilb...@interlog.com, jbottom...@parallels.com, linux-scsi@vger.kernel.org, linux-ker...@vger.kernel.org, vaughan@oracle.com Subject: [PATCH v4 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking X-Mailer: git-send-email 1.7.11.7 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 vaughan@oracle.com Reviewed-by: Joern Engel jo...@logfs.org Jörn -- What one programmer can do in one month, two programmers can do in two months. -- Fred Brooks -- 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 v4 2/4] [SCSI] sg: no need sg_open_exclusive_lock
On Wed, 17 July 2013 23:34:04 +0800, Vaughan Cao wrote: Date: Wed, 17 Jul 2013 23:34:04 +0800 From: Vaughan Cao vaughan@oracle.com To: jo...@logfs.org Cc: dgilb...@interlog.com, jbottom...@parallels.com, linux-scsi@vger.kernel.org, linux-ker...@vger.kernel.org, vaughan@oracle.com Subject: [PATCH v4 2/4] [SCSI] sg: no need sg_open_exclusive_lock X-Mailer: git-send-email 1.7.11.7 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 vaughan@oracle.com Reviewed-by: Joern Engel jo...@logfs.org Jörn -- Error protection by error detection and correction. -- from a university class -- 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 v4 3/4] [SCSI] sg: checking sdp-detached isn't protected when open
On Wed, 17 July 2013 23:34:05 +0800, Vaughan Cao wrote: -static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev); +static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev, int * reason); You can use ERR_PTR and friends instead of adding another parameter. 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; - } sfp = sg_add_sfp(sdp, dev); if (IS_ERR(sfp)) { retval = PTR_ERR(sfp); 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; + } if (!sfp) return ERR_PTR(-ENOMEM); Jörn -- Luck is when opportunity meets good preparation. -- Chinese proverb -- 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] mpt2sas: don't handle broadcast primitives
The handling of broadcast primitives involves _scsih_block_io_all_device(), which does what the name implies. I have observed cases with 60s of blocking io on all devices, caused by a single bad device. The downsides of this code are obvious, while the upsides are more elusive. Signed-off-by: Joern Engel jo...@logfs.org --- drivers/scsi/mpt2sas/mpt2sas_base.c |4 - drivers/scsi/mpt2sas/mpt2sas_scsih.c | 238 -- 2 files changed, 242 deletions(-) diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c index 21c0a27..bba2209 100644 --- a/drivers/scsi/mpt2sas/mpt2sas_base.c +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c @@ -586,9 +586,6 @@ _base_display_event_data(struct MPT2SAS_ADAPTER *ioc, printk(\n); return; } - case MPI2_EVENT_SAS_BROADCAST_PRIMITIVE: - desc = SAS Broadcast Primitive; - break; case MPI2_EVENT_SAS_INIT_DEVICE_STATUS_CHANGE: desc = SAS Init Device Status Change; break; @@ -4370,7 +4367,6 @@ mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc) /* here we enable the events we care about */ _base_unmask_events(ioc, MPI2_EVENT_SAS_DISCOVERY); - _base_unmask_events(ioc, MPI2_EVENT_SAS_BROADCAST_PRIMITIVE); _base_unmask_events(ioc, MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST); _base_unmask_events(ioc, MPI2_EVENT_SAS_DEVICE_STATUS_CHANGE); _base_unmask_events(ioc, MPI2_EVENT_SAS_ENCL_DEVICE_STATUS_CHANGE); diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c index 3fffb95..856a502 100644 --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c @@ -2915,31 +2915,6 @@ _scsih_fw_event_cleanup_queue(struct MPT2SAS_ADAPTER *ioc) } /** - * _scsih_ublock_io_all_device - unblock every device - * @ioc: per adapter object - * - * change the device state from block to running - */ -static void -_scsih_ublock_io_all_device(struct MPT2SAS_ADAPTER *ioc) -{ - struct MPT2SAS_DEVICE *sas_device_priv_data; - struct scsi_device *sdev; - - shost_for_each_device(sdev, ioc-shost) { - sas_device_priv_data = sdev-hostdata; - if (!sas_device_priv_data) - continue; - if (!sas_device_priv_data-block) - continue; - sas_device_priv_data-block = 0; - dewtprintk(ioc, sdev_printk(KERN_INFO, sdev, device_running, - handle(0x%04x)\n, - sas_device_priv_data-sas_target-handle)); - scsi_internal_device_unblock(sdev, SDEV_RUNNING); - } -} -/** * _scsih_ublock_io_device - set the device state to SDEV_RUNNING * @ioc: per adapter object * @handle: device handle @@ -2971,34 +2946,6 @@ _scsih_ublock_io_device(struct MPT2SAS_ADAPTER *ioc, u64 sas_address) } /** - * _scsih_block_io_all_device - set the device state to SDEV_BLOCK - * @ioc: per adapter object - * @handle: device handle - * - * During device pull we need to appropiately set the sdev state. - */ -static void -_scsih_block_io_all_device(struct MPT2SAS_ADAPTER *ioc) -{ - struct MPT2SAS_DEVICE *sas_device_priv_data; - struct scsi_device *sdev; - - shost_for_each_device(sdev, ioc-shost) { - sas_device_priv_data = sdev-hostdata; - if (!sas_device_priv_data) - continue; - if (sas_device_priv_data-block) - continue; - sas_device_priv_data-block = 1; - dewtprintk(ioc, sdev_printk(KERN_INFO, sdev, device_blocked, - handle(0x%04x)\n, - sas_device_priv_data-sas_target-handle)); - scsi_internal_device_block(sdev); - } -} - - -/** * _scsih_block_io_device - set the device state to SDEV_BLOCK * @ioc: per adapter object * @handle: device handle @@ -5829,166 +5776,6 @@ _scsih_sas_enclosure_dev_status_change_event(struct MPT2SAS_ADAPTER *ioc, } /** - * _scsih_sas_broadcast_primitive_event - handle broadcast events - * @ioc: per adapter object - * @fw_event: The fw_event_work object - * Context: user. - * - * Return nothing. - */ -static void -_scsih_sas_broadcast_primitive_event(struct MPT2SAS_ADAPTER *ioc, -struct fw_event_work *fw_event) -{ - struct scsi_cmnd *scmd; - unsigned long ser_no; - struct scsi_device *sdev; - u16 smid, handle; - u32 lun; - struct MPT2SAS_DEVICE *sas_device_priv_data; - u32 termination_count; - u32 query_count; - Mpi2SCSITaskManagementReply_t *mpi_reply; - Mpi2EventDataSasBroadcastPrimitive_t *event_data = fw_event-event_data; - u16 ioc_status; - unsigned long flags; - int r; - u8 max_retries = 0; - u8 task_abort_retries; - - mutex_lock(ioc-tm_cmds.mutex); - dewtprintk(ioc,
Re: [PATCH] mpt2sas: don't handle broadcast primitives
On Fri, 19 July 2013 18:06:59 -0400, Jörn Engel wrote: The handling of broadcast primitives involves _scsih_block_io_all_device(), which does what the name implies. I have observed cases with 60s of blocking io on all devices, caused by a single bad device. The downsides of this code are obvious, while the upsides are more elusive. And since this patch looks more like an April fools joke: I have gathered a few machine-months of testing, including tortures that specifically stress the removed codepaths. This is a serious submission and unless someone can show me a _very_ good reason for keeping the deleted code, I would like to get it merged. Jörn -- Computer system analysis is like child-rearing; you can do grievous damage, but you cannot ensure success. -- Tom DeMarco -- 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 v2 2/3] ufs: don't disable_irq() if the IRQ can be shared among devices
When removing the UFS driver, disable_irq() is called and the IRQ is not enabled again. Unfortunately, the IRQ is requested with IRQF_SHARED and it can be shared among several devices. So disabling the IRQ in this way is just breaking other devices which are sharing the IRQ. Signed-off-by: Akinobu Mita m...@fixstars.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Santosh Y santos...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pci.c| 1 - drivers/scsi/ufs/ufshcd-pltfrm.c | 1 - 2 files changed, 2 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c index de4c52b..2349c0e 100644 --- a/drivers/scsi/ufs/ufshcd-pci.c +++ b/drivers/scsi/ufs/ufshcd-pci.c @@ -91,7 +91,6 @@ static void ufshcd_pci_remove(struct pci_dev *pdev) { struct ufs_hba *hba = pci_get_drvdata(pdev); - disable_irq(pdev-irq); ufshcd_remove(hba); pci_set_drvdata(pdev, NULL); } diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index c42db40..94ba40c 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -144,7 +144,6 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev) { struct ufs_hba *hba = platform_get_drvdata(pdev); - disable_irq(hba-irq); ufshcd_remove(hba); return 0; } -- 1.8.3.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 v2 3/3] ufs: don't stop controller before scsi_remove_host()
scsi_remove_host() sends SYNCHRONIZE CACHE commands for write cache enabled scsi disk devices. So stopping controller working shouldn't be done before scsi_remove_host(). Signed-off-by: Akinobu Mita m...@fixstars.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Santosh Y santos...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index b743bd6..bd4d418 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -1657,11 +1657,11 @@ EXPORT_SYMBOL_GPL(ufshcd_resume); */ void ufshcd_remove(struct ufs_hba *hba) { + scsi_remove_host(hba-host); /* disable interrupts */ ufshcd_disable_intr(hba, hba-intr_mask); ufshcd_hba_stop(hba); - scsi_remove_host(hba-host); scsi_host_put(hba-host); } EXPORT_SYMBOL_GPL(ufshcd_remove); -- 1.8.3.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 v2 1/3] ufshcd-pci: release ioremapped region during removing driver
Before commit 2953f850c3b80bdca004967c83733365d8aa0aa2 ([SCSI] ufs: use devres functions for ufshcd), UFSHCI register was ioremapped by each glue-driver (ufshcd-pltfrm and ufshcd-pci) during probing and it was iounmapped by core-driver during removing driver. The commit converted ufshcd-pltfrm to use devres functions, but it didn't convert ufshcd-pci. Therefore, the change causes ufshcd-pci driver not to iounmap UFSHCI register region during removing driver. This fixes it by converting ufshcd-pci to use devres functions. Signed-off-by: Akinobu Mita m...@fixstars.com Cc: Seungwon Jeon tgih@samsung.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Santosh Y santos...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pci.c | 37 + 1 file changed, 9 insertions(+), 28 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c index 48be39a..de4c52b 100644 --- a/drivers/scsi/ufs/ufshcd-pci.c +++ b/drivers/scsi/ufs/ufshcd-pci.c @@ -93,10 +93,7 @@ static void ufshcd_pci_remove(struct pci_dev *pdev) disable_irq(pdev-irq); ufshcd_remove(hba); - pci_release_regions(pdev); pci_set_drvdata(pdev, NULL); - pci_clear_master(pdev); - pci_disable_device(pdev); } /** @@ -133,53 +130,37 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) void __iomem *mmio_base; int err; - err = pci_enable_device(pdev); + err = pcim_enable_device(pdev); if (err) { - dev_err(pdev-dev, pci_enable_device failed\n); - goto out_error; + dev_err(pdev-dev, pcim_enable_device failed\n); + return err; } pci_set_master(pdev); - - err = pci_request_regions(pdev, UFSHCD); + err = pcim_iomap_regions(pdev, 1 0, UFSHCD); if (err 0) { - dev_err(pdev-dev, request regions failed\n); - goto out_disable; + dev_err(pdev-dev, request and iomap failed\n); + return err; } - mmio_base = pci_ioremap_bar(pdev, 0); - if (!mmio_base) { - dev_err(pdev-dev, memory map failed\n); - err = -ENOMEM; - goto out_release_regions; - } + mmio_base = pcim_iomap_table(pdev)[0]; err = ufshcd_set_dma_mask(pdev); if (err) { dev_err(pdev-dev, set dma mask failed\n); - goto out_iounmap; + return err; } err = ufshcd_init(pdev-dev, hba, mmio_base, pdev-irq); if (err) { dev_err(pdev-dev, Initialization failed\n); - goto out_iounmap; + return err; } pci_set_drvdata(pdev, hba); return 0; - -out_iounmap: - iounmap(mmio_base); -out_release_regions: - pci_release_regions(pdev); -out_disable: - pci_clear_master(pdev); - pci_disable_device(pdev); -out_error: - return err; } static DEFINE_PCI_DEVICE_TABLE(ufshcd_pci_tbl) = { -- 1.8.3.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 v2 0/3] ufs: fix bugs in probing and removing driver paths
The changes in this patch set are almost same as the previous one that I send on Jul 8. But the previous version wasn't cleanly applied to the upstream kernel because it depend on my local work in progress patches. So this version fixes it and it also includes another bug fix in the same area. Akinobu Mita (3): ufshcd-pci: release ioremapped region during removing driver ufs: don't disable_irq() if the IRQ can be shared among devices ufs: don't stop controller before scsi_remove_host() drivers/scsi/ufs/ufshcd-pci.c| 38 +- drivers/scsi/ufs/ufshcd-pltfrm.c | 1 - drivers/scsi/ufs/ufshcd.c| 2 +- 3 files changed, 10 insertions(+), 31 deletions(-) Cc: Seungwon Jeon tgih@samsung.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Santosh Y santos...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org -- 1.8.3.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
Re: [PATCH RESEND 0/1] AHCI: Optimize interrupt processing
On Fri, 2013-07-19 at 14:01 -0700, Nicholas A. Bellinger wrote: On Fri, 2013-07-19 at 08:33 -0700, James Bottomley wrote: On Thu, 2013-07-18 at 23:34 -0700, Nicholas A. Bellinger wrote: diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 0101af5..191bc15 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1144,7 +1144,11 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, sector_size=%u PAGE_SIZE, PIO may malfunction\n, sdev-sector_size); - blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + if (!q-mq_ops) { + blk_queue_update_dma_alignment(q, sdev-sector_size - 1); + } else { + printk(Skipping dma_alignment for libata w/ scsi-mq\n); + } Amazingly enough there is a reason for the dma alignment, and it wasn't just to annoy you, so you can't blindly do this. The email thread is probably lost in the mists of time, but if I remember correctly the problem is that some ahci DMA controllers barf if the sector they're doing DMA on crosses a page boundary. Some are annoying enough to actually cause silent data corruption. You won't find every ahci DMA controller doing this, so the change will work for some, but it will be hard to identify those it won't work for until people start losing data. Thanks for the extra background. So at least from what I gather thus far this shouldn't be an issue for initial testing with scsi-mq - libata w/ ata_piix. The correct fix, obviously, is to do the bio copy on the kernel path for unaligned data. It is OK to assume that REQ_TYPE_FS data is correctly aligned (because of the block to page alignment). Indeed. Looking into the bio_copy_kern() breakage next.. OK, after further investigation the root cause is a actually a missing bio-bio_end_io() - bio_copy_kern_endio() - bio_put() from the blk_end_sync_rq() callback path that scsi-mq REQ_TYPE_BLOCK_PC is currently using. Including the following patch into the scsi-mq working branch now, and reverting the libata dma_alignment=0x03 hack. Alexander, care to give this a try..? --nab diff --git a/block/blk-exec.c b/block/blk-exec.c index 0761c89..70303d2 100644 --- a/block/blk-exec.c +++ b/block/blk-exec.c @@ -25,7 +25,10 @@ static void blk_end_sync_rq(struct request *rq, int error) struct completion *waiting = rq-end_io_data; rq-end_io_data = NULL; - if (!rq-q-mq_ops) { + if (rq-q-mq_ops) { + if (rq-bio) + bio_endio(rq-bio, error); + } else { __blk_put_request(rq-q, rq); } -- 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