PCI: Add PCI_RID_MASK macro

[PATCH] PCI: Add PCI_RID_MASK macro

Posted by Aksh Garg 1 month ago

Routing ID (RID) is a 16-bit value composed of bus number and devfn.
Add a PCI_RID_MASK macro to eliminate the use of magic number 0xffff
when masking for RIDs.

This provides a standard way for drivers to extract RIDs without
resorting to magic numbers.

Signed-off-by: Aksh Garg <a-garg7@ti.com>
---
 drivers/pci/controller/pcie-apple.c | 2 +-
 include/linux/pci.h                 | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 2d92fc79f6dd..6bf9fe102e2f 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -802,7 +802,7 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
 		u32 val;
 
 		val = readl_relaxed(port_rid2sid_addr(port, idx));
-		if ((val & 0xffff) == rid) {
+		if ((val & PCI_RID_MASK) == rid) {
 			apple_pcie_rid2sid_write(port, idx, 0);
 			bitmap_release_region(port->sid_map, idx, 0);
 			dev_dbg(&pdev->dev, "Released %x (%d)\n", val, idx);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 864775651c6f..e3d9730121c8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -71,6 +71,7 @@
 #define PCI_DEVID(bus, devfn)	((((u16)(bus)) << 8) | (devfn))
 /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
 #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
+#define PCI_RID_MASK	0xffff
 
 /* pci_slot represents a physical slot */
 struct pci_slot {
-- 
2.34.1

Re: [PATCH] PCI: Add PCI_RID_MASK macro

Posted by Marc Zyngier 1 month ago

On Mon, 16 Feb 2026 12:06:00 +0000,
Aksh Garg <a-garg7@ti.com> wrote:
> 
> Routing ID (RID) is a 16-bit value composed of bus number and devfn.

No. RID here means "Requester ID". Please lookup the PCI spec.

> Add a PCI_RID_MASK macro to eliminate the use of magic number 0xffff
> when masking for RIDs.

I'm not convinced this is universally true. Some RIDs (or RID-like
identifiers, such as Stream IDs and Device IDs) are larger than 16
bit, depending on the integration.

The fact that this particular integration uses a 16bit RID without any
extra qualifier in the upper bits doesn't make it universal.

> This provides a standard way for drivers to extract RIDs without
> resorting to magic numbers.
> 
> Signed-off-by: Aksh Garg <a-garg7@ti.com>
> ---
>  drivers/pci/controller/pcie-apple.c | 2 +-
>  include/linux/pci.h                 | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 2d92fc79f6dd..6bf9fe102e2f 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -802,7 +802,7 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
>  		u32 val;
>  
>  		val = readl_relaxed(port_rid2sid_addr(port, idx));
> -		if ((val & 0xffff) == rid) {
> +		if ((val & PCI_RID_MASK) == rid) {
>  			apple_pcie_rid2sid_write(port, idx, 0);
>  			bitmap_release_region(port->sid_map, idx, 0);
>  			dev_dbg(&pdev->dev, "Released %x (%d)\n", val, idx);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 864775651c6f..e3d9730121c8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -71,6 +71,7 @@
>  #define PCI_DEVID(bus, devfn)	((((u16)(bus)) << 8) | (devfn))
>  /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
>  #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
> +#define PCI_RID_MASK	0xffff

Even the naming of this macro clashes with the current nomenclature,
where we use PCI_DEVID()/pci_dev_id() to manipulate this information.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] PCI: Add PCI_RID_MASK macro

Posted by Aksh Garg 4 weeks, 1 day ago

Marc, Lukas,

On 16/02/26 18:56, Marc Zyngier wrote:
> On Mon, 16 Feb 2026 12:06:00 +0000,
> Aksh Garg <a-garg7@ti.com> wrote:
>>
>> Routing ID (RID) is a 16-bit value composed of bus number and devfn.
> 
> No. RID here means "Requester ID". Please lookup the PCI spec.
> 
>> Add a PCI_RID_MASK macro to eliminate the use of magic number 0xffff
>> when masking for RIDs.
> 
> I'm not convinced this is universally true. Some RIDs (or RID-like
> identifiers, such as Stream IDs and Device IDs) are larger than 16
> bit, depending on the integration.
> 
> The fact that this particular integration uses a 16bit RID without any
> extra qualifier in the upper bits doesn't make it universal.
> 
>> This provides a standard way for drivers to extract RIDs without
>> resorting to magic numbers.
>>
>> Signed-off-by: Aksh Garg <a-garg7@ti.com>
>> ---
>>   drivers/pci/controller/pcie-apple.c | 2 +-
>>   include/linux/pci.h                 | 1 +
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
>> index 2d92fc79f6dd..6bf9fe102e2f 100644
>> --- a/drivers/pci/controller/pcie-apple.c
>> +++ b/drivers/pci/controller/pcie-apple.c
>> @@ -802,7 +802,7 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
>>   		u32 val;
>>   
>>   		val = readl_relaxed(port_rid2sid_addr(port, idx));
>> -		if ((val & 0xffff) == rid) {
>> +		if ((val & PCI_RID_MASK) == rid) {
>>   			apple_pcie_rid2sid_write(port, idx, 0);
>>   			bitmap_release_region(port->sid_map, idx, 0);
>>   			dev_dbg(&pdev->dev, "Released %x (%d)\n", val, idx);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 864775651c6f..e3d9730121c8 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -71,6 +71,7 @@
>>   #define PCI_DEVID(bus, devfn)	((((u16)(bus)) << 8) | (devfn))
>>   /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
>>   #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
>> +#define PCI_RID_MASK	0xffff
> 
> Even the naming of this macro clashes with the current nomenclature,
> where we use PCI_DEVID()/pci_dev_id() to manipulate this information.
> 

Is the below proposed macro acceptable at the same place:
#define PCI_GET_DEVID(x) ((x) & 0xffff)

Regards,
Aksh Garg

> Thanks,
> 
> 	M.
>

Re: [PATCH] PCI: Add PCI_RID_MASK macro

Posted by Marc Zyngier 4 weeks, 1 day ago

On Tue, 17 Feb 2026 07:52:54 +0000,
Aksh Garg <a-garg7@ti.com> wrote:
> 
> Marc, Lukas,
> 
> On 16/02/26 18:56, Marc Zyngier wrote:
> > On Mon, 16 Feb 2026 12:06:00 +0000,
> > Aksh Garg <a-garg7@ti.com> wrote:
> >> 
> >> Routing ID (RID) is a 16-bit value composed of bus number and devfn.
> > 
> > No. RID here means "Requester ID". Please lookup the PCI spec.
> > 
> >> Add a PCI_RID_MASK macro to eliminate the use of magic number 0xffff
> >> when masking for RIDs.
> > 
> > I'm not convinced this is universally true. Some RIDs (or RID-like
> > identifiers, such as Stream IDs and Device IDs) are larger than 16
> > bit, depending on the integration.
> > 
> > The fact that this particular integration uses a 16bit RID without any
> > extra qualifier in the upper bits doesn't make it universal.
> > 
> >> This provides a standard way for drivers to extract RIDs without
> >> resorting to magic numbers.
> >> 
> >> Signed-off-by: Aksh Garg <a-garg7@ti.com>
> >> ---
> >>   drivers/pci/controller/pcie-apple.c | 2 +-
> >>   include/linux/pci.h                 | 1 +
> >>   2 files changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> >> index 2d92fc79f6dd..6bf9fe102e2f 100644
> >> --- a/drivers/pci/controller/pcie-apple.c
> >> +++ b/drivers/pci/controller/pcie-apple.c
> >> @@ -802,7 +802,7 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
> >>   		u32 val;
> >>     		val = readl_relaxed(port_rid2sid_addr(port, idx));
> >> -		if ((val & 0xffff) == rid) {
> >> +		if ((val & PCI_RID_MASK) == rid) {
> >>   			apple_pcie_rid2sid_write(port, idx, 0);
> >>   			bitmap_release_region(port->sid_map, idx, 0);
> >>   			dev_dbg(&pdev->dev, "Released %x (%d)\n", val, idx);
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 864775651c6f..e3d9730121c8 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -71,6 +71,7 @@
> >>   #define PCI_DEVID(bus, devfn)	((((u16)(bus)) << 8) | (devfn))
> >>   /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
> >>   #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
> >> +#define PCI_RID_MASK	0xffff
> > 
> > Even the naming of this macro clashes with the current nomenclature,
> > where we use PCI_DEVID()/pci_dev_id() to manipulate this information.
> > 
> 
> Is the below proposed macro acceptable at the same place:
> #define PCI_GET_DEVID(x) ((x) & 0xffff)

But what meaning does this have? What containing type are you
extracting the value from?

The code you're hacking extracts the value from a register. What if
the register had a completely different layout? Would you move the
mask to match the layout of that particular register?

If you really want to get rid of what you consider a magic number, add
a bunch of fields to the apple driver to describe the rid2sid
registers, and use that with FIELD_GET()/FIELD_PREP() to manipulate
the registers.

But what you are proposing here is not generic. It has no defined
semantics, and is actively misleading.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] PCI: Add PCI_RID_MASK macro

Posted by Lukas Wunner 1 month ago

On Mon, Feb 16, 2026 at 05:36:00PM +0530, Aksh Garg wrote:
> +++ b/include/linux/pci.h
> @@ -71,6 +71,7 @@
>  #define PCI_DEVID(bus, devfn)	((((u16)(bus)) << 8) | (devfn))
>  /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
>  #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
> +#define PCI_RID_MASK	0xffff

Why not instead introduce

#define PCI_RID(x) ((x) & 0xffff)

to go alongside PCI_SLOT() & friends?

Thanks,

Lukas

Re: [PATCH] PCI: Add PCI_RID_MASK macro

Posted by Siddharth Vadapalli 1 month ago

On Mon, 2026-02-16 at 17:36 +0530, Aksh Garg wrote:
> Routing ID (RID) is a 16-bit value composed of bus number and devfn.

nitpick: "Requester ID" might be a more appropriate description of BDF.

> Add a PCI_RID_MASK macro to eliminate the use of magic number 0xffff
> when masking for RIDs.
> 
> This provides a standard way for drivers to extract RIDs without
> resorting to magic numbers.
> 
> Signed-off-by: Aksh Garg <a-garg7@ti.com>

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

> ---
>  drivers/pci/controller/pcie-apple.c | 2 +-
>  include/linux/pci.h                 | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 2d92fc79f6dd..6bf9fe102e2f 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -802,7 +802,7 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
>  		u32 val;
>  
>  		val = readl_relaxed(port_rid2sid_addr(port, idx));
> -		if ((val & 0xffff) == rid) {
> +		if ((val & PCI_RID_MASK) == rid) {
>  			apple_pcie_rid2sid_write(port, idx, 0);
>  			bitmap_release_region(port->sid_map, idx, 0);
>  			dev_dbg(&pdev->dev, "Released %x (%d)\n", val, idx);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 864775651c6f..e3d9730121c8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -71,6 +71,7 @@
>  #define PCI_DEVID(bus, devfn)	((((u16)(bus)) << 8) | (devfn))
>  /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
>  #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
> +#define PCI_RID_MASK	0xffff
>  
>  /* pci_slot represents a physical slot */
>  struct pci_slot {

Regards,
Siddharth.