On 12/2/20 10:27 AM, Brian King wrote:
> On 12/1/20 6:53 PM, Tyrel Datwyler wrote:
>> 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.
>>
>> Allocate event pointers for each possible scsi channel and assign an
>> event for each channel that requires a cancel. Wait for completion each
>> submitted cancel.
>>
>> Signed-off-by: Tyrel Datwyler <tyreld@xxxxxxxxxxxxx>
>> ---
>> drivers/scsi/ibmvscsi/ibmvfc.c | 106 +++++++++++++++++++++------------
>> 1 file changed, 68 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index 0b6284020f06..97e8eed04b01 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -2339,32 +2339,52 @@ static int ibmvfc_cancel_all(struct scsi_device *sdev, int type)
>> {
>> struct ibmvfc_host *vhost = shost_priv(sdev->host);
>> struct ibmvfc_event *evt, *found_evt;
>> - union ibmvfc_iu rsp;
>> - int rsp_rc = -EBUSY;
>> + struct ibmvfc_event **evt_list;
>> + union ibmvfc_iu *rsp;
>> + int rsp_rc = 0;
>> unsigned long flags;
>> u16 status;
>> + int num_hwq = 1;
>> + int i;
>> + int ret = 0;
>>
>> 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;
>> - break;
>> + if (vhost->using_channels && vhost->scsi_scrqs.active_queues)
>> + num_hwq = vhost->scsi_scrqs.active_queues;
>> +
>> + evt_list = kcalloc(num_hwq, sizeof(*evt_list), GFP_KERNE> + rsp = kcalloc(num_hwq, sizeof(*rsp), GFP_KERNEL);
>
> Can't this just go on the stack? We don't want to be allocating memory
> during error recovery. Or, alternatively, you could put this in the
> vhost structure and protect it with a mutex. We only have enough events
> to single thread these anyway.
Yes, this could just go on the stack.
>
>> +
>> + for (i = 0; i < num_hwq; i++) {
>> + sdev_printk(KERN_INFO, sdev, "Cancelling outstanding commands on queue %d.\n", i);
>
> Prior to this patch, if there was nothing outstanding to the device and cancel_all was called,
> no messages would get printed. This is changing that behavior. Is that intentional? Additionally,
> it looks like this will get a lot more vebose, logging a message for each hw queue, regardless
> of whether there was anything outstanding. Perhaps you want to move this down to after the check
> for !found_evt?
It would actually print "no commands found to cancel". I think its fair to make
it less verbose or at least make them dbg output for each queue.
-Tyrel
>
>> +
>> + found_evt = NULL;
>> + list_for_each_entry(evt, &vhost->sent, queue) {
>> + if (evt->cmnd && evt->cmnd->device == sdev && evt->hwq == i) {
>> + found_evt = evt;
>> + break;
>> + }
>> }
>> - }
>>
>
>
>
