Re: [PATCH] dma-mapping: move dma configuration to bus infrastructure

2018-03-13 Thread kbuild test robot 
Hi Nipun,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc5 next-20180313]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Nipun-Gupta/dma-mapping-move-dma-configuration-to-bus-infrastructure/20180313-225250
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/base/platform.c:1133:5: sparse: symbol 'platform_dma_configure' was 
>> not declared. Should it be static?
>> drivers/base/platform.c:1149:6: sparse: symbol 'platform_dma_deconfigure' 
>> was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH] dma-mapping: platform_dma_configure() can be static

2018-03-13 Thread kbuild test robot 

Fixes: 9a019f425175 ("dma-mapping: move dma configuration to bus 
infrastructure")
Signed-off-by: Fengguang Wu 
---
 platform.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index adf94eb..dc9c790 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1130,7 +1130,7 @@ int platform_pm_restore(struct device *dev)
 
 #endif /* CONFIG_HIBERNATE_CALLBACKS */
 
-int platform_dma_configure(struct device *dev)
+static int platform_dma_configure(struct device *dev)
 {
enum dev_dma_attr attr;
int ret = 0;
@@ -1146,7 +1146,7 @@ int platform_dma_configure(struct device *dev)
return ret;
 }
 
-void platform_dma_deconfigure(struct device *dev)
+static void platform_dma_deconfigure(struct device *dev)
 {
of_dma_deconfigure(dev);
acpi_dma_deconfigure(dev);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

2018-03-13 Thread Andy Shevchenko 
On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook  wrote:
> On 03/13/2018 12:20 PM, Andy Shevchenko wrote:

>>> +   } else if (obuf[0] == '0' && obuf[1] == 'x') {
>>> +   n = sscanf(obuf, "%x", _iommu_devid);
>>> +   } else {
>>> +   n = sscanf(obuf, "%d", _iommu_devid);
>>> +   }

>> kstrtoint() ?

> I see various mechanisms for this sort of thing, and simply chose one.
> Am happy to use whatever is preferred.

sscanf() has an enormous overhead for cases like this.

simple

ret = kstrtoint();
if (ret)
 ... do error handling ...


-- 
With Best Regards,
Andy Shevchenko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

2018-03-13 Thread Andy Shevchenko 
On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook  wrote:
> On 03/13/2018 12:16 PM, Andy Shevchenko wrote:
>> On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook  wrote:

>>> +#include 
>>> +#include 
>>> +#include 
>>
>>
>> Keep in order?

> What order would that be? These few needed files are listed in the same
> order as which they appear in amd_iommu.c. I'm gonna need a preference
> spelled out, please (and a rationale, so I may better understand).

To increase readability and avoid potential header duplication (here
is can bus protocol implementation where the problem exists for real,
even in new code!)

>>> +/* DebugFS helpers */
>>> +#defineOBUFP   (obuf + oboff)
>>> +#defineOBUFLEN obuflen
>>> +#defineOBUFSPC (OBUFLEN - oboff)
>>> +#defineOSCNPRINTF(fmt, ...) \
>>> +   scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)
>>
>>
>> I don't see any advantages of this. Other way around, they will simple
>> makes things hard to read an understand in place.
>
>
> I used this technique in the CCP driver code (where it was accepted), in an
> effort to do the opposite of what you claim: make the code more readable.
> Given the 80 column limit, a large number of arguments, and very long
> statements, IMO something needs to give. I don't find the use of #defines to
> be obfuscating.
>
> I'm not trying to argue, but rather simply state the perspective / reasoning
> I used to create a source file I feel is manageable. I have 17 more iommu
> patches built upon this strategy, and this seems to be advantageous for all
> of them.

It's fine to me as long as it's fine to maintainer, but honestly
speaking I would avoid such code as much as possible. Imagine that
your "advantage" basically becomes disadvantage to everyone else who
is not familiar with the code.

Each time I see macro in the code, I would need to at least step on
it, run cscope, read, and come back. And at this point of time I
already forgot what this code is doing, it does use sNprintf() or
sCNprintf() or whatever wrapper on top of either...

>>> +   for (i = start ; i <= end ; i++)

>> Missed {}

> Wasn't sure about the M.O. given that the body of this loop is a single if
> statement. And I don't see anywhere in
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> in section 3.1 where curly braces are called for in this situation. May I
> ask for clarification on the style rule, please?

You can do nothing, though I'm guided by the end of section 3.0
(though it tells only about 'if' case).

>>> @@ -89,6 +89,7 @@
>>>   #define ACPI_DEVFLAG_ATSDIS 0x1000
>>>
>>>   #define LOOP_TIMEOUT   10
>>> +
>>>   /*
>>>* ACPI table definitions
>>>*

>> Doesn't belong to the patch.

> I'm sorry, I don't understand. The added blank line doesn't belong to the
> patch?

Correct.

-- 
With Best Regards,
Andy Shevchenko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

2018-03-13 Thread Gary R Hook 

On 03/13/2018 12:20 PM, Andy Shevchenko wrote:



+   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
+   PCI_BUS_NUM(amd_iommu_devid),
+   PCI_SLOT(amd_iommu_devid),
+   PCI_FUNC(amd_iommu_devid),


Perhaps at some point we will have an extension to %p to print PCI BDFs.


But until then  ;-)


+   if (strnchr(obuf, OBUFLEN, ':'))
+   {


Style


D'oh!


+   } else if (obuf[0] == '0' && obuf[1] == 'x') {
+   n = sscanf(obuf, "%x", _iommu_devid);
+   } else {
+   n = sscanf(obuf, "%d", _iommu_devid);
+   }


kstrtoint() ?


I see various mechanisms for this sort of thing, and simply chose one.
Am happy to use whatever is preferred.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

2018-03-13 Thread Gary R Hook 

On 03/13/2018 12:16 PM, Andy Shevchenko wrote:

On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook  wrote:


+   default n


Redundant


Roger that.


+#include 
+#include 
+#include 


Keep in order?


What order would that be? These few needed files are listed in the same
order as which they appear in amd_iommu.c. I'm gonna need a preference
spelled out, please (and a rationale, so I may better understand).


+#include "amd_iommu_proto.h"
+#include "amd_iommu_types.h"



+/* DebugFS helpers */
+#defineOBUFP   (obuf + oboff)
+#defineOBUFLEN obuflen
+#defineOBUFSPC (OBUFLEN - oboff)
+#defineOSCNPRINTF(fmt, ...) \
+   scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)


I don't see any advantages of this. Other way around, they will simple
makes things hard to read an understand in place.


I used this technique in the CCP driver code (where it was accepted), in 
an effort to do the opposite of what you claim: make the code more 
readable. Given the 80 column limit, a large number of arguments, and 
very long statements, IMO something needs to give. I don't find the use 
of #defines to be obfuscating.


I'm not trying to argue, but rather simply state the perspective / 
reasoning I used to create a source file I feel is manageable. I have 17 
more iommu patches built upon this strategy, and this seems to be 
advantageous for all of them.






+   for (i = start ; i <= end ; i++)


Missed {}


Wasn't sure about the M.O. given that the body of this loop is a single 
if statement. And I don't see anywhere in

https://www.kernel.org/doc/html/latest/process/coding-style.html
in section 3.1 where curly braces are called for in this situation. May 
I ask for clarification on the style rule, please?





+   if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
+   || amd_iommu_dev_table[i].data[1])
+   n++;
+   return n;
+}



+
+static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
+ char __user *ubuf,
+ size_t count, loff_t *offp)
+{
+   struct amd_iommu *iommu = filp->private_data;



+   unsigned int obuflen = 512;


Sounds like way too much.


I can tune these up.




+   if (!iommu)
+   return 0;


When this possible?


It was intended as a sanity check, but if this happens, much worse has 
already gone wrong. I'll remove.





+   obuf = kmalloc(OBUFLEN, GFP_KERNEL);
+   if (!obuf)
+   return -ENOMEM;
+
+   n = amd_iommu_count_valid_dtes(0, 0x);
+   oboff += OSCNPRINTF("%d\n", n);



+   return ret;
+}




@@ -89,6 +89,7 @@
  #define ACPI_DEVFLAG_ATSDIS 0x1000

  #define LOOP_TIMEOUT   10
+
  /*
   * ACPI table definitions
   *


Doesn't belong to the patch.


I'm sorry, I don't understand. The added blank line doesn't belong to 
the patch?





+#endif
+
+


Extra unneeded line.


Thanks,

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 5/5] iommu/amd - Add a debugfs entry to specify a IOMMU device table entry

2018-03-13 Thread Andy Shevchenko 
On Fri, Mar 9, 2018 at 2:51 AM, Gary R Hook  wrote:
> Initially (at boot) the device table values dumped are all of the
> active devices.  Add a devid debugfs file to allow the user to select a
> single device table entry to dump (active or not). Let any devid value
> greater than the maximum allowable PCI ID (0x) restore the
> behavior to that effective at boot.

> +   oboff += OSCNPRINTF("%02x:%02x:%x (%u / %04x)\n",
> +   PCI_BUS_NUM(amd_iommu_devid),
> +   PCI_SLOT(amd_iommu_devid),
> +   PCI_FUNC(amd_iommu_devid),

Perhaps at some point we will have an extension to %p to print PCI BDFs.

> +   if (strnchr(obuf, OBUFLEN, ':'))
> +   {

Style

> +   } else if (obuf[0] == '0' && obuf[1] == 'x') {
> +   n = sscanf(obuf, "%x", _iommu_devid);
> +   } else {
> +   n = sscanf(obuf, "%d", _iommu_devid);
> +   }

kstrtoint() ?

-- 
With Best Regards,
Andy Shevchenko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

2018-03-13 Thread Andy Shevchenko 
On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook  wrote:

> +   default n

Redundant


> +#include 
> +#include 
> +#include 

Keep in order?

> +#include "amd_iommu_proto.h"
> +#include "amd_iommu_types.h"

> +/* DebugFS helpers */
> +#defineOBUFP   (obuf + oboff)
> +#defineOBUFLEN obuflen
> +#defineOBUFSPC (OBUFLEN - oboff)
> +#defineOSCNPRINTF(fmt, ...) \
> +   scnprintf(OBUFP, OBUFSPC, fmt, ## __VA_ARGS__)

I don't see any advantages of this. Other way around, they will simple
makes things hard to read an understand in place.


> +   for (i = start ; i <= end ; i++)

Missed {}

> +   if ((amd_iommu_dev_table[i].data[0] ^ 0x3)
> +   || amd_iommu_dev_table[i].data[1])
> +   n++;
> +   return n;
> +}

> +
> +static ssize_t amd_iommu_debugfs_dtecount_read(struct file *filp,
> + char __user *ubuf,
> + size_t count, loff_t *offp)
> +{
> +   struct amd_iommu *iommu = filp->private_data;

> +   unsigned int obuflen = 512;

Sounds like way too much.

> +   if (!iommu)
> +   return 0;

When this possible?

> +   obuf = kmalloc(OBUFLEN, GFP_KERNEL);
> +   if (!obuf)
> +   return -ENOMEM;
> +
> +   n = amd_iommu_count_valid_dtes(0, 0x);
> +   oboff += OSCNPRINTF("%d\n", n);

> +   return ret;
> +}


> @@ -89,6 +89,7 @@
>  #define ACPI_DEVFLAG_ATSDIS 0x1000
>
>  #define LOOP_TIMEOUT   10
> +
>  /*
>   * ACPI table definitions
>   *

Doesn't belong to the patch.

> +#endif
> +
> +

Extra unneeded line.

-- 
With Best Regards,
Andy Shevchenko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCHv3] iommu/intel: Ratelimit each dmar fault printing

2018-03-13 Thread Dmitry Safonov via iommu 
Gentle ping?

On Mon, 2018-03-05 at 15:00 +, Dmitry Safonov wrote:
> Hi Joerg,
> 
> What do you think about v3?
> It looks like, I can solve my softlookups with just a bit more proper
> ratelimiting..
> 
> On Thu, 2018-02-15 at 19:17 +, Dmitry Safonov wrote:
> > There is a ratelimit for printing, but it's incremented each time
> > the
> > cpu recives dmar fault interrupt. While one interrupt may signal
> > about
> > *many* faults.
> > So, measuring the impact it turns out that reading/clearing one
> > fault
> > takes < 1 usec, and printing info about the fault takes ~170 msec.
> > 
> > Having in mind that maximum number of fault recording registers per
> > remapping hardware unit is 256.. IRQ handler may run for (170*256)
> > msec.
> > And as fault-serving loop runs without a time limit, during
> > servicing
> > new faults may occur..
> > 
> > Ratelimit each fault printing rather than each irq printing.
> > 
> > Fixes: commit c43fce4eebae ("iommu/vt-d: Ratelimit fault handler")
> > 
> > BUG: spinlock lockup suspected on CPU#0, CliShell/9903
> >  lock: 0x81a47440, .magic: dead4ead, .owner:
> > kworker/u16:2/8915, .owner_cpu: 6
> > CPU: 0 PID: 9903 Comm: CliShell
> > Call Trace:$\n'
> > [..] dump_stack+0x65/0x83$\n'
> > [..] spin_dump+0x8f/0x94$\n'
> > [..] do_raw_spin_lock+0x123/0x170$\n'
> > [..] _raw_spin_lock_irqsave+0x32/0x3a$\n'
> > [..] uart_chars_in_buffer+0x20/0x4d$\n'
> > [..] tty_chars_in_buffer+0x18/0x1d$\n'
> > [..] n_tty_poll+0x1cb/0x1f2$\n'
> > [..] tty_poll+0x5e/0x76$\n'
> > [..] do_select+0x363/0x629$\n'
> > [..] compat_core_sys_select+0x19e/0x239$\n'
> > [..] compat_SyS_select+0x98/0xc0$\n'
> > [..] sysenter_dispatch+0x7/0x25$\n'
> > [..]
> > NMI backtrace for cpu 6
> > CPU: 6 PID: 8915 Comm: kworker/u16:2
> > Workqueue: dmar_fault dmar_fault_work
> > Call Trace:$\n'
> > [..] wait_for_xmitr+0x26/0x8f$\n'
> > [..] serial8250_console_putchar+0x1c/0x2c$\n'
> > [..] uart_console_write+0x40/0x4b$\n'
> > [..] serial8250_console_write+0xe6/0x13f$\n'
> > [..] call_console_drivers.constprop.13+0xce/0x103$\n'
> > [..] console_unlock+0x1f8/0x39b$\n'
> > [..] vprintk_emit+0x39e/0x3e6$\n'
> > [..] printk+0x4d/0x4f$\n'
> > [..] dmar_fault+0x1a8/0x1fc$\n'
> > [..] dmar_fault_work+0x15/0x17$\n'
> > [..] process_one_work+0x1e8/0x3a9$\n'
> > [..] worker_thread+0x25d/0x345$\n'
> > [..] kthread+0xea/0xf2$\n'
> > [..] ret_from_fork+0x58/0x90$\n'
> > 
> > Cc: Alex Williamson 
> > Cc: David Woodhouse 
> > Cc: Ingo Molnar 
> > Cc: Joerg Roedel 
> > Cc: Lu Baolu 
> > Cc: iommu@lists.linux-foundation.org
> > Signed-off-by: Dmitry Safonov 
> > ---
> > Maybe it's worth to limit while(1) cycle.
> > If IOMMU generates faults with equal speed as irq handler cleans
> > them, it may turn into long-irq-disabled region again.
> > Not sure if it can happen anyway.
> > 
> >  drivers/iommu/dmar.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> > index accf58388bdb..6c4ea32ee6a9 100644
> > --- a/drivers/iommu/dmar.c
> > +++ b/drivers/iommu/dmar.c
> > @@ -1618,17 +1618,13 @@ irqreturn_t dmar_fault(int irq, void
> > *dev_id)
> > int reg, fault_index;
> > u32 fault_status;
> > unsigned long flag;
> > -   bool ratelimited;
> > static DEFINE_RATELIMIT_STATE(rs,
> >   DEFAULT_RATELIMIT_INTERVAL,
> >   DEFAULT_RATELIMIT_BURST);
> >  
> > -   /* Disable printing, simply clear the fault when
> > ratelimited
> > */
> > -   ratelimited = !__ratelimit();
> > -
> > raw_spin_lock_irqsave(>register_lock, flag);
> > fault_status = readl(iommu->reg + DMAR_FSTS_REG);
> > -   if (fault_status && !ratelimited)
> > +   if (fault_status && __ratelimit())
> > pr_err("DRHD: handling fault status reg %x\n",
> > fault_status);
> >  
> > /* TBD: ignore advanced fault log currently */
> > @@ -1638,6 +1634,8 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
> > fault_index = dma_fsts_fault_record_index(fault_status);
> > reg = cap_fault_reg_offset(iommu->cap);
> > while (1) {
> > +   /* Disable printing, simply clear the fault when
> > ratelimited */
> > +   bool ratelimited = !__ratelimit();
> > u8 fault_reason;
> > u16 source_id;
> > u64 guest_addr;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] dma-mapping: move dma configuration to bus infrastructure

2018-03-13 Thread Nipun Gupta 


> -Original Message-
> From: Robin Murphy [mailto:robin.mur...@arm.com]
> Sent: Tuesday, March 13, 2018 17:06
> 
> On 12/03/18 15:24, Nipun Gupta wrote:
> > The change introduces 'dma_configure' & 'dma_deconfigure'as
> > bus callback functions so each bus can choose to implement
> > its own dma configuration function.
> > This eases the addition of new busses w.r.t. adding the dma
> > configuration functionality.
> 
> It's probably worth clarifying - either in the commit message, the
> kerneldoc, or both - that the bus-specific aspect is that of mapping
> between a given device on the bus and the relevant firmware description
> of its DMA configuration.

