Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 09/13] ibmvfc: implement channel enquiry and setup
 commands
To:     Brian King <brking@linux.vnet.ibm.com>,
        james.bottomley@hansenpartnership.com
Cc:     martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        brking@linux.ibm.com
References: <20201126014824.123831-1-tyreld@linux.ibm.com>
 <20201126014824.123831-10-tyreld@linux.ibm.com>
 <5f873855-fdc2-4da4-a516-4db7b5236a48@linux.vnet.ibm.com>
From:   Tyrel Datwyler <tyreld@linux.ibm.com>
Message-ID: <c2f01390-8977-dee7-8f33-fe1ebb2b73b7@linux.ibm.com>
Date:   Mon, 30 Nov 2020 09:29:15 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <5f873855-fdc2-4da4-a516-4db7b5236a48@linux.vnet.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/27/20 9:49 AM, Brian King wrote:
> On 11/25/20 7:48 PM, Tyrel Datwyler wrote:
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> 
>> @@ -4462,6 +4464,118 @@ static void ibmvfc_discover_targets(struct ibmvfc_host *vhost)
>>  		ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
>>  }
>>  
>> +static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>> +{
>> +	struct ibmvfc_host *vhost = evt->vhost;
>> +	u32 mad_status = be16_to_cpu(evt->xfer_iu->channel_setup.common.status);
>> +	int level = IBMVFC_DEFAULT_LOG_LEVEL;
>> +
>> +	ibmvfc_free_event(evt);
>> +
>> +	switch (mad_status) {
>> +	case IBMVFC_MAD_SUCCESS:
>> +		ibmvfc_dbg(vhost, "Channel Setup succeded\n");
>> +		vhost->do_enquiry = 0;
>> +		break;
>> +	case IBMVFC_MAD_FAILED:
>> +		level += ibmvfc_retry_host_init(vhost);
>> +		ibmvfc_log(vhost, level, "Channel Setup failed\n");
>> +		fallthrough;
>> +	case IBMVFC_MAD_DRIVER_FAILED:
>> +		return;
>> +	default:
>> +		dev_err(vhost->dev, "Invalid Channel Setup response: 0x%x\n",
>> +			mad_status);
>> +		ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
>> +		return;
>> +	}
>> +
>> +	ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
>> +	wake_up(&vhost->work_wait_q);
>> +}
>> +
>> +static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>> +{
>> +	struct ibmvfc_channel_setup_mad *mad;
>> +	struct ibmvfc_channel_setup *setup_buf = vhost->channel_setup_buf;
>> +	struct ibmvfc_event *evt = ibmvfc_get_event(vhost);
>> +
>> +	memset(setup_buf, 0, sizeof(*setup_buf));
>> +	setup_buf->flags = cpu_to_be32(IBMVFC_CANCEL_CHANNELS);
>> +
>> +	ibmvfc_init_event(evt, ibmvfc_channel_setup_done, IBMVFC_MAD_FORMAT);
>> +	mad = &evt->iu.channel_setup;
>> +	memset(mad, 0, sizeof(*mad));
>> +	mad->common.version = cpu_to_be32(1);
>> +	mad->common.opcode = cpu_to_be32(IBMVFC_CHANNEL_SETUP);
>> +	mad->common.length = cpu_to_be16(sizeof(*mad));
>> +	mad->buffer.va = cpu_to_be64(vhost->channel_setup_dma);
>> +	mad->buffer.len = cpu_to_be32(sizeof(*vhost->channel_setup_buf));
>> +
>> +	ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_INIT_WAIT);
>> +
>> +	if (!ibmvfc_send_event(evt, vhost, default_timeout))
>> +		ibmvfc_dbg(vhost, "Sent channel setup\n");
>> +	else
>> +		ibmvfc_link_down(vhost, IBMVFC_LINK_DOWN);
>> +}
>> +
>> +static void ibmvfc_channel_enquiry_done(struct ibmvfc_event *evt)
>> +{
>> +	struct ibmvfc_host *vhost = evt->vhost;
>> +	struct ibmvfc_channel_enquiry *rsp = &evt->xfer_iu->channel_enquiry;
>> +	u32 mad_status = be16_to_cpu(rsp->common.status);
>> +	int level = IBMVFC_DEFAULT_LOG_LEVEL;
>> +
>> +	switch (mad_status) {
>> +	case IBMVFC_MAD_SUCCESS:
>> +		ibmvfc_dbg(vhost, "Channel Enquiry succeeded\n");
>> +		vhost->max_vios_scsi_channels = be32_to_cpu(rsp->num_scsi_subq_channels);
> 
> You need a ibmvfc_free_event(evt) here so you don't leak events.
> 

Indeed

>> +		break;
>> +	case IBMVFC_MAD_FAILED:
>> +		level += ibmvfc_retry_host_init(vhost);
>> +		ibmvfc_log(vhost, level, "Channel Enquiry failed\n");
>> +		ibmvfc_free_event(evt);
> 
> Looks like you are freeing this event twice due to the fallthrough...

Good catch

> 
>> +		fallthrough;
>> +	case IBMVFC_MAD_DRIVER_FAILED:
>> +		ibmvfc_free_event(evt);
>> +		return;
>> +	default:
>> +		dev_err(vhost->dev, "Invalid Channel Enquiry response: 0x%x\n",
>> +			mad_status);
>> +		ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
>> +		ibmvfc_free_event(evt);
>> +		return;
>> +	}
>> +
>> +	ibmvfc_channel_setup(vhost);
>> +}
>> +
> 
> 
>