| Message ID | 1412954137-4567-1-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/10/14 11:15 AM, Murali Karicheri wrote: > When PCI device driver such as that for e1000e tries to set dma mask > using dma_set_mask_and_coherent(), it fails because the dma_pfn_offset > is incorrect on a Keystone SoC. This patch fix this by adding a bus > notifier to set this correctly for PCI devices. > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > --- Looks good. I will pick this up after the merge window. Please send this patch along with PCI dts patches together once the rc1 is tagged. Regards, Santosh
On Fri, Oct 10, 2014 at 11:29:03AM -0400, Santosh Shilimkar wrote: > > > On 10/10/14 11:15 AM, Murali Karicheri wrote: >> When PCI device driver such as that for e1000e tries to set dma mask >> using dma_set_mask_and_coherent(), it fails because the dma_pfn_offset >> is incorrect on a Keystone SoC. This patch fix this by adding a bus >> notifier to set this correctly for PCI devices. >> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >> --- > Looks good. I will pick this up after the merge window. No it doesn't, this patch is crap. Really. Let's look again at what the patch is doing: if (platform_nb.notifier_call) bus_register_notifier(&platform_bus_type, &platform_nb); + if (platform_nb.notifier_call) + bus_register_notifier(&pci_bus_type, &platform_nb); Notice that both calls are using the same platform_nb structure, which is: static struct notifier_block platform_nb; and in turn this is: struct notifier_block { notifier_fn_t notifier_call; struct notifier_block __rcu *next; int priority; }; Notice that "next" pointer - these blocks are used as a single-linked list. So, this block gets registered for the platform bus, and is inserted into that bus notifier chain. That means "next" may be set to a non-NULL next notifier block. Then it gets registered against the PCI bus, which *will* overwrite the next pointer in platform_nb. There are several side effects from this: 1. Any subsequent notifiers on the platform bus which come after _this_ notifier are now orphaned, and will never be called. 2. Any subsequent notifiers on the PCI bus list which come after _this_ notifier will now also be called for the platform bus. 3. Subsequent notifiers registered against either list which are sorted after _this_ notifier will be attached to _both_ lists. In other words, this patch totally screws up the notifier lists for both buses, and while it may not be immediately obvious, if any of the above three scenarios occur, it will probably be very confusing to debug. So, one hell of a big NAK on this patch. Moreover, I have to ask why there wasn't some research done first into how notifiers work *before* writing this code, specifically to find out whether it is safe to register the same notifier block simultaneously onto two lists.
On 10/10/14 11:42 AM, Russell King - ARM Linux wrote: > On Fri, Oct 10, 2014 at 11:29:03AM -0400, Santosh Shilimkar wrote: >> >> >> On 10/10/14 11:15 AM, Murali Karicheri wrote: >>> When PCI device driver such as that for e1000e tries to set dma mask >>> using dma_set_mask_and_coherent(), it fails because the dma_pfn_offset >>> is incorrect on a Keystone SoC. This patch fix this by adding a bus >>> notifier to set this correctly for PCI devices. >>> >>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>> --- >> Looks good. I will pick this up after the merge window. > > No it doesn't, this patch is crap. Really. Let's look again at what the > patch is doing: > > if (platform_nb.notifier_call) > bus_register_notifier(&platform_bus_type, &platform_nb); > + if (platform_nb.notifier_call) > + bus_register_notifier(&pci_bus_type, &platform_nb); > > Notice that both calls are using the same platform_nb structure, which is: > > static struct notifier_block platform_nb; > > and in turn this is: > > struct notifier_block { > notifier_fn_t notifier_call; > struct notifier_block __rcu *next; > int priority; > }; > > Notice that "next" pointer - these blocks are used as a single-linked list. > So, this block gets registered for the platform bus, and is inserted into > that bus notifier chain. That means "next" may be set to a non-NULL > next notifier block. > > Then it gets registered against the PCI bus, which *will* overwrite the > next pointer in platform_nb. > Err.... You are dead right. I missed completely that it is using the same notifier block. Sorry for oversight and thanks for spotting it. Regards, Santosh
On 10/10/2014 11:42 AM, Russell King - ARM Linux wrote: > On Fri, Oct 10, 2014 at 11:29:03AM -0400, Santosh Shilimkar wrote: >> >> >> On 10/10/14 11:15 AM, Murali Karicheri wrote: >>> When PCI device driver such as that for e1000e tries to set dma mask >>> using dma_set_mask_and_coherent(), it fails because the dma_pfn_offset >>> is incorrect on a Keystone SoC. This patch fix this by adding a bus >>> notifier to set this correctly for PCI devices. >>> >>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> >>> --- >> Looks good. I will pick this up after the merge window. > > No it doesn't, this patch is crap. Really. Let's look again at what the > patch is doing: > > if (platform_nb.notifier_call) > bus_register_notifier(&platform_bus_type,&platform_nb); > + if (platform_nb.notifier_call) > + bus_register_notifier(&pci_bus_type,&platform_nb); > > Notice that both calls are using the same platform_nb structure, which is: > > static struct notifier_block platform_nb; > > and in turn this is: > > struct notifier_block { > notifier_fn_t notifier_call; > struct notifier_block __rcu *next; > int priority; > }; > > Notice that "next" pointer - these blocks are used as a single-linked list. > So, this block gets registered for the platform bus, and is inserted into > that bus notifier chain. That means "next" may be set to a non-NULL > next notifier block. > > Then it gets registered against the PCI bus, which *will* overwrite the > next pointer in platform_nb. > > There are several side effects from this: > > 1. Any subsequent notifiers on the platform bus which come after _this_ > notifier are now orphaned, and will never be called. > > 2. Any subsequent notifiers on the PCI bus list which come after _this_ > notifier will now also be called for the platform bus. > > 3. Subsequent notifiers registered against either list which are sorted > after _this_ notifier will be attached to _both_ lists. > > In other words, this patch totally screws up the notifier lists for both > buses, and while it may not be immediately obvious, if any of the above > three scenarios occur, it will probably be very confusing to debug. > > So, one hell of a big NAK on this patch. > > Moreover, I have to ask why there wasn't some research done first into > how notifiers work *before* writing this code, specifically to find out > whether it is safe to register the same notifier block simultaneously > onto two lists. > Thanks Russel for the comments. I didn't see any issues when I tested my PCI driver with this patch, but as you have pointed out there are side effects. I will work to address your concerns. Murali out,
On Friday 10 October 2014 11:15:37 Murali Karicheri wrote: > @@ -54,6 +55,8 @@ static void __init keystone_init(void) > keystone_pm_runtime_init(); > if (platform_nb.notifier_call) > bus_register_notifier(&platform_bus_type, &platform_nb); > + if (platform_nb.notifier_call) > + bus_register_notifier(&pci_bus_type, &platform_nb); > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > No, this looks very wrong. Santosh spent an enormous effort on obsoleting the platform notifier block by adding the range parser to the platform device probe path. You should really remove platform_nb and all associated code rather than adding more code to it. NAK Arnd
On 10/10/2014 02:22 PM, Arnd Bergmann wrote: > On Friday 10 October 2014 11:15:37 Murali Karicheri wrote: >> @@ -54,6 +55,8 @@ static void __init keystone_init(void) >> keystone_pm_runtime_init(); >> if (platform_nb.notifier_call) >> bus_register_notifier(&platform_bus_type,&platform_nb); >> + if (platform_nb.notifier_call) >> + bus_register_notifier(&pci_bus_type,&platform_nb); >> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); >> > > No, this looks very wrong. Santosh spent an enormous effort on obsoleting > the platform notifier block by adding the range parser to the platform > device probe path. > > You should really remove platform_nb and all associated code rather than > adding more code to it. > > NAK > > Arnd Arnd, I took a look at the recent work from Santosh where he has introduced dma-range property to set the dma_pfn_offset for platform devices. Are you referring to this commit below? commit 591c1ee465ce5372385dbc41e7d3e36cbb477bd8 Author: Santosh Shilimkar <santosh.shilimkar@ti.com> Date: Thu Apr 24 11:30:04 2014 -0400 of: configure the platform device dma parameters Retrieve DMA configuration from DT and setup platform device's DMA parameters. The DMA configuration in DT has to be specified using "dma-ranges" and "dma-coherent" properties if supported. We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops using "dma-coherent" device tree properties. The set_arch_dma_coherent_ops macro has to be defined by arch if it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is declared as nop. Based on this, dma configuration parameters get set for the device which is probed through DT. As PCI devices are attached to the PCI bus during scan, and we don't have DT nodes, we could use similar mechanism to pass the dma-range info from parent host platform device to the PCI devices by adding an of_pci_dma_configure() API and hook it to the PCI probe path some where? Please comment on this so that I can work on the right solution to address this issue for Keystone.
On 10/10/14 5:37 PM, Murali Karicheri wrote: > On 10/10/2014 02:22 PM, Arnd Bergmann wrote: >> On Friday 10 October 2014 11:15:37 Murali Karicheri wrote: >>> @@ -54,6 +55,8 @@ static void __init keystone_init(void) >>> keystone_pm_runtime_init(); >>> if (platform_nb.notifier_call) >>> bus_register_notifier(&platform_bus_type,&platform_nb); >>> + if (platform_nb.notifier_call) >>> + bus_register_notifier(&pci_bus_type,&platform_nb); >>> of_platform_populate(NULL, of_default_bus_match_table, NULL, >>> NULL); >>> >> >> No, this looks very wrong. Santosh spent an enormous effort on obsoleting >> the platform notifier block by adding the range parser to the platform >> device probe path. >> >> You should really remove platform_nb and all associated code rather than >> adding more code to it. >> >> NAK >> >> Arnd > Arnd, > > I took a look at the recent work from Santosh where he has introduced > dma-range property to set the dma_pfn_offset for platform devices. Are > you referring to this commit below? > Its the correct commit. > commit 591c1ee465ce5372385dbc41e7d3e36cbb477bd8 > Author: Santosh Shilimkar <santosh.shilimkar@ti.com> > Date: Thu Apr 24 11:30:04 2014 -0400 > > of: configure the platform device dma parameters > > Retrieve DMA configuration from DT and setup platform device's DMA > parameters. The DMA configuration in DT has to be specified using > "dma-ranges" and "dma-coherent" properties if supported. > > We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops > using "dma-coherent" device tree properties. > > The set_arch_dma_coherent_ops macro has to be defined by arch if > it supports coherent dma_ops. Otherwise, > set_arch_dma_coherent_ops() is > declared as nop. > > Based on this, dma configuration parameters get set for the device which > is probed through DT. > > As PCI devices are attached to the PCI bus during scan, and we don't > have DT nodes, we could use similar mechanism to pass the dma-range info > from parent host platform device to the PCI devices by adding an > of_pci_dma_configure() API and hook it to the PCI probe path some where? > Please comment on this so that I can work on the right solution to > address this issue for Keystone. > Adding the DT node parsing code in PCI bus probe path is the right way to go about it. You could re-use some of the helpers from dma parsing code. I let Arnd comment if he disagrees, otherwise I suggest to create an RFC patch and post it on the list. We can take it from there. That also reminded me xhci host code issue with dma-ranges since the devices are manually created there. I will review that thread as well after this merge window. Regards, Santosh
On Friday 10 October 2014 20:04:57 Santosh Shilimkar wrote: > On 10/10/14 5:37 PM, Murali Karicheri wrote: > > Based on this, dma configuration parameters get set for the device which > > is probed through DT. > > > > As PCI devices are attached to the PCI bus during scan, and we don't > > have DT nodes, we could use similar mechanism to pass the dma-range info > > from parent host platform device to the PCI devices by adding an > > of_pci_dma_configure() API and hook it to the PCI probe path some where? > > Please comment on this so that I can work on the right solution to > > address this issue for Keystone. > > > Adding the DT node parsing code in PCI bus probe path is the right way > to go about it. You could re-use some of the helpers from dma parsing > code. > > I let Arnd comment if he disagrees, otherwise I suggest to create an > RFC patch and post it on the list. We can take it from there. Yes, I think that is the correct way forward, we need this anyway to handle IOMMUs correctly, following the patches that Will Deacon did for platform device IOMMU configuration. > That also reminded me xhci host code issue with dma-ranges since the > devices are manually created there. I will review that thread as > well after this merge window. Right, manually created devices are always problematic, you should try to avoid those. Arnd
On 10/11/2014 04:37 PM, Arnd Bergmann wrote: > On Friday 10 October 2014 20:04:57 Santosh Shilimkar wrote: >> On 10/10/14 5:37 PM, Murali Karicheri wrote: >>> Based on this, dma configuration parameters get set for the device which >>> is probed through DT. >>> >>> As PCI devices are attached to the PCI bus during scan, and we don't >>> have DT nodes, we could use similar mechanism to pass the dma-range info >>> from parent host platform device to the PCI devices by adding an >>> of_pci_dma_configure() API and hook it to the PCI probe path some where? >>> Please comment on this so that I can work on the right solution to >>> address this issue for Keystone. >>> >> Adding the DT node parsing code in PCI bus probe path is the right way >> to go about it. You could re-use some of the helpers from dma parsing >> code. >> >> I let Arnd comment if he disagrees, otherwise I suggest to create an >> RFC patch and post it on the list. We can take it from there. > > Yes, I think that is the correct way forward, we need this anyway to > handle IOMMUs correctly, following the patches that Will Deacon did > for platform device IOMMU configuration. Arnd, Could you point me to a thread/link for Will Deacon's IOMMU work? Is it part of the kernel already? Murali > >> That also reminded me xhci host code issue with dma-ranges since the >> devices are manually created there. I will review that thread as >> well after this merge window. > > Right, manually created devices are always problematic, you should > try to avoid those. > > Arnd
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c index 7f352de..13afe95 100644 --- a/arch/arm/mach-keystone/keystone.c +++ b/arch/arm/mach-keystone/keystone.c @@ -15,6 +15,7 @@ #include <linux/of_platform.h> #include <linux/of_address.h> #include <linux/memblock.h> +#include <linux/pci.h> #include <asm/setup.h> #include <asm/mach/map.h> @@ -54,6 +55,8 @@ static void __init keystone_init(void) keystone_pm_runtime_init(); if (platform_nb.notifier_call) bus_register_notifier(&platform_bus_type, &platform_nb); + if (platform_nb.notifier_call) + bus_register_notifier(&pci_bus_type, &platform_nb); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); }
When PCI device driver such as that for e1000e tries to set dma mask using dma_set_mask_and_coherent(), it fails because the dma_pfn_offset is incorrect on a Keystone SoC. This patch fix this by adding a bus notifier to set this correctly for PCI devices. Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> --- arch/arm/mach-keystone/keystone.c | 3 +++ 1 file changed, 3 insertions(+)