On 12/5/16, 8:20 AM, "Christoph Hellwig" <h...@infradead.org> 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. 
>

Reply via email to