Re: [PATCH v2 3/3] cxlflash: Virtual LUN support

[Manoj Kumar <manoj@linux.vnet.ibm.com>]

Wendy:

Thanks for taking the time to review this patch. Comments inline below.

- Manoj Kumar

On 7/29/2015 5:13 PM, wenxiong@linux.vnet.ibm.com wrote:
>> +    /* Update the free_curr_idx */
>> +    if (bit_pos == 63)
>> +        lun_info->free_curr_idx = bit_word + 1;
>
> Predefined Macros for 63 and 64?

Good point. We will add definitions to indicate that the bit_word is 8 
bytes.

>> +/**
>> + * ba_clone() - frees a block from the block allocator
>> + * @ba_lun:    Block allocator from which to allocate a block.
>> + * @to_free:    Block to free.
>> + *
>> + * Return: 0 on success, -1 on failure
>> + */
>
> More accurate description about ba_clone() function.

Good catch. Will correct in the next version of this patch (v3).

>> +    rc = ba_init(&blka->ba_lun);
>
> init_ba() and ba_init(). Probably one of them needs more accurate name.

Agreed. We will disambiguate in v3.

> free cmd_buf and sense_buf?

Same issue that Brian had pointed out. We had already corrected earlier. 
You will see it in our next submission.

>> +    mutex_unlock(&blka->mutex);
>> +
>
> Should hold the lock for lightwight sync?
>
>> +    /*
>> +     * The following sequence is prescribed in the SISlite spec
>> +     * for syncing up with the AFU when adding LXT entries.
>> +     */
>> +    dma_wmb(); /* Make LXT updates are visible */
>> +
>> +    rhte->lxt_start = lxt;
>> +    dma_wmb(); /* Make RHT entry's LXT table update visible */
>> +
>> +    rhte->lxt_cnt = my_new_size;
>> +    dma_wmb(); /* Make RHT entry's LXT table size update visible */
>> +
>> +    cxlflash_afu_sync(afu, ctxid, rhndl, AFU_LW_SYNC);
>> +

cxlflash_afu_sync() does ensure that only one of these SYNC commands is 
outstanding at one time. No additional serialization is required.

> Should hold the lock for lightwight sync?
>
>> +    cxlflash_afu_sync(afu, ctxid, rhndl, AFU_HW_SYNC);

Same issue as the one discussed above. No additional serialization 
should be necessary.

>> +        /* Setup the LUN table on the first call */
>> +        rc = init_lun_table(cfg, lli);
>> +        if (rc) {
>> +            pr_err("%s: call to init_lun_table failed rc=%d!\n",
>> +                   __func__, rc);
>> +            goto out;
>> +        }
>> +
>> +        rc = init_ba(lli);
>> +        if (rc) {
>> +            pr_err("%s: call to init_ba failed rc=%d!\n",
>> +                   __func__, rc);
>> +            rc = -ENOMEM;
>
> Do you need to remove the entry you create in init_lun_table() if
> init_ba() fails?

The LUN table is global to the adapter. If there are two threads 
creating two virtual LUNs concurrently, the first one inserts the LUN 
into the table. Cannot have that table entry be deleted, even if 
init_ba() fails, as the other thread could be using it.