RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pcidriver

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxx]
> Sent: Thursday, March 10, 2011 5:21 PM
> To: KY Srinivasan
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxx; Haiyang Zhang; Mike Sterling; Abhishek Kane
> (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
> 
> On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
> > Make vmbus driver a platform pci driver. This is
> > in preparation to cleaning up irq allocation for this
> > driver.
> 
> The idea is nice, but the nameing is a bit confusing.
> 
> We have "platform drivers" which are much different from what you are
> doing here, you are just creating a "normal" pci driver.
> 
> Very minor comments below.
> 
> >
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > Signed-off-by: Mike Sterling <mike.sterling@xxxxxxxxxxxxx>
> > Signed-off-by: Abhishek Kane <v-abkane@xxxxxxxxxxxxx>
> > Signed-off-by: Hank Janssen <hjanssen@xxxxxxxxxxxxx>
> > ---
> >  drivers/staging/hv/vmbus_drv.c |   63 +++++++++++++++++++++++-------------
> ----
> >  1 files changed, 36 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > index 8b9394a..e4855ac 100644
> > --- a/drivers/staging/hv/vmbus_drv.c
> > +++ b/drivers/staging/hv/vmbus_drv.c
> > @@ -43,6 +43,8 @@
> >
> >  static struct device *root_dev; /* Root device */
> >
> > +struct pci_dev *hv_pci_dev;
> > +
> >  /* Main vmbus driver data structure */
> >  struct vmbus_driver_context {
> >
> > @@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> >  	}
> >  }
> >
> > -static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> > -	{
> > -		.ident = "Hyper-V",
> > -		.matches = {
> > -			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> Corporation"),
> > -			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > -			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > -		},
> > -	},
> > -	{ },
> > -};
> > -MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
> 
> You're sure it's safe to delete this now and just rely on the PCI ids,
> right?  For some wierd reason I thought we needed both to catch all
> types of systems, but I can't remember why.
I have tested this; I don't think we need the dmi table.

> 
> >
> > -static int __init vmbus_init(void)
> > +
> > +static int __devinit hv_pci_probe(struct pci_dev *pdev,
> > +				const struct pci_device_id *ent)
> >  {
> > -	DPRINT_INFO(VMBUS_DRV,
> > -		"Vmbus initializing.... current log level 0x%x (%x,%x)",
> > -		vmbus_loglevel, HIWORD(vmbus_loglevel),
> LOWORD(vmbus_loglevel));
> > -	/* Todo: it is used for loglevel, to be ported to new kernel. */
> > +	int err;
> >
> > -	if (!dmi_check_system(microsoft_hv_dmi_table))
> > -		return -ENODEV;
> > +	hv_pci_dev = pdev;
> >
> > -	return vmbus_bus_init();
> > -}
> > +	err = pci_enable_device(pdev);
> > +	if (err)
> > +		return err;
> >
> > -static void __exit vmbus_exit(void)
> > -{
> > -	vmbus_bus_exit();
> > -	/* Todo: it is used for loglevel, to be ported to new kernel. */
> > +	err = vmbus_bus_init();
> > +	if (err)
> > +		pci_disable_device(pdev);
> > +
> > +	return err;
> >  }
> >
> >  /*
> > @@ -931,10 +921,29 @@ static const struct pci_device_id
> microsoft_hv_pci_table[] = {
> >  };
> >  MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
> >
> > +static struct pci_driver platform_driver = {
> 
> "hv_bus_driver"?
> 
> > +	.name =           "hv-platform-pci",
> 
> How about "hv_bus" as a name, as that's what this really is.  It's a
> "bus adapter", like USB, Firewire, and all sorts of other bus
> controllers.

Sure; I will make these changes. Would you mind if I submit these name changes as a separate patch.

Regards,

K. Y
--
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/