[PATCH 2/4] staging: hv: Fix the code depending on struct netvsc_driver_context data order

[PATCH 2/4] staging: hv: Fix the code depending on struct netvsc_driver_context data order

[Haiyang Zhang]

The patch fixed the code depending on the exact order of fields in the
struct netvsc_driver_context. Now, we use container_of() instead of type
casting from the first field to the container struct.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/netvsc_drv.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index 364b6c7..c78624c 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -49,8 +49,6 @@ struct net_device_context {
 };
 
 struct netvsc_driver_context {
-	/* !! These must be the first 2 fields !! */
-	/* Which is a bug FIXME! */
 	struct driver_context drv_ctx;
 	struct netvsc_driver drv_obj;
 };
@@ -137,8 +135,8 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 	struct net_device_context *net_device_ctx = netdev_priv(net);
 	struct driver_context *driver_ctx =
 	    driver_to_driver_context(net_device_ctx->device_ctx->device.driver);
-	struct netvsc_driver_context *net_drv_ctx =
-		(struct netvsc_driver_context *)driver_ctx;
+	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
+		struct netvsc_driver_context, drv_ctx);
 	struct netvsc_driver *net_drv_obj = &net_drv_ctx->drv_obj;
 	struct hv_netvsc_packet *packet;
 	int ret;
@@ -342,8 +340,8 @@ static int netvsc_probe(struct device *device)
 {
 	struct driver_context *driver_ctx =
 		driver_to_driver_context(device->driver);
-	struct netvsc_driver_context *net_drv_ctx =
-		(struct netvsc_driver_context *)driver_ctx;
+	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
+		struct netvsc_driver_context, drv_ctx);
 	struct netvsc_driver *net_drv_obj = &net_drv_ctx->drv_obj;
 	struct vm_device *device_ctx = device_to_vm_device(device);
 	struct hv_device *device_obj = &device_ctx->device_obj;
@@ -414,8 +412,8 @@ static int netvsc_remove(struct device *device)
 {
 	struct driver_context *driver_ctx =
 		driver_to_driver_context(device->driver);
-	struct netvsc_driver_context *net_drv_ctx =
-		(struct netvsc_driver_context *)driver_ctx;
+	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
+		struct netvsc_driver_context, drv_ctx);
 	struct netvsc_driver *net_drv_obj = &net_drv_ctx->drv_obj;
 	struct vm_device *device_ctx = device_to_vm_device(device);
 	struct net_device *net = dev_get_drvdata(&device_ctx->device);
-- 
1.6.3.2

[PATCH 3/4] staging: hv: Fix the code depending on struct blkvsc_driver_context data order

[Haiyang Zhang]

