* [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands.
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170217125941.14319-1-hch@lst.de>
From: Scott Bauer <scott.bauer@intel.com>
We need to verify that the controller supports the security
commands before actually trying to issue them.
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
[hch: moved the check so that we don't call into the OPAL code if not
supported]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 1 +
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/pci.c | 2 +-
include/linux/nvme.h | 1 +
4 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8aeb4a623b65..44a1a257e0b5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1285,6 +1285,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
return -EIO;
}
+ ctrl->oacs = le16_to_cpu(id->oacs);
ctrl->vid = le16_to_cpu(id->vid);
ctrl->oncs = le16_to_cpup(&id->oncs);
atomic_set(&ctrl->abort_limit, id->acl + 1);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5126c4bbee1a..14cfc6f7facb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -140,6 +140,7 @@ struct nvme_ctrl {
u32 max_hw_sectors;
u16 oncs;
u16 vid;
+ u16 oacs;
atomic_t abort_limit;
u8 event_limit;
u8 vwc;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5db8a38a8b43..ddc51adb594d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,7 +1789,7 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if (!dev->ctrl.opal_dev) {
+ if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
dev->ctrl.opal_dev =
init_opal_dev(&dev->ctrl, &nvme_sec_submit);
}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3e2ed49c3ad8..0b676a02cf3e 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -244,6 +244,7 @@ enum {
NVME_CTRL_ONCS_DSM = 1 << 2,
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
+ NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
};
struct nvme_lbaf {
--
2.11.0
^ permalink raw reply related
* [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170217125941.14319-1-hch@lst.de>
Insted of bloating the containing structure with it all the time this
allocates struct opal_dev dynamically. Additionally this allows moving
the definition of struct opal_dev into sed-opal.c. For this a new
private data field is added to it that is passed to the send/receive
callback. After that a lot of internals can be made private as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: Scott Bauer <scott.bauer@intel.com>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>
---
block/opal_proto.h | 23 ++++++++++
block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++----
drivers/nvme/host/core.c | 9 ++--
drivers/nvme/host/nvme.h | 14 ++----
drivers/nvme/host/pci.c | 8 +++-
include/linux/sed-opal.h | 116 ++---------------------------------------------
6 files changed, 131 insertions(+), 140 deletions(-)
diff --git a/block/opal_proto.h b/block/opal_proto.h
index af9abc56c157..f40c9acf8895 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -19,6 +19,29 @@
#ifndef _OPAL_PROTO_H
#define _OPAL_PROTO_H
+/*
+ * These constant values come from:
+ * SPC-4 section
+ * 6.30 SECURITY PROTOCOL IN command / table 265.
+ */
+enum {
+ TCG_SECP_00 = 0,
+ TCG_SECP_01,
+};
+
+/*
+ * Token defs derived from:
+ * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
+ * 3.2.2 Data Stream Encoding
+ */
+enum opal_response_token {
+ OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
+ OPAL_DTA_TOKENID_SINT = 0xe1,
+ OPAL_DTA_TOKENID_UINT = 0xe2,
+ OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
+ OPAL_DTA_TOKENID_INVALID = 0X0
+};
+
#define DTAERROR_NO_METHOD_STATUS 0x89
#define GENERIC_HOST_SESSION_NUM 0x41
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bcdd5b6d02e8..d1c52ba4d62d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -31,6 +31,77 @@
#include "opal_proto.h"
+#define IO_BUFFER_LENGTH 2048
+#define MAX_TOKS 64
+
+typedef int (*opal_step)(struct opal_dev *dev);
+
+enum opal_atom_width {
+ OPAL_WIDTH_TINY,
+ OPAL_WIDTH_SHORT,
+ OPAL_WIDTH_MEDIUM,
+ OPAL_WIDTH_LONG,
+ OPAL_WIDTH_TOKEN
+};
+
+/*
+ * On the parsed response, we don't store again the toks that are already
+ * stored in the response buffer. Instead, for each token, we just store a
+ * pointer to the position in the buffer where the token starts, and the size
+ * of the token in bytes.
+ */
+struct opal_resp_tok {
+ const u8 *pos;
+ size_t len;
+ enum opal_response_token type;
+ enum opal_atom_width width;
+ union {
+ u64 u;
+ s64 s;
+ } stored;
+};
+
+/*
+ * From the response header it's not possible to know how many tokens there are
+ * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
+ * if we start dealing with messages that have more than that, we can increase
+ * this number. This is done to avoid having to make two passes through the
+ * response, the first one counting how many tokens we have and the second one
+ * actually storing the positions.
+ */
+struct parsed_resp {
+ int num;
+ struct opal_resp_tok toks[MAX_TOKS];
+};
+
+struct opal_dev {
+ bool supported;
+
+ void *data;
+ sec_send_recv *send_recv;
+
+ const opal_step *funcs;
+ void **func_data;
+ int state;
+ struct mutex dev_lock;
+ u16 comid;
+ u32 hsn;
+ u32 tsn;
+ u64 align;
+ u64 lowest_lba;
+
+ size_t pos;
+ u8 cmd[IO_BUFFER_LENGTH];
+ u8 resp[IO_BUFFER_LENGTH];
+
+ struct parsed_resp parsed;
+ size_t prev_d_len;
+ void *prev_data;
+
+ struct list_head unlk_lst;
+};
+
+
static const u8 opaluid[][OPAL_UID_LENGTH] = {
/* users */
[OPAL_SMUID_UID] =
@@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data)
static int opal_send_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->cmd, IO_BUFFER_LENGTH,
true);
}
static int opal_recv_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->resp, IO_BUFFER_LENGTH,
false);
}
@@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev)
return ret;
}
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
{
- if (opal_dev->initialized)
- return;
- INIT_LIST_HEAD(&opal_dev->unlk_lst);
- mutex_init(&opal_dev->dev_lock);
- opal_dev->send_recv = send_recv;
- if (check_opal_support(opal_dev) < 0)
+ struct opal_dev *dev;
+
+ dev = kmalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return NULL;
+
+ INIT_LIST_HEAD(&dev->unlk_lst);
+ mutex_init(&dev->dev_lock);
+ dev->data = data;
+ dev->send_recv = send_recv;
+ if (check_opal_support(dev) != 0) {
pr_debug("Opal is not supported on this device\n");
- opal_dev->initialized = true;
+ kfree(dev);
+ return NULL;
+ }
+ return dev;
}
EXPORT_SYMBOL(init_opal_dev);
@@ -2351,6 +2430,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
+ if (!dev)
+ return -ENOTSUPP;
if (!dev->supported) {
pr_err("Not supported\n");
return -ENOTSUPP;
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 04c48e75ff16..8aeb4a623b65 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return nvme_nvm_ioctl(ns, cmd, arg);
#endif
if (is_sed_ioctl(cmd))
- return sed_ioctl(&ns->ctrl->opal_dev, cmd,
+ return sed_ioctl(ns->ctrl->opal_dev, cmd,
(void __user *) arg);
return -ENOTTY;
}
@@ -1085,18 +1085,17 @@ static const struct pr_ops nvme_pr_ops = {
};
#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send)
{
+ struct nvme_ctrl *ctrl = data;
struct nvme_command cmd;
- struct nvme_ctrl *ctrl = NULL;
memset(&cmd, 0, sizeof(cmd));
if (send)
cmd.common.opcode = nvme_admin_security_send;
else
cmd.common.opcode = nvme_admin_security_recv;
- ctrl = container_of(dev, struct nvme_ctrl, opal_dev);
cmd.common.nsid = 0;
cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
cmd.common.cdw10[1] = cpu_to_le32(len);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 569cba14cede..5126c4bbee1a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -126,7 +126,7 @@ struct nvme_ctrl {
struct list_head node;
struct ida ns_ida;
- struct opal_dev opal_dev;
+ struct opal_dev *opal_dev;
char name[12];
char serial[20];
@@ -270,16 +270,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl);
void nvme_queue_scan(struct nvme_ctrl *ctrl);
void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
-#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-#else
-static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
-{
- return 0;
-}
-#endif /* CONFIG_BLK_DEV_SED_OPAL */
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send);
#define NVME_NR_AERS 1
void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e25d632bd9f2..5db8a38a8b43 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
if (dev->ctrl.admin_q)
blk_put_queue(dev->ctrl.admin_q);
kfree(dev->queues);
+ kfree(dev->ctrl.opal_dev);
kfree(dev);
}
@@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit);
+ if (!dev->ctrl.opal_dev) {
+ dev->ctrl.opal_dev =
+ init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ }
if (was_suspend)
- opal_unlock_from_suspend(&dev->ctrl.opal_dev);
+ opal_unlock_from_suspend(dev->ctrl.opal_dev);
result = nvme_setup_io_queues(dev);
if (result)
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index 205d520ea688..deee23d012e7 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -21,117 +21,14 @@
#include <uapi/linux/sed-opal.h>
#include <linux/kernel.h>
-/*
- * These constant values come from:
- * SPC-4 section
- * 6.30 SECURITY PROTOCOL IN command / table 265.
- */
-enum {
- TCG_SECP_00 = 0,
- TCG_SECP_01,
-};
struct opal_dev;
-#define IO_BUFFER_LENGTH 2048
-#define MAX_TOKS 64
-
-typedef int (*opal_step)(struct opal_dev *dev);
-typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-
-
-enum opal_atom_width {
- OPAL_WIDTH_TINY,
- OPAL_WIDTH_SHORT,
- OPAL_WIDTH_MEDIUM,
- OPAL_WIDTH_LONG,
- OPAL_WIDTH_TOKEN
-};
-
-/*
- * Token defs derived from:
- * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
- * 3.2.2 Data Stream Encoding
- */
-enum opal_response_token {
- OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
- OPAL_DTA_TOKENID_SINT = 0xe1,
- OPAL_DTA_TOKENID_UINT = 0xe2,
- OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
- OPAL_DTA_TOKENID_INVALID = 0X0
-};
-
-/*
- * On the parsed response, we don't store again the toks that are already
- * stored in the response buffer. Instead, for each token, we just store a
- * pointer to the position in the buffer where the token starts, and the size
- * of the token in bytes.
- */
-struct opal_resp_tok {
- const u8 *pos;
- size_t len;
- enum opal_response_token type;
- enum opal_atom_width width;
- union {
- u64 u;
- s64 s;
- } stored;
-};
-
-/*
- * From the response header it's not possible to know how many tokens there are
- * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
- * if we start dealing with messages that have more than that, we can increase
- * this number. This is done to avoid having to make two passes through the
- * response, the first one counting how many tokens we have and the second one
- * actually storing the positions.
- */
-struct parsed_resp {
- int num;
- struct opal_resp_tok toks[MAX_TOKS];
-};
-
-/**
- * struct opal_dev - The structure representing a OPAL enabled SED.
- * @supported: Whether or not OPAL is supported on this controller.
- * @send_recv: The combined sec_send/sec_recv function pointer.
- * @opal_step: A series of opal methods that are necessary to complete a command.
- * @func_data: An array of parameters for the opal methods above.
- * @state: Describes the current opal_step we're working on.
- * @dev_lock: Locks the entire opal_dev structure.
- * @parsed: Parsed response from controller.
- * @prev_data: Data returned from a method to the controller.
- * @unlk_lst: A list of Locking ranges to unlock on this device during a resume.
- */
-struct opal_dev {
- bool initialized;
- bool supported;
- sec_send_recv *send_recv;
-
- const opal_step *funcs;
- void **func_data;
- int state;
- struct mutex dev_lock;
- u16 comid;
- u32 hsn;
- u32 tsn;
- u64 align;
- u64 lowest_lba;
-
- size_t pos;
- u8 cmd[IO_BUFFER_LENGTH];
- u8 resp[IO_BUFFER_LENGTH];
-
- struct parsed_resp parsed;
- size_t prev_d_len;
- void *prev_data;
-
- struct list_head unlk_lst;
-};
+typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer,
+ size_t len, bool send);
#ifdef CONFIG_BLK_SED_OPAL
bool opal_unlock_from_suspend(struct opal_dev *dev);
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv);
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr);
static inline bool is_sed_ioctl(unsigned int cmd)
@@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev)
{
return false;
}
-static inline void init_opal_dev(struct opal_dev *opal_dev,
- sec_send_recv *send_recv)
-{
- opal_dev->supported = false;
- opal_dev->initialized = true;
-}
+#define init_opal_dev(data, send_recv) NULL
#endif /* CONFIG_BLK_SED_OPAL */
#endif /* LINUX_OPAL_H */
--
2.11.0
^ permalink raw reply related
* [PATCH 1/4] block/sed-opal: tone down not supported warnings
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170217125941.14319-1-hch@lst.de>
Not having OPAL or a sub-feature supported is an entirely normal
condition for many drives, so don't warn about it. Keep the messages,
but tone them down to debug only.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index e95b8a57053d..bcdd5b6d02e8 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -387,16 +387,16 @@ static int opal_discovery0_end(struct opal_dev *dev)
}
if (!supported) {
- pr_err("This device is not Opal enabled. Not Supported!\n");
+ pr_debug("This device is not Opal enabled. Not Supported!\n");
return -EOPNOTSUPP;
}
if (!single_user)
- pr_warn("Device doesn't support single user mode\n");
+ pr_debug("Device doesn't support single user mode\n");
if (!found_com_id) {
- pr_warn("Could not find OPAL comid for device. Returning early\n");
+ pr_debug("Could not find OPAL comid for device. Returning early\n");
return -EOPNOTSUPP;;
}
@@ -1951,7 +1951,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
mutex_init(&opal_dev->dev_lock);
opal_dev->send_recv = send_recv;
if (check_opal_support(opal_dev) < 0)
- pr_warn("Opal is not supported on this device\n");
+ pr_debug("Opal is not supported on this device\n");
opal_dev->initialized = true;
}
EXPORT_SYMBOL(init_opal_dev);
--
2.11.0
^ permalink raw reply related
* OPAL fixups
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
Hi all,
this contains a few more OPAL-related fixups. It tones down warnings a bit,
allocates the OPAL-ѕpecific data structure in a separate dynamic allocation,
checks for support of Security Send/Receive in NVMe before using them,
and makes sure we re-discovery the security capabilities after each reset.
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Ulf Hansson @ 2017-02-17 12:09 UTC (permalink / raw)
To: Ziji Hu
Cc: Linus Walleij, linux-mmc@vger.kernel.org, linux-block,
Adrian Hunter, Ritesh Harjani, Avri Altman, Arnd Bergmann,
Christoph Hellwig, Jens Axboe, Paolo Valente, Per Förlin
In-Reply-To: <e4836ece-d990-667c-27d0-4b19a2b4b160@marvell.com>
[...]
>
> I would like to suggest that you should try the multiple thread
> test mode of iozone, since you are testing *Multi* Queue.
Yes. That seems reasonable.
However, the most important part here is the comparison between the
different code bases.
>
> Besides, it seems that your eMMC transfer speed is quite low.
> It is normal that read speed can reach more than 100MB/s in HS400.
> Could you try a higher speed mode? The test result might be
> limited by the bus clock frequency.
Perhaps if Linus can share a branch of the code integrated for the
different tests, we all can help out running them on those HW we have
at hand. Would you be willing to help out here?
>
>>
>> As you can see there are no huge performance regressions with these
>> kinds of "raw" throughput tests.
>>
>> These iozone figures are unintuitive unless your head can
>> plot logarithmic, look at the charts here for a more visual presentation
>> of the iozone results:
>> https://docs.google.com/spreadsheets/d/1rm72TiGlTnzDeGLR__aqvjcJ2UkA-Ro3-XyKA8r1M-c
>>
>> Compare this to the performance change we got when first introducing
>> the asynchronous requests:
>> https://wiki.linaro.org/WorkingGroups/KernelArchived/Specs/StoragePerfMMC-async-req
>>
>> The patches need some issues fixed from the build server
>> complaints and some robustness hammering, but after that I
>> think they will be ripe for merging for v4.12.
>>
>
> Actually I have been following your thread for some time.
> But currently I'm a little confused.
> May I know the purpose of your patch?
I want MMC to move to the new BLKMQ interface and I want that because
of several reasons, see below.
1. It's new blk interface, all new development happens here. We should
use it to benefit from that.
2. The BLKMQ interface allow the MMC block device driver to be
significantly cleaner implemented - and I need that to be able to
maintain the code.
3. We want to make use of Paolo's BFQ-MK I/O scheduler, which
addresses provides guaranteed low latency. For example being able to
play a video clip, while doing a disc backup without getting frame
drops.
Kind regards
Uffe
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Ziji Hu @ 2017-02-17 11:53 UTC (permalink / raw)
To: Linus Walleij, linux-mmc@vger.kernel.org, linux-block
Cc: Ulf Hansson, Adrian Hunter, Ritesh Harjani, Avri Altman,
Arnd Bergmann, Christoph Hellwig, Jens Axboe, Paolo Valente,
Per Förlin
In-Reply-To: <CACRpkdYXk_MKhaeQLqGA+8Q4g5qy-DkRymC0Ecf+_qsm+cBoBQ@mail.gmail.com>
Hi Linus,
On 2017/2/17 17:33, Linus Walleij wrote:
> This week I tested the following:
>
> - Merge the in-flight BFQ work from Paolo with my MMC MQ patch set
> - Enable BFQ
> - Run a few iterations of classic throughput tests
>
> dd on whole internal eMMC, 7.38 GiB:
>
> sync
> echo 3 > /proc/sys/vm/drop_caches
> sync
> time dd if=/dev/mmcblk3 of=/dev/null
> time dd if=/dev/mmcblk3 of=/dev/null bs=1M
>
> iozone on a Noname SD card 2GB
>
> mount /dev/mmcblk0p1 /mnt
> sync
> echo 3 > /proc/sys/vm/drop_caches
> sync
> iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
>
> The results:
>
> Before patches (v4.10-rc8):
>
> 7918845952 bytes (7.4GB) copied, 194.504059 seconds, 38.8MB/s
> real 3m 14.51s
> user 0m 7.41s
> sys 1m 10.34s
>
> 7918845952 bytes (7.4GB) copied, 176.519531 seconds, 42.8MB/s
> real 2m 56.53s
> user 0m 0.06s
> sys 0m 36.57s
>
> Command line used: iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
> Output is in kBytes/sec
>
> random random
> kB reclen write rewrite read reread read write
> 20480 4 1960 2105 5991 6023 5202 40
> 20480 8 4636 4901 9087 9103 9066 80
> 20480 16 5522 5663 12237 12242 12206 163
> 20480 32 5976 6031 14915 14917 14901 333
> 20480 64 6286 6387 16737 16763 16738 678
> 20480 128 6720 6757 17876 17857 17865 1403
> 20480 256 6846 6909 18230 17568 16719 3039
> 20480 512 7204 7229 18471 18751 18834 7209
> 20480 1024 7257 7315 18684 18044 18095 7337
> 20480 2048 7322 7388 18605 18802 19437 7401
> 20480 4096 7553 7652 21510 21108 21503 7688
> 20480 8192 7534 7745 22164 22300 22490 7758
> 20480 16384 7357 7818 23053 23048 23056 7834
>
>
> After MMC MQ patches:
>
> 7918845952 bytes (7.4GB) copied, 196.907776 seconds, 38.4MB/s
> real 3m 16.91s
> user 0m 7.17s
> sys 1m 8.03s
>
> 7918845952 bytes (7.4GB) copied, 192.595734 seconds, 39.2MB/s
> real 3m 12.60s
> user 0m 0.12s
> sys 0m 33.11s
>
> Command line used: iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
> random random
> kB reclen write rewrite read reread read write
> 20480 4 2049 2154 5991 5998 5934 40
> 20480 8 4654 4921 9081 9075 9028 81
> 20480 16 5572 5747 12250 12252 12177 164
> 20480 32 6040 6084 14858 14895 14833 335
> 20480 64 6370 6449 16759 16770 16715 682
> 20480 128 6834 6814 17882 17843 17878 1411
> 20480 256 6892 6900 18526 18105 18430 3066
> 20480 512 7239 7254 18839 18864 18837 7258
> 20480 1024 7342 6453 18787 18161 17522 7343
> 20480 2048 7408 7439 17891 18211 19029 7472
> 20480 4096 7641 7703 20950 21044 20900 7705
> 20480 8192 7584 7811 22261 22170 22385 7809
> 20480 16384 7407 7873 23033 23050 23048 7905
>
>
> After MMC MQ+BFQ patches:
>
> 7918845952 bytes (7.4GB) copied, 197.097717 seconds, 38.3MB/s
> real 3m 17.10s
> user 0m 7.67s
> sys 1m 7.33s
>
> 7552+0 records in
> 7552+0 records out
> 7918845952 bytes (7.4GB) copied, 187.119538 seconds, 40.4MB/s
> real 3m 7.12s
> user 0m 0.11s
> sys 0m 34.61s
>
> Command line used: iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
> Output is in kBytes/sec
> random random
> kB reclen write rewrite read reread read write
> 20480 4 1734 1786 5923 5166 5894 40
> 20480 8 4614 4853 8950 8949 8909 80
> 20480 16 5525 5705 12086 12098 12040 164
> 20480 32 6027 6040 14765 14793 14755 334
> 20480 64 6341 6404 16696 16697 16670 680
> 20480 128 6799 6842 17830 17833 17814 1407
> 20480 256 6848 6849 17394 18251 17537 3054
> 20480 512 7191 7229 18545 18628 18801 7224
> 20480 1024 7241 7331 17845 17909 18206 7302
> 20480 2048 7375 7433 18794 19288 19675 7426
> 20480 4096 7583 7696 21024 21194 21082 7659
> 20480 8192 7555 7767 22068 22170 22168 7808
> 20480 16384 7350 7831 23021 23032 23050 7870
>
I would like to suggest that you should try the multiple thread
test mode of iozone, since you are testing *Multi* Queue.
Besides, it seems that your eMMC transfer speed is quite low.
It is normal that read speed can reach more than 100MB/s in HS400.
Could you try a higher speed mode? The test result might be
limited by the bus clock frequency.
>
> As you can see there are no huge performance regressions with these
> kinds of "raw" throughput tests.
>
> These iozone figures are unintuitive unless your head can
> plot logarithmic, look at the charts here for a more visual presentation
> of the iozone results:
> https://docs.google.com/spreadsheets/d/1rm72TiGlTnzDeGLR__aqvjcJ2UkA-Ro3-XyKA8r1M-c
>
> Compare this to the performance change we got when first introducing
> the asynchronous requests:
> https://wiki.linaro.org/WorkingGroups/KernelArchived/Specs/StoragePerfMMC-async-req
>
> The patches need some issues fixed from the build server
> complaints and some robustness hammering, but after that I
> think they will be ripe for merging for v4.12.
>
Actually I have been following your thread for some time.
But currently I'm a little confused.
May I know the purpose of your patch?
Thank you.
Best regards,
Hu Ziji
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] blk-mq-sched: don't hold queue_lock when calling exit_icq
From: Paolo Valente @ 2017-02-17 10:30 UTC (permalink / raw)
To: Jens Axboe; +Cc: Omar Sandoval, linux-block, kernel-team
In-Reply-To: <7F6BB427-828D-4609-8A89-6CFB87D57B85@linaro.org>
> Il giorno 16 feb 2017, alle ore 11:31, Paolo Valente =
<paolo.valente@linaro.org> ha scritto:
>=20
>>=20
>> Il giorno 15 feb 2017, alle ore 19:04, Jens Axboe <axboe@fb.com> ha =
scritto:
>>=20
>> On 02/15/2017 10:58 AM, Jens Axboe wrote:
>>> On 02/15/2017 10:24 AM, Paolo Valente wrote:
>>>>=20
>>>>> Il giorno 10 feb 2017, alle ore 19:32, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>>>>>=20
>>>>> From: Omar Sandoval <osandov@fb.com>
>>>>>=20
>>>>> None of the other blk-mq elevator hooks are called with this lock =
held.
>>>>> Additionally, it can lead to circular locking dependencies between
>>>>> queue_lock and the private scheduler lock.
>>>>>=20
>>>>=20
>>>> Hi Omar,
>>>> I'm sorry but it seems that a new potential deadlock has showed up.
>>>> See lockdep splat below.
>>>>=20
>>>> I've tried to think about different solutions than turning back to
>>>> deferring the body of exit_icq, but at no avail.
>>>=20
>>> Looks like a interaction between bfqd->lock and q->queue_lock. Since =
the
>>> core has no notion of you bfqd->lock, the naturally dependency here
>>> would be to nest bfqd->lock inside q->queue_lock. Is that possible =
for
>>> you?
>>>=20
>>> Looking at the code a bit, maybe it'd just be simpler to get rid of
>>> holding the queue lock for that spot. For the mq scheduler, we =
really
>>> don't want places where we invoke with that held already. Does the =
below
>>> work for you?
>>=20
>> Would need to remove one more lockdep assert. And only test this for
>> the mq parts, we'd need to spread a bit of love on the classic
>> scheduling icq exit path for this to work on that side.
>>=20
>=20
> Sorry Jens, same splat. What confuses me is the second column
> in the possible scenario:
>=20
> [ 139.368477] CPU0 CPU1
> [ 139.369129] ---- ----
> [ 139.369774] lock(&(&ioc->lock)->rlock);
> [ 139.370339] =
lock(&(&q->__queue_lock)->rlock);
> [ 139.390579] =
lock(&(&ioc->lock)->rlock);
> [ 139.391522] lock(&(&bfqd->lock)->rlock);
>=20
> I could not find any code path, related to the reported call traces,
> and taking first q->queue_lock and then ioc->lock.
>=20
> Any suggestion on how to go on, and hopefully help with this problem =
is
> welcome.
>=20
Jens,
this is just to tell you that I have found the link that still causes
the circular dependency: an ioc->lock nested into a queue_lock in
ioc_create_icq. I'll try to come back with a solution proposal.
Thanks,
Paolo
> Thanks,
> Paolo
>=20
>> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
>> index b12f9c87b4c3..546ff8f81ede 100644
>> --- a/block/blk-ioc.c
>> +++ b/block/blk-ioc.c
>> @@ -54,7 +54,7 @@ static void ioc_exit_icq(struct io_cq *icq)
>> icq->flags |=3D ICQ_EXITED;
>> }
>>=20
>> -/* Release an icq. Called with both ioc and q locked. */
>> +/* Release an icq. Called with ioc locked. */
>> static void ioc_destroy_icq(struct io_cq *icq)
>> {
>> struct io_context *ioc =3D icq->ioc;
>> @@ -62,7 +62,6 @@ static void ioc_destroy_icq(struct io_cq *icq)
>> struct elevator_type *et =3D q->elevator->type;
>>=20
>> lockdep_assert_held(&ioc->lock);
>> - lockdep_assert_held(q->queue_lock);
>>=20
>> radix_tree_delete(&ioc->icq_tree, icq->q->id);
>> hlist_del_init(&icq->ioc_node);
>> @@ -222,25 +221,34 @@ void exit_io_context(struct task_struct *task)
>> put_io_context_active(ioc);
>> }
>>=20
>> +static void __ioc_clear_queue(struct list_head *icq_list)
>> +{
>> + while (!list_empty(icq_list)) {
>> + struct io_cq *icq =3D list_entry(icq_list->next,
>> + struct io_cq, q_node);
>> + struct io_context *ioc =3D icq->ioc;
>> +
>> + spin_lock_irq(&ioc->lock);
>> + ioc_destroy_icq(icq);
>> + spin_unlock_irq(&ioc->lock);
>> + }
>> +}
>> +
>> /**
>> * ioc_clear_queue - break any ioc association with the specified =
queue
>> * @q: request_queue being cleared
>> *
>> - * Walk @q->icq_list and exit all io_cq's. Must be called with @q =
locked.
>> + * Walk @q->icq_list and exit all io_cq's.
>> */
>> void ioc_clear_queue(struct request_queue *q)
>> {
>> - lockdep_assert_held(q->queue_lock);
>> + LIST_HEAD(icq_list);
>>=20
>> - while (!list_empty(&q->icq_list)) {
>> - struct io_cq *icq =3D list_entry(q->icq_list.next,
>> - struct io_cq, q_node);
>> - struct io_context *ioc =3D icq->ioc;
>> + spin_lock_irq(q->queue_lock);
>> + list_splice_init(&q->icq_list, &icq_list);
>> + spin_unlock_irq(q->queue_lock);
>>=20
>> - spin_lock(&ioc->lock);
>> - ioc_destroy_icq(icq);
>> - spin_unlock(&ioc->lock);
>> - }
>> + __ioc_clear_queue(&icq_list);
>> }
>>=20
>> int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, =
int node)
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index 070d81bae1d5..1944aa1cb899 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -815,9 +815,7 @@ static void blk_release_queue(struct kobject =
*kobj)
>> blkcg_exit_queue(q);
>>=20
>> if (q->elevator) {
>> - spin_lock_irq(q->queue_lock);
>> ioc_clear_queue(q);
>> - spin_unlock_irq(q->queue_lock);
>> elevator_exit(q->elevator);
>> }
>>=20
>> diff --git a/block/elevator.c b/block/elevator.c
>> index a25bdd90b270..aaa1e9836512 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -985,9 +985,7 @@ static int elevator_switch(struct request_queue =
*q, struct elevator_type *new_e)
>> if (old_registered)
>> elv_unregister_queue(q);
>>=20
>> - spin_lock_irq(q->queue_lock);
>> ioc_clear_queue(q);
>> - spin_unlock_irq(q->queue_lock);
>> }
>>=20
>> /* allocate, init and register new elevator */
>>=20
>> --=20
>> Jens Axboe
^ permalink raw reply
* Some throughput tests with MQ and BFQ on MMC/SD
From: Linus Walleij @ 2017-02-17 9:33 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org, linux-block
Cc: Ulf Hansson, Adrian Hunter, Ritesh Harjani, Avri Altman,
Arnd Bergmann, Christoph Hellwig, Jens Axboe, Paolo Valente,
Per Förlin
This week I tested the following:
- Merge the in-flight BFQ work from Paolo with my MMC MQ patch set
- Enable BFQ
- Run a few iterations of classic throughput tests
dd on whole internal eMMC, 7.38 GiB:
sync
echo 3 > /proc/sys/vm/drop_caches
sync
time dd if=/dev/mmcblk3 of=/dev/null
time dd if=/dev/mmcblk3 of=/dev/null bs=1M
iozone on a Noname SD card 2GB
mount /dev/mmcblk0p1 /mnt
sync
echo 3 > /proc/sys/vm/drop_caches
sync
iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
The results:
Before patches (v4.10-rc8):
7918845952 bytes (7.4GB) copied, 194.504059 seconds, 38.8MB/s
real 3m 14.51s
user 0m 7.41s
sys 1m 10.34s
7918845952 bytes (7.4GB) copied, 176.519531 seconds, 42.8MB/s
real 2m 56.53s
user 0m 0.06s
sys 0m 36.57s
Command line used: iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
Output is in kBytes/sec
random random
kB reclen write rewrite read reread read write
20480 4 1960 2105 5991 6023 5202 40
20480 8 4636 4901 9087 9103 9066 80
20480 16 5522 5663 12237 12242 12206 163
20480 32 5976 6031 14915 14917 14901 333
20480 64 6286 6387 16737 16763 16738 678
20480 128 6720 6757 17876 17857 17865 1403
20480 256 6846 6909 18230 17568 16719 3039
20480 512 7204 7229 18471 18751 18834 7209
20480 1024 7257 7315 18684 18044 18095 7337
20480 2048 7322 7388 18605 18802 19437 7401
20480 4096 7553 7652 21510 21108 21503 7688
20480 8192 7534 7745 22164 22300 22490 7758
20480 16384 7357 7818 23053 23048 23056 7834
After MMC MQ patches:
7918845952 bytes (7.4GB) copied, 196.907776 seconds, 38.4MB/s
real 3m 16.91s
user 0m 7.17s
sys 1m 8.03s
7918845952 bytes (7.4GB) copied, 192.595734 seconds, 39.2MB/s
real 3m 12.60s
user 0m 0.12s
sys 0m 33.11s
Command line used: iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
random random
kB reclen write rewrite read reread read write
20480 4 2049 2154 5991 5998 5934 40
20480 8 4654 4921 9081 9075 9028 81
20480 16 5572 5747 12250 12252 12177 164
20480 32 6040 6084 14858 14895 14833 335
20480 64 6370 6449 16759 16770 16715 682
20480 128 6834 6814 17882 17843 17878 1411
20480 256 6892 6900 18526 18105 18430 3066
20480 512 7239 7254 18839 18864 18837 7258
20480 1024 7342 6453 18787 18161 17522 7343
20480 2048 7408 7439 17891 18211 19029 7472
20480 4096 7641 7703 20950 21044 20900 7705
20480 8192 7584 7811 22261 22170 22385 7809
20480 16384 7407 7873 23033 23050 23048 7905
After MMC MQ+BFQ patches:
7918845952 bytes (7.4GB) copied, 197.097717 seconds, 38.3MB/s
real 3m 17.10s
user 0m 7.67s
sys 1m 7.33s
7552+0 records in
7552+0 records out
7918845952 bytes (7.4GB) copied, 187.119538 seconds, 40.4MB/s
real 3m 7.12s
user 0m 0.11s
sys 0m 34.61s
Command line used: iozone -az -i0 -i1 -i2 -s 20m -I -f /mnt/foo.test
Output is in kBytes/sec
random random
kB reclen write rewrite read reread read write
20480 4 1734 1786 5923 5166 5894 40
20480 8 4614 4853 8950 8949 8909 80
20480 16 5525 5705 12086 12098 12040 164
20480 32 6027 6040 14765 14793 14755 334
20480 64 6341 6404 16696 16697 16670 680
20480 128 6799 6842 17830 17833 17814 1407
20480 256 6848 6849 17394 18251 17537 3054
20480 512 7191 7229 18545 18628 18801 7224
20480 1024 7241 7331 17845 17909 18206 7302
20480 2048 7375 7433 18794 19288 19675 7426
20480 4096 7583 7696 21024 21194 21082 7659
20480 8192 7555 7767 22068 22170 22168 7808
20480 16384 7350 7831 23021 23032 23050 7870
As you can see there are no huge performance regressions with these
kinds of "raw" throughput tests.
These iozone figures are unintuitive unless your head can
plot logarithmic, look at the charts here for a more visual presentation
of the iozone results:
https://docs.google.com/spreadsheets/d/1rm72TiGlTnzDeGLR__aqvjcJ2UkA-Ro3-XyKA8r1M-c
Compare this to the performance change we got when first introducing
the asynchronous requests:
https://wiki.linaro.org/WorkingGroups/KernelArchived/Specs/StoragePerfMMC-async-req
The patches need some issues fixed from the build server
complaints and some robustness hammering, but after that I
think they will be ripe for merging for v4.12.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/2] block/sed-opal: allocate struct opal_dev dynamically
From: Johannes Thumshirn @ 2017-02-17 9:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: scott.bauer, keith.busch, jonathan.derrick, axboe, linux-block,
linux-nvme
In-Reply-To: <20170217091139.GA19393@lst.de>
On 02/17/2017 10:11 AM, Christoph Hellwig wrote:
> I just moved the existing definition around 1:1.
Just saw it's - anyways. So sorry for the noise and I'll grab another coffee
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 2/2] block/sed-opal: allocate struct opal_dev dynamically
From: Christoph Hellwig @ 2017-02-17 9:11 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Christoph Hellwig, scott.bauer, keith.busch, jonathan.derrick,
axboe, linux-block, linux-nvme
In-Reply-To: <10ec775b-cbdc-7cac-336b-015d97c486de@suse.de>
On Fri, Feb 17, 2017 at 09:58:27AM +0100, Johannes Thumshirn wrote:
> On 02/16/2017 09:10 PM, Christoph Hellwig wrote:
> > -enum opal_response_token {
> > - OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
> > - OPAL_DTA_TOKENID_SINT = 0xe1,
> > - OPAL_DTA_TOKENID_UINT = 0xe2,
> > - OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
> > - OPAL_DTA_TOKENID_INVALID = 0X0
>
> Nit, here's a capital 'X' that slipped in
I just moved the existing definition around 1:1.
^ permalink raw reply
* Re: [PATCH 2/2] block/sed-opal: allocate struct opal_dev dynamically
From: Johannes Thumshirn @ 2017-02-17 8:58 UTC (permalink / raw)
To: Christoph Hellwig, scott.bauer, keith.busch, jonathan.derrick,
axboe
Cc: linux-block, linux-nvme
In-Reply-To: <20170216201053.1190-2-hch@lst.de>
On 02/16/2017 09:10 PM, Christoph Hellwig wrote:
> -enum opal_response_token {
> - OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
> - OPAL_DTA_TOKENID_SINT = 0xe1,
> - OPAL_DTA_TOKENID_UINT = 0xe2,
> - OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
> - OPAL_DTA_TOKENID_INVALID = 0X0
Nit, here's a capital 'X' that slipped in
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 12/17] md: raid1: avoid direct access to bvec table in process_checks()
From: kbuild test robot @ 2017-02-17 8:33 UTC (permalink / raw)
To: Ming Lei
Cc: kbuild-all, Shaohua Li, Jens Axboe, linux-kernel, linux-raid,
linux-block, Christoph Hellwig, NeilBrown, Ming Lei
In-Reply-To: <1487245547-24384-13-git-send-email-tom.leiming@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6270 bytes --]
Hi Ming,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.10-rc8]
[cannot apply to next-20170216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ming-Lei/md-cleanup-on-direct-access-to-bvec-table/20170216-210357
config: powerpc-cell_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from arch/powerpc/include/asm/page.h:331:0,
from arch/powerpc/include/asm/thread_info.h:34,
from include/linux/thread_info.h:25,
from include/asm-generic/preempt.h:4,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:59,
from include/linux/spinlock.h:50,
from include/linux/mmzone.h:7,
from include/linux/gfp.h:5,
from include/linux/slab.h:14,
from drivers/md/raid1.c:34:
drivers/md/raid1.c: In function 'raid1d':
>> include/asm-generic/memory_model.h:54:52: warning: 'sbio_pages$' may be used uninitialized in this function [-Wmaybe-uninitialized]
#define __page_to_pfn(page) (unsigned long)((page) - vmemmap)
^
drivers/md/raid1.c:2008:42: note: 'sbio_pages$' was declared here
struct page *pbio_pages[RESYNC_PAGES], *sbio_pages[RESYNC_PAGES];
^~~~~~~~~~
drivers/md/raid1.c:2075:9: warning: 'page_len$' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (memcmp(page_address(p),
^~~~~~~~~~~~~~~~~~~~~~~
page_address(s),
~~~~~~~~~~~~~~~~
page_len[j]))
~~~~~~~~~~~~
drivers/md/raid1.c:2007:6: note: 'page_len$' was declared here
int page_len[RESYNC_PAGES];
^~~~~~~~
In file included from arch/powerpc/include/asm/page.h:331:0,
from arch/powerpc/include/asm/thread_info.h:34,
from include/linux/thread_info.h:25,
from include/asm-generic/preempt.h:4,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:59,
from include/linux/spinlock.h:50,
from include/linux/mmzone.h:7,
from include/linux/gfp.h:5,
from include/linux/slab.h:14,
from drivers/md/raid1.c:34:
>> include/asm-generic/memory_model.h:54:52: warning: 'pbio_pages$' may be used uninitialized in this function [-Wmaybe-uninitialized]
#define __page_to_pfn(page) (unsigned long)((page) - vmemmap)
^
drivers/md/raid1.c:2008:15: note: 'pbio_pages$' was declared here
struct page *pbio_pages[RESYNC_PAGES], *sbio_pages[RESYNC_PAGES];
^~~~~~~~~~
drivers/md/raid1.c:1978:8: warning: 'pages$' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (r1_sync_page_io(rdev, sect, s,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pages[idx],
~~~~~~~~~~~
READ) != 0)
~~~~~
drivers/md/raid1.c:1872:15: note: 'pages$' was declared here
struct page *pages[RESYNC_PAGES];
^~~~~
vim +54 include/asm-generic/memory_model.h
a117e66e KAMEZAWA Hiroyuki 2006-03-27 38 ({ unsigned long __pfn = (pfn); \
c5d71243 Rafael J. Wysocki 2008-11-08 39 unsigned long __nid = arch_pfn_to_nid(__pfn); \
a117e66e KAMEZAWA Hiroyuki 2006-03-27 40 NODE_DATA(__nid)->node_mem_map + arch_local_page_offset(__pfn, __nid);\
a117e66e KAMEZAWA Hiroyuki 2006-03-27 41 })
a117e66e KAMEZAWA Hiroyuki 2006-03-27 42
67de6482 Andy Whitcroft 2006-06-23 43 #define __page_to_pfn(pg) \
aa462abe Ian Campbell 2011-08-17 44 ({ const struct page *__pg = (pg); \
a0140c1d KAMEZAWA Hiroyuki 2006-03-27 45 struct pglist_data *__pgdat = NODE_DATA(page_to_nid(__pg)); \
a0140c1d KAMEZAWA Hiroyuki 2006-03-27 46 (unsigned long)(__pg - __pgdat->node_mem_map) + \
a0140c1d KAMEZAWA Hiroyuki 2006-03-27 47 __pgdat->node_start_pfn; \
a117e66e KAMEZAWA Hiroyuki 2006-03-27 48 })
a117e66e KAMEZAWA Hiroyuki 2006-03-27 49
8f6aac41 Christoph Lameter 2007-10-16 50 #elif defined(CONFIG_SPARSEMEM_VMEMMAP)
8f6aac41 Christoph Lameter 2007-10-16 51
af901ca1 Andr� Goddard Rosa 2009-11-14 52 /* memmap is virtually contiguous. */
8f6aac41 Christoph Lameter 2007-10-16 53 #define __pfn_to_page(pfn) (vmemmap + (pfn))
32272a26 Martin Schwidefsky 2008-12-25 @54 #define __page_to_pfn(page) (unsigned long)((page) - vmemmap)
8f6aac41 Christoph Lameter 2007-10-16 55
a117e66e KAMEZAWA Hiroyuki 2006-03-27 56 #elif defined(CONFIG_SPARSEMEM)
a117e66e KAMEZAWA Hiroyuki 2006-03-27 57 /*
1a49123b Zhang Yanfei 2013-10-03 58 * Note: section's mem_map is encoded to reflect its start_pfn.
a117e66e KAMEZAWA Hiroyuki 2006-03-27 59 * section[i].section_mem_map == mem_map's address - start_pfn;
a117e66e KAMEZAWA Hiroyuki 2006-03-27 60 */
67de6482 Andy Whitcroft 2006-06-23 61 #define __page_to_pfn(pg) \
aa462abe Ian Campbell 2011-08-17 62 ({ const struct page *__pg = (pg); \
:::::: The code at line 54 was first introduced by commit
:::::: 32272a26974d2027384fd4010cd1780fca425d94 [S390] __page_to_pfn warnings
:::::: TO: Martin Schwidefsky <schwidefsky@de.ibm.com>
:::::: CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18602 bytes --]
^ permalink raw reply
* Re: [PATCH 00/17] md: cleanup on direct access to bvec table
From: Shaohua Li @ 2017-02-17 4:16 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <CACVXFVPRuikOn=zJ5fboGMyQs3rMxaUK4f-Lk0s-+HpPy+ov8w@mail.gmail.com>
On Fri, Feb 17, 2017 at 09:25:27AM +0800, Ming Lei wrote:
> Hi Shaohua,
>
> On Fri, Feb 17, 2017 at 6:16 AM, Shaohua Li <shli@kernel.org> wrote:
> > On Thu, Feb 16, 2017 at 07:45:30PM +0800, Ming Lei wrote:
> >> In MD's resync I/O path, there are lots of direct access to bio's
> >> bvec table. This patchset kills most of them, and the conversion
> >> is quite straightforward.
> >
> > I don't like this approach. The MD uses a hacky way to manage pages allocated,
> > this is the root of the problem. The patches add another hack way to do the
>
> Yes, I agree, and bio_iov_iter_get_pages() uses this kind of hacky way too
> actually.
>
> > management. I'd like to see explict management of the pages, for example, add
> > data structure in r1bio to manage the pages, then we can use existing API for
> > all the stuffes we need.
>
> Yeah, that is definitely clean, but we have to pay the following cost:
>
> - allocate at least N * (128 + 4) bytes per each r1_bio/r10_bio
> - N is pool_info.raid_disks for raid1, and conf->copies for raid10
>
> If we are happy to introduce the cost, I can take this way in V1.
It's not a big deal. The inflight bio shouldn't be big, so the r1_bio count
isn't big. We don't waste much.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 00/17] md: cleanup on direct access to bvec table
From: Ming Lei @ 2017-02-17 1:25 UTC (permalink / raw)
To: Shaohua Li
Cc: Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <20170216221642.sobqndndd7fbjoo7@kernel.org>
Hi Shaohua,
On Fri, Feb 17, 2017 at 6:16 AM, Shaohua Li <shli@kernel.org> wrote:
> On Thu, Feb 16, 2017 at 07:45:30PM +0800, Ming Lei wrote:
>> In MD's resync I/O path, there are lots of direct access to bio's
>> bvec table. This patchset kills most of them, and the conversion
>> is quite straightforward.
>
> I don't like this approach. The MD uses a hacky way to manage pages allocated,
> this is the root of the problem. The patches add another hack way to do the
Yes, I agree, and bio_iov_iter_get_pages() uses this kind of hacky way too
actually.
> management. I'd like to see explict management of the pages, for example, add
> data structure in r1bio to manage the pages, then we can use existing API for
> all the stuffes we need.
Yeah, that is definitely clean, but we have to pay the following cost:
- allocate at least N * (128 + 4) bytes per each r1_bio/r10_bio
- N is pool_info.raid_disks for raid1, and conf->copies for raid10
If we are happy to introduce the cost, I can take this way in V1.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 00/17] md: cleanup on direct access to bvec table
From: Shaohua Li @ 2017-02-16 22:16 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1487245547-24384-1-git-send-email-tom.leiming@gmail.com>
On Thu, Feb 16, 2017 at 07:45:30PM +0800, Ming Lei wrote:
> In MD's resync I/O path, there are lots of direct access to bio's
> bvec table. This patchset kills most of them, and the conversion
> is quite straightforward.
I don't like this approach. The MD uses a hacky way to manage pages allocated,
this is the root of the problem. The patches add another hack way to do the
management. I'd like to see explict management of the pages, for example, add
data structure in r1bio to manage the pages, then we can use existing API for
all the stuffes we need.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH] opal: Use empty structure when not defined
From: Scott Bauer @ 2017-02-16 20:52 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, linux-block, Jonathan Derrick, Jens Axboe
In-Reply-To: <20170216200708.GA16140@infradead.org>
On Thu, Feb 16, 2017 at 12:07:08PM -0800, Christoph Hellwig wrote:
> On Thu, Feb 16, 2017 at 11:45:29AM -0700, Scott Bauer wrote:
> > > + if (check_opal_support(dev) < 0) {
> > > pr_warn("Opal is not supported on this device\n");
> > > - opal_dev->initialized = true;
> > > + kfree(dev);
> > > + return NULL;
> >
> > We're going to have to change this check_opal_support to be != 0 instead of < 0.
>
> Yes. And we should simply turn all these printk into pr_debug anway -
> not having OPAL is a prefectly fine condition, no need to spam the log
> for it.
>
> And btw, I think we should check for bit 0 in OACS before ever doing
> a security send / receive.
That sounds reasonable. It can go in the nvme_send_recv function before we
actually attempt to send anything. The code paths fall nicely in that when
we attempt to look for support we have to do a discovery0 which will fail
on the OACS bit and we'll deallocate everything.
I'll spin this up now.
^ permalink raw reply
* Re: [PATCH 2/2] block/sed-opal: allocate struct opal_dev dynamically
From: Scott Bauer @ 2017-02-16 20:47 UTC (permalink / raw)
To: Christoph Hellwig
Cc: keith.busch, jonathan.derrick, axboe, linux-block, linux-nvme
In-Reply-To: <20170216201053.1190-2-hch@lst.de>
On Thu, Feb 16, 2017 at 09:10:53PM +0100, Christoph Hellwig wrote:
> Insted of bloating the containing structure with it all the time this
> allocates struct opal_dev dynamically. Additionally this allows moving
> the definition of struct opal_dev into sed-opal.c. For this a new
> private data field is added to it that is passed to the send/receive
> callback. After that a lot of internals can be made private as well.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Don't know if we need both but:
Tested-by: Scott Bauer <scott.bauer@intel.com>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>
^ permalink raw reply
* Re: [PATCH 1/2] block/sed-opal: tone down not supported warnings
From: Scott Bauer @ 2017-02-16 20:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: keith.busch, jonathan.derrick, axboe, linux-block, linux-nvme
In-Reply-To: <20170216201053.1190-1-hch@lst.de>
On Thu, Feb 16, 2017 at 09:10:52PM +0100, Christoph Hellwig wrote:
> Not having OPAL or a sub-feature supported is an entirely normal
> condition for many drives, so don't warn about it. Keep the messages,
> but tone them down to debug only.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>
^ permalink raw reply
* [PATCH 2/2] block/sed-opal: allocate struct opal_dev dynamically
From: Christoph Hellwig @ 2017-02-16 20:10 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170216201053.1190-1-hch@lst.de>
Insted of bloating the containing structure with it all the time this
allocates struct opal_dev dynamically. Additionally this allows moving
the definition of struct opal_dev into sed-opal.c. For this a new
private data field is added to it that is passed to the send/receive
callback. After that a lot of internals can be made private as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/opal_proto.h | 23 ++++++++++
block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++----
drivers/nvme/host/core.c | 9 ++--
drivers/nvme/host/nvme.h | 14 ++----
drivers/nvme/host/pci.c | 8 +++-
include/linux/sed-opal.h | 116 ++---------------------------------------------
6 files changed, 131 insertions(+), 140 deletions(-)
diff --git a/block/opal_proto.h b/block/opal_proto.h
index af9abc56c157..f40c9acf8895 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -19,6 +19,29 @@
#ifndef _OPAL_PROTO_H
#define _OPAL_PROTO_H
+/*
+ * These constant values come from:
+ * SPC-4 section
+ * 6.30 SECURITY PROTOCOL IN command / table 265.
+ */
+enum {
+ TCG_SECP_00 = 0,
+ TCG_SECP_01,
+};
+
+/*
+ * Token defs derived from:
+ * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
+ * 3.2.2 Data Stream Encoding
+ */
+enum opal_response_token {
+ OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
+ OPAL_DTA_TOKENID_SINT = 0xe1,
+ OPAL_DTA_TOKENID_UINT = 0xe2,
+ OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
+ OPAL_DTA_TOKENID_INVALID = 0X0
+};
+
#define DTAERROR_NO_METHOD_STATUS 0x89
#define GENERIC_HOST_SESSION_NUM 0x41
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bcdd5b6d02e8..d1c52ba4d62d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -31,6 +31,77 @@
#include "opal_proto.h"
+#define IO_BUFFER_LENGTH 2048
+#define MAX_TOKS 64
+
+typedef int (*opal_step)(struct opal_dev *dev);
+
+enum opal_atom_width {
+ OPAL_WIDTH_TINY,
+ OPAL_WIDTH_SHORT,
+ OPAL_WIDTH_MEDIUM,
+ OPAL_WIDTH_LONG,
+ OPAL_WIDTH_TOKEN
+};
+
+/*
+ * On the parsed response, we don't store again the toks that are already
+ * stored in the response buffer. Instead, for each token, we just store a
+ * pointer to the position in the buffer where the token starts, and the size
+ * of the token in bytes.
+ */
+struct opal_resp_tok {
+ const u8 *pos;
+ size_t len;
+ enum opal_response_token type;
+ enum opal_atom_width width;
+ union {
+ u64 u;
+ s64 s;
+ } stored;
+};
+
+/*
+ * From the response header it's not possible to know how many tokens there are
+ * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
+ * if we start dealing with messages that have more than that, we can increase
+ * this number. This is done to avoid having to make two passes through the
+ * response, the first one counting how many tokens we have and the second one
+ * actually storing the positions.
+ */
+struct parsed_resp {
+ int num;
+ struct opal_resp_tok toks[MAX_TOKS];
+};
+
+struct opal_dev {
+ bool supported;
+
+ void *data;
+ sec_send_recv *send_recv;
+
+ const opal_step *funcs;
+ void **func_data;
+ int state;
+ struct mutex dev_lock;
+ u16 comid;
+ u32 hsn;
+ u32 tsn;
+ u64 align;
+ u64 lowest_lba;
+
+ size_t pos;
+ u8 cmd[IO_BUFFER_LENGTH];
+ u8 resp[IO_BUFFER_LENGTH];
+
+ struct parsed_resp parsed;
+ size_t prev_d_len;
+ void *prev_data;
+
+ struct list_head unlk_lst;
+};
+
+
static const u8 opaluid[][OPAL_UID_LENGTH] = {
/* users */
[OPAL_SMUID_UID] =
@@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data)
static int opal_send_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->cmd, IO_BUFFER_LENGTH,
true);
}
static int opal_recv_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->resp, IO_BUFFER_LENGTH,
false);
}
@@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev)
return ret;
}
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
{
- if (opal_dev->initialized)
- return;
- INIT_LIST_HEAD(&opal_dev->unlk_lst);
- mutex_init(&opal_dev->dev_lock);
- opal_dev->send_recv = send_recv;
- if (check_opal_support(opal_dev) < 0)
+ struct opal_dev *dev;
+
+ dev = kmalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return NULL;
+
+ INIT_LIST_HEAD(&dev->unlk_lst);
+ mutex_init(&dev->dev_lock);
+ dev->data = data;
+ dev->send_recv = send_recv;
+ if (check_opal_support(dev) != 0) {
pr_debug("Opal is not supported on this device\n");
- opal_dev->initialized = true;
+ kfree(dev);
+ return NULL;
+ }
+ return dev;
}
EXPORT_SYMBOL(init_opal_dev);
@@ -2351,6 +2430,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
+ if (!dev)
+ return -ENOTSUPP;
if (!dev->supported) {
pr_err("Not supported\n");
return -ENOTSUPP;
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 04c48e75ff16..8aeb4a623b65 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return nvme_nvm_ioctl(ns, cmd, arg);
#endif
if (is_sed_ioctl(cmd))
- return sed_ioctl(&ns->ctrl->opal_dev, cmd,
+ return sed_ioctl(ns->ctrl->opal_dev, cmd,
(void __user *) arg);
return -ENOTTY;
}
@@ -1085,18 +1085,17 @@ static const struct pr_ops nvme_pr_ops = {
};
#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send)
{
+ struct nvme_ctrl *ctrl = data;
struct nvme_command cmd;
- struct nvme_ctrl *ctrl = NULL;
memset(&cmd, 0, sizeof(cmd));
if (send)
cmd.common.opcode = nvme_admin_security_send;
else
cmd.common.opcode = nvme_admin_security_recv;
- ctrl = container_of(dev, struct nvme_ctrl, opal_dev);
cmd.common.nsid = 0;
cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
cmd.common.cdw10[1] = cpu_to_le32(len);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 569cba14cede..5126c4bbee1a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -126,7 +126,7 @@ struct nvme_ctrl {
struct list_head node;
struct ida ns_ida;
- struct opal_dev opal_dev;
+ struct opal_dev *opal_dev;
char name[12];
char serial[20];
@@ -270,16 +270,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl);
void nvme_queue_scan(struct nvme_ctrl *ctrl);
void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
-#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-#else
-static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
-{
- return 0;
-}
-#endif /* CONFIG_BLK_DEV_SED_OPAL */
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send);
#define NVME_NR_AERS 1
void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e25d632bd9f2..5db8a38a8b43 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
if (dev->ctrl.admin_q)
blk_put_queue(dev->ctrl.admin_q);
kfree(dev->queues);
+ kfree(dev->ctrl.opal_dev);
kfree(dev);
}
@@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit);
+ if (!dev->ctrl.opal_dev) {
+ dev->ctrl.opal_dev =
+ init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ }
if (was_suspend)
- opal_unlock_from_suspend(&dev->ctrl.opal_dev);
+ opal_unlock_from_suspend(dev->ctrl.opal_dev);
result = nvme_setup_io_queues(dev);
if (result)
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index 205d520ea688..deee23d012e7 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -21,117 +21,14 @@
#include <uapi/linux/sed-opal.h>
#include <linux/kernel.h>
-/*
- * These constant values come from:
- * SPC-4 section
- * 6.30 SECURITY PROTOCOL IN command / table 265.
- */
-enum {
- TCG_SECP_00 = 0,
- TCG_SECP_01,
-};
struct opal_dev;
-#define IO_BUFFER_LENGTH 2048
-#define MAX_TOKS 64
-
-typedef int (*opal_step)(struct opal_dev *dev);
-typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-
-
-enum opal_atom_width {
- OPAL_WIDTH_TINY,
- OPAL_WIDTH_SHORT,
- OPAL_WIDTH_MEDIUM,
- OPAL_WIDTH_LONG,
- OPAL_WIDTH_TOKEN
-};
-
-/*
- * Token defs derived from:
- * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
- * 3.2.2 Data Stream Encoding
- */
-enum opal_response_token {
- OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
- OPAL_DTA_TOKENID_SINT = 0xe1,
- OPAL_DTA_TOKENID_UINT = 0xe2,
- OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
- OPAL_DTA_TOKENID_INVALID = 0X0
-};
-
-/*
- * On the parsed response, we don't store again the toks that are already
- * stored in the response buffer. Instead, for each token, we just store a
- * pointer to the position in the buffer where the token starts, and the size
- * of the token in bytes.
- */
-struct opal_resp_tok {
- const u8 *pos;
- size_t len;
- enum opal_response_token type;
- enum opal_atom_width width;
- union {
- u64 u;
- s64 s;
- } stored;
-};
-
-/*
- * From the response header it's not possible to know how many tokens there are
- * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
- * if we start dealing with messages that have more than that, we can increase
- * this number. This is done to avoid having to make two passes through the
- * response, the first one counting how many tokens we have and the second one
- * actually storing the positions.
- */
-struct parsed_resp {
- int num;
- struct opal_resp_tok toks[MAX_TOKS];
-};
-
-/**
- * struct opal_dev - The structure representing a OPAL enabled SED.
- * @supported: Whether or not OPAL is supported on this controller.
- * @send_recv: The combined sec_send/sec_recv function pointer.
- * @opal_step: A series of opal methods that are necessary to complete a command.
- * @func_data: An array of parameters for the opal methods above.
- * @state: Describes the current opal_step we're working on.
- * @dev_lock: Locks the entire opal_dev structure.
- * @parsed: Parsed response from controller.
- * @prev_data: Data returned from a method to the controller.
- * @unlk_lst: A list of Locking ranges to unlock on this device during a resume.
- */
-struct opal_dev {
- bool initialized;
- bool supported;
- sec_send_recv *send_recv;
-
- const opal_step *funcs;
- void **func_data;
- int state;
- struct mutex dev_lock;
- u16 comid;
- u32 hsn;
- u32 tsn;
- u64 align;
- u64 lowest_lba;
-
- size_t pos;
- u8 cmd[IO_BUFFER_LENGTH];
- u8 resp[IO_BUFFER_LENGTH];
-
- struct parsed_resp parsed;
- size_t prev_d_len;
- void *prev_data;
-
- struct list_head unlk_lst;
-};
+typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer,
+ size_t len, bool send);
#ifdef CONFIG_BLK_SED_OPAL
bool opal_unlock_from_suspend(struct opal_dev *dev);
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv);
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr);
static inline bool is_sed_ioctl(unsigned int cmd)
@@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev)
{
return false;
}
-static inline void init_opal_dev(struct opal_dev *opal_dev,
- sec_send_recv *send_recv)
-{
- opal_dev->supported = false;
- opal_dev->initialized = true;
-}
+#define init_opal_dev(data, send_recv) NULL
#endif /* CONFIG_BLK_SED_OPAL */
#endif /* LINUX_OPAL_H */
--
2.11.0
^ permalink raw reply related
* [PATCH 1/2] block/sed-opal: tone down not supported warnings
From: Christoph Hellwig @ 2017-02-16 20:10 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
Not having OPAL or a sub-feature supported is an entirely normal
condition for many drives, so don't warn about it. Keep the messages,
but tone them down to debug only.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/sed-opal.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index e95b8a57053d..bcdd5b6d02e8 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -387,16 +387,16 @@ static int opal_discovery0_end(struct opal_dev *dev)
}
if (!supported) {
- pr_err("This device is not Opal enabled. Not Supported!\n");
+ pr_debug("This device is not Opal enabled. Not Supported!\n");
return -EOPNOTSUPP;
}
if (!single_user)
- pr_warn("Device doesn't support single user mode\n");
+ pr_debug("Device doesn't support single user mode\n");
if (!found_com_id) {
- pr_warn("Could not find OPAL comid for device. Returning early\n");
+ pr_debug("Could not find OPAL comid for device. Returning early\n");
return -EOPNOTSUPP;;
}
@@ -1951,7 +1951,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
mutex_init(&opal_dev->dev_lock);
opal_dev->send_recv = send_recv;
if (check_opal_support(opal_dev) < 0)
- pr_warn("Opal is not supported on this device\n");
+ pr_debug("Opal is not supported on this device\n");
opal_dev->initialized = true;
}
EXPORT_SYMBOL(init_opal_dev);
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] opal: Use empty structure when not defined
From: Christoph Hellwig @ 2017-02-16 20:07 UTC (permalink / raw)
To: Scott Bauer
Cc: Christoph Hellwig, Keith Busch, linux-block, Jonathan Derrick,
Jens Axboe
In-Reply-To: <20170216184528.GA3899@sbauer-Z170X-UD5>
On Thu, Feb 16, 2017 at 11:45:29AM -0700, Scott Bauer wrote:
> > + if (check_opal_support(dev) < 0) {
> > pr_warn("Opal is not supported on this device\n");
> > - opal_dev->initialized = true;
> > + kfree(dev);
> > + return NULL;
>
> We're going to have to change this check_opal_support to be != 0 instead of < 0.
Yes. And we should simply turn all these printk into pr_debug anway -
not having OPAL is a prefectly fine condition, no need to spam the log
for it.
And btw, I think we should check for bit 0 in OACS before ever doing
a security send / receive.
^ permalink raw reply
* Re: [PATCH] opal: Use empty structure when not defined
From: Scott Bauer @ 2017-02-16 18:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, linux-block, Jonathan Derrick, Jens Axboe
In-Reply-To: <20170216075812.GA7662@infradead.org>
On Wed, Feb 15, 2017 at 11:58:12PM -0800, Christoph Hellwig wrote:
> I'd rather prefer to make the structure separately allocated as
> discussed before. Scott, can you test the patch below? I'm not near
> my devices I could test on.
>
> ---
> From b2cda0c7ec5c0ec66582655751838f519cfa1706 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 16 Feb 2017 08:49:56 +0100
> Subject: block/sed-opal: make struct opal_dev private
>
> This moves the definition of struct opal_dev into sed-opal.c. For this a
> new private data field is added to it that is passed to the send/receive
> callback. After that a lot of internals can be made private.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/opal_proto.h | 23 ++++++++++
> block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++----
> drivers/nvme/host/core.c | 9 ++--
> drivers/nvme/host/nvme.h | 14 ++----
> drivers/nvme/host/pci.c | 8 +++-
> include/linux/sed-opal.h | 116 ++---------------------------------------------
> 6 files changed, 131 insertions(+), 140 deletions(-)
>
> diff --git a/block/opal_proto.h b/block/opal_proto.h
> index af9abc56c157..f40c9acf8895 100644
> --- a/block/opal_proto.h
> +++ b/block/opal_proto.h
> @@ -19,6 +19,29 @@
> #ifndef _OPAL_PROTO_H
> #define _OPAL_PROTO_H
>
> +/*
> + * These constant values come from:
> + * SPC-4 section
> + * 6.30 SECURITY PROTOCOL IN command / table 265.
> + */
> +enum {
> + TCG_SECP_00 = 0,
> + TCG_SECP_01,
> +};
> +
> +/*
> + * Token defs derived from:
> + * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
> + * 3.2.2 Data Stream Encoding
> + */
> +enum opal_response_token {
> + OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
> + OPAL_DTA_TOKENID_SINT = 0xe1,
> + OPAL_DTA_TOKENID_UINT = 0xe2,
> + OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
> + OPAL_DTA_TOKENID_INVALID = 0X0
> +};
> +
> #define DTAERROR_NO_METHOD_STATUS 0x89
> #define GENERIC_HOST_SESSION_NUM 0x41
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e5159b..bdab4dfcbafd 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -31,6 +31,77 @@
>
> #include "opal_proto.h"
>
> +#define IO_BUFFER_LENGTH 2048
> +#define MAX_TOKS 64
> +
> +typedef int (*opal_step)(struct opal_dev *dev);
> +
> +enum opal_atom_width {
> + OPAL_WIDTH_TINY,
> + OPAL_WIDTH_SHORT,
> + OPAL_WIDTH_MEDIUM,
> + OPAL_WIDTH_LONG,
> + OPAL_WIDTH_TOKEN
> +};
> +
> +/*
> + * On the parsed response, we don't store again the toks that are already
> + * stored in the response buffer. Instead, for each token, we just store a
> + * pointer to the position in the buffer where the token starts, and the size
> + * of the token in bytes.
> + */
> +struct opal_resp_tok {
> + const u8 *pos;
> + size_t len;
> + enum opal_response_token type;
> + enum opal_atom_width width;
> + union {
> + u64 u;
> + s64 s;
> + } stored;
> +};
> +
> +/*
> + * From the response header it's not possible to know how many tokens there are
> + * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
> + * if we start dealing with messages that have more than that, we can increase
> + * this number. This is done to avoid having to make two passes through the
> + * response, the first one counting how many tokens we have and the second one
> + * actually storing the positions.
> + */
> +struct parsed_resp {
> + int num;
> + struct opal_resp_tok toks[MAX_TOKS];
> +};
> +
> +struct opal_dev {
> + bool supported;
> +
> + void *data;
> + sec_send_recv *send_recv;
> +
> + const opal_step *funcs;
> + void **func_data;
> + int state;
> + struct mutex dev_lock;
> + u16 comid;
> + u32 hsn;
> + u32 tsn;
> + u64 align;
> + u64 lowest_lba;
> +
> + size_t pos;
> + u8 cmd[IO_BUFFER_LENGTH];
> + u8 resp[IO_BUFFER_LENGTH];
> +
> + struct parsed_resp parsed;
> + size_t prev_d_len;
> + void *prev_data;
> +
> + struct list_head unlk_lst;
> +};
> +
> +
> static const u8 opaluid[][OPAL_UID_LENGTH] = {
> /* users */
> [OPAL_SMUID_UID] =
> @@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data)
>
> static int opal_send_cmd(struct opal_dev *dev)
> {
> - return dev->send_recv(dev, dev->comid, TCG_SECP_01,
> + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
> dev->cmd, IO_BUFFER_LENGTH,
> true);
> }
>
> static int opal_recv_cmd(struct opal_dev *dev)
> {
> - return dev->send_recv(dev, dev->comid, TCG_SECP_01,
> + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
> dev->resp, IO_BUFFER_LENGTH,
> false);
> }
> @@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev)
> return ret;
> }
>
> -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
> +struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
> {
> - if (opal_dev->initialized)
> - return;
> - INIT_LIST_HEAD(&opal_dev->unlk_lst);
> - mutex_init(&opal_dev->dev_lock);
> - opal_dev->send_recv = send_recv;
> - if (check_opal_support(opal_dev) < 0)
> + struct opal_dev *dev;
> +
> + dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return NULL;
> +
> + INIT_LIST_HEAD(&dev->unlk_lst);
> + mutex_init(&dev->dev_lock);
> + dev->data = data;
> + dev->send_recv = send_recv;
> + if (check_opal_support(dev) < 0) {
> pr_warn("Opal is not supported on this device\n");
> - opal_dev->initialized = true;
> + kfree(dev);
> + return NULL;
We're going to have to change this check_opal_support to be != 0 instead of < 0.
I tested on a controller that does not have opal enabled and I get a kick back of:
[ 112.296675] sed_opal:OPAL: Error on step function: 0 with error 1: Not Authorized
[ 112.306242] nvme1n1: p1 p2 p3
So we return the error 1 out of check_opal_support, and we'll never free the opal_dev.
There isnt any issues with potential crashes or other weird behavior
because we set dev->supported to be false, so if you try and call an ioctl on the
unsuported device you'll get a:
[ 149.550024] sed_opal:OPAL: Not supported
but the memory is still there.
Also, since init_opal_dev gets called from reset_work any time we do that we'll
spam the user with that pr_warn, is there a pr_warn_once or something we can use
instead?
I looked at the rest of the pr_warns and there is one in discovery0_end that we'll
want to convert to a pr_warn_once as well:
if (!single_user)
pr_warn("Device doesn't support single user mode\n");
Since we use discovery0 to get a comid every command we run on a non SUM device
will have that spammed to their dmesg.
Other than the above it looks fine to me.
^ permalink raw reply
* Re: [PATCH] opal: Use empty structure when not defined
From: Scott Bauer @ 2017-02-16 17:39 UTC (permalink / raw)
To: Jon Derrick; +Cc: Christoph Hellwig, Keith Busch, linux-block, Jens Axboe
In-Reply-To: <20170216173755.GA3204@sbauer-Z170X-UD5>
On Thu, Feb 16, 2017 at 10:37:55AM -0700, Scott Bauer wrote:
> On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> > It looks good to me at first glance but I can't apply it. What tree are
> > you on?
> >
> > On 02/16/2017 12:58 AM, Christoph Hellwig wrote:
> > > I'd rather prefer to make the structure separately allocated as
> > > discussed before. Scott, can you test the patch below? I'm not near
> > > my devices I could test on.
> > >
>
> He doesn't have the most recent changes with the uapi IOW fixes. I got it applied
> and tested and it works. Going to review it more aggressively now. We had something
> very similar in one of the previous patches before we switched off of using a sed_context
> struct.
>
> If you do want to apply and test I did the following:
> 1) Pull Jens' for-next
> 2) reset fb2a77e4a25ef63ff5b51b3bd53027077b402b0d --hard
> 3) apply his patch
> 4) git pull https://kernel.googlesource.com/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
> 5) resolve the 2 small conflicts in core.c and sed-opal.h
> in sed-opal.h take his init_opal_dev change and the newer sed_ioctl
> in core.c take his change but change arg to be (void __user *) arg);
6) pull the newest commit from https://github.com/ScottyBauer/sed-opal-temp to get the uapi header change
^ permalink raw reply
* Re: [PATCH] opal: Use empty structure when not defined
From: Scott Bauer @ 2017-02-16 17:37 UTC (permalink / raw)
To: Jon Derrick; +Cc: Christoph Hellwig, Keith Busch, linux-block, Jens Axboe
In-Reply-To: <84b78f2d-0238-0fa1-e0b3-d46e7d4cb9be@intel.com>
On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> It looks good to me at first glance but I can't apply it. What tree are
> you on?
>
> On 02/16/2017 12:58 AM, Christoph Hellwig wrote:
> > I'd rather prefer to make the structure separately allocated as
> > discussed before. Scott, can you test the patch below? I'm not near
> > my devices I could test on.
> >
He doesn't have the most recent changes with the uapi IOW fixes. I got it applied
and tested and it works. Going to review it more aggressively now. We had something
very similar in one of the previous patches before we switched off of using a sed_context
struct.
If you do want to apply and test I did the following:
1) Pull Jens' for-next
2) reset fb2a77e4a25ef63ff5b51b3bd53027077b402b0d --hard
3) apply his patch
4) git pull https://kernel.googlesource.com/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
5) resolve the 2 small conflicts in core.c and sed-opal.h
in sed-opal.h take his init_opal_dev change and the newer sed_ioctl
in core.c take his change but change arg to be (void __user *) arg);
^ permalink raw reply
* Re: [PATCH] opal: Use empty structure when not defined
From: Christoph Hellwig @ 2017-02-16 17:21 UTC (permalink / raw)
To: Jon Derrick
Cc: Christoph Hellwig, Keith Busch, linux-block, Scott Bauer,
Jens Axboe
In-Reply-To: <84b78f2d-0238-0fa1-e0b3-d46e7d4cb9be@intel.com>
On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> It looks good to me at first glance but I can't apply it. What tree are
> you on?
It's against Jens' for-next tree (actually from two days ago, but
nothing in this area should have changed since then)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox