Date: Thu, 10 Apr 2014 10:22:14 -0700
From: Brian Buhrow <buh...@nfbcal.org>
hello. Yes, if you could be more specific about what you're asking
for, I'll be happy to make the fixes.
Here are the ones I see. Most of these are indentation issues. I
know these all look like minor nits, but maintaining a consistent
style is important for quickly eyeballing mistakes (like `goto fail')
and avoiding visual dissonance while following code.
> --- sys/dev/ic/mpt_netbsd.c
> +++ sys/dev/ic/mpt_netbsd.c
> ...
> static void mpt_scsipi_request(struct scsipi_channel *,
> scsipi_adapter_req_t, void *);
> static void mpt_minphys(struct buf *);
> +static int mpt_ioctl(struct scsipi_channel *, u_long, void *, int,
> + struct proc *);
Last line should be indented like the mpt_scsipi_request continuation
line above: four columns past the function name.
> +/*Save the output of the config so we can rescan the bus in case of errors*/
> + mpt->sc_scsibus_dv = config_found(mpt->sc_dev, &mpt->sc_channel,
> scsiprint);
Comment should be indented with the code and have spaces around the
text, and lines should wrap at 80 lines:
/*
* Save the output of the config so we can rescan the bus in
* case of errors.
*/
mpt->sc_scsibus_dv = config_found(mpt->sc_dev, &mpt->sc_channel,
scsiprint);
(In this case, the comment is long enough it should be a multiline
comment.)
> - }
> +nrepl = mpt_drain_queue(mpt);
> return (nrepl != 0);
> }
The `nrepl =' line should be indented. It is also not necessary --
you can omit the nrepl variable altogether and just do `return
mpt_drain_queue(mpt) != 0;'.
> + int s, nrepl = 0;
> +
> +if (req->xfer == NULL) {
> + printf("mpt_timeout: NULL xfer for request index 0x%x,
> sequenc 0x%x\n",
Again, indent, and break lines. Also omit the extra intertoken space:
if (req->xfer == NULL) {
printf("mpt_timeout: NULL xfer for request index 0x%x,"
" sequence 0x%x\n", req->index, seq->sequence);
return;
}
Long strings can be split across lines with cpp concatenation.
> @@ -373,11 +373,28 @@ mpt_timeout(void *arg)
> mpt->timeouts++;
> if (mpt_intr(mpt)) {
> if (req->sequence != oseq) {
> + mpt->success ++;
No need for space in `mpt->success++;'.
> + /*
> + *Ensure the IOC is really done giving us data since it appears it can
> + *sometimes fail to give us interrupts under heavy load.
> + */
Space after `*' in comments.
> + if (nrepl ) {
No need for space here.
> + mpt->success ++;
Same.
> + /* don't try a hard reset since this mangles the PCI
> configuration registers */
Multiline comment (and prefer normal punctuation rules).
> + /* don't really need to mpt_free_request() since
> mpt_init() below will free all requests anyway */
Same.
> +static
> +int mpt_drain_queue(mpt_softc_t *mpt)
Function name at beginning of line:
static int
mpt_drain_queue(mpt_softc_t *mpt)
> + int restart = 0; /*nonzero if we need to restart the IOC*/
/* nonzero if we need to start the IOC */
> - switch (le16toh(mpt_reply->IOCStatus)) {
> + switch ((le16toh(mpt_reply->IOCStatus) & MPI_IOCSTATUS_MASK)) {
Omit needless parentheses.
> + /*
> + *FreeBSD and Linux indicate this is a phase error between
> + *the IOC and the drive itself.
> + *When this happens, the IOC becomes unhappy and stops
> processing
> + *all transactions. Call mpt_timeout which knows how to
> + *get the IOC back on its feet.
> + */
Fill paragraph, match indentation, space between `*' and text:
/*
* FreeBSD and Linux indicate this is a phase error
* between the IOC and the drive itself. When this
* happens, the IOC becomes unhappy and stops
* processing all transactions. Call mpt_timeout
* which knows how to get the IOC back on its feet.
*/
> + mpt_prt(mpt,"mpt_done: IOC indicates protocol error --
> recovering...");
> + xs->error = XS_TIMEOUT;
Match indentation.
> + if (le16toh(mpt_reply->IOCStatus) &
> MPI_IOCSTATUS_FLAG_LOG_INFO_AVAILABLE) {
Split long line.
> + /*mpt_enable_ints(mpt);*/
Don't comment out code; if anything, `#if notyet' or `#if 0' it.
> + return(0);
> + default:
> + return (ENOTTY);
Omit needless parentheses around a simple return operand.
> --- sys/dev/ic/mpt_netbsd.h
> +++ sys/dev/ic/mpt_netbsd.h
> ...
> + device_t sc_scsibus_dv; /*So we can rescan in case of errors*/
/* So we can rescan in case of errors */
