Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure
Mike Christie wrote: Mike Christie wrote: For drivers like sg and st, do mean the the sg list that is passed to functions like scsi_execute_async? If we kill that argument, and instead have sg.c and other scsi_execute_async callers just call blk helpers like blk_rq_map_user then we would not have to worry about drivers like sg needing to know about chaining right? I mean sg.c would not every interact with a scatterlist. It would just interact with a request and the blk helpers map data for it. There should be a return there. The scatterlist that sg and st interact with is bogus. It gets thrown away in scsi_execute_async and is only used for book keeping. I mean currently the scatterlist that sg and st use is bogus and gets thrown away. If we convert sg and st to use blk_rq_map_user then those drivers will not have to interact with a scatterlist at all. I'm not familiar with the dire details but this sounds like a good idea. Benny - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] zfcp: Avoid race condition on link up event
On Wednesday 08 August 2007 10:47, Swen Schillig wrote:
From: Christoph Schmitt [EMAIL PROTECTED]
Symptom: zfcp receives a response to a status read request
that is no longer valid in zfcp. This leads to a
kernel panic, since some memory has been overwritten
by the response reporting.
Problem: On receiving an unsolicited status, zfcp issues a
new status read request. On receiving the
unsolicited status link up, zfcp triggers an adapter
reopen. The new status read request and the reopen
can lead to a race where zfcp issues the request before
the reopen, but the hardware handles the reopen first.
Solution:Not issue the status read request to avoid the race
condition. The adapter reopen will enqueue 16 new
status read requests anyway.
Signed-off-by: Christoph Schmitt [EMAIL PROTECTED]
Signed-off-by: Martin Schwidefsky [EMAIL PROTECTED]
Signed-off-by: Swen Schillig [EMAIL PROTECTED]
---
drivers/s390/scsi/zfcp_fsf.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff -urpN linux-2.6/drivers/s390/scsi/zfcp_fsf.c
linux-2.6-patched/drivers/s390/scsi/zfcp_fsf.c
--- linux-2.6/drivers/s390/scsi/zfcp_fsf.c2007-08-08 10:13:39.0
+0200
+++ linux-2.6-patched/drivers/s390/scsi/zfcp_fsf.c2007-08-08
10:14:03.0 +0200
@@ -33,7 +33,7 @@ static int zfcp_fsf_send_fcp_command_tas
static int zfcp_fsf_send_fcp_command_task_management_handler(
struct zfcp_fsf_req *);
static int zfcp_fsf_abort_fcp_command_handler(struct zfcp_fsf_req *);
-static int zfcp_fsf_status_read_handler(struct zfcp_fsf_req *);
+static void zfcp_fsf_status_read_handler(struct zfcp_fsf_req *);
static int zfcp_fsf_send_ct_handler(struct zfcp_fsf_req *);
static int zfcp_fsf_send_els_handler(struct zfcp_fsf_req *);
static int zfcp_fsf_control_file_handler(struct zfcp_fsf_req *);
@@ -856,10 +856,10 @@ zfcp_fsf_status_read_port_closed(struct
*
* returns:
*/
-static int
+static void
zfcp_fsf_status_read_handler(struct zfcp_fsf_req *fsf_req)
{
- int retval = 0;
+ int retval;
struct zfcp_adapter *adapter = fsf_req-adapter;
struct fsf_status_read_buffer *status_buffer =
(struct fsf_status_read_buffer *) fsf_req-data;
@@ -869,7 +869,7 @@ zfcp_fsf_status_read_handler(struct zfcp
zfcp_hba_dbf_event_fsf_unsol(dism, adapter, status_buffer);
mempool_free(status_buffer, adapter-pool.data_status_read);
zfcp_fsf_req_free(fsf_req);
- goto out;
+ return;
}
zfcp_hba_dbf_event_fsf_unsol(read, adapter, status_buffer);
@@ -1061,6 +1061,15 @@ zfcp_fsf_status_read_handler(struct zfcp
* FIXME:
* allocation failure possible? (Is this code needed?)
*/
+ /*
+ * If we triggered an adapter reopen, then the reopen will also
+ * enqueue new status read requests. Not issuing a status read
+ * here avoids a race between the request send and the adapter
+ * reopen.
+ */
+ if (status_buffer-status_type == FSF_STATUS_READ_LINK_UP)
+ return;
+
retval = zfcp_fsf_status_read(adapter, 0);
if (retval 0) {
ZFCP_LOG_INFO(Failed to create unsolicited status read
@@ -1076,8 +1085,6 @@ zfcp_fsf_status_read_handler(struct zfcp
zfcp_erp_adapter_reopen(adapter, 0);
}
}
- out:
- return retval;
}
/*
James
please dump this patch.
The description is a bit misleading and the issue is solved within our
microcode.
Thanks
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure
On Wed, 08 Aug 2007 11:58:14 -0500 Mike Christie [EMAIL PROTECTED] wrote: FUJITA Tomonori wrote: On Tue, 07 Aug 2007 12:13:41 -0500 Mike Christie [EMAIL PROTECTED] wrote: FUJITA Tomonori wrote: Allocating 64K contiguous memory is not good so the next thing to do is converting sg to use the sg chaining support fully. Or it might be For LLDs like aic7xxx, I think we are stuck with a small scsi_host_template-sg_tablesize, so to continue to get large requests like before will we have to still allocate large segments? No. sg.c has: sizeof(struct scatterlist) * min(q-max_hw_segments, q-max_phys_segments) If a lld has small max_hw_segments, it doesn't allocate big contiguous memory. Are we talking about the same thing? Are you saying that it does not allocate big continuous memory for the scatterlist right? I was asking about continuous memory for the buffer sg.c copies data to/from for the IO operation. I was saying that currently for something like aic if we want to continue to support 8 MB commands (or whatever it was) like before, then because its sg_tablesize/max_hw_segments is so small we have to continue allocating large IO buffers. That did not change right? If so let me know because you save me :) Oops, sorry. I think that nothing changes about large IO buffers. You have to contiunue to allocate large IO buffers like before. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sg: increase sglist_len of the sg_scatter_hold structure
On Wed, 08 Aug 2007 12:20:43 -0500 Mike Christie [EMAIL PROTECTED] wrote: Mike Christie wrote: For drivers like sg and st, do mean the the sg list that is passed to functions like scsi_execute_async? If we kill that argument, and instead have sg.c and other scsi_execute_async callers just call blk helpers like blk_rq_map_user then we would not have to worry about drivers like sg needing to know about chaining right? I mean sg.c would not every interact with a scatterlist. It would just interact with a request and the blk helpers map data for it. There should be a return there. The scatterlist that sg and st interact with is bogus. It gets thrown away in scsi_execute_async and is only used for book keeping. I mean currently the scatterlist that sg and st use is bogus and gets thrown away. If we convert sg and st to use blk_rq_map_user then those drivers will not have to interact with a scatterlist at all. Yeah, we should kill scsi_execute_async. sg should not be the consumer even if the block layer has functions to allocate chaining sg. Right now I'm happy as long as scsi-ml has the simple routine to allocate chaining sg like Jens's branch. So we might easily move it to the block layer or the block layer might just take care of the sg list for scsi-ml, etc in the future. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/4] Expose Power Management Policy option to users
On Wed, 1 Aug 2007 14:16:51 -0700
Kristen Carlson Accardi [EMAIL PROTECTED] wrote:
I was able to duplicate Tejun's problem on an ATAPI device I had here.
this updated patch fixed my problem. These devices are just setting
PhyReady (N) and CommWake (W) in Serror on power state changes, ignoring
them seems to be fine, and fixed the problem for me.
Hi Tejun,
Were you able to test out this patch and see if it solved your ATAPI
device/ALPM issues?
Thanks,
Kristen
Enable Aggressive Link Power management for AHCI controllers.
This patch will set the correct bits to turn on Aggressive
Link Power Management (ALPM) for the ahci driver. This
will cause the controller and disk to negotiate a lower
power state for the link when there is no activity (see
the AHCI 1.x spec for details). This feature is mutually
exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
is disabled. ALPM will be enabled by default, but it is
settable via the scsi host syfs interface. Possible
settings for this feature are:
Setting Effect
--
min_power ALPM is enabled, and link set to enter
lowest power state (SLUMBER) when idle
Hot plug not allowed.
max_performance ALPM is disabled, Hot Plug is allowed
medium_power ALPM is enabled, and link set to enter
second lowest power state (PARTIAL) when
idle. Hot plug not allowed.
Signed-off-by: Kristen Carlson Accardi [EMAIL PROTECTED]
Index: 2.6-git/drivers/ata/ahci.c
===
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -48,6 +48,9 @@
#define DRV_NAME ahci
#define DRV_VERSION 2.3
+static int ahci_enable_alpm(struct ata_port *ap,
+ enum link_pm policy);
+static int ahci_disable_alpm(struct ata_port *ap);
enum {
AHCI_PCI_BAR= 5,
@@ -98,6 +101,7 @@ enum {
/* HOST_CAP bits */
HOST_CAP_SSC= (1 14), /* Slumber capable */
HOST_CAP_CLO= (1 24), /* Command List Override support */
+ HOST_CAP_ALPM = (1 26), /* Aggressive Link PM support */
HOST_CAP_SSS= (1 27), /* Staggered Spin-up */
HOST_CAP_SNTF = (1 29), /* SNotification register */
HOST_CAP_NCQ= (1 30), /* Native Command Queueing */
@@ -153,6 +157,8 @@ enum {
PORT_IRQ_PIOS_FIS | PORT_IRQ_D2H_REG_FIS,
/* PORT_CMD bits */
+ PORT_CMD_ASP= (1 27), /* Aggressive Slumber/Partial */
+ PORT_CMD_ALPE = (1 26), /* Aggressive Link PM enable */
PORT_CMD_ATAPI = (1 24), /* Device is ATAPI */
PORT_CMD_LIST_ON= (1 15), /* cmd list DMA engine running */
PORT_CMD_FIS_ON = (1 14), /* FIS DMA engine running */
@@ -244,6 +250,11 @@ static int ahci_pci_device_suspend(struc
static int ahci_pci_device_resume(struct pci_dev *pdev);
#endif
+static struct class_device_attribute *ahci_shost_attrs[] = {
+ class_device_attr_link_power_management_policy,
+ NULL
+};
+
static struct scsi_host_template ahci_sht = {
.module = THIS_MODULE,
.name = DRV_NAME,
@@ -261,6 +272,7 @@ static struct scsi_host_template ahci_sh
.slave_configure= ata_scsi_slave_config,
.slave_destroy = ata_scsi_slave_destroy,
.bios_param = ata_std_bios_param,
+ .shost_attrs= ahci_shost_attrs,
};
static const struct ata_port_operations ahci_ops = {
@@ -292,6 +304,8 @@ static const struct ata_port_operations
.port_suspend = ahci_port_suspend,
.port_resume= ahci_port_resume,
#endif
+ .enable_pm = ahci_enable_alpm,
+ .disable_pm = ahci_disable_alpm,
.port_start = ahci_port_start,
.port_stop = ahci_port_stop,
@@ -778,6 +792,156 @@ static void ahci_power_up(struct ata_por
writel(cmd | PORT_CMD_ICC_ACTIVE, port_mmio + PORT_CMD);
}
+static int ahci_disable_alpm(struct ata_port *ap)
+{
+ void __iomem *port_mmio = ahci_port_base(ap);
+ u32 cmd, scontrol;
+ struct ahci_port_priv *pp = ap-private_data;
+
+ /*
+ * disable Interface Power Management State Transitions
+ * This is accomplished by setting bits 8:11 of the
+ * SATA Control register
+ */
+ scontrol = readl(port_mmio + PORT_SCR_CTL);
+ scontrol |= (0x3 8);
+ writel(scontrol, port_mmio + PORT_SCR_CTL);
+
+ /* get the existing command bits */
+ cmd = readl(port_mmio + PORT_CMD);
+
+ /* disable ALPM and ASP */
+ cmd = ~PORT_CMD_ASP;
+ cmd = ~PORT_CMD_ALPE;
+
+ /* force the interface back to active */
+ cmd |= PORT_CMD_ICC_ACTIVE;
[PATCH] SCSI: Get SG_ALL out of the way
Jens hi playing with sglist and iscsi I stumbled on this. You might want to add it to the sglist tree. Or maybe James wants to include it for next merge window. === - Some 50 scsi drivers use SG_ALL for their .sg_tablesize So a value of 255 is a bit specific. Change it to ~0 as meaning MAX_ what ever unsigned type it will ever be. Signed-off-by: Boaz Harrosh [EMAIL PROTECTED] --- include/scsi/scsi_host.h |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 88f6871..b84a4eb 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -30,7 +30,7 @@ struct blk_queue_tags; * used in one scatter-gather request. */ #define SG_NONE 0 -#define SG_ALL 0xff +#define SG_ALL (~0) #define DISABLE_CLUSTERING 0 -- 1.5.2.2.249.g45fd - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: Get SG_ALL out of the way
Boaz Harrosh wrote: Jens hi playing with sglist and iscsi I stumbled on this. You might want to add it to the sglist tree. Or maybe James wants to include it for next merge window. === - Some 50 scsi drivers use SG_ALL for their .sg_tablesize So a value of 255 is a bit specific. Change it to ~0 as meaning MAX_ what ever unsigned type it will ever be. Signed-off-by: Boaz Harrosh [EMAIL PROTECTED] --- include/scsi/scsi_host.h |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 88f6871..b84a4eb 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -30,7 +30,7 @@ struct blk_queue_tags; *used in one scatter-gather request. */ #define SG_NONE 0 -#define SG_ALL 0xff +#define SG_ALL (~0) #define DISABLE_CLUSTERING 0 Sorry about that patch. I just did a make allmodconfig and it appears that some drivers use this constant as an Array size. I will submit a new Patch. Boaz - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Emulex FC HBA driver: fix overflow of statically allocated array
Hi,
The Coverity checker noticed that we may overrun a statically allocated
array in drivers/scsi/lpfc/lpfc_sli.c::lpfc_sli_hbqbuf_find().
The case is this; In 'struct lpfc_hba' we have
#define LPFC_MAX_HBQS 4
...
struct lpfc_hba {
...
struct hbq_s hbqs[LPFC_MAX_HBQS];
...
};
But then in lpfc_sli_hbqbuf_find() we have this code
hbqno = tag 16;
if (hbqno LPFC_MAX_HBQS)
return NULL;
if 'hbqno' ends up as exactely 4, then we won't return, and then this
list_for_each_entry(d_buf, phba-hbqs[hbqno].hbq_buffer_list, list) {
will cause an overflow of the statically allocated array at index 4,
since the valid indices are only 0-3.
I propose this patch, that simply changes the 'hbqno LPFC_MAX_HBQS'
into 'hbqno = LPFC_MAX_HBQS' as a possible fix.
Signed-off-by: Jesper Juhl [EMAIL PROTECTED]
---
drivers/scsi/lpfc/lpfc_sli.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index ce5ff2b..e5337ad 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -675,7 +675,7 @@ lpfc_sli_hbqbuf_find(struct lpfc_hba *phba, uint32_t tag)
uint32_t hbqno;
hbqno = tag 16;
- if (hbqno LPFC_MAX_HBQS)
+ if (hbqno = LPFC_MAX_HBQS)
return NULL;
list_for_each_entry(d_buf, phba-hbqs[hbqno].hbq_buffer_list, list) {
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Emulex FC HBA driver: fix overflow of statically allocated array
ACK -- james s Jesper Juhl wrote: I propose this patch, that simply changes the 'hbqno LPFC_MAX_HBQS' into 'hbqno = LPFC_MAX_HBQS' as a possible fix. Signed-off-by: Jesper Juhl [EMAIL PROTECTED] --- drivers/scsi/lpfc/lpfc_sli.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
