Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Jonathan Derrick]

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@xxxxxxxxx
>> 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
>>>>>
>