Hey Linus and the rest of you.
A simple fix for the io_request_lock issue leftovers in drivers/scsi/psi240i.c.
Not tested, but compiles. Diffed against 2.5.2-pre10. Please apply.
== Morten
--
"Det er ikke lett ? v?re menneske" - sitat fra en klok person.
mvh
Morten Helgesen
UNIX System Administrator & C Developer
Nextframe AS
[email protected] / 93445641
http://www.nextframe.net
--- vanilla-linux-2.5.2-pre10/drivers/scsi/psi240i.c Tue Jan 8 10:57:31 2002
+++ patched-linux-2.5.2-pre10/drivers/scsi/psi240i.c Tue Jan 8 14:48:56 2002
@@ -370,10 +370,11 @@
static void do_Irq_Handler (int irq, void *dev_id, struct pt_regs *regs)
{
unsigned long flags;
+ struct Scsi_Host *host = PsiHost[irq - 10];
- spin_lock_irqsave(&io_request_lock, flags);
+ spin_lock_irqsave(&host->host_lock, flags);
Irq_Handler(irq, dev_id, regs);
- spin_unlock_irqrestore(&io_request_lock, flags);
+ spin_unlock_irqrestore(&host->host_lock, flags);
}
/****************************************************************
* Name: Psi240i_QueueCommand
Morten Helgesen wrote:
>
> Hey Linus and the rest of you.
>
> A simple fix for the io_request_lock issue leftovers in drivers/scsi/psi240i.c.
> Not tested, but compiles. Diffed against 2.5.2-pre10. Please apply.
>
Morten,
There is a bit more involved than just switching
io_request_lock to host_lock. The former is global
so it could be called from anywhere.
>From the look of this line in the patch:
> + struct Scsi_Host *host = PsiHost[irq - 10];
It will work if the first controller is allocated irq 10,
the second one irq 11, etc. Unlikely ...
Looking at that driver it seems that it will need a
bit of surgery to pass perhaps a Scsi_Cmnd pointer
through the request_irq() function so you can
follow a pointer chain in do_Irq_Handler() to get
hold of the appropriate host_lock.
In the lk 2.5 series host_lock should indeed be held
when the callback "scsi_done" is invoked and that
most likely occurs in Irq_Handler(). So that is right.
BTW To get a better idea of what is involved, diff the
sym53c8xx driver in lk 2.4.15 and the one in the
lk 2.5 series now [kudos to Gerard Roudier].
Having been burnt by a well meaning advansys patch that
converted a kernel compile time error into a kernel
boot time freeze, it is a bit worrying the number
of "untested" patches of this nature appearing on lkml.
Doug Gilbert
> --- vanilla-linux-2.5.2-pre10/drivers/scsi/psi240i.c Tue Jan 8 10:57:31 2002
> +++ patched-linux-2.5.2-pre10/drivers/scsi/psi240i.c Tue Jan 8 14:48:56 2002
> @@ -370,10 +370,11 @@
> static void do_Irq_Handler (int irq, void *dev_id, struct pt_regs *regs)
> {
> unsigned long flags;
> + struct Scsi_Host *host = PsiHost[irq - 10];
>
> - spin_lock_irqsave(&io_request_lock, flags);
> + spin_lock_irqsave(&host->host_lock, flags);
> Irq_Handler(irq, dev_id, regs);
> - spin_unlock_irqrestore(&io_request_lock, flags);
> + spin_unlock_irqrestore(&host->host_lock, flags);
> }
> /****************************************************************
> * Name: Psi240i_QueueCommand
On Tue, Jan 08 2002, Douglas Gilbert wrote:
> Morten Helgesen wrote:
> >
> > Hey Linus and the rest of you.
> >
> > A simple fix for the io_request_lock issue leftovers in drivers/scsi/psi240i.c.
> > Not tested, but compiles. Diffed against 2.5.2-pre10. Please apply.
> >
>
> Morten,
> There is a bit more involved than just switching
> io_request_lock to host_lock. The former is global
> so it could be called from anywhere.
>
> >From the look of this line in the patch:
> > + struct Scsi_Host *host = PsiHost[irq - 10];
>
> It will work if the first controller is allocated irq 10,
> the second one irq 11, etc. Unlikely ...
Irk yes, that is very ugly! And it's even used currently in the driver
as well. How about passing the scsi host as device_id for the isr
instead?
--
Jens Axboe
Hey Jens.
I did the changes based on the changes done to the aha1740.c driver. Below is a diff
between a clean 2.4.15 and a clean 2.5.2-pre10, which shows the changes done to aha1740.c :
--- tars/linux/drivers/scsi/aha1740.c Sun Sep 30 21:26:07 2001
+++ clean-linux-2.5.2-pre10/drivers/scsi/aha1740.c Tue Jan 8 10:56:27 2002
@@ -213,6 +213,7 @@
/* A "high" level interrupt handler */
void aha1740_intr_handle(int irq, void *dev_id, struct pt_regs * regs)
{
+ struct Scsi_Host *host = aha_host[irq - 9];
void (*my_done)(Scsi_Cmnd *);
int errstatus, adapstat;
int number_serviced;
@@ -221,11 +222,10 @@
unsigned int base;
unsigned long flags;
- spin_lock_irqsave(&io_request_lock, flags);
-
- if (!aha_host[irq - 9])
+ if (!host)
panic("aha1740.c: Irq from unknown host!\n");
- base = aha_host[irq - 9]->io_port;
+ spin_lock_irqsave(&host->host_lock, flags);
+ base = host->io_port;
number_serviced = 0;
while(inb(G2STAT(base)) & G2STAT_INTPEND)
@@ -299,7 +299,7 @@
number_serviced++;
}
- spin_unlock_irqrestore(&io_request_lock, flags);
+ spin_unlock_irqrestore(&host->host_lock, flags);
}
int aha1740_queuecommand(Scsi_Cmnd * SCpnt, void (*done)(Scsi_Cmnd *))
I guess this is not the way to do it.
I now see the problem you guys have pointed out. Sorry, my bad.
== Morten
On Wed, Jan 09, 2002 at 11:46:38AM +0100, Jens Axboe wrote:
> On Tue, Jan 08 2002, Douglas Gilbert wrote:
> > Morten Helgesen wrote:
> > >
> > > Hey Linus and the rest of you.
> > >
> > > A simple fix for the io_request_lock issue leftovers in drivers/scsi/psi240i.c.
> > > Not tested, but compiles. Diffed against 2.5.2-pre10. Please apply.
> > >
> >
> > Morten,
> > There is a bit more involved than just switching
> > io_request_lock to host_lock. The former is global
> > so it could be called from anywhere.
> >
> > >From the look of this line in the patch:
> > > + struct Scsi_Host *host = PsiHost[irq - 10];
> >
> > It will work if the first controller is allocated irq 10,
> > the second one irq 11, etc. Unlikely ...
>
> Irk yes, that is very ugly! And it's even used currently in the driver
> as well. How about passing the scsi host as device_id for the isr
> instead?
>
> --
> Jens Axboe
>
> -
> 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/
--
"Det er ikke lett ? v?re menneske" - sitat fra en klok person.
mvh
Morten Helgesen
UNIX System Administrator & C Developer
Nextframe AS
[email protected] / 93445641
http://www.nextframe.net
>> = Morten Helgesen
> = Douglas Gilbert
>From the look of this line in the patch:
>> + struct Scsi_Host *host = PsiHost[irq - 10];
>
>It will work if the first controller is allocated irq 10,
>the second one irq 11, etc. Unlikely ...
No, I think Morten has the use of PsiHost right. The
entries in PsiHost are apparently stored by IRQ. It is not generally
the case that the first controller is at PsiHost[0], the second at
PsiHost[1], etc.
I agree with Jens in that the practice is rather ugly, but that
is the way the driver worked before io_request_lock disappeared and
I think that improving that stylistic issue is not a prerequisite
for conversion from io_request_lock to host->host_lock.
If I were you, Morten, I would go ahead with your patch
that makes the minimal changes and then, if you want, make stylistic
improvements as one or more separate patches, which are something
that you may want to talk over with the mainter of that driver, if
there currently is one.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
On Wed, Jan 09, 2002 at 05:28:00AM -0800, Adam J. Richter wrote:
> >> = Morten Helgesen
> > = Douglas Gilbert
>
> >From the look of this line in the patch:
> >> + struct Scsi_Host *host = PsiHost[irq - 10];
> >
> >It will work if the first controller is allocated irq 10,
> >the second one irq 11, etc. Unlikely ...
>
> No, I think Morten has the use of PsiHost right. The
> entries in PsiHost are apparently stored by IRQ. It is not generally
> the case that the first controller is at PsiHost[0], the second at
> PsiHost[1], etc.
Even though I am not _that_ familiar with the code in question, that is how I
understood it too.
>
> I agree with Jens in that the practice is rather ugly, but that
> is the way the driver worked before io_request_lock disappeared and
> I think that improving that stylistic issue is not a prerequisite
> for conversion from io_request_lock to host->host_lock.
Ugly it is. It was not my intention to clean up the code, just make the smallest change
necessary to get it to work. The maintainer should probably do a closer examination.
>
> If I were you, Morten, I would go ahead with your patch
> that makes the minimal changes and then, if you want, make stylistic
> improvements as one or more separate patches, which are something
> that you may want to talk over with the mainter of that driver, if
> there currently is one.
Sure - but who is the maintainer ? :)
>
> Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
> [email protected] \ / San Jose, California 95129-1034
> +1 408 261-6630 | g g d r a s i l United States of America
> fax +1 408 261-6631 "Free Software For The Rest Of Us."
> -
> 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/
--
"Det er ikke lett ? v?re menneske" - sitat fra en klok person.
mvh
Morten Helgesen
UNIX System Administrator & C Developer
Nextframe AS
[email protected] / 93445641
http://www.nextframe.net