login
Header Space

 
 

Re: [GIT PATCH] another tranche of SCSI updates for 2.6.26

Previous thread: Bank Draft Notification: 0789/05 by Central Bank of Nigeria on Sunday, April 27, 2008 - 2:14 pm. (1 message)

Next thread: [PATCH] kvm: add statics were possible, function definition in lapic.h by Harvey Harrison on Sunday, April 27, 2008 - 3:14 pm. (2 messages)
To: Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>
Cc: linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Sunday, April 27, 2008 - 2:14 pm

This represents the tree I had waitin on other mergers.  I'm not sure
this is it, because there are other features (like aic94xx running
abort) we're racing to get in.

The patch is available at:

master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git

The short changelog is:

Adrian Bunk (3):
      qla2xxx: make qla2x00_issue_iocb_timeout() static
      qla2xxx: qla_os.c, make 2 functions static
      FlashPoint: fix off-by-one errors

Andrew Vasquez (7):
      qla2xxx: Update version number to 8.02.01-k2.
      qla2xxx: Correct regression in relogin code.
      qla2xxx: Re-register FDMI information after a LIP.
      qla2xxx: Correct SRB usage-after-completion/free issues.
      qla2xxx: Correct ISP84XX verify-chip response handling.
      qla2xxx: Wakeup DPC thread to process any deferred-work requests.
      qla2xxx: Collapse RISC-RAM retrieval code during a firmware-dump.

David S. Miller (1):
      esp_scsi: Make cur_residue and tot_residue signed.

Denys Vlasenko (3):
      aic7xxx: add const
      aic7xxx: add static
      aic7xxx, aic79xx: deinline functions

FUJITA Tomonori (2):
      scsi_transport_sas: fix the lifetime of sas bsg objects
      bsg: add release callback support

Finn Thain (1):
      m68k: new mac_esp scsi driver

Hannes Reinecke (6):
      aic7xxx: Update _shipped files
      aic7xxx: teach aicasm to not emit unused debug code/data
      aic7xxx: Update type check in aicasm grammar
      use default attributes for scsi_host
      qla2xxx, lfpc: Rename 'state' attribute to 'link_state'
      add scsi_host and scsi_target to scsi_bus

James Bottomley (6):
      fix SLUB WARN_ON
      rework scsi_target allocation
      scsi_transport_spi: fix the attribute settings
      sysfs: make group is_valid return a mode_t
      ses: fix up functionality after class_device-&gt;device conversion
      st: fix up after class_device removal

James Smart (1):
      scsi_transport_fc: fc_user_scan correction

Jeff Garzik (2):
     ...
To: James Bottomley <James.Bottomley@...>
Cc: Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Sunday, April 27, 2008 - 9:34 pm

hm, got this crash with latest -git shortly after i rebased from this 
morning's git to this night's git, it looks SCSI related:

