LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] arch: powerpc: kernel: signal_32.c:  Remove unused function
From: Rickard Strandqvist @ 2014-12-07 22:07 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras
  Cc: Michael Neuling, Rickard Strandqvist, linux-kernel,
	Richard Weinberger, linuxppc-dev

Remove the function sys_debug_setcontext() that is not used anywhere.

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/kernel/signal_32.c |  107 ---------------------------------------
 1 file changed, 107 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index b171001..a571614 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -1297,113 +1297,6 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int r6, int r7, int r8,
 	return 0;
 }
 
-#ifdef CONFIG_PPC32
-int sys_debug_setcontext(struct ucontext __user *ctx,
-			 int ndbg, struct sig_dbg_op __user *dbg,
-			 int r6, int r7, int r8,
-			 struct pt_regs *regs)
-{
-	struct sig_dbg_op op;
-	int i;
-	unsigned char tmp;
-	unsigned long new_msr = regs->msr;
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
-	unsigned long new_dbcr0 = current->thread.debug.dbcr0;
-#endif
-
-	for (i=0; i<ndbg; i++) {
-		if (copy_from_user(&op, dbg + i, sizeof(op)))
-			return -EFAULT;
-		switch (op.dbg_type) {
-		case SIG_DBG_SINGLE_STEPPING:
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
-			if (op.dbg_value) {
-				new_msr |= MSR_DE;
-				new_dbcr0 |= (DBCR0_IDM | DBCR0_IC);
-			} else {
-				new_dbcr0 &= ~DBCR0_IC;
-				if (!DBCR_ACTIVE_EVENTS(new_dbcr0,
-						current->thread.debug.dbcr1)) {
-					new_msr &= ~MSR_DE;
-					new_dbcr0 &= ~DBCR0_IDM;
-				}
-			}
-#else
-			if (op.dbg_value)
-				new_msr |= MSR_SE;
-			else
-				new_msr &= ~MSR_SE;
-#endif
-			break;
-		case SIG_DBG_BRANCH_TRACING:
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
-			return -EINVAL;
-#else
-			if (op.dbg_value)
-				new_msr |= MSR_BE;
-			else
-				new_msr &= ~MSR_BE;
-#endif
-			break;
-
-		default:
-			return -EINVAL;
-		}
-	}
-
-	/* We wait until here to actually install the values in the
-	   registers so if we fail in the above loop, it will not
-	   affect the contents of these registers.  After this point,
-	   failure is a problem, anyway, and it's very unlikely unless
-	   the user is really doing something wrong. */
-	regs->msr = new_msr;
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
-	current->thread.debug.dbcr0 = new_dbcr0;
-#endif
-
-	if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
-	    || __get_user(tmp, (u8 __user *) ctx)
-	    || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
-		return -EFAULT;
-
-	/*
-	 * If we get a fault copying the context into the kernel's
-	 * image of the user's registers, we can't just return -EFAULT
-	 * because the user's registers will be corrupted.  For instance
-	 * the NIP value may have been updated but not some of the
-	 * other registers.  Given that we have done the access_ok
-	 * and successfully read the first and last bytes of the region
-	 * above, this should only happen in an out-of-memory situation
-	 * or if another thread unmaps the region containing the context.
-	 * We kill the task with a SIGSEGV in this situation.
-	 */
-	if (do_setcontext(ctx, regs, 1)) {
-		if (show_unhandled_signals)
-			printk_ratelimited(KERN_INFO "%s[%d]: bad frame in "
-					   "sys_debug_setcontext: %p nip %08lx "
-					   "lr %08lx\n",
-					   current->comm, current->pid,
-					   ctx, regs->nip, regs->link);
-
-		force_sig(SIGSEGV, current);
-		goto out;
-	}
-
-	/*
-	 * It's not clear whether or why it is desirable to save the
-	 * sigaltstack setting on signal delivery and restore it on
-	 * signal return.  But other architectures do this and we have
-	 * always done it up until now so it is probably better not to
-	 * change it.  -- paulus
-	 */
-	restore_altstack(&ctx->uc_stack);
-
-	set_thread_flag(TIF_RESTOREALL);
- out:
-	return 0;
-}
-#endif
-
 /*
  * OK, we're invoking a handler
  */
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH] arch: powerpc: kernel: traps.c:  Remove unused function
From: Rickard Strandqvist @ 2014-12-07 20:07 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras
  Cc: Michael Neuling, Rickard Strandqvist, Mahesh Salgaonkar,
	linux-kernel, Anton Blanchard, Tejun Heo, Christoph Lameter,
	linuxppc-dev

Remove the function vsx_assist_exception() that is not used anywhere.

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/kernel/traps.c |   15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0dc43f9..857e5f7 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1707,21 +1707,6 @@ void altivec_assist_exception(struct pt_regs *regs)
 }
 #endif /* CONFIG_ALTIVEC */
 
-#ifdef CONFIG_VSX
-void vsx_assist_exception(struct pt_regs *regs)
-{
-	if (!user_mode(regs)) {
-		printk(KERN_EMERG "VSX assist exception in kernel mode"
-		       " at %lx\n", regs->nip);
-		die("Kernel VSX assist exception", regs, SIGILL);
-	}
-
-	flush_vsx_to_thread(current);
-	printk(KERN_INFO "VSX assist not supported at %lx\n", regs->nip);
-	_exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
-}
-#endif /* CONFIG_VSX */
-
 #ifdef CONFIG_FSL_BOOKE
 void CacheLockingException(struct pt_regs *regs, unsigned long address,
 			   unsigned long error_code)
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH] arch: powerpc: kvm: book3s_pr.c:  Remove unused function
From: Rickard Strandqvist @ 2014-12-07 18:11 UTC (permalink / raw)
  To: Alexander Graf, Gleb Natapov
  Cc: kvm, Rickard Strandqvist, linux-kernel, kvm-ppc, Paul Mackerras,
	Paolo Bonzini, linuxppc-dev

Remove the function get_fpr_index() that is not used anywhere.

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/kvm/book3s_pr.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index cf2eb16..f573839 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -644,11 +644,6 @@ int kvmppc_handle_pagefault(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	return r;
 }
 
-static inline int get_fpr_index(int i)
-{
-	return i * TS_FPRWIDTH;
-}
-
 /* Give up external provider (FPU, Altivec, VSX) */
 void kvmppc_giveup_ext(struct kvm_vcpu *vcpu, ulong msr)
 {
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH] arch: powerpc: mm: slice.c:  Remove unused function
From: Rickard Strandqvist @ 2014-12-07 18:10 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras
  Cc: Michael Neuling, jmarchan@redhat.com, Rickard Strandqvist,
	linux-kernel, Aneesh Kumar K.V, linuxppc-dev, Ian Munsie,
	Anton Blanchard

Remove the function slice_set_psize() that is not used anywhere.

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/mm/slice.c |   29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index ded0ea1..0f432a7 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -645,35 +645,6 @@ void slice_set_user_psize(struct mm_struct *mm, unsigned int psize)
 	spin_unlock_irqrestore(&slice_convert_lock, flags);
 }
 
-void slice_set_psize(struct mm_struct *mm, unsigned long address,
-		     unsigned int psize)
-{
-	unsigned char *hpsizes;
-	unsigned long i, flags;
-	u64 *lpsizes;
-
-	spin_lock_irqsave(&slice_convert_lock, flags);
-	if (address < SLICE_LOW_TOP) {
-		i = GET_LOW_SLICE_INDEX(address);
-		lpsizes = &mm->context.low_slices_psize;
-		*lpsizes = (*lpsizes & ~(0xful << (i * 4))) |
-			((unsigned long) psize << (i * 4));
-	} else {
-		int index, mask_index;
-		i = GET_HIGH_SLICE_INDEX(address);
-		hpsizes = mm->context.high_slices_psize;
-		mask_index = i & 0x1;
-		index = i >> 1;
-		hpsizes[index] = (hpsizes[index] &
-				  ~(0xf << (mask_index * 4))) |
-			(((unsigned long)psize) << (mask_index * 4));
-	}
-
-	spin_unlock_irqrestore(&slice_convert_lock, flags);
-
-	copro_flush_all_slbs(mm);
-}
-
 void slice_set_range_psize(struct mm_struct *mm, unsigned long start,
 			   unsigned long len, unsigned int psize)
 {
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH] arch: powerpc: kernel: sys_ppc32.c:  Remove unused function
From: Rickard Strandqvist @ 2014-12-07 17:56 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras
  Cc: linuxppc-dev, linux-kernel, Rickard Strandqvist

Remove the function compat_sys_mmap2() that is not used anywhere.

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/kernel/sys_ppc32.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
index 8a28587..8f32cf8 100644
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -61,14 +61,6 @@ asmlinkage long ppc32_select(u32 n, compat_ulong_t __user *inp,
 	return compat_sys_select((int)n, inp, outp, exp, compat_ptr(tvp_x));
 }
 
-unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
-			  unsigned long prot, unsigned long flags,
-			  unsigned long fd, unsigned long pgoff)
-{
-	/* This should remain 12 even if PAGE_SIZE changes */
-	return sys_mmap(addr, len, prot, flags, fd, pgoff << 12);
-}
-
 /* 
  * long long munging:
  * The 32 bit ABI passes long longs in an odd even register pair.
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH] arch: powerpc: kvm: book3s.c:  Remove some unused functions
From: Rickard Strandqvist @ 2014-12-07 17:28 UTC (permalink / raw)
  To: Alexander Graf, Gleb Natapov
  Cc: kvm, Rickard Strandqvist, linux-kernel, kvm-ppc, Paul Mackerras,
	Paolo Bonzini, linuxppc-dev

Removes some functions that are not used anywhere:
kvmppc_core_load_guest_debugstate() kvmppc_core_load_host_debugstate()

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/kvm/book3s.c |    8 --------
 arch/powerpc/kvm/e500.c   |    8 --------
 2 files changed, 16 deletions(-)

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index b32db4b..888bf46 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -64,14 +64,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ NULL }
 };
 
-void kvmppc_core_load_host_debugstate(struct kvm_vcpu *vcpu)
-{
-}
-
-void kvmppc_core_load_guest_debugstate(struct kvm_vcpu *vcpu)
-{
-}
-
 void kvmppc_unfixup_split_real(struct kvm_vcpu *vcpu)
 {
 	if (vcpu->arch.hflags & BOOK3S_HFLAG_SPLIT_HACK) {
diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
index 2e02ed8..be4f3fd 100644
--- a/arch/powerpc/kvm/e500.c
+++ b/arch/powerpc/kvm/e500.c
@@ -299,14 +299,6 @@ void kvmppc_mmu_msr_notify(struct kvm_vcpu *vcpu, u32 old_msr)
 	kvmppc_e500_recalc_shadow_pid(to_e500(vcpu));
 }
 
-void kvmppc_core_load_host_debugstate(struct kvm_vcpu *vcpu)
-{
-}
-
-void kvmppc_core_load_guest_debugstate(struct kvm_vcpu *vcpu)
-{
-}
-
 static void kvmppc_core_vcpu_load_e500(struct kvm_vcpu *vcpu, int cpu)
 {
 	kvmppc_booke_vcpu_load(vcpu, cpu);
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH] arch: powerpc: kvm: book3s_32_mmu.c:  Remove unused function
From: Rickard Strandqvist @ 2014-12-07 17:20 UTC (permalink / raw)
  To: Alexander Graf, Gleb Natapov
  Cc: kvm, Rickard Strandqvist, linux-kernel, kvm-ppc, Paul Mackerras,
	Paolo Bonzini, linuxppc-dev

Remove the function sr_nx() that is not used anywhere.

This was partially found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 arch/powerpc/kvm/book3s_32_mmu.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_32_mmu.c b/arch/powerpc/kvm/book3s_32_mmu.c
index cd0b073..a2eb6d3 100644
--- a/arch/powerpc/kvm/book3s_32_mmu.c
+++ b/arch/powerpc/kvm/book3s_32_mmu.c
@@ -78,11 +78,6 @@ static inline bool sr_kp(u32 sr_raw)
 	return (sr_raw & 0x20000000) ? true: false;
 }
 
-static inline bool sr_nx(u32 sr_raw)
-{
-	return (sr_raw & 0x10000000) ? true: false;
-}
-
 static int kvmppc_mmu_book3s_32_xlate_bat(struct kvm_vcpu *vcpu, gva_t eaddr,
 					  struct kvmppc_pte *pte, bool data,
 					  bool iswrite);
-- 
1.7.10.4

^ permalink raw reply related

* Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
From: Sukadev Bhattiprolu @ 2014-12-07  7:37 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Michael Ellerman, peterz, linux-kernel, Arnaldo Carvalho de Melo,
	dev, Paul Mackerras, linuxppc-dev
In-Reply-To: <20141204124422.GA4195@krava.brq.redhat.com>

Jiri Olsa [jolsa@redhat.com] wrote:

| anyway we could assign directly to the param term name as you do,
| but I think we just need to mark the term as parametrized, like:
| 
| in /sys/bus/event_source/devices/pmu/events/event_name you have:
|   param2=?,bar=1,param1=?

I like the idea of just using a single ? for required parameters, but
the problem I had with this approach can be seen with these two sysfs
entries:

        $ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
        domain=0x2,offset=0xe0,starting_index=core,lpar=0x0

        $ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
        domain=0x3,offset=0xe0,starting_index=vcpu,lpar=sibling_guest_id

The parameter 'starting_index' refers to a core in one event and vcpu in
another event. We were trying to give a hint as to what it refers to.

Given that, 'starting_index' is not very intuitive, how about discarding
starting_index and replacing with what it really means for the event and,
use a simple '?' to indicate required parameter).

        $ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE
        domain=0x2,offset=0xe0,core=?,lpar=0x0

        $ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE
        domain=0x3,offset=0xe0,vcpu=?,lpar=?

perf list shows these as:

	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=?/ 
	hv_24x7/HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CHIP,vcpu=?,lpar=?/

command line would be

	-e hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=2/ 

	or

	-e hv_24x7/HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CHIP,vcpu=2,lpar=7/

and would fail if a required parameter is missing.

This would eliminate the need for new strings like 'sibling_guest_id' (or
as Cody calls it monopolizing strings...)

Following quick patch on top of the patchset shows the changes:

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 73d5bfc..a82bc64 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -27,6 +27,8 @@
 #include "hv-24x7-catalog.h"
 #include "hv-common.h"
 
+
+#if 0
 static const char *domain_to_index_string(unsigned domain)
 {
 	switch (domain) {
@@ -40,6 +42,7 @@ static const char *domain_to_index_string(unsigned domain)
 		return "UNKNOWN_DOMAIN_INDEX_STRING";
 	}
 }
+#endif
 
 static const char *event_domain_suffix(unsigned domain)
 {
@@ -114,7 +117,8 @@ static bool catalog_entry_domain_is_valid(unsigned domain)
 /* u3 0-6, one of HV_24X7_PERF_DOMAIN */
 EVENT_DEFINE_RANGE_FORMAT(domain, config, 0, 3);
 /* u16 */
-EVENT_DEFINE_RANGE_FORMAT(starting_index, config, 16, 31);
+EVENT_DEFINE_RANGE_FORMAT(core, config, 16, 31);
+EVENT_DEFINE_RANGE_FORMAT(vcpu, config, 16, 31);
 /* u32, see "data_offset" */
 EVENT_DEFINE_RANGE_FORMAT(offset, config, 32, 63);
 /* u16 */
@@ -127,7 +131,8 @@ EVENT_DEFINE_RANGE(reserved3, config2,  0, 63);
 static struct attribute *format_attrs[] = {
 	&format_attr_domain.attr,
 	&format_attr_offset.attr,
-	&format_attr_starting_index.attr,
+	&format_attr_core.attr,
+	&format_attr_vcpu.attr,
 	&format_attr_lpar.attr,
 	NULL,
 };
