* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
From: Lucas Stach @ 2021-04-21 17:43 UTC (permalink / raw)
To: Robin Gong, Shengjiu Wang
Cc: sumit.semwal@linaro.org, linaro-mm-sig@lists.linaro.org,
Linux-ALSA, linuxppc-dev@lists.ozlabs.org, Timur Tabi, Xiubo Li,
Fabio Estevam, s.hauer@pengutronix.de, Takashi Iwai,
Liam Girdwood, dri-devel@lists.freedesktop.org, linux-kernel,
Nicolin Chen, Mark Brown, dl-linux-imx, kernel@pengutronix.de,
perex@perex.cz, shawnguo@kernel.org, S.j. Wang,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <VE1PR04MB66887C731E32BDBB340B044C89479@VE1PR04MB6688.eurprd04.prod.outlook.com>
Am Mittwoch, dem 21.04.2021 um 14:54 +0000 schrieb Robin Gong:
> On 20201/04/20 22:01 Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Dienstag, dem 20.04.2021 um 13:47 +0000 schrieb Robin Gong:
> > > On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong:
> > > > > Hi Lucas,
> > > > >
> > > > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > Hi Robin,
> > > > > >
> > > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong:
> > > > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu
> > Wang:
> > > > > > > > > Hi
> > > > > > > > >
> > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > > > > > > > > <l.stach@pengutronix.de>
> > > > > > > > wrote:
> > > > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu
> > Wang:
> > > > > > > > > > > There are two requirements that we need to move the
> > > > > > > > > > > request of dma channel from probe to open.
> > > > > > > > > >
> > > > > > > > > > How do you handle -EPROBE_DEFER return code from the
> > > > > > > > > > channel request if you don't do it in probe?
> > > > > > > > >
> > > > > > > > > I use the dma_request_slave_channel or dma_request_channel
> > > > > > > > > instead of dmaengine_pcm_request_chan_of. so there should
> > > > > > > > > be not -EPROBE_DEFER return code.
> > > > > > > >
> > > > > > > > This is a pretty weak argument. The dmaengine device might
> > > > > > > > probe after you try to get the channel. Using a function to
> > > > > > > > request the channel that doesn't allow you to handle probe
> > > > > > > > deferral is IMHO a bug and should be fixed, instead of
> > > > > > > > building even more assumptions on top
> > > > > > of it.
> > > > > > > >
> > > > > > > > > > > - When dma device binds with power-domains, the power
> > > > > > > > > > > will be enabled when we request dma channel. If the
> > > > > > > > > > > request of dma channel happen on probe, then the
> > > > > > > > > > > power-domains will be always enabled after kernel boot
> > > > > > > > > > > up, which is not good for power saving, so we need
> > > > > > > > > > > to move the request of dma channel to .open();
> > > > > > > > > >
> > > > > > > > > > This is certainly something which could be fixed in the
> > > > > > > > > > dmaengine driver.
> > > > > > > > >
> > > > > > > > > Dma driver always call the pm_runtime_get_sync in
> > > > > > > > > device_alloc_chan_resources, the
> > > > > > > > > device_alloc_chan_resources is called when channel is
> > > > > > > > > requested. so power is enabled on channel
> > > > > > request.
> > > > > > > >
> > > > > > > > So why can't you fix the dmaengine driver to do that RPM
> > > > > > > > call at a later time when the channel is actually going to
> > > > > > > > be used? This will allow further power savings with other
> > > > > > > > slave devices than the audio
> > > > PCM.
> > > > > > > Hi Lucas,
> > > > > > > Thanks for your suggestion. I have tried to implement
> > > > > > > runtime autosuspend in fsl-edma driver on i.mx8qm/qxp with
> > > > > > > delay time (2
> > > > > > > sec) for this feature as below (or you can refer to
> > > > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> > > > > > > pm_runtime_put_autosuspend in all dmaengine driver interface
> > > > > > > like
> > > > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_d
> > > > > > > ma_c
> > > > > > > ycli
> > > > > > > c/
> > > > > > > device_tx_status...
> > > > > > >
> > > > > > >
> > > > > > > pm_runtime_use_autosuspend(fsl_chan->de
> > v);
> > > > > > > pm_runtime_set_autosuspend_delay(fsl_cha
> > n->
> > > > dev,
> > > > > > 2000);
> > > > > > >
> > > > > > > That could resolve this audio case since the autosuspend could
> > > > > > > suspend runtime after
> > > > > > > 2 seconds if there is no further dma transfer but only channel
> > > > > > request(device_alloc_chan_resources).
> > > > > > > But unfortunately, it cause another issue. As you know, on our
> > > > > > > i.mx8qm/qxp, power domain done by scfw
> > > > > > > (drivers/firmware/imx/scu-pd.c)
> > > > > > over mailbox:
> > > > > > > imx_sc_pd_power()->imx_scu_call_rpc()->
> > > > > > > imx_scu_ipc_write()->mbox_send_message()
> > > > > > > which means have to 'waits for completion', meanwhile, some
> > > > > > > driver like tty will call dmaengine interfaces in non-atomic
> > > > > > > case as below,
> > > > > > >
> > > > > > > static int uart_write(struct tty_struct *tty, const unsigned
> > > > > > > char *buf, int count) {
> > > > > > > .......
> > > > > > > port = uart_port_lock(state, flags);
> > > > > > > ......
> > > > > > > __uart_start(tty); //call
> > > > start_tx()->dmaengine_prep_slave_sg...
> > > > > > > uart_port_unlock(port, flags);
> > > > > > > return ret;
> > > > > > > }
> > > > > > >
> > > > > > > Thus dma runtime resume may happen in that timing window and
> > > > > > > cause
> > > > > > kernel alarm.
> > > > > > > I'm not sure whether there are similar limitations on other
> > > > > > > driver subsystem. But for me, It looks like the only way to
> > > > > > > resolve the contradiction between tty and scu-pd (hardware
> > > > > > > limitation on
> > > > > > > i.mx8qm/qxp) is to give up autosuspend and keep
> > > > > > > pm_runtime_get_sync
> > > > > > only in device_alloc_chan_resources because request channel is a
> > > > > > safe non-atomic phase.
> > > > > > > Do you have any idea? Thanks in advance.
> > > > > >
> > > > > > If you look closely at the driver you used as an example
> > > > > > (hidma.c) it looks like there is already something in there,
> > > > > > which looks very much like what you need
> > > > > > here:
> > > > > >
> > > > > > In hidma_issue_pending() the driver tries to get the device to
> > > > > > runtime
> > > > resume.
> > > > > > If this doesn't work, maybe due to the power domain code not
> > > > > > being able to be called in atomic context, the actual work of
> > > > > > waking up the dma hardware and issuing the descriptor is shunted to a
> > tasklet.
> > > > > >
> > > > > > If I'm reading this right, this is exactly what you need here to
> > > > > > be able to call the dmaengine code from atomic context: try the
> > > > > > rpm get and issue immediately when possible, otherwise shunt the
> > > > > > work to a
> > > > > > non- atomic context where you can deal with the requirements of
> > scu-pd.
> > > > > Yes, I can schedule_work to worker to runtime resume edma channel
> > > > > by
> > > > calling scu-pd.
> > > > > But that means all dmaengine interfaces should be taken care, not
> > > > > only
> > > > > issue_pending() but also
> > > > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/
> > > > > dmaengine_tx_status(). Not sure why hidma only take care
> > > > > issue_pending. Maybe their user case is just for memcpy/memset so
> > > > > that no further complicate case as ALSA or TTY.
> > > > > Besides, for autosuspend in cyclic, we have to add
> > > > > pm_runtime_get_sync into interrupt handler as qcom/bam_dma.c. but
> > > > > how could resolve the scu-pd's non-atmoic limitation in interrupt
> > handler?
> > > >
> > > > Sure, this all needs some careful analysis on how those functions
> > > > are called and what to do about atomic callers, but it should be
> > > > doable. I don't see any fundamental issues here.
> > > >
> > > > I don't see why you would ever need to wake the hardware in an
> > > > interrupt handler. Surely the hardware is already awake, as it
> > > > wouldn't signal an interrupt otherwise. And for the issue with
> > > > scu-pd you only care about the state transition of
> > > > suspended->running. If the hardware is already running/awake, the
> > > > runtime pm state handling is nothing more than bumping a refcount,
> > > > which is atomic safe. Putting the HW in suspend is already handled
> > asynchronously in a worker, so this is also atomic safe.
> > > But with autosuspend used, in corner case, may runtime suspended
> > > before falling Into edma interrupt handler if timeout happen with the
> > > delay value of pm_runtime_set_autosuspend_delay(). Thus, can't touch
> > > any edma interrupt status register unless runtime resume edma in
> > > interrupt handler while runtime resume function based on scu-pd's power
> > domain may block or sleep.
> > > I have a simple workaround that disable runtime suspend in
> > > issue_pending worker by calling pm_runtime_forbid() and then enable
> > > runtime auto suspend in dmaengine_terminate_all so that we could
> > > easily regard that edma channel is always in runtime resume between
> > > issue_pending and channel terminated and ignore the above interrupt
> > handler/scu-pd limitation.
> >
> > The IRQ handler is the point where you are informed by the hardware that a
> > specific operation is complete. I don't see any use-case where it would be valid
> > to drop the rpm refcount to 0 before the IRQ is handled. Surely the hardware
> > needs to stay awake until the currently queued operations are complete and if
> > the IRQ handler is the completion point the IRQ handler is the first point in
> > time where your autosuspend timer should start to run. There should never be
> > a situation where the timer expiry can get between IRQ signaling and the
> > handler code running.
> But the timer of runtime_auto_suspend decide when enter runtime suspend rather
> than hardware, while transfer data size and transfer rate on IP bus decide when the
> dma interrupt happen.
>
But it isn't the hardware that decides to drop the rpm refcount to 0
and starting the autosuspend timer, it's the driver.
> Generally, we can call pm_runtime_get_sync(fsl_chan->dev)/
> pm_runtime_mark_last_busy in interrupt handler to hope the runtime_auto_suspend
> timer expiry later than interrupt coming, but if the transfer data size is larger in cyclic
> and transfer rate is very slow like 115200 or lower on uart, the fix autosuspend timer
> 100ms/200ms maybe not enough, hence, runtime suspend may execute meanwhile
> the dma interrupt maybe triggered and caught by GIC(but interrupt handler prevent
> by spin_lock_irqsave in pm_suspend_timer_fn() ), and then interrupt handler start
> to run after runtime suspend.
If your driver code drops the rpm refcount to 0 and starts the
autosuspend timer while a cyclic transfer is still in flight this is
clearly a bug. Autosuspend is not there to paper over driver bugs, but
to amortize cost of actually suspending and resuming the hardware. Your
driver code must still work even if the timeout is 0, i.e. the hardware
is immediately suspended after you drop the rpm refcount to 0.
If you still have transfers queued/in-flight the driver code must keep
a rpm reference.
Regards,
Lucas
^ permalink raw reply
* [PATCH] powerpc/52xx: Fix an invalid ASM expression ('addi' used instead of 'add')
From: Christophe Leroy @ 2021-04-21 17:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
AS arch/powerpc/platforms/52xx/lite5200_sleep.o
arch/powerpc/platforms/52xx/lite5200_sleep.S: Assembler messages:
arch/powerpc/platforms/52xx/lite5200_sleep.S:184: Warning: invalid register expression
In the following code, 'addi' is wrong, has to be 'add'
/* local udelay in sram is needed */
udelay: /* r11 - tb_ticks_per_usec, r12 - usecs, overwrites r13 */
mullw r12, r12, r11
mftb r13 /* start */
addi r12, r13, r12 /* end */
Fixes: ee983079ce04 ("[POWERPC] MPC5200 low power mode")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/platforms/52xx/lite5200_sleep.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/52xx/lite5200_sleep.S b/arch/powerpc/platforms/52xx/lite5200_sleep.S
index 11475c58ea43..afee8b1515a8 100644
--- a/arch/powerpc/platforms/52xx/lite5200_sleep.S
+++ b/arch/powerpc/platforms/52xx/lite5200_sleep.S
@@ -181,7 +181,7 @@ sram_code:
udelay: /* r11 - tb_ticks_per_usec, r12 - usecs, overwrites r13 */
mullw r12, r12, r11
mftb r13 /* start */
- addi r12, r13, r12 /* end */
+ add r12, r13, r12 /* end */
1:
mftb r13 /* current */
cmp cr0, r13, r12
--
2.25.0
^ permalink raw reply related
* [PATCH v2] powerpc/kconfig: Restore alphabetic order of the selects under CONFIG_PPC
From: Christophe Leroy @ 2021-04-21 17:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Commit a7d2475af7ae ("powerpc: Sort the selects under CONFIG_PPC")
sorted all selects under CONFIG_PPC.
4 years later, several items have been introduced at wrong place,
a few other have been renamed without moving them to their correct
place.
Reorder them now.
While we are at it, simplify the test for a couple of them:
- PPC_64 && PPC_PSERIES is simplified in PPC_PSERIES
- PPC_64 && PPC_BOOK3S is simplified in PPC_BOOK3S_64
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Rebased on d20f726744a0 ("Automatic merge of 'next' into merge (2021-04-21 22:57)")
---
arch/powerpc/Kconfig | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index d2e31a578e26..b2970c8241d5 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -118,28 +118,29 @@ config PPC
# Please keep this list sorted alphabetically.
#
select ARCH_32BIT_OFF_T if PPC32
+ select ARCH_HAS_COPY_MC if PPC64
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEVMEM_IS_ALLOWED
+ select ARCH_HAS_DMA_MAP_DIRECT if PPC_PSERIES
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
select ARCH_HAS_GCOV_PROFILE_ALL
- select ARCH_HAS_KCOV
select ARCH_HAS_HUGEPD if HUGETLB_PAGE
+ select ARCH_HAS_KCOV
+ select ARCH_HAS_MEMBARRIER_CALLBACKS
+ select ARCH_HAS_MEMBARRIER_SYNC_CORE
select ARCH_HAS_MEMREMAP_COMPAT_ALIGN
select ARCH_HAS_MMIOWB if PPC64
+ select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
select ARCH_HAS_PHYS_TO_DMA
select ARCH_HAS_PMEM_API
- select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
select ARCH_HAS_PTE_DEVMAP if PPC_BOOK3S_64
select ARCH_HAS_PTE_SPECIAL
- select ARCH_HAS_MEMBARRIER_CALLBACKS
- select ARCH_HAS_MEMBARRIER_SYNC_CORE
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
- select ARCH_HAS_COPY_MC if PPC64
select ARCH_HAS_UBSAN_SANITIZE_ALL
select ARCH_HAVE_NMI_SAFE_CMPXCHG
select ARCH_KEEP_MEMBLOCK
@@ -161,9 +162,8 @@ config PPC
select BUILDTIME_TABLE_SORT
select CLONE_BACKWARDS
select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
- select DMA_OPS if PPC64
select DMA_OPS_BYPASS if PPC64
- select ARCH_HAS_DMA_MAP_DIRECT if PPC64 && PPC_PSERIES
+ select DMA_OPS if PPC64
select DYNAMIC_FTRACE if FUNCTION_TRACER
select EDAC_ATOMIC_SCRUB
select EDAC_SUPPORT
@@ -188,18 +188,16 @@ config PPC
select HAVE_ARCH_JUMP_LABEL_RELATIVE
select HAVE_ARCH_KASAN if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KASAN_VMALLOC if PPC32 && PPC_PAGE_SHIFT <= 14
- select HAVE_ARCH_KGDB
select HAVE_ARCH_KFENCE if PPC32
+ select HAVE_ARCH_KGDB
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_NVRAM_OPS
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_TRACEHOOK
select HAVE_ASM_MODVERSIONS
- select HAVE_C_RECORDMCOUNT
- select HAVE_STACKPROTECTOR if PPC64 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r13)
- select HAVE_STACKPROTECTOR if PPC32 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r2)
select HAVE_CONTEXT_TRACKING if PPC64
+ select HAVE_C_RECORDMCOUNT
select HAVE_DEBUG_KMEMLEAK
select HAVE_DEBUG_STACKOVERFLOW
select HAVE_DYNAMIC_FTRACE
@@ -213,10 +211,13 @@ config PPC
select HAVE_FUNCTION_TRACER
select HAVE_GCC_PLUGINS if GCC_VERSION >= 50200 # plugin support on gcc <= 5.1 is buggy on PPC
select HAVE_GENERIC_VDSO
+ select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC_BOOK3S_64 && SMP
+ select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !HAVE_HARDLOCKUP_DETECTOR_ARCH
select HAVE_HW_BREAKPOINT if PERF_EVENTS && (PPC_BOOK3S || PPC_8xx)
select HAVE_IDE
select HAVE_IOREMAP_PROT
select HAVE_IRQ_EXIT_ON_IRQ_STACK
+ select HAVE_IRQ_TIME_ACCOUNTING
select HAVE_KERNEL_GZIP
select HAVE_KERNEL_LZMA if DEFAULT_UIMAGE
select HAVE_KERNEL_LZO if DEFAULT_UIMAGE
@@ -228,25 +229,24 @@ config PPC
select HAVE_LIVEPATCH if HAVE_DYNAMIC_FTRACE_WITH_REGS
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
- select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC64 && PPC_BOOK3S && SMP
select HAVE_OPTPROBES
select HAVE_PERF_EVENTS
select HAVE_PERF_EVENTS_NMI if PPC64
- select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !HAVE_HARDLOCKUP_DETECTOR_ARCH
select HAVE_PERF_REGS
select HAVE_PERF_USER_STACK_DUMP
- select MMU_GATHER_RCU_TABLE_FREE
- select MMU_GATHER_PAGE_SIZE
select HAVE_REGS_AND_STACK_ACCESS_API
select HAVE_RELIABLE_STACKTRACE
+ select HAVE_RSEQ
select HAVE_SOFTIRQ_ON_OWN_STACK
+ select HAVE_STACKPROTECTOR if PPC32 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r2)
+ select HAVE_STACKPROTECTOR if PPC64 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r13)
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_VIRT_CPU_ACCOUNTING
- select HAVE_IRQ_TIME_ACCOUNTING
- select HAVE_RSEQ
select IOMMU_HELPER if PPC64
select IRQ_DOMAIN
select IRQ_FORCED_THREADING
+ select MMU_GATHER_PAGE_SIZE
+ select MMU_GATHER_RCU_TABLE_FREE
select MODULES_USE_ELF_RELA
select NEED_DMA_MAP_STATE if PPC64 || NOT_COHERENT_CACHE
select NEED_SG_DMA_LENGTH
--
2.25.0
^ permalink raw reply related
* [PATCH v2 2/2] powerpc: If kexec_build_elf_info() fails return immediately from elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-21 16:36 UTC (permalink / raw)
To: robh, dan.carpenter, mpe
Cc: devicetree, kbuild-all, lkp, nramas, linuxppc-dev, bauerman, dja
In-Reply-To: <20210421163610.23775-1-nramas@linux.microsoft.com>
Uninitialized local variable "elf_info" would be passed to
kexec_free_elf_info() if kexec_build_elf_info() returns an error
in elf64_load().
If kexec_build_elf_info() returns an error, return the error
immediately.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kexec/elf_64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 02662e72c53d..eeb258002d1e 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -45,7 +45,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
if (ret)
- goto out;
+ return ERR_PTR(ret);
if (image->type == KEXEC_TYPE_CRASH) {
/* min & max buffer values for kdump case */
--
2.31.0
^ permalink raw reply related
* [PATCH v2 1/2] powerpc: Free fdt on error in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-21 16:36 UTC (permalink / raw)
To: robh, dan.carpenter, mpe
Cc: devicetree, kbuild-all, lkp, nramas, linuxppc-dev, bauerman, dja
There are a few "goto out;" statements before the local variable "fdt"
is initialized through the call to of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being passed
to kvfree() in this function if there is an error before the call to
of_kexec_alloc_and_setup_fdt().
If there is any error after fdt is allocated, but before it is
saved in the arch specific kimage struct, free the fdt.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
arch/powerpc/kexec/elf_64.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
- goto out;
+ goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer(&kbuf);
if (ret)
- goto out;
+ goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+ goto out;
+
+out_free_fdt:
+ kvfree(fdt);
out:
kfree(modified_cmdline);
kexec_free_elf_info(&elf_info);
- /*
- * Once FDT buffer has been successfully passed to kexec_add_buffer(),
- * the FDT buffer address is saved in image->arch.fdt. In that case,
- * the memory cannot be freed here in case of any other error.
- */
- if (ret && !image->arch.fdt)
- kvfree(fdt);
-
return ret ? ERR_PTR(ret) : NULL;
}
--
2.31.0
^ permalink raw reply related
* [PATCH] powerpc/64s: Fix mm_cpumask memory ordering comment
From: Nicholas Piggin @ 2021-04-21 15:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
The memory ordering comment no longer applies, because mm_ctx_id is
no longer used anywhere. At best always been difficult to follow.
It's better to consider the load on which the slbmte depends on, which
the MMU depends on before it can start loading TLBs, rather than a
store which may or may not have a subsequent dependency chain to the
slbmte.
So update the comment and we use the load of the mm's user context ID.
This is much more analogous the radix ordering too, which is good.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/mmu_context.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/mm/mmu_context.c b/arch/powerpc/mm/mmu_context.c
index 18f20da0d348..a857af401738 100644
--- a/arch/powerpc/mm/mmu_context.c
+++ b/arch/powerpc/mm/mmu_context.c
@@ -43,24 +43,26 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
/*
* This full barrier orders the store to the cpumask above vs
- * a subsequent operation which allows this CPU to begin loading
- * translations for next.
+ * a subsequent load which allows this CPU/MMU to begin loading
+ * translations for 'next' from page table PTEs into the TLB.
*
- * When using the radix MMU that operation is the load of the
+ * When using the radix MMU, that operation is the load of the
* MMU context id, which is then moved to SPRN_PID.
*
* For the hash MMU it is either the first load from slb_cache
- * in switch_slb(), and/or the store of paca->mm_ctx_id in
- * copy_mm_to_paca().
+ * in switch_slb() to preload the SLBs, or the load of
+ * get_user_context which loads the context for the VSID hash
+ * to insert a new SLB, in the SLB fault handler.
*
* On the other side, the barrier is in mm/tlb-radix.c for
- * radix which orders earlier stores to clear the PTEs vs
- * the load of mm_cpumask. And pte_xchg which does the same
- * thing for hash.
+ * radix which orders earlier stores to clear the PTEs before
+ * the load of mm_cpumask to check which CPU TLBs should be
+ * flushed. For hash, pte_xchg to clear the PTE includes the
+ * barrier.
*
- * This full barrier is needed by membarrier when switching
- * between processes after store to rq->curr, before user-space
- * memory accesses.
+ * This full barrier is also needed by membarrier when
+ * switching between processes after store to rq->curr, before
+ * user-space memory accesses.
*/
smp_mb();
--
2.23.0
^ permalink raw reply related
* RE: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
From: Robin Gong @ 2021-04-21 14:54 UTC (permalink / raw)
To: Lucas Stach, Shengjiu Wang
Cc: sumit.semwal@linaro.org, linaro-mm-sig@lists.linaro.org,
Linux-ALSA, linuxppc-dev@lists.ozlabs.org, Timur Tabi, Xiubo Li,
Fabio Estevam, s.hauer@pengutronix.de, Takashi Iwai,
Liam Girdwood, dri-devel@lists.freedesktop.org, linux-kernel,
Nicolin Chen, Mark Brown, dl-linux-imx, kernel@pengutronix.de,
perex@perex.cz, shawnguo@kernel.org, S.j. Wang,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <d8d084aa7ff183e2f78128a46a0ce5241f357c9a.camel@pengutronix.de>
On 20201/04/20 22:01 Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Dienstag, dem 20.04.2021 um 13:47 +0000 schrieb Robin Gong:
> > On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong:
> > > > Hi Lucas,
> > > >
> > > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > Hi Robin,
> > > > >
> > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong:
> > > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu
> Wang:
> > > > > > > > Hi
> > > > > > > >
> > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > > > > > > > <l.stach@pengutronix.de>
> > > > > > > wrote:
> > > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu
> Wang:
> > > > > > > > > > There are two requirements that we need to move the
> > > > > > > > > > request of dma channel from probe to open.
> > > > > > > > >
> > > > > > > > > How do you handle -EPROBE_DEFER return code from the
> > > > > > > > > channel request if you don't do it in probe?
> > > > > > > >
> > > > > > > > I use the dma_request_slave_channel or dma_request_channel
> > > > > > > > instead of dmaengine_pcm_request_chan_of. so there should
> > > > > > > > be not -EPROBE_DEFER return code.
> > > > > > >
> > > > > > > This is a pretty weak argument. The dmaengine device might
> > > > > > > probe after you try to get the channel. Using a function to
> > > > > > > request the channel that doesn't allow you to handle probe
> > > > > > > deferral is IMHO a bug and should be fixed, instead of
> > > > > > > building even more assumptions on top
> > > > > of it.
> > > > > > >
> > > > > > > > > > - When dma device binds with power-domains, the power
> > > > > > > > > > will be enabled when we request dma channel. If the
> > > > > > > > > > request of dma channel happen on probe, then the
> > > > > > > > > > power-domains will be always enabled after kernel boot
> > > > > > > > > > up, which is not good for power saving, so we need
> > > > > > > > > > to move the request of dma channel to .open();
> > > > > > > > >
> > > > > > > > > This is certainly something which could be fixed in the
> > > > > > > > > dmaengine driver.
> > > > > > > >
> > > > > > > > Dma driver always call the pm_runtime_get_sync in
> > > > > > > > device_alloc_chan_resources, the
> > > > > > > > device_alloc_chan_resources is called when channel is
> > > > > > > > requested. so power is enabled on channel
> > > > > request.
> > > > > > >
> > > > > > > So why can't you fix the dmaengine driver to do that RPM
> > > > > > > call at a later time when the channel is actually going to
> > > > > > > be used? This will allow further power savings with other
> > > > > > > slave devices than the audio
> > > PCM.
> > > > > > Hi Lucas,
> > > > > > Thanks for your suggestion. I have tried to implement
> > > > > > runtime autosuspend in fsl-edma driver on i.mx8qm/qxp with
> > > > > > delay time (2
> > > > > > sec) for this feature as below (or you can refer to
> > > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> > > > > > pm_runtime_put_autosuspend in all dmaengine driver interface
> > > > > > like
> > > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_d
> > > > > > ma_c
> > > > > > ycli
> > > > > > c/
> > > > > > device_tx_status...
> > > > > >
> > > > > >
> > > > > > pm_runtime_use_autosuspend(fsl_chan->de
> v);
> > > > > > pm_runtime_set_autosuspend_delay(fsl_cha
> n->
> > > dev,
> > > > > 2000);
> > > > > >
> > > > > > That could resolve this audio case since the autosuspend could
> > > > > > suspend runtime after
> > > > > > 2 seconds if there is no further dma transfer but only channel
> > > > > request(device_alloc_chan_resources).
> > > > > > But unfortunately, it cause another issue. As you know, on our
> > > > > > i.mx8qm/qxp, power domain done by scfw
> > > > > > (drivers/firmware/imx/scu-pd.c)
> > > > > over mailbox:
> > > > > > imx_sc_pd_power()->imx_scu_call_rpc()->
> > > > > > imx_scu_ipc_write()->mbox_send_message()
> > > > > > which means have to 'waits for completion', meanwhile, some
> > > > > > driver like tty will call dmaengine interfaces in non-atomic
> > > > > > case as below,
> > > > > >
> > > > > > static int uart_write(struct tty_struct *tty, const unsigned
> > > > > > char *buf, int count) {
> > > > > > .......
> > > > > > port = uart_port_lock(state, flags);
> > > > > > ......
> > > > > > __uart_start(tty); //call
> > > start_tx()->dmaengine_prep_slave_sg...
> > > > > > uart_port_unlock(port, flags);
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > Thus dma runtime resume may happen in that timing window and
> > > > > > cause
> > > > > kernel alarm.
> > > > > > I'm not sure whether there are similar limitations on other
> > > > > > driver subsystem. But for me, It looks like the only way to
> > > > > > resolve the contradiction between tty and scu-pd (hardware
> > > > > > limitation on
> > > > > > i.mx8qm/qxp) is to give up autosuspend and keep
> > > > > > pm_runtime_get_sync
> > > > > only in device_alloc_chan_resources because request channel is a
> > > > > safe non-atomic phase.
> > > > > > Do you have any idea? Thanks in advance.
> > > > >
> > > > > If you look closely at the driver you used as an example
> > > > > (hidma.c) it looks like there is already something in there,
> > > > > which looks very much like what you need
> > > > > here:
> > > > >
> > > > > In hidma_issue_pending() the driver tries to get the device to
> > > > > runtime
> > > resume.
> > > > > If this doesn't work, maybe due to the power domain code not
> > > > > being able to be called in atomic context, the actual work of
> > > > > waking up the dma hardware and issuing the descriptor is shunted to a
> tasklet.
> > > > >
> > > > > If I'm reading this right, this is exactly what you need here to
> > > > > be able to call the dmaengine code from atomic context: try the
> > > > > rpm get and issue immediately when possible, otherwise shunt the
> > > > > work to a
> > > > > non- atomic context where you can deal with the requirements of
> scu-pd.
> > > > Yes, I can schedule_work to worker to runtime resume edma channel
> > > > by
> > > calling scu-pd.
> > > > But that means all dmaengine interfaces should be taken care, not
> > > > only
> > > > issue_pending() but also
> > > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/
> > > > dmaengine_tx_status(). Not sure why hidma only take care
> > > > issue_pending. Maybe their user case is just for memcpy/memset so
> > > > that no further complicate case as ALSA or TTY.
> > > > Besides, for autosuspend in cyclic, we have to add
> > > > pm_runtime_get_sync into interrupt handler as qcom/bam_dma.c. but
> > > > how could resolve the scu-pd's non-atmoic limitation in interrupt
> handler?
> > >
> > > Sure, this all needs some careful analysis on how those functions
> > > are called and what to do about atomic callers, but it should be
> > > doable. I don't see any fundamental issues here.
> > >
> > > I don't see why you would ever need to wake the hardware in an
> > > interrupt handler. Surely the hardware is already awake, as it
> > > wouldn't signal an interrupt otherwise. And for the issue with
> > > scu-pd you only care about the state transition of
> > > suspended->running. If the hardware is already running/awake, the
> > > runtime pm state handling is nothing more than bumping a refcount,
> > > which is atomic safe. Putting the HW in suspend is already handled
> asynchronously in a worker, so this is also atomic safe.
> > But with autosuspend used, in corner case, may runtime suspended
> > before falling Into edma interrupt handler if timeout happen with the
> > delay value of pm_runtime_set_autosuspend_delay(). Thus, can't touch
> > any edma interrupt status register unless runtime resume edma in
> > interrupt handler while runtime resume function based on scu-pd's power
> domain may block or sleep.
> > I have a simple workaround that disable runtime suspend in
> > issue_pending worker by calling pm_runtime_forbid() and then enable
> > runtime auto suspend in dmaengine_terminate_all so that we could
> > easily regard that edma channel is always in runtime resume between
> > issue_pending and channel terminated and ignore the above interrupt
> handler/scu-pd limitation.
>
> The IRQ handler is the point where you are informed by the hardware that a
> specific operation is complete. I don't see any use-case where it would be valid
> to drop the rpm refcount to 0 before the IRQ is handled. Surely the hardware
> needs to stay awake until the currently queued operations are complete and if
> the IRQ handler is the completion point the IRQ handler is the first point in
> time where your autosuspend timer should start to run. There should never be
> a situation where the timer expiry can get between IRQ signaling and the
> handler code running.
But the timer of runtime_auto_suspend decide when enter runtime suspend rather
than hardware, while transfer data size and transfer rate on IP bus decide when the
dma interrupt happen. Generally, we can call pm_runtime_get_sync(fsl_chan->dev)/
pm_runtime_mark_last_busy in interrupt handler to hope the runtime_auto_suspend
timer expiry later than interrupt coming, but if the transfer data size is larger in cyclic
and transfer rate is very slow like 115200 or lower on uart, the fix autosuspend timer
100ms/200ms maybe not enough, hence, runtime suspend may execute meanwhile
the dma interrupt maybe triggered and caught by GIC(but interrupt handler prevent
by spin_lock_irqsave in pm_suspend_timer_fn() ), and then interrupt handler start
to run after runtime suspend.
^ permalink raw reply
* Re: [PATCH 1/1] of/pci: Add IORESOURCE_MEM_64 to resource flags for 64-bit memory addresses
From: Leonardo Bras @ 2021-04-21 14:14 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Alexey Kardashevskiy, Frank Rowand,
linux-kernel@vger.kernel.org, PCI, linuxppc-dev
In-Reply-To: <CAL_JsqJXKVUFh9KrJjobn-jE-PFKN0w-V_i3qkfBrpTah4g8Xw@mail.gmail.com>
On Tue, 2021-04-20 at 17:34 -0500, Rob Herring wrote:
> > [...]
> > I think the point here is bus resources not getting the MEM_64 flag,
> > but device resources getting it correctly. Is that supposed to happen?
>
> I experimented with this on Arm with qemu and it seems fine there too.
> Looks like the BARs are first read and will have bit 2 set by default
> (or hardwired?). Now I'm just wondering why powerpc needs the code it
> has...
>
> Anyways, I'll apply the patch.
>
> Rob
Thanks Rob!
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Free fdt on error in elf64_load()
From: Santosh Sivaraj @ 2021-04-21 14:02 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, robh, dan.carpenter, mpe
Cc: devicetree, kbuild-all, lkp, linuxppc-dev, bauerman, dja
In-Reply-To: <407a9f66-8f91-9c6e-9653-738ba79a97b2@linux.microsoft.com>
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> On 4/20/21 10:35 PM, Santosh Sivaraj wrote:
> Hi Santosh,
>
>>
>>> There are a few "goto out;" statements before the local variable "fdt"
>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>>> elf64_load(). This will result in an uninitialized "fdt" being passed
>>> to kvfree() in this function if there is an error before the call to
>>> of_kexec_alloc_and_setup_fdt().
>>>
>>> If there is any error after fdt is allocated, but before it is
>>> saved in the arch specific kimage struct, free the fdt.
>>>
>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> arch/powerpc/kexec/elf_64.c | 16 ++++++----------
>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>>> index 5a569bb51349..02662e72c53d 100644
>>> --- a/arch/powerpc/kexec/elf_64.c
>>> +++ b/arch/powerpc/kexec/elf_64.c
>>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>>> initrd_len, cmdline);
>>> if (ret)
>>> - goto out;
>>> + goto out_free_fdt;
>>
>> Shouldn't there be a goto out_free_fdt if fdt_open_into fails?
>
> You are likely looking at elf_64.c in the mainline branch. The patch I
> have submitted is based on Rob's device-tree for-next branch. Please see
> the link below:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
That's right, I was indeed looking at the mainline. Sorry for the noise.
Thanks,
Santosh
>
>>
>>>
>>> fdt_pack(fdt);
>>>
>>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>> ret = kexec_add_buffer(&kbuf);
>>> if (ret)
>>> - goto out;
>>> + goto out_free_fdt;
>>>
>>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>>> image->arch.fdt = fdt;
>>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>> if (ret)
>>> pr_err("Error setting up the purgatory.\n");
>>>
>>> + goto out;
>>> +
>>> +out_free_fdt:
>>> + kvfree(fdt);
>>
>> Can just use kfree here?
> "fdt" is allocated through kvmalloc(). So it is freed using kvfree.
>
> thanks,
> -lakshmi
>
>>> out:
>>> kfree(modified_cmdline);
>>> kexec_free_elf_info(&elf_info);
>>>
>>> - /*
>>> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
>>> - * the FDT buffer address is saved in image->arch.fdt. In that case,
>>> - * the memory cannot be freed here in case of any other error.
>>> - */
>>> - if (ret && !image->arch.fdt)
>>> - kvfree(fdt);
>>> -
>>> return ret ? ERR_PTR(ret) : NULL;
>>> }
>>>
>>> --
>>> 2.31.0
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Free fdt on error in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-21 14:01 UTC (permalink / raw)
To: Michael Ellerman, robh, dan.carpenter
Cc: devicetree, kbuild-all, lkp, linuxppc-dev, bauerman, dja
In-Reply-To: <87r1j4aywp.fsf@mpe.ellerman.id.au>
On 4/21/21 12:18 AM, Michael Ellerman wrote:
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>> There are a few "goto out;" statements before the local variable "fdt"
>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>> elf64_load(). This will result in an uninitialized "fdt" being passed
>> to kvfree() in this function if there is an error before the call to
>> of_kexec_alloc_and_setup_fdt().
>>
>> If there is any error after fdt is allocated, but before it is
>> saved in the arch specific kimage struct, free the fdt.
>>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>
> I basically sent you the diff, so this should probably be:
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>
> Otherwise looks good to me, thanks for turning it into a proper patch
> and submitting it.
I will submit the patch again with the above change.
Thanks for reviewing the patch.
Could you please review [PATCH 2/2] as well?
thanks,
-lakshmi
>
> cheers
>
>
>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>> index 5a569bb51349..02662e72c53d 100644
>> --- a/arch/powerpc/kexec/elf_64.c
>> +++ b/arch/powerpc/kexec/elf_64.c
>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>> initrd_len, cmdline);
>> if (ret)
>> - goto out;
>> + goto out_free_fdt;
>>
>> fdt_pack(fdt);
>>
>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> ret = kexec_add_buffer(&kbuf);
>> if (ret)
>> - goto out;
>> + goto out_free_fdt;
>>
>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>> image->arch.fdt = fdt;
>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> if (ret)
>> pr_err("Error setting up the purgatory.\n");
>>
>> + goto out;
>> +
>> +out_free_fdt:
>> + kvfree(fdt);
>> out:
>> kfree(modified_cmdline);
>> kexec_free_elf_info(&elf_info);
>>
>> - /*
>> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
>> - * the FDT buffer address is saved in image->arch.fdt. In that case,
>> - * the memory cannot be freed here in case of any other error.
>> - */
>> - if (ret && !image->arch.fdt)
>> - kvfree(fdt);
>> -
>> return ret ? ERR_PTR(ret) : NULL;
>> }
>>
>> --
>> 2.31.0
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Free fdt on error in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-21 13:58 UTC (permalink / raw)
To: Santosh Sivaraj, robh, dan.carpenter, mpe
Cc: devicetree, kbuild-all, lkp, linuxppc-dev, bauerman, dja
In-Reply-To: <871rb4yzca.fsf@fossix.org>
On 4/20/21 10:35 PM, Santosh Sivaraj wrote:
Hi Santosh,
>
>> There are a few "goto out;" statements before the local variable "fdt"
>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>> elf64_load(). This will result in an uninitialized "fdt" being passed
>> to kvfree() in this function if there is an error before the call to
>> of_kexec_alloc_and_setup_fdt().
>>
>> If there is any error after fdt is allocated, but before it is
>> saved in the arch specific kimage struct, free the fdt.
>>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> arch/powerpc/kexec/elf_64.c | 16 ++++++----------
>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>> index 5a569bb51349..02662e72c53d 100644
>> --- a/arch/powerpc/kexec/elf_64.c
>> +++ b/arch/powerpc/kexec/elf_64.c
>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>> initrd_len, cmdline);
>> if (ret)
>> - goto out;
>> + goto out_free_fdt;
>
> Shouldn't there be a goto out_free_fdt if fdt_open_into fails?
You are likely looking at elf_64.c in the mainline branch. The patch I
have submitted is based on Rob's device-tree for-next branch. Please see
the link below:
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>
>>
>> fdt_pack(fdt);
>>
>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> ret = kexec_add_buffer(&kbuf);
>> if (ret)
>> - goto out;
>> + goto out_free_fdt;
>>
>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>> image->arch.fdt = fdt;
>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> if (ret)
>> pr_err("Error setting up the purgatory.\n");
>>
>> + goto out;
>> +
>> +out_free_fdt:
>> + kvfree(fdt);
>
> Can just use kfree here?
"fdt" is allocated through kvmalloc(). So it is freed using kvfree.
thanks,
-lakshmi
>> out:
>> kfree(modified_cmdline);
>> kexec_free_elf_info(&elf_info);
>>
>> - /*
>> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
>> - * the FDT buffer address is saved in image->arch.fdt. In that case,
>> - * the memory cannot be freed here in case of any other error.
>> - */
>> - if (ret && !image->arch.fdt)
>> - kvfree(fdt);
>> -
>> return ret ? ERR_PTR(ret) : NULL;
>> }
>>
>> --
>> 2.31.0
^ permalink raw reply
* Re: [PATCH v1] KVM: PPC: Book3S HV P9: implement kvmppc_xive_pull_vcpu in C
From: Cédric Le Goater @ 2021-04-21 13:31 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: Alexey Kardashevskiy, linuxppc-dev
In-Reply-To: <20210413133802.1691660-1-npiggin@gmail.com>
On 4/13/21 3:38 PM, Nicholas Piggin wrote:
> This is more symmetric with kvmppc_xive_push_vcpu, and has the advantage
> that it runs with the MMU on.
>
> The extra test added to the asm will go away with a future change.
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Another bit that came from the KVM Cify series.
>
> Thanks,
> Nick
>
> arch/powerpc/include/asm/kvm_ppc.h | 2 ++
> arch/powerpc/kvm/book3s_hv.c | 2 ++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 ++++
> arch/powerpc/kvm/book3s_xive.c | 31 +++++++++++++++++++++++++
> 4 files changed, 40 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 9531b1c1b190..73b1ca5a6471 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -672,6 +672,7 @@ extern int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
> extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> int level, bool line_status);
> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
> +extern void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu);
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> {
> @@ -712,6 +713,7 @@ static inline int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval) { retur
> static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> int level, bool line_status) { return -ENODEV; }
> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
> +static inline void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu) { }
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> { return 0; }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 4a532410e128..981bcaf787a8 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3570,6 +3570,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>
> trap = __kvmhv_vcpu_entry_p9(vcpu);
>
> + kvmppc_xive_pull_vcpu(vcpu);
> +
> /* Advance host PURR/SPURR by the amount used by guest */
> purr = mfspr(SPRN_PURR);
> spurr = mfspr(SPRN_SPURR);
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 75405ef53238..c11597f815e4 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1442,6 +1442,11 @@ guest_exit_cont: /* r9 = vcpu, r12 = trap, r13 = paca */
> bl kvmhv_accumulate_time
> #endif
> #ifdef CONFIG_KVM_XICS
> + /* If we came in through the P9 short path, xive pull is done in C */
> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
> + cmpwi r0, 0
> + bne 1f
> +
> /* We are exiting, pull the VP from the XIVE */
> lbz r0, VCPU_XIVE_PUSHED(r9)
> cmpwi cr0, r0, 0
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index e7219b6f5f9a..741bf1f4387a 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -127,6 +127,37 @@ void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu)
> }
> EXPORT_SYMBOL_GPL(kvmppc_xive_push_vcpu);
>
> +/*
> + * Pull a vcpu's context from the XIVE on guest exit.
> + * This assumes we are in virtual mode (MMU on)
> + */
> +void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu)
> +{
> + void __iomem *tima = local_paca->kvm_hstate.xive_tima_virt;
> +
> + if (!vcpu->arch.xive_pushed)
> + return;
> +
> + /*
> + * Should not have been pushed if there is no tima
> + */
> + if (WARN_ON(!tima))
> + return;
> +
> + eieio();
> + /* First load to pull the context, we ignore the value */
> + __raw_readl(tima + TM_SPC_PULL_OS_CTX);
> + /* Second load to recover the context state (Words 0 and 1) */
> + vcpu->arch.xive_saved_state.w01 = __raw_readq(tima + TM_QW1_OS);
This load could be removed on P10, since HW is configured to do the same.
It should save a few cycles.
C.
> + /* Fixup some of the state for the next load */
> + vcpu->arch.xive_saved_state.lsmfb = 0;
> + vcpu->arch.xive_saved_state.ack = 0xff;
> + vcpu->arch.xive_pushed = 0;
> + eieio();
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_xive_pull_vcpu);
> +
> /*
> * This is a simple trigger for a generic XIVE IRQ. This must
> * only be called for interrupts that support a trigger page
>
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Add shutdown() to vio_driver and vio_bus
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Tyrel Datwyler, mpe; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210402001325.939668-1-tyreld@linux.ibm.com>
On Thu, 1 Apr 2021 18:13:25 -0600, Tyrel Datwyler wrote:
> Currently, neither the vio_bus or vio_driver structures provide support
> for a shutdown() routine.
>
> Add support for shutdown() by allowing drivers to provide a
> implementation via function pointer in their vio_driver struct and
> provide a proper implementation in the driver template for the vio_bus
> that calls a vio drivers shutdown() if defined.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/pseries: Add shutdown() to vio_driver and vio_bus
https://git.kernel.org/powerpc/c/39d0099f94390eb7a677e1a5c9bb56a4daa242a1
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Stop calling printk in rtas_stop_self()
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
In-Reply-To: <20210418135413.1204031-1-mpe@ellerman.id.au>
On Sun, 18 Apr 2021 23:54:13 +1000, Michael Ellerman wrote:
> RCU complains about us calling printk() from an offline CPU:
>
> =============================
> WARNING: suspicious RCU usage
> 5.12.0-rc7-02874-g7cf90e481cb8 #1 Not tainted
> -----------------------------
> kernel/locking/lockdep.c:3568 RCU-list traversed in non-reader section!!
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/pseries: Stop calling printk in rtas_stop_self()
https://git.kernel.org/powerpc/c/ed8029d7b472369a010a1901358567ca3b6dbb0d
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Only define _TASK_CPU for 32-bit
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
In-Reply-To: <20210418131641.1186227-1-mpe@ellerman.id.au>
On Sun, 18 Apr 2021 23:16:41 +1000, Michael Ellerman wrote:
> We have some interesting code in our Makefile to define _TASK_CPU, based
> on awk'ing the value out of asm-offsets.h. It exists to circumvent some
> circular header dependencies that prevent us from referring to
> task_struct in the relevant code. See the comment around _TASK_CPU in
> smp.h for more detail.
>
> Maybe one day we can come up with a better solution, but for now we can
> at least limit that logic to 32-bit, because it's not needed for 64-bit.
Applied to powerpc/next.
[1/1] powerpc: Only define _TASK_CPU for 32-bit
https://git.kernel.org/powerpc/c/3027a37c06be364e6443d3df3adf45576fba50cb
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/kvm: Fix PR KVM with KUAP/MEM_KEYS enabled
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: aneesh.kumar, kvm-ppc
In-Reply-To: <20210419120139.1455937-1-mpe@ellerman.id.au>
On Mon, 19 Apr 2021 22:01:39 +1000, Michael Ellerman wrote:
> The changes to add KUAP support with the hash MMU broke booting of KVM
> PR guests. The symptom is no visible progress of the guest, or possibly
> just "SLOF" being printed to the qemu console.
>
> Host code is still executing, but breaking into xmon might show a stack
> trace such as:
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/kvm: Fix PR KVM with KUAP/MEM_KEYS enabled
https://git.kernel.org/powerpc/c/e4e8bc1df691ba5ba749d1e2b67acf9827e51a35
cheers
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/perf: Add platform specific check_attr_config
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Madhavan Srinivasan, mpe; +Cc: linuxppc-dev
In-Reply-To: <20210408074504.248211-2-maddy@linux.ibm.com>
On Thu, 8 Apr 2021 13:15:04 +0530, Madhavan Srinivasan wrote:
> Add platform specific attr.config value checks. Patch
> includes checks for both power9 and power10.
Applied to powerpc/next.
[2/2] powerpc/perf: Add platform specific check_attr_config
https://git.kernel.org/powerpc/c/d8a1d6c58986d8778768b15dc5bac0b4b082d345
cheers
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/pseries/iommu: Fix window size for direct mapping with pmem
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Alexey Kardashevskiy,
Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210420045404.438735-1-leobras.c@gmail.com>
On Tue, 20 Apr 2021 01:54:04 -0300, Leonardo Bras wrote:
> As of today, if the DDW is big enough to fit (1 << MAX_PHYSMEM_BITS) it's
> possible to use direct DMA mapping even with pmem region.
>
> But, if that happens, the window size (len) is set to
> (MAX_PHYSMEM_BITS - page_shift) instead of MAX_PHYSMEM_BITS, causing a
> pagesize times smaller DDW to be created, being insufficient for correct
> usage.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/pseries/iommu: Fix window size for direct mapping with pmem
https://git.kernel.org/powerpc/c/a9d2f9bb225fd2a764aef57738ab6c7f38d782ae
cheers
^ permalink raw reply
* Re: [PATCH] Documentation/powerpc: Add proper links for manual and tests
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Haren Myneni, mpe, linuxppc-dev; +Cc: abali
In-Reply-To: <08511c1e92ac239f20ac88c73c59d1f8cf02e6ad.camel@linux.ibm.com>
On Sun, 18 Apr 2021 12:29:42 -0700, Haren Myneni wrote:
> The links that are mentioned in this document are no longer
> valid. So changed the proper links for NXGZIP user manual and
> test cases.
Applied to powerpc/next.
[1/1] Documentation/powerpc: Add proper links for manual and tests
https://git.kernel.org/powerpc/c/2886e2df10beaf50352dad7a90907251bc692029
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/pseries/mce: Fix a typo in error type assignment
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Ganesh Goudar, mpe, linuxppc-dev; +Cc: mahesh, npiggin
In-Reply-To: <20210416125750.49550-1-ganeshgr@linux.ibm.com>
On Fri, 16 Apr 2021 18:27:50 +0530, Ganesh Goudar wrote:
> The error type is ICACHE and DCACHE, for case MCE_ERROR_TYPE_ICACHE.
Applied to powerpc/next.
[1/1] powerpc/pseries/mce: Fix a typo in error type assignment
https://git.kernel.org/powerpc/c/864ec4d40c83365b16483d88990e7e579537635c
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/mce: save ignore_event flag unconditionally for UE
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Ganesh Goudar, mpe, linuxppc-dev; +Cc: mahesh, npiggin
In-Reply-To: <20210407045816.352276-1-ganeshgr@linux.ibm.com>
On Wed, 7 Apr 2021 10:28:16 +0530, Ganesh Goudar wrote:
> When we hit an UE while using machine check safe copy routines,
> ignore_event flag is set and the event is ignored by mce handler,
> And the flag is also saved for defered handling and printing of
> mce event information, But as of now saving of this flag is done
> on checking if the effective address is provided or physical address
> is calculated, which is not right.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/mce: save ignore_event flag unconditionally for UE
https://git.kernel.org/powerpc/c/92d9d61be519f32f16c07602db5bcbe30a0836fe
cheers
^ permalink raw reply
* Re: [PATCH 0/2] pseries: UNISOLATE DRCs to signal device removal error
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Daniel Henrique Barboza, linuxppc-dev; +Cc: david
In-Reply-To: <20210416210216.380291-1-danielhb413@gmail.com>
On Fri, 16 Apr 2021 18:02:14 -0300, Daniel Henrique Barboza wrote:
> At this moment, PAPR [1] does not have a way to report errors during a device
> removal operation. This puts a strain in the hypervisor, which needs extra
> mechanisms to try to fallback and recover from an error that might have
> happened during the removal. The QEMU community has dealt with it during these
> years by either trying to preempt the error before sending the HP event or, in
> case of a guest side failure, reboot the guest to complete the removal process.
>
> [...]
Applied to powerpc/next.
[1/2] dlpar.c: introduce dlpar_unisolate_drc()
https://git.kernel.org/powerpc/c/0e3b3ff83ce24a7a01e467ca42e3e33e87195c0d
[2/2] hotplug-cpu.c: set UNISOLATE on dlpar_cpu_remove() failure
https://git.kernel.org/powerpc/c/29c9a2699e71a7866a98ebdf6ea38135d31b4e1f
cheers
^ permalink raw reply
* Re: [PATCH v3 1/4] powerpc: Remove probe_user_read_inst()
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Paul Mackerras, jniethe5, Christophe Leroy,
Benjamin Herrenschmidt, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5f6f82572242a59bfee1e19a71194d8f7ef5fca4.1618405715.git.christophe.leroy@csgroup.eu>
On Wed, 14 Apr 2021 13:08:40 +0000 (UTC), Christophe Leroy wrote:
> Its name comes from former probe_user_read() function.
> That function is now called copy_from_user_nofault().
>
> probe_user_read_inst() uses copy_from_user_nofault() to read only
> a few bytes. It is suboptimal.
>
> It does the same as get_user_inst() but in addition disables
> page faults.
>
> [...]
Applied to powerpc/next.
[1/4] powerpc: Remove probe_user_read_inst()
https://git.kernel.org/powerpc/c/6ac7897f08e04b47df3955d7691652e9d12d4068
[2/4] powerpc: Make probe_kernel_read_inst() common to PPC32 and PPC64
https://git.kernel.org/powerpc/c/6449078d50111c839bb7156c3b99b9def80eed42
[3/4] powerpc: Rename probe_kernel_read_inst()
https://git.kernel.org/powerpc/c/41d6cf68b5f611934bcc6a7d4a1a2d9bfd04b420
[4/4] powerpc: Move copy_from_kernel_nofault_inst()
https://git.kernel.org/powerpc/c/39352430aaa05fbe4ba710231c70b334513078f2
cheers
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/inst: ppc_inst_as_u64() becomes ppc_inst_as_ulong()
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, naveen.n.rao,
Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <22cadf29620664b600b82026d2a72b8b23351777.1618927318.git.christophe.leroy@csgroup.eu>
On Tue, 20 Apr 2021 14:02:06 +0000 (UTC), Christophe Leroy wrote:
> In order to simplify use on PPC32, change ppc_inst_as_u64()
> into ppc_inst_as_ulong() that returns the 32 bits instruction
> on PPC32.
>
> Will be used when porting OPTPROBES to PPC32.
Applied to powerpc/next.
[1/2] powerpc/inst: ppc_inst_as_u64() becomes ppc_inst_as_ulong()
https://git.kernel.org/powerpc/c/693557ebf407a85ea400a0b501bb97687d8f4856
[2/2] powerpc: Enable OPTPROBES on PPC32
https://git.kernel.org/powerpc/c/eacf4c0202654adfa94bbb17b5c5c77c0be14af8
cheers
^ permalink raw reply
* Re: [PATCH] selftests: timens: Fix gettime_perf to work on powerpc
From: Michael Ellerman @ 2021-04-21 13:08 UTC (permalink / raw)
To: Shuah Khan, Christophe Leroy, Andrei Vagin, Dmitry Safonov
Cc: linuxppc-dev, linux-kernel, linux-kselftest
In-Reply-To: <469f37ab91984309eb68c0fb47e8438cdf5b6463.1617198956.git.christophe.leroy@csgroup.eu>
On Wed, 31 Mar 2021 13:59:17 +0000 (UTC), Christophe Leroy wrote:
> On powerpc:
> - VDSO library is named linux-vdso32.so.1 or linux-vdso64.so.1
> - clock_gettime is named __kernel_clock_gettime()
>
> Ensure gettime_perf tries these names before giving up.
Applied to powerpc/next.
[1/1] selftests: timens: Fix gettime_perf to work on powerpc
https://git.kernel.org/powerpc/c/f56607e85ee38f2a5bb7096e24e2d40f35d714f9
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