| Message ID | 20191224044146.232713-1-saravanak@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] efi: arm: defer probe of PCIe backed efifb on DT systems | expand |
Hi Saravana,
I love your patch! Yet something to improve:
[auto build test ERROR on efi/next]
[cannot apply to rockchip/for-next keystone/next arm64/for-next/core arm-soc/for-next shawnguo/for-next clk/clk-next arm/for-next linux-rpi/for-rpi-next at91/at91-next v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Saravana-Kannan/efi-arm-defer-probe-of-PCIe-backed-efifb-on-DT-systems/20191225-182253
base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: arm64-alldefconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/firmware/efi/arm-init.o: In function `efifb_add_links':
>> arm-init.c:(.text+0x64): undefined reference to `of_pci_range_parser_init'
arm-init.c:(.text+0x64): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `of_pci_range_parser_init'
>> arm-init.c:(.text+0x78): undefined reference to `of_pci_range_parser_one'
arm-init.c:(.text+0x78): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `of_pci_range_parser_one'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
On Wed, Dec 25, 2019 at 6:46 AM kbuild test robot <lkp@intel.com> wrote: > > Hi Saravana, > > I love your patch! Yet something to improve: > > [auto build test ERROR on efi/next] > [cannot apply to rockchip/for-next keystone/next arm64/for-next/core arm-soc/for-next shawnguo/for-next clk/clk-next arm/for-next linux-rpi/for-rpi-next at91/at91-next v5.5-rc3 next-20191220] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Saravana-Kannan/efi-arm-defer-probe-of-PCIe-backed-efifb-on-DT-systems/20191225-182253 > base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next > config: arm64-alldefconfig (attached as .config) > compiler: aarch64-linux-gcc (GCC) 7.5.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.5.0 make.cross ARCH=arm64 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > drivers/firmware/efi/arm-init.o: In function `efifb_add_links': > >> arm-init.c:(.text+0x64): undefined reference to `of_pci_range_parser_init' > arm-init.c:(.text+0x64): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `of_pci_range_parser_init' > >> arm-init.c:(.text+0x78): undefined reference to `of_pci_range_parser_one' > arm-init.c:(.text+0x78): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `of_pci_range_parser_one' Ard, Not sure what's going on here. of_pci_range_parser_init() and of_pci_range_parser_one() has a stub if CONFIG_OF_ADDRESS isn't defined. So not sure why the bot is reporting "undefined symbol". Thoughts? Also, thoughts on my patch? -Saravana
On Thu, 9 Jan 2020 at 03:23, Saravana Kannan <saravanak@google.com> wrote: > > On Wed, Dec 25, 2019 at 6:46 AM kbuild test robot <lkp@intel.com> wrote: > > > > Hi Saravana, > > > > I love your patch! Yet something to improve: > > > > [auto build test ERROR on efi/next] > > [cannot apply to rockchip/for-next keystone/next arm64/for-next/core arm-soc/for-next shawnguo/for-next clk/clk-next arm/for-next linux-rpi/for-rpi-next at91/at91-next v5.5-rc3 next-20191220] > > [if your patch is applied to the wrong git tree, please drop us a note to help > > improve the system. BTW, we also suggest to use '--base' option to specify the > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > > > url: https://github.com/0day-ci/linux/commits/Saravana-Kannan/efi-arm-defer-probe-of-PCIe-backed-efifb-on-DT-systems/20191225-182253 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next > > config: arm64-alldefconfig (attached as .config) > > compiler: aarch64-linux-gcc (GCC) 7.5.0 > > reproduce: > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # save the attached .config to linux build tree > > GCC_VERSION=7.5.0 make.cross ARCH=arm64 > > > > If you fix the issue, kindly add following tag > > Reported-by: kbuild test robot <lkp@intel.com> > > > > All errors (new ones prefixed by >>): > > > > drivers/firmware/efi/arm-init.o: In function `efifb_add_links': > > >> arm-init.c:(.text+0x64): undefined reference to `of_pci_range_parser_init' > > arm-init.c:(.text+0x64): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `of_pci_range_parser_init' > > >> arm-init.c:(.text+0x78): undefined reference to `of_pci_range_parser_one' > > arm-init.c:(.text+0x78): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `of_pci_range_parser_one' > > Ard, > > Not sure what's going on here. of_pci_range_parser_init() and > of_pci_range_parser_one() has a stub if CONFIG_OF_ADDRESS isn't > defined. So not sure why the bot is reporting "undefined symbol". > Thoughts? > You'll need a #ifdef CONFIG_PCI somewhere, I guess. > Also, thoughts on my patch? > Looks ok to me, but I haven't had a chance to test it yet.
On Tue, 24 Dec 2019 at 05:41, Saravana Kannan <saravanak@google.com> wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > The new of_devlink support breaks PCIe probing on ARM platforms booting > via UEFI if the firmware exposes a EFI framebuffer that is backed by a > PCI device. The reason is that the probing order gets reversed, > resulting in a resource conflict on the framebuffer memory window when > the PCIe probes last, causing it to give up entirely. > > Given that we rely on PCI quirks to deal with EFI framebuffers that get > moved around in memory, we cannot simply drop the memory reservation, so > instead, let's use the device link infrastructure to register this > dependency, and force the probing to occur in the expected order. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > Co-developed-by: Saravana Kannan <saravanak@google.com> > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > > Hi Ard, > > I compile tested it and I think it should work. If you can actually run > and test it, that'd be nice. > > You can also optimize find_pci_overlap_node() by caching the result if > you think that's necessary. > > Right now this code will run always just like your code did. But once I > rename of_devlink to fw_devlink, this code won't be run if fw_devlink is > disabled. > > v1 -> v2: > - Rewrote the device linking part to not depend on initcall ordering > > drivers/firmware/efi/arm-init.c | 106 ++++++++++++++++++++++++++++++-- > 1 file changed, 102 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index 904fa09e6a6b..8b789ff83af0 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -10,10 +10,12 @@ > #define pr_fmt(fmt) "efi: " fmt > > #include <linux/efi.h> > +#include <linux/fwnode.h> > #include <linux/init.h> > #include <linux/memblock.h> > #include <linux/mm_types.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/of_fdt.h> > #include <linux/platform_device.h> > #include <linux/screen_info.h> > @@ -276,15 +278,111 @@ void __init efi_init(void) > efi_memmap_unmap(); > } > > +static bool efifb_overlaps_pci_range(const struct of_pci_range *range) > +{ > + u64 fb_base = screen_info.lfb_base; > + > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > + fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32; > + > + return fb_base >= range->cpu_addr && > + fb_base < (range->cpu_addr + range->size); > +} > + > +static struct device_node *find_pci_overlap_node(void) > +{ > + struct device_node *np; > + > + for_each_node_by_type(np, "pci") { > + struct of_pci_range_parser parser; > + struct of_pci_range range; > + int err; > + > + err = of_pci_range_parser_init(&parser, np); > + if (err) { > + pr_warn("of_pci_range_parser_init() failed: %d\n", err); > + continue; > + } > + > + for_each_of_pci_range(&parser, &range) > + if (efifb_overlaps_pci_range(&range)) > + return np; > + } > + return NULL; > +} > + > +/* > + * If the efifb framebuffer is backed by a PCI graphics controller, we have > + * to ensure that this relation is expressed using a device link when > + * running in DT mode, or the probe order may be reversed, resulting in a > + * resource reservation conflict on the memory window that the efifb > + * framebuffer steals from the PCIe host bridge. > + */ > +static int efifb_add_links(const struct fwnode_handle *fwnode, > + struct device *dev) > +{ > + struct device_node *sup_np; > + struct device *sup_dev; > + > + sup_np = find_pci_overlap_node(); > + > + /* > + * If there's no PCI graphics controller backing the efifb, we are > + * done here. > + */ > + if (!sup_np) > + return 0; > + > + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > + of_node_put(sup_np); > + > + /* > + * Return -ENODEV if the PCI graphics controller device hasn't been > + * registered yet. This ensures that efifb isn't allowed to probe > + * and this function is retried again when new devices are > + * registered. > + */ > + if (!sup_dev) > + return -ENODEV; > + > + /* > + * If this fails, retrying this function at a later point won't > + * change anything. So, don't return an error after this. > + */ > + if (!device_link_add(dev, sup_dev, 0)) > + dev_warn(dev, "device_link_add() failed\n"); > + > + put_device(sup_dev); > + > + return 0; > +} > + > +static struct fwnode_operations efifb_fwnode_ops = { Please make this const > + .add_links = efifb_add_links, > +}; > + > +static struct fwnode_handle efifb_fwnode = { > + .ops = &efifb_fwnode_ops, > +}; > + > static int __init register_gop_device(void) > { > - void *pd; > + struct platform_device *pd; > + int err; > > if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > return 0; > > - pd = platform_device_register_data(NULL, "efi-framebuffer", 0, > - &screen_info, sizeof(screen_info)); > - return PTR_ERR_OR_ZERO(pd); > + pd = platform_device_alloc("efi-framebuffer", 0); > + if (!pd) > + return -ENOMEM; > + Add if (IS_ENABLED(CONFIG_PCI)) here > + pd->dev.fwnode = &efifb_fwnode; > + > + err = platform_device_add_data(pd, &screen_info, sizeof(screen_info)); > + if (err) > + return err; > + > + return platform_device_add(pd); > } > subsys_initcall(register_gop_device); > -- > 2.24.1.735.g03f4e72817-goog > With the changes above Reviewed-by: Ard Biesheuvel <ardb@kernel.org> but it still needs testing as well.
On Thu, 9 Jan 2020 at 15:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Tue, 24 Dec 2019 at 05:41, Saravana Kannan <saravanak@google.com> wrote: > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > The new of_devlink support breaks PCIe probing on ARM platforms booting > > via UEFI if the firmware exposes a EFI framebuffer that is backed by a > > PCI device. The reason is that the probing order gets reversed, > > resulting in a resource conflict on the framebuffer memory window when > > the PCIe probes last, causing it to give up entirely. > > > > Given that we rely on PCI quirks to deal with EFI framebuffers that get > > moved around in memory, we cannot simply drop the memory reservation, so > > instead, let's use the device link infrastructure to register this > > dependency, and force the probing to occur in the expected order. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Co-developed-by: Saravana Kannan <saravanak@google.com> > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > > > Hi Ard, > > > > I compile tested it and I think it should work. If you can actually run > > and test it, that'd be nice. > > > > You can also optimize find_pci_overlap_node() by caching the result if > > you think that's necessary. > > > > Right now this code will run always just like your code did. But once I > > rename of_devlink to fw_devlink, this code won't be run if fw_devlink is > > disabled. > > > > v1 -> v2: > > - Rewrote the device linking part to not depend on initcall ordering > > > > drivers/firmware/efi/arm-init.c | 106 ++++++++++++++++++++++++++++++-- > > 1 file changed, 102 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > > index 904fa09e6a6b..8b789ff83af0 100644 > > --- a/drivers/firmware/efi/arm-init.c > > +++ b/drivers/firmware/efi/arm-init.c > > @@ -10,10 +10,12 @@ > > #define pr_fmt(fmt) "efi: " fmt > > > > #include <linux/efi.h> > > +#include <linux/fwnode.h> > > #include <linux/init.h> > > #include <linux/memblock.h> > > #include <linux/mm_types.h> > > #include <linux/of.h> > > +#include <linux/of_address.h> > > #include <linux/of_fdt.h> > > #include <linux/platform_device.h> > > #include <linux/screen_info.h> > > @@ -276,15 +278,111 @@ void __init efi_init(void) > > efi_memmap_unmap(); > > } > > > > +static bool efifb_overlaps_pci_range(const struct of_pci_range *range) > > +{ > > + u64 fb_base = screen_info.lfb_base; > > + > > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > + fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32; > > + > > + return fb_base >= range->cpu_addr && > > + fb_base < (range->cpu_addr + range->size); > > +} > > + > > +static struct device_node *find_pci_overlap_node(void) > > +{ > > + struct device_node *np; > > + > > + for_each_node_by_type(np, "pci") { > > + struct of_pci_range_parser parser; > > + struct of_pci_range range; > > + int err; > > + > > + err = of_pci_range_parser_init(&parser, np); > > + if (err) { > > + pr_warn("of_pci_range_parser_init() failed: %d\n", err); > > + continue; > > + } > > + > > + for_each_of_pci_range(&parser, &range) > > + if (efifb_overlaps_pci_range(&range)) > > + return np; > > + } > > + return NULL; > > +} > > + > > +/* > > + * If the efifb framebuffer is backed by a PCI graphics controller, we have > > + * to ensure that this relation is expressed using a device link when > > + * running in DT mode, or the probe order may be reversed, resulting in a > > + * resource reservation conflict on the memory window that the efifb > > + * framebuffer steals from the PCIe host bridge. > > + */ > > +static int efifb_add_links(const struct fwnode_handle *fwnode, > > + struct device *dev) > > +{ > > + struct device_node *sup_np; > > + struct device *sup_dev; > > + > > + sup_np = find_pci_overlap_node(); > > + > > + /* > > + * If there's no PCI graphics controller backing the efifb, we are > > + * done here. > > + */ > > + if (!sup_np) > > + return 0; > > + > > + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > > + of_node_put(sup_np); > > + > > + /* > > + * Return -ENODEV if the PCI graphics controller device hasn't been > > + * registered yet. This ensures that efifb isn't allowed to probe > > + * and this function is retried again when new devices are > > + * registered. > > + */ > > + if (!sup_dev) > > + return -ENODEV; > > + > > + /* > > + * If this fails, retrying this function at a later point won't > > + * change anything. So, don't return an error after this. > > + */ > > + if (!device_link_add(dev, sup_dev, 0)) > > + dev_warn(dev, "device_link_add() failed\n"); > > + > > + put_device(sup_dev); > > + > > + return 0; > > +} > > + > > +static struct fwnode_operations efifb_fwnode_ops = { > > Please make this const > > > + .add_links = efifb_add_links, > > +}; > > + > > +static struct fwnode_handle efifb_fwnode = { > > + .ops = &efifb_fwnode_ops, > > +}; > > + > > static int __init register_gop_device(void) > > { > > - void *pd; > > + struct platform_device *pd; > > + int err; > > > > if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > > return 0; > > > > - pd = platform_device_register_data(NULL, "efi-framebuffer", 0, > > - &screen_info, sizeof(screen_info)); > > - return PTR_ERR_OR_ZERO(pd); > > + pd = platform_device_alloc("efi-framebuffer", 0); > > + if (!pd) > > + return -ENOMEM; > > + > > Add > > if (IS_ENABLED(CONFIG_PCI)) > > here > > > + pd->dev.fwnode = &efifb_fwnode; > > + > > + err = platform_device_add_data(pd, &screen_info, sizeof(screen_info)); > > + if (err) > > + return err; > > + > > + return platform_device_add(pd); > > } > > subsys_initcall(register_gop_device); > > -- > > 2.24.1.735.g03f4e72817-goog > > > > With the changes above > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org> > > but it still needs testing as well. I'm having trouble reproducing the original issue, so it is difficult to confirm that it works as before. In any case, I'm inclined to just take this through the EFI tree for v5.6, and if it needs additional tweaks, we can apply them as fixes on top.
On Thu, Jan 9, 2020 at 6:06 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Tue, 24 Dec 2019 at 05:41, Saravana Kannan <saravanak@google.com> wrote: > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > The new of_devlink support breaks PCIe probing on ARM platforms booting > > via UEFI if the firmware exposes a EFI framebuffer that is backed by a > > PCI device. The reason is that the probing order gets reversed, > > resulting in a resource conflict on the framebuffer memory window when > > the PCIe probes last, causing it to give up entirely. > > > > Given that we rely on PCI quirks to deal with EFI framebuffers that get > > moved around in memory, we cannot simply drop the memory reservation, so > > instead, let's use the device link infrastructure to register this > > dependency, and force the probing to occur in the expected order. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Co-developed-by: Saravana Kannan <saravanak@google.com> > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > > > Hi Ard, > > > > I compile tested it and I think it should work. If you can actually run > > and test it, that'd be nice. > > > > You can also optimize find_pci_overlap_node() by caching the result if > > you think that's necessary. > > > > Right now this code will run always just like your code did. But once I > > rename of_devlink to fw_devlink, this code won't be run if fw_devlink is > > disabled. > > > > v1 -> v2: > > - Rewrote the device linking part to not depend on initcall ordering > > > > drivers/firmware/efi/arm-init.c | 106 ++++++++++++++++++++++++++++++-- > > 1 file changed, 102 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > > index 904fa09e6a6b..8b789ff83af0 100644 > > --- a/drivers/firmware/efi/arm-init.c > > +++ b/drivers/firmware/efi/arm-init.c > > @@ -10,10 +10,12 @@ > > #define pr_fmt(fmt) "efi: " fmt > > > > #include <linux/efi.h> > > +#include <linux/fwnode.h> > > #include <linux/init.h> > > #include <linux/memblock.h> > > #include <linux/mm_types.h> > > #include <linux/of.h> > > +#include <linux/of_address.h> > > #include <linux/of_fdt.h> > > #include <linux/platform_device.h> > > #include <linux/screen_info.h> > > @@ -276,15 +278,111 @@ void __init efi_init(void) > > efi_memmap_unmap(); > > } > > > > +static bool efifb_overlaps_pci_range(const struct of_pci_range *range) > > +{ > > + u64 fb_base = screen_info.lfb_base; > > + > > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > + fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32; > > + > > + return fb_base >= range->cpu_addr && > > + fb_base < (range->cpu_addr + range->size); > > +} > > + > > +static struct device_node *find_pci_overlap_node(void) > > +{ > > + struct device_node *np; > > + > > + for_each_node_by_type(np, "pci") { > > + struct of_pci_range_parser parser; > > + struct of_pci_range range; > > + int err; > > + > > + err = of_pci_range_parser_init(&parser, np); > > + if (err) { > > + pr_warn("of_pci_range_parser_init() failed: %d\n", err); > > + continue; > > + } > > + > > + for_each_of_pci_range(&parser, &range) > > + if (efifb_overlaps_pci_range(&range)) > > + return np; > > + } > > + return NULL; > > +} > > + > > +/* > > + * If the efifb framebuffer is backed by a PCI graphics controller, we have > > + * to ensure that this relation is expressed using a device link when > > + * running in DT mode, or the probe order may be reversed, resulting in a > > + * resource reservation conflict on the memory window that the efifb > > + * framebuffer steals from the PCIe host bridge. > > + */ > > +static int efifb_add_links(const struct fwnode_handle *fwnode, > > + struct device *dev) > > +{ > > + struct device_node *sup_np; > > + struct device *sup_dev; > > + > > + sup_np = find_pci_overlap_node(); > > + > > + /* > > + * If there's no PCI graphics controller backing the efifb, we are > > + * done here. > > + */ > > + if (!sup_np) > > + return 0; > > + > > + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > > + of_node_put(sup_np); > > + > > + /* > > + * Return -ENODEV if the PCI graphics controller device hasn't been > > + * registered yet. This ensures that efifb isn't allowed to probe > > + * and this function is retried again when new devices are > > + * registered. > > + */ > > + if (!sup_dev) > > + return -ENODEV; > > + > > + /* > > + * If this fails, retrying this function at a later point won't > > + * change anything. So, don't return an error after this. > > + */ > > + if (!device_link_add(dev, sup_dev, 0)) > > + dev_warn(dev, "device_link_add() failed\n"); > > + > > + put_device(sup_dev); > > + > > + return 0; > > +} > > + > > +static struct fwnode_operations efifb_fwnode_ops = { > > Please make this const Ack > > + .add_links = efifb_add_links, > > +}; > > + > > +static struct fwnode_handle efifb_fwnode = { > > + .ops = &efifb_fwnode_ops, > > +}; > > + > > static int __init register_gop_device(void) > > { > > - void *pd; > > + struct platform_device *pd; > > + int err; > > > > if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > > return 0; > > > > - pd = platform_device_register_data(NULL, "efi-framebuffer", 0, > > - &screen_info, sizeof(screen_info)); > > - return PTR_ERR_OR_ZERO(pd); > > + pd = platform_device_alloc("efi-framebuffer", 0); > > + if (!pd) > > + return -ENOMEM; > > + > > Add > > if (IS_ENABLED(CONFIG_PCI)) > > here As in around the line below where I set the fwnode? Then it's going to warn about unused variable. Maybe I should just do the if (IS_ENABLED(CONFIG_PCI)) inside the add_links() function to short circuit it. Responding to your other email here. Adding ifdef CONFIG_PCI was my first inclination, but then those 2 functions are defined if CONFIG_OF_ADDRESS are defined and if not the stubs are always there. There's no #ifdef CONFIG_PCI around any of them AFAICT. So I'm confused about what's going on. > > + pd->dev.fwnode = &efifb_fwnode; > > + > > + err = platform_device_add_data(pd, &screen_info, sizeof(screen_info)); > > + if (err) > > + return err; > > + > > + return platform_device_add(pd); > > } > > subsys_initcall(register_gop_device); > > -- > > 2.24.1.735.g03f4e72817-goog > > > > With the changes above Yeah, will make them all. > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org> > Thanks! -Saravana
On Thu, 9 Jan 2020 at 19:53, Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Jan 9, 2020 at 6:06 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > > On Tue, 24 Dec 2019 at 05:41, Saravana Kannan <saravanak@google.com> wrote: > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > The new of_devlink support breaks PCIe probing on ARM platforms booting > > > via UEFI if the firmware exposes a EFI framebuffer that is backed by a > > > PCI device. The reason is that the probing order gets reversed, > > > resulting in a resource conflict on the framebuffer memory window when > > > the PCIe probes last, causing it to give up entirely. > > > > > > Given that we rely on PCI quirks to deal with EFI framebuffers that get > > > moved around in memory, we cannot simply drop the memory reservation, so > > > instead, let's use the device link infrastructure to register this > > > dependency, and force the probing to occur in the expected order. > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > Co-developed-by: Saravana Kannan <saravanak@google.com> > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > --- > > > > > > Hi Ard, > > > > > > I compile tested it and I think it should work. If you can actually run > > > and test it, that'd be nice. > > > > > > You can also optimize find_pci_overlap_node() by caching the result if > > > you think that's necessary. > > > > > > Right now this code will run always just like your code did. But once I > > > rename of_devlink to fw_devlink, this code won't be run if fw_devlink is > > > disabled. > > > > > > v1 -> v2: > > > - Rewrote the device linking part to not depend on initcall ordering > > > > > > drivers/firmware/efi/arm-init.c | 106 ++++++++++++++++++++++++++++++-- > > > 1 file changed, 102 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > > > index 904fa09e6a6b..8b789ff83af0 100644 > > > --- a/drivers/firmware/efi/arm-init.c > > > +++ b/drivers/firmware/efi/arm-init.c > > > @@ -10,10 +10,12 @@ > > > #define pr_fmt(fmt) "efi: " fmt > > > > > > #include <linux/efi.h> > > > +#include <linux/fwnode.h> > > > #include <linux/init.h> > > > #include <linux/memblock.h> > > > #include <linux/mm_types.h> > > > #include <linux/of.h> > > > +#include <linux/of_address.h> > > > #include <linux/of_fdt.h> > > > #include <linux/platform_device.h> > > > #include <linux/screen_info.h> > > > @@ -276,15 +278,111 @@ void __init efi_init(void) > > > efi_memmap_unmap(); > > > } > > > > > > +static bool efifb_overlaps_pci_range(const struct of_pci_range *range) > > > +{ > > > + u64 fb_base = screen_info.lfb_base; > > > + > > > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > > + fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32; > > > + > > > + return fb_base >= range->cpu_addr && > > > + fb_base < (range->cpu_addr + range->size); > > > +} > > > + > > > +static struct device_node *find_pci_overlap_node(void) > > > +{ > > > + struct device_node *np; > > > + > > > + for_each_node_by_type(np, "pci") { > > > + struct of_pci_range_parser parser; > > > + struct of_pci_range range; > > > + int err; > > > + > > > + err = of_pci_range_parser_init(&parser, np); > > > + if (err) { > > > + pr_warn("of_pci_range_parser_init() failed: %d\n", err); > > > + continue; > > > + } > > > + > > > + for_each_of_pci_range(&parser, &range) > > > + if (efifb_overlaps_pci_range(&range)) > > > + return np; > > > + } > > > + return NULL; > > > +} > > > + > > > +/* > > > + * If the efifb framebuffer is backed by a PCI graphics controller, we have > > > + * to ensure that this relation is expressed using a device link when > > > + * running in DT mode, or the probe order may be reversed, resulting in a > > > + * resource reservation conflict on the memory window that the efifb > > > + * framebuffer steals from the PCIe host bridge. > > > + */ > > > +static int efifb_add_links(const struct fwnode_handle *fwnode, > > > + struct device *dev) > > > +{ > > > + struct device_node *sup_np; > > > + struct device *sup_dev; > > > + > > > + sup_np = find_pci_overlap_node(); > > > + > > > + /* > > > + * If there's no PCI graphics controller backing the efifb, we are > > > + * done here. > > > + */ > > > + if (!sup_np) > > > + return 0; > > > + > > > + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > > > + of_node_put(sup_np); > > > + > > > + /* > > > + * Return -ENODEV if the PCI graphics controller device hasn't been > > > + * registered yet. This ensures that efifb isn't allowed to probe > > > + * and this function is retried again when new devices are > > > + * registered. > > > + */ > > > + if (!sup_dev) > > > + return -ENODEV; > > > + > > > + /* > > > + * If this fails, retrying this function at a later point won't > > > + * change anything. So, don't return an error after this. > > > + */ > > > + if (!device_link_add(dev, sup_dev, 0)) > > > + dev_warn(dev, "device_link_add() failed\n"); > > > + > > > + put_device(sup_dev); > > > + > > > + return 0; > > > +} > > > + > > > +static struct fwnode_operations efifb_fwnode_ops = { > > > > Please make this const > > Ack > > > > + .add_links = efifb_add_links, > > > +}; > > > + > > > +static struct fwnode_handle efifb_fwnode = { > > > + .ops = &efifb_fwnode_ops, > > > +}; > > > + > > > static int __init register_gop_device(void) > > > { > > > - void *pd; > > > + struct platform_device *pd; > > > + int err; > > > > > > if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > > > return 0; > > > > > > - pd = platform_device_register_data(NULL, "efi-framebuffer", 0, > > > - &screen_info, sizeof(screen_info)); > > > - return PTR_ERR_OR_ZERO(pd); > > > + pd = platform_device_alloc("efi-framebuffer", 0); > > > + if (!pd) > > > + return -ENOMEM; > > > + > > > > Add > > > > if (IS_ENABLED(CONFIG_PCI)) > > > > here > > As in around the line below where I set the fwnode? Then it's going to > warn about unused variable. If you use if() instead of #if, the compiler will not warn about an unused variable here. > Maybe I should just do the if > (IS_ENABLED(CONFIG_PCI)) inside the add_links() function to short > circuit it. > No, let's put it it here. > Responding to your other email here. Adding ifdef CONFIG_PCI was my > first inclination, but then those 2 functions are defined if > CONFIG_OF_ADDRESS are defined and if not the stubs are always there. > There's no #ifdef CONFIG_PCI around any of them AFAICT. So I'm > confused about what's going on. > There is > > > + pd->dev.fwnode = &efifb_fwnode; > > > + > > > + err = platform_device_add_data(pd, &screen_info, sizeof(screen_info)); > > > + if (err) > > > + return err; > > > + > > > + return platform_device_add(pd); > > > } > > > subsys_initcall(register_gop_device); > > > -- > > > 2.24.1.735.g03f4e72817-goog > > > > > > > With the changes above > > Yeah, will make them all. > > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org> > > > > Thanks! > > -Saravana
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c index 904fa09e6a6b..8b789ff83af0 100644 --- a/drivers/firmware/efi/arm-init.c +++ b/drivers/firmware/efi/arm-init.c @@ -10,10 +10,12 @@ #define pr_fmt(fmt) "efi: " fmt #include <linux/efi.h> +#include <linux/fwnode.h> #include <linux/init.h> #include <linux/memblock.h> #include <linux/mm_types.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/of_fdt.h> #include <linux/platform_device.h> #include <linux/screen_info.h> @@ -276,15 +278,111 @@ void __init efi_init(void) efi_memmap_unmap(); } +static bool efifb_overlaps_pci_range(const struct of_pci_range *range) +{ + u64 fb_base = screen_info.lfb_base; + + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) + fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32; + + return fb_base >= range->cpu_addr && + fb_base < (range->cpu_addr + range->size); +} + +static struct device_node *find_pci_overlap_node(void) +{ + struct device_node *np; + + for_each_node_by_type(np, "pci") { + struct of_pci_range_parser parser; + struct of_pci_range range; + int err; + + err = of_pci_range_parser_init(&parser, np); + if (err) { + pr_warn("of_pci_range_parser_init() failed: %d\n", err); + continue; + } + + for_each_of_pci_range(&parser, &range) + if (efifb_overlaps_pci_range(&range)) + return np; + } + return NULL; +} + +/* + * If the efifb framebuffer is backed by a PCI graphics controller, we have + * to ensure that this relation is expressed using a device link when + * running in DT mode, or the probe order may be reversed, resulting in a + * resource reservation conflict on the memory window that the efifb + * framebuffer steals from the PCIe host bridge. + */ +static int efifb_add_links(const struct fwnode_handle *fwnode, + struct device *dev) +{ + struct device_node *sup_np; + struct device *sup_dev; + + sup_np = find_pci_overlap_node(); + + /* + * If there's no PCI graphics controller backing the efifb, we are + * done here. + */ + if (!sup_np) + return 0; + + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); + of_node_put(sup_np); + + /* + * Return -ENODEV if the PCI graphics controller device hasn't been + * registered yet. This ensures that efifb isn't allowed to probe + * and this function is retried again when new devices are + * registered. + */ + if (!sup_dev) + return -ENODEV; + + /* + * If this fails, retrying this function at a later point won't + * change anything. So, don't return an error after this. + */ + if (!device_link_add(dev, sup_dev, 0)) + dev_warn(dev, "device_link_add() failed\n"); + + put_device(sup_dev); + + return 0; +} + +static struct fwnode_operations efifb_fwnode_ops = { + .add_links = efifb_add_links, +}; + +static struct fwnode_handle efifb_fwnode = { + .ops = &efifb_fwnode_ops, +}; + static int __init register_gop_device(void) { - void *pd; + struct platform_device *pd; + int err; if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) return 0; - pd = platform_device_register_data(NULL, "efi-framebuffer", 0, - &screen_info, sizeof(screen_info)); - return PTR_ERR_OR_ZERO(pd); + pd = platform_device_alloc("efi-framebuffer", 0); + if (!pd) + return -ENOMEM; + + pd->dev.fwnode = &efifb_fwnode; + + err = platform_device_add_data(pd, &screen_info, sizeof(screen_info)); + if (err) + return err; + + return platform_device_add(pd); } subsys_initcall(register_gop_device);