[PATCH 0/6][Bionic SRU] Revert sauce and integrate upstream fix

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/6][Bionic SRU] Revert sauce and integrate upstream fix

Manoj Iyer
Please consider the following patches that reverts a sauce patch and
integrates the upstream fix for the Cavium zram driver.

These patches were tested by me on a Cavium ThunderX system on Bionic.
Please see bug https://launchpad.net/bugs/1787469 for test results.



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/6] Revert "UBUNTU: SAUCE: crypto: thunderx_zip: Fix fallout from CONFIG_VMAP_STACK"

Manoj Iyer
This reverts commit 37321174b776c96346b72a8498533d9df0e85a68.

BugLink: https://launchpad.net/bugs/1787469

Signed-off-by: Manoj Iyer <[hidden email]>
---
 drivers/crypto/cavium/zip/zip_crypto.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/cavium/zip/zip_crypto.c b/drivers/crypto/cavium/zip/zip_crypto.c
index 2fc9b03e1b40..8df4d26cf9d4 100644
--- a/drivers/crypto/cavium/zip/zip_crypto.c
+++ b/drivers/crypto/cavium/zip/zip_crypto.c
@@ -124,7 +124,7 @@ int zip_compress(const u8 *src, unsigned int slen,
  struct zip_kernel_ctx *zip_ctx)
 {
  struct zip_operation  *zip_ops   = NULL;
- struct zip_state      *zip_state;
+ struct zip_state      zip_state;
  struct zip_device     *zip = NULL;
  int ret;
 
@@ -135,23 +135,20 @@ int zip_compress(const u8 *src, unsigned int slen,
  if (!zip)
  return -ENODEV;
 
- zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
- if (!zip_state)
- return -ENOMEM;
-
+ memset(&zip_state, 0, sizeof(struct zip_state));
  zip_ops = &zip_ctx->zip_comp;
 
  zip_ops->input_len  = slen;
  zip_ops->output_len = *dlen;
  memcpy(zip_ops->input, src, slen);
 
- ret = zip_deflate(zip_ops, zip_state, zip);
+ ret = zip_deflate(zip_ops, &zip_state, zip);
 
  if (!ret) {
  *dlen = zip_ops->output_len;
  memcpy(dst, zip_ops->output, *dlen);
  }
- kfree(zip_state);
+
  return ret;
 }
 
@@ -160,7 +157,7 @@ int zip_decompress(const u8 *src, unsigned int slen,
    struct zip_kernel_ctx *zip_ctx)
 {
  struct zip_operation  *zip_ops   = NULL;
- struct zip_state      *zip_state;
+ struct zip_state      zip_state;
  struct zip_device     *zip = NULL;
  int ret;
 
@@ -171,10 +168,7 @@ int zip_decompress(const u8 *src, unsigned int slen,
  if (!zip)
  return -ENODEV;
 
- zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
- if (!zip_state)
- return -ENOMEM;
-
+ memset(&zip_state, 0, sizeof(struct zip_state));
  zip_ops = &zip_ctx->zip_decomp;
  memcpy(zip_ops->input, src, slen);
 
@@ -185,13 +179,13 @@ int zip_decompress(const u8 *src, unsigned int slen,
  zip_ops->input_len  = slen;
  zip_ops->output_len = *dlen;
 
- ret = zip_inflate(zip_ops, zip_state, zip);
+ ret = zip_inflate(zip_ops, &zip_state, zip);
 
  if (!ret) {
  *dlen = zip_ops->output_len;
  memcpy(dst, zip_ops->output, *dlen);
  }
- kfree(zip_state);
+
  return ret;
 }
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/6] crypto: cavium - Fix fallout from CONFIG_VMAP_STACK

Manoj Iyer
In reply to this post by Manoj Iyer
From: Jan Glauber <[hidden email]>

Enabling virtual mapped kernel stacks breaks the thunderx_zip
driver. On compression or decompression the executing CPU hangs
in an endless loop. The reason for this is the usage of __pa
by the driver which does no longer work for an address that is
not part of the 1:1 mapping.

The zip driver allocates a result struct on the stack and needs
to tell the hardware the physical address within this struct
that is used to signal the completion of the request.

As the hardware gets the wrong address after the broken __pa
conversion it writes to an arbitrary address. The zip driver then
waits forever for the completion byte to contain a non-zero value.

Allocating the result struct from 1:1 mapped memory resolves this
bug.

BugLink: https://launchpad.net/bugs/1787469

Signed-off-by: Jan Glauber <[hidden email]>
Reviewed-by: Robert Richter <[hidden email]>
Cc: stable <[hidden email]> # 4.14
Signed-off-by: Herbert Xu <[hidden email]>
(cherry picked from commit 37ff02acaa3d7be87ecb89f198a549ffd3ae2403)
Signed-off-by: Manoj Iyer <[hidden email]>
---
 drivers/crypto/cavium/zip/zip_crypto.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/cavium/zip/zip_crypto.c b/drivers/crypto/cavium/zip/zip_crypto.c
index 8df4d26cf9d4..b92b6e7e100f 100644
--- a/drivers/crypto/cavium/zip/zip_crypto.c
+++ b/drivers/crypto/cavium/zip/zip_crypto.c
@@ -124,7 +124,7 @@ int zip_compress(const u8 *src, unsigned int slen,
  struct zip_kernel_ctx *zip_ctx)
 {
  struct zip_operation  *zip_ops   = NULL;
- struct zip_state      zip_state;
+ struct zip_state      *zip_state;
  struct zip_device     *zip = NULL;
  int ret;
 
@@ -135,20 +135,23 @@ int zip_compress(const u8 *src, unsigned int slen,
  if (!zip)
  return -ENODEV;
 
- memset(&zip_state, 0, sizeof(struct zip_state));
+ zip_state = kzalloc(sizeof(*zip_state), GFP_ATOMIC);
+ if (!zip_state)
+ return -ENOMEM;
+
  zip_ops = &zip_ctx->zip_comp;
 
  zip_ops->input_len  = slen;
  zip_ops->output_len = *dlen;
  memcpy(zip_ops->input, src, slen);
 
- ret = zip_deflate(zip_ops, &zip_state, zip);
+ ret = zip_deflate(zip_ops, zip_state, zip);
 
  if (!ret) {
  *dlen = zip_ops->output_len;
  memcpy(dst, zip_ops->output, *dlen);
  }
-
+ kfree(zip_state);
  return ret;
 }
 
@@ -157,7 +160,7 @@ int zip_decompress(const u8 *src, unsigned int slen,
    struct zip_kernel_ctx *zip_ctx)
 {
  struct zip_operation  *zip_ops   = NULL;
- struct zip_state      zip_state;
+ struct zip_state      *zip_state;
  struct zip_device     *zip = NULL;
  int ret;
 
@@ -168,7 +171,10 @@ int zip_decompress(const u8 *src, unsigned int slen,
  if (!zip)
  return -ENODEV;
 
- memset(&zip_state, 0, sizeof(struct zip_state));
+ zip_state = kzalloc(sizeof(*zip_state), GFP_ATOMIC);
+ if (!zip_state)
+ return -ENOMEM;
+
  zip_ops = &zip_ctx->zip_decomp;
  memcpy(zip_ops->input, src, slen);
 
@@ -179,13 +185,13 @@ int zip_decompress(const u8 *src, unsigned int slen,
  zip_ops->input_len  = slen;
  zip_ops->output_len = *dlen;
 
- ret = zip_inflate(zip_ops, &zip_state, zip);
+ ret = zip_inflate(zip_ops, zip_state, zip);
 
  if (!ret) {
  *dlen = zip_ops->output_len;
  memcpy(dst, zip_ops->output, *dlen);
  }
-
+ kfree(zip_state);
  return ret;
 }
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/6] crypto: cavium - Limit result reading attempts

Manoj Iyer
In reply to this post by Manoj Iyer
From: Jan Glauber <[hidden email]>

After issuing a request an endless loop was used to read the
completion state from memory which is asynchronously updated
by the ZIP coprocessor.

Add an upper bound to the retry attempts to prevent a CPU getting stuck
forever in case of an error. Additionally, add a read memory barrier
and a small delay between the reading attempts.

BugLink: https://launchpad.net/bugs/1787469

Signed-off-by: Jan Glauber <[hidden email]>
Reviewed-by: Robert Richter <[hidden email]>
Cc: stable <[hidden email]> # 4.14
Signed-off-by: Herbert Xu <[hidden email]>
(cherry picked from commit c782a8c43e94ba6c09e9de2d69b5e3a5840ce61c)
Signed-off-by: Manoj Iyer <[hidden email]>
---
 drivers/crypto/cavium/zip/common.h      | 21 +++++++++++++++++++++
 drivers/crypto/cavium/zip/zip_deflate.c |  4 ++--
 drivers/crypto/cavium/zip/zip_inflate.c |  4 ++--
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/cavium/zip/common.h b/drivers/crypto/cavium/zip/common.h
index dc451e0a43c5..58fb3ed6e644 100644
--- a/drivers/crypto/cavium/zip/common.h
+++ b/drivers/crypto/cavium/zip/common.h
@@ -46,8 +46,10 @@
 #ifndef __COMMON_H__
 #define __COMMON_H__
 
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -149,6 +151,25 @@ struct zip_operation {
  u32   sizeofzops;
 };
 
+static inline int zip_poll_result(union zip_zres_s *result)
+{
+ int retries = 1000;
+
+ while (!result->s.compcode) {
+ if (!--retries) {
+ pr_err("ZIP ERR: request timed out");
+ return -ETIMEDOUT;
+ }
+ udelay(10);
+ /*
+ * Force re-reading of compcode which is updated
+ * by the ZIP coprocessor.
+ */
+ rmb();
+ }
+ return 0;
+}
+
 /* error messages */
 #define zip_err(fmt, args...) pr_err("ZIP ERR:%s():%d: " \
       fmt "\n", __func__, __LINE__, ## args)
diff --git a/drivers/crypto/cavium/zip/zip_deflate.c b/drivers/crypto/cavium/zip/zip_deflate.c
index 9a944b8c1e29..d7133f857d67 100644
--- a/drivers/crypto/cavium/zip/zip_deflate.c
+++ b/drivers/crypto/cavium/zip/zip_deflate.c
@@ -129,8 +129,8 @@ int zip_deflate(struct zip_operation *zip_ops, struct zip_state *s,
  /* Stats update for compression requests submitted */
  atomic64_inc(&zip_dev->stats.comp_req_submit);
 
- while (!result_ptr->s.compcode)
- continue;
+ /* Wait for completion or error */
+ zip_poll_result(result_ptr);
 
  /* Stats update for compression requests completed */
  atomic64_inc(&zip_dev->stats.comp_req_complete);
diff --git a/drivers/crypto/cavium/zip/zip_inflate.c b/drivers/crypto/cavium/zip/zip_inflate.c
index 50cbdd83dbf2..7e0d73e2f89e 100644
--- a/drivers/crypto/cavium/zip/zip_inflate.c
+++ b/drivers/crypto/cavium/zip/zip_inflate.c
@@ -143,8 +143,8 @@ int zip_inflate(struct zip_operation *zip_ops, struct zip_state *s,
  /* Decompression requests submitted stats update */
  atomic64_inc(&zip_dev->stats.decomp_req_submit);
 
- while (!result_ptr->s.compcode)
- continue;
+ /* Wait for completion or error */
+ zip_poll_result(result_ptr);
 
  /* Decompression requests completed stats update */
  atomic64_inc(&zip_dev->stats.decomp_req_complete);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/6] crypto: cavium - Prevent division by zero

Manoj Iyer
In reply to this post by Manoj Iyer
From: Jan Glauber <[hidden email]>

Avoid two potential divisions by zero when calculating average
values for the zip statistics.

BugLink: https://launchpad.net/bugs/1787469

Signed-off-by: Jan Glauber <[hidden email]>
Reviewed-by: Robert Richter <[hidden email]>
Signed-off-by: Herbert Xu <[hidden email]>
(cherry picked from commit a40c88045506ecba8e3ae75da19e8a2c53e23a41)
Signed-off-by: Manoj Iyer <[hidden email]>
---
 drivers/crypto/cavium/zip/zip_main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/cavium/zip/zip_main.c b/drivers/crypto/cavium/zip/zip_main.c
index 1cd8aa488185..79b449e0f955 100644
--- a/drivers/crypto/cavium/zip/zip_main.c
+++ b/drivers/crypto/cavium/zip/zip_main.c
@@ -482,10 +482,11 @@ static int zip_show_stats(struct seq_file *s, void *unused)
  atomic64_add(val, &st->pending_req);
  }
 
- avg_chunk = (atomic64_read(&st->comp_in_bytes) /
-     atomic64_read(&st->comp_req_complete));
- avg_cr = (atomic64_read(&st->comp_in_bytes) /
-  atomic64_read(&st->comp_out_bytes));
+ val = atomic64_read(&st->comp_req_complete);
+ avg_chunk = (val) ? atomic64_read(&st->comp_in_bytes) / val : 0;
+
+ val = atomic64_read(&st->comp_out_bytes);
+ avg_cr = (val) ? atomic64_read(&st->comp_in_bytes) / val : 0;
  seq_printf(s, "        ZIP Device %d Stats\n"
       "-----------------------------------\n"
       "Comp Req Submitted        : \t%lld\n"
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/6] crypto: cavium - Fix statistics pending request value

Manoj Iyer
In reply to this post by Manoj Iyer
From: Jan Glauber <[hidden email]>

The pending request counter was read from the wrong register. While
at it, there is no need to use an atomic for it as it is only read
localy in a loop.

BugLink: https://launchpad.net/bugs/1787469

Signed-off-by: Jan Glauber <[hidden email]>
Reviewed-by: Robert Richter <[hidden email]>
Signed-off-by: Herbert Xu <[hidden email]>
(cherry picked from commit 1cc7e01ff977770ce0651f4d347a84e360835c3b)
Signed-off-by: Manoj Iyer <[hidden email]>
---
 drivers/crypto/cavium/zip/zip_main.c | 13 +++++--------
 drivers/crypto/cavium/zip/zip_main.h |  1 -
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/cavium/zip/zip_main.c b/drivers/crypto/cavium/zip/zip_main.c
index 79b449e0f955..ae5b20c695ca 100644
--- a/drivers/crypto/cavium/zip/zip_main.c
+++ b/drivers/crypto/cavium/zip/zip_main.c
@@ -469,6 +469,8 @@ static int zip_show_stats(struct seq_file *s, void *unused)
  struct zip_stats  *st;
 
  for (index = 0; index < MAX_ZIP_DEVICES; index++) {
+ u64 pending = 0;
+
  if (zip_dev[index]) {
  zip = zip_dev[index];
  st  = &zip->stats;
@@ -476,10 +478,8 @@ static int zip_show_stats(struct seq_file *s, void *unused)
  /* Get all the pending requests */
  for (q = 0; q < ZIP_NUM_QUEUES; q++) {
  val = zip_reg_read((zip->reg_base +
-    ZIP_DBG_COREX_STA(q)));
- val = (val >> 32);
- val = val & 0xffffff;
- atomic64_add(val, &st->pending_req);
+    ZIP_DBG_QUEX_STA(q)));
+ pending += val >> 32 & 0xffffff;
  }
 
  val = atomic64_read(&st->comp_req_complete);
@@ -514,10 +514,7 @@ static int zip_show_stats(struct seq_file *s, void *unused)
        (u64)atomic64_read(&st->decomp_in_bytes),
        (u64)atomic64_read(&st->decomp_out_bytes),
        (u64)atomic64_read(&st->decomp_bad_reqs),
-       (u64)atomic64_read(&st->pending_req));
-
- /* Reset pending requests  count */
- atomic64_set(&st->pending_req, 0);
+       pending);
  }
  }
  return 0;
