Re: [PATCH v1] mm: relax deferred struct page requirements

2017-11-20 Thread Michal Hocko 
On Thu 16-11-17 20:46:01, Pavel Tatashin wrote:
> There is no need to have ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT,
> as all the page initialization code is in common code.
> 
> Also, there is no need to depend on MEMORY_HOTPLUG, as initialization code
> does not really use hotplug memory functionality. So, we can remove this
> requirement as well.
> 
> This patch allows to use deferred struct page initialization on all
> platforms with memblock allocator.
> 
> Tested on x86, arm64, and sparc. Also, verified that code compiles on
> PPC with CONFIG_MEMORY_HOTPLUG disabled.

There is slight risk that we will encounter corner cases on some
architectures with weird memory layout/topology but we should better
explicitly disable this code rather than make it opt-in so this looks
like an improvement to me.
 
> Signed-off-by: Pavel Tatashin 

Acked-by: Michal Hocko 

> ---
>  arch/powerpc/Kconfig | 1 -
>  arch/s390/Kconfig| 1 -
>  arch/x86/Kconfig | 1 -
>  mm/Kconfig   | 7 +--
>  4 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index cb782ac1c35d..1540348691c9 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -148,7 +148,6 @@ config PPC
>   select ARCH_MIGHT_HAVE_PC_PARPORT
>   select ARCH_MIGHT_HAVE_PC_SERIO
>   select ARCH_SUPPORTS_ATOMIC_RMW
> - select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
>   select ARCH_USE_BUILTIN_BSWAP
>   select ARCH_USE_CMPXCHG_LOCKREF if PPC64
>   select ARCH_WANT_IPC_PARSE_VERSION
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 863a62a6de3c..525c2e3df6f5 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -108,7 +108,6 @@ config S390
>   select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
>   select ARCH_SAVE_PAGE_KEYS if HIBERNATION
>   select ARCH_SUPPORTS_ATOMIC_RMW
> - select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
>   select ARCH_SUPPORTS_NUMA_BALANCING
>   select ARCH_USE_BUILTIN_BSWAP
>   select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index df3276d6bfe3..00a5446de394 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -69,7 +69,6 @@ config X86
>   select ARCH_MIGHT_HAVE_PC_PARPORT
>   select ARCH_MIGHT_HAVE_PC_SERIO
>   select ARCH_SUPPORTS_ATOMIC_RMW
> - select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
>   select ARCH_SUPPORTS_NUMA_BALANCING if X86_64
>   select ARCH_USE_BUILTIN_BSWAP
>   select ARCH_USE_QUEUED_RWLOCKS
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 9c4b80c2..c6bd0309ce7a 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -639,15 +639,10 @@ config MAX_STACK_SIZE_MB
>  
> A sane initial value is 80 MB.
>  
> -# For architectures that support deferred memory initialisation
> -config ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
> - bool
> -
>  config DEFERRED_STRUCT_PAGE_INIT
>   bool "Defer initialisation of struct pages to kthreads"
>   default n
> - depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
> - depends on NO_BOOTMEM && MEMORY_HOTPLUG
> + depends on NO_BOOTMEM
>   depends on !FLATMEM
>   help
> Ordinarily all struct pages are initialised during early boot in a
> -- 
> 2.15.0

-- 
Michal Hocko
SUSE Labs

[PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

2017-11-20 Thread Cyril Bur 
Currently the tm-unavailable test spins for a fixed amount of time in
an attempt to ensure the FPU/VMX/VSX facilities are off. This value was
experimentally tested to be long enough.

Problems may arise if kernel heuristics were to change. This patch
should future proof this test.

Signed-off-by: Cyril Bur 
---
Because the test no longer needs to use such a conservative time for
the busy wait, it actually runs much faster.


 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 92 --
 1 file changed, 84 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index e6a0fad2bfd0..54aeb7a7fbb1 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -33,6 +33,11 @@
 #define VEC_UNA_EXCEPTION  1
 #define VSX_UNA_EXCEPTION  2
 
+#define ERR_RETRY 1
+#define ERR_ADJUST 2
+
+#define COUNTER_INCREMENT (0x100)
+
 #define NUM_EXCEPTIONS 3
 #define err_at_line(status, errnum, format, ...) \
error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
@@ -45,6 +50,7 @@ struct Flags {
int touch_vec;
int result;
int exception;
+   uint64_t counter;
 } flags;
 
 bool expecting_failure(void)
@@ -87,14 +93,12 @@ void *ping(void *input)
 * Expected values for vs0 and vs32 after a TM failure. They must never
 * change, otherwise they got corrupted.
 */
+   long rc = 0;
uint64_t high_vs0 = 0x;
uint64_t low_vs0 = 0x;
uint64_t high_vs32 = 0x;
uint64_t low_vs32 = 0x;
 
-   /* Counter for busy wait */
-   uint64_t counter = 0x1ff00;
-
/*
 * Variable to keep a copy of CR register content taken just after we
 * leave the transactional state.
@@ -217,7 +221,7 @@ void *ping(void *input)
  [ex_fp] "i"  (FP_UNA_EXCEPTION),
  [ex_vec]"i"  (VEC_UNA_EXCEPTION),
  [ex_vsx]"i"  (VSX_UNA_EXCEPTION),
- [counter]   "r"  (counter)
+ [counter]   "r"  (flags.counter)
 
: "cr0", "ctr", "v10", "vs0", "vs10", "vs3", "vs32", "vs33",
  "vs34", "fr10"
@@ -232,14 +236,14 @@ void *ping(void *input)
if (expecting_failure() && !is_failure(cr_)) {
printf("\n\tExpecting the transaction to fail, %s",
"but it didn't\n\t");
-   flags.result++;
+   rc = ERR_ADJUST;
}
 
/* Check if we were not expecting a failure and a it occurred. */
if (!expecting_failure() && is_failure(cr_)) {
printf("\n\tUnexpected transaction failure 0x%02lx\n\t",
failure_code());
-   return (void *) -1;
+   rc = ERR_RETRY;
}
 
/*
@@ -249,7 +253,7 @@ void *ping(void *input)
if (is_failure(cr_) && !failure_is_unavailable()) {
printf("\n\tUnexpected failure cause 0x%02lx\n\t",
failure_code());
-   return (void *) -1;
+   rc = ERR_RETRY;
}
 
/* 0x4 is a success and 0xa is a fail. See comment in is_failure(). */
@@ -276,7 +280,7 @@ void *ping(void *input)
 
putchar('\n');
 
-   return NULL;
+   return (void *)rc;
 }
 
 /* Thread to force context switch */
@@ -291,6 +295,55 @@ void *pong(void *not_used)
sched_yield();
 }
 
+static void flags_set_counter(struct Flags *flags)
+{
+   uint64_t cr_;
+   int count = 0;
+
+   do {
+   if (count == 0)
+   printf("\tTrying 0x%08" PRIx64 "... ", flags->counter);
+   else
+   printf("%d, ", count);
+   fflush(stdout);
+   asm (
+   /*
+* Wait an amount of context switches so
+* load_fp and load_vec overflow and MSR.FP,
+* MSR.VEC, and MSR.VSX become zero (off).
+*/
+   "   mtctr   %[counter]  ;"
+
+   /* Decrement CTR branch if CTR non zero. */
+   "1: bdnz 1b ;"
+   "   tbegin. ;"
+   "   beq tfail   ;"
+
+   /* Get a facility unavailable */
+   "   fadd10, 10, 10  ;"
+   "   tend.   ;"
+   "tfail: ;"
+
+   /*
+* Give CR back to C so that it can check what
+

[PATCH 1/2] selftests/powerpc: Check for pthread errors in tm-unavailable

2017-11-20 Thread Cyril Bur 
Signed-off-by: Cyril Bur 
---
 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 43 +-
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index 96c37f84ce54..e6a0fad2bfd0 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -15,6 +15,7 @@
  */
 
 #define _GNU_SOURCE
+#include 
 #include 
 #include 
 #include 
@@ -33,6 +34,11 @@
 #define VSX_UNA_EXCEPTION  2
 
 #define NUM_EXCEPTIONS 3
+#define err_at_line(status, errnum, format, ...) \
+   error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
+
+#define pr_warn(code, format, ...) err_at_line(0, code, format, ##__VA_ARGS__)
+#define pr_err(code, format, ...) err_at_line(1, code, format, ##__VA_ARGS__)
 
 struct Flags {
int touch_fp;
@@ -303,10 +309,19 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 * checking if the failure cause is the one we expect.
 */
do {
+   int rc;
+
/* Bind 'ping' to CPU 0, as specified in 'attr'. */
-   pthread_create(, attr, ping, (void *) );
-   pthread_setname_np(t0, "ping");
-   pthread_join(t0, _value);
+   rc = pthread_create(, attr, ping, (void *) );
+   if (rc)
+   pr_err(rc, "pthread_create()");
+   rc = pthread_setname_np(t0, "ping");
+   if (rc)
+   pr_warn(rc, "pthread_setname_np");
+   rc = pthread_join(t0, _value);
+   if (rc)
+   pr_err(rc, "pthread_join");
+
retries--;
} while (ret_value != NULL && retries);
 
@@ -320,7 +335,7 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 
 int main(int argc, char **argv)
 {
-   int exception; /* FP = 0, VEC = 1, VSX = 2 */
+   int rc, exception; /* FP = 0, VEC = 1, VSX = 2 */
pthread_t t1;
pthread_attr_t attr;
cpu_set_t cpuset;
@@ -330,13 +345,23 @@ int main(int argc, char **argv)
CPU_SET(0, );
 
/* Init pthread attribute. */
-   pthread_attr_init();
+   rc = pthread_attr_init();
+   if (rc)
+   pr_err(rc, "pthread_attr_init()");
 
/* Set CPU 0 mask into the pthread attribute. */
-   pthread_attr_setaffinity_np(, sizeof(cpu_set_t), );
-
-   pthread_create(,  /* Bind 'pong' to CPU 0 */, pong, NULL);
-   pthread_setname_np(t1, "pong"); /* Name it for systemtap convenience */
+   rc = pthread_attr_setaffinity_np(, sizeof(cpu_set_t), );
+   if (rc)
+   pr_err(rc, "pthread_attr_setaffinity_np()");
+
+   rc = pthread_create(,  /* Bind 'pong' to CPU 0 */, pong, NULL);
+   if (rc)
+   pr_err(rc, "pthread_create()");
+
+   /* Name it for systemtap convenience */
+   rc = pthread_setname_np(t1, "pong");
+   if (rc)
+   pr_warn(rc, "pthread_create()");
 
flags.result = 0;
 
-- 
2.15.0

Re: [PATCH v2] powerpc/powernv: Add pci_reset_phbs parameter to issue a PHB reset

2017-11-20 Thread Balbir Singh 
On Thu, Nov 16, 2017 at 11:14 PM, Guilherme G. Piccoli
 wrote:
> On 11/16/2017 01:49 AM, Balbir Singh wrote:
>> On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli
>>  wrote:
>>> During a kdump kernel boot in PowerPC, we request a reset of the PHBs
>>> to the FW. It makes sense, since if we are booting a kdump kernel it
>>> means we had some trouble before and we cannot rely in the adapters'
>>> health; they could be in a bad state, hence the reset is needed.
>>>
>>> But this reset is useful not only in kdump - there are situations,
>>> specially when debugging drivers, that we could break an adapter in
>>> a way it requires such reset. One can tell to just go ahead and
>>> reboot the machine, but happens that many times doing kexec is much
>>> faster, and so preferable than a full power cycle.
>>>
>>> This patch adds the pci_reset_phbs parameter to perform such reset
>>> when desired by the user.
>>>
>>
>> Do we care to reset specific phbs or all of them? I guess all based on
>> your description.
>
> Exactly Balbir, it does reset all of them. We could add such
> granularity, but I don't see much usability..
> But if somebody feels it's useful, we can change...
>

OK.. makes sense, any reason why this can't be folded into reset_devices?
I guess we want reset_phbs to be independent of reset_devices

Balbir

Re: STRICT_KERNEL_RWX on PPC32 is broken on PowerMac G4

2017-11-20 Thread Balbir Singh 
On Sun, Nov 19, 2017 at 1:36 AM, LEROY Christophe
 wrote:
> Meelis Roos  a écrit :
>
>>> > > How early does it hang ? Any oops or trace ?
>>> >
>>> > Very early - instead oif kernel emssages, I see some repeated gibberish
>>> > of some characteers, and the background turns white.
>>> > I am booting from yaboot, background is normally black.
>>>
>>> Ok, could you try by replacing #ifdef CONFIG_STRICT_KERNEL_RWX by #if 0
>>> in arch/powerpc/lib/code-patching.c
>>
>>
>> With this change and CONFIG_STRICT_KERNEL_RWX=y, it still boots.
>>
>> BTW, I get these warnings (sorry for the word wrap from screen paste) -
>> may they be related or rather not?
>>
>>   WRAParch/powerpc/boot/zImage.pmac
>>   WRAParch/powerpc/boot/zImage.coff
>>   WRAParch/powerpc/boot/zImage.miboot
>> INFO: Uncompressed kernel (size 0x5d4c3c) overlaps the address of the
>> wrapper(0x40)
>> INFO: Fixing the link_address of wrapper to (0x60)
>>
>
> Then i believe there is something wrong with commit
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20171115=37bc3e5fd764fb258ff4fcbb90b6d1b67fb466c1
>
> Balbir, do you have any idea ?
>

Hmm.. interesting, so nobats works, but code-patching has issues? Any
chance you can boot with xmon=on on the command line
when you drop into the xmon> prompt, type dl to get the kernel log.

Balbir Singh.

Re: [alsa-devel] [PATCH 2/2] ASoC: fsl_ssi: serialize AC'97 register access operations

2017-11-20 Thread Nicolin Chen 
On Mon, Nov 20, 2017 at 11:16:07PM +0100, Maciej S. Szmigiero wrote:
> AC'97 register access operations (both read and write) on SSI use a one,
> shared set of SSI registers for AC'97 register address and data.
> This means that only one such access is possible at a time and so all these
> operations need to be serialized.
> 
> Since an AC'97 register access operation in this driver takes 100us+ let's
> use a mutex for this.
> 
> Use this opportunity to also change a default value returned from AC'97
> register read function from -1 to 0, since that's what AC'97 specs require
> to be returned when unknown / undefined registers are read.
> 
> Signed-off-by: Maciej S. Szmigiero 

>  static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
> @@ -1287,16 +1295,18 @@ static unsigned short fsl_ssi_ac97_read(struct 
> snd_ac97 *ac97,
>  {
>   struct regmap *regs = fsl_ac97_data->regs;
>  
> - unsigned short val = -1;
> + unsigned short val = 0;
>   u32 reg_val;
>   unsigned int lreg;
>   int ret;
>  
> + mutex_lock(_ac97_data->ac97_reg_lock);
> +
>   ret = clk_prepare_enable(fsl_ac97_data->clk);
>   if (ret) {
>   pr_err("ac97 read clk_prepare_enable failed: %d\n",
>   ret);
> - return -1;
> + goto ret_unlock;

It will return val (== 0) in this case. Will this be correctly
handled by callers? I find sound/ac97/bus.c checks if ret < 0
for ops->read().

So it might be better to add "val = ret;" before goto? Or use
val instead of ret directly?

>   }
>  
>   lreg = (reg & 0x7f) <<  12;
> @@ -1311,6 +1321,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
> *ac97,
>  
>   clk_disable_unprepare(fsl_ac97_data->clk);
>  
> +ret_unlock:
> + mutex_unlock(_ac97_data->ac97_reg_lock);
>   return val;
>  }

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

2017-11-20 Thread Nicolin Chen 
On Tue, Nov 21, 2017 at 01:32:09AM +0100, Maciej S. Szmigiero wrote:
> On 21.11.2017 01:00, Nicolin Chen wrote:
> > On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
> (..)
> >> We need to make sure, however, that only proper channel slots are enabled
> >> at playback start time since some AC'97 CODECs (like VT1613) were observed
> >> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
> >> an AC'97 link is started but before the CODEC is configured by its driver.
> > 
> > I don't really understand this part. Why do we need to *make sure*
> > and set SACCDIS and SACCEN again since they're initialized already> 
> > Could you please elaborate a bit more?
> 
> SACCDIS and SACCEN aren't just normal registers.
> They are a way to disable or enable bits in SACCST register - writing
> a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
> writing a '1' bit at some position in SACCEN sets the relevant bit in
> SACCST.
> 
> But a bit in SACCST can also be set (and so an AC'97 channel slot
> enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
> enough if this bit was set in SLOTREQ just once, bits in SACCST are
> 'sticky').

This makes sense now. It could be mentioned a bit in the commit log
as well.

> If an extra slot gets enabled that's a disaster for playback because some
> of normal left or right channel samples are instead redirected to this
> extra slot.
> 
> Unfortunately, the VT1613 codec on UDOO board (which is the only user of
> fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
> set extra bits in its SLOTREQ request - I've confirmed this on two boards
> bought a few months apart directly from UDOO shop.
> 
> A workaround implemented in fsl-asoc-card of setting an appropriate
> CODEC register so that slots 3/4 (the normal stereo playback slots) are
> used for S/PDIF seems to mostly fix this issue but since this codec is so
> untrustworthy then:

If this move is also a workaround, probably it'd be better to have
an fsl_ssi_ac97_xxx_war() function with some comments/descriptions,
and include it in the config(). Since it doesn't hurt to set these
registers again, I am personally fine with that. But it needs to be
clear -- otherwise, people may try to remove it later with a change
like: Removing redundant SACCEN/SACCDIS settings since they're done
during probe().

Re: [alsa-devel] [PATCH 1/2] ASoC: fsl_ssi: AC'97 ops need regmap, clock and cleaning up on failure

2017-11-20 Thread Maciej S. Szmigiero 
On 21.11.2017 01:32, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:14:55PM +0100, Maciej S. Szmigiero wrote:
(..)
>> @@ -1460,12 +1460,6 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>>  sizeof(fsl_ssi_ac97_dai));
>>  
>>  fsl_ac97_data = ssi_private;
> 
> By the way, is there any better way to register the ops for AC97
> while we could pass the ssi_private so as to remove the global
> fsl_ac97_data?

This might be possible (if SSI private data is provided to AC'97 codec in
codecs/ac97.c in platform device data which then is modified to make use
of it), but currently ASoC AC'97 only supports one controller per system
so for a real gain this limitation would have to be addressed first.

Maciej

Re: [alsa-devel] [PATCH 1/2] ASoC: fsl_ssi: AC'97 ops need regmap, clock and cleaning up on failure

2017-11-20 Thread Nicolin Chen 
On Mon, Nov 20, 2017 at 11:14:55PM +0100, Maciej S. Szmigiero wrote:
> AC'97 ops (register read / write) need SSI regmap and clock, so they have
> to be set after them.
> 
> We also need to set these ops back to NULL if we fail the probe.
> 
> Signed-off-by: Maciej S. Szmigiero 

Acked-by: Nicolin Chen 

> ---
>  sound/soc/fsl/fsl_ssi.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index dad80b4b0cfc..a71bb8391f61 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1460,12 +1460,6 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>   sizeof(fsl_ssi_ac97_dai));
>  
>   fsl_ac97_data = ssi_private;

By the way, is there any better way to register the ops for AC97
while we could pass the ssi_private so as to remove the global
fsl_ac97_data?

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

2017-11-20 Thread Maciej S. Szmigiero 
On 21.11.2017 01:00, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
(..)
>> We need to make sure, however, that only proper channel slots are enabled
>> at playback start time since some AC'97 CODECs (like VT1613) were observed
>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
>> an AC'97 link is started but before the CODEC is configured by its driver.
> 
> I don't really understand this part. Why do we need to *make sure*
> and set SACCDIS and SACCEN again since they're initialized already> 
> Could you please elaborate a bit more?

SACCDIS and SACCEN aren't just normal registers.
They are a way to disable or enable bits in SACCST register - writing
a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
writing a '1' bit at some position in SACCEN sets the relevant bit in
SACCST.

But a bit in SACCST can also be set (and so an AC'97 channel slot
enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
enough if this bit was set in SLOTREQ just once, bits in SACCST are
'sticky').
If an extra slot gets enabled that's a disaster for playback because some
of normal left or right channel samples are instead redirected to this
extra slot.

Unfortunately, the VT1613 codec on UDOO board (which is the only user of
fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
set extra bits in its SLOTREQ request - I've confirmed this on two boards
bought a few months apart directly from UDOO shop.

A workaround implemented in fsl-asoc-card of setting an appropriate
CODEC register so that slots 3/4 (the normal stereo playback slots) are
used for S/PDIF seems to mostly fix this issue but since this codec is so
untrustworthy then:
>> let's play safe here and make sure that no extra slots are enabled
>> every time a playback is started.

(..)
>> @@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream 
>> *substream, int cmd,
>>  case SNDRV_PCM_TRIGGER_START:
>>  case SNDRV_PCM_TRIGGER_RESUME:
>>  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> -if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
>> +if (fsl_ssi_is_ac97(ssi_private) &&
>> +!ssi_private->soc->imx21regs) {
>> +regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
>> +regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
>> +}
> 
> And second, why could we ignore them for STREAM_CAPTURE here?

Capture is not affected by SACCST register content.

> Well, at least this part could be moved into fsl_ssi_tx_config()
> since we have an abstraction layer of register configurations.

Will move this into fsl_ssi_tx_config() in a respin.

> 
> Thanks
> Nic

Thanks,
Maciej

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

2017-11-20 Thread Nicolin Chen 
On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
> In AC'97 mode we configure and start SSI RX / TX on probe path via
> a call to _fsl_ssi_set_dai_fmt() function.
> We don't need to call this function again later and in fact don't want to
> do it since this function temporarily sets STCR, SRCR and SCR to some
> intermediate values.
> 
> We need to make sure, however, that only proper channel slots are enabled
> at playback start time since some AC'97 CODECs (like VT1613) were observed
> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
> an AC'97 link is started but before the CODEC is configured by its driver.

I don't really understand this part. Why do we need to *make sure*
and set SACCDIS and SACCEN again since they're initialized already?

Could you please elaborate a bit more?

> Theoretically, this should be necessary only for the very first playback
> but let's play safe here and make sure that no extra slots are enabled
> every time a playback is started.
> 
> Signed-off-by: Maciej S. Szmigiero 
> ---
>  sound/soc/fsl/fsl_ssi.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 48bb850a34d9..dad80b4b0cfc 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -630,12 +630,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
> *ssi_private)
>   regmap_write(regs, CCSR_SSI_SACNT,
>   CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
>  
> - /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
> - if (!ssi_private->soc->imx21regs) {
> - regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
> - regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
> - }

> @@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream 
> *substream, int cmd,
>   case SNDRV_PCM_TRIGGER_START:
>   case SNDRV_PCM_TRIGGER_RESUME:
>   case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
> + if (fsl_ssi_is_ac97(ssi_private) &&
> + !ssi_private->soc->imx21regs) {
> + regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
> + regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
> + }

And second, why could we ignore them for STREAM_CAPTURE here?

Well, at least this part could be moved into fsl_ssi_tx_config()
since we have an abstraction layer of register configurations.

Thanks
Nic

> +
>   fsl_ssi_tx_config(ssi_private, true);
> - else
> + } else
>   fsl_ssi_rx_config(ssi_private, true);
>   break;
>  
> 
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: remove duplicated flag setting in fsl_ssi_setup_reg_vals()

2017-11-20 Thread Nicolin Chen 
On Mon, Nov 20, 2017 at 11:12:01PM +0100, Maciej S. Szmigiero wrote:
> We don't need to set CCSR_SSI_SIER_RFF0_EN / CCSR_SSI_SIER_TFE0_EN bits
> in reg->rx.sier / reg->tx.sier variables in a non-AC'97 mode considering we
> had just initialized these variables to these very values unconditionally a
> few lines earlier.
> 
> Signed-off-by: Maciej S. Szmigiero 

Acked-by: Nicolin Chen 

> ---
>  sound/soc/fsl/fsl_ssi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index f2f51e06e22c..48bb850a34d9 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -597,9 +597,7 @@ static void fsl_ssi_setup_reg_vals(struct fsl_ssi_private 
> *ssi_private)
>  
>   if (!fsl_ssi_is_ac97(ssi_private)) {
>   reg->rx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE;
> - reg->rx.sier |= CCSR_SSI_SIER_RFF0_EN;
>   reg->tx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE;
> - reg->tx.sier |= CCSR_SSI_SIER_TFE0_EN;
>   }
>  
>   if (ssi_private->use_dma) {
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

2017-11-20 Thread Maciej S. Szmigiero 
In AC'97 mode we configure and start SSI RX / TX on probe path via
a call to _fsl_ssi_set_dai_fmt() function.
We don't need to call this function again later and in fact don't want to
do it since this function temporarily sets STCR, SRCR and SCR to some
intermediate values.

We need to make sure, however, that only proper channel slots are enabled
at playback start time since some AC'97 CODECs (like VT1613) were observed
requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
an AC'97 link is started but before the CODEC is configured by its driver.
Theoretically, this should be necessary only for the very first playback
but let's play safe here and make sure that no extra slots are enabled
every time a playback is started.

Signed-off-by: Maciej S. Szmigiero 
---
 sound/soc/fsl/fsl_ssi.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 48bb850a34d9..dad80b4b0cfc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -630,12 +630,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
 
-   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
-   if (!ssi_private->soc->imx21regs) {
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
-   }
-
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
 * codec before a stream is started.
@@ -1076,6 +1070,9 @@ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai 
*cpu_dai, unsigned int fmt)
 {
struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
 
+   if (fsl_ssi_is_ac97(ssi_private))
+   return 0;
+
return _fsl_ssi_set_dai_fmt(cpu_dai->dev, ssi_private, fmt);
 }
 
@@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream 
*substream, int cmd,
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+   if (fsl_ssi_is_ac97(ssi_private) &&
+   !ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
+
fsl_ssi_tx_config(ssi_private, true);
-   else
+   } else
fsl_ssi_rx_config(ssi_private, true);
break;

[PATCH] ASoC: fsl_ssi: remove duplicated flag setting in fsl_ssi_setup_reg_vals()

2017-11-20 Thread Maciej S. Szmigiero 
We don't need to set CCSR_SSI_SIER_RFF0_EN / CCSR_SSI_SIER_TFE0_EN bits
in reg->rx.sier / reg->tx.sier variables in a non-AC'97 mode considering we
had just initialized these variables to these very values unconditionally a
few lines earlier.

Signed-off-by: Maciej S. Szmigiero 
---
 sound/soc/fsl/fsl_ssi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index f2f51e06e22c..48bb850a34d9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -597,9 +597,7 @@ static void fsl_ssi_setup_reg_vals(struct fsl_ssi_private 
*ssi_private)
 
if (!fsl_ssi_is_ac97(ssi_private)) {
reg->rx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE;
-   reg->rx.sier |= CCSR_SSI_SIER_RFF0_EN;
reg->tx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE;
-   reg->tx.sier |= CCSR_SSI_SIER_TFE0_EN;
}
 
if (ssi_private->use_dma) {

[PATCH 1/2] ASoC: fsl_ssi: AC'97 ops need regmap, clock and cleaning up on failure

2017-11-20 Thread Maciej S. Szmigiero 
AC'97 ops (register read / write) need SSI regmap and clock, so they have
to be set after them.

We also need to set these ops back to NULL if we fail the probe.

Signed-off-by: Maciej S. Szmigiero 
---
 sound/soc/fsl/fsl_ssi.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index dad80b4b0cfc..a71bb8391f61 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1460,12 +1460,6 @@ static int fsl_ssi_probe(struct platform_device *pdev)
sizeof(fsl_ssi_ac97_dai));
 
fsl_ac97_data = ssi_private;
-
-   ret = snd_soc_set_ac97_ops_of_reset(_ssi_ac97_ops, pdev);
-   if (ret) {
-   dev_err(>dev, "could not set AC'97 ops\n");
-   return ret;
-   }
} else {
/* Initialize this copy of the CPU DAI driver structure */
memcpy(_private->cpu_dai_drv, _ssi_dai_template,
@@ -1576,6 +1570,14 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return ret;
}
 
+   if (fsl_ssi_is_ac97(ssi_private)) {
+   ret = snd_soc_set_ac97_ops_of_reset(_ssi_ac97_ops, pdev);
+   if (ret) {
+   dev_err(>dev, "could not set AC'97 ops\n");
+   goto error_ac97_ops;
+   }
+   }
+
ret = devm_snd_soc_register_component(>dev, _ssi_component,
  _private->cpu_dai_drv, 1);
if (ret) {
@@ -1659,6 +1661,10 @@ static int fsl_ssi_probe(struct platform_device *pdev)
fsl_ssi_debugfs_remove(_private->dbg_stats);
 
 error_asoc_register:
+   if (fsl_ssi_is_ac97(ssi_private))
+   snd_soc_set_ac97_ops(NULL);
+
+error_ac97_ops:
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);

[PATCH 2/2] ASoC: fsl_ssi: serialize AC'97 register access operations

2017-11-20 Thread Maciej S. Szmigiero 
AC'97 register access operations (both read and write) on SSI use a one,
shared set of SSI registers for AC'97 register address and data.
This means that only one such access is possible at a time and so all these
operations need to be serialized.

Since an AC'97 register access operation in this driver takes 100us+ let's
use a mutex for this.

Use this opportunity to also change a default value returned from AC'97
register read function from -1 to 0, since that's what AC'97 specs require
to be returned when unknown / undefined registers are read.

Signed-off-by: Maciej S. Szmigiero 
---
 sound/soc/fsl/fsl_ssi.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index a71bb8391f61..9dea1b16de82 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -38,6 +38,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -265,6 +266,8 @@ struct fsl_ssi_private {
 
u32 fifo_watermark;
u32 dma_maxburst;
+
+   struct mutex ac97_reg_lock;
 };
 
 /*
@@ -1262,11 +1265,13 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
if (reg > 0x7f)
return;
 
+   mutex_lock(_ac97_data->ac97_reg_lock);
+
ret = clk_prepare_enable(fsl_ac97_data->clk);
if (ret) {
pr_err("ac97 write clk_prepare_enable failed: %d\n",
ret);
-   return;
+   goto ret_unlock;
}
 
lreg = reg <<  12;
@@ -1280,6 +1285,9 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
udelay(100);
 
clk_disable_unprepare(fsl_ac97_data->clk);
+
+ret_unlock:
+   mutex_unlock(_ac97_data->ac97_reg_lock);
 }
 
 static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
@@ -1287,16 +1295,18 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
 {
struct regmap *regs = fsl_ac97_data->regs;
 
-   unsigned short val = -1;
+   unsigned short val = 0;
u32 reg_val;
unsigned int lreg;
int ret;
 
+   mutex_lock(_ac97_data->ac97_reg_lock);
+
ret = clk_prepare_enable(fsl_ac97_data->clk);
if (ret) {
pr_err("ac97 read clk_prepare_enable failed: %d\n",
ret);
-   return -1;
+   goto ret_unlock;
}
 
lreg = (reg & 0x7f) <<  12;
@@ -1311,6 +1321,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
 
clk_disable_unprepare(fsl_ac97_data->clk);
 
+ret_unlock:
+   mutex_unlock(_ac97_data->ac97_reg_lock);
return val;
 }
 
@@ -1571,6 +1583,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
}
 
if (fsl_ssi_is_ac97(ssi_private)) {
+   mutex_init(_private->ac97_reg_lock);
ret = snd_soc_set_ac97_ops_of_reset(_ssi_ac97_ops, pdev);
if (ret) {
dev_err(>dev, "could not set AC'97 ops\n");
@@ -1665,6 +1678,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
snd_soc_set_ac97_ops(NULL);
 
 error_ac97_ops:
+   if (fsl_ssi_is_ac97(ssi_private))
+   mutex_destroy(_private->ac97_reg_lock);
+
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
@@ -1683,8 +1699,10 @@ static int fsl_ssi_remove(struct platform_device *pdev)
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
-   if (fsl_ssi_is_ac97(ssi_private))
+   if (fsl_ssi_is_ac97(ssi_private)) {
snd_soc_set_ac97_ops(NULL);
+   mutex_destroy(_private->ac97_reg_lock);
+   }
 
return 0;
 }

[PATCH] powerpc/vas, export chip_to_vas_id()

2017-11-20 Thread Sukadev Bhattiprolu 
>From 958f8db089f4b89407fc4b89bccd3eaef585aa96 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu 
Date: Mon, 20 Nov 2017 12:53:15 -0600
Subject: [PATCH 1/1] powerpc/vas, export chip_to_vas_id()

Export the symbol chip_to_vas_id() to fix a build failure when
CONFIG_CRYPTO_DEV_NX_COMPRESS_POWERNV=m.

Reported-by: Haren Myneni 
Reported-by: Josh Boyer 
Signed-off-by: Sukadev Bhattiprolu 
---

This was broken by the patch https://lkml.org/lkml/2017/11/7/915.

---
 arch/powerpc/platforms/powernv/vas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/platforms/powernv/vas.c 
b/arch/powerpc/platforms/powernv/vas.c
index c488621..aebbe95 100644
--- a/arch/powerpc/platforms/powernv/vas.c
+++ b/arch/powerpc/platforms/powernv/vas.c
@@ -135,6 +135,7 @@ int chip_to_vas_id(int chipid)
}
return -1;
 }
+EXPORT_SYMBOL(chip_to_vas_id);
 
 static int vas_probe(struct platform_device *pdev)
 {
-- 
2.7.4

Re: [PATCH v3 10/18] powerpc/vas, nx-842: Define and use chip_to_vas_id()

2017-11-20 Thread Josh Boyer 
On Tue, Nov 7, 2017 at 9:23 PM, Sukadev Bhattiprolu
 wrote:
> Define a helper, chip_to_vas_id() to map a given chip id to corresponding
> vas id.
>
> Normally, callers of vas_rx_win_open() and vas_tx_win_open() want the VAS
> window to be on the same chip where the calling thread is executing. These
> callers can pass in -1 for the VAS id.
>
> This interface will be useful if a thread running on one chip wants to open
> a window on another chip (like the NX-842 driver does during start up).
>
> Signed-off-by: Sukadev Bhattiprolu 

The Fedora kernel team seems to have hit an issue with this commit
when trying to build a rawhide kernel that includes it.  The failure
is:

ERROR: "chip_to_vas_id" [drivers/crypto/nx/nx-compress-powernv.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1
make: *** [Makefile:1218: modules] Error 2
+ exit 1
error: Bad exit status from /var/tmp/rpm-tmp.dmbw5D (%build)
RPM build errors:
Bad exit status from /var/tmp/rpm-tmp.dmbw5D (%build)
Child return code was: 1
EXCEPTION: [Error()]

> ---
>  arch/powerpc/include/asm/vas.h   |  9 +
>  arch/powerpc/platforms/powernv/vas.c | 11 +++
>  drivers/crypto/nx/nx-842-powernv.c   | 18 +++---
>  3 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h
> index fd5963a..044748f 100644
> --- a/arch/powerpc/include/asm/vas.h
> +++ b/arch/powerpc/include/asm/vas.h
> @@ -104,6 +104,15 @@ struct vas_tx_win_attr {
>  };
>
>  /*
> + * Helper to map a chip id to VAS id.
> + * For POWER9, this is a 1:1 mapping. In the future this maybe a 1:N
> + * mapping in which case, we will need to update this helper.
> + *
> + * Return the VAS id or -1 if no matching vasid is found.
> + */
> +int chip_to_vas_id(int chipid);
> +
> +/*
>   * Helper to initialize receive window attributes to defaults for an
>   * NX window.
>   */
> diff --git a/arch/powerpc/platforms/powernv/vas.c 
> b/arch/powerpc/platforms/powernv/vas.c
> index abb7090..cd9a733 100644
> --- a/arch/powerpc/platforms/powernv/vas.c
> +++ b/arch/powerpc/platforms/powernv/vas.c
> @@ -123,6 +123,17 @@ struct vas_instance *find_vas_instance(int vasid)
> return NULL;
>  }
>
> +int chip_to_vas_id(int chipid)
> +{
> +   int cpu;
> +
> +   for_each_possible_cpu(cpu) {
> +   if (cpu_to_chip_id(cpu) == chipid)
> +   return per_cpu(cpu_vas_id, cpu);
> +   }
> +   return -1;
> +}
> +

Likely need an EXPORT_SYMBOL here?

>  static int vas_probe(struct platform_device *pdev)
>  {
> return init_vas_instance(pdev);
> diff --git a/drivers/crypto/nx/nx-842-powernv.c 
> b/drivers/crypto/nx/nx-842-powernv.c
> index 874ddf5..eb221ed 100644
> --- a/drivers/crypto/nx/nx-842-powernv.c
> +++ b/drivers/crypto/nx/nx-842-powernv.c

Did anyone test this driver built as a module with this commit included?

josh

> @@ -847,24 +847,12 @@ static int __init nx842_powernv_probe_vas(struct 
> device_node *pn)
> return -EINVAL;
> }
>
> -   for_each_compatible_node(dn, NULL, "ibm,power9-vas-x") {
> -   if (of_get_ibm_chip_id(dn) == chip_id)
> -   break;
> -   }
> -
> -   if (!dn) {
> -   pr_err("Missing VAS device node\n");
> +   vasid = chip_to_vas_id(chip_id);
> +   if (vasid < 0) {
> +   pr_err("Unable to map chip_id %d to vasid\n", chip_id);
> return -EINVAL;
> }
>
> -   if (of_property_read_u32(dn, "ibm,vas-id", )) {
> -   pr_err("Missing ibm,vas-id device property\n");
> -   of_node_put(dn);
> -   return -EINVAL;
> -   }
> -
> -   of_node_put(dn);
> -
> for_each_child_of_node(pn, dn) {
> if (of_device_is_compatible(dn, "ibm,p9-nx-842")) {
> ret = vas_cfg_coproc_info(dn, chip_id, vasid);
> --
> 2.7.4
>

Re: [PATCH V7 3/3] hotplug/cpu: Fix crash with memoryless nodes

2017-11-20 Thread Nathan Fontenot 
On 11/16/2017 11:28 AM, Michael Bringmann wrote:
> On powerpc systems with shared configurations of CPUs and memory and
> memoryless nodes at boot, an event ordering problem was observed on
> a SLES12 build platforms with the hot-add of CPUs to the memoryless
> nodes.
> 
> * The most common error occurred when the memory SLAB driver attempted
>   to reference the memoryless node to which a CPU was being added
>   before the kernel had finished initializing all of the data structures
>   for the CPU and exited 'device_online' under DLPAR/hot-add.
> 
>   Normally the memoryless node would be initialized through the call
>   path device_online ... arch_update_cpu_topology ... find_cpu_nid
>   ...  try_online_node.  This patch ensures that the powerpc node will
>   be initialized as early as possible, even if it was memoryless and
>   CPU-less at the point when we are trying to hot-add a new CPU to it.
> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V7:
>   -- Make function find_cpu_nid() externally visible/usable so that
>  it may be used from hotplug-cpu.c
> ---
>  arch/powerpc/mm/numa.c   |3 ++-
>  arch/powerpc/platforms/pseries/hotplug-cpu.c |3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 163f4cc..d6d4f7c 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1310,7 +1310,7 @@ static long vphn_get_associativity(unsigned long cpu,
>   return rc;
>  }
> 
> -static inline int find_cpu_nid(int cpu)
> +int find_cpu_nid(int cpu)
>  {
>   __be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
>   int new_nid;
> @@ -1343,6 +1343,7 @@ static inline int find_cpu_nid(int cpu)
>  #endif
>   }
> 
> + printk(KERN_INFO "%s:%d cpu %d nid %d\n", __FUNCTION__, __LINE__, cpu, 
> new_nid);

This seems like a more likely pr_debug statement.

>   return new_nid;
>  }
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c 
> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index a7d14aa7..df8c732 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -340,6 +340,8 @@ static void pseries_remove_processor(struct device_node 
> *np)
>   cpu_maps_update_done();
>  }
> 
> +extern int find_cpu_nid(int cpu);
> +
>  static int dlpar_online_cpu(struct device_node *dn)
>  {
>   int rc = 0;
> @@ -364,6 +366,7 @@ static int dlpar_online_cpu(struct device_node *dn)
>   != CPU_STATE_OFFLINE);
>   cpu_maps_update_done();
>   timed_topology_update(1);
> + find_cpu_nid(cpu);

We don't use the returned node from this call, so I'm not sure why it gets
called. Perhaps its the possible call to try_online_node() that may get called
in find_cpu_nid(), if so perhpas naming the routine something slightly
different would be good, like find_and_online_cpu_nid?

-Nathan
>   rc = device_online(get_cpu_device(cpu));
>   if (rc)
>   goto out;
>

Re: RESEND [PATCH V7 2/3] poserpc/initnodes: Ensure nodes initialized for hotplug

2017-11-20 Thread Nathan Fontenot 
On 11/16/2017 11:27 AM, Michael Bringmann wrote:
> On powerpc systems which allow 'hot-add' of CPU, it may occur that
> the new resources are to be inserted into nodes that were not used
> for memory resources at bootup.  Many different configurations of
> PowerPC resources may need to be supported depending upon the
> environment.  Important characteristics of the nodes and operating
> environment include:
> 
> * Dedicated vs. shared resources.  Shared resources require

this should be shared CPUs require...since shared CPUs have their
affinity set to node 0 at boot and when hot-added.

>   information such as the VPHN hcall for CPU assignment to nodes.
>   Associativity decisions made based on dedicated resource rules,
>   such as associativity properties in the device tree, may vary
>   from decisions made using the values returned by the VPHN hcall.
> * memoryless nodes at boot.  Nodes need to be defined as 'possible'
>   at boot for operation with other code modules.  Previously, the
>   powerpc code would limit the set of possible nodes to those which
>   have memory assigned at boot, and were thus online.  Subsequent
>   add/remove of CPUs or memory would only work with this subset of
>   possible nodes.
> * memoryless nodes with CPUs at boot.  Due to the previous restriction
>   on nodes, nodes that had CPUs but no memory were being collapsed
>   into other nodes that did have memory at boot.  In practice this
>   meant that the node assignment presented by the runtime kernel
>   differed from the affinity and associativity attributes presented
>   by the device tree or VPHN hcalls.  Nodes that might be known to
>   the pHyp were not 'possible' in the runtime kernel because they did
>   not have memory at boot.
> 
> This patch fixes some problems encountered at runtime with
> configurations that support memory-less nodes, or that hot-add CPUs
> into nodes that are memoryless during system execution after boot.
> The problems of interest include,
> 
> * Nodes known to powerpc to be memoryless at boot, but to have
>   CPUs in them are allowed to be 'possible' and 'online'.  Memory
>   allocations for those nodes are taken from another node that does
>   have memory until and if memory is hot-added to the node.
> * Nodes which have no resources assigned at boot, but which may still
>   be referenced subsequently by affinity or associativity attributes,
>   are kept in the list of 'possible' nodes for powerpc.  Hot-add of
>   memory or CPUs to the system can reference these nodes and bring
>   them online instead of redirecting the references to one of the set
>   of nodes known to have memory at boot.
> 
> Note that this software operates under the context of CPU hotplug.
> We are not doing memory hotplug in this code, but rather updating
> the kernel's CPU topology (i.e. arch_update_cpu_topology /
> numa_update_cpu_topology).  We are initializing a node that may be
> used by CPUs or memory before it can be referenced as invalid by a
> CPU hotplug operation.  CPU hotplug operations are protected by a
> range of APIs including cpu_maps_update_begin/cpu_maps_update_done,
> cpus_read/write_lock / cpus_read/write_unlock, device locks, and more.
> Memory hotplug operations, including try_online_node, are protected
> by mem_hotplug_begin/mem_hotplug_done, device locks, and more.  In
> the case of CPUs being hot-added to a previously memoryless node, the
> try_online_node operation occurs wholly within the CPU locks with no
> overlap.  Using HMC hot-add/hot-remove operations, we have been able
> to add and remove CPUs to any possible node without failures.  HMC
> operations involve a degree self-serialization, though.

This may be able to be stated as simply saying that cpu hotplug operations
are serialized with the device_hotplug_lock.

> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V6:
>   -- Add some needed node initialization to runtime code that maps
>  CPUs based on VPHN associativity
>   -- Add error checks and alternate recovery for compile flag
>  CONFIG_MEMORY_HOTPLUG
>   -- Add alternate node selection recovery for !CONFIG_MEMORY_HOTPLUG
>   -- Add more information to the patch introductory text
> ---
>  arch/powerpc/mm/numa.c |   51 
> ++--
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 334a1ff..163f4cc 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -551,7 +551,7 @@ static int numa_setup_cpu(unsigned long lcpu)
>   nid = of_node_to_nid_single(cpu);
> 
>  out_present:
> - if (nid < 0 || !node_online(nid))
> + if (nid < 0 || !node_possible(nid))
>   nid = first_online_node;
> 
>   map_cpu_to_node(lcpu, nid);
> @@ -867,7 +867,7 @@ void __init dump_numa_cpu_topology(void)
>  }
> 
>  /* Initialize NODE_DATA for a node on the local memory */
> -static void __init setup_node_data(int

Re: [PATCH V7 1/3] powerpc/nodes: Ensure enough nodes avail for operations

2017-11-20 Thread Nathan Fontenot 


On 11/16/2017 11:24 AM, Michael Bringmann wrote:
> On powerpc systems which allow 'hot-add' of CPU or memory resources,
> it may occur that the new resources are to be inserted into nodes
> that were not used for these resources at bootup.  In the kernel,
> any node that is used must be defined and initialized.  These empty
> nodes may occur when,
> 
> * Dedicated vs. shared resources.  Shared resources require
>   information such as the VPHN hcall for CPU assignment to nodes.
>   Associativity decisions made based on dedicated resource rules,
>   such as associativity properties in the device tree, may vary
>   from decisions made using the values returned by the VPHN hcall.
> * memoryless nodes at boot.  Nodes need to be defined as 'possible'
>   at boot for operation with other code modules.  Previously, the
>   powerpc code would limit the set of possible nodes to those which
>   have memory assigned at boot, and were thus online.  Subsequent
>   add/remove of CPUs or memory would only work with this subset of
>   possible nodes.
> * memoryless nodes with CPUs at boot.  Due to the previous restriction
>   on nodes, nodes that had CPUs but no memory were being collapsed
>   into other nodes that did have memory at boot.  In practice this
>   meant that the node assignment presented by the runtime kernel
>   differed from the affinity and associativity attributes presented
>   by the device tree or VPHN hcalls.  Nodes that might be known to
>   the pHyp were not 'possible' in the runtime kernel because they did
>   not have memory at boot.
> 
> This patch ensures that sufficient nodes are defined to support
> configuration requirements after boot, as well as at boot.  This
> patch set fixes a couple of problems.
> 
> * Nodes known to powerpc to be memoryless at boot, but to have
>   CPUs in them are allowed to be 'possible' and 'online'.  Memory
>   allocations for those nodes are taken from another node that does
>   have memory until and if memory is hot-added to the node.
> * Nodes which have no resources assigned at boot, but which may still
>   be referenced subsequently by affinity or associativity attributes,
>   are kept in the list of 'possible' nodes for powerpc.  Hot-add of
>   memory or CPUs to the system can reference these nodes and bring
>   them online instead of redirecting to one of the set of nodes that
>   were known to have memory at boot.
> 
> This patch extracts the value of the lowest domain level (number of
> allocable resources) from the device tree property
> "ibm,max-associativity-domains" to use as the maximum number of nodes
> to setup as possibly available in the system.  This new setting will
> override the instruction,
> 
> nodes_and(node_possible_map, node_possible_map, node_online_map);
> 
> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().
> 
> If the "ibm,max-associativity-domains" property is not present at boot,
> no operation will be performed to define or enable additional nodes, or
> enable the above 'nodes_and()'.
> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V6:
>   -- Remove some node initialization/allocation from boot setup
>  to later in runtime to try to limit memory needs early on
>   -- Augment descriptive documentation for patch
> ---
>  arch/powerpc/mm/numa.c |   40 +---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index eb604b3..334a1ff 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -892,6 +892,37 @@ static void __init setup_node_data(int nid, u64 
> start_pfn, u64 end_pfn)
>   NODE_DATA(nid)->node_spanned_pages = spanned_pages;
>  }
> 
> +static void __init find_possible_nodes(void)
> +{
> + struct device_node *rtas;
> + u32 numnodes, i;
> +
> + if (min_common_depth <= 0)
> + return;
> +
> + rtas = of_find_node_by_path("/rtas");
> + if (!rtas)
> + return;
> +
> + if (of_property_read_u32_index(rtas,
> + "ibm,max-associativity-domains",
> + min_common_depth, ))
> + goto out;
> +
> + pr_info("numa: Nodes = %d (mcd = %d)\n", numnodes,
> + min_common_depth);

numa.c already has a pr_fmt define, no need to pre-pend "numa:" to the
information message.

-Nathan

> +
> + for (i = 0; i < numnodes; i++) {
> + if (!node_possible(i)) {
> + setup_node_data(i, 0, 0);
> + node_set(i, node_possible_map);
> + }
> + }
> +
> +out:
> + of_node_put(rtas);
> +}
> +
>  void __init initmem_init(void)
>  {
>   int nid, cpu;
> @@ -905,12 +936,15 @@ void __init initmem_init(void)
>   memblock_dump_all();
> 
>   /*
> -  * Reduce the possible NUMA nodes to the online NUMA nodes,
> -  * since we do not support node hotplug. This ensures that  we
> -

Re: [PATCH V2 3/3] postmigration/memory: Associativity & ibm,dynamic-memory-v2

2017-11-20 Thread Nathan Fontenot 
We may want to wait on this patch. I have been working on patches to separate
the LMB information from the device tree property format. Once those patches
are acceptable we can use a common routine for affinity updates.

-Nathan

On 11/16/2017 11:51 AM, Michael Bringmann wrote:
> postmigration/memory: Now apply changes to the associativity of memory
> blocks described by the 'ibm,dynamic-memory-v2' property regarding
> the topology of LPARS in Post Migration events.
> 
> * Extend the previous work done for the 'ibm,associativity-lookup-array'
>   to apply to either property 'ibm,dynamic-memory' or
>   'ibm,dynamic-memory-v2', whichever is present.
> * Add new code to parse the 'ibm,dynamic-memory-v2' property looking
>   for differences in block 'assignment', associativity indexes per
>   block, and any other difference currently known.
> 
> When block differences are recognized, the memory block may be removed,
> added, or updated depending upon the state of the new device tree
> property and differences from the migrated value of the property.
> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V2:
>   -- Remove unnecessary spacing changes from patch.
>   -- Improve patch description.
> ---
>  arch/powerpc/include/asm/prom.h |   12 ++
>  arch/powerpc/platforms/pseries/hotplug-memory.c |  169 
> ++-
>  2 files changed, 172 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index 825bd59..e16ef0f 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -92,6 +92,18 @@ struct of_drconf_cell {
>   u32 flags;
>  };
> 
> +/* The of_drconf_cell_v2 struct defines the layout of the LMB array
> + * specified in the device tree property
> + * ibm,dynamic-reconfiguration-memory/ibm,dynamic-memory-v2
> + */
> +struct of_drconf_cell_v2 {
> + u32 num_seq_lmbs;
> + u64 base_address;
> + u32 drc_index;
> + u32 aa_index;
> + u32 flags;
> +} __attribute__((packed));
> +
>  #define DRCONF_MEM_ASSIGNED  0x0008
>  #define DRCONF_MEM_AI_INVALID0x0040
>  #define DRCONF_MEM_RESERVED  0x0080
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c 
> b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index b37e6ad..bf9687b 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -1171,14 +1171,111 @@ static int pseries_update_drconf_memory(struct 
> of_reconfig_data *pr)
>   return rc;
>  }
> 
> +static inline int pseries_memory_v2_find_drc(u32 drc_index,
> + u64 *base_addr, unsigned long memblock_size,
> + struct of_drconf_cell_v2 **drmem,
> + struct of_drconf_cell_v2 *last_drmem)
> +{
> + struct of_drconf_cell_v2 *dm = (*drmem);
> +
> + while (dm < last_drmem) {
> + if ((be32_to_cpu(dm->drc_index) <= drc_index) &&
> + (drc_index <= (be32_to_cpu(dm->drc_index)+
> + be32_to_cpu(dm->num_seq_lmbs)-1))) {
> + int offset = drc_index - be32_to_cpu(dm->drc_index);
> + (*base_addr) = be64_to_cpu(dm->base_address) +
> + (offset * memblock_size);
> + break;
> + } else if (drc_index > (be32_to_cpu(dm->drc_index)+
> + be32_to_cpu(dm->num_seq_lmbs)-1)) {
> + dm++;
> + (*drmem) = dm;
> + } else if (be32_to_cpu(dm->drc_index) > drc_index) {
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int pseries_update_drconf_memory_v2(struct of_reconfig_data *pr)
> +{
> + struct of_drconf_cell_v2 *new_drmem, *old_drmem, *last_old_drmem;
> + unsigned long memblock_size;
> + u32 new_entries, old_entries;
> + u64 old_base_addr;
> + __be32 *p;
> + int i, rc = 0;
> +
> + if (rtas_hp_event)
> + return 0;
> +
> + memblock_size = pseries_memory_block_size();
> + if (!memblock_size)
> + return -EINVAL;
> +
> + /* The first int of the property is the number of lmb's
> +  * described by the property. This is followed by an array
> +  * of of_drconf_cell_v2 entries. Get the number of entries
> +  * and skip to the array of of_drconf_cell_v2's.
> +  */
> + p = (__be32 *) pr->old_prop->value;
> + if (!p)
> + return -EINVAL;
> + old_entries = be32_to_cpu(*p++);
> + old_drmem = (struct of_drconf_cell_v2 *)p;
> + last_old_drmem = old_drmem +
> + (sizeof(struct of_drconf_cell_v2) * old_entries);
> +
> + p = (__be32 *)pr->prop->value;
> + new_entries = be32_to_cpu(*p++);
> + new_drmem = (struct of_drconf_cell_v2 *)p;
> +
> + for (i = 0; i < new_entries; i++) {

Re: [PATCH V2 1/3] hotplug/mobility: Apply assoc updates for Post Migration Topo

2017-11-20 Thread Nathan Fontenot 
On 11/16/2017 11:50 AM, Michael Bringmann wrote:
> hotplug/mobility: Recognize more changes to the associativity of
> memory blocks described by the 'ibm,dynamic-memory' and 'cpu'
> properties when processing the topology of LPARS in Post Migration
> events.  Previous efforts only recognized whether a memory block's
> assignment had changed in the property.  Changes here include:
> 
> * Checking the aa_index values of the old/new properties and 'readd'
>   any block for which the setting has changed.
> * Checking for changes in cpus and submitting 'readd' ops for them.
> * Creating some common support routines for the submission of memory
>   or cpu 'readd' operations.
> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V2:
>   -- Try to improve patch header documentation.
> ---
>  arch/powerpc/platforms/pseries/hotplug-cpu.c|   64 
> +++
>  arch/powerpc/platforms/pseries/hotplug-memory.c |6 ++
>  arch/powerpc/platforms/pseries/mobility.c   |   47 +
>  arch/powerpc/platforms/pseries/pseries.h|2 +
>  4 files changed, 109 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c 
> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index fadb95e..d127c3a 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -634,6 +634,27 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
>   return rc;
>  }
> 
> +static int dlpar_cpu_readd_by_index(u32 drc_index)
> +{
> + int rc = 0;
> +
> + pr_info("Attempting to update CPU, drc index %x\n", drc_index);
> +
> + if (dlpar_cpu_remove_by_index(drc_index))
> + rc = -EINVAL;
> + else if (dlpar_cpu_add(drc_index))
> + rc = -EINVAL;
> +
> + if (rc)
> + pr_info("Failed to update cpu at drc_index %lx\n",
> + (unsigned long int)drc_index);
> + else
> + pr_info("CPU at drc_index %lx was updated\n",
> + (unsigned long int)drc_index);
> +
> + return rc;
> +}
> +
>  static int find_dlpar_cpus_to_remove(u32 *cpu_drcs, int cpus_to_remove)
>  {
>   struct device_node *dn;
> @@ -824,6 +845,9 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
>   else
>   rc = -EINVAL;
>   break;
> + case PSERIES_HP_ELOG_ACTION_READD:
> + rc = dlpar_cpu_readd_by_index(drc_index);
> + break;
>   default:
>   pr_err("Invalid action (%d) specified\n", hp_elog->action);
>   rc = -EINVAL;
> @@ -874,6 +898,42 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t 
> count)
> 
>  #endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
> 
> +static int pseries_update_drconf_cpu(struct of_reconfig_data *pr)

I think we can drop the 'drconf' piece from this function name.

I'm think you got that from the memory routines that use drconf, which
is really short for dynamic reconfiguration. This was used to state the
routine worked on memory represented in the dynamic-reconfiguration 
properties.

> +{
> + u32 old_entries, new_entries;
> + __be32 *p, *old_assoc, *new_assoc;
> +
> + if (strcmp(pr->dn->type, "cpu"))
> + return 0;
> +
> + /* The first int of the property is the number of domains's
> +  * described.  This is followed by an array of level values.
> +  */
> + p = (__be32 *) pr->old_prop->value;
> + if (!p)
> + return -EINVAL;
> + old_entries = be32_to_cpu(*p++);
> + old_assoc = p;
> +
> + p = (__be32 *)pr->prop->value;
> + if (!p)
> + return -EINVAL;
> + new_entries = be32_to_cpu(*p++);
> + new_assoc = p;
> +
> + if (old_entries == new_entries) {
> + int sz = old_entries * sizeof(int);
> +
> + if (!memcmp(old_assoc, new_assoc, sz))
> + pseries_cpu_readd_by_index(pr->dn->phandle);
> +
> + } else {
> + pseries_cpu_readd_by_index(pr->dn->phandle);
> + }
> +
> + return 0;
> +}
> +
>  static int pseries_smp_notifier(struct notifier_block *nb,
>   unsigned long action, void *data)
>  {
> @@ -887,6 +947,10 @@ static int pseries_smp_notifier(struct notifier_block 
> *nb,
>   case OF_RECONFIG_DETACH_NODE:
>   pseries_remove_processor(rd->dn);
>   break;
> + case OF_RECONFIG_UPDATE_PROPERTY:
> + if (!strcmp(rd->prop->name, "ibm,associativity"))
> + err = pseries_update_drconf_cpu(rd);
> + break;
>   }
>   return notifier_from_errno(err);
>  }
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c 
> b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 1d48ab4..c61cfc6 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -1160,6 +1160,12 @@ static

Re: Subject: [PATCH V4 3/4] hotplug/drc-info: Add code to search ibm,drc-info property

2017-11-20 Thread Nathan Fontenot 
On 11/16/2017 02:11 PM, Michael Bringmann wrote:
> rpadlpar_core.c: Provide parallel routines to search the older device-
> tree properties ("ibm,drc-indexes", "ibm,drc-names", "ibm,drc-types"
> and "ibm,drc-power-domains"), or the new property "ibm,drc-info".
> 
> The interface to examine the DRC information is changed from a "get"
> function that returns values for local verification elsewhere, to a
> "check" function that validates the 'name' and/or 'type' of a device
> node.  This update hides the format of the underlying device-tree
> properties, and concentrates the value checks into a single function
> without requiring the user to verify whether a search was successful.
> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V4:
>   -- Rename of_one_drc_info to of_read_drc_info_cell
>   -- Fix some spacing within arguments
> ---
>  drivers/pci/hotplug/rpadlpar_core.c |   13 ++--
>  drivers/pci/hotplug/rpaphp.h|4 +
>  drivers/pci/hotplug/rpaphp_core.c   |  110 
> +++
>  3 files changed, 92 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/rpadlpar_core.c 
> b/drivers/pci/hotplug/rpadlpar_core.c
> index a3449d7..fc01d7d 100644
> --- a/drivers/pci/hotplug/rpadlpar_core.c
> +++ b/drivers/pci/hotplug/rpadlpar_core.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "../pci.h"
>  #include "rpaphp.h"
> @@ -44,15 +45,14 @@ static struct device_node *find_vio_slot_node(char 
> *drc_name)
>  {
>   struct device_node *parent = of_find_node_by_name(NULL, "vdevice");
>   struct device_node *dn = NULL;
> - char *name;
>   int rc;
> 
>   if (!parent)
>   return NULL;
> 
>   while ((dn = of_get_next_child(parent, dn))) {
> - rc = rpaphp_get_drc_props(dn, NULL, , NULL, NULL);
> - if ((rc == 0) && (!strcmp(drc_name, name)))
> + rc = rpaphp_check_drc_props(dn, drc_name, NULL);
> + if (rc == 0)
>   break;
>   }
> 
> @@ -64,15 +64,12 @@ static struct device_node *find_php_slot_pci_node(char 
> *drc_name,
> char *drc_type)
>  {
>   struct device_node *np = NULL;
> - char *name;
> - char *type;
>   int rc;
> 
>   while ((np = of_find_node_by_name(np, "pci"))) {
> - rc = rpaphp_get_drc_props(np, NULL, , , NULL);
> + rc = rpaphp_check_drc_props(np, drc_name, drc_type);
>   if (rc == 0)
> - if (!strcmp(drc_name, name) && !strcmp(drc_type, type))
> - break;
> + break;
>   }
> 
>   return np;
> diff --git a/drivers/pci/hotplug/rpaphp.h b/drivers/pci/hotplug/rpaphp.h
> index 7db024e..8db5f2e 100644
> --- a/drivers/pci/hotplug/rpaphp.h
> +++ b/drivers/pci/hotplug/rpaphp.h
> @@ -91,8 +91,8 @@ struct slot {
> 
>  /* rpaphp_core.c */
>  int rpaphp_add_slot(struct device_node *dn);
> -int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
> - char **drc_name, char **drc_type, int *drc_power_domain);
> +int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
> + char *drc_type);
> 
>  /* rpaphp_slot.c */
>  void dealloc_slot_struct(struct slot *slot);
> diff --git a/drivers/pci/hotplug/rpaphp_core.c 
> b/drivers/pci/hotplug/rpaphp_core.c
> index 1e29aba..0a3b5f5 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include/* for eeh_add_device() */
>  #include /* rtas_call */
>  #include   /* for pci_controller */
> @@ -196,25 +197,21 @@ static int get_children_props(struct device_node *dn, 
> const int **drc_indexes,
>   return 0;
>  }
> 
> -/* To get the DRC props describing the current node, first obtain it's
> - * my-drc-index property.  Next obtain the DRC list from it's parent.  Use
> - * the my-drc-index for correlation, and obtain the requested properties.
> +
> +/* Verify the existence of 'drc_name' and/or 'drc_type' within the
> + * current node.  First obtain it's my-drc-index property.  Next,
> + * obtain the DRC info from it's parent.  Use the my-drc-index for
> + * correlation, and obtain/validate the requested properties.
>   */
> -int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
> - char **drc_name, char **drc_type, int *drc_power_domain)
> +
> +static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
> + char *drc_type, unsigned int my_index)
>  {
> + char *name_tmp, *type_tmp;
>   const int *indexes, *names;
>   const int *types, *domains;
> - const unsigned int *my_index;
> - char *name_tmp, *type_tmp;
>   int i, rc;
> 
> - my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
> - if (!my_index) {
> -

Re: [PATCH V4 2/4] pseries/drc-info: Search DRC properties for CPU indexes

2017-11-20 Thread Nathan Fontenot 
On 11/16/2017 02:11 PM, Michael Bringmann wrote:
> pseries/drc-info: Provide parallel routines to convert between
> drc_index and CPU numbers at runtime, using the older device-tree
> properties ("ibm,drc-indexes", "ibm,drc-names", "ibm,drc-types"
> and "ibm,drc-power-domains"), or the new property "ibm,drc-info".
> 
> Signed-off-by: Michael Bringmann 
> ---
> Changes in V4:
>   -- Rename of_one_drc_info to of_read_drc_info_cell
>   -- Fix some spacing within expressions
>   -- Make some style corrections
> ---
>  arch/powerpc/include/asm/prom.h |   15 +++
>  arch/powerpc/platforms/pseries/of_helpers.c |   60 ++
>  arch/powerpc/platforms/pseries/pseries_energy.c |  138 
> ++-
>  3 files changed, 185 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index 3243455..0ef41b1 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -96,6 +96,21 @@ struct of_drconf_cell {
>  #define DRCONF_MEM_AI_INVALID0x0040
>  #define DRCONF_MEM_RESERVED  0x0080
> 
> +struct of_drc_info {
> + char *drc_type;
> + char *drc_name_prefix;
> + u32 drc_index_start;
> + u32 drc_name_suffix_start;
> + u32 num_sequential_elems;
> + u32 sequential_inc;
> + u32 drc_power_domain;
> + u32 last_drc_index;
> +};
> +
> +extern int of_read_drc_info_cell(struct property **prop,
> + const __be32 **curval, struct of_drc_info *data);
> +
> +
>  /*
>   * There are two methods for telling firmware what our capabilities are.
>   * Newer machines have an "ibm,client-architecture-support" method on the
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c 
> b/arch/powerpc/platforms/pseries/of_helpers.c
> index 2798933..b36f1ae 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -2,6 +2,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "of_helpers.h"
> 
> @@ -36,3 +37,62 @@ struct device_node *pseries_of_derive_parent(const char 
> *path)
>   kfree(parent_path);
>   return parent ? parent : ERR_PTR(-EINVAL);
>  }
> +
> +
> +/* Helper Routines to convert between drc_index to cpu numbers */
> +
> +int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
> + struct of_drc_info *data)
> +{
> + const char *p;
> + const __be32 *p2;
> +
> + if (!data)
> + return -EINVAL;
> +
> + /* Get drc-type:encode-string */
> + p = data->drc_type = (char*) (*curval);
> + p = of_prop_next_string(*prop, p);
> + if (!p)
> + return -EINVAL;
> +
> + /* Get drc-name-prefix:encode-string */
> + data->drc_name_prefix = (char *)p;
> + p = of_prop_next_string(*prop, p);
> + if (!p)
> + return -EINVAL;
> +
> + /* Get drc-index-start:encode-int */
> + p2 = (const __be32 *)p;
> + p2 = of_prop_next_u32(*prop, p2, >drc_index_start);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get drc-name-suffix-start:encode-int */
> + p2 = of_prop_next_u32(*prop, p2, >drc_name_suffix_start);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get number-sequential-elements:encode-int */
> + p2 = of_prop_next_u32(*prop, p2, >num_sequential_elems);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get sequential-increment:encode-int */
> + p2 = of_prop_next_u32(*prop, p2, >sequential_inc);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get drc-power-domain:encode-int */
> + p2 = of_prop_next_u32(*prop, p2, >drc_power_domain);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Should now know end of current entry */
> + (*curval) = (void *)p2;
> + data->last_drc_index = data->drc_index_start +
> + ((data->num_sequential_elems - 1) * data->sequential_inc);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(of_read_drc_info_cell);
> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c 
> b/arch/powerpc/platforms/pseries/pseries_energy.c
> index 35c891a..b8f6603 100644
> --- a/arch/powerpc/platforms/pseries/pseries_energy.c
> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
> 
>  #define MODULE_VERS "1.0"
> @@ -38,26 +39,64 @@
>  static u32 cpu_to_drc_index(int cpu)
>  {
>   struct device_node *dn = NULL;
> - const int *indexes;
> - int i;
> + int thread_index;
>   int rc = 1;
>   u32 ret = 0;
> 
>   dn = of_find_node_by_path("/cpus");
>   if (dn == NULL)
>   goto err;
> - indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> - if (indexes == NULL)
> - goto err_of_node_put;
> +
>   /* Convert logical cpu number to core number */
> - i = cpu_core_index_of_thread(cpu);
> -

Re: [PATCH v2 0/8] powerpc: Support ibm,dynamic-memory-v2 property

2017-11-20 Thread Nathan Fontenot 
On 11/16/2017 10:51 PM, Bharata B Rao wrote:
> 
> On Thu, Nov 16, 2017 at 9:31 PM, Nathan Fontenot  > wrote:
> 
> 
> 
> On 11/15/2017 11:37 PM, Bharata B Rao wrote:
> > On Fri, Oct 20, 2017 at 6:51 PM, Nathan Fontenot 
>  
> >> wrote:
> >
> >     This patch set provides a set of updates to de-couple the LMB 
> information
> >     provided in the ibm,dynamic-memory device tree property from the 
> device
> >     tree property format. A part of this patch series introduces a new
> >     device tree property format for dynamic memory, 
> ibm-dynamic-meory-v2.
> >     By separating the device tree format from the information provided 
> by
> >     the device tree property consumers of this information need not know
> >     what format is currently being used and provide multiple parsing 
> routines
> >     for the information.
> >
> >     The first two patches update the of_get_assoc_arrays() and
> >     of_get_usable_memory() routines to look up the device node for the
> >     properties they parse. This is needed because the calling routines 
> for
> >     these two functions will not have the device node to pass in in
> >     subsequent patches.
> >
> >     The third patch adds a new kernel structure, struct drmem_lmb, that
> >     is used to represent each of the possible LMBs specified in the
> >     ibm,dynamic-memory* device tree properties. The patch adds code
> >     to parse the property and build the LMB array data, and updates 
> prom.c
> >     to use this new data structure instead of parsing the device tree 
> directly.
> >
> >     The fourth and fifth patches update the numa and pseries hotplug 
> code
> >     respectively to use the new LMB array data instead of parsing the
> >     device tree directly.
> >
> >     The sixth patch moves the of_drconf_cell struct to drmem.h where it
> >     fits better than prom.h
> >
> >     The seventh patch introduces support for the ibm,dynamic-memory-v2
> >     property format by updating the new drmem.c code to be able to parse
> >     and create this new device tree format.
> >
> >     The last patch in the series updates the architecture vector to 
> indicate
> >     support for ibm,dynamic-memory-v2.
> >
> >
> > Here we are consolidating LMBs into LMB sets but still end up working 
> with individual LMBs during hotplug. Can we instead start working with LMB 
> sets together during hotplug ? In other words
> 
> In a sense we do do this when handling memory DLPAR indexed-count 
> requests. This takes a starting
> drc index for a LMB and adds/removes the following  contiguous 
> LMBs. This operation is
> all-or-nothing, if any LMB fails to add/remove we revert back to the 
> original state.
> 
> 
> I am aware of count-indexed and we do use it for memory hotplug/unplug for 
> KVM on Power. However the RTAS and configure-connector calls there are still 
> per-LMB.
> 
> 
> Thi isn't exactly what you're asking for but...
> >
> > - The RTAS calls involved during DRC acquire stage can be done only 
> once per LMB set.
> > - One configure-connector call for the entire LMB set.
> 
> these two interfaces work on a single drc index, not a set of drc 
> indexes. Working on a set
> of LMBs would require extending the current rtas calls or creating new 
> ones.
> 
> 
> Yes.
>  
> 
> 
> One thing we can look into doing for indexed-count requests is to perform 
> each of the
> steps for all LMBs in the set at once, i.e. make the acquire call for 
> LMBs, then make the
> configure-connector calls for all the LMBs...
> 
> 
> That is what I am hinting at to check the feasibility of such a mechanism. 
> Given that all the LMBs of the set are supposed to have similar attributes 
> (like node associativity etc), it makes sense to have a single DRC acquire 
> call and single configure-connector call for the entire set.

I agree. I'll talk to pHyp development to see if this is something they are 
interested in pursuing.
If not we can submit updates to the PAPR to implement these new rtas calls even 
if they do not
support them in pHyp.

> 
> 
> The only drawback is this approach would make handling failures and 
> backing out of the
> updates a bit messier, but I've never really thought that optimizing for 
> the failure
> case to be as important.
> 
> 
> Yes, error recovery can be messy given that we have multiple calls under DRC 
> acquire call (get-sensor-state and set-indicator).
> 
> BTW I thought this reorganization involving ibm,drc-info and 
> ibm,dynamic-memory-v2 was for representing and hotplugging huge amounts of 
> memory efficiently and quickly. So you have not yet