[PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Shuah Khan]

pci defines PCI_DEVFN(), PCI_SLOT(), and PCI_FUNC() interfaces, however,
it doesn't have interfaces to return PCI bus and PCI device id. Drivers
(AMD IOMMU, and AER) implement module specific definitions for PCI_BUS()
and AMD_IOMMU driver also has a module specific interface to calculate PCI   
device id from bus number and devfn.

Add PCI_BUS and PCI_DEVID interfaces to return PCI bus number and PCI device
id respectively to avoid the need for duplicate definitions in other modules.
AER driver code and AMD IOMMU driver define PCI_BUS. AMD IOMMU driver defines
an interface to calculate device id from bus number, and devfn pair.

Signed-off-by: Shuah Khan <shuah.khan@hp.com>
---
 include/uapi/linux/pci.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
index 3c292bc0..6b2c8b3 100644
--- a/include/uapi/linux/pci.h
+++ b/include/uapi/linux/pci.h
@@ -30,6 +30,10 @@
 #define PCI_DEVFN(slot, func)	((((slot) & 0x1f) << 3) | ((func) & 0x07))
 #define PCI_SLOT(devfn)		(((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)		((devfn) & 0x07)
+#define PCI_DEVID(bus, devfn)	((((u16)bus) << 8) | devfn)
+
+/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
+#define PCI_BUS(x) (((x) >> 8) & 0xff)
 
 /* Ioctls for /proc/bus/pci/X/Y nodes. */
 #define PCIIOC_BASE		('P' << 24 | 'C' << 16 | 'I' << 8)
-- 
1.7.9.5

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Bjorn Helgaas]

On Mon, Feb 11, 2013 at 4:00 PM, Shuah Khan <shuah.khan@hp.com> wrote:
> pci defines PCI_DEVFN(), PCI_SLOT(), and PCI_FUNC() interfaces, however,
> it doesn't have interfaces to return PCI bus and PCI device id. Drivers
> (AMD IOMMU, and AER) implement module specific definitions for PCI_BUS()
> and AMD_IOMMU driver also has a module specific interface to calculate PCI
> device id from bus number and devfn.
>
> Add PCI_BUS and PCI_DEVID interfaces to return PCI bus number and PCI device
> id respectively to avoid the need for duplicate definitions in other modules.
> AER driver code and AMD IOMMU driver define PCI_BUS. AMD IOMMU driver defines
> an interface to calculate device id from bus number, and devfn pair.
>
> Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> ---
>  include/uapi/linux/pci.h |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
> index 3c292bc0..6b2c8b3 100644
> --- a/include/uapi/linux/pci.h
> +++ b/include/uapi/linux/pci.h
> @@ -30,6 +30,10 @@
>  #define PCI_DEVFN(slot, func)  ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)                (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)                ((devfn) & 0x07)
> +#define PCI_DEVID(bus, devfn)  ((((u16)bus) << 8) | devfn)
> +
> +/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
> +#define PCI_BUS(x) (((x) >> 8) & 0xff)
>
>  /* Ioctls for /proc/bus/pci/X/Y nodes. */
>  #define PCIIOC_BASE            ('P' << 24 | 'C' << 16 | 'I' << 8)

David, can you point me at a description of include/uapi ... what is
there and why, and how we should decide what new things go in
include/uapi/linux/pci.h as opposed to include/linux/pci.h?  Maybe
there should be something in Documentation/?

I'm guessing it's something to do with being exported to userland, but
I'm not sure the things in this patch (PCI_DEV_ID, PCI_BUS) are really
exportable in the sense of being used for syscalls, etc.

Bjorn

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Shuah Khan]

