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

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Wednesday, February 23, 2011 4:27 PM
> To: Haiyang Zhang
> Cc: Hank Janssen; KY Srinivasan; Abhishek Kane (Mindtree Consulting PVT LTD);
> gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx>
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > Signed-off-by: Hank Janssen <hjanssen@xxxxxxxxxxxxx>
> >
> > ---
> >  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/