mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Rasmus Andersen <rasmus@jaquet.dk>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>,
	linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org
Subject: Re: [NEW SCSI DRIVER] for 53c700 chip and NCR_D700 card against 2.4.4
Date: Mon, 14 May 2001 20:51:08 -0500	[thread overview]
Message-ID: <200105150151.UAA08944@jet.il.steeleye.com> (raw)
In-Reply-To: Message from Rasmus Andersen <rasmus@jaquet.dk>  of "Mon, 14 May 2001 22:23:06 +0200." <20010514222306.B840@jaquet.dk>

I've implemented most of these as you suggest, with these exceptions:

> > +	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 :( )

The KERN_XX only go at the beginning of an output line (otherwise you see rather annoying <n> embedded in the log---the smc-mca driver has a hugely irritating example of this).  print_msg doesn't do a carriage return by itself, so these three prints are really only one line (and hence all done under the KERN_INFO).

The "OUTSTANDING TAGS.." message is another one of these---it prints a list of tags all on one line.


rasmus@jaquet.dk said:
> +                       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.

Actually, I need to remove the release_region here (since it's done in NCR_700_release) and add a scsi_unregister.

There's no need to do a scsi_unregister in _release; scsi_unregister is only really used to tell the mid layer to relinquish a host after an error.  In _release, the mid layer is telling you that it has released the host and we need to clean up.

Thanks for the updates, I'll post another patch once I've got a few other people's suggestions rolled in.

James



  reply	other threads:[~2001-05-15  1:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2001-05-12 16:43 James Bottomley
2001-05-14 20:23 ` Rasmus Andersen
2001-05-15  1:51   ` James Bottomley [this message]
2001-05-13 21:43 Andries.Brouwer
2001-05-13 22:40 ` Alan Cox
2001-05-14 12:41   ` Richard Hirst
2001-05-14 14:58     ` Alan Cox
2001-05-13 23:57 Andries.Brouwer
2001-05-14  0:55 ` James Bottomley
2001-05-14 12:43   ` Richard Hirst

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200105150151.UAA08944@jet.il.steeleye.com \
    --to=james.bottomley@hansenpartnership.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=rasmus@jaquet.dk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®