public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] zfcp: updates for -bk
@ 2005-01-24  9:46 Heiko Carstens
  2005-01-24 14:22 ` Matthew Wilcox
  0 siblings, 1 reply; 8+ messages in thread
From: Heiko Carstens @ 2005-01-24  9:46 UTC (permalink / raw)
  To: James Bottomley, SCSI Mailing List

Hi James,

could you please apply the patch below?

Thanks,
Heiko

From: Heiko Carstens <heiko.carstens@de.ibm.com>
From: Andreas Herrmann <aherrman@de.ibm.com>
From: Maxim Shchetynin <maxim@de.ibm.com>

zfcp changes:
 - revert kfree patch leftovers
 - don't call del_timer_sync() in interrupt context
 - correct residual count handling for data underruns
 - mark LUN as ACCESS_DENIED on status LUN_SHARING_VIOLATION

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

diffstat:
 drivers/s390/scsi/zfcp_aux.c        |   37 ++++++++++++++++++++++--------------
 drivers/s390/scsi/zfcp_def.h        |   13 +-----------
 drivers/s390/scsi/zfcp_erp.c        |    6 ++---
 drivers/s390/scsi/zfcp_ext.h        |    2 -
 drivers/s390/scsi/zfcp_fsf.c        |   31 ++++++++++++++++--------------
 drivers/s390/scsi/zfcp_sysfs_port.c |    7 ++++--
 drivers/s390/scsi/zfcp_sysfs_unit.c |    7 ++++--
 7 files changed, 56 insertions(+), 47 deletions(-)

diff -urN a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
--- a/drivers/s390/scsi/zfcp_aux.c	2004-12-24 22:35:49.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_aux.c	2005-01-24 09:45:48.000000000 +0100
@@ -29,7 +29,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#define ZFCP_AUX_REVISION "$Revision: 1.145 $"
+#define ZFCP_AUX_REVISION "$Revision: 1.147 $"
 
 #include "zfcp_ext.h"
 
@@ -1099,9 +1099,9 @@
 }
 
 void
-zfcp_dummy_release(struct device *dev)
+zfcp_generic_services_release(struct device *dev)
 {
-	return;
+	kfree(dev);
 }
 
 /*
@@ -1119,6 +1119,7 @@
 {
 	int retval = 0;
 	struct zfcp_adapter *adapter;
+	struct device *gs;
 
 	/*
 	 * Note: It is safe to release the list_lock, as any list changes 
@@ -1195,13 +1196,19 @@
 	if (zfcp_sysfs_adapter_create_files(&ccw_device->dev))
 		goto sysfs_failed;
 
-	adapter->generic_services.parent = &adapter->ccw_device->dev;
-	adapter->generic_services.release = zfcp_dummy_release;
-	snprintf(adapter->generic_services.bus_id, BUS_ID_SIZE,
-		 "generic_services");
+	gs = kmalloc(sizeof(struct device), GFP_KERNEL);
+	if (!gs)
+		goto gs_failed;
+	memset(gs, 0, sizeof(struct device));
+
+	gs->parent = &adapter->ccw_device->dev;
+	gs->release = &zfcp_generic_services_release;
+	snprintf(gs->bus_id, BUS_ID_SIZE, "generic_services");
+
+	if (device_register(gs))
+		goto gs_free;
 
-	if (device_register(&adapter->generic_services))
-		goto generic_services_failed;
+	adapter->generic_services = gs;
 
 	/* put allocated adapter at list tail */
 	write_lock_irq(&zfcp_data.config_lock);
@@ -1213,7 +1220,9 @@
 
 	goto out;
 
- generic_services_failed:
+ gs_free:
+	kfree(gs);
+ gs_failed:
 	zfcp_sysfs_adapter_remove_files(&adapter->ccw_device->dev);
  sysfs_failed:
 	dev_set_drvdata(&ccw_device->dev, NULL);
@@ -1245,7 +1254,7 @@
 	int retval = 0;
 	unsigned long flags;
 
-	device_unregister(&adapter->generic_services);
+	device_unregister(adapter->generic_services);
 	zfcp_sysfs_adapter_remove_files(&adapter->ccw_device->dev);
 	dev_set_drvdata(&adapter->ccw_device->dev, NULL);
 	/* sanity check: no pending FSF requests */
@@ -1370,13 +1379,13 @@
 			return NULL;
 		}
 		port->d_id = d_id;
-		port->sysfs_device.parent = &adapter->generic_services;
+		port->sysfs_device.parent = adapter->generic_services;
 	} else {
 		snprintf(port->sysfs_device.bus_id,
 			 BUS_ID_SIZE, "0x%016llx", wwpn);
-	port->sysfs_device.parent = &adapter->ccw_device->dev;
+		port->sysfs_device.parent = &adapter->ccw_device->dev;
 	}
-	port->sysfs_device.release = zfcp_sysfs_port_release;
+	port->sysfs_device.release = &zfcp_sysfs_port_release;
 	dev_set_drvdata(&port->sysfs_device, port);
 
 	/* mark port unusable as long as sysfs registration is not complete */
diff -urN a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
--- a/drivers/s390/scsi/zfcp_def.h	2004-12-24 22:34:58.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_def.h	2005-01-24 09:47:52.000000000 +0100
@@ -34,7 +34,7 @@
 #ifndef ZFCP_DEF_H
 #define ZFCP_DEF_H
 
-#define ZFCP_DEF_REVISION "$Revision: 1.111 $"
+#define ZFCP_DEF_REVISION "$Revision: 1.115 $"
 
 /*************************** INCLUDES *****************************************/
 
@@ -903,14 +903,9 @@
 	spinlock_t              dbf_lock;
 	struct zfcp_adapter_mempool	pool;      /* Adapter memory pools */
 	struct qdio_initialize  qdio_init_data;    /* for qdio_establish */
-	struct device           generic_services;  /* directory for WKA ports */
+	struct device           *generic_services; /* directory for WKA ports */
 };
 
-/*
- * the struct device sysfs_device must be at the beginning of this structure.
- * pointer to struct device is used to free port structure in release function
- * of the device. don't change!
- */
 struct zfcp_port {
 	struct device          sysfs_device;   /* sysfs device */
 	struct list_head       list;	       /* list of remote ports */
@@ -932,10 +927,6 @@
         atomic_t               erp_counter;
 };
 
-/* the struct device sysfs_device must be at the beginning of this structure.
- * pointer to struct device is used to free unit structure in release function
- * of the device. don't change!
- */
 struct zfcp_unit {
 	struct device          sysfs_device;   /* sysfs device */
 	struct list_head       list;	       /* list of logical units */
diff -urN a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
--- a/drivers/s390/scsi/zfcp_erp.c	2004-12-24 22:35:50.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_erp.c	2005-01-24 09:47:52.000000000 +0100
@@ -31,7 +31,7 @@
 
 #define ZFCP_LOG_AREA			ZFCP_LOG_AREA_ERP
 
-#define ZFCP_ERP_REVISION "$Revision: 1.85 $"
+#define ZFCP_ERP_REVISION "$Revision: 1.86 $"
 
 #include "zfcp_ext.h"
 
@@ -369,7 +369,7 @@
 		ZFCP_LOG_NORMAL("error: initiation of Send ELS failed for port "
 				"0x%08x on adapter %s\n", d_id,
 				zfcp_get_busid_by_adapter(adapter));
-		del_timer_sync(send_els->timer);
+		del_timer(send_els->timer);
 		goto freemem;
 	}
 
@@ -969,7 +969,7 @@
 		debug_event(adapter->erp_dbf, 2, &erp_action->action,
 			    sizeof (int));
 		if (!(set_mask & ZFCP_STATUS_ERP_TIMEDOUT))
-			del_timer_sync(&erp_action->timer);
+			del_timer(&erp_action->timer);
 		erp_action->status |= set_mask;
 		zfcp_erp_action_ready(erp_action);
 		retval = 0;
diff -urN a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
--- a/drivers/s390/scsi/zfcp_ext.h	2004-12-24 22:34:26.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_ext.h	2005-01-24 09:45:48.000000000 +0100
@@ -32,7 +32,7 @@
 #ifndef ZFCP_EXT_H
 #define ZFCP_EXT_H
 
-#define ZFCP_EXT_REVISION "$Revision: 1.62 $"
+#define ZFCP_EXT_REVISION "$Revision: 1.63 $"
 
 #include "zfcp_def.h"
 
diff -urN a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
--- a/drivers/s390/scsi/zfcp_fsf.c	2004-12-24 22:35:23.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_fsf.c	2005-01-24 09:47:52.000000000 +0100
@@ -30,7 +30,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#define ZFCP_FSF_C_REVISION "$Revision: 1.88 $"
+#define ZFCP_FSF_C_REVISION "$Revision: 1.91 $"
 
 #include "zfcp_ext.h"
 
@@ -3203,7 +3203,9 @@
 			      sizeof (union fsf_status_qual));
 		debug_text_event(adapter->erp_dbf, 2,
 				 "fsf_s_l_sh_vio");
-		zfcp_erp_unit_failed(unit);
+		zfcp_erp_unit_access_denied(unit);
+		atomic_clear_mask(ZFCP_STATUS_UNIT_SHARED, &unit->status);
+		atomic_clear_mask(ZFCP_STATUS_UNIT_READONLY, &unit->status);
 		fsf_req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 
@@ -4320,22 +4322,23 @@
 
 	/* check for underrun */
 	if (unlikely(fcp_rsp_iu->validity.bits.fcp_resid_under)) {
-		ZFCP_LOG_DEBUG("A data underrun was detected for a command. "
-			       "unit 0x%016Lx, port 0x%016Lx, adapter %s. "
-			       "The response data length is "
-			       "%d, the original length was %d.\n",
-			       unit->fcp_lun,
-			       unit->port->wwpn,
-			       zfcp_get_busid_by_unit(unit),
-			       fcp_rsp_iu->fcp_resid,
-			       (int) zfcp_get_fcp_dl(fcp_cmnd_iu));
+		ZFCP_LOG_INFO("A data underrun was detected for a command. "
+			      "unit 0x%016Lx, port 0x%016Lx, adapter %s. "
+			      "The response data length is "
+			      "%d, the original length was %d.\n",
+			      unit->fcp_lun,
+			      unit->port->wwpn,
+			      zfcp_get_busid_by_unit(unit),
+			      fcp_rsp_iu->fcp_resid,
+			      (int) zfcp_get_fcp_dl(fcp_cmnd_iu));
 		/*
 		 * It may not have been possible to send all data and the
 		 * underrun on send may already be in scpnt->resid, so it's add
 		 * not equals in the below statement.
 		 */
-		scpnt->resid += fcp_rsp_iu->fcp_resid;
-		ZFCP_LOG_TRACE("scpnt->resid=0x%x\n", scpnt->resid);
+		scpnt->resid = fcp_rsp_iu->fcp_resid;
+		if (scpnt->request_bufflen - scpnt->resid < scpnt->underflow)
+			scpnt->result |= DID_ERROR << 16;
 	}
 
  skip_fsfstatus:
@@ -5023,7 +5026,7 @@
 		 * timer might be expired (absolutely unlikely)
 		 */
 		if (timer)
-			del_timer_sync(timer);
+			del_timer(timer);
 		write_lock_irqsave(&adapter->fsf_req_list_lock, flags);
 		list_del(&fsf_req->list);
 		write_unlock_irqrestore(&adapter->fsf_req_list_lock, flags);
diff -urN a/drivers/s390/scsi/zfcp_sysfs_port.c b/drivers/s390/scsi/zfcp_sysfs_port.c
--- a/drivers/s390/scsi/zfcp_sysfs_port.c	2004-12-24 22:35:28.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_sysfs_port.c	2005-01-24 09:45:48.000000000 +0100
@@ -28,7 +28,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#define ZFCP_SYSFS_PORT_C_REVISION "$Revision: 1.47 $"
+#define ZFCP_SYSFS_PORT_C_REVISION "$Revision: 1.49 $"
 
 #include "zfcp_ext.h"
 
@@ -41,7 +41,10 @@
 void
 zfcp_sysfs_port_release(struct device *dev)
 {
-	kfree(dev);
+	struct zfcp_port *port;
+
+	port = dev_get_drvdata(dev);
+	kfree(port);
 }
 
 /**
diff -urN a/drivers/s390/scsi/zfcp_sysfs_unit.c b/drivers/s390/scsi/zfcp_sysfs_unit.c
--- a/drivers/s390/scsi/zfcp_sysfs_unit.c	2004-12-24 22:35:50.000000000 +0100
+++ b/drivers/s390/scsi/zfcp_sysfs_unit.c	2005-01-24 09:45:48.000000000 +0100
@@ -28,7 +28,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#define ZFCP_SYSFS_UNIT_C_REVISION "$Revision: 1.30 $"
+#define ZFCP_SYSFS_UNIT_C_REVISION "$Revision: 1.33 $"
 
 #include "zfcp_ext.h"
 
@@ -41,7 +41,10 @@
 void
 zfcp_sysfs_unit_release(struct device *dev)
 {
-	kfree(dev);
+	struct zfcp_unit *unit;
+
+	unit = dev_get_drvdata(dev);
+	kfree(unit);
 }
 
 /**

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
  2005-01-24  9:46 [PATCH] zfcp: updates for -bk Heiko Carstens
@ 2005-01-24 14:22 ` Matthew Wilcox
  2005-01-24 14:48   ` Heiko Carstens
  0 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2005-01-24 14:22 UTC (permalink / raw)
  To: Heiko Carstens; +Cc: James Bottomley, SCSI Mailing List, Greg KH

On Mon, Jan 24, 2005 at 10:46:29AM +0100, Heiko Carstens wrote:
> @@ -1099,9 +1099,9 @@
>  }
>  
>  void
> -zfcp_dummy_release(struct device *dev)
> +zfcp_generic_services_release(struct device *dev)
>  {
> -	return;
> +	kfree(dev);
>  }
>  
>  /*

I thought that having release methods that just called kfree() were
also verboten?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
  2005-01-24 14:22 ` Matthew Wilcox