[   44.513114] Calling initcall 0xc1cece47: init_this_scsi_driver+0x0/0xd0()
[   47.919053] BUG: unable to handle kernel NULL pointer dereference at 00000004
[   47.927035] IP: [&lt;c09cf947&gt;] scsi_destroy_command_freelist+0x15/0x5a
[   47.931008] *pde = 00000000 
[   47.935253] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[   47.939004] Modules linked in:
[   47.939004] 
[   47.939004] Pid: 1, comm: swapper Not tainted (2.6.25-sched-devel.git-x86-latest.git #5)
[   47.939004] EIP: 0060:[&lt;c09cf947&gt;] EFLAGS: 00010217 CPU: 0
[   47.939004] EIP is at scsi_destroy_command_freelist+0x15/0x5a
[   47.939004] EAX: c0042000 EBX: 00000000 ECX: c199ba14 EDX: fffffffc
[   47.939004] ESI: c0042000 EDI: c0042034 EBP: f7c36ebc ESP: f7c36eb0
[   47.939004]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[   47.939004] Process swapper (pid: 1, ti=f7c36000 task=f7c4e000 task.ti=f7c36000)
[   47.939004] Stack: c0042000 00000000 00000000 f7c36ecc c09cfa4c c004225c c1a43378 f7c36ed4 
[   47.939004]        c0688535 f7c36ee8 c04e942b c0042260 c04e93e6 00000330 f7c36ef8 c04e9f20 
[   47.939004]        c004225c 00000002 f7c36f04 c04e9353 c0042000 f7c36f0c c0688aee f7c36f14 
[   47.939004] Call Trace:
[   47.939004]  [&lt;c09cfa4c&gt;] ? scsi_host_dev_release+0x79/0xa9
[   47.939004]  [&lt;c0688535&gt;] ? device_release+0x3e/0x54
[   47.939004]  [&lt;c04e942b&gt;] ? kobject_release+0x45/0x55
[   47.939004]  [&lt;c04e93e6&gt;] ? kobject_release+0x0/0x55
[   47.939004]  [&lt;c04e9f20&gt;] ? kref_put+0x3e/0x49
[   47.939004]  [&lt;c04e9353&gt;] ? kobject_put+0x41/0x46
[   47.939004]  [&lt;c0688aee&gt;] ? put_device+0x16/0x18
[   47.939004]  [&lt;c09cf9d1&gt;] ? scsi_host_put+0x12/0x14
[   47.939004]  [&lt;c09cfbac&gt;] ? scsi_unregister+0x1d/0x20
[   47.939004]  [&lt;c1cece2d&gt;] ? aha1542_detect+0x7d1/0x7eb
[   47.939004]  [&lt;c016de2e&gt;] ? trace_hardirqs_on+0xb/0xd
[   47.93...
To: Ingo Molnar <mingo@...>
Cc: Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Sunday, April 27, 2008 - 10:51 pm

sigh, every time I fix this free list stuff in one place, it breaks in
another.  This one is caused by the alloc-&gt;put sequence for the host (it
never got to scsi_add_host() where the freelist is allocated, so we need
to not release it in that case).

Try this; the signature for an uninitialised free list is easy (both
list pointers NULL), so the patch detects that and doesn't try to run
over the uninitialised list head.

James

---

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 12d69d7..dc36321 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -481,6 +481,14 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
  */
 void scsi_destroy_command_freelist(struct Scsi_Host *shost)
 {
+	if (shost-&gt;free_list.next == NULL &amp;&amp; shost-&gt;free_list.prev == NULL)
+		/*
+		 * If the next and prev pointers are NULL, that
+		 * means the list was never initialised, so it
+		 * doesn't need freeing
+		 */
+		return;
+
 	while (!list_empty(&amp;shost-&gt;free_list)) {
 		struct scsi_cmnd *cmd;
 


--
To: James Bottomley <James.Bottomley@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Monday, April 28, 2008 - 12:05 pm

Why aren't these things initialized?

You say that the signature of an uninitialised free list is trivial, but 
that's not at all true in general. It depends intimately on how the memory 
was allocated, and is thus very subtle indeed - some change to allocations 
can break something simple like this, by initializing it with random old 
memory contents.

So why not just initialize lists like this so early (ie at allocation 
time) that problems like this cannot happen? Instead of adding ugly and 
fragile cases to the freeing?

		Linus
--
To: Linus Torvalds <torvalds@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Monday, April 28, 2008 - 2:25 pm

They are, but not until we begin the freelist allocation.  That way we
can keep the list head being NULL as the signal for the freelist not

No, no; for us it's guaranteed to be NULL ... they're allocated in the
host memory area with kzalloc. (and before kzalloc, we were using
kmalloc/memset because the host area has an API guarantee of being zero

Because then I'd need another flag to know whether or not the free list
has actually been set up.  In theory, if we initialise the list,
list_empty() would do because when you're freeing you should always have
the reserve command on the free list ... but that would have prevented
us from seeing the bug Ingo reported recently (where we were freeing
with active commands), so I'm a bit reluctant to do that.

James


--
To: James Bottomley <James.Bottomley@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Monday, April 28, 2008 - 3:23 am

If we are already on the subject. It looks like we always have at most 1 command in the 
free list, so why the free list at all? or am I reading the code wrong?

Boaz


--
To: Boaz Harrosh <bharrosh@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Monday, April 28, 2008 - 8:13 am

Because list handlers are well understood mechanisms within the kernel.
Also because in low memory situations, one command per host is
sufficient to guarantee forward progress, but it's not going to be very
efficient.  Embedded and other low memory environments can increase the
size of the free list to improve their I/O path.

James


--
To: James Bottomley <James.Bottomley@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Monday, April 28, 2008 - 8:24 am

Ok that is what I thought, but inspecting the code, I can't find it. Is there
a config option or an external mechanism that let you do that? If not, is/was
there a ready made external patch that will enable such facility in someway?

Boaz
--
To: <bharrosh@...>
Cc: <James.Bottomley@...>, <mingo@...>, <akpm@...>, <torvalds@...>, <linux-scsi@...>, <linux-kernel@...>
Date: Monday, April 28, 2008 - 4:34 am

On Mon, 28 Apr 2008 10:23:22 +0300

scsi_add_host sets up one free command. If you call scsi_host_alloc
and then scsi_host_put (some LLDs do on their failure path), you hit
the above problem.
--
To: FUJITA Tomonori <fujita.tomonori@...>
Cc: <James.Bottomley@...>, <mingo@...>, <akpm@...>, <torvalds@...>, <linux-scsi@...>, <linux-kernel@...>
Date: Monday, April 28, 2008 - 4:40 am

That was not my question. I understand all about that problem. My question was:

We never have more then one command in the free list. So why do we need a free list
at all, we can just have a pointer to the extra command at host and thats it. We
don't need the all link-list to keep track of just one command

Boaz

--
To: James Bottomley <James.Bottomley@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>, Andy Whitcroft <apw@...>, Balbir Singh <balbir@...>
Date: Monday, April 28, 2008 - 3:15 am

Hi James,

Some of machines were also getting the same painc while bootup. This patch
fixes the kernel bug.

Tested-by: Kamalesh Babulal &lt;kamalesh@linux.vnet.ibm.com&gt;

-- 
Thanks &amp; Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.
--
To: James Bottomley <James.Bottomley@...>
Cc: Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, linux-scsi <linux-scsi@...>, linux-kernel <linux-kernel@...>
Date: Sunday, April 27, 2008 - 9:45 pm

did some more digging, regression is not too serious - excluding the ISA 

	Ingo

-----------------&gt;
Subject: qa: no scsi aha
From: Ingo Molnar &lt;mingo@elte.hu&gt;
Date: Mon Apr 28 03:34:16 CEST 2008

Signed-off-by: Ingo Molnar &lt;mingo@elte.hu&gt;
---
 drivers/scsi/Kconfig |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/drivers/scsi/Kconfig
===================================================================
--- linux.orig/drivers/scsi/Kconfig
+++ linux/drivers/scsi/Kconfig
@@ -406,6 +406,7 @@ config SCSI_ACARD
 config SCSI_AHA152X
 	tristate "Adaptec AHA152X/2825 support"
 	depends on ISA &amp;&amp; SCSI &amp;&amp; !64BIT
+	depends on 0
 	select SCSI_SPI_ATTRS
 	select CHECK_SIGNATURE
 	---help---
@@ -423,6 +424,7 @@ config SCSI_AHA152X
 config SCSI_AHA1542
 	tristate "Adaptec AHA1542 support"
 	depends on ISA &amp;&amp; SCSI &amp;&amp; ISA_DMA_API
+	depends on 0
 	---help---
 	  This is support for a SCSI host adapter.  It is explained in section
 	  3.4 of the SCSI-HOWTO, available from
@@ -437,6 +439,7 @@ config SCSI_AHA1542
 config SCSI_AHA1740
 	tristate "Adaptec AHA1740 support"
 	depends on EISA &amp;&amp; SCSI
+	depends on 0
 	---help---
 	  This is support for a SCSI host adapter.  It is explained in section
 	  3.5 of the SCSI-HOWTO, available from
--
Previous thread: Bank Draft Notification: 0789/05 by Central Bank of Nigeria on Sunday, April 27, 2008 - 2:14 pm. (1 message)

Next thread: [PATCH] kvm: add statics were possible, function definition in lapic.h by Harvey Harrison on Sunday, April 27, 2008 - 3:14 pm. (2 messages)
speck-geostationary