diff --git a/drivers/crypto/cavium/zip/zip_main.h b/drivers/crypto/cavium/zip/zip_main.h
index 64e051f60784..e1e4fa92ce80 100644
--- a/drivers/crypto/cavium/zip/zip_main.h
+++ b/drivers/crypto/cavium/zip/zip_main.h
@@ -74,7 +74,6 @@ struct zip_stats {
  atomic64_t    comp_req_complete;
  atomic64_t    decomp_req_submit;
  atomic64_t    decomp_req_complete;
- atomic64_t    pending_req;
  atomic64_t    comp_in_bytes;
  atomic64_t    comp_out_bytes;
  atomic64_t    decomp_in_bytes;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 6/6] crypto: cavium - Fix smp_processor_id() warnings

Manoj Iyer
In reply to this post by Manoj Iyer
From: Jan Glauber <[hidden email]>

Switch to raw_smp_processor_id() to prevent a number of
warnings from kernel debugging. We do not care about
preemption here, as the CPU number is only used as a
poor mans load balancing or device selection. If preemption
happens during a compress/decompress operation a small performance
hit will occur but everything will continue to work, so just
ignore it.

BugLink: https://launchpad.net/bugs/1787469

Signed-off-by: Jan Glauber <[hidden email]>
Reviewed-by: Robert Richter <[hidden email]>
Signed-off-by: Herbert Xu <[hidden email]>
(cherry picked from commit e7a9b05ca4c707ff4b46a77963db48d085d383e0)
Signed-off-by: Manoj Iyer <[hidden email]>
---
 drivers/crypto/cavium/zip/zip_device.c | 4 ++--
 drivers/crypto/cavium/zip/zip_main.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/cavium/zip/zip_device.c b/drivers/crypto/cavium/zip/zip_device.c