On Wed, 2013-02-20 at 18:19 -0700, Bjorn Helgaas wrote:
> On Mon, Feb 11, 2013 at 4:00 PM, Shuah Khan <shuah.khan@hp.com> wrote:
> > pci defines PCI_DEVFN(), PCI_SLOT(), and PCI_FUNC() interfaces, however,
> > it doesn't have interfaces to return PCI bus and PCI device id. Drivers
> > (AMD IOMMU, and AER) implement module specific definitions for PCI_BUS()
> > and AMD_IOMMU driver also has a module specific interface to calculate PCI
> > device id from bus number and devfn.
> >
> > Add PCI_BUS and PCI_DEVID interfaces to return PCI bus number and PCI device
> > id respectively to avoid the need for duplicate definitions in other modules.
> > AER driver code and AMD IOMMU driver define PCI_BUS. AMD IOMMU driver defines
> > an interface to calculate device id from bus number, and devfn pair.
> >
> > Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> > ---
> >  include/uapi/linux/pci.h |    4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
> > index 3c292bc0..6b2c8b3 100644
> > --- a/include/uapi/linux/pci.h
> > +++ b/include/uapi/linux/pci.h
> > @@ -30,6 +30,10 @@
> >  #define PCI_DEVFN(slot, func)  ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> >  #define PCI_SLOT(devfn)                (((devfn) >> 3) & 0x1f)
> >  #define PCI_FUNC(devfn)                ((devfn) & 0x07)
> > +#define PCI_DEVID(bus, devfn)  ((((u16)bus) << 8) | devfn)
> > +
> > +/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
> > +#define PCI_BUS(x) (((x) >> 8) & 0xff)
> >
> >  /* Ioctls for /proc/bus/pci/X/Y nodes. */
> >  #define PCIIOC_BASE            ('P' << 24 | 'C' << 16 | 'I' << 8)
> 
> David, can you point me at a description of include/uapi ... what is
> there and why, and how we should decide what new things go in
> include/uapi/linux/pci.h as opposed to include/linux/pci.h?  Maybe
> there should be something in Documentation/?
> 
> I'm guessing it's something to do with being exported to userland, but
> I'm not sure the things in this patch (PCI_DEV_ID, PCI_BUS) are really
> exportable in the sense of being used for syscalls, etc.
> 

Bjorn,David,

Looks like the following thread answers some of the questions about when
this uapi export was done on the existing defines.

https://lkml.org/lkml/2011/7/28/198 

Sounds like the concern is that the older defines PCI_DEVFN, PCI_SLOT,
PCI_FUNC,  and PCI_DEVID could be exported, but not the new ones I
added. I could find any discussion on whether these four older defines
are exportable or the reasons for the export in the above thread.

So the question is if uapi/linux.pci.h isn't the right place, do you
have a recommendation on where they belong. The only alternative I can
think of is include/linux/pci.h. It makes functional and logical sense
to add the new defines to where the existing ones are defines. At least,
not knowing the details of the change that moved PCI_DEVFN etc. to
uapi/pci.h, that is my conclusion.

-- Shuah

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Bjorn Helgaas]

On Mon, Feb 25, 2013 at 9:37 AM, Shuah Khan <shuah.khan@hp.com> wrote:
> On Wed, 2013-02-20 at 18:19 -0700, Bjorn Helgaas wrote:
>> On Mon, Feb 11, 2013 at 4:00 PM, Shuah Khan <shuah.khan@hp.com> wrote:
>> > pci defines PCI_DEVFN(), PCI_SLOT(), and PCI_FUNC() interfaces, however,
>> > it doesn't have interfaces to return PCI bus and PCI device id. Drivers
>> > (AMD IOMMU, and AER) implement module specific definitions for PCI_BUS()
>> > and AMD_IOMMU driver also has a module specific interface to calculate PCI
>> > device id from bus number and devfn.
>> >
>> > Add PCI_BUS and PCI_DEVID interfaces to return PCI bus number and PCI device
>> > id respectively to avoid the need for duplicate definitions in other modules.
>> > AER driver code and AMD IOMMU driver define PCI_BUS. AMD IOMMU driver defines
>> > an interface to calculate device id from bus number, and devfn pair.
>> >
>> > Signed-off-by: Shuah Khan <shuah.khan@hp.com>
>> > ---
>> >  include/uapi/linux/pci.h |    4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
>> > index 3c292bc0..6b2c8b3 100644
>> > --- a/include/uapi/linux/pci.h
>> > +++ b/include/uapi/linux/pci.h
>> > @@ -30,6 +30,10 @@
>> >  #define PCI_DEVFN(slot, func)  ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>> >  #define PCI_SLOT(devfn)                (((devfn) >> 3) & 0x1f)
>> >  #define PCI_FUNC(devfn)                ((devfn) & 0x07)
>> > +#define PCI_DEVID(bus, devfn)  ((((u16)bus) << 8) | devfn)
>> > +
>> > +/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
>> > +#define PCI_BUS(x) (((x) >> 8) & 0xff)
>> >
>> >  /* Ioctls for /proc/bus/pci/X/Y nodes. */
>> >  #define PCIIOC_BASE            ('P' << 24 | 'C' << 16 | 'I' << 8)
>>
>> David, can you point me at a description of include/uapi ... what is
>> there and why, and how we should decide what new things go in
>> include/uapi/linux/pci.h as opposed to include/linux/pci.h?  Maybe
>> there should be something in Documentation/?
>>
>> I'm guessing it's something to do with being exported to userland, but
>> I'm not sure the things in this patch (PCI_DEV_ID, PCI_BUS) are really
>> exportable in the sense of being used for syscalls, etc.
>>
>
> Bjorn,David,
>
> Looks like the following thread answers some of the questions about when
> this uapi export was done on the existing defines.
>
> https://lkml.org/lkml/2011/7/28/198
>
> Sounds like the concern is that the older defines PCI_DEVFN, PCI_SLOT,
> PCI_FUNC,  and PCI_DEVID could be exported, but not the new ones I
> added. I could find any discussion on whether these four older defines
> are exportable or the reasons for the export in the above thread.

