[PATCH] PCI: Remove redundant macro

[PATCH] PCI: Remove redundant macro

[zhangdongdong]

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 */
 
 /* SATA capability */
-- 
2.17.1

Re: [PATCH] PCI: Remove redundant macro

[Bjorn Helgaas]

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

Re: Re: [PATCH] PCI: Remove redundant macro

[DongdongZhang]

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