RE: [RFC 0/6] scsi: scsi transport for ufs devices
> -Original Message- > From: linux-scsi-ow...@vger.kernel.org > On Behalf Of Douglas Gilbert > Sent: Monday, July 30, 2018 9:53 PM > To: Hannes Reinecke ; Bart Van Assche > ; h...@lst.de; j...@linux.vnet.ibm.com; linux- > s...@vger.kernel.org; jthumsh...@suse.de; martin.peter...@oracle.com; > Avri Altman > Cc: Vinayak Holikatti ; Avi Shchislowski > ; Alex Lemberg ; > Stanislav Nijnikov ; subha...@codeaurora.org > Subject: Re: [RFC 0/6] scsi: scsi transport for ufs devices > > On 2018-07-30 02:13 PM, Hannes Reinecke wrote: > > On 07/30/2018 08:01 PM, Bart Van Assche wrote: > >> On Sun, 2018-07-29 at 16:33 +0300, Avri Altman wrote: > >>> Here is a proposal to use the scsi transport subsystem to manage > >>> ufs devices. > >>> > >>> scsi transport is a framework that allow to send scsi commands to > >>> a non-scsi devices. Still, it is flexible enough to allow > >>> sending non-scsi commands as well. We will use this framework to > >>> manage ufs devices by sending UPIU transactions. > >>> > >>> We added a new scsi transport module, a ufs-bsg LLD companion, > >>> and some new API to the ufs driver. > >> > >> My understanding is that all upstream code uses the bsg interface for > *SCSI* > >> commands. Sending UPIU commands over a bsg interface seems like > abuse of that > >> interface to me. Aren't you opening a can of worms with such an > approach? > >> > > I beg to disagree. > > > > bsg was precisely designed to handle non-SCSI commands, as this was the > main > > limitation of the original 'sg' interface. > > The original intention was to allow sending of transport frames for the > various > > SCSI transports (like FC or SAS), but there is no direct requirement for > > bsg to > > be limited to SCSI. > > Quite the contrary. > > I designed 'struct sg_io_v4' found in include/uapi/linux/bsg.h to handle > storage > related protocols, not just the SCSI command set. After the guard, the next > two fields in that structure are: > __u32 protocol; /* [i] 0 -> SCSI , */ > __u32 subprotocol; /* [i] 0 -> SCSI command, 1 -> SCSI task > management function, */ > > So there is lots of room for other protocols. Also the naming of fields is in > terms of request, response, data-in and data-out rather than SCSI command > specific terms (e.g. SCSI sense data maps to the response, while the SCSI > status is returned via a layered error mechanism). It also handles bidi data > transfers (which the sg_io_v3 interface does not). > > > NVMe didn't exist when struct sg_io_v4 was designed. If it was then I would > have made provisions for "metadata" transfers. One use for metadata > transfer might be to send protection information separately (i.e. as > metadata) rather than interleave with the user data as SCSI does. Is > NVMe metadata much used? And the extreme case: > bidi_data+bidi_metadata, > any examples of that? > > Doug Gilbert OK then. Let's give it a try. Will re-send it as patchset. Thanks, Avri
[PATCH 1/6] scsi: Add ufs transport class
Scsi transport is a framework that allow to send scsi commands to
a non-scsi devices. Still, it is flexible enough to allow
sending non-scsi commands as well. We will use this framework to
manage ufs devices by sending UPIU transactions.
In addition to the basic SCSI core objects this transport class
introduces two additional (currently empty) class objects:
“ufs-host” and “ufs-port”. There is only one “ufs-host” in the
system, but can be more-than-one “ufs-ports”.
Those classes are left empty for now, as ufs-sysfs already contains
an abundant amount of attributes.
A “ufs-port” is purely a software object. Evidently, the function
template takes no port as an argument, as the driver has no concept
of "port". We only need it as a hanging point in the bsg device tree,
so maybe a more appropriate term would be: "ufs-bsg-dev-node".
The ufs-port has a pretty lean structure. This is because we are
using upiu transactions that contains the outmost detailed info,
so we don't really need complex constructs to support it.
The transport will keep track of its ufs-ports via its scsi host.
Signed-off-by: Avri Altman
---
drivers/scsi/Kconfig | 13 ++
drivers/scsi/Makefile | 1 +
drivers/scsi/scsi_transport_ufs.c | 337 ++
include/scsi/scsi_transport_ufs.h | 40 +
4 files changed, 391 insertions(+)
create mode 100644 drivers/scsi/scsi_transport_ufs.c
create mode 100644 include/scsi/scsi_transport_ufs.h
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 8fc851a..9e99275 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -290,6 +290,19 @@ config SCSI_SAS_ATTRS
If you wish to export transport-specific information about
each attached SAS device to sysfs, say Y.
+config SCSI_UFS_ATTRS
+ tristate "UFS Transport Attributes"
+ depends on SCSI
+ select BLK_DEV_BSGLIB
+ help
+ Scsi transport is a framework that allow to send scsi commands to
+ a non-scsi devices. Still, it is flexible enough to allow sending
+ non-scsi commands as well. We will use this framework to manage
+ ufs devices by sending UPIU transactions.
+
+ If you wish to export transport-specific information about
+ each attached UFS device to sysfs, say Y.
+
source "drivers/scsi/libsas/Kconfig"
config SCSI_SRP_ATTRS
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index d1bad43..e0efc06 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_SCSI_ISCSI_ATTRS)+=
scsi_transport_iscsi.o
obj-$(CONFIG_SCSI_SAS_ATTRS) += scsi_transport_sas.o
obj-$(CONFIG_SCSI_SAS_LIBSAS) += libsas/
obj-$(CONFIG_SCSI_SRP_ATTRS) += scsi_transport_srp.o
+obj-$(CONFIG_SCSI_UFS_ATTRS) += scsi_transport_ufs.o
obj-$(CONFIG_SCSI_DH) += device_handler/
obj-$(CONFIG_LIBFC)+= libfc/
diff --git a/drivers/scsi/scsi_transport_ufs.c
b/drivers/scsi/scsi_transport_ufs.c
new file mode 100644
index 000..62aec49
--- /dev/null
+++ b/drivers/scsi/scsi_transport_ufs.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SCSI UFS transport class
+ *
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+
+#define UFS_HOST_ATTRS 0
+#define UFS_PORT_ATTRS 0
+
+struct ufs_internal {
+ struct scsi_transport_template t;
+ struct ufs_function_template *f;
+
+ struct device_attribute *host_attrs[UFS_HOST_ATTRS + 1];
+ struct device_attribute *port_attrs[UFS_PORT_ATTRS + 1];
+ struct transport_container port_attr_cont;
+};
+#define to_ufs_internal(tmpl) container_of(tmpl, struct ufs_internal, t)
+
+struct ufs_host_attrs {
+ atomic_t next_port_id;
+};
+#define to_ufs_host_attrs(x) ((struct ufs_host_attrs *)(x)->shost_data)
+
+static int ufs_bsg_dispatch(struct bsg_job *job)
+{
+ struct Scsi_Host *shost = dev_to_shost(job->dev);
+ struct ufs_internal *i = to_ufs_internal(shost->transportt);
+
+ /* check for payload_len when we'll support data transfer upiu */
+
+ return i->f->bsg_request(job);
+}
+
+static int ufs_bsg_initialize(struct Scsi_Host *shost, struct ufs_port *port)
+{
+ struct request_queue *q;
+ struct ufs_internal *i = to_ufs_internal(shost->transportt);
+
+ if (!i->f->bsg_request) {
+ pr_info("%s can't handle ufs requests\n", shost->hostt->name);
+ return 0;
+ }
+
+ q = bsg_setup_queue(&port->dev, dev_name(&port->dev),
+ ufs_bsg_dispatch, 0, NULL);
+ if (IS_ERR(q))
+ return PTR_ERR(q);
+ port->q = q;
+
+ return 0;
+}
+
+static int ufs_host_setup(struct transport_container *tc, struct device *dev,
+ struct device *cdev)
+{
+ struct Scsi_Host *shost = dev_to_s
[PATCH 0/6] scsi: scsi transport for ufs devices
Here is a proposal to use the scsi transport subsystem to manage ufs devices. scsi transport is a framework that allow to send scsi commands to a non-scsi devices. Still, it is flexible enough to allow sending non-scsi commands as well. We will use this framework to manage ufs devices by sending UPIU transactions. We added a new scsi transport module, a ufs-bsg LLD companion, and some new API to the ufs driver. For the time being, we will not use it for data transfer, but just for device management per-se. We are planning to add this capability in the future. We tested it on a Hikey-960 platform with a V4.14 kernel, "modernized" by recent bsg and ufs patches. We also used a htc11 with a V4.4 kernel, but needed much more transport/bsg/ufs patches to make it relevant. Avri Altman (6): scsi: Add ufs transport class scsi: ufs: Add ufs-bsg module scsi: ufs: Instantiate a ufs transport if its available scsi: ufs: Add API to execute raw upiu commands scsi: ufs-bsg: Add support for raw upiu in ufs_bsg_request() scsi: ufs-bsg: Add support for uic commands in ufs_bsg_request() drivers/scsi/Kconfig | 13 ++ drivers/scsi/Makefile | 1 + drivers/scsi/scsi_transport_ufs.c | 337 ++ drivers/scsi/ufs/Makefile | 1 + drivers/scsi/ufs/ufs_bsg.c| 292 + drivers/scsi/ufs/ufs_bsg.h| 76 + drivers/scsi/ufs/ufshcd.c | 232 ++ drivers/scsi/ufs/ufshcd.h | 4 + include/scsi/scsi_transport_ufs.h | 40 + include/uapi/scsi/scsi_bsg_ufs.h | 56 +++ 10 files changed, 1052 insertions(+) create mode 100644 drivers/scsi/scsi_transport_ufs.c create mode 100644 drivers/scsi/ufs/ufs_bsg.c create mode 100644 drivers/scsi/ufs/ufs_bsg.h create mode 100644 include/scsi/scsi_transport_ufs.h create mode 100644 include/uapi/scsi/scsi_bsg_ufs.h -- 1.9.1
[PATCH 5/6] scsi: ufs-bsg: Add support for raw upiu in ufs_bsg_request()
Do that for the currently supported UPIUs:
query, nop out, and task management.
We do not support UPIU of type scsi command yet, while
we are using the job's request and reply pointers to hold
the payload. We will look into it in later patches.
We might need to elaborate the raw upiu api for that.
We also still not supporting uic commands:
For first phase, we plan to use the existing api,
and send only uic commands that are already supported.
Anyway, all that will come in the next patch.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufs_bsg.c | 119 +++--
drivers/scsi/ufs/ufs_bsg.h | 1 +
2 files changed, 116 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 71826ba..d077e42 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -19,18 +19,129 @@ struct ufs_bsg {
.bsg_request = ufs_bsg_request,
};
+static int ufs_bsg_get_query_desc_size(struct utp_upiu_query *qr,
+ int *buff_len)
+{
+ int desc_size = be16_to_cpu(qr->length);
+ int desc_id = qr->idn;
+ int ret = 0;
+
+ if (qr->opcode != UPIU_QUERY_OPCODE_WRITE_DESC ||
+ desc_size <= 0)
+ return -EINVAL;
+
+ ret = ufshcd_map_desc_id_to_length(bsg_host->hba, desc_id, buff_len);
+
+ if (ret || !buff_len)
+ return -EINVAL;
+
+ *buff_len = min_t(int, *buff_len, desc_size);
+
+ return ret;
+}
+
+static int ufs_bsg_verify_query_size(unsigned int request_len,
+unsigned int reply_len,
+int rw, int buff_len)
+{
+ int min_req_len = sizeof(struct ufs_bsg_request);
+ int min_rsp_len = sizeof(struct ufs_bsg_reply);
+
+ if (rw == UFS_BSG_NOP)
+ goto null_buff;
+
+ if (rw == WRITE)
+ min_req_len += buff_len;
+
+null_buff:
+ if (min_req_len > request_len)
+ return -EINVAL;
+
+ if (min_rsp_len > reply_len)
+ return -EINVAL;
+
+ return 0;
+}
+
static int ufs_bsg_request(struct bsg_job *job)
{
struct ufs_bsg_request *bsg_request = job->request;
struct ufs_bsg_reply *bsg_reply = job->reply;
- int ret = -ENOTSUPP;
+ unsigned int request_len = job->request_len;
+ unsigned int reply_len = job->reply_len;
+ struct utp_upiu_query *qr;
+ __be32 *req_upiu = NULL;
+ __be32 *rsp_upiu = NULL;
+ int msgcode;
+ uint8_t *desc_buff = NULL;
+ int buff_len = 0;
+ int rw = UFS_BSG_NOP;
+ int ret;
+ ret = ufs_bsg_verify_query_size(request_len, reply_len, rw, buff_len);
+ if (ret) {
+ dev_err(job->dev, "not enough space assigned\n");
+ goto out;
+ }
bsg_reply->reply_payload_rcv_len = 0;
- /* Do Nothing for now */
- dev_err(job->dev, "unsupported message_code 0x%x\n",
- bsg_request->msgcode);
+ msgcode = bsg_request->msgcode;
+ switch (msgcode) {
+ case UPIU_TRANSACTION_QUERY_REQ:
+ qr = &bsg_request->tsf.qr;
+ if (qr->opcode == UPIU_QUERY_OPCODE_READ_DESC)
+ goto not_supported;
+
+ if (ufs_bsg_get_query_desc_size(qr, &buff_len))
+ goto null_desc_buff;
+
+ if (qr->opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
+ rw = WRITE;
+ desc_buff = ((uint8_t *)bsg_request) +
+ sizeof(struct ufs_bsg_request);
+ }
+
+null_desc_buff:
+ /* fall through */
+ case UPIU_TRANSACTION_NOP_OUT:
+ case UPIU_TRANSACTION_TASK_REQ:
+ /* Now that we know if its a read or write, verify again */
+ if (rw != UFS_BSG_NOP || buff_len) {
+ ret = ufs_bsg_verify_query_size(request_len, reply_len,
+ rw, buff_len);
+ if (ret) {
+ dev_err(job->dev,
+ "not enough space assigned\n");
+ goto out;
+ }
+ }
+
+ req_upiu = (__be32 *)&bsg_request->header;
+ rsp_upiu = (__be32 *)&bsg_reply->header;
+ ret = ufshcd_exec_raw_upiu_cmd(bsg_host->hba,
+ req_upiu, rsp_upiu, msgcode,
+ desc_buff, &buff_len, rw);
+
+ break;
+ case UPIU_TRANSACTION_UIC_CMD:
+ /* later */
+ case UPIU_TRANSACTION_COMMAND:
+ case UPIU_TRANSACTION_DATA_OUT:
+not_supported:
+ /* for the time being, we do not support data transfer upiu */
+ ret = -ENOTSUPP;
+ dev_err(job->dev, "unsupported m
[PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands
The UFS host software uses a combination of a host register set,
and Transfer Request Descriptors in system memory to communicate
with host controller hardware. In its mmio space, a separate places
are assigned to UTP Transfer Request Descriptor ("utrd") list, and
to UTP Task Management Request Descriptor ("utmrd") list.
The provided API supports utrd-typed requests: nop out
and device management commands. It also supports utmrd-type
requests: task management requests.
Other UPIU types are not supported for now.
We utilize the already existing code for tag and task work queues.
That is, all utrd-typed UPIUs are "disguised" as device management
commands. Similarly, the utmrd-typed UPUIs uses the task management
infrastructure.
It is up to the caller to fill the upiu request properly,
as it will be copied without any further input validations.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshcd.c | 225 ++
drivers/scsi/ufs/ufshcd.h | 4 +
2 files changed, 229 insertions(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c2ae406..5abf697 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -257,6 +257,8 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool
on,
static irqreturn_t ufshcd_intr(int irq, void *__hba);
static int ufshcd_change_power_mode(struct ufs_hba *hba,
struct ufs_pa_layer_attr *pwr_mode);
+static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp);
+
static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
{
return tag >= 0 && tag < hba->nutrs;
@@ -3106,6 +3108,229 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba,
EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
/**
+ * ufshcd_issue_tm_upiu_cmd - API for sending "utmrd" requests
+ * @hba - UFS hba
+ * @req_upiu - bsg request
+ * @rsp_upiu - bsg reply
+ *
+ * Those requests uses UTP Task Management Request Descriptor - utmrd.
+ * Therefore, it "rides" the task management infrastructure: uses its tag and
+ * tasks work queues.
+ */
+static int ufshcd_issue_tm_upiu_cmd(struct ufs_hba *hba,
+ __be32 *req_upiu,
+ __be32 *rsp_upiu)
+{
+ struct utp_task_req_desc *task_req_descp;
+ struct Scsi_Host *host = hba->host;
+ unsigned long flags;
+ int free_slot;
+ u8 tm_response = 0xF;
+ int err;
+ int task_tag;
+
+ wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot));
+ ufshcd_hold(hba, false);
+
+ spin_lock_irqsave(host->host_lock, flags);
+
+ task_req_descp = hba->utmrdl_base_addr;
+ task_req_descp += free_slot;
+
+ task_req_descp->header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
+ task_req_descp->header.dword_2 =
+ cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+
+ task_tag = hba->nutrs + free_slot;
+
+ req_upiu[0] |= cpu_to_be32(task_tag);
+
+ /* just copy the upiu request as it is */
+ memcpy(task_req_descp->task_req_upiu, req_upiu,
+ GENERAL_UPIU_REQUEST_SIZE);
+
+ /* send command to the controller */
+ __set_bit(free_slot, &hba->outstanding_tasks);
+
+ /* Make sure descriptors are ready before ringing the task doorbell */
+ wmb();
+
+ ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
+ /* Make sure that doorbell is committed immediately */
+ wmb();
+
+ spin_unlock_irqrestore(host->host_lock, flags);
+
+ /* wait until the task management command is completed */
+ err = wait_event_timeout(hba->tm_wq,
+ test_bit(free_slot, &hba->tm_condition),
+ msecs_to_jiffies(TM_CMD_TIMEOUT));
+ if (!err) {
+ dev_err(hba->dev, "%s: bsg task management cmd timed-out\n",
+ __func__);
+ if (ufshcd_clear_tm_cmd(hba, free_slot))
+ dev_WARN(hba->dev,
+"%s: unable clear tm cmd (slot %d) timeout\n",
+ __func__, free_slot);
+ err = -ETIMEDOUT;
+ } else {
+ err = ufshcd_task_req_compl(hba, free_slot, &tm_response);
+ }
+
+ /* just copy the upiu response as it is */
+ memcpy(rsp_upiu, task_req_descp->task_rsp_upiu,
+ GENERAL_UPIU_REQUEST_SIZE);
+
+ clear_bit(free_slot, &hba->tm_condition);
+ ufshcd_put_tm_slot(hba, free_slot);
+ wake_up(&hba->tm_tag_wq);
+
+ ufshcd_release(hba);
+ return err;
+}
+
+/**
+ * ufshcd_issue_devman_upiu_cmd - API for sending "utrd" type requests
+ * @hba: per-adapter instance
+ * @req_upiu: upiu request - 8 dwards
+ * @rsp_upiu: upiu reply - 8 dwards
+ * @msgcode: message code, one of UPIU Transaction Codes Initiator to Target
+ * @desc_buff: pointer to descriptor buffer, NULL if NA
+
[PATCH 3/6] scsi: ufs: Instantiate a ufs transport if its available
Call the Attach/Probe/Remove APIs.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshcd.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3560185..c2ae406 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -46,6 +46,7 @@
#include "ufs_quirks.h"
#include "unipro.h"
#include "ufs-sysfs.h"
+#include "ufs_bsg.h"
#define CREATE_TRACE_POINTS
#include
@@ -6651,6 +6652,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
hba->clk_scaling.is_allowed = true;
}
+ if (hba->host->transportt)
+ ufs_bsg_probe(hba);
+
scsi_scan_host(hba->host);
pm_runtime_put_sync(hba->dev);
}
@@ -7874,6 +7878,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
*/
void ufshcd_remove(struct ufs_hba *hba)
{
+ ufs_bsg_remove(hba->host);
ufs_sysfs_remove_nodes(hba->dev);
scsi_remove_host(hba->host);
/* disable interrupts */
@@ -8067,6 +8072,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
*mmio_base, unsigned int irq)
hba->is_irq_enabled = true;
}
+ ufs_bsg_attach_transport(hba);
+
err = scsi_add_host(host, hba->dev);
if (err) {
dev_err(hba->dev, "scsi_add_host failed\n");
--
1.9.1
[PATCH 2/6] scsi: ufs: Add ufs-bsg module
A LLD companion for the ufs scsi transport.
For now, just provide an API to instantiate the ufs transport,
allocating and removing ufs-ports.
As we are attaching the transport template to the scsi host
that was already alocated for the ufs hba,
we'll need to pay extra attention on where to call the
provided API from ufshcd.
For the time being, implements an empty bsg_request() -
will add some more functionality in coming patches.
Nonetheless, we reveal here the protocol we are planning to use:
UFS Transport Protocol Transactions. UFS transactions consist
of packets called UFS Protocol Information Units (UPIU).
There are UPIU’s defined for UFS SCSI commands, responses,
data in and data out, task management, utility functions,
vendor functions, transaction synchronization and control, and more.
By using UPIUs, we get access to the most fine-grained internals
of this protocol, and able to communicate with the device in ways,
that are sometimes beyond the capacity of the ufs driver.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/Makefile| 1 +
drivers/scsi/ufs/ufs_bsg.c | 127 +++
drivers/scsi/ufs/ufs_bsg.h | 74 +++
include/uapi/scsi/scsi_bsg_ufs.h | 56 +
4 files changed, 258 insertions(+)
create mode 100644 drivers/scsi/ufs/ufs_bsg.c
create mode 100644 drivers/scsi/ufs/ufs_bsg.h
create mode 100644 include/uapi/scsi/scsi_bsg_ufs.h
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 2c50f03..5227bfb 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -8,3 +8,4 @@ ufshcd-core-objs := ufshcd.o ufs-sysfs.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
+obj-$(CONFIG_SCSI_UFS_ATTRS) += ufs_bsg.o
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
new file mode 100644
index 000..71826ba
--- /dev/null
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SSCSI transport companion for UFS HBA
+ *
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+#include "ufs_bsg.h"
+
+struct ufs_bsg {
+ struct ufs_hba *hba;
+ struct ufs_port *port;
+};
+static struct ufs_bsg *bsg_host;
+
+static int ufs_bsg_request(struct bsg_job *job);
+
+static struct scsi_transport_template *ufs_transport_template;
+static struct ufs_function_template ufs_transport_functions = {
+ .bsg_request = ufs_bsg_request,
+};
+
+static int ufs_bsg_request(struct bsg_job *job)
+{
+ struct ufs_bsg_request *bsg_request = job->request;
+ struct ufs_bsg_reply *bsg_reply = job->reply;
+ int ret = -ENOTSUPP;
+
+ bsg_reply->reply_payload_rcv_len = 0;
+
+ /* Do Nothing for now */
+ dev_err(job->dev, "unsupported message_code 0x%x\n",
+ bsg_request->msgcode);
+
+ bsg_reply->result = ret;
+ if (ret)
+ job->reply_len = sizeof(uint32_t);
+ else
+ job->reply_len = sizeof(struct ufs_bsg_reply) +
+bsg_reply->reply_payload_rcv_len;
+
+ bsg_job_done(job, bsg_reply->result, bsg_reply->reply_payload_rcv_len);
+
+ return ret;
+}
+
+/**
+ * ufs_bsg_attach_transport - instantiate UFS transport template
+ * @hba: hba to attach the transport template to
+ *
+ * Call this before scsi_add_host() publishes it to the scsi midlayer
+ */
+void ufs_bsg_attach_transport(struct ufs_hba *hba)
+{
+ ufs_transport_template =
+ ufs_attach_transport(&ufs_transport_functions);
+ if (!ufs_transport_template)
+ return;
+
+ hba->host->transportt = ufs_transport_template;
+}
+
+/**
+ * ufs_bsg_remove - detach and remove the added ufs-ports
+ * @shost: scsi host that is parenting the ufs-ports
+ *
+ * Should be called when unloading the driver.
+ */
+void ufs_bsg_remove(struct Scsi_Host *shost)
+{
+ if (!bsg_host)
+ return;
+
+ ufs_remove_host(shost);
+
+ if (ufs_transport_template) {
+ ufs_release_transport(ufs_transport_template);
+ ufs_transport_template = NULL;
+ }
+
+ kfree(bsg_host);
+}
+
+/**
+ * ufs_bsg_probe - Add and report ufs device to ufs transport
+ * @hba: per adapter object
+ *
+ * Called during initial loading of the driver, and before scsi_scan_host.
+ * Should be called only if the attach phase was successful
+ */
+
+int ufs_bsg_probe(struct ufs_hba *hba)
+{
+ struct ufs_port *port;
+ int ret;
+
+ if (WARN_ON(!ufs_transport_template))
+ return -ENODEV;
+
+ bsg_host = kzalloc(sizeof(*bsg_host), GFP_KERNEL);
+ if (!bsg_host) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ port = ufs_port_alloc(hba->host);
+ if (!port) {
+ ret = -ENOMEM;
+ goto out_free_bsg_host;
+ }
+
+ re
[PATCH 6/6] scsi: ufs-bsg: Add support for uic commands in ufs_bsg_request()
Add support to those uic commands, that are currently supported
by ufshcd api: the variants of dme_{peer}_{set_get}.
At this point better not to add any new api, as careless
uic command may turn the device into a brick.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufs_bsg.c | 56 +-
drivers/scsi/ufs/ufs_bsg.h | 1 +
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index d077e42..441d096 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -63,6 +63,55 @@ static int ufs_bsg_verify_query_size(unsigned int
request_len,
return 0;
}
+static int ufs_bsg_exec_uic_cmd(struct uic_command *uc)
+{
+ u32 attr_sel = uc->argument1;
+ u8 attr_set = (uc->argument2 >> 16) & 0xff;
+ u32 mib_val = uc->argument3;
+ int cmd = uc->command;
+ int ret = 0;
+
+ switch (cmd) {
+ case UIC_CMD_DME_GET:
+ ret = ufshcd_dme_get_attr(bsg_host->hba, attr_sel,
+ &mib_val, DME_LOCAL);
+ break;
+ case UIC_CMD_DME_SET:
+ ret = ufshcd_dme_set_attr(bsg_host->hba, attr_sel, attr_set,
+ mib_val, DME_LOCAL);
+ break;
+ case UIC_CMD_DME_PEER_GET:
+ ret = ufshcd_dme_get_attr(bsg_host->hba, attr_sel,
+ &mib_val, DME_PEER);
+ break;
+ case UIC_CMD_DME_PEER_SET:
+ ret = ufshcd_dme_set_attr(bsg_host->hba, attr_sel, attr_set,
+ mib_val, DME_PEER);
+ break;
+ case UIC_CMD_DME_POWERON:
+ case UIC_CMD_DME_POWEROFF:
+ case UIC_CMD_DME_ENABLE:
+ case UIC_CMD_DME_RESET:
+ case UIC_CMD_DME_END_PT_RST:
+ case UIC_CMD_DME_LINK_STARTUP:
+ case UIC_CMD_DME_HIBER_ENTER:
+ case UIC_CMD_DME_HIBER_EXIT:
+ case UIC_CMD_DME_TEST_MODE:
+ ret = -ENOTSUPP;
+ pr_err("%s unsupported command 0x%x\n", __func__, cmd);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ if (ret)
+ pr_err("%s error in command 0x%x\n", __func__, cmd);
+
+ uc->argument3 = mib_val;
+
+ return ret;
+}
+
static int ufs_bsg_request(struct bsg_job *job)
{
struct ufs_bsg_request *bsg_request = job->request;
@@ -70,6 +119,7 @@ static int ufs_bsg_request(struct bsg_job *job)
unsigned int request_len = job->request_len;
unsigned int reply_len = job->reply_len;
struct utp_upiu_query *qr;
+ struct uic_command uc = {0};
__be32 *req_upiu = NULL;
__be32 *rsp_upiu = NULL;
int msgcode;
@@ -124,7 +174,11 @@ static int ufs_bsg_request(struct bsg_job *job)
break;
case UPIU_TRANSACTION_UIC_CMD:
- /* later */
+ memcpy(&uc, &bsg_request->tsf.uc, UIC_CMD_SIZE);
+ ret = ufs_bsg_exec_uic_cmd(&uc);
+ memcpy(&bsg_reply->tsf.uc, &uc, UIC_CMD_SIZE);
+
+ break;
case UPIU_TRANSACTION_COMMAND:
case UPIU_TRANSACTION_DATA_OUT:
not_supported:
diff --git a/drivers/scsi/ufs/ufs_bsg.h b/drivers/scsi/ufs/ufs_bsg.h
index 0fd2859..651ffb3 100644
--- a/drivers/scsi/ufs/ufs_bsg.h
+++ b/drivers/scsi/ufs/ufs_bsg.h
@@ -15,6 +15,7 @@
#define UFS_BSG_NOP (-1)
#define UPIU_TRANSACTION_UIC_CMD 0x1F
+#define UIC_CMD_SIZE (sizeof(u32) * 4)
enum {
REQ_UPIU_SIZE_DWORDS= 8,
--
1.9.1
[PATCH V7 0/2] Add UFS provisioning support in driver
This patch adds Configfs support to provision UFS device at runtime. This feature can be primarily useful in factory or assembly line as some devices may be required to be configured multiple times during initial system development phase. Configuration Descriptors can be written multiple times until bConfigDescrLock attribute is zero. Configuration descriptor buffer consists of Device and Unit descriptor configurable parameters which are parsed from vendor specific provisioning file and then passed via configfs node at runtime to provision ufs device. Changes since V6: 1)scsi: ufs: set the device reference clock setting Re-introduced this patch to provisioning patch set(as per comments from Evan). Used of_clk_get_by_name() and clk_get_rate() to set ref_clk frequency instead of passing freq via DT. 2)scsi: ufs: Add configfs support for UFS provisioning Updated error handling in case if kstrtoint fails while parsing input configuration buffer. Changes since V5: 1)scsi: ufs: set the device reference clock setting Removed this patch from provisioning patch set(as its not required to be set as dependent changes). This will be uploaded as a separate patch later. 2)scsi: ufs: Add configfs support for UFS provisioning Removed few extra debug prints. Updated permission of ufs_provision attribute from 0666 to 0644. Pass UFS device name as part of ufshcd_configfs_init() to support multiple UFS controller for embedded and removable UFS card. Changes since V4: 1)scsi: ufs: set the device reference clock setting Used "assigned-clock-rates" DT property to pass required ref clk frequency. 2)scsi: ufs: Add configfs support for ufs provisioning Combined previous patch(2) and patch(3) into single patch which adds configfs provisioning support in driver. Removed extra sw provisioning related fields (like lun_to_grow, commit) and its related code. Updated Documentation to match configuration descriptor buffer parameters to be passed as per specs. Removed global ufs_hba ptr added in ufs-configfs file and instead passed *hba in ufs configfs init()/store()/show() api's. This is to support embedded as well as removable ufs card provisioning via configfs. Changes since V3: 1)scsi: ufs: set the device reference clock setting Updated logic to retain default ref_clk frequency setting programmed in device in case if invalid value is passed via devicetree setting. Replaced of_property_read_u32() with device_property_read_u32(). Removed invalid checks. 2)scsi: ufs: Add ufs provisioning support Added pm_runtime_get/put_sync and scsi_block/unblock_request in runtime provisioning for stable operation. 3)scsi: ufs: Add configfs support for ufs provisioning Updated Documentation with missing buffer entries required for runtime provisioning. Used config option to support conditional compilation for configfs api's. Changes since V2: Added configfs support for ufs provisioning and removed sysfs support. Changes since V1: Added device tree entry to parse reference clock frequency instead of hardcoding 19.2 MHz, as it can vary for different vendors. Also removed setting ref_clk again during runtime provisioning as it will be already set during probe. Used get_unaligned_be*/put_unaligned_be* where applicable. Changes since RFC: Added check to avoid ufs runtime provisioning if Configuration decriptor lock attribute is set to one. Instead of parsing ref_clk frequency via device tree, used correct enum ref_clk_freq value(19.2 Mhz for proviosioning). Added config_descriptor sysfs entry to provision ufs and also updated documentation for its correct usage. Added more protection against bad data handling in sysfs code. Sayali Lokhande (1): scsi: ufs: Add configfs support for UFS provisioning Subhash Jadavani (1): scsi: ufs: set the device reference clock setting Documentation/ABI/testing/configfs-driver-ufs | 18 +++ drivers/scsi/ufs/Kconfig | 10 ++ drivers/scsi/ufs/Makefile | 1 + drivers/scsi/ufs/ufs-configfs.c | 162 ++ drivers/scsi/ufs/ufs.h| 8 ++ drivers/scsi/ufs/ufshcd-pltfrm.c | 2 + drivers/scsi/ufs/ufshcd.c | 91 ++- drivers/scsi/ufs/ufshcd.h | 12 ++ 8 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/configfs-driver-ufs create mode 100644 drivers/scsi/ufs/ufs-configfs.c -- The Qualcomm Innovation Center, Inc.
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On 31/07/18 16:14, kernelci.org bot wrote: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731) Full Boot Summary: https://kernelci.org/boot/all/job/next/branch/master/kernel/next-20180731/ Full Build Summary: https://kernelci.org/build/next/branch/master/kernel/next-20180731/ Tree: next Branch: master Git Describe: next-20180731 Git Commit: 85eac382fa06ac72adf891d04bf4d08fb09d80fa Git URL: http://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git Tested: 66 unique boards, 26 SoC families, 21 builds out of 200 Boot Regressions Detected: [...] x86: x86_64_defconfig: qemu: lab-baylibre: failing since 1 day (last pass: next-20180727 - first fail: next-20180730) lab-mhart: failing since 1 day (last pass: next-20180727 - first fail: next-20180730) lab-linaro-lkft: failing since 1 day (last pass: next-20180727 - first fail: next-20180730) I've run a few automated bisection on kernelci.org, it initially landed on this merge commit: ff719be3476a Merge remote-tracking branch 'scsi/for-next' The 2 parent commits boot fine, but not the resulting merge. So I did another bisection based on the first branch while merging the incoming one in each iteration, and that landed on this commit: commit d5038a13eca72fb216c07eb717169092e92284f1 Author: Johannes Thumshirn Date: Wed Jul 4 10:53:56 2018 +0200 scsi: core: switch to scsi-mq by default I then tried to revert it on top of next-20180731 and it did make it boot again. Now, I haven't looked much further in the code, it's entirely possible that the problem is on another incoming branch, in the code that this config enables. At least it seems to be narrowing down the scope for where to look for a problem. Hope this helps! Best wishes, Guillaume [...] --- For more info write to ___ Kernel-build-reports mailing list kernel-build-repo...@lists.linaro.org https://lists.linaro.org/mailman/listinfo/kernel-build-reports
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 11:05:36AM +0100, Guillaume Tucker wrote: > On 31/07/18 16:14, kernelci.org bot wrote: > > Boot Regressions Detected: > [...] > > x86: > > > > x86_64_defconfig: > > qemu: > > lab-baylibre: failing since 1 day (last pass: next-20180727 - > > first fail: next-20180730) > > lab-mhart: failing since 1 day (last pass: next-20180727 - > > first fail: next-20180730) > > lab-linaro-lkft: failing since 1 day (last pass: next-20180727 > > - first fail: next-20180730) > I've run a few automated bisection on kernelci.org, it initially > landed on this merge commit: > ff719be3476a Merge remote-tracking branch 'scsi/for-next' > The 2 parent commits boot fine, but not the resulting merge. So I > did another bisection based on the first branch while merging the > incoming one in each iteration, and that landed on this commit: > commit d5038a13eca72fb216c07eb717169092e92284f1 > Author: Johannes Thumshirn > Date: Wed Jul 4 10:53:56 2018 +0200 > > scsi: core: switch to scsi-mq by default > > > I then tried to revert it on top of next-20180731 and it did make it > boot again. Now, I haven't looked much further in the code, it's > entirely possible that the problem is on another incoming branch, in > the code that this config enables. At least it seems to be narrowing > down the scope for where to look for a problem. Copying in everyone else who signed off/acked/reviewed that commit. signature.asc Description: PGP signature
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 11:33:03AM +0100, Mark Brown wrote: > On Wed, Aug 01, 2018 at 11:05:36AM +0100, Guillaume Tucker wrote: > > On 31/07/18 16:14, kernelci.org bot wrote: > > > > Boot Regressions Detected: > > [...] > > > x86: > > > > > > x86_64_defconfig: > > > qemu: > > > lab-baylibre: failing since 1 day (last pass: next-20180727 > > > - first fail: next-20180730) > > > lab-mhart: failing since 1 day (last pass: next-20180727 - > > > first fail: next-20180730) > > > lab-linaro-lkft: failing since 1 day (last pass: > > > next-20180727 - first fail: next-20180730) > > > I've run a few automated bisection on kernelci.org, it initially > > landed on this merge commit: > > > ff719be3476a Merge remote-tracking branch 'scsi/for-next' > > > The 2 parent commits boot fine, but not the resulting merge. So I > > did another bisection based on the first branch while merging the > > incoming one in each iteration, and that landed on this commit: > > > commit d5038a13eca72fb216c07eb717169092e92284f1 > > Author: Johannes Thumshirn > > Date: Wed Jul 4 10:53:56 2018 +0200 > > > > scsi: core: switch to scsi-mq by default > > > > > > I then tried to revert it on top of next-20180731 and it did make it > > boot again. Now, I haven't looked much further in the code, it's > > entirely possible that the problem is on another incoming branch, in > > the code that this config enables. At least it seems to be narrowing > > down the scope for where to look for a problem. > > Copying in everyone else who signed off/acked/reviewed that commit. You may have to provide some clue, such as dmesg log, boot disk, ... I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq mode at default, even though without d5038a13eca72fb. Thanks, Ming
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > You may have to provide some clue, such as dmesg log, boot disk, ... > I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > mode at default, even though without d5038a13eca72fb. Boot logs and so on can be found here: https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ (these are today's but the symptoms are the same.) The ramdisk is unfortunately not linked through the UI, though we don't get that far. signature.asc Description: PGP signature
[PATCH v2] scsi: csiostor: update csio_get_flash_params()
- Updates csio_get_flash_params() to take care of ISSI, Macronix and
Winbond FLASH parts.
- Assume flash part size to be 4MB if it cannot be identified
Signed-off-by: Arjun Vynipadath
Signed-off-by: Varun Prakash
---
v2
- Simpilified switch-case statements
---
drivers/scsi/csiostor/csio_hw.c | 115 +++-
1 file changed, 102 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
index a10cf25..23d07e9 100644
--- a/drivers/scsi/csiostor/csio_hw.c
+++ b/drivers/scsi/csiostor/csio_hw.c
@@ -761,27 +761,116 @@ csio_hw_fw_dload(struct csio_hw *hw, uint8_t *fw_data,
uint32_t size)
static int
csio_hw_get_flash_params(struct csio_hw *hw)
{
+ /* Table for non-Numonix supported flash parts. Numonix parts are left
+* to the preexisting code. All flash parts have 64KB sectors.
+*/
+ static struct flash_desc {
+ u32 vendor_and_model_id;
+ u32 size_mb;
+ } supported_flash[] = {
+ { 0x150201, 4 << 20 }, /* Spansion 4MB S25FL032P */
+ };
+
+ u32 part, manufacturer;
+ u32 density, size = 0;
+ u32 flashid = 0;
int ret;
- uint32_t info = 0;
ret = csio_hw_sf1_write(hw, 1, 1, 0, SF_RD_ID);
if (!ret)
- ret = csio_hw_sf1_read(hw, 3, 0, 1, &info);
+ ret = csio_hw_sf1_read(hw, 3, 0, 1, &flashid);
csio_wr_reg32(hw, 0, SF_OP_A);/* unlock SF */
- if (ret != 0)
+ if (ret)
return ret;
- if ((info & 0xff) != 0x20) /* not a Numonix flash */
- return -EINVAL;
- info >>= 16;/* log2 of size */
- if (info >= 0x14 && info < 0x18)
- hw->params.sf_nsec = 1 << (info - 16);
- else if (info == 0x18)
- hw->params.sf_nsec = 64;
- else
- return -EINVAL;
- hw->params.sf_size = 1 << info;
+ /* Check to see if it's one of our non-standard supported Flash parts.
+*/
+ for (part = 0; part < ARRAY_SIZE(supported_flash); part++)
+ if (supported_flash[part].vendor_and_model_id == flashid) {
+ hw->params.sf_size = supported_flash[part].size_mb;
+ hw->params.sf_nsec =
+ hw->params.sf_size / SF_SEC_SIZE;
+ goto found;
+ }
+
+ /* Decode Flash part size. The code below looks repetative with
+* common encodings, but that's not guaranteed in the JEDEC
+* specification for the Read JADEC ID command. The only thing that
+* we're guaranteed by the JADEC specification is where the
+* Manufacturer ID is in the returned result. After that each
+* Manufacturer ~could~ encode things completely differently.
+* Note, all Flash parts must have 64KB sectors.
+*/
+ manufacturer = flashid & 0xff;
+ switch (manufacturer) {
+ case 0x20: { /* Micron/Numonix */
+ /* This Density -> Size decoding table is taken from Micron
+* Data Sheets.
+*/
+ density = (flashid >> 16) & 0xff;
+ switch (density) {
+ case 0x14 ... 0x19: /* 1MB - 32MB */
+ size = 1 << density;
+ break;
+ case 0x20: /* 64MB */
+ size = 1 << 26;
+ break;
+ case 0x21: /* 128MB */
+ size = 1 << 27;
+ break;
+ case 0x22: /* 256MB */
+ size = 1 << 28;
+ }
+ break;
+ }
+ case 0x9d: { /* ISSI -- Integrated Silicon Solution, Inc. */
+ /* This Density -> Size decoding table is taken from ISSI
+* Data Sheets.
+*/
+ density = (flashid >> 16) & 0xff;
+ switch (density) {
+ case 0x16: /* 32 MB */
+ size = 1 << 25;
+ break;
+ case 0x17: /* 64MB */
+ size = 1 << 26;
+ }
+ break;
+ }
+ case 0xc2: /* Macronix */
+ case 0xef: /* Winbond */ {
+ /* This Density -> Size decoding table is taken from
+* Macronix and Winbond Data Sheets.
+*/
+ density = (flashid >> 16) & 0xff;
+ switch (density) {
+ case 0x17: /* 8MB */
+ case 0x18: /* 16MB */
+ size = 1 << density;
+ }
+ }
+ }
+
+ /* If we didn't recognize the FLASH part, that's no real issue: the
+* Hardware/Software contract says that Hardware will _*ALWAYS*_
+* use a FLASH part which is at least 4MB in size and has 64KB
+* sect
Re: [PATCH 1/6] scsi: Add ufs transport class
Hi Avri,
On Wed, Aug 01, 2018 at 11:04:27AM +0300, Avri Altman wrote:
[...
> +#include
Why do you include bsg.h here and bsg-lib.h in the scsi_transport_ufs.h?
[...]
> +#define to_ufs_internal(tmpl)container_of(tmpl, struct ufs_internal,
> t)
I'd personally prefer this to be a inline function instead of a define
for type safety reasons.
> +
> +struct ufs_host_attrs {
> + atomic_t next_port_id;
> +};
> +#define to_ufs_host_attrs(x) ((struct ufs_host_attrs *)(x)->shost_data)
Ditto.
[...]
> +
> + port->id = atomic_inc_return(&ufs_host->next_port_id);
Any reason you can't use an IDA for the port->id?
[...]
> +
> + error = device_add(dev);
> +
> + if (error)
> + return error;
No blank line please.
[...]
> +#define dev_to_ufs_port(d) \
> + container_of((d), struct ufs_port, dev)
Inline function as well, please.
--
Johannes Thumshirn Storage
jthumsh...@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
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On 1 August 2018 at 11:59, Mark Brown wrote: > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > >> You may have to provide some clue, such as dmesg log, boot disk, ... > >> I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq >> mode at default, even though without d5038a13eca72fb. > > Boot logs and so on can be found here: > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > (these are today's but the symptoms are the same.) The ramdisk is > unfortunately not linked through the UI, though we don't get that far. And a full LAVA boot log from my lab http://lava.streamtester.net/scheduler/job/138067 QEMU command line here: http://lava.streamtester.net/scheduler/job/138067#L75 And a LAVA job definition, which includes the url of the ramdisk and kernel: http://lava.streamtester.net/scheduler/job/138067/definition#defline76 > > ___ > Kernel-build-reports mailing list > kernel-build-repo...@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/kernel-build-reports >
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 11:59:19AM +0100, Mark Brown wrote: > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > > > You may have to provide some clue, such as dmesg log, boot disk, ... > > > I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > > mode at default, even though without d5038a13eca72fb. > > Boot logs and so on can be found here: > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > (these are today's but the symptoms are the same.) The ramdisk is > unfortunately not linked through the UI, though we don't get that far. Can you give us a bit more information about you test setups? Like Qemu command line, etc..? From you kernel config I see you're not using virtio (as Ming already suggested). What medium are you booting off? Thanks, Johannes -- Johannes Thumshirn Storage jthumsh...@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
Re: [PATCH] scsi: csiostor: remove automatic irq affinity assignment
On Wed, Aug 01, 2018 at 08:33:23AM +0200, Hannes Reinecke wrote:
> On 07/31/2018 05:07 PM, Varun Prakash wrote:
> > If number of interrupt vectors are more than num_online_cpus()
> > then pci_alloc_irq_vectors_affinity() assigns cpumask based
> > on num_possible_cpus() to the remaining vectors because of
> > this interrupt does not generate for these vectors.
> >
> > This patch fixes this issue by using pci_alloc_irq_vectors()
> > instead of pci_alloc_irq_vectors_affinity().
> >
> > Signed-off-by: Varun Prakash
> > ---
> > drivers/scsi/csiostor/csio_isr.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> >
> > - cnt = pci_alloc_irq_vectors_affinity(hw->pdev, min, cnt,
> > - PCI_IRQ_MSIX | PCI_IRQ_AFFINITY, &desc);
> > + cnt = pci_alloc_irq_vectors(hw->pdev, min, cnt, PCI_IRQ_MSIX);
> > if (cnt < 0)
> > return cnt;
> >
> >
> Hmm.
> That patch (and the reasoning leading up to it) really sound dodgy.
> I'd rather fix the interrupt affinity code to handle this case correctly.
Yes, it is better to fix the affinity assignment code, I posted
this patch to start a discussion on this issue.
I can confirm that if number of interrupt vectors are more than
num_online cpus() then pci_alloc_irq_vectors_affinity() assigns
cpumask based on cpu_possible_mask to the remaining vectors.
Following is the logic for creating affinity masks
struct cpumask *
irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
{
...
/* Spread on present CPUs starting from affd->pre_vectors */
usedvecs = irq_build_affinity_masks(affd, curvec, affvecs,
node_to_cpumask, cpu_present_mask,
nmsk, masks);
/*
* Spread on non present CPUs starting from the next vector to be
* handled. If the spreading of present CPUs already exhausted the
* vector space, assign the non present CPUs to the already spread
* out vectors.
*/
if (usedvecs >= affvecs)
curvec = affd->pre_vectors;
else
curvec = affd->pre_vectors + usedvecs;
cpumask_andnot(npresmsk, cpu_possible_mask, cpu_present_mask);
usedvecs += irq_build_affinity_masks(affd, curvec, affvecs,
node_to_cpumask, npresmsk,
nmsk, masks);
...
}
> Thomas, can you help here?
>
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 12:24:00PM +0100, Matt Hart wrote: > And a full LAVA boot log from my lab > http://lava.streamtester.net/scheduler/job/138067 > > QEMU command line here: > http://lava.streamtester.net/scheduler/job/138067#L75 > > And a LAVA job definition, which includes the url of the ramdisk and kernel: > http://lava.streamtester.net/scheduler/job/138067/definition#defline76 I grabbed your qemu cmdline and kernel config and try to reproduce it locally. Johannes -- Johannes Thumshirn Storage jthumsh...@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
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 12:24:00PM +0100, Matt Hart wrote: > On 1 August 2018 at 11:59, Mark Brown wrote: > > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > > > >> You may have to provide some clue, such as dmesg log, boot disk, ... > > > >> I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > >> mode at default, even though without d5038a13eca72fb. > > > > Boot logs and so on can be found here: > > > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > > > (these are today's but the symptoms are the same.) The ramdisk is > > unfortunately not linked through the UI, though we don't get that far. > > And a full LAVA boot log from my lab > http://lava.streamtester.net/scheduler/job/138067 > > QEMU command line here: > http://lava.streamtester.net/scheduler/job/138067#L75 > > And a LAVA job definition, which includes the url of the ramdisk and kernel: > http://lava.streamtester.net/scheduler/job/138067/definition#defline76 > Thanks for the sharing! I can reproduce this issue with above script/initrd/kernel config, and looks the issue disappeared after 'scsi_mod.use_blk_mq=0' is passed. Not see such issue with zero-day ktest config. Looks a bit weird, given SCSI_MQ is nothing related with ramdisk. Thanks, Ming
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 07:52:21PM +0800, Ming Lei wrote: > On Wed, Aug 01, 2018 at 12:24:00PM +0100, Matt Hart wrote: > > On 1 August 2018 at 11:59, Mark Brown wrote: > > > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > > > > > >> You may have to provide some clue, such as dmesg log, boot disk, ... > > > > > >> I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > > >> mode at default, even though without d5038a13eca72fb. > > > > > > Boot logs and so on can be found here: > > > > > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > > > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > > > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > > > > > (these are today's but the symptoms are the same.) The ramdisk is > > > unfortunately not linked through the UI, though we don't get that far. > > > > And a full LAVA boot log from my lab > > http://lava.streamtester.net/scheduler/job/138067 > > > > QEMU command line here: > > http://lava.streamtester.net/scheduler/job/138067#L75 > > > > And a LAVA job definition, which includes the url of the ramdisk and kernel: > > http://lava.streamtester.net/scheduler/job/138067/definition#defline76 > > > > Thanks for the sharing! > > I can reproduce this issue with above script/initrd/kernel config, and looks > the issue disappeared after 'scsi_mod.use_blk_mq=0' is passed. > > Not see such issue with zero-day ktest config. > > Looks a bit weird, given SCSI_MQ is nothing related with ramdisk. Ahm and: qemu [...] -append "console=ttyS0,115200 root=/dev/ram0 debug verbose" $ grep CONFIG_BLK_DEV_RAM .config # CONFIG_BLK_DEV_RAM is not set Something is fishy here. -- Johannes Thumshirn Storage jthumsh...@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
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 07:52:19PM +0800, Ming Lei wrote: > On Wed, Aug 01, 2018 at 12:24:00PM +0100, Matt Hart wrote: > > On 1 August 2018 at 11:59, Mark Brown wrote: > > > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > > > > > >> You may have to provide some clue, such as dmesg log, boot disk, ... > > > > > >> I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > > >> mode at default, even though without d5038a13eca72fb. > > > > > > Boot logs and so on can be found here: > > > > > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > > > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > > > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > > > > > (these are today's but the symptoms are the same.) The ramdisk is > > > unfortunately not linked through the UI, though we don't get that far. > > > > And a full LAVA boot log from my lab > > http://lava.streamtester.net/scheduler/job/138067 > > > > QEMU command line here: > > http://lava.streamtester.net/scheduler/job/138067#L75 > > > > And a LAVA job definition, which includes the url of the ramdisk and kernel: > > http://lava.streamtester.net/scheduler/job/138067/definition#defline76 > > > > Thanks for the sharing! > > I can reproduce this issue with above script/initrd/kernel config, and looks > the issue disappeared after 'scsi_mod.use_blk_mq=0' is passed. > > Not see such issue with zero-day ktest config. > > Looks a bit weird, given SCSI_MQ is nothing related with ramdisk. Seems related with sr: 1) with scsi-mq [2.808204] ata2.01: NODEV after polling detection [2.809807] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 [2.827377] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM 2.5+ PQ: 0 ANSI: 5 2) without scsi_mq [5.549135] ata2.01: NODEV after polling detection [5.554404] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 [5.596143] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM 2.5+ PQ: 0 ANSI: 5 [5.637126] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray [5.637870] cdrom: Uniform CD-ROM driver Revision: 3.20 [5.648940] sr 1:0:0:0: Attached scsi CD-ROM sr0 [5.661605] sr 1:0:0:0: Attached scsi generic sg0 type 5 We may need to take a look at recent SCSI change. Thanks, Ming
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 08:00:44PM +0800, Ming Lei wrote: > On Wed, Aug 01, 2018 at 07:52:19PM +0800, Ming Lei wrote: > > On Wed, Aug 01, 2018 at 12:24:00PM +0100, Matt Hart wrote: > > > On 1 August 2018 at 11:59, Mark Brown wrote: > > > > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > > > > > > > >> You may have to provide some clue, such as dmesg log, boot disk, ... > > > > > > > >> I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > > > >> mode at default, even though without d5038a13eca72fb. > > > > > > > > Boot logs and so on can be found here: > > > > > > > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > > > > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > > > > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > > > > > > > (these are today's but the symptoms are the same.) The ramdisk is > > > > unfortunately not linked through the UI, though we don't get that far. > > > > > > And a full LAVA boot log from my lab > > > http://lava.streamtester.net/scheduler/job/138067 > > > > > > QEMU command line here: > > > http://lava.streamtester.net/scheduler/job/138067#L75 > > > > > > And a LAVA job definition, which includes the url of the ramdisk and > > > kernel: > > > http://lava.streamtester.net/scheduler/job/138067/definition#defline76 > > > > > > > Thanks for the sharing! > > > > I can reproduce this issue with above script/initrd/kernel config, and looks > > the issue disappeared after 'scsi_mod.use_blk_mq=0' is passed. > > > > Not see such issue with zero-day ktest config. > > > > Looks a bit weird, given SCSI_MQ is nothing related with ramdisk. > > Seems related with sr: > > 1) with scsi-mq > [2.808204] ata2.01: NODEV after polling detection > [2.809807] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 > [2.827377] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM 2.5+ > PQ: 0 ANSI: 5 > > 2) without scsi_mq > [5.549135] ata2.01: NODEV after polling detection > [5.554404] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 > [5.596143] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM 2.5+ > PQ: 0 ANSI: 5 > [5.637126] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray > [5.637870] cdrom: Uniform CD-ROM driver Revision: 3.20 > [5.648940] sr 1:0:0:0: Attached scsi CD-ROM sr0 > [5.661605] sr 1:0:0:0: Attached scsi generic sg0 type 5 > > > We may need to take a look at recent SCSI change. [2.052168] ata2.01: NODEV after polling detection [2.053690] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 [2.072269] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM 2.5+ P5 [2.107220] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray [2.107675] cdrom: Uniform CD-ROM driver Revision: 3.20 [2.111851] sr 1:0:0:0: Attached scsi CD-ROM sr0 [2.123560] sr 1:0:0:0: Attached scsi generic sg0 type 5 # cat /proc/cmdline console=ttyS0,115200 root=/dev/ram0 debug verbose $ grep SCSI_MQ .config CONFIG_SCSI_MQ_DEFAULT=y on Martin's latest 4.19/scsi-queue. -- Johannes Thumshirn Storage jthumsh...@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
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On 01/08/18 12:25, Johannes Thumshirn wrote: On Wed, Aug 01, 2018 at 11:59:19AM +0100, Mark Brown wrote: On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: You may have to provide some clue, such as dmesg log, boot disk, ... I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq mode at default, even though without d5038a13eca72fb. Boot logs and so on can be found here: https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ (these are today's but the symptoms are the same.) The ramdisk is unfortunately not linked through the UI, though we don't get that far. Can you give us a bit more information about you test setups? Like Qemu command line, etc..? From you kernel config I see you're not using virtio (as Ming already suggested). What medium are you booting off? Sure, sorry I should have put that in my first email. Here's a couple of LAVA boot tests, one passing with the commit reverted and one failing with plain next-20180731: https://lava.collabora.co.uk/scheduler/job/1215154 https://lava.collabora.co.uk/scheduler/job/1215173 The qemu command line can be found in the log, copying it here: /usr/bin/qemu-system-x86_64 -cpu host -enable-kvm -nographic -net nic,model=virtio,macaddr=DE:AD:BE:EF:AE:1B -net user -m 1024 -monitor none -kernel /var/lib/lava/dispatcher/tmp/1215173/deployimages-3p80s2zk/kernel/bzImage-85eac382fa06 -append "console=ttyS0,115200 root=/dev/ram0 debug verbose" -initrd /var/lib/lava/dispatcher/tmp/1215173/deployimages-3p80s2zk/ramdisk/rootfs.cpio.gz The kernel images I used have the git revision in their file name to make it clear where they came from, they were built with x86_64_defconfig. The links to the kernel and ramdisks can be found in the job definition: https://lava.collabora.co.uk/scheduler/job/1215154/definition Please let me know if you need any more details. I'm happy to run other boot tests on that same setup if that helps verifying things (such as enabling some VIRTIO configs...). Best wishes, Guillaume
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 01:37:17PM +0100, Guillaume Tucker wrote: > The kernel images I used have the git revision in their file name > to make it clear where they came from, they were built with > x86_64_defconfig. The links to the kernel and ramdisks can be > found in the job definition: > > https://lava.collabora.co.uk/scheduler/job/1215154/definition > > Please let me know if you need any more details. I'm happy to > run other boot tests on that same setup if that helps verifying > things (such as enabling some VIRTIO configs...). Yes, can you please enable CONFIG_BLK_DEV_RAM? See my other mails in this thread for details. Thanks, Johannes -- Johannes Thumshirn Storage jthumsh...@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
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On 01/08/18 13:40, Johannes Thumshirn wrote: On Wed, Aug 01, 2018 at 01:37:17PM +0100, Guillaume Tucker wrote: The kernel images I used have the git revision in their file name to make it clear where they came from, they were built with x86_64_defconfig. The links to the kernel and ramdisks can be found in the job definition: https://lava.collabora.co.uk/scheduler/job/1215154/definition Please let me know if you need any more details. I'm happy to run other boot tests on that same setup if that helps verifying things (such as enabling some VIRTIO configs...). Yes, can you please enable CONFIG_BLK_DEV_RAM? See my other mails in this thread for details. Sure, although it didn't make any apparent difference, still on next-20180731: https://lava.collabora.co.uk/scheduler/job/1215417 The .config file I used is available here, with just CONFIG_BLK_DEV_RAM=y on top of defconfig: https://people.collabora.com/~gtucker/lava/boot/debug/bzImage-85eac382fa06-blk-dev.config Best wishes, Guillaume
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 02:50:40PM +0100, Guillaume Tucker wrote: > > Sure, although it didn't make any apparent difference, still on > next-20180731: > > https://lava.collabora.co.uk/scheduler/job/1215417 > > The .config file I used is available here, with just > CONFIG_BLK_DEV_RAM=y on top of defconfig: > > > https://people.collabora.com/~gtucker/lava/boot/debug/bzImage-85eac382fa06-blk-dev.config OK, this is a deviation from what I see here (on mkp's 4.19/scsi-queue not next though). -- Johannes Thumshirn Storage jthumsh...@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
RE: [PATCH 1/6] scsi: Add ufs transport class
Johannes,
Thanks a lot for your comments.
Cheers,
Avri
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org
> On Behalf Of Johannes Thumshirn
> Sent: Wednesday, August 01, 2018 2:17 PM
> To: Avri Altman
> Cc: Christoph Hellwig ; Hannes Reinecke ;
> Bart Van Assche ; James E.J. Bottomley
> ; Martin K. Petersen
> ; linux-scsi@vger.kernel.org; Stanislav
> Nijnikov ; Avi Shchislowski
> ; Alex Lemberg ;
> Subhash Jadavani ; Vinayak Holikatti
>
> Subject: Re: [PATCH 1/6] scsi: Add ufs transport class
>
> Hi Avri,
>
> On Wed, Aug 01, 2018 at 11:04:27AM +0300, Avri Altman wrote:
> [...
>
> > +#include
>
> Why do you include bsg.h here and bsg-lib.h in the scsi_transport_ufs.h?
>
> [...]
Right, will move both to the same place.
>
> > +#define to_ufs_internal(tmpl) container_of(tmpl, struct
> ufs_internal, t)
>
> I'd personally prefer this to be a inline function instead of a define
> for type safety reasons.
Ok.
>
> > +
> > +struct ufs_host_attrs {
> > + atomic_t next_port_id;
> > +};
> > +#define to_ufs_host_attrs(x) ((struct ufs_host_attrs *)(x)->shost_data)
>
> Ditto.
Ok.
>
> [...]
>
> > +
> > + port->id = atomic_inc_return(&ufs_host->next_port_id);
>
> Any reason you can't use an IDA for the port->id?
Ok. Will change to use it.
>
> [...]
> > +
> > + error = device_add(dev);
> > +
> > + if (error)
> > + return error;
>
> No blank line please.
Done.
>
> [...]
>
> > +#define dev_to_ufs_port(d) \
> > + container_of((d), struct ufs_port, dev)
>
> Inline function as well, please.
Done.
>
> --
> Johannes Thumshirn Storage
> jthumsh...@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
Re: [PATCH 2/6] scsi: ufs: Add ufs-bsg module
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote:
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SSCSI transport companion for UFS HBA
What is "SSCSI"?
> diff --git a/drivers/scsi/ufs/ufs_bsg.h b/drivers/scsi/ufs/ufs_bsg.h
> new file mode 100644
[ ... ]
> +struct ufs_bsg_request {
> + uint32_t msgcode;
> + struct utp_upiu_header header;
> + union {
> + struct utp_upiu_query qr;
> + struct utp_upiu_task_reqtr;
> + /* use utp_upiu_query to host the 4 dwards of uic command */
What are "dwards"?
> + struct utp_upiu_query uc;
> + } tsf;
> + uint8_t data[0];
The offset of the data member will change if a member will be added to the
union with a larger size than the existing members. That seems like an API
design bug to me.
> +} __packed;
Would the data member offsets be the same without "__packed"? If so, __packed
should be left out because it results in generation of suboptimal code on
architectures that do not support unaligned multi-byte reads (e.g. IA-64).
> +struct ufs_bsg_reply {
> + /*
> + * The completion result. Result exists in two forms:
> + * if negative, it is an -Exxx system errno value. There will
> + * be no further reply information supplied.
> + * else, it's the 4-byte scsi error result, with driver, host,
> + * msg and status fields. The per-msgcode reply structure
> + * will contain valid data.
> + */
> + uint32_t result;
> +
> + /* If there was reply_payload, how much was recevied ? */
Did you perhaps mean "received"?
> + uint32_t reply_payload_rcv_len;
> +
> + struct utp_upiu_header header;
> + union {
> + struct utp_upiu_query qr;
> + struct utp_upiu_task_rsptr;
> + struct utp_upiu_query uc;
> + } tsf;
> + uint8_t data[0];
> +};
> +
> +struct ufs_bsg_raw_upiu {
> + struct ufs_bsg_upiu request;
> + struct ufs_bsg_upiu reply;
> +};
Are any of the above data structures needed by user space software? Should these
data structure definitions perhaps be moved to a header file under include/uapi?
Thanks,
Bart.
Re: [PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote: > + wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot)); The above is the weirdest API I have seen so far for tag allocation. Why does ufshcd_get_tm_free_slot() does a linear search through a bitmap instead of using the sbitmap data structure? > + ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); > + /* Make sure that doorbell is committed immediately */ > + wmb(); This is the first time that I see someone claiming that issuing a write memory barrier causes the write to be executed faster than without memory barrier. Are you sure that comment is correct and that that wmb() is necessary? > + spin_unlock_irqrestore(host->host_lock, flags); > + > + /* wait until the task management command is completed */ > + err = wait_event_timeout(hba->tm_wq, > + test_bit(free_slot, &hba->tm_condition), > + msecs_to_jiffies(TM_CMD_TIMEOUT)); Did you perhaps start implementing the ufshcd_issue_tm_upiu_cmd() function by copy/pasting ufshcd_issue_tm_cmd()? Please don't do that and instead avoid code duplication by moving shared code in a new function. > + /* ignore the returning value here - ufshcd_check_query_response is > + * bound to fail since dev_cmd.query and dev_cmd.type were left empty. > + * read the response directly ignoring all errors. > + */ Have you verified this patch with checkpatch? This is not the proper kernel comment style. Thanks, Bart.
Re: [PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote:
> +/**
> + * ufshcd_exec_raw_upiu_cmd - API function for sending raw upiu commands
> + * @hba: per-adapter instance
> + * @req_upiu:upiu request - 8 dwards
> + * @rsp_upiu:upiu reply - 8 dwards
> + * @msgcode: message code, one of UPIU Transaction Codes Initiator to Target
> + * @desc_buff: pointer to descriptor buffer, NULL if NA
> + * @buff_len:descriptor size, 0 if NA
> + * @rw: either READ or WRITE
> + *
> + * Supports UTP Transfer requests (nop and query), and UTP Task
> + * Management requests.
> + * It is up to the caller to fill the upiu conent properly, as it will
> + * be copied without any further input validations.
> + */
> +int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
> + __be32 *req_upiu, __be32 *rsp_upiu,
> + int msgcode,
> + u8 *desc_buff, int *buff_len, int rw)
> +{
Again, what are "dwards"?
Documenting the size of a data structure in a function header is very weird.
Please change the data type of req_upiu and rsp_upio into a pointer to a
structure of the proper size.
Thanks,
Bart.
Re: [PATCH 5/6] scsi: ufs-bsg: Add support for raw upiu in ufs_bsg_request()
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote:
> + if (qr->opcode != UPIU_QUERY_OPCODE_WRITE_DESC ||
> + desc_size <= 0)
> + return -EINVAL;
Please use the full line length and don't split lines if that is not necessary.
> + ret = ufshcd_map_desc_id_to_length(bsg_host->hba, desc_id, buff_len);
> +
> + if (ret || !buff_len)
> + return -EINVAL;
Why is buff_len only tested after it has been passed as an argument to
ufshcd_map_desc_id_to_length()? That seems weird to me.
> +static int ufs_bsg_verify_query_size(unsigned int request_len,
> + unsigned int reply_len,
> + int rw, int buff_len)
> +{
> + int min_req_len = sizeof(struct ufs_bsg_request);
> + int min_rsp_len = sizeof(struct ufs_bsg_reply);
> +
> + if (rw == UFS_BSG_NOP)
> + goto null_buff;
> +
> + if (rw == WRITE)
> + min_req_len += buff_len;
Can the "goto" statement be avoided by using a switch/case on 'rw'?
Thanks,
Bart.
Re: [PATCH 6/6] scsi: ufs-bsg: Add support for uic commands in ufs_bsg_request()
On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote:
> + struct uic_command uc = {0};
Please use "{ }" or "{}" for structure initialization as is done elsewhere in
the kernel instead of "{0}".
> +#define UIC_CMD_SIZE (sizeof(u32) * 4)
Please add a comment above this definition that explains whether this constant
comes from the spec or whether it has another origin.
Thanks,
Bart.
Re: [PATCH 1/2] sysfs: Introduce sysfs_{un,}break_active_protection()
On Mon, Jul 30, 2018 at 11:40:51AM -0700, Bart Van Assche wrote: > Introduce these two functions and export them such that the next patch > can add calls to these functions from the SCSI core. > > Signed-off-by: Bart Van Assche > Cc: Greg Kroah-Hartman > Cc: Tejun Heo > Cc: Acked-by: Tejun Heo Thanks. -- tejun
Re: [PATCH 2/2] Avoid that SCSI device removal through sysfs triggers a deadlock
On Mon, Jul 30, 2018 at 11:40:52AM -0700, Bart Van Assche wrote:
> A long time ago the unfortunate decision was taken to add a self-
> deletion attribute to the sysfs SCSI device directory. That decision
> was unfortunate because self-deletion is really tricky. We can't drop
> that attribute because widely used user space software depends on it,
> namely the rescan-scsi-bus.sh script. Hence this patch that avoids
> that writing into that attribute triggers a deadlock. See also commit
> 7973cbd9fbd9 ("[PATCH] add sysfs attributes to scan and delete
> scsi_devices").
Acked-by: Tejun Heo
Thanks.
--
tejun
Re: [PATCH 11/15] target: export initiator port values for all sessions
On 07/19/2018 12:07 PM, Bart Van Assche wrote: > On Thu, 2018-07-19 at 11:38 -0500, Mike Christie wrote: >> On 07/19/2018 10:37 AM, Bart Van Assche wrote: >>> The general recommendation for configfs is that each attribute contains a >>> single value, just like for sysfs. Patch 11/15 exports two values through >>> a single attribute. Have you considered to split the above into two >> >> What about just making it the initiator port transport id so it aligns >> with the get_initiator_port_transport_id() comment for the other patch. >> For iscsi it would be 1 value with the format from SPC/SAM >> "target_name,i,0x,isid"? > > That sounds fine to me. > >>> attributes, namely the initiator name and the ISID? Can the initiator name >>> be changed into a soft link to the se_node_acl configfs directory to make >>> it easy for shell scripts to retrieve additional initiator configuration >>> information? >> >> Because the kernel is creating the session instead of it being driven >> from a mkdir, there are no existing functions for this. I do not know >> configfs code well, but I think making a function to do this is possible >> though. > > Initially configfs did not support creation of a directory from the kernel > side. Last time I brought this up with Christoph he replied that this > functionality has been added to configfs (if I understood Christoph > correctly). > >> What about the dynamic_acl case? Just make those a normal file? > > I'm not that familiar with dynamic ACLs. Is there a difference between > "dynamic ACL" generation and "demo mode"? How does this interact with the > generate_node_acls mode? Ah sorry, I think I made up that term. I was referring to when se_node_acl->dynamic_node_acl=1, so generate_node_acls=1 and demo mode and dynamic_node_acl=1 is the same state. > >> Just to make sure we are on the same page too. The initiator name is >> always the name of the acl right? It looked like that from >> target_fabric_make_nodeacl but I was wondering if you are asking for the >> symlink because there are some fabric module quirks where that is not >> the case. If it's the same names, then you know the acl already from the >> initiator name file. > > It depends on what is called the "initiator name". E.g. the SRP target > driver supports multiple formats to refer to a single initiator port. The > first version of the ib_srpt driver supported only 128-bit GIDs as initiator > name. Since the 64-bit prefix of a GID may change due to a reboot, later > on support for 64-bit GUIDs was added. During login three formats for > the initiator name are verified: GID, GUID without "0x" prefix and GUID > with "0x" prefix. In any case, target_alloc_session() will store a > pointer to the first struct se_node_acl that matches in sess->se_node_acl. > I think using the name stored in struct se_node_acl is fine in all cases. > Hey Bart, I did patches to add symlinks. There is one problem that this will break userspace though. The userspace tools assume that they can tear down a tpgt while sessions are running because currently a rmdir on the acl will force running sessions to be shutdown. For example, a FC or iscsi initiator can be logged into the target and userspace currently does something like this simplified sequence: for each object under a tpgt ulink luns rmdir luns rmdir acls rmdir tpt The problem is that because the session has a symlink to the acl and configfs has done a config_item_get on the acl config_item to make sure it does not get freed while in use the "rmdir acl" will now fail. I agree knowing the acl info is useful, so how about the following: 1. Create a file named "acl" in the session's dir. 2. For dynamic_node_acl=0 the acl file will return a empty string or "generate_node_acls=1" or "demo mode enabled". 3. For dynamic_node_acl=1 the acl file will return se_node_acl->initiatorname which is the name of the acl in /tpgt_$N/acls/.
Re: [PATCH 11/15] target: export initiator port values for all sessions
On 08/01/2018 11:44 AM, Mike Christie wrote: > 1. Create a file named "acl" in the session's dir. > 2. For dynamic_node_acl=0 the acl file will return a empty string or Miswrote this. Above should be dynamic_node_acl=1 > "generate_node_acls=1" or "demo mode enabled". > 3. For dynamic_node_acl=1 the acl file will return This should be dynamic_node_acl=0 > se_node_acl->initiatorname which is the name of the acl in > /tpgt_$N/acls/. >
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Wed, 2018-08-01 at 11:44 -0500, Mike Christie wrote: > I agree knowing the acl info is useful, so how about the following: > > 1. Create a file named "acl" in the session's dir. > 2. For dynamic_node_acl=0 the acl file will return a empty string or > "generate_node_acls=1" or "demo mode enabled". > 3. For dynamic_node_acl=1 the acl file will return > se_node_acl->initiatorname which is the name of the acl in > /tpgt_$N/acls/. Hello Mike, That sounds fine to me. My personal preference is that the "acl" file is empty if demo mode is enabled. Thanks, Bart.
Re: [RFC 0/6] scsi: scsi transport for ufs devices
On Mon, 2018-07-30 at 14:52 -0400, Douglas Gilbert wrote: > I designed 'struct sg_io_v4' found in include/uapi/linux/bsg.h to handle > storage > related protocols, not just the SCSI command set. After the guard, the next > two fields in that structure are: > __u32 protocol; /* [i] 0 -> SCSI , */ > __u32 subprotocol; /* [i] 0 -> SCSI command, 1 -> SCSI task > management function, */ > > So there is lots of room for other protocols. Hi Doug, That's great, but it seems like most bsg drivers use other data structures than struct sg_io_v4. See e.g. struct fc_bsg_request and struct fc_bsg_reply in include/uapi/scsi/scsi_bsg_fc.h. See also struct iscsi_bsg_request and struct iscsi_bsg_reply in include/scsi/scsi_bsg_iscsi.h. The output of git grep -nH ' = job->request;' did not reveal any bsg driver that uses struct sg_io_v4. Did I perhaps misunderstand something? Thanks, Bart.
Re: [PATCH 2/2] Avoid that SCSI device removal through sysfs triggers a deadlock
On Mon, 2018-07-30 at 11:40 -0700, Bart Van Assche wrote:
> A long time ago the unfortunate decision was taken to add a self-
> deletion attribute to the sysfs SCSI device directory. That decision
> was unfortunate because self-deletion is really tricky. We can't drop
> that attribute because widely used user space software depends on it,
> namely the rescan-scsi-bus.sh script. Hence this patch that avoids
> that writing into that attribute triggers a deadlock. See also commit
> 7973cbd9fbd9 ("[PATCH] add sysfs attributes to scan and delete
> scsi_devices").
[ ... ]
During my kernel tests of today I noticed that this patch makes booting
significantly slower: boot time for a VM increases from 6s to 157s. Martin,
please drop this patch series.
Thanks,
Bart.
Re: [PATCH 2/2] Avoid that SCSI device removal through sysfs triggers a deadlock
On Wed, 2018-08-01 at 21:16 +, Bart Van Assche wrote: > During my kernel tests of today I noticed that this patch makes booting > significantly slower: boot time for a VM increases from 6s to 157s. Martin, > please drop this patch series. Please ignore my previous message - these two patches are fine. The trouble was caused by a patch in another tree that I had merged in during my tests. I will repost this series. Bart.
Re: next/master boot: 179 boots: 11 failed, 167 passed with 1 offline (next-20180731)
On Wed, Aug 01, 2018 at 02:06:11PM +0200, Johannes Thumshirn wrote: > On Wed, Aug 01, 2018 at 08:00:44PM +0800, Ming Lei wrote: > > On Wed, Aug 01, 2018 at 07:52:19PM +0800, Ming Lei wrote: > > > On Wed, Aug 01, 2018 at 12:24:00PM +0100, Matt Hart wrote: > > > > On 1 August 2018 at 11:59, Mark Brown wrote: > > > > > On Wed, Aug 01, 2018 at 06:51:09PM +0800, Ming Lei wrote: > > > > > > > > > >> You may have to provide some clue, such as dmesg log, boot disk, ... > > > > > > > > > >> I guess you don't use virtio-scsi/virtio-blk since both run at blk-mq > > > > >> mode at default, even though without d5038a13eca72fb. > > > > > > > > > > Boot logs and so on can be found here: > > > > > > > > > > https://kernelci.org/boot/id/5b618c9f59b514931f96ba97/ > > > > > https://kernelci.org/boot/id/5b618ca359b514904d96bac5/ > > > > > https://kernelci.org/boot/id/5b618cbc59b51492e896baad/ > > > > > > > > > > (these are today's but the symptoms are the same.) The ramdisk is > > > > > unfortunately not linked through the UI, though we don't get that far. > > > > > > > > And a full LAVA boot log from my lab > > > > http://lava.streamtester.net/scheduler/job/138067 > > > > > > > > QEMU command line here: > > > > http://lava.streamtester.net/scheduler/job/138067#L75 > > > > > > > > And a LAVA job definition, which includes the url of the ramdisk and > > > > kernel: > > > > http://lava.streamtester.net/scheduler/job/138067/definition#defline76 > > > > > > > > > > Thanks for the sharing! > > > > > > I can reproduce this issue with above script/initrd/kernel config, and > > > looks > > > the issue disappeared after 'scsi_mod.use_blk_mq=0' is passed. > > > > > > Not see such issue with zero-day ktest config. > > > > > > Looks a bit weird, given SCSI_MQ is nothing related with ramdisk. > > > > Seems related with sr: > > > > 1) with scsi-mq > > [2.808204] ata2.01: NODEV after polling detection > > [2.809807] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 > > [2.827377] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM > > 2.5+ PQ: 0 ANSI: 5 > > > > 2) without scsi_mq > > [5.549135] ata2.01: NODEV after polling detection > > [5.554404] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 > > [5.596143] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM > > 2.5+ PQ: 0 ANSI: 5 > > [5.637126] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray > > [5.637870] cdrom: Uniform CD-ROM driver Revision: 3.20 > > [5.648940] sr 1:0:0:0: Attached scsi CD-ROM sr0 > > [5.661605] sr 1:0:0:0: Attached scsi generic sg0 type 5 > > > > > > We may need to take a look at recent SCSI change. > > [2.052168] ata2.01: NODEV after polling detection > [2.053690] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100 > [2.072269] scsi 1:0:0:0: CD-ROMQEMU QEMU DVD-ROM 2.5+ > P5 > [2.107220] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray > [2.107675] cdrom: Uniform CD-ROM driver Revision: 3.20 > [2.111851] sr 1:0:0:0: Attached scsi CD-ROM sr0 > [2.123560] sr 1:0:0:0: Attached scsi generic sg0 type 5 > > # cat /proc/cmdline > console=ttyS0,115200 root=/dev/ram0 debug verbose > > $ grep SCSI_MQ .config > CONFIG_SCSI_MQ_DEFAULT=y > > on Martin's latest 4.19/scsi-queue. I just checked my daily test log, looks this issue is reported 1st time on next-20180731. Thanks, Ming
