lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 28 Oct 2022 13:22:30 -0700
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Bjorn Helgaas <helgaas@...nel.org>,
        Jonathan Derrick <jonathan.derrick@...ux.dev>
Cc:     nirmal.patel@...ux.intel.com, lorenzo.pieralisi@....com,
        hch@...radead.org, kw@...ux.com, robh@...nel.org,
        bhelgaas@...gle.com, michael.a.bottini@...el.com,
        rafael@...nel.org, me@...ityamohan.in, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

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

Powered by blists - more mailing lists