* Re: [PATCH V4 2/3] ASoC: fsl_asrc: replace the process_option table with function
From: Nicolin Chen @ 2019-04-19 18:22 UTC (permalink / raw)
To: S.j. Wang
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com,
linux-kernel@vger.kernel.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <0f7a6907c73e110c797b478fedaba2fc47b5e994.1555669068.git.shengjiu.wang@nxp.com>
On Fri, Apr 19, 2019 at 10:23:53AM +0000, S.j. Wang wrote:
> @@ -289,6 +318,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> return -EINVAL;
> }
>
> + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
Since the function always return 0, I am thinking of treating
this function as a lookup function, and then moving this call
right before the register settings -- as we have already made
sure that both inrate and outrate are supported.
> + if (ret) {
> + pair_err("No supported pre-processing options\n");
> + return ret;
> + }
And probably no longer need this error-out. If there's a new
limitation related to this function, I believe we can add it
to the rate validation section as we are doing now -- better
to have rate validation code at one place.
> @@ -380,8 +415,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> /* Apply configurations for pre- and post-processing */
Here:
- /* Apply configurations for pre- and post-processing */
+ /* Select and apply configurations for pre- and post-processing */
+ fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
> regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
> ASRCFG_PREMODi_MASK(index) | ASRCFG_POSTMODi_MASK(index),
> - ASRCFG_PREMOD(index, process_option[in][out][0]) |
> - ASRCFG_POSTMOD(index, process_option[in][out][1]));
> + ASRCFG_PREMOD(index, pre_proc) |
> + ASRCFG_POSTMOD(index, post_proc));
^ permalink raw reply
* Re: [PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
From: Nicolin Chen @ 2019-04-19 18:28 UTC (permalink / raw)
To: S.j. Wang
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com,
linux-kernel@vger.kernel.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <b450010bb24e18e01b78d32067d6efdb572a9cc4.1555669068.git.shengjiu.wang@nxp.com>
On Fri, Apr 19, 2019 at 10:23:56AM +0000, S.j. Wang wrote:
> Unify the supported input and output rate, add the
> 12kHz/24kHz/128kHz to the support list
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/fsl_asrc.c | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 2c4bbc3499db..0d06e738264a 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -27,13 +27,14 @@
> dev_dbg(&asrc_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
>
> /* Corresponding to process_option */
> -static int supported_input_rate[] = {
> - 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
> - 96000, 176400, 192000,
> +static unsigned int supported_asrc_rate[] = {
> + 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
> + 64000, 88200, 96000, 128000, 176400, 192000,
> };
>
> -static int supported_asrc_rate[] = {
> - 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000, 176400, 192000,
> +static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
> + .count = ARRAY_SIZE(supported_asrc_rate),
> + .list = supported_asrc_rate,
> };
>
> /**
> @@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> ideal = config->inclk == INCLK_NONE;
>
> /* Validate input and output sample rates */
> - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
> - if (inrate == supported_input_rate[in])
> + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
> + if (inrate == supported_asrc_rate[in])
> break;
Not sure if we still need it upon having hw_constraint. Maybe m2m
needs it?
^ permalink raw reply
* [PATCH] [v2] x86/mpx: fix recursive munmap() corruption
From: Dave Hansen @ 2019-04-19 19:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arch, hjl.tools, mhocko, rguenther, richard, gxt, jdike,
Dave Hansen, linux-um, x86, stable, luto, linux-mm, paulus,
yang.shi, akpm, linuxppc-dev, vbabka, anton.ivanov
Changes from v1:
* Fix compile errors on UML and non-x86 arches
* Clarify commit message and Fixes about the origin of the
bug and add the impact to powerpc / uml / unicore32
--
This is a bit of a mess, to put it mildly. But, it's a bug
that only seems to have showed up in 4.20 but wasn't noticed
until now because nobody uses MPX.
MPX has the arch_unmap() hook inside of munmap() because MPX
uses bounds tables that protect other areas of memory. When
memory is unmapped, there is also a need to unmap the MPX
bounds tables. Barring this, unused bounds tables can eat 80%
of the address space.
But, the recursive do_munmap() that gets called vi arch_unmap()
wreaks havoc with __do_munmap()'s state. It can result in
freeing populated page tables, accessing bogus VMA state,
double-freed VMAs and more.
To fix this, call arch_unmap() before __do_unmap() has a chance
to do anything meaningful. Also, remove the 'vma' argument
and force the MPX code to do its own, independent VMA lookup.
== UML / unicore32 impact ==
Remove unused 'vma' argument to arch_unmap(). No functional
change.
I compile tested this on UML but not unicore32.
== powerpc impact ==
powerpc uses arch_unmap() well to watch for munmap() on the
VDSO and zeroes out 'current->mm->context.vdso_base'. Moving
arch_unmap() makes this happen earlier in __do_munmap(). But,
'vdso_base' seems to only be used in perf and in the signal
delivery that happens near the return to userspace. I can not
find any likely impact to powerpc, other than the zeroing
happening a little earlier.
powerpc does not use the 'vma' argument and is unaffected by
its removal.
I compile-tested a 64-bit powerpc defconfig.
== x86 impact ==
For the common success case this is functionally identical to
what was there before. For the munmap() failure case, it's
possible that some MPX tables will be zapped for memory that
continues to be in use. But, this is an extraordinarily
unlikely scenario and the harm would be that MPX provides no
protection since the bounds table got reset (zeroed).
I can't imagine anyone doing this:
ptr = mmap();
// use ptr
ret = munmap(ptr);
if (ret)
// oh, there was an error, I'll
// keep using ptr.
Because if you're doing munmap(), you are *done* with the
memory. There's probably no good data in there _anyway_.
This passes the original reproducer from Richard Biener as
well as the existing mpx selftests/.
====
The long story:
munmap() has a couple of pieces:
1. Find the affected VMA(s)
2. Split the start/end one(s) if neceesary
3. Pull the VMAs out of the rbtree
4. Actually zap the memory via unmap_region(), including
freeing page tables (or queueing them to be freed).
5. Fixup some of the accounting (like fput()) and actually
free the VMA itself.
This specific ordering was actually introduced by:
dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
during the 4.20 merge window. The previous __do_munmap() code
was actually safe because the only thing after arch_unmap() was
remove_vma_list(). arch_unmap() could not see 'vma' in the
rbtree because it was detached, so it is not even capable of
doing operations unsafe for remove_vma_list()'s use of 'vma'.
Richard Biener reported a test that shows this in dmesg:
[1216548.787498] BUG: Bad rss-counter state mm:0000000017ce560b idx:1 val:551
[1216548.787500] BUG: non-zero pgtables_bytes on freeing mm: 24576
What triggered this was the recursive do_munmap() called via
arch_unmap(). It was freeing page tables that has not been
properly zapped.
But, the problem was bigger than this. For one, arch_unmap()
can free VMAs. But, the calling __do_munmap() has variables
that *point* to VMAs and obviously can't handle them just
getting freed while the pointer is still in use.
I tried a couple of things here. First, I tried to fix the page
table freeing problem in isolation, but I then found the VMA
issue. I also tried having the MPX code return a flag if it
modified the rbtree which would force __do_munmap() to re-walk
to restart. That spiralled out of control in complexity pretty
fast.
Just moving arch_unmap() and accepting that the bonkers failure
case might eat some bounds tables seems like the simplest viable
fix.
This was also reported in the following kernel bugzilla entry:
https://bugzilla.kernel.org/show_bug.cgi?id=203123
There are some reports that dd2283f2605 ("mm: mmap: zap pages
with read mmap_sem in munmap") triggered this issue. While that
commit certainly made the issues easier to hit, I belive the
fundamental issue has been with us as long as MPX itself, thus
the Fixes: tag below is for one of the original MPX commits.
Reported-by: Richard Biener <rguenther@suse.de>
Reported-by: H.J. Lu <hjl.tools@gmail.com>
Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: x86@kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: stable@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-um@lists.infradead.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linux-arch@vger.kernel.org
Cc: Guan Xuetao <gxt@pku.edu.cn>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
b/arch/powerpc/include/asm/mmu_context.h | 1 -
b/arch/um/include/asm/mmu_context.h | 1 -
b/arch/unicore32/include/asm/mmu_context.h | 1 -
b/arch/x86/include/asm/mmu_context.h | 6 +++---
b/arch/x86/include/asm/mpx.h | 5 ++---
b/arch/x86/mm/mpx.c | 10 ++++++----
b/include/asm-generic/mm_hooks.h | 1 -
b/mm/mmap.c | 15 ++++++++-------
8 files changed, 19 insertions(+), 21 deletions(-)
diff -puN mm/mmap.c~mpx-rss-pass-no-vma mm/mmap.c
--- a/mm/mmap.c~mpx-rss-pass-no-vma 2019-04-19 09:31:09.851509404 -0700
+++ b/mm/mmap.c 2019-04-19 09:31:09.864509404 -0700
@@ -2730,9 +2730,17 @@ int __do_munmap(struct mm_struct *mm, un
return -EINVAL;
len = PAGE_ALIGN(len);
+ end = start + len;
if (len == 0)
return -EINVAL;
+ /*
+ * arch_unmap() might do unmaps itself. It must be called
+ * and finish any rbtree manipulation before this code
+ * runs and also starts to manipulate the rbtree.
+ */
+ arch_unmap(mm, start, end);
+
/* Find the first overlapping VMA */
vma = find_vma(mm, start);
if (!vma)
@@ -2741,7 +2749,6 @@ int __do_munmap(struct mm_struct *mm, un
/* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
- end = start + len;
if (vma->vm_start >= end)
return 0;
@@ -2811,12 +2818,6 @@ int __do_munmap(struct mm_struct *mm, un
/* Detach vmas from rbtree */
detach_vmas_to_be_unmapped(mm, vma, prev, end);
- /*
- * mpx unmap needs to be called with mmap_sem held for write.
- * It is safe to call it before unmap_region().
- */
- arch_unmap(mm, vma, start, end);
-
if (downgrade)
downgrade_write(&mm->mmap_sem);
diff -puN arch/x86/include/asm/mmu_context.h~mpx-rss-pass-no-vma arch/x86/include/asm/mmu_context.h
--- a/arch/x86/include/asm/mmu_context.h~mpx-rss-pass-no-vma 2019-04-19 09:31:09.853509404 -0700
+++ b/arch/x86/include/asm/mmu_context.h 2019-04-19 09:31:09.865509404 -0700
@@ -277,8 +277,8 @@ static inline void arch_bprm_mm_init(str
mpx_mm_init(mm);
}
-static inline void arch_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
- unsigned long start, unsigned long end)
+static inline void arch_unmap(struct mm_struct *mm, unsigned long start,
+ unsigned long end)
{
/*
* mpx_notify_unmap() goes and reads a rarely-hot
@@ -298,7 +298,7 @@ static inline void arch_unmap(struct mm_
* consistently wrong.
*/
if (unlikely(cpu_feature_enabled(X86_FEATURE_MPX)))
- mpx_notify_unmap(mm, vma, start, end);
+ mpx_notify_unmap(mm, start, end);
}
/*
diff -puN include/asm-generic/mm_hooks.h~mpx-rss-pass-no-vma include/asm-generic/mm_hooks.h
--- a/include/asm-generic/mm_hooks.h~mpx-rss-pass-no-vma 2019-04-19 09:31:09.856509404 -0700
+++ b/include/asm-generic/mm_hooks.h 2019-04-19 09:31:09.865509404 -0700
@@ -18,7 +18,6 @@ static inline void arch_exit_mmap(struct
}
static inline void arch_unmap(struct mm_struct *mm,
- struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
}
diff -puN arch/x86/mm/mpx.c~mpx-rss-pass-no-vma arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~mpx-rss-pass-no-vma 2019-04-19 09:31:09.858509404 -0700
+++ b/arch/x86/mm/mpx.c 2019-04-19 09:31:09.866509404 -0700
@@ -881,9 +881,10 @@ static int mpx_unmap_tables(struct mm_st
* the virtual address region start...end have already been split if
* necessary, and the 'vma' is the first vma in this range (start -> end).
*/
-void mpx_notify_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
- unsigned long start, unsigned long end)
+void mpx_notify_unmap(struct mm_struct *mm, unsigned long start,
+ unsigned long end)
{
+ struct vm_area_struct *vma;
int ret;
/*
@@ -902,11 +903,12 @@ void mpx_notify_unmap(struct mm_struct *
* which should not occur normally. Being strict about it here
* helps ensure that we do not have an exploitable stack overflow.
*/
- do {
+ vma = find_vma(mm, start);
+ while (vma && vma->vm_start < end) {
if (vma->vm_flags & VM_MPX)
return;
vma = vma->vm_next;
- } while (vma && vma->vm_start < end);
+ }
ret = mpx_unmap_tables(mm, start, end);
if (ret)
diff -puN arch/x86/include/asm/mpx.h~mpx-rss-pass-no-vma arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~mpx-rss-pass-no-vma 2019-04-19 09:31:09.860509404 -0700
+++ b/arch/x86/include/asm/mpx.h 2019-04-19 09:31:09.866509404 -0700
@@ -78,8 +78,8 @@ static inline void mpx_mm_init(struct mm
*/
mm->context.bd_addr = MPX_INVALID_BOUNDS_DIR;
}
-void mpx_notify_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
- unsigned long start, unsigned long end);
+void mpx_notify_unmap(struct mm_struct *mm, unsigned long start,
+ unsigned long end);
unsigned long mpx_unmapped_area_check(unsigned long addr, unsigned long len,
unsigned long flags);
@@ -100,7 +100,6 @@ static inline void mpx_mm_init(struct mm
{
}
static inline void mpx_notify_unmap(struct mm_struct *mm,
- struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
}
diff -puN arch/um/include/asm/mmu_context.h~mpx-rss-pass-no-vma arch/um/include/asm/mmu_context.h
--- a/arch/um/include/asm/mmu_context.h~mpx-rss-pass-no-vma 2019-04-19 09:42:05.789507768 -0700
+++ b/arch/um/include/asm/mmu_context.h 2019-04-19 09:42:57.962507638 -0700
@@ -22,7 +22,6 @@ static inline int arch_dup_mmap(struct m
}
extern void arch_exit_mmap(struct mm_struct *mm);
static inline void arch_unmap(struct mm_struct *mm,
- struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
}
diff -puN arch/unicore32/include/asm/mmu_context.h~mpx-rss-pass-no-vma arch/unicore32/include/asm/mmu_context.h
--- a/arch/unicore32/include/asm/mmu_context.h~mpx-rss-pass-no-vma 2019-04-19 09:42:06.189507767 -0700
+++ b/arch/unicore32/include/asm/mmu_context.h 2019-04-19 09:43:25.425507569 -0700
@@ -88,7 +88,6 @@ static inline int arch_dup_mmap(struct m
}
static inline void arch_unmap(struct mm_struct *mm,
- struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
}
diff -puN arch/powerpc/include/asm/mmu_context.h~mpx-rss-pass-no-vma arch/powerpc/include/asm/mmu_context.h
--- a/arch/powerpc/include/asm/mmu_context.h~mpx-rss-pass-no-vma 2019-04-19 09:42:06.388507766 -0700
+++ b/arch/powerpc/include/asm/mmu_context.h 2019-04-19 09:43:27.392507564 -0700
@@ -237,7 +237,6 @@ extern void arch_exit_mmap(struct mm_str
#endif
static inline void arch_unmap(struct mm_struct *mm,
- struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
if (start <= mm->context.vdso_base && mm->context.vdso_base < end)
_
^ permalink raw reply
* Re: Avoiding merge conflicts while adding new docs - Was: Re: [PATCH 00/57] Convert files to ReST
From: Jonathan Corbet @ 2019-04-19 22:10 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-fbdev, Linux Doc Mailing List, linux-fpga, dri-devel,
linux-ide, dm-devel, target-devel, linux-riscv, linux-stm32,
xdp-newbies, linux-s390, linux-samsung-soc, linux-scsi,
linux-rdma, x86, linux-acpi, linux-arm-kernel, linux-watchdog,
linux-pm, Mauro Carvalho Chehab, linux-gpio, Thomas Gleixner,
linux-kbuild, Greg Kroah-Hartman, linux-usb, kexec, linux-kernel,
linux-security-module, netdev, bpf, linuxppc-dev
In-Reply-To: <20190418091526.00e074d1@coco.lan>
On Thu, 18 Apr 2019 09:42:23 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> After thinking a little bit and doing some tests, I think a good solution
> would be to add ":orphan:" markup to the new .rst files that were not
> added yet into the main body (e. g. something like the enclosed example).
Interesting...I didn't know about that. Yes, I think it makes sense to
put that in...
jon
^ permalink raw reply
* Re: [PATCH 0/8]
From: Tyrel Datwyler @ 2019-04-19 22:36 UTC (permalink / raw)
To: Sam Bobroff, Tyrel Datwyler; +Cc: linuxppc-dev
In-Reply-To: <20190415034109.GA9045@tungsten.ozlabs.ibm.com>
On 04/14/2019 08:41 PM, Sam Bobroff wrote:
> On Thu, Apr 11, 2019 at 05:55:33PM -0700, Tyrel Datwyler wrote:
>> On 03/19/2019 07:58 PM, Sam Bobroff wrote:
>>> Hi all,
>>>
>>> This patch set adds support for EEH recovery of hot plugged devices on pSeries
>>> machines. Specifically, devices discovered by PCI rescanning using
>>> /sys/bus/pci/rescan, which includes devices hotplugged by QEMU's device_add
>>> command. (pSeries doesn't currently use slot power control for hotplugging.)
>>
>> Slight nit that its not that pSeries doesn't support slot power control
>> hotplugging, its that QEMU pSeries guests don't support it. We most definitely
>> use the slot power control for hotplugging in PowerVM pSeries Linux guests. More
>
> Ah, I think I see what you mean: pSeries can (and does!) use slot power
> control for hotplugging, it's just that Linux doesn't. Right :-) I'll
> change it to "Linux on pSeries doesn't...." for the next version.
Not quite. A pSeries Linux PowerVM LPAR does use the slot power hotplugging. It
is only the pSeries Linux qemu guest that doesn't. The way that hotplug is
initiated is different for PowerVM and qemu. On PowerVM the command is sent from
the HMC down the RSCT stack which calls drmgr whereas with qemu hotplug
generates an interrupt which logs an event that is picked up by rtas_errd which
then calls drmgr with the "-V" flag. That flag was a special case that we added
to drmgr to work around the slot power hotplugging with rpaphp not working
correctly with qemu guests.
>
>> specifically we had to work around short comings in the rpaphp driver when
>> dealing with QEMU. This being that while at initial glance the design implies
>> that it had multiple devices per PHB in mind, it didn't, and only actually
>> supported a single slot per PHB. Further, when we developed the QEMU pci hotplug
>> feature we had to deal with only having a single PHB per QEMU guest and as a
>> result needed a way to plug multiple pci devices into a single PHB. Hence, came
>> the pci rescan work around in drmgr.
>>
>> Mike Roth and I have had discussions over the years to get the slot power
>> control hotplugging working properly with QEMU, and while I did get the RPA
>> hotplug driver fixed to register all available slots associated with a PHB, EEH
>> remained an issue. So, I'm very happy to see this patchset get that working with
>> the rescan work around.
>>
>>>
>>> As a side effect this also provides EEH support for devices removed by
>>> /sys/bus/pci/devices/*/remove and re-discovered by writing to /sys/bus/pci/rescan,
>>> on all platforms.
>>
>> +1, this seems like icing on the cake. ;)
>
> Yes :-)
>
> Although maybe I should mention that we can't really benefit from this
> on PowerNV *yet* because there seem to be some other problems with
> removing and re-scanning devices: in my tests devices are often unusable
> after being rediscovered.
>
> (I'm hoping to take a look at that soon.)
Interesting.
>
>>>
>>> The approach I've taken is to use the fact that the existing
>>> pcibios_bus_add_device() platform hooks (which are used to set up EEH on
>>> Virtual Function devices (VFs)) are actually called for all devices, so I've
>>> widened their scope and made other adjustments necessary to allow them to work
>>> for hotplugged and boot-time devices as well.
>>>
>>> Because some of the changes are in generic PowerPC code, it's
>>> possible that I've disturbed something for another PowerPC platform. I've tried
>>> to minimize this by leaving that code alone as much as possible and so there
>>> are a few cases where eeh_add_device_{early,late}() or eeh_add_sysfs_files() is
>>> called more than once. I think these can be looked at later, as duplicate calls
>>> are not harmful.
>>>
>>> The patch "Convert PNV_PHB_FLAG_EEH" isn't strictly necessary and I'm not sure
>>> if it's better to keep it, because it simplifies the code or drop it, because
>>> we may need a separate flag per PHB later on. Thoughts anyone?
>>>
>>> The first patch is a rework of the pcibios_init reordering patch I posted
>>> earlier, which I've included here because it's necessary for this set.
>>>
>>> I have done some testing for PowerNV on Power9 using a modified pnv_php module
>>> and some testing on pSeries with slot power control using a modified rpaphp
>>> module, and the EEH-related parts seem to work.
>>
>> I'm interested in what modifications with rpaphp. Its unclear if you are saying
>> rpaphp modified so that slot power hotplug works with a QEMU pSeries guest? If
>> thats the case it would be optimal to get that upstream and remove the work
>> rescan workaround for guests that don't need it.
>
> Unfortunately no, I didn't do enough work to really get it working. I
> just wanted to get an idea of how that code path interacted with the EEH
> code I was changing, so that hopefully when we get to fixing it, the EEH
> part will be easier to do.
>
> The hack I tested with was:
>
> - rtas_errd changed so that it doesn't pass -V to drmgr (-V seems to
> trigger drmgr to use the PCI rescan system rather that slot power
> control).
Correct, as I mentioned above we did that on purpose to basically hack around
rpaphp not working with qemu guests.
-Tyrel
> - of_pci_parse_addrs() changed so that if the assigned-addresses node
> is missing (which it is when the guest is running under QEMU/KVM) we
> call pci_setup_device() to configure the BARs.
>
> It did look pretty good -- the EEH part may actually work fine once we get
> the rest sorted out.
>
>>
>> -Tyrel
>>
>>>
>>> Cheers,
>>> Sam.
>>>
>>> Sam Bobroff (8):
>>> powerpc/64: Adjust order in pcibios_init()
>>> powerpc/eeh: Clear stale EEH_DEV_NO_HANDLER flag
>>> powerpc/eeh: Convert PNV_PHB_FLAG_EEH to global flag
>>> powerpc/eeh: Improve debug messages around device addition
>>> powerpc/eeh: Add eeh_show_enabled()
>>> powerpc/eeh: Initialize EEH address cache earlier
>>> powerpc/eeh: EEH for pSeries hot plug
>>> powerpc/eeh: Remove eeh_probe_devices() and eeh_addr_cache_build()
>>>
>>> arch/powerpc/include/asm/eeh.h | 19 +++--
>>> arch/powerpc/kernel/eeh.c | 33 ++++-----
>>> arch/powerpc/kernel/eeh_cache.c | 29 +-------
>>> arch/powerpc/kernel/eeh_driver.c | 4 ++
>>> arch/powerpc/kernel/of_platform.c | 3 +-
>>> arch/powerpc/kernel/pci-common.c | 4 --
>>> arch/powerpc/kernel/pci_32.c | 4 ++
>>> arch/powerpc/kernel/pci_64.c | 12 +++-
>>> arch/powerpc/platforms/powernv/eeh-powernv.c | 41 +++++------
>>> arch/powerpc/platforms/powernv/pci.c | 7 +-
>>> arch/powerpc/platforms/powernv/pci.h | 2 -
>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 75 +++++++++++---------
>>> arch/powerpc/platforms/pseries/pci.c | 7 +-
>>> 13 files changed, 122 insertions(+), 118 deletions(-)
>>>
>>
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-04-19 23:09 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <87a7go71hz.fsf@morokweng.localdomain>
On Wed, Apr 17, 2019 at 06:42:00PM -0300, Thiago Jung Bauermann wrote:
>
> Michael S. Tsirkin <mst@redhat.com> writes:
>
> > On Thu, Mar 21, 2019 at 09:05:04PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Michael S. Tsirkin <mst@redhat.com> writes:
> >>
> >> > On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
> >> >> >From what I understand of the ACCESS_PLATFORM definition, the host will
> >> >> only ever try to access memory addresses that are supplied to it by the
> >> >> guest, so all of the secure guest memory that the host cares about is
> >> >> accessible:
> >> >>
> >> >> If this feature bit is set to 0, then the device has same access to
> >> >> memory addresses supplied to it as the driver has. In particular,
> >> >> the device will always use physical addresses matching addresses
> >> >> used by the driver (typically meaning physical addresses used by the
> >> >> CPU) and not translated further, and can access any address supplied
> >> >> to it by the driver. When clear, this overrides any
> >> >> platform-specific description of whether device access is limited or
> >> >> translated in any way, e.g. whether an IOMMU may be present.
> >> >>
> >> >> All of the above is true for POWER guests, whether they are secure
> >> >> guests or not.
> >> >>
> >> >> Or are you saying that a virtio device may want to access memory
> >> >> addresses that weren't supplied to it by the driver?
> >> >
> >> > Your logic would apply to IOMMUs as well. For your mode, there are
> >> > specific encrypted memory regions that driver has access to but device
> >> > does not. that seems to violate the constraint.
> >>
> >> Right, if there's a pre-configured 1:1 mapping in the IOMMU such that
> >> the device can ignore the IOMMU for all practical purposes I would
> >> indeed say that the logic would apply to IOMMUs as well. :-)
> >>
> >> I guess I'm still struggling with the purpose of signalling to the
> >> driver that the host may not have access to memory addresses that it
> >> will never try to access.
> >
> > For example, one of the benefits is to signal to host that driver does
> > not expect ability to access all memory. If it does, host can
> > fail initialization gracefully.
>
> But why would the ability to access all memory be necessary or even
> useful? When would the host access memory that the driver didn't tell it
> to access?
When I say all memory I mean even memory not allowed by the IOMMU.
> >> >> >> > But the name "sev_active" makes me scared because at least AMD guys who
> >> >> >> > were doing the sensible thing and setting ACCESS_PLATFORM
> >> >> >>
> >> >> >> My understanding is, AMD guest-platform knows in advance that their
> >> >> >> guest will run in secure mode and hence sets the flag at the time of VM
> >> >> >> instantiation. Unfortunately we dont have that luxury on our platforms.
> >> >> >
> >> >> > Well you do have that luxury. It looks like that there are existing
> >> >> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
> >> >> > with how that path is slow. So you are trying to optimize for
> >> >> > them by clearing ACCESS_PLATFORM and then you have lost ability
> >> >> > to invoke DMA API.
> >> >> >
> >> >> > For example if there was another flag just like ACCESS_PLATFORM
> >> >> > just not yet used by anyone, you would be all fine using that right?
> >> >>
> >> >> Yes, a new flag sounds like a great idea. What about the definition
> >> >> below?
> >> >>
> >> >> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
> >> >> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
> >> >> exception that the IOMMU is explicitly defined to be off or bypassed
> >> >> when accessing memory addresses supplied to the device by the
> >> >> driver. This flag should be set by the guest if offered, but to
> >> >> allow for backward-compatibility device implementations allow for it
> >> >> to be left unset by the guest. It is an error to set both this flag
> >> >> and VIRTIO_F_ACCESS_PLATFORM.
> >> >
> >> > It looks kind of narrow but it's an option.
> >>
> >> Great!
> >>
> >> > I wonder how we'll define what's an iommu though.
> >>
> >> Hm, it didn't occur to me it could be an issue. I'll try.
>
> I rephrased it in terms of address translation. What do you think of
> this version? The flag name is slightly different too:
>
>
> VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same
> meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set,
> with the exception that address translation is guaranteed to be
> unnecessary when accessing memory addresses supplied to the device
> by the driver. Which is to say, the device will always use physical
> addresses matching addresses used by the driver (typically meaning
> physical addresses used by the CPU) and not translated further. This
> flag should be set by the guest if offered, but to allow for
> backward-compatibility device implementations allow for it to be
> left unset by the guest. It is an error to set both this flag and
> VIRTIO_F_ACCESS_PLATFORM.
Thanks, I'll think about this approach. Will respond next week.
> >> > Another idea is maybe something like virtio-iommu?
> >>
> >> You mean, have legacy guests use virtio-iommu to request an IOMMU
> >> bypass? If so, it's an interesting idea for new guests but it doesn't
> >> help with guests that are out today in the field, which don't have A
> >> virtio-iommu driver.
> >
> > I presume legacy guests don't use encrypted memory so why do we
> > worry about them at all?
>
> They don't use encrypted memory, but a host machine will run a mix of
> secure and legacy guests. And since the hypervisor doesn't know whether
> a guest will be secure or not at the time it is launched, legacy guests
> will have to be launched with the same configuration as secure guests.
OK and so I think the issue is that hosts generally fail if they set
ACCESS_PLATFORM and guests do not negotiate it.
So you can not just set ACCESS_PLATFORM for everyone.
Is that the issue here?
> >> >> > Is there any justification to doing that beyond someone putting
> >> >> > out slow code in the past?
> >> >>
> >> >> The definition of the ACCESS_PLATFORM flag is generic and captures the
> >> >> notion of memory access restrictions for the device. Unfortunately, on
> >> >> powerpc pSeries guests it also implies that the IOMMU is turned on
> >> >
> >> > IIUC that's really because on pSeries IOMMU is *always* turned on.
> >> > Platform has no way to say what you want it to say
> >> > which is bypass the iommu for the specific device.
> >>
> >> Yes, that's correct. pSeries guests running on KVM are in a gray area
> >> where theoretically they use an IOMMU but in practice KVM ignores it.
> >> It's unfortunate but it's the reality on the ground today. :-/
> >
> > Well it's not just the reality, virt setups need something that
> > emulated IOMMUs don't provide. That is not uncommon, e.g.
> > intel's VTD has a "cache mode" field which AFAIK is only used for virt.
>
> That's good to know. Thanks for this example.
>
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* RE: [EXT] Re: [PATCH V4 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
From: S.j. Wang @ 2019-04-20 7:16 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com,
linux-kernel@vger.kernel.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20190419181003.GA1136@Asurada-Nvidia.nvidia.com>
Hi
>
>
> On Fri, Apr 19, 2019 at 10:23:50AM +0000, S.j. Wang wrote:
> > When the output sample rate is [8kHz, 30kHz], the limitation of the
> > supported ratio range is (1/24, 8). In the driver we use (8kHz, 30kHz)
> > instead of [8kHz, 30kHz].
> > So this patch is to fix this issue and the potential rounding issue
> > with divider.
> >
> > Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz output
> > sample rate")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > sound/soc/fsl/fsl_asrc.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 0b937924d2e4..5b8adc7fb117 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -282,10 +282,10 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> > return -EINVAL;
> > }
> >
> > - if ((outrate > 8000 && outrate < 30000) &&
> > - (outrate/inrate > 24 || inrate/outrate > 8)) {
> > - pair_err("exceed supported ratio range [1/24, 8] for \
> > - inrate/outrate: %d/%d\n", inrate, outrate);
> > + if ((outrate >= 8000 && outrate <= 30000) &&
> > + (outrate > 24 * inrate || inrate > 8 * outrate)) {
> > + pair_err("exceed supported ratio range (1/24, 8) for
> > + inrate/outrate: %d/%d\n",
>
> Using one of the conditions:
> if (inrate > 8 * outrate)
> pair_err();
>
> This means:
> if (inrate <= 8 * outrate)
> /* Everything is fine */
>
> So the supported ratio range is still [1/24, 8] right?
>
Oh, yes, should still [1/24, 8].
> Thanks
^ permalink raw reply
* Re: [PATCH V4 2/3] ASoC: fsl_asrc: replace the process_option table with function
From: S.j. Wang @ 2019-04-20 7:21 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com,
linux-kernel@vger.kernel.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
Hi
>
>
> On Fri, Apr 19, 2019 at 10:23:53AM +0000, S.j. Wang wrote:
>
> > @@ -289,6 +318,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > return -EINVAL;
> > }
> >
> > + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
>
> Since the function always return 0, I am thinking of treating this function as
> a lookup function, and then moving this call right before the register
> settings -- as we have already made sure that both inrate and outrate are
> supported.
Ok.
>
> > + if (ret) {
> > + pair_err("No supported pre-processing options\n");
> > + return ret;
> > + }
>
> And probably no longer need this error-out. If there's a new limitation
> related to this function, I believe we can add it to the rate validation
> section as we are doing now -- better to have rate validation code at one
> place.
>
Ok.
> > @@ -380,8 +415,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > /* Apply configurations for pre- and post-processing */
>
> Here:
> - /* Apply configurations for pre- and post-processing */
> + /* Select and apply configurations for pre- and post-processing */
> + fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
> > regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
> > ASRCFG_PREMODi_MASK(index) |
> ASRCFG_POSTMODi_MASK(index),
> > - ASRCFG_PREMOD(index, process_option[in][out][0]) |
> > - ASRCFG_POSTMOD(index, process_option[in][out][1]));
> > + ASRCFG_PREMOD(index, pre_proc) |
> > + ASRCFG_POSTMOD(index, post_proc));
^ permalink raw reply
* Re: [PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
From: S.j. Wang @ 2019-04-20 7:23 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com,
linux-kernel@vger.kernel.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
Hi.
>
> On Fri, Apr 19, 2019 at 10:23:56AM +0000, S.j. Wang wrote:
> > Unify the supported input and output rate, add the 12kHz/24kHz/128kHz
> > to the support list
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > sound/soc/fsl/fsl_asrc.c | 32 +++++++++++++++++++-------------
> > 1 file changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 2c4bbc3499db..0d06e738264a 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -27,13 +27,14 @@
> > dev_dbg(&asrc_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
> > ##__VA_ARGS__)
> >
> > /* Corresponding to process_option */ -static int
> > supported_input_rate[] = {
> > - 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
> > - 96000, 176400, 192000,
> > +static unsigned int supported_asrc_rate[] = {
> > + 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
> > + 64000, 88200, 96000, 128000, 176400, 192000,
> > };
> >
> > -static int supported_asrc_rate[] = {
> > - 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
> 176400, 192000,
> > +static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
> > + .count = ARRAY_SIZE(supported_asrc_rate),
> > + .list = supported_asrc_rate,
> > };
> >
> > /**
> > @@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> > ideal = config->inclk == INCLK_NONE;
> >
> > /* Validate input and output sample rates */
> > - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
> > - if (inrate == supported_input_rate[in])
> > + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
> > + if (inrate == supported_asrc_rate[in])
> > break;
>
> Not sure if we still need it upon having hw_constraint. Maybe m2m needs it?
Yes.
Best regards
Wang shengjiu
^ permalink raw reply
* Re: [PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
From: Nicolin Chen @ 2019-04-20 7:29 UTC (permalink / raw)
To: S.j. Wang
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com,
linux-kernel@vger.kernel.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <AM0PR04MB646884E539AD85971FD255AEE3200@AM0PR04MB6468.eurprd04.prod.outlook.com>
On Sat, Apr 20, 2019 at 07:23:59AM +0000, S.j. Wang wrote:
> > > /* Validate input and output sample rates */
> > > - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
> > > - if (inrate == supported_input_rate[in])
> > > + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
> > > + if (inrate == supported_asrc_rate[in])
> > > break;
> >
> > Not sure if we still need it upon having hw_constraint. Maybe m2m needs it?
> Yes.
OK. Then we can leave it there. Thanks
^ permalink raw reply
* Re: [PATCH] [v2] x86/mpx: fix recursive munmap() corruption
From: Michael Ellerman @ 2019-04-20 11:00 UTC (permalink / raw)
To: Dave Hansen, linux-kernel
Cc: linux-arch, hjl.tools, mhocko, rguenther, gxt, jdike, Dave Hansen,
linux-um, x86, stable, luto, linux-mm, paulus, richard, yang.shi,
akpm, linuxppc-dev, vbabka, anton.ivanov
In-Reply-To: <20190419194747.5E1AD6DC@viggo.jf.intel.com>
Dave Hansen <dave.hansen@linux.intel.com> writes:
> Changes from v1:
> * Fix compile errors on UML and non-x86 arches
> * Clarify commit message and Fixes about the origin of the
> bug and add the impact to powerpc / uml / unicore32
>
> --
>
> This is a bit of a mess, to put it mildly. But, it's a bug
> that only seems to have showed up in 4.20 but wasn't noticed
> until now because nobody uses MPX.
>
> MPX has the arch_unmap() hook inside of munmap() because MPX
> uses bounds tables that protect other areas of memory. When
> memory is unmapped, there is also a need to unmap the MPX
> bounds tables. Barring this, unused bounds tables can eat 80%
> of the address space.
>
> But, the recursive do_munmap() that gets called vi arch_unmap()
> wreaks havoc with __do_munmap()'s state. It can result in
> freeing populated page tables, accessing bogus VMA state,
> double-freed VMAs and more.
>
> To fix this, call arch_unmap() before __do_unmap() has a chance
> to do anything meaningful. Also, remove the 'vma' argument
> and force the MPX code to do its own, independent VMA lookup.
>
> == UML / unicore32 impact ==
>
> Remove unused 'vma' argument to arch_unmap(). No functional
> change.
>
> I compile tested this on UML but not unicore32.
>
> == powerpc impact ==
>
> powerpc uses arch_unmap() well to watch for munmap() on the
> VDSO and zeroes out 'current->mm->context.vdso_base'. Moving
> arch_unmap() makes this happen earlier in __do_munmap(). But,
> 'vdso_base' seems to only be used in perf and in the signal
> delivery that happens near the return to userspace. I can not
> find any likely impact to powerpc, other than the zeroing
> happening a little earlier.
Yeah I agree.
> powerpc does not use the 'vma' argument and is unaffected by
> its removal.
>
> I compile-tested a 64-bit powerpc defconfig.
Thanks.
cheers
^ permalink raw reply
* [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
From: Daniel Baluta @ 2019-04-20 15:41 UTC (permalink / raw)
To: broonie@kernel.org
Cc: Daniel Baluta, alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
nicoleotsuka@gmail.com, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
SAI provide multiple master clock source options selectable
via bit MSEL of TCR2/RCR2.
All possible master clock sources are stored in sai->mclk_clk
array. Current implementation assumes that MCLK0 source is always
busclk, but this is wrong!
For example, on i.MX8QM we have:
00b - Bus Clock selected.
01b - Master Clock (MCLK) 1 option selected.
10b - Master Clock (MCLK) 2 option selected.
11b - Master Clock (MCLK) 3 option selected.
while on i.MX6SX we have:
00b - Master Clock (MCLK) 1 option selected.
01b - Master Clock (MCLK) 1 option selected.
10b - Master Clock (MCLK) 2 option selected.
11b - Master Clock (MCLK) 3 option selected.
So, this patch will read mclk0 source clock from device tree.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
sound/soc/fsl/fsl_sai.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index d2a4dc744fd7..faa8de87ff83 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -829,8 +829,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
sai->bus_clk = NULL;
}
- sai->mclk_clk[0] = sai->bus_clk;
- for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
+ for (i = 0; i < FSL_SAI_MCLK_MAX; i++) {
sprintf(tmp, "mclk%d", i);
sai->mclk_clk[i] = devm_clk_get(&pdev->dev, tmp);
if (IS_ERR(sai->mclk_clk[i])) {
--
2.17.1
^ permalink raw reply related
* [PATCH 0/2] Add runtime PM for SAI digital audio interface
From: Daniel Baluta @ 2019-04-20 15:59 UTC (permalink / raw)
To: broonie@kernel.org
Cc: Daniel Baluta, alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
nicoleotsuka@gmail.com, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
First patch uses system PM handlers to implement runtime PM. While
the second patch moves clock handling from startup/shutdown to runtime
PM handlers.
Daniel Baluta (1):
ASoC: fsl: sai: Add support for runtime pm
Shengjiu Wang (1):
ASoC: fsl: Move clock operation to PM runtime
sound/soc/fsl/fsl_sai.c | 70 ++++++++++++++++++++++++++++++++---------
1 file changed, 55 insertions(+), 15 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 1/2] ASoC: fsl: sai: Add support for runtime pm
From: Daniel Baluta @ 2019-04-20 15:59 UTC (permalink / raw)
To: broonie@kernel.org
Cc: Daniel Baluta, alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
nicoleotsuka@gmail.com, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20190420155846.10027-1-daniel.baluta@nxp.com>
Basically the same actions as for system PM, so make use
of pm_runtime_force_suspend/pm_runtime_force_resume.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
sound/soc/fsl/fsl_sai.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index b563004fb89f..aeeb07b74177 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -9,6 +9,7 @@
#include <linux/dmaengine.h>
#include <linux/module.h>
#include <linux/of_address.h>
+#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/time.h>
@@ -898,6 +899,8 @@ static int fsl_sai_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sai);
+ pm_runtime_enable(&pdev->dev);
+
ret = devm_snd_soc_register_component(&pdev->dev, &fsl_component,
&fsl_sai_dai, 1);
if (ret)
@@ -917,8 +920,8 @@ static const struct of_device_id fsl_sai_ids[] = {
};
MODULE_DEVICE_TABLE(of, fsl_sai_ids);
-#ifdef CONFIG_PM_SLEEP
-static int fsl_sai_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int fsl_sai_runtime_suspend(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
@@ -928,7 +931,7 @@ static int fsl_sai_suspend(struct device *dev)
return 0;
}
-static int fsl_sai_resume(struct device *dev)
+static int fsl_sai_runtime_resume(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
@@ -940,10 +943,13 @@ static int fsl_sai_resume(struct device *dev)
regmap_write(sai->regmap, FSL_SAI_RCSR, 0);
return regcache_sync(sai->regmap);
}
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
static const struct dev_pm_ops fsl_sai_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(fsl_sai_suspend, fsl_sai_resume)
+ SET_RUNTIME_PM_OPS(fsl_sai_runtime_suspend,
+ fsl_sai_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static struct platform_driver fsl_sai_driver = {
--
2.17.1
^ permalink raw reply related
* [PATCH 2/2] ASoC: fsl: Move clock operation to PM runtime
From: Daniel Baluta @ 2019-04-20 15:59 UTC (permalink / raw)
To: broonie@kernel.org
Cc: Daniel Baluta, alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
nicoleotsuka@gmail.com, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20190420155846.10027-1-daniel.baluta@nxp.com>
From: Shengjiu Wang <shengjiu.wang@nxp.com>
Turn off/on clocks when device enters suspend/resume. This
helps saving power.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
sound/soc/fsl/fsl_sai.c | 54 +++++++++++++++++++++++++++++++++--------
1 file changed, 44 insertions(+), 10 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index aeeb07b74177..a224c07ce31f 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -594,15 +594,8 @@ static int fsl_sai_startup(struct snd_pcm_substream *substream,
{
struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
- struct device *dev = &sai->pdev->dev;
int ret;
- ret = clk_prepare_enable(sai->bus_clk);
- if (ret) {
- dev_err(dev, "failed to enable bus clock: %d\n", ret);
- return ret;
- }
-
regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE,
FSL_SAI_CR3_TRCE);
@@ -619,8 +612,6 @@ static void fsl_sai_shutdown(struct snd_pcm_substream *substream,
bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE, 0);
-
- clk_disable_unprepare(sai->bus_clk);
}
static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
@@ -925,6 +916,14 @@ static int fsl_sai_runtime_suspend(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[0]]);
+
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[1]]);
+
+ clk_disable_unprepare(sai->bus_clk);
+
regcache_cache_only(sai->regmap, true);
regcache_mark_dirty(sai->regmap);
@@ -934,6 +933,25 @@ static int fsl_sai_runtime_suspend(struct device *dev)
static int fsl_sai_runtime_resume(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(sai->bus_clk);
+ if (ret) {
+ dev_err(dev, "failed to enable bus clock: %d\n", ret);
+ return ret;
+ }
+
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK)) {
+ ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[1]]);
+ if (ret)
+ goto disable_bus_clk;
+ }
+
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE)) {
+ ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[0]]);
+ if (ret)
+ goto disable_tx_clk;
+ }
regcache_cache_only(sai->regmap, false);
regmap_write(sai->regmap, FSL_SAI_TCSR, FSL_SAI_CSR_SR);
@@ -941,7 +959,23 @@ static int fsl_sai_runtime_resume(struct device *dev)
usleep_range(1000, 2000);
regmap_write(sai->regmap, FSL_SAI_TCSR, 0);
regmap_write(sai->regmap, FSL_SAI_RCSR, 0);
- return regcache_sync(sai->regmap);
+
+ ret = regcache_sync(sai->regmap);
+ if (ret)
+ goto disable_rx_clk;
+
+ return 0;
+
+disable_rx_clk:
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[0]]);
+disable_tx_clk:
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[1]]);
+disable_bus_clk:
+ clk_disable_unprepare(sai->bus_clk);
+
+ return ret;
}
#endif /* CONFIG_PM */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
From: Nicolin Chen @ 2019-04-21 5:37 UTC (permalink / raw)
To: Daniel Baluta
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
broonie@kernel.org, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20190420154038.14576-1-daniel.baluta@nxp.com>
By following the pattern of previous Subjects:
ASoC: fsl_sai: Fix clock Source for mclk0
On Sat, Apr 20, 2019 at 03:41:04PM +0000, Daniel Baluta wrote:
> SAI provide multiple master clock source options selectable
> via bit MSEL of TCR2/RCR2.
>
> All possible master clock sources are stored in sai->mclk_clk
> array. Current implementation assumes that MCLK0 source is always
> busclk, but this is wrong!
>
> For example, on i.MX8QM we have:
>
> 00b - Bus Clock selected.
> 01b - Master Clock (MCLK) 1 option selected.
> 10b - Master Clock (MCLK) 2 option selected.
> 11b - Master Clock (MCLK) 3 option selected.
>
> while on i.MX6SX we have:
>
> 00b - Master Clock (MCLK) 1 option selected.
> 01b - Master Clock (MCLK) 1 option selected.
> 10b - Master Clock (MCLK) 2 option selected.
> 11b - Master Clock (MCLK) 3 option selected.
>
> So, this patch will read mclk0 source clock from device tree.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
> sound/soc/fsl/fsl_sai.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index d2a4dc744fd7..faa8de87ff83 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -829,8 +829,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
> sai->bus_clk = NULL;
> }
>
> - sai->mclk_clk[0] = sai->bus_clk;
> - for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
> + for (i = 0; i < FSL_SAI_MCLK_MAX; i++) {
> sprintf(tmp, "mclk%d", i);
Firstly, according to your commit message, neither imx8qm nor
imx6sx has an "mclk0" clock in the clock list. Either of them
starts with "mclk1". So, before you change the driver, I don't
think it's even a right thing to define an "mclk0" in the DT.
> sai->mclk_clk[i] = devm_clk_get(&pdev->dev, tmp);
> if (IS_ERR(sai->mclk_clk[i])) {
Secondly, this would break existing DT bindings of imx6sx and
imx7 platforms as they both have clock-names defined in DTB:
clock-names = "bus", "mclk1", "mclk2", "mclk3";
Since there's no "mclk0", the entire probe() would error-out.
And mainline has a DT backward-compatible policy, which means
you can't just rename the "bus" in the DTBs but would have to
support them, not to mention "mclk0" is still questionable.
So the right way to fix it is, in my option, to differentiate
the mclk_clk[0] clock source name with the compatible string.
Then you can get the clock name and simply do:
- sai->mclk_clk[0] = sai->bus_clk;
+ sai->mclk_clk[0] = devm_clk_get(&pdev->dev, tmp);
+ if (IS_ERR(sai->mclk_clk[0)) {
+ /* error-out*/
+ }
^ permalink raw reply
* Re: [PATCH 0/2] Add runtime PM for SAI digital audio interface
From: Nicolin Chen @ 2019-04-21 5:42 UTC (permalink / raw)
To: Daniel Baluta
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
broonie@kernel.org, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20190420155846.10027-1-daniel.baluta@nxp.com>
On Sat, Apr 20, 2019 at 03:59:03PM +0000, Daniel Baluta wrote:
> First patch uses system PM handlers to implement runtime PM. While
> the second patch moves clock handling from startup/shutdown to runtime
> PM handlers.
>
> ASoC: fsl: sai: Add support for runtime pm
> ASoC: fsl: Move clock operation to PM runtime
Both patches should use the prefix:
ASoC: fsl_sai: Xxxxx yyyy
Thanks
^ permalink raw reply
* Re: [PATCH 1/2] ASoC: fsl: sai: Add support for runtime pm
From: Nicolin Chen @ 2019-04-21 5:43 UTC (permalink / raw)
To: Daniel Baluta
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
broonie@kernel.org, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20190420155846.10027-2-daniel.baluta@nxp.com>
On Sat, Apr 20, 2019 at 03:59:04PM +0000, Daniel Baluta wrote:
> @@ -898,6 +899,8 @@ static int fsl_sai_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, sai);
>
> + pm_runtime_enable(&pdev->dev);
You will need pm_runtime_disable() somewhere, like remove().
Thanks
^ permalink raw reply
* Re: [PATCH 2/2] ASoC: fsl: Move clock operation to PM runtime
From: Nicolin Chen @ 2019-04-21 5:54 UTC (permalink / raw)
To: Daniel Baluta
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
broonie@kernel.org, dl-linux-imx, festevam@gmail.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20190420155846.10027-3-daniel.baluta@nxp.com>
On Sat, Apr 20, 2019 at 03:59:05PM +0000, Daniel Baluta wrote:
> Turn off/on clocks when device enters suspend/resume. This
> helps saving power.
> @@ -934,6 +933,25 @@ static int fsl_sai_runtime_suspend(struct device *dev)
> static int fsl_sai_runtime_resume(struct device *dev)
> {
> struct fsl_sai *sai = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(sai->bus_clk);
> + if (ret) {
> + dev_err(dev, "failed to enable bus clock: %d\n", ret);
> + return ret;
> + }
> +
> + if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK)) {
> + ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[1]]);
> + if (ret)
> + goto disable_bus_clk;
> + }
> +
> + if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE)) {
> + ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[0]]);
> + if (ret)
> + goto disable_tx_clk;
> + }
The driver only enables mclk_clks for I2S master mode. But this
change enables them for I2S slave mode also. It doesn't sound a
right thing to me since we are supposed to save power?
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
From: Daniel Baluta @ 2019-04-21 7:26 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, Daniel Baluta, S.j. Wang, tiwai@suse.com,
lgirdwood@gmail.com, broonie@kernel.org, dl-linux-imx,
festevam@gmail.com, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190421053749.GA5552@Asurada>
Hi Nicolin,
Thanks for review!
On Sun, Apr 21, 2019 at 8:39 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> By following the pattern of previous Subjects:
> ASoC: fsl_sai: Fix clock Source for mclk0
I see. Will fix in v2.
>
> On Sat, Apr 20, 2019 at 03:41:04PM +0000, Daniel Baluta wrote:
> > SAI provide multiple master clock source options selectable
> > via bit MSEL of TCR2/RCR2.
> >
> > All possible master clock sources are stored in sai->mclk_clk
> > array. Current implementation assumes that MCLK0 source is always
> > busclk, but this is wrong!
> >
> > For example, on i.MX8QM we have:
> >
> > 00b - Bus Clock selected.
> > 01b - Master Clock (MCLK) 1 option selected.
> > 10b - Master Clock (MCLK) 2 option selected.
> > 11b - Master Clock (MCLK) 3 option selected.
> >
> > while on i.MX6SX we have:
> >
> > 00b - Master Clock (MCLK) 1 option selected.
> > 01b - Master Clock (MCLK) 1 option selected.
> > 10b - Master Clock (MCLK) 2 option selected.
> > 11b - Master Clock (MCLK) 3 option selected.
> >
> > So, this patch will read mclk0 source clock from device tree.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> > sound/soc/fsl/fsl_sai.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index d2a4dc744fd7..faa8de87ff83 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
> > @@ -829,8 +829,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
> > sai->bus_clk = NULL;
> > }
> >
> > - sai->mclk_clk[0] = sai->bus_clk;
> > - for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
> > + for (i = 0; i < FSL_SAI_MCLK_MAX; i++) {
> > sprintf(tmp, "mclk%d", i);
>
> Firstly, according to your commit message, neither imx8qm nor
> imx6sx has an "mclk0" clock in the clock list. Either of them
> starts with "mclk1". So, before you change the driver, I don't
> think it's even a right thing to define an "mclk0" in the DT.
From what I understand mclk0 means option 00b of MSEL bits which is:
* busclk for i.MX8
* mclk1 for i.MX6/7.
Adding a mclk0 in the DT and making it point to the correct option
(busclk or mclk1) does no harm as the driver doesn't yet parse mclk0.
I have already sent a patch to add mclk0 to imx6/7 DTS here:
https://lkml.org/lkml/2019/4/20/56
So, even if the DT change gets accepted first there shouldn't pe any
problem, as the driver won't parse mclk0 string yet.
Even if the current patch gets accepted first it shouldn't be any problem
also as the probe will try to find "mclk0" in the DT and will just print a
warning and move on looking for mclk1, mclk2...
>
> > sai->mclk_clk[i] = devm_clk_get(&pdev->dev, tmp);
> > if (IS_ERR(sai->mclk_clk[i])) {
>
> Secondly, this would break existing DT bindings of imx6sx and
> imx7 platforms as they both have clock-names defined in DTB:
> clock-names = "bus", "mclk1", "mclk2", "mclk3";
> Since there's no "mclk0", the entire probe() would error-out.
Not exactly. The probe won't error-out. It will just print a warning message
dev_err(&pdev->dev, "failed to get mclk%d clock: %ld\n"
and move on.
The functionality, will still be the same.
> And mainline has a DT backward-compatible policy, which means
> you can't just rename the "bus" in the DTBs but would have to
> support them, not to mention "mclk0" is still questionable.
My patch doesn't rename "bus" in the DTB. "bus" clock stays there. It just
adds another clock "mclk0".
In my opinion, the current implementation of fsl_sai has a bug for imx6/7.
Currently, fsl_sai.c driver does:
sai->mclk_clk[0] = sai->bus_clk;
is wrong, because on imx6/7 mclk_clk[0] should point to the same clk
as mclk_clk[1]
>
> So the right way to fix it is, in my option, to differentiate
> the mclk_clk[0] clock source name with the compatible string.
> Then you can get the clock name and simply do:
> - sai->mclk_clk[0] = sai->bus_clk;
> + sai->mclk_clk[0] = devm_clk_get(&pdev->dev, tmp);
> + if (IS_ERR(sai->mclk_clk[0)) {
> + /* error-out*/
> + }
My approach is to add mclk0 in the DT and make it point to:
* busclk for i.MX8
* mclk1 for i.MX6/7.
So, here it is how the DT nodes will look like:
$ arch/arm/boot/dts/imx6sx.dtsi
clocks = <&clks IMX6SX_CLK_SAI1_IPG>,
<&clks IMX6SX_CLK_SAI1>,
<&clks IMX6SX_CLK_SAI1>,
<&clks 0>, <&clks 0>;
clock-names = "bus", "mclk0", "mclk1", "mclk2", "mclk3";
$ arch/arm64/boot/dts/freescale/fsl-imx8mq.dtsi
clocks = <&clk IMX8MQ_CLK_SAI2_IPG>,
<&clk IMX8MQ_CLK_SAI2_IPG>,
<&clk IMX8MQ_CLK_SAI2_ROOT>,
<&clk IMX8MQ_CLK_DUMMY>, <&clk IMX8MQ_CLK_DUMMY>;
clock-names = "bus", "mclk0", "mclk1", "mclk2", "mclk3";
This approach makes busclk/mclk0 handling generic and avoids the looking
for compatible strings.
thanks,
Daniel.
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
From: Nicolin Chen @ 2019-04-21 8:04 UTC (permalink / raw)
To: Daniel Baluta
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, Daniel Baluta, S.j. Wang, tiwai@suse.com,
lgirdwood@gmail.com, broonie@kernel.org, dl-linux-imx,
festevam@gmail.com, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAEnQRZDs_gnS8ehjM2M_y+Yw0Ge-Sq=A2c9BV-g=P_d0+O40hQ@mail.gmail.com>
On Sun, Apr 21, 2019 at 10:26:40AM +0300, Daniel Baluta wrote:
> > Firstly, according to your commit message, neither imx8qm nor
> > imx6sx has an "mclk0" clock in the clock list. Either of them
> > starts with "mclk1". So, before you change the driver, I don't
> > think it's even a right thing to define an "mclk0" in the DT.
>
> From what I understand mclk0 means option 00b of MSEL bits which is:
> * busclk for i.MX8
> * mclk1 for i.MX6/7.
MSEL bit is used for an internal clock MUX to select four clock
inputs. However, these four clock inputs aren't exactly 1:1 of
SAI's inputs. As fas as I can tell, SAI only has one bus clock
and three MCLK[1-3]; the internal clock MUX maps the bus clock
or MCLK1 to its input0, and then linearly maps MCLK[1-3] to its
inputs[1-3]. So it doesn't sound right to me that you define an
"MCLK0" in the DT, as it's supposed to describe input clocks of
SAI block, other than its internal clock MUX's.
> Adding a mclk0 in the DT and making it point to the correct option
> (busclk or mclk1) does no harm as the driver doesn't yet parse mclk0.
I know it's making driver easier, but unfortunately it's not a
right thing to do from my point of view.
> >
> > > sai->mclk_clk[i] = devm_clk_get(&pdev->dev, tmp);
> > > if (IS_ERR(sai->mclk_clk[i])) {
> >
> > Secondly, this would break existing DT bindings of imx6sx and
> > imx7 platforms as they both have clock-names defined in DTB:
> > clock-names = "bus", "mclk1", "mclk2", "mclk3";
> > Since there's no "mclk0", the entire probe() would error-out.
>
> Not exactly. The probe won't error-out. It will just print a warning message
You're right about this part. I didn't look further as the patch
ends at the IS_ERR, so made a wrong assumption. Sorry.
> In my opinion, the current implementation of fsl_sai has a bug for imx6/7.
>
> Currently, fsl_sai.c driver does:
>
> sai->mclk_clk[0] = sai->bus_clk;
>
> is wrong, because on imx6/7 mclk_clk[0] should point to the same clk
> as mclk_clk[1]
You are right. It should be fixed, but not by this approach IMHO.
Thanks
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
From: Nicolin Chen @ 2019-04-21 8:26 UTC (permalink / raw)
To: Daniel Baluta
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, Daniel Baluta, S.j. Wang, tiwai@suse.com,
lgirdwood@gmail.com, broonie@kernel.org, dl-linux-imx,
festevam@gmail.com, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190421080439.GA8784@Asurada>
On Sun, Apr 21, 2019 at 01:04:39AM -0700, Nicolin Chen wrote:
> On Sun, Apr 21, 2019 at 10:26:40AM +0300, Daniel Baluta wrote:
> > > Firstly, according to your commit message, neither imx8qm nor
> > > imx6sx has an "mclk0" clock in the clock list. Either of them
> > > starts with "mclk1". So, before you change the driver, I don't
> > > think it's even a right thing to define an "mclk0" in the DT.
> >
> > From what I understand mclk0 means option 00b of MSEL bits which is:
> > * busclk for i.MX8
> > * mclk1 for i.MX6/7.
>
> MSEL bit is used for an internal clock MUX to select four clock
> inputs. However, these four clock inputs aren't exactly 1:1 of
> SAI's inputs. As fas as I can tell, SAI only has one bus clock
> and three MCLK[1-3]; the internal clock MUX maps the bus clock
> or MCLK1 to its input0, and then linearly maps MCLK[1-3] to its
> inputs[1-3]. So it doesn't sound right to me that you define an
> "MCLK0" in the DT, as it's supposed to describe input clocks of
> SAI block, other than its internal clock MUX's.
Daniel, I think I's saying this too confident, though I do feel
so :) But if you can prove me wrong and justify that there is an
"MCLK0" as an external input of the SAI block, I will agree with
this change.
Thanks
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
From: Daniel Baluta @ 2019-04-21 8:40 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, Daniel Baluta, S.j. Wang, tiwai@suse.com,
lgirdwood@gmail.com, broonie@kernel.org, dl-linux-imx,
festevam@gmail.com, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190421082627.GB8304@Asurada>
On Sun, Apr 21, 2019 at 11:26 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Sun, Apr 21, 2019 at 01:04:39AM -0700, Nicolin Chen wrote:
> > On Sun, Apr 21, 2019 at 10:26:40AM +0300, Daniel Baluta wrote:
> > > > Firstly, according to your commit message, neither imx8qm nor
> > > > imx6sx has an "mclk0" clock in the clock list. Either of them
> > > > starts with "mclk1". So, before you change the driver, I don't
> > > > think it's even a right thing to define an "mclk0" in the DT.
> > >
> > > From what I understand mclk0 means option 00b of MSEL bits which is:
> > > * busclk for i.MX8
> > > * mclk1 for i.MX6/7.
> >
> > MSEL bit is used for an internal clock MUX to select four clock
> > inputs. However, these four clock inputs aren't exactly 1:1 of
> > SAI's inputs. As fas as I can tell, SAI only has one bus clock
> > and three MCLK[1-3]; the internal clock MUX maps the bus clock
> > or MCLK1 to its input0, and then linearly maps MCLK[1-3] to its
> > inputs[1-3]. So it doesn't sound right to me that you define an
> > "MCLK0" in the DT, as it's supposed to describe input clocks of
> > SAI block, other than its internal clock MUX's.
>
> Daniel, I think I's saying this too confident, though I do feel
> so :) But if you can prove me wrong and justify that there is an
> "MCLK0" as an external input of the SAI block, I will agree with
> this change.
Thanks a lot Nicolin for your input on this! Really appreciate it.
Let me have some time to further investigate it and really figure
out what happens at the hardware level.
thanks,
Daniel.
^ permalink raw reply
* Re: [kernel,v2,2/2] powerpc/mm_iommu: Allow pinning large regions
From: Michael Ellerman @ 2019-04-21 14:07 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Alexey Kardashevskiy, Aneesh Kumar K.V, kvm-ppc, David Gibson
In-Reply-To: <20190403041233.57619-3-aik@ozlabs.ru>
On Wed, 2019-04-03 at 04:12:33 UTC, Alexey Kardashevskiy wrote:
> When called with vmas_arg==NULL, get_user_pages_longterm() allocates
> an array of nr_pages*8 which can easily get greater that the max order,
> for example, registering memory for a 256GB guest does this and fails
> in __alloc_pages_nodemask().
>
> This adds a loop over chunks of entries to fit the max order limit.
>
> Fixes: 678e174c4c16 ("powerpc/mm/iommu: allow migration of cma allocated pages during mm_iommu_do_alloc", 2019-03-05)
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/7a3a4d763837d3aa654cd10590309504
cheers
^ permalink raw reply
* Re: [kernel,v2,1/2] powerpc/mm_iommu: Fix potential deadlock
From: Michael Ellerman @ 2019-04-21 14:07 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Alexey Kardashevskiy, Aneesh Kumar K.V, kvm-ppc, David Gibson
In-Reply-To: <20190403041233.57619-2-aik@ozlabs.ru>
On Wed, 2019-04-03 at 04:12:32 UTC, Alexey Kardashevskiy wrote:
> Currently mm_iommu_do_alloc() is called in 2 cases:
> - VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl() for normal memory:
> this locks &mem_list_mutex and then locks mm::mmap_sem
> several times when adjusting locked_vm or pinning pages;
> - vfio_pci_nvgpu_regops::mmap() for GPU memory:
> this is called with mm::mmap_sem held already and it locks
> &mem_list_mutex.
>
> So one can craft a userspace program to do special ioctl and mmap in
> 2 threads concurrently and cause a deadlock which lockdep warns about
> (below).
>
> We did not hit this yet because QEMU constructs the machine in a single
> thread.
>
> This moves the overlap check next to where the new entry is added and
> reduces the amount of time spent with &mem_list_mutex held.
>
> This moves locked_vm adjustment from under &mem_list_mutex.
>
> This relies on mm_iommu_adjust_locked_vm() doing nothing when entries==0.
>
> This is one of the lockdep warnings:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.1.0-rc2-le_nv2_aikATfstn1-p1 #363 Not tainted
> ------------------------------------------------------
> qemu-system-ppc/8038 is trying to acquire lock:
> 000000002ec6c453 (mem_list_mutex){+.+.}, at: mm_iommu_do_alloc+0x70/0x490
>
> but task is already holding lock:
> 00000000fd7da97f (&mm->mmap_sem){++++}, at: vm_mmap_pgoff+0xf0/0x160
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&mm->mmap_sem){++++}:
> lock_acquire+0xf8/0x260
> down_write+0x44/0xa0
> mm_iommu_adjust_locked_vm.part.1+0x4c/0x190
> mm_iommu_do_alloc+0x310/0x490
> tce_iommu_ioctl.part.9+0xb84/0x1150 [vfio_iommu_spapr_tce]
> vfio_fops_unl_ioctl+0x94/0x430 [vfio]
> do_vfs_ioctl+0xe4/0x930
> ksys_ioctl+0xc4/0x110
> sys_ioctl+0x28/0x80
> system_call+0x5c/0x70
>
> -> #0 (mem_list_mutex){+.+.}:
> __lock_acquire+0x1484/0x1900
> lock_acquire+0xf8/0x260
> __mutex_lock+0x88/0xa70
> mm_iommu_do_alloc+0x70/0x490
> vfio_pci_nvgpu_mmap+0xc0/0x130 [vfio_pci]
> vfio_pci_mmap+0x198/0x2a0 [vfio_pci]
> vfio_device_fops_mmap+0x44/0x70 [vfio]
> mmap_region+0x5d4/0x770
> do_mmap+0x42c/0x650
> vm_mmap_pgoff+0x124/0x160
> ksys_mmap_pgoff+0xdc/0x2f0
> sys_mmap+0x40/0x80
> system_call+0x5c/0x70
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&mm->mmap_sem);
> lock(mem_list_mutex);
> lock(&mm->mmap_sem);
> lock(mem_list_mutex);
>
> *** DEADLOCK ***
>
> 1 lock held by qemu-system-ppc/8038:
> #0: 00000000fd7da97f (&mm->mmap_sem){++++}, at: vm_mmap_pgoff+0xf0/0x160
>
> Fixes: c10c21efa4bc ("powerpc/vfio/iommu/kvm: Do not pin device memory", 2018-12-19)
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/eb9d7a62c38628ab0ba6e59d22d7cb79
cheers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox