From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sricharan" Subject: RE: [PATCH V7 01/11] iommu/of: Refactor of_iommu_configure() for error handling Date: Tue, 31 Jan 2017 18:41:44 +0530 Message-ID: <006001d27bc3$96156770$c2403650$@codeaurora.org> References: <1485188293-20263-1-git-send-email-sricharan@codeaurora.org> <1485188293-20263-2-git-send-email-sricharan@codeaurora.org> <4388779a-0e83-fadc-83f4-98c46c88d42e@semihalf.com> <009901d278c7$34fee230$9efca690$@codeaurora.org> <003601d27ac6$84866840$8d9338c0$@codeaurora.org> <8387d057-15bd-500c-134e-0e731ff837d6@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <8387d057-15bd-500c-134e-0e731ff837d6-5wv7dgnIgG8@public.gmane.org> Content-Language: en-us List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: 'Robin Murphy' , 'Tomasz Nowicki' , will.deacon-5wv7dgnIgG8@public.gmane.org, joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org, lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-arm-msm@vger.kernel.org Hi Robin, >>>>>> I gave the whole patch set a try on ThunderX. really_probe() is failing >>>>>> on dma_configure()->of_pci_iommu_init() for each PCI device. >>>>> >>>>> When you say "failing", do you mean cleanly, or with a crash? I've >>>>> managed to hit __of_match_node() dereferencing NULL from >>>>> of_iommu_xlate() in a horribly complicated chain of events, which I'm >>>>> trying to figure out now, and I wonder if the two might be related. >>>> >>>> Sorry that there is crash still. __of_match_node seems to checking >>>> for NULL arguments , feels like some invalid pointer was passed in. >>>> Is there any particular sequence to try for this ? >>> >>> Ah, I did figure it out - it wasn't actually a NULL dereference, but an >>> unmapped address. Turns out __iommu_of_table is in initdata, so any >>> driver probing after init, connected to an unprobed IOMMU (in this case >>> disabled in DT), trips over trying to match the now-freed table. I'm >>> working on the fix - technically the bug's in my patch (#2) anyway ;) >>> >> >> Ok, thanks for bringing this out. There is also an issue that >> Sinan has mentioned while testing the ACPI hotplug path, probably >> its related to the above, not sure. I will try to check more on that >> in the meanwhile. Then, taking your fix and fixing the hotplug case >> i will do one more repost. > >OK, I've finally settled on the below fixup for patch #2 - I have some >follow-on ideas for eventually getting of the magic table altogether, >but they can wait until we've got the baseline functionality sorted. >Updated full patch here: > >http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=5616af885f7c5c24f7239d5c689583b2b583c407 Thanks, will use this. Yes, the magic iommu_of_table makes little use with probe deferral in place, except that we are using it at the moment. And the other hotplug issue is not related this though. Regards, Sricharan >-----8<----- > >diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >index 349bd1d01612..1f92d98237d5 100644 >--- a/drivers/iommu/of_iommu.c >+++ b/drivers/iommu/of_iommu.c >@@ -96,6 +96,19 @@ int of_get_dma_window(struct device_node *dn, const >char *prefix, int index, > } > EXPORT_SYMBOL_GPL(of_get_dma_window); > >+static bool of_iommu_driver_present(struct device_node *np) >+{ >+ /* >+ * If the IOMMU still isn't ready by the time we reach init, assume >+ * it never will be. We don't want to defer indefinitely, nor attempt >+ * to dereference __iommu_of_table after it's been freed. >+ */ >+ if (system_state > SYSTEM_BOOTING) >+ return false; >+ >+ return of_match_node(&__iommu_of_table, np); >+} >+ > static const struct iommu_ops > *of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec) > { >@@ -105,7 +118,7 @@ static const struct iommu_ops > > ops = iommu_get_instance(fwnode); > if ((ops && !ops->of_xlate) || >- (!ops && !of_match_node(&__iommu_of_table, iommu_spec->np))) >+ (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; > > err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops); >-- >To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: "Sricharan" To: "'Robin Murphy'" , "'Tomasz Nowicki'" , , , , , , , , , , References: <1485188293-20263-1-git-send-email-sricharan@codeaurora.org> <1485188293-20263-2-git-send-email-sricharan@codeaurora.org> <4388779a-0e83-fadc-83f4-98c46c88d42e@semihalf.com> <009901d278c7$34fee230$9efca690$@codeaurora.org> <003601d27ac6$84866840$8d9338c0$@codeaurora.org> <8387d057-15bd-500c-134e-0e731ff837d6@arm.com> In-Reply-To: <8387d057-15bd-500c-134e-0e731ff837d6@arm.com> Subject: RE: [PATCH V7 01/11] iommu/of: Refactor of_iommu_configure() for error handling Date: Tue, 31 Jan 2017 18:41:44 +0530 Message-ID: <006001d27bc3$96156770$c2403650$@codeaurora.org> MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: Hi Robin, >>>>>> I gave the whole patch set a try on ThunderX. really_probe() is failing >>>>>> on dma_configure()->of_pci_iommu_init() for each PCI device. >>>>> >>>>> When you say "failing", do you mean cleanly, or with a crash? I've >>>>> managed to hit __of_match_node() dereferencing NULL from >>>>> of_iommu_xlate() in a horribly complicated chain of events, which I'm >>>>> trying to figure out now, and I wonder if the two might be related. >>>> >>>> Sorry that there is crash still. __of_match_node seems to checking >>>> for NULL arguments , feels like some invalid pointer was passed in. >>>> Is there any particular sequence to try for this ? >>> >>> Ah, I did figure it out - it wasn't actually a NULL dereference, but an >>> unmapped address. Turns out __iommu_of_table is in initdata, so any >>> driver probing after init, connected to an unprobed IOMMU (in this case >>> disabled in DT), trips over trying to match the now-freed table. I'm >>> working on the fix - technically the bug's in my patch (#2) anyway ;) >>> >> >> Ok, thanks for bringing this out. There is also an issue that >> Sinan has mentioned while testing the ACPI hotplug path, probably >> its related to the above, not sure. I will try to check more on that >> in the meanwhile. Then, taking your fix and fixing the hotplug case >> i will do one more repost. > >OK, I've finally settled on the below fixup for patch #2 - I have some >follow-on ideas for eventually getting of the magic table altogether, >but they can wait until we've got the baseline functionality sorted. >Updated full patch here: > >http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=5616af885f7c5c24f7239d5c689583b2b583c407 Thanks, will use this. Yes, the magic iommu_of_table makes little use with probe deferral in place, except that we are using it at the moment. And the other hotplug issue is not related this though. Regards, Sricharan >-----8<----- > >diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >index 349bd1d01612..1f92d98237d5 100644 >--- a/drivers/iommu/of_iommu.c >+++ b/drivers/iommu/of_iommu.c >@@ -96,6 +96,19 @@ int of_get_dma_window(struct device_node *dn, const >char *prefix, int index, > } > EXPORT_SYMBOL_GPL(of_get_dma_window); > >+static bool of_iommu_driver_present(struct device_node *np) >+{ >+ /* >+ * If the IOMMU still isn't ready by the time we reach init, assume >+ * it never will be. We don't want to defer indefinitely, nor attempt >+ * to dereference __iommu_of_table after it's been freed. >+ */ >+ if (system_state > SYSTEM_BOOTING) >+ return false; >+ >+ return of_match_node(&__iommu_of_table, np); >+} >+ > static const struct iommu_ops > *of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec) > { >@@ -105,7 +118,7 @@ static const struct iommu_ops > > ops = iommu_get_instance(fwnode); > if ((ops && !ops->of_xlate) || >- (!ops && !of_match_node(&__iommu_of_table, iommu_spec->np))) >+ (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; > > err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops); >-- >To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: sricharan@codeaurora.org (Sricharan) Date: Tue, 31 Jan 2017 18:41:44 +0530 Subject: [PATCH V7 01/11] iommu/of: Refactor of_iommu_configure() for error handling In-Reply-To: <8387d057-15bd-500c-134e-0e731ff837d6@arm.com> References: <1485188293-20263-1-git-send-email-sricharan@codeaurora.org> <1485188293-20263-2-git-send-email-sricharan@codeaurora.org> <4388779a-0e83-fadc-83f4-98c46c88d42e@semihalf.com> <009901d278c7$34fee230$9efca690$@codeaurora.org> <003601d27ac6$84866840$8d9338c0$@codeaurora.org> <8387d057-15bd-500c-134e-0e731ff837d6@arm.com> Message-ID: <006001d27bc3$96156770$c2403650$@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Robin, >>>>>> I gave the whole patch set a try on ThunderX. really_probe() is failing >>>>>> on dma_configure()->of_pci_iommu_init() for each PCI device. >>>>> >>>>> When you say "failing", do you mean cleanly, or with a crash? I've >>>>> managed to hit __of_match_node() dereferencing NULL from >>>>> of_iommu_xlate() in a horribly complicated chain of events, which I'm >>>>> trying to figure out now, and I wonder if the two might be related. >>>> >>>> Sorry that there is crash still. __of_match_node seems to checking >>>> for NULL arguments , feels like some invalid pointer was passed in. >>>> Is there any particular sequence to try for this ? >>> >>> Ah, I did figure it out - it wasn't actually a NULL dereference, but an >>> unmapped address. Turns out __iommu_of_table is in initdata, so any >>> driver probing after init, connected to an unprobed IOMMU (in this case >>> disabled in DT), trips over trying to match the now-freed table. I'm >>> working on the fix - technically the bug's in my patch (#2) anyway ;) >>> >> >> Ok, thanks for bringing this out. There is also an issue that >> Sinan has mentioned while testing the ACPI hotplug path, probably >> its related to the above, not sure. I will try to check more on that >> in the meanwhile. Then, taking your fix and fixing the hotplug case >> i will do one more repost. > >OK, I've finally settled on the below fixup for patch #2 - I have some >follow-on ideas for eventually getting of the magic table altogether, >but they can wait until we've got the baseline functionality sorted. >Updated full patch here: > >http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=5616af885f7c5c24f7239d5c689583b2b583c407 Thanks, will use this. Yes, the magic iommu_of_table makes little use with probe deferral in place, except that we are using it at the moment. And the other hotplug issue is not related this though. Regards, Sricharan >-----8<----- > >diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >index 349bd1d01612..1f92d98237d5 100644 >--- a/drivers/iommu/of_iommu.c >+++ b/drivers/iommu/of_iommu.c >@@ -96,6 +96,19 @@ int of_get_dma_window(struct device_node *dn, const >char *prefix, int index, > } > EXPORT_SYMBOL_GPL(of_get_dma_window); > >+static bool of_iommu_driver_present(struct device_node *np) >+{ >+ /* >+ * If the IOMMU still isn't ready by the time we reach init, assume >+ * it never will be. We don't want to defer indefinitely, nor attempt >+ * to dereference __iommu_of_table after it's been freed. >+ */ >+ if (system_state > SYSTEM_BOOTING) >+ return false; >+ >+ return of_match_node(&__iommu_of_table, np); >+} >+ > static const struct iommu_ops > *of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec) > { >@@ -105,7 +118,7 @@ static const struct iommu_ops > > ops = iommu_get_instance(fwnode); > if ((ops && !ops->of_xlate) || >- (!ops && !of_match_node(&__iommu_of_table, iommu_spec->np))) >+ (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; > > err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops); >-- >To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >the body of a message to majordomo at vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html