Re: [PATCH 3/4] qla2xxx: Add Block Multi Queue functionality.
Hi Christoph,
On 11/4/16, 4:00 PM, "Christoph Hellwig" wrote:
>On Fri, Nov 04, 2016 at 09:33:32AM -0700, himanshu.madh...@cavium.com wrote:
>> From: Michael Hernandez
>>
>> Tell the SCSI layer how many hardware queues we have based on the
>> number of max queue pairs created. The number of max queue pairs
>> created will depend on number of MSI X vector count or number of CPU's
>> in a system.
>
>Anf for that you must use pci_alloc_irq_vectors with the
>PCI_IRQ_AFFINITY flag for all new code. Please rework that code to use
>that.
Thanks for the input. We will rework this patch and resubmit the series.
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 0/2] qla2xxx: fix errors in PCI device remove with ongoing I/O
On 11/7/16, 11:53 AM, "Mauricio Faria de Oliveira" wrote: >This patchset addresses a couple of errors that might happen during >PCI device remove (e.g., PCI hotplug, PowerVM DLPAR), which prevent >the successful removal and re-addition of the adapter to the system, >and cause an oops and/or invalid DMA access (triggers an EEH event). > >It allowed several cycles of PCI device add/remove with ongoing I/O, >to complete successfully without triggering oopses or EEH events. > >Verified on v4.9-rc3. > >Test-case: >--- ># lspci ><...> >001d:70:00.0 Fibre Channel: QLogic Corp. ISP2532-based ... >001d:70:00.1 Fibre Channel: QLogic Corp. ISP2532-based ... ><...> > ># for sd in $(find /sys/bus/pci/devices/001d:70:00.*/ \ > -name 'sd*' -printf "%f\n"); do \ >dd if=/dev/$sd of=/dev/null iflag=nocache & done > ># echo 1 | tee /sys/bus/pci/devices/001d:70:00.*/remove >(this either works or not) > ># echo 1 > /sys/bus/pci/rescan > >Before: >--- ><...> >EEH: Frozen PHB#1d-PE#70 detected >qla2xxx [001d:70:00.1]-8042:2: PCI/Register disconnect, exiting. ><...> >EEH: Detected PCI bus error on PHB#29-PE#70 ><...> >(and/or) >Unable to handle kernel paging request for data at address 0x0138 ><...> >NIP [d4700a40] qla2xxx_queuecommand+0x80/0x3f0 [qla2xxx] >LR [d4700a10] qla2xxx_queuecommand+0x50/0x3f0 [qla2xxx] > >(command does not return; adapter cannot be re-added) > >After: >--- ><...> >qla2xxx [001d:70:00.0]-801c:1: Abort command issued nexus=1:0:0 -- 1 2003. ><...> >qla2xxx [001d:70:00.1]-801c:2: Abort command issued nexus=2:3:0 -- 1 2003. ><...> > >(command does return; adapter can be re-added correctly) > > >Mauricio Faria de Oliveira (2): > qla2xxx: do not queue commands when unloading > qla2xxx: fix invalid DMA access after command aborts in PCI device >remove > > drivers/scsi/qla2xxx/qla_os.c | 14 ++ > 1 file changed, 14 insertions(+) > >-- >1.8.3.1 > Thanks for the patches. Series Looks Good. Acked-by: Himanshu Madhani >
Re: [PATCH 3/4] qla2xxx: Add Block Multi Queue functionality.
Hi Ewan,
On 11/7/16, 8:43 AM, "Ewan D. Milne" wrote:
>On Fri, 2016-11-04 at 09:33 -0700, himanshu.madh...@cavium.com wrote:
>> From: Michael Hernandez
>>
>> Tell the SCSI layer how many hardware queues we have based on the
>> number of max queue pairs created. The number of max queue pairs
>> created will depend on number of MSI X vector count or number of CPU's
>> in a system.
>>
>> This feature can be turned on via CONFIG_SCSI_MQ_DEFAULT or passing
>> scsi_mod.use_blk_mq=Y as a parameter to the kernel
>> Queue pair creation depend on module parameter "ql2xmqsupport", which
>> need to be enabled to create queue pair.
>>
>
>I don't understand this change at all. Setting ->nr_hw_queues causes
>the block layer to allocate a number of queues for that Scsi_Host
>object, but it does not appear as if this code uses that functionality.
>There is nothing in the patch that examines the tag to see which queue
>the request came in on, in order to map it to a hardware queue.
>
>Instead, this patch seems to be reworking the mechanism involved in
>NPIV vport creation, which creates an entirely separate Scsi_Host
>object. The driver was already creating separate request queues to
>the card for vports, so what does this have to do with Block-MQ?
Thanks for the review comments. We are reworking patch series to address
your review comments.
>
>-Ewan
>
>
Thanks,
Himanshu
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 2/4] qla2xxx: Fix mailbox command timeout due to starvation
Hi Ewan,
On 11/7/16, 7:53 AM, "Ewan D. Milne" wrote:
>On Fri, 2016-11-04 at 09:33 -0700, himanshu.madh...@cavium.com wrote:
>...
>> @@ -2349,6 +2349,17 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
>> return atomic_read(&vha->loop_state) == LOOP_READY;
>> }
>>
>> +static void qla2x00_destroy_mbx_wq(struct qla_hw_data *ha)
>> +{
>> +struct workqueue_struct *wq = ha->mbx_wq;
>> +
>> +if (wq) {
>> +ha->mbx_wq = NULL;
>> +flush_workqueue(wq);
>> +destroy_workqueue(wq);
>> +}
>> +}
>> +
>> /*
>> * PCI driver interface
>> */
>
>There is already a function qla2x00_destroy_deferred_work() that
>destroys 3 other workqueues.
>
>...
>
>> @@ -3059,6 +3079,8 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
>>
>> qla2x00_free_fw_dump(ha);
>>
>> +qla2x00_destroy_mbx_wq(ha);
>> +
>> pci_disable_pcie_error_reporting(pdev);
>> pci_disable_device(pdev);
>> }
>
>This code path (pci_driver->shutdown) does not appear to destroy the
>other workqueues created by the driver. ???
>
>> @@ -5011,6 +5033,8 @@ void qla2x00_relogin(struct scsi_qla_host *vha)
>> */
>> qla2x00_free_sysfs_attr(base_vha, false);
>>
>> +qla2x00_destroy_mbx_wq(ha);
>> +
>> fc_remove_host(base_vha->host);
>>
>> scsi_remove_host(base_vha->host);
>
>See above.
Ack. will fix up patch to address these comments.
>
>-Ewan
Thanks,
Himanshu
Re: [PATCH] qla2xxx: do not abort all commands in the adapter during EEH recovery
On 11/14/16, 1:26 PM, "Mauricio Faria de Oliveira"
wrote:
>The previous commit ("qla2xxx: fix invalid DMA access after command
>aborts in PCI device remove") introduced a regression during an EEH
>recovery, since the change to the qla2x00_abort_all_cmds() function
>calls qla2xxx_eh_abort(), which verifies the EEH recovery condition
>but handles it heavy-handed. (commit a465537ad1a4 "qla2xxx: Disable
>the adapter and skip error recovery in case of register disconnect.")
>
>This problem warrants a more general/optimistic solution right into
>qla2xxx_eh_abort() (eg in case a real command abort arrives during
>EEH recovery, or if it takes long enough to trigger command aborts);
>but it's still worth to add a check to ensure the code added by the
>previous commit is correct and contained within its owner function.
>
>This commit just adds a 'if (!ha->flags.eeh_busy)' check around it.
>(ahem; a trivial fix for this -rc series; sorry for this oversight.)
>
>With it applied, both PCI device remove and EEH recovery works fine.
>
>Fixes: 1535aa75a3d8 ("scsi: qla2xxx: fix invalid DMA access after
>command aborts in PCI device remove")
>Signed-off-by: Mauricio Faria de Oliveira
>---
> drivers/scsi/qla2xxx/qla_os.c | 21 +
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 567fa080e261..56d6142852a5 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -1456,15 +1456,20 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
> for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
> sp = req->outstanding_cmds[cnt];
> if (sp) {
>- /* Get a reference to the sp and drop the lock.
>- * The reference ensures this sp->done() call
>- * - and not the call in qla2xxx_eh_abort() -
>- * ends the SCSI command (with result 'res').
>+ /* Don't abort commands in adapter during EEH
>+ * recovery as it's not accessible/responding.
>*/
>- sp_get(sp);
>- spin_unlock_irqrestore(&ha->hardware_lock,
>flags);
>- qla2xxx_eh_abort(GET_CMD_SP(sp));
>- spin_lock_irqsave(&ha->hardware_lock, flags);
>+ if (!ha->flags.eeh_busy) {
>+ /* Get a reference to the sp and drop
>the lock.
>+ * The reference ensures this
>sp->done() call
>+ * - and not the call in
>qla2xxx_eh_abort() -
>+ * ends the SCSI command (with result
>'res').
>+ */
>+ sp_get(sp);
>+
>spin_unlock_irqrestore(&ha->hardware_lock, flags);
>+ qla2xxx_eh_abort(GET_CMD_SP(sp));
>+ spin_lock_irqsave(&ha->hardware_lock,
>flags);
>+ }
> req->outstanding_cmds[cnt] = NULL;
> sp->done(vha, sp, res);
> }
>--
>1.8.3.1
>
Acked-by: Himanshu Madhani
Thanks,
Himanshu
Re: [PATCH v2 2/5] qla2xxx: Fix mailbox command timeout due to starvation
On 11/30/16, 11:58 PM, "Hannes Reinecke" wrote: >On 11/30/2016 09:24 PM, Himanshu Madhani wrote: >> From: Samy >> >> Signed-off-by: Samy >> Signed-off-by: Himanshu Madhani >> --- >> drivers/scsi/qla2xxx/qla_def.h | 3 ++ >> drivers/scsi/qla2xxx/qla_mbx.c | 88 >> ++ >> drivers/scsi/qla2xxx/qla_os.c | 24 >> 3 files changed, 91 insertions(+), 24 deletions(-) >> >This could do with a some description. Thanks for the review. Will add more description when submitting revised series. > >Otherwise: > >Reviewed-by: Hannes Reinecke > >Cheers > >Hannes >-- >Dr. Hannes Reinecke Teamlead Storage & Networking >h...@suse.de +49 911 74053 688 >SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg >GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton >HRB 21284 (AG Nürnberg)
Re: [PATCH v2 3/5] qla2xxx: Add multiple queue pair functionality.
Hi Hannes,
On 12/1/16, 12:39 AM, "Hannes Reinecke" wrote:
>On 11/30/2016 09:24 PM, Himanshu Madhani wrote:
>> From: Michael Hernandez
>>
>> Replaced existing multiple queue functionality with framework
>> that allows for the creation of pairs of request and response queues,
>> either at start of day or dynamically.
>>
>> Signed-off-by: Sawan Chandak
>> Signed-off-by: Michael Hernandez
>> Signed-off-by: Himanshu Madhani
>> ---
>> drivers/scsi/qla2xxx/Makefile | 3 +-
>> drivers/scsi/qla2xxx/qla_attr.c | 36 ++--
>> drivers/scsi/qla2xxx/qla_bottom.c | 398
>> ++
>> drivers/scsi/qla2xxx/qla_dbg.c| 4 +-
>> drivers/scsi/qla2xxx/qla_def.h| 114 +--
>> drivers/scsi/qla2xxx/qla_gbl.h| 34 +++-
>> drivers/scsi/qla2xxx/qla_init.c | 14 +-
>> drivers/scsi/qla2xxx/qla_inline.h | 30 +++
>> drivers/scsi/qla2xxx/qla_iocb.c | 56 ++
>> drivers/scsi/qla2xxx/qla_isr.c| 109 +--
>> drivers/scsi/qla2xxx/qla_mbx.c| 44 +++--
>> drivers/scsi/qla2xxx/qla_mid.c| 116 +--
>> drivers/scsi/qla2xxx/qla_mq.c | 279 ++
>> drivers/scsi/qla2xxx/qla_os.c | 235 +++---
>> drivers/scsi/qla2xxx/qla_target.c | 4 +
>> drivers/scsi/qla2xxx/qla_top.c| 95 +
>> 16 files changed, 1230 insertions(+), 341 deletions(-)
>> create mode 100644 drivers/scsi/qla2xxx/qla_bottom.c
>> create mode 100644 drivers/scsi/qla2xxx/qla_mq.c
>> create mode 100644 drivers/scsi/qla2xxx/qla_top.c
>>
>[ .. ]
>> diff --git a/drivers/scsi/qla2xxx/qla_bottom.c
>> b/drivers/scsi/qla2xxx/qla_bottom.c
>> new file mode 100644
>> index 000..8bf757e
>> --- /dev/null
>> +++ b/drivers/scsi/qla2xxx/qla_bottom.c
>> @@ -0,0 +1,398 @@
>> +/*
>> + * QLogic Fibre Channel HBA Driver
>> + * Copyright (c) 2016 QLogic Corporation
>> + *
>> + * See LICENSE.qla2xxx for copyright and licensing details.
>> + */
>> +#include "qla_def.h"
>> +
>> +/**
>> + * qla2xxx_start_scsi_mq() - Send a SCSI command to the ISP
>> + * @sp: command to send to the ISP
>> + *
>> + * Returns non-zero if a failure occurred, else zero.
>> + */
>> +
>> +static int
>> +qla2xxx_start_scsi_mq(srb_t *sp)
>> +{
>> +int nseg;
>> +unsigned long flags;
>> +uint32_t*clr_ptr;
>> +uint32_tindex;
>> +uint32_thandle;
>> +struct cmd_type_7 *cmd_pkt;
>> +uint16_tcnt;
>> +uint16_treq_cnt;
>> +uint16_ttot_dsds;
>> +struct req_que *req = NULL;
>> +struct rsp_que *rsp = NULL;
>> +struct scsi_cmnd *cmd = GET_CMD_SP(sp);
>> +struct scsi_qla_host *vha = sp->fcport->vha;
>> +struct qla_hw_data *ha = vha->hw;
>> +struct qla_qpair *qpair = sp->qpair;
>> +
>> +/* Setup qpair pointers */
>> +rsp = qpair->rsp;
>> +req = qpair->req;
>> +
>> +/* So we know we haven't pci_map'ed anything yet */
>> +tot_dsds = 0;
>> +
>> +/* Send marker if required */
>> +if (vha->marker_needed != 0) {
>> +if (qla2x00_marker(vha, req, rsp, 0, 0, MK_SYNC_ALL) !=
>> +QLA_SUCCESS)
>> +return QLA_FUNCTION_FAILED;
>> +vha->marker_needed = 0;
>> +}
>> +
>> +/* Acquire qpair specific lock */
>> +spin_lock_irqsave(&qpair->qp_lock, flags);
>> +
>> +/* Check for room in outstanding command list. */
>> +handle = req->current_outstanding_cmd;
>> +for (index = 1; index < req->num_outstanding_cmds; index++) {
>> +handle++;
>> +if (handle == req->num_outstanding_cmds)
>> +handle = 1;
>> +if (!req->outstanding_cmds[handle])
>> +break;
>> +}
>> +if (index == req->num_outstanding_cmds)
>> +goto queuing_error;
>> +
>> +/* Map the sg table so we have an accurate count of sg entries needed */
>> +if (scsi_sg_count(cmd)) {
>> +nseg = dma_map_sg(&ha->pdev->dev, scsi_sglist(cmd),
>> +scsi_sg_count(cmd), cmd->sc_data_direction);
>> +if (unlikely(!nseg))
>> +goto queuing_error;
>> +} else
>> +nseg = 0;
>> +
>> +tot_dsds = nseg;
>> +req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
>> +if (req->cnt < (req_cnt + 2)) {
>> +cnt = IS_SHADOW_REG_CAPABLE(ha) ? *req->out_ptr :
>> +RD_REG_DWORD_RELAXED(req->req_q_out);
>> +if (req->ring_index < cnt)
>> +req->cnt = cnt - req->ring_index;
>> +else
>> +req->cnt = req->length -
>> +(req->ring_index - cnt);
>> +if (req->cnt < (req_cnt + 2))
>> +goto queuing_error;
>> +}
>> +
>> +/* Build command packet. */
>> +req->current_outstanding_cmd = handle;
>> +req->outstanding_cmds[handle] = sp;
>> +sp->handle = handle;
>> +cmd->host_scribble = (unsigned char *)(unsigned long)handle;
Re: [PATCH v2 4/5] qla2xxx: Add Block Multi Queue functionality.
On 12/1/16, 2:25 AM, "Christoph Hellwig" wrote:
>> -pci_disable_msix(ha->pdev);
>> +pci_free_irq_vectors(ha->pdev);
>
>Please make the switch to pci_alloc_irq_vectors / pci_free_irq_vectors
>a se[arate patch.
Ack. We’ll split up patch.
>
>> +ret = pci_alloc_irq_vectors(ha->pdev,
>> +MIN_MSIX_COUNT, ha->msix_count, PCI_IRQ_MSIX);
>
>And for proper blk-mq support you must use PCI_IRQ_AFFINITY to get
>the affinity right.
Ack.
>
>> /* Recalculate queue values */
>> -if (ql2xmqsupport) {
>> +if (ha->mqiobase && ql2xmqsupport) {
>
>Where is that change coming from?
This change was introduced in previous patch.
>
>> cpus = num_online_cpus();
>> ha->max_req_queues = (ha->msix_count - 1 > cpus) ?
>> (cpus + 1) : (ha->msix_count - 1);
>> -ha->max_rsp_queues = ha->max_req_queues;
>>
>> /* ATIOQ needs 1 vector. That's 1 less QPair */
>> if (QLA_TGT_MODE_ENABLED())
>> ha->max_req_queues--;
>
>And don't do your own look at online cpus and queue spreading, that's
>what PCI_IRQ_AFFINITY and blk_mq_pci_map_queues are for.
We’ll update patch with the recommendation.
>
>> "MSI: Enabled.\n");
>> @@ -3273,11 +3261,10 @@ struct qla_init_msix_entry {
>>
>> if (ha->flags.msix_enabled)
>> qla24xx_disable_msix(ha);
>> -else if (ha->flags.msi_enabled) {
>> -free_irq(ha->pdev->irq, rsp);
>> -pci_disable_msi(ha->pdev);
>> -} else
>> -free_irq(ha->pdev->irq, rsp);
>> +else {
>> +free_irq(pci_irq_vector(ha->pdev, 0), rsp);
>> +pci_free_irq_vectors(ha->pdev);
>> +}
>
>Please also kill off qla24xx_disable_msix, and have a single
>callsite that iterates over all vectors and finally calls
>pci_free_irq_vectors.
Ack. We’ll update in revised patch.
>
>> -if (ql2xmqsupport) {
>> +if (ql2xmqsupport && ha->max_qpairs) {
>
>Where does this come from?
This was introduced in earlier patch
>
Re: [PATCH v3 1/6] qla2xxx: Only allow operational MBX to proceed during RESET.
On 12/5/16, 8:01 AM, "Christoph Hellwig" wrote: >Can you describe the changes in the body a bit more? Why do you >only want these commands to be sent? Why is the warning added >when it takes longer than nessecary? Otherwise this patch >looks fine to me. This patch is allowing only ROM mailbox command which are necessary to initialize chip after a reset has been issued. In a target environment, there could be a user space daemon which can issue statistics and other management mailbox command which are non-critical. This patch will timeout non critical mailbox commands immediately rather than waiting for timeout, if driver detects that chip reset has been issued or chip reset is in progress. Thanks, Himanshu >
Re: [PATCH v3 2/6] qla2xxx: Fix mailbox command timeout due to starvation
On 12/5/16, 8:03 AM, "Christoph Hellwig" wrote: >On Fri, Dec 02, 2016 at 01:44:53PM -0800, Himanshu Madhani wrote: >> From: Samy >> >> This patch helps resolve some of the mailbox timeout issue discovered >> during large SAN emulation testing where 1000+ initiators are trying >> to log into target mode personality. Since current mailbox interface >> handles submission and processing of commands in a sequential order, >> command could timeout resulting in some initiator not being able to >> log into target. > >How does the patch hange anything? You're still synchronously >waiting for the command, it's just that the command processing >is offloaded to a workqueue now. The mailbox timeout was issue here where command submitted to mailbox could wait for more than time allowed to wait_for_completion. We’ll see lot of mailbox timeout if the number of initiators trying to log into target mode personality increases to few thousands. >
Re: [PATCH v3 4/6] qla2xxx: Add multiple queue pair functionality.
On 12/4/16, 11:38 PM, "Hannes Reinecke" wrote:
>On 12/02/2016 10:44 PM, Himanshu Madhani wrote:
>> From: Michael Hernandez
>>
>> Replaced existing multiple queue functionality with framework
>> that allows for the creation of pairs of request and response queues,
>> either at start of day or dynamically.
>>
>> Signed-off-by: Sawan Chandak
>> Signed-off-by: Michael Hernandez
>> Signed-off-by: Himanshu Madhani
>> ---
>> drivers/scsi/qla2xxx/Makefile | 3 +-
>> drivers/scsi/qla2xxx/qla_attr.c | 36 ++--
>> drivers/scsi/qla2xxx/qla_bottom.c | 398
>> ++
>> drivers/scsi/qla2xxx/qla_dbg.c| 4 +-
>> drivers/scsi/qla2xxx/qla_def.h| 105 --
>> drivers/scsi/qla2xxx/qla_gbl.h| 32 ++-
>> drivers/scsi/qla2xxx/qla_init.c | 14 +-
>> drivers/scsi/qla2xxx/qla_inline.h | 30 +++
>> drivers/scsi/qla2xxx/qla_iocb.c | 56 ++
>> drivers/scsi/qla2xxx/qla_isr.c| 101 +-
>> drivers/scsi/qla2xxx/qla_mbx.c| 33 ++--
>> drivers/scsi/qla2xxx/qla_mid.c| 116 +--
>> drivers/scsi/qla2xxx/qla_mq.c | 236 ++
>> drivers/scsi/qla2xxx/qla_os.c | 237 +++
>> drivers/scsi/qla2xxx/qla_top.c| 95 +
>> 15 files changed, 1153 insertions(+), 343 deletions(-)
>> create mode 100644 drivers/scsi/qla2xxx/qla_bottom.c
>> create mode 100644 drivers/scsi/qla2xxx/qla_mq.c
>> create mode 100644 drivers/scsi/qla2xxx/qla_top.c
>>
>> diff --git a/drivers/scsi/qla2xxx/Makefile b/drivers/scsi/qla2xxx/Makefile
>> index 44def6b..ca04260 100644
>> --- a/drivers/scsi/qla2xxx/Makefile
>> +++ b/drivers/scsi/qla2xxx/Makefile
>> @@ -1,6 +1,7 @@
>> qla2xxx-y := qla_os.o qla_init.o qla_mbx.o qla_iocb.o qla_isr.o qla_gs.o \
>> qla_dbg.o qla_sup.o qla_attr.o qla_mid.o qla_dfs.o qla_bsg.o \
>> -qla_nx.o qla_mr.o qla_nx2.o qla_target.o qla_tmpl.o
>> +qla_nx.o qla_mr.o qla_nx2.o qla_target.o qla_tmpl.o qla_mq.o \
>> +qla_top.o qla_bottom.o
>>
>> obj-$(CONFIG_SCSI_QLA_FC) += qla2xxx.o
>> obj-$(CONFIG_TCM_QLA2XXX) += tcm_qla2xxx.o
>> diff --git a/drivers/scsi/qla2xxx/qla_attr.c
>> b/drivers/scsi/qla2xxx/qla_attr.c
>> index fe7469c..47eb4d5 100644
>> --- a/drivers/scsi/qla2xxx/qla_attr.c
>> +++ b/drivers/scsi/qla2xxx/qla_attr.c
>> @@ -1988,9 +1988,9 @@ struct device_attribute *qla2x00_host_attrs[] = {
>> scsi_qla_host_t *base_vha = shost_priv(fc_vport->shost);
>> scsi_qla_host_t *vha = NULL;
>> struct qla_hw_data *ha = base_vha->hw;
>> -uint16_t options = 0;
>> int cnt;
>> struct req_que *req = ha->req_q_map[0];
>> +struct qla_qpair *qpair;
>>
>> ret = qla24xx_vport_create_req_sanity_check(fc_vport);
>> if (ret) {
>> @@ -2075,15 +2075,9 @@ struct device_attribute *qla2x00_host_attrs[] = {
>> qlt_vport_create(vha, ha);
>> qla24xx_vport_disable(fc_vport, disable);
>>
>> -if (ha->flags.cpu_affinity_enabled) {
>> -req = ha->req_q_map[1];
>> -ql_dbg(ql_dbg_multiq, vha, 0xc000,
>> -"Request queue %p attached with "
>> -"VP[%d], cpu affinity =%d\n",
>> -req, vha->vp_idx, ha->flags.cpu_affinity_enabled);
>> -goto vport_queue;
>> -} else if (ql2xmaxqueues == 1 || !ha->npiv_info)
>> +if (!ql2xmqsupport || !ha->npiv_info)
>> goto vport_queue;
>> +
>> /* Create a request queue in QoS mode for the vport */
>> for (cnt = 0; cnt < ha->nvram_npiv_size; cnt++) {
>> if (memcmp(ha->npiv_info[cnt].port_name, vha->port_name, 8) == 0
>> @@ -2095,20 +2089,20 @@ struct device_attribute *qla2x00_host_attrs[] = {
>> }
>>
>> if (qos) {
>> -ret = qla25xx_create_req_que(ha, options, vha->vp_idx, 0, 0,
>> -qos);
>> -if (!ret)
>> +qpair = qla2xxx_create_qpair(vha, qos, vha->vp_idx);
>> +if (!qpair)
>> ql_log(ql_log_warn, vha, 0x7084,
>> -"Can't create request queue for VP[%d]\n",
>> +"Can't create qpair for VP[%d]\n",
>> vha->vp_idx);
>> else {
>> ql_dbg(ql_dbg_multiq, vha, 0xc001,
>> -"Request Que:%d Q0s: %d) created for VP[%d]\n",
>> -ret, qos, vha->vp_idx);
>> +"Queue pair: %d Qos: %d) created for VP[%d]\n",
>> +qpair->id, qos, vha->vp_idx);
>> ql_dbg(ql_dbg_user, vha, 0x7085,
>> -"Request Que:%d Q0s: %d) created for VP[%d]\n",
>> -ret, qos, vha->vp_idx);
>> -req = ha->req_q_map[ret];
>> +"Queue Pair: %d Qos: %d) created for VP[%d]\n",
>> +qpair->id, qos, vha->vp_idx);
>> +req = qpair->req;
>> +vha->qpair = qpa
Re: [PATCH v3 6/6] qla2xxx: Fix Target mode handling with Multiqueue changes.
On 12/4/16, 11:42 PM, "Hannes Reinecke" wrote:
>On 12/02/2016 10:44 PM, Himanshu Madhani wrote:
>> From: Quinn Tran
>>
>> - Fix race condition between dpc_thread accessing Multiqueue resources
>> and qla2x00_remove_one thread trying to free resource.
>> - Fix out of order free for Multiqueue resources. Also, Multiqueue
>> interrupts needs a workqueue. Interrupt needed to stop before
>> the wq can be destroyed.
>>
>> Signed-off-by: Quinn Tran
>> Signed-off-by: Himanshu Madhani
>> ---
>> drivers/scsi/qla2xxx/qla_def.h | 3 ++-
>> drivers/scsi/qla2xxx/qla_isr.c | 20 +++--
>> drivers/scsi/qla2xxx/qla_mq.c | 2 +-
>> drivers/scsi/qla2xxx/qla_os.c | 51
>> +++---
>> 4 files changed, 49 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>> index 607d539..e613535 100644
>> --- a/drivers/scsi/qla2xxx/qla_def.h
>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>> @@ -2734,7 +2734,8 @@ struct isp_operations {
>>
>> #define QLA_MSIX_DEFAULT0x00
>> #define QLA_MSIX_RSP_Q 0x01
>> -#define QLA_MSIX_QPAIR_MULTIQ_RSP_Q 0x02
>> +#define QLA_ATIO_VECTOR 0x02
>> +#define QLA_MSIX_QPAIR_MULTIQ_RSP_Q 0x03
>>
>> #define QLA_MIDX_DEFAULT0
>> #define QLA_MIDX_RSP_Q 1
>> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>> index 14f27a7..03384cf 100644
>> --- a/drivers/scsi/qla2xxx/qla_isr.c
>> +++ b/drivers/scsi/qla2xxx/qla_isr.c
>> @@ -2976,6 +2976,7 @@ struct qla_init_msix_entry {
>> static struct qla_init_msix_entry msix_entries[] = {
>> { "qla2xxx (default)", qla24xx_msix_default },
>> { "qla2xxx (rsp_q)", qla24xx_msix_rsp_q },
>> +{ "qla2xxx (atio_q)", qla83xx_msix_atio_q },
>> { "qla2xxx (qpair_multiq)", qla2xxx_msix_rsp_q },
>> };
>>
>> @@ -2984,17 +2985,10 @@ struct qla_init_msix_entry {
>> { "qla2xxx (rsp_q)", qla82xx_msix_rsp_q },
>> };
>>
>> -static struct qla_init_msix_entry qla83xx_msix_entries[] = {
>> -{ "qla2xxx (default)", qla24xx_msix_default },
>> -{ "qla2xxx (rsp_q)", qla24xx_msix_rsp_q },
>> -{ "qla2xxx (atio_q)", qla83xx_msix_atio_q },
>> -};
>> -
>> static int
>> qla24xx_enable_msix(struct qla_hw_data *ha, struct rsp_que *rsp)
>> {
>> #define MIN_MSIX_COUNT 2
>> -#define ATIO_VECTOR 2
>> int i, ret;
>> struct qla_msix_entry *qentry;
>> scsi_qla_host_t *vha = pci_get_drvdata(ha->pdev);
>> @@ -3051,7 +3045,7 @@ struct qla_init_msix_entry {
>> }
>>
>> /* Enable MSI-X vectors for the base queue */
>> -for (i = 0; i < 2; i++) {
>> +for (i = 0; i < (QLA_MSIX_RSP_Q + 1); i++) {
>> qentry = &ha->msix_entries[i];
>> qentry->handle = rsp;
>> rsp->msix = qentry;
>> @@ -3068,6 +3062,7 @@ struct qla_init_msix_entry {
>> if (ret)
>> goto msix_register_fail;
>> qentry->have_irq = 1;
>> +qentry->in_use = 1;
>>
>> /* Register for CPU affinity notification. */
>> irq_set_affinity_notifier(qentry->vector, &qentry->irq_notify);
>> @@ -3087,14 +3082,15 @@ struct qla_init_msix_entry {
>> * queue.
>> */
>> if (QLA_TGT_MODE_ENABLED() && IS_ATIO_MSIX_CAPABLE(ha)) {
>> -qentry = &ha->msix_entries[ATIO_VECTOR];
>> +qentry = &ha->msix_entries[QLA_ATIO_VECTOR];
>> rsp->msix = qentry;
>> qentry->handle = rsp;
>> scnprintf(qentry->name, sizeof(qentry->name),
>> -qla83xx_msix_entries[ATIO_VECTOR].name);
>> +msix_entries[QLA_ATIO_VECTOR].name);
>> +qentry->in_use = 1;
>> ret = request_irq(qentry->vector,
>> -qla83xx_msix_entries[ATIO_VECTOR].handler,
>> -0, qla83xx_msix_entries[ATIO_VECTOR].name, rsp);
>> +msix_entries[QLA_ATIO_VECTOR].handler,
>> +0, msix_entries[QLA_ATIO_VECTOR].name, rsp);
>> qentry->have_irq = 1;
>> }
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_mq.c b/drivers/scsi/qla2xxx/qla_mq.c
>> index a64b7b0..5543b4c 100644
>> --- a/drivers/scsi/qla2xxx/qla_mq.c
>> +++ b/drivers/scsi/qla2xxx/qla_mq.c
>> @@ -118,7 +118,7 @@ struct qla_qpair *qla2xxx_create_qpair(struct
>> scsi_qla_host *vha, int qos, int v
>> qpair->vp_idx = vp_idx;
>>
>> for (i = 0; i < ha->msix_count; i++) {
>> -msix = &ha->msix_entries[i + 2];
>> +msix = &ha->msix_entries[i];
>> if (msix->in_use)
>> continue;
>> qpair->msix = msix;
>> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>> index 3371b3f..91d8e7a 100644
>> --- a/drivers/scsi/qla2xxx/qla_os.c
>> +++ b/drivers/scsi/qla2xxx/qla_os.c
>> @@ -434,24 +434,41 @@ static void qla2x00_free_q
Re: [PATCH v3 3/6] qla2xxx: Utilize pci_alloc_irq_vectors/pci_free_irq_vectors calls.
On 12/5/16, 4:55 AM, "Christoph Hellwig" wrote:
>>+ void *handle;
>
>Just curious: why do you need this new handle field instead of just
>passing the rsp as the old code did?
We wanted to make it more generic pointer with new Q-pair frameworks.
So just renamed it. we’ll remove old rsp reference in reworked patch.
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH v3 4/6] qla2xxx: Add multiple queue pair functionality.
On 12/5/16, 12:43 PM, "linux-scsi-ow...@vger.kernel.org on behalf of Madhani, Himanshu" wrote: > >We need to have the spinlock because currently our NPIV implementation does >not utilizes >Q-pair framework. Looks like sent it prematurely. This comment should be read as following We need to have the spinlock because currently our NPIV implementation does not utilizes block mq tagging framework.
Re: [PATCH v3 4/6] qla2xxx: Add multiple queue pair functionality.
On 12/5/16, 8:20 AM, "Christoph Hellwig" wrote:
>> create mode 100644 drivers/scsi/qla2xxx/qla_bottom.c
>> create mode 100644 drivers/scsi/qla2xxx/qla_mq.c
>> create mode 100644 drivers/scsi/qla2xxx/qla_top.c
>
>What's the point of three new fairly small files, two of them very
>oddly named? Can't we keep the code together by logical groups?
Ack. Will merge code
>
>> +/* distill these fields down to 'online=0/1'
>> + * ha->flags.eeh_busy
>> + * ha->flags.pci_channel_io_perm_failure
>> + * base_vha->loop_state
>> + */
>> +uint32_t online:1;
>> +/* move vha->flags.difdix_supported here */
>> +uint32_t difdix_supported:1;
>> +uint32_t delete_in_progress:1;
>
>These probably should be atomic bitops.
Ack. Will update send out series.
>
>> +#define QLA_VHA_MARK_BUSY(__vha, __bail) do { \
>> +atomic_inc(&__vha->vref_count); \
>> +mb(); \
>> +if (__vha->flags.delete_progress) { \
>> +atomic_dec(&__vha->vref_count); \
>> +__bail = 1; \
>> +} else {\
>> +__bail = 0; \
>> +} \
>> } while (0)
>
>Something like this should be an inline function, not a macro
>that returns through an argument.
>
>(and move to lower case it while at it, please).
>
>Btw, the way vref_count is used looks incredibly buggy, instead
>of the busy wait just add a waie queue to sleep on in
>qla24xx_deallocate_vp_id.
We’ll work on this change and post as a separate patch.
>
>> +QLA_QPAIR_MARK_BUSY(qpair, bail);
>> +if (unlikely(bail))
>> +return NULL;
>> +
>> +sp = mempool_alloc(qpair->srb_mempool, flag);
>> +if (!sp)
>> +goto done;
>> +
>> +memset(sp, 0, sizeof(*sp));
>> +sp->fcport = fcport;
>> +sp->iocbs = 1;
>
>FYI, the blk-mq model would be to allocate the srps as part of the
>scsi_cmd by setting the cmd_size field in the host template. Note
>that this is also supported for the non-blk-mq path.
>
>> +qpair->delete_in_progress = 1;
>> +while (atomic_read(&qpair->ref_count))
>> +msleep(500);
>
>Please use a waitqueue instead of a busy wait here as well.
We’ll work on cleanup patch as separate patch.
>
>> +int ql2xmqsupport;
>> +module_param(ql2xmqsupport, int, S_IRUGO);
>> +MODULE_PARM_DESC(ql2xmqsupport,
>> +"Enable on demand multiple queue pairs support "
>> +"Default is 0 for no support. "
>> +"Set it to 1 to turn on mq qpair support.");
>
>Why isn't this enabled by default?
Ack. Will enable it by default.
>
>> +ha->queue_pair_map = kzalloc(sizeof(struct qla_qpair *)
>> +* ha->max_qpairs, GFP_KERNEL);
>
>Use kcalloc instead.
Ack. Will update this.
>
Re: [PATCH v4 3/6] qla2xxx: Utilize pci_alloc_irq_vectors/pci_free_irq_vectors calls.
Hi Christoph,
On 12/7/16, 11:03 AM, "Christoph Hellwig" wrote:
>> static int
>> qla24xx_enable_msix(struct qla_hw_data *ha, struct rsp_que *rsp)
>> {
>> #define MIN_MSIX_COUNT 2
>> #define ATIO_VECTOR 2
>> int i, ret;
>> struct qla_msix_entry *qentry;
>> scsi_qla_host_t *vha = pci_get_drvdata(ha->pdev);
>>
>> +ret = pci_alloc_irq_vectors(ha->pdev,
>> +MIN_MSIX_COUNT, ha->msix_count, PCI_IRQ_MSIX|PCI_IRQ_AFFINITY);
>
>Given that as-is the code only uses two vectors, and they are not
>for per-cpu queues using PCI_IRQ_AFFINITY is actually wrong, and
>you're better off without it. Also please fix the spacing:
>tabs for aligning continued arguments, and spaces around operators
>(although the revised version won't have an operator - so this is
>just for future reference:
>
Ack. Agree here this change should not be part of this patch.
> ret = pci_alloc_irq_vectors(ha->pdev, MIN_MSIX_COUNT,
> ha->msix_count, PCI_IRQ_MSIX);
>
>> +struct rsp_que *rsp = (struct rsp_que *)e->handle;
>
>No need for the cast here.
Ack. Will post update in new series.
>
>> +struct rsp_que *rsp = (struct rsp_que *)e->handle;
>
>Same here.
Will send new series soon.
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH v4 4/6] qla2xxx: Add multiple queue pair functionality.
Hi Christoph, On 12/7/16, 11:05 AM, "Christoph Hellwig" wrote: >On Tue, Dec 06, 2016 at 11:07:44AM -0800, Himanshu Madhani wrote: >> From: Michael Hernandez >> >> Replaced existing multiple queue functionality with framework >> that allows for the creation of pairs of request and response queues, >> either at start of day or dynamically. >> >> Queue pair creation depend on module parameter "ql2xmqsupport", >> which need to be enabled to create queue pair. > >This doesn't seem to address any of the feedback from the last >round of feedback, was is sent out before that? Following changes are incorporated into current patch - Merged code from new files into existing code - Enabled multi queue support by default. - Update to use kcalloc instead of kzalloc. Following review comments are not addressed as part of this patch - Using bits-ops instead of uint32_t. - Converting macros into inline function. - Fixing the vref counting in the driver. - Using wait queue instead of busy wait. - Remove driver allocated mempools for SRB and use the one passed in via command. These changes are going to need us to run regression tests and would take bit longer. Would you be willing to accept this patch and we will post updates as new patch later. Thanks, - Himanshu
Re: [PATCH] linux/types.h: enable endian checks for all sparse builds
Hi Mike/Bart,
On 12/8/16, 8:17 AM, "virtualization-boun...@lists.linux-foundation.org on
behalf of Michael S. Tsirkin"
wrote:
>On Thu, Dec 08, 2016 at 06:38:11AM +, Bart Van Assche wrote:
>> On 12/07/16 21:54, Michael S. Tsirkin wrote:
>> > On Thu, Dec 08, 2016 at 05:21:47AM +, Bart Van Assche wrote:
>> >> Additionally, there are notable exceptions to the rule that most drivers
>> >> are endian-clean, e.g. drivers/scsi/qla2xxx. I would appreciate it if it
>> >> would remain possible to check such drivers with sparse without enabling
>> >> endianness checks. Have you considered to change #ifdef __CHECK_ENDIAN__
>> >> into e.g. #ifndef __DONT_CHECK_ENDIAN__?
>> >
>> > The right thing is probably just to fix these, isn't it?
>> > Until then, why not just ignore the warnings?
>>
>> Neither option is realistic. With endian-checking enabled the qla2xxx
>> driver triggers so many warnings that it becomes a real challenge to
>> filter the non-endian warnings out manually:
>>
>> $ for f in "" CF=-D__CHECK_ENDIAN__; do make M=drivers/scsi/qla2xxx C=2\
>> $f | &grep -c ': warning:'; done
>> 4
>> 752
>
>You can always revert this patch in your tree, or whatever. It does not
>look like this will get fixed otherwise.
>
>> If you think it would be easy to fix the endian warnings triggered by
>> the qla2xxx driver, you are welcome to try to fix these.
>>
>> Bart.
>
>Yea, this hardware was designed by someone who thought mixing
>LE and BE all over the place is a good idea.
>But who said it should be easy?
>
>Maybe this change will be enough to motivate the maintainers.
>
>Here's a minor buglet for you as a motivator:
>
>if (ct_rsp->header.response !=
>cpu_to_be16(CT_ACCEPT_RESPONSE)) {
>ql_dbg(ql_dbg_disc + ql_dbg_buffer, vha,
> 0x2077,
>"%s failed rejected request on port_id:
> %02x%02x%02x Compeltion status 0x%x, response 0x%x\n",
>routine, vha->d_id.b.domain,
>vha->d_id.b.area, vha->d_id.b.al_pa,
> comp_status, ct_rsp->header.response);
>
>
>response is BE and isn't printed correctly.
>
>another:
>
>eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
>size += 4 + 4;
>
>ql_dbg(ql_dbg_disc, vha, 0x20bc,
>"Max_Frame_Size = %x.\n", eiter->a.max_frame_size);
>
>printed too late, it's be by that time.
>
>Here's another suspicious line
>
>ctio24->u.status1.flags = (atio->u.isp24.attr << 9) |
>cpu_to_le16(CTIO7_FLAGS_STATUS_MODE_1 |
>CTIO7_FLAGS_TERMINATE);
>
>shifting attr by 9 bits gives different results on BE and LE,
>mixing it with le16 looks rather strange.
>
>Another:
>
>ha->flags.dport_enabled =
>(mid_init_cb->init_cb.firmware_options_1 & BIT_7) != 0;
>
>BIT_7 is native endian, firmware_options_1 is LE I think.
>
>
>
>Look at qla27xx_find_valid_image as well.
>
>if (pri_image_status.signature != QLA27XX_IMG_STATUS_SIGN)
>
>qla27xx_image_status seems to be data coming from flash, but is
>somehow native-endian? Maybe ...
>
>
>lun = a->u.isp24.fcp_cmnd.lun;
>
>I think lun here is in hardware format (le?), code treats it
>as native.
>
>
>Not to speak about interface abuse all over the place.
>How about this:
>
>uint32_t *
>qla24xx_read_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t
>faddr,
>uint32_t dwords)
>{
>uint32_t i;
>struct qla_hw_data *ha = vha->hw;
>
>/* Dword reads to flash. */
>for (i = 0; i < dwords; i++, faddr++)
>dwptr[i] = cpu_to_le32(qla24xx_read_flash_dword(ha,
>flash_data_addr(ha, faddr)));
>
>return dwptr;
>}
>
>OK so we convert to LE ...
>
>qla24xx_read_flash_data(vha, dcode, faddr, 4);
>
>risc_addr = be32_to_cpu(dcode[2]);
>*srisc_addr = *srisc_addr == 0 ? risc_addr : *srisc_addr;
>risc_size = be32_to_cpu(dcode[3]);
>
>then happily assume it's BE.
>
>And again, coming from flash, it's unlikely to actually be in the native
>endian-ness as callers seem to assume. I'm guessing it's all BE.
>
>I poked at it a bit and was able to cut down # of warnings
>from 1700 to 1400 in an hour. Someone familiar with the code
>should look at it.
We’ll take a look and send patches to resolve these warnings.
>
>--
>MST
>___
>Virtualization mailing list
>virtualizat...@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/virtualization
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 16/22] qla2xxx: Improve RSCN handling in driver
Hi Hannes, On 12/7/16, 3:32 AM, "Hannes Reinecke" wrote: >This is a bit odd. >If you already have code for fixing the to-do, why is the to-do there? >And if the code doesn't work, what's the point of including it here? > >I would suggest removing the code; the comment should be giving enough >of a hint of what needs to be implemented. Thanks for the review comments. This is one looks like got left off. We’ll fix this code and resubmit this series. Thanks, - Himanshu >
Re: [PATCH v4 4/6] qla2xxx: Add multiple queue pair functionality.
On 12/9/16, 5:10 AM, "Christoph Hellwig" wrote:
>Ok. We'll still need to use PCI_IRQ_AFFINITY here after it's removed
>i nthe previous patch, but with pci_irq_alloc_vectors_affinity so that
>we can get the separate non-assignment vector right as in the previous
>discussion.
Ack. Will fix up patch and resend series.
Thanks,
- Himanshu
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH] scsi/qla2xxx: label endian-ness for many fields
On 12/9/16, 12:45 PM, "Michael S. Tsirkin" wrote:
>This adds endian-ness labels for lots of qla structs.
>Doing this cuts down number of sparse warnings from ~1700 to ~1400.
>Will help find and resolve some of real issues down the road.
>
>Signed-off-by: Michael S. Tsirkin
>
>---
>
>Compile-tested only.
We’ll review and run regression test with this change and update.
Thanks,
- Himanshu
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH v4 4/6] qla2xxx: Add multiple queue pair functionality.
Hi Martin,
On 12/9/16, 11:39 AM, "Madhani, Himanshu" wrote:
>
>On 12/9/16, 5:10 AM, "Christoph Hellwig" wrote:
>
>>Ok. We'll still need to use PCI_IRQ_AFFINITY here after it's removed
>>i nthe previous patch, but with pci_irq_alloc_vectors_affinity so that
>>we can get the separate non-assignment vector right as in the previous
>>discussion.
>
>Ack. Will fix up patch and resend series.
Would you be pulling changes for this new call into scsi tree anytime soon
https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=irq/for-block
>
>Thanks,
>- Himanshu
>
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 00/22] qla2xxx: Target code enhancemets and feature update
Hi Nic, On 12/6/16, 12:30 PM, "Himanshu Madhani" wrote: >Hi Nic, > >Please consider this series for target-pending. This series is based on >scsi-misc series that was submitted earlier today. > >Here's link for that series > http://marc.info/?l=linux-scsi&m=148105128210090&w=2 > >This series contains following changes > >o Clean up Target code and tcm_qla2xxx for cleaner interaction. >o Enhanced and strengthen support for T10-DIF/PI. >o Renamed qlini_mode to qlop_mode for better readbility. >o Improve driver capability to manage dual personality > (i.e. Initiator and Target) simultaneously. >o Added capability to send non-critical MBX via IOCB path. >o Some of the pending bug-fixes for target code. > >Thanks, >Himanshu > >Himanshu Madhani (3): > qla2xxx: Include ATIO queue in firmware dump when in target mode > qla2xxx: Set tcm_qla2xxx version to automatically track qla2xxx >version. > qla2xxx: Clear BIT 15 in NVRAM during initialization. > >Joe Carnuccio (1): > qla2xxx: Rerfactor use of sp context > >Quinn Tran (18): > qla2xxx: Fix wrong IOCB type assumption. > qla2xxx: Add DebugFS node of irq vector cpuid > qla2xxx: Collect addtional debug data for FW > qla2xxx: Fix crash due to null pointer access. > qla2xxx: Refactore target code to remove symbol dependency > qla2xxx: Refactor T10-DIF PI support > qla2xxx: Add framework for Async fabric discovery. > qla2xxx: Refactor session management code. > qla2xxx: Add Dual mode support in the driver > qla2xxx: Fix invalid handle erroneous message. > qla2xxx: Add interrupt polling mechanism > qla2xxx: Rename qlini_mode parameter > qla2xxx: Improve RSCN handling in driver > qla2xxx: Fix slow mem alloc behind lock > qla2xxx: Reduce exess wait during chip reset > qla2xxx: Allow relogin to go through if remote login did not finish > qla2xxx: Improve submission of non critical MB interface. > qla2xxx: Add check for corrupt ATIO. > > drivers/scsi/qla2xxx/qla_attr.c| 366 - > drivers/scsi/qla2xxx/qla_bsg.c | 21 +- > drivers/scsi/qla2xxx/qla_dbg.c |2 +- > drivers/scsi/qla2xxx/qla_dbg.h |1 + > drivers/scsi/qla2xxx/qla_def.h | 287 +++- > drivers/scsi/qla2xxx/qla_dfs.c | 237 ++- > drivers/scsi/qla2xxx/qla_fw.h | 107 +- > drivers/scsi/qla2xxx/qla_gbl.h | 86 +- > drivers/scsi/qla2xxx/qla_gs.c | 733 - > drivers/scsi/qla2xxx/qla_init.c| 1604 +++- > drivers/scsi/qla2xxx/qla_inline.h | 34 +- > drivers/scsi/qla2xxx/qla_iocb.c| 172 ++- > drivers/scsi/qla2xxx/qla_isr.c | 414 -- > drivers/scsi/qla2xxx/qla_mbx.c | 553 +-- > drivers/scsi/qla2xxx/qla_mid.c | 15 +- > drivers/scsi/qla2xxx/qla_mr.c | 52 +- > drivers/scsi/qla2xxx/qla_os.c | 454 -- > drivers/scsi/qla2xxx/qla_target.c | 2890 +--- > drivers/scsi/qla2xxx/qla_target.h | 257 ++-- > drivers/scsi/qla2xxx/qla_tmpl.c| 24 + > drivers/scsi/qla2xxx/tcm_qla2xxx.c | 753 -- > drivers/scsi/qla2xxx/tcm_qla2xxx.h | 16 +- > 22 files changed, 6658 insertions(+), 2420 deletions(-) > >-- >1.8.3.1 > Would you please review/comment on these patches. Thanks, Himanshu >
Re: [PATCH 00/22] qla2xxx: Target code enhancemets and feature update
Hi Bart, On 12/12/16, 3:10 PM, "Madhani, Himanshu" wrote: >Hi Nic, > > > >On 12/6/16, 12:30 PM, "Himanshu Madhani" wrote: > >>Hi Nic, >> >>Please consider this series for target-pending. This series is based on >>scsi-misc series that was submitted earlier today. >> >>Here's link for that series >> http://marc.info/?l=linux-scsi&m=148105128210090&w=2 >> >>This series contains following changes >> >>o Clean up Target code and tcm_qla2xxx for cleaner interaction. >>o Enhanced and strengthen support for T10-DIF/PI. >>o Renamed qlini_mode to qlop_mode for better readbility. >>o Improve driver capability to manage dual personality >> (i.e. Initiator and Target) simultaneously. >>o Added capability to send non-critical MBX via IOCB path. >>o Some of the pending bug-fixes for target code. >> >>Thanks, >>Himanshu >> >>Himanshu Madhani (3): >> qla2xxx: Include ATIO queue in firmware dump when in target mode >> qla2xxx: Set tcm_qla2xxx version to automatically track qla2xxx >>version. >> qla2xxx: Clear BIT 15 in NVRAM during initialization. >> >>Joe Carnuccio (1): >> qla2xxx: Rerfactor use of sp context >> >>Quinn Tran (18): >> qla2xxx: Fix wrong IOCB type assumption. >> qla2xxx: Add DebugFS node of irq vector cpuid >> qla2xxx: Collect addtional debug data for FW >> qla2xxx: Fix crash due to null pointer access. >> qla2xxx: Refactore target code to remove symbol dependency >> qla2xxx: Refactor T10-DIF PI support >> qla2xxx: Add framework for Async fabric discovery. >> qla2xxx: Refactor session management code. >> qla2xxx: Add Dual mode support in the driver >> qla2xxx: Fix invalid handle erroneous message. >> qla2xxx: Add interrupt polling mechanism >> qla2xxx: Rename qlini_mode parameter >> qla2xxx: Improve RSCN handling in driver >> qla2xxx: Fix slow mem alloc behind lock >> qla2xxx: Reduce exess wait during chip reset >> qla2xxx: Allow relogin to go through if remote login did not finish >> qla2xxx: Improve submission of non critical MB interface. >> qla2xxx: Add check for corrupt ATIO. >> >> drivers/scsi/qla2xxx/qla_attr.c| 366 - >> drivers/scsi/qla2xxx/qla_bsg.c | 21 +- >> drivers/scsi/qla2xxx/qla_dbg.c |2 +- >> drivers/scsi/qla2xxx/qla_dbg.h |1 + >> drivers/scsi/qla2xxx/qla_def.h | 287 +++- >> drivers/scsi/qla2xxx/qla_dfs.c | 237 ++- >> drivers/scsi/qla2xxx/qla_fw.h | 107 +- >> drivers/scsi/qla2xxx/qla_gbl.h | 86 +- >> drivers/scsi/qla2xxx/qla_gs.c | 733 - >> drivers/scsi/qla2xxx/qla_init.c| 1604 +++- >> drivers/scsi/qla2xxx/qla_inline.h | 34 +- >> drivers/scsi/qla2xxx/qla_iocb.c| 172 ++- >> drivers/scsi/qla2xxx/qla_isr.c | 414 -- >> drivers/scsi/qla2xxx/qla_mbx.c | 553 +-- >> drivers/scsi/qla2xxx/qla_mid.c | 15 +- >> drivers/scsi/qla2xxx/qla_mr.c | 52 +- >> drivers/scsi/qla2xxx/qla_os.c | 454 -- >> drivers/scsi/qla2xxx/qla_target.c | 2890 >> +--- >> drivers/scsi/qla2xxx/qla_target.h | 257 ++-- >> drivers/scsi/qla2xxx/qla_tmpl.c| 24 + >> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 753 -- >> drivers/scsi/qla2xxx/tcm_qla2xxx.h | 16 +- >> 22 files changed, 6658 insertions(+), 2420 deletions(-) >> >>-- >>1.8.3.1 >> > >Would you please review/comment on these patches. > >Thanks, >Himanshu >> Can we get some review comments so that I can address them before sending updated series. Thanks, - Himanshu >
Re: [PATCH 14/22] qla2xxx: Add interrupt polling mechanism
Hi Bart, Christoph, On 12/15/16, 1:27 AM, "Bart Van Assche" wrote: >On 12/14/2016 10:06 PM, Christoph Hellwig wrote: >> On Tue, Dec 06, 2016 at 12:30:43PM -0800, Himanshu Madhani wrote: >>> From: Quinn Tran >>> >>> This patch adds capability to poll for an interrupt, If hardware >>> does not generate any interrupt for 2 seconds. >> >> This description sounds like the hardware might be buggy and not >> generate interrupts, in which case 2 seconds is a very long time. >> >> Can you explain the intention a bit better? > >In addition to Christoph's question: is this a workaround for a hardware >bug or for a firmware bug? If it is a workaround for a firmware bug, >since the qla2xxx firmware can be upgraded, why is an interrupt polling >mechanism added to the kernel driver instead of fixing the firmware? > >Bart. This issue was seen where we were debugging intermittent slow switch response related to cable pull in a small set of test environment. We thought it was a HW issue but turns out to be the switch. We left the patch in the test cycle to see if the problem re-occur and missed taking it out before submission. Since then we have not seen the problem re-occur. We’ll drop this patch from the series. Thanks, - Himanshu >
Re: [PATCH 03/22] qla2xxx: Set tcm_qla2xxx version to automatically track qla2xxx version.
On 12/14/16, 1:12 PM, "Christoph Hellwig" wrote: >Please just remove TCM_QLA2XXX_VERSION entirely and use QLA2XXX_VERSION >directly instead. Ack. Will update patch and resubmit. >
Re: [PATCH 15/22] qla2xxx: Rename qlini_mode parameter
On 12/14/16, 1:07 PM, "Christoph Hellwig" wrote:
>On Tue, Dec 06, 2016 at 12:30:44PM -0800, Himanshu Madhani wrote:
>> From: Quinn Tran
>>
>> Qlogic's adapter is able to behave in multiple modes:
>> initiator, target, exclusive/either, and dual/both.
>>
>> This patch renames the qlini_mode -> qlop_mode and allow
>> different setting for each port and exchange resource control.
>
>Removing an old module parameter will break existing setups. Please
>add the new one overriding the old parameter, but don't remove the old
>one.
Ack. Will revise patch to preserve old parameter.
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 01/22] qla2xxx: Fix wrong IOCB type assumption.
On 12/14/16, 1:09 PM, "Christoph Hellwig" wrote: >On Tue, Dec 06, 2016 at 12:30:30PM -0800, Himanshu Madhani wrote: >> +uint32_t unpacked_lun = 0; > >Please remove the unpacked_lun variable as well, and just directly >pass 0 to qlt_issue_task_mgmt. Ack, will update and resend this patch. >
Re: [PATCH 08/22] qla2xxx: Refactore target code to remove symbol dependency
Hi Christoph, On 12/14/16, 1:18 PM, "Christoph Hellwig" wrote: > - the new qla2x00_free_fcport is entirely pointless, please drop > that part of the patch (and even if it wasn't pointless it should > have been a patch on it's own) > - please use struct names and not typedefs for your new structures > - pretty much avery single items in your list should be a separate > patch. And some of them are actively counterproductive and should > be dropped: > >- Remove direct access of scsi_status field in se_cmd > - Remove se_cmd from qlt_do_ctio_completion > - Remove se_cmd access in srr code section > - Move se_cmd struct outside of qla_tgt_cmd/qla_tgt_mgmt_cmd. We combined the patches which includes fixes, enhancements and cleanups to Support multiple target stacks, reviews from customer, and to improve code maintainability. This patch tries to organize code logically which is spread across qla_target and tcm_qla2xxx. We will split this patch into multiple subset and resubmit the series. Thanks, - Himanshu >
Re: [PATCH 05/22] qla2xxx: Add DebugFS node of irq vector cpuid
Hi Christoph, On 12/14/16, 1:13 PM, "Christoph Hellwig" wrote: >Doesn't make sense - an interrupt can be assigned to multiple >CPUIDs, and you can see the assignment based on the irq count >in /proc/interrupts > We Added this for our debug purpose. We’ll drop this patch in next series. Thanks, Himanshu >
Re: [PATCH v2 00/10] qla2xxx: Bug fixes for driver.
On 12/22/16, 1:25 AM, "Christoph Hellwig" wrote: >The whole series looks fine: > >Reviewed-by: Christoph Hellwig > >(not sure why you dropped my Reviewed-by: tags for all the previously >reviewed patches, though) Thanks for the review. Looks like oversight while sending updated series. Bart, Do you want me to send series updating Reviewed-by Tag or would you be able to update while applying to target-pending tree? Thanks, Himanshu >
Re: [PATCH v2 00/10] qla2xxx: Bug fixes for driver.
Hi Bart,
On 12/22/16, 9:02 AM, "Bart Van Assche" wrote:
>On Thu, 2016-12-22 at 16:44 +0000, Madhani, Himanshu wrote:
>> Do you want me to send series updating Reviewed-by Tag or would you be able
>> to update
>> while applying to target-pending tree?
>
>Hello Himanshu,
>
>The past three days I have been busy with rewriting the blk-mq-sched code so I
>have
>not yet had the time to review this patch series. I plan to do this tomorrow.
>
>BTW, it is a good idea to wait for feedback from everyone who is likely to post
>feedback before reposting a patch series such that all review comments can be
>addressed at once. Additionally, since v2 of this patch series was posted less
>than
>24 hours ago it is too early anyway to repost this patch series.
>
>Bart.
Understood. I’ll hold off on sending update to this series.
I am going to post series where we have done additional improvements and added
new features.
I wanted to get review comments before most of the folks take some time off
next week.
Thanks,
Himanshu
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed.
On 12/23/16, 1:01 AM, "Bart Van Assche" wrote:
>On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>> index f7df01b..b14455e 100644
>> --- a/drivers/scsi/qla2xxx/qla_def.h
>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>> @@ -1556,7 +1556,8 @@ struct link_statistics {
>> struct atio {
>> uint8_t entry_type; /* Entry type. */
>> uint8_t entry_count;/* Entry count. */
>> -uint8_t data[58];
>> +uint16_tattr_n_length;
>> +uint8_t data[56];
>> uint32_tsignature;
>> #define ATIO_PROCESSED 0xDEADDEAD /* Signature */
>> };
>
>Are struct atio and/or struct atio_from_isp the description of a firmware
>data structure? If so, do all firmware versions initialize the attr_n_length
>field or is there a minimum firmware version required for this field to be
>valid? Please mention any changed dependencies on the firmware version in the
>patch description. Please also fix the typo in the patch title when reposting
>this patch ("corrputed").
They represent same data structure from firmware. We will send follow up patch
to
Clean up redundant structure.
>
>> +/* This packet is corrupted. The header + payload
>> + * can not be trusted. There is no point in passing
>> + * it further up.
>> + */
>
>This is not the proper Linux kernel comment style (see also
>Documentation/process/coding-style.rst).
Ack. Will update patch with correct format.
>
>> diff --git a/drivers/scsi/qla2xxx/qla_target.h
>> b/drivers/scsi/qla2xxx/qla_target.h
>> index f26c5f6..f31d343 100644
>> --- a/drivers/scsi/qla2xxx/qla_target.h
>> +++ b/drivers/scsi/qla2xxx/qla_target.h
>> @@ -427,13 +427,33 @@ struct atio_from_isp {
>> struct {
>> uint8_t entry_type;/* Entry type. */
>> uint8_t entry_count; /* Entry count. */
>> -uint8_t data[58];
>> +uint16_t attr_n_length;
>> +#define FCP_CMD_LENGTH_MASK 0x0fff
>> +#define FCP_CMD_LENGTH_MIN 0x38
>> +uint8_t data[56];
>> uint32_t signature;
>> #define ATIO_PROCESSED 0xDEADDEAD /* Signature */
>> } raw;
>> } u;
>> } __packed;
>>
>> +static inline int fcpcmd_is_corrupted(struct atio *atio)
>> +{
>> +if (atio->entry_type == ATIO_TYPE7 &&
>> +(le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
>> +FCP_CMD_LENGTH_MIN))
>> +return 1;
>> +else
>> +return 0;
>> +}
>
>The above code shows that attr_n_length is a little endian field so please
>declare it as little endian where appropriate (__le16 instead of uint16_t).
Ack. Will update the patch
>
>Bart.
Re: [PATCH v2 06/10] qla2xxx: Fix crash due to null pointer access.
On 12/23/16, 12:47 AM, "Bart Van Assche" wrote: >This patch needs a more detailed description. There are many changes in this >patch. What changes are the changes that prevent the NULL pointer dereference? >What changes (if any) were made as the result of code inspection? We saw this stack trace on one test setup which never was reproduced. This patch is result of code inspection only and we have not seen this being Reproduced or reported by customer.
Re: [PATCH v2 04/10] qla2xxx: Reset reserved field in firmware options to 0.
On 12/23/16, 12:37 AM, "Bart Van Assche" wrote: >Please use cpu_to_le32() in new code instead of __constant_cpu_to_le32(). >gcc generates the same code for both conversion functions but the former >function makes source code easier to read. Ack. Will update the patch. >
Re: [PATCH v2 02/10] qla2xxx: Include ATIO queue in firmware dump when in target mode
On 12/23/16, 12:32 AM, "Bart Van Assche" wrote:
>Sparse reports this because the atio_q_in pointer is declared as uint32_t
>__iomem*.
>Does this perhaps mean that a readl() call is missing?
Ack. Will fix in revised series.
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 1/2] scsi: qla2xxx: silent -Wformat-security warning
On 12/26/16, 5:23 AM, "Nicolas Iooss" wrote: >qla24xx_enable_msix() calls scnprintf() with a non-literal format >string. This makes clang report -Wformat-security warnings when >compiling this function: > >drivers/scsi/qla2xxx/qla_isr.c:3083:7: error: format string is not a >string literal (potentially insecure) [-Werror,-Wformat-security] >msix_entries[i].name); >^~~~ >drivers/scsi/qla2xxx/qla_isr.c:3083:7: note: treat the string as an >argument to avoid this >msix_entries[i].name); >^ >"%s", >drivers/scsi/qla2xxx/qla_isr.c:3119:7: error: format string is not a >string literal (potentially insecure) [-Werror,-Wformat-security] >msix_entries[QLA_ATIO_VECTOR].name); >^~ >drivers/scsi/qla2xxx/qla_isr.c:3119:7: note: treat the string as an >argument to avoid this >msix_entries[QLA_ATIO_VECTOR].name); >^ >"%s", > >Even though msix_entries[...].name are initialized as literal strings >with no % character and are never modified, introduce a "%s" format >parameter in order to silent this -Wformat-security warning and make >clang able to detect at compile time real bugs related to string >formatting. > >Signed-off-by: Nicolas Iooss >--- > drivers/scsi/qla2xxx/qla_isr.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c >index 5093ca9b02ec..474b415217df 100644 >--- a/drivers/scsi/qla2xxx/qla_isr.c >+++ b/drivers/scsi/qla2xxx/qla_isr.c >@@ -3080,7 +3080,7 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct >rsp_que *rsp) > qentry->handle = rsp; > rsp->msix = qentry; > scnprintf(qentry->name, sizeof(qentry->name), >- msix_entries[i].name); >+ "%s", msix_entries[i].name); > if (IS_P3P_TYPE(ha)) > ret = request_irq(qentry->vector, > qla82xx_msix_entries[i].handler, >@@ -3116,7 +3116,7 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct >rsp_que *rsp) > rsp->msix = qentry; > qentry->handle = rsp; > scnprintf(qentry->name, sizeof(qentry->name), >- msix_entries[QLA_ATIO_VECTOR].name); >+ "%s", msix_entries[QLA_ATIO_VECTOR].name); > qentry->in_use = 1; > ret = request_irq(qentry->vector, > msix_entries[QLA_ATIO_VECTOR].handler, >-- >2.11.0 > Looks Good. Acked-by: Himanshu Madhani >
Re: [PATCH 2/2] scsi: qla2xxx: make msix_entries const
On 12/26/16, 5:23 AM, "Nicolas Iooss" wrote:
>msix_entries and qla82xx_msix_entries arrays are never modified in
>drivers/scsi/qla2xxx/qla_isr.c. Move their contents to read-only data.
>
>Signed-off-by: Nicolas Iooss
>---
> drivers/scsi/qla2xxx/qla_isr.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>index 474b415217df..b9c113e47346 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -3003,14 +3003,14 @@ struct qla_init_msix_entry {
> irq_handler_t handler;
> };
>
>-static struct qla_init_msix_entry msix_entries[] = {
>+static const struct qla_init_msix_entry msix_entries[] = {
> { "qla2xxx (default)", qla24xx_msix_default },
> { "qla2xxx (rsp_q)", qla24xx_msix_rsp_q },
> { "qla2xxx (atio_q)", qla83xx_msix_atio_q },
> { "qla2xxx (qpair_multiq)", qla2xxx_msix_rsp_q },
> };
>
>-static struct qla_init_msix_entry qla82xx_msix_entries[] = {
>+static const struct qla_init_msix_entry qla82xx_msix_entries[] = {
> { "qla2xxx (default)", qla82xx_msix_default },
> { "qla2xxx (rsp_q)", qla82xx_msix_rsp_q },
> };
>@@ -3284,7 +3284,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
> int qla25xx_request_irq(struct qla_hw_data *ha, struct qla_qpair *qpair,
> struct qla_msix_entry *msix, int vector_type)
> {
>- struct qla_init_msix_entry *intr = &msix_entries[vector_type];
>+ const struct qla_init_msix_entry *intr = &msix_entries[vector_type];
> scsi_qla_host_t *vha = pci_get_drvdata(ha->pdev);
> int ret;
>
>--
>2.11.0
>
Looks Good.
Acked-By: Himanshu Madhani
>
Re: [PATCH] qla2xxx: Get mutex lock before checking optrom_state
Hi Milan,
On 12/24/16, 8:32 AM, "linux-scsi-ow...@vger.kernel.org on behalf of Milan P.
Gandhi"
wrote:
>Hello,
>
>There is a race condition with qla2xxx optrom functions where
>one thread might modify optrom buffer, optrom_state while
>other thread is still reading from it.
>
>In couple of crashes, it was found that we had successfully
>passed the following 'if' check where we confirm optrom_state
>to be QLA_SREADING. But by the time we acquired mutex lock
>to proceed with memory_read_from_buffer function, some other
>thread/process had already modified that option rom buffer
>and optrom_state from QLA_SREADING to QLA_SWAITING. Then
>we got ha->optrom_buffer 0x0 and crashed the system:
>
>if (ha->optrom_state != QLA_SREADING)
>return 0;
>
>mutex_lock(&ha->optrom_mutex);
>rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
>ha->optrom_region_size);
>mutex_unlock(&ha->optrom_mutex);
>
>
>With current optrom function we get following crash due to
>a race condition:
>
>[ 1479.466679] BUG: unable to handle kernel NULL pointer dereference at
> (null)
>[ 1479.466707] IP: [] memcpy+0x6/0x110
>[...]
>[ 1479.473673] Call Trace:
>[ 1479.474296] [] ? memory_read_from_buffer+0x3c/0x60
>[ 1479.474941] [] qla2x00_sysfs_read_optrom+0x9c/0xc0
>[qla2xxx]
>[ 1479.475571] [] read+0xdb/0x1f0
>[ 1479.476206] [] vfs_read+0x9e/0x170
>[ 1479.476839] [] SyS_read+0x7f/0xe0
>[ 1479.477466] [] system_call_fastpath+0x16/0x1b
>
>
>Below patch modifies qla2x00_sysfs_read_optrom,
>qla2x00_sysfs_write_optrom functions to get the mutex_lock
>before checking ha->optrom_state to avoid similar crashes.
>
>The patch was applied and tested and same crashes were no
>longer observed again.
>
>
>Tested-by: Milan P. Gandhi
>Signed-off-by: Milan P. Gandhi
>---
> drivers/scsi/qla2xxx/qla_attr.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
>index da5ae11..47ea164 100644
>--- a/drivers/scsi/qla2xxx/qla_attr.c
>+++ b/drivers/scsi/qla2xxx/qla_attr.c
>@@ -329,12 +329,15 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct
>kobject *kobj,
> struct qla_hw_data *ha = vha->hw;
> ssize_t rval = 0;
>
>+ mutex_lock(&ha->optrom_mutex);
>+
> if (ha->optrom_state != QLA_SREADING)
>- return 0;
>+ goto out;
>
>- mutex_lock(&ha->optrom_mutex);
> rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
> ha->optrom_region_size);
>+
>+out:
> mutex_unlock(&ha->optrom_mutex);
>
> return rval;
>@@ -349,14 +352,19 @@ qla2x00_sysfs_write_optrom(struct file *filp, struct
>kobject *kobj,
> struct device, kobj)));
> struct qla_hw_data *ha = vha->hw;
>
>- if (ha->optrom_state != QLA_SWRITING)
>+ mutex_lock(&ha->optrom_mutex);
>+
>+ if (ha->optrom_state != QLA_SWRITING) {
>+ mutex_unlock(&ha->optrom_mutex);
> return -EINVAL;
>- if (off > ha->optrom_region_size)
>+ }
>+ if (off > ha->optrom_region_size) {
>+ mutex_unlock(&ha->optrom_mutex);
> return -ERANGE;
>+ }
> if (off + count > ha->optrom_region_size)
> count = ha->optrom_region_size - off;
>
>- mutex_lock(&ha->optrom_mutex);
> memcpy(&ha->optrom_buffer[off], buf, count);
> mutex_unlock(&ha->optrom_mutex);
>
>--
Thanks for the Patch. This looks good.
Acked-by: Himanshu Madhani
>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
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 05/11] qla2xxx: Fix wrong argument in sp done callback.
On 1/9/17, 5:17 AM, "Christoph Hellwig" wrote:
>On Fri, Dec 23, 2016 at 08:23:33PM -0800, Himanshu Madhani wrote:
>> From: Quinn Tran
>>
>> Callback for sp->done expects scsi_qla_host is passed in as argument,
>> Instead qla_hw_data is passed in.
>
>Please also fix the prototype of the done and free callbacks to use the
>actual type instead of two void pointers, so that the compiler can catch
>bugs like this in the future.
I have patch for the fixing of prototype of done and free callbacks which I was
planning to send out after this series. I’ll include that in V2 of this series.
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 03/11] qla2xxx: Make trace flags more readable.
On 1/9/17, 5:11 AM, "Christoph Hellwig" wrote:
>On Fri, Dec 23, 2016 at 08:23:31PM -0800, Himanshu Madhani wrote:
>> From: Quinn Tran
>>
>> This patch does not change any functionality.
>
>...
>> +++ b/drivers/scsi/qla2xxx/qla_target.c
>> @@ -3294,7 +3294,6 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd)
>> return EIO;
>> }
>> cmd->aborted = 1;
>> -cmd->cmd_flags |= BIT_6;
>
>This seems to be a change in behavior.
>
Will fix the patch. Intent was not to change any behavior. Just update the
flags to make it readable.
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH] qla2xxx: rename {vendor|hba}_indentifer to {vendor|hba}_identifer
On 12/29/16, 2:20 PM, "Colin King" wrote:
>From: Colin Ian King
>
>Rename the vendor_indentifer and hba_indentifer fields to correct spelling.
>
>Signed-off-by: Colin Ian King
>---
> drivers/scsi/qla2xxx/qla_def.h | 4 ++--
> drivers/scsi/qla2xxx/qla_gs.c | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>index f7df01b..ef699b6 100644
>--- a/drivers/scsi/qla2xxx/qla_def.h
>+++ b/drivers/scsi/qla2xxx/qla_def.h
>@@ -2247,7 +2247,7 @@ struct ct_fdmiv2_hba_attr {
> uint32_t num_ports;
> uint8_t fabric_name[WWN_SIZE];
> uint8_t bios_name[32];
>- uint8_t vendor_indentifer[8];
>+ uint8_t vendor_identifier[8];
> } a;
> };
>
>@@ -2422,7 +2422,7 @@ struct ct_sns_req {
> } rsnn_nn;
>
> struct {
>- uint8_t hba_indentifier[8];
>+ uint8_t hba_identifier[8];
> } ghat;
>
> struct {
>diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
>index 94e8a85..ee3df87 100644
>--- a/drivers/scsi/qla2xxx/qla_gs.c
>+++ b/drivers/scsi/qla2xxx/qla_gs.c
>@@ -1939,15 +1939,15 @@ qla2x00_fdmiv2_rhba(scsi_qla_host_t *vha)
> /* Vendor Identifier */
> eiter = entries + size;
> eiter->type = cpu_to_be16(FDMI_HBA_TYPE_VENDOR_IDENTIFIER);
>- snprintf(eiter->a.vendor_indentifer, sizeof(eiter->a.vendor_indentifer),
>+ snprintf(eiter->a.vendor_identifier, sizeof(eiter->a.vendor_identifier),
> "%s", "QLGC");
>- alen = strlen(eiter->a.vendor_indentifer);
>+ alen = strlen(eiter->a.vendor_identifier);
> alen += 4 - (alen & 3);
> eiter->len = cpu_to_be16(4 + alen);
> size += 4 + alen;
>
> ql_dbg(ql_dbg_disc, vha, 0x20b1,
>- "Vendor Identifier = %s.\n", eiter->a.vendor_indentifer);
>+ "Vendor Identifier = %s.\n", eiter->a.vendor_identifier);
>
> /* Update MS request size. */
> qla2x00_update_ms_fdmi_iocb(vha, size + 16);
>--
>2.10.2
Looks good
Acked-by: Himanshu Madhani
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 04/11] qla2xxx: Cleanup SRR code.
Hi Christoph,
On 1/9/17, 5:13 AM, "Christoph Hellwig" wrote:
>s/Cleanup/Remove/ ?
>
>Maybe add a little blurb on why we even had this huge pile of dead code
>despite not using selective retransmissions, and why you don't plan to
>use it any time soon.
>
>Otherwise looks fine:
>
>Reviewed-by: Christoph Hellwig
Thanks for review. During initial implementation, tape support was included
but not enabled by default on target. So far, we don’t see any target customers
Requesting this support. Since this code is not being used actively we want
to remove it and we will add back if there are any request in future for SRR
Support
Thanks,
Himanshu
>
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 1/2] qla2xxx: fix MSI-X vector affinity
On 1/11/17, 8:55 AM, "Christoph Hellwig" wrote:
>The first two or three vectors in qla2xxx adapter are global and
>not associated with a specific queue. They should not have IRQ
>affinity assigned.
>
>Signed-off-by: Christoph Hellwig
>---
> drivers/scsi/qla2xxx/qla_def.h | 2 +-
> drivers/scsi/qla2xxx/qla_isr.c | 15 +++
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>index ef699b6..e5e0301 100644
>--- a/drivers/scsi/qla2xxx/qla_def.h
>+++ b/drivers/scsi/qla2xxx/qla_def.h
>@@ -2732,7 +2732,7 @@ struct isp_operations {
> #define QLA_MSIX_FW_MODE(m) (((m) & (BIT_7|BIT_8|BIT_9)) >> 7)
> #define QLA_MSIX_FW_MODE_1(m) (QLA_MSIX_FW_MODE(m) == 1)
>
>-#define QLA_MSIX_DEFAULT 0x00
>+#define QLA_BASE_VECTORS 2 /* default + RSP */
> #define QLA_MSIX_RSP_Q0x01
> #define QLA_ATIO_VECTOR 0x02
> #define QLA_MSIX_QPAIR_MULTIQ_RSP_Q 0x03
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>index b9c113e..5f389a3 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -3018,13 +3018,20 @@ static const struct qla_init_msix_entry
>qla82xx_msix_entries[] = {
> static int
> qla24xx_enable_msix(struct qla_hw_data *ha, struct rsp_que *rsp)
> {
>-#define MIN_MSIX_COUNT2
> int i, ret;
> struct qla_msix_entry *qentry;
> scsi_qla_host_t *vha = pci_get_drvdata(ha->pdev);
>+ struct irq_affinity desc = {
>+ .pre_vectors = QLA_BASE_VECTORS,
>+ };
>+
>+ if (QLA_TGT_MODE_ENABLED() && IS_ATIO_MSIX_CAPABLE(ha))
>+ desc.pre_vectors++;
>+
>+ ret = pci_alloc_irq_vectors_affinity(ha->pdev, QLA_BASE_VECTORS,
>+ ha->msix_count, PCI_IRQ_MSIX | PCI_IRQ_AFFINITY,
>+ &desc);
>
>- ret = pci_alloc_irq_vectors(ha->pdev, MIN_MSIX_COUNT, ha->msix_count,
>- PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
> if (ret < 0) {
> ql_log(ql_log_fatal, vha, 0x00c7,
> "MSI-X: Failed to enable support, "
>@@ -3075,7 +3082,7 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct
>rsp_que *rsp)
> }
>
> /* Enable MSI-X vectors for the base queue */
>- for (i = 0; i < (QLA_MSIX_RSP_Q + 1); i++) {
>+ for (i = 0; i < QLA_BASE_VECTORS; i++) {
> qentry = &ha->msix_entries[i];
> qentry->handle = rsp;
> rsp->msix = qentry;
>--
>2.1.4
>
Thanks for cleanup. Looks Good.
Acked-by: Himanshu Madhani
>
Re: [PATCH 2/2] qla2xxx: remove irq_affinity_notifier
On 1/11/17, 8:55 AM, "Christoph Hellwig" wrote:
>Now that qla2xxx uses the IRQ layer affinity assignment affinity
>won't change over the life time of a device and the notifiers are
>useless.
>
>Signed-off-by: Christoph Hellwig
>---
> drivers/scsi/qla2xxx/qla_def.h | 1 -
> drivers/scsi/qla2xxx/qla_isr.c | 73 --
> 2 files changed, 74 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>index e5e0301..8c2e60a 100644
>--- a/drivers/scsi/qla2xxx/qla_def.h
>+++ b/drivers/scsi/qla2xxx/qla_def.h
>@@ -2754,7 +2754,6 @@ struct qla_msix_entry {
> uint16_t entry;
> char name[30];
> void *handle;
>- struct irq_affinity_notify irq_notify;
> int cpuid;
> };
>
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>index 5f389a3..ca0b7dc 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -19,10 +19,6 @@ static void qla2x00_status_entry(scsi_qla_host_t *, struct
>rsp_que *, void *);
> static void qla2x00_status_cont_entry(struct rsp_que *, sts_cont_entry_t *);
> static void qla2x00_error_entry(scsi_qla_host_t *, struct rsp_que *,
> sts_entry_t *);
>-static void qla_irq_affinity_notify(struct irq_affinity_notify *,
>-const cpumask_t *);
>-static void qla_irq_affinity_release(struct kref *);
>-
>
> /**
> * qla2100_intr_handler() - Process interrupts for the ISP2100 and ISP2200.
>@@ -2572,14 +2568,6 @@ void qla24xx_process_response_queue(struct
>scsi_qla_host *vha,
> if (!vha->flags.online)
> return;
>
>- if (rsp->msix && rsp->msix->cpuid != smp_processor_id()) {
>- /* if kernel does not notify qla of IRQ's CPU change,
>- * then set it here.
>- */
>- rsp->msix->cpuid = smp_processor_id();
>- ha->tgt.rspq_vector_cpuid = rsp->msix->cpuid;
>- }
>-
> while (rsp->ring_ptr->signature != RESPONSE_PROCESSED) {
> pkt = (struct sts_entry_24xx *)rsp->ring_ptr;
>
>@@ -3076,9 +3064,6 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct
>rsp_que *rsp)
> qentry->have_irq = 0;
> qentry->in_use = 0;
> qentry->handle = NULL;
>- qentry->irq_notify.notify = qla_irq_affinity_notify;
>- qentry->irq_notify.release = qla_irq_affinity_release;
>- qentry->cpuid = -1;
> }
>
> /* Enable MSI-X vectors for the base queue */
>@@ -3100,18 +3085,6 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct
>rsp_que *rsp)
> goto msix_register_fail;
> qentry->have_irq = 1;
> qentry->in_use = 1;
>-
>- /* Register for CPU affinity notification. */
>- irq_set_affinity_notifier(qentry->vector, &qentry->irq_notify);
>-
>- /* Schedule work (ie. trigger a notification) to read cpu
>- * mask for this specific irq.
>- * kref_get is required because
>- * irq_affinity_notify() will do
>- * kref_put().
>- */
>- kref_get(&qentry->irq_notify.kref);
>- schedule_work(&qentry->irq_notify.work);
> }
>
> /*
>@@ -3308,49 +3281,3 @@ int qla25xx_request_irq(struct qla_hw_data *ha, struct
>qla_qpair *qpair,
> msix->handle = qpair;
> return ret;
> }
>-
>-
>-/* irq_set_affinity/irqbalance will trigger notification of cpu mask update */
>-static void qla_irq_affinity_notify(struct irq_affinity_notify *notify,
>- const cpumask_t *mask)
>-{
>- struct qla_msix_entry *e =
>- container_of(notify, struct qla_msix_entry, irq_notify);
>- struct qla_hw_data *ha;
>- struct scsi_qla_host *base_vha;
>- struct rsp_que *rsp = e->handle;
>-
>- /* user is recommended to set mask to just 1 cpu */
>- e->cpuid = cpumask_first(mask);
>-
>- ha = rsp->hw;
>- base_vha = pci_get_drvdata(ha->pdev);
>-
>- ql_dbg(ql_dbg_init, base_vha, 0x,
>- "%s: host %ld : vector %d cpu %d \n", __func__,
>- base_vha->host_no, e->vector, e->cpuid);
>-
>- if (e->have_irq) {
>- if ((IS_QLA83XX(ha) || IS_QLA27XX(ha)) &&
>- (e->entry == QLA83XX_RSPQ_MSIX_ENTRY_NUMBER)) {
>- ha->tgt.rspq_vector_cpuid = e->cpuid;
>- ql_dbg(ql_dbg_init, base_vha, 0x,
>- "%s: host%ld: rspq vector %d cpu %d runtime
>change\n",
>- __func__, base_vha->host_no, e->vector, e->cpuid);
>- }
>- }
>-}
>-
>-static void qla_irq_affinity_release(struct kref *ref)
>-{
>- struct irq_affinity_notify *notify =
>- container_of(ref, struct irq_affinity_notify, kref);
>- struct qla_msix_entry *e =
>- container_of(notify, struct qla_msix_entry, irq_notify);
>- struct rsp_que *rsp = e->handle;
>- str
Re: [PATCH 02/11] qla2xxx: Cleanup TMF code translation from qla_target.
On 1/11/17, 12:05 PM, "Bart Van Assche" wrote:
>On 12/23/2016 08:23 PM, Himanshu Madhani wrote:
>> static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd *mcmd, uint32_t
>> lun,
>> -uint8_t tmr_func, uint32_t tag)
>> +uint16_t tmr_func, uint32_t tag)
>> {
>> struct qla_tgt_sess *sess = mcmd->sess;
>> struct se_cmd *se_cmd = &mcmd->se_cmd;
>> +int transl_tmr_func;
>> +
>> +switch (tmr_func) {
>> +case QLA_TGT_ABTS:
>> +pr_debug("%ld: ABTS received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_ABORT_TASK;
>> +break;
>> +case QLA_TGT_2G_ABORT_TASK:
>> +pr_debug("%ld: 2G Abort Task received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_ABORT_TASK;
>> +break;
>> +case QLA_TGT_CLEAR_ACA:
>> +pr_debug("%ld: CLEAR_ACA received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_CLEAR_ACA;
>> +break;
>> +case QLA_TGT_TARGET_RESET:
>> +pr_debug("%ld: TARGET_RESET received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_TARGET_WARM_RESET;
>> +break;
>> +case QLA_TGT_LUN_RESET:
>> +pr_debug("%ld: LUN_RESET received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_LUN_RESET;
>> +break;
>> +case QLA_TGT_CLEAR_TS:
>> +pr_debug("%ld: CLEAR_TS received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_CLEAR_TASK_SET;
>> +break;
>> +case QLA_TGT_ABORT_TS:
>> +pr_debug("%ld: ABORT_TS received\n", sess->vha->host_no);
>> +transl_tmr_func = TMR_ABORT_TASK_SET;
>> +break;
>> +default:
>> +pr_debug("%ld: Unknown task mgmt fn 0x%x\n",
>> +sess->vha->host_no, tmr_func);
>> +return -ENOSYS;
>> +break;
>> +}
>>
>> return target_submit_tmr(se_cmd, sess->se_sess, NULL, lun, mcmd,
>> tmr_func, GFP_ATOMIC, tag, TARGET_SCF_ACK_KREF);
>
>Hello Himanshu and Quinn,
>
>This patch introduces a new compiler warning when building with W=1,
>namely that transl_tmr_func is set but not used. Please review the code.
Sure. Will update patch.
>Thanks,
>
>Bart.
Re: [PATCH 03/11] qla2xxx: Make trace flags more readable.
On 1/11/17, 12:47 PM, "Bart Van Assche" wrote: >On Fri, 2016-12-23 at 20:23 -0800, Himanshu Madhani wrote: >> From: Quinn Tran >> >> This patch does not change any functionality. > >Please also mention in the patch description what the purpose of cmd_flags >is. If I remember correctly the only purpose of most flags is to make >analyzing crash dumps easier? Yes. You are correct. I will add blurb about need for having trc_flags in the patch commit message > >Bart.
Re: [PATCH 02/11] qla2xxx: Cleanup TMF code translation from qla_target.
On 1/11/17, 1:08 PM, "Bart Van Assche" wrote: >On 12/23/2016 08:23 PM, Himanshu Madhani wrote: >> From: Quinn Tran >> >> Move code code which converts Task Mgmt Command flags for >> ATIO to TCM #defines, from qla2xxx driver to tcm_qla2xxx >> driver. >> >> Reviewed-by: Christoph Hellwig >> Signed-off-by: Quinn Tran >> Signed-off-by: Himanshu Madhani >> --- >> drivers/scsi/qla2xxx/qla_target.c | 75 >> -- >> drivers/scsi/qla2xxx/qla_target.h | 6 ++- >> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 39 +++- >> 3 files changed, 49 insertions(+), 71 deletions(-) >> >> diff --git a/drivers/scsi/qla2xxx/qla_target.c >> b/drivers/scsi/qla2xxx/qla_target.c >> index 47acc26..6e58848 100644 >> --- a/drivers/scsi/qla2xxx/qla_target.c >> +++ b/drivers/scsi/qla2xxx/qla_target.c >> @@ -1592,8 +1592,9 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host >> *vha, >> mcmd->sess = sess; >> memcpy(&mcmd->orig_iocb.abts, abts, sizeof(mcmd->orig_iocb.abts)); >> mcmd->reset_count = vha->hw->chip_reset; >> +mcmd->tmr_func = QLA_TGT_ABTS; >> >> -rc = ha->tgt.tgt_ops->handle_tmr(mcmd, lun, TMR_ABORT_TASK, >> +rc = ha->tgt.tgt_ops->handle_tmr(mcmd, 0, mcmd->tmr_func, >> abts->exchange_addr_to_abort); > >Does this change remove the last user of the 'lun' variable from this >function? If so, please remove that variable entirely. We will need to preserve lun variable, since handle_tmr is also called from qlt_issue_task_mgmt and qlt_handle_task_mgmt. > >Thanks, > >Bart.
Re: [PATCH 08/11] qla2xxx: Add framework for Async fabric discovery.
On 1/11/17, 12:08 PM, "Bart Van Assche" wrote:
>On 12/23/2016 08:23 PM, Himanshu Madhani wrote:
>> +sess->logout_completed = 0;
>> +be_sid[0] = sess->d_id.b.domain;
>> +be_sid[1] = sess->d_id.b.area;
>> +be_sid[2] = sess->d_id.b.al_pa;
>
>Hello Himanshu and Quinn,
>
>When building the qlt_create_sess() code with W=1 the compiler warns
>that values are assigned to the elements of the be_sid[] array but that
>these values are not used. Please review the code.
Thanks for review. will review code and update patch.
>
>Thanks,
>
>Bart.
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 09/11] qla2xxx: Add Dual mode support in the driver
On 1/11/17, 11:51 AM, "Bart Van Assche" wrote: >On 12/23/2016 08:23 PM, Himanshu Madhani wrote: >> -#define QLA_TGT_MODE_ENABLED() (ql2x_ini_mode != QLA2XXX_INI_MODE_ENABLED) >> +#define QLA_TGT_MODE_ENABLED() \ >> +((ql2x_ini_mode != QLA2XXX_INI_MODE_ENABLED) || \ >> + (ql2x_ini_mode == QLA2XXX_INI_MODE_DUAL)) > >Hello Himanshu and Quinn, > >Is this change needed? It looks redundant to me. For all possible >ql2x_ini_mode values both the old and the new expression will yield the >same value. Agree. This is redundant. I’ll remove it. > >Anyway, dual mode is a welcome improvement! > >Bart.
Re: [PATCH v2 00/12] qla2xxx: Feature updates for target.
Hi Bart, On 1/17/17, 8:56 AM, "Bart Van Assche" wrote: >On Mon, 2017-01-16 at 12:35 -0800, Himanshu Madhani wrote: >> Please consider this updated series for inclusion in target-pending. > >Hello Himanshu, > >What kernel have these patches been generated against? These patches >neither apply cleanly on top of kernel v4.10-rc1 nor on top of kernel >v4.10-rc4. These were tested on top of 4.10-rc3 + target Bug fixes series that is pending to be included. Here’s link for the target bug fixes series. http://www.spinics.net/lists/target-devel/msg13801.html > >Bart.
Re: [PATCH v2 08/12] qla2xxx: Add framework for Async fabric discovery.
On 1/17/17, 2:27 PM, "Bart Van Assche" wrote: >%phC Yes. It looks like typo in the message. Will update this patch with other sparse warnings fixes. Thanks, Himanshu >
Re: [PATCH 1/2] qla2xxx: Fix a recently introduced memory leak
On 1/23/17, 8:34 AM, "Bart Van Assche" wrote:
>qla2x00_probe_one() allocates IRQs before it initializes rsp_q_map
>so IRQs must be freed even if rsp_q_map allocation did not occur.
>This was detected by kmemleak.
>
>Fixes: 4fa183455988 ("scsi: qla2xxx: Utilize
>pci_alloc_irq_vectors/pci_free_irq_vectors calls")
>Signed-off-by: Bart Van Assche
>Cc: Michael Hernandez
>Cc: Himanshu Madhani
>Cc: Christoph Hellwig
>Cc:
>---
> drivers/scsi/qla2xxx/qla_isr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>index dc88a09f9043..a94b0b6bd030 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -3242,7 +3242,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
>* from a probe failure context.
>*/
> if (!ha->rsp_q_map || !ha->rsp_q_map[0])
>- return;
>+ goto free_irqs;
> rsp = ha->rsp_q_map[0];
>
> if (ha->flags.msix_enabled) {
>@@ -3262,6 +3262,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
> free_irq(pci_irq_vector(ha->pdev, 0), rsp);
> }
>
>+free_irqs:
> pci_free_irq_vectors(ha->pdev);
> }
>
>--
>2.11.0
Thanks Bart. Looks good.
Acked-By: Himanshu Madhani
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash
On 1/23/17, 8:34 AM, "Bart Van Assche" wrote:
>Avoid that issuing a LIP as follows:
>
> find /sys -name 'issue_lip'|while read f; do echo 1 > $f; done
>
>triggers the following:
>
>BUG: unable to handle kernel NULL pointer dereference at (null)
>Call Trace:
> qla2x00_abort_all_cmds+0xed/0x140 [qla2xxx]
> qla2x00_abort_isp_cleanup+0x1e3/0x280 [qla2xxx]
> qla2x00_abort_isp+0xef/0x690 [qla2xxx]
> qla2x00_do_dpc+0x36c/0x880 [qla2xxx]
> kthread+0x10c/0x140
>
>Fixes: 1535aa75a3d8 ("qla2xxx: fix invalid DMA access after command aborts in
>PCI device remove")
>Signed-off-by: Bart Van Assche
>Cc: Naresh Bannoth
>Cc: Mauricio Faria de Oliveira
>Cc: Himanshu Madhani
>Cc:
>---
> drivers/scsi/qla2xxx/qla_os.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 0a000ecf0881..ae9c5a7b239a 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -1600,6 +1600,7 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
> srb_t *sp;
> struct qla_hw_data *ha = vha->hw;
> struct req_que *req;
>+ struct scsi_cmnd *scmd;
>
> qlt_host_reset_handler(ha);
>
>@@ -1613,6 +1614,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
> for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
> sp = req->outstanding_cmds[cnt];
> if (sp) {
>+ scmd = GET_CMD_SP(sp);
>+
> /* Don't abort commands in adapter during EEH
>* recovery as it's not accessible/responding.
>*/
>@@ -1624,7 +1627,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
>*/
> sp_get(sp);
>
> spin_unlock_irqrestore(&ha->hardware_lock, flags);
>- qla2xxx_eh_abort(GET_CMD_SP(sp));
>+ if (scmd)
>+ qla2xxx_eh_abort(scmd);
> spin_lock_irqsave(&ha->hardware_lock,
> flags);
> }
> req->outstanding_cmds[cnt] = NULL;
>--
>2.11.0
>
Looks Good.
Acked-by: Himanshu Madhani
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH v4 00/12] qla2xxx: Feature updates for target.
On 1/24/17, 9:24 AM, "Bart Van Assche" wrote:
>On 01/19/2017 10:29 PM, Himanshu Madhani wrote:
>> Please consider this updated series for inclusion in target-pending.
>>
>> Changes from v3 --> v4
>>
>> o Fixed warnings reported by smatch tool.
>> o Fixed indentatation warnings reported by smatch tool.
>>
>> Changes from v2 --> v3
>>
>> o Cleaned up sparse warnings reported by you.
>>
>> Changes from v1 --> v2
>>
>> o Updated review comments and added Reviewed-by tags for necessary patches.
>> o We left the one particular review comment to add helper routine to
>> be addressed at later time in the follow up bug fixes pathes which we
>> will be sending in few weeks.
>> o Added patch to simplify SRB usage in driver.
>> o Cleaned up warnings reported by sparse option w=1.
>
>Hello Himanshu,
>
>One newly introduced sparse warning has not yet been addressed. The
>patch below addresses that warning. I would appreciate it if someone
>from the qla2xxx team could review this patch.
>
>Thanks,
>
>Bart.
>
>
>From: Bart Van Assche
>Subject: [PATCH] qla2xxx: Avoid using variable-length arrays
>
>This patch does not change any functionality but avoids that sparse
>complains about using variable-length arrays.
>
>Signed-off-by: Bart Van Assche
>Cc: Himanshu Madhani
>Cc: Quinn Tran
>---
> drivers/scsi/qla2xxx/qla_os.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 817c5940df76..ec7e36c33e26 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -4272,8 +4272,8 @@ struct scsi_qla_host *qla2x00_create_host(struct
>scsi_host_template *sht,
> spin_lock_init(&vha->cmd_list_lock);
> init_waitqueue_head(&vha->fcport_waitQ);
>
>- vha->gnl.size =
>- sizeof(struct get_name_list_extended[ha->max_loop_id+1]);
>+ vha->gnl.size = sizeof(struct get_name_list_extended) *
>+ (ha->max_loop_id + 1);
> vha->gnl.l = dma_alloc_coherent(&ha->pdev->dev,
> vha->gnl.size, &vha->gnl.ldma, GFP_KERNEL);
> if (!vha->gnl.l) {
>--
>2.11.0
>
>
Looks Good. Thanks for correcting this.
Acked-by: Himanshu Madhani
Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash
On 1/24/17, 6:59 AM, "Mauricio Faria de Oliveira"
wrote:
>Hi Bart,
>
>First of all, sorry for the new bug; I didn't realize the pointer could
>be NULL at this scenario.
>
>On 01/23/2017 02:34 PM, Bart Van Assche wrote:
>> @@ -1624,7 +1627,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
>> */
>> sp_get(sp);
>>
>> spin_unlock_irqrestore(&ha->hardware_lock, flags);
>> -qla2xxx_eh_abort(GET_CMD_SP(sp));
>> +if (scmd)
>> +qla2xxx_eh_abort(scmd);
>> spin_lock_irqsave(&ha->hardware_lock,
>> flags);
>> }
>
>Now, this chunk has a problem with reference counting (and unnecessary
>spin-locking), which we can avoid by simply moving up this NULL check.
>
>The call to sp_get() increments the sp->ref_count, but if you skip the
>call to qla2xxx_eh_abort() you don't get the decrement from the call to
>sp->done() at abort handling from ISR, e.g., qla24xx_abort_iocb_entry().
>[or if the command completed successfully between issue/complete abort,
>at the completion from ISR, e.g., qla2x00_process_completed_request().]
>
>The sp->done() call just below this chunk was supposed to drop the
>initial reference [set at qla2xxx_queuecommand()] at a time we did
>not call qla2xxx_eh_abort() yet... but now that we __may__ call it
>(and get that sp->done() call from the ISR abort handling), we need
>to only increment it if we're going to drop it.
>
>That should be resolved with this slight change to your patch
>(which also helps w/ the spin-locking). What do you/others think?
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 0a000ecf0881..a17cb63b3fd5 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -1600,6 +1600,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
> srb_t *sp;
> struct qla_hw_data *ha = vha->hw;
> struct req_que *req;
>+ struct scsi_cmnd *scmd;
>
> qlt_host_reset_handler(ha);
>
>@@ -1613,10 +1614,12 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data
>*ha)
> for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
> sp = req->outstanding_cmds[cnt];
> if (sp) {
>+ scmd = GET_CMD_SP(sp);
>+
> /* Don't abort commands in adapter
>during EEH
> * recovery as it's not
>accessible/responding.
> */
>- if (!ha->flags.eeh_busy) {
>+ if (scmd && !ha->flags.eeh_busy) {
> /* Get a reference to the sp
>and drop the lock.
> * The reference ensures this
>sp->done() call
> * - and not the call in
>qla2xxx_eh_abort() -
>@@ -1624,7 +1627,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
> */
> sp_get(sp);
>
>spin_unlock_irqrestore(&ha->hardware_lock, flags);
>- qla2xxx_eh_abort(GET_CMD_SP(sp));
>+ qla2xxx_eh_abort(scmd);
>
>spin_lock_irqsave(&ha->hardware_lock, flags);
> }
> req->outstanding_cmds[cnt] = NULL;
>
>
>Signed-off-by: Mauricio Faria de Oliveira
>
>
>--
>Mauricio Faria de Oliveira
>IBM Linux Technology Center
This is more appropriate fix. Looks good.
Re: [PATCH 00/15] qla2xxx: Bug Fixes and updates for target.
Hi Bart, On 2/3/17, 8:26 AM, "Bart Van Assche" wrote: >On Thu, 2017-02-02 at 11:42 -0800, Himanshu Madhani wrote: >> Please consider this series for inclusion in target-pending. > >Hello Himanshu, > >What tree have these patches been generated against? Not all patches can be >applied on top of the scsi-target-for-v4.11 branch. These patches were rebased on 4.10-rc6+ and qla2xxx patches accepted for 4.11 branch. I’ll rebase them on top of scsi-target-for-v4.11 brach and repost. > >Thanks, > >Bart.
Re: [PATCH v2 00/14] qla2xxx: Bug Fixes and updates for target.
Hi Bart, On 2/8/17, 7:02 AM, "Bart Van Assche" wrote: >On Fri, 2017-02-03 at 14:40 -0800, Himanshu Madhani wrote: >> Please consider this series for inclusion in target-pending. > >Hello Himanshu, > >I gave this patch series a try on a system equipped with two QLogic FC HBAs >where an FC cable connects both HBAs to each other. qlini_mode has been set >to "dual" such that both ports support initiator and target mode. Without this >patch series both ports log in to each other and lsscsi shows several LUNs. >With this patch series applied the two ports do not log in to each other >anymore and strange messages appear in the kernel log: > >[ cut here ] >WARNING: CPU: 0 PID: 5 at lib/kobject.c:244 kobject_add_internal+0x118/0x350 >kobject_add_internal failed for 3:0:0:0 (error: -2 parent: target3:0:0) >Modules linked in: target_core_pscsi target_core_iblock target_core_file >tcm_qla2xxx target_core_mod fcoe libfcoe libfc qla2xxx scsi_transport_fc >netconsole af_packet hid_generic usbhid hid sg virtio_balloon i2c_piix4 >acpi_cpufreq button ib_iser rdma_cm iw_cm ib_cm ib_core configfs iscsi_tcp >libiscsi_tcp libiscsi scsi_transport_iscsi autofs4 ext4 crc16 jbd2 mbcache >sr_mod cdrom ata_generic pata_acpi virtio_net virtio_blk virtio_gpu >drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm i2c_core >ata_piix xhci_pci libata xhci_hcd intel_agp usbcore intel_gtt virtio_pci >scsi_mod virtio_ring usb_common agpgart virtio >CPU: 0 PID: 5 Comm: kworker/u16:0 Not tainted 4.10.0-rc6-dbg+ #1 >Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 >Workqueue: scsi_wq_3 fc_scsi_scan_rport [scsi_transport_fc] >Call Trace: > dump_stack+0x85/0xc2 > __warn+0xcb/0xf0 > warn_slowpath_fmt+0x4f/0x60 > kobject_add_internal+0x118/0x350 > kobject_add+0x68/0xb0 > device_add+0xfa/0x600 > scsi_sysfs_add_sdev+0x8e/0x270 [scsi_mod] > scsi_probe_and_add_lun+0xca1/0xe30 [scsi_mod] > __scsi_scan_target+0xd3/0x5f0 [scsi_mod] > scsi_scan_target+0xf1/0x100 [scsi_mod] > fc_scsi_scan_rport+0xc0/0xd0 [scsi_transport_fc] > process_one_work+0x1f4/0x6e0 > worker_thread+0x4e/0x4a0 > kthread+0x10c/0x140 > ret_from_fork+0x31/0x40 >---[ end trace 86f40b52873f9529 ]--- >scsi 3:0:0:0: failed to add device: -2 I’ll try to reproduce this. We have not see this issue in our testing and post update. Thanks, Himanshu > >Bart. >Western Digital Corporation (and its subsidiaries) E-mail Confidentiality >Notice & Disclaimer: > >This e-mail and any files transmitted with it may contain confidential or >legally privileged information of WDC and/or its affiliates, and are intended >solely for the use of the individual or entity to which they are addressed. If >you are not the intended recipient, any disclosure, copying, distribution or >any action taken or omitted to be taken in reliance on it, is prohibited. If >you have received this e-mail in error, please notify the sender immediately >and delete the e-mail in its entirety from your system.
Re: [PATCH v2 03/14] qla2xxx: Allow vref count to timeout on vport delete.
On 2/8/17, 5:03 AM, "Christoph Hellwig" wrote:
>On Fri, Feb 03, 2017 at 02:40:44PM -0800, Himanshu Madhani wrote:
>> -spin_lock_irqsave(&ha->vport_slock, flags);
>> -while (atomic_read(&vha->vref_count)) {
>> -spin_unlock_irqrestore(&ha->vport_slock, flags);
>> -
>> +while (count-- && atomic_read(&vha->vref_count))
>> msleep(500);
>
>Please add a wait queue to skeep on while waiting for vref_count to
>drop to zero.
Thanks for the review. Will update patch and resend series.
Re: [PATCH v2 06/14] qla2xxx: Improve T10-DIF/PI handling in driver.
Hi Bart,
On 2/8/17, 10:57 AM, "Bart Van Assche" wrote:
>On Fri, 2017-02-03 at 14:40 -0800, Himanshu Madhani wrote:
>> +/* Response code and sense key */
>> +((uint32_t *)ctio->u.status1.sense_data)[0] =
>> +cpu_to_le32((0x70 << 24) | (sense_key << 8));
>> +/* Additional sense length */
>> +((uint32_t *)ctio->u.status1.sense_data)[1] = cpu_to_le32(0x0a);
>> +/* ASC and ASCQ */
>> +((uint32_t *)ctio->u.status1.sense_data)[3] =
>> +cpu_to_le32((asc << 24) | (ascq << 16));
>
>Please use put_unaligned_le32() instead of open-coding it.
>
>> struct qla_tgt_cmd {
>> @@ -885,11 +895,25 @@ struct qla_tgt_cmd {
>> struct list_head cmd_list;
>>
>> struct atio_from_isp atio;
>> -/* t10dif */
>> +
>> +/* T10-DIF */
>> +#define DIF_ERR_NONE 0
>> +#define DIF_ERR_GRD 1
>> +#define DIF_ERR_REF 2
>> +#define DIF_ERR_APP 3
>> +int8_t dif_err_code;
>> struct scatterlist *prot_sg;
>> uint32_t prot_sg_cnt;
>> -uint32_t blk_sz;
>> +uint32_t blk_sz, num_blks;
>> +uint8_t scsi_status, sense_key, asc, ascq;
>> +
>> struct crc_context *ctx;
>> +uint32_tprot_op;
>> +uint32_tprot_type;
>> +uint8_t *cdb;
>> +uint64_tlba;
>> +uint16_ta_guard, e_guard, a_app_tag, e_app_tag;
>> +uint32_ta_ref_tag, e_ref_tag;
>>
>> uint64_t jiffies_at_alloc;
>> uint64_t jiffies_at_free;
>
>There are already equivalents of prot_op, prot_type, cdb and lba in struct
>se_cmd. I think a few weeks ago Christoph had asked you not to duplicate
>se_cmd fields into struct qla_tgt_cmd?
Thanks for the review comments. Looks like that was a miss in rebasing with
latest patch.
Will fix up patch and resend.
>
>Bart.
>Western Digital Corporation (and its subsidiaries) E-mail Confidentiality
>Notice & Disclaimer:
>
>This e-mail and any files transmitted with it may contain confidential or
>legally privileged information of WDC and/or its affiliates, and are intended
>solely for the use of the individual or entity to which they are addressed. If
>you are not the intended recipient, any disclosure, copying, distribution or
>any action taken or omitted to be taken in reliance on it, is prohibited. If
>you have received this e-mail in error, please notify the sender immediately
>and delete the e-mail in its entirety from your system.
>
Re: [PATCH v2 06/14] qla2xxx: Improve T10-DIF/PI handling in driver.
Hi Nic,
On 2/7/17, 8:13 PM, "Nicholas A. Bellinger" wrote:
>On Fri, 2017-02-03 at 14:40 -0800, Himanshu Madhani wrote:
>> From: Quinn Tran
>>
>> Add routines to support T10 DIF tag.
>>
>> Signed-off-by: Quinn Tran
>> Signed-off-by: Anil Gurumurthy
>> Signed-off-by: Himanshu Madhani
>> ---
>> drivers/scsi/qla2xxx/qla_dbg.h | 1 +
>> drivers/scsi/qla2xxx/qla_def.h | 17 ++
>> drivers/scsi/qla2xxx/qla_target.c | 598
>> +
>> drivers/scsi/qla2xxx/qla_target.h | 37 ++-
>> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 84 +-
>> 5 files changed, 465 insertions(+), 272 deletions(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_dbg.h b/drivers/scsi/qla2xxx/qla_dbg.h
>> index e1fc4e6..c6bffe9 100644
>> --- a/drivers/scsi/qla2xxx/qla_dbg.h
>> +++ b/drivers/scsi/qla2xxx/qla_dbg.h
>> @@ -348,6 +348,7 @@ void __attribute__((format (printf, 4, 5)))
>> #define ql_dbg_tgt 0x4000 /* Target mode */
>> #define ql_dbg_tgt_mgt 0x2000 /* Target mode management */
>> #define ql_dbg_tgt_tmr 0x1000 /* Target mode task management */
>> +#define ql_dbg_tgt_dif 0x0800 /* Target mode dif */
>>
>> extern int qla27xx_dump_mpi_ram(struct qla_hw_data *, uint32_t, uint32_t *,
>> uint32_t, void **);
>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>> index 8bc..d6436fc 100644
>> --- a/drivers/scsi/qla2xxx/qla_def.h
>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>> @@ -2189,6 +2189,23 @@ struct qlt_plogi_ack_t {
>> void*fcport;
>> };
>>
>> +enum qla_tgt_prot_op {
>> +QLA_PROT_NORMAL = 0,
>> +QLA_PROT_DIN_INSERT,
>> +QLA_PROT_DOUT_INSERT,
>> +QLA_PROT_DIN_STRIP,
>> +QLA_PROT_DOUT_STRIP,
>> +QLA_PROT_DIN_PASS,
>> +QLA_PROT_DOUT_PASS,
>> +};
>> +
>> +enum qla_tgt_prot_type {
>> +QLA_TGT_PROT_TYPE0,
>> +QLA_TGT_PROT_TYPE1,
>> +QLA_TGT_PROT_TYPE2,
>> +QLA_TGT_PROT_TYPE3,
>> +};
>> +
>
>I don't get it, why are you duplicating target_prot_op and
>target_prot_type..?
We will fix it and resubmit this.
Thanks,
Himanshu
Re: [PATCH v2 02/14] qla2xxx: Allow relogin to proceed if remote login did not finish
On 2/8/17, 10:42 AM, "Bart Van Assche" wrote: >The above code occurs two times in this patch. We try to avoid duplicating >code in the Linux kernel, especially code that contains hardcoded constants. >Have you considered to change the name of plogi_nack_done_jiff into e.g. >plogi_done_deadline and to assign jiffies + HZ to that variable instead of >jiffies? Thanks for the review. Will update patch and resend series.
Re: [PATCH v2 04/14] qla2xxx: Use IOCB interface to submit non-critical MBX.
On 2/8/17, 10:48 AM, "Bart Van Assche" wrote:
>On Fri, 2017-02-03 at 14:40 -0800, Himanshu Madhani wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
>> index db6bd92..4225256 100644
>> --- a/drivers/scsi/qla2xxx/qla_mbx.c
>> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
>> @@ -10,6 +10,29 @@
>> #include
>> #include
>>
>> +static struct mb_cmd_name {
>> +uint16_t cmd;
>> +char *str;
>> +} mb_str[] = {
>> +{0x,"unknown"},
>> +{MBC_GET_PORT_DATABASE, "GPDB"},
>> +{MBC_GET_ID_LIST, "GIDList"},
>> +{MBC_GET_LINK_PRIV_STATS, "Stats"},
>> +};
>> +
>> +static char *mb_to_str(uint16_t cmd)
>> +{
>> +int i;
>> +struct mb_cmd_name *e;
>> +
>> +for (i = 0; i < ARRAY_SIZE(mb_str); i++) {
>> +e = mb_str + i;
>> +if (cmd == e->cmd)
>> +return e->str;
>> +}
>> +return mb_str[0].str; /* unknown */
>> +}
>
>Please use const char * instead of char * in the struct definition and for
>the mb_to_str() function return type. Please also leave out the element with
>index 0x from the mb_str[] array and make mb_to_str() return "unknown"
>instead of mb_str[0].str if lookup fails.
Will update patch and resend.
>
>Bart.
>Western Digital Corporation (and its subsidiaries) E-mail Confidentiality
>Notice & Disclaimer:
>
>This e-mail and any files transmitted with it may contain confidential or
>legally privileged information of WDC and/or its affiliates, and are intended
>solely for the use of the individual or entity to which they are addressed. If
>you are not the intended recipient, any disclosure, copying, distribution or
>any action taken or omitted to be taken in reliance on it, is prohibited. If
>you have received this e-mail in error, please notify the sender immediately
>and delete the e-mail in its entirety from your system.
>
Re: [PATCH 0/3] qla2xxx: Bug fixes and cleanup for the driver.
Hi Martin,
On 2/20/17, 7:16 PM, "Martin K. Petersen" wrote:
> "Himanshu" == Himanshu Madhani writes:
Hi Himanshu,
Himanshu> This series contains small cleanup + fix for regression that
Himanshu> was introduced by pci_alloc_irq_vectors_affinity() call in
Himanshu> driver.
Himanshu> Please apply this series to 4.10/scsi-fixes at your earliest
Himanshu> convenience.
4.10 is out and this series does not apply to 4.11/scsi-queue. Please
rebase.
Also, please make sure your "Fixes: d745952 ("scsi: qla2xxx:..." have a
12-char hash. And add a stable tag if you want these in 4.10.x.
Thanks!
--
Martin K. Petersen Oracle Linux Engineering
Sure. Will rebase series on top of 4.11/scsi-queue and send it across.
Thanks,
Himanshu
RE: [PATCH] qla2xxx: fix spelling mistake: "seperator" -> "separator"
> -Original Message-
> From: Colin King [mailto:colin.k...@canonical.com]
> Sent: Thursday, February 23, 2017 2:57 AM
> To: qla2xxx-upstr...@qlogic.com; James E . J . Bottomley
> ; Martin K . Petersen
> ; linux-scsi@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Subject: [PATCH] qla2xxx: fix spelling mistake: "seperator" -> "separator"
>
> From: Colin Ian King
>
> trivial fix to spelling mistake in pr_err message
>
> Signed-off-by: Colin Ian King
> ---
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index c2f8c35..8e8ab0f 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -1792,7 +1792,7 @@ static struct se_wwn
> *tcm_qla2xxx_npiv_make_lport(
>
> p = strchr(tmp, '@');
> if (!p) {
> - pr_err("Unable to locate NPIV '@' seperator\n");
> + pr_err("Unable to locate NPIV '@' separator\n");
> return ERR_PTR(-EINVAL);
> }
> *p++ = '\0';
> --
> 2.10.2
Looks Good
Acked-by: Himanshu Madhani
RE: [PATCH v3 00/14] qla2xxx: Bug Fixes and updates for target.
Hi Nic/Bart, Can we get some review comments for this series. Thanks, Himanshu > -Original Message- > From: Himanshu Madhani [mailto:himanshu.madh...@cavium.com] > Sent: Friday, February 24, 2017 1:37 PM > To: target-de...@vger.kernel.org; bart.vanass...@sandisk.com; > n...@linux-iscsi.org > Cc: Malavali, Giridhar ; linux- > s...@vger.kernel.org; Madhani, Himanshu > > Subject: [PATCH v3 00/14] qla2xxx: Bug Fixes and updates for target. > > Hi Nic, > > Please consider this series for inclusion in target-pending. > > This series contains following changes. > > o Fix for the deadlock because of inconsistent lock usage reported by you. > o Added patch to submit non-critical MBX command via IOCB path. > o Improved T10-DIF/PI handling with target stack. > o Changed scsi host lookup method. > o Some minor bug fixes. > > Changes from v2 --> v3 > > o Updated patch to use waitq instead of while loop for waiting. > o Removed duplication of Target stack definitations for T10-DIF/PI. > o In addition, addressed review comments from Bart & Christoph. > > Changes from v1 -> v2 > > o Rebased series based on scsi-target-for-v4.11 branch. > > Please apply to target-pending. > > Thanks, > Himanshu > > Anil Gurumurthy (1): > qla2xxx: Export DIF stats via debugfs > > Himanshu Madhani (2): > qla2xxx: Add DebugFS node to display Port Database > qla2xxx: Update driver version to 9.00.00.00-k > > Joe Carnuccio (1): > qla2xxx: Allow vref count to timeout on vport delete. > > Quinn Tran (10): > qla2xxx: Fix delayed response to command for loop mode/direct connect. > qla2xxx: Allow relogin to proceed if remote login did not finish > qla2xxx: Use IOCB interface to submit non-critical MBX. > qla2xxx: Improve T10-DIF/PI handling in driver. > qla2xxx: Change scsi host lookup method. > qla2xxx: Fix memory leak for abts processing > qla2xxx: Fix request queue corruption. > qla2xxx: Fix inadequate lock protection for ABTS. > qla2xxx: Add async new target notification > qla2xxx: Fix sess_lock & hardware_lock lock order problem. > > drivers/scsi/qla2xxx/Kconfig | 1 + > drivers/scsi/qla2xxx/qla_attr.c| 4 +- > drivers/scsi/qla2xxx/qla_dbg.h | 1 + > drivers/scsi/qla2xxx/qla_def.h | 46 ++- > drivers/scsi/qla2xxx/qla_dfs.c | 115 +- > drivers/scsi/qla2xxx/qla_gbl.h | 12 +- > drivers/scsi/qla2xxx/qla_init.c| 85 ++--- > drivers/scsi/qla2xxx/qla_isr.c | 41 ++- > drivers/scsi/qla2xxx/qla_mbx.c | 304 ++-- > drivers/scsi/qla2xxx/qla_mid.c | 14 +- > drivers/scsi/qla2xxx/qla_os.c | 24 +- > drivers/scsi/qla2xxx/qla_target.c | 729 +++ > -- > drivers/scsi/qla2xxx/qla_target.h | 36 +- > drivers/scsi/qla2xxx/qla_version.h | 6 +- > drivers/scsi/qla2xxx/tcm_qla2xxx.c | 49 ++- > 15 files changed, 1049 insertions(+), 418 deletions(-) > > -- > 1.8.3.1
RE: [PATCH v3 00/14] qla2xxx: Bug Fixes and updates for target.
> -Original Message- > From: Bart Van Assche [mailto:bart.vanass...@sandisk.com] > Sent: Tuesday, February 28, 2017 2:10 PM > To: Madhani, Himanshu ; target- > de...@vger.kernel.org; n...@linux-iscsi.org > Cc: linux-scsi@vger.kernel.org; Malavali, Giridhar > > Subject: Re: [PATCH v3 00/14] qla2xxx: Bug Fixes and updates for target. > > On Tue, 2017-02-28 at 22:00 +, Madhani, Himanshu wrote: > > Can we get some review comments for this series. > > Hello Himanshu, > > The merge window opened one week ago and is still open. Anyone who > contributes to the Linux kernel should know that patches should not be > posted during the merge window because it's a busy time for most kernel > contributors. Additionally, no feedback should be expected during the merge > window for patches that are not bug fixes for changes that went in during > the merge window. Anyway, I'll have a look at these patches as soon as I > have the time. > > Bart. Sorry for the ignorance. I did not realize merge window was open. Thanks Himanshu
RE: [PATCH] qla2xxx: Fix ql_dump_buffer
> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Thursday, March 2, 2017 5:15 PM
> To: qla2xxx-upstr...@qlogic.com
> Cc: James E.J. Bottomley ; Martin K. Petersen
> ; linux-scsi@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] qla2xxx: Fix ql_dump_buffer
>
> Recent printk changes for KERN_CONT cause this logging to be defectively
> emitted on multiple lines. Fix it.
>
> Also reduces object size a trivial amount.
>
> $ size drivers/scsi/qla2xxx/qla_dbg.o*
>text data bss dec hex filename
> 39125 0 0 3912598d5
> drivers/scsi/qla2xxx/qla_dbg.o.new
> 39164 0 0 3916498fc
> drivers/scsi/qla2xxx/qla_dbg.o.old
>
> Signed-off-by: Joe Perches
> ---
> drivers/scsi/qla2xxx/qla_dbg.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> index 21d9fb7fc887..51b4179469d1 100644
> --- a/drivers/scsi/qla2xxx/qla_dbg.c
> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
> @@ -2707,13 +2707,9 @@ ql_dump_buffer(uint32_t level, scsi_qla_host_t
> *vha, int32_t id,
> "%-+5d 0 1 2 3 4 5 6 7 8 9 A B C D E F\n", size);
> ql_dbg(level, vha, id,
> "- ---\n");
> - for (cnt = 0; cnt < size; cnt++, buf++) {
> - if (cnt % 16 == 0)
> - ql_dbg(level, vha, id, "%04x:", cnt & ~0xFU);
> - printk(" %02x", *buf);
> - if (cnt % 16 == 15)
> - printk("\n");
> + for (cnt = 0; cnt < size; cnt += 16) {
> + ql_dbg(level, vha, id, "%04x: ", cnt);
> + print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16,
> 1,
> +buf + cnt, min(16U, size - cnt), false);
> }
> - if (cnt % 16 != 0)
> - printk("\n");
> }
> --
> 2.10.0.rc2.1.g053435c
Looks Good.
Acked-by: Himanshu Madhani
RE: [PATCH v3 00/14] qla2xxx: Bug Fixes and updates for target.
> -Original Message- > From: Bart Van Assche [mailto:bart.vanass...@sandisk.com] > Sent: Sunday, March 5, 2017 4:43 PM > To: Madhani, Himanshu ; target- > de...@vger.kernel.org; n...@linux-iscsi.org > Cc: linux-scsi@vger.kernel.org; Malavali, Giridhar > > Subject: Re: [PATCH v3 00/14] qla2xxx: Bug Fixes and updates for target. > > On Fri, 2017-02-24 at 13:37 -0800, Himanshu Madhani wrote: > > Please consider this series for inclusion in target-pending. > > Hello Himanshu, > > I applied this patch series on top of kernel v4.11-rc1 and installed it on a > test > system. Unfortunately the regression I reported one month ago against > v2 of this patch series still occurs with v3 of this patch series: > > INFO: task kworker/2:1:71 blocked for more than 120 seconds. > Not tainted 4.11.0-rc1-dbg+ #1 > "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > kworker/2:1 D071 2 0x > Workqueue: events qlt_free_session_done [qla2xxx] Call Trace: > __schedule+0x1ef/0xa70 > schedule+0x40/0x90 > schedule_timeout+0x258/0x4a0 > wait_for_completion+0x108/0x170 > target_wait_for_sess_cmds+0x56/0x1b0 [target_core_mod] > tcm_qla2xxx_free_session+0x45/0x90 [tcm_qla2xxx] > qlt_free_session_done+0x44a/0x550 [qla2xxx] > process_one_work+0x20d/0x6c0 > worker_thread+0x4e/0x4a0 > kthread+0x10c/0x140 > ret_from_fork+0x31/0x40 > > Bart We'll take a look.
Re: [PATCH 01/43] qla2xxx: Fix stale memory access for name pointer
Hi Bart,
> On Dec 20, 2017, at 10:25 AM, Bart Van Assche wrote:
>
> On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote:
>> Name pointer for sp describing each command is assigned with stack
>> frame's memory. The stack frame could eventually be re-use, where
>> name pointer access can get get garbage. This patch designates
>> static memory for name pointer to fix this problem.
>
> Which stack memory accesses have been removed by this patch? Sorry but I
> haven't found any stack memory access changes in this patch. Additionally,
> I haven't found any changes in this patch that look useful to me. Are you
> aware that for statements like "str = "unknown"" the compiler allocates
> static memory for the string "unknown”?
>
Sure. The intention of patch was to cleanup and make sure there is memory
allocated
on the stack for name.
>> +struct sp_name {
>> +uint16_t cmd;
>> +const char *str;
>> +};
>> +
>
> [ ... ]
>
>> +struct sp_name sp_str[] = {
>> +{ SPCN_UNKNOWN, "unknown" },
>> +{ SPCN_GIDPN, "gidpn" },
>> +{ SPCN_GPSC, "gpsc" },
>> +{ SPCN_GPNID, "gpnid" },
>> +{ SPCN_GPNFT, "gpnft" },
>> +{ SPCN_GNNID, "gnnid" },
>> +{ SPCN_GFPNID, "gfpnid" },
>> +{ SPCN_GFFID, "gffid" },
>> +{ SPCN_LOGIN, "login" },
>> +{ SPCN_LOGOUT, "logout" },
>> +{ SPCN_ADISC, "adisc" },
>> +{ SPCN_GNLIST, "gnlist" },
>> +{ SPCN_GPDB, "gpdb" },
>> +{ SPCN_TMF, "tmf" },
>> +{ SPCN_ABORT, "abort" },
>> +{ SPCN_NACK, "nack" },
>> +{ SPCN_BSG_RPT, "bsg_els_rpt" },
>> +{ SPCN_BSG_HST, "bsg_els_hst" },
>> +{ SPCN_BSG_CT, "bsg_ct" },
>> +{ SPCN_BSG_FX_MGMT, "bsg_fx_mgmt" },
>> +{ SPCN_ELS_DCMD, "ELS_DCMD" },
>> +{ SPCN_FXDISC, "fxdisc" },
>> +{ SPCN_PRLI, "prli" },
>> +{ SPCN_NVME_LS, "nvme_ls" },
>> +{ SPCN_NVME_CMD, "nvme_cmd" },
>> +};
>
> If you want to keep the sp_str[] array after what I wrote above, please
> remove the sp_name structure and change sp_str[] into something like the
> following:
>
> static const char *sp_str[] = {
> [SPCN_UNKNOWN] = "unknown",
> ...
> };
>
I will drop this patch from the current submission.
>> --- a/drivers/scsi/qla2xxx/qla_mbx.c
>> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
>> @@ -14,6 +14,7 @@ static struct mb_cmd_name {
>> uint16_t cmd;
>> const char *str;
>> } mb_str[] = {
>> +{0, "unknown mb"},
>> {MBC_GET_PORT_DATABASE, "GPDB"},
>> {MBC_GET_ID_LIST, "GIDList"},
>> {MBC_GET_LINK_PRIV_STATS, "Stats"},
>> @@ -24,12 +25,12 @@ static const char *mb_to_str(uint16_t cmd)
>> int i;
>> struct mb_cmd_name *e;
>>
>> -for (i = 0; i < ARRAY_SIZE(mb_str); i++) {
>> +for (i = 1; i < ARRAY_SIZE(mb_str); i++) {
>> e = mb_str + i;
>> if (cmd == e->cmd)
>> return e->str;
>> }
>> -return "unknown";
>> +return mb_str[0].str;
>> }
>
> Sorry but the above change does not look useful to me in any way. Is this
> just code churn?
>
Sure. will drop this change
> Thanks,
>
> Bart.
Thanks,
- Himanshu
Re: [PATCH 02/43] qla2xxx: Fix NULL pointer access for fcport structure
> On Dec 20, 2017, at 10:26 AM, Bart Van Assche wrote: > > On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote: >> From: Quinn Tran >> >> when preocessing iocb in a timeout case, driver was trying to log messages > ^^^ > processing? > > Bart. Will fix in v2 of the series Thanks, - Himanshu
Re: [PATCH 03/43] qla2xxx: Use IOCB path to submit Control VP MBX command
> On Dec 20, 2017, at 10:29 AM, Bart Van Assche wrote:
>
> On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote:
>> @@ -536,7 +541,7 @@ struct sp_name {
>> #define SRB_NVME_CMD 19
>> #define SRB_NVME_LS 20
>> #define SRB_PRLI_CMD 21
>> -
>> +#define SRB_CTRL_VP 22
>> enum {
>
> Please keep the blank line between the #define block and the enum definition.
>
Sure
>> +/*
>> + * qla24xx_control_vp
>> + * Enable a virtual port for given host
>> + *
>> + * Input:
>> + * ha = adapter block pointer.
>> + * vhba = virtual adapter (unused)
>> + * index = index number for enabled VP
>> + *
>> + * Returns:
>> + * qla2xxx local function return status code.
>> + *
>> + * Context:
>> + * Kernel context.
>> + */
>> +int
>> +qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
>
> Have you considered to use the kernel-doc style for this function header? See
> also https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt.
>
> Thanks,
>
> Bart.
Will update to kernel-doc style for the header.
Thanks,
- Himanshu
Re: [PATCH 06/43] qla2xxx: Fix stale mem access for IRQ name
> On Dec 20, 2017, at 10:39 AM, Bart Van Assche wrote: > > On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote: >> From: Quinn Tran >> >> IRQ name pointer for INTx/MSI was pointing at stale stack frame. >> cat /proc/interrupts will trigger stale mem access. Fix it by >> creating dedicated space for IRQ name. > > What stale stack frame are you referring to? Just like in the first patch > of this series, I don't see any stack accesses being removed by this patch. > Is this patch useful? If not, can this patch be left out? > > Thanks, > > Bart. will drop this patch. Thanks, - Himanshu
Re: [PATCH 07/43] qla2xxx: Add ability to track IOCB resource for FW
> On Dec 20, 2017, at 10:51 AM, Bart Van Assche wrote:
>
> On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote:
>> +#define QLA_ENA_FW_RES_TRACKING(_ha) { \
>> +int i; \
>> +_ha->base_qpair->fw_res_tracking = 1; \
>> +for (i = 0; i < _ha->max_qpairs; i++) { \
>> +if (_ha->queue_pair_map[i]) \
>> +_ha->queue_pair_map[i]->fw_res_tracking = 1; \
>> +} \
>> +}
>> +
>> +#define QLA_DIS_FW_RES_TRACKING(_ha) { \
>> +int i; \
>> +_ha->base_qpair->fw_res_tracking = 0; \
>> +for (i = 0; i < _ha->max_qpairs; i++) { \
>> +if (_ha->queue_pair_map[i]) \
>> +_ha->queue_pair_map[i]->fw_res_tracking = 0; \
>> +} \
>> +}
>
> Has this patch been verified with checkpatch? Did checkpatch suggest to
> surround
> these macros with do / while (0)?
>
Checkpatch did not suggest to use do/while loop.
I did however saw indentation warning which i did not change since it looked
like false positive
WARNING: suspect code indent for conditional statements (16, 16)
#133: FILE: drivers/scsi/qla2xxx/qla_def.h:4460:
+ if (_ha->queue_pair_map[i]) \
+ _ha->queue_pair_map[i]->fw_res_tracking = 1; \
WARNING: suspect code indent for conditional statements (16, 16)
#142: FILE: drivers/scsi/qla2xxx/qla_def.h:4469:
+ if (_ha->queue_pair_map[i]) \
+ _ha->queue_pair_map[i]->fw_res_tracking = 0; \
>> int
>> qla2x00_dfs_setup(scsi_qla_host_t *vha)
>> @@ -504,6 +776,33 @@ qla2x00_dfs_setup(scsi_qla_host_t *vha)
>> "Unable to create debugFS naqp node.\n");
>> goto out;
>> }
>> +
>> +if (ql2xtrackfwres) {
>> +ha->tgt.dfs_ini_iocbs =
>> +debugfs_create_file("reserve_ini_iocbs",
>> +0400, ha->dfs_dir, vha, &dfs_ini_iocbs_ops);
>> +if (!ha->tgt.dfs_ini_iocbs) {
>> +ql_log(ql_log_warn, vha, 0xd011,
>> +"Unable to create debugFS reserve_ini_iocbs
>> node.\n");
>> +goto out;
>> +}
>
> The patch description shows how to control the new attributes through sysfs
> but this code adds new debugfs attributes. Sorry but that really confuses me.
>
>> +int ql2xtrackfwres;
>> +module_param(ql2xtrackfwres, int, 0444);
>> +MODULE_PARM_DESC(ql2xtrackfwres,
>> + "Track FW resource. 0(default): disabled");
>
> Why is this a module parameter instead of e.g. a sysfs attribute? Module
> parameters are annoying if the qla2xxx driver is not built as a module.
>
Looks like sysfs example is used instead of DebugFS in the commit message
>> @@ -6412,6 +6433,9 @@ qlt_enable_vha(struct scsi_qla_host *vha)
>> qla24xx_disable_vp(vha);
>> qla24xx_enable_vp(vha);
>> } else {
>> +if (ql2xtrackfwres && (IS_QLA83XX(ha) || IS_QLA27XX(ha)))
>> +QLA_ENA_FW_RES_TRACKING(ha);
>
> Why is there a dependency on the adapter type in the above code?
>
We do not want to support this feature on 8G and older adapters.
> Thanks,
>
> Bart.
Thanks,
- Himanshu
Re: [PATCH 14/43] qla2xxx: Add option for use reserve exch for ELS
Hi Bart, > On Dec 20, 2017, at 10:53 AM, Bart Van Assche wrote: > > On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote: >> +int qla2xuseresexchforels; >> +module_param(qla2xuseresexchforels, int, 0444); >> +MODULE_PARM_DESC(qla2xuseresexchforels, >> + "Reserve 1/2 of emergency exchanges for ELS.\n" >> + " 0 (default): disabled"); > > Same question here as for another kernel module parameter: does this have to > be a kernel module parameter or could this have been implemented as a sysfs > attribute? > > Thanks, > > Bart. This does not have to be module parameter. Let me work on changing this to sysfs and submit in next series with the correction Thanks, - Himanshu
Re: [PATCH 21/43] qla2xxx: Remove calling cancel_work_sync()
> On Dec 20, 2017, at 10:56 AM, Bart Van Assche wrote: > > On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote: >> @@ -1234,8 +1234,6 @@ void qlt_schedule_sess_for_deletion(struct fc_port >> *sess, >> ql_dbg(ql_dbg_tgt, sess->vha, 0xe001, >> "Scheduling sess %p for deletion\n", sess); >> >> -/* use cancel to push work element through before re-queue */ >> -cancel_work_sync(&sess->del_work); >> INIT_WORK(&sess->del_work, qla24xx_delete_sess_fn); >> queue_work(sess->vha->hw->wq, &sess->del_work); > > Why had this cancel_work_sync() call been introduced in > qlt_schedule_sess_for_deletion() and why do you think it is safe to remove > this call? Both questions should have been answered in the patch description. > > Thanks, > > Bart. I will drop this patch from current series and will update commit message with details as suggested and submit in next series. Thanks, - Himanshu
Re: [PATCH 22/43] qla2xxx: Add switch command to simplify fabric discovery
Hi Ewan,
> On Dec 20, 2017, at 12:09 PM, Ewan D. Milne wrote:
>
> On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote:
>> @@ -4915,12 +4941,32 @@ static int qlt_24xx_handle_els(struct scsi_qla_host
>> *vha,
>> }
>>
>> if (sess != NULL) {
>> +bool delete = false;
>> spin_lock_irqsave(&tgt->ha->tgt.sess_lock, flags);
>> switch (sess->fw_login_state) {
>> +case DSC_LS_PLOGI_PEND:
>> case DSC_LS_PLOGI_COMP:
>> case DSC_LS_PRLI_COMP:
>> break;
>> default:
>> +delete = true;
>
> Nitpick: git quiltimport complains about the whitespace error
> (space before tab) on the above line.
>
>
Thanks for review. Will fix in v2
Thanks,
- Himanshu
Re: [PATCH 2/9] scsi: qla2xxx: Use zeroing allocator rather than allocator/memset
> On Dec 30, 2017, at 7:28 AM, Himanshu Jha wrote:
>
> Use dma_zalloc_coherent and vzalloc instead of dma_alloc_coherent and
> vmalloc respectively, followed by memset 0.
>
> Generated-by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci
>
> Suggested-by: Luis R. Rodriguez
> Signed-off-by: Himanshu Jha
> ---
> drivers/scsi/qla2xxx/qla_attr.c| 5 ++---
> drivers/scsi/qla2xxx/qla_bsg.c | 9 +++--
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 5 +
> 3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 9ce28c4..f46b015 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -1843,14 +1843,13 @@ qla2x00_get_fc_host_stats(struct Scsi_Host *shost)
> if (qla2x00_reset_active(vha))
> goto done;
>
> - stats = dma_alloc_coherent(&ha->pdev->dev,
> - sizeof(*stats), &stats_dma, GFP_KERNEL);
> + stats = dma_zalloc_coherent(&ha->pdev->dev, sizeof(*stats),
> + &stats_dma, GFP_KERNEL);
> if (!stats) {
> ql_log(ql_log_warn, vha, 0x707d,
> "Failed to allocate memory for stats.\n");
> goto done;
> }
> - memset(stats, 0, sizeof(*stats));
>
> rval = QLA_FUNCTION_FAILED;
> if (IS_FWI2_CAPABLE(ha)) {
> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index e3ac707..e2d5d3c 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -1435,7 +1435,7 @@ qla2x00_optrom_setup(struct bsg_job *bsg_job,
> scsi_qla_host_t *vha,
> ha->optrom_state = QLA_SREADING;
> }
>
> - ha->optrom_buffer = vmalloc(ha->optrom_region_size);
> + ha->optrom_buffer = vzalloc(ha->optrom_region_size);
> if (!ha->optrom_buffer) {
> ql_log(ql_log_warn, vha, 0x7059,
> "Read: Unable to allocate memory for optrom retrieval "
> @@ -1445,7 +1445,6 @@ qla2x00_optrom_setup(struct bsg_job *bsg_job,
> scsi_qla_host_t *vha,
> return -ENOMEM;
> }
>
> - memset(ha->optrom_buffer, 0, ha->optrom_region_size);
> return 0;
> }
>
> @@ -2314,16 +2313,14 @@ qla2x00_get_priv_stats(struct bsg_job *bsg_job)
> if (!IS_FWI2_CAPABLE(ha))
> return -EPERM;
>
> - stats = dma_alloc_coherent(&ha->pdev->dev,
> - sizeof(*stats), &stats_dma, GFP_KERNEL);
> + stats = dma_zalloc_coherent(&ha->pdev->dev, sizeof(*stats),
> + &stats_dma, GFP_KERNEL);
> if (!stats) {
> ql_log(ql_log_warn, vha, 0x70e2,
> "Failed to allocate memory for stats.\n");
> return -ENOMEM;
> }
>
> - memset(stats, 0, sizeof(*stats));
> -
> rval = qla24xx_get_isp_stats(base_vha, stats, stats_dma, options);
>
> if (rval == QLA_SUCCESS) {
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 3f82ea1..aadfeaa 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -1635,16 +1635,13 @@ static int tcm_qla2xxx_init_lport(struct
> tcm_qla2xxx_lport *lport)
> return rc;
> }
>
> - lport->lport_loopid_map = vmalloc(sizeof(struct tcm_qla2xxx_fc_loopid) *
> - 65536);
> + lport->lport_loopid_map = vzalloc(sizeof(struct tcm_qla2xxx_fc_loopid)
> * 65536);
> if (!lport->lport_loopid_map) {
> pr_err("Unable to allocate lport->lport_loopid_map of %zu
> bytes\n",
> sizeof(struct tcm_qla2xxx_fc_loopid) * 65536);
> btree_destroy32(&lport->lport_fcport_map);
> return -ENOMEM;
> }
> - memset(lport->lport_loopid_map, 0, sizeof(struct tcm_qla2xxx_fc_loopid)
> -* 65536);
> pr_debug("qla2xxx: Allocated lport_loopid_map of %zu bytes\n",
> sizeof(struct tcm_qla2xxx_fc_loopid) * 65536);
> return 0;
> --
> 2.7.4
>
Looks good
Acked-by: Himanshu Madhani
Re: [PATCH v2 00/37] qla2xxx: driver updates
Hi Martin, > On Jan 3, 2018, at 9:59 PM, Martin K. Petersen > wrote: > > > Hi Himanshu! > >> This series contains number of improvments in handling of switch >> registration commands in the driver. Switch commands are now submitted >> via IOCB patch asynchronously instead of mailbox interface. > > Applied to 4.16/scsi-queue. However, ... > >> 17 files changed, 2891 insertions(+), 1131 deletions(-) > > Next time please don't submit a series this big. It is really, really > painful to review. Especially when some of the patches are huge. > > I would much rather have 3-4 sets of 8-10 patches scattered throughout > the development cycle. Or a slow, steady trickle of bug fixes with maybe > a few bigger "new feature" series of logically related patches posted > separately to facilitate review. > Understood. Will make sure to send out smaller series for future. > Thanks! > Martin > > -- > Martin K. PetersenOracle Linux Engineering Thanks, - Himanshu
Re: [linux-next][qla2xxx][85caa95]kernel BUG at lib/list_debug.c:31!
Hello Abdul, > On Jan 9, 2018, at 7:54 AM, Bart Van Assche wrote: > > On Tue, 2018-01-09 at 14:44 +0530, Abdul Haleem wrote: >> Greeting's, >> >> Linux next kernel panics on powerpc when module qla2xxx is load/unload. >> >> Machine Type: Power 8 PowerVM LPAR >> Kernel : 4.15.0-rc2-next-20171211 >> gcc : version 4.8.5 >> Test type: module load/unload few times >> >> Trace messages: >> --- >> qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA Driver: >> 10.00.00.03-k. >> qla2xxx [0106:a0:00.0]-001a: : MSI-X vector count: 32. >> qla2xxx [0106:a0:00.0]-001d: : Found an ISP2532 irq 505 iobase >> 0xaeb324e6. >> qla2xxx [0106:a0:00.0]-00c6:1: MSI-X: Failed to enable support with 32 >> vectors, using 16 vectors. >> qla2xxx [0106:a0:00.0]-00fb:1: QLogic QLE2562 - PCIe 2-port 8Gb FC Adapter. >> qla2xxx [0106:a0:00.0]-00fc:1: ISP2532: PCIe (5.0GT/s x8) @ 0106:a0:00.0 >> hdma- host#=1 fw=8.06.00 (90d5). >> qla2xxx [0106:a0:00.1]-001a: : MSI-X vector count: 32. >> qla2xxx [0106:a0:00.1]-001d: : Found an ISP2532 irq 506 iobase >> 0xa46f1774. >> qla2xxx [0106:a0:00.1]-00c6:2: MSI-X: Failed to enable support with 32 >> vectors, using 16 vectors. >> 2xxx >> qla2xxx [0106:a0:00.1]-00fb:2: QLogic QLE2562 - PCIe 2-port 8Gb FC Adapter. >> qla2xxx [0106:a0:00.1]-00fc:2: ISP2532: PCIe (5.0GT/s x8) @ 0106:a0:00.1 >> hdma- host#=2 fw=8.06.00 (90d5). >> 0:00.0]-500a:1: LOOP UP detected (8 Gbps). >> qla2xxx [0106:a0:00.1]-500a:2: LOOP UP detected (8 Gbps). >> list_add double add: new=8d33e594, prev=8d33e594, >> next=adef1df4. >> [ cut here ] >> kernel BUG at lib/list_debug.c:31! >> Oops: Exception in kernel mode, sig: 5 [#1] >> LE SMP NR_CPUS=2048 NUMA pSeries >> Dumping ftrace buffer: >> (ftrace buffer empty) >> Modules linked in: qla2xxx(E) tg3(E) ibmveth(E) xt_CHECKSUM(E) >> iptable_mangle(E) ipt_MASQUERADE(E) nf_nat_masquerade_ipv4(E) >> iptable_nat(E) nf_nat_ipv4(E) nf_nat(E) nf_conntrack_ipv4(E) >> nf_defrag_ipv4(E) xt_conntrack(E) nf_conntrack(E) ipt_REJECT(E) >> nf_reject_ipv4(E) tun(E) bridge(E) stp(E) llc(E) kvm_pr(E) kvm(E) >> sctp_diag(E) sctp(E) libcrc32c(E) tcp_diag(E) udp_diag(E) >> ebtable_filter(E) ebtables(E) dccp_diag(E) ip6table_filter(E) dccp(E) >> ip6_tables(E) iptable_filter(E) inet_diag(E) unix_diag(E) >> af_packet_diag(E) netlink_diag(E) xts(E) sg(E) vmx_crypto(E) >> pseries_rng(E) nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) grace(E) >> sunrpc(E) binfmt_misc(E) ip_tables(E) ext4(E) mbcache(E) jbd2(E) >> fscrypto(E) sd_mod(E) ibmvscsi(E) scsi_transport_srp(E) nvme_fc(E) >> nvme_fabrics(E) nvme_core(E) scsi_transport_fc(E) >> ptp(E) pps_core(E) dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E) >> [last unloaded: qla2xxx] >> CPU: 7 PID: 22230 Comm: qla2xxx_1_dpc Tainted: GE >> 4.15.0-rc2-next-20171211-autotest-autotest #1 >> NIP: c0511040 LR: c051103c CTR: 00655170 >> REGS: 9b7356fa TRAP: 0700 Tainted: GE >> (4.15.0-rc2-next-20171211-autotest-autotest) >> MSR: 80010282b033 CR: 2222 >> XER: 0009 >> CFAR: c0170594 SOFTE: 0 >> GPR00: c051103c c000fc293ac0 c10f1d00 0058 >> GPR04: c0028fcccdd0 c0028fce3798 8000374060b8 >> GPR08: c0d435ec 00028ef9 2717 >> GPR12: ce734980 c01215d8 c002886996c0 >> GPR16: 0020 c002813d83f8 0001 >> GPR20: 2000 2000 0002 c002813dc808 >> GPR24: 0003 0001 c0027f5a5c20 c002813dced0 >> GPR28: c0027f5a5d90 c0027f5a5d90 c0027f5a5c00 c002813dc7f8 >> NIP [c0511040] __list_add_valid+0x70/0xb0 >> LR [c051103c] __list_add_valid+0x6c/0xb0 >> Call Trace: >> [c000fc293ac0] [c051103c] __list_add_valid+0x6c/0xb0 (unreliable) >> [c000fc293b20] [d51f1a08] qla24xx_async_gnl+0x108/0x420 [qla2xxx] >> [c000fc293bc0] [d51e762c] qla2x00_do_work+0x18c/0x8c0 [qla2xxx] >> [c000fc293ce0] [d51e8180] qla2x00_relogin+0x420/0xff0 [qla2xxx] >> [c000fc293dc0] [c012172c] kthread+0x15c/0x1a0 >> [c000fc293e30] [c000b4e8] ret_from_kernel_thread+0x5c/0x74 >> Instruction dump: >> 41de0018 38210060 3861 e8010010 7c0803a6 4e800020 3c62ffae 7d445378 >> 38631748 7d254b78 4bc5f51d 6000 <0fe0> 3c62ffae 7cc43378 386316f8 >> ---[ end trace a41bc8bd434657f1 ]--- >> >> Kernel panic - not syncing: Fatal exception >> Dumping ftrace buffer: >> (ftrace buffer empty) >> Rebooting in 10 seconds.. >> >> This trace back to the below code path: >> >> # gdb -batch vmlinux -ex 'list *(0xc0511040)' >> 0xc0511040 is in __list_add_valid (lib/list_debug.c:29). >> 24 "list_add corruption. next->prev should be prev >> (%p),
Re: [linux-next][qla2xxx][85caa95]kernel BUG at lib/list_debug.c:31!
> On Jan 10, 2018, at 9:38 PM, Abdul Haleem wrote: > > On Tue, 2018-01-09 at 18:09 +, Madhani, Himanshu wrote: >> Hello Abdul, >> >>> On Jan 9, 2018, at 7:54 AM, Bart Van Assche wrote: >>> >>> On Tue, 2018-01-09 at 14:44 +0530, Abdul Haleem wrote: >>>> Greeting's, >>>> >>>> Linux next kernel panics on powerpc when module qla2xxx is load/unload. >>>> >>>> Machine Type: Power 8 PowerVM LPAR >>>> Kernel : 4.15.0-rc2-next-20171211 >>>> gcc : version 4.8.5 >>>> Test type: module load/unload few times >>>> >>>> Trace messages: >>>> --- >>>> qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA Driver: >>>> 10.00.00.03-k. >>>> qla2xxx [0106:a0:00.0]-001a: : MSI-X vector count: 32. >>>> qla2xxx [0106:a0:00.0]-001d: : Found an ISP2532 irq 505 iobase >>>> 0xaeb324e6. >>>> qla2xxx [0106:a0:00.0]-00c6:1: MSI-X: Failed to enable support with 32 >>>> vectors, using 16 vectors. >>>> qla2xxx [0106:a0:00.0]-00fb:1: QLogic QLE2562 - PCIe 2-port 8Gb FC Adapter. >>>> qla2xxx [0106:a0:00.0]-00fc:1: ISP2532: PCIe (5.0GT/s x8) @ 0106:a0:00.0 >>>> hdma- host#=1 fw=8.06.00 (90d5). >>>> qla2xxx [0106:a0:00.1]-001a: : MSI-X vector count: 32. >>>> qla2xxx [0106:a0:00.1]-001d: : Found an ISP2532 irq 506 iobase >>>> 0xa46f1774. >>>> qla2xxx [0106:a0:00.1]-00c6:2: MSI-X: Failed to enable support with 32 >>>> vectors, using 16 vectors. >>>> 2xxx >>>> qla2xxx [0106:a0:00.1]-00fb:2: QLogic QLE2562 - PCIe 2-port 8Gb FC Adapter. >>>> qla2xxx [0106:a0:00.1]-00fc:2: ISP2532: PCIe (5.0GT/s x8) @ 0106:a0:00.1 >>>> hdma- host#=2 fw=8.06.00 (90d5). >>>> 0:00.0]-500a:1: LOOP UP detected (8 Gbps). >>>> qla2xxx [0106:a0:00.1]-500a:2: LOOP UP detected (8 Gbps). >>>> list_add double add: new=8d33e594, prev=8d33e594, >>>> next=adef1df4. >>>> [ cut here ] >>>> kernel BUG at lib/list_debug.c:31! >>>> Oops: Exception in kernel mode, sig: 5 [#1] >>>> LE SMP NR_CPUS=2048 NUMA pSeries >>>> Dumping ftrace buffer: >>>> (ftrace buffer empty) >>>> Modules linked in: qla2xxx(E) tg3(E) ibmveth(E) xt_CHECKSUM(E) >>>> iptable_mangle(E) ipt_MASQUERADE(E) nf_nat_masquerade_ipv4(E) >>>> iptable_nat(E) nf_nat_ipv4(E) nf_nat(E) nf_conntrack_ipv4(E) >>>> nf_defrag_ipv4(E) xt_conntrack(E) nf_conntrack(E) ipt_REJECT(E) >>>> nf_reject_ipv4(E) tun(E) bridge(E) stp(E) llc(E) kvm_pr(E) kvm(E) >>>> sctp_diag(E) sctp(E) libcrc32c(E) tcp_diag(E) udp_diag(E) >>>> ebtable_filter(E) ebtables(E) dccp_diag(E) ip6table_filter(E) dccp(E) >>>> ip6_tables(E) iptable_filter(E) inet_diag(E) unix_diag(E) >>>> af_packet_diag(E) netlink_diag(E) xts(E) sg(E) vmx_crypto(E) >>>> pseries_rng(E) nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) grace(E) >>>> sunrpc(E) binfmt_misc(E) ip_tables(E) ext4(E) mbcache(E) jbd2(E) >>>> fscrypto(E) sd_mod(E) ibmvscsi(E) scsi_transport_srp(E) nvme_fc(E) >>>> nvme_fabrics(E) nvme_core(E) scsi_transport_fc(E) >>>> ptp(E) pps_core(E) dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E) >>>> [last unloaded: qla2xxx] >>>> CPU: 7 PID: 22230 Comm: qla2xxx_1_dpc Tainted: GE >>>> 4.15.0-rc2-next-20171211-autotest-autotest #1 >>>> NIP: c0511040 LR: c051103c CTR: 00655170 >>>> REGS: 9b7356fa TRAP: 0700 Tainted: GE >>>> (4.15.0-rc2-next-20171211-autotest-autotest) >>>> MSR: 80010282b033 CR: >>>> 2222 XER: 0009 >>>> CFAR: c0170594 SOFTE: 0 >>>> GPR00: c051103c c000fc293ac0 c10f1d00 0058 >>>> GPR04: c0028fcccdd0 c0028fce3798 8000374060b8 >>>> GPR08: c0d435ec 00028ef9 2717 >>>> GPR12: ce734980 c01215d8 c002886996c0 >>>> GPR16: 0020 c002813d83f8 0001 >>>> GPR20: 2000 2000 0002 c002813dc808 >>>> GPR24: 0003 0001 c0027f5a5c20 c002813dced0 >>>> GPR28: c0027f5a5d90 c0027f5a5d90 c0027f5a5c00 c002813dc7f8 >
Re: A qla2xxx commit cause Linux no response, has not fixed in lastest version 4.15-rc6
Hi Nic, Chang, > On Jan 12, 2018, at 9:28 PM, Nicholas A. Bellinger > wrote: > > Hi Chang & Co, > > (Adding list + Himanshu CC') > > On Sun, 2018-01-07 at 10:21 +, Changlimin wrote: >> Hi, >> It seems the qla2xxx commit cause Linux no response, has not fixed in >> lastest version 4.15-rc6. >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=726b85487067d7f5b23495bc33c484b8517c4074 >> >> > > Thanks for reporting + including debug log. :) > >> lspci: >> 0a:00.0 Fibre Channel: QLogic Corp. ISP2532-based 8Gb Fibre Channel to PCI >> Express HBA (rev 02) >> 0a:00.1 Fibre Channel: QLogic Corp. ISP2532-based 8Gb Fibre Channel to PCI >> Express HBA (rev 02) >> >> syslog: >> qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA Driver: 8.07.00.38-k. >> qla2xxx [:0a:00.0]-001a: : MSI-X vector count: 32. >> qla2xxx [:0a:00.0]-001d: : Found an ISP2532 irq 16 iobase >> 0xb0d5cc501000. >> qla2xxx [:0a:00.0]-00c6:1: MSI-X: Failed to enable support with 32 >> vectors, using 26 vectors. >> scsi host1: qla2xxx >> qla2xxx [:0a:00.0]-00fb:1: QLogic HPAJ764A - HP 8Gb Dual Channel PCI-e >> 2.0 FC HBA. >> qla2xxx [:0a:00.0]-00fc:1: ISP2532: PCIe (5.0GT/s x8) @ :0a:00.0 >> hdma+ host#=1 fw=8.03.00 (90d5). >> qla2xxx [:0a:00.1]-001a: : MSI-X vector count: 32. >> qla2xxx [:0a:00.1]-001d: : Found an ISP2532 irq 17 iobase >> 0xb0d5cc5d9000. >> qla2xxx [:0a:00.1]-00c6:2: MSI-X: Failed to enable support with 32 >> vectors, using 26 vectors. >> scsi host2: qla2xxx >> qla2xxx [:0a:00.1]-00fb:2: QLogic HPAJ764A - HP 8Gb Dual Channel PCI-e >> 2.0 FC HBA. >> qla2xxx [:0a:00.1]-00fc:2: ISP2532: PCIe (5.0GT/s x8) @ :0a:00.1 >> hdma+ host#=2 fw=8.03.00 (90d5). >> qla2xxx [:0a:00.0]-500a:1: LOOP UP detected (8 Gbps). >> qla2xxx [:0a:00.1]-500a:2: LOOP UP detected (8 Gbps). >> >> The attached file is the module log. >> >> Do you have any advice? > > Quinn & Himanshu folks, any comments..? > What is the issue here? I am not clear form the snippet above. One thing I noticed that, if you are using 4.15-rc6 driver version should be 10.00.00.02-k but the snippet shows 8.07.00.38-k which tells me you might Thanks, - Himanshu
Re: [PATCH] drivers/scsi/qla2xxx: fix double free bug after firmware timeout
Hi Max, > On Jan 15, 2018, at 9:26 AM, Max Kellermann wrote: > > When the qla2xxx firmware is unavailable, eventually > qla2x00_sp_timeout() is reached, which calls the timeout function and > frees the srb_t instance. > > The timeout function always resolves to qla2x00_async_iocb_timeout(), > which invokes another callback function called "done". All of these > qla2x00_*_sp_done() callbacks also free the srb_t instance; after > returning to qla2x00_sp_timeout(), it is freed again. > > The fix is to remove the "sp->free(sp)" call from qla2x00_sp_timeout() > and add it to those code paths in qla2x00_async_iocb_timeout() which > do not already free the object. > > This is how it looks like with KASAN: > > BUG: KASAN: use-after-free in qla2x00_sp_timeout+0x228/0x250 > Read of size 8 at addr 88278147a590 by task swapper/2/0 > > Allocated by task 1502: > save_stack+0x33/0xa0 > kasan_kmalloc+0xa0/0xd0 > kmem_cache_alloc+0xb8/0x1c0 > mempool_alloc+0xd6/0x260 > qla24xx_async_gnl+0x3c5/0x1100 > > Freed by task 0: > save_stack+0x33/0xa0 > kasan_slab_free+0x72/0xc0 > kmem_cache_free+0x75/0x200 > qla24xx_async_gnl_sp_done+0x556/0x9e0 > qla2x00_async_iocb_timeout+0x1c7/0x420 > qla2x00_sp_timeout+0x16d/0x250 > call_timer_fn+0x36/0x200 > > The buggy address belongs to the object at 88278147a440 > which belongs to the cache qla2xxx_srbs of size 344 > The buggy address is located 336 bytes inside of > 344-byte region [88278147a440, 88278147a598) > > Signed-off-by: Max Kellermann > --- > drivers/scsi/qla2xxx/qla_init.c |3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index b5b48ddca962..801890564e00 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -58,7 +58,6 @@ qla2x00_sp_timeout(unsigned long __data) > req->outstanding_cmds[sp->handle] = NULL; > iocb = &sp->u.iocb_cmd; > iocb->timeout(sp); > - sp->free(sp); > spin_unlock_irqrestore(&vha->hw->hardware_lock, flags); > } > > @@ -121,9 +120,11 @@ qla2x00_async_iocb_timeout(void *data) > ea.data[1] = lio->u.logio.data[1]; > ea.sp = sp; > qla24xx_handle_plogi_done_event(fcport->vha, &ea); > + sp->free(sp); > break; > case SRB_LOGOUT_CMD: > qlt_logo_completion_handler(fcport, QLA_FUNCTION_TIMEOUT); > + sp->free(sp); > break; > case SRB_CT_PTHRU_CMD: > case SRB_MB_IOCB: > We have patch to prevent this double free in 4.16/scsi-queue already. https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.16/scsi-queue&id=045d6ea200af794ba15515984cff63787a7fc3c0 Thanks, - Himanshu
Re: [PATCH] drivers/scsi/qla2xxx: fix double free bug after firmware timeout
Hi Max, > On Jan 15, 2018, at 12:37 PM, Max Kellermann wrote: > > On 2018/01/15 20:58, "Madhani, Himanshu" wrote: >> We have patch to prevent this double free in 4.16/scsi-queue >> already. > > No, let me repeat: this is a different bug! > > Your bug is about the free call after waiting for completion > synchronously in qla24xx_els_dcmd_iocb(), after it was already freed > by qla2x00_sp_timeout(). > > My bug is about free in qla2x00_*_sp_done() and again in > qla2x00_sp_timeout(). My patch description describes exactly that. > > And you know what? My patch fixes both bugs. It is superior to the > one that was merged 4 weeks later, isn't it? > > You NACKed my patch 5 weeks ago, and I explained to you that you were > talking about a different bug, but you never replied to that. > > Max Thanks for refreshing my memory. Sorry about not getting back to you earlier. I was out for couple weeks so it got dropped out of my bucket for response. I do agree there is double free issue here. I’ll discuss this internally with folks who has deeper history of this code and will get back to you and see which approach we want to take. Thanks, - Himanshu
Re: A qla2xxx commit cause Linux no response, has not fixed in lastest version 4.15-rc6
Hi Chang, > On Jan 15, 2018, at 4:27 PM, Changlimin wrote: > > Hi Himanshu, > The issue is: When insmod the qla2xxx.ko from 4.15-rc6, linux hang. >From the log file attached. I see that you are trying to load driver from >4.9.x in 4.15.0-rc6. [ 279.898704] qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA Driver: 8.07.00.38-k-debug. 4.15.0-rc6 had driver version 10.00.00.02-k. Would you check if you have all the driver changes pulled in with kernel 4.15.0-rc6. > I have git bisect the commits. > The issue was introduced in commit: 726b85487067d7f5b23495bc33c484b8517c4074 > qla2xxx: Add framework for async fabric discovery. > The previous commit is good: 5d964837c6a743193c63c8912f98834c7457ba5c > qla2xxx: Track I-T nexus as single fc_port struct . > > Regards > Chang Limin > > -Original Message- > From: Madhani, Himanshu [mailto:himanshu.madh...@cavium.com] > Sent: Tuesday, January 16, 2018 12:58 AM > To: Nicholas A. Bellinger > Cc: changlimin (Cloud); Tran, Quinn; jifuliang (Cloud); zhangguanghui > (Cloud); zhangzijian (Cloud); target-devel; linux-scsi > Subject: Re: A qla2xxx commit cause Linux no response, has not fixed in > lastest version 4.15-rc6 > > Hi Nic, Chang, > >> On Jan 12, 2018, at 9:28 PM, Nicholas A. Bellinger >> wrote: >> >> Hi Chang & Co, >> >> (Adding list + Himanshu CC') >> >> On Sun, 2018-01-07 at 10:21 +, Changlimin wrote: >>> Hi, >>> It seems the qla2xxx commit cause Linux no response, has not fixed in >>> lastest version 4.15-rc6. >>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.g >>> it/commit/?id=726b85487067d7f5b23495bc33c484b8517c4074 >>> >> >> Thanks for reporting + including debug log. :) >> >>> lspci: >>> 0a:00.0 Fibre Channel: QLogic Corp. ISP2532-based 8Gb Fibre Channel >>> to PCI Express HBA (rev 02) >>> 0a:00.1 Fibre Channel: QLogic Corp. ISP2532-based 8Gb Fibre Channel >>> to PCI Express HBA (rev 02) >>> >>> syslog: >>> qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA Driver: >>> 8.07.00.38-k. >>> qla2xxx [:0a:00.0]-001a: : MSI-X vector count: 32. >>> qla2xxx [:0a:00.0]-001d: : Found an ISP2532 irq 16 iobase >>> 0xb0d5cc501000. >>> qla2xxx [:0a:00.0]-00c6:1: MSI-X: Failed to enable support with 32 >>> vectors, using 26 vectors. >>> scsi host1: qla2xxx >>> qla2xxx [:0a:00.0]-00fb:1: QLogic HPAJ764A - HP 8Gb Dual Channel PCI-e >>> 2.0 FC HBA. >>> qla2xxx [:0a:00.0]-00fc:1: ISP2532: PCIe (5.0GT/s x8) @ :0a:00.0 >>> hdma+ host#=1 fw=8.03.00 (90d5). >>> qla2xxx [:0a:00.1]-001a: : MSI-X vector count: 32. >>> qla2xxx [:0a:00.1]-001d: : Found an ISP2532 irq 17 iobase >>> 0xb0d5cc5d9000. >>> qla2xxx [:0a:00.1]-00c6:2: MSI-X: Failed to enable support with 32 >>> vectors, using 26 vectors. >>> scsi host2: qla2xxx >>> qla2xxx [:0a:00.1]-00fb:2: QLogic HPAJ764A - HP 8Gb Dual Channel PCI-e >>> 2.0 FC HBA. >>> qla2xxx [:0a:00.1]-00fc:2: ISP2532: PCIe (5.0GT/s x8) @ :0a:00.1 >>> hdma+ host#=2 fw=8.03.00 (90d5). >>> qla2xxx [:0a:00.0]-500a:1: LOOP UP detected (8 Gbps). >>> qla2xxx [:0a:00.1]-500a:2: LOOP UP detected (8 Gbps). >>> >>> The attached file is the module log. >>> >>> Do you have any advice? >> >> Quinn & Himanshu folks, any comments..? >> > > What is the issue here? I am not clear form the snippet above. > > One thing I noticed that, if you are using 4.15-rc6 driver version should be > 10.00.00.02-k but the snippet shows 8.07.00.38-k which tells me you might > > Thanks, > - Himanshu > Thanks, - Himanshu
Re: A qla2xxx commit cause Linux no response, has not fixed in lastest version 4.15-rc6
Hi Chang, > On Jan 15, 2018, at 10:49 PM, Changlimin wrote: > > Hi Himanshu, > This is my progress. > First, I compiled 4.15-rc6, I found linux hang when booting, the stack > showed something wrong in qla2xxx driver. Can you provide me detail steps of how you compiled 4.15-rc6. Also provide me details of how you are loading driver and also provide complete log file. I do not see how you will be able to load driver which is from 4.9.x when you compile fresh 4.15.0-rc6. Just FYI, I build test system with 8G/16G/32G adapter with 4.15.0-rc6 kernel and I am not able to see hang that you are describing. # uname -r 4.15.0-rc6+ # modprobe qla2xxx # fcc.sh FC HBAs: HBA Port NamePort ID State Device host3 21:00:00:24:ff:7e:f5:80 01:0d:00 OnlineQLE2742 FW:v8.05.63 DVR:v10.00.00.04-k host4 21:00:00:24:ff:7e:f5:81 01:0e:00 OnlineQLE2742 FW:v8.05.63 DVR:v10.00.00.04-k host5 21:00:00:0e:1e:12:e9:a0 01:06:00 OnlineQLE8362 FW:v8.03.06 DVR:v10.00.00.04-k host6 21:00:00:0e:1e:12:e9:a1 01:14:00 OnlineQLE8362 FW:v8.03.06 DVR:v10.00.00.04-k host7 21:00:00:24:ff:46:0a:5c 01:0d:00 OnlineQLE2562 FW:v8.03.00 DVR:v10.00.00.04-k host8 21:00:00:24:ff:46:0a:5d 01:15:00 OnlineQLE2562 FW:v8.03.00 DVR:v10.00.00.04-k # modinfo qla2xxx | more filename: /lib/modules/4.15.0-rc6+/kernel/drivers/scsi/qla2xxx/qla2xxx.ko firmware: ql2500_fw.bin firmware: ql2400_fw.bin firmware: ql2322_fw.bin firmware: ql2300_fw.bin firmware: ql2200_fw.bin firmware: ql2100_fw.bin version:10.00.00.04-k license:GPL description:QLogic Fibre Channel HBA Driver author: QLogic Corporation srcversion: 6CBCF1372A7756690E83CC3 > Second, I want to find which commit introduced the issue. So I tried many > times via git bisect to linux kernel. > Finally, I found the commit 726b85487067d7f5b23495bc33c484b8517c4074 > introduced the issue. The attached log is related to this commit. > Also ubuntu kernel has this issue: > > https://launchpad.net/ubuntu/+archive/primary/+files/linux-image-4.13.0-25-generic_4.13.0-25.29_amd64.deb > > https://launchpad.net/ubuntu/+archive/primary/+files/linux-image-extra-4.13.0-25-generic_4.13.0-25.29_amd64.deb > > Regards > Chang Limin > > -Original Message- > From: Madhani, Himanshu [mailto:himanshu.madh...@cavium.com] > Sent: Tuesday, January 16, 2018 12:59 PM > To: changlimin (Cloud) > Cc: Nicholas A. Bellinger; Tran, Quinn; jifuliang (Cloud); zhangguanghui > (Cloud); zhangzijian (Cloud); target-devel; linux-scsi > Subject: Re: A qla2xxx commit cause Linux no response, has not fixed in > lastest version 4.15-rc6 > > Hi Chang, > >> On Jan 15, 2018, at 4:27 PM, Changlimin wrote: >> >> Hi Himanshu, >> The issue is: When insmod the qla2xxx.ko from 4.15-rc6, linux hang. > > From the log file attached. I see that you are trying to load driver from > 4.9.x in 4.15.0-rc6. > > [ 279.898704] qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA > Driver: 8.07.00.38-k-debug. > > 4.15.0-rc6 had driver version 10.00.00.02-k. Would you check if you have all > the driver changes pulled in with kernel 4.15.0-rc6. > >> I have git bisect the commits. >> The issue was introduced in commit: 726b85487067d7f5b23495bc33c484b8517c4074 >> qla2xxx: Add framework for async fabric discovery. >> The previous commit is good: 5d964837c6a743193c63c8912f98834c7457ba5c >> qla2xxx: Track I-T nexus as single fc_port struct . >> >> Regards >> Chang Limin >> >> -Original Message- >> From: Madhani, Himanshu [mailto:himanshu.madh...@cavium.com] >> Sent: Tuesday, January 16, 2018 12:58 AM >> To: Nicholas A. Bellinger >> Cc: changlimin (Cloud); Tran, Quinn; jifuliang (Cloud); zhangguanghui >> (Cloud); zhangzijian (Cloud); target-devel; linux-scsi >> Subject: Re: A qla2xxx commit cause Linux no response, has not fixed >> in lastest version 4.15-rc6 >> >> Hi Nic, Chang, >> >>> On Jan 12, 2018, at 9:28 PM, Nicholas A. Bellinger >>> wrote: >>> >>> Hi Chang & Co, >>> >>> (Adding list + Himanshu CC') >>> >>> On Sun, 2018-01-07 at 10:21 +, Changlimin wrote: >>>> Hi, >>>> It seems the qla2xxx commit cause Linux no response, has not fixed in >>>> lastest version 4.15-rc6. >>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable. >>>> g >>>> it/commit/?id=726b85487067d7f5b23495bc33c484b8517c4074 >>>> >>> >>> Thanks for reporting + including debug log. :) >>> >
Re: [PATCH] drivers/scsi/qla2xxx: fix double free bug after firmware timeout
Max, > On Jan 15, 2018, at 12:37 PM, Max Kellermann wrote: > > On 2018/01/15 20:58, "Madhani, Himanshu" wrote: >> We have patch to prevent this double free in 4.16/scsi-queue >> already. > > No, let me repeat: this is a different bug! > > Your bug is about the free call after waiting for completion > synchronously in qla24xx_els_dcmd_iocb(), after it was already freed > by qla2x00_sp_timeout(). > > My bug is about free in qla2x00_*_sp_done() and again in > qla2x00_sp_timeout(). My patch description describes exactly that. > > And you know what? My patch fixes both bugs. It is superior to the > one that was merged 4 weeks later, isn't it? > > You NACKed my patch 5 weeks ago, and I explained to you that you were > talking about a different bug, but you never replied to that. > > Max Just to update you, we are working on fixing this internally and will post patch soon. I’ll add you are Reporter and please review and let us know your comments. Thanks, - Himanshu
Re: [LSF/MM TOPIC] Improving Asynchronous SCSI Disk Probing
Hi Bart, > On Jan 17, 2018, at 3:24 PM, Bart Van Assche wrote: > > When the SCSI scanning code discovers a SCSI device it calls the driver core > function device_add() to associate a SCSI ULD with the device. The driver > core invokes the probing function for the matching SCSI ULP, e.g. sd_probe(). > In order to minimize the time needed to scan SCSI targets that have a large > number of LUNs, the SCSI disk driver scans LUNs asynchronously by starting > the actual probing work asynchronously from inside sd_probe() > > An unfortunate aspect of how SCSI disk probing works today is that there is > unnecessary serialization between probing and removal activity. For a > possible approach of how to increase SCSI disk probing concurrency, see also > [PATCH] sd: Increase SCSI disk probing concurrency, linux-scsi mailing list, > December 2017 (https://www.spinics.net/lists/linux-scsi/msg115657.html). > > A second unfortunate aspect of SCSI disk probing is that certain race > conditions in the block layer are hit if removal starts before asynchronous > probing has finished. This is because the driver core is unaware that the > SCSI disk code works asynchronously. > > Additionally, the SCSI disk asynchronous probing approach is incompatible > with the power management code. The power management code calls > wait_for_device_probe() in the driver core to wait for device probing > activity to finish. wait_for_device_probe() however is unaware of the > asynchronous probing in the SCSI sd driver and hence doesn't wait for the > SCSI sd probing activity to finish. > > My proposal is to hold a session to discus potential solutions for > increasing SCSI disk probing concurrency in a way that is compatible with > the driver core and the power management subsystem. We have run into issue with asyc probing and would be interested in the discussion. Thanks, - Himanshu
FC-NVMe fixes for qla2xxx
Hi Christoph, I had submitted series for FC-NVME fixes in qla2xxx driver on Jan 19, 2018. http://lists.infradead.org/pipermail/linux-nvme/2018-January/015073.html The series was cut against Martin’s 4.16/scsi-queue tree and I would like his series to be merged via Martin’s tree. Please review. Thanks, - Himanshu
Re: qla2xxx UBSAN warning in 4.14-rc1
Hi Meelis, > On Jan 24, 2018, at 2:18 PM, Meelis Roos wrote: > >>> Hello, I decided to widen the coverage of my kernel testbed and put some >>> FC cards into servers. This one is a PCI-X QLA2340 in HP Proliant DL 380 >>> G4 (first 64-bit generation of Proliants). I got a UBSAN warning from >>> qla2xxx before probing for the firmware. >> >> Would it be possible for you to test the (completely untested) patch below? > > It compiles without warnings and the driver loads without warnings. > > Meanwhile I tried the following patch, also successfully. > > However, the same problem is present in qla24xx_mbx_completion (and can > also be trivially patched over). > > I did not understand the logic of what's goind on with mailboxes - there > seem to be up to 32 of them and for some reason, a bitmask is used for > iterating over them, with mboxes = ha->mcp->in_mb filtering out some > mailboxes, and in_mb bitmap value comes from firmware? > > diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c > index 2fd79129bb2a..7868930ae1c8 100644 > --- a/drivers/scsi/qla2xxx/qla_isr.c > +++ b/drivers/scsi/qla2xxx/qla_isr.c > @@ -272,7 +272,7 @@ qla2x00_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0) > struct device_reg_2xxx __iomem *reg = &ha->iobase->isp; > > /* Read all mbox registers? */ > - mboxes = (1 << ha->mbx_count) - 1; > + mboxes = (1ULL << ha->mbx_count) - 1; > if (!ha->mcp) > ql_dbg(ql_dbg_async, vha, 0x5001, "MBX pointer ERROR.\n"); > else > > -- > Meelis Roos (mr...@linux.ee) Since I did could not get hold of 4G adapter for testing, i was not able to get to this one fixed in time. Bart’s change looks good and with your testing should be good to include. I also noticed qla24xx_mbx_completion() will need this fix. I was able to confirm it on my setup with 16/32G adapter. diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c index d1e7fd905f16..b97b14a89ac3 100644 --- a/drivers/scsi/qla2xxx/qla_isr.c +++ b/drivers/scsi/qla2xxx/qla_isr.c @@ -272,7 +272,7 @@ qla2x00_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0) struct device_reg_2xxx __iomem *reg = &ha->iobase->isp; /* Read all mbox registers? */ - mboxes = (1 << ha->mbx_count) - 1; + mboxes = (ha->mbx_count != 32 ? 1U << ha->mbx_count : 0) - 1U; if (!ha->mcp) ql_dbg(ql_dbg_async, vha, 0x5001, "MBX pointer ERROR.\n"); else @@ -2881,7 +2881,7 @@ qla24xx_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0) struct device_reg_24xx __iomem *reg = &ha->iobase->isp24; /* Read all mbox registers? */ - mboxes = (1 << ha->mbx_count) - 1; + mboxes = (ha->mbx_count != 32 ? 1U << ha->mbx_count : 0) - 1U; if (!ha->mcp) ql_dbg(ql_dbg_async, vha, 0x504e, "MBX pointer ERROR.\n”); Would you care to send formal patch and add my ACK to it? Thanks for all the effort on getting this tested on your setup. Thanks, - Himanshu
Re: [PATCH] qla2xxx: Avoid triggering undefined behavior in qla2x00_mbx_completion()
> On Jan 25, 2018, at 8:24 AM, Bart Van Assche wrote: > > A left shift must shift less than the bit width of the left argument. > Avoid triggering undefined behavior if ha->mbx_count == 32. > > This patch avoids that UBSAN reports the following complaint: > > UBSAN: Undefined behaviour in drivers/scsi/qla2xxx/qla_isr.c:275:14 > shift exponent 32 is too large for 32-bit type 'int' > Call Trace: > dump_stack+0x4e/0x6c > ubsan_epilogue+0xd/0x3b > __ubsan_handle_shift_out_of_bounds+0x112/0x14c > qla2x00_mbx_completion+0x1c5/0x25d [qla2xxx] > qla2300_intr_handler+0x1ea/0x3bb [qla2xxx] > qla2x00_mailbox_command+0x77b/0x139a [qla2xxx] > qla2x00_mbx_reg_test+0x83/0x114 [qla2xxx] > qla2x00_chip_diag+0x354/0x45f [qla2xxx] > qla2x00_initialize_adapter+0x2c2/0xa4e [qla2xxx] > qla2x00_probe_one+0x1681/0x392e [qla2xxx] > pci_device_probe+0x10b/0x1f1 > driver_probe_device+0x21f/0x3a4 > __driver_attach+0xa9/0xe1 > bus_for_each_dev+0x6e/0xb5 > driver_attach+0x22/0x3c > bus_add_driver+0x1d1/0x2ae > driver_register+0x78/0x130 > __pci_register_driver+0x75/0xa8 > qla2x00_module_init+0x21b/0x267 [qla2xxx] > do_one_initcall+0x5a/0x1e2 > do_init_module+0x9d/0x285 > load_module+0x20db/0x38e3 > SYSC_finit_module+0xa8/0xbc > SyS_finit_module+0x9/0xb > do_syscall_64+0x77/0x271 > entry_SYSCALL64_slow_path+0x25/0x25 > > Reported-by: Meelis Roos > Signed-off-by: Bart Van Assche > Cc: Himanshu Madhani > --- > drivers/scsi/qla2xxx/qla_isr.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c > index 16c43bd9bb83..79f538448364 100644 > --- a/drivers/scsi/qla2xxx/qla_isr.c > +++ b/drivers/scsi/qla2xxx/qla_isr.c > @@ -272,7 +272,8 @@ qla2x00_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0) > struct device_reg_2xxx __iomem *reg = &ha->iobase->isp; > > /* Read all mbox registers? */ > - mboxes = (1 << ha->mbx_count) - 1; > + WARN_ON_ONCE(ha->mbx_count > 32); > + mboxes = (1ULL << ha->mbx_count) - 1; > if (!ha->mcp) > ql_dbg(ql_dbg_async, vha, 0x5001, "MBX pointer ERROR.\n"); > else > @@ -2884,7 +2885,8 @@ qla24xx_mbx_completion(scsi_qla_host_t *vha, uint16_t > mb0) > struct device_reg_24xx __iomem *reg = &ha->iobase->isp24; > > /* Read all mbox registers? */ > - mboxes = (1 << ha->mbx_count) - 1; > + WARN_ON_ONCE(ha->mbx_count > 32); > + mboxes = (1ULL << ha->mbx_count) - 1; > if (!ha->mcp) > ql_dbg(ql_dbg_async, vha, 0x504e, "MBX pointer ERROR.\n"); > else > -- > 2.16.0 > Thanks for the formal patch. Acked-by: Himanshu Madhani Thanks, - Himanshu
Re: A qla2xxx commit cause Linux no response, has not fixed in lastest version 4.15-rc6
Hi Chang, > On Jan 18, 2018, at 4:51 AM, Changlimin wrote: > > Hi Himanshu， > Today I reproduced the issue in my server. > First, I compiled kernel 4.15-rc6 (make localmodconfig; make; make > modules_install; make install), then start the kernel with parameter > modprobe.blacklist=qla2xxx. > Second, tail -f /var/log/syslog > Third, modprobe qla2xxx ql2xextended_error_logging=0x1e40 , the log is > syslog-1e40.txt > The syslog-7fff is got when modprobe qla2xxx > ql2xextended_error_logging=0x7fff > > BTW, I haven't load driver from 4.9.x to kernel 4.15-rc6. > When I checkout kernel commit 726b85487067d7f5b23495bc33c484b8517c4074, all > kernel code is 4.9.x. > Sorry for extended delay in the response. From the syslog that you sent me, I do see driver version 10.00.00.02-k which is from 4.15.0-rc6 so atleast you are using the correct driver. (in your email earlier you mentioned 8.07.xx which was confusing) Jan 18 20:30:23 cvknode25 kernel: [ 100.991309] qla2xxx [:00:00.0]-0005: : QLogic Fibre Channel HBA Driver: 10.00.00.02-k-debug. Jan 18 20:30:23 cvknode25 kernel: [ 100.991486] qla2xxx [:0a:00.0]-001d: : Found an ISP2532 irq 16 iobase 0x67aad9fd. Jan 18 20:30:23 cvknode25 kernel: [ 101.651676] qla2xxx [:0a:00.0]-4800:1: DPC handler sleeping. Jan 18 20:30:23 cvknode25 kernel: [ 101.651677] scsi host1: qla2xxx Also I do see Jan 18 20:30:24 cvknode25 kernel: [ 102.624987] qla2xxx [:0a:00.0]-500a:1: LOOP UP detected (8 Gbps). i.e. driver was able to bring up 8G link So having said that i still do not have clear picture from the logs provided, why you are encountering issue. Can you please share you configuration details. I would like to see how is your system setup and see if i can replicate in our lab here. > Regards > Chang Limin > > -Original Message- > From: Madhani, Himanshu [mailto:himanshu.madh...@cavium.com] > Sent: Thursday, January 18, 2018 2:26 AM > To: changlimin (Cloud) > Cc: Nicholas A. Bellinger; Tran, Quinn; jifuliang (Cloud); zhangguanghui > (Cloud); zhangzijian (Cloud); target-devel; linux-scsi > Subject: Re: A qla2xxx commit cause Linux no response, has not fixed in > lastest version 4.15-rc6 > > Hi Chang, > >> On Jan 15, 2018, at 10:49 PM, Changlimin wrote: >> >> Hi Himanshu, >> This is my progress. >> First, I compiled 4.15-rc6, I found linux hang when booting, the stack >> showed something wrong in qla2xxx driver. > > Can you provide me detail steps of how you compiled 4.15-rc6. Also provide me > details of how you are loading driver and also provide complete log file. > > I do not see how you will be able to load driver which is from 4.9.x when you > compile fresh 4.15.0-rc6. > > Just FYI, I build test system with 8G/16G/32G adapter with 4.15.0-rc6 kernel > and I am not able to see hang that you are describing. > > # uname -r > 4.15.0-rc6+ > > # modprobe qla2xxx > > # fcc.sh > FC HBAs: > HBA Port NamePort ID State Device > host3 21:00:00:24:ff:7e:f5:80 01:0d:00 OnlineQLE2742 FW:v8.05.63 > DVR:v10.00.00.04-k > host4 21:00:00:24:ff:7e:f5:81 01:0e:00 OnlineQLE2742 FW:v8.05.63 > DVR:v10.00.00.04-k > host5 21:00:00:0e:1e:12:e9:a0 01:06:00 OnlineQLE8362 FW:v8.03.06 > DVR:v10.00.00.04-k > host6 21:00:00:0e:1e:12:e9:a1 01:14:00 OnlineQLE8362 FW:v8.03.06 > DVR:v10.00.00.04-k > host7 21:00:00:24:ff:46:0a:5c 01:0d:00 OnlineQLE2562 FW:v8.03.00 > DVR:v10.00.00.04-k > host8 21:00:00:24:ff:46:0a:5d 01:15:00 OnlineQLE2562 FW:v8.03.00 > DVR:v10.00.00.04-k > > # modinfo qla2xxx | more > > filename: > /lib/modules/4.15.0-rc6+/kernel/drivers/scsi/qla2xxx/qla2xxx.ko > firmware: ql2500_fw.bin > firmware: ql2400_fw.bin > firmware: ql2322_fw.bin > firmware: ql2300_fw.bin > firmware: ql2200_fw.bin > firmware: ql2100_fw.bin > version:10.00.00.04-k > license:GPL > description:QLogic Fibre Channel HBA Driver > author: QLogic Corporation > srcversion: 6CBCF1372A7756690E83CC3 > > >> Second, I want to find which commit introduced the issue. So I tried many >> times via git bisect to linux kernel. >> Finally, I found the commit 726b85487067d7f5b23495bc33c484b8517c4074 >> introduced the issue. The attached log is related to this commit. >> Also ubuntu kernel has this issue: >> >> https://launchpad.net/ubuntu/+archive/primary/+files/linux-image-4.13. >> 0-25-generic_4.13.0-25.29_amd64.deb >> >> https://launchpad.net/ubuntu/+archive/primary/+files/linux-image-extra >> -4.13.0-25-generic_4.13.0
Re: [PATCH 0/6] Six qla2xxx and qla4xxx patches
Hi Martin, > On Jan 30, 2018, at 7:04 PM, Martin K. Petersen > wrote: > > > Bart, > >> The patches in this series are what I came up with after having analyzed >> the source code of the qla[24]xxx drivers with several source code analysis >> tools (scripts/kernel-doc, gcc, sparse and smatch). None of the patches in >> this series have been tested. Yet I'm asking you to consider at least the >> first patch of this series for kernel v4.16 because it fixes a locking bug >> in one of the SCSI patches queued for kernel v4.16. > > Himanshu and team: Please review! > Series looks good Acked-by: Himanshu Madhani > -- > Martin K. PetersenOracle Linux Engineering Thanks, - Himanshu
Re: [PATCH] qla2xxx: Fix NULL pointer crash due to active timer for ABTS
Hi Johannes, > On Feb 13, 2018, at 2:38 AM, Johannes Thumshirn wrote: > > Thanks Himanshu, > Reviewed-by: Johannes Thumshirn > > Do you happen to know which commit it actually fixes? > Thanks for asking (I did not add fixes commit ID because it was pre 4.x kernel) Here’s commit id 4440e46d5db7b445a961a8849b2a31fa7fd1 which is fixed by this patch. > -- > Johannes Thumshirn Storage > jthu > msh...@suse.de+49 911 74053 689 > SUSE > LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Jane > Smithard, Graham Norton > HRB 21284 (AG Nürnberg) > Key fingerprint = EC38 > 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 Thanks, - Himanshu
Re: [PATCH] qla2xxx: Fix NULL pointer crash due to active timer for ABTS
Hi John,
> On Feb 13, 2018, at 2:54 AM, John Garry wrote:
>
> On 12/02/2018 18:28, Himanshu Madhani wrote:
>> This patch fixes NULL pointer crash due to active timer running
>> for abort IOCB.
>>
>>> From crash dump analysis it was discoverd that get_next_timer_interrupt()
>> encountered a corrupted entry on the timer list.
>>
>> #9 [95e1f6f0fd40] page_fault at 914fe8f8
>>[exception RIP: get_next_timer_interrupt+440]
>>RIP: 90ea3088 RSP: 95e1f6f0fdf0 RFLAGS: 00010013
>>RAX: 95e1f6451028 RBX: 000218e2389e5f40 RCX: 0001232ad600
>>RDX: 0001 RSI: 95e1f6f0fdf0 RDI: 01232ad6
>>RBP: 95e1f6f0fe40 R8: 95e1f6451188 R9: 0001
>>R10: 0016 R11: 0016 R12: 0001232ad5f6
>>R13: 95e1f645 R14: 95e1f6f0fdf8 R15: 95e1f6f0fe10
>>ORIG_RAX: CS: 0010 SS: 0018
>>
>> Looking at the assembly of get_next_timer_interrupt(), address came
>> from %r8 (95e1f6451188) which is pointing to list_head with single
>> entry at 95e5ff621178.
>>
>> 0x90ea307a : mov(%r8),%rdx
>> 0x90ea307d : cmp%r8,%rdx
>> 0x90ea3080 : je
>> 0x90ea30a7
>> 0x90ea3082 : nopw
>> 0x0(%rax,%rax,1)
>> 0x90ea3088 : testb
>> $0x1,0x18(%rdx)
>>
>> crash> rd 95e1f6451188 10
>> 95e1f6451188: 95e5ff621178 95e5ff621178 x.b.x.b.
>> 95e1f6451198: 95e1f6451198 95e1f6451198 ..E...E.
>> 95e1f64511a8: 95e1f64511a8 95e1f64511a8 ..E...E.
>> 95e1f64511b8: 95e77cf509a0 95e77cf509a0 ...|...|
>> 95e1f64511c8: 95e1f64511c8 95e1f64511c8 ..E...E.
>>
>> crash> rd 95e5ff621178 10
>> 95e5ff621178: 0001 95e15936aa00 ..6Y
>> 95e5ff621188:
>> 95e5ff621198: 00a0 0010
>> 95e5ff6211a8: 95e5ff621198 000c ..b.
>> 95e5ff6211b8: 0f58 95e751f8d720 X... ..Q
>>
>> 95e5ff621178 belongs to freed mempool object at 95e5ff621080.
>>
>> CACHENAME OBJSIZE ALLOCATED TOTAL SLABS
>> SSIZE
>> 95dc7fd74d00 mnt_cache384 19785 24948594
>> 16k
>> SLAB MEMORYNODE TOTAL ALLOCATED FREE
>> dc5dabfd8800 95e5ff62 1 42 2913
>> FREE / [ALLOCATED]
>>95e5ff621080 (cpu 6 cache)
>>
>> Examining the contents of that memory reveals a pointer to a constant
>> string in the driver, "abort\0", which is set by qla24xx_async_abort_cmd().
>>
>> crash> rd c059277c 20
>> c059277c: 6e490074726f6261 0074707572726574 abort.Interrupt.
>> c059278c: 00676e696c6c6f50 6920726576697244 Polling.Driver i
>> c059279c: 646f6d207325206e 6974736554000a65 n %s mode..Testi
>> c05927ac: 636976656420676e 786c252074612065 ng device at %lx
>> c05927bc: 6b63656843000a2e 646f727020676e69 ...Checking prod
>> c05927cc: 6f20444920746375 0a2e706968632066 uct ID of chip..
>> c05927dc: 5120646e756f4600 204130303232414c .Found QLA2200A
>> c05927ec: 43000a2e70696843 20676e696b636568 Chip...Checking
>> c05927fc: 65786f626c69616d 6c636e69000a2e73 mailboxes...incl
>> c059280c: 756e696c2f656475 616d2d616d642f78 ude/linux/dma-ma
>>
>> crash> struct -ox srb_iocb
>> struct srb_iocb {
>> union {
>> struct {...} logio;
>> struct {...} els_logo;
>> struct {...} tmf;
>> struct {...} fxiocb;
>> struct {...} abt;
>> struct ct_arg ctarg;
>> struct {...} mbx;
>> struct {...} nack;
>>[0x0 ] } u;
>>[0xb8] struct timer_list timer;
>>[0x108] void (*timeout)(void *);
>> }
>> SIZE: 0x110
>>
>> crash> ! bc
>> ibase=16
>> obase=10
>> B8+40
>> F8
>>
>> The object is a srb_t, and at offset 0xf8 within that structure
>> (i.e. 95e5ff621080 + f8 -> 95e5ff621178) is a struct timer_list.
>>
>> Cc: #4.4+
>> Signed-off-by: Himanshu Madhani
>> ---
>> Hi Martin,
>>
>> This patch addresses crash due to NULL pointer access because driver
>> left active timer running for abort IOCB.
>>
>> Please apply this patch to 4.16/scsi-fixes at your earliest convenience.
>>
>> Thanks,
>> Himanshu
>> ---
>> drivers/scsi/qla2xxx/qla_init.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_init.c
>> b/drivers/scsi/qla2xxx/qla_init.c
>> index 2dea1129d396..04870621e712 100644
>> --- a/drivers/scsi/qla2xxx/qla_init.c
>> +++ b/drivers/scsi/qla2xxx/qla_init.c
>> @@ -1527,6 +1527,7 @@ qla24xx_abort_sp_done(void *ptr, int res)
>> srb_t *sp = ptr;
>> st
Re: [PATCH] qla2xxx: Fix NULL pointer crash due to active timer for ABTS
Hi John,
> On Feb 13, 2018, at 2:54 AM, John Garry wrote:
>
> On 12/02/2018 18:28, Himanshu Madhani wrote:
>> This patch fixes NULL pointer crash due to active timer running
>> for abort IOCB.
>>
>>> From crash dump analysis it was discoverd that get_next_timer_interrupt()
>> encountered a corrupted entry on the timer list.
>>
>> #9 [95e1f6f0fd40] page_fault at 914fe8f8
>>[exception RIP: get_next_timer_interrupt+440]
>>RIP: 90ea3088 RSP: 95e1f6f0fdf0 RFLAGS: 00010013
>>RAX: 95e1f6451028 RBX: 000218e2389e5f40 RCX: 0001232ad600
>>RDX: 0001 RSI: 95e1f6f0fdf0 RDI: 01232ad6
>>RBP: 95e1f6f0fe40 R8: 95e1f6451188 R9: 0001
>>R10: 0016 R11: 0016 R12: 0001232ad5f6
>>R13: 95e1f645 R14: 95e1f6f0fdf8 R15: 95e1f6f0fe10
>>ORIG_RAX: CS: 0010 SS: 0018
>>
>> Looking at the assembly of get_next_timer_interrupt(), address came
>> from %r8 (95e1f6451188) which is pointing to list_head with single
>> entry at 95e5ff621178.
>>
>> 0x90ea307a : mov(%r8),%rdx
>> 0x90ea307d : cmp%r8,%rdx
>> 0x90ea3080 : je
>> 0x90ea30a7
>> 0x90ea3082 : nopw
>> 0x0(%rax,%rax,1)
>> 0x90ea3088 : testb
>> $0x1,0x18(%rdx)
>>
>> crash> rd 95e1f6451188 10
>> 95e1f6451188: 95e5ff621178 95e5ff621178 x.b.x.b.
>> 95e1f6451198: 95e1f6451198 95e1f6451198 ..E...E.
>> 95e1f64511a8: 95e1f64511a8 95e1f64511a8 ..E...E.
>> 95e1f64511b8: 95e77cf509a0 95e77cf509a0 ...|...|
>> 95e1f64511c8: 95e1f64511c8 95e1f64511c8 ..E...E.
>>
>> crash> rd 95e5ff621178 10
>> 95e5ff621178: 0001 95e15936aa00 ..6Y
>> 95e5ff621188:
>> 95e5ff621198: 00a0 0010
>> 95e5ff6211a8: 95e5ff621198 000c ..b.
>> 95e5ff6211b8: 0f58 95e751f8d720 X... ..Q
>>
>> 95e5ff621178 belongs to freed mempool object at 95e5ff621080.
>>
>> CACHENAME OBJSIZE ALLOCATED TOTAL SLABS
>> SSIZE
>> 95dc7fd74d00 mnt_cache384 19785 24948594
>> 16k
>> SLAB MEMORYNODE TOTAL ALLOCATED FREE
>> dc5dabfd8800 95e5ff62 1 42 2913
>> FREE / [ALLOCATED]
>>95e5ff621080 (cpu 6 cache)
>>
>> Examining the contents of that memory reveals a pointer to a constant
>> string in the driver, "abort\0", which is set by qla24xx_async_abort_cmd().
>>
>> crash> rd c059277c 20
>> c059277c: 6e490074726f6261 0074707572726574 abort.Interrupt.
>> c059278c: 00676e696c6c6f50 6920726576697244 Polling.Driver i
>> c059279c: 646f6d207325206e 6974736554000a65 n %s mode..Testi
>> c05927ac: 636976656420676e 786c252074612065 ng device at %lx
>> c05927bc: 6b63656843000a2e 646f727020676e69 ...Checking prod
>> c05927cc: 6f20444920746375 0a2e706968632066 uct ID of chip..
>> c05927dc: 5120646e756f4600 204130303232414c .Found QLA2200A
>> c05927ec: 43000a2e70696843 20676e696b636568 Chip...Checking
>> c05927fc: 65786f626c69616d 6c636e69000a2e73 mailboxes...incl
>> c059280c: 756e696c2f656475 616d2d616d642f78 ude/linux/dma-ma
>>
>> crash> struct -ox srb_iocb
>> struct srb_iocb {
>> union {
>> struct {...} logio;
>> struct {...} els_logo;
>> struct {...} tmf;
>> struct {...} fxiocb;
>> struct {...} abt;
>> struct ct_arg ctarg;
>> struct {...} mbx;
>> struct {...} nack;
>>[0x0 ] } u;
>>[0xb8] struct timer_list timer;
>>[0x108] void (*timeout)(void *);
>> }
>> SIZE: 0x110
>>
>> crash> ! bc
>> ibase=16
>> obase=10
>> B8+40
>> F8
>>
>> The object is a srb_t, and at offset 0xf8 within that structure
>> (i.e. 95e5ff621080 + f8 -> 95e5ff621178) is a struct timer_list.
>>
>> Cc: #4.4+
>> Signed-off-by: Himanshu Madhani
>> ---
>> Hi Martin,
>>
>> This patch addresses crash due to NULL pointer access because driver
>> left active timer running for abort IOCB.
>>
>> Please apply this patch to 4.16/scsi-fixes at your earliest convenience.
>>
>> Thanks,
>> Himanshu
>> ---
>> drivers/scsi/qla2xxx/qla_init.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_init.c
>> b/drivers/scsi/qla2xxx/qla_init.c
>> index 2dea1129d396..04870621e712 100644
>> --- a/drivers/scsi/qla2xxx/qla_init.c
>> +++ b/drivers/scsi/qla2xxx/qla_init.c
>> @@ -1527,6 +1527,7 @@ `(void *ptr, int res)
>> srb_t *sp = ptr;
>> struct srb_iocb *abt =
Re: [PATCH] qla2xxx: Fix NULL pointer crash due to active timer for ABTS
Hi Martin,
> On Feb 14, 2018, at 11:58 AM, Madhani, Himanshu
> wrote:
>
> Hi John,
>
>> On Feb 13, 2018, at 2:54 AM, John Garry wrote:
>>
>> On 12/02/2018 18:28, Himanshu Madhani wrote:
>>> This patch fixes NULL pointer crash due to active timer running
>>> for abort IOCB.
>>>
>>>> From crash dump analysis it was discoverd that get_next_timer_interrupt()
>>> encountered a corrupted entry on the timer list.
>>>
>>> #9 [95e1f6f0fd40] page_fault at 914fe8f8
>>> [exception RIP: get_next_timer_interrupt+440]
>>> RIP: 90ea3088 RSP: 95e1f6f0fdf0 RFLAGS: 00010013
>>> RAX: 95e1f6451028 RBX: 000218e2389e5f40 RCX: 0001232ad600
>>> RDX: 0001 RSI: 95e1f6f0fdf0 RDI: 01232ad6
>>> RBP: 95e1f6f0fe40 R8: 95e1f6451188 R9: 0001
>>> R10: 0016 R11: 0016 R12: 0001232ad5f6
>>> R13: 95e1f645 R14: 95e1f6f0fdf8 R15: 95e1f6f0fe10
>>> ORIG_RAX: CS: 0010 SS: 0018
>>>
>>> Looking at the assembly of get_next_timer_interrupt(), address came
>>> from %r8 (95e1f6451188) which is pointing to list_head with single
>>> entry at 95e5ff621178.
>>>
>>> 0x90ea307a : mov(%r8),%rdx
>>> 0x90ea307d : cmp%r8,%rdx
>>> 0x90ea3080 : je
>>> 0x90ea30a7
>>> 0x90ea3082 : nopw
>>> 0x0(%rax,%rax,1)
>>> 0x90ea3088 : testb
>>> $0x1,0x18(%rdx)
>>>
>>> crash> rd 95e1f6451188 10
>>> 95e1f6451188: 95e5ff621178 95e5ff621178 x.b.x.b.
>>> 95e1f6451198: 95e1f6451198 95e1f6451198 ..E...E.
>>> 95e1f64511a8: 95e1f64511a8 95e1f64511a8 ..E...E.
>>> 95e1f64511b8: 95e77cf509a0 95e77cf509a0 ...|...|
>>> 95e1f64511c8: 95e1f64511c8 95e1f64511c8 ..E...E.
>>>
>>> crash> rd 95e5ff621178 10
>>> 95e5ff621178: 0001 95e15936aa00 ..6Y
>>> 95e5ff621188:
>>> 95e5ff621198: 00a0 0010
>>> 95e5ff6211a8: 95e5ff621198 000c ..b.
>>> 95e5ff6211b8: 0f58 95e751f8d720 X... ..Q
>>>
>>> 95e5ff621178 belongs to freed mempool object at 95e5ff621080.
>>>
>>> CACHENAME OBJSIZE ALLOCATED TOTAL SLABS
>>> SSIZE
>>> 95dc7fd74d00 mnt_cache384 19785 24948594
>>> 16k
>>> SLAB MEMORYNODE TOTAL ALLOCATED FREE
>>> dc5dabfd8800 95e5ff62 1 42 2913
>>> FREE / [ALLOCATED]
>>> 95e5ff621080 (cpu 6 cache)
>>>
>>> Examining the contents of that memory reveals a pointer to a constant
>>> string in the driver, "abort\0", which is set by qla24xx_async_abort_cmd().
>>>
>>> crash> rd c059277c 20
>>> c059277c: 6e490074726f6261 0074707572726574 abort.Interrupt.
>>> c059278c: 00676e696c6c6f50 6920726576697244 Polling.Driver i
>>> c059279c: 646f6d207325206e 6974736554000a65 n %s mode..Testi
>>> c05927ac: 636976656420676e 786c252074612065 ng device at %lx
>>> c05927bc: 6b63656843000a2e 646f727020676e69 ...Checking prod
>>> c05927cc: 6f20444920746375 0a2e706968632066 uct ID of chip..
>>> c05927dc: 5120646e756f4600 204130303232414c .Found QLA2200A
>>> c05927ec: 43000a2e70696843 20676e696b636568 Chip...Checking
>>> c05927fc: 65786f626c69616d 6c636e69000a2e73 mailboxes...incl
>>> c059280c: 756e696c2f656475 616d2d616d642f78 ude/linux/dma-ma
>>>
>>> crash> struct -ox srb_iocb
>>> struct srb_iocb {
>>> union {
>>> struct {...} logio;
>>> struct {...} els_logo;
>>> struct {...} tmf;
>>> struct {...} fxiocb;
>>> struct {...} abt;
>>> struct ct_arg ctarg;
>>> struct {...} mbx;
>>> struct {...} nack;
>>> [0x0 ] } u;
>>> [0xb8] struct timer_list timer;
>>> [0x108] voi
Re: [PATCH V2] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure
Hi Bill,
> On Mar 4, 2018, at 9:02 PM, Bill Kuzeja wrote:
>
> Because of the shifting around of code in qla2x00_probe_one recently,
> failures during adapter initialization can lead to problems, i.e. NULL
> pointer crashes and doubly freed data structures which cause eventual
> panics.
>
> This V2 version makes the relevant memory free routines idempotent, so
> repeat calls won't cause any harm. I also removed the problematic
> probe_init_failed exit point as it is not needed.
>
> Fixes: d64d6c5671db ("scsi: qla2xxx: Fix NULL pointer crash due to probe
> failure")
> Signed-off-by: Bill Kuzeja
>
> ---
>
> Some of these issues are due to:
>
> commit d64d6c5671db scsi: qla2xxx: Fix NULL pointer crash due to probe failure
>
> where some frees were moved around, as well as the error exit from
> a qla2x00_request_irqs failure.
>
> This was a fix for:
>
> commit d74595278f4a scsi: qla2xxx: Add multiple queue pair functionality.
>
> which caused problems of its own.
>
> To reproduce these issues, I run a test where I break the card early in
> init, (and also through kprobe fault injection). This way, I've been able
> to hit several different types of crashes, all started by failures of
> various routines called throughout the probe.
>
> The problematic routines that fail and their exit points are:
>
> qla2x00_alloc_queues => probe_init_failed
> initialize_adapter=> probe_failed
> kthread_create=> probe_failed
> scsi_add_host => probe_failed
>
> Exit points are ordered in this way:
>
> probe_init_failed:
>qla2x00_free_req_que(ha, req);
>ha->req_q_map[0] = NULL;
>clear_bit(0, ha->req_qid_map);
>qla2x00_free_rsp_que(ha, rsp);
>ha->rsp_q_map[0] = NULL;
>clear_bit(0, ha->rsp_qid_map);
>ha->max_req_queues = ha->max_rsp_queues = 0;
>
> probe_failed:
>if (base_vha->timer_active)
>qla2x00_stop_timer(base_vha);
> ...
>qla2x00_free_device(base_vha);
>
>scsi_host_put(base_vha->host);
>
> probe_hw_failed:
>qla2x00_mem_free(ha);
>qla2x00_free_req_que(ha, req);
>qla2x00_free_rsp_que(ha, rsp);
>qla2x00_clear_drv_active(ha);
>
> Note that qla2x00_free_device calls qla2x00_mem_free and
> qla2x00_free_rsp_que and qla2x00_free_req_que. So if we exit to
> probe_failed or probe_init_failed, we'll end up calling these
> routines multiple times.
>
> These routines are not idempotent, I am making them so. This solves
> most of the issues.
>
> Also probe_init_failed is not needed. In the place that it is called,
> ha->req_q_map[0] and ha->rsp_q_map[0] are not setup. And we now call
> qla2x00_free_rsp_que/qla2x00_free_req_que below in probe_hw_failed.
> I removed this exit point entirely.
>
> Along the way I found that the return code for qla2x00_alloc_queues
> never really causes us to exit out of the probe routine.
>
> In order to fail...
>
>if (!qla2x00_alloc_queues(ha, req, rsp)) {
>
> ...we must return 0. However, internally, if this routine succeeds it
> returns 1 and if it fails it returns -ENOMEM. So I am modifying
> qla2x00_alloc_queues to fall in line with other return conventions
> where zero is a success (and obviously have changed the probe routine
> accordingly).
>
> One more issue falls out of this case: when qla2x00_abort_all_cmds
> is invoked from qla2x00_free_device and request queues are not
> allocated (qla2x00_alloc_queues has not succeeded), we crash on a NULL
> pointer (ha->req_q_map[que]). So check for this at the start of
> qla2x00_abort_all_cmds and exit accordingly.
>
> I've tested out these changes thoroughly failing initialization at
> various times. I've also used kprobes to inject errors to force us
> into various error paths.
> ---
> drivers/scsi/qla2xxx/qla_os.c | 59 +++
> 1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index afcb5567..3860bdfc 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -454,7 +454,7 @@ static int qla2x00_alloc_queues(struct qla_hw_data *ha,
> struct req_que *req,
> ha->req_q_map[0] = req;
> set_bit(0, ha->rsp_qid_map);
> set_bit(0, ha->req_qid_map);
> - return 1;
> + return 0;
>
> fail_qpair_map:
> kfree(ha->base_qpair);
> @@ -471,6 +471,9 @@ static int qla2x00_alloc_queues(struct qla_hw_data *ha,
> struct req_que *req,
>
> static void qla2x00_free_req_que(struct qla_hw_data *ha, struct req_que *req)
> {
> + if (!ha->req_q_map)
> + return;
> +
> if (IS_QLAFX00(ha)) {
> if (req && req->ring_fx00)
> dma_free_coherent(&ha->pdev->dev,
> @@ -481,14 +484,17 @@ static void qla2x00_free_req_que(struct qla_hw_data
> *ha, struct req_que *req)
> (req->length + 1) * sizeof(request_t),
> req->ring, req->dma);
>
> - if (req)
>
Re: [PATCH 1/3] qla2xxx: fix error message on
> On Mar 8, 2018, at 5:44 AM, Meelis Roos wrote:
>
> This patch fixes IO traps caught by hardware when mailbox command fails on
> qla2200. The error handler assumes newer firmware that is available on 2400
> and
> newer HBA-s.
>
> This causes ugly crashes on sparc64.
>
> Fix it with separate debug prints on different firmware generations like most
> other places do.
>
> Note: the debug line identifier is the same 0x1198 for both cases. Maybe it
> needs to be renumbered in the new case?
>
> Signed-off-by: Meelis Roos
>
> ---
> drivers/scsi/qla2xxx/qla_mbx.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 7397aed..3a81555 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -503,11 +503,19 @@ qla2x00_mailbox_command(scsi_qla_host_t *vha, mbx_cmd_t
> *mcp)
> }
> pr_warn(" cmd=%x \n", command);
> }
> - ql_dbg(ql_dbg_mbx, vha, 0x1198,
> - "host_status=%#x intr_ctrl=%#x intr_status=%#x\n",
> - RD_REG_DWORD(®->isp24.host_status),
> - RD_REG_DWORD(®->isp24.ictrl),
> - RD_REG_DWORD(®->isp24.istatus));
> + if (IS_FWI2_CAPABLE(ha) && !(IS_P3P_TYPE(ha))) {
> + ql_dbg(ql_dbg_mbx, vha, 0x1198,
> + "host_status=%#x intr_ctrl=%#x intr_status=%#x\n",
> + RD_REG_DWORD(®->isp24.host_status),
> + RD_REG_DWORD(®->isp24.ictrl),
> + RD_REG_DWORD(®->isp24.istatus));
> + } else {
> + ql_dbg(ql_dbg_mbx, vha, 0x1198,
use 0x1206 here for numbering.
> + "ctrl_status=%#x ictrl=%#x istatus=%#x\n",
> + RD_REG_WORD(®->isp.ctrl_status),
> + RD_REG_WORD(®->isp.ictrl),
> + RD_REG_WORD(®->isp.istatus));
> + }
> } else {
> ql_dbg(ql_dbg_mbx, base_vha, 0x1021, "Done %s.\n", __func__);
> }
> --
> 2.1.4
>
Also, you need to update qla_dbg.c with this number
Here’s diff will look like
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 1abc8a9064b3..5fd44c50bbac 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -14,7 +14,7 @@
* | Module Init and Probe| 0x0193 | 0x0146 |
* | || 0x015b-0x0160 |
* | || 0x016e
|
- * | Mailbox commands | 0x1205 | 0x11a2-0x11ff |
+ * | Mailbox commands | 0x1206 | 0x11a2-0x11ff |
* | Device Discovery | 0x2134 | 0x210e-0x2116 |
* | || 0x211a |
* | || 0x211c-0x2128 |
With these changes, you can add
Acked-by: Himanshu Madhani
Thanks,
- Himanshu
Re: [PATCH 2/3] qla2xxx: fx00 copypaste typo
> On Mar 8, 2018, at 5:44 AM, Meelis Roos wrote:
>
> Fix an obvious copy-paste error in freeing QLAFX00 response queue - the code
> checked for rsp->ring but freed rsp->ring_fx00.
>
> Signed-off-by: Meelis Roos
>
> ---
> drivers/scsi/qla2xxx/qla_os.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index afcb5567..3c7bc2d 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -490,7 +490,7 @@ static void qla2x00_free_req_que(struct qla_hw_data *ha,
> struct req_que *req)
> static void qla2x00_free_rsp_que(struct qla_hw_data *ha, struct rsp_que *rsp)
> {
> if (IS_QLAFX00(ha)) {
> - if (rsp && rsp->ring)
> + if (rsp && rsp->ring_fx00)
> dma_free_coherent(&ha->pdev->dev,
> (rsp->length_fx00 + 1) * sizeof(request_t),
> rsp->ring_fx00, rsp->dma_fx00);
> --
> 2.1.4
>
Looks good
Acked-by: Himanshu Madhani
Thanks,
- Himanshu
