> >> +
> >> +MODULE_LICENSE ("GPL");
> >> +MODULE_VERSION (MEGASAS_VERSION);
> >> +MODULE_AUTHOR ("sreenivas.bagalkote@xxxxxxxx");
> >> +MODULE_DESCRIPTION ("LSI Logic MegaRAID SAS Driver");
> >
> >Normal style would be:
> >
> >MODULE_LICENSE("GPL");
> >MODULE_VERSION(MEGASAS_VERSION);
> >MODULE_AUTHOR("sreenivas.bagalkote@xxxxxxxx");
> >MODULE_DESCRIPTION("LSI Logic MegaRAID SAS Driver");
> >
>
> I will remove the alignment. I am curious however, to know
> why this is less readable or against the normal style.
I find it more readable. And apparently many core kernel hackers thing
the same.
> I will change the * placement. But does the alignment (of data types and
> variables) offend Linux coding style? If it does, I will remove that.
If you look at core code we generally don't do it. It's not a hard
requirement, but it makes reading the driver and thus finding and fixing
bugs much easier for core developers.
> >> +static int
> >> +megasas_issue_polled(struct megasas_instance* instance,
> >struct megasas_cmd*
> >> cmd)
> >> +{
> >> + int i;
> >> + u32 msecs = MFI_POLL_TIMEOUT_SECS * 1000;
> >> +
> >> + struct megasas_header* frame_hdr = (struct
> >> megasas_header*)cmd->frame;
> >
> >megasas_header is one of the member of union megasas_frame,
> >which is the
> >type of cmd->frame. Please use Cs static typing.
> >
>
> Could you please elaborate this point? Are you saying use:
> struct megasas_header* frame_hdr = &cmd->frame->hdr instead of what
> I have above? If that's what you have in mind, then yes, I will do that.
Yes. In general most places you have to use casts in C are bugs - either
in your code or in the API (e.g. in the Linux timer APIs)
> >> + for( i=0; i < msecs && (frame_hdr->cmd_status == 0xff); i++ ) {
> >> + rmb();
> >> + msleep(1);
> >> + }
> >
> >who is supposed to change frame_hdr->cmd_status while this
> >loop is running?
>
> In the previous statement, I am issuing the frame to the FW. FW will
> change the cmd_status. I will add a comment.
So the frame_hdr is somewhere in iomremapped space? If so you need to
use the proper interfaces (read* or ioread*) to access it.
> I will change here also.
>
> >> + cmd = megasas_build_cmd( instance, scmd, &frame_count );
> >> +
> >> + if (!cmd) {
> >> + done(scmd);
> >> + return 0;
> >
> >I think this should return HOST_BUSY?
> >
>
> I will add a readable comment here. I return NULL cmd when I can complete
> the command from queue routine itself.
I'd have to look over the patch in detail again, but IIRC this is a
resource shortage, in which case not completing the command but returning
busy is the right thing to do. Let's look over it again in the next
revision.
> >I don't think you should implement an abort handler at all if you
> >don't do anything.
> >
>
> My reset handler will still get called, won't it be?
Yes.
> Moreover, isn't
> it a good diagnostic message to be printed from abort handler? Please
> consider.
Don't think so. You'll get a message about the resets from the midlayer
anyway.
> >please allocate the scsi_host directly and early in your probe routing,
> >so you can allocate your private data directly through scsi_host_alloc
> >
>
> The instance is allocated on DMA memory. scsi_host_alloc uses kmalloc().
> I allocate instance on DMA memory because some of its members get their
> values refreshed from FW. Currently only producer and consumer are used. But
> in near future, we will have more such members.
Okay. I'd suggest putting those DMAable members into a separate struct
that is kmalloced so we have them nicely separated.
> >> + instance->host_lock = &instance->lock;
> >
> >please avoid using the host_lock pointer.
> >
>
> Okay. Am I allowed to use scsi host's per-host lock inside my
> reset handler? Is my usage of lock inside the reset_handler correct?
->queuemand and the ->eh_*handler routines are called with the midlayers
per-host lock held. You can of course release and reaquire them inside
these routines (with spin_unlock_irq / spin_lock_irq)
-
: 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