Okay.

>
> > The change also updates the PCI, Platform and ACPI bus to use
> > new introduced callbacks.
> >
> > Signed-off-by: Nipun Gupta 
> > ---
> >   - This patch is based on the comments on:
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> rk.kernel.org%2Fpatch%2F10259087%2F=02%7C01%7Cnipun.gupta%40nxp.com%7
> Cc541100ecb944e7650a408d588d69ab0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C636565377665676631=k2Xjn5B1GECx4UjCg9tChOpOrD3NPM7BkzIXLLSv3rI%
> 3D=0
> >   - I have validated for PCI and platform, but not for AMBA as I
> > do not have infrastructure to validate it.
> > Can anyone please validate them on AMBA?
> >
> >   drivers/amba/bus.c  | 38 -
> >   drivers/base/dd.c   | 14 +++
> >   drivers/base/dma-mapping.c  | 41 ---
> >   drivers/base/platform.c | 36 ++-
> >   drivers/pci/pci-driver.c| 59 -
> 
> >   include/linux/device.h  |  6 +
> >   include/linux/dma-mapping.h | 12 -
> >   7 files changed, 124 insertions(+), 82 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 594c228..58241d2 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -20,6 +20,8 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> > +#include 
> >
> >   #include 
> >
> > @@ -171,6 +173,28 @@ static int amba_pm_runtime_resume(struct device
> *dev)
> >   }
> >   #endif /* CONFIG_PM */
> >
> > +int amba_dma_configure(struct device *dev)
> > +{
> > +   enum dev_dma_attr attr;
> > +   int ret = 0;
> > +
> > +   if (dev->of_node) {
> > +   ret = of_dma_configure(dev, dev->of_node);
> > +   } else if (has_acpi_companion(dev)) {
> > +   attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
> > +   if (attr != DEV_DMA_NOT_SUPPORTED)
> > +   ret = acpi_dma_configure(dev, attr);
> > +   }
> > +
> > +   return ret;
> > +}
> 
> I would be inclined to have amba_bustype just reference
> platform_dma_configure() directly rather than duplicate it like this,
> since there's no sensible reason for them to ever differ.

I think dma_common_configure() having this as the common code seems pretty
Decent. All the busses will probably call this API.

> 
> > +
> > +void amba_dma_deconfigure(struct device *dev)
> > +{
> > +   of_dma_deconfigure(dev);
> > +   acpi_dma_deconfigure(dev);
> > +}
> > +
> >   static const struct dev_pm_ops amba_pm = {
> > .suspend= pm_generic_suspend,
> > .resume = pm_generic_resume,
> > @@ -190,12 +214,14 @@ static int amba_pm_runtime_resume(struct device
> *dev)
> >* so we call the bus "amba".
> >*/
> >   struct bus_type amba_bustype = {
> > -   .name   = "amba",
> > -   .dev_groups = amba_dev_groups,
> > -   .match  = amba_match,
> > -   .uevent = amba_uevent,
> > -   .pm = _pm,
> > -   .force_dma  = true,
> > +   .name   = "amba",
> > +   .dev_groups = amba_dev_groups,
> > +   .match  = amba_match,
> > +   .uevent = amba_uevent,
> > +   .pm = _pm,
> > +   .dma_configure  = amba_dma_configure,
> > +   .dma_deconfigure= amba_dma_deconfigure,
> > +   .force_dma  = true,
> 
> This patch should also be removing force_dma because it no longer makes
> sense. If DMA configuration is now done by a bus-level callback, then a
> bus which wants its children to get DMA configuration needs to implement
> that callback; there's nowhere to force a "default" global behaviour any
> more.

Agree. We will also need to pass a force_dma flag in of_dma_configure() as
Christoph suggests. Ill update this.

> 
> >   };
> >
> >   static int __init amba_init(void)
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index de6fd09..f124f3f 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -421,9 +421,11 @@ static int really_probe(struct device *dev, struct
> device_driver *drv)
> > if (ret)
> > goto pinctrl_bind_failed;
> >
> > -   ret = dma_configure(dev);
> > -   if (ret)
> > -   goto dma_failed;
> > +   if

RE: [PATCH] dma-mapping: move dma configuration to bus infrastructure

2018-03-13 Thread Nipun Gupta 


> -Original Message-
> From: Christoph Hellwig [mailto:h...@lst.de]
> Sent: Tuesday, March 13, 2018 13:05
> > +int amba_dma_configure(struct device *dev)
> > +{
> > +   enum dev_dma_attr attr;
> > +   int ret = 0;
> > +
> > +   if (dev->of_node) {
> > +   ret = of_dma_configure(dev, dev->of_node);
> > +   } else if (has_acpi_companion(dev)) {
> > +   attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
> > +   if (attr != DEV_DMA_NOT_SUPPORTED)
> > +   ret = acpi_dma_configure(dev, attr);
> > +   }
> > +
> > +   return ret;
> 
> This code sniplet is duplicated so many times that I think we should
> just have some sort of dma_common_configure() for it that the various
> busses can use.

Agree. There is no good point in duplicating the code.
So this new API will be part of 'drivers/base/dma-mapping.c' file?

> 
> > +void amba_dma_deconfigure(struct device *dev)
> > +{
> > +   of_dma_deconfigure(dev);
> > +   acpi_dma_deconfigure(dev);
> > +}
> 
> As mention in my previous reply I think we don't even need a deconfigure
> callback at this point - just remove the ACPI and OF wrappers and
> clear the dma ops.
> 
> Also in this series we should replace the force_dma flag by use of the
> proper method, e.g. give a force parameter to of_dma_configure and the
> new dma_common_configure helper that the busses that want it can set.

I am more inclined to what Robin states in other mail to keep symmetry.
i.e. to keep dma_configure() and dma_deconfigure() and call
dev->bus->dma_configure from dma_configure(). Is this okay?

Thanks,
Nipun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Vivek Gautam 
Hi Robin,


On Tue, Mar 13, 2018 at 6:19 PM, Robin Murphy  wrote:
> On 13/03/18 09:55, Vivek Gautam wrote:
>>
>> On Tue, Mar 13, 2018 at 3:10 PM, Rafael J. Wysocki 
>> wrote:
>>>
>>> On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote:

 The lists managing the device-links can be traversed to
 find the link between two devices. The device_link_add() APIs
 does traverse these lists to check if there's already a link
 setup between the two devices.
 So, add a new APIs, device_link_find(), to find an existing
 device link between two devices - suppliers and consumers.

 Signed-off-by: Vivek Gautam 
 Cc: Rafael J. Wysocki 
 Cc: Greg Kroah-Hartman 
 ---

   * New patch added to this series.

   drivers/base/core.c| 30 +++---
   include/linux/device.h |  2 ++
   2 files changed, 29 insertions(+), 3 deletions(-)

 diff --git a/drivers/base/core.c b/drivers/base/core.c
 index 5847364f25d9..e8c9774e4ba2 100644
 --- a/drivers/base/core.c
 +++ b/drivers/base/core.c
 @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device
 *dev, void *not_used)
return 0;
   }

 +/**
 + * device_link_find - find any existing link between two devices.
 + * @consumer: Consumer end of the link.
 + * @supplier: Supplier end of the link.
 + *
 + * Returns pointer to the existing link between a supplier and
 + * and consumer devices, or NULL if no link exists.
 + */
 +struct device_link *device_link_find(struct device *consumer,
 +  struct device *supplier)
 +{
 + struct device_link *link = NULL;
 +
 + if (!consumer || !supplier)
 + return NULL;
 +
 + list_for_each_entry(link, >links.consumers, s_node)
 + if (link->consumer == consumer)
 + break;
 +
>>>
>>>
>>> Any mutual exclusion?
>>>
>>> Or is the caller expected to take care of it?  And if so, then how?
>>
>>
>> I think it's better that we take care of lock here in the code rather
>> than depending
>> on the caller.
>> But i can't take device_links_write_lock() since device_link_add()
>> already takes that.
>
>
> Well, the normal pattern is to break out the internal helper function as-is,
> then add a public wrapper which validates inputs, handles locking, etc.,
> equivalently to existing caller(s). See what device_link_del() and others
> do, e.g.:
>
> static struct device_link *__device_link_find(struct device *consumer,
> struct device *supplier)
> {
> list_for_each_entry(link, >links.consumers, s_node)
> if (link->consumer == consumer)
> return link;
> return NULL;
> }
>
> struct device_link *device_link_find(struct device *consumer,
> struct device *supplier)
> {
> struct device_link *link;
>
> if (!consumer || !supplier)
> return NULL;
>
> device_links_write_lock();
> link = __device_link_find(consumer, supplier);
> device_links_write_unlock();
> return link;
> }
>
> where device_link_add() would call __device_link_find() directly.

Right, I understand it now. Thanks for detailed explanation.

regards
Vivek

>
> However, as Tomasz points out (and I hadn't really considered), if the only
> reasonable thing to with a link once you've found it is to delete it, then
> in terms of the public API it may well make more sense to just implement
> something like a device_link_remove() which does both in one go.
>
> Robin.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-13 Thread Christoph Hellwig 
On Tue, Mar 13, 2018 at 12:11:49PM +, Robin Murphy wrote:
> Taking a step back, though, provided the original rationale about 
> dma_declare_coherent_memory() is still valid, I wonder if we should simply 
> permit the USB code to call dma_{alloc,free}_from_dev_coherent() directly 
> here instead of being "good" and indirecting through the top-level DMA API 
> (which is the part which leads to problems). Given that it's a specific DMA 
> bounce buffer implementation within a core API, not just any old driver 
> code, I personally would consider that reasonable.

Looking back I don't really understand why we even indirect the "classic"
per-device dma_declare_coherent_memory use case through the DMA API.

It seems like a pretty different use case to me.  In the USB case we
also have the following additional twist in that it doesn't even need
the mapping to be coherent.

So maybe for now the quick fix is to move the sleep check as suggested
earlier in this thread, but in the long run we probably need to do some
major rework of how dma_declare_coherent_memory and friends work.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code

2018-03-13 Thread Christoph Hellwig 
On Mon, Mar 12, 2018 at 02:48:51PM -0500, Tom Lendacky wrote:
> Ok, I found one issue that allows this to work when the IOMMU isn't
> enabled (see below).

Thanks, folded!

> But the bigger issue is when the IOMMU is enabled.  The IOMMU code uses
> a common mapping routine to create the I/O page tables.  This routine
> assumes that all memory being mapped is encrypted and therefore sets the
> encryption bit in the I/O page tables.  With this patch, the call to
> dma_alloc_direct() now returns un-encrypted memory which results in an
> encryption mis-match.  I think keeping dma_alloc_direct() as it was prior
> to this patch is the way to go.  It allows SME DMA allocations to remain
> encrypted and avoids added complexity in the amd_iommu.c file.  This
> would mean that SEV would still have special DMA operations (so that the
> alloc/free can change the memory to un-encrypted).
> 
> What do you think?

In terms of logic you are right.  I still don't like keeping a just
slightly tweaked version of dma_alloc_direct around just for this, it
will be perpetually out of sync in terms of features and bug fixes.

What do you think about this version that does the decision at runtime:


http://git.infradead.org/users/hch/misc.git/commitdiff/b89f24dc856595dc7610d672bf077195ab0dabf4

The full tree is available here for testing:

git://git.infradead.org/users/hch/misc.git dma-direct-x86
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Robin Murphy 

On 13/03/18 09:55, Vivek Gautam wrote:

On Tue, Mar 13, 2018 at 3:10 PM, Rafael J. Wysocki  wrote:

On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote:

The lists managing the device-links can be traversed to
find the link between two devices. The device_link_add() APIs
does traverse these lists to check if there's already a link
setup between the two devices.
So, add a new APIs, device_link_find(), to find an existing
device link between two devices - suppliers and consumers.

Signed-off-by: Vivek Gautam 
Cc: Rafael J. Wysocki 
Cc: Greg Kroah-Hartman 
---

  * New patch added to this series.

  drivers/base/core.c| 30 +++---
  include/linux/device.h |  2 ++
  2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5847364f25d9..e8c9774e4ba2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void 
*not_used)
   return 0;
  }

