Re: [PATCH v2 0/8] VMD add second rootbus support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 15, 2024 at 10:22:48AM +0100, Szymon Durawa wrote:
> This patch series implements second rootbus support inside Intel VMD module.
> Current implementation allows VMD to take ownership of devices only on first
> bus (Rootbus0). Starting from Intel Arrow Lake, VMD exposes second bus 
> (Rootbus1) to allow VMD to own devices on this bus as well. VMD MMIO BARs
> (CFGBAR. MEMBAR1 and MEMBAR2) are now shared between Rootbus0 and Rootbus1.
> Reconfiguration of 3 MMIO BARs is required by resizing current MMIO BARs ranges.
> It allows to find/register Rootbus1 and discovers devices behind it.
> 
> Patches 1 to 6 introduce code refactoring without functional changes.
> Patch 7 implements VMD Rootbus1 and patch 8 provides workaround for rootbus
> number hardwired to fixed non-zero value. Patch 8 is necessary for correct 
> enumeration attached devices behind Rottbus1. Without it user cannot access
> those devices.
> 
> Changes from v1:
> - splitting series into more commits, requested by Bjorn
> - adding helper functions, suggested by Bjorn
> - minor typos and unclear wording updated, suggested by Bjorn
> 
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Suggested-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx>
> Signed-off-by: Szymon Durawa <szymon.durawa@xxxxxxxxxxxxxxx>
> 
> Szymon Durawa (8):
>   PCI: vmd: Add vmd_bus_enumeration()
>   PCI: vmd: Add vmd_configure_cfgbar()
>   PCI: vmd: Add vmd_configure_membar() and
>     vmd_configure_membar1_membar2()
>   PCI: vmd: Add vmd_create_bus()
>   PCI: vmd: Replace hardcoded values with enum and defines
>   PCI: vmd: Convert bus and busn_start to an array
>   PCI: vmd: Add support for second rootbus under VMD
>   PCI: vmd: Add workaround for rootbus number hardwired to fixed
>     non-zero value
> 
>  drivers/pci/controller/vmd.c | 467 ++++++++++++++++++++++++++---------
>  1 file changed, 357 insertions(+), 110 deletions(-)
>  mode change 100644 => 100755 drivers/pci/controller/vmd.c

It looks like only the cover letter was sent to linux-pci, and the
actual patches weren't.  Can you repost this with the correct cc list?
You might as well address the nits below at the same time and make it
a v3 to avoid confusion.

When you do, can you rephrase these to say they add support for a
second *Root Port*, which I think is what they do.

The additional root bus is just a consequence of having another Root
Port.  The root bus has no existence by itself, so it doesn't really
make sense to talk about "finding" or "registering" the root bus.

Also rewrap commit logs to fill 75 columns.  Include the names of new
helpers in the commit log.

Add blank lines between paragraphs (noticed in 8/8 comment, might be
elsewhere as well).

In 6/8, replace "Bus and busn_start are converted ..." with "Convert
..." as you did in the subject.

In 7/8,
s/enhacement/enhancement/
s/This patch add/Add/
s/workaraund/workaround/ (also in 8/8 comment)
Replace "rootbus" here with "Root Port".  "Devices behind rootbus"
doesn't really make sense.

In 8/8 comment, I'm not sure what "It assigns setup as broken" means.
Recast "... are updated to the same value" to imperative mood ("update
x to the same value").

The "BUS0" nomenclature seems heavily embedded in the vmd driver but
is really a misnomer.  Maybe that reflects similar terminology in an
internal spec?  Any hard-wiring of bus numbers reflects a property of
the way a *Root Port* works, so using the right name will make this
easier to understand, especially since there are other Root Ports with
the same hard wiring.

Bjorn




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux