[hardy] SRU reguest for LP#199934

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[hardy] SRU reguest for LP#199934

Stefan Bader-2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

https://bugs.launchpad.net/ubuntu/+source/linux-meta/+bug/199934

SRU justification:

Impact: There are changes in the stable 2.6.24 tree that avoid problems with
the gdth SCSI driver. Without these changes, the kernel will panic on reboot or
when issuing certain control commands (see bug report)

Fix: The fix to this problem has been included in the 2.6.24.y stable tree but
still has one problem (WARN_ON) mentionen. There is a second quite straight
forward patch upstream that will address this issue.

Testcase: See bug report.

- --

When all other means of communication fail, try words!


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIpfNPP+TjRTJVqvQRAl8FAKD/Ou4wYDezldgTXvzO6Qmh8OBnDQCghmfO
u0i+S9FgTHOHcMEbetT1ldo=
=sgrY
-----END PGP SIGNATURE-----

From 94429518999e4e6b8f84807afa4bf089a63da0b4 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <[hidden email]>
Date: Thu, 6 Mar 2008 02:10:13 +0000
Subject: [PATCH] SCSI: gdth: fix to internal commands execution

Bug: #199934

picked from linux-2.6.24.y 94429518999e4e6b8f84807afa4bf089a63da0b4
commit: ee54cc6af95a7fa09da298493b853a9e64fa8abd

The recent patch named:
  [SCSI] gdth: !use_sg cleanup and use of scsi accessors

has done a bad job in handling internal commands issued by gdth_execute().

Internal commands are issued with device gdth_cmd_str ready made directly
to the card, without any mapping or translations of scsi commands. So here
I added a gdth_cmd_str pointer to the gdth_cmndinfo private structure which
is then copied directly to host.

following this patch is a cleanup that removes the home cooked accessors
and reverts them to regular scsi_cmnd accessors. Since they are not used
anymore. After review maybe the 2 patches should be squashed together.

FIXME: There is still a problem with gdth_get_info(). as reported there
   is a WARN_ON trigerd in dma_free_coherent() when doing:
   $ cat /proc/sys/gdth/0

Signed-off-by: Boaz Harrosh <[hidden email]>
Tested-by: Joerg Dorchain: <[hidden email]>
Tested-by: Stefan Priebe <[hidden email]>
Tested-by: Jon Chelton <[hidden email]>
Signed-off-by: James Bottomley <[hidden email]>
Signed-off-by: Chris Wright <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
Signed-off-by: Stefan Bader <[hidden email]>
---
 drivers/scsi/gdth.c |   30 ++++++++++++------------------
 drivers/scsi/gdth.h |    1 +
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index a443300..b8b67f6 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -160,7 +160,7 @@ static void gdth_readapp_event(gdth_ha_str *ha, unchar application,
 static void gdth_clear_events(void);
 
 static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
-                                    char *buffer, ushort count, int to_buffer);
+                                    char *buffer, ushort count);
 static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp);
 static int gdth_fill_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, ushort hdrive);
 
@@ -439,8 +439,8 @@ static struct gdth_cmndinfo *gdth_get_cmndinfo(gdth_ha_str *ha)
  for (i=0; i<GDTH_MAXCMDS; ++i) {
  if (ha->cmndinfo[i].index == 0) {
  priv = &ha->cmndinfo[i];
- priv->index = i+1;
  memset(priv, 0, sizeof(*priv));
+ priv->index = i+1;
  break;
  }
  }
@@ -487,7 +487,6 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     gdth_ha_str *ha = shost_priv(sdev->host);
     Scsi_Cmnd *scp;
     struct gdth_cmndinfo cmndinfo;
-    struct scatterlist one_sg;
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;
 
@@ -501,13 +500,10 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     /* use request field to save the ptr. to completion struct. */
     scp->request = (struct request *)&wait;
     scp->timeout_per_command = timeout*HZ;
-    sg_init_one(&one_sg, gdtcmd, sizeof(*gdtcmd));
-    gdth_set_sglist(scp, &one_sg);
-    gdth_set_sg_count(scp, 1);
-    gdth_set_bufflen(scp, sizeof(*gdtcmd));
     scp->cmd_len = 12;
     memcpy(scp->cmnd, cmnd, 12);
     cmndinfo.priority = IOCTL_PRI;
+    cmndinfo.internal_cmd_str = gdtcmd;
     cmndinfo.internal_command = 1;
 
     TRACE(("__gdth_execute() cmd 0x%x\n", scp->cmnd[0]));
@@ -2348,7 +2344,7 @@ static void gdth_next(gdth_ha_str *ha)
  * buffers, kmap_atomic() as needed.
  */
 static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