+/**
+ * device_link_find - find any existing link between two devices.
+ * @consumer: Consumer end of the link.
+ * @supplier: Supplier end of the link.
+ *
+ * Returns pointer to the existing link between a supplier and
+ * and consumer devices, or NULL if no link exists.
+ */
+struct device_link *device_link_find(struct device *consumer,
+  struct device *supplier)
+{
+ struct device_link *link = NULL;
+
+ if (!consumer || !supplier)
+ return NULL;
+
+ list_for_each_entry(link, >links.consumers, s_node)
+ if (link->consumer == consumer)
+ break;
+


Any mutual exclusion?

Or is the caller expected to take care of it?  And if so, then how?


I think it's better that we take care of lock here in the code rather
than depending
on the caller.
But i can't take device_links_write_lock() since device_link_add()
already takes that.


Well, the normal pattern is to break out the internal helper function 
as-is, then add a public wrapper which validates inputs, handles 
locking, etc., equivalently to existing caller(s). See what 
device_link_del() and others do, e.g.:


static struct device_link *__device_link_find(struct device *consumer,
struct device *supplier)
{
list_for_each_entry(link, >links.consumers, s_node)
if (link->consumer == consumer)
return link;
return NULL;
}

struct device_link *device_link_find(struct device *consumer,
struct device *supplier)
{
struct device_link *link;

if (!consumer || !supplier)
return NULL;

device_links_write_lock();
link = __device_link_find(consumer, supplier);  
device_links_write_unlock();
return link;
}

where device_link_add() would call __device_link_find() directly.

However, as Tomasz points out (and I hadn't really considered), if the 
only reasonable thing to with a link once you've found it is to delete 
it, then in terms of the public API it may well make more sense to just 
implement something like a device_link_remove() which does both in one go.


Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-13 Thread Robin Murphy 

On 11/03/18 18:01, Fredrik Noring wrote:

Hi Christoph,


The point is that you should always use a pool, period.
dma_alloc*/dma_free* are fundamentally expensive operations on my
architectures, so if you call them from a fast path you are doing
something wrong.


The author's comment in commit b3476675320e "usb: dma bounce buffer support"
seems to suggest that the greatest concern is space efficiency as opposed to
speed. I tried to evaluate strict pool allocations, similar to the patch
below, but that didn't turn out as I expected.

I chose a 64 KiB pool maximum since it has been the largest requested size I
have observed in USB traces (which may not hold in general, of course). This
change caused the USB mass storage driver to get stuck in some kind of memory
deadlock, with endless busy-looping on 64 KiB allocation failures.

I also tried a progression of pool sizes including nonpowers of two, for
example 12288, to make better use of the 256 KiB memory capacity. However,
the following part of dma_pool_create in linux/mm/dmapool.c is somewhat
puzzling:

 if (!boundary)
 boundary = allocation;
 else if ((boundary < size) || (boundary & (boundary - 1)))
 return NULL;

Is the boundary variable required to be a power of two only when it is
explicitly given as nonzero?


I would think so; realistically, the notion of non-power-of-two 
boundaries doesn't make a great deal of sense. IIRC, XHCI has a 64KB 
boundary limitation, but [OE]HCI don't (as far as I could see from the 
specs).




Fredrik

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 77eef8acff94..8cc8fbc91c76 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -26,7 +26,7 @@

  /* FIXME tune these based on pool statistics ... */
  static size_t pool_max[HCD_BUFFER_POOLS] = {
-   32, 128, 512, 2048,
+   32, 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768, 65536
  };

  void __init usb_init_pool_max(void)
@@ -76,7 +76,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
 continue;
 snprintf(name, sizeof(name), "buffer-%d", size);
 hcd->pool[i] = dma_pool_create(name, hcd->self.sysdev,
-   size, size, 0);
+   size, min_t(size_t, 4096, size), 0);
 if (!hcd->pool[i]) {
 hcd_buffer_destroy(hcd);
 return -ENOMEM;
@@ -140,7 +140,7 @@ void *hcd_buffer_alloc(
 if (size <= pool_max[i])
 return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
 }
-   return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);


Taking a step back, though, provided the original rationale about 
dma_declare_coherent_memory() is still valid, I wonder if we should 
simply permit the USB code to call dma_{alloc,free}_from_dev_coherent() 
directly here instead of being "good" and indirecting through the 
top-level DMA API (which is the part which leads to problems). Given 
that it's a specific DMA bounce buffer implementation within a core API, 
not just any old driver code, I personally would consider that reasonable.


Robin.


+   return NULL;
  }

  void hcd_buffer_free(
@@ -169,5 +169,5 @@ void hcd_buffer_free(
 return;
 }
 }