index ccf21fb91513..f174ec29ed69 100644
--- a/drivers/crypto/cavium/zip/zip_device.c
+++ b/drivers/crypto/cavium/zip/zip_device.c
@@ -87,12 +87,12 @@ u32 zip_load_instr(union zip_inst_s *instr,
  * Distribute the instructions between the enabled queues based on
  * the CPU id.
  */
- if (smp_processor_id() % 2 == 0)
+ if (raw_smp_processor_id() % 2 == 0)
  queue = 0;
  else
  queue = 1;
 
- zip_dbg("CPU Core: %d Queue number:%d", smp_processor_id(), queue);
+ zip_dbg("CPU Core: %d Queue number:%d", raw_smp_processor_id(), queue);
 
  /* Take cmd buffer lock */
  spin_lock(&zip_dev->iq[queue].lock);
diff --git a/drivers/crypto/cavium/zip/zip_main.c b/drivers/crypto/cavium/zip/zip_main.c
index ae5b20c695ca..be055b9547f6 100644
--- a/drivers/crypto/cavium/zip/zip_main.c
+++ b/drivers/crypto/cavium/zip/zip_main.c
@@ -113,7 +113,7 @@ struct zip_device *zip_get_device(int node)
  */
 int zip_get_node_id(void)
 {
- return cpu_to_node(smp_processor_id());
+ return cpu_to_node(raw_smp_processor_id());
 }
 
 /* Initializes the ZIP h/w sub-system */
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/6][Bionic SRU] Revert sauce and integrate upstream fix