-                                    char *buffer, ushort count, int to_buffer)
+                                    char *buffer, ushort count)
 {
     ushort cpcount,i, max_sg = gdth_sg_count(scp);
     ushort cpsum,cpnow;
@@ -2374,10 +2370,7 @@ static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
             }
             local_irq_save(flags);
             address = kmap_atomic(sg_page(sl), KM_BIO_SRC_IRQ) + sl->offset;
-            if (to_buffer)
-                memcpy(buffer, address, cpnow);
-            else
-                memcpy(address, buffer, cpnow);
+            memcpy(address, buffer, cpnow);
             flush_dcache_page(sg_page(sl));
             kunmap_atomic(address, KM_BIO_SRC_IRQ);
             local_irq_restore(flags);
@@ -2431,7 +2424,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         strcpy(inq.vendor,ha->oem_name);
         sprintf(inq.product,"Host Drive  #%02d",t);
         strcpy(inq.revision,"   ");
-        gdth_copy_internal_data(ha, scp, (char*)&inq, sizeof(gdth_inq_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&inq, sizeof(gdth_inq_data));
         break;
 
       case REQUEST_SENSE:
@@ -2441,7 +2434,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         sd.key       = NO_SENSE;
         sd.info      = 0;
         sd.add_length= 0;
-        gdth_copy_internal_data(ha, scp, (char*)&sd, sizeof(gdth_sense_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&sd, sizeof(gdth_sense_data));
         break;
 
       case MODE_SENSE:
@@ -2453,7 +2446,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         mpd.bd.block_length[0] = (SECTOR_SIZE & 0x00ff0000) >> 16;
         mpd.bd.block_length[1] = (SECTOR_SIZE & 0x0000ff00) >> 8;
         mpd.bd.block_length[2] = (SECTOR_SIZE & 0x000000ff);
-        gdth_copy_internal_data(ha, scp, (char*)&mpd, sizeof(gdth_modep_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&mpd, sizeof(gdth_modep_data));
         break;
 
       case READ_CAPACITY:
@@ -2463,7 +2456,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         else
             rdc.last_block_no = cpu_to_be32(ha->hdr[t].size-1);
         rdc.block_length  = cpu_to_be32(SECTOR_SIZE);
-        gdth_copy_internal_data(ha, scp, (char*)&rdc, sizeof(gdth_rdcap_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&rdc, sizeof(gdth_rdcap_data));
         break;
 
       case SERVICE_ACTION_IN:
@@ -2475,7 +2468,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
             rdc16.last_block_no = cpu_to_be64(ha->hdr[t].size-1);
             rdc16.block_length  = cpu_to_be32(SECTOR_SIZE);
             gdth_copy_internal_data(ha, scp, (char*)&rdc16,
-                                                 sizeof(gdth_rdcap16_data), 0);
+                                                 sizeof(gdth_rdcap16_data));
         } else {
             scp->result = DID_ABORT << 16;
         }
@@ -2845,6 +2838,7 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
 static int gdth_special_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
 {
     register gdth_cmd_str *cmdp;
+    struct gdth_cmndinfo *cmndinfo = gdth_cmnd_priv(scp);
     int cmd_index;
 
     cmdp= ha->pccb;
@@ -2853,7 +2847,7 @@ static int gdth_special_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
     if (ha->type==GDT_EISA && ha->cmd_cnt>0)
         return 0;
 
-    gdth_copy_internal_data(ha, scp, (char *)cmdp, sizeof(gdth_cmd_str), 1);
+    *cmdp = *cmndinfo->internal_cmd_str;
     cmdp->RequestBuffer = scp;
 
     /* search free command index */
diff --git a/drivers/scsi/gdth.h b/drivers/scsi/gdth.h
index 1434c6b..26e4e92 100644
--- a/drivers/scsi/gdth.h
+++ b/drivers/scsi/gdth.h
@@ -915,6 +915,7 @@ typedef struct {
     struct gdth_cmndinfo {                      /* per-command private info */
         int index;
         int internal_command;                   /* don't call scsi_done */
+        gdth_cmd_str *internal_cmd_str;         /* crier for internal messages*/
         dma_addr_t sense_paddr;                 /* sense dma-addr */
         unchar priority;
         int timeout;
--
1.5.4.3


From ff83efacf2b77a1fe8942db6613825a4b80ee5e2 Mon Sep 17 00:00:00 2001
From: James Bottomley <[hidden email]>
Date: Sun, 17 Feb 2008 11:24:51 -0600
Subject: [PATCH] [SCSI] gdth: don't call pci_free_consistent under spinlock

Bug: #199934

picked from upstream ff83efacf2b77a1fe8942db6613825a4b80ee5e2

The spinlock is held over too large a region: pscratch is a permanent
address (it's allocated at boot time and never changes).  All you need
the smp lock for is mediating the scratch in use flag, so fix this by
moving the spinlock into the case where we set the pscratch_busy flag
to false.

Cc: Stable Tree <[hidden email]>
Signed-off-by: James Bottomley <[hidden email]>
Signed-off-by: Stefan Bader <[hidden email]>
---
 drivers/scsi/gdth_proc.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
index de57734..ce0228e 100644
--- a/drivers/scsi/gdth_proc.c
+++ b/drivers/scsi/gdth_proc.c
@@ -694,15 +694,13 @@ static void gdth_ioctl_free(gdth_ha_str *ha, int size, char *buf, ulong64 paddr)
 {
     ulong flags;
 
-    spin_lock_irqsave(&ha->smp_lock, flags);
-
     if (buf == ha->pscratch) {
+ spin_lock_irqsave(&ha->smp_lock, flags);
         ha->scratch_busy = FALSE;
+ spin_unlock_irqrestore(&ha->smp_lock, flags);
     } else {
         pci_free_consistent(ha->pdev, size, buf, paddr);
     }
-
-    spin_unlock_irqrestore(&ha->smp_lock, flags);
 }
 
 #ifdef GDTH_IOCTL_PROC
--
1.5.4.3


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [hardy] SRU reguest for LP#199934

Tim Gardner-2
Stefan Bader wrote:

> https://bugs.launchpad.net/ubuntu/+source/linux-meta/+bug/199934
>
> SRU justification:
>
> Impact: There are changes in the stable 2.6.24 tree that avoid problems with
> the gdth SCSI driver. Without these changes, the kernel will panic on reboot or
> when issuing certain control commands (see bug report)
>
> Fix: The fix to this problem has been included in the 2.6.24.y stable tree but
> still has one problem (WARN_ON) mentionen. There is a second quite straight
> forward patch upstream that will address this issue.
>
> Testcase: See bug report.
>

ACK - its a stable tree backport, shouldn't be much of an issue. It also
appears to fix the problem as stated in the LP report.

--
Tim Gardner [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [hardy] SRU reguest for LP#199934

Stefan Bader-2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tim Gardner wrote:

> Stefan Bader wrote:
>> https://bugs.launchpad.net/ubuntu/+source/linux-meta/+bug/199934
>>
>> SRU justification:
>>
>> Impact: There are changes in the stable 2.6.24 tree that avoid problems with
>> the gdth SCSI driver. Without these changes, the kernel will panic on reboot or
>> when issuing certain control commands (see bug report)
>>
>> Fix: The fix to this problem has been included in the 2.6.24.y stable tree but
>> still has one problem (WARN_ON) mentionen. There is a second quite straight
>> forward patch upstream that will address this issue.
>>
>> Testcase: See bug report.
>>
>
> ACK - its a stable tree backport, shouldn't be much of an issue. It also
> appears to fix the problem as stated in the LP report.
>
There is one part which is not in the stable tree, yet. That is the part that
moves the spinlock. But I found that simple and obvious enough (and it is part
of upstream).

Stefan

- --

When all other means of communication fail, try words!


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIqX1AP+TjRTJVqvQRArNRAKCXH/41cfZkrEudTXH4J0fNy35j0wCdH7hm
fBi/0RPH42/5ayOkw+KUny0=
=KjBQ
-----END PGP SIGNATURE-----

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [hardy] SRU reguest for LP#199934

Stefan Bader-2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stefan Bader wrote:

> Tim Gardner wrote:
>> Stefan Bader wrote:
>>> https://bugs.launchpad.net/ubuntu/+source/linux-meta/+bug/199934
>>>
>>> SRU justification:
>>>
>>> Impact: There are changes in the stable 2.6.24 tree that avoid problems with
>>> the gdth SCSI driver. Without these changes, the kernel will panic on reboot or
>>> when issuing certain control commands (see bug report)
>>>
>>> Fix: The fix to this problem has been included in the 2.6.24.y stable tree but
>>> still has one problem (WARN_ON) mentionen. There is a second quite straight
>>> forward patch upstream that will address this issue.
>>>
>>> Testcase: See bug report.
>>>
>> ACK - its a stable tree backport, shouldn't be much of an issue. It also
>> appears to fix the problem as stated in the LP report.
>
> There is one part which is not in the stable tree, yet. That is the part that
> moves the spinlock. But I found that simple and obvious enough (and it is part
> of upstream).
>
> Stefan
>
No forget what I said. I assumed the spinlock patch must not be in since it was
referenced in the fix and after that there wasn't anything. But in fact that
one comes before the other. I suspect thy might have been done independently.
So both parts are in stable.

Stefan
- --

When all other means of communication fail, try words!


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIqYioP+TjRTJVqvQRAkzQAJ0U1YIOizr/g/4BoYWZlixFhmsitgCgo4Ek
qKGqfvYLN30dB7UPCkjGVM0=
=6P0+
-----END PGP SIGNATURE-----

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team