@@ -280,19 +285,23 @@ static unsigned core_domains[] = {
 
 static char *event_fmt(struct hv_24x7_event_data *event, unsigned domain)
 {
+	const char *sindex;
 	const char *lpar;
 
-	if (is_physical_domain(domain))
+	if (is_physical_domain(domain)) {
 		lpar = "0x0";
-	else
-		lpar = "$sibling_guest_id";
+		sindex = "core";
+	} else {
+		lpar = "?";
+		sindex = "vcpu";
+	}
 
 	return kasprintf(GFP_KERNEL,
-			"domain=0x%x,offset=0x%x,starting_index=%s,lpar=%s",
+			"domain=0x%x,offset=0x%x,%s=?,lpar=%s",
 			domain,
 			be16_to_cpu(event->event_counter_offs) +
 				be16_to_cpu(event->event_group_record_offs),
-			domain_to_index_string(domain),
+			sindex,
 			lpar);
 }
 
@@ -1061,9 +1070,17 @@ out:
 static unsigned long event_24x7_request(struct perf_event *event, u64 *res,
 		bool success_expected)
 {
+	u16 idx;
+	unsigned domain = event_get_domain(event);
+
+	if (is_physical_domain(domain))
+		idx = event_get_core(event);
+	else
+		idx = event_get_vcpu(event);
+
 	return single_24x7_request(event_get_domain(event),
 				event_get_offset(event),
-				event_get_starting_index(event),
+				idx,
 				event_get_lpar(event),
 				res,
 				success_expected);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index f8674c1..d208fef 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -826,7 +826,7 @@ static char *format_alias(char *buf, int len, struct perf_pmu *pmu,
 	list_for_each_entry(term, &alias->terms, list)
 		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
 			used += snprintf(buf + used, sub_non_neg(len, used),
-					",%s=$%s", term->config,
+					",%s=%s", term->config,
 					term->val.str);
 
 	if (sub_non_neg(len, used) > 0) {

^ permalink raw reply related

* Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
From: Jiri Olsa @ 2014-12-06 12:20 UTC (permalink / raw)
  To: Cody P Schafer
  Cc: Michael Ellerman, Peter Zijlstra, LKML, Arnaldo Carvalho de Melo,
	Paul Mackerras, Sukadev Bhattiprolu, Linux PPC
In-Reply-To: <CAPoQQ-1m9oRQ7wLfRHFNNZyw7fvCoMa_DX7msP2QsY_i2QyHug@mail.gmail.com>

On Fri, Dec 05, 2014 at 06:05:26PM -0500, Cody P Schafer wrote:
> On Thu, Dec 4, 2014 at 7:44 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote:
> >> From: Cody P Schafer <cody@linux.vnet.ibm.com>
> >>
> >> Enable event specification like:
> >>
> >>       pmu/event_name,param1=0x1,param2=0x4/
> >>
> >> Assuming that
> >>
> >>       /sys/bus/event_source/devices/pmu/events/event_name
> >>
> >> Contains something like
> >>
> >>       param2=$foo,bar=1,param1=$baz
> >
> > oops.. sorry to be PITA on this one.. I might have missed something
> > in the previous discussion but I guess I might have finally some
> > opinion on this ;-)
> >
> > here's how I think your patchset works:
> >
> > in /sys/bus/event_source/devices/pmu/events/event_name you can actually have:
> >
> >    param2=foo,bar=1,param1=baz
> >
> > notice no '$', thats what you add later in 'perf list' output, right?
> >
> > Moreover it actually does not matter whats in value 'param2=HERE',
> > because it's not used in the config code at all apart from the
> > 'perf list' display processing.
> >
> > So when we discussed the '$' name way, I thought it'd be like:
> >
> > in /sys/bus/event_source/devices/pmu/events/event_name you have:
> >   param2=$foo,bar=1,param1=$baz
> >
> > and on command line you'd use:
> >   pmu/event_name,foo=0x1,bar=0x4/
> >
> > to assign directly to the $var, which would justify the $var
> > syntax I think..
> >
> 
> Agreed, what you've described above sounds like a good idea.
> 
> Compared to monopolizing all strings (which is what I did when
> initialy writing this), using a '$' prefix would allow less pain when
> some events suddenly need non-integer parameters.
> 
> > anyway we could assign directly to the param term name as you do,
> > but I think we just need to mark the term as parametrized, like:
> >
> > in /sys/bus/event_source/devices/pmu/events/event_name you have:
> >   param2=?,bar=1,param1=?
> >
> > and on command line you'd use:
> >   pmu/event_name,param2=0x1,param1=0x4/
> >
> > while the config code would check that the param substitution is
> > done only for terms with '?' in value, like 'param2=?' and not
> > for all PARSE_EVENTS__TERM_TYPE_STR type terms (as of now)
> 
> I prefer the `foo=0x1` as mentioned previously: it makes the user
> interface much less painful as we can have event-specific names for
> register/hcall fields.
> 
> I'm pretty sure the code used to do this, not sure when it was removed
> (haven't been following this patchset closely).

right, I recall seeing the 2 indirect assignments earlier,
but it was without the '$' marks

> 
> That said: I haven't fiddled with this code in a while (it's Suka's at
> this point), and there might be arguments the other way on both of
> those.

I guess I'm ok with both ways, maybe slightly inclined to
the '$' variable style one ;-)

jirka

^ permalink raw reply

* Re: [PATCH v5 1/4] tools/perf: support parsing parameterized events
From: Cody P Schafer @ 2014-12-05 23:05 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Michael Ellerman, Peter Zijlstra, LKML, Arnaldo Carvalho de Melo,
	Paul Mackerras, Sukadev Bhattiprolu, Linux PPC
In-Reply-To: <20141204124422.GA4195@krava.brq.redhat.com>

On Thu, Dec 4, 2014 at 7:44 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, Dec 02, 2014 at 06:09:35PM -0800, Sukadev Bhattiprolu wrote:
>> From: Cody P Schafer <cody@linux.vnet.ibm.com>
>>
>> Enable event specification like:
>>
>>       pmu/event_name,param1=0x1,param2=0x4/
>>
>> Assuming that
>>
>>       /sys/bus/event_source/devices/pmu/events/event_name
>>
>> Contains something like
>>
>>       param2=$foo,bar=1,param1=$baz
>
> oops.. sorry to be PITA on this one.. I might have missed something
> in the previous discussion but I guess I might have finally some
> opinion on this ;-)
>
> here's how I think your patchset works:
>
> in /sys/bus/event_source/devices/pmu/events/event_name you can actually have:
>
>    param2=foo,bar=1,param1=baz
>
> notice no '$', thats what you add later in 'perf list' output, right?
>
> Moreover it actually does not matter whats in value 'param2=HERE',
> because it's not used in the config code at all apart from the
> 'perf list' display processing.
>
> So when we discussed the '$' name way, I thought it'd be like:
>
> in /sys/bus/event_source/devices/pmu/events/event_name you have:
>   param2=$foo,bar=1,param1=$baz
>
> and on command line you'd use:
>   pmu/event_name,foo=0x1,bar=0x4/
>
> to assign directly to the $var, which would justify the $var
> syntax I think..
>

Agreed, what you've described above sounds like a good idea.

Compared to monopolizing all strings (which is what I did when
initialy writing this), using a '$' prefix would allow less pain when
some events suddenly need non-integer parameters.

> anyway we could assign directly to the param term name as you do,
> but I think we just need to mark the term as parametrized, like:
>
> in /sys/bus/event_source/devices/pmu/events/event_name you have:
>   param2=?,bar=1,param1=?
>
> and on command line you'd use:
>   pmu/event_name,param2=0x1,param1=0x4/
>
> while the config code would check that the param substitution is
> done only for terms with '?' in value, like 'param2=?' and not
> for all PARSE_EVENTS__TERM_TYPE_STR type terms (as of now)

I prefer the `foo=0x1` as mentioned previously: it makes the user
interface much less painful as we can have event-specific names for
register/hcall fields.

I'm pretty sure the code used to do this, not sure when it was removed
(haven't been following this patchset closely).

That said: I haven't fiddled with this code in a while (it's Suka's at
this point), and there might be arguments the other way on both of
those.

^ permalink raw reply

* Re: [PATCH linux-next] Documentation: Build mic/mpssd only for x86_64
From: Dan Streetman @ 2014-12-05 19:53 UTC (permalink / raw)
  To: Ashutosh Dixit
  Cc: Jonathan Corbet, Peter Foley, Siva Yerramreddy, Jiri Kosina,
	Randy Dunlap, linux-doc@vger.kernel.org, Dutt, Sudeep,
	Yokoyama, Caz, Daniel Borkmann, Greg Kroah-Hartman, Rao, Nikhil,
	linuxppc-dev@lists.ozlabs.org, Jennings, linux-kernel,
	Chandramouli, Dasaratharaman
In-Reply-To: <m3iohq7yem.fsf@adixit-yct.ra.intel.com>

On Fri, Dec 5, 2014 at 1:18 PM, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> On Thu, Dec 04 2014 at 07:27:06 PM, Dan Streetman <ddstreet@ieee.org> wrote:
>> On Thu, Dec 4, 2014 at 4:27 PM, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>>> mic/mpssd along with MIC drivers are currently only usable on
>>> x86_64. So build mic/mpssd only for x86_64 to avoid build breaks on
>>> big-endian systems.
>>
>> Only building for x86_64 is fine, but in that case what's the point of
>> leaving the htole16() et. al. functions in mpssd.c?  Shouldn't they be
>> removed?
>
> I am hoping that once glibc is fixed we can remove this limitation on
> building only for x86_64, so I'd rather htole16() et. al. stayed.

FYI, the bug I opened with glibc:
https://sourceware.org/bugzilla/show_bug.cgi?id=17679

was rejected because Andreas states "C does not allow function calls
in file scope."  The suggestion was made to try to fix gcc, but
honestly I don't have the time (or energy) to push for a gcc change
just for this. :-)

