Re: [PATCH 2/2 2.6.29-rc] cxgb3i - add handling of chip reset
Karen Xie wrote:
[PATCH 2/2 2.6.29-rc] cxgb3i - add support of chip reset
From: Karen Xie k...@chelsio.com
The cxgb3 driver would reset the chip due to an EEH event or a fatal error
and notifies the upper layer modules (iscsi, rdma).
Upon receiving the notification the cxgb3i driver would
- close all the iscsi tcp connections and mark the associated sessions as
failed due to connection error,
- re-initialize its offload functions,
- re-initialize the chip's ddp functions.
The iscsi error recovery mechanism will re-establish the tcp connection after
reset.
This patch requires the the following cxgb3 patch in the linux-next git tree
(http://git.kernel.org/?p=linux/kernel/git/sfr/linux-next.git;a=commitdiff;h=cb0bc205959bf8c60acae9c71f3da0597e756f8e).
static inline int ddp_find_unused_entries(struct cxgb3i_ddp_info *ddp,
@@ -439,14 +444,15 @@ EXPORT_SYMBOL_GPL(cxgb3i_ddp_tag_reserve);
* @tag: ddp tag
* ddp cleanup for a given ddp tag and release all the resources held
*/
-void cxgb3i_ddp_tag_release(struct t3cdev *tdev, u32 tag)
+int cxgb3i_ddp_tag_release(struct t3cdev *tdev, u32 tag)
{
struct cxgb3i_ddp_info *ddp = tdev-ulp_iscsi;
u32 idx;
+ int err = -EINVAL;
if (!ddp) {
ddp_log_error(release ddp tag 0x%x, ddp NULL.\n, tag);
- return;
+ return err;
}
idx = (tag PPOD_IDX_SHIFT) ddp-idx_mask;
@@ -457,17 +463,26 @@ void cxgb3i_ddp_tag_release(struct t3cdev *tdev, u32
tag)
if (!gl) {
ddp_log_error(release ddp 0x%x, idx 0x%x, gl NULL.\n,
tag, idx);
- return;
+ return err;
}
npods = (gl-nelem + PPOD_PAGES_MAX - 1) PPOD_PAGES_SHIFT;
+ if (!npods) {
+ ddp_log_error(release ddp 0x%x, 0x%x, gl elem %u.\n,
+ tag, idx, gl-nelem);
+ return err;
+ }
ddp_log_debug(ddp tag 0x%x, release idx 0x%x, npods %u.\n,
tag, idx, npods);
- clear_ddp_map(ddp, idx, npods);
+ if (clear_ddp_map(ddp, tag, idx, npods) == npods)
+ err = 0;
ddp_unmark_entries(ddp, idx, npods);
cxgb3i_ddp_release_gl(gl, ddp-pdev);
If this fails, do we leak this memory?
When is ddp_release called? Would it be called to clean up in situati
this could fail?
+/**
+ * cxgb3i_adapter_remove - release all the resources held and cleanup any
+ * h/w settings
The docbook comment for the function description should only be one
line. I think you did this in some other places.
-void cxgb3i_conn_closing(struct s3_conn *c3cn)
+void cxgb3i_conn_closing(struct s3_conn *c3cn, int error)
{
struct iscsi_conn *conn;
read_lock(c3cn-callback_lock);
conn = c3cn-user_data;
- if (conn c3cn-state != C3CN_STATE_ESTABLISHED)
- iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED);
+ if (conn c3cn-state != C3CN_STATE_ESTABLISHED) {
+ if (error)
+ iscsi_session_failure(conn-session,
+ ISCSI_ERR_CONN_FAILED);
+ else
+ iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED);
+ }
read_unlock(c3cn-callback_lock);
If you have a ref to the conn, you can just use iscsi_conn_failure here.
I thought when you got the pci error event you were just going to use
the host session iter and loop over the session and fail them.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH 2/2 2.6.29-rc] cxgb3i - add handling of chip reset
Some other comments about the host and snic allocation.
Karen Xie wrote:
+/**
+ * cxgb3i_adapter_open - initiate or update a s3 adapter structure and
+ * any h/w settings
+ * @t3dev: t3cdev adapter
+ */
+static inline int adapter_update(struct cxgb3i_adapter *snic)
The function name and comment got out of sync.
-void cxgb3i_adapter_remove(struct t3cdev *t3dev)
+void cxgb3i_adapter_open(struct t3cdev *t3dev)
{
- int i;
- struct cxgb3i_adapter *snic;
+ struct cxgb3i_adapter *snic = cxgb3i_adapter_find_by_tdev(t3dev);
+ int err;
- /* remove from the list */
- write_lock(cxgb3i_snic_rwlock);
- list_for_each_entry(snic, cxgb3i_snic_list, list_head) {
- if (snic-tdev == t3dev) {
- list_del(snic-list_head);
- break;
- }
+ if (snic)
+ err = adapter_update(snic);
+ else {
+ snic = kzalloc(sizeof(*snic), GFP_KERNEL);
+ if (snic) {
+ spin_lock_init(snic-lock);
+ snic-tdev = t3dev;
+ err = adapter_add(snic);
+ } else
+ err = -ENOMEM;
Does the snic represent the actual card. So if I had a dual ported card,
I would have one snic and then have multple shosts/cxgb3i_hba for each
port right?
It seemed strange because cxgb3i_hba_host_add sets the shost's parent to
the pci device, and we get the pci device from the snic-pdev. And so I
thought we see a pci device per port, so we would also should have a
snic per port?
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
RE: [PATCH 2/2 2.6.29-rc] cxgb3i - add handling of chip reset
-Original Message-
From: Mike Christie [mailto:micha...@cs.wisc.edu]
Sent: Wednesday, February 25, 2009 9:30 AM
To: Karen Xie
Cc: open-iscsi@googlegroups.com; linux-s...@vger.kernel.org;
james.bottom...@hansenpartnership.com
Subject: Re: [PATCH 2/2 2.6.29-rc] cxgb3i - add handling of chip reset
Some other comments about the host and snic allocation.
Karen Xie wrote:
+/**
+ * cxgb3i_adapter_open - initiate or update a s3 adapter structure
and
+ * any h/w settings
+ * @t3dev: t3cdev adapter
+ */
+static inline int adapter_update(struct cxgb3i_adapter *snic)
The function name and comment got out of sync.
[Karen] Thanks, will fix it.
-void cxgb3i_adapter_remove(struct t3cdev *t3dev)
+void cxgb3i_adapter_open(struct t3cdev *t3dev)
{
- int i;
- struct cxgb3i_adapter *snic;
+ struct cxgb3i_adapter *snic =
cxgb3i_adapter_find_by_tdev(t3dev);
+ int err;
- /* remove from the list */
- write_lock(cxgb3i_snic_rwlock);
- list_for_each_entry(snic, cxgb3i_snic_list, list_head) {
- if (snic-tdev == t3dev) {
- list_del(snic-list_head);
- break;
- }
+ if (snic)
+ err = adapter_update(snic);
+ else {
+ snic = kzalloc(sizeof(*snic), GFP_KERNEL);
+ if (snic) {
+ spin_lock_init(snic-lock);
+ snic-tdev = t3dev;
+ err = adapter_add(snic);
+ } else
+ err = -ENOMEM;
Does the snic represent the actual card. So if I had a dual ported card,
I would have one snic and then have multple shosts/cxgb3i_hba for each
port right?
[Karen] Yes.
It seemed strange because cxgb3i_hba_host_add sets the shost's parent to
the pci device, and we get the pci device from the snic-pdev. And so I
thought we see a pci device per port, so we would also should have a
snic per port?
[Karen] Each port/cxgb3i_hba has a pointer point back to the
snic/adapter. So one snic per adapter, each snic contains one or more
ports, each port corresponds to one shost.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
