Re: linux-next: build failure after merge of the scsi tree
Quoting James Bottomley : On Fri, 2013-01-11 at 11:35 -0600, Brian King wrote: On 01/11/2013 10:05 AM, Greg KH wrote: > On Fri, Jan 11, 2013 at 03:37:17PM +, James Bottomley wrote: >> On Fri, 2013-01-11 at 09:27 -0600, Brian King wrote: >>> It looks like this was a due to the fact that the new patches >>> added __devinit tags in the same merge window the __devinit tag >>> itself was getting removed. >> >> Not exactly. The patch which makes them nops went into 3.8. Now >> there's a patch queued in, Gregs tree I presume, to remove them all and >> the #defines which causes the compile failure. >> >>> As to the sparse warnings, this patch fixed the ones that >>> were actual bugs in the new code, although we could have >>> made that more clear in the patch description. >>> >>> http://marc.info/?l=linux-scsi&m=135716576204083&w=2 >> >> Ah, thanks ... I've been on holiday for a while, so I did miss that. >> >>> There is one outstanding issue I am aware of which was an >>> array bounds compiler warning which looks to be a misdetection >>> by the compiler. Wendy and I discussed adding a BUG_ON >>> to stop the compiler from complaining. >>> >>> Wendy - lets queue these two changes up ASAP. They should both >>> be very simple changes. >> >> If it's a simple gcc bug, just ignore it. >> >> I do need you to redo the patches to remove the __dev annotations, >> though. We can't risk introducing a bisect killing compile breakage if >> Greg's tree merges before mine in the next merge window. > > This change should be pushed to Linus in time for 3.8-final, so there > should not be any bisect issues. We can do this either way. James - what is your preference? Drop everything and do a resend of the entire series or delta patches on top of what is currently in your tree? Drop everything and resend still, I think. There's still a rebase problem, because the merge failure will happen if I rebase the misc tree to beyond Greg's merge point and I'd rather not have to worry about it. Thanks, James Hi James, I just re-sent all patches which are against git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git. This kernel already have Greg's "Drivers: scsi: remove _dev* attributes" patch. Let me know if you have any questions for these patches. Thanks for for your help! Wendy -- 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/8] V2 ipr: Implement block iopoll
Hello Wen Xiong, On Sat, Jan 12, 2013 at 7:43 AM, wrote: > This patch implements blk iopoll in ipr driver for performance improvement. Can you provide the performance numbers with/without the io polling? It would be interesting to know. > Signed-off-by: Wen Xiong > --- > drivers/scsi/ipr.c | 221 > + > drivers/scsi/ipr.h |6 + > 2 files changed, 178 insertions(+), 49 deletions(-) > > Index: b/drivers/scsi/ipr.c > === > --- a/drivers/scsi/ipr.c2013-01-11 16:11:20.062476228 -0600 > +++ b/drivers/scsi/ipr.c2013-01-11 16:11:50.275910225 -0600 > @@ -108,6 +108,7 @@ static const struct ipr_chip_cfg_t ipr_c > .max_cmds = 100, > .cache_line_size = 0x20, > .clear_isr = 1, > + .iopoll_weight = 0, > { > .set_interrupt_mask_reg = 0x0022C, > .clr_interrupt_mask_reg = 0x00230, > @@ -132,6 +133,7 @@ static const struct ipr_chip_cfg_t ipr_c > .max_cmds = 100, > .cache_line_size = 0x20, > .clear_isr = 1, > + .iopoll_weight = 0, > { > .set_interrupt_mask_reg = 0x00288, > .clr_interrupt_mask_reg = 0x0028C, > @@ -156,6 +158,7 @@ static const struct ipr_chip_cfg_t ipr_c > .max_cmds = 1000, > .cache_line_size = 0x20, > .clear_isr = 0, > + .iopoll_weight = 64, > { > .set_interrupt_mask_reg = 0x00010, > .clr_interrupt_mask_reg = 0x00018, > @@ -3560,6 +3563,95 @@ static struct device_attribute ipr_ioa_r > .store = ipr_store_reset_adapter > }; > > +static int ipr_iopoll(struct blk_iopoll *iop, int budget); > + /** > + * ipr_show_iopoll_weight - Show ipr polling mode > + * @dev: class device struct > + * @buf: buffer > + * > + * Return value: > + * number of bytes printed to buffer > + **/ > +static ssize_t ipr_show_iopoll_weight(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct Scsi_Host *shost = class_to_shost(dev); > + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; > + unsigned long lock_flags = 0; > + int len; > + > + spin_lock_irqsave(shost->host_lock, lock_flags); > + len = snprintf(buf, PAGE_SIZE, "%d\n", ioa_cfg->iopoll_weight); > + spin_unlock_irqrestore(shost->host_lock, lock_flags); > + > + return len; > +} > + > +/** > + * ipr_store_iopoll_weight - Change the adapter's polling mode > + * @dev: class device struct > + * @buf: buffer > + * > + * Return value: > + * number of bytes printed to buffer > + **/ > +static ssize_t ipr_store_iopoll_weight(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct Scsi_Host *shost = class_to_shost(dev); > + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; > + unsigned long user_iopoll_weight; > + unsigned long lock_flags = 0; > + int i; > + > + if (!ioa_cfg->sis64) { > + dev_info(&ioa_cfg->pdev->dev, "blk-iopoll not supported on > this adapter\n"); > + return -EINVAL; > + } > + if (kstrtoul(buf, 10, &user_iopoll_weight)) > + return -EINVAL; > + > + if (user_iopoll_weight > 256) { > + dev_info(&ioa_cfg->pdev->dev, "Invalid blk-iopoll weight. It > must be less than 256\n"); > + return -EINVAL; > + } > + > + if (user_iopoll_weight == ioa_cfg->iopoll_weight) { > + dev_info(&ioa_cfg->pdev->dev, "Current blk-iopoll weight has > the same weight\n"); > + return strlen(buf); > + } > + > + if (blk_iopoll_enabled && ioa_cfg->iopoll_weight && > + ioa_cfg->sis64 && ioa_cfg->nvectors > 1) { > + for (i = 1; i < ioa_cfg->hrrq_num; i++) > + blk_iopoll_disable(&ioa_cfg->hrrq[i].iopoll); > + } > + > + spin_lock_irqsave(shost->host_lock, lock_flags); > + ioa_cfg->iopoll_weight = user_iopoll_weight; > + if (blk_iopoll_enabled && ioa_cfg->iopoll_weight && > + ioa_cfg->sis64 && ioa_cfg->nvectors > 1) { > + for (i = 1; i < ioa_cfg->hrrq_num; i++) { > + blk_iopoll_init(&ioa_cfg->hrrq[i].iopoll, > + ioa_cfg->iopoll_weight, ipr_iopoll); > + blk_iopoll_enable(&ioa_cfg->hrrq[i].iopoll); > + } > + } > + spin_unlock_irqrestore(shost->host_lock, lock_flags); > + > + return strlen(buf)
[PATCH 4/8] V2 ipr: Add support for MSI-X and distributed completion
The new generation IBM SAS Controllers will support MSI-X interrupts and Distributed Completion Processing features. This patch add these support in ipr device driver. Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c | 718 - drivers/scsi/ipr.h | 70 +++-- 2 files changed, 595 insertions(+), 193 deletions(-) Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 13:31:35.105915246 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 16:13:41.442476346 -0600 @@ -98,6 +98,7 @@ static unsigned int ipr_transop_timeout static unsigned int ipr_debug = 0; static unsigned int ipr_max_devs = IPR_DEFAULT_SIS64_DEVS; static unsigned int ipr_dual_ioa_raid = 1; +static unsigned int ipr_number_of_msix = 2; static DEFINE_SPINLOCK(ipr_driver_lock); /* This table describes the differences between DMA controller chips */ @@ -215,6 +216,8 @@ MODULE_PARM_DESC(dual_ioa_raid, "Enable module_param_named(max_devs, ipr_max_devs, int, 0); MODULE_PARM_DESC(max_devs, "Specify the maximum number of physical devices. " "[Default=" __stringify(IPR_DEFAULT_SIS64_DEVS) "]"); +module_param_named(number_of_msix, ipr_number_of_msix, int, 0); +MODULE_PARM_DESC(number_of_msix, "Specify the number of MSIX interrupts to use on capable adapters (1 - 5). (default:2)"); MODULE_LICENSE("GPL"); MODULE_VERSION(IPR_DRIVER_VERSION); @@ -595,8 +598,11 @@ static void ipr_reinit_ipr_cmnd(struct i struct ipr_ioasa *ioasa = &ipr_cmd->s.ioasa; struct ipr_ioasa64 *ioasa64 = &ipr_cmd->s.ioasa64; dma_addr_t dma_addr = ipr_cmd->dma_addr; + int hrrq_id; + hrrq_id = ioarcb->cmd_pkt.hrrq_id; memset(&ioarcb->cmd_pkt, 0, sizeof(struct ipr_cmd_pkt)); + ioarcb->cmd_pkt.hrrq_id = hrrq_id; ioarcb->data_transfer_length = 0; ioarcb->read_data_transfer_length = 0; ioarcb->ioadl_len = 0; @@ -646,12 +652,16 @@ static void ipr_init_ipr_cmnd(struct ipr * pointer to ipr command struct **/ static -struct ipr_cmnd *__ipr_get_free_ipr_cmnd(struct ipr_ioa_cfg *ioa_cfg) +struct ipr_cmnd *__ipr_get_free_ipr_cmnd(struct ipr_hrr_queue *hrrq) { - struct ipr_cmnd *ipr_cmd; + struct ipr_cmnd *ipr_cmd = NULL; + + if (likely(!list_empty(&hrrq->hrrq_free_q))) { + ipr_cmd = list_entry(hrrq->hrrq_free_q.next, + struct ipr_cmnd, queue); + list_del(&ipr_cmd->queue); + } - ipr_cmd = list_entry(ioa_cfg->free_q.next, struct ipr_cmnd, queue); - list_del(&ipr_cmd->queue); return ipr_cmd; } @@ -666,7 +676,8 @@ struct ipr_cmnd *__ipr_get_free_ipr_cmnd static struct ipr_cmnd *ipr_get_free_ipr_cmnd(struct ipr_ioa_cfg *ioa_cfg) { - struct ipr_cmnd *ipr_cmd = __ipr_get_free_ipr_cmnd(ioa_cfg); + struct ipr_cmnd *ipr_cmd = + __ipr_get_free_ipr_cmnd(&ioa_cfg->hrrq[IPR_INIT_HRRQ]); ipr_init_ipr_cmnd(ipr_cmd, ipr_lock_and_done); return ipr_cmd; } @@ -761,13 +772,12 @@ static int ipr_set_pcix_cmd_reg(struct i **/ static void ipr_sata_eh_done(struct ipr_cmnd *ipr_cmd) { - struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg; struct ata_queued_cmd *qc = ipr_cmd->qc; struct ipr_sata_port *sata_port = qc->ap->private_data; qc->err_mask |= AC_ERR_OTHER; sata_port->ioasa.status |= ATA_BUSY; - list_add_tail(&ipr_cmd->queue, &ioa_cfg->free_q); + list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q); ata_qc_complete(qc); } @@ -783,14 +793,13 @@ static void ipr_sata_eh_done(struct ipr_ **/ static void ipr_scsi_eh_done(struct ipr_cmnd *ipr_cmd) { - struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg; struct scsi_cmnd *scsi_cmd = ipr_cmd->scsi_cmd; scsi_cmd->result |= (DID_ERROR << 16); scsi_dma_unmap(ipr_cmd->scsi_cmd); scsi_cmd->scsi_done(scsi_cmd); - list_add_tail(&ipr_cmd->queue, &ioa_cfg->free_q); + list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q); } /** @@ -805,24 +814,30 @@ static void ipr_scsi_eh_done(struct ipr_ static void ipr_fail_all_ops(struct ipr_ioa_cfg *ioa_cfg) { struct ipr_cmnd *ipr_cmd, *temp; + struct ipr_hrr_queue *hrrq; ENTER; - list_for_each_entry_safe(ipr_cmd, temp, &ioa_cfg->pending_q, queue) { - list_del(&ipr_cmd->queue); + for_each_hrrq(hrrq, ioa_cfg) { + list_for_each_entry_safe(ipr_cmd, + temp, &hrrq->hrrq_pending_q, queue) { + list_del(&ipr_cmd->queue); + + ipr_cmd->s.ioasa.hdr.ioasc = + cpu_to_be32(IPR_IOASC_IOA_WAS_RESET); + ipr_cmd->s.ioasa.hdr.ilid = + cpu_to_be32(IPR_DRIVER_ILID); - ipr_cmd->s.ioasa.hdr.ioasc = cpu_to_be3
[PATCH 3/8] V2 ipr: Resource path error logging cleanup
From: Brian King The resource path as displayed by the ipr driver is the location string identifying a location on the SAS fabric. This patch adds the SCSI host number such that error logs can be more easily correlated in multiple adapter configurations. Signed-off-by: Brian King Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c | 78 - drivers/scsi/ipr.h |5 ++- 2 files changed, 56 insertions(+), 27 deletions(-) Index: b/drivers/scsi/ipr.h === --- a/drivers/scsi/ipr.h2013-01-11 13:30:56.053727535 -0600 +++ b/drivers/scsi/ipr.h2013-01-11 13:31:35.095911184 -0600 @@ -409,7 +409,7 @@ struct ipr_config_table_entry64 { __be64 dev_id; __be64 lun; __be64 lun_wwn[2]; -#define IPR_MAX_RES_PATH_LENGTH24 +#define IPR_MAX_RES_PATH_LENGTH48 __be64 res_path; struct ipr_std_inq_data std_inq_data; u8 reserved2[4]; @@ -1722,7 +1722,8 @@ struct ipr_ucode_image_header { if (ipr_is_device(hostrcb)) { \ if ((hostrcb)->ioa_cfg->sis64) {\ printk(KERN_ERR IPR_NAME ": %s: " fmt, \ - ipr_format_res_path(hostrcb->hcam.u.error64.fd_res_path, \ + ipr_format_res_path(hostrcb->ioa_cfg, \ + hostrcb->hcam.u.error64.fd_res_path, \ hostrcb->rp_buffer, \ sizeof(hostrcb->rp_buffer)),\ __VA_ARGS__); \ Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 13:31:21.703731278 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 13:31:35.105915246 -0600 @@ -1166,14 +1166,15 @@ static int ipr_is_same_device(struct ipr } /** - * ipr_format_res_path - Format the resource path for printing. + * __ipr_format_res_path - Format the resource path for printing. * @res_path: resource path * @buf: buffer + * @len: length of buffer provided * * Return value: * pointer to buffer **/ -static char *ipr_format_res_path(u8 *res_path, char *buffer, int len) +static char *__ipr_format_res_path(u8 *res_path, char *buffer, int len) { int i; char *p = buffer; @@ -1187,6 +1188,27 @@ static char *ipr_format_res_path(u8 *res } /** + * ipr_format_res_path - Format the resource path for printing. + * @ioa_cfg: ioa config struct + * @res_path: resource path + * @buf: buffer + * @len: length of buffer provided + * + * Return value: + * pointer to buffer + **/ +static char *ipr_format_res_path(struct ipr_ioa_cfg *ioa_cfg, +u8 *res_path, char *buffer, int len) +{ + char *p = buffer; + + *p = '\0'; + p += snprintf(p, buffer + len - p, "%d/", ioa_cfg->host->host_no); + __ipr_format_res_path(res_path, p, len - (buffer - p)); + return buffer; +} + +/** * ipr_update_res_entry - Update the resource entry. * @res: resource entry struct * @cfgtew:config table entry wrapper struct @@ -1226,8 +1248,8 @@ static void ipr_update_res_entry(struct if (res->sdev && new_path) sdev_printk(KERN_INFO, res->sdev, "Resource path: %s\n", - ipr_format_res_path(res->res_path, buffer, - sizeof(buffer))); + ipr_format_res_path(res->ioa_cfg, + res->res_path, buffer, sizeof(buffer))); } else { res->flags = cfgtew->u.cfgte->flags; if (res->flags & IPR_IS_IOA_RESOURCE) @@ -1613,8 +1635,8 @@ static void ipr_log_sis64_config_error(s ipr_err_separator; ipr_err("Device %d : %s", i + 1, -ipr_format_res_path(dev_entry->res_path, buffer, -sizeof(buffer))); + __ipr_format_res_path(dev_entry->res_path, + buffer, sizeof(buffer))); ipr_log_ext_vpd(&dev_entry->vpd); ipr_err("-New Device Information-\n"); @@ -1960,14 +1982,16 @@ static void ipr_log64_fabric_path(struct ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s\n", path_active_desc[i].desc, path_state_desc[j].desc, -ipr_format_res_path(fabric->res_path, buffer, -sizeof(buffer))); +ipr_format_res_path(hostrcb->ioa_cfg, +
[PATCH 5/8] V2 ipr: Reduce lock contention
This patch reduces lock contention while implementing distributed completion processing. Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c | 323 + drivers/scsi/ipr.h | 21 +-- 2 files changed, 240 insertions(+), 104 deletions(-) Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 13:40:15.992165067 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 16:11:20.062476228 -0600 @@ -552,7 +552,8 @@ static void ipr_trc_hook(struct ipr_cmnd struct ipr_trace_entry *trace_entry; struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg; - trace_entry = &ioa_cfg->trace[ioa_cfg->trace_index++]; + trace_entry = &ioa_cfg->trace[atomic_add_return + (1, &ioa_cfg->trace_index)%IPR_NUM_TRACE_ENTRIES]; trace_entry->time = jiffies; trace_entry->op_code = ipr_cmd->ioarcb.cmd_pkt.cdb[0]; trace_entry->type = type; @@ -563,6 +564,7 @@ static void ipr_trc_hook(struct ipr_cmnd trace_entry->cmd_index = ipr_cmd->cmd_index & 0xff; trace_entry->res_handle = ipr_cmd->ioarcb.res_handle; trace_entry->u.add_data = add_data; + wmb(); } #else #define ipr_trc_hook(ipr_cmd, type, add_data) do { } while (0) @@ -697,9 +699,15 @@ static void ipr_mask_and_clear_interrupt u32 clr_ints) { volatile u32 int_reg; + int i; /* Stop new interrupts */ - ioa_cfg->allow_interrupts = 0; + for (i = 0; i < ioa_cfg->hrrq_num; i++) { + spin_lock(&ioa_cfg->hrrq[i]._lock); + ioa_cfg->hrrq[i].allow_interrupts = 0; + spin_unlock(&ioa_cfg->hrrq[i]._lock); + } + wmb(); /* Set interrupt mask to stop all new interrupts */ if (ioa_cfg->sis64) @@ -818,6 +826,7 @@ static void ipr_fail_all_ops(struct ipr_ ENTER; for_each_hrrq(hrrq, ioa_cfg) { + spin_lock(&hrrq->_lock); list_for_each_entry_safe(ipr_cmd, temp, &hrrq->hrrq_pending_q, queue) { list_del(&ipr_cmd->queue); @@ -837,6 +846,7 @@ static void ipr_fail_all_ops(struct ipr_ del_timer(&ipr_cmd->timer); ipr_cmd->done(ipr_cmd); } + spin_unlock(&hrrq->_lock); } LEAVE; } @@ -991,12 +1001,9 @@ static void ipr_send_blocking_cmd(struct static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg) { if (ioa_cfg->hrrq_num == 1) - ioa_cfg->hrrq_index = 0; - else { - if (++ioa_cfg->hrrq_index >= ioa_cfg->hrrq_num) - ioa_cfg->hrrq_index = 1; - } - return ioa_cfg->hrrq_index; + return 0; + else + return (atomic_add_return(1, &ioa_cfg->hrrq_index) % (ioa_cfg->hrrq_num - 1)) + 1; } /** @@ -1018,7 +1025,7 @@ static void ipr_send_hcam(struct ipr_ioa struct ipr_cmnd *ipr_cmd; struct ipr_ioarcb *ioarcb; - if (ioa_cfg->allow_cmds) { + if (ioa_cfg->hrrq[IPR_INIT_HRRQ].allow_cmds) { ipr_cmd = ipr_get_free_ipr_cmnd(ioa_cfg); list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_pending_q); list_add_tail(&hostrcb->queue, &ioa_cfg->hostrcb_pending_q); @@ -2564,7 +2571,7 @@ static int ipr_reset_reload(struct ipr_i /* If we got hit with a host reset while we were already resetting the adapter for some reason, and the reset failed. */ - if (ioa_cfg->ioa_is_dead) { + if (ioa_cfg->hrrq[IPR_INIT_HRRQ].ioa_is_dead) { ipr_trace; return FAILED; } @@ -3205,7 +3212,8 @@ static void ipr_worker_thread(struct wor restart: do { did_work = 0; - if (!ioa_cfg->allow_cmds || !ioa_cfg->allow_ml_add_del) { + if (!ioa_cfg->hrrq[IPR_INIT_HRRQ].allow_cmds || + !ioa_cfg->allow_ml_add_del) { spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); return; } @@ -3453,7 +3461,7 @@ static ssize_t ipr_show_adapter_state(st int len; spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags); - if (ioa_cfg->ioa_is_dead) + if (ioa_cfg->hrrq[IPR_INIT_HRRQ].ioa_is_dead) len = snprintf(buf, PAGE_SIZE, "offline\n"); else len = snprintf(buf, PAGE_SIZE, "online\n"); @@ -3479,14 +3487,20 @@ static ssize_t ipr_store_adapter_state(s struct Scsi_Host *shost = class_to_shost(dev); struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; unsigned long lock_flags; - int result = count; + int result = count, i; if (!capable(CAP_SYS_ADMIN)) return -EACCES; spin_lo
[PATCH 7/8] V2 ipr: Driver version 2.6.0
Bump driver version. Signed-off-by: Wen Xiong --- drivers/scsi/ipr.h |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: b/drivers/scsi/ipr.h === --- a/drivers/scsi/ipr.h2013-01-11 16:11:50.275910225 -0600 +++ b/drivers/scsi/ipr.h2013-01-11 16:12:03.673719736 -0600 @@ -39,8 +39,8 @@ /* * Literals */ -#define IPR_DRIVER_VERSION "2.5.4" -#define IPR_DRIVER_DATE "(July 11, 2012)" +#define IPR_DRIVER_VERSION "2.6.0" +#define IPR_DRIVER_DATE "(November 16, 2012)" /* * IPR_MAX_CMD_PER_LUN: This defines the maximum number of outstanding -- -- 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 6/8] V2 ipr: Implement block iopoll
This patch implements blk iopoll in ipr driver for performance improvement. Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c | 221 + drivers/scsi/ipr.h |6 + 2 files changed, 178 insertions(+), 49 deletions(-) Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 16:11:20.062476228 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 16:11:50.275910225 -0600 @@ -108,6 +108,7 @@ static const struct ipr_chip_cfg_t ipr_c .max_cmds = 100, .cache_line_size = 0x20, .clear_isr = 1, + .iopoll_weight = 0, { .set_interrupt_mask_reg = 0x0022C, .clr_interrupt_mask_reg = 0x00230, @@ -132,6 +133,7 @@ static const struct ipr_chip_cfg_t ipr_c .max_cmds = 100, .cache_line_size = 0x20, .clear_isr = 1, + .iopoll_weight = 0, { .set_interrupt_mask_reg = 0x00288, .clr_interrupt_mask_reg = 0x0028C, @@ -156,6 +158,7 @@ static const struct ipr_chip_cfg_t ipr_c .max_cmds = 1000, .cache_line_size = 0x20, .clear_isr = 0, + .iopoll_weight = 64, { .set_interrupt_mask_reg = 0x00010, .clr_interrupt_mask_reg = 0x00018, @@ -3560,6 +3563,95 @@ static struct device_attribute ipr_ioa_r .store = ipr_store_reset_adapter }; +static int ipr_iopoll(struct blk_iopoll *iop, int budget); + /** + * ipr_show_iopoll_weight - Show ipr polling mode + * @dev: class device struct + * @buf: buffer + * + * Return value: + * number of bytes printed to buffer + **/ +static ssize_t ipr_show_iopoll_weight(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct Scsi_Host *shost = class_to_shost(dev); + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; + unsigned long lock_flags = 0; + int len; + + spin_lock_irqsave(shost->host_lock, lock_flags); + len = snprintf(buf, PAGE_SIZE, "%d\n", ioa_cfg->iopoll_weight); + spin_unlock_irqrestore(shost->host_lock, lock_flags); + + return len; +} + +/** + * ipr_store_iopoll_weight - Change the adapter's polling mode + * @dev: class device struct + * @buf: buffer + * + * Return value: + * number of bytes printed to buffer + **/ +static ssize_t ipr_store_iopoll_weight(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct Scsi_Host *shost = class_to_shost(dev); + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; + unsigned long user_iopoll_weight; + unsigned long lock_flags = 0; + int i; + + if (!ioa_cfg->sis64) { + dev_info(&ioa_cfg->pdev->dev, "blk-iopoll not supported on this adapter\n"); + return -EINVAL; + } + if (kstrtoul(buf, 10, &user_iopoll_weight)) + return -EINVAL; + + if (user_iopoll_weight > 256) { + dev_info(&ioa_cfg->pdev->dev, "Invalid blk-iopoll weight. It must be less than 256\n"); + return -EINVAL; + } + + if (user_iopoll_weight == ioa_cfg->iopoll_weight) { + dev_info(&ioa_cfg->pdev->dev, "Current blk-iopoll weight has the same weight\n"); + return strlen(buf); + } + + if (blk_iopoll_enabled && ioa_cfg->iopoll_weight && + ioa_cfg->sis64 && ioa_cfg->nvectors > 1) { + for (i = 1; i < ioa_cfg->hrrq_num; i++) + blk_iopoll_disable(&ioa_cfg->hrrq[i].iopoll); + } + + spin_lock_irqsave(shost->host_lock, lock_flags); + ioa_cfg->iopoll_weight = user_iopoll_weight; + if (blk_iopoll_enabled && ioa_cfg->iopoll_weight && + ioa_cfg->sis64 && ioa_cfg->nvectors > 1) { + for (i = 1; i < ioa_cfg->hrrq_num; i++) { + blk_iopoll_init(&ioa_cfg->hrrq[i].iopoll, + ioa_cfg->iopoll_weight, ipr_iopoll); + blk_iopoll_enable(&ioa_cfg->hrrq[i].iopoll); + } + } + spin_unlock_irqrestore(shost->host_lock, lock_flags); + + return strlen(buf); +} + +static struct device_attribute ipr_iopoll_weight_attr = { + .attr = { + .name = "iopoll_weight", + .mode = S_IRUGO | S_IWUSR, + }, + .show = ipr_show_iopoll_weight, + .store = ipr_store_iopoll_weight +}; + /** * ipr_alloc_ucode_buffer - Allocates a microcode download buffer * @buf_len: buffer length @@ -3928,6
[PATCH 8/8] V2 ipr: Fix sparse error in ipr driver
This patch fixes the following sparse error: CHECK drivers/scsi/ipr.c spinlock.h:147:9: warning: context imbalance in 'ipr_reset_reload' - unexpected unlock Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c | 69 + 1 file changed, 17 insertions(+), 52 deletions(-) Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 16:13:50.803418076 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 16:14:53.092477895 -0600 @@ -2553,36 +2553,6 @@ static void ipr_oper_timeout(struct ipr_ } /** - * ipr_reset_reload - Reset/Reload the IOA - * @ioa_cfg: ioa config struct - * @shutdown_type: shutdown type - * - * This function resets the adapter and re-initializes it. - * This function assumes that all new host commands have been stopped. - * Return value: - * SUCCESS / FAILED - **/ -static int ipr_reset_reload(struct ipr_ioa_cfg *ioa_cfg, - enum ipr_shutdown_type shutdown_type) -{ - if (!ioa_cfg->in_reset_reload) - ipr_initiate_ioa_reset(ioa_cfg, shutdown_type); - - spin_unlock_irq(ioa_cfg->host->host_lock); - wait_event(ioa_cfg->reset_wait_q, !ioa_cfg->in_reset_reload); - spin_lock_irq(ioa_cfg->host->host_lock); - - /* If we got hit with a host reset while we were already resetting -the adapter for some reason, and the reset failed. */ - if (ioa_cfg->hrrq[IPR_INIT_HRRQ].ioa_is_dead) { - ipr_trace; - return FAILED; - } - - return SUCCESS; -} - -/** * ipr_find_ses_entry - Find matching SES in SES table * @res: resource entry struct of SES * @@ -4797,22 +4767,18 @@ static int ipr_slave_alloc(struct scsi_d return rc; } -/** - * ipr_eh_host_reset - Reset the host adapter - * @scsi_cmd: scsi command struct - * - * Return value: - * SUCCESS / FAILED - **/ -static int __ipr_eh_host_reset(struct scsi_cmnd *scsi_cmd) +static int ipr_eh_host_reset(struct scsi_cmnd *cmd) { struct ipr_ioa_cfg *ioa_cfg; - int rc; + unsigned long lock_flags = 0; + int rc = SUCCESS; ENTER; - ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata; + ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata; + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags); if (!ioa_cfg->in_reset_reload) { + ipr_initiate_ioa_reset(ioa_cfg, IPR_SHUTDOWN_ABBREV); dev_err(&ioa_cfg->pdev->dev, "Adapter being reset as a result of error recovery.\n"); @@ -4820,20 +4786,19 @@ static int __ipr_eh_host_reset(struct sc ioa_cfg->sdt_state = GET_DUMP; } - rc = ipr_reset_reload(ioa_cfg, IPR_SHUTDOWN_ABBREV); - - LEAVE; - return rc; -} - -static int ipr_eh_host_reset(struct scsi_cmnd *cmd) -{ - int rc; + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); + wait_event(ioa_cfg->reset_wait_q, !ioa_cfg->in_reset_reload); + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags); - spin_lock_irq(cmd->device->host->host_lock); - rc = __ipr_eh_host_reset(cmd); - spin_unlock_irq(cmd->device->host->host_lock); + /* If we got hit with a host reset while we were already resetting +the adapter for some reason, and the reset failed. */ + if (ioa_cfg->hrrq[IPR_INIT_HRRQ].ioa_is_dead) { + ipr_trace; + rc = FAILED; + } + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); + LEAVE; return rc; } -- -- 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 2/8] V2 ipr: Handler ID memory allocation failure
From: Brian King Add code to handle memory allocation failures at module load time. Reported-by: Fengguang Wu Signed-off-by: Brian King Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c |7 +++ 1 file changed, 7 insertions(+) Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 13:30:56.053727535 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 13:31:21.703731278 -0600 @@ -8516,6 +8516,10 @@ static int ipr_alloc_mem(struct ipr_ioa_ BITS_TO_LONGS(ioa_cfg->max_devs_supported), GFP_KERNEL); ioa_cfg->vset_ids = kzalloc(sizeof(unsigned long) * BITS_TO_LONGS(ioa_cfg->max_devs_supported), GFP_KERNEL); + + if (!ioa_cfg->target_ids || !ioa_cfg->array_ids + || !ioa_cfg->vset_ids) + goto out_free_res_entries; } for (i = 0; i < ioa_cfg->max_devs_supported; i++) { @@ -8591,6 +8595,9 @@ out_free_vpd_cbs: ioa_cfg->vpd_cbs, ioa_cfg->vpd_cbs_dma); out_free_res_entries: kfree(ioa_cfg->res_entries); + kfree(ioa_cfg->target_ids); + kfree(ioa_cfg->array_ids); + kfree(ioa_cfg->vset_ids); goto out; } -- -- 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 0/8] V2 Add support for new IBM SAS controllers
This is version 2 of ipr patches to support new IBM SAS controllers. In V2, we have fixed the following suggestions/warning/sparse errors: 1.Changed simple_strtoul() to kstrtoul() in ipr_restore_iopoll_weight routine. 2.Removed the __dev annotations. 3.Fixed unlock bugs which are caused by my previous patches(reported by sparse). 4.BUG_ON gcc 4.7 warning. 5.Fixed sparse error in original ipr driver. Let me know if you have any questions. Thanks for your help! Wendy -- -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] V2 ipr: Add sereral new CCIN definitions for new adapters support
Add the appropriate definitions and table entries for new adapter support. Signed-off-by: Wen Xiong --- drivers/scsi/ipr.c | 10 ++ drivers/scsi/ipr.h |5 + 2 files changed, 15 insertions(+) Index: b/drivers/scsi/ipr.c === --- a/drivers/scsi/ipr.c2013-01-11 13:29:48.162478273 -0600 +++ b/drivers/scsi/ipr.c2013-01-11 13:30:56.053727535 -0600 @@ -9277,6 +9277,8 @@ static struct pci_device_id ipr_pci_tabl { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROC_FPGA_E2, PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57B2, 0, 0, 0 }, { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROC_FPGA_E2, + PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57C0, 0, 0, 0 }, + { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROC_FPGA_E2, PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57C3, 0, 0, 0 }, { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROC_FPGA_E2, PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57C4, 0, 0, 0 }, @@ -9290,6 +9292,14 @@ static struct pci_device_id ipr_pci_tabl PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57C8, 0, 0, 0 }, { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROCODILE, PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57CE, 0, 0, 0 }, + { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROCODILE, + PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57D5, 0, 0, 0 }, + { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROCODILE, + PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57D6, 0, 0, 0 }, + { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROCODILE, + PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57D7, 0, 0, 0 }, + { PCI_VENDOR_ID_IBM, PCI_DEVICE_ID_IBM_CROCODILE, + PCI_VENDOR_ID_IBM, IPR_SUBS_DEV_ID_57D8, 0, 0, 0 }, { } }; MODULE_DEVICE_TABLE(pci, ipr_pci_table); Index: b/drivers/scsi/ipr.h === --- a/drivers/scsi/ipr.h2013-01-11 13:29:53.292478040 -0600 +++ b/drivers/scsi/ipr.h2013-01-11 13:30:56.053727535 -0600 @@ -82,6 +82,7 @@ #define IPR_SUBS_DEV_ID_57B40x033B #define IPR_SUBS_DEV_ID_57B20x035F +#define IPR_SUBS_DEV_ID_57C00x0352 #define IPR_SUBS_DEV_ID_57C30x0353 #define IPR_SUBS_DEV_ID_57C40x0354 #define IPR_SUBS_DEV_ID_57C60x0357 @@ -94,6 +95,10 @@ #define IPR_SUBS_DEV_ID_574D0x0356 #define IPR_SUBS_DEV_ID_57C80x035D +#define IPR_SUBS_DEV_ID_57D50x03FB +#define IPR_SUBS_DEV_ID_57D60x03FC +#define IPR_SUBS_DEV_ID_57D70x03FF +#define IPR_SUBS_DEV_ID_57D80x03FE #define IPR_NAME "ipr" /* -- -- 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] cxgb4i: Remove the scsi host device when removing device
When doing a hotplug remove of a cxgb4 device, there are still dandling symlinks at /sys/class/scsi_host/hostX to the removed PCI device. The upper layer device may also try to send data, which may crash the system. The DETACH message from the lower level driver is sent to the ULD when the device is removed, when the scsi host should be removed from the system, avoiding any problems. After this patch, there are no more dangling symlinks and many attempts to crash the system while there is SCSI activity and removing the device have failed. Adding the device back again works as expected, with the scsi hosts showing up again. Based on a patch by Karen Xie. Cc: Karen Xie Cc: Divy La Rey Signed-off-by: Thadeu Lima de Souza Cascardo --- drivers/scsi/cxgbi/cxgb4i/cxgb4i.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c index f924b3c..3fecf35 100644 --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c @@ -1564,6 +1564,7 @@ static int t4_uld_state_change(void *handle, enum cxgb4_state state) break; case CXGB4_STATE_DETACH: pr_info("cdev 0x%p, DETACH.\n", cdev); + cxgbi_device_unregister(cdev); break; default: pr_info("cdev 0x%p, unknown state %d.\n", cdev, state); -- 1.7.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 v12 7/9] libata: scsi: no poll when ODD is powered off
Hello, On Fri, Jan 11, 2013 at 11:16:26AM +0800, Aaron Lu wrote: > OK, will make it atomic in next version, thanks for the advice. > > Perhaps I can add two scsi helper functions in scsi_lib.c like: > > void sdev_disable_disk_events(struct scsi_device *sdev) > { > atomic_inc(&sdev->disk_events_disable_depth); > } > > void sdev_enable_disk_events(struct scsi_device *sdev) > { > if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0)) > return; > atomic_dec(&sdev->disk_events_disable_depth); > } > > And call them in ATA layer. Do you like this? Sounds good to me. James, how does the series look to you? Thanks! -- tejun -- 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: linux-next: build failure after merge of the scsi tree
On Fri, 2013-01-11 at 11:35 -0600, Brian King wrote: > On 01/11/2013 10:05 AM, Greg KH wrote: > > On Fri, Jan 11, 2013 at 03:37:17PM +, James Bottomley wrote: > >> On Fri, 2013-01-11 at 09:27 -0600, Brian King wrote: > >>> It looks like this was a due to the fact that the new patches > >>> added __devinit tags in the same merge window the __devinit tag > >>> itself was getting removed. > >> > >> Not exactly. The patch which makes them nops went into 3.8. Now > >> there's a patch queued in, Gregs tree I presume, to remove them all and > >> the #defines which causes the compile failure. > >> > >>> As to the sparse warnings, this patch fixed the ones that > >>> were actual bugs in the new code, although we could have > >>> made that more clear in the patch description. > >>> > >>> http://marc.info/?l=linux-scsi&m=135716576204083&w=2 > >> > >> Ah, thanks ... I've been on holiday for a while, so I did miss that. > >> > >>> There is one outstanding issue I am aware of which was an > >>> array bounds compiler warning which looks to be a misdetection > >>> by the compiler. Wendy and I discussed adding a BUG_ON > >>> to stop the compiler from complaining. > >>> > >>> Wendy - lets queue these two changes up ASAP. They should both > >>> be very simple changes. > >> > >> If it's a simple gcc bug, just ignore it. > >> > >> I do need you to redo the patches to remove the __dev annotations, > >> though. We can't risk introducing a bisect killing compile breakage if > >> Greg's tree merges before mine in the next merge window. > > > > This change should be pushed to Linus in time for 3.8-final, so there > > should not be any bisect issues. > > We can do this either way. > > James - what is your preference? Drop everything and do a resend of the > entire series or delta patches on top of what is currently in your tree? Drop everything and resend still, I think. There's still a rebase problem, because the merge failure will happen if I rebase the misc tree to beyond Greg's merge point and I'd rather not have to worry about it. Thanks, 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: linux-next: build failure after merge of the scsi tree
On 01/11/2013 10:05 AM, Greg KH wrote: > On Fri, Jan 11, 2013 at 03:37:17PM +, James Bottomley wrote: >> On Fri, 2013-01-11 at 09:27 -0600, Brian King wrote: >>> It looks like this was a due to the fact that the new patches >>> added __devinit tags in the same merge window the __devinit tag >>> itself was getting removed. >> >> Not exactly. The patch which makes them nops went into 3.8. Now >> there's a patch queued in, Gregs tree I presume, to remove them all and >> the #defines which causes the compile failure. >> >>> As to the sparse warnings, this patch fixed the ones that >>> were actual bugs in the new code, although we could have >>> made that more clear in the patch description. >>> >>> http://marc.info/?l=linux-scsi&m=135716576204083&w=2 >> >> Ah, thanks ... I've been on holiday for a while, so I did miss that. >> >>> There is one outstanding issue I am aware of which was an >>> array bounds compiler warning which looks to be a misdetection >>> by the compiler. Wendy and I discussed adding a BUG_ON >>> to stop the compiler from complaining. >>> >>> Wendy - lets queue these two changes up ASAP. They should both >>> be very simple changes. >> >> If it's a simple gcc bug, just ignore it. >> >> I do need you to redo the patches to remove the __dev annotations, >> though. We can't risk introducing a bisect killing compile breakage if >> Greg's tree merges before mine in the next merge window. > > This change should be pushed to Linus in time for 3.8-final, so there > should not be any bisect issues. We can do this either way. James - what is your preference? Drop everything and do a resend of the entire series or delta patches on top of what is currently in your tree? Thanks, Brian -- Brian King Power Linux I/O IBM Linux Technology Center -- 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 v1 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors
blk_rq_map_sg() function merges the physically contiguous pages to use same scatter-gather node without checking if their page descriptors are contiguous or not. Now when dma_map_sg() is called on the scatter gather list, it would take the base page pointer from each node (one by one) and iterates through all of the pages in same sg node by keep incrementing the base page pointer with the assumption that physically contiguous pages will have their page descriptor address contiguous which may not be true if SPARSEMEM config is enabled. So here we may end referring to invalid page descriptor. Following table shows the example of physically contiguous pages but their page descriptor addresses non-contiguous. --- | Page Descriptor| Physical Address | -- | 0xc1e43fdc | 0xd000 | | 0xc2052000 | 0xe000 | --- With this patch, relevant blk-merge functions will also check if the physically contiguous pages are having page descriptors address contiguous or not? If not then, these pages are separated to be in different scatter-gather nodes. Signed-off-by: Subhash Jadavani --- block/blk-merge.c |6 ++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/block/blk-merge.c b/block/blk-merge.c index 936a110..623fca5 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -42,6 +42,9 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q, goto new_segment; if (!BIOVEC_SEG_BOUNDARY(q, bvprv, bv)) goto new_segment; + if ((bvprv->bv_page != bv->bv_page) && + (bvprv->bv_page + 1) != bv->bv_page) + goto new_segment; seg_size += bv->bv_len; bvprv = bv; @@ -126,6 +129,9 @@ __blk_segment_map_sg(struct request_queue *q, struct bio_vec *bvec, goto new_segment; if (!BIOVEC_SEG_BOUNDARY(q, *bvprv, bvec)) goto new_segment; + if ((bvprv->bv_page != bvec->bv_page) && + ((bvprv->bv_page + 1) != bvec->bv_page)) + goto new_segment; (*sg)->length += nbytes; } else { -- -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- 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: linux-next: build failure after merge of the scsi tree
On Fri, Jan 11, 2013 at 03:37:17PM +, James Bottomley wrote: > On Fri, 2013-01-11 at 09:27 -0600, Brian King wrote: > > It looks like this was a due to the fact that the new patches > > added __devinit tags in the same merge window the __devinit tag > > itself was getting removed. > > Not exactly. The patch which makes them nops went into 3.8. Now > there's a patch queued in, Gregs tree I presume, to remove them all and > the #defines which causes the compile failure. > > > As to the sparse warnings, this patch fixed the ones that > > were actual bugs in the new code, although we could have > > made that more clear in the patch description. > > > > http://marc.info/?l=linux-scsi&m=135716576204083&w=2 > > Ah, thanks ... I've been on holiday for a while, so I did miss that. > > > There is one outstanding issue I am aware of which was an > > array bounds compiler warning which looks to be a misdetection > > by the compiler. Wendy and I discussed adding a BUG_ON > > to stop the compiler from complaining. > > > > Wendy - lets queue these two changes up ASAP. They should both > > be very simple changes. > > If it's a simple gcc bug, just ignore it. > > I do need you to redo the patches to remove the __dev annotations, > though. We can't risk introducing a bisect killing compile breakage if > Greg's tree merges before mine in the next merge window. This change should be pushed to Linus in time for 3.8-final, so there should not be any bisect issues. thanks, greg k-h -- 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: linux-next: build failure after merge of the scsi tree
On Fri, 2013-01-11 at 09:27 -0600, Brian King wrote: > It looks like this was a due to the fact that the new patches > added __devinit tags in the same merge window the __devinit tag > itself was getting removed. Not exactly. The patch which makes them nops went into 3.8. Now there's a patch queued in, Gregs tree I presume, to remove them all and the #defines which causes the compile failure. > As to the sparse warnings, this patch fixed the ones that > were actual bugs in the new code, although we could have > made that more clear in the patch description. > > http://marc.info/?l=linux-scsi&m=135716576204083&w=2 Ah, thanks ... I've been on holiday for a while, so I did miss that. > There is one outstanding issue I am aware of which was an > array bounds compiler warning which looks to be a misdetection > by the compiler. Wendy and I discussed adding a BUG_ON > to stop the compiler from complaining. > > Wendy - lets queue these two changes up ASAP. They should both > be very simple changes. If it's a simple gcc bug, just ignore it. I do need you to redo the patches to remove the __dev annotations, though. We can't risk introducing a bisect killing compile breakage if Greg's tree merges before mine in the next merge window. 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: linux-next: build failure after merge of the scsi tree
It looks like this was a due to the fact that the new patches added __devinit tags in the same merge window the __devinit tag itself was getting removed. As to the sparse warnings, this patch fixed the ones that were actual bugs in the new code, although we could have made that more clear in the patch description. http://marc.info/?l=linux-scsi&m=135716576204083&w=2 There is one outstanding issue I am aware of which was an array bounds compiler warning which looks to be a misdetection by the compiler. Wendy and I discussed adding a BUG_ON to stop the compiler from complaining. Wendy - lets queue these two changes up ASAP. They should both be very simple changes. Thanks, Brian On 01/11/2013 01:34 AM, James Bottomley wrote: > On Fri, 2013-01-11 at 12:03 +1100, Stephen Rothwell wrote: >> Hi James, >> >> After merging the scsi tree, today's linux-next build (powerpc >> ppc64_defconfig) failed like this: >> >> drivers/scsi/ipr.c:9138:22: error: expected '=', ',', ';', 'asm' or >> '__attribute__' before 'ipr_enable_msix' >> drivers/scsi/ipr.c:9165:22: error: expected '=', ',', ';', 'asm' or >> '__attribute__' before 'ipr_enable_msi' >> drivers/scsi/ipr.c:9188:23: error: expected '=', ',', ';', 'asm' or >> '__attribute__' before 'name_msi_vectors' >> drivers/scsi/ipr.c:9200:22: error: expected '=', ',', ';', 'asm' or >> '__attribute__' before 'ipr_request_other_msi_irqs' >> drivers/scsi/ipr.c: In function 'ipr_probe_ioa': >> drivers/scsi/ipr.c:9422:4: error: implicit declaration of function >> 'ipr_enable_msix' [-Werror=implicit-function-declaration] >> drivers/scsi/ipr.c:9425:4: error: implicit declaration of function >> 'ipr_enable_msi' [-Werror=implicit-function-declaration] >> drivers/scsi/ipr.c:9517:3: error: implicit declaration of function >> 'name_msi_vectors' [-Werror=implicit-function-declaration] >> drivers/scsi/ipr.c:9523:4: error: implicit declaration of function >> 'ipr_request_other_msi_irqs' [-Werror=implicit-function-declaration] > > OK, fine, I'll drop all the ipr patches. I've been waiting for a month > for them to fix the smatch and sparse warnings. Please resend the > series with all the fixes. > > Thanks, > > 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 > -- Brian King Power Linux I/O IBM Linux Technology Center -- 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 V5 3/4] [SCSI] ufs: Add Platform glue driver for ufshcd
On 1/11/2013 4:11 PM, Sujit Reddy Thumma wrote: On 1/9/2013 5:41 PM, vinayak holikatti wrote: On Mon, Jan 7, 2013 at 1:11 PM, Sujit Reddy Thumma wrote: Hi Vinayak, I have few comments below: +#ifdef CONFIG_PM +/** + * ufshcd_pltfrm_suspend - suspend power management function + * @pdev: pointer to Platform device handle + * @mesg: power state + * + * Returns -ENOSYS What breaks if you return 0 instead of return -ENOSYS? Returning error seems to break platform suspend/resume until all the TODO's are addressed. If the current s/w cannot make h/w suspend, it should be okay to let the rest of the system be suspended. In that case how will the controller be in a working state once it resumes. It does not make sense to return zero and to notify the OS that everything is fine. Since the suspend routine doesn't do anything except returning zero, no power/clocks would be removed and the controller should be in the same state after resume. Do you see any system that removes power/clocks to controllers during suspend without knowledge of corresponding drivers? If so, then such systems must be fixed. In any case, blocking entire system suspend just because s/w isn't taking care of powering down one controller is not a good idea. I would like to hear from others on this as well. Yes, i agree with Sujit that there is no point in blocking the entire system suspend just because ufshcd haven't implemented their suspend functionality. returning 0 from this function should be fine. And as Sujit already mentioned, if during resume you don't find the UFS (controller / phy) state as it was left in suspend then it's a particular system's issue and which needs to be fixed. + */ +static int ufshcd_pltfrm_suspend(struct platform_device *pdev, +pm_message_t mesg) +{ + /* +* TODO: +* 1. Call ufshcd_suspend +* 2. Do bus specific power management +*/ + + return -ENOSYS; Returning error doesn't allow entire system to be suspended. Perhaps, you can do disable_irq() and return 0? +} + +/** + * ufshcd_pltfrm_resume - resume power management function + * @pdev: pointer to Platform device handle + * + * Returns -ENOSYS + */ +static int ufshcd_pltfrm_resume(struct platform_device *pdev) +{ + /* +* TODO: +* 1. Call ufshcd_resume. +* 2. Do bus specific wake up +*/ + + return -ENOSYS; enable_irq() and return 0? +} +#endif + +static int __devexit ufshcd_pltfrm_remove(struct platform_device *pdev) +{ + struct resource *mem_res; + struct resource *irq_res; + resource_size_t mem_size; + struct ufs_hba *hba = platform_get_drvdata(pdev); + + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); It would be better to save the irq number under "struct ufs_hba" during probe. So here during remove you just need simply need to call the "free_irq(irq_res->start, hba)" Will modify the code accordingly in the next patchset. + + if (!irq_res) + dev_err(&pdev->dev, "ufshcd: IRQ resource not available\n"); + else + free_irq(irq_res->start, hba); The documentation of free_irq says: "... On a shared IRQ the caller must ensure the interrupt is disabled on the card it drives before calling this function. .." I don't see disable_irq() getting called either here or ufshcd_remove(). Why would you want to disable the entire IRQ line when it is shared? Logical thing is to disable the interrupt on the controller. Since you have enabled the irq in ufshcd_init() and decremented the desc->depth you need to need to do disable_irq(). disable_irq() doesn't disable the irq line until all the shared irq drivers disables it. Also, on some systems not calling disable_irq() could be a problem - the power to wakeup irq monitor block couldn't be turned off if there are active irqs. + + ufshcd_remove(hba); Remove should be exactly opposite of probe(). So shouldn't you call the ufshcd_remove() first and then free_irq() after that. Some bugging controllers might raise the interrupt after resources are removed. this sequence will prevent it. Could you please add the same as comment in above code sequence? + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); You might want to save the pointer to mem_res in "struct ufs_hba" during probe and may use the same here. + if (!mem_res) + dev_err(&pdev->dev, "ufshcd: Memory resource not available\n"); + else { + mem_size = resource_size(mem_res); + release_mem_region(mem_res->start, mem_size); + } + platform_set_drvdata(pdev, NULL); + return 0; +} + +static const struct of_device_id ufs_of_match[] = { + { .compatible = "jedec,ufs-1.1"}, +}; + +static struct platform_driver ufshcd_pltfrm_driver = { + .probe = ufshcd_pltfrm_probe, + .remove = __d
Re: [PATCH V5 3/4] [SCSI] ufs: Add Platform glue driver for ufshcd
On 1/9/2013 5:41 PM, vinayak holikatti wrote: On Mon, Jan 7, 2013 at 1:11 PM, Sujit Reddy Thumma wrote: Hi Vinayak, I have few comments below: +#ifdef CONFIG_PM +/** + * ufshcd_pltfrm_suspend - suspend power management function + * @pdev: pointer to Platform device handle + * @mesg: power state + * + * Returns -ENOSYS What breaks if you return 0 instead of return -ENOSYS? Returning error seems to break platform suspend/resume until all the TODO's are addressed. If the current s/w cannot make h/w suspend, it should be okay to let the rest of the system be suspended. In that case how will the controller be in a working state once it resumes. It does not make sense to return zero and to notify the OS that everything is fine. Since the suspend routine doesn't do anything except returning zero, no power/clocks would be removed and the controller should be in the same state after resume. Do you see any system that removes power/clocks to controllers during suspend without knowledge of corresponding drivers? If so, then such systems must be fixed. In any case, blocking entire system suspend just because s/w isn't taking care of powering down one controller is not a good idea. I would like to hear from others on this as well. + */ +static int ufshcd_pltfrm_suspend(struct platform_device *pdev, +pm_message_t mesg) +{ + /* +* TODO: +* 1. Call ufshcd_suspend +* 2. Do bus specific power management +*/ + + return -ENOSYS; Returning error doesn't allow entire system to be suspended. Perhaps, you can do disable_irq() and return 0? +} + +/** + * ufshcd_pltfrm_resume - resume power management function + * @pdev: pointer to Platform device handle + * + * Returns -ENOSYS + */ +static int ufshcd_pltfrm_resume(struct platform_device *pdev) +{ + /* +* TODO: +* 1. Call ufshcd_resume. +* 2. Do bus specific wake up +*/ + + return -ENOSYS; enable_irq() and return 0? +} +#endif + +static int __devexit ufshcd_pltfrm_remove(struct platform_device *pdev) +{ + struct resource *mem_res; + struct resource *irq_res; + resource_size_t mem_size; + struct ufs_hba *hba = platform_get_drvdata(pdev); + + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); It would be better to save the irq number under "struct ufs_hba" during probe. So here during remove you just need simply need to call the "free_irq(irq_res->start, hba)" Will modify the code accordingly in the next patchset. + + if (!irq_res) + dev_err(&pdev->dev, "ufshcd: IRQ resource not available\n"); + else + free_irq(irq_res->start, hba); The documentation of free_irq says: "... On a shared IRQ the caller must ensure the interrupt is disabled on the card it drives before calling this function. .." I don't see disable_irq() getting called either here or ufshcd_remove(). Why would you want to disable the entire IRQ line when it is shared? Logical thing is to disable the interrupt on the controller. Since you have enabled the irq in ufshcd_init() and decremented the desc->depth you need to need to do disable_irq(). disable_irq() doesn't disable the irq line until all the shared irq drivers disables it. Also, on some systems not calling disable_irq() could be a problem - the power to wakeup irq monitor block couldn't be turned off if there are active irqs. + + ufshcd_remove(hba); Remove should be exactly opposite of probe(). So shouldn't you call the ufshcd_remove() first and then free_irq() after that. Some bugging controllers might raise the interrupt after resources are removed. this sequence will prevent it. Could you please add the same as comment in above code sequence? + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); You might want to save the pointer to mem_res in "struct ufs_hba" during probe and may use the same here. + if (!mem_res) + dev_err(&pdev->dev, "ufshcd: Memory resource not available\n"); + else { + mem_size = resource_size(mem_res); + release_mem_region(mem_res->start, mem_size); + } + platform_set_drvdata(pdev, NULL); + return 0; +} + +static const struct of_device_id ufs_of_match[] = { + { .compatible = "jedec,ufs-1.1"}, +}; + +static struct platform_driver ufshcd_pltfrm_driver = { + .probe = ufshcd_pltfrm_probe, + .remove = __devexit_p(ufshcd_pltfrm_remove), +#ifdef CONFIG_PM CONFIG_PM_SLEEP would be better? the current implementation looks fine. Also, can you move legacy suspend/resume Ok, callbacks below to dev_pm_ops? + .suspend = ufshcd_pltfrm_suspend, + .resume = ufshcd_pltfrm_resume, +#endif + .driver = { + .name = "ufshcd", + .owner = THIS_MODULE, + .of_match_table = ufs_of_match, + }, +}; -- Rega
RE: [patch] [SCSI] bfa: fix strncpy() limiter in bfad_start_ops()
-Original Message- From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Dan Carpenter Sent: Thursday, January 10, 2013 2:36 PM To: Anil Gurumurthy Cc: Vijay Mohan Guvva; James E.J. Bottomley; linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org; kernel-janit...@vger.kernel.org Subject: [patch] [SCSI] bfa: fix strncpy() limiter in bfad_start_ops() The closing parenthesis is in the wrong place so it takes the sizeof a pointer instead of the sizeof the buffer minus one. Signed-off-by: Dan Carpenter diff --git a/drivers/scsi/bfa/bfad.c b/drivers/scsi/bfa/bfad.c index e6bf126..a5f7690 100644 --- a/drivers/scsi/bfa/bfad.c +++ b/drivers/scsi/bfa/bfad.c @@ -1034,7 +1034,7 @@ bfad_start_ops(struct bfad_s *bfad) { sizeof(driver_info.host_os_patch) - 1); strncpy(driver_info.os_device_name, bfad->pci_name, - sizeof(driver_info.os_device_name - 1)); + sizeof(driver_info.os_device_name) - 1); /* FCS driver info init */ spin_lock_irqsave(&bfad->bfad_lock, flags); -- Looks good. Thanks for the patch, Dan. Acked-by: Anil Gurumurthy 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