Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Oct 2022 14:40:14 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Jonathan Derrick <jonathan.derrick@linux.dev>
Cc:     "David E. Box" <david.e.box@linux.intel.com>,
        nirmal.patel@linux.intel.com, lorenzo.pieralisi@arm.com,
        hch@infradead.org, kw@linux.com, robh@kernel.org,
        bhelgaas@google.com, michael.a.bottini@intel.com,
        rafael@kernel.org, me@adhityamohan.in, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data
Message-ID: <20221028194014.GA907046@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5c0a8a41-db58-ac73-da08-681b20156786@linux.dev>
Precedence: bulk

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