Hi Brian,
Thanks for reviewing. Comments inline below.
-matt
On Jul 24, 2015, at 3:15 PM, Brian King wrote:
> On 07/16/2015 06:26 PM, Matthew R. Ochs wrote:
>> +
>> +/**
>> + * 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
>> + */
>> +static int ba_clone(struct ba_lun *ba_lun, u64 to_clone)
>> +{
>> + struct ba_lun_info *lun_info =
>> + (struct ba_lun_info *)ba_lun->ba_lun_handle;
>> +
>> + if (validate_alloc(lun_info, to_clone)) {
>> + pr_err("%s: AUN %llX is not allocated on lun_id %llX\n",
>> + __func__, to_clone, ba_lun->lun_id);
>
> Suggest using dev_err here instead to scope the error to the adapter.
Sure, we plan on transitioning to dev_* prints where it make sense. This routine
in particular will likely not be transitioned, but we can add a dev print in the
outer caller to track the device.
>
>> + return -1;
>
> Might be nice to return a better error to the user in both of the error cases
> in this code.
We can look at doing this although in this particular case I'm not sure it would
help things much from a user perspective. These failures are more indicative
of an internal driver bug and not one a user would be all that interested in.
>> +
>> + rc = ba_init(&blka->ba_lun);
>> + if (rc) {
>> + pr_err("%s: cannot init block_alloc, rc=%d\n", __func__, rc);
>> + goto init_ba_exit;
>
> The goto here is unnecessary
We'll remove it.
>> +
>> + result = scsi_execute(sdev, scsi_cmd, DMA_TO_DEVICE, cmd_buf,
>> + CMD_BUFSIZE, sense_buf,
>> + (MC_DISCOVERY_TIMEOUT*HZ), 5, 0, NULL);
>
> 5 seconds seems a little on the short side for this command. Perhaps using
> sdev->request_queue->rq_timeout would be better?
Sure, we'll look into using the rq_timeout value.
>
>> +
>> + if (result) {
>> + pr_err("%s: command failed for offset %lld"
>> + " result=0x%x\n", __func__, offset, result);
>> + rc = -EIO;
>> + goto out;
>> + }
>> + }
>> +
>> +out:
>
> You need to free cmd_buf and sense_buf here.
Good catch. We just fixed this yesterday.
>> + nsectors = (resize->req_size * CXLFLASH_BLOCK_SIZE) / gli->blk_len;
>> + new_size = (nsectors + MC_CHUNK_SIZE - 1) / MC_CHUNK_SIZE;
>
> Use DIV_ROUND_UP instead.
Will do.
>> + lli->lun_index = cfg->promote_lun_index;
>> + writeq_be(lli->lun_id[0],
>> + &afu->afu_map->global.fc_port[0]
>> + [cfg->promote_lun_index]);
>
> Would improve readability IMHO if you either put the second parm all on one
> line or used a local variable to make it better fit on one line, rather than
> line breaking where you did.
I agree. We've been doing this to other places in the driver as well for
the same reason. Will look at fixing this one too as part of v3.
>
>> + writeq_be(lli->lun_id[1],
>> + &afu->afu_map->global.fc_port[1]
>> + [cfg->promote_lun_index]);
>> + cfg->promote_lun_index++;
>> + pr_debug("%s: Virtual LUN on slot %d id0=%llx, id1=%llx\n",
>> + __func__, lli->lun_index, lli->lun_id[0],
>> + lli->lun_id[1]);
>
> Suggest changing the pr_debug calls to dev_dbg calls where possible to scope
> the messages to an adapter.
Same response as earlier. We'll look at refactoring all of the pr_*'s to
dev_*'s where
possible and makes sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