I do think it's too bad that glibc hto* functions don't work in file
scope (i.e. with static/global var initialization) for constant values
(using them with variables is different, which is why
__bswap_constant_* works in file scope but __bswap_* doesn't), since
it's simply a 2-step process that's entirely doable by the
preprocessor and/or compiler; just check if target endianness matches
desired endianness, and if not the compiler does a simple byte swap of
the constant value.

Anyway, I doubt glibc and/or gcc will be updated anytime soon to allow this.

>  As I
> said earlier, I'm fine with your patch too, but restricting the build
> for x86_64 seems to be the smallest patch which provides an acceptable
> solution.

Fine with me too.  It'll get big-endian building again.

^ permalink raw reply

* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
From: Scott Wood @ 2014-12-05 18:52 UTC (permalink / raw)
  To: Greg Kurz; +Cc: linuxppc-dev
In-Reply-To: <20141205151341.11028.47570.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> The smt-enabled kernel parameter basically leaves unwanted cpus executing
> in firmware or wherever they happen to be. The very same applies to the
> ibm,smt-enabled DT property which is no more used by anything known. These
> are hacks that shoudn't be used in a production environment.
> 
> Quoting mpe, "there are better ways for firmware to disable SMT".

Those "better ways" don't apply to Freescale chips, where the OS enables
(or not) SMT without any interaction with firmware.  I don't care about
the ibm,smt-enabled property, but can we please keep the smt-enabled
boot option?

> It also has an evil side effect on the split-core feature for powernv. The
> code needs all the cpus to participate to the split mode update: it relies
> on smp_send_reschedule() to get offline ones to do so. This doesn't work with
> cpus that haven't come up... The consequence is a kernel hang on powernv when
> trying to limit the number of hw threads at boot time (e.g. smt-enabled to
> anything but 8 on POWER8).

In that case could you disable the option only on that hardware?

> This patch simply removes both the smt-enabled kernel parameter and the
> ibm,smt-enabled property for all platforms. The new default is to start
> all hw threads. That leaves /sys the only supported API to change SMT
> settings.

How would you use /sys for this?  Are you talking about CPU hotplug?

-Scott

^ permalink raw reply

* Re: [RESEND PATCH v4] dmaengine: Driver support for FSL RaidEngine device.
From: Scott Wood @ 2014-12-05 18:33 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Harninder Rai, Naveen Burmi, dmaengine, xuelin.shi, linuxppc-dev,
	dan.j.williams
In-Reply-To: <20141205162823.GN3411@intel.com>

On Fri, 2014-12-05 at 21:58 +0530, Vinod Koul wrote:
> On Fri, Oct 17, 2014 at 03:28:20PM +0800, xuelin.shi@freescale.com wrote:
> > +/*
> > + * drivers/dma/fsl_raid.c
> > + *
> > + * Freescale RAID Engine device driver
> > + *
> > + * Author:
> > + *	Harninder Rai <harninder.rai@freescale.com>
> > + *	Naveen Burmi <naveenburmi@freescale.com>
> > + *
> > + * Rewrite:
> > + *	Xuelin Shi <xuelin.shi@freescale.com>
> > + *
> > + * Copyright (c) 2010-2014 Freescale Semiconductor, Inc.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions are met:
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in the
> > + *       documentation and/or other materials provided with the distribution.
> > + *     * Neither the name of Freescale Semiconductor nor the
> > + *       names of its contributors may be used to endorse or promote products
> > + *       derived from this software without specific prior written permission.
> hmmm, this doesnt sound right. BSD header in kernel code
> I am not a lawyer but for kernel this doesn't sound right. 

There's plenty of dual-licensed code in the kernel (try greping for
"Dual BSD" or "in source and binary").  It's fine if the code isn't
derived from other code which is GPL-only or otherwise incompatibly
licensed.  Even if portions are considered derivative it ought to be
possible to specify expanded permissions on the portions which aren't
derivative.

This isn't even the first dual-licensed dmaengine driver; see
drivers/dma/ioat/dma_v3.c.  There's also drivers/dma/ioat/pci.c which
claims MODULE_LICENSE("Dual BSD/GPL") but has a GPL-only comment header.

> Why cant this be only GPL? Why does this deviate from norm?

Why must it be only GPL?  Insisting on that is not the norm in Linux. 
Here are Linus's comments on the matter:

http://yarchive.net/comp/linux/dual_license_bsd_gpl.html

-Scott

^ permalink raw reply

* Re: [PATCH linux-next] Documentation: Build mic/mpssd only for x86_64
From: Ashutosh Dixit @ 2014-12-05 18:29 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: linux-kernel@vger.kernel.org, Peter Foley, Siva Yerramreddy,
	Jiri Kosina, Randy Dunlap, linux-doc@vger.kernel.org,
	Dutt, Sudeep, Yokoyama, Caz, Daniel Borkmann, Greg Kroah-Hartman,
	Rao, Nikhil, linuxppc-dev@lists.ozlabs.org, Jennings,
	Dan Streetman, Chandramouli, Dasaratharaman
In-Reply-To: <20141205111818.551a870f@lwn.net>

On Fri, Dec 05 2014 at 08:18:18 AM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Thu,  4 Dec 2014 13:27:29 -0800
> Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>
>> mic/mpssd along with MIC drivers are currently only usable on
>> x86_64. So build mic/mpssd only for x86_64 to avoid build breaks on
>> big-endian systems.
>
> I can certainly apply this.  But it seems to me that this kind of code
> doesn't belong in the documentation directory.  How about a patch to move
> it to tools/ ?

Yes, let's resolve the situation for now and then we'll submit another
patch soon to move it to tools.

^ permalink raw reply

* Re: [PATCH linux-next] Documentation: Build mic/mpssd only for x86_64
From: Ashutosh Dixit @ 2014-12-05 18:26 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: linux-kernel@vger.kernel.org, Peter Foley, Siva Yerramreddy,
	Jiri Kosina, Randy Dunlap, linux-doc@vger.kernel.org,
	Dutt, Sudeep, Yokoyama, Caz, Daniel Borkmann, Greg Kroah-Hartman,
	Rao, Nikhil, linuxppc-dev@lists.ozlabs.org, Jennings,
	Dan Streetman, Chandramouli, Dasaratharaman
In-Reply-To: <20141205111818.551a870f@lwn.net>

On Fri, Dec 05 2014 at 08:18:18 AM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Thu,  4 Dec 2014 13:27:29 -0800
> Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>
>> mic/mpssd along with MIC drivers are currently only usable on
>> x86_64. So build mic/mpssd only for x86_64 to avoid build breaks on
>> big-endian systems.
>
> I can certainly apply this.  But it seems to me that this kind of code
> doesn't belong in the documentation directory.  How about a patch to move
> it to tools/ ?

Yes, let's resolve the situation for now and then we'll submit another
patch soon to move it to tools.

^ permalink raw reply

* Re: [PATCH linux-next] Documentation: Build mic/mpssd only for x86_64
From: Ashutosh Dixit @ 2014-12-05 18:18 UTC (permalink / raw)
  To: Dan Streetman
  Cc: Jonathan Corbet, Peter Foley, Siva Yerramreddy, Jiri Kosina,
	Randy Dunlap, linux-doc@vger.kernel.org, Dutt, Sudeep,
	Yokoyama, Caz, Daniel Borkmann, Greg Kroah-Hartman, Rao, Nikhil,
	linuxppc-dev@lists.ozlabs.org, Jennings, linux-kernel,
	Chandramouli, Dasaratharaman
In-Reply-To: <CALZtONBUDyN7OOMV9ziDXMhFN9+bOcsYvJ0an_7T0VV0Hz+8WQ@mail.gmail.com>

On Thu, Dec 04 2014 at 07:27:06 PM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Thu, Dec 4, 2014 at 4:27 PM, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>> mic/mpssd along with MIC drivers are currently only usable on
>> x86_64. So build mic/mpssd only for x86_64 to avoid build breaks on
>> big-endian systems.
>
> Only building for x86_64 is fine, but in that case what's the point of
> leaving the htole16() et. al. functions in mpssd.c?  Shouldn't they be
> removed?

I am hoping that once glibc is fixed we can remove this limitation on
building only for x86_64, so I'd rather htole16() et. al. stayed.  As I
said earlier, I'm fine with your patch too, but restricting the build
for x86_64 seems to be the smallest patch which provides an acceptable
solution.

^ permalink raw reply

* Re: [PATCH linux-next] Documentation: Build mic/mpssd only for x86_64
From: Jonathan Corbet @ 2014-12-05 16:18 UTC (permalink / raw)
  To: Ashutosh Dixit
  Cc: linux-kernel, Peter Foley, Siva Yerramreddy, Jiri Kosina,
	Randy Dunlap, linux-doc, Sudeep Dutt, Caz Yokoyama,
	Daniel Borkmann, Greg Kroah-Hartman, Nikhil Rao, linuxppc-dev,
	Jennings, Dan Streetman, Dasaratharaman Chandramouli
In-Reply-To: <25c04eb72c621fc9bdf0aae25782ff5bad5a4b97.1417728278.git.ashutosh.dixit@intel.com>

On Thu,  4 Dec 2014 13:27:29 -0800
Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:

> mic/mpssd along with MIC drivers are currently only usable on
> x86_64. So build mic/mpssd only for x86_64 to avoid build breaks on
> big-endian systems.

I can certainly apply this.  But it seems to me that this kind of code
doesn't belong in the documentation directory.  How about a patch to move
it to tools/ ?

jon

^ permalink raw reply

* Re: [RESEND PATCH v4] dmaengine: Driver support for FSL RaidEngine device.
From: Vinod Koul @ 2014-12-05 16:28 UTC (permalink / raw)
  To: xuelin.shi
  Cc: dmaengine, Harninder Rai, dan.j.williams, linuxppc-dev,
	Naveen Burmi
In-Reply-To: <1413530900-22732-1-git-send-email-xuelin.shi@freescale.com>

On Fri, Oct 17, 2014 at 03:28:20PM +0800, xuelin.shi@freescale.com wrote:
> +/*
> + * drivers/dma/fsl_raid.c
> + *
> + * Freescale RAID Engine device driver
> + *
> + * Author:
> + *	Harninder Rai <harninder.rai@freescale.com>
> + *	Naveen Burmi <naveenburmi@freescale.com>
> + *
> + * Rewrite:
> + *	Xuelin Shi <xuelin.shi@freescale.com>
> + *
> + * Copyright (c) 2010-2014 Freescale Semiconductor, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of Freescale Semiconductor nor the
> + *       names of its contributors may be used to endorse or promote products
> + *       derived from this software without specific prior written permission.
hmmm, this doesnt sound right. BSD header in kernel code
I am not a lawyer but for kernel this doesn't sound right. Why cant this be
only GPL? Why does this deviate from norm?

> + *
> + * ALTERNATIVELY, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") as published by the Free Software
> + * Foundation, either version 2 of that License or (at your option) any
> + * later version.
> + *
> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + *
> + * Theory of operation:
> + *
> + * General capabilities:
> + *	RAID Engine (RE) block is capable of offloading XOR, memcpy and P/Q
> + *	calculations required in RAID5 and RAID6 operations. RE driver
> + *	registers with Linux's ASYNC layer as dma driver. RE hardware
> + *	maintains strict ordering of the requests through chained
> + *	command queueing.
okay I see driver is use re_xxx which is a very common term imo. I think we
need to protect the symbols by adding fsl_re_ tag. otherwise it will
conflict if someone does generic raid engine and decides to name it re_xxx

> + *
> + * Data flow:
> + *	Software RAID layer of Linux (MD layer) maintains RAID partitions,
> + *	strips, stripes etc. It sends requests to the underlying AYSNC layer
> + *	which further passes it to RE driver. ASYNC layer decides which request
> + *	goes to which job ring of RE hardware. For every request processed by
> + *	RAID Engine, driver gets an interrupt unless coalescing is set. The
> + *	per job ring interrupt handler checks the status register for errors,
> + *	clears the interrupt and leave the post interrupt processing to the irq
> + *	thread.
> + */
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>
> +#include <linux/dmaengine.h>
> +#include <linux/io.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +
> +#include "dmaengine.h"
> +#include "fsl_raid.h"
> +
> +#define MAX_XOR_SRCS		16
> +#define MAX_PQ_SRCS		16
> +#define MAX_INITIAL_DESCS	256
> +#define MAX_DESCS_LIMIT		(4 * MAX_INITIAL_DESCS)
> +#define FRAME_FORMAT		0x1
> +#define MAX_DATA_LENGTH		(1024*1024)
these need to be namespaced

> +
> +static enum dma_status re_jr_tx_status(struct dma_chan *chan,
> +		dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> +	enum dma_status ret;
> +	struct re_jr *jr = container_of(chan, struct re_jr, chan);
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +
> +	if (ret != DMA_COMPLETE) {
> +		re_jr_cleanup_descs(jr);
why do you do cleanup here?

> +		ret = dma_cookie_status(chan, cookie, txstate);
and then call again
> +	}

this is clearly not the expectation of tx_status callback.

  * device_tx_status
     - Should report the bytes left to go over on the given channel
     - Should only care about the transaction descriptor passed as
       argument, not the currently active one on a given channel
     - The tx_state argument might be NULL
     - Should use dma_set_residue to report it
     - In the case of a cyclic transfer, it should only take into
       account the current period.
     - This function can be called in an interrupt context.


> +
> +static struct dma_async_tx_descriptor *re_jr_prep_genq(
> +		struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
> +		unsigned int src_cnt, const unsigned char *scf, size_t len,
> +		unsigned long flags)
> +{
> +	struct re_jr *jr;
> +	struct fsl_re_dma_async_tx_desc *desc;
> +	struct xor_cdb *xor;
> +	struct cmpnd_frame *cf;
> +	u32 cdb;
> +	unsigned int i, j;
> +
> +	if (len > MAX_DATA_LENGTH) {
> +		pr_err("Length greater than %d not supported\n",
> +		       MAX_DATA_LENGTH);
> +		return NULL;
> +	}
here you are putting onus on client to know your max length magically. Also
you should consider splitting the txn to multiple of max lengths and process
them. That would make it really nice driver

> +int re_jr_probe(struct platform_device *ofdev,
> +		struct device_node *np, u8 q, u32 off)
> +{
> +	struct device *dev;
> +	struct re_drv_private *repriv;
> +	struct re_jr *jr;
> +	struct dma_device *dma_dev;
> +	u32 ptr;
> +	u32 status;
> +	int ret = 0, rc;
> +	struct platform_device *jr_ofdev;
> +
> +	dev = &ofdev->dev;
> +	repriv = dev_get_drvdata(dev);
> +	dma_dev = &repriv->dma_dev;
> +
> +	jr = devm_kzalloc(dev, sizeof(*jr), GFP_KERNEL);
> +	if (!jr) {
> +		dev_err(dev, "No free memory for allocating JR struct\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* create platform device for jr node */
> +	jr_ofdev = of_platform_device_create(np, NULL, dev);
> +	if (jr_ofdev == NULL) {
> +		dev_err(dev, "Not able to create ofdev for jr %d\n", q);
> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +	dev_set_drvdata(&jr_ofdev->dev, jr);
shouldn't this be last thing you set... once everything is initialized right
> +
> +	/* read reg property from dts */
> +	rc = of_property_read_u32(np, "reg", &ptr);
> +	if (rc) {
> +		dev_err(dev, "Reg property not found in JR number %d\n", q);
> +		ret = -ENODEV;
> +		goto err_free;
> +	}
> +
> +	jr->jrregs = (struct jr_config_regs *)((u8 *)repriv->re_regs +
> +			off + ptr);
> +
> +	/* read irq property from dts */
> +	jr->irq = irq_of_parse_and_map(np, 0);
> +	if (jr->irq == NO_IRQ) {
> +		dev_err(dev, "No IRQ defined for JR %d\n", q);
> +		ret = -ENODEV;
> +		goto err_free;
> +	}
> +
> +	ret = devm_request_threaded_irq(&jr_ofdev->dev, jr->irq, re_jr_isr,
> +					re_jr_isr_thread, 0, jr->name, jr);
the dmaengine API expects that you run a tasklet. Pls convert this

> +
> +	if (ret) {
> +		dev_err(dev, "Unable to register JR interrupt for JR %d\n", q);
> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	snprintf(jr->name, sizeof(jr->name), "re_jr%02d", q);
> +
> +	repriv->re_jrs[q] = jr;
> +	jr->chan.device = dma_dev;
> +	jr->chan.private = jr;
> +	jr->dev = &jr_ofdev->dev;
> +	jr->re_dev = repriv;
> +
> +	spin_lock_init(&jr->desc_lock);
> +	INIT_LIST_HEAD(&jr->ack_q);
> +	INIT_LIST_HEAD(&jr->active_q);
> +	INIT_LIST_HEAD(&jr->submit_q);
> +	INIT_LIST_HEAD(&jr->free_q);
> +
> +	list_add_tail(&jr->chan.device_node, &dma_dev->channels);
> +	dma_dev->chancnt++;
This is filled by framework, pls remove this


> +/* Probe function for RAID Engine */
> +static int raide_probe(struct platform_device *ofdev)
> +{
> +	struct re_drv_private *repriv;
> +	struct device_node *np;
> +	struct device_node *child;
> +	u32 off;
> +	u8 ridx = 0;
> +	struct dma_device *dma_dev;
> +	struct resource *res;
> +	int rc;
> +	struct device *dev = &ofdev->dev;
> +
> +	dev_info(dev, "Freescale RAID Engine driver\n");
noise, pls remove this and other places

> +#define MAX_RE_JRS		4
> +
> +#define RE_DPAA_MODE		(1 << 30)
> +#define RE_NON_DPAA_MODE	(1 << 31)
> +#define RE_GFM_POLY		0x1d000000
> +#define RE_JR_INB_JOB_ADD(x)	((x) << 16)
> +#define RE_JR_OUB_JOB_RMVD(x)	((x) << 16)
> +#define RE_JR_CFG1_CBSI		0x08000000
> +#define RE_JR_CFG1_CBS0		0x00080000
> +#define RE_JR_OUB_SLOT_FULL_SHIFT	8
> +#define RE_JR_OUB_SLOT_FULL(x)	((x) >> RE_JR_OUB_SLOT_FULL_SHIFT)
> +#define RE_JR_INB_SLOT_AVAIL_SHIFT	8
> +#define RE_JR_INB_SLOT_AVAIL(x)	((x) >> RE_JR_INB_SLOT_AVAIL_SHIFT)
reading thru driver made me curious on what JR stands for?

> +#define RE_PQ_OPCODE		0x1B
> +#define RE_XOR_OPCODE		0x1A
> +#define RE_MOVE_OPCODE		0x8
> +#define FRAME_DESC_ALIGNMENT	16
> +#define RE_BLOCK_SIZE		0x3 /* 4096 bytes */
> +#define CACHEABLE_INPUT_OUTPUT	0x0
> +#define BUFFERABLE_OUTPUT	0x0
> +#define INTERRUPT_ON_ERROR	0x1
> +#define DATA_DEPENDENCY		0x1
> +#define ENABLE_DPI		0x0
> +#define RING_SIZE		0x400
> +#define RING_SIZE_MASK		(RING_SIZE - 1)
> +#define RING_SIZE_SHIFT		8
these are in header, pls namespace them

> +/* Data protection/integrity related fields */
> +#define DPI_APPS_MASK		0xC0000000
> +#define DPI_APPS_SHIFT		30
> +#define DPI_REF_MASK		0x30000000
> +#define DPI_REF_SHIFT		28
> +#define DPI_GUARD_MASK		0x0C000000
> +#define DPI_GUARD_SHIFT		26
> +#define DPI_ATTR_MASK		0x03000000
> +#define DPI_ATTR_SHIFT		24
> +#define DPI_META_MASK		0x0000FFFF
here too and whole of the driver

-- 
~Vinod

^ permalink raw reply

* [PATCH] powerpc: drop the cpu_bootable hook
From: Greg Kurz @ 2014-12-05 15:42 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20141205151517.11028.6894.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

All powerpc platforms share the same cpu_bootable hook, which does nothing
but { return 1 }. It is not needed anymore. Let's drop it at last.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

v3: drop smp_generic_cpu_bootable() as well

I knew that posting a few minutes before leaving is bad idea on fridays... ;)

 arch/powerpc/include/asm/smp.h       |    1 -
 arch/powerpc/kernel/smp.c            |   14 +-------------
 arch/powerpc/platforms/85xx/smp.c    |    1 -
 arch/powerpc/platforms/cell/smp.c    |    1 -
 arch/powerpc/platforms/powernv/smp.c |    1 -
 arch/powerpc/platforms/pseries/smp.c |    1 -
 6 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index fae8cad..76edb9c 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -50,7 +50,6 @@ struct smp_ops_t {
 	void  (*give_timebase)(void);
 	int   (*cpu_disable)(void);
 	void  (*cpu_die)(unsigned int nr);
-	int   (*cpu_bootable)(unsigned int nr);
 };
 
 extern void smp_send_debugger_break(void);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8d7f114..e8bd931 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -82,17 +82,6 @@ volatile unsigned int cpu_callin_map[NR_CPUS];
 
 static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
 
-/*
- * Returns 1 if the specified cpu should be brought up during boot.
- * Used to inhibit booting threads if they've been disabled or
- * limited on the command line
- */
-int smp_generic_cpu_bootable(unsigned int nr)
-{
-	return 1;
-}
-
-
 #ifdef CONFIG_PPC64
 int smp_generic_kick_cpu(int nr)
 {
@@ -489,8 +478,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
 	    cpu_thread_in_subcore(cpu))
 		return -EBUSY;
 
-	if (smp_ops == NULL ||
-	    (smp_ops->cpu_bootable && !smp_ops->cpu_bootable(cpu)))
+	if (smp_ops == NULL)
 		return -EINVAL;
 
 	cpu_idle_thread_init(cpu, tidle);
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index d7c1e69..3aedc35 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -326,7 +326,6 @@ out:
 
 struct smp_ops_t smp_85xx_ops = {
 	.kick_cpu = smp_85xx_kick_cpu,
-	.cpu_bootable = smp_generic_cpu_bootable,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_disable	= generic_cpu_disable,
 	.cpu_die	= generic_cpu_die,
diff --git a/arch/powerpc/platforms/cell/smp.c b/arch/powerpc/platforms/cell/smp.c
index c8017a7..09f4eec 100644
--- a/arch/powerpc/platforms/cell/smp.c
+++ b/arch/powerpc/platforms/cell/smp.c
@@ -142,7 +142,6 @@ static struct smp_ops_t bpa_iic_smp_ops = {
 	.probe		= smp_iic_probe,
 	.kick_cpu	= smp_cell_kick_cpu,
 	.setup_cpu	= smp_cell_setup_cpu,
-	.cpu_bootable	= smp_generic_cpu_bootable,
 };
 
 /* This is called very early */
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 4753958..2e8a727 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -191,7 +191,6 @@ static struct smp_ops_t pnv_smp_ops = {
 	.probe		= xics_smp_probe,
 	.kick_cpu	= pnv_smp_kick_cpu,
 	.setup_cpu	= pnv_smp_setup_cpu,
-	.cpu_bootable	= smp_generic_cpu_bootable,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_disable	= pnv_smp_cpu_disable,
 	.cpu_die	= generic_cpu_die,
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index a3555b1..505a689 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -222,7 +222,6 @@ static struct smp_ops_t pSeries_xics_smp_ops = {
 	.probe		= pSeries_smp_probe,
 	.kick_cpu	= smp_pSeries_kick_cpu,
 	.setup_cpu	= smp_xics_setup_cpu,
-	.cpu_bootable	= smp_generic_cpu_bootable,
 };
 
 /* This is called very early */

^ permalink raw reply related

* Re: [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable()
From: Greg Kurz @ 2014-12-05 15:34 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20141205151455.11028.573.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

Subject should read "powerpc: drop useless code in smp_generic_cpu_bootable()"
actually...

On Fri, 05 Dec 2014 16:15:12 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> The following assertions are always true:
> - threads_per_core > 0
> - cpu & (threads_per_core - 1) < threads_per_core
> 
> It means smp_generic_cpu_bootable() always returns true.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/smp.c |   11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 9577791..8d7f114 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -89,17 +89,6 @@ static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
>   */
>  int smp_generic_cpu_bootable(unsigned int nr)
>  {
> -	/* Special case - we inhibit secondary thread startup
> -	 * during boot if the user requests it.
> -	 */
> -	if (system_state == SYSTEM_BOOTING && cpu_has_feature(CPU_FTR_SMT)) {
> -		if (!threads_per_core && cpu_thread_in_core(nr) != 0)
> -			return 0;
> -		if (threads_per_core
> -		    && cpu_thread_in_core(nr) >= threads_per_core)
> -			return 0;
> -	}
> -
>  	return 1;
>  }
>  
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply

* [PATCH 2 4/4] powerpc: drop the cpu_bootable hook
From: Greg Kurz @ 2014-12-05 15:15 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20141205150405.11028.27445.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

All powerpc platforms share the same cpu_bootable hook, which does nothing
but { return 1 }. It is not needed anymore. Let's drop it at last.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/smp.h       |    1 -
 arch/powerpc/kernel/smp.c            |    3 +--
 arch/powerpc/platforms/85xx/smp.c    |    1 -
 arch/powerpc/platforms/cell/smp.c    |    1 -
 arch/powerpc/platforms/powernv/smp.c |    1 -
 arch/powerpc/platforms/pseries/smp.c |    1 -
 6 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index fae8cad..76edb9c 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -50,7 +50,6 @@ struct smp_ops_t {
 	void  (*give_timebase)(void);
 	int   (*cpu_disable)(void);
 	void  (*cpu_die)(unsigned int nr);
-	int   (*cpu_bootable)(unsigned int nr);
 };
 
 extern void smp_send_debugger_break(void);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8d7f114..de6d60f 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -489,8 +489,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
 	    cpu_thread_in_subcore(cpu))
 		return -EBUSY;
 
-	if (smp_ops == NULL ||
-	    (smp_ops->cpu_bootable && !smp_ops->cpu_bootable(cpu)))
+	if (smp_ops == NULL)
 		return -EINVAL;
 
 	cpu_idle_thread_init(cpu, tidle);
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index d7c1e69..3aedc35 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -326,7 +326,6 @@ out:
 
 struct smp_ops_t smp_85xx_ops = {
 	.kick_cpu = smp_85xx_kick_cpu,
-	.cpu_bootable = smp_generic_cpu_bootable,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_disable	= generic_cpu_disable,
 	.cpu_die	= generic_cpu_die,
diff --git a/arch/powerpc/platforms/cell/smp.c b/arch/powerpc/platforms/cell/smp.c
index c8017a7..09f4eec 100644
--- a/arch/powerpc/platforms/cell/smp.c
+++ b/arch/powerpc/platforms/cell/smp.c
@@ -142,7 +142,6 @@ static struct smp_ops_t bpa_iic_smp_ops = {
 	.probe		= smp_iic_probe,
 	.kick_cpu	= smp_cell_kick_cpu,
 	.setup_cpu	= smp_cell_setup_cpu,
-	.cpu_bootable	= smp_generic_cpu_bootable,
 };
 
 /* This is called very early */
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 4753958..2e8a727 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -191,7 +191,6 @@ static struct smp_ops_t pnv_smp_ops = {
 	.probe		= xics_smp_probe,
 	.kick_cpu	= pnv_smp_kick_cpu,
 	.setup_cpu	= pnv_smp_setup_cpu,
-	.cpu_bootable	= smp_generic_cpu_bootable,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_disable	= pnv_smp_cpu_disable,
 	.cpu_die	= generic_cpu_die,
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index a3555b1..505a689 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -222,7 +222,6 @@ static struct smp_ops_t pSeries_xics_smp_ops = {
 	.probe		= pSeries_smp_probe,
 	.kick_cpu	= smp_pSeries_kick_cpu,
 	.setup_cpu	= smp_xics_setup_cpu,
-	.cpu_bootable	= smp_generic_cpu_bootable,
 };
 
 /* This is called very early */

^ permalink raw reply related

* [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable()
From: Greg Kurz @ 2014-12-05 15:15 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20141205150405.11028.27445.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

The following assertions are always true:
- threads_per_core > 0
- cpu & (threads_per_core - 1) < threads_per_core

It means smp_generic_cpu_bootable() always returns true.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c |   11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 9577791..8d7f114 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -89,17 +89,6 @@ static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
  */
 int smp_generic_cpu_bootable(unsigned int nr)
 {
-	/* Special case - we inhibit secondary thread startup
-	 * during boot if the user requests it.
-	 */
-	if (system_state == SYSTEM_BOOTING && cpu_has_feature(CPU_FTR_SMT)) {
-		if (!threads_per_core && cpu_thread_in_core(nr) != 0)
-			return 0;
-		if (threads_per_core
-		    && cpu_thread_in_core(nr) >= threads_per_core)
-			return 0;
-	}
-
 	return 1;
 }
 

^ permalink raw reply related

* [PATCH 2 2/4] powerpc: drop smt_enabled_at_boot
From: Greg Kurz @ 2014-12-05 15:14 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20141205150405.11028.27445.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

The paths using the smt_enabled_at_boot variable reveal the following
lifecycle:

initial value:                  int smt_enabled_at_boot = 1
                                        |
                                        |
check_smt_enabled():            smt_enabled_at_boot = threads_per_core
                                        |
                                        |
setup_tlb_core_data():          if (smt_enabled_at_boot >= 2
                                        |
                                        |
smp_generic_cpu_bootable():     if (!smt_enabled_at_boot
                                        |
                                if (smt_enabled_at_boot

It appears that smt_enabled_at_boot is just a duplicate of threads_per_core.
Let's drop it.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/smp.h |    2 --
 arch/powerpc/kernel/setup_64.c |   15 +--------------
 arch/powerpc/kernel/smp.c      |    8 +++-----
 3 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index 5a6614a..fae8cad 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -174,8 +174,6 @@ static inline void set_hard_smp_processor_id(int cpu, int phys)
 #endif /* !CONFIG_SMP */
 #endif /* !CONFIG_PPC64 */
 
-extern int smt_enabled_at_boot;
-
 extern int smp_mpic_probe(void);
 extern void smp_mpic_setup_cpu(int cpu);
 extern int smp_generic_kick_cpu(int nr);
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 29c1845..ba80480 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -114,7 +114,7 @@ static void setup_tlb_core_data(void)
 		 * or e6500 tablewalk mode, or else TLB handlers
 		 * will be racy and could produce duplicate entries.
 		 */
-		if (smt_enabled_at_boot >= 2 &&
+		if (threads_per_core >= 2 &&
 		    !mmu_has_feature(MMU_FTR_USE_TLBRSRV) &&
 		    book3e_htw_mode != PPC_HTW_E6500) {
 			/* Should we panic instead? */
@@ -129,18 +129,6 @@ static void setup_tlb_core_data(void)
 }
 #endif
 
-#ifdef CONFIG_SMP
-
-static void check_smt_enabled(void)
-{
-	/* Default to enabling all threads */
-	smt_enabled_at_boot = threads_per_core;
-}
-
-#else
-#define check_smt_enabled()
-#endif /* CONFIG_SMP */
-
 /** Fix up paca fields required for the boot cpu */
 static void fixup_boot_paca(void)
 {
@@ -462,7 +450,6 @@ void __init setup_system(void)
 	xmon_setup();
 
 	smp_setup_cpu_maps();
-	check_smt_enabled();
 	setup_tlb_core_data();
 
 	/*
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8b2d2dc..9577791 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -80,8 +80,6 @@ struct smp_ops_t *smp_ops;
 /* Can't be static due to PowerMac hackery */
 volatile unsigned int cpu_callin_map[NR_CPUS];
 
-int smt_enabled_at_boot = 1;
-
 static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
 
 /*
@@ -95,10 +93,10 @@ int smp_generic_cpu_bootable(unsigned int nr)
 	 * during boot if the user requests it.
 	 */
 	if (system_state == SYSTEM_BOOTING && cpu_has_feature(CPU_FTR_SMT)) {
-		if (!smt_enabled_at_boot && cpu_thread_in_core(nr) != 0)
+		if (!threads_per_core && cpu_thread_in_core(nr) != 0)
 			return 0;
-		if (smt_enabled_at_boot
-		    && cpu_thread_in_core(nr) >= smt_enabled_at_boot)
+		if (threads_per_core
+		    && cpu_thread_in_core(nr) >= threads_per_core)
 			return 0;
 	}
 

^ permalink raw reply related

* [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
From: Greg Kurz @ 2014-12-05 15:14 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <20141205150405.11028.27445.stgit@bahia.lab.toulouse-stg.fr.ibm.com>

The smt-enabled kernel parameter basically leaves unwanted cpus executing
in firmware or wherever they happen to be. The very same applies to the
ibm,smt-enabled DT property which is no more used by anything known. These
are hacks that shoudn't be used in a production environment.

Quoting mpe, "there are better ways for firmware to disable SMT".

It also has an evil side effect on the split-core feature for powernv. The
code needs all the cpus to participate to the split mode update: it relies
on smp_send_reschedule() to get offline ones to do so. This doesn't work with
cpus that haven't come up... The consequence is a kernel hang on powernv when
trying to limit the number of hw threads at boot time (e.g. smt-enabled to
anything but 8 on POWER8).

This patch simply removes both the smt-enabled kernel parameter and the
ibm,smt-enabled property for all platforms. The new default is to start
all hw threads. That leaves /sys the only supported API to change SMT
settings.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

v2: also drop ibm,smt-enabled

 arch/powerpc/kernel/setup_64.c |   46 ----------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 49f553b..29c1845 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -131,57 +131,11 @@ static void setup_tlb_core_data(void)
 
 #ifdef CONFIG_SMP
 
-static char *smt_enabled_cmdline;
-
-/* Look for ibm,smt-enabled OF option */
 static void check_smt_enabled(void)
 {
-	struct device_node *dn;
-	const char *smt_option;
-
 	/* Default to enabling all threads */
 	smt_enabled_at_boot = threads_per_core;
-
-	/* Allow the command line to overrule the OF option */
-	if (smt_enabled_cmdline) {
-		if (!strcmp(smt_enabled_cmdline, "on"))
-			smt_enabled_at_boot = threads_per_core;
-		else if (!strcmp(smt_enabled_cmdline, "off"))
-			smt_enabled_at_boot = 0;
-		else {
-			int smt;
-			int rc;
-
-			rc = kstrtoint(smt_enabled_cmdline, 10, &smt);
-			if (!rc)
-				smt_enabled_at_boot =
-					min(threads_per_core, smt);
-		}
-	} else {
-		dn = of_find_node_by_path("/options");
-		if (dn) {
-			smt_option = of_get_property(dn, "ibm,smt-enabled",
-						     NULL);
-
-			if (smt_option) {
-				if (!strcmp(smt_option, "on"))
-					smt_enabled_at_boot = threads_per_core;
-				else if (!strcmp(smt_option, "off"))
-					smt_enabled_at_boot = 0;
-			}
-
-			of_node_put(dn);
-		}
-	}
-}
-
-/* Look for smt-enabled= cmdline option */
-static int __init early_smt_enabled(char *p)
-{
-	smt_enabled_cmdline = p;
-	return 0;
 }
-early_param("smt-enabled", early_smt_enabled);
 
 #else
 #define check_smt_enabled()

^ permalink raw reply related

* [PATCH 2 0/4] powerpc: don't mess with SMT at boot time
From: Greg Kurz @ 2014-12-05 15:13 UTC (permalink / raw)
  To: linuxppc-dev

As requested by mpe, this series now covers both the smt-enabled
kernel parameter and the ibm,smt-enabled property. The cleanup was
split into 3 separate patches to ease review, but I guess they
could be folded into a single patch as well.

---

Greg Kurz (4):
      powerpc: drop the ability to tweak SMT mode at boot time
      powerpc: drop smt_enabled_at_boot
      powerpc: drop smp_generic_cpu_bootable()
      powerpc: drop the cpu_bootable hook


 arch/powerpc/include/asm/smp.h       |    3 --
 arch/powerpc/kernel/setup_64.c       |   61 +---------------------------------
 arch/powerpc/kernel/smp.c            |   16 +--------
 arch/powerpc/platforms/85xx/smp.c    |    1 -
 arch/powerpc/platforms/cell/smp.c    |    1 -
 arch/powerpc/platforms/powernv/smp.c |    1 -
 arch/powerpc/platforms/pseries/smp.c |    1 -
 7 files changed, 2 insertions(+), 82 deletions(-)

--
Greg

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox