From: Boaz Harrosh <bharrosh@panasas.com>
To: Matthew Wilcox <willy@linux.intel.com>
Cc: linux-scsi@vger.kernel.org, Matthew Wilcox <matthew@wil.cx>
Subject: Re: [PATCH] gdth: Stop abusing ->done for internal commands
Date: Tue, 25 Sep 2007 19:30:29 +0200 [thread overview]
Message-ID: <46F945B5.2030305@panasas.com> (raw)
In-Reply-To: <1190738524757-git-send-email-willy@linux.intel.com>
On Tue, Sep 25 2007 at 18:42 +0200, Matthew Wilcox <willy@linux.intel.com> wrote:
> From: Matthew Wilcox <matthew@wil.cx>
>
> The ->done member was being used to mark commands as being internal.
> I decided to put a magic number in ->underflow instead. I believe this
> to be safe as no current user of ->underflow has any of the bottom 9
> bits set.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
> drivers/scsi/gdth.c | 19 +++++++++++--------
> drivers/scsi/gdth_proc.c | 4 ++--
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 8a6a5f8..42a200f 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -691,6 +691,9 @@ static const struct file_operations gdth_fops = {
> .release = gdth_close,
> };
>
> +#define GDTH_MAGIC 0xc2e7c389 /* I got it from /dev/urandom */
> +#define IS_GDTH_INTERNAL_CMD(scp) (scp->underflow == GDTH_MAGIC)
> +
> #include "gdth_proc.h"
> #include "gdth_proc.c"
>
> @@ -713,7 +716,7 @@ static void gdth_scsi_done(struct scsi_cmnd *scp)
> {
> TRACE2(("gdth_scsi_done()\n"));
>
> - if (scp->done == gdth_scsi_done)
> + if (IS_GDTH_INTERNAL_CMD(scp))
> complete((struct completion *)scp->request);
> else
> scp->scsi_done(scp);
> @@ -738,7 +741,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
> scp->cmd_len = 12;
> memcpy(scp->cmnd, cmnd, 12);
> scp->SCp.this_residual = IOCTL_PRI; /* priority */
> - scp->done = gdth_scsi_done;
> + scp->underflow = GDTH_MAGIC;
> gdth_queuecommand(scp, NULL);
> wait_for_completion(&wait);
>
> @@ -2319,7 +2322,7 @@ static void gdth_putq(int hanum,Scsi_Cmnd *scp,unchar priority)
> ha = HADATA(gdth_ctr_tab[hanum]);
> spin_lock_irqsave(&ha->smp_lock, flags);
>
> - if (scp->done != gdth_scsi_done) {
> + if (!IS_GDTH_INTERNAL_CMD(scp)) {
> scp->SCp.this_residual = (int)priority;
> b = virt_ctr ? NUMDATA(scp->device->host)->busnum:scp->device->channel;
> t = scp->device->id;
> @@ -2382,7 +2385,7 @@ static void gdth_next(int hanum)
> for (nscp = pscp = ha->req_first; nscp; nscp = (Scsi_Cmnd *)nscp->SCp.ptr) {
> if (nscp != pscp && nscp != (Scsi_Cmnd *)pscp->SCp.ptr)
> pscp = (Scsi_Cmnd *)pscp->SCp.ptr;
> - if (nscp->done != gdth_scsi_done) {
> + if (!IS_GDTH_INTERNAL_CMD(nscp)) {
> b = virt_ctr ?
> NUMDATA(nscp->device->host)->busnum : nscp->device->channel;
> t = nscp->device->id;
> @@ -2408,7 +2411,7 @@ static void gdth_next(int hanum)
> firsttime = FALSE;
> }
>
> - if (nscp->done != gdth_scsi_done) {
> + if (!IS_GDTH_INTERNAL_CMD(nscp)) {
> if (nscp->SCp.phase == -1) {
> nscp->SCp.phase = CACHESERVICE; /* default: cache svc. */
> if (nscp->cmnd[0] == TEST_UNIT_READY) {
> @@ -2471,7 +2474,7 @@ static void gdth_next(int hanum)
> else
> gdth_scsi_done(nscp);
> }
> - } else if (nscp->done == gdth_scsi_done) {
> + } else if (IS_GDTH_INTERNAL_CMD(nscp)) {
> if (!(cmd_index=gdth_special_cmd(hanum,nscp)))
> this_cmd = FALSE;
> next_cmd = FALSE;
> @@ -3812,7 +3815,7 @@ static int gdth_sync_event(int hanum,int service,unchar index,Scsi_Cmnd *scp)
> scp->sense_buffer[2] = NOT_READY;
> scp->result = (DID_OK << 16) | (CHECK_CONDITION << 1);
> }
> - if (scp->done != gdth_scsi_done) {
> + if (!IS_GDTH_INTERNAL_CMD(scp)) {
> ha->dvr.size = sizeof(ha->dvr.eu.sync);
> ha->dvr.eu.sync.ionode = hanum;
> ha->dvr.eu.sync.service = service;
> @@ -4910,7 +4913,7 @@ static int gdth_queuecommand(struct scsi_cmnd *scp,
> #endif
>
> priority = DEFAULT_PRI;
> - if (scp->done == gdth_scsi_done)
> + if (IS_GDTH_INTERNAL_CMD(scp))
> priority = scp->SCp.this_residual;
> else
> gdth_update_timeout(hanum, scp, scp->timeout_per_command * 6);
> diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
> index 32982eb..1f8d9a5 100644
> --- a/drivers/scsi/gdth_proc.c
> +++ b/drivers/scsi/gdth_proc.c
> @@ -805,7 +805,7 @@ static void gdth_stop_timeout(int hanum, int busnum, int id)
> spin_lock_irqsave(&ha->smp_lock, flags);
>
> for (scp = ha->req_first; scp; scp = (Scsi_Cmnd *)scp->SCp.ptr) {
> - if (scp->done != gdth_scsi_done) {
> + if (!IS_GDTH_INTERNAL_CMD(scp)) {
> b = virt_ctr ?
> NUMDATA(scp->device->host)->busnum : scp->device->channel;
> t = scp->device->id;
> @@ -829,7 +829,7 @@ static void gdth_start_timeout(int hanum, int busnum, int id)
> spin_lock_irqsave(&ha->smp_lock, flags);
>
> for (scp = ha->req_first; scp; scp = (Scsi_Cmnd *)scp->SCp.ptr) {
> - if (scp->done != gdth_scsi_done) {
> + if (!IS_GDTH_INTERNAL_CMD(scp)) {
> b = virt_ctr ?
> NUMDATA(scp->device->host)->busnum : scp->device->channel;
> t = scp->device->id;
OK This is on top of the first patch that I will take care of,
right? (http://www.spinics.net/lists/linux-scsi/msg19470.html)
Here 2 it will not merge with my patchset. So I'll take it off your hands
and rebase it to mine.
In my patchset I have introduced a gdth_cmnd_priv. That is associated
with every command and is put on cmnd->host_scribble, I than moved lots
of abused Scp and other members to that new structure. I also put a
simple boolean that states that these are Internal commands. So what
is left of this patch is the:
- scp->done = gdth_scsi_done;
I will add that one line removal to your previous patch.
> Ah; I was basing this on linus + scsi-misc. I guess scsi_lib hadn't
> been converted because I just took the code from there.
Yes scsi_lib was not converted, but scsi.c was.
Boaz
next prev parent reply other threads:[~2007-09-25 17:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-09-25 16:42 Remove ->done from scsi_cmnd Matthew Wilcox
2007-09-25 16:42 ` [PATCH] Fix mistaken uses of ->done Matthew Wilcox
2007-09-25 16:42 ` [PATCH] pluto: Don't abuse ->done for internal commands Matthew Wilcox
2007-09-25 16:42 ` [PATCH] gdth: Stop abusing " Matthew Wilcox
2007-09-25 17:30 ` Boaz Harrosh [this message]
2007-09-25 17:33 ` Matthew Wilcox
2007-09-25 16:42 ` [PATCH] Get rid of scsi_cmnd->done Matthew Wilcox
2007-09-25 17:05 ` Boaz Harrosh
2007-09-25 17:09 ` Matthew Wilcox
2007-09-26 19:54 ` Remove ->done from scsi_cmnd Christoph Hellwig
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=46F945B5.2030305@panasas.com \
--to=bharrosh@panasas.com \
--cc=linux-scsi@vger.kernel.org \
--cc=matthew@wil.cx \
--cc=willy@linux.intel.com \
/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