The patch fixed the code depending on the exact order of fields in the
struct blkvsc_driver_context. Now, we use container_of() instead of type
casting from the first field to the container struct.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/blkvsc_drv.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
index 293ab8e..8f38895 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -117,8 +117,6 @@ struct block_device_context {
 
 /* Per driver */
 struct blkvsc_driver_context {
-	/* !! These must be the first 2 fields !! */
-	/* FIXME this is a bug! */
 	struct driver_context drv_ctx;
 	struct storvsc_driver_object drv_obj;
 };
@@ -248,7 +246,7 @@ static int blkvsc_probe(struct device *device)
 	struct driver_context *driver_ctx =
 				driver_to_driver_context(device->driver);
 	struct blkvsc_driver_context *blkvsc_drv_ctx =
-				(struct blkvsc_driver_context *)driver_ctx;
+		container_of(driver_ctx, struct blkvsc_driver_context, drv_ctx);
 	struct storvsc_driver_object *storvsc_drv_obj =
 				&blkvsc_drv_ctx->drv_obj;
 	struct vm_device *device_ctx = device_to_vm_device(device);
@@ -733,7 +731,7 @@ static int blkvsc_remove(struct device *device)
 	struct driver_context *driver_ctx =
 				driver_to_driver_context(device->driver);
 	struct blkvsc_driver_context *blkvsc_drv_ctx =
-				(struct blkvsc_driver_context *)driver_ctx;
+		container_of(driver_ctx, struct blkvsc_driver_context, drv_ctx);
 	struct storvsc_driver_object *storvsc_drv_obj =
 				&blkvsc_drv_ctx->drv_obj;
 	struct vm_device *device_ctx = device_to_vm_device(device);
@@ -856,7 +854,7 @@ static int blkvsc_submit_request(struct blkvsc_request *blkvsc_req,
 	struct driver_context *driver_ctx =
 			driver_to_driver_context(device_ctx->device.driver);
 	struct blkvsc_driver_context *blkvsc_drv_ctx =
-			(struct blkvsc_driver_context *)driver_ctx;
+		container_of(driver_ctx, struct blkvsc_driver_context, drv_ctx);
 	struct storvsc_driver_object *storvsc_drv_obj =
 			&blkvsc_drv_ctx->drv_obj;
 	struct hv_storvsc_request *storvsc_req;
-- 
1.6.3.2

[PATCH 4/4] staging: hv: Fix the code depending on struct storvsc_driver_context data order

[Haiyang Zhang]

The patch fixed the code depending on the exact order of fields in the
struct storvsc_driver_context. Now, we use container_of() instead of type
casting from the first field to the container struct.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/storvsc_drv.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index 33acee5..a364627 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -64,8 +64,6 @@ struct storvsc_cmd_request {
 };
 
 struct storvsc_driver_context {
-	/* !! These must be the first 2 fields !! */
-	/* FIXME this is a bug... */
 	struct driver_context drv_ctx;
 	struct storvsc_driver_object drv_obj;
 };
@@ -223,8 +221,8 @@ static int storvsc_probe(struct device *device)
 	int ret;
 	struct driver_context *driver_ctx =
 				driver_to_driver_context(device->driver);
-	struct storvsc_driver_context *storvsc_drv_ctx =
-				(struct storvsc_driver_context *)driver_ctx;
+	struct storvsc_driver_context *storvsc_drv_ctx = container_of(
+		driver_ctx, struct storvsc_driver_context, drv_ctx);
 	struct storvsc_driver_object *storvsc_drv_obj =
 				&storvsc_drv_ctx->drv_obj;
 	struct vm_device *device_ctx = device_to_vm_device(device);
@@ -308,8 +306,8 @@ static int storvsc_remove(struct device *device)
 	int ret;
 	struct driver_context *driver_ctx =
 			driver_to_driver_context(device->driver);
-	struct storvsc_driver_context *storvsc_drv_ctx =
-			(struct storvsc_driver_context *)driver_ctx;
+	struct storvsc_driver_context *storvsc_drv_ctx = container_of(
+		driver_ctx, struct storvsc_driver_context, drv_ctx);
 	struct storvsc_driver_object *storvsc_drv_obj =
 			&storvsc_drv_ctx->drv_obj;
 	struct vm_device *device_ctx = device_to_vm_device(device);
@@ -606,8 +604,8 @@ static int storvsc_queuecommand_lck(struct scsi_cmnd *scmnd,
 	struct vm_device *device_ctx = host_device_ctx->device_ctx;
 	struct driver_context *driver_ctx =
 		driver_to_driver_context(device_ctx->device.driver);
-	struct storvsc_driver_context *storvsc_drv_ctx =
-		(struct storvsc_driver_context *)driver_ctx;
+	struct storvsc_driver_context *storvsc_drv_ctx = container_of(
+		driver_ctx, struct storvsc_driver_context, drv_ctx);
 	struct storvsc_driver_object *storvsc_drv_obj =
 		&storvsc_drv_ctx->drv_obj;
 	struct hv_storvsc_request *request;
-- 
1.6.3.2

Re: [PATCH 3/4] staging: hv: Fix the code depending on struct blkvsc_driver_context data order

[Greg KH]

On Wed, Feb 23, 2011 at 12:19:57PM -0800, Haiyang Zhang wrote:
> The patch fixed the code depending on the exact order of fields in the
> struct blkvsc_driver_context. Now, we use container_of() instead of type
> casting from the first field to the container struct.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> ---
>  drivers/staging/hv/blkvsc_drv.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> index 293ab8e..8f38895 100644
> --- a/drivers/staging/hv/blkvsc_drv.c
> +++ b/drivers/staging/hv/blkvsc_drv.c
> @@ -117,8 +117,6 @@ struct block_device_context {
>  
>  /* Per driver */
>  struct blkvsc_driver_context {
> -	/* !! These must be the first 2 fields !! */
> -	/* FIXME this is a bug! */
>  	struct driver_context drv_ctx;
>  	struct storvsc_driver_object drv_obj;
>  };
> @@ -248,7 +246,7 @@ static int blkvsc_probe(struct device *device)
>  	struct driver_context *driver_ctx =
>  				driver_to_driver_context(device->driver);
>  	struct blkvsc_driver_context *blkvsc_drv_ctx =
> -				(struct blkvsc_driver_context *)driver_ctx;
> +		container_of(driver_ctx, struct blkvsc_driver_context, drv_ctx);

Same container_of() complaint here, make it a macro or inline function.

thanks,

greg k-h

Re: [PATCH 4/4] staging: hv: Fix the code depending on struct storvsc_driver_context data order

[Greg KH]

On Wed, Feb 23, 2011 at 12:19:58PM -0800, Haiyang Zhang wrote:
> The patch fixed the code depending on the exact order of fields in the
> struct storvsc_driver_context. Now, we use container_of() instead of type
> casting from the first field to the container struct.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> ---
>  drivers/staging/hv/storvsc_drv.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
> index 33acee5..a364627 100644
> --- a/drivers/staging/hv/storvsc_drv.c
> +++ b/drivers/staging/hv/storvsc_drv.c
> @@ -64,8 +64,6 @@ struct storvsc_cmd_request {
>  };
>  
>  struct storvsc_driver_context {
> -	/* !! These must be the first 2 fields !! */
> -	/* FIXME this is a bug... */
>  	struct driver_context drv_ctx;
>  	struct storvsc_driver_object drv_obj;
>  };
> @@ -223,8 +221,8 @@ static int storvsc_probe(struct device *device)
>  	int ret;
>  	struct driver_context *driver_ctx =
>  				driver_to_driver_context(device->driver);
> -	struct storvsc_driver_context *storvsc_drv_ctx =
> -				(struct storvsc_driver_context *)driver_ctx;
> +	struct storvsc_driver_context *storvsc_drv_ctx = container_of(
> +		driver_ctx, struct storvsc_driver_context, drv_ctx);

Same container_of() macro/inline function issue.

thanks,

greg k-h

Re: [PATCH 2/4] staging: hv: Fix the code depending on struct netvsc_driver_context data order

[Thomas Gleixner]

On Wed, 23 Feb 2011, Haiyang Zhang wrote:
> @@ -137,8 +135,8 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>  	struct net_device_context *net_device_ctx = netdev_priv(net);
>  	struct driver_context *driver_ctx =
>  	    driver_to_driver_context(net_device_ctx->device_ctx->device.driver);
> -	struct netvsc_driver_context *net_drv_ctx =
> -		(struct netvsc_driver_context *)driver_ctx;
> +	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
> +		struct netvsc_driver_context, drv_ctx);

While the code is correct, it's still horrible to read.

  	struct net_device_context *dev_ctx = netdev_priv(net);
	struct netvsc_drv_ctx *net_drv_ctx;
  	struct netvsc_driver *net_drv_obj;
  	struct driver_context *drv_ctx;
  	struct hv_netvsc_packet *packet;
  	int ret;

	drv_ctx = driver_to_driver_context(dev_ctx->device_ctx->device.driver);
	net_drv_ctx = container_of(driver_ctx, struct netvsc_drv_ctx, drv_ctx);
	net_drv_obj = &net_drv_ctx->drv_obj;

Line breaking code just to fulfil the 80 chars requirement is a
horrible idea.

I just explained somewhere else, that these line breaks are equaly
inefficient to parse by the human eye as the extra wide lines. Have
you ever seen a newspaper which uses a column width which spawns the
whole size of the paper? Probably not, simply because nobody can read
it fluently and fast.

So just mechanically converting existing code which was written based
on that horrible though popular codingstyle which requires wide screen
monitors is not going to result in readable code.

You need to do more than just inserting random line breaks as you see fit.

	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
		struct netvsc_driver_context, drv_ctx);

is a total horrible construct.

So
     	struct netvsc_driver_context *net_drv_ctx;

	net_drv_ctx = container_of(driver_ctx, struct netvsc_driver_context,
                                   drv_ctx);

is way better, as you can cleary see, that the extra line belongs to
the arguments of container_of(). Try to decode that in the above
original code w/o looking twice or more.

It's still not perfect but way better to read. So when converting (or
writing new) code think whether your struct names or variable names
need to be that long

     	struct netvsc_driver_context *net_drv_ctx;
vs.
	struct netsvc_drv_ctx *net_drv_ctx;

carries exact the same amount of information, but more condensed and lets

	net_drv_ctx = container_of(driver_ctx, struct netvsc_drv_ctx, drv_ctx);

fit into one line.

So yes, it's more work for you in the first place, but in the end it's
better readable code and you make it way easier for those who are
spending their (quality) time to review it.

Thanks,

	tglx

Re: [PATCH 2/4] staging: hv: Fix the code depending on struct netvsc_driver_context data order

[Greg KH]

On Wed, Feb 23, 2011 at 12:19:56PM -0800, Haiyang Zhang wrote:
> The patch fixed the code depending on the exact order of fields in the
> struct netvsc_driver_context. Now, we use container_of() instead of type
> casting from the first field to the container struct.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> ---
>  drivers/staging/hv/netvsc_drv.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
> index 364b6c7..c78624c 100644
> --- a/drivers/staging/hv/netvsc_drv.c
> +++ b/drivers/staging/hv/netvsc_drv.c
> @@ -49,8 +49,6 @@ struct net_device_context {
>  };
>  
>  struct netvsc_driver_context {
> -	/* !! These must be the first 2 fields !! */
> -	/* Which is a bug FIXME! */
>  	struct driver_context drv_ctx;
>  	struct netvsc_driver drv_obj;
>  };
> @@ -137,8 +135,8 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>  	struct net_device_context *net_device_ctx = netdev_priv(net);
>  	struct driver_context *driver_ctx =
>  	    driver_to_driver_context(net_device_ctx->device_ctx->device.driver);
> -	struct netvsc_driver_context *net_drv_ctx =
> -		(struct netvsc_driver_context *)driver_ctx;
> +	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
> +		struct netvsc_driver_context, drv_ctx);
>  	struct netvsc_driver *net_drv_obj = &net_drv_ctx->drv_obj;
>  	struct hv_netvsc_packet *packet;
>  	int ret;
> @@ -342,8 +340,8 @@ static int netvsc_probe(struct device *device)
>  {
>  	struct driver_context *driver_ctx =
>  		driver_to_driver_context(device->driver);
> -	struct netvsc_driver_context *net_drv_ctx =
> -		(struct netvsc_driver_context *)driver_ctx;
> +	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
> +		struct netvsc_driver_context, drv_ctx);

