[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Murali Karicheri]

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(+)

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);
 }
 
-- 
1.7.9.5

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Santosh Shilimkar]

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

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Santosh Shilimkar]

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

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Murali Karicheri]

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,

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Arnd Bergmann]

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

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Murali Karicheri]

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.

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Santosh Shilimkar]

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

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Arnd Bergmann]

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

[PATCH] ARM: keystone: add bus notifier to set dma_pfn_offset for pci devices

[Murali Karicheri]

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


-- 
Murali Karicheri
Linux Kernel, Texas Instruments