I think David's disintegration script took include/linux/pci.h, left
the #ifdef __KERNEL__ parts there, and moved everything else (which
wasn't much) to include/uapi/linux/pci.h.

It's obvious that the PCIIOC_ #defines need to be exported to
user-space for ioctls.  It's not obvious to me why PCI_DEVFN,
PCI_SLOT, and PCI_FUNC need to be exported to user-space.  But I can
imagine user-space using functionality like that, even if it's not
connected to a kernel interface.  I assume the intent of the
disintegration is that only include/uapi would be exposed to
user-space, so keeping those definitions in include/linux/pci.h would
break any user programs that used them.

> So the question is if uapi/linux.pci.h isn't the right place, do you
> have a recommendation on where they belong. The only alternative I can
> think of is include/linux/pci.h. It makes functional and logical sense
> to add the new defines to where the existing ones are defines. At least,
> not knowing the details of the change that moved PCI_DEVFN etc. to
> uapi/pci.h, that is my conclusion.

Using the linux-fullhist tree, I found these:

059d367 Import 2.1.82 -- moved PCI_DEVFN outside #ifdef __KERNEL__
b039547 Import 2.1.76 -- PCI_DEVFN was inside #ifdef __KERNEL__
f6d9739 Import 2.1.68pre1 -- added #ifdef __KERNEL__ (enclosing PCI_DEVFN)
940649f Import 1.3.0 -- added PCI_DEVFN

There's no indication of *why* PCI_DEVFN was exported, of course.

Bottom line, I think it's reasonable to keep PCI_DEVFN, et al., in
uapi/linux/pci.h to keep from breaking user-programs, even though if
we were adding them today we would probably put them in the
kernel-only linux/pci.h.  For the new ones you're adding, I'd propose
putting them in the kernel-only linux/pci.h because we know no user
programs use them.

It's not nice and consistent, but it does follow the simple rule of
"don't expose things to user-space unnecessarily."  We might want to
add a comment to keep somebody from cleaning it up later.

Bjorn

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Shuah Khan]