-   dma_free_coherent(hcd->self.sysdev, size, addr, dma);
+   BUG();  /* The buffer could not have been allocated. */
  }
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 176900528822..2075f1e22e32 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -189,7 +189,7 @@ struct usb_hcd {
 struct usb_hcd  *primary_hcd;


-#define HCD_BUFFER_POOLS   4
+#define HCD_BUFFER_POOLS   11
 struct dma_pool *pool[HCD_BUFFER_POOLS];

 int state;


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: move dma configuration to bus infrastructure

2018-03-13 Thread Robin Murphy 

On 12/03/18 15:24, Nipun Gupta wrote:

The change introduces 'dma_configure' & 'dma_deconfigure'as
bus callback functions so each bus can choose to implement
its own dma configuration function.
This eases the addition of new busses w.r.t. adding the dma
configuration functionality.


It's probably worth clarifying - either in the commit message, the 
kerneldoc, or both - that the bus-specific aspect is that of mapping 
between a given device on the bus and the relevant firmware description 
of its DMA configuration.



The change also updates the PCI, Platform and ACPI bus to use
new introduced callbacks.

Signed-off-by: Nipun Gupta 
---
  - This patch is based on the comments on:
https://patchwork.kernel.org/patch/10259087/
  - I have validated for PCI and platform, but not for AMBA as I
do not have infrastructure to validate it.
Can anyone please validate them on AMBA?

  drivers/amba/bus.c  | 38 -
  drivers/base/dd.c   | 14 +++
  drivers/base/dma-mapping.c  | 41 ---
  drivers/base/platform.c | 36 ++-
  drivers/pci/pci-driver.c| 59 -
  include/linux/device.h  |  6 +
  include/linux/dma-mapping.h | 12 -
  7 files changed, 124 insertions(+), 82 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 594c228..58241d2 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -20,6 +20,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  
  #include 
  
@@ -171,6 +173,28 @@ static int amba_pm_runtime_resume(struct device *dev)

  }
  #endif /* CONFIG_PM */
  
+int amba_dma_configure(struct device *dev)

+{
+   enum dev_dma_attr attr;
+   int ret = 0;
+
+   if (dev->of_node) {
+   ret = of_dma_configure(dev, dev->of_node);
+   } else if (has_acpi_companion(dev)) {
+   attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
+   if (attr != DEV_DMA_NOT_SUPPORTED)
+   ret = acpi_dma_configure(dev, attr);
+   }
+
+   return ret;
+}


I would be inclined to have amba_bustype just reference 
platform_dma_configure() directly rather than duplicate it like this, 
since there's no sensible reason for them to ever differ.