container_of calls should be turned into either a macro, or an inline
function, to make them smaller and easier to understand what is going
on.  Do that and you will solve the line-length problems as well.

Please do that here.

thanks,

greg k-h

Re: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[Greg KH]

On Wed, Feb 23, 2011 at 12:19:55PM -0800, Haiyang Zhang wrote:
> The patch fixed the code depending on the exact order of fields in the
> struct vmbus_driver_context, so the unused field drv_ctx can be removed,
> and drv_obj doesn't have to be the second field in this structure.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> ---
>  drivers/staging/hv/blkvsc_drv.c  |    2 ++
>  drivers/staging/hv/netvsc_drv.c  |    2 ++
>  drivers/staging/hv/storvsc_drv.c |    2 ++
>  drivers/staging/hv/vmbus.h       |    2 +-
>  drivers/staging/hv/vmbus_drv.c   |   14 +-------------
>  5 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> index 36a0adb..293ab8e 100644
> --- a/drivers/staging/hv/blkvsc_drv.c
> +++ b/drivers/staging/hv/blkvsc_drv.c
> @@ -177,6 +177,8 @@ static int blkvsc_drv_init(int (*drv_init)(struct hv_driver *drv))
>  	struct driver_context *drv_ctx = &g_blkvsc_drv.drv_ctx;
>  	int ret;
>  
> +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> +
>  	storvsc_drv_obj->ring_buffer_size = blkvsc_ringbuffer_size;
>  
>  	/* Callback to client driver to complete the initialization */
> diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
> index 03f9740..364b6c7 100644
> --- a/drivers/staging/hv/netvsc_drv.c
> +++ b/drivers/staging/hv/netvsc_drv.c
> @@ -500,6 +500,8 @@ static int netvsc_drv_init(int (*drv_init)(struct hv_driver *drv))
>  	struct driver_context *drv_ctx = &g_netvsc_drv.drv_ctx;
>  	int ret;
>  
> +	drv_ctx->hv_drv = &net_drv_obj->base;
> +
>  	net_drv_obj->ring_buf_size = ring_size * PAGE_SIZE;
>  	net_drv_obj->recv_cb = netvsc_recv_callback;
>  	net_drv_obj->link_status_change = netvsc_linkstatus_callback;
> diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
> index a8427ff..33acee5 100644
> --- a/drivers/staging/hv/storvsc_drv.c
> +++ b/drivers/staging/hv/storvsc_drv.c
> @@ -140,6 +140,8 @@ static int storvsc_drv_init(int (*drv_init)(struct hv_driver *drv))
>  	struct storvsc_driver_object *storvsc_drv_obj = &g_storvsc_drv.drv_obj;
>  	struct driver_context *drv_ctx = &g_storvsc_drv.drv_ctx;
>  
> +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> +
>  	storvsc_drv_obj->ring_buffer_size = storvsc_ringbuffer_size;
>  
>  	/* Callback to client driver to complete the initialization */
> diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
> index 42f2adb..fd9d00f 100644
> --- a/drivers/staging/hv/vmbus.h
> +++ b/drivers/staging/hv/vmbus.h
> @@ -30,8 +30,8 @@
>  
>  struct driver_context {
>  	struct hv_guid class_id;
> -
>  	struct device_driver driver;
> +	struct hv_driver *hv_drv;

If you have a pointer to hv_driver, why do you need a full 'struct
device_driver' here?  That sounds really wrong.

Actually, having 'struct device_driver' within a structure called
"driver_context" seems wrong, this should be what 'struct hv_driver'
really is, right?

So no, I can't take this patch, as it isn't solving the root problem
here.

thanks,

greg k-h

RE: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, February 23, 2011 4:27 PM
> To: Haiyang Zhang
> Cc: Hank Janssen; KY Srinivasan; Abhishek Kane (Mindtree Consulting PVT LTD);
> gregkh@suse.de; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: [PATCH 1/4] staging: hv: Fix the code depending on struct
> vmbus_driver_context data order
> 
> On Wed, Feb 23, 2011 at 12:19:55PM -0800, Haiyang Zhang wrote:
> > The patch fixed the code depending on the exact order of fields in the
> > struct vmbus_driver_context, so the unused field drv_ctx can be removed,
> > and drv_obj doesn't have to be the second field in this structure.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> >
> > ---
> >  drivers/staging/hv/blkvsc_drv.c  |    2 ++
> >  drivers/staging/hv/netvsc_drv.c  |    2 ++
> >  drivers/staging/hv/storvsc_drv.c |    2 ++
> >  drivers/staging/hv/vmbus.h       |    2 +-
> >  drivers/staging/hv/vmbus_drv.c   |   14 +-------------
> >  5 files changed, 8 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> > index 36a0adb..293ab8e 100644
> > --- a/drivers/staging/hv/blkvsc_drv.c
> > +++ b/drivers/staging/hv/blkvsc_drv.c
> > @@ -177,6 +177,8 @@ static int blkvsc_drv_init(int (*drv_init)(struct hv_driver
> *drv))
> >  	struct driver_context *drv_ctx = &g_blkvsc_drv.drv_ctx;
> >  	int ret;
> >
> > +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> > +
> >  	storvsc_drv_obj->ring_buffer_size = blkvsc_ringbuffer_size;
> >
> >  	/* Callback to client driver to complete the initialization */
> > diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
> > index 03f9740..364b6c7 100644
> > --- a/drivers/staging/hv/netvsc_drv.c
> > +++ b/drivers/staging/hv/netvsc_drv.c
> > @@ -500,6 +500,8 @@ static int netvsc_drv_init(int (*drv_init)(struct hv_driver
> *drv))
> >  	struct driver_context *drv_ctx = &g_netvsc_drv.drv_ctx;
> >  	int ret;
> >
> > +	drv_ctx->hv_drv = &net_drv_obj->base;
> > +
> >  	net_drv_obj->ring_buf_size = ring_size * PAGE_SIZE;
> >  	net_drv_obj->recv_cb = netvsc_recv_callback;
> >  	net_drv_obj->link_status_change = netvsc_linkstatus_callback;
> > diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
> > index a8427ff..33acee5 100644
> > --- a/drivers/staging/hv/storvsc_drv.c
> > +++ b/drivers/staging/hv/storvsc_drv.c
> > @@ -140,6 +140,8 @@ static int storvsc_drv_init(int (*drv_init)(struct hv_driver
> *drv))
> >  	struct storvsc_driver_object *storvsc_drv_obj = &g_storvsc_drv.drv_obj;
> >  	struct driver_context *drv_ctx = &g_storvsc_drv.drv_ctx;
> >
> > +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> > +
> >  	storvsc_drv_obj->ring_buffer_size = storvsc_ringbuffer_size;
> >
> >  	/* Callback to client driver to complete the initialization */
> > diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
> > index 42f2adb..fd9d00f 100644
> > --- a/drivers/staging/hv/vmbus.h
> > +++ b/drivers/staging/hv/vmbus.h
> > @@ -30,8 +30,8 @@
> >
> >  struct driver_context {
> >  	struct hv_guid class_id;
> > -
> >  	struct device_driver driver;
> > +	struct hv_driver *hv_drv;
> 
> If you have a pointer to hv_driver, why do you need a full 'struct
> device_driver' here?  That sounds really wrong.
> 
> Actually, having 'struct device_driver' within a structure called
> "driver_context" seems wrong, this should be what 'struct hv_driver'
> really is, right?

We could certainly use better names to describe the layering here: 
Think of driver_context as a representation of a generic hyperV device 
driver. So, this structure will embed the generic struct device_driver.
The pointer to hv_drv allows for further specialization based on 
the device type. For instance the block driver has its own hv_driver while
the network driver has its own instance of hv_driver. I am not defending
this layering, and I agree we could name these abstractions to be more 
intuitive and also considerably improve the layering.

 

Regards,

K. Y

> 
> So no, I can't take this patch, as it isn't solving the root problem
> here.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[Greg KH]

On Wed, Feb 23, 2011 at 10:44:37PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Wednesday, February 23, 2011 4:27 PM
> > To: Haiyang Zhang
> > Cc: Hank Janssen; KY Srinivasan; Abhishek Kane (Mindtree Consulting PVT LTD);
> > gregkh@suse.de; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org
> > Subject: Re: [PATCH 1/4] staging: hv: Fix the code depending on struct
> > vmbus_driver_context data order
> > 
> > On Wed, Feb 23, 2011 at 12:19:55PM -0800, Haiyang Zhang wrote:
> > > The patch fixed the code depending on the exact order of fields in the
> > > struct vmbus_driver_context, so the unused field drv_ctx can be removed,
> > > and drv_obj doesn't have to be the second field in this structure.
> > >
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > >
> > > ---
> > >  drivers/staging/hv/blkvsc_drv.c  |    2 ++
> > >  drivers/staging/hv/netvsc_drv.c  |    2 ++
> > >  drivers/staging/hv/storvsc_drv.c |    2 ++
> > >  drivers/staging/hv/vmbus.h       |    2 +-
> > >  drivers/staging/hv/vmbus_drv.c   |   14 +-------------
> > >  5 files changed, 8 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> > > index 36a0adb..293ab8e 100644
> > > --- a/drivers/staging/hv/blkvsc_drv.c
> > > +++ b/drivers/staging/hv/blkvsc_drv.c
> > > @@ -177,6 +177,8 @@ static int blkvsc_drv_init(int (*drv_init)(struct hv_driver
> > *drv))
> > >  	struct driver_context *drv_ctx = &g_blkvsc_drv.drv_ctx;
> > >  	int ret;
> > >
> > > +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> > > +
> > >  	storvsc_drv_obj->ring_buffer_size = blkvsc_ringbuffer_size;
> > >
> > >  	/* Callback to client driver to complete the initialization */
> > > diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
> > > index 03f9740..364b6c7 100644
> > > --- a/drivers/staging/hv/netvsc_drv.c
> > > +++ b/drivers/staging/hv/netvsc_drv.c
> > > @@ -500,6 +500,8 @@ static int netvsc_drv_init(int (*drv_init)(struct hv_driver
> > *drv))
> > >  	struct driver_context *drv_ctx = &g_netvsc_drv.drv_ctx;
> > >  	int ret;
> > >
> > > +	drv_ctx->hv_drv = &net_drv_obj->base;
> > > +
> > >  	net_drv_obj->ring_buf_size = ring_size * PAGE_SIZE;
> > >  	net_drv_obj->recv_cb = netvsc_recv_callback;
> > >  	net_drv_obj->link_status_change = netvsc_linkstatus_callback;
> > > diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
> > > index a8427ff..33acee5 100644
> > > --- a/drivers/staging/hv/storvsc_drv.c
> > > +++ b/drivers/staging/hv/storvsc_drv.c
> > > @@ -140,6 +140,8 @@ static int storvsc_drv_init(int (*drv_init)(struct hv_driver
> > *drv))
> > >  	struct storvsc_driver_object *storvsc_drv_obj = &g_storvsc_drv.drv_obj;
> > >  	struct driver_context *drv_ctx = &g_storvsc_drv.drv_ctx;
> > >
> > > +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> > > +
> > >  	storvsc_drv_obj->ring_buffer_size = storvsc_ringbuffer_size;
> > >
> > >  	/* Callback to client driver to complete the initialization */
> > > diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
> > > index 42f2adb..fd9d00f 100644
> > > --- a/drivers/staging/hv/vmbus.h
> > > +++ b/drivers/staging/hv/vmbus.h
> > > @@ -30,8 +30,8 @@
> > >
> > >  struct driver_context {
> > >  	struct hv_guid class_id;
> > > -
> > >  	struct device_driver driver;
> > > +	struct hv_driver *hv_drv;
> > 
> > If you have a pointer to hv_driver, why do you need a full 'struct
> > device_driver' here?  That sounds really wrong.
> > 
> > Actually, having 'struct device_driver' within a structure called
> > "driver_context" seems wrong, this should be what 'struct hv_driver'
> > really is, right?
> 
> We could certainly use better names to describe the layering here: 
> Think of driver_context as a representation of a generic hyperV device 
> driver. So, this structure will embed the generic struct device_driver.

That's fine, and is what all other driver subsystems do.  But they name
it correctly, unlike this subsystem :)

> The pointer to hv_drv allows for further specialization based on 
> the device type. For instance the block driver has its own hv_driver while
> the network driver has its own instance of hv_driver. I am not defending
> this layering, and I agree we could name these abstractions to be more 
> intuitive and also considerably improve the layering.

The layering is almost ok, there is still one more layer here than is
needed, and it should be removed (I already removed lots of layers that
were not needed, just didn't get to this one.)  But the naming also
needs to be fixed up as it is wrong from a "driver model" standpoint
with the rest of the kernel.

thanks,

greg k-h

RE: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[Haiyang Zhang]

> From: Greg KH [mailto:greg@kroah.com]
> The layering is almost ok, there is still one more layer here than is
> needed, and it should be removed (I already removed lots of layers that
> were not needed, just didn't get to this one.)  But the naming also
> needs to be fixed up as it is wrong from a "driver model" standpoint
> with the rest of the kernel.

So, how about rename the "struct driver_context" to "struct gen_drv_ctx"
in this patch? We can deal with the layering in next round of patches.

Thanks,

- Haiyang

Re: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[Greg KH]

On Wed, Feb 23, 2011 at 10:55:12PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:greg@kroah.com]
> > The layering is almost ok, there is still one more layer here than is
> > needed, and it should be removed (I already removed lots of layers that
> > were not needed, just didn't get to this one.)  But the naming also
> > needs to be fixed up as it is wrong from a "driver model" standpoint
> > with the rest of the kernel.
> 
> So, how about rename the "struct driver_context" to "struct gen_drv_ctx"
> in this patch? We can deal with the layering in next round of patches.

No, it's not a "driver context" at all.  It is a "hyperv driver", so
name it as such.

A "context" is a void pointer or something that the driver uses
privately.  And you already have a pointer to the context in the base
structure so you don't need your own.

You should work on removing the layering now, that will clean this all
up.

thanks,

greg k-h

RE: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, February 23, 2011 5:48 PM
> To: KY Srinivasan
> Cc: Haiyang Zhang; Hank Janssen; Abhishek Kane (Mindtree Consulting PVT LTD);
> gregkh@suse.de; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: [PATCH 1/4] staging: hv: Fix the code depending on struct
> vmbus_driver_context data order
> 
> On Wed, Feb 23, 2011 at 10:44:37PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Wednesday, February 23, 2011 4:27 PM
> > > To: Haiyang Zhang
> > > Cc: Hank Janssen; KY Srinivasan; Abhishek Kane (Mindtree Consulting PVT
> LTD);
> > > gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org
> > > Subject: Re: [PATCH 1/4] staging: hv: Fix the code depending on struct
> > > vmbus_driver_context data order
> > >
> > > On Wed, Feb 23, 2011 at 12:19:55PM -0800, Haiyang Zhang wrote:
> > > > The patch fixed the code depending on the exact order of fields in the
> > > > struct vmbus_driver_context, so the unused field drv_ctx can be removed,
> > > > and drv_obj doesn't have to be the second field in this structure.
> > > >
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > >
> > > > ---
> > > >  drivers/staging/hv/blkvsc_drv.c  |    2 ++
> > > >  drivers/staging/hv/netvsc_drv.c  |    2 ++
> > > >  drivers/staging/hv/storvsc_drv.c |    2 ++
> > > >  drivers/staging/hv/vmbus.h       |    2 +-
> > > >  drivers/staging/hv/vmbus_drv.c   |   14 +-------------
> > > >  5 files changed, 8 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/hv/blkvsc_drv.c
> b/drivers/staging/hv/blkvsc_drv.c
> > > > index 36a0adb..293ab8e 100644
> > > > --- a/drivers/staging/hv/blkvsc_drv.c
> > > > +++ b/drivers/staging/hv/blkvsc_drv.c
> > > > @@ -177,6 +177,8 @@ static int blkvsc_drv_init(int (*drv_init)(struct
> hv_driver
> > > *drv))
> > > >  	struct driver_context *drv_ctx = &g_blkvsc_drv.drv_ctx;
> > > >  	int ret;
> > > >
> > > > +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> > > > +
> > > >  	storvsc_drv_obj->ring_buffer_size = blkvsc_ringbuffer_size;
> > > >
> > > >  	/* Callback to client driver to complete the initialization */
> > > > diff --git a/drivers/staging/hv/netvsc_drv.c
> b/drivers/staging/hv/netvsc_drv.c
> > > > index 03f9740..364b6c7 100644
> > > > --- a/drivers/staging/hv/netvsc_drv.c
> > > > +++ b/drivers/staging/hv/netvsc_drv.c
> > > > @@ -500,6 +500,8 @@ static int netvsc_drv_init(int (*drv_init)(struct
> hv_driver
> > > *drv))
> > > >  	struct driver_context *drv_ctx = &g_netvsc_drv.drv_ctx;
> > > >  	int ret;
> > > >
> > > > +	drv_ctx->hv_drv = &net_drv_obj->base;
> > > > +
> > > >  	net_drv_obj->ring_buf_size = ring_size * PAGE_SIZE;
> > > >  	net_drv_obj->recv_cb = netvsc_recv_callback;
> > > >  	net_drv_obj->link_status_change = netvsc_linkstatus_callback;
> > > > diff --git a/drivers/staging/hv/storvsc_drv.c
> b/drivers/staging/hv/storvsc_drv.c
> > > > index a8427ff..33acee5 100644
> > > > --- a/drivers/staging/hv/storvsc_drv.c
> > > > +++ b/drivers/staging/hv/storvsc_drv.c
> > > > @@ -140,6 +140,8 @@ static int storvsc_drv_init(int (*drv_init)(struct
> hv_driver
> > > *drv))
> > > >  	struct storvsc_driver_object *storvsc_drv_obj = &g_storvsc_drv.drv_obj;
> > > >  	struct driver_context *drv_ctx = &g_storvsc_drv.drv_ctx;
> > > >
> > > > +	drv_ctx->hv_drv = &storvsc_drv_obj->base;
> > > > +
> > > >  	storvsc_drv_obj->ring_buffer_size = storvsc_ringbuffer_size;
> > > >
> > > >  	/* Callback to client driver to complete the initialization */
> > > > diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
> > > > index 42f2adb..fd9d00f 100644
> > > > --- a/drivers/staging/hv/vmbus.h
> > > > +++ b/drivers/staging/hv/vmbus.h
> > > > @@ -30,8 +30,8 @@
> > > >
> > > >  struct driver_context {
> > > >  	struct hv_guid class_id;
> > > > -
> > > >  	struct device_driver driver;
> > > > +	struct hv_driver *hv_drv;
> > >
> > > If you have a pointer to hv_driver, why do you need a full 'struct
> > > device_driver' here?  That sounds really wrong.
> > >
> > > Actually, having 'struct device_driver' within a structure called
> > > "driver_context" seems wrong, this should be what 'struct hv_driver'
> > > really is, right?
> >
> > We could certainly use better names to describe the layering here:
> > Think of driver_context as a representation of a generic hyperV device
> > driver. So, this structure will embed the generic struct device_driver.
> 
> That's fine, and is what all other driver subsystems do.  But they name
> it correctly, unlike this subsystem :)
>

 
We will get there!

> > The pointer to hv_drv allows for further specialization based on
> > the device type. For instance the block driver has its own hv_driver while
> > the network driver has its own instance of hv_driver. I am not defending
> > this layering, and I agree we could name these abstractions to be more
> > intuitive and also considerably improve the layering.
> 
> The layering is almost ok, there is still one more layer here than is
> needed, and it should be removed (I already removed lots of layers that
> were not needed, just didn't get to this one.)  But the naming also
> needs to be fixed up as it is wrong from a "driver model" standpoint
> with the rest of the kernel.

We will get rid of this additional layering. We could just as well embed the 
needed information. 

Regards,

K. Y  
> 
> thanks,
> 
> greg k-h

RE: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order

[Haiyang Zhang]

> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, February 23, 2011 4:27 PM
> >  struct driver_context {
> >  	struct hv_guid class_id;
> > -
> >  	struct device_driver driver;
> > +	struct hv_driver *hv_drv;
> 
> If you have a pointer to hv_driver, why do you need a full 'struct
> device_driver' here?  That sounds really wrong.
> 
> Actually, having 'struct device_driver' within a structure called
> "driver_context" seems wrong, this should be what 'struct hv_driver'
> really is, right?

The hv_driver contains Hyper-V specific data, like hv_guid. But the
struct driver_context contains data and functions related to Linux
kernel side, such as the struct device_driver defined by kernel. So,
they are kept separately.

(just saw Ky's email, which further explained the layering.)

Thanks,

- Haiyang