Disclaimer: I know nothing of this device/hw, the scsi protocol or
anything remotely applicable the functioning of this driver. The
stuff below is just nit-picking and PITA-ing :) Pro-active kernel
janitoring, if you like.
On Sat, May 12, 2001 at 11:43:04AM -0500, James Bottomley wrote:
[...]
> +struct Scsi_Host * __init
> +NCR_700_detect(Scsi_Host_Template *tpnt,
> + struct NCR_700_Host_Parameters *hostdata)
> +{
> + __u32 *script = kmalloc(sizeof(SCRIPT), GFP_KERNEL);
> + __u32 pScript;
> + struct Scsi_Host *host;
> + static int banner = 0;
> + int j;
> +
[...]
> +
> + host = scsi_register(tpnt, 4);
> + if(script == NULL) {
> + printk(KERN_ERR "53c700: Failed to allocate script, detatching\n");
> + scsi_unregister(host);
> + return NULL;
> + }
You are not checking if the scsi_register went ok or not.
[...]
> +STATIC void
> +free_slot(struct NCR_700_command_slot *slot,
> + struct NCR_700_Host_Parameters *hostdata)
> +{
> + int hash;
> + struct NCR_700_command_slot **forw, **back;
> +
> +
> + if((slot->state & NCR_700_SLOT_MASK) != NCR_700_SLOT_MAGIC) {
> + printk(" SLOT %p is not MAGIC!!!\n", slot);
> + }
> + if(slot->state == NCR_700_SLOT_FREE) {
> + printk(" SLOT %p is FREE!!!\n", slot);
> + }
Could you be persuaded to add KERN_SOMETHING to the printks above?
[...]
> +STATIC __u32
> +process_extended_message(struct Scsi_Host *host,
> + struct NCR_700_Host_Parameters *hostdata,
> + Scsi_Cmnd *SCp, __u32 dsp, __u32 dsps)
> +{
> + __u32 resume_offset = dsp, temp = dsp + 8;
> + __u8 pun = 0xff, lun = 0xff;
> +
> + if(SCp != NULL) {
> + pun = SCp->target;
> + lun = SCp->lun;
> + }
> +
> + switch(hostdata->msgin[2]) {
[...]
> +
> + default:
> + printk(KERN_INFO "scsi%d (%d:%d): Unexpected message %s: ",
> + host->host_no, pun, lun,
> + NCR_700_phase[(dsps & 0xf00) >> 8]);
> + print_msg(hostdata->msgin);
> + printk("\n");
It would be nice with a KERN_XX before "\n" (yes, I recognize that
print_msg does not do this :( )
[...]
> +STATIC __u32
> +process_script_interrupt(__u32 dsps, __u32 dsp, Scsi_Cmnd *SCp,
> + struct Scsi_Host *host,
> + struct NCR_700_Host_Parameters *hostdata)
> +{
[...]
> + } else if((dsps & 0xfffff0f0) == A_UNEXPECTED_PHASE) {
> + __u8 i = (dsps & 0xf00) >> 8;
> +
> + printk(KERN_ERR "scsi%d: (%d:%d), UNEXPECTED PHASE %s (%s)\n",
> + host->host_no, pun, lun,
> + NCR_700_phase[i],
> + sbcl_to_string(inb(host->base + SBCL_REG)));
> + printk(" len = %d, cmd =", SCp->cmd_len);
A KERN_ERR prefix?
[...]
> + retry:
> + if(slot == NULL) {
> + struct NCR_700_command_slot *s = find_ITL_Nexus(hostdata,
>reselection_id, lun);
> + printk(KERN_ERR "scsi%d: (%d:%d) RESELECTED but no saved
>command (MSG = %02x %02x %02x)!!\n",
> + host->host_no, reselection_id, lun,
> + hostdata->msgin[0], hostdata->msgin[1],
> + hostdata->msgin[2]);
> + printk(KERN_ERR " OUTSTANDING TAGS:");
> + while(s != NULL) {
> + if(s->cmnd->target == reselection_id &&
> + s->cmnd->lun == lun) {
> + printk("%d ", s->tag);
KERN_ERR?
> + if(s->tag == hostdata->msgin[2]) {
> + printk(" ***FOUND*** \n");
As above.
> + slot = s;
> + goto retry;
> + }
> +
> + }
> + s = s->ITL_back;
> + }
> + printk("\n");
KERN_ERR?
[...]
> +void
> +NCR_700_intr(int irq, void *dev_id, struct pt_regs *regs)
> +{
[...]
> + if(dsp == Ent_SendMessage + 8 + hostdata->pScript) {
> + /* It wants to reply to some part of
> + * our message */
> + __u32 temp = inl(host->base + TEMP_REG);
> +
> + int count = (hostdata->script[Ent_SendMessage/4] &
>0xffffff) - ((inl(host->base + DBC_REG) & 0xffffff) + NCR_700_data_residual(host));
> + printk("scsi%d (%d:%d) PHASE MISMATCH IN SEND MESSAGE
>%d remain, return %p[%04x], phase %s\n", host->host_no, pun, lun, count, (void
>*)temp, temp - hostdata->pScript, sbcl_to_string(inb(host->base + SBCL_REG)));
KERN_ERR?
Also, some places you have KERN_ERR on debug output and some places
not. If you could be persuaded to put KERN_XX on the debug output too,
it would be nice (but not crucial).
[...]
> +
> +STATIC int
> +NCR_700_abort(Scsi_Cmnd * SCp)
> +{
> + struct NCR_700_command_slot *slot;
> + struct NCR_700_Host_Parameters *hostdata =
> + (struct NCR_700_Host_Parameters *)SCp->host->hostdata[0];
> +
> + printk("scsi%d (%d:%d) New error handler wants to abort command\n\t",
> + SCp->host->host_no, SCp->target, SCp->lun);
KERN_XX?
[...]
> +
> +STATIC int
> +NCR_700_bus_reset(Scsi_Cmnd * SCp)
> +{
> + printk("scsi%d (%d:%d) New error handler wants BUS reset, cmd %p\n\t",
> + SCp->host->host_no, SCp->target, SCp->lun, SCp);
KERN_XX?
> + print_command(SCp->cmnd);
> + NCR_700_internal_bus_reset(SCp->host);
> + return SUCCESS;
> +}
> +
> +STATIC int
> +NCR_700_dev_reset(Scsi_Cmnd * SCp)
> +{
> + printk("scsi%d (%d:%d) New error handler wants device reset\n\t",
> + SCp->host->host_no, SCp->target, SCp->lun);
KERN_XX?
> + print_command(SCp->cmnd);
> +
> + return FAILED;
> +}
> +
> +STATIC int
> +NCR_700_host_reset(Scsi_Cmnd * SCp)
> +{
> + printk("scsi%d (%d:%d) New error handler wants HOST reset\n\t",
> + SCp->host->host_no, SCp->target, SCp->lun);
KERN_XX?
> + print_command(SCp->cmnd);
> +
> + NCR_700_internal_bus_reset(SCp->host);
> + NCR_700_chip_reset(SCp->host);
> + return SUCCESS;
> +}
[...]
+STATIC int __init
+D700_detect(Scsi_Host_Template *tpnt)
+{
+ int slot = 0;
+ int found = 0;
+ int differential;
+ int banner = 1;
[...]
+ if(hostdata == NULL) {
+ printk(KERN_ERR "NCR D700: Failed to allocate
+host data for channel %d, detatching\n", i);
+ continue;
+ }
+ memset(hostdata, 0, sizeof(struct
+NCR_700_Host_Parameters));
+ request_region(region, 64, "NCR_D700");
Would you be kind enough to check the return code of the request_region?
+ /* Fill in the three required pieces of hostdata */
+ hostdata->base = region;
+ hostdata->differential = (((1<<i) & differential) != 0);+
+ hostdata->clock = NCR_D700_CLOCK_MHZ;
+ /* and register the chip */
+ if((host = NCR_700_detect(tpnt, hostdata)) == NULL) {
+ kfree(hostdata);
+ continue;
+ }
...and to release it in your error paths?
+ host->irq = irq;
+ /* FIXME: Read this from SUS */
+ host->this_id = id_array[slot * 2 + i];
+ printk(KERN_NOTICE "NCR D700: SIOP%d, SCSI id is %d\n",
+ i, host->this_id);
+ if(request_irq(irq, NCR_700_intr, SA_SHIRQ, "NCR_D700",
+host)) {
+ printk(KERN_ERR "NCR D700, channel %d: irq
+problem, detatching\n", i);
+ NCR_700_release(host);
+ release_region(host->base, 64);
+ continue;
+ }
You need to kfree(hostdata) here. I would also recommend putting
a scsi_unregister into NCR_700_release since you need to balance
the scsi_register done in NCR_700_detect.
I recommend using forward gotos in your error paths for sanity.
There can be used with loops&continues as well and will help you
not have these resource leaks (across local scope anyway) since
you have two function exit points: The normal one and the error
one (you can collapse these to one exit path if you care strongly
about this).
--
Regards,
Rasmus([EMAIL PROTECTED])
Which is worse: Ignorance or Apathy?
Who knows? Who cares?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
