On 02/27/2018 04:18 PM, Kuzeja, William wrote:
>
> Thanks for the quick reply and analysis Hannes! My apologies in advance, I'm stuck on
> Outlook here at work - I'll try to format this to be readable (hopefully it doesn't get
> mangled).
>
> On 02/27/2018 06:01 AM, Hannes Reinecke wrote:
>
>> Hmm. Isn't is rather the case that the labels and gotos are mixed up?
>> We have this order of labels:
>
>> probe_init_failed
>> probe_failed
>> probe_hw_failed
>> iospace_config_failed
>> disable_device
>
>> but this potential calling order:
>
>> disable_device
>> iospace_config_failed
>> probe_hw_failed
>> probe_hw_failed
>> probe_init_failed
>> probe_failed
>> probe_failed
>> probe_failed
>
>> So it looks to me as if the 'probe_init_failed' and 'probe_failed'
>> labels should be exchanged...
>> But yeah, we need to fix this up.
>> I've run into this issue myself recently :-(
>
> So, that is partly the problem. But.....if we switch the order, we still have problems.
> Consider the code with order switched:
>
> probe_failed:
> ....
> qla2x00_free_device(base_vha);
> scsi_host_put(base_vha->host);
>
> probe_init_failed:
> qla2x00_free_req_que(ha, req);
> ha->req_q_map[0] = NULL; alloc_queues succeeds
> 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_hw_failed:
> qla2x00_mem_free(ha);
> qla2x00_free_req_que(ha, req);
> qla2x00_free_rsp_que(ha, rsp);
> qla2x00_clear_drv_active(ha);
>
> 1) We call probe_init_failed only if qla2x00_alloc_queues fails. But if this routine fails,
> ha->req_q_map = NULL (as well as rsp_q_map), so we crash doing
> ha->req_q_map[0] = NULL. So, I can remove these assignments. But the
> qla2x00_free_req_que calls are redundant (they are done below), so we're left
> with clearing out max_req_queues and max_rsp_queues for this label. Since
> we're freeing ha anyways, I'm not sure if this is worth having another label for.
>
> 2) We still have the problem where in probe_failed, qla2x00_free_device does
> frees which are repeated later on in probe_hw_failed.
>
> qla2x00_free_device -> qla2x00_mem_free
> qla2x00_free_device -> qla2x00_free_queues -> qla2x00_free_req_que
>
> 3) Also, I still need another label for when qla2x00_mem_alloc fails. We can't just
> jump to probe_hw_failed.
>
> I suppose we could do something like this:
>
> probe_failed:
> probe_failed = 1;
> ....
> qla2x00_free_device(base_vha);
> scsi_host_put(base_vha->host);
>
> probe_init_failed:
> ha->max_req_queues = ha->max_rsp_queues = 0;
>
> probe_hw_failed:
> if (!probe_failed) {
> qla2x00_mem_free(ha);
> qla2x00_free_req_que(ha, req);
> qla2x00_free_rsp_que(ha, rsp);
> }
Why not make all of these idempotent, so that they won't crash when
called twice? Then we can drop the 'probe_failed' marker, and the entire
code becomes more readable.
Plus the diff will be smaller, too...
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +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)