+
+void amba_dma_deconfigure(struct device *dev)
+{
+   of_dma_deconfigure(dev);
+   acpi_dma_deconfigure(dev);
+}
+
  static const struct dev_pm_ops amba_pm = {
.suspend= pm_generic_suspend,
.resume = pm_generic_resume,
@@ -190,12 +214,14 @@ static int amba_pm_runtime_resume(struct device *dev)
   * so we call the bus "amba".
   */
  struct bus_type amba_bustype = {
-   .name   = "amba",
-   .dev_groups = amba_dev_groups,
-   .match  = amba_match,
-   .uevent = amba_uevent,
-   .pm = _pm,
-   .force_dma  = true,
+   .name   = "amba",
+   .dev_groups = amba_dev_groups,
+   .match  = amba_match,
+   .uevent = amba_uevent,
+   .pm = _pm,
+   .dma_configure  = amba_dma_configure,
+   .dma_deconfigure= amba_dma_deconfigure,
+   .force_dma  = true,


This patch should also be removing force_dma because it no longer makes 
sense. If DMA configuration is now done by a bus-level callback, then a 
bus which wants its children to get DMA configuration needs to implement 
that callback; there's nowhere to force a "default" global behaviour any 
more.



  };
  
  static int __init amba_init(void)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index de6fd09..f124f3f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -421,9 +421,11 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
if (ret)
goto pinctrl_bind_failed;
  
-	ret = dma_configure(dev);

-   if (ret)
-   goto dma_failed;
+   if (dev->bus->dma_configure) {
+   ret = dev->bus->dma_configure(dev);
+   if (ret)
+   goto dma_failed;
+   }
  
  	if (driver_sysfs_add(dev)) {

printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
@@ -486,7 +488,8 @@ static int really_probe(struct device *dev, struct 
device_driver *drv)
goto done;
  
  probe_failed:

-   dma_deconfigure(dev);
+   if (dev->bus->dma_deconfigure)
+   dev->bus->dma_deconfigure(dev);
  dma_failed:
if (dev->bus)
blocking_notifier_call_chain(>bus->p->bus_notifier,
@@ -895,7 +898,8 @@ static void __device_release_driver(struct device *dev, 
struct device *parent)
drv->remove(dev);
  
  		device_links_driver_cleanup(dev);

-   dma_deconfigure(dev);
+   if (dev->bus->dma_deconfigure)
+

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Tomasz Figa 
On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam
 wrote:
> Hi Tomasz,
>
> On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa  wrote:
>> Hi Vivek,
>>
>> Thanks for the patch.
>>
>> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam
>>  wrote:
>>> The lists managing the device-links can be traversed to
>>> find the link between two devices. The device_link_add() APIs
>>> does traverse these lists to check if there's already a link
>>> setup between the two devices.
>>> So, add a new APIs, device_link_find(), to find an existing
>>> device link between two devices - suppliers and consumers.
>>
>> I'm wondering if this API would be useful for anything else that the
>> problem we're trying to solve with deleting links without storing them
>> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a
>> better alternative?
>
> Yea, that sounds simpler i think. Will add this API instead of
> find_link(). Thanks.

Perhaps let's wait for a moment to see if there are other opinions. :)

Rafael, Lucas, any thoughts?

Best regards,
Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Vivek Gautam 
Hi Tomasz,

On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa  wrote:
> Hi Vivek,
>
> Thanks for the patch.
>
> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam
>  wrote:
>> The lists managing the device-links can be traversed to
>> find the link between two devices. The device_link_add() APIs
>> does traverse these lists to check if there's already a link
>> setup between the two devices.
>> So, add a new APIs, device_link_find(), to find an existing
>> device link between two devices - suppliers and consumers.
>
> I'm wondering if this API would be useful for anything else that the
> problem we're trying to solve with deleting links without storing them
> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a
> better alternative?

Yea, that sounds simpler i think. Will add this API instead of
find_link(). Thanks.

regards
vivek
>
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Tomasz Figa 
Hi Vivek,

Thanks for the patch.

On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam
 wrote:
> The lists managing the device-links can be traversed to
> find the link between two devices. The device_link_add() APIs
> does traverse these lists to check if there's already a link
> setup between the two devices.
> So, add a new APIs, device_link_find(), to find an existing
> device link between two devices - suppliers and consumers.

I'm wondering if this API would be useful for anything else that the
problem we're trying to solve with deleting links without storing them
anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a
better alternative?

Best regards,
Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Vivek Gautam 
On Tue, Mar 13, 2018 at 2:25 PM, Vivek Gautam
 wrote:
> The lists managing the device-links can be traversed to
> find the link between two devices. The device_link_add() APIs
> does traverse these lists to check if there's already a link
> setup between the two devices.
> So, add a new APIs, device_link_find(), to find an existing
> device link between two devices - suppliers and consumers.
>
> Signed-off-by: Vivek Gautam 
> Cc: Rafael J. Wysocki 
> Cc: Greg Kroah-Hartman 
> ---
>
>  * New patch added to this series.
>
>  drivers/base/core.c| 30 +++---
>  include/linux/device.h |  2 ++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5847364f25d9..e8c9774e4ba2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, 
> void *not_used)
> return 0;
>  }
>
> +/**
> + * device_link_find - find any existing link between two devices.
> + * @consumer: Consumer end of the link.
> + * @supplier: Supplier end of the link.
> + *
> + * Returns pointer to the existing link between a supplier and
> + * and consumer devices, or NULL if no link exists.
> + */
> +struct device_link *device_link_find(struct device *consumer,
> +struct device *supplier)
> +{
> +   struct device_link *link = NULL;
> +
> +   if (!consumer || !supplier)
> +   return NULL;
> +
> +   list_for_each_entry(link, >links.consumers, s_node)
> +   if (link->consumer == consumer)
> +   break;
> +
> +   return link;

My bad, this too needs fixing (didn't add the changes to the patch :( )

   list_for_each_entry(link, >links.consumers, s_node)
   if (link->consumer == consumer)
   return link;

   return NULL;

> +}
> +EXPORT_SYMBOL_GPL(device_link_find);
> +
>  /**
>   * device_link_add - Create a link between two devices.
>   * @consumer: Consumer end of the link.
> @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device 
> *consumer,
> goto out;
> }
>
> -   list_for_each_entry(link, >links.consumers, s_node)
> -   if (link->consumer == consumer)
> -   goto out;
> +   link = device_link_find(consumer, supplier);
> +   if (link)
> +   goto out;
>
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (!link)
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b093405ed525..13bc1884c3eb 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct 
> device *dev);
>  struct device_link *device_link_add(struct device *consumer,
> struct device *supplier, u32 flags);
>  void device_link_del(struct device_link *link);
> +struct device_link *device_link_find(struct device *consumer,
> +struct device *supplier);
>
>  #ifdef CONFIG_PRINTK
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Vivek Gautam 
On Tue, Mar 13, 2018 at 3:10 PM, Rafael J. Wysocki  wrote:
> On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote:
>> The lists managing the device-links can be traversed to
>> find the link between two devices. The device_link_add() APIs
>> does traverse these lists to check if there's already a link
>> setup between the two devices.
>> So, add a new APIs, device_link_find(), to find an existing
>> device link between two devices - suppliers and consumers.
>>
>> Signed-off-by: Vivek Gautam 
>> Cc: Rafael J. Wysocki 
>> Cc: Greg Kroah-Hartman 
>> ---
>>
>>  * New patch added to this series.
>>
>>  drivers/base/core.c| 30 +++---
>>  include/linux/device.h |  2 ++
>>  2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 5847364f25d9..e8c9774e4ba2 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, 
>> void *not_used)
>>   return 0;
>>  }
>>
>> +/**
>> + * device_link_find - find any existing link between two devices.
>> + * @consumer: Consumer end of the link.
>> + * @supplier: Supplier end of the link.
>> + *
>> + * Returns pointer to the existing link between a supplier and
>> + * and consumer devices, or NULL if no link exists.
>> + */
>> +struct device_link *device_link_find(struct device *consumer,
>> +  struct device *supplier)
>> +{
>> + struct device_link *link = NULL;
>> +
>> + if (!consumer || !supplier)
>> + return NULL;
>> +
>> + list_for_each_entry(link, >links.consumers, s_node)
>> + if (link->consumer == consumer)
>> + break;
>> +
>
> Any mutual exclusion?
>
> Or is the caller expected to take care of it?  And if so, then how?

I think it's better that we take care of lock here in the code rather
than depending
on the caller.
But i can't take device_links_write_lock() since device_link_add()
already takes that.

regards
Vivek

>
>> + return link;
>> +}
>> +EXPORT_SYMBOL_GPL(device_link_find);
>> +
>>  /**
>>   * device_link_add - Create a link between two devices.
>>   * @consumer: Consumer end of the link.
>> @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device 
>> *consumer,
>>   goto out;
>>   }
>>
>> - list_for_each_entry(link, >links.consumers, s_node)
>> - if (link->consumer == consumer)
>> - goto out;
>> + link = device_link_find(consumer, supplier);
>> + if (link)
>> + goto out;
>>
>>   link = kzalloc(sizeof(*link), GFP_KERNEL);
>>   if (!link)
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index b093405ed525..13bc1884c3eb 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct 
>> device *dev);
>>  struct device_link *device_link_add(struct device *consumer,
>>   struct device *supplier, u32 flags);
>>  void device_link_del(struct device_link *link);
>> +struct device_link *device_link_find(struct device *consumer,
>> +  struct device *supplier);
>>
>>  #ifdef CONFIG_PRINTK
>>
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Rafael J. Wysocki 
On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote:
> The lists managing the device-links can be traversed to
> find the link between two devices. The device_link_add() APIs
> does traverse these lists to check if there's already a link
> setup between the two devices.
> So, add a new APIs, device_link_find(), to find an existing
> device link between two devices - suppliers and consumers.
> 
> Signed-off-by: Vivek Gautam 
> Cc: Rafael J. Wysocki 
> Cc: Greg Kroah-Hartman 
> ---
> 
>  * New patch added to this series.
> 
>  drivers/base/core.c| 30 +++---
>  include/linux/device.h |  2 ++
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5847364f25d9..e8c9774e4ba2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, 
> void *not_used)
>   return 0;
>  }
>  
> +/**
> + * device_link_find - find any existing link between two devices.
> + * @consumer: Consumer end of the link.
> + * @supplier: Supplier end of the link.
> + *
> + * Returns pointer to the existing link between a supplier and
> + * and consumer devices, or NULL if no link exists.
> + */
> +struct device_link *device_link_find(struct device *consumer,
> +  struct device *supplier)
> +{
> + struct device_link *link = NULL;
> +
> + if (!consumer || !supplier)
> + return NULL;
> +
> + list_for_each_entry(link, >links.consumers, s_node)
> + if (link->consumer == consumer)
> + break;
> +

Any mutual exclusion?

Or is the caller expected to take care of it?  And if so, then how?

> + return link;
> +}
> +EXPORT_SYMBOL_GPL(device_link_find);
> +
>  /**
>   * device_link_add - Create a link between two devices.
>   * @consumer: Consumer end of the link.
> @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device 
> *consumer,
>   goto out;
>   }
>  
> - list_for_each_entry(link, >links.consumers, s_node)
> - if (link->consumer == consumer)
> - goto out;
> + link = device_link_find(consumer, supplier);
> + if (link)
> + goto out;
>  
>   link = kzalloc(sizeof(*link), GFP_KERNEL);
>   if (!link)
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b093405ed525..13bc1884c3eb 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct 
> device *dev);
>  struct device_link *device_link_add(struct device *consumer,
>   struct device *supplier, u32 flags);
>  void device_link_del(struct device_link *link);
> +struct device_link *device_link_find(struct device *consumer,
> +  struct device *supplier);
>  
>  #ifdef CONFIG_PRINTK
>  
> 


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v9 5/5] iommu/arm-smmu: Add support for qcom,smmu-v2 variant

2018-03-13 Thread Vivek Gautam 
qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
clock and power requirements. This smmu core is used with
multiple masters on msm8996, viz. mdss, video, etc.
Add bindings for the same.

Signed-off-by: Vivek Gautam 
Reviewed-by: Rob Herring 
Reviewed-by: Tomasz Figa 
---
 .../devicetree/bindings/iommu/arm,smmu.txt | 42 ++
 drivers/iommu/arm-smmu.c   | 14 
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 8a6ffce12af5..7c71a6ed465a 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -17,10 +17,19 @@ conditions.
 "arm,mmu-401"
 "arm,mmu-500"
 "cavium,smmu-v2"
+"qcom,-smmu-v2", "qcom,smmu-v2"
 
   depending on the particular implementation and/or the
   version of the architecture implemented.
 
+  A number of Qcom SoCs use qcom,smmu-v2 version of the IP.
+  "qcom,-smmu-v2" represents a soc specific compatible
+  string that should be present along with the "qcom,smmu-v2"
+  to facilitate SoC specific clocks/power connections and to
+  address specific bug fixes.
+  An example string would be -
+  "qcom,msm8996-smmu-v2", "qcom,smmu-v2".
+
 - reg   : Base address and size of the SMMU.
 
 - #global-interrupts : The number of global interrupts exposed by the
@@ -71,6 +80,22 @@ conditions.
   or using stream matching with #iommu-cells = <2>, and
   may be ignored if present in such cases.
 
+- clock-names:List of the names of clocks input to the device. The
+  required list depends on particular implementation and
+  is as follows:
+  - for "qcom,smmu-v2":
+- "bus": clock required for downstream bus access and
+ for the smmu ptw,
+- "iface": clock required to access smmu's registers
+   through the TCU's programming interface.
+  - unspecified for other implementations.
+
+- clocks: Specifiers for all clocks listed in the clock-names property,
+  as per generic clock bindings.
+
+- power-domains:  Specifiers for power domains required to be powered on for
+  the SMMU to operate, as per generic power domain bindings.
+
 ** Deprecated properties:
 
 - mmu-masters (deprecated in favour of the generic "iommus" binding) :
@@ -137,3 +162,20 @@ conditions.
 iommu-map = <0  0 0x400>;
 ...
 };
+
+   /* Qcom's arm,smmu-v2 implementation */
+   smmu4: iommu {
+   compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
+   reg = <0xd0 0x1>;
+
+   #global-interrupts = <1>;
+   interrupts = ,
+,
+;
+   #iommu-cells = <1>;
+   power-domains = < MDSS_GDSC>;
+
+   clocks = < SMMU_MDP_AXI_CLK>,
+< SMMU_MDP_AHB_CLK>;
+   clock-names = "bus", "iface";
+   };
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 64953ff2281f..1ef6ac56a347 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -119,6 +119,7 @@ enum arm_smmu_implementation {
GENERIC_SMMU,
ARM_MMU500,
CAVIUM_SMMUV2,
+   QCOM_SMMUV2,
 };
 
 struct arm_smmu_s2cr {
@@ -2000,6 +2001,17 @@ ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, 
GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
 ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
 
+static const char * const qcom_smmuv2_clks[] = {
+   "bus", "iface",
+};
+
+static const struct arm_smmu_match_data qcom_smmuv2 = {
+   .version = ARM_SMMU_V2,
+   .model = QCOM_SMMUV2,
+   .clks = qcom_smmuv2_clks,
+   .num_clks = ARRAY_SIZE(qcom_smmuv2_clks),
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v1", .data = _generic_v1 },
{ .compatible = "arm,smmu-v2", .data = _generic_v2 },
@@ -2007,6 +2019,7 @@ static const struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,mmu-401", .data = _mmu401 },
{ .compatible = "arm,mmu-500", .data = _mmu500 },
{ .compatible = "cavium,smmu-v2", .data = _smmuv2 },
+   { .compatible = "qcom,smmu-v2", .data = _smmuv2 },
{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
@@ -2381,6 +2394,7 @@ IOMMU_OF_DECLARE(arm_mmu400, "arm,mmu-400");

[PATCH v9 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

2018-03-13 Thread Vivek Gautam 
From: Sricharan R 

Finally add the device link between the master device and
smmu, so that the smmu gets runtime enabled/disabled only when the
master needs it. This is done from add_device callback which gets
called once when the master is added to the smmu.

Signed-off-by: Sricharan R 
Signed-off-by: Vivek Gautam 
Reviewed-by: Tomasz Figa 
---
 drivers/iommu/arm-smmu.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 56a04ae80bf3..64953ff2281f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
 
iommu_device_link(>iommu, dev);
 
+   if (pm_runtime_enabled(smmu->dev)) {
+   struct device_link *link;
+
+   /*
+* Establish the link between smmu and master, so that the
+* smmu gets runtime enabled/disabled as per the master's
+* needs.
+*/
+   link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
+   if (!link) {
+   dev_warn(smmu->dev,
+"Unable to add link to the consumer %s\n",
+dev_name(dev));
+   ret = -ENODEV;
+   goto out_unlink;
+   }
+   }
+
arm_smmu_rpm_put(smmu);
 
return 0;
 
+out_unlink:
+   iommu_device_unlink(>iommu, dev);
+   arm_smmu_master_free_smes(fwspec);
 out_rpm_put:
arm_smmu_rpm_put(smmu);
 out_cfg_free:
@@ -1486,6 +1507,14 @@ static void arm_smmu_remove_device(struct device *dev)
cfg  = fwspec->iommu_priv;
smmu = cfg->smmu;
 
+   if (pm_runtime_enabled(smmu->dev)) {
+   struct device_link *link;
+
+   link = device_link_find(dev, smmu->dev);
+   if (link)
+   device_link_del(link);
+   }
+
ret = arm_smmu_rpm_get(smmu);
if (ret < 0)
return;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v9 2/5] iommu/arm-smmu: Add pm_runtime/sleep ops

2018-03-13 Thread Vivek Gautam 
From: Sricharan R 

The smmu needs to be functional only when the respective
master's using it are active. The device_link feature
helps to track such functional dependencies, so that the
iommu gets powered when the master device enables itself
using pm_runtime. So by adapting the smmu driver for
runtime pm, above said dependency can be addressed.

This patch adds the pm runtime/sleep callbacks to the
driver and also the functions to parse the smmu clocks
from DT and enable them in resume/suspend.

Signed-off-by: Sricharan R 
Signed-off-by: Archit Taneja 
[vivek: Clock rework to request bulk of clocks]
Signed-off-by: Vivek Gautam 
Reviewed-by: Tomasz Figa 
---
 drivers/iommu/arm-smmu.c | 60 ++--
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 69e7c60792a8..d5873d545024 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,6 +48,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -205,6 +206,8 @@ struct arm_smmu_device {
u32 num_global_irqs;
u32 num_context_irqs;
unsigned int*irqs;
+   struct clk_bulk_data*clks;
+   int num_clks;
 
u32 cavium_id_base; /* Specific to Cavium */
 
@@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct 
arm_smmu_device *smmu)
 struct arm_smmu_match_data {
enum arm_smmu_arch_version version;
enum arm_smmu_implementation model;
+   const char * const *clks;
+   int num_clks;
 };
 
 #define ARM_SMMU_MATCH_DATA(name, ver, imp)\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
 
 ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
@@ -1919,6 +1924,23 @@ static const struct of_device_id arm_smmu_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
+static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
+  const char * const *clks)
+{
+   int i;
+
+   if (smmu->num_clks < 1)
+   return;
+
+   smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
+ sizeof(*smmu->clks), GFP_KERNEL);
+   if (!smmu->clks)
+   return;
+
+   for (i = 0; i < smmu->num_clks; i++)
+   smmu->clks[i].id = clks[i];
+}
+
 #ifdef CONFIG_ACPI
 static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
 {
@@ -2001,6 +2023,9 @@ static int arm_smmu_device_dt_probe(struct 
platform_device *pdev,
data = of_device_get_match_data(dev);
smmu->version = data->version;
smmu->model = data->model;
+   smmu->num_clks = data->num_clks;
+
+   arm_smmu_fill_clk_data(smmu, data->clks);
 
parse_driver_options(smmu);
 
@@ -2099,6 +2124,14 @@ static int arm_smmu_device_probe(struct platform_device 
*pdev)
smmu->irqs[i] = irq;
}
 
+   err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
+   if (err)
+   return err;
+
+   err = clk_bulk_prepare(smmu->num_clks, smmu->clks);
+   if (err)
+   return err;
+
err = arm_smmu_device_cfg_probe(smmu);
if (err)
return err;
@@ -2181,6 +2214,9 @@ static int arm_smmu_device_remove(struct platform_device 
*pdev)
 
/* Turn the thing off */
writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+
+   clk_bulk_unprepare(smmu->num_clks, smmu->clks);
+
return 0;
 }
 
@@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct 
device *dev)
return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
+static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
+{
+   struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+   return clk_bulk_enable(smmu->num_clks, smmu->clks);
+}
+
+static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
+{
+   struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+   clk_bulk_disable(smmu->num_clks, smmu->clks);
+
+   return 0;
+}
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+   SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
+   SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
+  arm_smmu_runtime_resume, NULL)
+};
 
 static struct platform_driver arm_smmu_driver = {
.driver = {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v9 3/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

2018-03-13 Thread Vivek Gautam 
From: Sricharan R 

The smmu device probe/remove and add/remove master device callbacks
gets called when the smmu is not linked to its master, that is without
the context of the master device. So calling runtime apis in those places
separately.

Signed-off-by: Sricharan R 
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam 
Reviewed-by: Tomasz Figa 
---
 drivers/iommu/arm-smmu.c | 95 
 1 file changed, 87 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d5873d545024..56a04ae80bf3 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
{ 0, NULL},
 };
 
+static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
+{
+   if (pm_runtime_enabled(smmu->dev))
+   return pm_runtime_get_sync(smmu->dev);
+
+   return 0;
+}
+
+static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
+{
+   if (pm_runtime_enabled(smmu->dev))
+   pm_runtime_put(smmu->dev);
+}
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
return container_of(dom, struct arm_smmu_domain, domain);
@@ -913,11 +927,15 @@ static void arm_smmu_destroy_domain_context(struct 
iommu_domain *domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
struct arm_smmu_cfg *cfg = _domain->cfg;
-   int irq;
+   int ret, irq;
 
if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
return;
 
+   ret = arm_smmu_rpm_get(smmu);
+   if (ret < 0)
+   return;
+
/*
 * Disable the context bank and free the page tables before freeing
 * it.
@@ -932,6 +950,8 @@ static void arm_smmu_destroy_domain_context(struct 
iommu_domain *domain)
 
free_io_pgtable_ops(smmu_domain->pgtbl_ops);
__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
+
+   arm_smmu_rpm_put(smmu);
 }
 
 static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
@@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
return -ENODEV;
 
smmu = fwspec_smmu(fwspec);
+
+   ret = arm_smmu_rpm_get(smmu);
+   if (ret < 0)
+   return ret;
+
/* Ensure that the domain is finalised */
ret = arm_smmu_init_domain_context(domain, smmu);
if (ret < 0)
-   return ret;
+   goto rpm_put;
 
/*
 * Sanity check the domain. We don't support domains across
@@ -1230,29 +1255,47 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
}
 
/* Looks ok, so add the device to the domain */
-   return arm_smmu_domain_add_master(smmu_domain, fwspec);
+   ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
+
+rpm_put:
+   arm_smmu_rpm_put(smmu);
+   return ret;
 }
 
 static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot)
 {
struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+   struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+   struct arm_smmu_device *smmu = smmu_domain->smmu;
+   int ret;
 
if (!ops)
return -ENODEV;
 
-   return ops->map(ops, iova, paddr, size, prot);
+   arm_smmu_rpm_get(smmu);
+   ret = ops->map(ops, iova, paddr, size, prot);
+   arm_smmu_rpm_put(smmu);
+
+   return ret;
 }
 
 static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 size_t size)
 {
struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+   struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+   struct arm_smmu_device *smmu = smmu_domain->smmu;
+   size_t ret;
 
if (!ops)
return 0;
 
-   return ops->unmap(ops, iova, size);
+   arm_smmu_rpm_get(smmu);
+   ret = ops->unmap(ops, iova, size);
+   arm_smmu_rpm_put(smmu);
+
+   return ret;
 }
 
 static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
@@ -1407,14 +1450,22 @@ static int arm_smmu_add_device(struct device *dev)
while (i--)
cfg->smendx[i] = INVALID_SMENDX;
 
+   ret = arm_smmu_rpm_get(smmu);
+   if (ret < 0)
+   goto out_cfg_free;
+
ret = arm_smmu_master_alloc_smes(dev);
if (ret)
-   goto out_cfg_free;
+   goto out_rpm_put;
 
iommu_device_link(>iommu, dev);
 
+   arm_smmu_rpm_put(smmu);
+
return 0;
 
+out_rpm_put:
+   arm_smmu_rpm_put(smmu);
 out_cfg_free:
kfree(cfg);
 out_free:
@@ -1427,7

[PATCH v9 0/5] iommu/arm-smmu: Add runtime pm/sleep support

2018-03-13 Thread Vivek Gautam 
This series provides the support for turning on the arm-smmu's
clocks/power domains using runtime pm. This is done using the
recently introduced device links patches, which lets the smmu's
runtime to follow the master's runtime pm, so the smmu remains
powered only when the masters use it.
As not all implementations support clock/power gating, we are checking
for a valid 'smmu->dev's pm_domain' to conditionally enable the runtime
power management for such smmu implementations that can support it.

This series also adds support for Qcom's arm-smmu-v2 variant that
has different clocks and power requirements.

