[PATCH 0/2, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
Hello Martin,
This patch series avoids that SCSI device removal through sysfs triggers a
deadlock. Please consider this patch series for the v4.19 kernel.
Thanks,
Bart.
Bart Van Assche (2):
sysfs: Introduce sysfs_{un,}break_active_protection()
Avoid that SCSI device removal through sysfs triggers a deadlock
drivers/scsi/scsi_sysfs.c | 20 --
fs/sysfs/file.c | 44 +++
include/linux/sysfs.h | 14 +
3 files changed, 76 insertions(+), 2 deletions(-)
--
2.18.0
Re: [PATCH 2/2] Avoid that SCSI device removal through sysfs triggers a deadlock
On Wed, 2018-08-01 at 22:58 +, Bart Van Assche wrote: > On Wed, 2018-08-01 at 21:16 +0000, 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. The following patch fixes the boot time regression I had reported yesterday: https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg74940.html. Bart.
[PATCH] sr: Avoid that opening a CD-ROM hangs with runtime power management enabled
Surround scsi_execute() calls with scsi_autopm_get_device() and
scsi_autopm_put_device(). Note: removing sr_mutex protection from
the scsi_cd_get() and scsi_cd_put() calls is safe because the
purpose of sr_mutex is to serialize cdrom_*() calls.
This patch avoids that complaints similar to the following appear in
the kernel log if runtime power management is enabled:
INFO: task systemd-udevd:650 blocked for more than 120 seconds.
Not tainted 4.18.0-rc7-dbg+ #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
systemd-udevd D28176 650513 0x0104
Call Trace:
__schedule+0x444/0xfe0
schedule+0x4e/0xe0
schedule_preempt_disabled+0x18/0x30
__mutex_lock+0x41c/0xc70
mutex_lock_nested+0x1b/0x20
__blkdev_get+0x106/0x970
blkdev_get+0x22c/0x5a0
blkdev_open+0xe9/0x100
do_dentry_open.isra.19+0x33e/0x570
vfs_open+0x7c/0xd0
path_openat+0x6e3/0x1120
do_filp_open+0x11c/0x1c0
do_sys_open+0x208/0x2d0
__x64_sys_openat+0x59/0x70
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Signed-off-by: Bart Van Assche
Cc: Maurizio Lombardi
Cc: Johannes Thumshirn
Cc: Alan Stern
Cc:
---
drivers/scsi/sr.c | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 3f3cb72e0c0c..d0389b20574d 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -523,18 +523,26 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
static int sr_block_open(struct block_device *bdev, fmode_t mode)
{
struct scsi_cd *cd;
+ struct scsi_device *sdev;
int ret = -ENXIO;
+ cd = scsi_cd_get(bdev->bd_disk);
+ if (!cd)
+ goto out;
+
+ sdev = cd->device;
+ scsi_autopm_get_device(sdev);
check_disk_change(bdev);
mutex_lock(_mutex);
- cd = scsi_cd_get(bdev->bd_disk);
- if (cd) {
- ret = cdrom_open(>cdi, bdev, mode);
- if (ret)
- scsi_cd_put(cd);
- }
+ ret = cdrom_open(>cdi, bdev, mode);
mutex_unlock(_mutex);
+
+ scsi_autopm_put_device(sdev);
+ if (ret)
+ scsi_cd_put(cd);
+
+out:
return ret;
}
@@ -562,6 +570,8 @@ static int sr_block_ioctl(struct block_device *bdev,
fmode_t mode, unsigned cmd,
if (ret)
goto out;
+ scsi_autopm_get_device(sdev);
+
/*
* Send SCSI addressing ioctls directly to mid level, send other
* ioctls to cdrom/block level.
@@ -570,15 +580,18 @@ static int sr_block_ioctl(struct block_device *bdev,
fmode_t mode, unsigned cmd,
case SCSI_IOCTL_GET_IDLUN:
case SCSI_IOCTL_GET_BUS_NUMBER:
ret = scsi_ioctl(sdev, cmd, argp);
- goto out;
+ goto put;
}
ret = cdrom_ioctl(>cdi, bdev, mode, cmd, arg);
if (ret != -ENOSYS)
- goto out;
+ goto put;
ret = scsi_ioctl(sdev, cmd, argp);
+put:
+ scsi_autopm_put_device(sdev);
+
out:
mutex_unlock(_mutex);
return ret;
--
2.18.0
Re: [PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands
On Thu, 2018-08-02 at 11:05 +, Avri Altman wrote: > -Original Message- > > From: Bart Van Assche > > Sent: Wednesday, August 01, 2018 6:28 PM > > [ ... ] > > > + 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, >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. > > Yes I did. > I wanted to avoid changing any of the driver's core functionality, just > adding the new one. That's not how it should be done. It is considered important in the Linux kernel to avoid code duplication so please have another look at how to avoid code duplication. 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: [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: [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 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: [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 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 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 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, _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, >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 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.
[PATCH 2/2] libiscsi: Annotate fall-through
This patch avoids that building with W=1 causes the compiler to complain about fall-through. Signed-off-by: Bart Van Assche Cc: Lee Duncan Cc: Chris Leech --- drivers/scsi/libiscsi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index b36bafd5a058..d5f2f368040f 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -1705,6 +1705,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc) sc->result = DID_NO_CONNECT << 16; break; } + /* fall through */ case ISCSI_STATE_IN_RECOVERY: reason = FAILURE_SESSION_IN_RECOVERY; sc->result = DID_IMM_RETRY << 16; -- 2.18.0
[PATCH 1/2] libiscsi: Annotate locking assumptions
This patch avoids that sparse reports the following:
drivers/scsi/libiscsi.c:1844:23: warning: context imbalance in
'iscsi_exec_task_mgmt_fn' - unexpected unlock
Signed-off-by: Bart Van Assche
Signed-off-by: Lee Duncan
Signed-off-by: Chris Leech
---
drivers/scsi/libiscsi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d6093838f5f2..b36bafd5a058 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1832,6 +1832,7 @@ static void iscsi_tmf_timedout(struct timer_list *t)
static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
struct iscsi_tm *hdr, int age,
int timeout)
+ __must_hold(>frwd_lock)
{
struct iscsi_session *session = conn->session;
struct iscsi_task *task;
--
2.18.0
[PATCH 0/2] Improve libiscsi source code annotations
Hello Martin, The two patches in this series suppress one gcc 8 compiler warning and one sparse warning. Please consider these for kernel v4.19. Note: these patches had been posted for the first time a while ago but I had not yet had the time to send these to you. See also https://www.mail-archive.com/open-iscsi@googlegroups.com/msg10136.html. Thanks, Bart. Bart Van Assche (2): libiscsi: Annotate locking assumptions libiscsi: Annotate fall-through drivers/scsi/libiscsi.c | 2 ++ 1 file changed, 2 insertions(+) -- 2.18.0
[PATCH 2/2] Avoid that SCSI device removal through sysfs triggers a deadlock
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").
This patch avoids that self-removal triggers the following deadlock:
==
WARNING: possible circular locking dependency detected
4.18.0-rc2-dbg+ #5 Not tainted
--
modprobe/6539 is trying to acquire lock:
8323c4cd (kn->count#202){}, at: kernfs_remove_by_name_ns+0x45/0x90
but task is already holding lock:
a6ec2c69 (>scan_mutex){+.+.}, at: scsi_remove_host+0x21/0x150
[scsi_mod]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (>scan_mutex){+.+.}:
__mutex_lock+0xfe/0xc70
mutex_lock_nested+0x1b/0x20
scsi_remove_device+0x26/0x40 [scsi_mod]
sdev_store_delete+0x27/0x30 [scsi_mod]
dev_attr_store+0x3e/0x50
sysfs_kf_write+0x87/0xa0
kernfs_fop_write+0x190/0x230
__vfs_write+0xd2/0x3b0
vfs_write+0x101/0x270
ksys_write+0xab/0x120
__x64_sys_write+0x43/0x50
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #0 (kn->count#202){}:
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0CPU1
lock(>scan_mutex);
lock(kn->count#202);
lock(>scan_mutex);
lock(kn->count#202);
*** DEADLOCK ***
2 locks held by modprobe/6539:
#0: efaf9298 (>mutex){}, at:
device_release_driver_internal+0x68/0x360
#1: a6ec2c69 (>scan_mutex){+.+.}, at:
scsi_remove_host+0x21/0x150 [scsi_mod]
stack backtrace:
CPU: 10 PID: 6539 Comm: modprobe Not tainted 4.18.0-rc2-dbg+ #5
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.0.0-prebuilt.qemu-project.org 04/01/2014
Call Trace:
dump_stack+0xa4/0xf5
print_circular_bug.isra.34+0x213/0x221
__lock_acquire+0x1a7e/0x1b50
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
See also https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg54525.html.
Fixes: ac0ece9174ac ("scsi: use device_remove_file_self() instead of
device_schedule_callback()")
Signed-off-by: Bart Van Assche
Cc: Greg Kroah-Hartman
Cc: Tejun Heo
Cc: Johannes Thumshirn
Cc:
---
drivers/scsi/scsi_sysfs.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index de122354d09a..3aee9464a7bf 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -722,8 +722,24 @@ static ssize_t
sdev_store_delete(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- if (device_remove_file_self(dev, attr))
- scsi_remove_device(to_scsi_device(dev));
+ struct kernfs_node *kn;
+
[PATCH 0/2] Avoid that SCSI device removal through sysfs triggers a deadlock
Hello Martin,
This patch series avoids that SCSI device removal through sysfs triggers a
deadlock. Please consider this patch series for the v4.19 kernel.
Thanks,
Bart.
Bart Van Assche (2):
sysfs: Introduce sysfs_{un,}break_active_protection()
Avoid that SCSI device removal through sysfs triggers a deadlock
drivers/scsi/scsi_sysfs.c | 20 --
fs/sysfs/file.c | 44 +++
include/linux/sysfs.h | 14 +
3 files changed, 76 insertions(+), 2 deletions(-)
--
2.18.0
[PATCH 1/2] sysfs: Introduce sysfs_{un,}break_active_protection()
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:
---
fs/sysfs/file.c | 44 +++
include/linux/sysfs.h | 14 ++
2 files changed, 58 insertions(+)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 5c13f29bfcdb..118fa197a35f 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -405,6 +405,50 @@ int sysfs_chmod_file(struct kobject *kobj, const struct
attribute *attr,
}
EXPORT_SYMBOL_GPL(sysfs_chmod_file);
+/**
+ * sysfs_break_active_protection - break "active" protection
+ * @kobj: The kernel object @attr is associated with.
+ * @attr: The attribute to break the "active" protection for.
+ *
+ * With sysfs, just like kernfs, deletion of an attribute is postponed until
+ * all active .show() and .store() callbacks have finished unless this function
+ * is called. Hence this function is useful in methods that implement self
+ * deletion.
+ */
+struct kernfs_node *sysfs_break_active_protection(struct kobject *kobj,
+ const struct attribute *attr)
+{
+ struct kernfs_node *kn;
+
+ kobject_get(kobj);
+ kn = kernfs_find_and_get(kobj->sd, attr->name);
+ if (kn)
+ kernfs_break_active_protection(kn);
+ return kn;
+}
+EXPORT_SYMBOL_GPL(sysfs_break_active_protection);
+
+/**
+ * sysfs_unbreak_active_protection - restore "active" protection
+ * @kn: Pointer returned by sysfs_break_active_protection().
+ *
+ * Undo the effects of sysfs_break_active_protection(). Since this function
+ * calls kernfs_put() on the kernfs node that corresponds to the 'attr'
+ * argument passed to sysfs_break_active_protection() that attribute may have
+ * been removed between the sysfs_break_active_protection() and
+ * sysfs_unbreak_active_protection() calls, it is not safe to access @kn after
+ * this function has returned.
+ */
+void sysfs_unbreak_active_protection(struct kernfs_node *kn)
+{
+ struct kobject *kobj = kn->parent->priv;
+
+ kernfs_unbreak_active_protection(kn);
+ kernfs_put(kn);
+ kobject_put(kobj);
+}
+EXPORT_SYMBOL_GPL(sysfs_unbreak_active_protection);
+
/**
* sysfs_remove_file_ns - remove an object attribute with a custom ns tag
* @kobj: object we're acting for
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index b8bfdc173ec0..3c12198c0103 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -237,6 +237,9 @@ int __must_check sysfs_create_files(struct kobject *kobj,
const struct attribute **attr);
int __must_check sysfs_chmod_file(struct kobject *kobj,
const struct attribute *attr, umode_t mode);
+struct kernfs_node *sysfs_break_active_protection(struct kobject *kobj,
+ const struct attribute *attr);
+void sysfs_unbreak_active_protection(struct kernfs_node *kn);
void sysfs_remove_file_ns(struct kobject *kobj, const struct attribute *attr,
const void *ns);
bool sysfs_remove_file_self(struct kobject *kobj, const struct attribute
*attr);
@@ -350,6 +353,17 @@ static inline int sysfs_chmod_file(struct kobject *kobj,
return 0;
}
+static inline struct kernfs_node *
+sysfs_break_active_protection(struct kobject *kobj,
+ const struct attribute *attr)
+{
+ return NULL;
+}
+
+static inline void sysfs_unbreak_active_protection(struct kernfs_node *kn)
+{
+}
+
static inline void sysfs_remove_file_ns(struct kobject *kobj,
const struct attribute *attr,
const void *ns)
--
2.18.0
Re: [RFC 0/6] scsi: scsi transport for ufs devices
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? Bart.
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
On Mon, 2018-07-30 at 10:31 -0700, t...@kernel.org wrote: > On Mon, Jul 30, 2018 at 05:28:11PM +0000, Bart Van Assche wrote: > > It's not clear to me how the sysfs_break_active_protection() should obtain > > the struct kernfs_node pointer to the attribute. Calling that function > > before > > device_remove_file_self() causes a double call to > > kernfs_break_active_protection(), > > which is wrong. Calling kernfs_find_and_get(kobj->sd, attr->name) after the > > So, if you braek active protection explicitly, there's no need to call > remove_self(). It can just use regular remove. But how to avoid that scsi_remove_device(to_scsi_device(dev)) gets called multiple times when using device_remove_self() and in case of concurrent writes into the SCSI device "delete" sysfs attribute? Thanks, Bart.
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
On Mon, 2018-07-30 at 07:13 -0700, t...@kernel.org wrote: > Also, wouldn't it be better to just expose sysfs_break/unbreak and > then do sth like the following from scsi? > > kobject_get(); > sysfs_break_active_protection(); > do normal sysfs removal; > sysfs_unbreak..(); > kobject_put(); Hello Tejun, It's not clear to me how the sysfs_break_active_protection() should obtain the struct kernfs_node pointer to the attribute. Calling that function before device_remove_file_self() causes a double call to kernfs_break_active_protection(), which is wrong. Calling kernfs_find_and_get(kobj->sd, attr->name) after the attribute has been removed results in a NULL pointer because the attribute that that call tries to look up has already been removed. Should I proceed with the approach proposed in the patches attached to a previous e-mail? Thanks, Bart.
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
On Thu, 2018-07-26 at 06:35 -0700, Tejun Heo wrote: > Making removal asynchronous this way sometimes causes issues because > whether the user sees the device released or not is racy. Hello Tejun, How could this change cause any user-visible changes? My understanding is that any work queued with task_work_add() is executed before system call execution leaves kernel context and returns back to user space context. Thanks, Bart.
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
On Thu, 2018-07-26 at 07:14 -0700, t...@kernel.org wrote:
> Hello,
>
> On Thu, Jul 26, 2018 at 02:09:41PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-07-26 at 06:35 -0700, Tejun Heo wrote:
> > > Making removal asynchronous this way sometimes causes issues because
> > > whether the user sees the device released or not is racy.
> > > kernfs/sysfs have mechanisms to deal with these cases - remove_self
> > > and kernfs_break_active_protection(). Have you looked at those?
> >
> > Hello Tejun,
> >
> > The call stack in the patch description shows that sdev_store_delete() is
> > involved in the deadlock. The implementation of that function is as follows:
> >
> > static ssize_t
> > sdev_store_delete(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t count)
> > {
> > if (device_remove_file_self(dev, attr))
> > scsi_remove_device(to_scsi_device(dev));
> > return count;
> > };
> >
> > device_remove_file_self() calls sysfs_remove_file_self() and that last
> > function calls kernfs_remove_self(). In other words, kernfs_remove_self()
> > is already being used. Please let me know if I misunderstood your comment.
>
> So, here, because scsi_remove_device() is the one involved in the
> circular dependency, just breaking the dependency chain on the file
> itself (self removal) isn't enough. You can wrap the whole operation
> with kernfs_break_active_protection() to also move
> scsi_remove_device() invocation outside the kernfs synchronization.
> This will need to be piped through sysfs but shouldn't be too complex.
Hello Tejun,
I have tried to implement the above but my implementation triggered a kernel
warning. Can you have a look at the attached patches and see what I did wrong?
Thanks,
Bart.
The kernel warning I ran into is as follows:
kernfs_put: 6:0:0:0/delete: released with incorrect active_ref 2147483647
WARNING: CPU: 6 PID: 1014 at fs/kernfs/dir.c:527 kernfs_put+0x136/0x180
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.0.0-prebuilt.qemu-project.org 04/01/2014
RIP: 0010:kernfs_put+0x136/0x180
Call Trace:
evict+0xc1/0x190
__dentry_kill+0xc4/0x150
shrink_dentry_list+0x9e/0x1c0
shrink_dcache_parent+0x63/0x70
d_invalidate+0x42/0xb0
lookup_fast+0x278/0x2a0
walk_component+0x38/0x450
link_path_walk+0x2a8/0x4f0
path_openat+0x89/0x13a0
do_filp_open+0x8c/0xf0
do_sys_open+0x1a6/0x230
do_syscall_64+0x4f/0x110
entry_SYSCALL_64_after_hwframe+0x44/0xa9
From f280e1cb31eda63c47db02574dfdfaee8a3f1dbc Mon Sep 17 00:00:00 2001
From: Bart Van Assche
Date: Thu, 26 Jul 2018 09:23:08 -0700
Subject: [PATCH 1/3] fs/sysfs: Introduce sysfs_remove_file_self_w_cb()
This patch makes it possible to execute more code than only the
__kernfs_remove() call while the active protection is dropped.
Signed-off-by: Bart Van Assche
Cc: Greg Kroah-Hartman
Cc: Rafael J. Wysocki
---
fs/sysfs/file.c | 17 -
include/linux/sysfs.h | 16
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 5c13f29bfcdb..db9386070571 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -429,7 +429,12 @@ EXPORT_SYMBOL_GPL(sysfs_remove_file_ns);
*
* See kernfs_remove_self() for details.
*/
-bool sysfs_remove_file_self(struct kobject *kobj, const struct attribute *attr)
+bool sysfs_remove_file_self_w_cb(struct kobject *kobj,
+ const struct attribute *attr,
+ void (*cb)(struct kobject *kobj,
+ const struct attribute *attr,
+ void *data),
+ void *data)
{
struct kernfs_node *parent = kobj->sd;
struct kernfs_node *kn;
@@ -440,11 +445,21 @@ bool sysfs_remove_file_self(struct kobject *kobj, const struct attribute *attr)
return false;
ret = kernfs_remove_self(kn);
+ if (ret && cb) {
+ kernfs_break_active_protection(kn);
+ cb(kobj, attr, data);
+ kernfs_break_active_protection(kn);
+ }
kernfs_put(kn);
return ret;
}
+bool sysfs_remove_file_self(struct kobject *kobj, const struct attribute *attr)
+{
+ return sysfs_remove_file_self_w_cb(kobj, attr, NULL, NULL);
+}
+
void sysfs_remove_files(struct kobject *kobj, const struct attribute **ptr)
{
int i;
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index b8bfdc173ec0..3c954d677736 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -239,6 +239,12 @@ int __must_check sysfs_chmod_file(struct kobject *kobj,
const struct attribute *attr, umode_t mode);
void sysfs_remove_file_ns(struct kobject *kobj, const struct attribute *attr,
const void *ns);
+bool sysfs_remove_file_self_w_cb(struct kobject *kobj,
+ const struct attribute *attr,
+ void (*cb)(struct kobject *kobj,
+ const struct attribute *attr,
+ void *),
+ void *data);
bool sysfs
[PATCH] qedi: Fix a potential buffer overflow
Tell snprintf() to store at most 255 characters in the output buffer instead of 256. This patch avoids that smatch reports the following warning: drivers/scsi/qedi/qedi_main.c:891: qedi_get_boot_tgt_info() error: snprintf() is printing too much 256 vs 255 Signed-off-by: Bart Van Assche Cc: Cc: --- drivers/scsi/qedi/qedi_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c index 682f3ce31014..ea62180d9ec8 100644 --- a/drivers/scsi/qedi/qedi_main.c +++ b/drivers/scsi/qedi/qedi_main.c @@ -888,7 +888,7 @@ static void qedi_get_boot_tgt_info(struct nvm_iscsi_block *block, ipv6_en = !!(block->generic.ctrl_flags & NVM_ISCSI_CFG_GEN_IPV6_ENABLED); - snprintf(tgt->iscsi_name, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n", + snprintf(tgt->iscsi_name, sizeof(tgt->iscsi_name), "%s\n", block->target[index].target_name.byte); tgt->ipv6_en = ipv6_en; -- 2.18.0
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
On Thu, 2018-07-26 at 06:35 -0700, Tejun Heo wrote:
> Making removal asynchronous this way sometimes causes issues because
> whether the user sees the device released or not is racy.
> kernfs/sysfs have mechanisms to deal with these cases - remove_self
> and kernfs_break_active_protection(). Have you looked at those?
Hello Tejun,
The call stack in the patch description shows that sdev_store_delete() is
involved in the deadlock. The implementation of that function is as follows:
static ssize_t
sdev_store_delete(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
if (device_remove_file_self(dev, attr))
scsi_remove_device(to_scsi_device(dev));
return count;
};
device_remove_file_self() calls sysfs_remove_file_self() and that last
function calls kernfs_remove_self(). In other words, kernfs_remove_self()
is already being used. Please let me know if I misunderstood your comment.
Thanks,
Bart.
[PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
This patch avoids that self-removal triggers the following deadlock:
==
WARNING: possible circular locking dependency detected
4.18.0-rc2-dbg+ #5 Not tainted
--
modprobe/6539 is trying to acquire lock:
8323c4cd (kn->count#202){}, at: kernfs_remove_by_name_ns+0x45/0x90
but task is already holding lock:
a6ec2c69 (>scan_mutex){+.+.}, at: scsi_remove_host+0x21/0x150
[scsi_mod]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (>scan_mutex){+.+.}:
__mutex_lock+0xfe/0xc70
mutex_lock_nested+0x1b/0x20
scsi_remove_device+0x26/0x40 [scsi_mod]
sdev_store_delete+0x27/0x30 [scsi_mod]
dev_attr_store+0x3e/0x50
sysfs_kf_write+0x87/0xa0
kernfs_fop_write+0x190/0x230
__vfs_write+0xd2/0x3b0
vfs_write+0x101/0x270
ksys_write+0xab/0x120
__x64_sys_write+0x43/0x50
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #0 (kn->count#202){}:
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0CPU1
lock(>scan_mutex);
lock(kn->count#202);
lock(>scan_mutex);
lock(kn->count#202);
*** DEADLOCK ***
2 locks held by modprobe/6539:
#0: efaf9298 (>mutex){}, at:
device_release_driver_internal+0x68/0x360
#1: a6ec2c69 (>scan_mutex){+.+.}, at:
scsi_remove_host+0x21/0x150 [scsi_mod]
stack backtrace:
CPU: 10 PID: 6539 Comm: modprobe Not tainted 4.18.0-rc2-dbg+ #5
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.0.0-prebuilt.qemu-project.org 04/01/2014
Call Trace:
dump_stack+0xa4/0xf5
print_circular_bug.isra.34+0x213/0x221
__lock_acquire+0x1a7e/0x1b50
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
See also https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg54525.html.
Suggested-by: Eric W. Biederman
Fixes: ac0ece9174ac ("scsi: use device_remove_file_self() instead of
device_schedule_callback()")
Signed-off-by: Bart Van Assche
Cc: Eric W. Biederman
Cc: Tejun Heo
Cc: Hannes Reinecke
Cc: Johannes Thumshirn
Cc: Ingo Molnar
Cc: Oleg Nesterov
Cc:
---
drivers/scsi/scsi_sysfs.c | 48 +++
kernel/task_work.c| 1 +
2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index de122354d09a..c43f645900d4 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
@@ -718,14 +719,53 @@ store_rescan_field (struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field);
+struct remove_dev_work {
+ struct callback_headhead;
+ struct scsi_device *sdev;
+};
+
+static void delete_sdev(struct callback_head *head)
+{
+ struct remove_dev_work *work = container_of(head, typeof(*work), head);
+ struct scsi_device *sdev = work->sdev;
+
+ scsi_remove_device(sdev);
+ kfree(work);
+ scsi_device_put(sdev);
+}
+
static ssize_t
sdev_store_d
Re: [PATCH v2] scsi_debug: add cmd abort option to every_nth
On Sat, 2018-07-21 at 01:10 -0400, Douglas Gilbert wrote: > This patch is motivated by a response in the thread: > Re: [PATCH 0/5]stop normal completion path entering a timeout req > by Jianchao Wang . It generalizes the error injection of > blk_abort_request() to use scsi_debug's "every_nth" mechanism. > Ref with original patch to scsi_debug: > > https://lore.kernel.org/lkml/a68ad043-26a1-d3d8-2009-504ba4230...@oracle.com/ Reviewed-by: Bart Van Assche
Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete
On Mon, 2018-07-23 at 08:37 -0600, Keith Busch wrote:
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 8932ae81a15a..2715cdaa669c 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -296,6 +296,20 @@ enum blk_eh_timer_return scsi_times_out(struct request
> *req)
> rtn = host->hostt->eh_timed_out(scmd);
>
> if (rtn == BLK_EH_DONE) {
> + /*
> + * For blk-mq, we must set the request state to complete now
> + * before sending the request to the scsi error handler. This
> + * will prevent a use-after-free in the event the LLD manages
> + * to complete the request before the error handler finishes
> + * processing this timed out request.
> + *
> + * If the request was already completed, then the LLD beat the
> + * time out handler from transferring the request to the scsi
> + * error handler. In that case we can return immediately as no
> + * further action is required.
> + */
> + if (req->q->mq_ops && !blk_mq_mark_complete(req))
> + return rtn;
> if (scsi_abort_command(scmd) != SUCCESS) {
> set_host_byte(scmd, DID_TIME_OUT);
> scsi_eh_scmd_add(scmd);
Hello Keith,
What will happen if a completion occurs after scsi_times_out() has started and
before or during the host->hostt->eh_timed_out()? Can that cause a
use-after-free
in .eh_timed_out()? Can that cause .eh_timed_out() to return BLK_EH_RESET_TIMER
when it should return BLK_EH_DONE? Can that cause blk_mq_rq_timed_out() to call
blk_add_timer() when that function shouldn't be called?
Thanks,
Bart.
Re: [PATCH] scsi_debug: add cmd abort option to every_nth
On Fri, 2018-07-20 at 15:21 -0400, Douglas Gilbert wrote:
> /* Complete the processing of the thread that queued a SCSI command to this
> @@ -4459,6 +4462,11 @@ static int schedule_resp(struct scsi_cmnd *cmnd,
> struct sdebug_dev_info *devip,
> sd_dp->issuing_cpu = raw_smp_processor_id();
> sd_dp->defer_t = SDEB_DEFER_WQ;
> schedule_work(_dp->ew.work);
> + if (unlikely(sqcp->inj_cmd_abort)) {
> + blk_abort_request(cmnd->request);
> + sdev_printk(KERN_INFO, sdp, "abort request tag %d\n",
> + cmnd->request->tag);
> + }
> }
> if (unlikely((SDEBUG_OPT_Q_NOISE & sdebug_opts) &&
>(scsi_result == device_qfull_result)))
Should the sdev_printk() call occur before the blk_abort_request() call to
avoid that the sdev_printk() call triggers a use-after-free?
Does the above change cause schedule_resp() to call both blk_abort_request()
and scsi_done()? I think that's wrong. A SCSI driver should call one of
these two functions but not both.
Thanks,
Bart.
Re: [PATCH 02/15] target: fix isid copying and comparision
On Fri, 2018-07-20 at 16:08 -0500, Mike Christie wrote:
> Hey Bart and Christoph,
>
> Bart, I noticed we basically had what you are requesting but Christoph
> had moved the id code from the fabric drivers to lio core in this commit:
>
> commit 2650d71e244fb3637b5f58a0080682a8bf9c7091
> Author: Christoph Hellwig
> Date: Fri May 1 17:47:58 2015 +0200
>
> target: move transport ID handling to the core
Hello Mike,
I'm not in favor of reverting Christoph's patch because that patch simplified
the target code significantly. On the other hand, it's inconvenient that with
the current approach there is some code and knowledge in the target core that
should be in target drivers. I think that the code for parsing the initiator
name in srp_get_pr_transport_id() should be in the SRP target driver instead
of the core. When I added support for a new initiator name format in the SRP
target driver I overlooked that I had to update srp_get_pr_transport_id()
because that function is in the core instead of ib_srpt.c. See also commit
2dc98f09f9e6 ("IB/srpt: Use the source GUID as session name"). Christoph, do
you want me to add support for the new ib_srpt initiator name format in
drivers/target/target_core_fabric_lib.c or should I find a way to move the
code for parsing the initiator name into ib_srpt.c?
Thanks,
Bart.
Re: [PATCH 11/15] target: export initiator port values for all sessions
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? > 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. Bart.
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Wed, 2018-07-18 at 21:15 -0500, Mike Christie wrote: > Oh wait, I think I know what you mean. I should have written "se_node_acl" instead of "se_portal_group". Bart.
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
> + if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
> + len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n",
> +se_sess->se_node_acl->initiatorname,
> +_sess->sess_bin_isid);
> + } else {
> + len = snprintf(page, PAGE_SIZE, "%s\n",
> +se_sess->se_node_acl->initiatorname);
> + }
Hello Mike,
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
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?
Thanks,
Bart.
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On Wed, 2018-07-18 at 22:47 -0500, Mike Christie wrote: > Hey Bart, I looked into this some more and this value is also being used > as the scsiAttIntrPortIndex. For that use, does it need to be unique > across a target when the target has multiple ports? > > So I think it needs to be on the se_wwn right? Hello Mike, I think the best solution would be to decouple the session index that is used to export session information through configfs from the session index used by the SCSI-MIB (scsiAttIntrPortIndex). That approach would allow to make both indexes consecutive integers in all cases for both interfaces. However, neither configfs nor the SCSI-MIB requires that session indexes are consecutive integers. So I think moving se_sess_idr_lock and se_sess_idr into struct se_wwn is fine. Bart.
Re: [PATCH 02/15] target: fix isid copying and comparision
On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: > On 07/18/2018 07:03 PM, Mike Christie wrote: > > On 07/18/2018 05:09 PM, Bart Van Assche wrote: > > > [ ... ] > > > is that these involve a transport ID and that that transport ID can be up > > > to 228 > > > bytes long for iSCSI. > > > > I am talking about the Initiator Session ID above. That along with the > > iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout > > table 508 or in SAM 5r21 checkout table A.4. > > > > So in the SCSI specs as part of the Initiator Port Transport ID we have > > this from that SAM table: > > > > The Initiator Session Identifier (ISID) portion of the string is a UTF-8 > > encoded hexadecimal representation of a six byte binary value. > > > > --- > > > > In the PR parts of SPC it sometimes mentions only "Transport ID" but > > then clarifies the initiator port so I am assuming in those cases it > > means "Initiator Port Transport ID" so it is both the name and isid for > > iscsi. > > It looks like we are supposed to go by what the initiator specifies in > the TPID field, so it can be either the Transport ID or Initiator Port > Transport ID. Hello Mike, Since the ISID is iSCSI-specific I think that all code that knows about the ISID and its encoding should be in the iSCSI target driver instead of the target core. Do you think an approach similar to that of the SCST function iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller of that function is in source file scst/src/scst_targ.c: res = sess->tgt->tgtt->get_initiator_port_transport_id( sess->tgt, sess, >transport_id); Thanks, Bart.
Re: [PATCH 12/15] target: add callout to test a session
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > + int (*test_session)(struct se_session *, u8 timeout); Does any of the patches in this series define a test_session callback function? What is the unit of the timeout parameter? 1/HZ, ms or s? Thanks, Bart.
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > Export the initiator port info in configfs Does configfs support soft links? Can this information be exported as a soft link from the session directory to the struct se_portal_group configfs object? Thanks, Bart.
Re: [PATCH 09/15] target: add session dir in configfs
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
> +static void target_fabric_session_release(struct config_item *item)
> +{
> + struct se_session *se_sess = container_of(to_config_group(item),
> + struct se_session, group);
> + target_release_session(se_sess);
> +}
Please consider changing the target_fabric_ prefix into target_ for all new
functions. Otherwise this patch looks fine to me.
Bart.
Re: [PATCH 08/15] target: add session removal function
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > This adds a function to remove a session which should be used by > drivers that use target_setup_session. > > All the drivers but iscsi and tcm_fc were doing calling > transport_deregister_session_configfs and then immediately calling > transport_deregister_session or just calling > transport_deregisteir_session. ^ transport_deregister_session? Anyway: Reviewed-by: Bart Van Assche
Re: [PATCH 07/15] target: rename target_alloc_session
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > Rename target_alloc_session to target_setup_session to avoid > confusion with the other transport session allocation > function that only allocates the session and because > the target_alloc_session does so more. It allocates the ^ much? > session, sets up the nacl and egisters the session. > > The next patch will then add a remove function to match the > setup in this one, so it should make sense for all drivers, > except iscsi, to just call those 2 functions to setup and remove > a session. Anyway: Reviewed-by: Bart Van Assche
Re: [PATCH 06/15] target: make transport_init_session_tags static
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > transport_init_session_tags is only called from target_core_transport.c > so make it static. Reviewed-by: Bart Van Assche
Re: [PATCH 05/15] target: remove sess_get_index
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > sess_get_index is meaninless for most drivers. For iscsi, it ^^ meaningless? > is the same as the se_session->sid now and for fcoe it was just > the port id which would not work if multiple initiators > connected to the same target port. So just use the se_session sid > for all drivers and remove sess_get_index callout. Anyway: Reviewed-by: Bart Van Assche
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > diff --git a/drivers/target/target_core_transport.c > b/drivers/target/target_core_transport.c > index 75ddbbb..97a1ee5 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -55,6 +55,8 @@ > > static struct workqueue_struct *target_completion_wq; > static struct kmem_cache *se_sess_cache; > +static DEFINE_SPINLOCK(se_sess_idr_lock); > +static DEFINE_IDR(se_sess_idr); Is it necessary that se_sess_idr_lock and se_sess_idr are global? Could these two data structures be members of the data structure associated with /sys/kernel/config/target/iscsi/$port/$tpg (struct se_portal_group?)? Thanks, Bart.
Re: [PATCH 03/15] target: fix __transport_register_session locking
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > When __transport_register_session is called from > transport_register_session irqs will already have been disabled, > so we do not want the unlock irq call to enable them until > the higher level has done the final > spin_unlock_irqrestore/spin_unlock_irq. > > This has __transport_register_session use the save/restore > call. Reviewed-by: Bart Van Assche
Re: [PATCH 02/15] target: fix isid copying and comparision
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
> The isid is 48 bits, and in hex string format it's 12 bytes.
> We are currently copying the 12 byte hex string to a u64
> so we can easily compare it, but this has the problem that
> only 8 bytes of the 12 bytes are copied.
>
> The next patches will want to print se_session sess_bin_isid
> so this has us use hex2bin to when converting from the hex
> sting to the bin value.
^
string?
Which spec defines that an ISID is 48 bits? All I know about SCSI registrations
is that these involve a transport ID and that that transport ID can be up to 228
bytes long for iSCSI.
> if (isid != NULL) {
> - pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
> + if (hex2bin((u8 *)_reg->pr_reg_bin_isid, isid, 6)) {
> + pr_err("Invalid isid %s\n", isid);
> + goto free_reg;
> + }
Why is it necessary to convert the ISID from hex to binary format? If this
conversion is necessary, isn't that something that should be handled by the
iSCSI target driver instead of the target core?
Thanks,
Bart.
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On Wed, 2018-07-18 at 01:22 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > Signed-off-by: Sreekanth Reddy > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94b939b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + st->smid = 0; > } How can this patch be correct without memory barrier between the atomic set and the st->smid assignment? Bart.
Re: [PATCH] sg: fix minor memory leak in error path
On 07/12/18 13:30, Tony Battersby wrote: Fix a minor memory leak when there is an error opening a /dev/sg device. Reviewed-by: Bart Van Assche
Re: [PATCH] sg: fix minor memory leak in error path
On Thu, 2018-07-12 at 14:46 -0400, Tony Battersby wrote: > Fix a minor memory leak when there is an error opening a /dev/sg device. Please Cc: stable for patches like this and please also add a Fixes: tag. Thanks, Bart.
Re: [PATCH] scsi: sd_zbc: Fix variable type and bogus comment
On 07/02/18 23:23, Damien Le Moal wrote: Fix the description of sd_zbc_check_zone_size() to correctly explain that the returned value is a number of device blocks, not bytes. Additionally, the 32 bits "ret" variable used in this function may truncate the 64 bits zone_blocks variable value upon return. To fix this, change "ret" type to s64. Reviewed-by: Bart Van Assche
Re: [PATCH] klist: Make it safe to use klists in atomic context
On 06/28/18 08:37, Greg Kroah-Hartman wrote:
On Fri, Jun 22, 2018 at 02:54:49PM -0700, Bart Van Assche wrote:
In the scsi_transport_srp implementation it cannot be avoided to
iterate over a klist from atomic context when using the legacy
block layer instead of blk-mq. Hence this patch that makes it safe
to use klists in atomic context. This patch avoids that lockdep
reports the following:
WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
Possible interrupt unsafe locking scenario:
CPU0CPU1
lock(&(>k_lock)->rlock);
local_irq_disable();
lock(&(>__queue_lock)->rlock);
lock(&(>k_lock)->rlock);
lock(&(>__queue_lock)->rlock);
stack backtrace:
Workqueue: kblockd blk_timeout_work
Call Trace:
dump_stack+0xa4/0xf5
check_usage+0x6e6/0x700
__lock_acquire+0x185d/0x1b50
lock_acquire+0xd2/0x260
_raw_spin_lock+0x32/0x50
klist_next+0x47/0x190
device_for_each_child+0x8e/0x100
srp_timed_out+0xaf/0x1d0 [scsi_transport_srp]
scsi_times_out+0xd4/0x410 [scsi_mod]
blk_rq_timed_out+0x36/0x70
blk_timeout_work+0x1b5/0x220
process_one_work+0x4fe/0xad0
worker_thread+0x63/0x5a0
kthread+0x1c1/0x1e0
ret_from_fork+0x24/0x30
See also commit c9ddf73476ff ("scsi: scsi_transport_srp: Fix shost
to rport translation").
Signed-off-by: Bart Van Assche
Cc: Martin K. Petersen
Cc: James Bottomley
---
lib/klist.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
No objection from me:
Acked-by: Greg Kroah-Hartman
This should probably go through the scsi tree, right?
Thanks Greg for the ack. I wasn't sure which maintainer to send this
patch to since lib/klist.c is not mentioned in the MAINTAINERS file.
Martin, are you OK with queuing this patch?
Thanks,
Bart.
[PATCH] sd_zbc: Remove an assignment from sd_zbc_setup_report_cmnd()
Since nr_bytes == blk_rq_bytes(rq) == rq->__data_len, the rq->__data_len = nr_bytes assignment does not modify the value of rq->__data_len. Hence remove that assignment. Note: the code in sd_done() that sets the residual to zero for zone report requests is not affected by this patch. Signed-off-by: Bart Van Assche Reviewed-by: Damien Le Moal Cc: Hannes Reinecke Cc: Johannes Thumshirn --- drivers/scsi/sd_zbc.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index a14fef11776e..160b79619d30 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -148,12 +148,6 @@ int sd_zbc_setup_report_cmnd(struct scsi_cmnd *cmd) cmd->transfersize = sdkp->device->sector_size; cmd->allowed = 0; - /* -* Report may return less bytes than requested. Make sure -* to report completion on the entire initial request. -*/ - rq->__data_len = nr_bytes; - return BLKPREP_OK; } -- 2.17.1
Re: Possible race in completion with SRP after abort with latest upstream kernel 4.17.0+
On 06/07/18 08:34, Laurence Oberman wrote: On Thu, 2018-06-07 at 15:21 +, Bart Van Assche wrote: On Thu, 2018-06-07 at 09:23 -0400, Laurence Oberman wrote: Have you seen this before, let me know what else you want from the dump while I look further. Hello Laurence, I haven't seen this before and I can't reproduce this by running srp- tests against Linus' latest master. So it would be appreciated if you could tell me how to reproduce this behavior or if you could run a bisect. OK, let me see if I can get it to fail reliably to narrow it down. Hello Laurence, In the kernel oops report I found v4.17.0+ as kernel version. Is that kernel v4.17 with some patches applied or rather a kernel close to v4.18-rc1? Since the v4.18 merge window closed several bug fixes have been merged upstream for the block layer. Thanks, Bart.
Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset
On 06/24/18 23:10, Sreekanth Reddy wrote: Before calling scsi_internal_device_block_nowait() API; driver sets sas_device_priv_data->block flag as one. And in the scsih_qcmd() driver checks for this flag as shown below and return the commands with host busy status. } else if (sas_target_priv_data->tm_busy || sas_device_priv_data->block) return SCSI_MLQUEUE_DEVICE_BUSY; That's exactly the kind of construct that should occur in the SCSI core or block layer core and not in a SCSI LLD. Additionally, as explained before, the construct you described above is racy. Bart.
Re: [PATCH 2/2] qedi: Fix truncation of target name
On Wed, 2018-06-27 at 05:14 -0700, Nilesh Javali wrote: > Use sprintf instead of snprintf to fix truncation of target name. > This fix is extension of patch > "scsi: qedi: Fix truncation of CHAP name and secret". > > Signed-off-by: Nilesh Javali > --- > drivers/scsi/qedi/qedi_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c > index cf274a7..85491da 100644 > --- a/drivers/scsi/qedi/qedi_main.c > +++ b/drivers/scsi/qedi/qedi_main.c > @@ -888,8 +888,8 @@ static void qedi_get_boot_tgt_info(struct nvm_iscsi_block > *block, > ipv6_en = !!(block->generic.ctrl_flags & >NVM_ISCSI_CFG_GEN_IPV6_ENABLED); > > - snprintf(tgt->iscsi_name, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n", > - block->target[index].target_name.byte); > + sprintf(tgt->iscsi_name, "%.*s\n", NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, > + block->target[index].target_name.byte); > > tgt->ipv6_en = ipv6_en; Also this patch changes code that is fine into code that can trigger a buffer overflow. Additionally, for humans it is much harder than necessary to verify the above code. Please consider to use sizeof(tgt->iscsi_name) - 2 instead of NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN. Thanks, Bart.
Re: [PATCH 1/2] qedi: Correct the size of target name
On Wed, 2018-06-27 at 05:14 -0700, Nilesh Javali wrote:
> There is potential buffer overflow while getting the target
> name from the NVRAM. Correct the size of the buffer to avoid
> overflow.
>
> Signed-off-by: Nilesh Javali
> ---
> drivers/scsi/qedi/qedi_iscsi.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qedi/qedi_iscsi.h b/drivers/scsi/qedi/qedi_iscsi.h
> index 1126077..d690330 100644
> --- a/drivers/scsi/qedi/qedi_iscsi.h
> +++ b/drivers/scsi/qedi/qedi_iscsi.h
> @@ -225,7 +225,7 @@ struct qedi_work_map {
>
> struct qedi_boot_target {
> char ip_addr[64];
> - char iscsi_name[255];
> + char iscsi_name[256];
> u32 ipv6_en;
> };
Has the number 256 perhaps been derived from the following paragraph
in the iSCSI spec? If so, please mention this in the patch description.
From https://tools.ietf.org/html/rfc3720:
If not otherwise specified, the maximum length of a simple-value (not
its encoded representation) is 255 bytes, not including the delimiter
(comma or zero byte).
Thanks,
Bart.
[PATCH] Avoid that SCSI device removal through sysfs triggers a deadlock
This patch avoids that self-removal triggers the following deadlock:
==
WARNING: possible circular locking dependency detected
4.18.0-rc2-dbg+ #5 Not tainted
--
modprobe/6539 is trying to acquire lock:
8323c4cd (kn->count#202){}, at: kernfs_remove_by_name_ns+0x45/0x90
but task is already holding lock:
a6ec2c69 (>scan_mutex){+.+.}, at: scsi_remove_host+0x21/0x150
[scsi_mod]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (>scan_mutex){+.+.}:
__mutex_lock+0xfe/0xc70
mutex_lock_nested+0x1b/0x20
scsi_remove_device+0x26/0x40 [scsi_mod]
sdev_store_delete+0x27/0x30 [scsi_mod]
dev_attr_store+0x3e/0x50
sysfs_kf_write+0x87/0xa0
kernfs_fop_write+0x190/0x230
__vfs_write+0xd2/0x3b0
vfs_write+0x101/0x270
ksys_write+0xab/0x120
__x64_sys_write+0x43/0x50
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #0 (kn->count#202){}:
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0CPU1
lock(>scan_mutex);
lock(kn->count#202);
lock(>scan_mutex);
lock(kn->count#202);
*** DEADLOCK ***
2 locks held by modprobe/6539:
#0: efaf9298 (>mutex){}, at:
device_release_driver_internal+0x68/0x360
#1: a6ec2c69 (>scan_mutex){+.+.}, at:
scsi_remove_host+0x21/0x150 [scsi_mod]
stack backtrace:
CPU: 10 PID: 6539 Comm: modprobe Not tainted 4.18.0-rc2-dbg+ #5
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.0.0-prebuilt.qemu-project.org 04/01/2014
Call Trace:
dump_stack+0xa4/0xf5
print_circular_bug.isra.34+0x213/0x221
__lock_acquire+0x1a7e/0x1b50
lock_acquire+0xd2/0x260
__kernfs_remove+0x424/0x4a0
kernfs_remove_by_name_ns+0x45/0x90
remove_files.isra.1+0x3a/0x90
sysfs_remove_group+0x5c/0xc0
sysfs_remove_groups+0x39/0x60
device_remove_attrs+0x82/0xb0
device_del+0x251/0x580
__scsi_remove_device+0x19f/0x1d0 [scsi_mod]
scsi_forget_host+0x37/0xb0 [scsi_mod]
scsi_remove_host+0x9b/0x150 [scsi_mod]
sdebug_driver_remove+0x4b/0x150 [scsi_debug]
device_release_driver_internal+0x241/0x360
device_release_driver+0x12/0x20
bus_remove_device+0x1bc/0x290
device_del+0x259/0x580
device_unregister+0x1a/0x70
sdebug_remove_adapter+0x8b/0xf0 [scsi_debug]
scsi_debug_exit+0x76/0xe8 [scsi_debug]
__x64_sys_delete_module+0x1c1/0x280
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe
See also https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg54525.html.
Suggested-by: Eric W. Biederman
Fixes: ac0ece9174ac ("scsi: use device_remove_file_self() instead of
device_schedule_callback()")
Signed-off-by: Bart Van Assche
Cc: Eric W. Biederman
Cc: Tejun Heo
Cc: Hannes Reinecke
Cc: Johannes Thumshirn
Cc: Ingo Molnar
Cc: Oleg Nesterov
Cc:
---
drivers/scsi/scsi_sysfs.c | 48 +++
kernel/task_work.c| 1 +
2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 7943b762c12d..f14e92f1d292 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
@@ -718,14 +719,53 @@ store_rescan_field (struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field);
+struct remove_dev_work {
+ struct callback_headhead;
+ struct scsi_device *sdev;
+};
+
+static void delete_sdev(struct callback_head *head)
+{
+ struct remove_dev_work *work = container_of(head, typeof(*work), head);
+ struct scsi_device *sdev = work->sdev;
+
+ scsi_remove_device(sdev);
+ kfree(work);
+ scsi_device_put(sdev);
+}
+
static ssize_t
sdev_store_d
Re: [PATCH] scsi: ibmvscsi: Improve strings handling
On 06/26/18 12:10, Breno Leitao wrote: if (ppartition_name) strncpy(partition_name, ppartition_name, - sizeof(partition_name)); + sizeof(partition_name) - 1); Please use strlcpy() instead of trying to emulate it. Thanks, Bart.
[PATCH] sd: Remove a superfluous assignment
Since blk_rq_bytes(req) returns req->__data_len, assigning that value
to req->__data_len is superfluous. Hence remove that assignment.
See also commit 5db44863b6eb ("[SCSI] sd: Implement support for WRITE SAME").
Signed-off-by: Bart Van Assche
---
drivers/scsi/sd.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9421d9877730..3463f21f91ee 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2028,7 +2028,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
} else {
sdkp->device->no_write_same = 1;
sd_config_write_same(sdkp);
- req->__data_len = blk_rq_bytes(req);
req->rq_flags |= RQF_QUIET;
}
break;
--
2.17.1
[PATCH] qedi: Fix misleading indentation
This patch avoids that smatch reports the following warnings: drivers/scsi/qedi/qedi_fw_api.c:129: init_sqe() warn: inconsistent indenting drivers/scsi/qedi/qedi_fw_api.c:137: init_sqe() warn: inconsistent indenting Signed-off-by: Bart Van Assche Cc: qlogic-storage-upstr...@cavium.com --- drivers/scsi/qedi/qedi_fw_api.c | 30 -- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/drivers/scsi/qedi/qedi_fw_api.c b/drivers/scsi/qedi/qedi_fw_api.c index a269da1a6c75..387dc87e4d22 100644 --- a/drivers/scsi/qedi/qedi_fw_api.c +++ b/drivers/scsi/qedi/qedi_fw_api.c @@ -126,22 +126,24 @@ static void init_sqe(struct iscsi_task_params *task_params, sgl_task_params, dif_task_params); - if (scsi_is_slow_sgl(sgl_task_params->num_sges, -sgl_task_params->small_mid_sge)) - num_sges = ISCSI_WQE_NUM_SGES_SLOWIO; - else - num_sges = min(sgl_task_params->num_sges, - (u16)SCSI_NUM_SGES_SLOW_SGL_THR); - } + if (scsi_is_slow_sgl(sgl_task_params->num_sges, +sgl_task_params->small_mid_sge)) + num_sges = ISCSI_WQE_NUM_SGES_SLOWIO; + else + num_sges = min(sgl_task_params->num_sges, + (u16)SCSI_NUM_SGES_SLOW_SGL_THR); + } - SET_FIELD(task_params->sqe->flags, ISCSI_WQE_NUM_SGES, num_sges); - SET_FIELD(task_params->sqe->contlen_cdbsize, ISCSI_WQE_CONT_LEN, - buf_size); + SET_FIELD(task_params->sqe->flags, ISCSI_WQE_NUM_SGES, + num_sges); + SET_FIELD(task_params->sqe->contlen_cdbsize, ISCSI_WQE_CONT_LEN, + buf_size); - if (GET_FIELD(pdu_header->hdr_second_dword, - ISCSI_CMD_HDR_TOTAL_AHS_LEN)) - SET_FIELD(task_params->sqe->contlen_cdbsize, ISCSI_WQE_CDB_SIZE, - cmd_params->extended_cdb_sge.sge_len); + if (GET_FIELD(pdu_header->hdr_second_dword, + ISCSI_CMD_HDR_TOTAL_AHS_LEN)) + SET_FIELD(task_params->sqe->contlen_cdbsize, + ISCSI_WQE_CDB_SIZE, + cmd_params->extended_cdb_sge.sge_len); } break; case ISCSI_TASK_TYPE_INITIATOR_READ: -- 2.17.1
Re: [PATCH] qedi: Fix static checker warning
On 06/25/18 05:32, Nilesh Javali wrote: This patch fixes the static checker warning, drivers/scsi/qedi/qedi_main.c:891 qedi_get_boot_tgt_info() error: snprintf() is printing too much 256 vs 255 Which static checker produced this warning? Signed-off-by: Nilesh Javali --- drivers/scsi/qedi/qedi_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c index cf274a7..85491da 100644 --- a/drivers/scsi/qedi/qedi_main.c +++ b/drivers/scsi/qedi/qedi_main.c @@ -888,8 +888,8 @@ static void qedi_get_boot_tgt_info(struct nvm_iscsi_block *block, ipv6_en = !!(block->generic.ctrl_flags & NVM_ISCSI_CFG_GEN_IPV6_ENABLED); - snprintf(tgt->iscsi_name, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n", -block->target[index].target_name.byte); + sprintf(tgt->iscsi_name, "%.*s\n", NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, + block->target[index].target_name.byte); tgt->ipv6_en = ipv6_en; Since sizeof(tgt->iscsi_name) == 255, since NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN == 256 and since sizeof(block->target[index].target_name.byte) == 256, I think you are making a potential buffer overflow worse instead of just suppressing a static checker warning. Bart.
[PATCH] klist: Make it safe to use klists in atomic context
In the scsi_transport_srp implementation it cannot be avoided to
iterate over a klist from atomic context when using the legacy
block layer instead of blk-mq. Hence this patch that makes it safe
to use klists in atomic context. This patch avoids that lockdep
reports the following:
WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
Possible interrupt unsafe locking scenario:
CPU0CPU1
lock(&(>k_lock)->rlock);
local_irq_disable();
lock(&(>__queue_lock)->rlock);
lock(&(>k_lock)->rlock);
lock(&(>__queue_lock)->rlock);
stack backtrace:
Workqueue: kblockd blk_timeout_work
Call Trace:
dump_stack+0xa4/0xf5
check_usage+0x6e6/0x700
__lock_acquire+0x185d/0x1b50
lock_acquire+0xd2/0x260
_raw_spin_lock+0x32/0x50
klist_next+0x47/0x190
device_for_each_child+0x8e/0x100
srp_timed_out+0xaf/0x1d0 [scsi_transport_srp]
scsi_times_out+0xd4/0x410 [scsi_mod]
blk_rq_timed_out+0x36/0x70
blk_timeout_work+0x1b5/0x220
process_one_work+0x4fe/0xad0
worker_thread+0x63/0x5a0
kthread+0x1c1/0x1e0
ret_from_fork+0x24/0x30
See also commit c9ddf73476ff ("scsi: scsi_transport_srp: Fix shost
to rport translation").
Signed-off-by: Bart Van Assche
Cc: Martin K. Petersen
Cc: James Bottomley
---
lib/klist.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/lib/klist.c b/lib/klist.c
index 0507fa5d84c5..f6b547812fe3 100644
--- a/lib/klist.c
+++ b/lib/klist.c
@@ -336,8 +336,9 @@ struct klist_node *klist_prev(struct klist_iter *i)
void (*put)(struct klist_node *) = i->i_klist->put;
struct klist_node *last = i->i_cur;
struct klist_node *prev;
+ unsigned long flags;
- spin_lock(>i_klist->k_lock);
+ spin_lock_irqsave(>i_klist->k_lock, flags);
if (last) {
prev = to_klist_node(last->n_node.prev);
@@ -356,7 +357,7 @@ struct klist_node *klist_prev(struct klist_iter *i)
prev = to_klist_node(prev->n_node.prev);
}
- spin_unlock(>i_klist->k_lock);
+ spin_unlock_irqrestore(>i_klist->k_lock, flags);
if (put && last)
put(last);
@@ -377,8 +378,9 @@ struct klist_node *klist_next(struct klist_iter *i)
void (*put)(struct klist_node *) = i->i_klist->put;
struct klist_node *last = i->i_cur;
struct klist_node *next;
+ unsigned long flags;
- spin_lock(>i_klist->k_lock);
+ spin_lock_irqsave(>i_klist->k_lock, flags);
if (last) {
next = to_klist_node(last->n_node.next);
@@ -397,7 +399,7 @@ struct klist_node *klist_next(struct klist_iter *i)
next = to_klist_node(next->n_node.next);
}
- spin_unlock(>i_klist->k_lock);
+ spin_unlock_irqrestore(>i_klist->k_lock, flags);
if (put && last)
put(last);
--
2.17.1
Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset
On 06/22/18 09:38, Sreekanth Reddy wrote: In driver's .resume() callback function, driver is doing IOC reset operation. And as per your suggestion we tried using scsi_internal_device_block_nowait() to block the all the devices attached to the HBA before going for IOC reset operation. During system resume time as drives will be in quiesce state and calling scsi_internal_device_block_nowait() return with error status -22. So you suggested us to try using lock_system_sleep() API before calling scsi_internal_device_block_nowait(), so that we don't get -22 return status when we call scsi_internal_device_block_nowait() API during resume time. We tried the same and we see system get hang during hibernation. Please correct me if I am wrong or if I have wrongly understood your suggestions. I feel that the fix which have posted is the better fix then using scsi_internal_device_block_nowait()/scsi_internal_device_unblock_nowait() APIs. Hello Sreekanth, It seems like there is a misunderstanding: what I proposed was to use lock_system_sleep() to delay hibernation until unlock_system_sleep() has been called. I had not realized that the mpt3sas driver can call scsi_internal_device_block_nowait() during system resume. There is another issue with the scsi_internal_device_block_nowait() calls by the mpt3sas driver: callers like _scsih_sas_broadcast_primitive_event() seem to assume that all scsih_qcmd() calls have finished as soon as scsi_internal_device_block_nowait() has returned. However, that assumption is not correct, especially when using scsi-mq. If the patch "mpt3sas: Fix calltrace observed while running IO & host reset" would be allowed upstream, will the issues mentioned above ever be addressed? Bart.
Re: qla2xxx and smatch warnings about uninitialized variables
On 06/22/18 14:06, Dan Carpenter wrote: On Fri, Jun 22, 2018 at 08:43:21AM -0700, Bart Van Assche wrote: Hello Himanshu, The latest smatch version reports a large number of warnings about uninitialized variables for the qla2xxx driver. I think these warnings indicate real bugs. Can you have a look at these warnings? Or we could silence a lot of them by adding "qla8044_rd_reg_indirect 2" to the smatch_data/kernel.ignore_uninitialized_param file. Hello Dan, Since the warnings seem to indicate real bugs to me, I'm not sure it's a good idea to suppress these warnings. Thanks, Bart.
qla2xxx and smatch warnings about uninitialized variables
Hello Himanshu, The latest smatch version reports a large number of warnings about uninitialized variables for the qla2xxx driver. I think these warnings indicate real bugs. Can you have a look at these warnings? Thanks, Bart. $ make M=drivers/scsi/qla2xxx C=2 CHECK="smatch -p=kernel" CHECK drivers/scsi/qla2xxx/qla_os.c CC [M] drivers/scsi/qla2xxx/qla_os.o CHECK drivers/scsi/qla2xxx/qla_init.c CC [M] drivers/scsi/qla2xxx/qla_init.o CHECK drivers/scsi/qla2xxx/qla_mbx.c CC [M] drivers/scsi/qla2xxx/qla_mbx.o CHECK drivers/scsi/qla2xxx/qla_iocb.c CC [M] drivers/scsi/qla2xxx/qla_iocb.o CHECK drivers/scsi/qla2xxx/qla_isr.c CC [M] drivers/scsi/qla2xxx/qla_isr.o CHECK drivers/scsi/qla2xxx/qla_gs.c CC [M] drivers/scsi/qla2xxx/qla_gs.o CHECK drivers/scsi/qla2xxx/qla_dbg.c CC [M] drivers/scsi/qla2xxx/qla_dbg.o CHECK drivers/scsi/qla2xxx/qla_sup.c CC [M] drivers/scsi/qla2xxx/qla_sup.o CHECK drivers/scsi/qla2xxx/qla_attr.c CC [M] drivers/scsi/qla2xxx/qla_attr.o CHECK drivers/scsi/qla2xxx/qla_mid.c CC [M] drivers/scsi/qla2xxx/qla_mid.o CHECK drivers/scsi/qla2xxx/qla_dfs.c CC [M] drivers/scsi/qla2xxx/qla_dfs.o CHECK drivers/scsi/qla2xxx/qla_bsg.c CC [M] drivers/scsi/qla2xxx/qla_bsg.o CHECK drivers/scsi/qla2xxx/qla_nx.c drivers/scsi/qla2xxx/qla_nx.c:1016: qla82xx_flash_wait_write_finish() error: uninitialized symbol 'val'. drivers/scsi/qla2xxx/qla_nx.c:1213: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'. CC [M] drivers/scsi/qla2xxx/qla_nx.o CHECK drivers/scsi/qla2xxx/qla_mr.c drivers/scsi/qla2xxx/qla_mr.c:2268: qlafx00_ioctl_iosb_entry() error: uninitialized symbol 'res'. CC [M] drivers/scsi/qla2xxx/qla_mr.o drivers/scsi/qla2xxx/qla_mr.c: In function ‘qlafx00_fx_disc’: drivers/scsi/qla2xxx/qla_mr.c:1882:4: warning: ‘strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation] strncpy(phost_info->nodename, ^ p_sysid->nodename, NODENAME_LENGTH); ~~~ drivers/scsi/qla2xxx/qla_mr.c:1886:4: warning: ‘strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation] strncpy(phost_info->release, ^~~~ p_sysid->release, RELEASE_LENGTH); ~ drivers/scsi/qla2xxx/qla_mr.c:1888:4: warning: ‘strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation] strncpy(phost_info->version, ^~~~ p_sysid->version, VERSION_LENGTH); ~ drivers/scsi/qla2xxx/qla_mr.c:1890:4: warning: ‘strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation] strncpy(phost_info->machine, ^~~~ p_sysid->machine, MACHINE_LENGTH); ~ drivers/scsi/qla2xxx/qla_mr.c:1892:4: warning: ‘strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation] strncpy(phost_info->domainname, ^~~ p_sysid->domainname, DOMNAME_LENGTH); CHECK drivers/scsi/qla2xxx/qla_nx2.c drivers/scsi/qla2xxx/qla_nx2.c:135: qla8044_read_write_crb_reg() error: uninitialized symbol 'value'. drivers/scsi/qla2xxx/qla_nx2.c:149: qla8044_poll_wait_for_ready() error: uninitialized symbol 'temp'. drivers/scsi/qla2xxx/qla_nx2.c:678: qla8044_poll_reg() error: uninitialized symbol 'value'. drivers/scsi/qla2xxx/qla_nx2.c:697: qla8044_poll_reg() error: uninitialized symbol 'value'. drivers/scsi/qla2xxx/qla_nx2.c:921: qla8044_poll_read_list() error: uninitialized symbol 'value'. drivers/scsi/qla2xxx/qla_nx2.c:1193: qla8044_ms_mem_write_128b() error: uninitialized symbol 'agt_ctrl'. drivers/scsi/qla2xxx/qla_nx2.c:2235: qla8044_minidump_process_control() error: uninitialized symbol 'read_value'. drivers/scsi/qla2xxx/qla_nx2.c:2261: qla8044_minidump_process_control() error: uninitialized symbol 'read_value'. drivers/scsi/qla2xxx/qla_nx2.c:2286: qla8044_minidump_process_control() error: uninitialized symbol 'read_value'. drivers/scsi/qla2xxx/qla_nx2.c:2344: qla8044_minidump_process_rdcrb() error: uninitialized symbol 'r_value'. drivers/scsi/qla2xxx/qla_nx2.c:2413: qla8044_minidump_process_rdmem() error: uninitialized symbol 'r_data'. drivers/scsi/qla2xxx/qla_nx2.c:2507: qla8044_minidump_process_l2tag() error: uninitialized symbol 'c_value_r'. drivers/scsi/qla2xxx/qla_nx2.c:2519: qla8044_minidump_process_l2tag() error: uninitialized symbol 'r_value'. drivers/scsi/qla2xxx/qla_nx2.c:2554: qla8044_minidump_process_l1cache() error: uninitialized symbol 'r_value'. drivers/scsi/qla2xxx/qla_nx2.c:2615: qla8044_minidump_process_rdmux() error: uninitialized symbol 'r_value'.
Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset
On 06/21/18 22:35, Sreekanth Reddy wrote: No, lock_system_sleep() is not inserted in the interrupt context. we have inserted it in .resume() call back function just before issuing the IOC reset. That's the wrong place to insert a lock_system_sleep() call. Please have a look at drivers/scsi/scsi_transport_spi.c for an example of how to use that function. Thanks, Bart.
Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset
On Thu, 2018-06-21 at 15:41 +0530, Sreekanth Reddy wrote: > Bart, we tried using lock_system_sleep() before calling IOC reset > operation in .resume() callback function and unlock_system_sleep() > after the IOC reset. With this code change we see system is going to > hang state during hibernation and we just see below messages, > > [ 625.788598] PM: hibernation entry > Jun 21 05:37:33 localhost kernel: PM: hibernation entry > [ 627.428159] PM: Syncing filesystems ... > Jun 21 05:37:34 localhost kernel: PM: Syncing filesystems ... > [ 628.756119] PM: done. > [ 628.758707] Freezing user space processes ... (elapsed 0.001 seconds) done. > [ 628.768340] OOM killer disabled. > [ 628.772010] PM: Preallocating image memory... done (allocated 197704 pages) > [ 632.554470] PM: Allocated 790816 kbytes in 3.77 seconds (209.76 MB/s) > [ 632.561664] Freezing remaining freezable tasks ... (elapsed 0.002 > seconds) done. > [ 632.572269] Suspending console(s) (use no_console_suspend to debug) > > > The fix which we have posted looks simple and we don't see any side > effects of it. > We have done complete regression testing on our fix and we don't see > any issue with it. So please consider our fix which have posted. scsi_internal_device_block_nowait() can be called by the mpt3sas driver from interrupt context. lock_system_sleep() locks a mutex and hence must not be called from interrupt context. Does the above mean that (a) a call to lock_system_sleep() was inserted in an interrupt handler and (b) that the above test was performed with a kernel in which lockdep was disabled? Bart.
Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset
On Wed, 2018-06-20 at 09:18 +0530, Chaitra Basappa wrote: > We have tried with calling scsi_internal_device_block_nowait() API before > doing IOC reset (i.e. host reset) and called > scsi_internal_device_unblock_nowait() after performing IOC reset. > We have tested this code change with various test cases such as > adding/removing target drives or expanders during diag reset with and > without IOs and at high level we see all are working but we observe below > error messages while performing hibernation operation, > > sd 1:0:0:0: device_block, handle(0x0028) > BRCM Debug: sdev->sdev_state: 5 before device_block_nowait > BRCM Debug: sdev->sdev_state: 5 after_device_block_nowait > sd 1:0:0:0: device_block failed with return(-22) for handle(0x0028) > . > . > sd 0:0:0:0: device_unblock and setting to running, handle(0x0028) > sd 0:0:0:0: device_unblock failed with return(-22) for handle(0x0028) > performing a block followed by an unblock > sd 0:0:0:0: retried device_block failed with return(-22) for handle(0x0028) > sd 0:0:0:0: retried device_unblock failed with return(-22) for > handle(0x0028) > > We are observing these messages during of system resume time, during which > driver issues IOC reset operation in the .resume() callback function. > In the above error messages we see that drives are in SDEV_QUIESCE state. > When drives are SDEV_QUIESCE state then moving these drives to > SDEV_BLOCK state is not allowed and hence we observe above error messages. > > SDEV_QUIESCE state means that Device quiescent. No block commands will be > accepted, only specials (which originate in the midlayer). Neither scsi_internal_device_block_nowait() nor scsi_internal_device_unblock_nowait() should ever have been changed from static into exported functions. But that's another discussion. Regarding the adverse interaction of scsi_internal_device_block_nowait() and scsi_internal_device_unblock_nowait() with the power management code, have you considered to surround code that blocks and unblocks SCSI devices with lock_system_sleep() / unlock_system_sleep() to avoid that these functions fail with error code -22? Thanks, Bart.
Re: [PATCH] scsi: let __scsi_remove_device do the blk_put_queue in one place
On Wed, 2018-06-20 at 18:58 +0200, Anthoine Bourgeois wrote: > On Wed, Jun 20, 2018 at 01:50:38PM +0000, Bart Van Assche wrote: > > On Wed, 2018-06-20 at 11:57 +0200, anthoine.bourge...@gmail.com wrote: > > > The function scsi_device_dev_release_usercontext calls blk_put_queue > > > with request_queue then set request_queue to NULL. If the function > > > scsi_device_dev_release_usercontext is racy then the next call to > > > blk_put_queue will trigger the NULL pointer dereference below. > > > > How did you trigger this bug? Which SCSI LLD drivers were involved, and > > which scenario or workload triggered this kernel oops? > > > > I think iscsi_tcp is my LLD driver. Here a list of my modules with > 'scsi' name: > # lsmod|grep scsi > iscsi_tcp 20480 4 > libiscsi_tcp 24576 1 iscsi_tcp > libiscsi 57344 3 ib_iser,libiscsi_tcp,iscsi_tcp > scsi_transport_iscsi 106496 4 ib_iser,libiscsi,iscsi_tcp > > The bug is trigger by a 'iscsiadm -m node -T targetname --logout' but it > occurs maybe 1-2% of the times. Hello Anthoine, As far as I know the same scsi_device_dev_release_usercontext() function works reliably for other SCSI LLDs. So you may want to report this to the iSCSI initiator driver maintainers. Thanks, Bart.
Re: [PATCH] scsi: let __scsi_remove_device do the blk_put_queue in one place
On Wed, 2018-06-20 at 11:57 +0200, anthoine.bourge...@gmail.com wrote: > The function scsi_device_dev_release_usercontext calls blk_put_queue > with request_queue then set request_queue to NULL. If the function > scsi_device_dev_release_usercontext is racy then the next call to > blk_put_queue will trigger the NULL pointer dereference below. How did you trigger this bug? Which SCSI LLD drivers were involved, and which scenario or workload triggered this kernel oops? Thanks, Bart.
[PATCH v2 0/8] mpt3sas: Fix static source code checker complaints
Hello Martin, This patch series addresses the complaints reported by multiple static source code analysis tools about the mpt3sas source code. Please consider these patches for kernel v4.19. Thanks, Bart. Changes compared to v1: - Addressed Randy Dunlap's feedback about kernel-doc headers. Bart Van Assche (8): mpt3sas: Fix indentation mpt3sas: Remove set-but-not-used variables mpt3sas: Annotate switch/case fall-through mpt3sas: Introduce struct mpt3sas_nvme_cmd mpt3sas: Fix _transport_smp_handler() error path mpt3sas: Fix a race condition in mpt3sas_base_hard_reset_handler() mpt3sas: Split _base_reset_handler(), mpt3sas_scsih_reset_handler() and mpt3sas_ctl_reset_handler() mpt3sas: Improve kernel-doc headers drivers/scsi/mpt3sas/mpt3sas_base.c | 366 +++-- drivers/scsi/mpt3sas/mpt3sas_base.h | 23 +- drivers/scsi/mpt3sas/mpt3sas_config.c | 74 ++-- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 357 +--- drivers/scsi/mpt3sas/mpt3sas_scsih.c| 428 +++- drivers/scsi/mpt3sas/mpt3sas_transport.c| 62 ++- drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c | 18 +- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c| 3 - 8 files changed, 568 insertions(+), 763 deletions(-) -- 2.17.0
[PATCH v2 5/8] mpt3sas: Fix _transport_smp_handler() error path
This patch avoids that smatch complains about a double unlock on
ioc->transport_cmds.mutex.
Fixes: 651a01364994 ("scsi: scsi_transport_sas: switch to bsg-lib for SMP
passthrough")
Signed-off-by: Bart Van Assche
Cc: Christoph Hellwig
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
Cc: sta...@vger.kernel.org
---
drivers/scsi/mpt3sas/mpt3sas_transport.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 05d506d78c66..f4b02dd7f6cf 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -1936,12 +1936,12 @@ _transport_smp_handler(struct bsg_job *job, struct
Scsi_Host *shost,
pr_info(MPT3SAS_FMT "%s: host reset in progress!\n",
__func__, ioc->name);
rc = -EFAULT;
- goto out;
+ goto job_done;
}
rc = mutex_lock_interruptible(>transport_cmds.mutex);
if (rc)
- goto out;
+ goto job_done;
if (ioc->transport_cmds.status != MPT3_CMD_NOT_USED) {
pr_err(MPT3SAS_FMT "%s: transport_cmds in use\n", ioc->name,
@@ -2066,6 +2066,7 @@ _transport_smp_handler(struct bsg_job *job, struct
Scsi_Host *shost,
out:
ioc->transport_cmds.status = MPT3_CMD_NOT_USED;
mutex_unlock(>transport_cmds.mutex);
+job_done:
bsg_job_done(job, rc, reslen);
}
--
2.17.0
[PATCH v2 8/8] mpt3sas: Improve kernel-doc headers
Avoids that warnings about the kernel headers appear when building
with W=1. Remove useless "@Returns - Nothing" clauses. Change
"@Return - " into "Return: ".
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
Cc: Randy Dunlap
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 213 +--
drivers/scsi/mpt3sas/mpt3sas_config.c | 74 ++---
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 267 ++
drivers/scsi/mpt3sas/mpt3sas_scsih.c| 288 +---
drivers/scsi/mpt3sas/mpt3sas_transport.c| 55 ++--
drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c | 18 +-
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c| 3 -
7 files changed, 363 insertions(+), 555 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index c2c523269414..1b63a7e218aa 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -103,7 +103,10 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc);
/**
* _scsih_set_fwfault_debug - global setting of ioc->fwfault_debug.
+ * @val: ?
+ * @kp: ?
*
+ * Return: ?
*/
static int
_scsih_set_fwfault_debug(const char *val, const struct kernel_param *kp)
@@ -132,8 +135,6 @@ module_param_call(mpt3sas_fwfault_debug,
_scsih_set_fwfault_debug,
* @ioc: per adapter object
* @reply: reply message frame(lower 32bit addr)
* @index: System request message index.
- *
- * @Returns - Nothing
*/
static void
_base_clone_reply_to_sys_mem(struct MPT3SAS_ADAPTER *ioc, u32 reply,
@@ -156,7 +157,7 @@ _base_clone_reply_to_sys_mem(struct MPT3SAS_ADAPTER *ioc,
u32 reply,
* _base_clone_mpi_to_sys_mem - Writes/copies MPI frames
* to system/BAR0 region.
*
- * @dst_iomem: Pointer to the destinaltion location in BAR0 space.
+ * @dst_iomem: Pointer to the destination location in BAR0 space.
* @src: Pointer to the Source data.
* @size: Size of data to be copied.
*/
@@ -197,7 +198,7 @@ _base_clone_to_sys_mem(void __iomem *dst_iomem, void *src,
u32 size)
* @smid: system request message index
* @sge_chain_count: Scatter gather chain count.
*
- * @Return: chain address.
+ * Return: the chain address.
*/
static inline void __iomem*
_base_get_chain(struct MPT3SAS_ADAPTER *ioc, u16 smid,
@@ -223,7 +224,7 @@ _base_get_chain(struct MPT3SAS_ADAPTER *ioc, u16 smid,
* @smid: system request message index
* @sge_chain_count: Scatter gather chain count.
*
- * @Return - Physical chain address.
+ * Return: Physical chain address.
*/
static inline phys_addr_t
_base_get_chain_phys(struct MPT3SAS_ADAPTER *ioc, u16 smid,
@@ -248,7 +249,7 @@ _base_get_chain_phys(struct MPT3SAS_ADAPTER *ioc, u16 smid,
* @ioc: per adapter object
* @smid: system request message index
*
- * @Returns - Pointer to buffer location in BAR0.
+ * Return: Pointer to buffer location in BAR0.
*/
static void __iomem *
@@ -270,7 +271,7 @@ _base_get_buffer_bar0(struct MPT3SAS_ADAPTER *ioc, u16 smid)
* @ioc: per adapter object
* @smid: system request message index
*
- * @Returns - Pointer to buffer location in BAR0.
+ * Return: Pointer to buffer location in BAR0.
*/
static phys_addr_t
_base_get_buffer_phys_bar0(struct MPT3SAS_ADAPTER *ioc, u16 smid)
@@ -291,7 +292,7 @@ _base_get_buffer_phys_bar0(struct MPT3SAS_ADAPTER *ioc, u16
smid)
* @ioc: per adapter object
* @chain_buffer_dma: Chain buffer dma address.
*
- * @Returns - Pointer to chain buffer. Or Null on Failure.
+ * Return: Pointer to chain buffer. Or Null on Failure.
*/
static void *
_base_get_chain_buffer_dma_to_chain_buffer(struct MPT3SAS_ADAPTER *ioc,
@@ -322,8 +323,6 @@ _base_get_chain_buffer_dma_to_chain_buffer(struct
MPT3SAS_ADAPTER *ioc,
* @ioc: per adapter object.
* @mpi_request: mf request pointer.
* @smid: system request message index.
- *
- * @Returns: Nothing.
*/
static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
void *mpi_request, u16 smid)
@@ -496,8 +495,9 @@ static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
* mpt3sas_remove_dead_ioc_func - kthread context to remove dead ioc
* @arg: input argument, used to derive ioc
*
- * Return 0 if controller is removed from pci subsystem.
- * Return -1 for other case.
+ * Return:
+ * 0 if controller is removed from pci subsystem.
+ * -1 for other case.
*/
static int mpt3sas_remove_dead_ioc_func(void *arg)
{
@@ -517,9 +517,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
/**
* _base_fault_reset_work - workq handling ioc fault conditions
* @work: input argument, used to derive ioc
- * Context: sleep.
*
- * Return nothing.
+ * Context: sleep.
*/
static void
_base_fault_reset_work(struct work_struct *work)
@@ -610,9 +609,8 @@ _base_fault_reset_work(struct work_struct *work)
/**
* mpt3sas_base_start_watchdog - start the fault_reset_work_q
* @ioc: per adapter object
- * Context: sleep.
*
- * R
[PATCH v2 3/8] mpt3sas: Annotate switch/case fall-through
This patch avoids that gcc complains about switch/case fall-through when building with W=1. Signed-off-by: Bart Van Assche Cc: Sathya Prakash Cc: Chaitra P B Cc: Suganath Prabu Subramani --- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 1 + drivers/scsi/mpt3sas/mpt3sas_scsih.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index 3269ef43f07e..07d28b7d5f1c 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -970,6 +970,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, } /* drop to default case for posting the request */ } + /* fall through */ default: ioc->build_sg_mpi(ioc, psge, data_out_dma, data_out_sz, data_in_dma, data_in_sz); diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index cac9a264e152..f1a748081fe4 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -5414,6 +5414,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply) case MPI2_IOCSTATUS_SCSI_DATA_OVERRUN: scsi_set_resid(scmd, 0); + /* fall through */ case MPI2_IOCSTATUS_SCSI_RECOVERED_ERROR: case MPI2_IOCSTATUS_SUCCESS: scmd->result = (DID_OK << 16) | scsi_status; @@ -6444,6 +6445,7 @@ _scsih_sas_topology_change_event(struct MPT3SAS_ADAPTER *ioc, if (!test_bit(handle, ioc->pend_os_device_add)) break; + /* fall through */ case MPI2_EVENT_SAS_TOPO_RC_TARG_ADDED: @@ -7160,6 +7162,7 @@ _scsih_pcie_topology_change_event(struct MPT3SAS_ADAPTER *ioc, event_data->PortEntry[i].PortStatus &= 0xF0; event_data->PortEntry[i].PortStatus |= MPI26_EVENT_PCIE_TOPO_PS_DEV_ADDED; + /* fall through */ case MPI26_EVENT_PCIE_TOPO_PS_DEV_ADDED: if (ioc->shost_recovery) break; -- 2.17.0
[PATCH v2 1/8] mpt3sas: Fix indentation
Modify the indentation such that smatch no longer complains about
inconsistent indenting.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 40
drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 +-
3 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 569392d0d4c9..798ee62c97f1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -633,7 +633,7 @@ mpt3sas_base_start_watchdog(struct MPT3SAS_ADAPTER *ioc)
if (!ioc->fault_reset_work_q) {
pr_err(MPT3SAS_FMT "%s: failed (line=%d)\n",
ioc->name, __func__, __LINE__);
- return;
+ return;
}
spin_lock_irqsave(>ioc_reset_in_progress_lock, flags);
if (ioc->fault_reset_work_q)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b8d131a455d0..42a8d79be7ec 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2256,7 +2256,7 @@ scsih_slave_configure(struct scsi_device *sdev)
ds = "SSP";
} else {
qdepth = MPT3SAS_SATA_QUEUE_DEPTH;
-if (raid_device->device_info &
+ if (raid_device->device_info &
MPI2_SAS_DEVICE_INFO_SATA_DEVICE)
ds = "SATA";
else
@@ -4004,19 +4004,19 @@ _scsih_issue_delayed_event_ack(struct MPT3SAS_ADAPTER
*ioc, u16 smid, U16 event,
static void
_scsih_issue_delayed_sas_io_unit_ctrl(struct MPT3SAS_ADAPTER *ioc,
u16 smid, u16 handle)
- {
- Mpi2SasIoUnitControlRequest_t *mpi_request;
- u32 ioc_state;
- int i = smid - ioc->internal_smid;
- unsigned long flags;
+{
+ Mpi2SasIoUnitControlRequest_t *mpi_request;
+ u32 ioc_state;
+ int i = smid - ioc->internal_smid;
+ unsigned long flags;
- if (ioc->remove_host) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
+ if (ioc->remove_host) {
+ dewtprintk(ioc, pr_info(MPT3SAS_FMT
"%s: host has been removed\n",
__func__, ioc->name));
- return;
- } else if (ioc->pci_error_recovery) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
+ return;
+ } else if (ioc->pci_error_recovery) {
+ dewtprintk(ioc, pr_info(MPT3SAS_FMT
"%s: host in pci error recovery\n",
__func__, ioc->name));
return;
@@ -4674,19 +4674,19 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd
*scmd)
}
- /* host recovery or link resets sent via IOCTLs */
- if (ioc->shost_recovery || ioc->ioc_link_reset_in_progress)
+ if (ioc->shost_recovery || ioc->ioc_link_reset_in_progress) {
+ /* host recovery or link resets sent via IOCTLs */
return SCSI_MLQUEUE_HOST_BUSY;
-
- /* device has been deleted */
- else if (sas_target_priv_data->deleted) {
+ } else if (sas_target_priv_data->deleted) {
+ /* device has been deleted */
scmd->result = DID_NO_CONNECT << 16;
scmd->scsi_done(scmd);
return 0;
- /* device busy with task management */
} else if (sas_target_priv_data->tm_busy ||
- sas_device_priv_data->block)
+ sas_device_priv_data->block) {
+ /* device busy with task management */
return SCSI_MLQUEUE_DEVICE_BUSY;
+ }
/*
* Bug work around for firmware SATL handling. The loop
@@ -5812,7 +5812,7 @@ _scsih_expander_add(struct MPT3SAS_ADAPTER *ioc, u16
handle)
}
_scsih_expander_node_add(ioc, sas_expander);
-return 0;
+ return 0;
out_fail:
@@ -9519,7 +9519,7 @@ _mpt3sas_fw_work(struct MPT3SAS_ADAPTER *ioc, struct
fw_event_work *fw_event)
break;
case MPT3SAS_PORT_ENABLE_COMPLETE:
ioc->start_scan = 0;
- if (missing_delay[0] != -1 && missing_delay[1] != -1)
+ if (missing_delay[0] != -1 && missing_delay[1] != -1)
mpt3sas_base_update_missing_delay(ioc, missing_delay[0],
missing_delay[1]);
dewtprintk(ioc, pr_info(MPT3SAS_FMT
diff --git a/drivers/scsi/mpt3sas/mpt3sas_tra
[PATCH v2 2/8] mpt3sas: Remove set-but-not-used variables
This patch does not change any functionality.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 10 --
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 -
2 files changed, 15 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 798ee62c97f1..9c233ddc5b2b 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2127,11 +2127,9 @@ base_is_prp_possible(struct MPT3SAS_ADAPTER *ioc,
struct _pcie_device *pcie_device, struct scsi_cmnd *scmd, int sge_count)
{
u32 data_length = 0;
- struct scatterlist *sg_scmd;
bool build_prp = true;
data_length = scsi_bufflen(scmd);
- sg_scmd = scsi_sglist(scmd);
/* If Datalenth is <= 16K and number of SGE’s entries are <= 2
* we built IEEE SGL
@@ -2162,11 +2160,9 @@ _base_check_pcie_native_sgl(struct MPT3SAS_ADAPTER *ioc,
Mpi25SCSIIORequest_t *mpi_request, u16 smid, struct scsi_cmnd *scmd,
struct _pcie_device *pcie_device)
{
- struct scatterlist *sg_scmd;
int sges_left;
/* Get the SG list pointer and info. */
- sg_scmd = scsi_sglist(scmd);
sges_left = scsi_dma_map(scmd);
if (sges_left < 0) {
sdev_printk(KERN_ERR, scmd->device,
@@ -3472,11 +3468,8 @@ mpt3sas_base_put_smid_hi_priority(struct MPT3SAS_ADAPTER
*ioc, u16 smid,
u64 *request;
if (ioc->is_mcpu_endpoint) {
- MPI2RequestHeader_t *request_hdr;
-
__le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid);
- request_hdr = (MPI2RequestHeader_t *)mfp;
/* TBD 256 is offset within sys register. */
mpi_req_iomem = (void __force *)ioc->chip
+ MPI_FRAME_START_OFFSET
@@ -3539,13 +3532,10 @@ mpt3sas_base_put_smid_default(struct MPT3SAS_ADAPTER
*ioc, u16 smid)
Mpi2RequestDescriptorUnion_t descriptor;
void *mpi_req_iomem;
u64 *request;
- MPI2RequestHeader_t *request_hdr;
if (ioc->is_mcpu_endpoint) {
__le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid);
- request_hdr = (MPI2RequestHeader_t *)mfp;
-
_clone_sg_entries(ioc, (void *) mfp, smid);
/* TBD 256 is offset within sys register */
mpi_req_iomem = (void __force *)ioc->chip +
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 42a8d79be7ec..cac9a264e152 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -1653,7 +1653,6 @@ scsih_target_destroy(struct scsi_target *starget)
struct _raid_device *raid_device;
struct _pcie_device *pcie_device;
unsigned long flags;
- struct sas_rphy *rphy;
sas_target_priv_data = starget->hostdata;
if (!sas_target_priv_data)
@@ -1693,7 +1692,6 @@ scsih_target_destroy(struct scsi_target *starget)
}
spin_lock_irqsave(>sas_device_lock, flags);
- rphy = dev_to_rphy(starget->dev.parent);
sas_device = __mpt3sas_get_sdev_from_target(ioc, sas_target_priv_data);
if (sas_device && (sas_device->starget == starget) &&
(sas_device->id == starget->id) &&
@@ -6873,7 +6871,6 @@ _scsih_pcie_add_device(struct MPT3SAS_ADAPTER *ioc, u16
handle)
Mpi2ConfigReply_t mpi_reply;
struct _pcie_device *pcie_device;
struct _enclosure_node *enclosure_dev;
- u32 pcie_device_type;
u32 ioc_status;
u64 wwid;
@@ -6935,8 +6932,6 @@ _scsih_pcie_add_device(struct MPT3SAS_ADAPTER *ioc, u16
handle)
pcie_device->port_num = pcie_device_pg0.PortNum;
pcie_device->fast_path = (le32_to_cpu(pcie_device_pg0.Flags) &
MPI26_PCIEDEV0_FLAGS_FAST_PATH_CAPABLE) ? 1 : 0;
- pcie_device_type = pcie_device->device_info &
- MPI26_PCIE_DEVINFO_MASK_DEVICE_TYPE;
pcie_device->enclosure_handle =
le16_to_cpu(pcie_device_pg0.EnclosureHandle);
--
2.17.0
[PATCH v2 6/8] mpt3sas: Fix a race condition in mpt3sas_base_hard_reset_handler()
Since ioc->shost_recovery is set after ioc->reset_in_progress_mutex
is obtained, if concurrent resets are issued there is a short time
during which ioc->reset_in_progress_mutex is locked and
ioc->shost_recovery == 0. Avoid that this can cause trouble by
unconditionally locking ioc->shost_recovery.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 10 +-
drivers/scsi/mpt3sas/mpt3sas_base.h | 1 -
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 22e9aa0c5156..e51b42ad7a4a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6934,14 +6934,7 @@ mpt3sas_base_hard_reset_handler(struct MPT3SAS_ADAPTER
*ioc,
mpt3sas_halt_firmware(ioc);
/* wait for an active reset in progress to complete */
- if (!mutex_trylock(>reset_in_progress_mutex)) {
- do {
- ssleep(1);
- } while (ioc->shost_recovery == 1);
- dtmprintk(ioc, pr_info(MPT3SAS_FMT "%s: exit\n", ioc->name,
- __func__));
- return ioc->ioc_reset_in_progress_status;
- }
+ mutex_lock(>reset_in_progress_mutex);
spin_lock_irqsave(>ioc_reset_in_progress_lock, flags);
ioc->shost_recovery = 1;
@@ -6990,7 +6983,6 @@ mpt3sas_base_hard_reset_handler(struct MPT3SAS_ADAPTER
*ioc,
ioc->name, __func__, ((r == 0) ? "SUCCESS" : "FAILED")));
spin_lock_irqsave(>ioc_reset_in_progress_lock, flags);
- ioc->ioc_reset_in_progress_status = r;
ioc->shost_recovery = 0;
spin_unlock_irqrestore(>ioc_reset_in_progress_lock, flags);
ioc->ioc_reset_count++;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index c6c36750deaa..a23ae2eb9c09 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1165,7 +1165,6 @@ struct MPT3SAS_ADAPTER {
struct mutexreset_in_progress_mutex;
spinlock_t ioc_reset_in_progress_lock;
u8 ioc_link_reset_in_progress;
- u8 ioc_reset_in_progress_status;
u8 ignore_loginfos;
u8 remove_host;
--
2.17.0
[PATCH v2 7/8] mpt3sas: Split _base_reset_handler(), mpt3sas_scsih_reset_handler() and mpt3sas_ctl_reset_handler()
Split each of these functions in three functions - one function
per reset phase. This patch does not change any functionality but
makes the code easier to read.
Note: it is much easier to review the git diff -w output after
having applied this patch than by reviewing the patch itself.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 118 ++-
drivers/scsi/mpt3sas/mpt3sas_base.h | 15 ++--
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 89 +++-
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 92 +++--
4 files changed, 166 insertions(+), 148 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index e51b42ad7a4a..c2c523269414 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6815,65 +6815,69 @@ mpt3sas_base_detach(struct MPT3SAS_ADAPTER *ioc)
}
/**
- * _base_reset_handler - reset callback handler (for base)
+ * _base_pre_reset_handler - pre reset handler
* @ioc: per adapter object
- * @reset_phase: phase
- *
- * The handler for doing any required cleanup or initialization.
- *
- * The reset phase can be MPT3_IOC_PRE_RESET, MPT3_IOC_AFTER_RESET,
- * MPT3_IOC_DONE_RESET
- *
- * Return nothing.
*/
-static void
-_base_reset_handler(struct MPT3SAS_ADAPTER *ioc, int reset_phase)
+static void _base_pre_reset_handler(struct MPT3SAS_ADAPTER *ioc)
{
- mpt3sas_scsih_reset_handler(ioc, reset_phase);
- mpt3sas_ctl_reset_handler(ioc, reset_phase);
- switch (reset_phase) {
- case MPT3_IOC_PRE_RESET:
- dtmprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: MPT3_IOC_PRE_RESET\n", ioc->name, __func__));
- break;
- case MPT3_IOC_AFTER_RESET:
- dtmprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: MPT3_IOC_AFTER_RESET\n", ioc->name, __func__));
- if (ioc->transport_cmds.status & MPT3_CMD_PENDING) {
- ioc->transport_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->transport_cmds.smid);
- complete(>transport_cmds.done);
- }
- if (ioc->base_cmds.status & MPT3_CMD_PENDING) {
- ioc->base_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->base_cmds.smid);
- complete(>base_cmds.done);
- }
- if (ioc->port_enable_cmds.status & MPT3_CMD_PENDING) {
- ioc->port_enable_failed = 1;
- ioc->port_enable_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->port_enable_cmds.smid);
- if (ioc->is_driver_loading) {
- ioc->start_scan_failed =
- MPI2_IOCSTATUS_INTERNAL_ERROR;
- ioc->start_scan = 0;
- ioc->port_enable_cmds.status =
- MPT3_CMD_NOT_USED;
- } else
- complete(>port_enable_cmds.done);
- }
- if (ioc->config_cmds.status & MPT3_CMD_PENDING) {
- ioc->config_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->config_cmds.smid);
- ioc->config_cmds.smid = USHRT_MAX;
- complete(>config_cmds.done);
+ mpt3sas_scsih_pre_reset_handler(ioc);
+ mpt3sas_ctl_pre_reset_handler(ioc);
+ dtmprintk(ioc, pr_info(MPT3SAS_FMT
+ "%s: MPT3_IOC_PRE_RESET\n", ioc->name, __func__));
+}
+
+/**
+ * _base_after_reset_handler - after reset handler
+ * @ioc: per adapter object
+ */
+static void _base_after_reset_handler(struct MPT3SAS_ADAPTER *ioc)
+{
+ mpt3sas_scsih_after_reset_handler(ioc);
+ mpt3sas_ctl_after_reset_handler(ioc);
+ dtmprintk(ioc, pr_info(MPT3SAS_FMT
+ "%s: MPT3_IOC_AFTER_RESET\n", ioc->name, __func__));
+ if (ioc->transport_cmds.status & MPT3_CMD_PENDING) {
+ ioc->transport_cmds.status |= MPT3_CMD_RESET;
+ mpt3sas_base_free_smid(ioc, ioc->transport_cmds.smid);
+ complete(>transport_cmds.done);
+ }
+ if (ioc->base_cmds.status & MPT3_CMD_PENDING) {
+ ioc->base_cmds.status |= MPT3_CMD_RESET;
+ mpt3sas_base_free_smid(ioc, ioc->base_cmds.smid);
+ complete(>base_cmds.done);
+ }
+ if (ioc->port_enable_cmds.status & MPT3_CMD_PENDING) {
+ ioc->port_enable_failed = 1;
+ ioc->port_enabl
[PATCH v2 4/8] mpt3sas: Introduce struct mpt3sas_nvme_cmd
Make _base_build_nvme_prp() easier to read by introducing a structure
to access NVMe command fields.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 13 -
drivers/scsi/mpt3sas/mpt3sas_base.h | 7 +--
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 9c233ddc5b2b..22e9aa0c5156 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1836,6 +1836,8 @@ _base_build_nvme_prp(struct MPT3SAS_ADAPTER *ioc, u16
smid,
u32 offset, entry_len;
u32 page_mask_result, page_mask;
size_t length;
+ struct mpt3sas_nvme_cmd *nvme_cmd =
+ (void *)nvme_encap_request->NVMe_Command;
/*
* Not all commands require a data transfer. If no data, just return
@@ -1843,15 +1845,8 @@ _base_build_nvme_prp(struct MPT3SAS_ADAPTER *ioc, u16
smid,
*/
if (!data_in_sz && !data_out_sz)
return;
- /*
-* Set pointers to PRP1 and PRP2, which are in the NVMe command.
-* PRP1 is located at a 24 byte offset from the start of the NVMe
-* command. Then set the current PRP entry pointer to PRP1.
-*/
- prp1_entry = (__le64 *)(nvme_encap_request->NVMe_Command +
- NVME_CMD_PRP1_OFFSET);
- prp2_entry = (__le64 *)(nvme_encap_request->NVMe_Command +
- NVME_CMD_PRP2_OFFSET);
+ prp1_entry = _cmd->prp1;
+ prp2_entry = _cmd->prp2;
prp_entry = prp1_entry;
/*
* For the PRP entries, use the specially allocated buffer of
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index f02974c0be4a..c6c36750deaa 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -143,14 +143,17 @@
* NVMe defines
*/
#defineNVME_PRP_SIZE 8 /* PRP size */
-#defineNVME_CMD_PRP1_OFFSET24 /* PRP1 offset in NVMe
cmd */
-#defineNVME_CMD_PRP2_OFFSET32 /* PRP2 offset in NVMe
cmd */
#defineNVME_ERROR_RESPONSE_SIZE16 /* Max NVME Error
Response */
#define NVME_TASK_ABORT_MIN_TIMEOUT6
#define NVME_TASK_ABORT_MAX_TIMEOUT60
#define NVME_TASK_MNGT_CUSTOM_MASK (0x0010)
#defineNVME_PRP_PAGE_SIZE 4096/* Page size */
+struct mpt3sas_nvme_cmd {
+ u8 rsvd[24];
+ __le64 prp1;
+ __le64 prp2;
+};
/*
* reset phases
--
2.17.0
[PATCH 2/8] mpt3sas: Remove set-but-not-used variables
This patch does not change any functionality.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 10 --
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 -
2 files changed, 15 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 798ee62c97f1..9c233ddc5b2b 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2127,11 +2127,9 @@ base_is_prp_possible(struct MPT3SAS_ADAPTER *ioc,
struct _pcie_device *pcie_device, struct scsi_cmnd *scmd, int sge_count)
{
u32 data_length = 0;
- struct scatterlist *sg_scmd;
bool build_prp = true;
data_length = scsi_bufflen(scmd);
- sg_scmd = scsi_sglist(scmd);
/* If Datalenth is <= 16K and number of SGE’s entries are <= 2
* we built IEEE SGL
@@ -2162,11 +2160,9 @@ _base_check_pcie_native_sgl(struct MPT3SAS_ADAPTER *ioc,
Mpi25SCSIIORequest_t *mpi_request, u16 smid, struct scsi_cmnd *scmd,
struct _pcie_device *pcie_device)
{
- struct scatterlist *sg_scmd;
int sges_left;
/* Get the SG list pointer and info. */
- sg_scmd = scsi_sglist(scmd);
sges_left = scsi_dma_map(scmd);
if (sges_left < 0) {
sdev_printk(KERN_ERR, scmd->device,
@@ -3472,11 +3468,8 @@ mpt3sas_base_put_smid_hi_priority(struct MPT3SAS_ADAPTER
*ioc, u16 smid,
u64 *request;
if (ioc->is_mcpu_endpoint) {
- MPI2RequestHeader_t *request_hdr;
-
__le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid);
- request_hdr = (MPI2RequestHeader_t *)mfp;
/* TBD 256 is offset within sys register. */
mpi_req_iomem = (void __force *)ioc->chip
+ MPI_FRAME_START_OFFSET
@@ -3539,13 +3532,10 @@ mpt3sas_base_put_smid_default(struct MPT3SAS_ADAPTER
*ioc, u16 smid)
Mpi2RequestDescriptorUnion_t descriptor;
void *mpi_req_iomem;
u64 *request;
- MPI2RequestHeader_t *request_hdr;
if (ioc->is_mcpu_endpoint) {
__le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid);
- request_hdr = (MPI2RequestHeader_t *)mfp;
-
_clone_sg_entries(ioc, (void *) mfp, smid);
/* TBD 256 is offset within sys register */
mpi_req_iomem = (void __force *)ioc->chip +
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 42a8d79be7ec..cac9a264e152 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -1653,7 +1653,6 @@ scsih_target_destroy(struct scsi_target *starget)
struct _raid_device *raid_device;
struct _pcie_device *pcie_device;
unsigned long flags;
- struct sas_rphy *rphy;
sas_target_priv_data = starget->hostdata;
if (!sas_target_priv_data)
@@ -1693,7 +1692,6 @@ scsih_target_destroy(struct scsi_target *starget)
}
spin_lock_irqsave(>sas_device_lock, flags);
- rphy = dev_to_rphy(starget->dev.parent);
sas_device = __mpt3sas_get_sdev_from_target(ioc, sas_target_priv_data);
if (sas_device && (sas_device->starget == starget) &&
(sas_device->id == starget->id) &&
@@ -6873,7 +6871,6 @@ _scsih_pcie_add_device(struct MPT3SAS_ADAPTER *ioc, u16
handle)
Mpi2ConfigReply_t mpi_reply;
struct _pcie_device *pcie_device;
struct _enclosure_node *enclosure_dev;
- u32 pcie_device_type;
u32 ioc_status;
u64 wwid;
@@ -6935,8 +6932,6 @@ _scsih_pcie_add_device(struct MPT3SAS_ADAPTER *ioc, u16
handle)
pcie_device->port_num = pcie_device_pg0.PortNum;
pcie_device->fast_path = (le32_to_cpu(pcie_device_pg0.Flags) &
MPI26_PCIEDEV0_FLAGS_FAST_PATH_CAPABLE) ? 1 : 0;
- pcie_device_type = pcie_device->device_info &
- MPI26_PCIE_DEVINFO_MASK_DEVICE_TYPE;
pcie_device->enclosure_handle =
le16_to_cpu(pcie_device_pg0.EnclosureHandle);
--
2.17.0
[PATCH 5/8] mpt3sas: Introduce struct mpt3sas_nvme_cmd
Make _base_build_nvme_prp() easier to read by introducing a structure
to access NVMe command fields.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 13 -
drivers/scsi/mpt3sas/mpt3sas_base.h | 7 +--
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 67e1b603f287..d681e4f2691a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1834,6 +1834,8 @@ _base_build_nvme_prp(struct MPT3SAS_ADAPTER *ioc, u16
smid,
u32 offset, entry_len;
u32 page_mask_result, page_mask;
size_t length;
+ struct mpt3sas_nvme_cmd *nvme_cmd =
+ (void *)nvme_encap_request->NVMe_Command;
/*
* Not all commands require a data transfer. If no data, just return
@@ -1841,15 +1843,8 @@ _base_build_nvme_prp(struct MPT3SAS_ADAPTER *ioc, u16
smid,
*/
if (!data_in_sz && !data_out_sz)
return;
- /*
-* Set pointers to PRP1 and PRP2, which are in the NVMe command.
-* PRP1 is located at a 24 byte offset from the start of the NVMe
-* command. Then set the current PRP entry pointer to PRP1.
-*/
- prp1_entry = (__le64 *)(nvme_encap_request->NVMe_Command +
- NVME_CMD_PRP1_OFFSET);
- prp2_entry = (__le64 *)(nvme_encap_request->NVMe_Command +
- NVME_CMD_PRP2_OFFSET);
+ prp1_entry = _cmd->prp1;
+ prp2_entry = _cmd->prp2;
prp_entry = prp1_entry;
/*
* For the PRP entries, use the specially allocated buffer of
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index f02974c0be4a..c6c36750deaa 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -143,14 +143,17 @@
* NVMe defines
*/
#defineNVME_PRP_SIZE 8 /* PRP size */
-#defineNVME_CMD_PRP1_OFFSET24 /* PRP1 offset in NVMe
cmd */
-#defineNVME_CMD_PRP2_OFFSET32 /* PRP2 offset in NVMe
cmd */
#defineNVME_ERROR_RESPONSE_SIZE16 /* Max NVME Error
Response */
#define NVME_TASK_ABORT_MIN_TIMEOUT6
#define NVME_TASK_ABORT_MAX_TIMEOUT60
#define NVME_TASK_MNGT_CUSTOM_MASK (0x0010)
#defineNVME_PRP_PAGE_SIZE 4096/* Page size */
+struct mpt3sas_nvme_cmd {
+ u8 rsvd[24];
+ __le64 prp1;
+ __le64 prp2;
+};
/*
* reset phases
--
2.17.0
[PATCH 7/8] mpt3sas: Fix a race condition in mpt3sas_base_hard_reset_handler()
Since ioc->shost_recovery is set after ioc->reset_in_progress_mutex
is obtained, if concurrent resets are issued there is a short time
during which ioc->reset_in_progress_mutex is locked and
ioc->shost_recovery == 0. Avoid that this can cause trouble by
unconditionally locking ioc->shost_recovery.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 10 +-
drivers/scsi/mpt3sas/mpt3sas_base.h | 1 -
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index d681e4f2691a..c5cb1b2333c0 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6932,14 +6932,7 @@ mpt3sas_base_hard_reset_handler(struct MPT3SAS_ADAPTER
*ioc,
mpt3sas_halt_firmware(ioc);
/* wait for an active reset in progress to complete */
- if (!mutex_trylock(>reset_in_progress_mutex)) {
- do {
- ssleep(1);
- } while (ioc->shost_recovery == 1);
- dtmprintk(ioc, pr_info(MPT3SAS_FMT "%s: exit\n", ioc->name,
- __func__));
- return ioc->ioc_reset_in_progress_status;
- }
+ mutex_lock(>reset_in_progress_mutex);
spin_lock_irqsave(>ioc_reset_in_progress_lock, flags);
ioc->shost_recovery = 1;
@@ -6988,7 +6981,6 @@ mpt3sas_base_hard_reset_handler(struct MPT3SAS_ADAPTER
*ioc,
ioc->name, __func__, ((r == 0) ? "SUCCESS" : "FAILED")));
spin_lock_irqsave(>ioc_reset_in_progress_lock, flags);
- ioc->ioc_reset_in_progress_status = r;
ioc->shost_recovery = 0;
spin_unlock_irqrestore(>ioc_reset_in_progress_lock, flags);
ioc->ioc_reset_count++;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index c6c36750deaa..a23ae2eb9c09 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1165,7 +1165,6 @@ struct MPT3SAS_ADAPTER {
struct mutexreset_in_progress_mutex;
spinlock_t ioc_reset_in_progress_lock;
u8 ioc_link_reset_in_progress;
- u8 ioc_reset_in_progress_status;
u8 ignore_loginfos;
u8 remove_host;
--
2.17.0
[PATCH 4/8] mpt3sas: Fix kernel-doc warnings
This patch avoids that warnings about the kernel headers appear when
building with W=1.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 16 +-
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 251 +++-
drivers/scsi/mpt3sas/mpt3sas_scsih.c| 36 +--
drivers/scsi/mpt3sas/mpt3sas_transport.c| 10 +-
drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c | 18 +-
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c| 3 -
6 files changed, 186 insertions(+), 148 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 9c233ddc5b2b..67e1b603f287 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -103,7 +103,8 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc);
/**
* _scsih_set_fwfault_debug - global setting of ioc->fwfault_debug.
- *
+ * @val: ?
+ * @kp: ?
*/
static int
_scsih_set_fwfault_debug(const char *val, const struct kernel_param *kp)
@@ -197,7 +198,7 @@ _base_clone_to_sys_mem(void __iomem *dst_iomem, void *src,
u32 size)
* @smid: system request message index
* @sge_chain_count: Scatter gather chain count.
*
- * @Return: chain address.
+ * Returns the chain address.
*/
static inline void __iomem*
_base_get_chain(struct MPT3SAS_ADAPTER *ioc, u16 smid,
@@ -322,8 +323,6 @@ _base_get_chain_buffer_dma_to_chain_buffer(struct
MPT3SAS_ADAPTER *ioc,
* @ioc: per adapter object.
* @mpi_request: mf request pointer.
* @smid: system request message index.
- *
- * @Returns: Nothing.
*/
static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
void *mpi_request, u16 smid)
@@ -1358,7 +1357,6 @@ union reply_descriptor {
* _base_interrupt - MPT adapter (IOC) specific interrupt handler.
* @irq: irq number (not used)
* @bus_id: bus identifier cookie == pointer to MPT_ADAPTER structure
- * @r: pt_regs pointer (not used)
*
* Return IRQ_HANDLE if processed, else IRQ_NONE.
*/
@@ -1777,7 +1775,7 @@ _base_build_sg(struct MPT3SAS_ADAPTER *ioc, void *psge,
* describes the first data memory segment, and PRP2 contains a pointer to a
PRP
* list located elsewhere in memory to describe the remaining data memory
* segments. The PRP list will be contiguous.
-
+ *
* The native SGL for NVMe devices is a Physical Region Page (PRP). A PRP
* consists of a list of PRP entries to describe a number of noncontigous
* physical memory segments as a single memory buffer, just as a SGL does.
Note
@@ -3350,7 +3348,6 @@ _base_mpi_ep_writeq(__u64 b, volatile void __iomem *addr,
/**
* _base_writeq - 64 bit write to MMIO
- * @ioc: per adapter object
* @b: data payload
* @addr: address in MMIO space
* @writeq_lock: spin lock
@@ -4980,6 +4977,7 @@ mpt3sas_base_get_iocstate(struct MPT3SAS_ADAPTER *ioc,
int cooked)
/**
* _base_wait_on_iocstate - waiting on a particular ioc state
+ * @ioc: ?
* @ioc_state: controller state { READY, OPERATIONAL, or RESET }
* @timeout: timeout in second
*
@@ -5011,7 +5009,6 @@ _base_wait_on_iocstate(struct MPT3SAS_ADAPTER *ioc, u32
ioc_state, int timeout)
* _base_wait_for_doorbell_int - waiting for controller interrupt(generated by
* a write to the doorbell)
* @ioc: per adapter object
- * @timeout: timeout in second
*
* Returns 0 for success, non-zero for failure.
*
@@ -5529,6 +5526,7 @@ mpt3sas_base_scsi_enclosure_processor(struct
MPT3SAS_ADAPTER *ioc,
/**
* _base_get_port_facts - obtain port facts reply and save in ioc
* @ioc: per adapter object
+ * @port: ?
*
* Returns 0 for success, non-zero for failure.
*/
@@ -6109,7 +6107,7 @@ _base_event_notification(struct MPT3SAS_ADAPTER *ioc)
/**
* mpt3sas_base_validate_event_type - validating event types
* @ioc: per adapter object
- * @event: firmware event
+ * @event_type: firmware event
*
* This will turn on firmware event notification when application
* ask for that event. We don't mask events that are already enabled.
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 07d28b7d5f1c..f84ffa07f525 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -412,7 +412,7 @@ mpt3sas_ctl_event_callback(struct MPT3SAS_ADAPTER *ioc, u8
msix_index,
/**
* _ctl_verify_adapter - validates ioc_number passed from application
- * @ioc: per adapter object
+ * @ioc_number: ?
* @iocpp: The ioc pointer is returned in this.
* @mpi_version: will be MPI2_VERSION for mpt2ctl ioctl device &
* MPI25_VERSION | MPI26_VERSION for mpt3ctl ioctl device.
@@ -516,9 +516,9 @@ mpt3sas_ctl_reset_handler(struct MPT3SAS_ADAPTER *ioc, int
reset_phase)
/**
* _ctl_fasync -
- * @fd -
- * @filep -
- * @mode -
+ * @fd: ?
+ * @filep: ?
+ * @mode: ?
*
* Called when application request fasyn callback handler.
*/
@@ -530,8 +530,8 @@ _ctl_fasync(int fd, struct file *filep, i
[PATCH 8/8] mpt3sas: Split _base_reset_handler(), mpt3sas_scsih_reset_handler() and mpt3sas_ctl_reset_handler()
Split each of these functions in three functions - one function
per reset phase. This patch does not change any functionality but
makes the code easier to read.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 118 ++-
drivers/scsi/mpt3sas/mpt3sas_base.h | 15 ++--
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 91 +++--
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 94 +++--
4 files changed, 168 insertions(+), 150 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index c5cb1b2333c0..395c7ddb0be1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6813,65 +6813,69 @@ mpt3sas_base_detach(struct MPT3SAS_ADAPTER *ioc)
}
/**
- * _base_reset_handler - reset callback handler (for base)
+ * _base_pre_reset_handler - pre reset handler
* @ioc: per adapter object
- * @reset_phase: phase
- *
- * The handler for doing any required cleanup or initialization.
- *
- * The reset phase can be MPT3_IOC_PRE_RESET, MPT3_IOC_AFTER_RESET,
- * MPT3_IOC_DONE_RESET
- *
- * Return nothing.
*/
-static void
-_base_reset_handler(struct MPT3SAS_ADAPTER *ioc, int reset_phase)
+static void _base_pre_reset_handler(struct MPT3SAS_ADAPTER *ioc)
{
- mpt3sas_scsih_reset_handler(ioc, reset_phase);
- mpt3sas_ctl_reset_handler(ioc, reset_phase);
- switch (reset_phase) {
- case MPT3_IOC_PRE_RESET:
- dtmprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: MPT3_IOC_PRE_RESET\n", ioc->name, __func__));
- break;
- case MPT3_IOC_AFTER_RESET:
- dtmprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: MPT3_IOC_AFTER_RESET\n", ioc->name, __func__));
- if (ioc->transport_cmds.status & MPT3_CMD_PENDING) {
- ioc->transport_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->transport_cmds.smid);
- complete(>transport_cmds.done);
- }
- if (ioc->base_cmds.status & MPT3_CMD_PENDING) {
- ioc->base_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->base_cmds.smid);
- complete(>base_cmds.done);
- }
- if (ioc->port_enable_cmds.status & MPT3_CMD_PENDING) {
- ioc->port_enable_failed = 1;
- ioc->port_enable_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->port_enable_cmds.smid);
- if (ioc->is_driver_loading) {
- ioc->start_scan_failed =
- MPI2_IOCSTATUS_INTERNAL_ERROR;
- ioc->start_scan = 0;
- ioc->port_enable_cmds.status =
- MPT3_CMD_NOT_USED;
- } else
- complete(>port_enable_cmds.done);
- }
- if (ioc->config_cmds.status & MPT3_CMD_PENDING) {
- ioc->config_cmds.status |= MPT3_CMD_RESET;
- mpt3sas_base_free_smid(ioc, ioc->config_cmds.smid);
- ioc->config_cmds.smid = USHRT_MAX;
- complete(>config_cmds.done);
+ mpt3sas_scsih_pre_reset_handler(ioc);
+ mpt3sas_ctl_pre_reset_handler(ioc);
+ dtmprintk(ioc, pr_info(MPT3SAS_FMT
+ "%s: MPT3_IOC_PRE_RESET\n", ioc->name, __func__));
+}
+
+/**
+ * _base_after_reset_handler - after reset handler
+ * @ioc: per adapter object
+ */
+static void _base_after_reset_handler(struct MPT3SAS_ADAPTER *ioc)
+{
+ mpt3sas_scsih_after_reset_handler(ioc);
+ mpt3sas_ctl_after_reset_handler(ioc);
+ dtmprintk(ioc, pr_info(MPT3SAS_FMT
+ "%s: MPT3_IOC_AFTER_RESET\n", ioc->name, __func__));
+ if (ioc->transport_cmds.status & MPT3_CMD_PENDING) {
+ ioc->transport_cmds.status |= MPT3_CMD_RESET;
+ mpt3sas_base_free_smid(ioc, ioc->transport_cmds.smid);
+ complete(>transport_cmds.done);
+ }
+ if (ioc->base_cmds.status & MPT3_CMD_PENDING) {
+ ioc->base_cmds.status |= MPT3_CMD_RESET;
+ mpt3sas_base_free_smid(ioc, ioc->base_cmds.smid);
+ complete(>base_cmds.done);
+ }
+ if (ioc->port_enable_cmds.status & MPT3_CMD_PENDING) {
+ ioc->port_enable_failed = 1;
+ ioc->port_enable_cmds.status |= MPT3_CMD_RESET;
+ mpt3sas_base_free_smid(ioc, ioc->port_enable_cmds.smid);
+ if
[PATCH 6/8] mpt3sas: Fix _transport_smp_handler() error path
This patch avoids that smatch complains about a double unlock on
ioc->transport_cmds.mutex.
Fixes: 651a01364994 ("scsi: scsi_transport_sas: switch to bsg-lib for SMP
passthrough")
Signed-off-by: Bart Van Assche
Cc: Christoph Hellwig
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
Cc: sta...@vger.kernel.org
---
drivers/scsi/mpt3sas/mpt3sas_transport.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 77a8ec401461..53224d97f6c1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -1938,12 +1938,12 @@ _transport_smp_handler(struct bsg_job *job, struct
Scsi_Host *shost,
pr_info(MPT3SAS_FMT "%s: host reset in progress!\n",
__func__, ioc->name);
rc = -EFAULT;
- goto out;
+ goto job_done;
}
rc = mutex_lock_interruptible(>transport_cmds.mutex);
if (rc)
- goto out;
+ goto job_done;
if (ioc->transport_cmds.status != MPT3_CMD_NOT_USED) {
pr_err(MPT3SAS_FMT "%s: transport_cmds in use\n", ioc->name,
@@ -2068,6 +2068,7 @@ _transport_smp_handler(struct bsg_job *job, struct
Scsi_Host *shost,
out:
ioc->transport_cmds.status = MPT3_CMD_NOT_USED;
mutex_unlock(>transport_cmds.mutex);
+job_done:
bsg_job_done(job, rc, reslen);
}
--
2.17.0
[PATCH 3/8] mpt3sas: Annotate switch/case fall-through
This patch avoids that gcc complains about switch/case fall-through when building with W=1. Signed-off-by: Bart Van Assche Cc: Sathya Prakash Cc: Chaitra P B Cc: Suganath Prabu Subramani --- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 1 + drivers/scsi/mpt3sas/mpt3sas_scsih.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index 3269ef43f07e..07d28b7d5f1c 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -970,6 +970,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, } /* drop to default case for posting the request */ } + /* fall through */ default: ioc->build_sg_mpi(ioc, psge, data_out_dma, data_out_sz, data_in_dma, data_in_sz); diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index cac9a264e152..f1a748081fe4 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -5414,6 +5414,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply) case MPI2_IOCSTATUS_SCSI_DATA_OVERRUN: scsi_set_resid(scmd, 0); + /* fall through */ case MPI2_IOCSTATUS_SCSI_RECOVERED_ERROR: case MPI2_IOCSTATUS_SUCCESS: scmd->result = (DID_OK << 16) | scsi_status; @@ -6444,6 +6445,7 @@ _scsih_sas_topology_change_event(struct MPT3SAS_ADAPTER *ioc, if (!test_bit(handle, ioc->pend_os_device_add)) break; + /* fall through */ case MPI2_EVENT_SAS_TOPO_RC_TARG_ADDED: @@ -7160,6 +7162,7 @@ _scsih_pcie_topology_change_event(struct MPT3SAS_ADAPTER *ioc, event_data->PortEntry[i].PortStatus &= 0xF0; event_data->PortEntry[i].PortStatus |= MPI26_EVENT_PCIE_TOPO_PS_DEV_ADDED; + /* fall through */ case MPI26_EVENT_PCIE_TOPO_PS_DEV_ADDED: if (ioc->shost_recovery) break; -- 2.17.0
[PATCH 0/8] mpt3sas: Fix static source code checker complaints
Hello Martin, This patch series addresses the complaints reported by multiple static source code analysis tools about the mpt3sas source code. Please consider these patches for kernel v4.19. Thanks, Bart. Bart Van Assche (8): mpt3sas: Fix indentation mpt3sas: Remove set-but-not-used variables mpt3sas: Annotate switch/case fall-through mpt3sas: Fix kernel-doc warnings mpt3sas: Introduce struct mpt3sas_nvme_cmd mpt3sas: Fix _transport_smp_handler() error path mpt3sas: Fix a race condition in mpt3sas_base_hard_reset_handler() mpt3sas: Split _base_reset_handler(), mpt3sas_scsih_reset_handler() and mpt3sas_ctl_reset_handler() drivers/scsi/mpt3sas/mpt3sas_base.c | 169 +- drivers/scsi/mpt3sas/mpt3sas_base.h | 23 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 343 +++- drivers/scsi/mpt3sas/mpt3sas_scsih.c| 178 +- drivers/scsi/mpt3sas/mpt3sas_transport.c| 17 +- drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c | 18 +- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c| 3 - 7 files changed, 393 insertions(+), 358 deletions(-) -- 2.17.0
[PATCH 1/8] mpt3sas: Fix indentation
Modify the indentation such that smatch no longer complains about
inconsistent indenting.
Signed-off-by: Bart Van Assche
Cc: Sathya Prakash
Cc: Chaitra P B
Cc: Suganath Prabu Subramani
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 40
drivers/scsi/mpt3sas/mpt3sas_transport.c | 2 +-
3 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 569392d0d4c9..798ee62c97f1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -633,7 +633,7 @@ mpt3sas_base_start_watchdog(struct MPT3SAS_ADAPTER *ioc)
if (!ioc->fault_reset_work_q) {
pr_err(MPT3SAS_FMT "%s: failed (line=%d)\n",
ioc->name, __func__, __LINE__);
- return;
+ return;
}
spin_lock_irqsave(>ioc_reset_in_progress_lock, flags);
if (ioc->fault_reset_work_q)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b8d131a455d0..42a8d79be7ec 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2256,7 +2256,7 @@ scsih_slave_configure(struct scsi_device *sdev)
ds = "SSP";
} else {
qdepth = MPT3SAS_SATA_QUEUE_DEPTH;
-if (raid_device->device_info &
+ if (raid_device->device_info &
MPI2_SAS_DEVICE_INFO_SATA_DEVICE)
ds = "SATA";
else
@@ -4004,19 +4004,19 @@ _scsih_issue_delayed_event_ack(struct MPT3SAS_ADAPTER
*ioc, u16 smid, U16 event,
static void
_scsih_issue_delayed_sas_io_unit_ctrl(struct MPT3SAS_ADAPTER *ioc,
u16 smid, u16 handle)
- {
- Mpi2SasIoUnitControlRequest_t *mpi_request;
- u32 ioc_state;
- int i = smid - ioc->internal_smid;
- unsigned long flags;
+{
+ Mpi2SasIoUnitControlRequest_t *mpi_request;
+ u32 ioc_state;
+ int i = smid - ioc->internal_smid;
+ unsigned long flags;
- if (ioc->remove_host) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
+ if (ioc->remove_host) {
+ dewtprintk(ioc, pr_info(MPT3SAS_FMT
"%s: host has been removed\n",
__func__, ioc->name));
- return;
- } else if (ioc->pci_error_recovery) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
+ return;
+ } else if (ioc->pci_error_recovery) {
+ dewtprintk(ioc, pr_info(MPT3SAS_FMT
"%s: host in pci error recovery\n",
__func__, ioc->name));
return;
@@ -4674,19 +4674,19 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd
*scmd)
}
- /* host recovery or link resets sent via IOCTLs */
- if (ioc->shost_recovery || ioc->ioc_link_reset_in_progress)
+ if (ioc->shost_recovery || ioc->ioc_link_reset_in_progress) {
+ /* host recovery or link resets sent via IOCTLs */
return SCSI_MLQUEUE_HOST_BUSY;
-
- /* device has been deleted */
- else if (sas_target_priv_data->deleted) {
+ } else if (sas_target_priv_data->deleted) {
+ /* device has been deleted */
scmd->result = DID_NO_CONNECT << 16;
scmd->scsi_done(scmd);
return 0;
- /* device busy with task management */
} else if (sas_target_priv_data->tm_busy ||
- sas_device_priv_data->block)
+ sas_device_priv_data->block) {
+ /* device busy with task management */
return SCSI_MLQUEUE_DEVICE_BUSY;
+ }
/*
* Bug work around for firmware SATL handling. The loop
@@ -5812,7 +5812,7 @@ _scsih_expander_add(struct MPT3SAS_ADAPTER *ioc, u16
handle)
}
_scsih_expander_node_add(ioc, sas_expander);
-return 0;
+ return 0;
out_fail:
@@ -9519,7 +9519,7 @@ _mpt3sas_fw_work(struct MPT3SAS_ADAPTER *ioc, struct
fw_event_work *fw_event)
break;
case MPT3SAS_PORT_ENABLE_COMPLETE:
ioc->start_scan = 0;
- if (missing_delay[0] != -1 && missing_delay[1] != -1)
+ if (missing_delay[0] != -1 && missing_delay[1] != -1)
mpt3sas_base_update_missing_delay(ioc, missing_delay[0],
missing_delay[1]);
dewtprintk(ioc, pr_info(MPT3SAS_FMT
diff --git a/drivers/scsi/mpt3sas/mpt3sas_tra
Re: [PATCH] mpt3sas: Fix calltrace observed while running IO & host reset
On Thu, 2018-06-14 at 15:56 +0530, Chaitra Basappa wrote: > Bart, > Please see my replies inline. Hello Chaitra, Please don't use inline replies. That makes it very hard to follow the conversation. BTW, in the headers of your e-mail I found the following: "X-Mailer: Microsoft Outlook 14.0". Please use another e-mail client that is better suited for collaboration on open source mailing lists. If outgoing e-mails have to pass through an Exchange server, the following e-mail clients support Exchange servers and are better suited for open source collaboration: * Evolution (Linux only). * Thunderbird + ExQuilla plugin. * If IMAP has been enabled on the Exchange server that you are using then that means that you can choose from the many open source e-mail clients that support IMAP. Thanks, Bart.
Re: [PATCH v1] mpt3sas: Fix calltrace observed while running IO & host reset
On Thu, 2018-06-14 at 06:25 -0400, Chaitra P B wrote:
> As a part of host reset operation, driver will flushout all IOs
> outstanding at driver level with "DID_RESET" result.
> To find which are all commands outstanding at the driver level,
> driver loops with smid starting from one to HBA queue depth and
> calls mpt3sas_scsih_scsi_lookup_get() to get scmd as shown below
>
> for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
> scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
> if (!scmd)
> continue;
>
> But in mpt3sas_scsih_scsi_lookup_get() function, driver returns some
> scsi cmnds which are not outstanding at the driver level (possibly request
> is constructed at block layer since QUEUE_FLAG_QUIESCED is not set. Even if
> driver uses scsi_block_requests and scsi_unblock_requests, issue still
> persists as they will be just blocking further IO from scsi layer and
> not from block layer) and these commands are flushed with
> DID_RESET host bytes thus resulting into above kernel BUG.
Have you considered to call scsi_target_block() before flushing out pending
I/O and to call scsi_target_unblock() after flushing out pending I/O has
finished? I think that would be a much cleaner solution than the proposed
patch. The effect of scsi_target_block() is explained above
scsi_internal_device_block():
/**
* scsi_internal_device_block - try to transition to the SDEV_BLOCK state
* @sdev: device to block
*
* Pause SCSI command processing on the specified device and wait until all
* ongoing scsi_request_fn() / scsi_queue_rq() calls have finished. May sleep.
*
* Returns zero if successful or a negative error code upon failure.
*
* Note:
* This routine transitions the device to the SDEV_BLOCK state (which must be
* a legal transition). When the device is in this state, command processing
* is paused until the device leaves the SDEV_BLOCK state. See also
* scsi_internal_device_unblock().
*
* To do: avoid that scsi_send_eh_cmnd() calls queuecommand() after
* scsi_internal_device_block() has blocked a SCSI device and also
* remove the rport mutex lock and unlock calls from srp_queuecommand().
*/
Thanks,
Bart.
Re: [PATCH] mpt3sas: Fix calltrace observed while running IO & host reset
On Wed, 2018-06-13 at 15:46 +0530, Chaitra Basappa wrote: > When host reset is issued from application, through ioctl reset handler > _ctl_do_reset() -> mpt3sas_base_hard_reset_handler() sets > “ioc->shost_recovery” flag. > If “ioc->shost_recovery” flag is set then driver will return all the > incoming SCSI cmds with “SCSI_MLQUEUE_HOST_BUSY” in the scsih_qcmd(). And > hence no new request gets processed by the driver until the reset completes, > which guarantees that the smid won't change. Hello Chaitra, The patch at the start of this e-mail thread checks whether st->smid is zero. That check could only be useful if there would be code in the mpt3sas driver that clears that field upon command completion. However, I haven't found any such code in the mpt3sas driver. Another concern is that setting ioc->shost_recovery prevents new calls of scsih_qcmd() to submit any commands. But I don't think that setting that flag prevents any scsih_qcmd() calls that had already been started to submit a new command. In other words, I don't think that checking whether or not st->smid == 0 is sufficient to fix the reported race. Bart.
Re: [PATCH] mpt3sas: Fix calltrace observed while running IO & host reset
On Tue, 2018-06-12 at 09:17 -0400, Chaitra P B wrote:
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 23902ad..96e523a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -1489,7 +1489,7 @@ struct scsi_cmnd *
> scmd = scsi_host_find_tag(ioc->shost, unique_tag);
> if (scmd) {
> st = scsi_cmd_priv(scmd);
> - if (st->cb_idx == 0xFF)
> + if (st->cb_idx == 0xFF || st->smid == 0)
> scmd = NULL;
> }
> }
What guarantees that st->smid won't change after it has been checked and
before scmd is used?
Thanks,
Bart.
Re: Possible race in completion with SRP after abort with latest upstream kernel 4.17.0+
On Thu, 2018-06-07 at 09:23 -0400, Laurence Oberman wrote: > Have you seen this before, let me know what else you want from the dump > while I look further. Hello Laurence, I haven't seen this before and I can't reproduce this by running srp-tests against Linus' latest master. So it would be appreciated if you could tell me how to reproduce this behavior or if you could run a bisect. Thanks, Bart.
Re: [PATCH] qla2xxx: Fix setting lower transfer speed if GPSC fails
On Sun, 2018-06-03 at 22:09 -0700, Himanshu Madhani wrote: > This patch prevents driver from setting lower default speed > of 1 GB/sec, if the switch does not support Get Port Speed > Capabilities (GPSC) command. Setting this default speed results > into much lower write performance for large sequential WRITE. > This patch modifies driver to check for gpsc_supported flags and > prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set > default speed of 1 GB/sec. If driver does not send this mailbox > command, firmware assumes maximum supported link speed and will > operate at the max speed. > > Cc: sta...@vger.kernel.org > Signed-off-by: Himanshu Madhani Is this a regression? In other words, does this patch need a "Fixes:" tag? Thanks, Bart.
Re: [PATCH] sd_zbc: Fix sd_zbc_check_zone_size() error path
On Thu, 2018-05-31 at 17:42 +0900, Damien Le Moal wrote:
> If a drive with variable zone sizes or an invalid last zone size is
> detected, the local variable this_zone_blocks is set to 0 and early
> retrun from the function triggered, but this does not result in an
> error return. The local variable zone_blocks must be set to 0 for an
> error to be returned.
>
> Fixes: ccce20fc7968 ("scsi: sd_zbc: Avoid that resetting a zone fails
> sporadically")
> Signed-off-by: Damien Le Moal
> Cc: Bart Van Assche
> ---
> drivers/scsi/sd_zbc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 323e3dc4bc59..850c803a6b3d 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -442,7 +442,7 @@ static s64 sd_zbc_check_zone_size(struct scsi_disk *sdkp)
> } else if (this_zone_blocks != zone_blocks &&
> (block + this_zone_blocks < sdkp->capacity
> || this_zone_blocks > zone_blocks)) {
> - this_zone_blocks = 0;
> + zone_blocks = 0;
> goto out;
> }
> block += this_zone_blocks;
Reviewed-by: Bart Van Assche
[PATCH v2] scsi_transport_srp: Fix shost to rport translation
Since an SRP remote port is attached as a child to shost->shost_gendev
and as the only child, the translation from the shost pointer into an
rport pointer must happen by looking up the shost child that is an
rport. This patch fixes the following KASAN complaint:
BUG: KASAN: slab-out-of-bounds in srp_timed_out+0x57/0x110 [scsi_transport_srp]
Read of size 4 at addr 880035d3fcc0 by task kworker/1:0H/19
CPU: 1 PID: 19 Comm: kworker/1:0H Not tainted 4.16.0-rc3-dbg+ #1
Workqueue: kblockd blk_mq_timeout_work
Call Trace:
dump_stack+0x85/0xc7
print_address_description+0x65/0x270
kasan_report+0x231/0x350
srp_timed_out+0x57/0x110 [scsi_transport_srp]
scsi_times_out+0xc7/0x3f0 [scsi_mod]
blk_mq_terminate_expired+0xc2/0x140
bt_iter+0xbc/0xd0
blk_mq_queue_tag_busy_iter+0x1c7/0x350
blk_mq_timeout_work+0x325/0x3f0
process_one_work+0x441/0xa50
worker_thread+0x76/0x6c0
kthread+0x1b2/0x1d0
ret_from_fork+0x24/0x30
Fixes: e68ca75200fe ("scsi_transport_srp: Reduce failover time")
Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Johannes Thumshirn <jthumsh...@suse.de>
Cc: Jason Gunthorpe <j...@mellanox.com>
Cc: Doug Ledford <dledf...@redhat.com>
Cc: Laurence Oberman <lober...@redhat.com>
Cc: sta...@vger.kernel.org
---
Changes compared to v1: added a check in srp_timed_out() that checks whether
or not rport != NULL.
drivers/scsi/scsi_transport_srp.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_transport_srp.c
b/drivers/scsi/scsi_transport_srp.c
index 36f6190931bc..456ce9f19569 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -51,6 +51,8 @@ struct srp_internal {
struct transport_container rport_attr_cont;
};
+static int scsi_is_srp_rport(const struct device *dev);
+
#define to_srp_internal(tmpl) container_of(tmpl, struct srp_internal, t)
#definedev_to_rport(d) container_of(d, struct srp_rport, dev)
@@ -60,9 +62,24 @@ static inline struct Scsi_Host *rport_to_shost(struct
srp_rport *r)
return dev_to_shost(r->dev.parent);
}
+static int find_child_rport(struct device *dev, void *data)
+{
+ struct device **child = data;
+
+ if (scsi_is_srp_rport(dev)) {
+ WARN_ON_ONCE(*child);
+ *child = dev;
+ }
+ return 0;
+}
+
static inline struct srp_rport *shost_to_rport(struct Scsi_Host *shost)
{
- return transport_class_to_srp_rport(>shost_gendev);
+ struct device *child = NULL;
+
+ WARN_ON_ONCE(device_for_each_child(>shost_gendev, ,
+ find_child_rport) < 0);
+ return child ? dev_to_rport(child) : NULL;
}
/**
@@ -600,7 +617,8 @@ enum blk_eh_timer_return srp_timed_out(struct scsi_cmnd
*scmd)
struct srp_rport *rport = shost_to_rport(shost);
pr_debug("timeout for sdev %s\n", dev_name(>sdev_gendev));
- return rport->fast_io_fail_tmo < 0 && rport->dev_loss_tmo < 0 &&
+ return rport && rport->fast_io_fail_tmo < 0 &&
+ rport->dev_loss_tmo < 0 &&
i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
BLK_EH_RESET_TIMER : BLK_EH_NOT_HANDLED;
}
--
2.16.3
Re: [PATCH V3] scsi: ufs: Add specific callback for setting DMA mask
On Sun, 2018-05-20 at 07:54 +0530, Alim Akhtar wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a355d98..9a1374e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7781,6 +7781,9 @@ EXPORT_SYMBOL_GPL(ufshcd_dealloc_host);
> */
> static int ufshcd_set_dma_mask(struct ufs_hba *hba)
> {
> + if (hba->vops && hba->vops->set_dma_mask)
> + return hba->vops->set_dma_mask(hba);
> +
> if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
> if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
> return 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 1332e54..89c6dae 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -297,6 +297,7 @@ struct ufs_pwr_mode_info {
> * @resume: called during host controller PM callback
> * @dbg_register_dump: used to dump controller debug information
> * @phy_initialization: used to initialize phys
> + * @set_dma_mask: used to set variant specific DMA mask
> */
> struct ufs_hba_variant_ops {
> const char *name;
> @@ -325,6 +326,7 @@ struct ufs_hba_variant_ops {
> int (*resume)(struct ufs_hba *, enum ufs_pm_op);
> void(*dbg_register_dump)(struct ufs_hba *hba);
> int (*phy_initialization)(struct ufs_hba *);
> + int (*set_dma_mask)(struct ufs_hba *hba);
> };
I want to see the code that sets the .set_dma_mask callback function. Where
is it? If it is outside the upstream kernel, please consider to send it
upstream before making changes like this. Adding support for out-of-tree
kernel code is frowned upon big time in the kernel community.
Bart.
Re: [PATCH 3/3] scsi: avoid to hold host-wide counter of host_busy for scsi_mq
On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> +struct scsi_host_mq_in_flight {
> + int cnt;
> +};
> +
> +static void scsi_host_check_in_flight(struct request *rq, void *data,
> + bool reserved)
> +{
> + struct scsi_host_mq_in_flight *in_flight = data;
> +
> + if (blk_mq_request_started(rq))
> + in_flight->cnt++;
> +}
> +
> /**
> * scsi_host_busy - Return the host busy counter
> * @shost: Pointer to Scsi_Host to inc.
> **/
> int scsi_host_busy(struct Scsi_Host *shost)
> {
> - return atomic_read(>host_busy);
> + struct scsi_host_mq_in_flight in_flight = {
> + .cnt = 0,
> + };
> +
> + if (!shost->use_blk_mq)
> + return atomic_read(>host_busy);
> +
> + blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight,
> + _flight);
> + return in_flight.cnt;
> }
> EXPORT_SYMBOL(scsi_host_busy);
This patch introduces a subtle behavior change that has not been explained
in the commit message. If a SCSI request gets requeued that results in a
decrease of the .host_busy counter by scsi_device_unbusy() before the request
is requeued and an increase of the host_busy counter when scsi_queue_rq() is
called again. During that time such requests have the state MQ_RQ_COMPLETE and
hence blk_mq_request_started() will return true and scsi_host_check_in_flight()
will include these requests. In other words, this patch introduces a subtle
behavior change that has not been explained in the commit message. Hence I'm
doubt that this change is correct.
Bart.
Re: [PATCH 2/3] scsi: read host_busy via scsi_host_busy()
On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> show_host_busy(struct device *dev, struct device_attribute *attr, char *buf)
> {
> struct Scsi_Host *shost = class_to_shost(dev);
> - return snprintf(buf, 20, "%d\n", atomic_read(>host_busy));
> + return snprintf(buf, 20, "%d\n", scsi_host_busy(shost));
> }
> static DEVICE_ATTR(host_busy, S_IRUGO, show_host_busy, NULL);
The ", 20" part is cargo-cult programming. Since you have to touch this code,
please either use "sprintf(buf, ...)" or use "scnprintf(buf, PAGE_SIZE, ...)".
Thanks,
Bart.
Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path
On Fri, 2018-04-27 at 09:39 -0600, Jens Axboe wrote: > blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight, > _flight); > return in_flight.cnt + atomic_read(>host_busy); > > The atomic read is basically free, once we get rid of the dirty of that > variable on each IO. Hello Jens, What makes you think that " + atomic_read(>host_busy)" is necessary? I am not aware of any code outside the SCSI core that modifies the host_busy member. Thanks, Bart.