@ 2005-01-24 14:48   ` Heiko Carstens
  2005-01-24 15:31     ` James Bottomley
  0 siblings, 1 reply; 8+ messages in thread
From: Heiko Carstens @ 2005-01-24 14:48 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Greg KH, James Bottomley, SCSI Mailing List, willy

> >  void
> > -zfcp_dummy_release(struct device *dev)
> > +zfcp_generic_services_release(struct device *dev)
> >  {
> > -   return;
> > +   kfree(dev);
> >  }
> > 
> >  /*
> 
> I thought that having release methods that just called kfree() were
> also verboten?

We do a kmalloc(sizeof(struce device),...) somewhere and this 
is how we get rid of it again.
How are we supposed to free this object otherwise? The release
function gets called when there is no more reference to this
object and that's the earliest point we may free it.

Thanks,
Heiko


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
  2005-01-24 14:48   ` Heiko Carstens
@ 2005-01-24 15:31     ` James Bottomley
  2005-01-25  6:08       ` Heiko Carstens
  0 siblings, 1 reply; 8+ messages in thread
From: James Bottomley @ 2005-01-24 15:31 UTC (permalink / raw)
  To: Heiko Carstens; +Cc: Matthew Wilcox, Greg KH, SCSI Mailing List, willy

On Mon, 2005-01-24 at 15:48 +0100, Heiko Carstens wrote:
> > I thought that having release methods that just called kfree() were
> > also verboten?
> 
> We do a kmalloc(sizeof(struce device),...) somewhere and this 
> is how we get rid of it again.
> How are we supposed to free this object otherwise? The release
> function gets called when there is no more reference to this
> object and that's the earliest point we may free it.

Right, but we've said before this is the wrong way to do it.

Originally this generic device was part of your adapter structure.  Now
you're trying to separate it and causing these problems.  What it's
apparently telling us is that you have some problem with the object
lifetime rules in your code.

Why should this generic_services device have different lifetime rules
from the object in which it used to reside?

James



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
  2005-01-24 15:31     ` James Bottomley
@ 2005-01-25  6:08       ` Heiko Carstens
  2005-01-25 15:20         ` James Bottomley
  0 siblings, 1 reply; 8+ messages in thread
From: Heiko Carstens @ 2005-01-25  6:08 UTC (permalink / raw)
  To: James Bottomley; +Cc: Greg KH, SCSI Mailing List, Matthew Wilcox, willy

> > > I thought that having release methods that just called kfree() were
> > > also verboten?
> > We do a kmalloc(sizeof(struce device),...) somewhere and this 
> > is how we get rid of it again.
> > How are we supposed to free this object otherwise? The release
> > function gets called when there is no more reference to this
> > object and that's the earliest point we may free it.
> Right, but we've said before this is the wrong way to do it.

I think I missed something. What Greg objected to was that we had used
kfree as the release function. Also he objected to the point that we
were using the pointer to the struct device, which gets passed to the
release function, to free different (larger) objects by using the simple
hack to place the struct device at the beginning of other structs.
This patch does nothing like this. Actually it cleans just all those
things and leftovers up.

> Originally this generic device was part of your adapter structure.  Now
> you're trying to separate it and causing these problems.  What it's

Could you please elaborate where this patch does cause a problem?

> apparently telling us is that you have some problem with the object
> lifetime rules in your code.
> Why should this generic_services device have different lifetime rules
> from the object in which it used to reside?

It doesn't. And this was not supposed to fix anything. It's just that
Andreas liked the code better with allocating the generic_services
struct device seperately. Please note that the lifetime of the adapter
device this struct device was embedded into is always longer than the
lifetime of the generic_services device.
Therefore I can't see why you complain about this piece of code.

Since I didn't get an answer to this:
http://marc.theaimsgroup.com/?l=linux-scsi&m=110551973013105&w=2
my assumption was that it's that obvious to everyone else that I'm
wrong that it isn't even worth writing an answer.
Now I follow Greg's rules and still get complaints...

Thanks,
Heiko


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
  2005-01-25  6:08       ` Heiko Carstens
@ 2005-01-25 15:20         ` James Bottomley
  0 siblings, 0 replies; 8+ messages in thread
From: James Bottomley @ 2005-01-25 15:20 UTC (permalink / raw)
  To: Heiko Carstens; +Cc: Greg KH, SCSI Mailing List, Matthew Wilcox, willy

On Tue, 2005-01-25 at 07:08 +0100, Heiko Carstens wrote:
> > Originally this generic device was part of your adapter structure.  Now
> > you're trying to separate it and causing these problems.  What it's
> 
> Could you please elaborate where this patch does cause a problem?

You're look to be breaking the simplicity rules.  Object lifetimes are
very tricky things to manage, so what I want you to explain is why you
have to make this more difficult buy making the object separately
managed instead of treating it as a sub object of the adapter which
shares the adapter's lifetime rules.  What's the justification for
this? 

> > apparently telling us is that you have some problem with the object
> > lifetime rules in your code.
> > Why should this generic_services device have different lifetime rules
> > from the object in which it used to reside?
> 
> It doesn't. And this was not supposed to fix anything. It's just that
> Andreas liked the code better with allocating the generic_services
> struct device seperately. Please note that the lifetime of the adapter
> device this struct device was embedded into is always longer than the
> lifetime of the generic_services device.
> Therefore I can't see why you complain about this piece of code.

generic devices are supposed to represent existing kernel objects, which
is why they're usually embedded in existing structures.  It seems to be
unique within the kernel to allocate a generic device without an
enveloping structure (like a scsi_device, mca_device, pci_device etc),
so I was wondering why you need to do it this way?

Simplicity says that you want to manage the lifetimes of as few objects
as you can get away with (SCSI tries to put all lifetime rules in either
the mid-layer or the transport classes so the driver code doesn't
usually have to worry about them).  The reason being that lifetime rules
are very easy to get wrong and have fairly nasty consequences if you do
get them wrong (pointers into free'd memory etc.).

James


> Since I didn't get an answer to this:
> http://marc.theaimsgroup.com/?l=linux-scsi&m=110551973013105&w=2
> my assumption was that it's that obvious to everyone else that I'm
> wrong that it isn't even worth writing an answer.
> Now I follow Greg's rules and still get complaints...
> 
> Thanks,
> Heiko
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
@ 2005-01-25 18:10 Martin Peschke3
  2005-01-25 18:40 ` James Bottomley
  0 siblings, 1 reply; 8+ messages in thread
From: Martin Peschke3 @ 2005-01-25 18:10 UTC (permalink / raw)
  To: James Bottomley
  Cc: Greg KH, Heiko Carstens, SCSI Mailing List, linux-scsi-owner,
	Matthew Wilcox, willy





James,

both Heiko and Andreas aren't available for several days.
Let me try to answer your questions.

> You're look to be breaking the simplicity rules.  Object lifetimes are
> very tricky things to manage, so what I want you to explain is why you
> have to make this more difficult buy making the object separately
> managed instead of treating it as a sub object of the adapter which
> shares the adapter's lifetime rules.  What's the justification for
> this?

Actually, you will find the adapter structure be an anchor for several
other objects, or lists of them respectively. We tried to organize
all the driver private data in a sane way. That means there is a tree
of objects representing the SAN topology, including target ports and
subordinate LUNs, many of which have an associated mid-layer structure.
I can't imagine that anybody would ever argue to collapse all that into
the adapter structure. So, why all the fuss about this particular object?
The generic services object has nothing to do with the adapter. It's about
fabric switch services, or certain well-known FC (target) ports, which can
be accessed in the fabric through an adapter. It's not local but remote,
like other targets, so to speak. We might also have chosen to call it
"fabric switch".

> generic devices are supposed to represent existing kernel objects, which
> is why they're usually embedded in existing structures.  It seems to be
> unique within the kernel to allocate a generic device without an
> enveloping structure (like a scsi_device, mca_device, pci_device etc),
> so I was wondering why you need to do it this way?

It gives us a separate subdirectory where to put all the (special)
services ports instead of dumping them all into the adapter's directory
among the other (commonplace) target ports. The latter appear by WWPN
(64 bit, which is supposed to be much more persistent and descriptive),
the former don't even have one, but need to be represented in a different
way, namely by their well-known D_IDs (24 bit, descriptive in their own
way because defined by FC standards).

On the other hand, one might argue that we don't try to group
commonplace target ports by node (which I won't advocate).

Besides, given that the adapter structure is already refcounted, it
might be confusing to have more than one refcount or struct device
within one structure.

> Simplicity says that you want to manage the lifetimes of as few objects
> as you can get away with (SCSI tries to put all lifetime rules in either
> the mid-layer or the transport classes so the driver code doesn't
> usually have to worry about them).  The reason being that lifetime rules
> are very easy to get wrong and have fairly nasty consequences if you do
> get them wrong (pointers into free'd memory etc.).

Agreed. That would be our trade-off.


Martin



|---------+-------------------------------->
|         |           James Bottomley      |
|         |           <James.Bottomley@Stee|
|         |           lEye.com>            |
|         |           Sent by:             |
|         |           linux-scsi-owner@vger|
|         |           .kernel.org          |
|         |                                |
|         |                                |
|         |           25/01/2005 16:20     |
|         |                                |
|---------+-------------------------------->
  >---------------------------------------------------------------------------------------------------------------|
  |                                                                                                               |
  |       To:       Heiko Carstens/Germany/IBM@IBMDE                                                              |
  |       cc:       Greg KH <greg@kroah.com>, SCSI Mailing List <linux-scsi@vger.kernel.org>, Matthew Wilcox      |
  |        <matthew@wil.cx>, willy@www.linux.org.uk                                                               |
  |       Subject:  Re: [PATCH] zfcp: updates for -bk                                                             |
  |                                                                                                               |
  >---------------------------------------------------------------------------------------------------------------|




On Tue, 2005-01-25 at 07:08 +0100, Heiko Carstens wrote:
> > Originally this generic device was part of your adapter structure.  Now
> > you're trying to separate it and causing these problems.  What it's
>
> Could you please elaborate where this patch does cause a problem?

You're look to be breaking the simplicity rules.  Object lifetimes are
very tricky things to manage, so what I want you to explain is why you
have to make this more difficult buy making the object separately
managed instead of treating it as a sub object of the adapter which
shares the adapter's lifetime rules.  What's the justification for
this?

> > apparently telling us is that you have some problem with the object
> > lifetime rules in your code.
> > Why should this generic_services device have different lifetime rules
> > from the object in which it used to reside?
>
> It doesn't. And this was not supposed to fix anything. It's just that
> Andreas liked the code better with allocating the generic_services
> struct device seperately. Please note that the lifetime of the adapter
> device this struct device was embedded into is always longer than the
> lifetime of the generic_services device.
> Therefore I can't see why you complain about this piece of code.

generic devices are supposed to represent existing kernel objects, which
is why they're usually embedded in existing structures.  It seems to be
unique within the kernel to allocate a generic device without an
enveloping structure (like a scsi_device, mca_device, pci_device etc),
so I was wondering why you need to do it this way?

Simplicity says that you want to manage the lifetimes of as few objects
as you can get away with (SCSI tries to put all lifetime rules in either
the mid-layer or the transport classes so the driver code doesn't
usually have to worry about them).  The reason being that lifetime rules
are very easy to get wrong and have fairly nasty consequences if you do
get them wrong (pointers into free'd memory etc.).

James


> Since I didn't get an answer to this:
> http://marc.theaimsgroup.com/?l=linux-scsi&m=110551973013105&w=2
> my assumption was that it's that obvious to everyone else that I'm
> wrong that it isn't even worth writing an answer.
> Now I follow Greg's rules and still get complaints...
>
> Thanks,
> Heiko
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] zfcp: updates for -bk
  2005-01-25 18:10 Martin Peschke3
@ 2005-01-25 18:40 ` James Bottomley
  0 siblings, 0 replies; 8+ messages in thread
From: James Bottomley @ 2005-01-25 18:40 UTC (permalink / raw)
  To: Martin Peschke3
  Cc: Greg KH, Heiko Carstens, SCSI Mailing List, linux-scsi-owner,
	Matthew Wilcox, willy

On Tue, 2005-01-25 at 19:10 +0100, Martin Peschke3 wrote:
> Actually, you will find the adapter structure be an anchor for several
> other objects, or lists of them respectively. We tried to organize
> all the driver private data in a sane way. That means there is a tree
> of objects representing the SAN topology, including target ports and
> subordinate LUNs, many of which have an associated mid-layer structure.
> I can't imagine that anybody would ever argue to collapse all that into
> the adapter structure. So, why all the fuss about this particular object?
> The generic services object has nothing to do with the adapter. It's about
> fabric switch services, or certain well-known FC (target) ports, which can
> be accessed in the fabric through an adapter. It's not local but remote,
> like other targets, so to speak. We might also have chosen to call it
> "fabric switch".

OK, take a look at this:

http://marc.theaimsgroup.com/?l=linux-scsi&m=110546207223304

and tell me if it does everything you need.  If it doesn't, you could
try adding all the other bits to the fc transport class.

James



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2005-01-25 18:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-01-24  9:46 [PATCH] zfcp: updates for -bk Heiko Carstens
2005-01-24 14:22 ` Matthew Wilcox
2005-01-24 14:48   ` Heiko Carstens
2005-01-24 15:31     ` James Bottomley
2005-01-25  6:08       ` Heiko Carstens
2005-01-25 15:20         ` James Bottomley
  -- strict thread matches above, loose matches on Subject: below --
2005-01-25 18:10 Martin Peschke3
2005-01-25 18:40 ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox