Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id tomod_devicetable.h

[Greg KH]

On Wed, Aug 24, 2011 at 04:46:11PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@xxxxxxxxx]
> > Sent: Tuesday, August 23, 2011 10:59 PM
> > To: KY Srinivasan
> > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; Haiyang Zhang
> > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > mod_devicetable.h
> > 
> > On Wed, Aug 24, 2011 at 12:44:38AM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:greg@xxxxxxxxx]
> > > > Sent: Tuesday, August 23, 2011 6:42 PM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; Haiyang Zhang
> > > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > > > mod_devicetable.h
> > > >
> > > > On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > > > > In preparation for implementing vmbus aliases for auto-loading
> > > > > Hyper-V drivers, define vmbus specific device ID.
> > > > >
> > > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > > > > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > > > > ---
> > > > >  include/linux/mod_devicetable.h |    7 +++++++
> > > > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/mod_devicetable.h
> > > > b/include/linux/mod_devicetable.h
> > > > > index ae28e93..5a12377 100644
> > > > > --- a/include/linux/mod_devicetable.h
> > > > > +++ b/include/linux/mod_devicetable.h
> > > > > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > > > >  };
> > > > >  #define VIRTIO_DEV_ANY_ID	0xffffffff
> > > > >
> > > > > +/*
> > > > > + * For Hyper-V devices we use the device guid as the id.
> > > > > + */
> > > > > +struct hv_vmbus_device_id {
> > > > > +	__u8 guid[16];
> > > > > +};
> > > >
> > > > Why do you not need a driver_data pointer here?  Are you sure you aren't
> > > > ever going to need it in the future?  Hint, I think you will...
> > >
> > > I am not sure I am following you here; the guid is the device ID and it is
> > > guaranteed to remain the same. What is the driver _data pointer here
> > > you are referring to here.  While some device id have the _data pointer,
> > > there are others that don't - for instance struct virtio_device_id. In
> > > our case, I am not sure how I would use this private pointer.
> > 
> > You use it like all other drivers use it, only if needed.
> 
> Fair enough; the point is I am not sure how I would use it.
> 
> > 
> > Hint, I think you need to use it in your hv_utils driver, it would
> > reduce the size of your code and simplify your logic.
> 
> Could you expand on this. Currently the util driver handles a bunch 
> services that have their own guids - and these have been included
> in the idtable. How would having the pointer simplify this code.

It would allow you, in your probe function, to do something different
depending on the guid that the probe function was matching on.  So you
would not have to check the guid again to do that, just use the data
pointed in that void pointer and away you go.

As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
variable which uses the driver_info field to contain a quirk for the
device.

> I looked at the usage of this in PCI and it appears to be for supporting
> dynamic  IDs for existing drivers.

No, that's exactly wrong.  dynamic ids play havoc with this pointer,
making some drivers not be able to handle dynamic ids because they rely
on it for some driver-specific information to be passed in it, which
dynamic ids can not handle.

Oh, have you remembered to turn off dynamic ids for these devices?  Or
do you support them properly?

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/