Took some reference from the exynos runtime patches [1].

With conditional runtime pm now, we avoid touching dev->power.lock
in fastpaths for smmu implementations that don't need to do anything
useful with pm_runtime.
This lets us to use the much-argued pm_runtime_get_sync/put_sync()
calls in map/unmap callbacks so that the clients do not have to
worry about handling any of the arm-smmu's power.

Previous version of this patch series is @ [5].

[v9]
   * Removed 'rpm_supported' flag, instead checking on pm_domain
 to enable runtime pm.
   * Creating device link only when the runtime pm is enabled, as we
 don't need a device link besides managing the power dependency
 between supplier and consumer devices.
   * Introducing a patch to add device_link_find() API that finds
 and existing link between supplier and consumer devices.
 Also, made necessary change to device_link_add() to use this API.
   * arm_smmu_remove_device() now uses this device_link_find() to find
 the device link between smmu device and the master device, and then
 delete this link.
   * Dropped the destroy_domain_context() fix [8] as it was rather,
 introducing catastrophically bad problem by destroying
 'good dev's domain context.
   * Added 'Reviwed-by' tag for Tomasz's review.

[v8]
   * Major change -
 - Added a flag 'rpm_supported' which each platform that supports
   runtime pm, can enable, and we enable runtime_pm over arm-smmu
   only when this flag is set.
 - Adding the conditional pm_runtime_get/put() calls to .map, .unmap
   and .attach_dev ops.
 - Dropped the patch [6] that exported pm_runtim_get/put_suupliers(),
   and also dropped the user driver patch [7] for these APIs.

   * Clock code further cleanup
 - doing only clk_bulk_enable() and clk_bulk_disable() in runtime pm
   callbacks. We shouldn't be taking a slow path (clk_prepare/unprepare())
   from these runtime pm callbacks. Thereby, moved clk_bulk_prepare() to
   arm_smmu_device_probe(), and clk_bulk_unprepare() to
   arm_smmu_device_remove().
 - clk data filling to a common method arm_smmu_fill_clk_data() that
   fills the clock ids and number of clocks.

   * Addressed other nits and comments
 - device_link_add() error path fixed.
 - Fix for checking negative error value from pm_runtime_get_sync().
 - Documentation redo.

   * Added another patch fixing the error path in arm_smmu_attach_dev()
 to destroy allocated domain context.

[v7]
   * Addressed review comments given by Robin Murphy -
 - Added device_link_del() in .remove_device path.
 - Error path cleanup in arm_smmu_add_device().
 - Added pm_runtime_get/put_sync() in .remove path, and replaced
pm_runtime_force_suspend() with pm_runtime_disable().
 - clk_names cleanup in arm_smmu_init_clks()
   * Added 'Reviewed-by' given by Rob H.

[V6]
   * Added Ack given by Rafael to first patch in the series.
   * Addressed Rob Herring's comment for adding soc specific compatible
 string as well besides 'qcom,smmu-v2'.

[V5]
   * Dropped runtime pm calls from "arm_smmu_unmap" op as discussed over
 the list [3] for the last patch series.
   * Added a patch to export pm_runtime_get/put_suppliers() APIs to the
 series as agreed with Rafael [4].
   * Added the related patch for msm drm iommu layer to use
 pm_runtime_get/put_suppliers() APIs in msm_mmu_funcs.
   * Dropped arm-mmu500 clock patch since that would break existing
 platforms.
   * Changed compatible 'qcom,msm8996-smmu-v2' to 'qcom,smmu-v2' to reflect
 the IP version rather than the platform on which it is used.
 The same IP is used across multiple platforms including msm8996,
 and sdm845 etc.
   * Using clock bulk APIs to handle the clocks available to the IP as
 suggested by Stephen Boyd.
   * The first patch in v4 version of the patch-series:
 ("iommu/arm-smmu: Fix the error path in arm_smmu_add_device") has
 already made it to mainline.

[V4]
   * Reworked the clock handling part. We now take clock names as data
 in the driver for supported compatible versions, and loop over them
 to get, enable, and disable the clocks.
   * Using qcom,msm8996 based compatibles for bindings instead of a generic
 qcom compatible.
   * Refactor MMU500 patch to just add the necessary clock names

[PATCH v9 1/5] driver core: Find an existing link between two devices

2018-03-13 Thread Vivek Gautam 
The lists managing the device-links can be traversed to
find the link between two devices. The device_link_add() APIs
does traverse these lists to check if there's already a link
setup between the two devices.
So, add a new APIs, device_link_find(), to find an existing
device link between two devices - suppliers and consumers.

Signed-off-by: Vivek Gautam 
Cc: Rafael J. Wysocki 
Cc: Greg Kroah-Hartman 
---

 * New patch added to this series.

 drivers/base/core.c| 30 +++---
 include/linux/device.h |  2 ++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5847364f25d9..e8c9774e4ba2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev, void 
*not_used)
return 0;
 }
 
+/**
+ * device_link_find - find any existing link between two devices.
+ * @consumer: Consumer end of the link.
+ * @supplier: Supplier end of the link.
+ *
+ * Returns pointer to the existing link between a supplier and
+ * and consumer devices, or NULL if no link exists.
+ */
+struct device_link *device_link_find(struct device *consumer,
+struct device *supplier)
+{
+   struct device_link *link = NULL;
+
+   if (!consumer || !supplier)
+   return NULL;
+
+   list_for_each_entry(link, >links.consumers, s_node)
+   if (link->consumer == consumer)
+   break;
+
+   return link;
+}
+EXPORT_SYMBOL_GPL(device_link_find);
+
 /**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
@@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device *consumer,
goto out;
}
 
-   list_for_each_entry(link, >links.consumers, s_node)
-   if (link->consumer == consumer)
-   goto out;
+   link = device_link_find(consumer, supplier);
+   if (link)
+   goto out;
 
link = kzalloc(sizeof(*link), GFP_KERNEL);
if (!link)
diff --git a/include/linux/device.h b/include/linux/device.h
index b093405ed525..13bc1884c3eb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct device 
*dev);
 struct device_link *device_link_add(struct device *consumer,
struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
+struct device_link *device_link_find(struct device *consumer,
+struct device *supplier);
 
 #ifdef CONFIG_PRINTK
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: move dma configuration to bus infrastructure

2018-03-13 Thread Christoph Hellwig 
> +int amba_dma_configure(struct device *dev)
> +{
> + enum dev_dma_attr attr;
> + int ret = 0;
> +
> + if (dev->of_node) {
> + ret = of_dma_configure(dev, dev->of_node);
> + } else if (has_acpi_companion(dev)) {
> + attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
> + if (attr != DEV_DMA_NOT_SUPPORTED)
> + ret = acpi_dma_configure(dev, attr);
> + }
> +
> + return ret;

This code sniplet is duplicated so many times that I think we should
just have some sort of dma_common_configure() for it that the various
busses can use.

> +void amba_dma_deconfigure(struct device *dev)
> +{
> + of_dma_deconfigure(dev);
> + acpi_dma_deconfigure(dev);
> +}

As mention in my previous reply I think we don't even need a deconfigure
callback at this point - just remove the ACPI and OF wrappers and
clear the dma ops.

Also in this series we should replace the force_dma flag by use of the
proper method, e.g. give a force parameter to of_dma_configure and the
new dma_common_configure helper that the busses that want it can set.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: move dma configuration to bus infrastructure

2018-03-13 Thread h...@lst.de 
On Tue, Mar 13, 2018 at 04:22:53AM +, Nipun Gupta wrote:
> > Isn't this one or the other one but not both?
> > 
> > Something like:
> > 
> > if (dev->of_node)
> > of_dma_deconfigure(dev);
> > else
> > acpi_dma_deconfigure(dev);
> > 
> > should work.
> 
> I understand your point. Seems reasonable as we should not expect
> the 'of/acpi DMA deconfigure' API to not fail when they are not configured.
> 
> But, here we would also need to get dma_device (just as we get in
> 'pci_dma_configure') and need a check on it as for PCI there 'of_node'
> is present in the dma_dev.

Both of_dma_deconfigure and acpi_dma_deconfigure just end up calling
arch_teardown_dma_ops.  So my preference would be to just remove
of_dma_deconfigure and acpi_dma_deconfigure and call arch_teardown_dma_ops
as a prep patch before this one.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu