[PATCH] powerpc/eeh: Set channel state after notifying the drivers
When a PCI error is encountered 6th time in an hour we
set the channel state to perm_failure and notify the
driver about the permanent failure.
However, after upstream commit 38ddc011478e ("powerpc/eeh:
Make permanently failed devices non-actionable"), EEH handler
stops calling any routine once the device is marked as
permanent failure. This issue can lead to fatal consequences
like kernel hang with certain PCI devices.
Following log is observed with lpfc driver, with and without
this change, Without this change kernel hangs, If PCI error
is encountered 6 times for a device in an hour.
Without the change
EEH: Beginning: 'error_detected(permanent failure)'
PCI 0132:60:00.0#60: EEH: not actionable (1,1,1)
PCI 0132:60:00.1#60: EEH: not actionable (1,1,1)
EEH: Finished:'error_detected(permanent failure)'
With the change
EEH: Beginning: 'error_detected(permanent failure)'
EEH: Invoking lpfc->error_detected(permanent failure)
EEH: lpfc driver reports: 'disconnect'
EEH: Invoking lpfc->error_detected(permanent failure)
EEH: lpfc driver reports: 'disconnect'
EEH: Finished:'error_detected(permanent failure)'
To fix the issue, set channel state to permanent failure after
notifying the drivers.
Fixes: 38ddc011478e ("powerpc/eeh: Make permanently failed devices
non-actionable")
Suggested-by: Mahesh Salgaonkar
Signed-off-by: Ganesh Goudar
---
arch/powerpc/kernel/eeh_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index f279295179bd..438568a472d0 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -1065,10 +1065,10 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
eeh_slot_error_detail(pe, EEH_LOG_PERM);
/* Notify all devices that they're about to go down. */
- eeh_set_channel_state(pe, pci_channel_io_perm_failure);
eeh_set_irq_state(pe, false);
eeh_pe_report("error_detected(permanent failure)", pe,
eeh_report_failure, NULL);
+ eeh_set_channel_state(pe, pci_channel_io_perm_failure);
/* Mark the PE to be removed permanently */
eeh_pe_state_mark(pe, EEH_PE_REMOVED);
@@ -1185,10 +1185,10 @@ void eeh_handle_special_event(void)
/* Notify all devices to be down */
eeh_pe_state_clear(pe, EEH_PE_PRI_BUS, true);
- eeh_set_channel_state(pe, pci_channel_io_perm_failure);
eeh_pe_report(
"error_detected(permanent failure)", pe,
eeh_report_failure, NULL);
+ eeh_set_channel_state(pe, pci_channel_io_perm_failure);
pci_lock_rescan_remove();
list_for_each_entry(hose, &hose_list, list_node) {
--
2.39.1
[PATCH AUTOSEL 6.1 17/38] powerpc/85xx: Fix unannotated intra-function call warning
From: Sathvika Vasireddy [ Upstream commit 8afffce6aa3bddc940ac1909627ff1e772b6cbf1 ] objtool throws the following warning: arch/powerpc/kernel/head_85xx.o: warning: objtool: .head.text+0x1a6c: unannotated intra-function call Fix the warning by annotating KernelSPE symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END macros. Reported-by: kernel test robot Signed-off-by: Sathvika Vasireddy Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/20230128124138.1066176-1...@linux.ibm.com Signed-off-by: Sasha Levin --- arch/powerpc/kernel/head_85xx.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/head_85xx.S b/arch/powerpc/kernel/head_85xx.S index 52c0ab416326a..d3939849f4550 100644 --- a/arch/powerpc/kernel/head_85xx.S +++ b/arch/powerpc/kernel/head_85xx.S @@ -862,7 +862,7 @@ _GLOBAL(load_up_spe) * SPE unavailable trap from kernel - print a message, but let * the task use SPE in the kernel until it returns to user mode. */ -KernelSPE: +SYM_FUNC_START_LOCAL(KernelSPE) lwz r3,_MSR(r1) orisr3,r3,MSR_SPE@h stw r3,_MSR(r1) /* enable use of SPE after return */ @@ -879,6 +879,7 @@ KernelSPE: #endif .align 4,0 +SYM_FUNC_END(KernelSPE) #endif /* CONFIG_SPE */ /* -- 2.39.0
[PATCH AUTOSEL 6.1 18/38] powerpc/kvm: Fix unannotated intra-function call warning
From: Sathvika Vasireddy
[ Upstream commit fe6de81b610e5d0b9d2231acff2de74a35482e7d ]
objtool throws the following warning:
arch/powerpc/kvm/booke.o: warning: objtool: kvmppc_fill_pt_regs+0x30:
unannotated intra-function call
Fix the warning by setting the value of 'nip' using the _THIS_IP_ macro,
without using an assembly bl/mflr sequence to save the instruction
pointer.
Reported-by: kernel test robot
Suggested-by: Michael Ellerman
Signed-off-by: Sathvika Vasireddy
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230128124158.1066251-1...@linux.ibm.com
Signed-off-by: Sasha Levin
---
arch/powerpc/kvm/booke.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 7b4920e9fd263..3852209989f04 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -912,16 +912,15 @@ static int kvmppc_handle_debug(struct kvm_vcpu *vcpu)
static void kvmppc_fill_pt_regs(struct pt_regs *regs)
{
- ulong r1, ip, msr, lr;
+ ulong r1, msr, lr;
asm("mr %0, 1" : "=r"(r1));
asm("mflr %0" : "=r"(lr));
asm("mfmsr %0" : "=r"(msr));
- asm("bl 1f; 1: mflr %0" : "=r"(ip));
memset(regs, 0, sizeof(*regs));
regs->gpr[1] = r1;
- regs->nip = ip;
+ regs->nip = _THIS_IP_;
regs->msr = msr;
regs->link = lr;
}
--
2.39.0
[PATCH AUTOSEL 6.1 20/38] powerpc/64: Fix perf profiling asynchronous interrupt handlers
From: Nicholas Piggin
[ Upstream commit c28548012ee2bac55772ef7685138bd1124b80c3 ]
Interrupt entry sets the soft mask to IRQS_ALL_DISABLED to match the
hard irq disabled state. So when should_hard_irq_enable() returns true
because we want PMI interrupts in irq handlers, MSR[EE] is enabled but
PMIs just get soft-masked. Fix this by clearing IRQS_PMI_DISABLED before
enabling MSR[EE].
This also tidies some of the warnings, no need to duplicate them in
both should_hard_irq_enable() and do_hard_irq_enable().
Signed-off-by: Nicholas Piggin
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230121100156.2824054-1-npig...@gmail.com
Signed-off-by: Sasha Levin
---
arch/powerpc/include/asm/hw_irq.h | 41 ++-
arch/powerpc/kernel/dbell.c | 2 +-
arch/powerpc/kernel/irq.c | 2 +-
arch/powerpc/kernel/time.c| 2 +-
4 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_irq.h
b/arch/powerpc/include/asm/hw_irq.h
index 77fa88c2aed0d..265d0eb7ed796 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -173,6 +173,15 @@ static inline notrace unsigned long
irq_soft_mask_or_return(unsigned long mask)
return flags;
}
+static inline notrace unsigned long irq_soft_mask_andc_return(unsigned long
mask)
+{
+ unsigned long flags = irq_soft_mask_return();
+
+ irq_soft_mask_set(flags & ~mask);
+
+ return flags;
+}
+
static inline unsigned long arch_local_save_flags(void)
{
return irq_soft_mask_return();
@@ -331,10 +340,11 @@ bool power_pmu_wants_prompt_pmi(void);
* is a different soft-masked interrupt pending that requires hard
* masking.
*/
-static inline bool should_hard_irq_enable(void)
+static inline bool should_hard_irq_enable(struct pt_regs *regs)
{
if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG)) {
- WARN_ON(irq_soft_mask_return() == IRQS_ENABLED);
+ WARN_ON(irq_soft_mask_return() != IRQS_ALL_DISABLED);
+ WARN_ON(!(get_paca()->irq_happened & PACA_IRQ_HARD_DIS));
WARN_ON(mfmsr() & MSR_EE);
}
@@ -347,8 +357,17 @@ static inline bool should_hard_irq_enable(void)
*
* TODO: Add test for 64e
*/
- if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !power_pmu_wants_prompt_pmi())
- return false;
+ if (IS_ENABLED(CONFIG_PPC_BOOK3S_64)) {
+ if (!power_pmu_wants_prompt_pmi())
+ return false;
+ /*
+* If PMIs are disabled then IRQs should be disabled as well,
+* so we shouldn't see this condition, check for it just in
+* case because we are about to enable PMIs.
+*/
+ if (WARN_ON_ONCE(regs->softe & IRQS_PMI_DISABLED))
+ return false;
+ }
if (get_paca()->irq_happened & PACA_IRQ_MUST_HARD_MASK)
return false;
@@ -358,18 +377,16 @@ static inline bool should_hard_irq_enable(void)
/*
* Do the hard enabling, only call this if should_hard_irq_enable is true.
+ * This allows PMI interrupts to profile irq handlers.
*/
static inline void do_hard_irq_enable(void)
{
- if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG)) {
- WARN_ON(irq_soft_mask_return() == IRQS_ENABLED);
- WARN_ON(get_paca()->irq_happened & PACA_IRQ_MUST_HARD_MASK);
- WARN_ON(mfmsr() & MSR_EE);
- }
/*
-* This allows PMI interrupts (and watchdog soft-NMIs) through.
-* There is no other reason to enable this way.
+* Asynch interrupts come in with IRQS_ALL_DISABLED,
+* PACA_IRQ_HARD_DIS, and MSR[EE]=0.
*/
+ if (IS_ENABLED(CONFIG_PPC_BOOK3S_64))
+ irq_soft_mask_andc_return(IRQS_PMI_DISABLED);
get_paca()->irq_happened &= ~PACA_IRQ_HARD_DIS;
__hard_irq_enable();
}
@@ -452,7 +469,7 @@ static inline bool arch_irq_disabled_regs(struct pt_regs
*regs)
return !(regs->msr & MSR_EE);
}
-static __always_inline bool should_hard_irq_enable(void)
+static __always_inline bool should_hard_irq_enable(struct pt_regs *regs)
{
return false;
}
diff --git a/arch/powerpc/kernel/dbell.c b/arch/powerpc/kernel/dbell.c
index f55c6fb34a3a0..5712dd846263c 100644
--- a/arch/powerpc/kernel/dbell.c
+++ b/arch/powerpc/kernel/dbell.c
@@ -27,7 +27,7 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(doorbell_exception)
ppc_msgsync();
- if (should_hard_irq_enable())
+ if (should_hard_irq_enable(regs))
do_hard_irq_enable();
kvmppc_clear_host_ipi(smp_processor_id());
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 9ede61a5a469e..55142ff649f3f 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -238,7 +238,7 @@ static void __do_irq(struct pt_regs *regs, unsigned long
oldsp)
irq = static_call(p
[PATCH AUTOSEL 5.15 09/17] powerpc/85xx: Fix unannotated intra-function call warning
From: Sathvika Vasireddy [ Upstream commit 8afffce6aa3bddc940ac1909627ff1e772b6cbf1 ] objtool throws the following warning: arch/powerpc/kernel/head_85xx.o: warning: objtool: .head.text+0x1a6c: unannotated intra-function call Fix the warning by annotating KernelSPE symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END macros. Reported-by: kernel test robot Signed-off-by: Sathvika Vasireddy Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/20230128124138.1066176-1...@linux.ibm.com Signed-off-by: Sasha Levin --- arch/powerpc/kernel/head_fsl_booke.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S index 0a9a0f301474d..5cb599c42b597 100644 --- a/arch/powerpc/kernel/head_fsl_booke.S +++ b/arch/powerpc/kernel/head_fsl_booke.S @@ -849,7 +849,7 @@ _GLOBAL(load_up_spe) * SPE unavailable trap from kernel - print a message, but let * the task use SPE in the kernel until it returns to user mode. */ -KernelSPE: +SYM_FUNC_START_LOCAL(KernelSPE) lwz r3,_MSR(r1) orisr3,r3,MSR_SPE@h stw r3,_MSR(r1) /* enable use of SPE after return */ @@ -866,6 +866,7 @@ KernelSPE: #endif .align 4,0 +SYM_FUNC_END(KernelSPE) #endif /* CONFIG_SPE */ /* -- 2.39.0
[PATCH AUTOSEL 5.15 10/17] powerpc/kvm: Fix unannotated intra-function call warning
From: Sathvika Vasireddy
[ Upstream commit fe6de81b610e5d0b9d2231acff2de74a35482e7d ]
objtool throws the following warning:
arch/powerpc/kvm/booke.o: warning: objtool: kvmppc_fill_pt_regs+0x30:
unannotated intra-function call
Fix the warning by setting the value of 'nip' using the _THIS_IP_ macro,
without using an assembly bl/mflr sequence to save the instruction
pointer.
Reported-by: kernel test robot
Suggested-by: Michael Ellerman
Signed-off-by: Sathvika Vasireddy
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230128124158.1066251-1...@linux.ibm.com
Signed-off-by: Sasha Levin
---
arch/powerpc/kvm/booke.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 8c15c90dd3a97..4d2864725d840 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -913,16 +913,15 @@ static int kvmppc_handle_debug(struct kvm_vcpu *vcpu)
static void kvmppc_fill_pt_regs(struct pt_regs *regs)
{
- ulong r1, ip, msr, lr;
+ ulong r1, msr, lr;
asm("mr %0, 1" : "=r"(r1));
asm("mflr %0" : "=r"(lr));
asm("mfmsr %0" : "=r"(msr));
- asm("bl 1f; 1: mflr %0" : "=r"(ip));
memset(regs, 0, sizeof(*regs));
regs->gpr[1] = r1;
- regs->nip = ip;
+ regs->nip = _THIS_IP_;
regs->msr = msr;
regs->link = lr;
}
--
2.39.0
[PATCH AUTOSEL 5.10 08/13] powerpc/kvm: Fix unannotated intra-function call warning
From: Sathvika Vasireddy
[ Upstream commit fe6de81b610e5d0b9d2231acff2de74a35482e7d ]
objtool throws the following warning:
arch/powerpc/kvm/booke.o: warning: objtool: kvmppc_fill_pt_regs+0x30:
unannotated intra-function call
Fix the warning by setting the value of 'nip' using the _THIS_IP_ macro,
without using an assembly bl/mflr sequence to save the instruction
pointer.
Reported-by: kernel test robot
Suggested-by: Michael Ellerman
Signed-off-by: Sathvika Vasireddy
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230128124158.1066251-1...@linux.ibm.com
Signed-off-by: Sasha Levin
---
arch/powerpc/kvm/booke.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 75381beb7514a..c291021653ecc 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -887,16 +887,15 @@ static int kvmppc_handle_debug(struct kvm_vcpu *vcpu)
static void kvmppc_fill_pt_regs(struct pt_regs *regs)
{
- ulong r1, ip, msr, lr;
+ ulong r1, msr, lr;
asm("mr %0, 1" : "=r"(r1));
asm("mflr %0" : "=r"(lr));
asm("mfmsr %0" : "=r"(msr));
- asm("bl 1f; 1: mflr %0" : "=r"(ip));
memset(regs, 0, sizeof(*regs));
regs->gpr[1] = r1;
- regs->nip = ip;
+ regs->nip = _THIS_IP_;
regs->msr = msr;
regs->link = lr;
}
--
2.39.0
[PATCH AUTOSEL 5.10 07/13] powerpc/85xx: Fix unannotated intra-function call warning
From: Sathvika Vasireddy [ Upstream commit 8afffce6aa3bddc940ac1909627ff1e772b6cbf1 ] objtool throws the following warning: arch/powerpc/kernel/head_85xx.o: warning: objtool: .head.text+0x1a6c: unannotated intra-function call Fix the warning by annotating KernelSPE symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END macros. Reported-by: kernel test robot Signed-off-by: Sathvika Vasireddy Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/20230128124138.1066176-1...@linux.ibm.com Signed-off-by: Sasha Levin --- arch/powerpc/kernel/head_fsl_booke.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S index 586a6ac501e97..f8ad833506c9e 100644 --- a/arch/powerpc/kernel/head_fsl_booke.S +++ b/arch/powerpc/kernel/head_fsl_booke.S @@ -883,7 +883,7 @@ _GLOBAL(load_up_spe) * SPE unavailable trap from kernel - print a message, but let * the task use SPE in the kernel until it returns to user mode. */ -KernelSPE: +SYM_FUNC_START_LOCAL(KernelSPE) lwz r3,_MSR(r1) orisr3,r3,MSR_SPE@h stw r3,_MSR(r1) /* enable use of SPE after return */ @@ -900,6 +900,7 @@ KernelSPE: #endif .align 4,0 +SYM_FUNC_END(KernelSPE) #endif /* CONFIG_SPE */ /* -- 2.39.0
[PATCH AUTOSEL 5.4 05/10] powerpc/85xx: Fix unannotated intra-function call warning
From: Sathvika Vasireddy [ Upstream commit 8afffce6aa3bddc940ac1909627ff1e772b6cbf1 ] objtool throws the following warning: arch/powerpc/kernel/head_85xx.o: warning: objtool: .head.text+0x1a6c: unannotated intra-function call Fix the warning by annotating KernelSPE symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END macros. Reported-by: kernel test robot Signed-off-by: Sathvika Vasireddy Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/20230128124138.1066176-1...@linux.ibm.com Signed-off-by: Sasha Levin --- arch/powerpc/kernel/head_fsl_booke.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S index 519d49547e2f3..c899f2743bc9e 100644 --- a/arch/powerpc/kernel/head_fsl_booke.S +++ b/arch/powerpc/kernel/head_fsl_booke.S @@ -880,7 +880,7 @@ _GLOBAL(load_up_spe) * SPE unavailable trap from kernel - print a message, but let * the task use SPE in the kernel until it returns to user mode. */ -KernelSPE: +SYM_FUNC_START_LOCAL(KernelSPE) lwz r3,_MSR(r1) orisr3,r3,MSR_SPE@h stw r3,_MSR(r1) /* enable use of SPE after return */ @@ -897,6 +897,7 @@ KernelSPE: #endif .align 4,0 +SYM_FUNC_END(KernelSPE) #endif /* CONFIG_SPE */ /* -- 2.39.0
[PATCH AUTOSEL 5.4 06/10] powerpc/kvm: Fix unannotated intra-function call warning
From: Sathvika Vasireddy
[ Upstream commit fe6de81b610e5d0b9d2231acff2de74a35482e7d ]
objtool throws the following warning:
arch/powerpc/kvm/booke.o: warning: objtool: kvmppc_fill_pt_regs+0x30:
unannotated intra-function call
Fix the warning by setting the value of 'nip' using the _THIS_IP_ macro,
without using an assembly bl/mflr sequence to save the instruction
pointer.
Reported-by: kernel test robot
Suggested-by: Michael Ellerman
Signed-off-by: Sathvika Vasireddy
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230128124158.1066251-1...@linux.ibm.com
Signed-off-by: Sasha Levin
---
arch/powerpc/kvm/booke.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index be9a45874194f..fc2bb33a4e0e2 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -887,16 +887,15 @@ static int kvmppc_handle_debug(struct kvm_run *run,
struct kvm_vcpu *vcpu)
static void kvmppc_fill_pt_regs(struct pt_regs *regs)
{
- ulong r1, ip, msr, lr;
+ ulong r1, msr, lr;
asm("mr %0, 1" : "=r"(r1));
asm("mflr %0" : "=r"(lr));
asm("mfmsr %0" : "=r"(msr));
- asm("bl 1f; 1: mflr %0" : "=r"(ip));
memset(regs, 0, sizeof(*regs));
regs->gpr[1] = r1;
- regs->nip = ip;
+ regs->nip = _THIS_IP_;
regs->msr = msr;
regs->link = lr;
}
--
2.39.0
[PATCH AUTOSEL 4.19 4/6] powerpc/85xx: Fix unannotated intra-function call warning
From: Sathvika Vasireddy [ Upstream commit 8afffce6aa3bddc940ac1909627ff1e772b6cbf1 ] objtool throws the following warning: arch/powerpc/kernel/head_85xx.o: warning: objtool: .head.text+0x1a6c: unannotated intra-function call Fix the warning by annotating KernelSPE symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END macros. Reported-by: kernel test robot Signed-off-by: Sathvika Vasireddy Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/20230128124138.1066176-1...@linux.ibm.com Signed-off-by: Sasha Levin --- arch/powerpc/kernel/head_fsl_booke.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S index 2386ce2a9c6e4..1c8097fc75fff 100644 --- a/arch/powerpc/kernel/head_fsl_booke.S +++ b/arch/powerpc/kernel/head_fsl_booke.S @@ -889,7 +889,7 @@ _GLOBAL(load_up_spe) * SPE unavailable trap from kernel - print a message, but let * the task use SPE in the kernel until it returns to user mode. */ -KernelSPE: +SYM_FUNC_START_LOCAL(KernelSPE) lwz r3,_MSR(r1) orisr3,r3,MSR_SPE@h stw r3,_MSR(r1) /* enable use of SPE after return */ @@ -906,6 +906,7 @@ KernelSPE: #endif .align 4,0 +SYM_FUNC_END(KernelSPE) #endif /* CONFIG_SPE */ /* -- 2.39.0
[PATCH AUTOSEL 4.19 5/6] powerpc/kvm: Fix unannotated intra-function call warning
From: Sathvika Vasireddy
[ Upstream commit fe6de81b610e5d0b9d2231acff2de74a35482e7d ]
objtool throws the following warning:
arch/powerpc/kvm/booke.o: warning: objtool: kvmppc_fill_pt_regs+0x30:
unannotated intra-function call
Fix the warning by setting the value of 'nip' using the _THIS_IP_ macro,
without using an assembly bl/mflr sequence to save the instruction
pointer.
Reported-by: kernel test robot
Suggested-by: Michael Ellerman
Signed-off-by: Sathvika Vasireddy
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230128124158.1066251-1...@linux.ibm.com
Signed-off-by: Sasha Levin
---
arch/powerpc/kvm/booke.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index a9ca016da6702..5d8cc8a637e74 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -898,16 +898,15 @@ static int kvmppc_handle_debug(struct kvm_run *run,
struct kvm_vcpu *vcpu)
static void kvmppc_fill_pt_regs(struct pt_regs *regs)
{
- ulong r1, ip, msr, lr;
+ ulong r1, msr, lr;
asm("mr %0, 1" : "=r"(r1));
asm("mflr %0" : "=r"(lr));
asm("mfmsr %0" : "=r"(msr));
- asm("bl 1f; 1: mflr %0" : "=r"(ip));
memset(regs, 0, sizeof(*regs));
regs->gpr[1] = r1;
- regs->nip = ip;
+ regs->nip = _THIS_IP_;
regs->msr = msr;
regs->link = lr;
}
--
2.39.0
[PATCH AUTOSEL 4.14 3/5] powerpc/85xx: Fix unannotated intra-function call warning
From: Sathvika Vasireddy [ Upstream commit 8afffce6aa3bddc940ac1909627ff1e772b6cbf1 ] objtool throws the following warning: arch/powerpc/kernel/head_85xx.o: warning: objtool: .head.text+0x1a6c: unannotated intra-function call Fix the warning by annotating KernelSPE symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END macros. Reported-by: kernel test robot Signed-off-by: Sathvika Vasireddy Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/20230128124138.1066176-1...@linux.ibm.com Signed-off-by: Sasha Levin --- arch/powerpc/kernel/head_fsl_booke.S | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S index 60a0aeefc4a76..8df09f8208f4f 100644 --- a/arch/powerpc/kernel/head_fsl_booke.S +++ b/arch/powerpc/kernel/head_fsl_booke.S @@ -888,7 +888,7 @@ _GLOBAL(load_up_spe) * SPE unavailable trap from kernel - print a message, but let * the task use SPE in the kernel until it returns to user mode. */ -KernelSPE: +SYM_FUNC_START_LOCAL(KernelSPE) lwz r3,_MSR(r1) orisr3,r3,MSR_SPE@h stw r3,_MSR(r1) /* enable use of SPE after return */ @@ -905,6 +905,7 @@ KernelSPE: #endif .align 4,0 +SYM_FUNC_END(KernelSPE) #endif /* CONFIG_SPE */ /* -- 2.39.0
[PATCH AUTOSEL 4.14 4/5] powerpc/kvm: Fix unannotated intra-function call warning
From: Sathvika Vasireddy
[ Upstream commit fe6de81b610e5d0b9d2231acff2de74a35482e7d ]
objtool throws the following warning:
arch/powerpc/kvm/booke.o: warning: objtool: kvmppc_fill_pt_regs+0x30:
unannotated intra-function call
Fix the warning by setting the value of 'nip' using the _THIS_IP_ macro,
without using an assembly bl/mflr sequence to save the instruction
pointer.
Reported-by: kernel test robot
Suggested-by: Michael Ellerman
Signed-off-by: Sathvika Vasireddy
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20230128124158.1066251-1...@linux.ibm.com
Signed-off-by: Sasha Levin
---
arch/powerpc/kvm/booke.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 071b87ee682f8..8a202636676f9 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -888,16 +888,15 @@ static int kvmppc_handle_debug(struct kvm_run *run,
struct kvm_vcpu *vcpu)
static void kvmppc_fill_pt_regs(struct pt_regs *regs)
{
- ulong r1, ip, msr, lr;
+ ulong r1, msr, lr;
asm("mr %0, 1" : "=r"(r1));
asm("mflr %0" : "=r"(lr));
asm("mfmsr %0" : "=r"(msr));
- asm("bl 1f; 1: mflr %0" : "=r"(ip));
memset(regs, 0, sizeof(*regs));
regs->gpr[1] = r1;
- regs->nip = ip;
+ regs->nip = _THIS_IP_;
regs->msr = msr;
regs->link = lr;
}
--
2.39.0
Re: [External] : RE: [EXT] [PATCH v2 1/1] PCI: layerscape: Add EP mode support for ls1028a
yes, it is more about sort the list using .data and .compatible. key
much better if it we keep this as suggested by Frank,
static const struct of_device_id ls_pcie_ep_of_match[] = {
+ { .compatible = "fsl,ls1028a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
Thanks,
Alok
On 2/9/2023 3:53 AM, Bjorn Helgaas wrote:
On Tue, Feb 07, 2023 at 04:20:21PM +, Frank Li wrote:
Subject: Re: [External] : RE: [EXT] [PATCH v2 1/1] PCI: layerscape: Add EP
mode support for ls1028a
{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
+ { .compatible = "fsl,ls1028a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
can it be like this for better readability. ?
It is just chip name and follow name conversion, which already
upstreamed and documented.
Why do you think it not is good readability?
I thought maybe ALOK's point was to sort the list, which does make a
lot of sense. But if you want to sort by the .data member, I would
think you would make .compatible a secondary sort key, which means
ls1028a would come before ls1046a, so you would end up with this
instead:
static const struct of_device_id ls_pcie_ep_of_match[] = {
+ { .compatible = "fsl,ls1028a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
{ .compatible = "fsl,lx2160ar2-pcie-ep", .data = &lx2_ep_drvdata },
{ },
};
[PATCH 02/11] fbdev: Transfer video= option strings to caller; clarify ownership
In fb_get_options(), always duplicate the returned option string and
transfer ownership of the memory to the function's caller.
Until now, only the global option string got duplicated and transferred
to the caller; the per-driver options were owned by fb_get_options().
In the end, it was impossible for the function's caller to detect if
it had to release the string's memory buffer. Hence, all calling drivers
leak the memory buffer. The leaks have existed ever since, but drivers
only call fb_get_option() once as part of module initialization. So the
amount of leaked memory is not significant.
Fix the semantics of fb_get_option() by unconditionally transferring
ownership of the memory buffer to the caller. Later patches can resolve
the memory leaks in the fbdev drivers.
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/fb_cmdline.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_cmdline.c
b/drivers/video/fbdev/core/fb_cmdline.c
index 6792010d6716..702b00b71870 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -30,13 +30,17 @@ EXPORT_SYMBOL_GPL(fb_mode_option);
* (video=:)
* @option: the option will be stored here
*
+ * The caller owns the string returned in @option and is
+ * responsible for releasing the memory.
+ *
* NOTE: Needed to maintain backwards compatibility
*/
int fb_get_options(const char *name, char **option)
{
- char *opt, *options = NULL;
+ const char *options = NULL;
int retval = 0;
int name_len = strlen(name), i;
+ char *opt;
if (name_len && ofonly && strncmp(name, "offb", 4))
retval = 1;
@@ -55,12 +59,16 @@ int fb_get_options(const char *name, char **option)
}
/* No match, pass global option */
if (!options && option && fb_mode_option)
- options = kstrdup(fb_mode_option, GFP_KERNEL);
+ options = fb_mode_option;
if (options && !strncmp(options, "off", 3))
retval = 1;
- if (option)
- *option = options;
+ if (option) {
+ if (options)
+ *option = kstrdup(options, GFP_KERNEL);
+ else
+ *option = NULL;
+ }
return retval;
}
--
2.39.1
[PATCH 00/11] drm,fbdev: Move video= option to drivers/video
The kernel's video= option sets the initial video mode. It is shared by fbdev and DRM, but located within the fbdev code. Move it to drivers/video/ and adapt callers. Allows DRM (and others) to use the option without depending on fbdev. While at it, fix the interface of the lookup functions. This requires a number of changes. First clarify the ownership of the option string in patch 2. The helper fb_get_options() returns the video= parameter's value. It's sometimes a copy and sometimes the original string. Hence callers, mostly fbdev drivers, have just leaked the returned string. Change this to always duplicate the option string in fb_get_options() and transfer ownership of the copy to the caller. We can then start to fix the memory leaks in the fbdev drivers. There's a global video= setting and a number of per-output settings. In patches 3 to 5, support explicit lookup of the global video option and lookup the string in fbdev's modedb and in a PS3 driver. Then avoid exporting the global setting's internal state variable in patch 6. Finally, in patches 7 to 11, move the video= option to drivers/video. It can be used directly in DRM and a PS3 driver. This fixes any memory leaks from the returned option string. For fbdev drivers, the helper fb_get_options() remains as an adapter aroudn the new interface. Tested with DRM and fbdev and built for the PS3. Thomas Zimmermann (11): fbdev: Fix contact info in fb_cmdline.c fbdev: Transfer video= option strings to caller; clarify ownership fbdev: Support NULL for name in option-string lookup drivers/ps3: Read video= option with fb_get_option() fbdev: Read video= option with fb_get_option() in modedb fbdev: Unexport fb_mode_option fbdev: Move option-string lookup into helper fbdev: Handle video= parameter in video/cmdline.c driver/ps3: Include for mode parsing drm: Include for mode parsing drm: Fix comment on mode parsing drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/drm_connector.c | 9 +- drivers/gpu/drm/drm_modes.c | 3 +- drivers/ps3/ps3av.c | 9 +- drivers/video/Kconfig | 3 + drivers/video/Makefile| 1 + drivers/video/cmdline.c | 133 ++ drivers/video/fbdev/Kconfig | 5 +- drivers/video/fbdev/core/Makefile | 3 +- drivers/video/fbdev/core/fb_cmdline.c | 94 +- drivers/video/fbdev/core/modedb.c | 8 +- include/linux/fb.h| 1 - include/video/cmdline.h | 20 13 files changed, 202 insertions(+), 89 deletions(-) create mode 100644 drivers/video/cmdline.c create mode 100644 include/video/cmdline.h base-commit: 1a019dd7a5d25f7c1c9b77931138290e28947e6a -- 2.39.1
[PATCH 01/11] fbdev: Fix contact info in fb_cmdline.c
Fix Daniel's email address. No functional changes. Signed-off-by: Thomas Zimmermann Cc: Daniel Vetter --- drivers/video/fbdev/core/fb_cmdline.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c index 3b5bd666b952..6792010d6716 100644 --- a/drivers/video/fbdev/core/fb_cmdline.c +++ b/drivers/video/fbdev/core/fb_cmdline.c @@ -12,7 +12,7 @@ * for more details. * * Authors: - *Vetter + *Daniel Vetter */ #include #include -- 2.39.1
[PATCH 09/11] driver/ps3: Include for mode parsing
Include in ps3av.c to get video_get_options() and
avoid the dependency on . The replaced function
fb_get_options() is just a tiny wrapper around video_get_opions(). No
functional changes.
Signed-off-by: Thomas Zimmermann
---
drivers/ps3/ps3av.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/ps3/ps3av.c b/drivers/ps3/ps3av.c
index 8f3e60f1bfe2..f6c9e56bdba7 100644
--- a/drivers/ps3/ps3av.c
+++ b/drivers/ps3/ps3av.c
@@ -11,13 +11,14 @@
#include
#include
#include
-#include
#include
#include
#include
#include
+#include
+
#include "vuart.h"
#define BUFSIZE 4096 /* vuart buf size */
@@ -921,9 +922,7 @@ EXPORT_SYMBOL_GPL(ps3av_audio_mute);
static int ps3av_probe(struct ps3_system_bus_device *dev)
{
-#ifdef CONFIG_FB
- char *mode_option = NULL;
-#endif
+ const char *mode_option;
int res;
int id;
@@ -971,14 +970,9 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
ps3av_get_hw_conf(ps3av);
-#ifdef CONFIG_FB
- fb_get_options(NULL, &mode_option);
- if (mode_option) {
- if (!strcmp(mode_option, "safe"))
- safe_mode = 1;
- kfree(mode_option);
- }
-#endif /* CONFIG_FB */
+ mode_option = video_get_options(NULL);
+ if (mode_option && !strcmp(mode_option, "safe"))
+ safe_mode = 1;
id = ps3av_auto_videomode(&ps3av->av_hw_conf);
if (id < 0) {
printk(KERN_ERR "%s: invalid id :%d\n", __func__, id);
--
2.39.1
[PATCH 04/11] drivers/ps3: Read video= option with fb_get_option()
Get the kernel's global video= parameter with fb_get_option(). Done
to unexport the internal fbdev state fb_mode_config. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/ps3/ps3av.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/ps3/ps3av.c b/drivers/ps3/ps3av.c
index 516e6d14d32e..8f3e60f1bfe2 100644
--- a/drivers/ps3/ps3av.c
+++ b/drivers/ps3/ps3av.c
@@ -921,6 +921,9 @@ EXPORT_SYMBOL_GPL(ps3av_audio_mute);
static int ps3av_probe(struct ps3_system_bus_device *dev)
{
+#ifdef CONFIG_FB
+ char *mode_option = NULL;
+#endif
int res;
int id;
@@ -969,8 +972,12 @@ static int ps3av_probe(struct ps3_system_bus_device *dev)
ps3av_get_hw_conf(ps3av);
#ifdef CONFIG_FB
- if (fb_mode_option && !strcmp(fb_mode_option, "safe"))
- safe_mode = 1;
+ fb_get_options(NULL, &mode_option);
+ if (mode_option) {
+ if (!strcmp(mode_option, "safe"))
+ safe_mode = 1;
+ kfree(mode_option);
+ }
#endif /* CONFIG_FB */
id = ps3av_auto_videomode(&ps3av->av_hw_conf);
if (id < 0) {
--
2.39.1
[PATCH 07/11] fbdev: Move option-string lookup into helper
Move the lookup of the option string into an internal helper. No
functional changes.
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/fb_cmdline.c | 60 ---
1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_cmdline.c
b/drivers/video/fbdev/core/fb_cmdline.c
index 512da89ff8b5..f811c7b679e1 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -21,6 +21,36 @@ static char *video_options[FB_MAX] __read_mostly;
static const char *fb_mode_option __read_mostly;
static int ofonly __read_mostly;
+static const char *__fb_get_options(const char *name)
+{
+ const char *options = NULL;
+ size_t name_len = 0;
+
+ if (name)
+ name_len = strlen(name);
+
+ if (name_len) {
+ unsigned int i;
+ const char *opt;
+
+ for (i = 0; i < ARRAY_SIZE(video_options); ++i) {
+ if (!video_options[i])
+ continue;
+ if (video_options[i][0] == '\0')
+ continue;
+ opt = video_options[i];
+ if (!strncmp(opt, name, name_len) && opt[name_len] ==
':')
+ options = opt + name_len + 1;
+ }
+ }
+
+ /* No match, return global options */
+ if (!options)
+ options = fb_mode_option;
+
+ return options;
+}
+
/**
* fb_get_options - get kernel boot parameters
* @name: framebuffer name as it would appear in
@@ -35,36 +65,18 @@ static int ofonly __read_mostly;
*/
int fb_get_options(const char *name, char **option)
{
- const char *options = NULL;
int retval = 0;
- size_t name_len;
- char *opt;
+ const char *options;
- if (name)
- name_len = strlen(name);
-
- if (name_len && ofonly && strncmp(name, "offb", 4))
+ if (name && ofonly && strncmp(name, "offb", 4))
retval = 1;
- if (name_len && !retval) {
- unsigned int i;
+ options = __fb_get_options(name);
- for (i = 0; i < FB_MAX; i++) {
- if (video_options[i] == NULL)
- continue;
- if (!video_options[i][0])
- continue;
- opt = video_options[i];
- if (!strncmp(name, opt, name_len) &&
- opt[name_len] == ':')
- options = opt + name_len + 1;
- }
+ if (options) {
+ if (!strncmp(options, "off", 3))
+ retval = 1;
}
- /* No match, pass global option */
- if (!options && option && fb_mode_option)
- options = fb_mode_option;
- if (options && !strncmp(options, "off", 3))
- retval = 1;
if (option) {
if (options)
--
2.39.1
[PATCH 08/11] fbdev: Handle video= parameter in video/cmdline.c
Handle the command-line parameter video= in video/cmdline.c. Implement
the fbdev helper fb_get_options() on top. Will allows to handle the
kernel parameter in DRM without fbdev dependencies.
Note that __video_get_options() has the meaning of its return value
inverted compared to fb_get_options(). The new helper returns true if
the adapter has been enabled, and false otherwise.
There is the ofonly parameter, which disables output for non-OF-based
framebuffers. It is only for offb and looks like a workaround. The actual
purpose it not clear to me. Use 'video=off' or 'nomodeset' instead.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/Kconfig | 2 +-
drivers/video/Kconfig | 3 +
drivers/video/Makefile| 1 +
drivers/video/cmdline.c | 133 ++
drivers/video/fbdev/Kconfig | 5 +-
drivers/video/fbdev/core/Makefile | 3 +-
drivers/video/fbdev/core/fb_cmdline.c | 93 +++---
include/video/cmdline.h | 20
8 files changed, 172 insertions(+), 88 deletions(-)
create mode 100644 drivers/video/cmdline.c
create mode 100644 include/video/cmdline.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index dc0f94f02a82..81c8a99c744a 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -10,13 +10,13 @@ menuconfig DRM
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
select DRM_PANEL_ORIENTATION_QUIRKS
select HDMI
- select FB_CMDLINE
select I2C
select DMA_SHARED_BUFFER
select SYNC_FILE
# gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
# device and dmabuf fd. Let's make sure that is available for our userspace.
select KCMP
+ select VIDEO_CMDLINE
select VIDEO_NOMODESET
help
Kernel-level support for the Direct Rendering Infrastructure (DRI)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 6d2fde6c5d11..bf05363d8906 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -11,6 +11,9 @@ config APERTURE_HELPERS
Support tracking and hand-over of aperture ownership. Required
by graphics drivers for firmware-provided framebuffers.
+config VIDEO_CMDLINE
+ bool
+
config VIDEO_NOMODESET
bool
default n
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index a50eb528ed3c..831c9fa57a6c 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_APERTURE_HELPERS)+= aperture.o
obj-$(CONFIG_VGASTATE)+= vgastate.o
+obj-$(CONFIG_VIDEO_CMDLINE) += cmdline.o
obj-$(CONFIG_VIDEO_NOMODESET) += nomodeset.o
obj-$(CONFIG_HDMI)+= hdmi.o
diff --git a/drivers/video/cmdline.c b/drivers/video/cmdline.c
new file mode 100644
index ..d3d257489c3d
--- /dev/null
+++ b/drivers/video/cmdline.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Based on the fbdev code in drivers/video/fbdev/core/fb_cmdline:
+ *
+ * Copyright (C) 2014 Intel Corp
+ * Copyright (C) 1994 Martin Schaller
+ *
+ * 2001 - Documented with DocBook
+ * - Brad Douglas
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors:
+ *Daniel Vetter
+ */
+
+#include /* for FB_MAX */
+#include
+
+#include
+
+/*
+ * FB_MAX is the maximum number of framebuffer devices and also
+ * the maximum number of video= parameters. Although not directly
+ * related to each other, it makes sense to keep it that way.
+ */
+static const char *video_options[FB_MAX] __read_mostly;
+static const char *video_option __read_mostly;
+static int video_of_only __read_mostly;
+
+static const char *__video_get_option_string(const char *name)
+{
+ const char *options = NULL;
+ size_t name_len = 0;
+
+ if (name)
+ name_len = strlen(name);
+
+ if (name_len) {
+ unsigned int i;
+ const char *opt;
+
+ for (i = 0; i < ARRAY_SIZE(video_options); ++i) {
+ if (!video_options[i])
+ continue;
+ if (video_options[i][0] == '\0')
+ continue;
+ opt = video_options[i];
+ if (!strncmp(opt, name, name_len) && opt[name_len] ==
':')
+ options = opt + name_len + 1;
+ }
+ }
+
+ /* No match, return global options */
+ if (!options)
+ options = video_option;
+
+ return options;
+}
+
+/**
+ * video_get_options - get kernel boot parameters
+ * @name: name of the output as it would appear in the boot parameter
+ * line (video=:)
+ *
+ * Looks up the video= options for the given name. Names are con
[PATCH 03/11] fbdev: Support NULL for name in option-string lookup
Ignore the per-driver video options if no driver name has been
specified to fb_get_option(). Return the global options in this
case.
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/fb_cmdline.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fb_cmdline.c
b/drivers/video/fbdev/core/fb_cmdline.c
index 702b00b71870..cc8a88e8f308 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -39,13 +39,18 @@ int fb_get_options(const char *name, char **option)
{
const char *options = NULL;
int retval = 0;
- int name_len = strlen(name), i;
+ size_t name_len;
char *opt;
+ if (name)
+ name_len = strlen(name);
+
if (name_len && ofonly && strncmp(name, "offb", 4))
retval = 1;
if (name_len && !retval) {
+ unsigned int i;
+
for (i = 0; i < FB_MAX; i++) {
if (video_options[i] == NULL)
continue;
--
2.39.1
[PATCH 06/11] fbdev: Unexport fb_mode_option
There are no external users of fb_mode_option. Unexport the variable
and declare it static.
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/fb_cmdline.c | 4 +---
include/linux/fb.h| 1 -
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_cmdline.c
b/drivers/video/fbdev/core/fb_cmdline.c
index cc8a88e8f308..512da89ff8b5 100644
--- a/drivers/video/fbdev/core/fb_cmdline.c
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -18,11 +18,9 @@
#include
static char *video_options[FB_MAX] __read_mostly;
+static const char *fb_mode_option __read_mostly;
static int ofonly __read_mostly;
-const char *fb_mode_option;
-EXPORT_SYMBOL_GPL(fb_mode_option);
-
/**
* fb_get_options - get kernel boot parameters
* @name: framebuffer name as it would appear in
diff --git a/include/linux/fb.h b/include/linux/fb.h
index daf336385613..054e8950b274 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -764,7 +764,6 @@ struct dmt_videomode {
const struct fb_videomode *mode;
};
-extern const char *fb_mode_option;
extern const struct fb_videomode vesa_modes[];
extern const struct dmt_videomode dmt_modes[];
--
2.39.1
[PATCH 05/11] fbdev: Read video= option with fb_get_option() in modedb
Get the kernel's global video= parameter with fb_get_option(). Done
to unexport the internal fbdev state fb_mode_config. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/modedb.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/core/modedb.c
b/drivers/video/fbdev/core/modedb.c
index 6473e0dfe146..23cf8eba785d 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -620,6 +620,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
const struct fb_videomode *default_mode,
unsigned int default_bpp)
{
+ char *mode_option_buf = NULL;
int i;
/* Set up defaults */
@@ -635,8 +636,10 @@ int fb_find_mode(struct fb_var_screeninfo *var,
default_bpp = 8;
/* Did the user specify a video mode? */
- if (!mode_option)
- mode_option = fb_mode_option;
+ if (!mode_option) {
+ fb_get_options(NULL, &mode_option_buf);
+ mode_option = mode_option_buf;
+ }
if (mode_option) {
const char *name = mode_option;
unsigned int namelen = strlen(name);
@@ -715,6 +718,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
res_specified = 1;
}
done:
+ kfree(mode_option_buf);
if (cvt) {
struct fb_videomode cvt_mode;
int ret;
--
2.39.1
[PATCH 11/11] drm: Fix comment on mode parsing
Do not claim that there's a default mode in the video= option parser. if no option string has been given, the parser does nothing. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_modes.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 40d482a01178..ac9a406250c5 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -2339,8 +2339,7 @@ static int drm_mode_parse_cmdline_named_mode(const char *name, * @mode: preallocated drm_cmdline_mode structure to fill out * * This parses @mode_option command line modeline for modes and options to - * configure the connector. If @mode_option is NULL the default command line - * modeline in fb_mode_option will be parsed instead. + * configure the connector. * * This uses the same parameters as the fb modedb.c, except for an extra * force-enable, force-enable-digital and force-disable bit at the end:: -- 2.39.1
[PATCH 10/11] drm: Include for mode parsing
Include in drm_connector.c to get video_get_options()
and avoid the dependency on . The replaced function
fb_get_options() is just a tiny wrapper around video_get_opions(). No
functional changes.
Include to get fwnode_handle_put(), which had been
provided via .
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_connector.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 9d0250c28e9b..ca5fb54b7aab 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -33,9 +33,11 @@
#include
#include
-#include
+#include
#include
+#include
+
#include "drm_crtc_internal.h"
#include "drm_internal.h"
@@ -154,9 +156,10 @@ EXPORT_SYMBOL(drm_get_connector_type_name);
static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
{
struct drm_cmdline_mode *mode = &connector->cmdline_mode;
- char *option = NULL;
+ const char *option;
- if (fb_get_options(connector->name, &option))
+ option = video_get_options(connector->name);
+ if (!option)
return;
if (!drm_mode_parse_command_line_for_connector(option,
--
2.39.1
[PATCH 00/24 v2] Documentation: correct lots of spelling errors (series 1)
Correct many spelling errors in Documentation/ as reported by codespell. Maintainers of specific kernel subsystems are only Cc-ed on their respective patches, not the entire series. These patches are based on linux-next-20230209. [PATCH 01/24] Documentation: arm: correct spelling [PATCH 02/24] Documentation: block: correct spelling [PATCH 03/24] Documentation: core-api: correct spelling [PATCH 04/24] Documentation: fault-injection: correct spelling [PATCH 05/24] Documentation: fb: correct spelling [PATCH 06/24] Documentation: features: correct spelling [PATCH 07/24] Documentation: input: correct spelling [PATCH 08/24] Documentation: isdn: correct spelling [PATCH 09/24] Documentation: livepatch: correct spelling [PATCH 10/24] Documentation: locking: correct spelling [PATCH 11/24] Documentation: mm: correct spelling [PATCH 12/24] Documentation: openrisc: correct spelling [PATCH 13/24] Documentation: PCI: correct spelling [PATCH 14/24] Documentation: powerpc: correct spelling [PATCH 15/24] Documentation: s390: correct spelling [PATCH 16/24] Documentation: scheduler: correct spelling [PATCH 17/24] Documentation: security: correct spelling [PATCH 18/24] Documentation: timers: correct spelling [PATCH 19/24] Documentation: tools/rtla: correct spelling [PATCH 20/24] Documentation: trace/rv: correct spelling [PATCH 21/24] Documentation: trace: correct spelling [PATCH 22/24] Documentation: w1: correct spelling [PATCH 23/24] Documentation: x86: correct spelling [PATCH 24/24] Documentation: xtensa: correct spelling diffstat: Documentation/PCI/endpoint/pci-vntb-howto.rst|2 +- Documentation/PCI/msi-howto.rst |2 +- Documentation/arm/arm.rst|2 +- Documentation/arm/ixp4xx.rst |4 ++-- Documentation/arm/keystone/knav-qmss.rst |2 +- Documentation/arm/stm32/stm32-dma-mdma-chaining.rst |6 +++--- Documentation/arm/sunxi/clocks.rst |2 +- Documentation/arm/swp_emulation.rst |2 +- Documentation/arm/tcm.rst|2 +- Documentation/arm/vlocks.rst |2 +- Documentation/block/data-integrity.rst |2 +- Documentation/core-api/packing.rst |2 +- Documentation/core-api/padata.rst|2 +- Documentation/fault-injection/fault-injection.rst|2 +- Documentation/fb/sm712fb.rst |2 +- Documentation/fb/sstfb.rst |2 +- Documentation/features/core/thread-info-in-task/arch-support.txt |2 +- Documentation/input/devices/iforce-protocol.rst |2 +- Documentation/input/multi-touch-protocol.rst |2 +- Documentation/isdn/interface_capi.rst|2 +- Documentation/isdn/m_isdn.rst|2 +- Documentation/livepatch/reliable-stacktrace.rst |2 +- Documentation/locking/lockdep-design.rst |4 ++-- Documentation/locking/locktorture.rst|2 +- Documentation/locking/locktypes.rst |2 +- Documentation/locking/preempt-locking.rst|2 +- Documentation/mm/hmm.rst |4 ++-- Documentation/mm/hwpoison.rst|2 +- Documentation/openrisc/openrisc_port.rst |4 ++-- Documentation/power/suspend-and-interrupts.rst |2 +- Documentation/powerpc/kasan.txt |2 +- Documentation/powerpc/papr_hcalls.rst|2 +- Documentation/powerpc/qe_firmware.rst|4 ++-- Documentation/powerpc/vas-api.rst|4 ++-- Documentation/s390/pci.rst |4 ++-- Documentation/s390/vfio-ccw.rst |2 +- Documentation/scheduler/sched-bwc.rst|2 +- Documentation/scheduler/sched-energy.rst |4 ++-- Documentation/security/digsig.rst|4 ++-- Documentation/security/keys/core.rst |2 +- Documentation/security/secrets/coco.rst |2 +- Documentation/timers/hrtimers.rst|2 +- Documentation/tools/rtla/rtla-timerlat-top.rst |2 +- Documentation/trace/coresight/coresight-etm4x-reference.rst |2 +- Documentation/trace/events.rst
Re: [PATCH v4 14/18] gpio: regmap: Add missing header(s)
On Wed, Feb 8, 2023 at 6:34 PM Andy Shevchenko wrote: > Do not imply that some of the generic headers may be always included. > Instead, include explicitly what we are direct user of. > > While at it, split out the GPIO group of headers. > > Signed-off-by: Andy Shevchenko Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v4 16/18] gpiolib: Deduplicate forward declarations in consumer.h
On Wed, Feb 8, 2023 at 6:34 PM Andy Shevchenko wrote: > The struct fwnode_handle pointer is used in both branches of ifdeffery, > no need to have a copy of the same in each of them, just make it global. > > Signed-off-by: Andy Shevchenko Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v4 17/18] gpiolib: Group forward declarations in consumer.h
On Wed, Feb 8, 2023 at 6:34 PM Andy Shevchenko wrote: > For better maintenance group the forward declarations together. > > Signed-off-by: Andy Shevchenko Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v4 12/18] gpio: aggregator: Add missing header(s)
On Wed, Feb 8, 2023 at 6:34 PM Andy Shevchenko wrote: > Do not imply that some of the generic headers may be always included. > Instead, include explicitly what we are direct user of. > > While at it, drop unused linux/gpio.h and split out the GPIO group of > headers. > > Signed-off-by: Andy Shevchenko > Reviewed-by: Geert Uytterhoeven Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v4 13/18] gpio: reg: Add missing header(s)
On Wed, Feb 8, 2023 at 6:34 PM Andy Shevchenko wrote: > Do not imply that some of the generic headers may be always included. > Instead, include explicitly what we are direct user of. > > While at it, split out the GPIO group of headers. > > Signed-off-by: Andy Shevchenko Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v4 18/18] gpiolib: Clean up headers
On Wed, Feb 8, 2023 at 6:34 PM Andy Shevchenko wrote: > There is a few things done: > - include only the headers we are direct user of > - when pointer is in use, provide a forward declaration > - add missing headers > - group generic headers and subsystem headers > - sort each group alphabetically > > Signed-off-by: Andy Shevchenko Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v4 02/18] ARM: s3c24xx: Use the right include
On Wed, Feb 8, 2023 at 6:39 PM Krzysztof Kozlowski wrote: > On 08/02/2023 18:33, Andy Shevchenko wrote: > > From: Linus Walleij > > > > The file s3c64xx.c is including despite using no > > symbols from the file, however it needs it to implicitly bring in > > of_have_populated_dt() so include explicitly instead. > > > > Signed-off-by: Linus Walleij > > Signed-off-by: Andy Shevchenko > > --- > > arch/arm/mach-s3c/s3c64xx.c | 2 +- > > It's not s3c24xx anymore, so subject prefix: > ARM: s3c64xx: My mistake, mea culpa. Yours, Linus Walleij
Re: [PATCH v4 03/18] hte: tegra-194: Use proper includes
On 2/8/23 11:03 PM, Andy Shevchenko wrote: > From: Linus Walleij > > The test driver uses the gpiod consumer API so include the right > header. This may cause a problem with > struct of_device_id being implcitly pulled in by the legacy > header so include > explicitly as well. > > While at it, drop explicit moduleparam.h (it's included with module.h) > and sort the headers. > > Signed-off-by: Linus Walleij > Signed-off-by: Andy Shevchenko > --- > drivers/hte/hte-tegra194-test.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/hte/hte-tegra194-test.c b/drivers/hte/hte-tegra194-test.c > index 5d776a185bd6..358d4a10c6a1 100644 > --- a/drivers/hte/hte-tegra194-test.c > +++ b/drivers/hte/hte-tegra194-test.c > @@ -6,14 +6,14 @@ > */ > > #include > -#include > -#include > +#include > +#include > #include > -#include > -#include > +#include > +#include > #include > +#include > #include > -#include > > /* > * This sample HTE GPIO test driver demonstrates HTE API usage by enabling Acked-by: Dipen Patel
[PATCH v3 1/1] PCI: layerscape: Add EP mode support for ls1028a
From: Xiaowei Bao
Add PCIe EP mode support for ls1028a.
Signed-off-by: Xiaowei Bao
Signed-off-by: Hou Zhiqiang
Signed-off-by: Frank Li
Acked-by: Roy Zang
---
Change from v2 to v3
order by .compatible
Change from v2 to v2
Added
Signed-off-by: Frank Li
Acked-by: Roy Zang
All other patches were already accepte by maintainer in
https://lore.kernel.org/lkml/2022223457.10599-1-leoyang...@nxp.com/
But missed this one.
Re-post
drivers/pci/controller/dwc/pci-layerscape-ep.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index ad99707b3b99..c640db60edc6 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -110,6 +110,7 @@ static const struct ls_pcie_ep_drvdata lx2_ep_drvdata = {
};
static const struct of_device_id ls_pcie_ep_of_match[] = {
+ { .compatible = "fsl,ls1028a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
--
2.34.1
RE: [External] : RE: [EXT] [PATCH v2 1/1] PCI: layerscape: Add EP mode support for ls1028a
>
> Caution: EXT Email
>
> yes, it is more about sort the list using .data and .compatible. key
>
> much better if it we keep this as suggested by Frank,
>
> static const struct of_device_id ls_pcie_ep_of_match[] = {
> + { .compatible = "fsl,ls1028a-pcie-ep", .data = &ls1_ep_drvdata },
> { .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> { .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
Thanks, v3 sent.
Frank Li
>
>
>
> Thanks,
>
> Alok
Re: [PATCH] powerpc: Fix device node refcounting
On 2/7/23 9:14 AM, Nathan Lynch wrote: > > (cc'ing a few possibly interested people) > > Brian King writes: >> While testing fixes to the hvcs hotplug code, kmemleak was reporting >> potential memory leaks. This was tracked down to the struct device_node >> object associated with the hvcs device. Looking at the leaked >> object in crash showed that the kref in the kobject in the device_node >> had a reference count of 1 still, and the release function was never >> getting called as a result of this. This adds an of_node_put in >> pSeries_reconfig_remove_node in order to balance the refcounting >> so that we actually free the device_node in the case of it being >> allocated in pSeries_reconfig_add_node. > > My concern here would be whether the additional put is the right thing > to do in all cases. The questions it raises for me are: > > - Is it safe for nodes that were present at boot, instead of added > dynamically? Yes. of_node_release has a check to see if OF_DYNAMIC is set. If it is not set, the release function is a noop. > - Is it correct for all types of nodes, or is there something specific > to hvcs that leaves a dangling refcount? I would welcome more testing and I shared the same concern. I did do some DLPARs of a virtual ethernet device with the change along with CONFIG_PAGE_POISONING enabled and did not run into any issues. However if I do a DLPAR remove of a virtual ethernet device without the change with kmemleak enabled it does not detect any leaked memory. Thanks, Brian > > Just hoping we're not stepping into a situation where we're preventing > leaks in some situations but doing use-after-free in others. :-) > >> >> Signed-off-by: Brian King >> --- >> arch/powerpc/platforms/pseries/reconfig.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/powerpc/platforms/pseries/reconfig.c >> b/arch/powerpc/platforms/pseries/reconfig.c >> index 599bd2c78514..8cb7309b19a4 100644 >> --- a/arch/powerpc/platforms/pseries/reconfig.c >> +++ b/arch/powerpc/platforms/pseries/reconfig.c >> @@ -77,6 +77,7 @@ static int pSeries_reconfig_remove_node(struct device_node >> *np) >> } >> >> of_detach_node(np); >> +of_node_put(np); >> of_node_put(parent); >> return 0; > > In a situation like this where the of_node_put() call isn't obviously > connected to one of the of_ iterator APIs or similar, I would prefer a > comment indicating which "get" it balances. I suppose it corresponds to > the node initialization itself, i.e. the of_node_init() call sites in > pSeries_reconfig_add_node() and drivers/of/fdt.c::populate_node(). -- Brian King Power Linux I/O IBM Linux Technology Center
Re: [PATCH] powerpc: Fix device node refcounting
Brian King writes: > On 2/7/23 9:14 AM, Nathan Lynch wrote: >> Brian King writes: >>> While testing fixes to the hvcs hotplug code, kmemleak was reporting >>> potential memory leaks. This was tracked down to the struct device_node >>> object associated with the hvcs device. Looking at the leaked >>> object in crash showed that the kref in the kobject in the device_node >>> had a reference count of 1 still, and the release function was never >>> getting called as a result of this. This adds an of_node_put in >>> pSeries_reconfig_remove_node in order to balance the refcounting >>> so that we actually free the device_node in the case of it being >>> allocated in pSeries_reconfig_add_node. >> >> My concern here would be whether the additional put is the right thing >> to do in all cases. The questions it raises for me are: >> >> - Is it safe for nodes that were present at boot, instead of added >> dynamically? > > Yes. of_node_release has a check to see if OF_DYNAMIC is set. If it is not > set, > the release function is a noop. Yes, but to be more specific - does the additional of_node_put() risk underflowing the refcount on nodes without the OF_DYNAMIC flag? I suspect it's OK. If it's not, then I would expect to see warnings from the refcount code when that case is exercised. > >> - Is it correct for all types of nodes, or is there something specific >> to hvcs that leaves a dangling refcount? > > I would welcome more testing and I shared the same concern. I did do some > DLPARs of a virtual ethernet device with the change along with > CONFIG_PAGE_POISONING > enabled and did not run into any issues. However if I do a DLPAR remove of a > virtual > ethernet device without the change with kmemleak enabled it does not detect > any > leaked memory. Seems odd. If the change is generically correct, then without it applied I would expect kmemleak to flag a leak on removal of any type of dynamically-added node. On the other hand, if the change is for some reason not correct for virtual ethernet devices, then I would expect it to cause complaints from the refcount code and/or allocator debug facilities. But if I understand correctly, neither of those things is happening.
Re: [PATCH] powerpc: Fix device node refcounting
On 2/9/23 11:11 AM, Nathan Lynch wrote: > Brian King writes: >> On 2/7/23 9:14 AM, Nathan Lynch wrote: >>> Brian King writes: While testing fixes to the hvcs hotplug code, kmemleak was reporting potential memory leaks. This was tracked down to the struct device_node object associated with the hvcs device. Looking at the leaked object in crash showed that the kref in the kobject in the device_node had a reference count of 1 still, and the release function was never getting called as a result of this. This adds an of_node_put in pSeries_reconfig_remove_node in order to balance the refcounting so that we actually free the device_node in the case of it being allocated in pSeries_reconfig_add_node. >>> >>> My concern here would be whether the additional put is the right thing >>> to do in all cases. The questions it raises for me are: >>> >>> - Is it safe for nodes that were present at boot, instead of added >>> dynamically? >> >> Yes. of_node_release has a check to see if OF_DYNAMIC is set. If it is not >> set, >> the release function is a noop. > > Yes, but to be more specific - does the additional of_node_put() risk > underflowing the refcount on nodes without the OF_DYNAMIC flag? I > suspect it's OK. If it's not, then I would expect to see warnings from > the refcount code when that case is exercised. Agreed. I have not seen any refcount underflow warnings in the testing I've done so far. > >> >>> - Is it correct for all types of nodes, or is there something specific >>> to hvcs that leaves a dangling refcount? >> >> I would welcome more testing and I shared the same concern. I did do some >> DLPARs of a virtual ethernet device with the change along with >> CONFIG_PAGE_POISONING >> enabled and did not run into any issues. However if I do a DLPAR remove of a >> virtual >> ethernet device without the change with kmemleak enabled it does not detect >> any >> leaked memory. > > Seems odd. If the change is generically correct, then without it applied > I would expect kmemleak to flag a leak on removal of any type of > dynamically-added node. On the other hand, if the change is for some > reason not correct for virtual ethernet devices, then I would expect it > to cause complaints from the refcount code and/or allocator debug > facilities. But if I understand correctly, neither of those things is > happening. Agreed. I'll do some more testing with and without the change and see what that yields. -Brian -- Brian King Power Linux I/O IBM Linux Technology Center
Re: [PATCH v4 1/7] kcsan: Add atomic builtin stubs for 32-bit systems
> On 9 Feb 2023, at 10:14 am, Rohan McLure wrote:
>
>
>
>> On 8 Feb 2023, at 11:23 pm, Christophe Leroy
>> wrote:
>>
>>
>>
>> Le 08/02/2023 à 04:21, Rohan McLure a écrit :
>>> KCSAN instruments calls to atomic builtins, and will in turn call these
>>> builtins itself. As such, architectures supporting KCSAN must have
>>> compiler support for these atomic primitives.
>>>
>>> Since 32-bit systems are unlikely to have 64-bit compiler builtins,
>>> provide a stub for each missing builtin, and use BUG() to assert
>>> unreachability.
>>>
>>> In commit 725aea873261 ("xtensa: enable KCSAN"), xtensa implements these
>>> locally, but does not advertise the fact with preprocessor macros. To
>>> avoid production of duplicate symbols, do not build the stubs on xtensa.
>>> A future patch will remove the xtensa implementation of these stubs.
>>>
>>> Signed-off-by: Rohan McLure
>>> ---
>>> v4: New patch
>>> ---
>>> kernel/kcsan/Makefile | 3 ++
>>> kernel/kcsan/stubs.c | 78 +++
>>> 2 files changed, 81 insertions(+)
>>> create mode 100644 kernel/kcsan/stubs.c
>>
>> I think it would be better to merge patch 1 and patch 2, that way we
>> would keep the history and see that stubs.c almost comes from xtensa.
>>
>> The summary would then be:
>>
>> arch/xtensa/lib/Makefile | 1 -
>> kernel/kcsan/Makefile | 2 +-
>> arch/xtensa/lib/kcsan-stubs.c => kernel/kcsan/stubs.c | 26
>> +-
>> 3 files changed, 26 insertions(+), 3 deletions(-)
>>
>>
>>>
>>> diff --git a/kernel/kcsan/Makefile b/kernel/kcsan/Makefile
>>> index 8cf70f068d92..5dfc5825aae9 100644
>>> --- a/kernel/kcsan/Makefile
>>> +++ b/kernel/kcsan/Makefile
>>> @@ -12,6 +12,9 @@ CFLAGS_core.o := $(call cc-option,-fno-conserve-stack) \
>>> -fno-stack-protector -DDISABLE_BRANCH_PROFILING
>>>
>>> obj-y := core.o debugfs.o report.o
>>> +ifndef XTENSA
>>> + obj-y += stubs.o
>>
>> obj-$(CONFIG_32BIT) += stubs.o
>>
>> That would avoid the #if CONFIG_32BIT in stubs.c
>
> Thanks. Yes happy to do this.
>
>>
>>> +endif
>>>
>>> KCSAN_INSTRUMENT_BARRIERS_selftest.o := y
>>> obj-$(CONFIG_KCSAN_SELFTEST) += selftest.o
>>> diff --git a/kernel/kcsan/stubs.c b/kernel/kcsan/stubs.c
>>> new file mode 100644
>>> index ..ec5cd99be422
>>> --- /dev/null
>>> +++ b/kernel/kcsan/stubs.c
>>> @@ -0,0 +1,78 @@
>>> +// SPDX-License Identifier: GPL-2.0
>>
>> Missing - between License and Identifier ?
>>
>>> +
>>> +#include
>>> +#include
>>> +
>>> +#ifdef CONFIG_32BIT
>>
>> Should be handled in Makefile
>>
>>> +
>>> +#if !__has_builtin(__atomic_store_8)
>>
>> Does any 32 bit ARCH support that ? Is that #if required ?
>>
>> If yes, do we really need the #if for each and every function, can't we
>> just check for one and assume that if we don't have __atomic_store_8 we
>> don't have any of the functions ?
>
> Turns out that testing with gcc provides 8-byte atomic builtins on x86
> and arm on 32-bit. However I believe it should just suffice to check for
> __atomic_store_8 or any other such builtin i.e. if an arch implements one it
> likely implements them all from what I’ve seen.
In reality, __has_builtin only specifies that GCC is aware of the existance of
the builtin, but linking against libatomic may still be required. Let’s
remove this check, and have ppc32 and xtensa opt into compiling this stubs file.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108734
>
>>
>>
>>> +void __atomic_store_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_load_8)
>>> +u64 __atomic_load_8(const volatile void *p, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_exchange_8)
>>> +u64 __atomic_exchange_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_compare_exchange_8)
>>> +bool __atomic_compare_exchange_8(volatile void *p1, void *p2, u64 v, bool
>>> b, int i1, int i2)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_fetch_add_8)
>>> +u64 __atomic_fetch_add_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_fetch_sub_8)
>>> +u64 __atomic_fetch_sub_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_fetch_and_8)
>>> +u64 __atomic_fetch_and_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_fetch_or_8)
>>> +u64 __atomic_fetch_or_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_fetch_xor_8)
>>> +u64 __atomic_fetch_xor_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BUG();
>>> +}
>>> +#endif
>>> +
>>> +#if !__has_builtin(__atomic_fetch_nand_8)
>>> +u64 __atomic_fetch_nand_8(volatile void *p, u64 v, int i)
>>> +{
>>> + BU
Re: [PATCH 1/3] powerpc/code-patching: Add generic memory patching
On Thu, 2023-02-09 at 07:15 +, Christophe Leroy wrote:
> > +static inline int patch_uint(u32 *addr, unsigned int val)
> > +{
> > + return patch_instruction(addr, ppc_inst(val));
>
> Would it make more sense that patch_instruction() calls patch_uint()
> instead of the reverse ?
>
That's what I had originally, but I figured it would be nicer to see
'patch_instruction' in the disassembly given it's still the main usage.
It's equivalent otherwise though.
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index b00112d7ad46..0f7e9949faf0 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -20,16 +20,14 @@
> > #include
> > #include
> >
> > -static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr,
> > u32 *patch_addr)
> > +static int __patch_memory(void *exec_addr, unsigned long val, void
> > *patch_addr,
> > + bool is_dword)
> > {
> > - if (!ppc_inst_prefixed(instr)) {
> > - u32 val = ppc_inst_val(instr);
> > -
> > - __put_kernel_nofault(patch_addr, &val, u32,
> > failed);
> > - } else {
> > - u64 val = ppc_inst_as_ulong(instr);
> > -
> > + if (IS_ENABLED(CONFIG_PPC64) && unlikely(is_dword)) {
> > __put_kernel_nofault(patch_addr, &val, u64,
> > failed);
> > + } else {
> > + unsigned int val32 = val;
>
> Why unsigned int and not u32 as before ?
>
No particular reason, I just tend to use int/long over 32/64 in code
compiled on 32 bit as well and there was a long period of time between
removing the original vars and fixing the big endian issue.
> > +#ifdef CONFIG_PPC64
> > +
> > +int patch_instruction(u32 *addr, ppc_inst_t instr)
> > +{
> > + if (ppc_inst_prefixed(instr))
> > + return patch_memory(addr, ppc_inst_as_ulong(instr),
> > true);
> > + else
> > + return patch_memory(addr, ppc_inst_val(instr),
> > false);
> > +}
> > +NOKPROBE_SYMBOL(patch_instruction)
> > +
> > +int patch_uint(void *addr, unsigned int val)
> > +{
> > + return patch_memory(addr, val, false);
> > +}
> > +NOKPROBE_SYMBOL(patch_uint)
> > +
> > +int patch_ulong(void *addr, unsigned long val)
> > +{
> > + return patch_memory(addr, val, true);
> > +}
> > +NOKPROBE_SYMBOL(patch_ulong)
> > +
> > +#else
> > +
> > +noinline int patch_instruction(u32 *addr, ppc_inst_t instr)
> > +{
> > + return patch_memory(addr, ppc_inst_val(instr), false);
> > +}
>
> A comment explaining the reason for the noinline would be welcome
> here.
Yeah makes sense
> By the way, would the noinline change anything on PPC64 ? If not we
> could have a common function as ppc_inst_prefixed() constant folds to
> false in PPC32.
On ppc64 that prevents patch_branch() from calling patch_memory()
directly with is_dword=false.
Re: [PATCH v2 1/2] powerpc/ps3: Change updateboltedpp panic to info
Geoff Levand writes: > On 1/16/23 23:26, Christophe Leroy wrote: >> Le 16/01/2023 à 21:08, Geoff Levand a écrit : >>> >>> As mentioned, I'd really like to keep PS3 included in ppc64_defconfig. My >>> original patch that basically just ignores the call to >>> mmu_hash_ops.updateboltedpp allows that, and I haven't experienced any >>> problems >>> with it yet. >> >> When you say you have not experienced any problems with it, do you mean >> that STRICT_RWX works for you ? When you select CONFIG_DEBUG_RODATA_TEST >> it tells you that it works ? Otherwise it looks incorrect to enable >> something that doesn't work. > > What I mean is that the system boots up, and works as expected. > I have not tried with CONFIG_DEBUG_RODATA_TEST set. > >>> My preference would be to keep PS3 in ppc64_defconfig, and either apply my >>> original patch, or I keep that patch in my ps3-linux repo on kernel.org. >>> Then, >>> if we end up adding runtime support for RWX I then fixup PS3 to use that. >>> >> >> In that case I see two solutions: >> 1/ Implement updateboltedpp for PS3. > > I'm now looking into if this is possible. > >> 2/ Implement arch_parse_debug_rodata() to always set rodata_enabled = >> false on PS3, and update free_initmem() to only call mark_initmem_nx() >> when strict_kernel_rwx_enabled() returns true. > > OK, I'll consider this if I cannot get updateboltedp working. I'll take this series as-is for 6.3, there's no point panicking. Hopefully one of the above options can be implemented in future. cheers
Re: [PATCH v4 08/18] gpiolib: remove gpio_set_debounce()
On Wed, Feb 08, 2023 at 07:33:33PM +0200, Andy Shevchenko wrote: > From: Arnd Bergmann > > gpio_set_debounce() only has a single user, which is trivially > converted to gpiod_set_debounce(). > > Signed-off-by: Arnd Bergmann > Reviewed-by: Linus Walleij > Reviewed-by: Andy Shevchenko > Signed-off-by: Andy Shevchenko Acked-by: Dmitry Torokhov Thanks. -- Dmitry
[powerpc:next] BUILD SUCCESS 6acecfa485d3de955c35a18730c106ddf1e7600e
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next branch HEAD: 6acecfa485d3de955c35a18730c106ddf1e7600e powerpc/kcsan: Add KCSAN Support elapsed time: 747m configs tested: 62 configs skipped: 2 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: um x86_64_defconfig um i386_defconfig powerpc allnoconfig arc defconfig x86_64allnoconfig s390 allmodconfig alpha defconfig s390defconfig s390 allyesconfig sh allmodconfig m68k allyesconfig m68k allmodconfig arc allyesconfig alphaallyesconfig x86_64 defconfig mips allyesconfig powerpc allmodconfig x86_64 rhel-8.3 ia64 allmodconfig x86_64randconfig-a006 x86_64 allyesconfig i386 randconfig-a001 i386 randconfig-a003 x86_64 rhel-8.3-bpf i386 randconfig-a016 x86_64 rhel-8.3-syz x86_64 rhel-8.3-kunit i386 randconfig-a005 x86_64 rhel-8.3-kvm arc randconfig-r043-20230209 arm randconfig-r046-20230209 x86_64randconfig-a004 i386 randconfig-a014 x86_64randconfig-a013 i386defconfig i386 randconfig-a012 x86_64randconfig-a011 x86_64randconfig-a002 x86_64randconfig-a015 arm defconfig x86_64rhel-8.3-kselftests x86_64 rhel-8.3-func arm64allyesconfig arm allyesconfig i386 allyesconfig clang tested configs: s390 randconfig-r044-20230209 i386 randconfig-a006 i386 randconfig-a002 i386 randconfig-a004 hexagon randconfig-r045-20230209 hexagon randconfig-r041-20230209 x86_64randconfig-a016 riscvrandconfig-r042-20230209 x86_64randconfig-a012 x86_64randconfig-a014 i386 randconfig-a013 i386 randconfig-a011 i386 randconfig-a015 x86_64randconfig-a005 x86_64randconfig-a001 x86_64randconfig-a003 x86_64 rhel-8.3-rust -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
linux-next: build failure after merge of the powerpc tree
Hi all,
After merging the powerpc tree, today's linux-next build (powerpc64
allnoconfig) failed like this:
arch/powerpc/kernel/setup_64.c: In function 'early_setup':
arch/powerpc/kernel/setup_64.c:400:34: error: 'struct thread_info' has no
member named 'cpu'
400 | task_thread_info(current)->cpu = boot_cpuid; // fix
task_cpu(current)
| ^~
Caused by commit
0ecf51ca51e5 ("powerpc/64: Fix task_cpu in early boot when booting non-zero
cpuid")
# CONFIG_SMP is not set
I applied the following fix up for today.
From: Stephen Rothwell
Date: Fri, 10 Feb 2023 14:21:33 +1100
Subject: [PATCH] fixup for "powerpc/64: Fix task_cpu in early boot when booting
non-zero cpuid"
Signed-off-by: Stephen Rothwell
---
arch/powerpc/kernel/setup_64.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 78d8a105764b..b2e0d3ce4261 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -397,7 +397,9 @@ void __init early_setup(unsigned long dt_ptr)
setup_paca(paca_ptrs[boot_cpuid]); /* install the paca into registers */
// smp_processor_id() now reports boot_cpuid
+#ifdef CONFIG_SMP
task_thread_info(current)->cpu = boot_cpuid; // fix task_cpu(current)
+#endif
/*
* Configure exception handlers. This include setting up trampolines
--
2.39.1
--
Cheers,
Stephen Rothwell
pgpvPhlPyCYGI.pgp
Description: OpenPGP digital signature
Re: [PATCH mm-unstable v1 17/26] powerpc/mm: support __HAVE_ARCH_PTE_SWP_EXCLUSIVE on 32bit book3s
David Hildenbrand writes:
> We already implemented support for 64bit book3s in commit bff9beaa2e80
> ("powerpc/pgtable: support __HAVE_ARCH_PTE_SWP_EXCLUSIVE for book3s")
>
> Let's support __HAVE_ARCH_PTE_SWP_EXCLUSIVE also in 32bit by reusing yet
> unused LSB 2 / MSB 29. There seems to be no real reason why that bit cannot
> be used, and reusing it avoids having to steal one bit from the swap
> offset.
>
> While at it, mask the type in __swp_entry().
>
> Cc: Michael Ellerman
> Cc: Nicholas Piggin
> Cc: Christophe Leroy
> Signed-off-by: David Hildenbrand
> ---
> arch/powerpc/include/asm/book3s/32/pgtable.h | 38 +---
> 1 file changed, 33 insertions(+), 5 deletions(-)
I gave this a quick test on a ppc32 machine, everything seems fine.
Your test_swp_exclusive.c passes, and an LTP run looks normal.
Acked-by: Michael Ellerman (powerpc)
cheers
> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h
> b/arch/powerpc/include/asm/book3s/32/pgtable.h
> index 75823f39e042..0ecb3a58f23f 100644
> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> @@ -42,6 +42,9 @@
> #define _PMD_PRESENT_MASK (PAGE_MASK)
> #define _PMD_BAD (~PAGE_MASK)
>
> +/* We borrow the _PAGE_USER bit to store the exclusive marker in swap PTEs.
> */
> +#define _PAGE_SWP_EXCLUSIVE _PAGE_USER
> +
> /* And here we include common definitions */
>
> #define _PAGE_KERNEL_RO 0
> @@ -363,17 +366,42 @@ static inline void __ptep_set_access_flags(struct
> vm_area_struct *vma,
> #define pmd_page(pmd)pfn_to_page(pmd_pfn(pmd))
>
> /*
> - * Encode and decode a swap entry.
> - * Note that the bits we use in a PTE for representing a swap entry
> - * must not include the _PAGE_PRESENT bit or the _PAGE_HASHPTE bit (if used).
> - * -- paulus
> + * Encode/decode swap entries and swap PTEs. Swap PTEs are all PTEs that
> + * are !pte_none() && !pte_present().
> + *
> + * Format of swap PTEs (32bit PTEs):
> + *
> + * 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3
> + * 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> + * <- offset > < type -> E H P
> + *
> + * E is the exclusive marker that is not stored in swap entries.
> + * _PAGE_PRESENT (P) and __PAGE_HASHPTE (H) must be 0.
> + *
> + * For 64bit PTEs, the offset is extended by 32bit.
> */
> #define __swp_type(entry)((entry).val & 0x1f)
> #define __swp_offset(entry) ((entry).val >> 5)
> -#define __swp_entry(type, offset)((swp_entry_t) { (type) | ((offset) <<
> 5) })
> +#define __swp_entry(type, offset)((swp_entry_t) { ((type) & 0x1f) |
> ((offset) << 5) })
> #define __pte_to_swp_entry(pte) ((swp_entry_t) { pte_val(pte)
> >> 3 })
> #define __swp_entry_to_pte(x)((pte_t) { (x).val << 3 })
>
> +#define __HAVE_ARCH_PTE_SWP_EXCLUSIVE
> +static inline int pte_swp_exclusive(pte_t pte)
> +{
> + return pte_val(pte) & _PAGE_SWP_EXCLUSIVE;
> +}
> +
> +static inline pte_t pte_swp_mkexclusive(pte_t pte)
> +{
> + return __pte(pte_val(pte) | _PAGE_SWP_EXCLUSIVE);
> +}
> +
> +static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> +{
> + return __pte(pte_val(pte) & ~_PAGE_SWP_EXCLUSIVE);
> +}
> +
> /* Generic accessors to PTE bits */
> static inline int pte_write(pte_t pte) { return
> !!(pte_val(pte) & _PAGE_RW);}
> static inline int pte_read(pte_t pte){ return 1; }
> --
> 2.39.0
>
>
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v2 01/19] powerpc/rtas: handle extended delays safely in early boot
Nathan Lynch writes:
> Michael Ellerman writes:
>> Nathan Lynch via B4 Submission Endpoint
>> writes:
>>> From: Nathan Lynch
>>>
>>> Some code that runs early in boot calls RTAS functions that can return
>>> -2 or 990x statuses, which mean the caller should retry. An example is
>>> pSeries_cmo_feature_init(), which invokes ibm,get-system-parameter but
>>> treats these benign statuses as errors instead of retrying.
>>>
>>> pSeries_cmo_feature_init() and similar code should be made to retry
>>> until they succeed or receive a real error, using the usual pattern:
>>>
>>> do {
>>> rc = rtas_call(token, etc...);
>>> } while (rtas_busy_delay(rc));
>>>
>>> But rtas_busy_delay() will perform a timed sleep on any 990x
>>> status. This isn't safe so early in boot, before the CPU scheduler and
>>> timer subsystem have initialized.
>>>
>>> The -2 RTAS status is much more likely to occur during single-threaded
>>> boot than 990x in practice, at least on PowerVM. This is because -2
>>> usually means that RTAS made progress but exhausted its self-imposed
>>> timeslice, while 990x is associated with concurrent requests from the
>>> OS causing internal contention. Regardless, according to the language
>>> in PAPR, the OS should be prepared to handle either type of status at
>>> any time.
>>>
>>> Add a fallback path to rtas_busy_delay() to handle this as safely as
>>> possible, performing a small delay on 990x. Include a counter to
>>> detect retry loops that aren't making progress and bail out.
>>>
>>> This was found by inspection and I'm not aware of any real
>>> failures. However, the implementation of rtas_busy_delay() before
>>> commit 38f7b7067dae ("powerpc/rtas: rtas_busy_delay() improvements")
>>> was not susceptible to this problem, so let's treat this as a
>>> regression.
>>>
>>> Signed-off-by: Nathan Lynch
>>> Fixes: 38f7b7067dae ("powerpc/rtas: rtas_busy_delay() improvements")
>>> ---
>>> arch/powerpc/kernel/rtas.c | 48
>>> +-
>>> 1 file changed, 47 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index 795225d7f138..ec2df09a70cf 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -606,6 +606,46 @@ unsigned int rtas_busy_delay_time(int status)
>>> return ms;
>>> }
>>>
>>> +/*
>>> + * Early boot fallback for rtas_busy_delay().
>>> + */
>>> +static bool __init rtas_busy_delay_early(int status)
>>> +{
>>> + static size_t successive_ext_delays __initdata;
>>> + bool ret;
>>
>> I think the logic would be easier to read if this was called "wait", but
>> maybe that's just me.
>
> Maybe "retry"? That communicates what the function is telling callers to do.
Yeah, that's even better.
cheers
Re: [PATCH v2 11/19] powerpc/rtas: add work area allocator
Nathan Lynch writes:
> Michael Ellerman writes:
>> Nathan Lynch via B4 Submission Endpoint
>> writes:
...
>>> +struct rtas_work_area * __ref rtas_work_area_alloc(size_t size)
>>> +{
>>> + struct rtas_work_area *area;
>>> + unsigned long addr;
>>> +
>>> + might_sleep();
>>> +
>>> + WARN_ON(size > RTAS_WORK_AREA_MAX_ALLOC_SZ);
>>> + size = min_t(size_t, size, RTAS_WORK_AREA_MAX_ALLOC_SZ);
>>
>> This seems unsafe.
>>
>> If you return a buffer smaller than the caller asks for they're likely
>> to read/write past the end of it and corrupt memory.
>
> OK, let's figure out another way to handle this.
>
>> AFAIK genalloc doesn't have guard pages or anything fancy to save us
>> from that - but maybe I'm wrong, I've never used it.
>
> Yeah we would have to build our own thing on top of it. And I don't
> think it could be something that traps on access, it would have to be a
> check in rtas_work_area_free(), after the fact.
I *think* we could use the MMU. We'd just have to allocate whole pages,
and then vmap() them (create a mapping in vmalloc space), and then give
the vmalloc space address back to the caller. They'd then operate on
that address, meaning any overflow would trap. You already have
rtas_work_area_phys() for passing the phys address to RTAS.
But that would be a lot more complicated than your suggestion below.
>> There's only three callers in the end, seems like we should just return
>> NULL if the size is too large and have callers check the return value.
>
> There are more conversions to do, and a property I hope to maintain is
> that requests can't fail. Existing users of rtas_data_buf don't have
> error paths for failure to acquire the buffer.
>
> I believe the allocation size passed to rtas_work_area_alloc() can be
> known at build time in all cases. Maybe we could prevent inappropriate
> requests from being built with a compile-time assertion (untested):
>
> /* rtas-work-area.h */
>
> static inline struct rtas_work_area *rtas_work_area_alloc(size_t sz)
> {
> static_assert(sz < RTAS_WORK_AREA_MAX_ALLOC_SZ);
> return __rtas_work_area_alloc(sz);
> }
>
> I think this would be OK? If I can't make it work I'll fall back to
> returning NULL as you suggest, but it will make for more churn (and
> risk) in the conversions.
Yeah if the sizes are always known at compile time that is a much better
solution.
cheers
