| Message ID | 1439569159-10051-3-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote: > When pci-host-generic looks for the probe-only property, it seems > to trust the DT to be correctly written, and assumes that there > is a parameter to the property. > > Unfortunately, this is not always the case, and some firmware expose > this property naked. The driver ends up making a decision based on > whatever the property pointer points to, which is likely to be junk. > > Switch to the common of_pci.c implementation that doesn't suffer > from this problem. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/host/pci-host-generic.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 265dd25..545ff4e 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev) > int err; > const char *type; > const struct of_device_id *of_id; > - const int *prop; > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev) > return -EINVAL; > } > > - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); > - if (prop) { > - if (*prop) > - pci_add_flags(PCI_PROBE_ONLY); > - else > - pci_clear_flags(PCI_PROBE_ONLY); > - } > + of_pci_check_probe_only(of_chosen); Do we need support for pci-probe-only in pci-host-generic at all? You're removing the use in amd-overdrive.dts, and there are no other DTs in the kernel tree that mention it. If we can live without it, that would be nice. It seems like a relic from days when we couldn't reliably assign resources. (I'm not saying we can do that reliably even today, but I'd rather make it reliable than turn it off.) > of_id = of_match_node(gen_pci_of_match, np); > pci->cfg.ops = of_id->data; > -- > 2.1.4 > -- 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 Fri, Aug 14, 2015 at 05:40:51PM +0100, Bjorn Helgaas wrote: > On Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote: > > When pci-host-generic looks for the probe-only property, it seems > > to trust the DT to be correctly written, and assumes that there > > is a parameter to the property. > > > > Unfortunately, this is not always the case, and some firmware expose > > this property naked. The driver ends up making a decision based on > > whatever the property pointer points to, which is likely to be junk. > > > > Switch to the common of_pci.c implementation that doesn't suffer > > from this problem. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > drivers/pci/host/pci-host-generic.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > > index 265dd25..545ff4e 100644 > > --- a/drivers/pci/host/pci-host-generic.c > > +++ b/drivers/pci/host/pci-host-generic.c > > @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev) > > int err; > > const char *type; > > const struct of_device_id *of_id; > > - const int *prop; > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); > > - if (prop) { > > - if (*prop) > > - pci_add_flags(PCI_PROBE_ONLY); > > - else > > - pci_clear_flags(PCI_PROBE_ONLY); > > - } > > + of_pci_check_probe_only(of_chosen); > > Do we need support for pci-probe-only in pci-host-generic at all? > You're removing the use in amd-overdrive.dts, and there are no other > DTs in the kernel tree that mention it. > > If we can live without it, that would be nice. It seems like a relic from > days when we couldn't reliably assign resources. (I'm not saying we can do > that reliably even today, but I'd rather make it reliable than turn it > off.) Kvmtool certainly uses it (and generates its own DT, hence why you don't see it in mainline). Not sure about qemu, though. Will -- 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 Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote: > When pci-host-generic looks for the probe-only property, it seems > to trust the DT to be correctly written, and assumes that there > is a parameter to the property. > > Unfortunately, this is not always the case, and some firmware expose > this property naked. The driver ends up making a decision based on > whatever the property pointer points to, which is likely to be junk. > > Switch to the common of_pci.c implementation that doesn't suffer > from this problem. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/host/pci-host-generic.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 265dd25..545ff4e 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev) > int err; > const char *type; > const struct of_device_id *of_id; > - const int *prop; > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev) > return -EINVAL; > } > > - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); > - if (prop) { > - if (*prop) > - pci_add_flags(PCI_PROBE_ONLY); > - else > - pci_clear_flags(PCI_PROBE_ONLY); > - } > + of_pci_check_probe_only(of_chosen); You could probably just make this take void, as the probe-only property is always in the /chosen node. Either way: Acked-by: Will Deacon <will.deacon@arm.com> Will -- 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 14.08.2015, at 09:43, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Aug 14, 2015 at 05:40:51PM +0100, Bjorn Helgaas wrote: >> On Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote: >>> When pci-host-generic looks for the probe-only property, it seems >>> to trust the DT to be correctly written, and assumes that there >>> is a parameter to the property. >>> >>> Unfortunately, this is not always the case, and some firmware expose >>> this property naked. The driver ends up making a decision based on >>> whatever the property pointer points to, which is likely to be junk. >>> >>> Switch to the common of_pci.c implementation that doesn't suffer >>> from this problem. >>> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> drivers/pci/host/pci-host-generic.c | 9 +-------- >>> 1 file changed, 1 insertion(+), 8 deletions(-) >>> >>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c >>> index 265dd25..545ff4e 100644 >>> --- a/drivers/pci/host/pci-host-generic.c >>> +++ b/drivers/pci/host/pci-host-generic.c >>> @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev) >>> int err; >>> const char *type; >>> const struct of_device_id *of_id; >>> - const int *prop; >>> struct device *dev = &pdev->dev; >>> struct device_node *np = dev->of_node; >>> struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); >>> @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev) >>> return -EINVAL; >>> } >>> >>> - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); >>> - if (prop) { >>> - if (*prop) >>> - pci_add_flags(PCI_PROBE_ONLY); >>> - else >>> - pci_clear_flags(PCI_PROBE_ONLY); >>> - } >>> + of_pci_check_probe_only(of_chosen); >> >> Do we need support for pci-probe-only in pci-host-generic at all? >> You're removing the use in amd-overdrive.dts, and there are no other >> DTs in the kernel tree that mention it. >> >> If we can live without it, that would be nice. It seems like a relic from >> days when we couldn't reliably assign resources. (I'm not saying we can do >> that reliably even today, but I'd rather make it reliable than turn it >> off.) > > Kvmtool certainly uses it (and generates its own DT, hence why you don't > see it in mainline). Not sure about qemu, though. QEMU definitely doesn't do proble-only. Is this driver used on real PPC machines too? In that case you also won't see the dt, because it comes with the hardware ;). Alex -- 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 Fri, Aug 14, 2015 at 11:43 AM, Will Deacon <will.deacon@arm.com> wrote: > On Fri, Aug 14, 2015 at 05:40:51PM +0100, Bjorn Helgaas wrote: >> On Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote: >> > When pci-host-generic looks for the probe-only property, it seems >> > to trust the DT to be correctly written, and assumes that there >> > is a parameter to the property. >> > >> > Unfortunately, this is not always the case, and some firmware expose >> > this property naked. The driver ends up making a decision based on >> > whatever the property pointer points to, which is likely to be junk. >> > >> > Switch to the common of_pci.c implementation that doesn't suffer >> > from this problem. >> > >> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> > --- >> > drivers/pci/host/pci-host-generic.c | 9 +-------- >> > 1 file changed, 1 insertion(+), 8 deletions(-) >> > >> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c >> > index 265dd25..545ff4e 100644 >> > --- a/drivers/pci/host/pci-host-generic.c >> > +++ b/drivers/pci/host/pci-host-generic.c >> > @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev) >> > int err; >> > const char *type; >> > const struct of_device_id *of_id; >> > - const int *prop; >> > struct device *dev = &pdev->dev; >> > struct device_node *np = dev->of_node; >> > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); >> > @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev) >> > return -EINVAL; >> > } >> > >> > - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); >> > - if (prop) { >> > - if (*prop) >> > - pci_add_flags(PCI_PROBE_ONLY); >> > - else >> > - pci_clear_flags(PCI_PROBE_ONLY); >> > - } >> > + of_pci_check_probe_only(of_chosen); >> >> Do we need support for pci-probe-only in pci-host-generic at all? >> You're removing the use in amd-overdrive.dts, and there are no other >> DTs in the kernel tree that mention it. >> >> If we can live without it, that would be nice. It seems like a relic from >> days when we couldn't reliably assign resources. (I'm not saying we can do >> that reliably even today, but I'd rather make it reliable than turn it >> off.) > > Kvmtool certainly uses it (and generates its own DT, hence why you don't > see it in mainline). Not sure about qemu, though. Do you know why kvmtool wants probe-only? Would something break if we didn't support probe-only? I guess we'd be looking for a case where Linux assigns resources and that assignment doesn't work with kvmtool? "pci-probe-only" doesn't appear in qemu (git://git.qemu-project.org/qemu.git). (I guess Alexander confirmed that later.) Alexander wrote: > QEMU definitely doesn't do proble-only. > Is this driver used on real PPC machines too? I think you're asking about pci-host-generic.c, and the answer is "no," because pci-host-generic.c currently depends on CONFIG_ARM. Bjorn -- 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 Fri, Aug 14, 2015 at 09:26:21PM +0100, Bjorn Helgaas wrote: > On Fri, Aug 14, 2015 at 11:43 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Aug 14, 2015 at 05:40:51PM +0100, Bjorn Helgaas wrote: > >> Do we need support for pci-probe-only in pci-host-generic at all? > >> You're removing the use in amd-overdrive.dts, and there are no other > >> DTs in the kernel tree that mention it. > >> > >> If we can live without it, that would be nice. It seems like a relic from > >> days when we couldn't reliably assign resources. (I'm not saying we can do > >> that reliably even today, but I'd rather make it reliable than turn it > >> off.) > > > > Kvmtool certainly uses it (and generates its own DT, hence why you don't > > see it in mainline). Not sure about qemu, though. > > Do you know why kvmtool wants probe-only? Would something break if we > didn't support probe-only? I guess we'd be looking for a case where > Linux assigns resources and that assignment doesn't work with kvmtool? It's basically because the BARs aren't writable other than to find the region size. It could fixed with a bit of pain, but it doesn't help older kvmtools that do work with mainline today. Will -- 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/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 265dd25..545ff4e 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev) int err; const char *type; const struct of_device_id *of_id; - const int *prop; struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev) return -EINVAL; } - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL); - if (prop) { - if (*prop) - pci_add_flags(PCI_PROBE_ONLY); - else - pci_clear_flags(PCI_PROBE_ONLY); - } + of_pci_check_probe_only(of_chosen); of_id = of_match_node(gen_pci_of_match, np); pci->cfg.ops = of_id->data;
When pci-host-generic looks for the probe-only property, it seems to trust the DT to be correctly written, and assumes that there is a parameter to the property. Unfortunately, this is not always the case, and some firmware expose this property naked. The driver ends up making a decision based on whatever the property pointer points to, which is likely to be junk. Switch to the common of_pci.c implementation that doesn't suffer from this problem. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/pci/host/pci-host-generic.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)