>>>>> "Sreekanth" == Sreekanth Reddy <sreekanth.re...@avagotech.com> writes:

Sreekanth,

It's fine that you outline the 96 / 12 = 8 layout in the patch
description. But that relationship is not made clear when reading the
code. Please add a comment describing why things are set up this way.

> @@ -1009,8 +1009,15 @@ _base_interrupt(int irq, void *bus_id)
>       }
>  
>       wmb();
> -     writel(reply_q->reply_post_host_index | (msix_index <<
> -         MPI2_RPHI_MSIX_INDEX_SHIFT), &ioc->chip->ReplyPostHostIndex);
> +     if (ioc->msix96_vector) {
> +             writel(reply_q->reply_post_host_index | ((msix_index  & 7) <<
> +                     MPI2_RPHI_MSIX_INDEX_SHIFT),
> +                             ioc->replyPostRegisterIndex[msix_index/8]);
> +     } else {
> +             writel(reply_q->reply_post_host_index | (msix_index <<
> +                     MPI2_RPHI_MSIX_INDEX_SHIFT),
> +                             &ioc->chip->ReplyPostHostIndex);
> +     }

Too many brackets. Why don't you do:

        index = reply_q->reply_post_host_index |
                ((msix_index & 7) << MPI_RPHI_MSIX_INDEX_SHIFT);

        if (ioc->msix96_vector)
                writel(index, ioc->replyPostRegisterIndex[msix_index / 8]);
        else
                writel(index, &ioc->chip->ReplyPostHostIndex);

> +     if (ioc->msix96_vector && ioc->reply_queue_count > 8) {
> +             /* If this is an 96 vector supported device,
> +             set up ReplyPostIndex addresses */

Bad comment formatting.

> +             ioc->replyPostRegisterIndex = kcalloc(12,
> +                     sizeof(resource_size_t *), GFP_KERNEL);
[...]
> +             for (i = 0; i < 12; i++) {

Make 12 a constant or at the very least a variable with a comment.

> +                     ioc->replyPostRegisterIndex[i] = (resource_size_t *)
> +                             ((u8 *)&ioc->chip->Doorbell +
> +                             MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
> +                             (i * 0x10));

0x10 - Another magic constant.

> @@ -4522,8 +4554,16 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER 
> *ioc, int sleep_flag)
>  
>       /* initialize reply post host index */
>       list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
> -             writel(reply_q->msix_index << MPI2_RPHI_MSIX_INDEX_SHIFT,
> -                 &ioc->chip->ReplyPostHostIndex);
> +             if (ioc->msix96_vector) {
> +                     writel((reply_q->msix_index & 7)<<
> +                        MPI2_RPHI_MSIX_INDEX_SHIFT,
> +                        ioc->replyPostRegisterIndex[reply_q->msix_index/8]);
> +             } else {
> +                     writel(reply_q->msix_index <<
> +                             MPI2_RPHI_MSIX_INDEX_SHIFT,
> +                                     &ioc->chip->ReplyPostHostIndex);
> +             }
> +

Too many brackets.

-- 
Martin K. Petersen      Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Reply via email to