[PATCH v1 RESEND] mpt3sas: Swap I/O memory read value back to cpu endianness
Swap the I/O memory read value back to cpu endianness before storing it
in a data structures which are defined in the MPI headers where
u8 components are not defined in the endianness order.
In this area from day one mpt3sas driver is using le32_to_cpu() &
cpu_to_le32() APIs. But in the patch cf6bf9710c
(mpt3sas: Bug fix for big endian systems) we have removed these APIs
before reading I/O memory which we should haven't done it. So
in this patch I am correcting it by adding these APIs back
before accessing I/O memory.
v1: Changelog:
Replaced writeq API with __raw_writeq() & mmiowb() APIs.
Signed-off-by: Sreekanth Reddy
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 94359d8..c75e88a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3350,11 +3350,10 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
spinlock_t *writeq_lock)
{
unsigned long flags;
- __u64 data_out = b;
spin_lock_irqsave(writeq_lock, flags);
- writel((u32)(data_out), addr);
- writel((u32)(data_out >> 32), (addr + 4));
+ __raw_writel((u32)(b), addr);
+ __raw_writel((u32)(b >> 32), (addr + 4));
mmiowb();
spin_unlock_irqrestore(writeq_lock, flags);
}
@@ -3374,7 +3373,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
static inline void
_base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock)
{
- writeq(b, addr);
+ __raw_writeq(b, addr);
+ mmiowb();
}
#else
static inline void
@@ -5275,7 +5275,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
/* send message 32-bits at a time */
for (i = 0, failed = 0; i < request_bytes/4 && !failed; i++) {
- writel((u32)(request[i]), >chip->Doorbell);
+ writel(cpu_to_le32(request[i]), >chip->Doorbell);
if ((_base_wait_for_doorbell_ack(ioc, 5)))
failed = 1;
}
@@ -5296,7 +5296,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
}
/* read the first two 16-bits, it gives the total length of the reply */
- reply[0] = (u16)(readl(>chip->Doorbell)
+ reply[0] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
if ((_base_wait_for_doorbell_int(ioc, 5))) {
@@ -5305,7 +5305,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
ioc->name, __LINE__);
return -EFAULT;
}
- reply[1] = (u16)(readl(>chip->Doorbell)
+ reply[1] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
@@ -5319,7 +5319,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
if (i >= reply_bytes/2) /* overflow case */
readl(>chip->Doorbell);
else
- reply[i] = (u16)(readl(>chip->Doorbell)
+ reply[i] = le16_to_cpu(readl(>chip->Doorbell)
& MPI2_DOORBELL_DATA_MASK);
writel(0, >chip->HostInterruptStatus);
}
--
1.8.3.1
[PATCH v1] mpt3sas: correct reset of smid while clearing scsi tracker
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. v1 changelog: Added memory barriers before & after atomic_set() API. Signed-off-by: Sreekanth Reddy --- drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 902610d..94359d8 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) return NULL; chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset]; + /* Adding memory barrier before atomic operation. */ + smp_mb__before_atomic(); atomic_inc(>chain_lookup[smid - 1].chain_offset); return chain_req; } @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, return; st->cb_idx = 0xFF; st->direct_io = 0; - st->smid = 0; + /* Adding memory barrier before atomic operation. */ + smp_mb__before_atomic(); atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); + /* Adding memory barrier after atomic operation. */ + smp_mb__after_atomic(); + st->smid = 0; } /** -- 1.8.3.1
Re: [PATCH 00/16] qla2xxx: Updates for the driver
Hi Martin, > On Jul 30, 2018, at 7:29 PM, Martin K. Petersen > wrote: > > External Email > > Himanshu, > >> This patch series addresses issue with N2N connection for FCP and >> FC-NVMe by moving login to state machine and handle various state >> change. >> >> Please apply this series to 4.19/scsi-queue at your earliest. > > Does not apply, patch #2 fails. Please resubmit, thanks! > Patch #2 depends on 4.18/scsi-fixes patch https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.18/scsi-fixes=36eb8ff672faee83ccce60c191f0fef07c6adce6 Is this something you will fix in merge window, if I resubmit this series without the change which is dependent on this patch. > -- > Martin K. Petersen Oracle Linux Engineering Thanks, - Himanshu
Re: [PATCH] scsi: csiostor: update csio_get_flash_params()
Arjun,
> + switch (density) {
> + case 0x14: /* 1MB */
> + size = 1 << 20;
> + break;
It seems a bit silly to have a switch statement for this. Why not just
do:
size = 1 << density;
?
> + case 0x15: /* 2MB */
> + size = 1 << 21;
> + break;
> + case 0x16: /* 4MB */
> + size = 1 << 22;
> + break;
> + case 0x17: /* 8MB */
> + size = 1 << 23;
> + break;
> + case 0x18: /* 16MB */
> + size = 1 << 24;
> + break;
> + case 0x19: /* 32MB */
> + size = 1 << 25;
> + break;
> + case 0x20: /* 64MB */
> + size = 1 << 26;
> + break;
> + case 0x21: /* 128MB */
> + size = 1 << 27;
> + break;
> + case 0x22: /* 256MB */
> + size = 1 << 28;
> + break;
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 0/9] tcmu: configuration fixes and cleanups
Mike, > The first patch fixes a locking bug in the command setup path. The > rest of the patches fix several bugs and cleanup the setup and > configuration code paths. Applied to 4.19/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 0/2] Improve libiscsi source code annotations
Bart, > The two patches in this series suppress one gcc 8 compiler warning and > one sparse warning. Please consider these for kernel v4.19. Applied to 4.19/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] qedi: Fix a potential buffer overflow
Bart, > Tell snprintf() to store at most 255 characters in the output buffer > instead of 256. This patch avoids that smatch reports the following > warning: Applied to 4.18/scsi-fixes. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/16] qla2xxx: Updates for the driver
Himanshu, > This patch series addresses issue with N2N connection for FCP and > FC-NVMe by moving login to state machine and handle various state > change. > > Please apply this series to 4.19/scsi-queue at your earliest. Does not apply, patch #2 fails. Please resubmit, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] qla2xxx: Fix memory leak for allocating abort IOCB
Himanshu, > In the case of IOCB QFull, Initiator code can leave behind a stale > pointer to an SRB structure on the outstanding command array. Applied to 4.18/scsi-fixes. Thanks! -- Martin K. Petersen Oracle Linux Engineering
[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
Re: [RFC 0/6] scsi: scsi transport for ufs devices
On 2018-07-30 02:13 PM, Hannes Reinecke wrote: On 07/30/2018 08:01 PM, Bart Van Assche wrote: On Sun, 2018-07-29 at 16:33 +0300, Avri Altman wrote: Here is a proposal to use the scsi transport subsystem to manage ufs devices. scsi transport is a framework that allow to send scsi commands to a non-scsi devices. Still, it is flexible enough to allow sending non-scsi commands as well. We will use this framework to manage ufs devices by sending UPIU transactions. We added a new scsi transport module, a ufs-bsg LLD companion, and some new API to the ufs driver. My understanding is that all upstream code uses the bsg interface for *SCSI* commands. Sending UPIU commands over a bsg interface seems like abuse of that interface to me. Aren't you opening a can of worms with such an approach? I beg to disagree. bsg was precisely designed to handle non-SCSI commands, as this was the main limitation of the original 'sg' interface. The original intention was to allow sending of transport frames for the various SCSI transports (like FC or SAS), but there is no direct requirement for bsg to be limited to SCSI. Quite the contrary. I designed 'struct sg_io_v4' found in include/uapi/linux/bsg.h to handle storage related protocols, not just the SCSI command set. After the guard, the next two fields in that structure are: __u32 protocol; /* [i] 0 -> SCSI , */ __u32 subprotocol; /* [i] 0 -> SCSI command, 1 -> SCSI task management function, */ So there is lots of room for other protocols. Also the naming of fields is in terms of request, response, data-in and data-out rather than SCSI command specific terms (e.g. SCSI sense data maps to the response, while the SCSI status is returned via a layered error mechanism). It also handles bidi data transfers (which the sg_io_v3 interface does not). NVMe didn't exist when struct sg_io_v4 was designed. If it was then I would have made provisions for "metadata" transfers. One use for metadata transfer might be to send protection information separately (i.e. as metadata) rather than interleave with the user data as SCSI does. Is NVMe metadata much used? And the extreme case: bidi_data+bidi_metadata, any examples of that? Doug Gilbert
[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;
+
+ kn = sysfs_break_active_protection(>kobj, >attr);
+
[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 07/30/2018 08:01 PM, Bart Van Assche wrote: On Sun, 2018-07-29 at 16:33 +0300, Avri Altman wrote: Here is a proposal to use the scsi transport subsystem to manage ufs devices. scsi transport is a framework that allow to send scsi commands to a non-scsi devices. Still, it is flexible enough to allow sending non-scsi commands as well. We will use this framework to manage ufs devices by sending UPIU transactions. We added a new scsi transport module, a ufs-bsg LLD companion, and some new API to the ufs driver. My understanding is that all upstream code uses the bsg interface for *SCSI* commands. Sending UPIU commands over a bsg interface seems like abuse of that interface to me. Aren't you opening a can of worms with such an approach? I beg to disagree. bsg was precisely designed to handle non-SCSI commands, as this was the main limitation of the original 'sg' interface. The original intention was to allow sending of transport frames for the various SCSI transports (like FC or SAS), but there is no direct requirement for bsg to be limited to SCSI. Quite the contrary. Cheers, Hannes
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
Hello, On Mon, Jul 30, 2018 at 05:50:02PM +, Bart Van Assche wrote: > On Mon, 2018-07-30 at 10:31 -0700, t...@kernel.org wrote: > > On Mon, Jul 30, 2018 at 05:28:11PM +, 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? So, scsi_remove_device() internally protects using scan_mutex and if the whole thing is wrapped with break_active_prot, I don't think you need to call remove_file_self at all, right? Thanks. -- tejun
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 +, 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
Hello, Bart. On Mon, Jul 30, 2018 at 05:28:11PM +, 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. > 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? I don't have an objection for that. Thanks. -- tejun
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.
Greetings!!
Good day I am contacting you regarding a project i wish to execute with you,i will send you a comprehensive report once i get your response with your name and phone number to my mail. Regards, Mr.James Robertson
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
On Sun, Jul 29, 2018 at 04:03:41AM +, 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. > > 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. Ah, you're right. This isn't workqueue. Hmm... yeah, needing to allocate sth in removal path is a bit icky but, it should be okay I guess. I *think* the kernfs active break/unbreak is likely gonna be cleaner tho. Thanks. -- tejun
Re: [PATCH, RESEND] Avoid that SCSI device removal through sysfs triggers a deadlock
Hello, Bart.
On Thu, Jul 26, 2018 at 09:57:40PM +, Bart Van Assche wrote:
...
> @@ -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);
unbreak?
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();
Thanks.
--
tejun
