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 <bryantly@xxxxxxxxxxxxxxxxxx> > +M: Michael Cyr <mikecyr@xxxxxxxxxxxxxxxxxx> > +L: linux-scsi@xxxxxxxxxxxxxxx > +L: target-devel@xxxxxxxxxxxxxxx > +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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
