| Message ID | 1465383890-13538-5-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jun 08, 2016 at 12:04:50PM +0100, Lorenzo Pieralisi wrote: > The arm pcibios_enable_device() implementation exists solely > to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since > on those systems the PCI resources are currently not claimed (ie > inserted in the PCI resource tree - which means their parent > pointer is not correctly set-up) therefore they can not be enabled > since this would trigger PCI set-ups failures. > > After removing the pci=firmware command line option in: > > commit 903589ca7165 ("ARM: 8554/1: kernel: pci: remove pci=firmware > command line parameter handling") > > (that was used to set the PCI_PROBE_ONLY flag through the command line) > and by introducing resources claiming in the PCI host controllers > set-ups that have PCI_PROBE_ONLY as a probe option, there is no need for > arch specific pcibios_enable_device() implementations anymore in that > the kernel can rely on the generic pcibios_enable_device() > implementation without resorting to arch specific code to work around > the missing resources claiming enumeration step. > > On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned > either in pcibios initialization code or PCI host controllers drivers; > since the PCI resource assignment API takes care of inserting the > assigned resources in the resource tree, the resources parent pointers > are correctly set-up, which means that this patch leaves behaviour > unchanged for all arm PCI set-ups that do not set the PCI_PROBE_ONLY > flag. > > Remove the pcibios_enable_device() function from the arm arch back-end > so that the kernel now uses its generic implementation. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Acked-by: Will Deacon <will.deacon@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Russell King <linux@arm.linux.org.uk> > --- > arch/arm/kernel/bios32.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 05e61a2..488545f 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > return start; > } > > -/** > - * pcibios_enable_device - Enable I/O and memory. > - * @dev: PCI device to be enabled > - */ > -int pcibios_enable_device(struct pci_dev *dev, int mask) > -{ > - if (pci_has_flag(PCI_PROBE_ONLY)) > - return 0; > - > - return pci_enable_resources(dev, mask); > -} This looks great. What about the PCI_PROBE_ONLY test in pci_common_init_dev()? Don't we need to either remove that test (if it's impossible to get there with PCI_PROBE_ONLY set), or add a pci_bus_claim_resources() call as we did in pci_host_common_probe()? I think it's unlikely that we'd get to pci_common_init_dev() with PCI_PROBE_ONLY set: - the only way to set PCI_PROBE_ONLY on ARM is to call of_pci_check_probe_only(), - the only ARM caller of of_pci_check_probe_only() is pci_host_common_probe(), - pci_host_common_probe() doesn't call pci_common_init_dev(). But I guess it's possible to imagine a platform with both a generic PCI bridge and a MVEBU, R-Car, or Tegra bridge. Then pci_host_common_probe() could set PCI_PROBE_ONLY, and we'd claim resources under the generic bridge via the previous patch, but still not claim those under the MVEBU bridge. Then enabling the MVEBU devices would fail. I know this is a ridiculous scenario, but the code looks inconsistent as it is. > int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, > enum pci_mmap_state mmap_state, int write_combine) > { > -- > 2.6.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Guan] On Wed, Jun 08, 2016 at 12:04:50PM +0100, Lorenzo Pieralisi wrote: > The arm pcibios_enable_device() implementation exists solely > to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since > on those systems the PCI resources are currently not claimed (ie > inserted in the PCI resource tree - which means their parent > pointer is not correctly set-up) therefore they can not be enabled > since this would trigger PCI set-ups failures. > > After removing the pci=firmware command line option in: > > commit 903589ca7165 ("ARM: 8554/1: kernel: pci: remove pci=firmware > command line parameter handling") 903589ca7165 removed "pci=firmware" from ARM and from Documentation/kernel-parameters.txt (where it was documented as ARM-specific). I think unicore32 copied that code from arm, so unicore32 is one of the few remaining arches that can set PCI_PROBE_ONLY. I suspect this was just copied from arm and may not be necessary on unicore32. Guan, can you confirm that PCI_PROBE_ONLY is necessary on unicore32, or that it can be removed? > (that was used to set the PCI_PROBE_ONLY flag through the command line) > and by introducing resources claiming in the PCI host controllers > set-ups that have PCI_PROBE_ONLY as a probe option, there is no need for > arch specific pcibios_enable_device() implementations anymore in that > the kernel can rely on the generic pcibios_enable_device() > implementation without resorting to arch specific code to work around > the missing resources claiming enumeration step. > > On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned > either in pcibios initialization code or PCI host controllers drivers; > since the PCI resource assignment API takes care of inserting the > assigned resources in the resource tree, the resources parent pointers > are correctly set-up, which means that this patch leaves behaviour > unchanged for all arm PCI set-ups that do not set the PCI_PROBE_ONLY > flag. > > Remove the pcibios_enable_device() function from the arm arch back-end > so that the kernel now uses its generic implementation. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Acked-by: Will Deacon <will.deacon@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Russell King <linux@arm.linux.org.uk> > --- > arch/arm/kernel/bios32.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 05e61a2..488545f 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > return start; > } > > -/** > - * pcibios_enable_device - Enable I/O and memory. > - * @dev: PCI device to be enabled > - */ > -int pcibios_enable_device(struct pci_dev *dev, int mask) > -{ > - if (pci_has_flag(PCI_PROBE_ONLY)) > - return 0; > - > - return pci_enable_resources(dev, mask); > -} > - > int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, > enum pci_mmap_state mmap_state, int write_combine) > { > -- > 2.6.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> [+cc Guan] > > On Wed, Jun 08, 2016 at 12:04:50PM +0100, Lorenzo Pieralisi wrote: >> The arm pcibios_enable_device() implementation exists solely >> to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since >> on those systems the PCI resources are currently not claimed (ie >> inserted in the PCI resource tree - which means their parent >> pointer is not correctly set-up) therefore they can not be enabled >> since this would trigger PCI set-ups failures. >> >> After removing the pci=firmware command line option in: >> >> commit 903589ca7165 ("ARM: 8554/1: kernel: pci: remove pci=firmware >> command line parameter handling") > > 903589ca7165 removed "pci=firmware" from ARM and from > Documentation/kernel-parameters.txt (where it was documented as > ARM-specific). > > I think unicore32 copied that code from arm, so unicore32 is one of > the few remaining arches that can set PCI_PROBE_ONLY. I suspect this > was just copied from arm and may not be necessary on unicore32. > > Guan, can you confirm that PCI_PROBE_ONLY is necessary on unicore32, > or that it can be removed? > Yes, this code was copied from arm, and it's not necessary. It can be removed safely. Guan Xuetao -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 22, 2016 at 05:43:58PM -0500, Bjorn Helgaas wrote: > On Wed, Jun 08, 2016 at 12:04:50PM +0100, Lorenzo Pieralisi wrote: > > The arm pcibios_enable_device() implementation exists solely > > to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since > > on those systems the PCI resources are currently not claimed (ie > > inserted in the PCI resource tree - which means their parent > > pointer is not correctly set-up) therefore they can not be enabled > > since this would trigger PCI set-ups failures. > > > > After removing the pci=firmware command line option in: > > > > commit 903589ca7165 ("ARM: 8554/1: kernel: pci: remove pci=firmware > > command line parameter handling") > > > > (that was used to set the PCI_PROBE_ONLY flag through the command line) > > and by introducing resources claiming in the PCI host controllers > > set-ups that have PCI_PROBE_ONLY as a probe option, there is no need for > > arch specific pcibios_enable_device() implementations anymore in that > > the kernel can rely on the generic pcibios_enable_device() > > implementation without resorting to arch specific code to work around > > the missing resources claiming enumeration step. > > > > On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned > > either in pcibios initialization code or PCI host controllers drivers; > > since the PCI resource assignment API takes care of inserting the > > assigned resources in the resource tree, the resources parent pointers > > are correctly set-up, which means that this patch leaves behaviour > > unchanged for all arm PCI set-ups that do not set the PCI_PROBE_ONLY > > flag. > > > > Remove the pcibios_enable_device() function from the arm arch back-end > > so that the kernel now uses its generic implementation. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Acked-by: Will Deacon <will.deacon@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Russell King <linux@arm.linux.org.uk> > > --- > > arch/arm/kernel/bios32.c | 12 ------------ > > 1 file changed, 12 deletions(-) > > > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > > index 05e61a2..488545f 100644 > > --- a/arch/arm/kernel/bios32.c > > +++ b/arch/arm/kernel/bios32.c > > @@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, > > return start; > > } > > > > -/** > > - * pcibios_enable_device - Enable I/O and memory. > > - * @dev: PCI device to be enabled > > - */ > > -int pcibios_enable_device(struct pci_dev *dev, int mask) > > -{ > > - if (pci_has_flag(PCI_PROBE_ONLY)) > > - return 0; > > - > > - return pci_enable_resources(dev, mask); > > -} > > This looks great. > > What about the PCI_PROBE_ONLY test in pci_common_init_dev()? Don't we > need to either remove that test (if it's impossible to get there with > PCI_PROBE_ONLY set), or add a pci_bus_claim_resources() call as we did > in pci_host_common_probe()? Yes, you are right, I went for the second option given what you say below and sent you and Russell an additional patch that should be added to this series. > I think it's unlikely that we'd get to pci_common_init_dev() with > PCI_PROBE_ONLY set: > > - the only way to set PCI_PROBE_ONLY on ARM is to call > of_pci_check_probe_only(), > > - the only ARM caller of of_pci_check_probe_only() is > pci_host_common_probe(), > > - pci_host_common_probe() doesn't call pci_common_init_dev(). > > But I guess it's possible to imagine a platform with both a generic > PCI bridge and a MVEBU, R-Car, or Tegra bridge. Then > pci_host_common_probe() could set PCI_PROBE_ONLY, and we'd claim > resources under the generic bridge via the previous patch, but still > not claim those under the MVEBU bridge. Then enabling the MVEBU > devices would fail. > > I know this is a ridiculous scenario, but the code looks inconsistent > as it is. Well, I do not think that's an *existing* scenario, but this does not mean that the code is correct, so you are right and it has to be fixed, please let me know if the patch I sent is fine. I really have to remove PCI_PROBE_ONLY entirely from ARM/ARM64 kernels. Thanks ! Lorenzo > > > int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, > > enum pci_mmap_state mmap_state, int write_combine) > > { > > -- > > 2.6.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 05e61a2..488545f 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -590,18 +590,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, return start; } -/** - * pcibios_enable_device - Enable I/O and memory. - * @dev: PCI device to be enabled - */ -int pcibios_enable_device(struct pci_dev *dev, int mask) -{ - if (pci_has_flag(PCI_PROBE_ONLY)) - return 0; - - return pci_enable_resources(dev, mask); -} - int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, enum pci_mmap_state mmap_state, int write_combine) {
The arm pcibios_enable_device() implementation exists solely to prevent enabling PCI resources on PCI_PROBE_ONLY systems, since on those systems the PCI resources are currently not claimed (ie inserted in the PCI resource tree - which means their parent pointer is not correctly set-up) therefore they can not be enabled since this would trigger PCI set-ups failures. After removing the pci=firmware command line option in: commit 903589ca7165 ("ARM: 8554/1: kernel: pci: remove pci=firmware command line parameter handling") (that was used to set the PCI_PROBE_ONLY flag through the command line) and by introducing resources claiming in the PCI host controllers set-ups that have PCI_PROBE_ONLY as a probe option, there is no need for arch specific pcibios_enable_device() implementations anymore in that the kernel can rely on the generic pcibios_enable_device() implementation without resorting to arch specific code to work around the missing resources claiming enumeration step. On !PCI_PROBE_ONLY PCI bus set-ups, resources are always assigned either in pcibios initialization code or PCI host controllers drivers; since the PCI resource assignment API takes care of inserting the assigned resources in the resource tree, the resources parent pointers are correctly set-up, which means that this patch leaves behaviour unchanged for all arm PCI set-ups that do not set the PCI_PROBE_ONLY flag. Remove the pcibios_enable_device() function from the arm arch back-end so that the kernel now uses its generic implementation. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Acked-by: Will Deacon <will.deacon@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Russell King <linux@arm.linux.org.uk> --- arch/arm/kernel/bios32.c | 12 ------------ 1 file changed, 12 deletions(-)