Recent updates in pHyp Firmware and VIOS releases provide new infrastructure
towards enabling Subordinate Command Response Queues (Sub-CRQs) such that each
Sub-CRQ is a channel backed by an actual hardware queue in the FC stack on the
partner VIOS. Sub-CRQs are registered with the firmware via hypercalls and then
negotiated with the VIOS via new Management Datagrams (MADs) for channel setup.
This initial implementation adds the necessary Sub-CRQ framework and implements
the new MADs for negotiating and assigning a set of Sub-CRQs to associated VIOS
HW backed channels. The event pool and locking still leverages the legacy single
queue implementation, and as such lock contention is problematic when increasing
the number of queues. However, this initial work demonstrates a 1.2x factor
increase in IOPs when configured with two HW queues despite lock contention.
changes in v2:
* Patch 4: changed firmware support logging to dev_warn_once
* Patch 6: adjusted locking
* Patch 15: dropped logging verbosity, moved cancel event tracking into subqueue
* Patch 17: removed write permission for migration module parameters
drive hard reset after update to num of scsi channels
changes in v2:
* Patch 4: NULL'd scsi_scrq reference after deallocation
* Patch 6: Added switch case to handle XPORT event
* Patch 9: fixed ibmvfc_event leak and double free
* added support for cancel command with MQ
* added parameter toggles for MQ settings
Tyrel Datwyler (18):
ibmvfc: add vhost fields and defaults for MQ enablement
ibmvfc: define hcall wrapper for registering a Sub-CRQ
ibmvfc: add Subordinate CRQ definitions
ibmvfc: add alloc/dealloc routines for SCSI Sub-CRQ Channels
ibmvfc: add Sub-CRQ IRQ enable/disable routine
ibmvfc: add handlers to drain and complete Sub-CRQ responses
ibmvfc: define Sub-CRQ interrupt handler routine
ibmvfc: map/request irq and register Sub-CRQ interrupt handler
ibmvfc: implement channel enquiry and setup commands
ibmvfc: advertise client support for using hardware channels
ibmvfc: set and track hw queue in ibmvfc_event struct
ibmvfc: send commands down HW Sub-CRQ when channelized
ibmvfc: register Sub-CRQ handles with VIOS during channel setup
ibmvfc: add cancel mad initialization helper
ibmvfc: send Cancel MAD down each hw scsi channel
ibmvfc: enable MQ and set reasonable defaults
ibmvfc: provide modules parameters for MQ settings
ibmvfc: drop host lock when completing commands in CRQ
drivers/scsi/ibmvscsi/ibmvfc.c | 721 +++++++++++++++++++++++++++++----
drivers/scsi/ibmvscsi/ibmvfc.h | 79 +++-
2 files changed, 711 insertions(+), 89 deletions(-)
--
2.27.0
Introduce several new vhost fields for managing MQ state of the adapter
as well as initial defaults for MQ enablement.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 9 ++++++++-
drivers/scsi/ibmvscsi/ibmvfc.h | 13 +++++++++++--
2 files changed, 19 insertions(+), 3 deletions(-)
Create an irq mapping for the hw_irq number provided from phyp firmware.
Request an irq assigned our Sub-CRQ interrupt handler.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Extract the hwq id from a SCSI command and store it in the ibmvfc_event
structure to identify which Sub-CRQ to send the command down when
channels are being utilized.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 5 +++++
drivers/scsi/ibmvscsi/ibmvfc.h | 1 +
2 files changed, 6 insertions(+)
@@ -781,6 +781,7 @@ struct ibmvfc_event {structcompletioncomp;structcompletion*eh_comp;structtimer_listtimer;+u16hwq;};/* a pool of event structs for use */
Add a helper routine for initializing a Cancel MAD. This will be useful
for a channelized client that needs to send a Cancel commands down every
channel commands were sent for a particular LUN.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 67 ++++++++++++++++++++--------------
1 file changed, 39 insertions(+), 28 deletions(-)
When the client has negotiated the use of channels all vfcFrames are
required to go down a Sub-CRQ channel or it is a protocoal violation. If
the adapter state is channelized submit vfcFrames to the appropriate
Sub-CRQ via the h_send_sub_crq() helper.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
@@ -1513,15 +1522,19 @@ static int ibmvfc_send_event(struct ibmvfc_event *evt,structibmvfc_host*vhost,unsignedlongtimeout){__be64*crq_as_u64=(__be64*)&evt->crq;+intchannel_cmd=0;intrc;/* Copy the IU into the transfer area */*evt->xfer_iu=evt->iu;-if(evt->crq.format==IBMVFC_CMD_FORMAT)+if(evt->crq.format==IBMVFC_CMD_FORMAT){evt->xfer_iu->cmd.tag=cpu_to_be64((u64)evt);-elseif(evt->crq.format==IBMVFC_MAD_FORMAT)+channel_cmd=1;+}elseif(evt->crq.format==IBMVFC_MAD_FORMAT){evt->xfer_iu->mad_common.tag=cpu_to_be64((u64)evt);-else+if(evt->xfer_iu->mad_common.opcode==IBMVFC_TMF_MAD)+channel_cmd=1;+}elseBUG();list_add_tail(&evt->queue,&vhost->sent);
@@ -1534,8 +1547,17 @@ static int ibmvfc_send_event(struct ibmvfc_event *evt,mb();-if((rc=ibmvfc_send_crq(vhost,be64_to_cpu(crq_as_u64[0]),-be64_to_cpu(crq_as_u64[1])))){+if(vhost->using_channels&&channel_cmd)+rc=ibmvfc_send_sub_crq(vhost,+vhost->scsi_scrqs.scrqs[evt->hwq].vios_cookie,+be64_to_cpu(crq_as_u64[0]),+be64_to_cpu(crq_as_u64[1]),+0,0);+else+rc=ibmvfc_send_crq(vhost,be64_to_cpu(crq_as_u64[0]),+be64_to_cpu(crq_as_u64[1]));++if(rc){list_del(&evt->queue);del_timer(&evt->timer);
In general the client needs to send Cancel MADs and task management
commands down the same channel as the command(s) intended to cancel or
abort. The client assigns cancel keys per LUN and thus must send a
Cancel down each channel commands were submitted for that LUN. Further,
the client then must wait for those cancel completions prior to
submitting a LUN RESET or ABORT TASK SET.
Add a cancel event pointer and cancel rsp iu storage to the
ibmvfc_sub_queue struct such that the cancel routine can assign a cancel
event to each applicable queue. When in legacy CRQ mode we fake treating
it as a subqueue by using a subqueue struct allocated on the stack. Wait
for completion of each submitted cancel.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 104 ++++++++++++++++++++++-----------
drivers/scsi/ibmvscsi/ibmvfc.h | 38 ++++++------
2 files changed, 90 insertions(+), 52 deletions(-)
@@ -2339,67 +2339,103 @@ static int ibmvfc_cancel_all(struct scsi_device *sdev, int type){structibmvfc_host*vhost=shost_priv(sdev->host);structibmvfc_event*evt,*found_evt;-unionibmvfc_iursp;-intrsp_rc=-EBUSY;+structibmvfc_sub_queue*scrqs;+structibmvfc_sub_queuelegacy_crq;+intrsp_rc=0;unsignedlongflags;u16status;+intcancel_cnt=0;+intnum_hwq;+intret=0;+inti;ENTER;spin_lock_irqsave(vhost->host->host_lock,flags);-found_evt=NULL;-list_for_each_entry(evt,&vhost->sent,queue){-if(evt->cmnd&&evt->cmnd->device==sdev){-found_evt=evt;+if(vhost->using_channels&&vhost->scsi_scrqs.active_queues){+num_hwq=vhost->scsi_scrqs.active_queues;+scrqs=vhost->scsi_scrqs.scrqs;+}else{+/* Use ibmvfc_sub_queue on the stack to fake legacy CRQ as a subqueue */+num_hwq=1;+scrqs=&legacy_crq;+}++for(i=0;i<num_hwq;i++){+scrqs[i].cancel_event=NULL;+found_evt=NULL;+list_for_each_entry(evt,&vhost->sent,queue){+if(evt->cmnd&&evt->cmnd->device==sdev&&evt->hwq==i){+found_evt=evt;+cancel_cnt++;+break;+}+}++if(!found_evt)+continue;++if(vhost->logged_in){+scrqs[i].cancel_event=ibmvfc_init_tmf(vhost,sdev,type);+scrqs[i].cancel_event->hwq=i;+scrqs[i].cancel_event->sync_iu=&scrqs[i].cancel_rsp;+rsp_rc=ibmvfc_send_event(scrqs[i].cancel_event,vhost,default_timeout);+if(rsp_rc)+break;+}else{+rsp_rc=-EBUSY;break;}}-if(!found_evt){+spin_unlock_irqrestore(vhost->host->host_lock,flags);++if(!cancel_cnt){if(vhost->log_level>IBMVFC_DEFAULT_LOG_LEVEL)sdev_printk(KERN_INFO,sdev,"No events found to cancel\n");-spin_unlock_irqrestore(vhost->host->host_lock,flags);return0;}-if(vhost->logged_in){-evt=ibmvfc_init_tmf(vhost,sdev,type);-evt->sync_iu=&rsp;-rsp_rc=ibmvfc_send_event(evt,vhost,default_timeout);-}--spin_unlock_irqrestore(vhost->host->host_lock,flags);-if(rsp_rc!=0){sdev_printk(KERN_ERR,sdev,"Failed to send cancel event. rc=%d\n",rsp_rc);/* If failure is received, the host adapter is most likely goingthroughreset,returnsuccesssothecallerwillwaitforthecommandbeingcancelledtogetreturned*/-return0;+gotofree_events;}sdev_printk(KERN_INFO,sdev,"Cancelling outstanding commands.\n");-wait_for_completion(&evt->comp);-status=be16_to_cpu(rsp.mad_common.status);-spin_lock_irqsave(vhost->host->host_lock,flags);-ibmvfc_free_event(evt);-spin_unlock_irqrestore(vhost->host->host_lock,flags);+for(i=0;i<num_hwq;i++){+if(!scrqs[i].cancel_event)+continue;-if(status!=IBMVFC_MAD_SUCCESS){-sdev_printk(KERN_WARNING,sdev,"Cancel failed with rc=%x\n",status);-switch(status){-caseIBMVFC_MAD_DRIVER_FAILED:-caseIBMVFC_MAD_CRQ_ERROR:-/* Host adapter most likely going through reset, return success to-thecallerwillwaitforthecommandbeingcancelledtogetreturned*/-return0;-default:-return-EIO;-};+wait_for_completion(&scrqs[i].cancel_event->comp);+status=be16_to_cpu(scrqs[i].cancel_rsp.mad_common.status);++if(status!=IBMVFC_MAD_SUCCESS){+sdev_printk(KERN_WARNING,sdev,"Cancel failed with rc=%x\n",status);+switch(status){+caseIBMVFC_MAD_DRIVER_FAILED:+caseIBMVFC_MAD_CRQ_ERROR:+/* Host adapter most likely going through reset, return success to+thecallerwillwaitforthecommandbeingcancelledtogetreturned*/+gotofree_events;+default:+ret=-EIO;+gotofree_events;+};+}}sdev_printk(KERN_INFO,sdev,"Successfully cancelled outstanding commands\n");-return0;+free_events:+spin_lock_irqsave(vhost->host->host_lock,flags);+for(i=0;i<num_hwq;i++)+if(scrqs[i].cancel_event)+ibmvfc_free_event(scrqs[i].cancel_event);+spin_unlock_irqrestore(vhost->host->host_lock,flags);++returnret;}/**
Add the various module parameter toggles for adjusting the MQ
characteristics at boot/load time as well as a device attribute for
changing the client scsi channel request amount.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 76 +++++++++++++++++++++++++++++-----
1 file changed, 66 insertions(+), 10 deletions(-)
@@ -40,6 +40,12 @@ static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;staticunsignedintibmvfc_debug=IBMVFC_DEBUG;staticunsignedintlog_level=IBMVFC_DEFAULT_LOG_LEVEL;staticunsignedintcls3_error=IBMVFC_CLS3_ERROR;+staticunsignedintmq_enabled=IBMVFC_MQ;+staticunsignedintnr_scsi_hw_queues=IBMVFC_SCSI_HW_QUEUES;+staticunsignedintnr_scsi_channels=IBMVFC_SCSI_CHANNELS;+staticunsignedintmig_channels_only=IBMVFC_MIG_NO_SUB_TO_CRQ;+staticunsignedintmig_no_less_channels=IBMVFC_MIG_NO_N_TO_M;+staticLIST_HEAD(ibmvfc_head);staticDEFINE_SPINLOCK(ibmvfc_driver_lock);staticstructscsi_transport_template*ibmvfc_transport_template;
@@ -49,6 +55,22 @@ MODULE_AUTHOR("Brian King <brking@linux.vnet.ibm.com>");MODULE_LICENSE("GPL");MODULE_VERSION(IBMVFC_DRIVER_VERSION);+module_param_named(mq,mq_enabled,uint,S_IRUGO);+MODULE_PARM_DESC(mq,"Enable multiqueue support. "+"[Default="__stringify(IBMVFC_MQ)"]");+module_param_named(scsi_host_queues,nr_scsi_hw_queues,uint,S_IRUGO);+MODULE_PARM_DESC(scsi_host_queues,"Number of SCSI Host submission queues. "+"[Default="__stringify(IBMVFC_SCSI_HW_QUEUES)"]");+module_param_named(scsi_hw_channels,nr_scsi_channels,uint,S_IRUGO);+MODULE_PARM_DESC(scsi_hw_channels,"Number of hw scsi channels to request. "+"[Default="__stringify(IBMVFC_SCSI_CHANNELS)"]");+module_param_named(mig_channels_only,mig_channels_only,uint,S_IRUGO);+MODULE_PARM_DESC(mig_channels_only,"Prevent migration to non-channelized system. "+"[Default="__stringify(IBMVFC_MIG_NO_SUB_TO_CRQ)"]");+module_param_named(mig_no_less_channels,mig_no_less_channels,uint,S_IRUGO);+MODULE_PARM_DESC(mig_no_less_channels,"Prevent migration to system with less channels. "+"[Default="__stringify(IBMVFC_MIG_NO_N_TO_M)"]");+module_param_named(init_timeout,init_timeout,uint,S_IRUGO|S_IWUSR);MODULE_PARM_DESC(init_timeout,"Initialization timeout in seconds. ""[Default="__stringify(IBMVFC_INIT_TIMEOUT)"]");
@@ -823,7 +845,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)crq->cur=0;if(vhost->scsi_scrqs.scrqs){-for(i=0;i<IBMVFC_SCSI_HW_QUEUES;i++){+for(i=0;i<nr_scsi_hw_queues;i++){scrq=&vhost->scsi_scrqs.scrqs[i];memset(scrq->msgs,0,PAGE_SIZE);scrq->cur=0;
If the ibmvfc client adapter requests channels it must submit a number
of Sub-CRQ handles matching the number of channels being requested. The
VIOS in its response will overwrite the actual number of channel
resources allocated which may be less than what was requested. The
client then must store the VIOS Sub-CRQ handle for each queue. This VIOS
handle is needed as a parameter with h_send_sub_crq().
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
Previous patches have plumbed the necessary Sub-CRQ interface and
channel negotiation MADs to fully channelized hardware queues.
Advertise client support via NPIV Login capability
IBMVFC_CAN_USE_CHANNELS when the client bits have MQ enabled via
vhost->mq_enabled, or when channels were already in use during a
subsequent NPIV Login. The later is required because channel support is
only renegotiated after a CRQ pair is broken. Simple NPIV Logout/Logins
require the client to continue to advertise the channel capability until
the CRQ pair between the client is broken.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 4 ++++
1 file changed, 4 insertions(+)
New NPIV_ENQUIRY_CHANNEL and NPIV_SETUP_CHANNEL management datagrams
(MADs) were defined in a previous patchset. If the client advertises a
desire to use channels and the partner VIOS is channel capable then the
client must proceed with channel enquiry to determine the maximum number
of channels the VIOS is capable of providing, and registering SubCRQs
via channel setup with the VIOS immediately following NPIV Login. This
handshaking should not be performed for subsequent NPIV Logins unless
the CRQ connection has been reset.
Implement these two new MADs and issue them following a successful NPIV
login where the VIOS has set the SUPPORT_CHANNELS capability bit in the
NPIV Login response.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 135 ++++++++++++++++++++++++++++++++-
drivers/scsi/ibmvscsi/ibmvfc.h | 3 +
2 files changed, 136 insertions(+), 2 deletions(-)
@@ -806,6 +806,8 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)spin_lock_irqsave(vhost->host->host_lock,flags);vhost->state=IBMVFC_NO_CRQ;vhost->logged_in=0;+vhost->do_enquiry=1;+vhost->using_channels=0;/* Clean out the queue */memset(crq->msgs,0,PAGE_SIZE);
Each Sub-CRQ has its own interrupt. A hypercall is required to toggle
the IRQ state. Provide the necessary mechanism via a helper function.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
The logic for iterating over the Sub-CRQ responses is similiar to that
of the primary CRQ. Add the necessary handlers for processing those
responses.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 80 ++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
@@ -3381,6 +3381,86 @@ static int ibmvfc_toggle_scrq_irq(struct ibmvfc_sub_queue *scrq, int enable)returnrc;}+staticvoidibmvfc_handle_scrq(structibmvfc_crq*crq,structibmvfc_host*vhost)+{+structibmvfc_event*evt=(structibmvfc_event*)be64_to_cpu(crq->ioba);+unsignedlongflags;++switch(crq->valid){+caseIBMVFC_CRQ_CMD_RSP:+break;+caseIBMVFC_CRQ_XPORT_EVENT:+return;+default:+dev_err(vhost->dev,"Got and invalid message type 0x%02x\n",crq->valid);+return;+}++/* The only kind of payload CRQs we should get are responses to+*thingswesend.Makesurethisresponseistosomethingwe+*actuallysent+*/+if(unlikely(!ibmvfc_valid_event(&vhost->pool,evt))){+dev_err(vhost->dev,"Returned correlation_token 0x%08llx is invalid!\n",+crq->ioba);+return;+}++if(unlikely(atomic_read(&evt->free))){+dev_err(vhost->dev,"Received duplicate correlation_token 0x%08llx!\n",+crq->ioba);+return;+}++del_timer(&evt->timer);+list_del(&evt->queue);+ibmvfc_trc_end(evt);+spin_unlock_irqrestore(vhost->host->host_lock,flags);+evt->done(evt);+spin_lock_irqsave(vhost->host->host_lock,flags);+}++staticstructibmvfc_crq*ibmvfc_next_scrq(structibmvfc_sub_queue*scrq)+{+structibmvfc_crq*crq;++crq=&scrq->msgs[scrq->cur].crq;+if(crq->valid&0x80){+if(++scrq->cur==scrq->size)+scrq->cur=0;+rmb();+}else+crq=NULL;++returncrq;+}++staticvoidibmvfc_drain_sub_crq(structibmvfc_sub_queue*scrq)+{+structibmvfc_crq*crq;+unsignedlongflags;+intdone=0;++spin_lock_irqsave(scrq->vhost->host->host_lock,flags);+while(!done){+while((crq=ibmvfc_next_scrq(scrq))!=NULL){+ibmvfc_handle_scrq(crq,scrq->vhost);+crq->valid=0;+wmb();+}++ibmvfc_toggle_scrq_irq(scrq,1);+if((crq=ibmvfc_next_scrq(scrq))!=NULL){+ibmvfc_toggle_scrq_irq(scrq,0);+ibmvfc_handle_scrq(crq,scrq->vhost);+crq->valid=0;+wmb();+}else+done=1;+}+spin_unlock_irqrestore(scrq->vhost->host->host_lock,flags);+}+/***ibmvfc_init_tgt-Setthenextinitjobstepforthetarget*@tgt:ibmvfctargetstruct
Sub-CRQs are registred with firmware via a hypercall. Abstract that
interface into a simpler helper function.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
Subordinate Command Response Queues (Sub CRQ) are used in conjunction
with the primary CRQ when more than one queue is needed by the virtual
IO adapter. Recent phyp firmware versions support Sub CRQ's with ibmvfc
adapters. This feature is a prerequisite for supporting multiple
hardware backed submission queues in the vfc adapter.
The Sub CRQ command element differs from the standard CRQ in that it is
32bytes long as opposed to 16bytes for the latter. Despite this extra
16bytes the ibmvfc protocol will use the original CRQ command element
mapped to the first 16bytes of the Sub CRQ element initially.
Add definitions for the Sub CRQ command element and queue.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
Allocate a set of Sub-CRQs in advance. During channel setup the client
and VIOS negotiate the number of queues the VIOS supports and the number
that the client desires to request. Its possible that the final channel
resources allocated is less than requested, but the client is still
responsible for sending handles for every queue it is hoping for.
Also, provide deallocation cleanup routines.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 129 +++++++++++++++++++++++++++++++++
drivers/scsi/ibmvscsi/ibmvfc.h | 1 +
2 files changed, 130 insertions(+)
@@ -793,6 +793,8 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)unsignedlongflags;structvio_dev*vdev=to_vio_dev(vhost->dev);structibmvfc_crq_queue*crq=&vhost->crq;+structibmvfc_sub_queue*scrq;+inti;/* Close the CRQ */do{
@@ -809,6 +811,14 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)memset(crq->msgs,0,PAGE_SIZE);crq->cur=0;+if(vhost->scsi_scrqs.scrqs){+for(i=0;i<IBMVFC_SCSI_HW_QUEUES;i++){+scrq=&vhost->scsi_scrqs.scrqs[i];+memset(scrq->msgs,0,PAGE_SIZE);+scrq->cur=0;+}+}+/* And re-open it again */rc=plpar_hcall_norets(H_REG_CRQ,vdev->unit_address,crq->msg_token,PAGE_SIZE);
@@ -4983,6 +4993,118 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)returnretrc;}+staticintibmvfc_register_scsi_channel(structibmvfc_host*vhost,+intindex)+{+structdevice*dev=vhost->dev;+structvio_dev*vdev=to_vio_dev(dev);+structibmvfc_sub_queue*scrq=&vhost->scsi_scrqs.scrqs[index];+intrc=-ENOMEM;++ENTER;++scrq->msgs=(structibmvfc_sub_crq*)get_zeroed_page(GFP_KERNEL);+if(!scrq->msgs)+returnrc;++scrq->size=PAGE_SIZE/sizeof(*scrq->msgs);+scrq->msg_token=dma_map_single(dev,scrq->msgs,PAGE_SIZE,+DMA_BIDIRECTIONAL);++if(dma_mapping_error(dev,scrq->msg_token))+gotodma_map_failed;++rc=h_reg_sub_crq(vdev->unit_address,scrq->msg_token,PAGE_SIZE,+&scrq->cookie,&scrq->hw_irq);++if(rc){+dev_warn(dev,"Error registering sub-crq: %d\n",rc);+if(rc==H_PARAMETER)+dev_warn_once(dev,"Firmware may not support MQ\n");+gotoreg_failed;+}++scrq->hwq_id=index;+scrq->vhost=vhost;++LEAVE;+return0;++reg_failed:+dma_unmap_single(dev,scrq->msg_token,PAGE_SIZE,DMA_BIDIRECTIONAL);+dma_map_failed:+free_page((unsignedlong)scrq->msgs);+LEAVE;+returnrc;+}++staticvoidibmvfc_deregister_scsi_channel(structibmvfc_host*vhost,intindex)+{+structdevice*dev=vhost->dev;+structvio_dev*vdev=to_vio_dev(dev);+structibmvfc_sub_queue*scrq=&vhost->scsi_scrqs.scrqs[index];+longrc;++ENTER;++do{+rc=plpar_hcall_norets(H_FREE_SUB_CRQ,vdev->unit_address,+scrq->cookie);+}while(rc==H_BUSY||H_IS_LONG_BUSY(rc));++if(rc)+dev_err(dev,"Failed to free sub-crq[%d]: rc=%ld\n",index,rc);++dma_unmap_single(dev,scrq->msg_token,PAGE_SIZE,DMA_BIDIRECTIONAL);+free_page((unsignedlong)scrq->msgs);+LEAVE;+}++staticintibmvfc_init_sub_crqs(structibmvfc_host*vhost)+{+inti,j;++ENTER;++vhost->scsi_scrqs.scrqs=kcalloc(IBMVFC_SCSI_HW_QUEUES,+sizeof(*vhost->scsi_scrqs.scrqs),+GFP_KERNEL);+if(!vhost->scsi_scrqs.scrqs)+return-1;++for(i=0;i<IBMVFC_SCSI_HW_QUEUES;i++){+if(ibmvfc_register_scsi_channel(vhost,i)){+for(j=i;j>0;j--)+ibmvfc_deregister_scsi_channel(vhost,j-1);+kfree(vhost->scsi_scrqs.scrqs);+vhost->scsi_scrqs.scrqs=NULL;+vhost->scsi_scrqs.active_queues=0;+LEAVE;+return-1;+}+}++LEAVE;+return0;+}++staticvoidibmvfc_release_sub_crqs(structibmvfc_host*vhost)+{+inti;++ENTER;+if(!vhost->scsi_scrqs.scrqs)+return;++for(i=0;i<IBMVFC_SCSI_HW_QUEUES;i++)+ibmvfc_deregister_scsi_channel(vhost,i);++kfree(vhost->scsi_scrqs.scrqs);+vhost->scsi_scrqs.scrqs=NULL;+vhost->scsi_scrqs.active_queues=0;+LEAVE;+}+/***ibmvfc_free_mem-Freememoryforvhost*@vhost:ibmvfchoststruct
@@ -5239,6 +5361,12 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)gotoremove_shost;}+if(vhost->mq_enabled){+rc=ibmvfc_init_sub_crqs(vhost);+if(rc)+dev_warn(dev,"Failed to allocate Sub-CRQs. rc=%d\n",rc);+}+if(shost_to_fc_host(shost)->rqst_q)blk_queue_max_segments(shost_to_fc_host(shost)->rqst_q,1);dev_set_drvdata(dev,vhost);
@@ -5296,6 +5424,7 @@ static int ibmvfc_remove(struct vio_dev *vdev)ibmvfc_purge_requests(vhost,DID_ERROR);spin_unlock_irqrestore(vhost->host->host_lock,flags);ibmvfc_free_event_pool(vhost);+ibmvfc_release_sub_crqs(vhost);ibmvfc_free_mem(vhost);spin_lock(&ibmvfc_driver_lock);
Turn on MQ by default and set sane values for the upper limit on hw
queues for the scsi host, and number of hw scsi channels to request from
the partner VIOS.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Reviewed-by: Brian King <redacted>
---
drivers/scsi/ibmvscsi/ibmvfc.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
The legacy CRQ holds the host lock the even while completing commands.
This presents a problem when in legacy single queue mode and
nr_hw_queues is greater than one since calling scsi_done() introduces
the potential for deadlock.
If nr_hw_queues is greater than one drop the hostlock in the legacy CRQ
path when completing a command.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
From: Brian King <hidden> Date: 2020-12-04 14:48:06
On 12/2/20 8:07 PM, Tyrel Datwyler wrote:
quoted hunk
@@ -4983,6 +4993,118 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost) return retrc; }+static int ibmvfc_register_scsi_channel(struct ibmvfc_host *vhost,+ int index)+{+ struct device *dev = vhost->dev;+ struct vio_dev *vdev = to_vio_dev(dev);+ struct ibmvfc_sub_queue *scrq = &vhost->scsi_scrqs.scrqs[index];+ int rc = -ENOMEM;++ ENTER;++ scrq->msgs = (struct ibmvfc_sub_crq *)get_zeroed_page(GFP_KERNEL);+ if (!scrq->msgs)+ return rc;++ scrq->size = PAGE_SIZE / sizeof(*scrq->msgs);+ scrq->msg_token = dma_map_single(dev, scrq->msgs, PAGE_SIZE,+ DMA_BIDIRECTIONAL);++ if (dma_mapping_error(dev, scrq->msg_token))+ goto dma_map_failed;++ rc = h_reg_sub_crq(vdev->unit_address, scrq->msg_token, PAGE_SIZE,+ &scrq->cookie, &scrq->hw_irq);++ if (rc) {+ dev_warn(dev, "Error registering sub-crq: %d\n", rc);+ if (rc == H_PARAMETER)+ dev_warn_once(dev, "Firmware may not support MQ\n");+ goto reg_failed;+ }++ scrq->hwq_id = index;+ scrq->vhost = vhost;++ LEAVE;+ return 0;++reg_failed:+ dma_unmap_single(dev, scrq->msg_token, PAGE_SIZE, DMA_BIDIRECTIONAL);+dma_map_failed:+ free_page((unsigned long)scrq->msgs);+ LEAVE;+ return rc;+}++static void ibmvfc_deregister_scsi_channel(struct ibmvfc_host *vhost, int index)+{+ struct device *dev = vhost->dev;+ struct vio_dev *vdev = to_vio_dev(dev);+ struct ibmvfc_sub_queue *scrq = &vhost->scsi_scrqs.scrqs[index];+ long rc;++ ENTER;++ do {+ rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,+ scrq->cookie);+ } while (rc == H_BUSY || H_IS_LONG_BUSY(rc));++ if (rc)+ dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n", index, rc);++ dma_unmap_single(dev, scrq->msg_token, PAGE_SIZE, DMA_BIDIRECTIONAL);+ free_page((unsigned long)scrq->msgs);+ LEAVE;+}++static int ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)+{+ int i, j;++ ENTER;++ vhost->scsi_scrqs.scrqs = kcalloc(IBMVFC_SCSI_HW_QUEUES,+ sizeof(*vhost->scsi_scrqs.scrqs),+ GFP_KERNEL);+ if (!vhost->scsi_scrqs.scrqs)+ return -1;++ for (i = 0; i < IBMVFC_SCSI_HW_QUEUES; i++) {+ if (ibmvfc_register_scsi_channel(vhost, i)) {+ for (j = i; j > 0; j--)+ ibmvfc_deregister_scsi_channel(vhost, j - 1);+ kfree(vhost->scsi_scrqs.scrqs);+ vhost->scsi_scrqs.scrqs = NULL;+ vhost->scsi_scrqs.active_queues = 0;+ LEAVE;+ return -1;+ }+ }++ LEAVE;+ return 0;+}++static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)+{+ int i;++ ENTER;+ if (!vhost->scsi_scrqs.scrqs)+ return;++ for (i = 0; i < IBMVFC_SCSI_HW_QUEUES; i++)+ ibmvfc_deregister_scsi_channel(vhost, i);++ kfree(vhost->scsi_scrqs.scrqs);+ vhost->scsi_scrqs.scrqs = NULL;+ vhost->scsi_scrqs.active_queues = 0;+ LEAVE;+}+ /** * ibmvfc_free_mem - Free memory for vhost * @vhost: ibmvfc host struct
@@ -5239,6 +5361,12 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id) goto remove_shost; }+ if (vhost->mq_enabled) {+ rc = ibmvfc_init_sub_crqs(vhost);+ if (rc)+ dev_warn(dev, "Failed to allocate Sub-CRQs. rc=%d\n", rc);
So, I think if you end up down this path, you will have:
vhost->scsi_scrqs.scrqs == NULL
vhost->scsi_scrqs.active_queues == 0
And you proceed with discovery. You will proceed with enquiry and channel setup.
Then, I think you could end up in queuecommand doing this:
evt->hwq = hwq % vhost->scsi_scrqs.active_queues;
And that is a divide by zero...
I wonder if it would be better in this scenario where registering the sub crqs fails,
if you just did:
vhost->do_enquiry = 0;
vhost->mq_enabled = 0;
vhost->using_channels = 0;
It looks like you only try to allocate the subcrqs in probe, so if that fails, we'd
never end up using mq, so just disabling in this case seems reasonable.
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
From: Brian King <hidden> Date: 2020-12-04 14:52:46
On 12/2/20 8:07 PM, Tyrel Datwyler wrote:
quoted hunk
The logic for iterating over the Sub-CRQ responses is similiar to that
of the primary CRQ. Add the necessary handlers for processing those
responses.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 80 ++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
@@ -3381,6 +3381,86 @@ static int ibmvfc_toggle_scrq_irq(struct ibmvfc_sub_queue *scrq, int enable)returnrc;}+staticvoidibmvfc_handle_scrq(structibmvfc_crq*crq,structibmvfc_host*vhost)+{+structibmvfc_event*evt=(structibmvfc_event*)be64_to_cpu(crq->ioba);+unsignedlongflags;++switch(crq->valid){+caseIBMVFC_CRQ_CMD_RSP:+break;+caseIBMVFC_CRQ_XPORT_EVENT:+return;+default:+dev_err(vhost->dev,"Got and invalid message type 0x%02x\n",crq->valid);+return;+}++/* The only kind of payload CRQs we should get are responses to+*thingswesend.Makesurethisresponseistosomethingwe+*actuallysent+*/+if(unlikely(!ibmvfc_valid_event(&vhost->pool,evt))){+dev_err(vhost->dev,"Returned correlation_token 0x%08llx is invalid!\n",+crq->ioba);+return;+}++if(unlikely(atomic_read(&evt->free))){+dev_err(vhost->dev,"Received duplicate correlation_token 0x%08llx!\n",+crq->ioba);+return;+}++del_timer(&evt->timer);+list_del(&evt->queue);+ibmvfc_trc_end(evt);>+spin_unlock_irqrestore(vhost->host->host_lock,flags);
You can't do this here... You are grabbing the host lock in ibmvfc_drain_sub_crq
and saving the irqflags to a local in that function, then doing a spin_unlock_irqrestore
and restoring irqflags using an uninitialized local in this function...
I'm assuming this will get sorted out with the locking changes we've been discussing off-list...
From: Brian King <hidden> Date: 2020-12-04 21:28:32
On 12/2/20 8:08 PM, Tyrel Datwyler wrote:
quoted hunk
In general the client needs to send Cancel MADs and task management
commands down the same channel as the command(s) intended to cancel or
abort. The client assigns cancel keys per LUN and thus must send a
Cancel down each channel commands were submitted for that LUN. Further,
the client then must wait for those cancel completions prior to
submitting a LUN RESET or ABORT TASK SET.
Add a cancel event pointer and cancel rsp iu storage to the
ibmvfc_sub_queue struct such that the cancel routine can assign a cancel
event to each applicable queue. When in legacy CRQ mode we fake treating
it as a subqueue by using a subqueue struct allocated on the stack. Wait
for completion of each submitted cancel.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 104 ++++++++++++++++++++++-----------
drivers/scsi/ibmvscsi/ibmvfc.h | 38 ++++++------
2 files changed, 90 insertions(+), 52 deletions(-)
@@ -2339,67 +2339,103 @@ static int ibmvfc_cancel_all(struct scsi_device *sdev, int type){structibmvfc_host*vhost=shost_priv(sdev->host);structibmvfc_event*evt,*found_evt;-unionibmvfc_iursp;-intrsp_rc=-EBUSY;+structibmvfc_sub_queue*scrqs;+structibmvfc_sub_queuelegacy_crq;+intrsp_rc=0;unsignedlongflags;u16status;+intcancel_cnt=0;+intnum_hwq;+intret=0;+inti;ENTER;spin_lock_irqsave(vhost->host->host_lock,flags);-found_evt=NULL;-list_for_each_entry(evt,&vhost->sent,queue){-if(evt->cmnd&&evt->cmnd->device==sdev){-found_evt=evt;+if(vhost->using_channels&&vhost->scsi_scrqs.active_queues){+num_hwq=vhost->scsi_scrqs.active_queues;+scrqs=vhost->scsi_scrqs.scrqs;+}else{+/* Use ibmvfc_sub_queue on the stack to fake legacy CRQ as a subqueue */+num_hwq=1;+scrqs=&legacy_crq;+}++for(i=0;i<num_hwq;i++){+scrqs[i].cancel_event=NULL;+found_evt=NULL;+list_for_each_entry(evt,&vhost->sent,queue){+if(evt->cmnd&&evt->cmnd->device==sdev&&evt->hwq==i){+found_evt=evt;+cancel_cnt++;+break;+}+}++if(!found_evt)+continue;++if(vhost->logged_in){+scrqs[i].cancel_event=ibmvfc_init_tmf(vhost,sdev,type);+scrqs[i].cancel_event->hwq=i;+scrqs[i].cancel_event->sync_iu=&scrqs[i].cancel_rsp;+rsp_rc=ibmvfc_send_event(scrqs[i].cancel_event,vhost,default_timeout);+if(rsp_rc)+break;
It looks like if you have two outstanding commands, on two different hwqs, and you succeed
in sending a cancel for the first hwq but fail sending it for the second hwq due to
something happening like a xport event of some sort, then you would end up falling down
into free_events where you'd call ibmvfc_free_event which will do a list_add_tail to add
the event to the free list without having even pulled the event off the sent list, which
will result in list corruption as now the free list and sent list will be intermingled.
It would probably be better to only free the events if you never sent them or if you
are sure they completed. So, you might need to have to wait for the completion of
the cancel events that did get sent, which would likely be completed via purge_all.
+ } else {
+ rsp_rc = -EBUSY;
break;
}
}
- if (!found_evt) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+
+ if (!cancel_cnt) {
if (vhost->log_level > IBMVFC_DEFAULT_LOG_LEVEL)
sdev_printk(KERN_INFO, sdev, "No events found to cancel\n");
- spin_unlock_irqrestore(vhost->host->host_lock, flags);
return 0;
}
- if (vhost->logged_in) {
- evt = ibmvfc_init_tmf(vhost, sdev, type);
- evt->sync_iu = &rsp;
- rsp_rc = ibmvfc_send_event(evt, vhost, default_timeout);
- }
-
- spin_unlock_irqrestore(vhost->host->host_lock, flags);
-
if (rsp_rc != 0) {
sdev_printk(KERN_ERR, sdev, "Failed to send cancel event. rc=%d\n", rsp_rc);
/* If failure is received, the host adapter is most likely going
through reset, return success so the caller will wait for the command
being cancelled to get returned */
- return 0;
+ goto free_events;
}
sdev_printk(KERN_INFO, sdev, "Cancelling outstanding commands.\n");
- wait_for_completion(&evt->comp);
- status = be16_to_cpu(rsp.mad_common.status);
- spin_lock_irqsave(vhost->host->host_lock, flags);
- ibmvfc_free_event(evt);
- spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ for (i = 0; i < num_hwq; i++) {
+ if (!scrqs[i].cancel_event)
+ continue;
- if (status != IBMVFC_MAD_SUCCESS) {
- sdev_printk(KERN_WARNING, sdev, "Cancel failed with rc=%x\n", status);
- switch (status) {
- case IBMVFC_MAD_DRIVER_FAILED:
- case IBMVFC_MAD_CRQ_ERROR:
- /* Host adapter most likely going through reset, return success to
- the caller will wait for the command being cancelled to get returned */
- return 0;
- default:
- return -EIO;
- };
+ wait_for_completion(&scrqs[i].cancel_event->comp);
+ status = be16_to_cpu(scrqs[i].cancel_rsp.mad_common.status);
+
+ if (status != IBMVFC_MAD_SUCCESS) {
+ sdev_printk(KERN_WARNING, sdev, "Cancel failed with rc=%x\n", status);
+ switch (status) {
+ case IBMVFC_MAD_DRIVER_FAILED:
+ case IBMVFC_MAD_CRQ_ERROR:
+ /* Host adapter most likely going through reset, return success to
+ the caller will wait for the command being cancelled to get returned */
+ goto free_events;
Similar comment here... What about the rest of the outstanding cancel commands? Do you need
to wait for those to complete before freeing them?
+ default:
+ ret = -EIO;
+ goto free_events;
+ };
+ }
}
sdev_printk(KERN_INFO, sdev, "Successfully cancelled outstanding commands\n");
- return 0;
+free_events:
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ for (i = 0; i < num_hwq; i++)
+ if (scrqs[i].cancel_event)
+ ibmvfc_free_event(scrqs[i].cancel_event);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+
+ return ret;
}
/**
--
Brian King
Power Linux I/O
IBM Linux Technology Center
From: Brian King <hidden> Date: 2020-12-04 21:37:33
On 12/2/20 8:08 PM, Tyrel Datwyler wrote:
quoted hunk
The legacy CRQ holds the host lock the even while completing commands.
This presents a problem when in legacy single queue mode and
nr_hw_queues is greater than one since calling scsi_done() introduces
the potential for deadlock.
If nr_hw_queues is greater than one drop the hostlock in the legacy CRQ
path when completing a command.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Similar comment here as previously. The flags parameter is an output for
spin_lock_irqsave but an input for spin_unlock_irqrestore. You'll need
to rethink the locking here. You could just do a spin_unlock_irq / spin_lock_irq
here and that would probably be OK, but probably isn't the best.
}
/**
--
Brian King
Power Linux I/O
IBM Linux Technology Center
@@ -4983,6 +4993,118 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost) return retrc; }+static int ibmvfc_register_scsi_channel(struct ibmvfc_host *vhost,+ int index)+{+ struct device *dev = vhost->dev;+ struct vio_dev *vdev = to_vio_dev(dev);+ struct ibmvfc_sub_queue *scrq = &vhost->scsi_scrqs.scrqs[index];+ int rc = -ENOMEM;++ ENTER;++ scrq->msgs = (struct ibmvfc_sub_crq *)get_zeroed_page(GFP_KERNEL);+ if (!scrq->msgs)+ return rc;++ scrq->size = PAGE_SIZE / sizeof(*scrq->msgs);+ scrq->msg_token = dma_map_single(dev, scrq->msgs, PAGE_SIZE,+ DMA_BIDIRECTIONAL);++ if (dma_mapping_error(dev, scrq->msg_token))+ goto dma_map_failed;++ rc = h_reg_sub_crq(vdev->unit_address, scrq->msg_token, PAGE_SIZE,+ &scrq->cookie, &scrq->hw_irq);++ if (rc) {+ dev_warn(dev, "Error registering sub-crq: %d\n", rc);+ if (rc == H_PARAMETER)+ dev_warn_once(dev, "Firmware may not support MQ\n");+ goto reg_failed;+ }++ scrq->hwq_id = index;+ scrq->vhost = vhost;++ LEAVE;+ return 0;++reg_failed:+ dma_unmap_single(dev, scrq->msg_token, PAGE_SIZE, DMA_BIDIRECTIONAL);+dma_map_failed:+ free_page((unsigned long)scrq->msgs);+ LEAVE;+ return rc;+}++static void ibmvfc_deregister_scsi_channel(struct ibmvfc_host *vhost, int index)+{+ struct device *dev = vhost->dev;+ struct vio_dev *vdev = to_vio_dev(dev);+ struct ibmvfc_sub_queue *scrq = &vhost->scsi_scrqs.scrqs[index];+ long rc;++ ENTER;++ do {+ rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,+ scrq->cookie);+ } while (rc == H_BUSY || H_IS_LONG_BUSY(rc));++ if (rc)+ dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n", index, rc);++ dma_unmap_single(dev, scrq->msg_token, PAGE_SIZE, DMA_BIDIRECTIONAL);+ free_page((unsigned long)scrq->msgs);+ LEAVE;+}++static int ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)+{+ int i, j;++ ENTER;++ vhost->scsi_scrqs.scrqs = kcalloc(IBMVFC_SCSI_HW_QUEUES,+ sizeof(*vhost->scsi_scrqs.scrqs),+ GFP_KERNEL);+ if (!vhost->scsi_scrqs.scrqs)+ return -1;++ for (i = 0; i < IBMVFC_SCSI_HW_QUEUES; i++) {+ if (ibmvfc_register_scsi_channel(vhost, i)) {+ for (j = i; j > 0; j--)+ ibmvfc_deregister_scsi_channel(vhost, j - 1);+ kfree(vhost->scsi_scrqs.scrqs);+ vhost->scsi_scrqs.scrqs = NULL;+ vhost->scsi_scrqs.active_queues = 0;+ LEAVE;+ return -1;+ }+ }++ LEAVE;+ return 0;+}++static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)+{+ int i;++ ENTER;+ if (!vhost->scsi_scrqs.scrqs)+ return;++ for (i = 0; i < IBMVFC_SCSI_HW_QUEUES; i++)+ ibmvfc_deregister_scsi_channel(vhost, i);++ kfree(vhost->scsi_scrqs.scrqs);+ vhost->scsi_scrqs.scrqs = NULL;+ vhost->scsi_scrqs.active_queues = 0;+ LEAVE;+}+ /** * ibmvfc_free_mem - Free memory for vhost * @vhost: ibmvfc host struct
@@ -5239,6 +5361,12 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id) goto remove_shost; }+ if (vhost->mq_enabled) {+ rc = ibmvfc_init_sub_crqs(vhost);+ if (rc)+ dev_warn(dev, "Failed to allocate Sub-CRQs. rc=%d\n", rc);
So, I think if you end up down this path, you will have:
vhost->scsi_scrqs.scrqs == NULL
vhost->scsi_scrqs.active_queues == 0
And you proceed with discovery. You will proceed with enquiry and channel setup.
Then, I think you could end up in queuecommand doing this
evt->hwq = hwq % vhost->scsi_scrqs.active_queues;
And that is a divide by zero...
Actually, we would bite the dust earlier than that but it requires the sub-crq
allocation to fail for a reason other than lack of firmware support. In the no
firmware support case the VIOS doesn't report channel support and we skip the
enquiry and setup steps. However, in the case where there is support and
allocation fails we would dereference a NULL pointer trying to write the channel
sub-crq handles into the channel_setup MAD.
I wonder if it would be better in this scenario where registering the sub crqs fails,
if you just did:
vhost->do_enquiry = 0;
vhost->mq_enabled = 0;
vhost->using_channels = 0;
It looks like you only try to allocate the subcrqs in probe, so if that fails, we'd
never end up using mq, so just disabling in this case seems reasonable.
This breaks migration from legacy to a target with channel support. It appears
that migration for that case is already broken anyways. Need to rethink sub-crq
setup. Maybe best to actually do it during the negoation steps instead of in probe.
-Tyrel
The logic for iterating over the Sub-CRQ responses is similiar to that
of the primary CRQ. Add the necessary handlers for processing those
responses.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 80 ++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)
@@ -3381,6 +3381,86 @@ static int ibmvfc_toggle_scrq_irq(struct ibmvfc_sub_queue *scrq, int enable)returnrc;}+staticvoidibmvfc_handle_scrq(structibmvfc_crq*crq,structibmvfc_host*vhost)+{+structibmvfc_event*evt=(structibmvfc_event*)be64_to_cpu(crq->ioba);+unsignedlongflags;++switch(crq->valid){+caseIBMVFC_CRQ_CMD_RSP:+break;+caseIBMVFC_CRQ_XPORT_EVENT:+return;+default:+dev_err(vhost->dev,"Got and invalid message type 0x%02x\n",crq->valid);+return;+}++/* The only kind of payload CRQs we should get are responses to+*thingswesend.Makesurethisresponseistosomethingwe+*actuallysent+*/+if(unlikely(!ibmvfc_valid_event(&vhost->pool,evt))){+dev_err(vhost->dev,"Returned correlation_token 0x%08llx is invalid!\n",+crq->ioba);+return;+}++if(unlikely(atomic_read(&evt->free))){+dev_err(vhost->dev,"Received duplicate correlation_token 0x%08llx!\n",+crq->ioba);+return;+}++del_timer(&evt->timer);+list_del(&evt->queue);+ibmvfc_trc_end(evt);>+spin_unlock_irqrestore(vhost->host->host_lock,flags);
You can't do this here... You are grabbing the host lock in ibmvfc_drain_sub_crq
and saving the irqflags to a local in that function, then doing a spin_unlock_irqrestore
and restoring irqflags using an uninitialized local in this function...
I'm assuming this will get sorted out with the locking changes we've been discussing off-list...
Correct, moving to per-queue locks and flags stored in the queue struct.
-Tyrel
The legacy CRQ holds the host lock the even while completing commands.
This presents a problem when in legacy single queue mode and
nr_hw_queues is greater than one since calling scsi_done() introduces
the potential for deadlock.
If nr_hw_queues is greater than one drop the hostlock in the legacy CRQ
path when completing a command.
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Similar comment here as previously. The flags parameter is an output for
spin_lock_irqsave but an input for spin_unlock_irqrestore. You'll need
to rethink the locking here. You could just do a spin_unlock_irq / spin_lock_irq
here and that would probably be OK, but probably isn't the best.
Yeah, this will also get its own lock and flags saved in the per-queue struct in
the next spin.
-Tyrel