diff mbox series

[V7,3/4] PCI: vmd: Add vmd_device_data

Message ID 20221025004411.2910026-4-david.e.box@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: vmd: Enable PCIe ASPM and LTR on select hardware | expand

Commit Message

David E. Box Oct. 25, 2022, 12:44 a.m. UTC 
Add vmd_device_data to allow adding additional info for driver data.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
 V7
   - Moved PCI_VDEVICE into separate earlier patch.
 V6
   - Inline the declarations for driver data in the vmd_ids list.
     Suggested by Jonathan
 V5
   - New patch

 drivers/pci/controller/vmd.c | 80 +++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 25 deletions(-)

Comments

Bjorn Helgaas Oct. 28, 2022, 7:13 p.m. UTC | #1 
On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> Add vmd_device_data to allow adding additional info for driver data.

>  	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		},
> +	},

It looks like these devices come in families where several device IDs
share the same features.  I think this would be more readable if you
defined each family outside this table and simply referenced the
family here.  E.g., you could do something like:

  static struct vmd_device_data vmd_v1 = {
    .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
		VMD_FEAT_HAS_BUS_RESTRICTIONS |
		VMD_FEAT_OFFSET_FIRST_VECTOR,
  };

  {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
    .driver_data = (kernel_ulong_t) &vmd_v1,

Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
instead of repeating it a half dozen times.

>  	{0,}
>  };
>  MODULE_DEVICE_TABLE(pci, vmd_ids);
> -- 
> 2.25.1
>
Jonathan Derrick Oct. 28, 2022, 7:18 p.m. UTC | #2 
On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>> Add vmd_device_data to allow adding additional info for driver data.
> 
>>  	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
>> -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
>> +		(kernel_ulong_t)&(struct vmd_device_data) {
>> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
>> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
>> +		},
>> +	},
> 
> It looks like these devices come in families where several device IDs
> share the same features.  I think this would be more readable if you
> defined each family outside this table and simply referenced the
> family here.  E.g., you could do something like:
> 
>   static struct vmd_device_data vmd_v1 = {
>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>   };
I seem to recall it being similar to this in one of the previous revisions
It's fine with me either way

> 
>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>     .driver_data = (kernel_ulong_t) &vmd_v1,
> 
> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> instead of repeating it a half dozen times.
> 
>>  	{0,}
>>  };
>>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>> -- 
>> 2.25.1
>>
Bjorn Helgaas Oct. 28, 2022, 7:40 p.m. UTC | #3 
On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> > On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> >> Add vmd_device_data to allow adding additional info for driver data.
> > 
> >>  	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> >> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
> >> -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
> >> +		(kernel_ulong_t)&(struct vmd_device_data) {
> >> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> >> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> >> +		},
> >> +	},
> > 
> > It looks like these devices come in families where several device IDs
> > share the same features.  I think this would be more readable if you
> > defined each family outside this table and simply referenced the
> > family here.  E.g., you could do something like:
> > 
> >   static struct vmd_device_data vmd_v1 = {
> >     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > 		VMD_FEAT_OFFSET_FIRST_VECTOR,
> >   };
>
> I seem to recall it being similar to this in one of the previous revisions
> It's fine with me either way

Indeed it was:
https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
I'd forgotten that.

At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
that used the 467f data.  The current series adds two more (0x7d0b
0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
descriptive, but the code was definitely shorter:

  +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
  +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
  +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
  +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), (kernel_ulong_t)&vmd_467f_data },

I do wish pci_device_id.driver_data were a void pointer, as it is for
of_device_id, which makes it much more natural to express [1], but
that ship has long sailed.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768

> >   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> >     .driver_data = (kernel_ulong_t) &vmd_v1,
> > 
> > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > instead of repeating it a half dozen times.
> > 
> >>  	{0,}
> >>  };
> >>  MODULE_DEVICE_TABLE(pci, vmd_ids);
> >> -- 
> >> 2.25.1
> >>
David E. Box Oct. 28, 2022, 8:22 p.m. UTC | #4 
On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
> > On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> > > On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> > > > Add vmd_device_data to allow adding additional info for driver data.
> > > >  	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > > > -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > > -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > > -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
> > > > +		(kernel_ulong_t)&(struct vmd_device_data) {
> > > > +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > > +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > > +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > > > +		},
> > > > +	},
> > > 
> > > It looks like these devices come in families where several device IDs
> > > share the same features.  I think this would be more readable if you
> > > defined each family outside this table and simply referenced the
> > > family here.  E.g., you could do something like:
> > > 
> > >   static struct vmd_device_data vmd_v1 = {
> > >     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > 		VMD_FEAT_OFFSET_FIRST_VECTOR,
> > >   };
> > 
> > I seem to recall it being similar to this in one of the previous revisions
> > It's fine with me either way
> 
> Indeed it was:
> https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
> I'd forgotten that.
> 
> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
> that used the 467f data.  The current series adds two more (0x7d0b
> 0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
> descriptive, but the code was definitely shorter:
> 
>   +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>   +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>   +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>   +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> (kernel_ulong_t)&vmd_467f_data },

