* Re: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure
From: Sudeep Holla @ 2014-03-21 12:04 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linuxppc-dev@lists.ozlabs.org, Paul Mackerras,
linux-kernel@vger.kernel.org, Sudeep Holla
In-Reply-To: <532BB5AC.3000904@linux.vnet.ibm.com>
Hi Anshuman,
On 21/03/14 03:44, Anshuman Khandual wrote:
> On 03/10/2014 04:42 PM, Sudeep Holla wrote:
>> Hi Anshuman,
>>
>> On 07/03/14 06:14, Anshuman Khandual wrote:
>>> On 03/07/2014 09:36 AM, Anshuman Khandual wrote:
>>>> On 02/19/2014 09:36 PM, Sudeep Holla wrote:
>>>>> From: Sudeep Holla <sudeep.holla@arm.com>
>>>>>
>>>>> This patch removes the redundant sysfs cacheinfo code by making use o=
f
>>>>> the newly introduced generic cacheinfo infrastructure.
[...]
>>>> When it is UNIFIED we return index 0, which is correct. But the index
>>>> for instruction and data cache seems to be swapped which wrong. This
>>>> will fetch invalid properties for any given cache type.
>>>>
>>
>> Ah, that's silly mistake on my side, will fix it.
>>
>>>> I have done some initial review and testing for this patch's impact on
>>>> PowerPC (ppc64 POWER specifically). I am trying to do some code clean-=
up
>>>> and re-arrangements. Will post out soon. Thanks !
>>
>> Thanks for taking time for testing and reviewing these patches.
>=20
> Now that you got some of the problems to work on and resend the patches, =
I will
> hold on to the clean up patches I had.
>=20
I have done most of the changes but still unable to find why the shared_cpu=
_map
is getting incorrect on PPC. All the other wrong entries are fixed. Any clu=
e on
shared_cpu_map ?
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Gautham R Shenoy @ 2014-03-21 12:31 UTC (permalink / raw)
To: David Laight
Cc: linuxppc-dev@ozlabs.org, 'Viresh Kumar', Linux PM list,
ego@linux.vnet.ibm.com
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D0F6E5386@AcuExch.aculab.com>
On Fri, Mar 21, 2014 at 12:01:07PM +0000, David Laight wrote:
> From: Viresh Kumar
>
> > On 21 March 2014 16:34, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > > Heh! Well, that wasn't the reason why this was sent out as a separate
> > > patch, but never mind. Though I don't understand why it would be
> > > difficult to review the patch though.
> >
> > Because the initial driver wasn't complete earlier. There were 2-3 patches
> > after the first one which are changing what the first patch has added.
> > Nothing else :)
> >
> > >> > +static void powernv_read_cpu_freq(void *ret_freq)
> > >> > +{
> > >> > + unsigned long pmspr_val;
> > >> > + s8 local_pstate_id;
> > >> > + int *cur_freq, freq, pstate_id;
> > >> > +
> > >> > + cur_freq = (int *)ret_freq;
> > >>
> > >> You don't need cur_freq variable at all..
> > >
> > > I don't like it either. But the compiler complains without this hack
> > > :-(
> >
> > Why would the compiler warn for doing this?:
> >
> > *(int *)ret_freq = freq;
>
> Because it is very likely to be wrong.
> In general casts of pointers to integer types are dangerous.
> In this case why not make the function return the value?
Because this function is called via an smp_call_function(). And we
need a way of returning the value to the caller.
>
> David
>
>
^ permalink raw reply
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Viresh Kumar @ 2014-03-21 12:34 UTC (permalink / raw)
To: David Laight
Cc: ego@linux.vnet.ibm.com, Linux PM list, linuxppc-dev@ozlabs.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D0F6E5386@AcuExch.aculab.com>
On 21 March 2014 17:31, David Laight <David.Laight@aculab.com> wrote:
>> *(int *)ret_freq = freq;
>
> Because it is very likely to be wrong.
> In general casts of pointers to integer types are dangerous.
Where are we converting pointers to integers? We are doing a
cast from 'void * ' to 'int *' and then using indirection operator
to set its value.
^ permalink raw reply
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Gautham R Shenoy @ 2014-03-21 13:04 UTC (permalink / raw)
To: Viresh Kumar; +Cc: ego@linux.vnet.ibm.com, Linux PM list, linuxppc-dev
In-Reply-To: <CAKohpo=k-e1GLiLRNtJ3Xi3+NUWW1+gvtCNr_4Ag60j49dZvdQ@mail.gmail.com>
On Fri, Mar 21, 2014 at 05:15:34PM +0530, Viresh Kumar wrote:
> On 21 March 2014 16:34, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > Heh! Well, that wasn't the reason why this was sent out as a separate
> > patch, but never mind. Though I don't understand why it would be
> > difficult to review the patch though.
>
> Because the initial driver wasn't complete earlier. There were 2-3 patches
> after the first one which are changing what the first patch has added.
> Nothing else :)
>
> >> > +static void powernv_read_cpu_freq(void *ret_freq)
> >> > +{
> >> > + unsigned long pmspr_val;
> >> > + s8 local_pstate_id;
> >> > + int *cur_freq, freq, pstate_id;
> >> > +
> >> > + cur_freq = (int *)ret_freq;
> >>
> >> You don't need cur_freq variable at all..
> >
> > I don't like it either. But the compiler complains without this hack
> > :-(
>
> Why would the compiler warn for doing this?:
>
> *(int *)ret_freq = freq;
The compiler doesn't complain if I do this.
>
> >> > + pmspr_val = get_pmspr(SPRN_PMSR);
> >> > +
> >> > + /* The local pstate id corresponds bits 48..55 in the PMSR.
> >> > + * Note: Watch out for the sign! */
> >> > + local_pstate_id = (pmspr_val >> 48) & 0xFF;
> >> > + pstate_id = local_pstate_id;
> >>
> >> similarly local_pstate_id
> >
> > well, I am interested in the bits 48..55 of pmspr_val. That's the
> > pstate_id which can be negative. So I'll like to keep
> > local_pstate_id.
>
> Can you please explain at bit level how getting the value in a s8
> first and then into a s32 will help you here? Instead of getting it
> directly into s32.
Consider the case when pmspr = 0x00feffffffffffff;
We are interested in extracting the value 'fe'. And ensure that when
we store this value into an int, we get the sign extension right.
So the following doesn't work:
pstate_id = (pmspr_val >> 48) & 0xFFFFFFFF;
Since we get pstate_id = 0x000000fe, which is not the same as
0xfffffffe.
Of course, we could do the following, but I wasn't sure if
two levels of type casting helped on the readability front.
pstate_id = (int) ((s8)((pmspr_val >> 48) & 0xFF));
--
Thanks and Regards
gautham.
>
^ permalink raw reply
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Viresh Kumar @ 2014-03-21 13:12 UTC (permalink / raw)
To: ego@linux.vnet.ibm.com; +Cc: linuxppc-dev@ozlabs.org, Linux PM list
In-Reply-To: <20140321130450.GD2493@in.ibm.com>
On 21 March 2014 18:34, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> Consider the case when pmspr = 0x00feffffffffffff;
>
> We are interested in extracting the value 'fe'. And ensure that when
> we store this value into an int, we get the sign extension right.
>
> So the following doesn't work:
>
> pstate_id = (pmspr_val >> 48) & 0xFFFFFFFF;
What about pstate_id = (pmspr_val >> 48) & 0xFF; ??
^ permalink raw reply
* Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
From: Gautham R Shenoy @ 2014-03-21 13:23 UTC (permalink / raw)
To: Viresh Kumar
Cc: ego@linux.vnet.ibm.com, Linux PM list, linuxppc-dev,
Anton Blanchard, Srivatsa S. Bhat
In-Reply-To: <CAKohpo=JqHOPF_pdcAsvRgc1vENmS=Sg9pS8ZXPGhfMFnA8ZUA@mail.gmail.com>
Hi Viresh,
On Fri, Mar 21, 2014 at 04:24:27PM +0530, Viresh Kumar wrote:
> On 21 March 2014 16:13, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
>
> >> > + pr_debug("PState id %d freq %d MHz\n", id, freq);
> >> > + powernv_freqs[i].driver_data = i;
> >>
> >> I don't think you are using this field at all and this is the field you can
> >> use for driver_data and so you can get rid of powernv_pstate_ids[ ].
> >
> > Using driver_data to record powernv_pstate_ids won't work since
> > powernv_pstate_ids can be negative. So a pstate_id -3 can be confused
> > with CPUFREQ_BOOST_FREQ thereby not displaying the frequency
> > corresponding to pstate id -3. So for now I think we will be retaining
> > powernv_pstate_ids.
>
> As I said earlier, this field is only used by cpufreq drivers and cpufreq core
> isn't supposed to touch it. CPUFREQ_BOOST_FREQ and other macros
> are there for .frequency field and not this one.
>
Ok, I had based my code on linus's git tree. I checked the 'pm-cpufreq'
branch of Rafael's 'linux-pm' tree and freq_table.c contains the
following code snippet in show_available_frequencies:
if (show_boost ^ (table[i].driver_data == CPUFREQ_BOOST_FREQ))
continue;
I suspect we're talking about different code bases. So could you
please tell me which one should I be looking at ?
>
> >> > +static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >> > +{
> >> > + int base, i;
> >> > +
> >> > +#ifdef CONFIG_SMP
> >>
> >> What will break if you don't have this ifdef here? Without that as well
> >> below code should work?
>
> Missed this one?
>
> >> > + base = cpu_first_thread_sibling(policy->cpu);
> >> > +
> >> > + for (i = 0; i < threads_per_core; i++)
> >> > + cpumask_set_cpu(base + i, policy->cpus);
> >> > +#endif
> >> > + policy->cpuinfo.transition_latency = 25000;
> >> > +
> >> > + policy->cur = powernv_freqs[0].frequency;
> >> > + cpufreq_frequency_table_get_attr(powernv_freqs, policy->cpu);
> >>
> >> This doesn't exist anymore.
> >
> > Didn't get this comment!
>
> cpufreq_frequency_table_get_attr() routine doesn't exist anymore.
>
> >> > +static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> >> > +{
> >> > + cpufreq_frequency_table_put_attr(policy->cpu);
> >> > + return 0;
> >> > +}
> >>
> >> You don't need this..
> >
> > Why not ?
>
> Because cpufreq_frequency_table_get_attr() and
> cpufreq_frequency_table_put_attr() don't exist anymore :)
Well, it does in the codebases that I was looking at. May be I've been
looking at the wrong place.
>
> >> > +module_init(powernv_cpufreq_init);
> >> > +module_exit(powernv_cpufreq_exit);
> >>
> >> Place these right after the routines without any blank lines in
> > between.
> >
> > Is this the new convention ?
>
> Don't know I have been following this since long time, probably from the
> time I started with Mainline stuff.. I have seen many maintainers advising this
> as it make code more readable, specially if these routines are quite big..
>
> Probably it isn't mentioned in coding guidelines but people follow it most of
> the times.
Ok. I was not aware of this. Good to know :-)
>
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
From: Viresh Kumar @ 2014-03-21 13:34 UTC (permalink / raw)
To: ego@linux.vnet.ibm.com
Cc: Linux PM list, linuxppc-dev@ozlabs.org, Anton Blanchard,
Srivatsa S. Bhat
In-Reply-To: <20140321132326.GE2493@in.ibm.com>
On 21 March 2014 18:53, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> Ok, I had based my code on linus's git tree. I checked the 'pm-cpufreq'
Always check his bleeding-edge or linux-next branch.
> branch of Rafael's 'linux-pm' tree and freq_table.c contains the
> following code snippet in show_available_frequencies:
>
> if (show_boost ^ (table[i].driver_data == CPUFREQ_BOOST_FREQ))
> continue;
>
> I suspect we're talking about different code bases. So could you
> please tell me which one should I be looking at ?
Okay, there is some problem here. I will check how can I simplify this..
And yes, you were correct.
>> Because cpufreq_frequency_table_get_attr() and
>> cpufreq_frequency_table_put_attr() don't exist anymore :)
>
> Well, it does in the codebases that I was looking at. May be I've been
> looking at the wrong place.
Check the one I mentioned.
^ permalink raw reply
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Gautham R Shenoy @ 2014-03-21 14:25 UTC (permalink / raw)
To: Viresh Kumar
Cc: ego@linux.vnet.ibm.com, Linux PM list, linuxppc-dev@ozlabs.org
In-Reply-To: <CAKohpomHHToqOND=ZtMeqBd11X3VA=PeZ2K6Tyj31y2V4G9-uA@mail.gmail.com>
On Fri, Mar 21, 2014 at 06:42:50PM +0530, Viresh Kumar wrote:
> On 21 March 2014 18:34, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> > Consider the case when pmspr = 0x00feffffffffffff;
> >
> > We are interested in extracting the value 'fe'. And ensure that when
> > we store this value into an int, we get the sign extension right.
> >
> > So the following doesn't work:
> >
> > pstate_id = (pmspr_val >> 48) & 0xFFFFFFFF;
>
> What about pstate_id = (pmspr_val >> 48) & 0xFF; ??
Nope.
Suppose the pmspr_val is contained in the register
%rax, and pstate_id corresponds to the address -20(%rbp) then:
pstate_id = (pmspr_val >> 48) & 0xFF;
would corrspond to
shrq $48, %rax // Left shift by 48 bits
andl $255, %eax // Mask the lower 32 bits of %rax with 0x000000FF
movl %eax, -20(%rbp) // Store the lower 32 bits of %rax
into pstate_id
On the other hand,
pstate_id = (int)((s8)((pmspr_val >> 48) & 0xFF));
would correspond to:
shrq $48, %rax // Left shift by 48 bits.
movsbl %al, %eax // Move the lower 8 bits of %rax to %eax
with sign-extension.
movl %eax, -20(%rbp) // store the result in pstate_id;
So with this, we are getting the sign extension due to the use of movsbl.
And if local_pstate_id corresponds to the address -1(%rbp) then:
local_pstate_id = (pmspr_val >> 48) & 0xFF;
pstate_id = local_pstate_id;
would correspond to:
shrq $48, %rax // Left shift by 48 bits
movb %al, -1(%rbp) //copy the lower 8 bits to local_pstate_id
movsbl -1(%rbp), %eax //move the contents of
local_pstate_id to %eax with sign extension.
movl %eax, -20(%rbp) // Store the result in pstate_id
Hope this helps :-)
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
From: Vaidyanathan Srinivasan @ 2014-03-21 14:48 UTC (permalink / raw)
To: Gautham R Shenoy
Cc: Linux PM list, Viresh Kumar, linuxppc-dev, Anton Blanchard,
Srivatsa S. Bhat
In-Reply-To: <20140321104317.GA2493@in.ibm.com>
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2014-03-21 16:13:17]:
> Hi Viresh,
>
> On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
> > On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy
> > <ego@linux.vnet.ibm.com> wrote:
> > > From: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> >
> > Hi Vaidy,
> >
Hi Viresh,
Thanks for the detailed review.
> > > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > > index 4b029c0..4ba1632 100644
> > > --- a/drivers/cpufreq/Kconfig
> > > +++ b/drivers/cpufreq/Kconfig
> > > @@ -48,6 +48,7 @@ config CPU_FREQ_STAT_DETAILS
> > > choice
> > > prompt "Default CPUFreq governor"
> > > default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ || ARM_SA1110_CPUFREQ
> > > + default CPU_FREQ_DEFAULT_GOV_ONDEMAND if POWERNV_CPUFREQ
> >
> > Probably we should remove SA1100's entry as well from here. This is
> > not the right way of doing it. Imagine 100 platforms having entries here.
> > If you want it, then select it from your platforms Kconfig.
>
> Sure. Will move these bits and the other governor related bits to the
> Powernv Kconfig.
>
> > > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> > > new file mode 100644
> > > index 0000000..ab1551f
> > > --- /dev/null
> > > +
> > > +#define pr_fmt(fmt) "powernv-cpufreq: " fmt
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/cpufreq.h>
> > > +#include <linux/of.h>
> > > +#include <asm/cputhreads.h>
> >
> > That's it? Sure?
> >
> > Even if things compile for you, you must explicitly include all the
> > files on which
> > you depend.
>
> Ok.
>
> >
> > > +
> > > + WARN_ON(len_ids != len_freqs);
> > > + nr_pstates = min(len_ids, len_freqs) / sizeof(u32);
> > > + WARN_ON(!nr_pstates);
> >
> > Why do you want to continue here?
>
> Good point. We might be better off exiting at this point.
Yes, we could return here. We do not generally come till this point
if the platform has a problem discovering PStates from device tree.
But there is no useful debug info after this point if nr_pstates is 0.
> >
> > > + pr_debug("NR PStates %d\n", nr_pstates);
> > > + for (i = 0; i < nr_pstates; i++) {
> > > + u32 id = be32_to_cpu(pstate_ids[i]);
> > > + u32 freq = be32_to_cpu(pstate_freqs[i]);
> > > +
> > > + pr_debug("PState id %d freq %d MHz\n", id, freq);
> > > + powernv_freqs[i].driver_data = i;
> >
> > I don't think you are using this field at all and this is the field you can
> > use for driver_data and so you can get rid of powernv_pstate_ids[ ].
>
> Using driver_data to record powernv_pstate_ids won't work since
> powernv_pstate_ids can be negative. So a pstate_id -3 can be confused
> with CPUFREQ_BOOST_FREQ thereby not displaying the frequency
> corresponding to pstate id -3. So for now I think we will be retaining
> powernv_pstate_ids.
Yeah, I had the driver written using driver_data to store pstates.
Gautham found the bug that we are missing one PState when we match the
ID with CPUFREQ_BOOST_FREQ!
We did not know that you have taken care of those issues. Ideally
I did expect that driver_data should not be touched by the framework.
Thanks for fixing that and allowing the back-end driver to use
driver_data.
> >
> > > + powernv_freqs[i].frequency = freq * 1000; /* kHz */
> > > + powernv_pstate_ids[i] = id;
> > > + }
> > > + /* End of list marker entry */
> > > + powernv_freqs[i].driver_data = 0;
> >
> > Not required.
>
> Ok.
> >
> > > + powernv_freqs[i].frequency = CPUFREQ_TABLE_END;
> > > +
> > > + /* Print frequency table */
> > > + for (i = 0; powernv_freqs[i].frequency != CPUFREQ_TABLE_END; i++)
> > > + pr_debug("%d: %d\n", i, powernv_freqs[i].frequency);
> >
> > You have already printed this table earlier..
>
> Fair enough.
>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct freq_attr *powernv_cpu_freq_attr[] = {
> > > + &cpufreq_freq_attr_scaling_available_freqs,
> > > + NULL,
> > > +};
> >
> > Can use this instead: cpufreq_generic_attr?
>
> In this patch yes. But later patch introduces an additional attribute
> for displaying the nominal frequency. Will handle that part in a clean
> way in the next version.
>
> >
> > > +/* Helper routines */
> > > +
> > > +/* Access helpers to power mgt SPR */
> > > +
> > > +static inline unsigned long get_pmspr(unsigned long sprn)
> >
> > Looks big enough not be inlined?
>
> It is called from one function. It has been defined separately for
> readability.
Let the compiler decide. The code is straight forward :)
I wanted to keep this SPR operations in a separate function to
facilitate debug and also introduce any timing/sequence change here if
required. Keeping this separate is helpful, if inlining is
successful, we get a bonus :)
> >
> > > +{
> > > + switch (sprn) {
> > > + case SPRN_PMCR:
> > > + return mfspr(SPRN_PMCR);
> > > +
> > > + case SPRN_PMICR:
> > > + return mfspr(SPRN_PMICR);
> > > +
> > > + case SPRN_PMSR:
> > > + return mfspr(SPRN_PMSR);
> > > + }
> > > + BUG();
> > > +}
> > > +
> > > +static inline void set_pmspr(unsigned long sprn, unsigned long val)
> > > +{
> >
> > Same here..
>
> Same reason as above.
>
> >
> > > + switch (sprn) {
> > > + case SPRN_PMCR:
> > > + mtspr(SPRN_PMCR, val);
> > > + return;
> > > +
> > > + case SPRN_PMICR:
> > > + mtspr(SPRN_PMICR, val);
> > > + return;
> > > +
> > > + case SPRN_PMSR:
> > > + mtspr(SPRN_PMSR, val);
> > > + return;
> > > + }
> > > + BUG();
> > > +}
> > > +
> > > +static void set_pstate(void *pstate)
> > > +{
> > > + unsigned long val;
> > > + unsigned long pstate_ul = *(unsigned long *) pstate;
> >
> > Why not sending value only to this routine instead of pointer?
>
> Well this function is called via an smp_call_function. so, cannot send
> a value :(
>
> >
> > > +
> > > + val = get_pmspr(SPRN_PMCR);
> > > + val = val & 0x0000ffffffffffffULL;
> >
> > Maybe a blank line here?
>
> Ok.
>
> >
> > > + /* Set both global(bits 56..63) and local(bits 48..55) PStates */
> > > + val = val | (pstate_ul << 56) | (pstate_ul << 48);
> >
> > here as well?
>
> Ok.
> >
>
> > > + pr_debug("Setting cpu %d pmcr to %016lX\n", smp_processor_id(), val);
> > > + set_pmspr(SPRN_PMCR, val);
> > > +}
> > > +
> > > +static int powernv_set_freq(cpumask_var_t cpus, unsigned int new_index)
> > > +{
> > > + unsigned long val = (unsigned long) powernv_pstate_ids[new_index];
> >
> > I think automatic type conversion will happen here.
>
> Ok. Will fix this.
Most of the conversions are verbose mainly to help readability of the
required sign-extensions. There is scope to make these concise.
> >
> > > +
> > > + /*
> > > + * Use smp_call_function to send IPI and execute the
> > > + * mtspr on target cpu. We could do that without IPI
> > > + * if current CPU is within policy->cpus (core)
> > > + */
> >
> > Hmm, interesting I also feel there are cases where this routine can
> > get called from other CPUs. Can you list those use cases where it can
> > happen? Governors will mostly call this from one of the CPUs present
> > in policy->cpus.
>
> Consider the case when the governor is userspace and we are executing
>
> # echo freqvalue >
> /sys/devices/system/cpu/cpu<i>/cpufreq/scaling_setspeed
>
> from a cpu <j> which is not in policy->cpus of cpu i.
>
>
> > > +static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > > +{
> > > + int base, i;
> > > +
> > > +#ifdef CONFIG_SMP
> >
> > What will break if you don't have this ifdef here? Without that as well
> > below code should work?
> >
> > > + base = cpu_first_thread_sibling(policy->cpu);
> > > +
> > > + for (i = 0; i < threads_per_core; i++)
> > > + cpumask_set_cpu(base + i, policy->cpus);
> > > +#endif
> > > + policy->cpuinfo.transition_latency = 25000;
> > > +
> > > + policy->cur = powernv_freqs[0].frequency;
> > > + cpufreq_frequency_table_get_attr(powernv_freqs, policy->cpu);
> >
> > This doesn't exist anymore.
>
> Didn't get this comment!
>
> >
> > > + return cpufreq_frequency_table_cpuinfo(policy, powernv_freqs);
> >
> > Have you written this driver long time back? CPUFreq core has been
> > cleaned up heavily since last few kernel releases and I think there are
> > better helper routines available now.
>
> Yup it was written quite a while ago. And yeah, CPUFreq has changed
> quite a bit since the last time I saw it :-)
Yes, the driver is more than a year old and based on even older
implementation that I had written. I kept up until I got a compiler
warning or functional issue. Definitely could not catch-up with your
cleanups :)
> >
> > > +}
> > > +
> > > +static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> > > +{
> > > + cpufreq_frequency_table_put_attr(policy->cpu);
> > > + return 0;
> > > +}
> >
> > You don't need this..
>
> Why not ?
>
> >
> > > +static int powernv_cpufreq_verify(struct cpufreq_policy *policy)
> > > +{
> > > + return cpufreq_frequency_table_verify(policy, powernv_freqs);
> > > +}
> >
> > use generic verify function pointer instead..
> >
> > > +static int powernv_cpufreq_target(struct cpufreq_policy *policy,
> > > + unsigned int target_freq,
> > > + unsigned int relation)
> >
> > use target_index() instead..
> >
> > > +{
> > > + int rc;
> > > + struct cpufreq_freqs freqs;
> > > + unsigned int new_index;
> > > +
> > > + cpufreq_frequency_table_target(policy, powernv_freqs, target_freq,
> > > + relation, &new_index);
> > > +
> > > + freqs.old = policy->cur;
> > > + freqs.new = powernv_freqs[new_index].frequency;
> > > + freqs.cpu = policy->cpu;
> > > +
> > > + mutex_lock(&freq_switch_mutex);
> >
> > Why do you need this lock for?
>
> I guess it was to serialize accesses to PMCR. But Srivatsa's patch
> converts this to a per-core lock which probably is no longer required
> after your cpufreq_freq_transition_begin/end() patch series.
Right. Frequency transitions are per-core, the h/w SPRs are per-core.
We need to prevent threads from colliding on h/w SPR access. We do
make the lock per-core later in the series as mentioned above.
Gautham has addressed most comments.
Thanks,
Vaidy
^ permalink raw reply
* Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
From: Gautham R Shenoy @ 2014-03-21 14:54 UTC (permalink / raw)
To: Viresh Kumar
Cc: ego@linux.vnet.ibm.com, Linux PM list, linuxppc-dev,
Anton Blanchard, Srivatsa S. Bhat
In-Reply-To: <CAKohpo=JqHOPF_pdcAsvRgc1vENmS=Sg9pS8ZXPGhfMFnA8ZUA@mail.gmail.com>
On Fri, Mar 21, 2014 at 04:24:27PM +0530, Viresh Kumar wrote:
> >> > +static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >> > +{
> >> > + int base, i;
> >> > +
> >> > + base = cpu_first_thread_sibling(policy->cpu);
> >> > +
> >> > + for (i = 0; i < threads_per_core; i++)
> >> > + cpumask_set_cpu(base + i, policy->cpus);
> >> > + policy->cpuinfo.transition_latency = 25000;
> >> > +
> >> > + policy->cur = powernv_freqs[0].frequency;
> >> > + cpufreq_frequency_table_get_attr(powernv_freqs, policy->cpu);
> >>
> >> This doesn't exist anymore.
Ok, I guess the right thing to do at this point is call
cpufreq_table_validate_and_show(policy, powernv_freqs);
Will fix the code to take care of this.
--
Thanks and Regards
gautham.
^ permalink raw reply
* RE: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: David Laight @ 2014-03-21 15:01 UTC (permalink / raw)
To: 'Viresh Kumar'
Cc: ego@linux.vnet.ibm.com, Linux PM list, linuxppc-dev@ozlabs.org
In-Reply-To: <CAKohpo=E1UEcLhbZHtmBJcD3S3trm8ypT7++quF_t6XHDXAvHA@mail.gmail.com>
From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> On 21 March 2014 17:31, David Laight <David.Laight@aculab.com> wrote:
> >> *(int *)ret_freq =3D freq;
> >
> > Because it is very likely to be wrong.
> > In general casts of pointers to integer types are dangerous.
>=20
> Where are we converting pointers to integers? We are doing a
> cast from 'void * ' to 'int *' and then using indirection operator
> to set its value.
You mis-parsed what I wrote, try:
In general casts of 'pointer to integer' types are dangerous.
Somewhere, much higher up the call stack, the address of an integer
variable is being taken and then passed as the 'void *' parameter.
The 'problem' is that it is quite easily to pass the address of
a 'long' instead. On 32bit and LE systems this won't always
be a problem. On 64bit BE it all goes wrong.
David
^ permalink raw reply
* Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper
From: Geoff Levand @ 2014-03-21 17:28 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev
In-Reply-To: <1395328213-19206-1-git-send-email-clg@fr.ibm.com>
Hi Cédric,
On Thu, 2014-03-20 at 16:09 +0100, Cédric Le Goater wrote:
> The following patchset adds support for 64bit little endian boot
> wrapper. It is based on original code from Andrew Tauferner.
I tested this on PS3 (64 bit BE) and found no regression.
Tested by: Geoff Levand <geoff@infradead.org>
^ permalink raw reply
* Re: [PATCH] mtd: m25p80: Modify the name of mtd_info
From: Scott Wood @ 2014-03-21 17:34 UTC (permalink / raw)
To: Hou Zhiqiang; +Cc: linuxppc-dev, mingkai.hu, linux-mtd, dwmw2
In-Reply-To: <1395400578-5637-1-git-send-email-B48286@freescale.com>
On Fri, 2014-03-21 at 19:16 +0800, Hou Zhiqiang wrote:
> @@ -1009,8 +1012,17 @@ static int m25p_probe(struct spi_device *spi)
>
> if (data && data->name)
> flash->mtd.name = data->name;
> - else
> - flash->mtd.name = dev_name(&spi->dev);
> + else{
Whitespace
> + ret = of_address_to_resource(mnp, 0, &res);
> + if (ret) {
> + dev_err(&spi->dev, "failed to get spi master resource\n");
> + return ret;
> + }
> + flash->mtd.name = kasprintf(GFP_KERNEL, "spi%x.%d",
> + (unsigned)res.start, spi->chip_select);
Don't use "unsigned" by itself. Don't cast physical addresses to
"unsigned int" -- use "unsigned long long".
-Scott
^ permalink raw reply
* Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper
From: Cedric Le Goater @ 2014-03-21 17:40 UTC (permalink / raw)
To: Geoff Levand; +Cc: linuxppc-dev
In-Reply-To: <1395422919.26958.6.camel@smoke>
On 03/21/2014 06:28 PM, Geoff Levand wrote:
> Hi Cédric,
>
> On Thu, 2014-03-20 at 16:09 +0100, Cédric Le Goater wrote:
>> The following patchset adds support for 64bit little endian boot
>> wrapper. It is based on original code from Andrew Tauferner.
>
> I tested this on PS3 (64 bit BE) and found no regression.
>
> Tested by: Geoff Levand <geoff@infradead.org>
Thanks !
C.
^ permalink raw reply
* Re: [PATCH 3/4] ARCH: AUDIT: implement syscall_get_arch for all arches
From: Richard Guy Briggs @ 2014-03-21 19:13 UTC (permalink / raw)
To: Eric Paris
Cc: linux-mips, linux-ia64, linux-parisc, microblaze-uclinux, linux,
linux-audit, sparclinux, linuxppc-dev
In-Reply-To: <1395266643-3139-3-git-send-email-eparis@redhat.com>
On 14/03/19, Eric Paris wrote:
> For all arches which support audit implement syscall_get_arch()
> They are all pretty easy and straight forward, stolen from how the call
> to audit_syscall_entry() determines the arch.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Cc: linux-ia64@vger.kernel.org
> Cc: microblaze-uclinux@itee.uq.edu.au
> Cc: linux-mips@linux-mips.org
> Cc: linux@lists.openrisc.net
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: sparclinux@vger.kernel.org
Acked-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> arch/ia64/include/asm/syscall.h | 6 ++++++
> arch/microblaze/include/asm/syscall.h | 5 +++++
> arch/mips/include/asm/syscall.h | 2 +-
> arch/openrisc/include/asm/syscall.h | 5 +++++
> arch/parisc/include/asm/syscall.h | 11 +++++++++++
> arch/powerpc/include/asm/syscall.h | 12 ++++++++++++
> arch/sparc/include/asm/syscall.h | 8 ++++++++
> include/uapi/linux/audit.h | 1 +
> 8 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/arch/ia64/include/asm/syscall.h b/arch/ia64/include/asm/syscall.h
> index a7ff1c6..1d0b875 100644
> --- a/arch/ia64/include/asm/syscall.h
> +++ b/arch/ia64/include/asm/syscall.h
> @@ -13,6 +13,7 @@
> #ifndef _ASM_SYSCALL_H
> #define _ASM_SYSCALL_H 1
>
> +#include <uapi/linux/audit.h>
> #include <linux/sched.h>
> #include <linux/err.h>
>
> @@ -79,4 +80,9 @@ static inline void syscall_set_arguments(struct task_struct *task,
>
> ia64_syscall_get_set_arguments(task, regs, i, n, args, 1);
> }
> +
> +static inline int syscall_get_arch(void)
> +{
> + return AUDIT_ARCH_IA64;
> +}
> #endif /* _ASM_SYSCALL_H */
> diff --git a/arch/microblaze/include/asm/syscall.h b/arch/microblaze/include/asm/syscall.h
> index 9bc4317..53cfaf3 100644
> --- a/arch/microblaze/include/asm/syscall.h
> +++ b/arch/microblaze/include/asm/syscall.h
> @@ -1,6 +1,7 @@
> #ifndef __ASM_MICROBLAZE_SYSCALL_H
> #define __ASM_MICROBLAZE_SYSCALL_H
>
> +#include <uapi/linux/audit.h>
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <asm/ptrace.h>
> @@ -99,4 +100,8 @@ static inline void syscall_set_arguments(struct task_struct *task,
> asmlinkage long do_syscall_trace_enter(struct pt_regs *regs);
> asmlinkage void do_syscall_trace_leave(struct pt_regs *regs);
>
> +static inline int syscall_get_arch(void)
> +{
> + return AUDIT_ARCH_MICROBLAZE;
> +}
> #endif /* __ASM_MICROBLAZE_SYSCALL_H */
> diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
> index fc556d8..992b6ab 100644
> --- a/arch/mips/include/asm/syscall.h
> +++ b/arch/mips/include/asm/syscall.h
> @@ -103,7 +103,7 @@ extern const unsigned long sysn32_call_table[];
>
> static inline int syscall_get_arch(void)
> {
> - int arch = EM_MIPS;
> + int arch = AUDIT_ARCH_MIPS;
> #ifdef CONFIG_64BIT
> arch |= __AUDIT_ARCH_64BIT;
> #endif
> diff --git a/arch/openrisc/include/asm/syscall.h b/arch/openrisc/include/asm/syscall.h
> index b752bb6..2db9f1c 100644
> --- a/arch/openrisc/include/asm/syscall.h
> +++ b/arch/openrisc/include/asm/syscall.h
> @@ -19,6 +19,7 @@
> #ifndef __ASM_OPENRISC_SYSCALL_H__
> #define __ASM_OPENRISC_SYSCALL_H__
>
> +#include <uapi/linux/audit.h>
> #include <linux/err.h>
> #include <linux/sched.h>
>
> @@ -71,4 +72,8 @@ syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
> memcpy(®s->gpr[3 + i], args, n * sizeof(args[0]));
> }
>
> +static inline int syscall_get_arch(void)
> +{
> + return AUDIT_ARCH_OPENRISC;
> +}
> #endif
> diff --git a/arch/parisc/include/asm/syscall.h b/arch/parisc/include/asm/syscall.h
> index 8bdfd2c..a5eba95 100644
> --- a/arch/parisc/include/asm/syscall.h
> +++ b/arch/parisc/include/asm/syscall.h
> @@ -3,6 +3,8 @@
> #ifndef _ASM_PARISC_SYSCALL_H_
> #define _ASM_PARISC_SYSCALL_H_
>
> +#include <uapi/linux/audit.h>
> +#include <linux/compat.h>
> #include <linux/err.h>
> #include <asm/ptrace.h>
>
> @@ -37,4 +39,13 @@ static inline void syscall_get_arguments(struct task_struct *tsk,
> }
> }
>
> +static inline int syscall_get_arch(void)
> +{
> + int arch = AUDIT_ARCH_PARISC;
> +#ifdef CONFIG_64BIT
> + if (!is_compat_task())
> + arch = AUDIT_ARCH_PARISC64;
> +#endif
> + return arch;
> +}
> #endif /*_ASM_PARISC_SYSCALL_H_*/
> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> index b54b2ad..4271544 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -13,6 +13,8 @@
> #ifndef _ASM_SYSCALL_H
> #define _ASM_SYSCALL_H 1
>
> +#include <uapi/linux/audit.h>
> +#include <linux/compat.h>
> #include <linux/sched.h>
>
> /* ftrace syscalls requires exporting the sys_call_table */
> @@ -86,4 +88,14 @@ static inline void syscall_set_arguments(struct task_struct *task,
> memcpy(®s->gpr[3 + i], args, n * sizeof(args[0]));
> }
>
> +static inline int syscall_get_arch(void)
> +{
> + int arch = AUDIT_ARCH_PPC;
> +
> +#ifdef CONFIG_PPC64
> + if (!is_32bit_task())
> + arch = AUDIT_ARCH_PPC64;
> +#endif
> + return arch;
> +}
> #endif /* _ASM_SYSCALL_H */
> diff --git a/arch/sparc/include/asm/syscall.h b/arch/sparc/include/asm/syscall.h
> index 025a02a..fed3d51 100644
> --- a/arch/sparc/include/asm/syscall.h
> +++ b/arch/sparc/include/asm/syscall.h
> @@ -1,9 +1,11 @@
> #ifndef __ASM_SPARC_SYSCALL_H
> #define __ASM_SPARC_SYSCALL_H
>
> +#include <uapi/linux/audit.h>
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <asm/ptrace.h>
> +#include <asm/thread_info.h>
>
> /*
> * The syscall table always contains 32 bit pointers since we know that the
> @@ -124,4 +126,10 @@ static inline void syscall_set_arguments(struct task_struct *task,
> regs->u_regs[UREG_I0 + i + j] = args[j];
> }
>
> +static inline int syscall_get_arch(void)
> +{
> + return test_thread_flag(TIF_32BIT) ? AUDIT_ARCH_SPARC
> + : AUDIT_ARCH_SPARC64;
> +}
> +
> #endif /* __ASM_SPARC_SYSCALL_H */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9af01d7..8496cfa 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -343,6 +343,7 @@ enum {
> #define AUDIT_ARCH_IA64 (EM_IA_64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
> #define AUDIT_ARCH_M32R (EM_M32R)
> #define AUDIT_ARCH_M68K (EM_68K)
> +#define AUDIT_ARCH_MICROBLAZE (EM_MICROBLAZE)
> #define AUDIT_ARCH_MIPS (EM_MIPS)
> #define AUDIT_ARCH_MIPSEL (EM_MIPS|__AUDIT_ARCH_LE)
> #define AUDIT_ARCH_MIPS64 (EM_MIPS|__AUDIT_ARCH_64BIT)
> --
> 1.8.5.3
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 4/4] ARCH: AUDIT: audit_syscall_entry() should not require the arch
From: Richard Guy Briggs @ 2014-03-21 19:18 UTC (permalink / raw)
To: Eric Paris
Cc: linux-mips, linux-ia64, linux-parisc, user-mode-linux-devel,
linux-s390, linux-sh, microblaze-uclinux, linux-xtensa, x86,
linux-audit, linux-alpha, sparclinux, linuxppc-dev, linux,
linux-arm-kernel
In-Reply-To: <1395266643-3139-4-git-send-email-eparis@redhat.com>
On 14/03/19, Eric Paris wrote:
> We have a function where the arch can be queried, syscall_get_arch().
> So rather than have every single piece of arch specific code use and/or
> duplicate syscall_get_arch(), just have the audit code use the
> syscall_get_arch() code.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Cc: linux-alpha@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-ia64@vger.kernel.org
> Cc: microblaze-uclinux@itee.uq.edu.au
> Cc: linux-mips@linux-mips.org
> Cc: linux@lists.openrisc.net
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: sparclinux@vger.kernel.org
> Cc: user-mode-linux-devel@lists.sourceforge.net
> Cc: linux-xtensa@linux-xtensa.org
> Cc: x86@kernel.org
Acked-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> arch/alpha/kernel/ptrace.c | 2 +-
> arch/arm/kernel/ptrace.c | 4 ++--
> arch/ia64/kernel/ptrace.c | 2 +-
> arch/microblaze/kernel/ptrace.c | 3 +--
> arch/mips/kernel/ptrace.c | 4 +---
> arch/openrisc/kernel/ptrace.c | 3 +--
> arch/parisc/kernel/ptrace.c | 9 +++------
> arch/powerpc/kernel/ptrace.c | 7 ++-----
> arch/s390/kernel/ptrace.c | 4 +---
> arch/sh/kernel/ptrace_32.c | 14 +-------------
> arch/sh/kernel/ptrace_64.c | 17 +----------------
> arch/sparc/kernel/ptrace_64.c | 9 ++-------
> arch/um/kernel/ptrace.c | 3 +--
> arch/x86/kernel/ptrace.c | 8 ++------
> arch/x86/um/asm/ptrace.h | 4 ----
> arch/xtensa/kernel/ptrace.c | 2 +-
> include/linux/audit.h | 7 ++++---
> 17 files changed, 25 insertions(+), 77 deletions(-)
>
> diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c
> index 86d8351..d9ee817 100644
> --- a/arch/alpha/kernel/ptrace.c
> +++ b/arch/alpha/kernel/ptrace.c
> @@ -321,7 +321,7 @@ asmlinkage unsigned long syscall_trace_enter(void)
> if (test_thread_flag(TIF_SYSCALL_TRACE) &&
> tracehook_report_syscall_entry(current_pt_regs()))
> ret = -1UL;
> - audit_syscall_entry(AUDIT_ARCH_ALPHA, regs->r0, regs->r16, regs->r17, regs->r18, regs->r19);
> + audit_syscall_entry(regs->r0, regs->r16, regs->r17, regs->r18, regs->r19);
> return ret ?: current_pt_regs()->r0;
> }
>
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 0dd3b79..c9d2b34 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -943,8 +943,8 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
> if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
> trace_sys_enter(regs, scno);
>
> - audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
> - regs->ARM_r2, regs->ARM_r3);
> + audit_syscall_entry(scno, regs->ARM_r0, regs->ARM_r1, regs->ARM_r2,
> + regs->ARM_r3);
>
> return scno;
> }
> diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
> index b7a5fff..6f54d51 100644
> --- a/arch/ia64/kernel/ptrace.c
> +++ b/arch/ia64/kernel/ptrace.c
> @@ -1219,7 +1219,7 @@ syscall_trace_enter (long arg0, long arg1, long arg2, long arg3,
> ia64_sync_krbs();
>
>
> - audit_syscall_entry(AUDIT_ARCH_IA64, regs.r15, arg0, arg1, arg2, arg3);
> + audit_syscall_entry(regs.r15, arg0, arg1, arg2, arg3);
>
> return 0;
> }
> diff --git a/arch/microblaze/kernel/ptrace.c b/arch/microblaze/kernel/ptrace.c
> index 39cf508..bb10637 100644
> --- a/arch/microblaze/kernel/ptrace.c
> +++ b/arch/microblaze/kernel/ptrace.c
> @@ -147,8 +147,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> */
> ret = -1L;
>
> - audit_syscall_entry(EM_MICROBLAZE, regs->r12, regs->r5, regs->r6,
> - regs->r7, regs->r8);
> + audit_syscall_entry(regs->r12, regs->r5, regs->r6, regs->r7, regs->r8);
>
> return ret ?: regs->r12;
> }
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 65ba622..c06bb82 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -671,9 +671,7 @@ asmlinkage void syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->regs[2]);
>
> - audit_syscall_entry(syscall_get_arch(),
> - regs->regs[2],
> - regs->regs[4], regs->regs[5],
> + audit_syscall_entry(regs->regs[2], regs->regs[4], regs->regs[5],
> regs->regs[6], regs->regs[7]);
> }
>
> diff --git a/arch/openrisc/kernel/ptrace.c b/arch/openrisc/kernel/ptrace.c
> index 71a2a0c..4f59fa4 100644
> --- a/arch/openrisc/kernel/ptrace.c
> +++ b/arch/openrisc/kernel/ptrace.c
> @@ -187,8 +187,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> */
> ret = -1L;
>
> - audit_syscall_entry(AUDIT_ARCH_OPENRISC, regs->gpr[11],
> - regs->gpr[3], regs->gpr[4],
> + audit_syscall_entry(regs->gpr[11], regs->gpr[3], regs->gpr[4],
> regs->gpr[5], regs->gpr[6]);
>
> return ret ? : regs->gpr[11];
> diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
> index e842ee2..7481457 100644
> --- a/arch/parisc/kernel/ptrace.c
> +++ b/arch/parisc/kernel/ptrace.c
> @@ -276,14 +276,11 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>
> #ifdef CONFIG_64BIT
> if (!is_compat_task())
> - audit_syscall_entry(AUDIT_ARCH_PARISC64,
> - regs->gr[20],
> - regs->gr[26], regs->gr[25],
> - regs->gr[24], regs->gr[23]);
> + audit_syscall_entry(regs->gr[20], regs->gr[26], regs->gr[25],
> + regs->gr[24], regs->gr[23]);
> else
> #endif
> - audit_syscall_entry(AUDIT_ARCH_PARISC,
> - regs->gr[20] & 0xffffffff,
> + audit_syscall_entry(regs->gr[20] & 0xffffffff,
> regs->gr[26] & 0xffffffff,
> regs->gr[25] & 0xffffffff,
> regs->gr[24] & 0xffffffff,
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 2e3d2bf..524a943 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -1788,14 +1788,11 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>
> #ifdef CONFIG_PPC64
> if (!is_32bit_task())
> - audit_syscall_entry(AUDIT_ARCH_PPC64,
> - regs->gpr[0],
> - regs->gpr[3], regs->gpr[4],
> + audit_syscall_entry(regs->gpr[0], regs->gpr[3], regs->gpr[4],
> regs->gpr[5], regs->gpr[6]);
> else
> #endif
> - audit_syscall_entry(AUDIT_ARCH_PPC,
> - regs->gpr[0],
> + audit_syscall_entry(regs->gpr[0],
> regs->gpr[3] & 0xffffffff,
> regs->gpr[4] & 0xffffffff,
> regs->gpr[5] & 0xffffffff,
> diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
> index e65c91c..2e2e7bb5 100644
> --- a/arch/s390/kernel/ptrace.c
> +++ b/arch/s390/kernel/ptrace.c
> @@ -812,9 +812,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->gprs[2]);
>
> - audit_syscall_entry(is_compat_task() ?
> - AUDIT_ARCH_S390 : AUDIT_ARCH_S390X,
> - regs->gprs[2], regs->orig_gpr2,
> + audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
> regs->gprs[3], regs->gprs[4],
> regs->gprs[5]);
> out:
> diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
> index 668c816..c1a6b89 100644
> --- a/arch/sh/kernel/ptrace_32.c
> +++ b/arch/sh/kernel/ptrace_32.c
> @@ -484,17 +484,6 @@ long arch_ptrace(struct task_struct *child, long request,
> return ret;
> }
>
> -static inline int audit_arch(void)
> -{
> - int arch = EM_SH;
> -
> -#ifdef CONFIG_CPU_LITTLE_ENDIAN
> - arch |= __AUDIT_ARCH_LE;
> -#endif
> -
> - return arch;
> -}
> -
> asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> {
> long ret = 0;
> @@ -513,8 +502,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->regs[0]);
>
> - audit_syscall_entry(audit_arch(), regs->regs[3],
> - regs->regs[4], regs->regs[5],
> + audit_syscall_entry(regs->regs[3], regs->regs[4], regs->regs[5],
> regs->regs[6], regs->regs[7]);
>
> return ret ?: regs->regs[0];
> diff --git a/arch/sh/kernel/ptrace_64.c b/arch/sh/kernel/ptrace_64.c
> index af90339..5cea973 100644
> --- a/arch/sh/kernel/ptrace_64.c
> +++ b/arch/sh/kernel/ptrace_64.c
> @@ -504,20 +504,6 @@ asmlinkage int sh64_ptrace(long request, long pid,
> return sys_ptrace(request, pid, addr, data);
> }
>
> -static inline int audit_arch(void)
> -{
> - int arch = EM_SH;
> -
> -#ifdef CONFIG_64BIT
> - arch |= __AUDIT_ARCH_64BIT;
> -#endif
> -#ifdef CONFIG_CPU_LITTLE_ENDIAN
> - arch |= __AUDIT_ARCH_LE;
> -#endif
> -
> - return arch;
> -}
> -
> asmlinkage long long do_syscall_trace_enter(struct pt_regs *regs)
> {
> long long ret = 0;
> @@ -536,8 +522,7 @@ asmlinkage long long do_syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->regs[9]);
>
> - audit_syscall_entry(audit_arch(), regs->regs[1],
> - regs->regs[2], regs->regs[3],
> + audit_syscall_entry(regs->regs[1], regs->regs[2], regs->regs[3],
> regs->regs[4], regs->regs[5]);
>
> return ret ?: regs->regs[9];
> diff --git a/arch/sparc/kernel/ptrace_64.c b/arch/sparc/kernel/ptrace_64.c
> index c13c9f2..9ddc492 100644
> --- a/arch/sparc/kernel/ptrace_64.c
> +++ b/arch/sparc/kernel/ptrace_64.c
> @@ -1076,13 +1076,8 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->u_regs[UREG_G1]);
>
> - audit_syscall_entry((test_thread_flag(TIF_32BIT) ?
> - AUDIT_ARCH_SPARC :
> - AUDIT_ARCH_SPARC64),
> - regs->u_regs[UREG_G1],
> - regs->u_regs[UREG_I0],
> - regs->u_regs[UREG_I1],
> - regs->u_regs[UREG_I2],
> + audit_syscall_entry(regs->u_regs[UREG_G1], regs->u_regs[UREG_I0],
> + regs->u_regs[UREG_I1], regs->u_regs[UREG_I2],
> regs->u_regs[UREG_I3]);
>
> return ret;
> diff --git a/arch/um/kernel/ptrace.c b/arch/um/kernel/ptrace.c
> index 694d551..62435ef 100644
> --- a/arch/um/kernel/ptrace.c
> +++ b/arch/um/kernel/ptrace.c
> @@ -165,8 +165,7 @@ static void send_sigtrap(struct task_struct *tsk, struct uml_pt_regs *regs,
> */
> void syscall_trace_enter(struct pt_regs *regs)
> {
> - audit_syscall_entry(HOST_AUDIT_ARCH,
> - UPT_SYSCALL_NR(®s->regs),
> + audit_syscall_entry(UPT_SYSCALL_NR(®s->regs),
> UPT_SYSCALL_ARG1(®s->regs),
> UPT_SYSCALL_ARG2(®s->regs),
> UPT_SYSCALL_ARG3(®s->regs),
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 7461f50..46dfba6 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -1488,15 +1488,11 @@ long syscall_trace_enter(struct pt_regs *regs)
> trace_sys_enter(regs, regs->orig_ax);
>
> if (IS_IA32)
> - audit_syscall_entry(AUDIT_ARCH_I386,
> - regs->orig_ax,
> - regs->bx, regs->cx,
> + audit_syscall_entry(regs->orig_ax, regs->bx, regs->cx,
> regs->dx, regs->si);
> #ifdef CONFIG_X86_64
> else
> - audit_syscall_entry(AUDIT_ARCH_X86_64,
> - regs->orig_ax,
> - regs->di, regs->si,
> + audit_syscall_entry(regs->orig_ax, regs->di, regs->si,
> regs->dx, regs->r10);
> #endif
>
> diff --git a/arch/x86/um/asm/ptrace.h b/arch/x86/um/asm/ptrace.h
> index 54f8102..e59eef2 100644
> --- a/arch/x86/um/asm/ptrace.h
> +++ b/arch/x86/um/asm/ptrace.h
> @@ -47,8 +47,6 @@ struct user_desc;
>
> #ifdef CONFIG_X86_32
>
> -#define HOST_AUDIT_ARCH AUDIT_ARCH_I386
> -
> extern int ptrace_get_thread_area(struct task_struct *child, int idx,
> struct user_desc __user *user_desc);
>
> @@ -57,8 +55,6 @@ extern int ptrace_set_thread_area(struct task_struct *child, int idx,
>
> #else
>
> -#define HOST_AUDIT_ARCH AUDIT_ARCH_X86_64
> -
> #define PT_REGS_R8(r) UPT_R8(&(r)->regs)
> #define PT_REGS_R9(r) UPT_R9(&(r)->regs)
> #define PT_REGS_R10(r) UPT_R10(&(r)->regs)
> diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
> index 562fac6..4d54b48 100644
> --- a/arch/xtensa/kernel/ptrace.c
> +++ b/arch/xtensa/kernel/ptrace.c
> @@ -342,7 +342,7 @@ void do_syscall_trace_enter(struct pt_regs *regs)
> do_syscall_trace();
>
> #if 0
> - audit_syscall_entry(current, AUDIT_ARCH_XTENSA..);
> + audit_syscall_entry(...);
> #endif
> }
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 4b2983e..62c9d98 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
> #include <linux/sched.h>
> #include <linux/ptrace.h>
> #include <uapi/linux/audit.h>
> +#include <asm/syscall.h>
>
> struct audit_sig_info {
> uid_t uid;
> @@ -135,12 +136,12 @@ static inline void audit_free(struct task_struct *task)
> if (unlikely(task->audit_context))
> __audit_free(task);
> }
> -static inline void audit_syscall_entry(int arch, int major, unsigned long a0,
> +static inline void audit_syscall_entry(int major, unsigned long a0,
> unsigned long a1, unsigned long a2,
> unsigned long a3)
> {
> if (unlikely(current->audit_context))
> - __audit_syscall_entry(arch, major, a0, a1, a2, a3);
> + __audit_syscall_entry(syscall_get_arch(), major, a0, a1, a2, a3);
> }
> static inline void audit_syscall_exit(void *pt_regs)
> {
> @@ -316,7 +317,7 @@ static inline int audit_alloc(struct task_struct *task)
> }
> static inline void audit_free(struct task_struct *task)
> { }
> -static inline void audit_syscall_entry(int arch, int major, unsigned long a0,
> +static inline void audit_syscall_entry(int major, unsigned long a0,
> unsigned long a1, unsigned long a2,
> unsigned long a3)
> { }
> --
> 1.8.5.3
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 9/9] powerpc/pm: support deep sleep feature on T1040
From: Scott Wood @ 2014-03-21 21:16 UTC (permalink / raw)
To: David Laight
Cc: linuxppc-dev@lists.ozlabs.org, 'Kevin Hao', Chenhui Zhao,
Jason.Jin@freescale.com, linux-kernel@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D0F6E40C4@AcuExch.aculab.com>
On Fri, 2014-03-21 at 09:21 +0000, David Laight wrote:
> From: Scott Wood [mailto:scottwood@freescale.com]
> > On Thu, 2014-03-20 at 11:59 +0000, David Laight wrote:
> > > I tried to work out what the 'twi, isync' instructions were for (in in_le32()).
> > > The best I could come up with was to ensure a synchronous bus-fault.
> > > But bus faults are probably only expected during device probing - not
> > > normal operation, and the instructions will have a significant cost.
> > >
> > > Additionally in_le32() and out_le32() both start with a 'sync' instruction.
> > > In many cases that isn't needed either - an explicit iosync() can be
> > > used after groups of instructions.
> >
> > The idea is that it's better to be maximally safe by default, and let
> > performance critical sections be optimized using raw accessors and
> > explicit synchronization if needed, than to have hard-to-debug bugs due
> > to missing/wrong sync. A lot of I/O is slow enough that the performance
> > impact doesn't really matter, but the brain-time cost of getting the
> > sync right is still there.
>
> Hmmm....
>
> That might be an excuse for the 'sync', but not the twi and isync.
That might be true if I/O is always cache inhibited and guarded, in
which case I think we can rely on that to ensure that the load has
completed before we do things like wrtee or rfi. In any case, I'd want
to hear Ben's explanation.
> I was setting up a dma request (for the ppc 83xx PCIe bridge) and
> was doing back to back little-endian writes into memory.
> I had difficulty finding and including header files containing
> the definitions for byteswapped accesses I needed.
> arch/powerpc/include/asm/swab.h contains some - but I couldn't
> work out how to get it included (apart from giving the full path).
>
> In any case you need to understand when synchronisation is
> required - otherwise you will get it wrong.
> Especially since non-byteswapped accesses are done by direct
> access.
Yes, it's bad that rawness combines the lack of byteswapping with the
lack of synchronization. Ideally the raw accessors would also come in
big and little endian form, plus a native endian form if it's really
needed.
-Scott
^ permalink raw reply
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Benjamin Herrenschmidt @ 2014-03-21 22:56 UTC (permalink / raw)
To: ego; +Cc: Viresh Kumar, Linux PM list, linuxppc-dev
In-Reply-To: <20140321110445.GB2493@in.ibm.com>
On Fri, 2014-03-21 at 16:34 +0530, Gautham R Shenoy wrote:
> > >
> > > +/*
> > > + * Computes the current frequency on this cpu
> > > + * and stores the result in *ret_freq.
> > > + */
> > > +static void powernv_read_cpu_freq(void *ret_freq)
> > > +{
> > > + unsigned long pmspr_val;
> > > + s8 local_pstate_id;
> > > + int *cur_freq, freq, pstate_id;
> > > +
> > > + cur_freq = (int *)ret_freq;
> >
> > You don't need cur_freq variable at all..
>
> I don't like it either. But the compiler complains without this hack
> :-(
Casting integers into void * is a recipe for disaster... what is that
supposed to be about ? We lose all type checking and get exposed
to endian issues etc... the day somebody uses a different type on both
sides.
Also is "freq" a frequency ? In this case an int isn't big enough.
Cheers,
Ben.
^ permalink raw reply
* [RFC][PATCH] perf: Add 'merge-recursive' callchain option
From: Sukadev Bhattiprolu @ 2014-03-22 2:01 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Anton Blanchard, linux-kernel, linuxppc-dev,
Paul Mackerras, Jiri Olsa
>From 9ad9432dab2bf4d1c8e6ff9201e88d5ae9f3994a Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Wed, 19 Mar 2014 20:24:22 -0500
Subject: [PATCH 1/1] perf: Add 'merge-recursive' callchain option
Powerpc saves the link register (LR) with each sample to help resolve
callchains for programs involving tail calls. Sometimes the value in the
LR is same as the NIP register. Other times it is not. This results in
callchains samples like these:
3547953334790 0x1ec0 [0x78]: PERF_RECORD_SAMPLE(IP, 2): 4667/4667: 0x80a7be3c58 period: 1773985 addr: 0
... chain: nr:9
..... 0: fffffffffffffe00
..... 1: 00000080a7be3c58 __random
..... 2: 00000080a7be3c40 __random
..... 3: 00000080a7be4270 rand
..... 4: 0000000010000784 do_my_sprintf
..... 5: 00000000100009d8 main
..... 6: 00000080a7bc482c
..... 7: 00000080a7bc4a34
..... 8: 0000000000000000
... thread: sprintft:4667
...... dso: /usr/lib64/libc-2.18.so
67470723107802 0x2f10 [0x78]: PERF_RECORD_SAMPLE(IP, 0x2): 5706/5706: 0x80a7be3c20 period: 872629 addr: 0
... chain: nr:9
..... 0: fffffffffffffe00
..... 1: 00000080a7be3c20 __random
..... 2: 00000080a7be4270 rand
..... 3: 00000080a7be4270 rand
..... 4: 0000000010000784
..... 5: 00000000100009d8
..... 6: 00000080a7bc482c
..... 7: 00000080a7bc4a34
..... 8: 0000000000000000
... thread: sprintft:5706
...... dso: /usr/lib64/libc-2.18.so
67470738362072 0x4cf8 [0x78]: PERF_RECORD_SAMPLE(IP, 0x2): 5706/5706: 0x80a7be3c14 period: 869774 addr: 0
... chain: nr:9
..... 0: fffffffffffffe00
..... 1: 00000080a7be3c14 __random
..... 2: 00000080a7be4270 rand
..... 3: 00000080a7be4270 rand
..... 4: 0000000010000784 do_my_sprintf
..... 5: 00000000100009d8 main
..... 6: 00000080a7bc482c
..... 7: 00000080a7bc4a34
..... 8: 0000000000000000
... thread: sprintft:5706
...... dso: /usr/lib64/libc-2.18.so
In the perf tool, these samples end up on different branches of the RB-Tree
resulting in duplicated arcs in the final call-graph.
15.02% sprintft libc-2.18.so [.] __random
|
--- __random
|
|--58.45%-- rand
| rand
| do_my_sprintf
| main
| generic_start_main.isra.0
| __libc_start_main
| 0x0
|
--41.55%-- __random
rand
do_my_sprintf
main
generic_start_main.isra.0
__libc_start_main
0x0
This is an RFC for adding a 'merge-recursive' call-graph option that would
discard the duplicate entries resulting in a more compact call-graph. The
duplicates can be either identical instruction pointer values or IP values
within the same function.
perf report --call-graph=fractal,0.5,callee,function,merge-recursive
15.02% sprintft libc-2.18.so [.] __random
|
---__random
rand
do_my_sprintf
main
generic_start_main.isra.0
__libc_start_main
(nil)
This option could also be used to collapse call-graphs of recursive functions.
AFAICT, the existing CCKEY_FUNCTION does not address this case because
the callchain lengths can vary across samples.
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
tools/perf/builtin-report.c | 14 ++++++++++++--
tools/perf/util/callchain.h | 1 +
tools/perf/util/machine.c | 39 +++++++++++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index d882b6f..0b68749 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -647,6 +647,16 @@ parse_callchain_opt(const struct option *opt, const char *arg, int unset)
callchain_param.key = CCKEY_ADDRESS;
else
return -1;
+
+ tok2 = strtok(NULL, ",");
+ if (!tok2)
+ goto setup;
+
+ if (!strncmp(tok2, "merge-recursive", strlen("merge-recursive")))
+ callchain_param.merge_recursive = 1;
+ else
+ return -1;
+
setup:
if (callchain_register_param(&callchain_param) < 0) {
pr_err("Can't register callchain params\n");
@@ -762,8 +772,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
"regex filter to identify parent, see: '--sort parent'"),
OPT_BOOLEAN('x', "exclude-other", &symbol_conf.exclude_other,
"Only display entries with parent-match"),
- OPT_CALLBACK_DEFAULT('g', "call-graph", &report, "output_type,min_percent[,print_limit],call_order",
- "Display callchains using output_type (graph, flat, fractal, or none) , min percent threshold, optional print limit, callchain order, key (function or address). "
+ OPT_CALLBACK_DEFAULT('g', "call-graph", &report, "output_type,min_percent[,print_limit],call_order,merge-recursive",
+ "Display callchains using output_type (graph, flat, fractal, or none) , min percent threshold, optional print limit, callchain order, key (function or address), merge or don't merge recursive calls"
"Default: fractal,0.5,callee,function", &parse_callchain_opt, callchain_default_opt),
OPT_INTEGER(0, "max-stack", &report.max_stack,
"Set the maximum stack depth when parsing the callchain, "
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 8ad97e9..d8d0822 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -53,6 +53,7 @@ struct callchain_param {
sort_chain_func_t sort;
enum chain_order order;
enum chain_key key;
+ u32 merge_recursive;
};
struct callchain_list {
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index eb26544..e74ad95 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1272,6 +1272,32 @@ struct branch_info *sample__resolve_bstack(struct perf_sample *sample,
return bi;
}
+static int duplicate_symbol(struct symbol *sym, char **prev_name)
+{
+ char *prev = *prev_name;
+
+ if (sym) {
+ if (prev) {
+ if (!strcmp(sym->name, prev))
+ return 1; /* duplicate */
+ free(prev);
+ }
+
+ *prev_name = prev = strdup(sym->name);
+ if (!prev) {
+ pr_debug("%s: Unable to alloc memory\n", __func__);
+ return -ENOMEM;
+ }
+ } else if (prev) {
+ /* new symbol is NULL, so forget prev name */
+ free(prev);
+ *prev_name = NULL;
+ }
+
+ /* Not duplicate */
+ return 0;
+}
+
static int machine__resolve_callchain_sample(struct machine *machine,
struct thread *thread,
struct ip_callchain *chain,
@@ -1283,6 +1309,8 @@ static int machine__resolve_callchain_sample(struct machine *machine,
int chain_nr = min(max_stack, (int)chain->nr);
int i;
int err;
+ u64 prev_ip = 0;
+ char *prev_name = NULL;
callchain_cursor_reset(&callchain_cursor);
@@ -1324,6 +1352,10 @@ static int machine__resolve_callchain_sample(struct machine *machine,
continue;
}
+ if (callchain_param.merge_recursive && prev_ip == ip)
+ continue;
+ prev_ip = ip;
+
al.filtered = false;
thread__find_addr_location(thread, machine, cpumode,
MAP__FUNCTION, ip, &al);
@@ -1340,6 +1372,13 @@ static int machine__resolve_callchain_sample(struct machine *machine,
}
}
+ if (callchain_param.merge_recursive) {
+ if ((err = duplicate_symbol(al.sym, &prev_name)) > 0)
+ continue; /* duplicate */
+ else if (err < 0)
+ return err;
+ }
+
err = callchain_cursor_append(&callchain_cursor,
ip, al.map, al.sym);
if (err)
--
1.7.9.5
^ permalink raw reply related
* Re: Tasks stuck in futex code (in 3.14-rc6)
From: Srikar Dronamraju @ 2014-03-22 2:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Zijlstra, LKML, Davidlohr Bueso, Paul Mackerras,
Thomas Gleixner, Paul McKenney, ppc-dev, Ingo Molnar
In-Reply-To: <CA+55aFyX0sj_MjTas3RYWWOa1W-xc3t0Af_0vdmsmgui8Apqpw@mail.gmail.com>
> > So reverting and applying v3 3/4 and 4/4 patches works for me.
>
> Ok, I verified that the above endds up resulting in the same tree as
> the minimal patch I sent out, modulo (a) some comments and (b) an
> #ifdef CONFIG_SMP in futex_get_mm() that doesn't really matter.
>
> So I committed the minimal patch with your tested-by.
>
Just to be sure, I have verified with latest mainline with HEAD having
commit 08edb33c4 (Merge branch 'drm-fixes' of
git://people.freedesktop.org/~airlied/linux) which also has the commit
11d4616bd0 futex:( revert back to the explicit waiter counting code).
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: Tasks stuck in futex code (in 3.14-rc6)
From: Davidlohr Bueso @ 2014-03-22 3:36 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Peter Zijlstra, Linus Torvalds, LKML, Paul Mackerras,
Thomas Gleixner, Paul McKenney, ppc-dev, Ingo Molnar
In-Reply-To: <20140322022754.GA21677@linux.vnet.ibm.com>
On Sat, 2014-03-22 at 07:57 +0530, Srikar Dronamraju wrote:
> > > So reverting and applying v3 3/4 and 4/4 patches works for me.
> >
> > Ok, I verified that the above endds up resulting in the same tree as
> > the minimal patch I sent out, modulo (a) some comments and (b) an
> > #ifdef CONFIG_SMP in futex_get_mm() that doesn't really matter.
> >
> > So I committed the minimal patch with your tested-by.
> >
>
> Just to be sure, I have verified with latest mainline with HEAD having
> commit 08edb33c4 (Merge branch 'drm-fixes' of
> git://people.freedesktop.org/~airlied/linux) which also has the commit
> 11d4616bd0 futex:( revert back to the explicit waiter counting code).
Thanks for double checking.
^ permalink raw reply
* Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
From: Viresh Kumar @ 2014-03-22 3:43 UTC (permalink / raw)
To: Vaidyanathan Srinivasan
Cc: Gautham R Shenoy, Linux PM list, linuxppc-dev@ozlabs.org,
Anton Blanchard, Srivatsa S. Bhat
In-Reply-To: <20140321144818.GA30371@dirshya.in.ibm.com>
On 21 March 2014 20:18, Vaidyanathan Srinivasan
<svaidy@linux.vnet.ibm.com> wrote:
> Yeah, I had the driver written using driver_data to store pstates.
> Gautham found the bug that we are missing one PState when we match the
> ID with CPUFREQ_BOOST_FREQ!
I see..
> We did not know that you have taken care of those issues. Ideally
> I did expect that driver_data should not be touched by the framework.
> Thanks for fixing that and allowing the back-end driver to use
> driver_data.
No, I haven't fixed anything yet. And this piece of code still exists.
I will see if I can get this fixed, by that time you can continue the
way your code is there in this version.
^ permalink raw reply
* Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
From: Viresh Kumar @ 2014-03-22 3:43 UTC (permalink / raw)
To: ego@linux.vnet.ibm.com
Cc: Linux PM list, linuxppc-dev@ozlabs.org, Anton Blanchard,
Srivatsa S. Bhat
In-Reply-To: <20140321145452.GA8755@in.ibm.com>
On 21 March 2014 20:24, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> Ok, I guess the right thing to do at this point is call
>
> cpufreq_table_validate_and_show(policy, powernv_freqs);
>
> Will fix the code to take care of this.
Yes.
^ permalink raw reply
* [PATCH] arch/powerpc: Use RCU_INIT_POINTER(x, NULL) in platforms/cell/spu_syscalls.c
From: Monam Agarwal @ 2014-03-22 6:50 UTC (permalink / raw)
To: arnd, benh, paulus, linuxppc-dev, cbe-oss-dev, linux-kernel
Here rcu_assign_pointer() is ensuring that the
initialization of a structure is carried out before storing a pointer
to that structure.
So, rcu_assign_pointer(p, NULL) can always safely be converted to
RCU_INIT_POINTER(p, NULL).
Signed-off-by: Monam Agarwal <monamagarwal123@gmail.com>
---
arch/powerpc/platforms/cell/spu_syscalls.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/cell/spu_syscalls.c b/arch/powerpc/platforms/cell/spu_syscalls.c
index 3844f13..38e0a1a 100644
--- a/arch/powerpc/platforms/cell/spu_syscalls.c
+++ b/arch/powerpc/platforms/cell/spu_syscalls.c
@@ -170,7 +170,7 @@ EXPORT_SYMBOL_GPL(register_spu_syscalls);
void unregister_spu_syscalls(struct spufs_calls *calls)
{
BUG_ON(spufs_calls->owner != calls->owner);
- rcu_assign_pointer(spufs_calls, NULL);
+ RCU_INIT_POINTER(spufs_calls, NULL);
synchronize_rcu();
}
EXPORT_SYMBOL_GPL(unregister_spu_syscalls);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver->get() method
From: Gautham R Shenoy @ 2014-03-22 7:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, ego, Linux PM list, Viresh Kumar
In-Reply-To: <1395442590.3460.85.camel@pasglop>
Hi Ben,
On Sat, Mar 22, 2014 at 09:56:30AM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2014-03-21 at 16:34 +0530, Gautham R Shenoy wrote:
>
> > > >
> > > > +/*
> > > > + * Computes the current frequency on this cpu
> > > > + * and stores the result in *ret_freq.
> > > > + */
> > > > +static void powernv_read_cpu_freq(void *ret_freq)
> > > > +{
> > > > + unsigned long pmspr_val;
> > > > + s8 local_pstate_id;
> > > > + int *cur_freq, freq, pstate_id;
> > > > +
> > > > + cur_freq = (int *)ret_freq;
> > >
> > > You don't need cur_freq variable at all..
> >
> > I don't like it either. But the compiler complains without this hack
> > :-(
>
> Casting integers into void * is a recipe for disaster... what is that
> supposed to be about ?
Like I mentioned elsewhere on this thread, we're calling
powernv_read_cpu_freq via an smp_call_function(). We use this to
obtain the frequency on the cpu where powernv_read_cpu_freq
executes and return it to the caller of smp_call_function.
> We lose all type checking and get exposed
> to endian issues etc... the day somebody uses a different type on both
> sides.
>
Yes, I understand the problem now. I'll think of a safer way to pass
the return value.
> Also is "freq" a frequency ? In this case an int isn't big enough.
freq is the frequency stored in the cpufreq_table. The value is in
kHz. So, int should be big enough.
> Cheers,
> Ben.
>
>
--
Thanks and Regards
gautham.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox