Re: CVS commit: src/sys/dev/ic

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

   Date: Thu, 10 Apr 2014 10:22:14 -0700
   From: Brian Buhrow <buhrow%nfbcal.org@localhost>

           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 */