On Thu, 2016-06-16 at 16:20 -0500, Bryant G. Ly wrote:
> This driver is a pick up of the old IBM VIO scsi Target Driver
> that was started by Nick and Fujita 2-4 years ago.
> http://comments.gmane.org/gmane.linux.scsi/90119
(style trivia only, nothing important enough to force a respin
but nice to fix one day)
Please use git format-patch -M so file renames are more obvious.
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> +IBM Power Virtual SCSI Device Target Driver
> +M: Bryant G. Ly <bryan...@linux.vnet.ibm.com>
> +M: Michael Cyr <mike...@linux.vnet.ibm.com>
> +L: linux-scsi@vger.kernel.org
> +L: target-de...@vger.kernel.org
> +S: Supportedybe
> +F: drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
Maybe
F: drivers/scsi/ibmscsi_tgt/
> +F: drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
> +F: drivers/scsi/ibmvscsi_tgt/libsrp.c
> +F: drivers/scsi/ibmvscsi_tgt/libsrp.h
and these become unnecessary
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
[]
> +/**
> + * ibmvscsis_establish_new_q() - Establish new CRQ queue
> + */
If you use kernel-doc comment style,
please describe the function arguments too.
> +static long ibmvscsis_establish_new_q(struct scsi_info *vscsi, uint
> new_state)
> +{
> + long rc = ADAPT_SUCCESS;
> + uint format;
> +
> + vscsi->flags &= PRESERVE_FLAG_FIELDS;
> + vscsi->rsp_q_timer.timer_pops = 0;
> + vscsi->debit = 0;
> + vscsi->credit = 0;
> + rc = vio_enable_interrupts(vscsi->dma_dev);
> + if (rc) {
> + pr_warn("reset_queue: failed to enable interrupts, rc %ld\n",
> + rc);
> + } else {
> + rc = ibmvscsis_check_init_msg(vscsi, &format);
> + if (rc) {
> + dev_err(&vscsi->dev, "reset_queue: check_init_msg
> failed, rc %ld\n",
> + rc);
> + } else if (format == UNUSED_FORMAT &&
> + new_state == WAIT_CONNECTION) {
> + rc = ibmvscsis_send_init_message(vscsi, INIT_MSG);
> + switch (rc) {
> + case H_SUCCESS:
> + case H_DROPPED:
> + case H_CLOSED:
> + rc = ADAPT_SUCCESS;
> + break;
> +
> + case H_PARAMETER:
> + case H_HARDWARE:
> + break;
> +
> + default:
> + vscsi->state = UNDEFINED;
> + rc = H_HARDWARE;
> + break;
> + }
> + }
> + }
> +
> + return rc;
> +}
This sort of code can have indent reduced by doing
rc = vio_enable_interrupts(...)
if (rc) {
pr_warn(...);
return rc;
}
rc = ibmvscsis_check_init_msg(...)
if (rc) {
dev_err(...);
return rc;
}
if (format == UNUSED_FORMAT && new_state == WAIT_CONNECTION) {
rc = ibmvscsis_send_init_message(vscsi, INIT_MSG);
switch (rc) {
etc...
}
}
Why use pr_warn and dev_err in the same function?
Shouldn't dev_<level> be used whenever a struct device is available?
Also it's nice to use %s, __func__ instead of directly
embedding a function like name into the format string.
A lot of the pr_debug uses seem to be function tracing
and could be eliminated altogether.
> + /* can transition from this state to UNCONFIGURING */
> + case UNDEFINED:
Comment style for switch / case labels could not be indented
but start at the same indent level as the case statement.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
