Re: [PATCH] raid6/altivec: adding vpermxor implementation for raid6 Q syndrome

Hi Daniel,

Just to respond to your comments,

The inline asm line cannot be formatted over multiple lines due to the
unrolling process, but we can take out the volatile.

The pagefault_disable() also seems to be an old method of disabling
preemption, but no longer actually works to disable preemption.
Preempt_disable should be used instead now. So the use of
pagefault_disable() in crc32d-vpmsum_glue.c is actually a bug.

Thanks,
Matt

On Tue, Apr 4, 2017 at 11:51 AM, Daniel Axtens [off-list ref] wrote:

Hi Matt,

quoted

Woops, totally missed that big chunk of makefile in the commit.
I had a chat with Oliver last week about the backwards compatibility stuff.
This will work for all versions >= 207S.

From what I can tell there is almost no difference between
pagefault_disable() and preempt_disable(), but I'll follow that up
when I'm in the office next.

Cool, good to know.

See you when you're next in!

Regards,
Daniel

quoted

Thanks for the review,

Matt

On Tue, Apr 4, 2017 at 7:44 AM, Daniel Axtens [off-list ref] wrote:

quoted

quoted

In that function, the flow is:
 pagefault_disable();
 enable_kernel_altivec();
 <vectorised function>
 pagefault_enable();

There are a few things that it would be nice (but by no means essential)
to find out:
 - what is the difference between pagefault and prempt enable/disable
 - is it required to disable altivec after the end of the function or
   can we leave that enabled?

Answering my own question here, dc4fbba11e46 ("powerpc: Create
disable_kernel_{fp,altivec,vsx,spe}()") adds the disable_ function, and
it's a no-op except under debug conditions. So it should stay.

Regards,
Daniel

quoted

quoted

+
+int raid6_have_altivec_vpermxor(void);
+#if $# == 1
+int raid6_have_altivec_vpermxor(void)
+{
+    /* Check if CPU has both altivec and the vpermxor instruction*/

Please add a space: s|ion*/|ion */|

quoted

+# ifdef __KERNEL__
+    return (cpu_has_feature(CONFIG_ALTIVEC) &&
+            cpu_has_feature(CPU_FTR_ARCH_207S));

I assume this is future-proof - an ISA 3.00 cpu will advertise 2.07S
compat?

quoted

+# else
+    return 1;
+#endif
+
+}
+#endif
+
+const struct raid6_calls raid6_vpermxor$# = {
+    raid6_vpermxor$#_gen_syndrome,
+    NULL,
+    raid6_have_altivec_vpermxor,
+    "vpermxor$#",
+    0
+};
+#endif
--
2.9.3

Apart from that this patch looks good and I expect I will be able to
formally Review v2.

Regards,
Daniel