I prefer this too but don't know what's the best name. Could just be by the
platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
you think Jonathan?

David

> 
> I do wish pci_device_id.driver_data were a void pointer, as it is for
> of_device_id, which makes it much more natural to express [1], but
> that ship has long sailed.
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
> 
> > >   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > >     .driver_data = (kernel_ulong_t) &vmd_v1,
> > > 
> > > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > > instead of repeating it a half dozen times.
> > > 
> > > >  	{0,}
> > > >  };
> > > >  MODULE_DEVICE_TABLE(pci, vmd_ids);
> > > > -- 
> > > > 2.25.1
> > > >
Jonathan Derrick Oct. 28, 2022, 9:14 p.m. UTC | #5 
On 10/28/2022 3:22 PM, David E. Box wrote:
> On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
>>> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
>>>> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>>>>> Add vmd_device_data to allow adding additional info for driver data.
>>>>>  	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
>>>>> +		(kernel_ulong_t)&(struct vmd_device_data) {
>>>>> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>> +		},
>>>>> +	},
>>>>
>>>> It looks like these devices come in families where several device IDs
>>>> share the same features.  I think this would be more readable if you
>>>> defined each family outside this table and simply referenced the
>>>> family here.  E.g., you could do something like:
>>>>
>>>>   static struct vmd_device_data vmd_v1 = {
>>>>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>   };
>>>
>>> I seem to recall it being similar to this in one of the previous revisions
>>> It's fine with me either way
>>
>> Indeed it was:
>> https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
>> I'd forgotten that.
>>
>> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
>> that used the 467f data.  The current series adds two more (0x7d0b
>> 0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
>> descriptive, but the code was definitely shorter:
>>
>>   +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>>   +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>>   +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>>   +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>> (kernel_ulong_t)&vmd_467f_data },
> 
> I prefer this too but don't know what's the best name. Could just be by the
> platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
> you think Jonathan?
> 
> David
vmd_client_data ? (meaning product class; client vs enterprise)


> 
>>
>> I do wish pci_device_id.driver_data were a void pointer, as it is for
>> of_device_id, which makes it much more natural to express [1], but
>> that ship has long sailed.
>>
>> [1] 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>>
>>>>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>     .driver_data = (kernel_ulong_t) &vmd_v1,
>>>>
>>>> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
>>>> instead of repeating it a half dozen times.
>>>>
>>>>>  	{0,}
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>> -- 
>>>>> 2.25.1
>>>>>
>
Nirmal Patel Oct. 28, 2022, 9:24 p.m. UTC | #6 
On 10/28/2022 2:14 PM, Jonathan Derrick wrote:
>
> On 10/28/2022 3:22 PM, David E. Box wrote:
>> On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
>>> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
>>>> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
>>>>> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>>>>>> Add vmd_device_data to allow adding additional info for driver data.
>>>>>>  	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>>> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>>> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>>> -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
>>>>>> +		(kernel_ulong_t)&(struct vmd_device_data) {
>>>>>> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>>> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>>> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>>> +		},
>>>>>> +	},
>>>>> It looks like these devices come in families where several device IDs
>>>>> share the same features.  I think this would be more readable if you
>>>>> defined each family outside this table and simply referenced the
>>>>> family here.  E.g., you could do something like:
>>>>>
>>>>>   static struct vmd_device_data vmd_v1 = {
>>>>>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>>   };
>>>> I seem to recall it being similar to this in one of the previous revisions
>>>> It's fine with me either way
>>> Indeed it was:
>>> https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
>>> I'd forgotten that.
>>>
>>> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
>>> that used the 467f data.  The current series adds two more (0x7d0b
>>> 0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
>>> descriptive, but the code was definitely shorter:
>>>
>>>   +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>>>   +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>>>   +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>>>   +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>> (kernel_ulong_t)&vmd_467f_data },
>> I prefer this too but don't know what's the best name. Could just be by the
>> platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
>> you think Jonathan?
>>
>> David
> vmd_client_data ? (meaning product class; client vs enterprise)
Product class would make sense. We can keep adding new IDs.
>
>>> I do wish pci_device_id.driver_data were a void pointer, as it is for
>>> of_device_id, which makes it much more natural to express [1], but
>>> that ship has long sailed.
>>>
>>> [1] 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>>>
>>>>>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>>     .driver_data = (kernel_ulong_t) &vmd_v1,
>>>>>
>>>>> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
>>>>> instead of repeating it a half dozen times.
>>>>>
>>>>>>  	{0,}
>>>>>>  };
>>>>>>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>>> -- 
>>>>>> 2.25.1
>>>>>>
Christoph Hellwig Oct. 31, 2022, 7:04 a.m. UTC | #7 
On Fri, Oct 28, 2022 at 02:13:08PM -0500, Bjorn Helgaas wrote:
> It looks like these devices come in families where several device IDs
> share the same features.  I think this would be more readable if you
> defined each family outside this table and simply referenced the
> family here.  E.g., you could do something like:
> 
>   static struct vmd_device_data vmd_v1 = {
>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>   };
> 
>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>     .driver_data = (kernel_ulong_t) &vmd_v1,
> 
> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> instead of repeating it a half dozen times.

I wonder why we need the ltr field at all.  For those that set it
is always the same value, so it could just be a quirk flag to set it.

Tat being said I think thegrouping makes a lot of sense, but I'd just
do it with a #define for the set of common quirk flags.
David E. Box Oct. 31, 2022, 3:40 p.m. UTC | #8 
On Mon, 2022-10-31 at 00:04 -0700, Christoph Hellwig wrote:
> On Fri, Oct 28, 2022 at 02:13:08PM -0500, Bjorn Helgaas wrote:
> > It looks like these devices come in families where several device IDs
> > share the same features.  I think this would be more readable if you
> > defined each family outside this table and simply referenced the
> > family here.  E.g., you could do something like:
> > 
> >   static struct vmd_device_data vmd_v1 = {
> >     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > 		VMD_FEAT_OFFSET_FIRST_VECTOR,
> >   };
> > 
> >   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> >     .driver_data = (kernel_ulong_t) &vmd_v1,
> > 
> > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > instead of repeating it a half dozen times.
> 
> I wonder why we need the ltr field at all.  For those that set it
> is always the same value, so it could just be a quirk flag to set it.

Yeah, this makes sense particularly since this isn't intended as a permanent
fix. I'll get rid of it.

> 
> Tat being said I think thegrouping makes a lot of sense, but I'd just
> do it with a #define for the set of common quirk flags.

Works for me. I'll create a VMD_FEATS_CLIENT group but I'll keep the ltr quirk
separate since future client systems won't be using it.

David
diff mbox series

Patch

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9dedca714c18..a53d88fd820c 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -68,6 +68,10 @@  enum vmd_features {
 	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
 };
 
+struct vmd_device_data {
+	enum vmd_features features;
+};
+
 static DEFINE_IDA(vmd_instance_ida);
 
 /*
@@ -709,11 +713,12 @@  static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 	vmd_bridge->native_dpc = root_bridge->native_dpc;
 }
 
-static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
+static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
 	struct resource *res;
 	u32 upper_bits;
+	unsigned long features = info->features;
 	unsigned long flags;
 	LIST_HEAD(resources);
 	resource_size_t offset[2] = {0};
@@ -882,7 +887,8 @@  static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	unsigned long features = (unsigned long) id->driver_data;
+	struct vmd_device_data *info = (struct vmd_device_data *)id->driver_data;
+	unsigned long features = info->features;
 	struct vmd_dev *vmd;
 	int err;
 
@@ -927,7 +933,7 @@  static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	spin_lock_init(&vmd->cfg_lock);
 	pci_set_drvdata(dev, vmd);
-	err = vmd_enable_domain(vmd, features);
+	err = vmd_enable_domain(vmd, info);
 	if (err)
 		goto out_release_instance;
 
@@ -995,35 +1001,59 @@  static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
 
 static const struct pci_device_id vmd_ids[] = {
 	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
+		},
+	},
 	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_CAN_BYPASS_MSI_REMAP,
+		},
+	},
 	{PCI_VDEVICE(INTEL, 0x467f),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{PCI_VDEVICE(INTEL, 0x4c3d),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{PCI_VDEVICE(INTEL, 0xa77f),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{PCI_VDEVICE(INTEL, 0x7d0b),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{PCI_VDEVICE(INTEL, 0xad0b),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{0,}
 };
 MODULE_DEVICE_TABLE(pci, vmd_ids);