On Mon, 2013-02-25 at 14:23 -0700, Bjorn Helgaas wrote:
> On Mon, Feb 25, 2013 at 9:37 AM, Shuah Khan <shuah.khan@hp.com> wrote:
> > On Wed, 2013-02-20 at 18:19 -0700, Bjorn Helgaas wrote:
> >> On Mon, Feb 11, 2013 at 4:00 PM, Shuah Khan <shuah.khan@hp.com> wrote:
> >> > pci defines PCI_DEVFN(), PCI_SLOT(), and PCI_FUNC() interfaces, however,
> >> > it doesn't have interfaces to return PCI bus and PCI device id. Drivers
> >> > (AMD IOMMU, and AER) implement module specific definitions for PCI_BUS()
> >> > and AMD_IOMMU driver also has a module specific interface to calculate PCI
> >> > device id from bus number and devfn.
> >> >
> >> > Add PCI_BUS and PCI_DEVID interfaces to return PCI bus number and PCI device
> >> > id respectively to avoid the need for duplicate definitions in other modules.
> >> > AER driver code and AMD IOMMU driver define PCI_BUS. AMD IOMMU driver defines
> >> > an interface to calculate device id from bus number, and devfn pair.
> >> >
> >> > Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> >> > ---
> >> >  include/uapi/linux/pci.h |    4 ++++
> >> >  1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
> >> > index 3c292bc0..6b2c8b3 100644
> >> > --- a/include/uapi/linux/pci.h
> >> > +++ b/include/uapi/linux/pci.h
> >> > @@ -30,6 +30,10 @@
> >> >  #define PCI_DEVFN(slot, func)  ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> >> >  #define PCI_SLOT(devfn)                (((devfn) >> 3) & 0x1f)
> >> >  #define PCI_FUNC(devfn)                ((devfn) & 0x07)
> >> > +#define PCI_DEVID(bus, devfn)  ((((u16)bus) << 8) | devfn)
> >> > +
> >> > +/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
> >> > +#define PCI_BUS(x) (((x) >> 8) & 0xff)
> >> >
> >> >  /* Ioctls for /proc/bus/pci/X/Y nodes. */
> >> >  #define PCIIOC_BASE            ('P' << 24 | 'C' << 16 | 'I' << 8)
> >>
> >> David, can you point me at a description of include/uapi ... what is
> >> there and why, and how we should decide what new things go in
> >> include/uapi/linux/pci.h as opposed to include/linux/pci.h?  Maybe
> >> there should be something in Documentation/?
> >>
> >> I'm guessing it's something to do with being exported to userland, but
> >> I'm not sure the things in this patch (PCI_DEV_ID, PCI_BUS) are really
> >> exportable in the sense of being used for syscalls, etc.
> >>
> >
> > Bjorn,David,
> >
> > Looks like the following thread answers some of the questions about when
> > this uapi export was done on the existing defines.
> >
> > https://lkml.org/lkml/2011/7/28/198
> >
> > Sounds like the concern is that the older defines PCI_DEVFN, PCI_SLOT,
> > PCI_FUNC,  and PCI_DEVID could be exported, but not the new ones I
> > added. I could find any discussion on whether these four older defines
> > are exportable or the reasons for the export in the above thread.
> 
> I think David's disintegration script took include/linux/pci.h, left
> the #ifdef __KERNEL__ parts there, and moved everything else (which
> wasn't much) to include/uapi/linux/pci.h.
> 
> It's obvious that the PCIIOC_ #defines need to be exported to
> user-space for ioctls.  It's not obvious to me why PCI_DEVFN,
> PCI_SLOT, and PCI_FUNC need to be exported to user-space.  But I can
> imagine user-space using functionality like that, even if it's not
> connected to a kernel interface.  I assume the intent of the
> disintegration is that only include/uapi would be exposed to
> user-space, so keeping those definitions in include/linux/pci.h would
> break any user programs that used them.
> 
> > So the question is if uapi/linux.pci.h isn't the right place, do you
> > have a recommendation on where they belong. The only alternative I can
> > think of is include/linux/pci.h. It makes functional and logical sense
> > to add the new defines to where the existing ones are defines. At least,
> > not knowing the details of the change that moved PCI_DEVFN etc. to
> > uapi/pci.h, that is my conclusion.
> 
> Using the linux-fullhist tree, I found these:
> 
> 059d367 Import 2.1.82 -- moved PCI_DEVFN outside #ifdef __KERNEL__
> b039547 Import 2.1.76 -- PCI_DEVFN was inside #ifdef __KERNEL__
> f6d9739 Import 2.1.68pre1 -- added #ifdef __KERNEL__ (enclosing PCI_DEVFN)
> 940649f Import 1.3.0 -- added PCI_DEVFN
> 
> There's no indication of *why* PCI_DEVFN was exported, of course.
> 
> Bottom line, I think it's reasonable to keep PCI_DEVFN, et al., in
> uapi/linux/pci.h to keep from breaking user-programs, even though if
> we were adding them today we would probably put them in the
> kernel-only linux/pci.h.  For the new ones you're adding, I'd propose
> putting them in the kernel-only linux/pci.h because we know no user
> programs use them.

Yeah. These have been in uapi long enough to continue to keep them
there. :)

> 
> It's not nice and consistent, but it does follow the simple rule of
> "don't expose things to user-space unnecessarily."  We might want to
> add a comment to keep somebody from cleaning it up later.

ok. Will resend patches adding the new defines to linux/pci.h and
renaming PCI_BUS() to PCI_BUS_NR() or PCI_BUS_NUM() like you suggested.

Thanks,
-- Shuah

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Shuah Khan]

On Mon, 2013-02-25 at 14:53 -0700, Shuah Khan wrote:
> On Mon, 2013-02-25 at 14:23 -0700, Bjorn Helgaas wrote:
> > On Mon, Feb 25, 2013 at 9:37 AM, Shuah Khan <shuah.khan@hp.com> wrote:
> > > On Wed, 2013-02-20 at 18:19 -0700, Bjorn Helgaas wrote:
> > >> On Mon, Feb 11, 2013 at 4:00 PM, Shuah Khan <shuah.khan@hp.com> wrote:

> > 
> > It's not nice and consistent, but it does follow the simple rule of
> > "don't expose things to user-space unnecessarily."  We might want to
> > add a comment to keep somebody from cleaning it up later.
> 
> ok. Will resend patches adding the new defines to linux/pci.h and
> renaming PCI_BUS() to PCI_BUS_NR() or PCI_BUS_NUM() like you suggested.
> 
> Thanks,
> -- Shuah
> 

Bjorn/Joerg,

I added PCI_BUS_NUM() amd PCI_DEVID() to linux/pci.h. Please note that
changing PCI_BUS() to PCI_BUS_NUM() required additional changes to AMD
IOMMU source files and aer driver. Essentially in addition to removing
local PCI_BUS() define, PCI_BUS() usages are changed to PCI_BUS_NUM(). I
am resending the patches.

Thanks,
-- Shuah

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[Bjorn Helgaas]

On Mon, Feb 11, 2013 at 4:00 PM, Shuah Khan <shuah.khan@hp.com> wrote:
> pci defines PCI_DEVFN(), PCI_SLOT(), and PCI_FUNC() interfaces, however,
> it doesn't have interfaces to return PCI bus and PCI device id. Drivers
> (AMD IOMMU, and AER) implement module specific definitions for PCI_BUS()
> and AMD_IOMMU driver also has a module specific interface to calculate PCI
> device id from bus number and devfn.
>
> Add PCI_BUS and PCI_DEVID interfaces to return PCI bus number and PCI device
> id respectively to avoid the need for duplicate definitions in other modules.
> AER driver code and AMD IOMMU driver define PCI_BUS. AMD IOMMU driver defines
> an interface to calculate device id from bus number, and devfn pair.
>
> Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> ---
>  include/uapi/linux/pci.h |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
> index 3c292bc0..6b2c8b3 100644
> --- a/include/uapi/linux/pci.h
> +++ b/include/uapi/linux/pci.h
> @@ -30,6 +30,10 @@
>  #define PCI_DEVFN(slot, func)  ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)                (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)                ((devfn) & 0x07)
> +#define PCI_DEVID(bus, devfn)  ((((u16)bus) << 8) | devfn)
> +
> +/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
> +#define PCI_BUS(x) (((x) >> 8) & 0xff)

BTW, in the next round, maybe we should call this PCI_BUS_NR() or
similar to avoid confusion with "struct pci_bus"?

Bjorn

Re: [PATCH 1/4] pci: Add PCI_BUS() and PCI_DEVID() interfaces to return bus number and device id

[David Howells]

Bjorn Helgaas <bhelgaas@google.com> wrote:

> David, can you point me at a description of include/uapi ... what is
> there and why, and how we should decide what new things go in
> include/uapi/linux/pci.h as opposed to include/linux/pci.h?  Maybe
> there should be something in Documentation/?

Probably in CodingStyle, Submitting* or somewhere similar.

> I'm guessing it's something to do with being exported to userland, but
> I'm not sure the things in this patch (PCI_DEV_ID, PCI_BUS) are really
> exportable in the sense of being used for syscalls, etc.

As a rule, if it's in uapi/ then it's exported to userspace; if it's not, then
it isn't.  The old headers where disintegrated along the lines of __KERNEL__
delimited sections by my scripts.

David