Kleber Souza
In reply to this post by Manoj Iyer
On 08/17/18 16:42, Manoj Iyer wrote:
> Please consider the following patches that reverts a sauce patch and
> integrates the upstream fix for the Cavium zram driver.
>
> These patches were tested by me on a Cavium ThunderX system on Bionic.
> Please see bug https://launchpad.net/bugs/1787469 for test results.
>
>
>

Reverts a SAUCE, which is good, and apply follow-up fixes from vendor.
Limited to platform driver.

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/6][Bionic SRU] Revert sauce and integrate upstream fix

Khalid Elmously
In reply to this post by Manoj Iyer
On 2018-08-17 09:42:00 , Manoj Iyer wrote:
> Please consider the following patches that reverts a sauce patch and
> integrates the upstream fix for the Cavium zram driver.
>
> These patches were tested by me on a Cavium ThunderX system on Bionic.
> Please see bug https://launchpad.net/bugs/1787469 for test results.
>
>
>
Acked-by: Khalid Elmously <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [PATCH 0/6][Bionic SRU] Revert sauce and integrate upstream fix

Khalid Elmously
In reply to this post by Manoj Iyer
..to bionic/master-next


On 2018-08-17 09:42:00 , Manoj Iyer wrote:

> Please consider the following patches that reverts a sauce patch and
> integrates the upstream fix for the Cavium zram driver.
>
> These patches were tested by me on a Cavium ThunderX system on Bionic.
> Please see bug https://launchpad.net/bugs/1787469 for test results.
>
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team