| Message ID | 20241213094617.1149-1-zhangdongdong@eswincomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: Remove redundant macro | expand |
On Fri, Dec 13, 2024 at 05:46:17PM +0800, zhangdongdong@eswincomputing.com wrote: > From: Dongdong Zhang <zhangdongdong@eswincomputing.com> > > Removed the duplicate macro definition PCI_VSEC_HDR from > pci_regs.h to avoid redundancy. Updated the VFIO PCI code > to use the existing `PCI_VNDR_HEADER` macro for consistency, > ensuring minimal changes to the codebase. > > Signed-off-by: Dongdong Zhang <zhangdongdong@eswincomputing.com> > --- > drivers/vfio/pci/vfio_pci_config.c | 3 ++- > include/uapi/linux/pci_regs.h | 1 - > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > index ea2745c1ac5e..c30748912ff1 100644 > --- a/drivers/vfio/pci/vfio_pci_config.c > +++ b/drivers/vfio/pci/vfio_pci_config.c > @@ -1389,7 +1389,8 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo > > switch (ecap) { > case PCI_EXT_CAP_ID_VNDR: > - ret = pci_read_config_dword(pdev, epos + PCI_VSEC_HDR, &dword); > + ret = pci_read_config_dword(pdev, epos + PCI_VNDR_HEADER, > + &dword); > if (ret) > return pcibios_err_to_errno(ret); > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h > index 1601c7ed5fab..7b6cad788de3 100644 > --- a/include/uapi/linux/pci_regs.h > +++ b/include/uapi/linux/pci_regs.h > @@ -1001,7 +1001,6 @@ > #define PCI_ACS_CTRL 0x06 /* ACS Control Register */ > #define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */ > > -#define PCI_VSEC_HDR 4 /* extended cap - vendor-specific */ > #define PCI_VSEC_HDR_LEN_SHIFT 20 /* shift for length field */ We should resolve the duplication of PCI_VSEC_HDR and PCI_VNDR_HEADER, but I don't like the fact that we're left with this dangling PCI_VSEC_HDR_LEN_SHIFT. That leaves vfio using PCI_VNDR_HEADER and PCI_VSEC_HDR_LEN_SHIFT, which don't match at all. I think you should remove PCI_VSEC_HDR_LEN_SHIFT as well and change vfio to use PCI_VNDR_HEADER_LEN() instead. It's somewhat dicey removing things from pci_regs.h since it's in include/uapi/, but this is such a niche thing we might be able to get away with it. Bjorn
Hi Bjorn, Thank you for reviewing my patch and providing detailed feedback! I agree with your observation regarding the mismatch left between `PCI_VNDR_HEADER` and `PCI_VSEC_HDR_LEN_SHIFT`. Based on your suggestion, I plan to address this by: 1. Removing `PCI_VSEC_HDR_LEN_SHIFT` entirely. 2. Updating the VFIO PCI code to use `PCI_VNDR_HEADER_LEN()` instead, ensuring consistent naming and functionality. 3. Justifying the removal of these macros (`PCI_VSEC_HDR` and `PCI_VSEC_HDR_LEN_SHIFT`) in `pci_regs.h`, despite it being part of `include/uapi/`. As you noted, this is a niche case, and the impact on userspace should be minimal. I’ll send an updated patch shortly, incorporating these changes and testing to ensure everything works as expected. Thanks again for your insights! Please let me know if there’s anything else I should address in the revised patch. Best regards, Dongdong Zhang > -----原始邮件----- > 发件人: "Bjorn Helgaas" <helgaas@kernel.org> > 发送时间:2024-12-14 01:37:01 (星期六) > 收件人: zhangdongdong@eswincomputing.com > 抄送: alex.williamson@redhat.com, bhelgaas@google.com, yishaih@nvidia.com, avihaih@nvidia.com, yi.l.liu@intel.com, ankita@nvidia.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org > 主题: Re: [PATCH] PCI: Remove redundant macro > > On Fri, Dec 13, 2024 at 05:46:17PM +0800, zhangdongdong@eswincomputing.com wrote: > > From: Dongdong Zhang <zhangdongdong@eswincomputing.com> > > > > Removed the duplicate macro definition PCI_VSEC_HDR from > > pci_regs.h to avoid redundancy. Updated the VFIO PCI code > > to use the existing `PCI_VNDR_HEADER` macro for consistency, > > ensuring minimal changes to the codebase. > > > > Signed-off-by: Dongdong Zhang <zhangdongdong@eswincomputing.com> > > --- > > drivers/vfio/pci/vfio_pci_config.c | 3 ++- > > include/uapi/linux/pci_regs.h | 1 - > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > index ea2745c1ac5e..c30748912ff1 100644 > > --- a/drivers/vfio/pci/vfio_pci_config.c > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > @@ -1389,7 +1389,8 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo > > > > switch (ecap) { > > case PCI_EXT_CAP_ID_VNDR: > > - ret = pci_read_config_dword(pdev, epos + PCI_VSEC_HDR, &dword); > > + ret = pci_read_config_dword(pdev, epos + PCI_VNDR_HEADER, > > + &dword); > > if (ret) > > return pcibios_err_to_errno(ret); > > > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h > > index 1601c7ed5fab..7b6cad788de3 100644 > > --- a/include/uapi/linux/pci_regs.h > > +++ b/include/uapi/linux/pci_regs.h > > @@ -1001,7 +1001,6 @@ > > #define PCI_ACS_CTRL 0x06 /* ACS Control Register */ > > #define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */ > > > > -#define PCI_VSEC_HDR 4 /* extended cap - vendor-specific */ > > #define PCI_VSEC_HDR_LEN_SHIFT 20 /* shift for length field */ > > We should resolve the duplication of PCI_VSEC_HDR and PCI_VNDR_HEADER, > but I don't like the fact that we're left with this dangling > PCI_VSEC_HDR_LEN_SHIFT. > > That leaves vfio using PCI_VNDR_HEADER and PCI_VSEC_HDR_LEN_SHIFT, > which don't match at all. > > I think you should remove PCI_VSEC_HDR_LEN_SHIFT as well and change > vfio to use PCI_VNDR_HEADER_LEN() instead. > > It's somewhat dicey removing things from pci_regs.h since it's in > include/uapi/, but this is such a niche thing we might be able to get > away with it. > > Bjorn
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c index ea2745c1ac5e..c30748912ff1 100644 --- a/drivers/vfio/pci/vfio_pci_config.c +++ b/drivers/vfio/pci/vfio_pci_config.c @@ -1389,7 +1389,8 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo switch (ecap) { case PCI_EXT_CAP_ID_VNDR: - ret = pci_read_config_dword(pdev, epos + PCI_VSEC_HDR, &dword); + ret = pci_read_config_dword(pdev, epos + PCI_VNDR_HEADER, + &dword); if (ret) return pcibios_err_to_errno(ret); diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index 1601c7ed5fab..7b6cad788de3 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -1001,7 +1001,6 @@ #define PCI_ACS_CTRL 0x06 /* ACS Control Register */ #define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */ -#define PCI_VSEC_HDR 4 /* extended cap - vendor-specific */ #define PCI_VSEC_HDR_LEN_SHIFT 20 /* shift for length field */ /* SATA capability */