| Message ID | 1511328675-21981-10-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Dropping the extra mailing lists and Dave, this is a rather trivial thing. On Wed, 2017-11-22 at 00:30 -0500, Sinan Kaya wrote: > pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as > where a PCI device is present. This restricts the device drivers to be > reused for other domain numbers. > > Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't > extract the domain number. Other places, use the actual domain number from > the device. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> <SNIP> > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -419,7 +419,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) > { > - dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0)); > + uint16_t devfn = PCI_DEVFN(0, 0) The would have to be "unsigned int" according to the function signature. > + > + dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, devfn); But the most straightforward change is to simply convert to: dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); Can you please resend like that. Looks like this is a part of abigger series, so others may prefer latter form too, to avoid the variable. Regards, Joonas
On 11/22/2017 2:52 AM, Joonas Lahtinen wrote: > Dropping the extra mailing lists and Dave, this is a rather trivial thing. > > On Wed, 2017-11-22 at 00:30 -0500, Sinan Kaya wrote: >> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as >> where a PCI device is present. This restricts the device drivers to be >> reused for other domain numbers. >> >> Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't >> extract the domain number. Other places, use the actual domain number from >> the device. >> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > <SNIP> > >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -419,7 +419,9 @@ static int i915_getparam(struct drm_device *dev, void *data, >> >> static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) >> { >> - dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0)); >> + uint16_t devfn = PCI_DEVFN(0, 0) > > The would have to be "unsigned int" according to the function > signature. Even though the return value is unsigned int, PCI_DEVFN cannot be bigger than 16 bits. http://elixir.free-electrons.com/linux/v4.14.1/source/include/uapi/linux/pci.h#L31 /* * The PCI interface treats multi-function devices as independent * devices. The slot/function address of each device is encoded * in a single byte as follows: * * 7:3 = slot * 2:0 = function */ It is common practice to use u16 for keeping devfn information in the kernel. > >> + >> + dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, devfn); > > But the most straightforward change is to simply convert to: > > dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, > PCI_DEVFN(0, 0)); > > Can you please resend like that. I did this at the beginning and but, I hit a checkpatch problem with more than 80 characters. That's why, I moved the devfn value assignment to a different line. > > Looks like this is a part of abigger series, so others may prefer > latter form too, to avoid the variable. > > Regards, Joonas >
On Wed, 22 Nov 2017, Sinan Kaya <okaya@codeaurora.org> wrote: > On 11/22/2017 2:52 AM, Joonas Lahtinen wrote: >> Dropping the extra mailing lists and Dave, this is a rather trivial thing. >> >> On Wed, 2017-11-22 at 00:30 -0500, Sinan Kaya wrote: >>> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as >>> where a PCI device is present. This restricts the device drivers to be >>> reused for other domain numbers. >>> >>> Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't >>> extract the domain number. Other places, use the actual domain number from >>> the device. >>> >>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> >> <SNIP> >> >>> +++ b/drivers/gpu/drm/i915/i915_drv.c >>> @@ -419,7 +419,9 @@ static int i915_getparam(struct drm_device *dev, void *data, >>> >>> static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) >>> { >>> - dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0)); >>> + uint16_t devfn = PCI_DEVFN(0, 0) >> >> The would have to be "unsigned int" according to the function >> signature. > > Even though the return value is unsigned int, PCI_DEVFN cannot be bigger > than 16 bits. > > http://elixir.free-electrons.com/linux/v4.14.1/source/include/uapi/linux/pci.h#L31 > > /* > * The PCI interface treats multi-function devices as independent > * devices. The slot/function address of each device is encoded > * in a single byte as follows: > * > * 7:3 = slot > * 2:0 = function > */ > > It is common practice to use u16 for keeping devfn information in the > kernel. > >> >>> + >>> + dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, devfn); >> >> But the most straightforward change is to simply convert to: >> >> dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, >> PCI_DEVFN(0, 0)); >> >> Can you please resend like that. > > I did this at the beginning and but, I hit a checkpatch problem with > more than 80 characters. That's why, I moved the devfn value assignment > to a different line. Please ignore checkpatch when it makes the code worse. 80 is not a strict limit. BR, Jani. > >> >> Looks like this is a part of abigger series, so others may prefer >> latter form too, to avoid the variable. >> >> Regards, Joonas >>
On 11/22/2017 01:50 PM, Jani Nikula wrote: >>> dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, >>> PCI_DEVFN(0, 0)); >>> >>> Can you please resend like that. >> I did this at the beginning and but, I hit a checkpatch problem with >> more than 80 characters. That's why, I moved the devfn value assignment >> to a different line. > Please ignore checkpatch when it makes the code worse. 80 is not a > strict limit. How about this: dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); That should fit, and it's very readable.
On Wed, 22 Nov 2017, Timur Tabi <timur@codeaurora.org> wrote: > On 11/22/2017 01:50 PM, Jani Nikula wrote: >>>> dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, >>>> PCI_DEVFN(0, 0)); >>>> >>>> Can you please resend like that. >>> I did this at the beginning and but, I hit a checkpatch problem with >>> more than 80 characters. That's why, I moved the devfn value assignment >>> to a different line. >> Please ignore checkpatch when it makes the code worse. 80 is not a >> strict limit. > > How about this: > > dev_priv->bridge_dev = > pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); > > That should fit, and it's very readable. *shrug* I'm fine with either, though I think the first one I like more when I git grep stuff. Get more contex. But let's not get hung up on this. BR, Jani.
On Wed, 2017-11-22 at 11:28 -0500, Sinan Kaya wrote: > On 11/22/2017 2:52 AM, Joonas Lahtinen wrote: > > Dropping the extra mailing lists and Dave, this is a rather trivial thing. > > > > On Wed, 2017-11-22 at 00:30 -0500, Sinan Kaya wrote: > > > pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as > > > where a PCI device is present. This restricts the device drivers to be > > > reused for other domain numbers. > > > > > > Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't > > > extract the domain number. Other places, use the actual domain number from > > > the device. > > > > > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > > > <SNIP> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -419,7 +419,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > > > > > static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) > > > { > > > - dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0)); > > > + uint16_t devfn = PCI_DEVFN(0, 0) > > > > The would have to be "unsigned int" according to the function > > signature. > > Even though the return value is unsigned int, PCI_DEVFN cannot be bigger > than 16 bits. > > http://elixir.free-electrons.com/linux/v4.14.1/source/include/uapi/linux/pci.h#L31 > > /* > * The PCI interface treats multi-function devices as independent > * devices. The slot/function address of each device is encoded > * in a single byte as follows: > * > * 7:3 = slot > * 2:0 = function > */ > > It is common practice to use u16 for keeping devfn information in the > kernel. A quick run of 'git grep "PCI_DEVFN" | egrep "(uint|u)(16|32)"' vs. 'git grep "PCI_DEVFN" | grep "unsigned"' didn't indicate that. And following the function signature would be the thing to do, unless there is some compelling reason not to. But it's not really relevant as a variable is not needed. > > > + > > > + dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, devfn); > > > > But the most straightforward change is to simply convert to: > > > > dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, > > PCI_DEVFN(0, 0)); > > > > Can you please resend like that. > > I did this at the beginning and but, I hit a checkpatch problem with > more than 80 characters. That's why, I moved the devfn value assignment > to a different line. It is not longer than 80 characters when you break line before the PCI_DEVFN like I proposed. Or you can break it after the "=", too, either way works fine. Regards, Joonas
On 11/23/2017 2:42 AM, Joonas Lahtinen wrote: > It is not longer than 80 characters when you break line before the > PCI_DEVFN like I proposed. Or you can break it after the "=", too, > either way works fine. I posted V2 with this suggestion. https://patchwork.kernel.org/patch/10071759/ Let me know what you think about the domain number call here. u32 domain = pci_domain_nr(dev_priv->drm.pdev->bus); I don't have a way to test this change. I traced the drm_device's pdev assignment in the code. It should work fine but I don't have a proof. If you tell me for sure that this driver will never use a non-zero domain or this driver will never run on another architecture, I can hard-code it as 0 as well.
On Thu, 2017-11-23 at 13:37 -0500, Sinan Kaya wrote: > On 11/23/2017 2:42 AM, Joonas Lahtinen wrote: > > It is not longer than 80 characters when you break line before the > > PCI_DEVFN like I proposed. Or you can break it after the "=", too, > > either way works fine. > > I posted V2 with this suggestion. > > https://patchwork.kernel.org/patch/10071759/ > > Let me know what you think about the domain number call here. > > u32 domain = pci_domain_nr(dev_priv->drm.pdev->bus); The function return signature is 'int' as well as the parameter in pci_get_domain_bus_and_slot is type 'int', so why fight it and try to make the variable 'u32'? > I don't have a way to test this change. I traced the drm_device's pdev > assignment in the code. It should work fine but I don't have a proof. Send the patch to intel-gfx@lists.freedesktop.org and it should get automatically tested by our CI system. Regards, Joonas
On 11/27/2017 3:56 AM, Joonas Lahtinen wrote: > On Thu, 2017-11-23 at 13:37 -0500, Sinan Kaya wrote: >> On 11/23/2017 2:42 AM, Joonas Lahtinen wrote: >>> It is not longer than 80 characters when you break line before the >>> PCI_DEVFN like I proposed. Or you can break it after the "=", too, >>> either way works fine. >> >> I posted V2 with this suggestion. >> >> https://patchwork.kernel.org/patch/10071759/ >> >> Let me know what you think about the domain number call here. >> >> u32 domain = pci_domain_nr(dev_priv->drm.pdev->bus); > > The function return signature is 'int' as well as the parameter in > pci_get_domain_bus_and_slot is type 'int', so why fight it and try to > make the variable 'u32'? Agreed. I made this change on V3. > >> I don't have a way to test this change. I traced the drm_device's pdev >> assignment in the code. It should work fine but I don't have a proof. > > Send the patch to intel-gfx@lists.freedesktop.org and it should get > automatically tested by our CI system. Thanks, Let me post V3 and put this address on CC. > > Regards, Joonas >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f45cfe..2ca7603 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -419,7 +419,9 @@ static int i915_getparam(struct drm_device *dev, void *data, static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) { - dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0)); + uint16_t devfn = PCI_DEVFN(0, 0) + + dev_priv->bridge_dev = pci_get_domain_bus_and_slot(0, 0, devfn); if (!dev_priv->bridge_dev) { DRM_ERROR("bridge device not found\n"); return -1;
pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as where a PCI device is present. This restricts the device drivers to be reused for other domain numbers. Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't extract the domain number. Other places, use the actual domain number from the device. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/gpu/drm/i915/i915_drv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)