[PATCH 4/8] zfcp: fix lock imbalance by reworking request queue locking

[Steffen Maier]

From: Martin Peschke 

This patch adds wait_event_interruptible_lock_irq_timeout(), which is a
straight-forward descendant of wait_event_interruptible_timeout() and
wait_event_interruptible_lock_irq().

The zfcp driver used to call wait_event_interruptible_timeout()
in combination with some intricate and error-prone locking. Using
wait_event_interruptible_lock_irq_timeout() as a replacement
nicely cleans up that locking.

This rework removes a situation that resulted in a locking imbalance
in zfcp_qdio_sbal_get():

BUG: workqueue leaked lock or atomic: events/1/0xff00/10
last function: zfcp_fc_wka_port_offline+0x0/0xa0 [zfcp]

It was introduced by commit c2af7545aaff3495d9bf9a7608c52f0af86fb194
"[SCSI] zfcp: Do not wait for SBALs on stopped queue", which had a new
code path related to ZFCP_STATUS_ADAPTER_QDIOUP that took an early exit
without a required lock being held. The problem occured when a
special, non-SCSI I/O request was being submitted in process context,
when the adapter's queues had been torn down. In this case the bug
surfaced when the Fibre Channel port connection for a well-known address
was closed during a concurrent adapter shut-down procedure, which is a
rare constellation.

This patch also fixes these warnings from the sparse tool (make C=1):

drivers/s390/scsi/zfcp_qdio.c:224:12: warning: context imbalance in
 'zfcp_qdio_sbal_check' - wrong count at exit
drivers/s390/scsi/zfcp_qdio.c:244:5: warning: context imbalance in
 'zfcp_qdio_sbal_get' - unexpected unlock

Last but not least, we get rid of that crappy lock-unlock-lock
sequence at the beginning of the critical section.

It is okay to call zfcp_erp_adapter_reopen() with req_q_lock held.

Reported-by: Mikulas Patocka 
Reported-by: Heiko Carstens 
Signed-off-by: Martin Peschke 
Cc: sta...@vger.kernel.org #2.6.35+
Cc: Andrew Morton 
Cc: David Howells 
Cc: Jens Axboe 
Cc: "Paul E. McKenney" 
Cc: Daniel Vetter 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steffen Maier 
---
 drivers/s390/scsi/zfcp_qdio.c |8 +
 include/linux/wait.h  |   57 ++
 2 files changed, 59 insertions(+), 6 deletions(-)

--- a/drivers/s390/scsi/zfcp_qdio.c
+++ b/drivers/s390/scsi/zfcp_qdio.c
@@ -224,11 +224,9 @@ int zfcp_qdio_sbals_from_sg(struct zfcp_
 
 static int zfcp_qdio_sbal_check(struct zfcp_qdio *qdio)
 {
-   spin_lock_irq(&qdio->req_q_lock);
if (atomic_read(&qdio->req_q_free) ||
!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP))
return 1;
-   spin_unlock_irq(&qdio->req_q_lock);
return 0;
 }
 
@@ -246,9 +244,8 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio
 {
long ret;
 
-   spin_unlock_irq(&qdio->req_q_lock);
-   ret = wait_event_interruptible_timeout(qdio->req_q_wq,
-  zfcp_qdio_sbal_check(qdio), 5 * HZ);
+   ret = wait_event_interruptible_lock_irq_timeout(qdio->req_q_wq,
+  zfcp_qdio_sbal_check(qdio), qdio->req_q_lock, 5 * HZ);
 
if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP))
return -EIO;
@@ -262,7 +259,6 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio
zfcp_erp_adapter_reopen(qdio->adapter, 0, "qdsbg_1");
}
 
-   spin_lock_irq(&qdio->req_q_lock);
return -EIO;
 }
 
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -811,6 +811,63 @@ do {   
\
__ret;  \
 })
 
+#define __wait_event_interruptible_lock_irq_timeout(wq, condition, \
+   lock, ret)  \
+do {   \
+   DEFINE_WAIT(__wait);\
+   \
+   for (;;) {  \
+   prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);  \
+   if (condition)  \
+   break;  \
+   if (signal_pending(current)) {  \
+   ret = -ERESTARTSYS; \
+   break;  \
+   }   \
+   spin_unlock_irq(&lock); \
+   ret = schedule_timeout(ret);\
+   spin_lock_irq(&

[PATCH RESEND 1/3] zfcp: fix lock imbalance by reworking request queue locking

[Steffen Maier]

From: Martin Peschke 

This patch adds wait_event_interruptible_lock_irq_timeout(), which is a
straight-forward descendant of wait_event_interruptible_timeout() and
wait_event_interruptible_lock_irq().

The zfcp driver used to call wait_event_interruptible_timeout()
in combination with some intricate and error-prone locking. Using
wait_event_interruptible_lock_irq_timeout() as a replacement
nicely cleans up that locking.

This rework removes a situation that resulted in a locking imbalance
in zfcp_qdio_sbal_get():

BUG: workqueue leaked lock or atomic: events/1/0xff00/10
last function: zfcp_fc_wka_port_offline+0x0/0xa0 [zfcp]

It was introduced by commit c2af7545aaff3495d9bf9a7608c52f0af86fb194
"[SCSI] zfcp: Do not wait for SBALs on stopped queue", which had a new
code path related to ZFCP_STATUS_ADAPTER_QDIOUP that took an early exit
without a required lock being held. The problem occured when a
special, non-SCSI I/O request was being submitted in process context,
when the adapter's queues had been torn down. In this case the bug
surfaced when the Fibre Channel port connection for a well-known address
was closed during a concurrent adapter shut-down procedure, which is a
rare constellation.

This patch also fixes these warnings from the sparse tool (make C=1):

drivers/s390/scsi/zfcp_qdio.c:224:12: warning: context imbalance in
 'zfcp_qdio_sbal_check' - wrong count at exit
drivers/s390/scsi/zfcp_qdio.c:244:5: warning: context imbalance in
 'zfcp_qdio_sbal_get' - unexpected unlock

Last but not least, we get rid of that crappy lock-unlock-lock
sequence at the beginning of the critical section.

It is okay to call zfcp_erp_adapter_reopen() with req_q_lock held.

Reported-by: Mikulas Patocka 
Reported-by: Heiko Carstens 
Signed-off-by: Martin Peschke 
Cc: sta...@vger.kernel.org #2.6.35+
Cc: Andrew Morton 
Cc: David Howells 
Cc: Jens Axboe 
Cc: "Paul E. McKenney" 
Cc: Daniel Vetter 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steffen Maier 
---
 drivers/s390/scsi/zfcp_qdio.c |8 +
 include/linux/wait.h  |   57 ++
 2 files changed, 59 insertions(+), 6 deletions(-)

--- a/drivers/s390/scsi/zfcp_qdio.c
+++ b/drivers/s390/scsi/zfcp_qdio.c
@@ -224,11 +224,9 @@ int zfcp_qdio_sbals_from_sg(struct zfcp_
 
 static int zfcp_qdio_sbal_check(struct zfcp_qdio *qdio)
 {
-   spin_lock_irq(&qdio->req_q_lock);
if (atomic_read(&qdio->req_q_free) ||
!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP))
return 1;
-   spin_unlock_irq(&qdio->req_q_lock);
return 0;
 }
 
@@ -246,9 +244,8 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio
 {
long ret;
 
-   spin_unlock_irq(&qdio->req_q_lock);
-   ret = wait_event_interruptible_timeout(qdio->req_q_wq,
-  zfcp_qdio_sbal_check(qdio), 5 * HZ);
+   ret = wait_event_interruptible_lock_irq_timeout(qdio->req_q_wq,
+  zfcp_qdio_sbal_check(qdio), qdio->req_q_lock, 5 * HZ);
 
if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP))
return -EIO;
@@ -262,7 +259,6 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio
zfcp_erp_adapter_reopen(qdio->adapter, 0, "qdsbg_1");
}
 
-   spin_lock_irq(&qdio->req_q_lock);
return -EIO;
 }
 
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -811,6 +811,63 @@ do {   
\
__ret;  \
 })
 
+#define __wait_event_interruptible_lock_irq_timeout(wq, condition, \
+   lock, ret)  \
+do {   \
+   DEFINE_WAIT(__wait);\
+   \
+   for (;;) {  \
+   prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);  \
+   if (condition)  \
+   break;  \
+   if (signal_pending(current)) {  \
+   ret = -ERESTARTSYS; \
+   break;  \
+   }   \
+   spin_unlock_irq(&lock); \
+   ret = schedule_timeout(ret);\
+   spin_lock_irq(&

Re: [PATCH 7/7] drivers/s390/scsi/zfcp_cfdc.c: remove invalid reference to list iterator variable

[Steffen Maier]

Hi Julia,

sorry for the long delay until I finally responded.
Thanks a lot for your report and patch.
I'll queue this and send it for v3.6rcX hopefully soon.

On 07/08/2012 01:37 PM, Julia Lawall wrote:
> From: Julia Lawall 
>
> If list_for_each_entry, etc complete a traversal of the list, the iterator
> variable ends up pointing to an address at an offset from the list head,
> and not a meaningful structure.  Thus this value should not be used after
> the end of the iterator.  Replace port->adapter->scsi_host by
> adapter->scsi_host.
>
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
>
> Signed-off-by: Julia Lawall 
>
> ---
> This is not tested, an I am not sure that this is the right change.
> Indeed, I'm not at all sure how the original code could have worked, since
> port->adapter->scsi_host should be a completely random value.

This is most probably a copy & paste oversight in
commit a1ca48319a9aa1c5b57ce142f538e76050bb8972
"[SCSI] zfcp: Move ACL/CFDC code to zfcp_cfdc.c"
v2.6.37
where the content of

static void zfcp_erp_port_access_changed(struct zfcp_port *port, char *id,
void *ref)
{
   struct scsi_device *sdev;
   int status = atomic_read(&port->status);

   if (!(status & (ZFCP_STATUS_COMMON_ACCESS_DENIED |
   ZFCP_STATUS_COMMON_ACCESS_BOXED))) {
   shost_for_each_device(sdev, port->adapter->scsi_host)
   

was merged into
zfcp_cfdc_adapter_access_changed(struct zfcp_adapter *adapter)

Since this code is for older hardware and users not using NPIV
and this is only executed on dynamic access changes,
nobody has noticed this so far I guess.

>   drivers/s390/scsi/zfcp_cfdc.c |2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/s390/scsi/zfcp_cfdc.c b/drivers/s390/scsi/zfcp_cfdc.c
> index fab2c25..8ed63aa 100644
> --- a/drivers/s390/scsi/zfcp_cfdc.c
> +++ b/drivers/s390/scsi/zfcp_cfdc.c
> @@ -293,7 +293,7 @@ void zfcp_cfdc_adapter_access_changed(struct zfcp_adapter 
> *adapter)
>   }
>   read_unlock_irqrestore(&adapter->port_list_lock, flags);
>
> - shost_for_each_device(sdev, port->adapter->scsi_host) {
> + shost_for_each_device(sdev, adapter->scsi_host) {
>   zfcp_sdev = sdev_to_zfcp(sdev);
>   status = atomic_read(&zfcp_sdev->status);
>   if ((status & ZFCP_STATUS_COMMON_ACCESS_DENIED) ||
>

Steffen

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

kernel module to list sockets with their multicast group joins and respective processes

[Steffen Maier]

Hello,

below in the text you find a small kernel module as a proof of concept, 
that allows a listing of all current multicast group joins for (UDP/IPv4) 
sockets (NOTE: sockets, neither IP-level nor netdevice-level) along with 
the corresponding process(es) and filedescriptors.


Why did I do this?

Recently two different people asked me about Linux' behavior with respect 
to UDP/IPv4 multicast sockets. The first question was, why two different 
sockets that join different groups receive messages from the respective 
other group (if they are only bound to the wildcard address). Obviously 
this is handled differently in Linux for IPv4, where the socket matching 
for incoming message is done solely on the 4-tuple of addresses and ports, 
and IPv6, where the explicit socket joins are taken into account [1,2]. 
The second question was, if there is a possibility to get a list of all 
processes and the multicast groups they joined. Apparently the usual 
approach involving "netstat -g" (aka /proc/net/igmp[6]) only reports 
aggregates on the level of the host or IP respectively. 
/proc/net/dev_mcast is similar just one level below for the MAC group 
addresses on data link layer. Funny is the fact, that the IGMP code does 
track all group joins for each socket separately. Since I could not find 
any existing method to read that useful information (in user space), I 
wrote the module.


[1] http://www.uwsg.iu.edu/hypermail/linux/net/0211.1/0003.html
[2] http://www.ussg.iu.edu/hypermail/linux/kernel/0110.2/1511.html

Maybe this is useful for somebody, so I share it here.

Regards,
Steffen.

-- sockjoin.c --:

/* sockjoin: Prints UDP/IPv4 sockets and their joined multicast groups.
 *
 * This is more information than the usual "netstat -g" aka
 * /proc/net/igmp (on network layer) and /proc/net/dev_mcast (on data
 * link layer) can provide. Specifically, such data lacks the
 * information which sockets issued explicit group joins, since IP
 * just keeps track of the aggregated union of all group joins of the
 * host. Thankfully the kernel tracks more information internally with
 * a list of current group joins for each socket. This Linux kernel
 * module fits the pieces together and prints it nicely formatted
 * (including the processes accessing multicast sockets) on module load in
 * the kernel log. Future versions should enhance user friendlyness by
 * introduction of a procfs interface.
 *
 * Linux kernel module for Linux version 2.6.
 *
 * (c) 2007 Steffen Maier <[EMAIL PROTECTED]>
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 */

#include  /* printk, KERN_*, NIPQUAD */
#include  /* MODULE_AUTHOR, MODULE_LICENSE,
 MODULE_DESCRIPTION */
#include  /* __init, __exit, module_init(x), module_exit(x) */
#include  /* NULL */
#include  /* udp_hash, udp_hash_lock, UDP_HTABLE_SIZE */
#include  /* ntohs */
#include  /* struct sock */
#include  /* PF_INET */
#include  /* struct ip_mc_socklist */
#include  /* struct ip_mreqn */
#include  /* struct inet_sock, inet_sk */
#include  /* fcheck_files */
#include  /* struct file */
#include  /* read_lock, read_unlock */
#include  /* struct task_struct, tasklist_lock, task_lock,
task_unlock, for_each_process */
#include  /* struct net_device, dev_get_by_index */
#include  /* LINUX_VERSION_CODE, KERNEL_VERSION */

#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0))
# error SockJoin is not designed to work with kernel version earlier than 2.6.0
#endif

/* prototypes (forward declarations) */
/*  internal helper functions */
static inline void search_pids(struct sock *sk, struct ip_mc_socklist *jl);
static void iterate_main(void);
/*  module support */
static int __init sockjoin_init(void);
static void __exit sockjoin_exit(void);

/* beginning of debugging stuff ... */

/* supplement to NIPQUAD */
#define NIPFMT "%u.%u.%u.%u"

#define SOCKJOIN_DEBUG KERN_DEBUG
#if 1  /* don't or do print file name in debug statements */
# define SJDBGFMT "%s:%d:%c: "
# define SJDBGARG __PRETTY_FUNCTION__,__LINE__,(in_interrupt()?'I':'-')
#else
# define SJDBGFMT __FILE__":%d:"__PRETTY_FUNCTION__":%c: "
# define SJDBGARG __LINE__,(in_interrupt()?'I':'-')
#endif

#ifdef SJDEBU

[PATCH 1/2] tracing/events: block: track and print if unplug was explicit or schedule

[Steffen Maier]

Just like blktrace distinguishes explicit and schedule by means of
BLK_TA_UNPLUG_IO and BLK_TA_UNPLUG_TIMER, actually make use of the
existing argument "explicit" to distinguish the two cases in the one
common tracepoint block_unplug.

Complements v2.6.39 commit 49cac01e1fa7 ("block: make unplug timer trace
event correspond to the schedule() unplug") and commit d9c978331790
("block: remove block_unplug_timer() trace point").

Signed-off-by: Steffen Maier 
---
 include/trace/events/block.h | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 81b43f5bdf23..a13613d27cee 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -470,6 +470,11 @@ TRACE_EVENT(block_plug,
TP_printk("[%s]", __entry->comm)
 );
 
+#define show_block_unplug_explicit(val)\
+   __print_symbolic(val,   \
+{false, "schedule"},   \
+{true,  "explicit"})
+
 DECLARE_EVENT_CLASS(block_unplug,
 
TP_PROTO(struct request_queue *q, unsigned int depth, bool explicit),
@@ -478,15 +483,18 @@ DECLARE_EVENT_CLASS(block_unplug,
 
TP_STRUCT__entry(
__field( int,   nr_rq   )
+   __field( bool,  explicit)
__array( char,  comm,   TASK_COMM_LEN   )
),
 
TP_fast_assign(
__entry->nr_rq = depth;
+   __entry->explicit = explicit;
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
 
-   TP_printk("[%s] %d", __entry->comm, __entry->nr_rq)
+   TP_printk("[%s] %d %s", __entry->comm, __entry->nr_rq,
+ show_block_unplug_explicit(__entry->explicit))
 );
 
 /**
-- 
2.13.5

[RFC] tracing/events: block: also try to get dev_t via driver core for some events

[Steffen Maier]

Complements v2.6.31 commit 55782138e47d ("tracing/events: convert block
trace points to TRACE_EVENT()") for cases where rq->rq_disk == NULL:
  block_rq_requeue, block_rq_insert, block_rq_issue;
and for cases where bio == NULL:
  block_getrq, block_sleeprq.

NB: The NULL pointer check for q->kobj.parent is certainly racy and
I don't have enough experience if it's good enough for a trace event.

Since I don't know when above cases would occur,
I'm not sure this is worth it.

Signed-off-by: Steffen Maier 
---
 include/trace/events/block.h | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index cffedc26e8a3..c476b7af404b 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -86,7 +86,9 @@ TRACE_EVENT(block_rq_requeue,
),
 
TP_fast_assign(
-   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) : 0;
+   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) :
+   (q->kobj.parent ?
+container_of(q->kobj.parent, struct device, kobj)->devt : 0);
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
 
@@ -162,7 +164,9 @@ DECLARE_EVENT_CLASS(block_rq,
),
 
TP_fast_assign(
-   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) : 0;
+   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) :
+   (q->kobj.parent ?
+container_of(q->kobj.parent, struct device, kobj)->devt : 0);
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
__entry->bytes = blk_rq_bytes(rq);
@@ -397,7 +401,9 @@ DECLARE_EVENT_CLASS(block_get_rq,
 ),
 
TP_fast_assign(
-   __entry->dev= bio ? bio_dev(bio) : 0;
+   __entry->dev= bio ? bio_dev(bio) :
+   (q->kobj.parent ?
+container_of(q->kobj.parent, struct device, kobj)->devt : 0);
__entry->sector = bio ? bio->bi_iter.bi_sector : 0;
__entry->nr_sector  = bio ? bio_sectors(bio) : 0;
blk_fill_rwbs(__entry->rwbs,
-- 
2.13.5

[PATCH 2/2] tracing/events: block: dev_t via driver core for plug and unplug events

[Steffen Maier]

Complements v2.6.31 commit 55782138e47d ("tracing/events: convert block
trace points to TRACE_EVENT()") to be equivalent to traditional blktrace
output. Also this allows event filtering to not always get all (un)plug
events.

NB: The NULL pointer check for q->kobj.parent is certainly racy and
I don't have enough experience if it's good enough for a trace event.
The change did work for my cases (block device read/write I/O on
zfcp-attached SCSI disks and dm-mpath on top).

While I haven't seen any prior art using driver core (parent) relations
for trace events, there are other cases using this when no direct pointer
exists between objects, such as:
 #define to_scsi_target(d)  container_of(d, struct scsi_target, dev)
 static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 {
return to_scsi_target(sdev->sdev_gendev.parent);
 }

This is the object model we make use of here:

struct gendisk {
struct hd_struct {
struct device {  /*container_of*/
struct kobject kobj; <--+
dev_t  devt; /*deref*/  |
} __dev;|
} part0;|
struct request_queue *queue; ..+|
}  :|
   :|
struct request_queue {  <..+|
/* queue kobject */ |
struct kobject {|
struct kobject *parent; +
} kobj;
}

The parent pointer comes from:
 #define disk_to_dev(disk)  (&(disk)->part0.__dev)
int blk_register_queue(struct gendisk *disk)
struct device *dev = disk_to_dev(disk);
struct request_queue *q = disk->queue;
ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
^^^parent

$ ls -d /sys/block/sdf/queue
/sys/block/sda/queue
$ cat /sys/block/sdf/dev
80:0

A partition does not have its own request queue:

$ cat /sys/block/sdf/sdf1/dev
8:81
$ ls -d /sys/block/sdf/sdf1/queue
ls: cannot access '/sys/block/sdf/sdf1/queue': No such file or directory

The difference to blktrace parsed output is that block events don't use the
partition's minor number but the containing block device's minor number:

$ dd if=/dev/sdf1 count=1

$ cat /sys/kernel/debug/tracing/trace
block_bio_remap: 8,80 R 2048 + 32 <- (8,81) 0
block_bio_queue: 8,80 R 2048 + 32 [dd]
block_getrq: 8,80 R 2048 + 32 [dd]
block_plug: 8,80 [dd]

block_rq_insert: 8,80 R 16384 () 2048 + 32 [dd]
block_unplug: 8,80 [dd] 1 explicit
  
block_rq_issue: 8,80 R 16384 () 2048 + 32 [dd]
block_rq_complete: 8,80 R () 2048 + 32 [0]

$ btrace /dev/sdf1
  8,80   11 0.0 240240  A   R 2048 + 32 <- (8,81) 0
  8,81   12 0.000220890 240240  Q   R 2048 + 32 [dd]
  8,81   13 0.000229639 240240  G   R 2048 + 32 [dd]
  8,81   14 0.000231805 240240  P   N [dd]
^^
  8,81   15 0.000234671 240240  I   R 2048 + 32 [dd]
  8,81   16 0.000236365 240240  U   N [dd] 1
^^
  8,81   17 0.000238527 240240  D   R 2048 + 32 [dd]
  8,81   22 0.000613741 0  C   R 2048 + 32 [0]

Signed-off-by: Steffen Maier 
---
 include/trace/events/block.h | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index a13613d27cee..cffedc26e8a3 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -460,14 +460,18 @@ TRACE_EVENT(block_plug,
TP_ARGS(q),
 
TP_STRUCT__entry(
+   __field( dev_t, dev )
__array( char,  comm,   TASK_COMM_LEN   )
),
 
TP_fast_assign(
+   __entry->dev = q->kobj.parent ?
+   container_of(q->kobj.parent, struct device, kobj)->devt : 0;
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
 
-   TP_printk("[%s]", __entry->comm)
+   TP_printk("%d,%d [%s]",
+ MAJOR(__entry->dev), MINOR(__entry->dev), __entry->comm)
 );
 
 #define show_block_unplug_explicit(val)\
@@ -482,18 +486,23 @@ DECLARE_EVENT_CLASS(block_unplug,
TP_ARGS(q, depth, explicit),
 
TP_STRUCT__entry(
+   __field( dev_t, dev )
__field( int,   nr_rq   )
__field( bool,  explicit)
__array( char,  comm,   TASK_COMM_LEN   )
),
 
TP_fast_assign(
+   __entry->dev   = q->kobj.parent ?
+   container_of(q->kobj.parent

[PATCH 0/2] tracing/events: block: bring more on a par with blktrace

[Steffen Maier]

I had the need to understand I/O request processing in detail.
But I also had the need to enrich block traces with other trace events
including my own dynamic kprobe events. So I preferred block trace events
over blktrace to get everything nicely sorted into one ftrace output.
However, I missed device filtering for (un)plug events and also
the difference between the two flavors of unplug.

The first two patches bring block trace events closer to their
counterpart in blktrace tooling.

The last patch is just an RFC. I still kept it in this patch set because
it is inspired by PATCH 2/2.

Steffen Maier (3):
  tracing/events: block: track and print if unplug was explicit or
schedule
  tracing/events: block: dev_t via driver core for plug and unplug
events
  tracing/events: block: also try to get dev_t via driver core for some
events

 include/trace/events/block.h | 33 -
 1 file changed, 28 insertions(+), 5 deletions(-)

-- 
2.13.5

Re: [PATCH] [SCSI] zfcp: NULL check before some freeing functions is not needed.

[Steffen Maier]

Thanks, this was already submitted for v4.12 with
https://www.spinics.net/lists/linux-scsi/msg125190.html
based on
https://www.spinics.net/lists/linux-s390/msg21307.html

Also, previously seen in
https://www.spinics.net/lists/linux-s390/msg22035.html

On 12/02/2018 09:52 PM, Thomas Meyer wrote:

NULL check before some freeing functions is not needed.

Signed-off-by: Thomas Meyer 
---

diff -u -p a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
--- a/drivers/s390/scsi/zfcp_aux.c
+++ b/drivers/s390/scsi/zfcp_aux.c
@@ -248,20 +248,13 @@ static int zfcp_allocate_low_mem_buffers
  
  static void zfcp_free_low_mem_buffers(struct zfcp_adapter *adapter)

  {
-   if (adapter->pool.erp_req)
-   mempool_destroy(adapter->pool.erp_req);
-   if (adapter->pool.scsi_req)
-   mempool_destroy(adapter->pool.scsi_req);
-   if (adapter->pool.scsi_abort)
-   mempool_destroy(adapter->pool.scsi_abort);
-   if (adapter->pool.qtcb_pool)
-   mempool_destroy(adapter->pool.qtcb_pool);
-   if (adapter->pool.status_read_req)
-   mempool_destroy(adapter->pool.status_read_req);
-   if (adapter->pool.sr_data)
-   mempool_destroy(adapter->pool.sr_data);
-   if (adapter->pool.gid_pn)
-   mempool_destroy(adapter->pool.gid_pn);
+   mempool_destroy(adapter->pool.erp_req);
+   mempool_destroy(adapter->pool.scsi_req);
+   mempool_destroy(adapter->pool.scsi_abort);
+   mempool_destroy(adapter->pool.qtcb_pool);
+   mempool_destroy(adapter->pool.status_read_req);
+   mempool_destroy(adapter->pool.sr_data);
+   mempool_destroy(adapter->pool.gid_pn);
  }
  
  /**





--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 4.14 110/165] scsi: core: return BLK_STS_OK for DID_OK in __scsi_error_from_host_byte()

[Steffen Maier]

Hopefully I haven't missed it in the stable queue, but do we need the 
following on top (effectively not applying e39a97353e53)?:


commit cbe095e2b584623b882ebaf6c18e0b9077baa3f7
Author: Bart Van Assche 
Date:   Thu Apr 5 10:32:59 2018 -0700

Revert "scsi: core: return BLK_STS_OK for DID_OK in 
__scsi_error_from_host_byte()"


The description of commit e39a97353e53 is wrong: it mentions that 
commit
2a842acab109 introduced a bug in __scsi_error_from_host_byte() 
although that
commit did not change the behavior of that function.  Additionally, 
commit

e39a97353e53 introduced a bug: it causes commands that fail with
hostbyte=DID_OK and driverbyte=DRIVER_SENSE to be completed with
BLK_STS_OK. Hence revert that commit.

Fixes: e39a97353e53 ("scsi: core: return BLK_STS_OK for DID_OK in 
__scsi_error_from_host_byte()")

Reported-by: Damien Le Moal 
Signed-off-by: Bart Van Assche 
Cc: Hannes Reinecke 
Cc: Douglas Gilbert 
Cc: Damien Le Moal 
Cc: Christoph Hellwig 
Cc: Lee Duncan 
Cc: sta...@vger.kernel.org
Reviewed-by: Christoph Hellwig 
Reviewed-by: Hannes Reinecke 
Signed-off-by: Martin K. Petersen 


On 05/24/2018 11:38 AM, Greg Kroah-Hartman wrote:

4.14-stable review patch.  If anyone has any objections, please let me know.

--

From: Hannes Reinecke 

[ Upstream commit e39a97353e5378eb46bf01679799c5704d397f32 ]

When converting __scsi_error_from_host_byte() to BLK_STS error codes the
case DID_OK was forgotten, resulting in it always returning an error.

Fixes: 2a842acab109 ("block: introduce new block status code type")
Cc: Doug Gilbert 
Signed-off-by: Hannes Reinecke 
Reviewed-by: Douglas Gilbert 
Signed-off-by: Martin K. Petersen 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
  drivers/scsi/scsi_lib.c |2 ++
  1 file changed, 2 insertions(+)

--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -720,6 +720,8 @@ static blk_status_t __scsi_error_from_ho
int result)
  {
switch (host_byte(result)) {
+   case DID_OK:
+   return BLK_STS_OK;
case DID_TRANSPORT_FAILFAST:
return BLK_STS_TRANSPORT;
case DID_TARGET_FAILURE:





--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 4.14 199/496] scsi: core: return BLK_STS_OK for DID_OK in __scsi_error_from_host_byte()

[Steffen Maier]

question:
drop because of upstream's

cbe095e2b584 ("Revert "scsi: core: return BLK_STS_OK for DID_OK in 
__scsi_error_from_host_byte()"")

Fixes: e39a97353e53 ("scsi: core: return BLK_STS_OK for DID_OK in
__scsi_error_from_host_byte()")

as in stable-queue's

c40541008b93 ("drop a scsi patch from 4.14")

?

On 05/28/2018 11:59 AM, Greg Kroah-Hartman wrote:

4.14-stable review patch.  If anyone has any objections, please let me know.

--

From: Hannes Reinecke 

[ Upstream commit e39a97353e5378eb46bf01679799c5704d397f32 ]

When converting __scsi_error_from_host_byte() to BLK_STS error codes the
case DID_OK was forgotten, resulting in it always returning an error.

Fixes: 2a842acab109 ("block: introduce new block status code type")
Cc: Doug Gilbert 
Signed-off-by: Hannes Reinecke 
Reviewed-by: Douglas Gilbert 
Signed-off-by: Martin K. Petersen 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
  drivers/scsi/scsi_lib.c |2 ++
  1 file changed, 2 insertions(+)

--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -720,6 +720,8 @@ static blk_status_t __scsi_error_from_ho
int result)
  {
switch (host_byte(result)) {
+   case DID_OK:
+   return BLK_STS_OK;
case DID_TRANSPORT_FAILFAST:
return BLK_STS_TRANSPORT;
case DID_TARGET_FAILURE:





--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] scsi: zfcp: fix use-after-free in zfcp_unit_remove

[Steffen Maier]

On 11/26/20 4:12 PM, Benjamin Block wrote:

On Thu, Nov 26, 2020 at 08:07:32PM +0800, Qinglang Miao wrote:

在 2020/11/26 17:42, Benjamin Block 写道:

On Thu, Nov 26, 2020 at 09:13:53AM +0100, Cornelia Huck wrote:

On Thu, 26 Nov 2020 09:27:41 +0800
Qinglang Miao  wrote:

在 2020/11/26 1:06, Benjamin Block 写道:

On Fri, Nov 20, 2020 at 03:48:54PM +0800, Qinglang Miao wrote:



Let's go by example. If we assume the reference count of `unit->dev` is
R, and the function starts with R = 1 (otherwise the deivce would've
been freed already), we get:

  int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun)
  {
struct zfcp_unit *unit;
struct scsi_device *sdev;
write_lock_irq(&port->unit_list_lock);
// unit->dev (R = 1)
unit = _zfcp_unit_find(port, fcp_lun);
// get_device(&unit->dev)
// unit->dev (R = 2)
if (unit)
list_del(&unit->list);
write_unlock_irq(&port->unit_list_lock);
if (!unit)
return -EINVAL;
sdev = zfcp_unit_sdev(unit);
if (sdev) {
scsi_remove_device(sdev);
scsi_device_put(sdev);
}
// unit->dev (R = 2)
put_device(&unit->dev);
// unit->dev (R = 1)
device_unregister(&unit->dev);
// unit->dev (R = 0)
return 0;
  }

If we now apply this patch, we'd end up with R = 1 after
`device_unregister()`, and the device would not be properly removed.

If you still think that's wrong, then you'll need to better explain why.


Hi Banjamin and Cornelia,

Your replies make me reliaze that I've been holding a mistake understanding
of put_device() as well as reference count.

Thanks for you two's patient explanation !!

BTW, should I send a v2 on these two patches to move the position of
put_device()?


Feel free to do so.

I think having the `put_device()` call after `device_unregister()` in
both `zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` is more
natural, because it ought to be the last time we touch the object in
both functions.


If you move put_device(), you could add a comment like we did here to explain 
which (hidden) get_device is undone:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=ef4021fe5fd77ced0323cede27979d80a56211ca
("scsi: zfcp: fix to prevent port_remove with pure auto scan LUNs (only sdevs)")
So in this patch it could be:
put_device(&unit->dev); /* undo _zfcp_unit_find() */
And in the other patch it could be:
put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */
Then it would be clearer next time somebody looks at the code.

Especially for the other patch on zfcp_sysfs_port_remove_store() moving the 
put_device(&port->dev) to at least *after* the call of 
zfcp_erp_port_shutdown(port, 0, "syprs_1") would make the code cleaner to me. 
Along the idead of passing the port to zfcp_erp_port_shutdown with the 
reference we got from zfcp_get_port_by_wwpn(). That said, the current code is 
of course still correct as we currently have the port ref of the earlier 
device_register so passing the port to zfcp_erp_port_shutdown() is safe.


If we wanted to make the gets and puts nicely nested, then we could move the 
puts to just before the device_unregister, but that's bike shedding:

device_register()   --+
get_device() --+  |
put_device() --+  |
device_unregister() --+

Benjamin's suggested move location works for me, too. After all, the kdoc of 
device_unregister explicitly mentions the possibility that other refs might 
continue to exist after device_unregister was called:

device_register()   --+
get_device() -|--+
device_unregister() --+  |
put_device() +

--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] scsi: zfcp: fix indentation coding style issue

[Steffen Maier]

Thanks, will queue it for our next cleanup submission.

On 11/27/20 2:06 PM, Yevhen Viktorov wrote:

code indent should use tabs where possible

Signed-off-by: Yevhen Viktorov 
---
  drivers/s390/scsi/zfcp_def.h | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
index da8a5ceb615c..a2a59cbb330a 100644
--- a/drivers/s390/scsi/zfcp_def.h
+++ b/drivers/s390/scsi/zfcp_def.h
@@ -157,7 +157,7 @@ struct zfcp_adapter {
u32 fsf_lic_version;
u32 adapter_features;  /* FCP channel features */
u32 connection_features; /* host connection 
features */
-u32hardware_version;  /* of FCP channel */
+   u32 hardware_version;  /* of FCP channel */
u32 fc_security_algorithms; /* of FCP channel */
u32 fc_security_algorithms_old; /* of FCP channel */
u16 timer_ticks;   /* time int for a tick */
@@ -181,7 +181,7 @@ struct zfcp_adapter {
rwlock_terp_lock;
wait_queue_head_t   erp_done_wqh;
struct zfcp_erp_action  erp_action;/* pending error recovery */
-atomic_terp_counter;
+   atomic_terp_counter;
u32 erp_total_count;   /* total nr of enqueued erp
  actions */
u32 erp_low_mem_count; /* nr of erp actions waiting
@@ -217,7 +217,7 @@ struct zfcp_port {
u32d_id;   /* D_ID */
u32handle; /* handle assigned by FSF */
struct zfcp_erp_action erp_action; /* pending error recovery */
-atomic_t   erp_counter;
+   atomic_t   erp_counter;
u32maxframe_size;
u32supported_classes;
u32connection_info;




--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: PROBLEM: special sense code asc,ascq=04h,0Ch abort scsi scan in the middle

[Steffen Maier]

t0/scsi_host/host0/scan

kernel: scsi scan: device exists on 0:0:17:0
kernel: scsi scan: Sending REPORT LUNS to host 0 channel 0 id 17 (try 0)
kernel: scsi scan: REPORT LUNS successful (try 0) result 0x0
kernel: sd 0:0:17:0: scsi scan: REPORT LUN scan
kernel: scsi scan: device exists on 0:0:17:0
kernel: sd 0:0:17:0: Unexpected response from lun 1 while scanning, scan aborted

Example 2: PORT RECOVERY

this causes a short interruption of I/O to all LUNs at that target port

includes a scsi target (re)scan of rport-0:0-17 / 0x5005076802100c1a

[root@host:/sys/bus/ccw/drivers/zfcp/0.0.3c40/0x5005076802100c1a](0)#
 echo 0 >| failed

kernel: scsi scan: device exists on 0:0:17:0
kernel: scsi scan: Sending REPORT LUNS to host 0 channel 0 id 17 (try 0)
kernel: sd 0:0:17:0: Done: RETRY
kernel: sd 0:0:17:0:  Result: hostbyte=DID_IMM_RETRY driverbyte=DRIVER_OK
kernel: sd 0:0:17:0: CDB: Report luns: a0 00 00 00 00 00 00 00 10 00 00 00
kernel: scsi scan: REPORT LUNS successful (try 0) result 0x0
kernel: sd 0:0:17:0: scsi scan: REPORT LUN scan
kernel: scsi scan: device exists on 0:0:17:0
kernel: sd 0:0:17:0: Unexpected response from lun 1 while scanning, scan aborted
kernel: scsi scan: device exists on 0:0:17:0
kernel: scsi scan: device exists on 0:0:17:2

Two trailing "device exists" are from zfcp's unit recovery for each
lun at the recovered remote port. This causes additional individual
scsi_scan_target() calls without wildcards but for a specific lun instead.

Example 3: ADAPTER RECOVERY

this causes a short interruption of I/O over all paths through this FCP device

includes recovery of rport-0:0-17 / 0x5005076802100c1a

[root@host:/sys/bus/ccw/drivers/zfcp/0.0.3c40](0)# echo 0 >| failed

kernel: qdio: 0.0.3c40 ZFCP on SC 1b using AI:1 QEBSM:1 PCI:1 TDD:1 SIGA: W A
kernel: scsi scan: device exists on 0:0:17:0
kernel: scsi scan: Sending REPORT LUNS to host 0 channel 0 id 17 (try 0)
kernel: scsi scan: REPORT LUNS successful (try 0) result 0x0
kernel: sd 0:0:17:0: scsi scan: REPORT LUN scan
kernel: scsi scan: device exists on 0:0:17:0
kernel: sd 0:0:17:0: Unexpected response from lun 1 while scanning, scan aborted
kernel: scsi scan: device exists on 0:0:17:0
kernel: scsi scan: device exists on 0:0:17:2

DETAILS
===

Square brackets indicate where above requirements come into play.

[4]
scsi_scan_target(prnt, 0/*channel*/, id/*target*/, SCAN_WILD_CARD/*lun*/, rscan)
__scsi_scan_target()
scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL); [3]
scsi_report_lun_scan(starget, bflags, rescan) [2] {
 foreach lun in report lun response {
  scsi_probe_and_add_lun() {
   if exists => "kernel: scsi scan: device exists on "
   else {
scsi_alloc_sdev() {
 ret = shost->hostt->slave_alloc() => zfcp_scsi_slave_alloc() {
if (!unit && !(allow_lun_scan && npiv)) {
put_device(&port->dev);
return -ENXIO;  [1]
}
 }
 if (ret) {
/*
 * if LLDD reports slave not present, don't clutter
 * console with alloc failure messages
 */
if (ret == -ENXIO)
display_failure_msg = 0;
goto out_device_destroy;
 }
}
if allocation failed, return early with SCSI_SCAN_NO_RESPONSE
else continue lun probing
   }
  }
  if (res == SCSI_SCAN_NO_RESPONSE) {
/*
 * Got some results, but now none, abort.
 */
sdev_printk(KERN_ERR, sdev,
"Unexpected response"
" from lun %d while scanning, scan"
" aborted\n", lun);
break;
  }
 }
}


-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: special sense code asc,ascq=04h,0Ch abort scsi scan in the middle

[Steffen Maier]

On 10/14/2013 03:32 PM, Hannes Reinecke wrote:

On 10/14/2013 03:18 PM, Hannes Reinecke wrote:

On 10/14/2013 02:51 PM, Steffen Maier wrote:

On 10/14/2013 01:13 PM, Hannes Reinecke wrote:

On 10/13/2013 07:23 PM, Vaughan Cao wrote:

[1.] One line summary of the problem:
special sense code asc,ascq=04h,0Ch abort scsi scan in the middle

[2.] Full description of the problem/report:
For instance, storage represents 8 iscsi LUNs, however the LUN No.7
is not well configured or has something wrong.
Then messages received:
kernel: scsi 5:0:0:0: Unexpected response from lun 7 while scanning, scan 
aborted
Which will make LUN No.8 unavailable.
It's confirmed that Windows and Solaris systems will continue the
scan and make LUN No.1,2,3,4,5,6 and 8 available.

Log snippet is as below:
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: scsi scan: INQUIRY pass 1 
length 36
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: Send: 0x8801e9bd4280
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: CDB: Inquiry: 12 00 00 00 24 
00
Aug 24 00:32:49 vmhodtest019 kernel: buffer = 0x8801f71fc180, bufflen = 36, 
queuecommand 0xa00b99e7
Aug 24 00:32:49 vmhodtest019 kernel: leaving scsi_dispatch_cmnd()
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: Done: 0x8801e9bd4280 
SUCCESS
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: Result: hostbyte=DID_OK 
driverbyte=DRIVER_OK
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: CDB: Inquiry: 12 00 00 00 24 
00
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: Sense Key : Not Ready 
[current]
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: Add. Sense: Logical unit not 
accessible, target port in unavailable state
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:7: scsi host busy 1 failed 0
Aug 24 00:32:49 vmhodtest019 kernel: 0 sectors total, 36 bytes done.
Aug 24 00:32:49 vmhodtest019 kernel: scsi scan: INQUIRY failed with code 
0x802
Aug 24 00:32:49 vmhodtest019 kernel: scsi 5:0:0:0: Unexpected response from lun 
7 while scanning, scan aborted

According to scsi_report_lun_scan(), I found:
Linux use an inquiry command to probe a lun according to the result
of report_lun command.
It assumes every probe cmd will get a legal result. Otherwise, it
regards the whole peripheral not exist or dead.
If the return of inquiry passes its legal checking and indicates
'LUN not present', it won't break but also continue with the scan
process.
In the log, inquiry to LUN7 return a sense - asc,ascq=04h,0Ch
(Logical unit not accessible, target port in unavailable state).
And this is ignored, so scsi_probe_lun() returns -EIO and the scan
process is aborted.

I have two questions:
1. Is it correct for hardware to return a sense 04h,0Ch to inquiry
again, even after presenting this lun in responce to REPORT_LUN
command?

Yes, this is correct. 'REPORT LUNS' is supported in 'Unavailable' state.


2. Since windows and solaris can continue scan, is it reasonable for
linux to do the same, even for a fault-tolerance purpose?


Hmm. Yes, and no.

_Actually_ this is an issue with the target, as it looks as if it
will return the above sense code while sending an 'INQUIRY' to the
device.
SPC explicitely states that the INQUIRY command should _not_ fail
for unavailable devices.
But yeah, we probably should work around this issues.
Nevertheless, please raise this issue with your array vendor.

Please try the attached patch.

Cheers,

Hannes




 From b0e90778f012010c881f8bdc03bce63a36921b77 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke 
Date: Mon, 14 Oct 2013 13:11:22 +0200
Subject: [PATCH] scsi_scan: continue report_lun_scan after error

When scsi_probe_and_add_lun() fails in scsi_report_lun_scan() this
does _not_ indicate that the entire target is done for.
So continue scanning for the remaining devices.

Signed-off-by: Hannes Reinecke 

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 307a811..973a121 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1484,13 +1484,12 @@ static int scsi_report_lun_scan(struct scsi_target 
*starget, int bflags,
lun, NULL, NULL, rescan, NULL);
if (res == SCSI_SCAN_NO_RESPONSE) {
/*
-* Got some results, but now none, abort.
+* Got some results, but now none, ignore.
 */
sdev_printk(KERN_ERR, sdev,
"Unexpected response"
-   " from lun %d while scanning, scan"
-   " aborted\n", lun);
-   break;
+   " from lun %d while scanning,"
+   " ignoring device\n", lun);
}
  

Re: [PATCH] s390: scsi: zfcp_aux.c: Cleaning up missing null-terminate in conjunction with strncpy

[Steffen Maier]

On 07/26/2014 04:36 PM, Rickard Strandqvist wrote:

Replacing strncpy with strlcpy to avoid strings that lacks null terminate.

Signed-off-by: Rickard Strandqvist 
---
  drivers/s390/scsi/zfcp_aux.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
index 8004b07..a23ba76 100644
--- a/drivers/s390/scsi/zfcp_aux.c
+++ b/drivers/s390/scsi/zfcp_aux.c
@@ -101,7 +101,7 @@ static void __init zfcp_init_device_setup(char *devstr)
token = strsep(&str, ",");
if (!token || strlen(token) >= ZFCP_BUS_ID_SIZE)
goto err_out;


Due to the check for strlen(token) >= ZFCP_BUS_ID_SIZE we should be safe 
even with strlcpy because we would reject any user string that does not 
fit into busid including the trailing zero character.


Since it works either way, we can change it though,

Acked-by: Steffen Maier 


-   strncpy(busid, token, ZFCP_BUS_ID_SIZE);
+   strlcpy(busid, token, ZFCP_BUS_ID_SIZE);

token = strsep(&str, ",");
if (!token || kstrtoull(token, 0, (unsigned long long *) &wwpn))



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] mm, mempool: do not allow atomic resizing

[Steffen Maier]

for the zfcp part:
Acked-by: Steffen Maier 

On 03/08/2015 04:19 AM, David Rientjes wrote:

Allocating a large number of elements in atomic context could quickly
deplete memory reserves, so just disallow atomic resizing entirely.

Nothing currently uses mempool_resize() with anything other than
GFP_KERNEL, so convert existing callers to drop the gfp_mask.

Signed-off-by: David Rientjes 
---
  drivers/s390/scsi/zfcp_erp.c | 4 ++--
  fs/cifs/connect.c| 6 ++
  include/linux/mempool.h  | 2 +-
  mm/mempool.c | 9 +
  4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -738,11 +738,11 @@ static int zfcp_erp_adapter_strategy_open_fsf(struct 
zfcp_erp_action *act)
return ZFCP_ERP_FAILED;

if (mempool_resize(act->adapter->pool.sr_data,
-  act->adapter->stat_read_buf_num, GFP_KERNEL))
+  act->adapter->stat_read_buf_num))
return ZFCP_ERP_FAILED;

if (mempool_resize(act->adapter->pool.status_read_req,
-  act->adapter->stat_read_buf_num, GFP_KERNEL))
+  act->adapter->stat_read_buf_num))
return ZFCP_ERP_FAILED;

atomic_set(&act->adapter->stat_miss, act->adapter->stat_read_buf_num);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -773,8 +773,7 @@ static void clean_demultiplex_info(struct TCP_Server_Info 
*server)

length = atomic_dec_return(&tcpSesAllocCount);
if (length > 0)
-   mempool_resize(cifs_req_poolp, length + cifs_min_rcv,
-   GFP_KERNEL);
+   mempool_resize(cifs_req_poolp, length + cifs_min_rcv);
  }

  static int
@@ -848,8 +847,7 @@ cifs_demultiplex_thread(void *p)

length = atomic_inc_return(&tcpSesAllocCount);
if (length > 1)
-   mempool_resize(cifs_req_poolp, length + cifs_min_rcv,
-   GFP_KERNEL);
+   mempool_resize(cifs_req_poolp, length + cifs_min_rcv);

set_freezable();
while (server->tcpStatus != CifsExiting) {
diff --git a/include/linux/mempool.h b/include/linux/mempool.h
--- a/include/linux/mempool.h
+++ b/include/linux/mempool.h
@@ -29,7 +29,7 @@ extern mempool_t *mempool_create_node(int min_nr, 
mempool_alloc_t *alloc_fn,
mempool_free_t *free_fn, void *pool_data,
gfp_t gfp_mask, int nid);

-extern int mempool_resize(mempool_t *pool, int new_min_nr, gfp_t gfp_mask);
+extern int mempool_resize(mempool_t *pool, int new_min_nr);
  extern void mempool_destroy(mempool_t *pool);
  extern void * mempool_alloc(mempool_t *pool, gfp_t gfp_mask);
  extern void mempool_free(void *element, mempool_t *pool);
diff --git a/mm/mempool.c b/mm/mempool.c
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -113,23 +113,24 @@ EXPORT_SYMBOL(mempool_create_node);
   *  mempool_create().
   * @new_min_nr: the new minimum number of elements guaranteed to be
   *  allocated for this pool.
- * @gfp_mask:   the usual allocation bitmask.
   *
   * This function shrinks/grows the pool. In the case of growing,
   * it cannot be guaranteed that the pool will be grown to the new
   * size immediately, but new mempool_free() calls will refill it.
+ * This function may sleep.
   *
   * Note, the caller must guarantee that no mempool_destroy is called
   * while this function is running. mempool_alloc() & mempool_free()
   * might be called (eg. from IRQ contexts) while this function executes.
   */
-int mempool_resize(mempool_t *pool, int new_min_nr, gfp_t gfp_mask)
+int mempool_resize(mempool_t *pool, int new_min_nr)
  {
void *element;
void **new_elements;
unsigned long flags;

BUG_ON(new_min_nr <= 0);
+   might_sleep();

spin_lock_irqsave(&pool->lock, flags);
if (new_min_nr <= pool->min_nr) {
@@ -145,7 +146,7 @@ int mempool_resize(mempool_t *pool, int new_min_nr, gfp_t 
gfp_mask)
spin_unlock_irqrestore(&pool->lock, flags);

/* Grow the pool */
-   new_elements = kmalloc(new_min_nr * sizeof(*new_elements), gfp_mask);
+   new_elements = kmalloc(new_min_nr * sizeof(*new_elements), GFP_KERNEL);
if (!new_elements)
return -ENOMEM;

@@ -164,7 +165,7 @@ int mempool_resize(mempool_t *pool, int new_min_nr, gfp_t 
gfp_mask)

while (pool->curr_nr < pool->min_nr) {
spin_unlock_irqrestore(&pool->lock, flags);
-   element = pool->alloc(gfp_mask, pool->pool_data);
+   element = pool->alloc(GFP_KERNEL, pool->pool_data);
if (!element)
     

Re: [PATCH v2 4/8] s390: replace zfcp_qdio_sbale_count by sg_nents

[Steffen Maier]

Thanks, looks good.

I've added it to my queue for sending zfcp patches upstream next time 
(might take a while).


On 09/18/2015 02:57 PM, LABBE Corentin wrote:

The zfcp_qdio_sbale_count function do the same work than sg_nents().
So replace it by sg_nents() for removing duplicate code.

Signed-off-by: LABBE Corentin 
---
  drivers/s390/scsi/zfcp_fsf.c  |  3 +--
  drivers/s390/scsi/zfcp_qdio.h | 15 ---
  2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 522a633..edc137a 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -985,8 +985,7 @@ static int zfcp_fsf_setup_ct_els_sbals(struct zfcp_fsf_req 
*req,
if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_resp))
return -EIO;

-   zfcp_qdio_set_data_div(qdio, &req->qdio_req,
-   zfcp_qdio_sbale_count(sg_req));
+   zfcp_qdio_set_data_div(qdio, &req->qdio_req, sg_nents(sg_req));
zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
zfcp_qdio_set_scount(qdio, &req->qdio_req);
return 0;
diff --git a/drivers/s390/scsi/zfcp_qdio.h b/drivers/s390/scsi/zfcp_qdio.h
index 497cd37..85cdb82 100644
--- a/drivers/s390/scsi/zfcp_qdio.h
+++ b/drivers/s390/scsi/zfcp_qdio.h
@@ -225,21 +225,6 @@ void zfcp_qdio_set_data_div(struct zfcp_qdio *qdio,
  }

  /**
- * zfcp_qdio_sbale_count - count sbale used
- * @sg: pointer to struct scatterlist
- */
-static inline
-unsigned int zfcp_qdio_sbale_count(struct scatterlist *sg)
-{
-   unsigned int count = 0;
-
-   for (; sg; sg = sg_next(sg))
-   count++;
-
-   return count;
-}
-
-/**
   * zfcp_qdio_real_bytes - count bytes used
   * @sg: pointer to struct scatterlist
   */



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()

[Steffen Maier]

On 02/19/2019 08:27 AM, Jason Yan wrote:

If we remove the scsi disk when running io with fio, oops occured with
the following condition.

[scsi_eh_0]  [fio]
scsi_end_request
   ->blk_update_request
 ->end_bio(io returned to userspace)
  close
->sd_release
   ->scsi_disk_put
  ->scsi_disk_release
  ->disk->private_data = 
NULL;

   ->scsi_mq_uninit_cmd
 ->scsi_uninit_cmd
   ->scsi_cmd_to_driver
 ->drv is NULL, Oops

There is a small window between blk_update_request() and
scsi_mq_uninit_cmd() that scsi disk may have been released. This will
cause a oops like below:



To fix this, get a refcount of scsi_disk in sd_init_command() to ensure
it will not be released before sd_uninit_command().

Signed-off-by: Jason Yan 
---
  drivers/scsi/sd.c | 46 +++---
  1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5464d467e23e..6bdb8fbb570f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1249,42 +1249,64 @@ static blk_status_t sd_setup_read_write_cmnd(struct 
scsi_cmnd *SCpnt)
  static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
  {
struct request *rq = cmd->request;
+   struct scsi_disk *sdkp = NULL;


This pre-init with NULL kinda prevents static compile warnings on uninitialized 
use?



+   blk_status_t ret;

switch (req_op(rq)) {



}
+
+   if (!ret) {
+   sdkp = scsi_disk(rq->rq_disk);
+   get_device(&sdkp->dev);
+   }
+
+   return ret;
  }

  static void sd_uninit_command(struct scsi_cmnd *SCpnt)
  {
struct request *rq = SCpnt->request;
u8 *cmnd;
+   struct scsi_disk *sdkp = NULL;


dito



if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
mempool_free(rq->special_vec.bv_page, sd_page_pool);
@@ -1295,6 +1317,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
SCpnt->cmd_len = 0;
mempool_free(cmnd, sd_cdb_pool);
}
+   sdkp = scsi_disk(rq->rq_disk);
+   put_device(&sdkp->dev);
  }

  /**




--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Linux 5.3-rc1

[Steffen Maier]

On 7/23/19 7:28 AM, James Bottomley wrote:

On Mon, 2019-07-22 at 19:42 -0700, Guenter Roeck wrote:

On 7/22/19 4:45 PM, James Bottomley wrote:

[linux-scsi added to cc]
On Mon, 2019-07-22 at 15:21 -0700, Guenter Roeck wrote:

On Sun, Jul 21, 2019 at 02:33:38PM -0700, Linus Torvalds wrote:

[ ... ]


Go test,



Things looked pretty good until a few days ago. Unfortunately,
the last few days brought in a couple of issues.

riscv:virt:defconfig:scsi[virtio]
riscv:virt:defconfig:scsi[virtio-pci]

Boot tests crash with no useful backtrace. Bisect points to
merge ac60602a6d8f ("Merge tag 'dma-mapping-5.3-1'"). Log is at
https://kerneltests.org/builders/qemu-riscv64-master/builds/238/s
teps
/qemubuildcommand_1/logs/stdio

ppc:mpc8544ds:mpc85xx_defconfig:sata-sii3112
ppc64:pseries:pseries_defconfig:sata-sii3112
ppc64:pseries:pseries_defconfig:little:sata-sii3112
ppc64:ppce500:corenet64_smp_defconfig:e5500:sata-sii3112

ata1: lost interrupt (Status 0x50)
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
ata1.00: failed command: READ DMA

and many similar errors. Boot ultimately times out. Bisect points
to
merge
f65420df914a ("Merge tag 'scsi-fixes'").

Logs:
https://kerneltests.org/builders/qemu-ppc64-master/builds/1212/st
eps/
qemubuildcommand/logs/stdio
https://kerneltests.org/builders/qemu-ppc-master/builds/1255/step
s/qe
mubuildcommand/logs/stdio

Guenter

---
riscv bisect log

# bad: [5f9e832c137075045d15cd6899ab0505cfb2ca4b] Linus 5.3-rc1
# good: [bdd17bdef7d8da4d8eee254abb4c92d8a566bdc1] scsi: core:
take
the DMA max mapping size into account



# first bad commit: [ac60602a6d8f6830dee89f4b87ee005f62eb7171]
Merge
tag 'dma-mapping-5.3-1' of git://git.infradead.org/users/hch/dma-
mapping



When a bisect lands on a merge commit it usually indicates bad
interaction between two trees.  The way to find it is to do a
bisect,
but merge up to the other side of the scsi-fixes pull before
running
tests so the interaction is exposed in the bisect.



Can you provide instructions for dummies ?


do a man git-bisect and then follow the 'Automatically bisect with
temporary modifications' example.  You substitute
168c79971b4a7be7011e73bf488b740a8e1135c8 for hot-fix


However my money is on:


commit bdd17bdef7d8da4d8eee254abb4c92d8a566bdc1
Author: Christoph Hellwig 
Date:   Mon Jun 17 14:19:54 2019 +0200

  scsi: core: take the DMA max mapping size into account
   
Now that I look at the code again:



+   shost->max_sectors = min_t(unsigned int, shost-

max_sectors,

+   dma_max_mapping_size(dev) << SECTOR_SHIFT);

That shift looks to be the wrong way around (should be >>).  I bet
something is giving a very large number which becomes zero on left
shift, meaning max_sectors gets set to zero.



That does indeed look bad, but changing it doesn't make a difference.


Odd, all the other changes are driver specific (and not in ATA) apart
from this one:

commit 7ad388d8e4c703980b7018b938cdeec58832d78d
Author: Christoph Hellwig 
Date:   Mon Jun 17 14:19:53 2019 +0200

 scsi: core: add a host / host template field for the virt boundary


I suppose it could be because the virt_boundary_mask isn't set, but
that should just set zero, which is what block usually does.


I found max_segment_size unexpectedly to be UINT_MAX with zfcp today in our CI. 
My investigations are still very early, but I thought, I share a few thoughts 
as I'm way too unfamiliar with the DMA business and thus hope for help.


Above commit introduced an unconditional call to blk_queue_virt_boundary(q, 
shost->virt_boundary_mask), _after_ blk_queue_max_segment_size(q, 
shost->max_segment_size).


Looking at the source, dma_set_max_seg_size() seems to unconditionally 
overwrite max_segment_size:



/**
 * blk_queue_virt_boundary - set boundary rules for bio merging
 * @q:  the request queue for the device
 * @mask:  the memory boundary mask
 **/
void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
{
q->limits.virt_boundary_mask = mask;

/*
 * Devices that require a virtual boundary do not support scatter/gather
 * I/O natively, but instead require a descriptor list entry for each
 * page (which might not be idential to the Linux PAGE_SIZE).  Because
 * of that they are not limited by our notion of "segment size".
 */
q->limits.max_segment_size = UINT_MAX;
}
EXPORT_SYMBOL(blk_queue_virt_boundary);


Wild guess: Do we need to make the call to blk_queue_virt_boundary() 
conditional?

Cf. https://www.spinics.net/lists/linux-scsi/msg131077.html ("[PATCH v2] iser: 
explicitly set shost max_segment_size if non virtual boundary devices")


--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitz

Re: Linux 5.3-rc1

[Steffen Maier]

On 7/23/19 6:19 PM, Guenter Roeck wrote:

On Tue, Jul 23, 2019 at 08:33:15AM -0700, James Bottomley wrote:
[ ... ]


Yes, I think so.  Can someone try this, or something like it.

Thanks,

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9381171c2fc0..4715671a1537 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1793,7 +1793,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct 
request_queue *q)
dma_set_seg_boundary(dev, shost->dma_boundary);
  
  	blk_queue_max_segment_size(q, shost->max_segment_size);

-   blk_queue_virt_boundary(q, shost->virt_boundary_mask);
+   if (shost->virt_boundary_mask)
+   blk_queue_virt_boundary(q, shost->virt_boundary_mask);
dma_set_max_seg_size(dev, queue_max_segment_size(q));
  
  	/*


This fixes the problem for me.


+1
(on a first glimpse with zfcp)


--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[PATCH 1/3] docs: s390: restore important non-kdoc parts of s390dbf.rst

[Steffen Maier]

Complements previous ("s390: include/asm/debug.h add kerneldoc markups")
which seemed to have dropped important non-kdoc parts such as
user space interface (level, size, flush)
as well as views and caution regarding strings in the sprintf view.

Signed-off-by: Steffen Maier 
---
 Documentation/s390/s390dbf.rst | 339 +
 1 file changed, 339 insertions(+)

diff --git a/Documentation/s390/s390dbf.rst b/Documentation/s390/s390dbf.rst
index d2595b548879..01d66251643d 100644
--- a/Documentation/s390/s390dbf.rst
+++ b/Documentation/s390/s390dbf.rst
@@ -112,6 +112,345 @@ Kernel Interfaces:
 Predefined views:
 -
 
+extern struct debug_view debug_hex_ascii_view;
+
+extern struct debug_view debug_raw_view;
+
+extern struct debug_view debug_sprintf_view;
+
+Examples
+
+
+::
+
+  /*
+   * hex_ascii- + raw-view Example
+   */
+
+  #include 
+  #include 
+
+  static debug_info_t* debug_info;
+
+  static int init(void)
+  {
+  /* register 4 debug areas with one page each and 4 byte data field */
+
+  debug_info = debug_register ("test", 1, 4, 4 );
+  debug_register_view(debug_info,&debug_hex_ascii_view);
+  debug_register_view(debug_info,&debug_raw_view);
+
+  debug_text_event(debug_info, 4 , "one ");
+  debug_int_exception(debug_info, 4, 4711);
+  debug_event(debug_info, 3, &debug_info, 4);
+
+  return 0;
+  }
+
+  static void cleanup(void)
+  {
+  debug_unregister (debug_info);
+  }
+
+  module_init(init);
+  module_exit(cleanup);
+
+::
+
+  /*
+   * sprintf-view Example
+   */
+
+  #include 
+  #include 
+
+  static debug_info_t* debug_info;
+
+  static int init(void)
+  {
+  /* register 4 debug areas with one page each and data field for */
+  /* format string pointer + 2 varargs (= 3 * sizeof(long))   */
+
+  debug_info = debug_register ("test", 1, 4, sizeof(long) * 3);
+  debug_register_view(debug_info,&debug_sprintf_view);
+
+  debug_sprintf_event(debug_info, 2 , "first event in 
%s:%i\n",__FILE__,__LINE__);
+  debug_sprintf_exception(debug_info, 1, "pointer to debug info: 
%p\n",&debug_info);
+
+  return 0;
+  }
+
+  static void cleanup(void)
+  {
+  debug_unregister (debug_info);
+  }
+
+  module_init(init);
+  module_exit(cleanup);
+
+Debugfs Interface
+-
+Views to the debug logs can be investigated through reading the corresponding
+debugfs-files:
+
+Example::
+
+  > ls /sys/kernel/debug/s390dbf/dasd
+  flush  hex_ascii  level pages raw
+  > cat /sys/kernel/debug/s390dbf/dasd/hex_ascii | sort -k2,2 -s
+  00 00974733272:680099 2 - 02 0006ad7e  07 ea 4a 90 | 
+  00 00974733272:682210 2 - 02 0006ade6  46 52 45 45 | FREE
+  00 00974733272:682213 2 - 02 0006adf6  07 ea 4a 90 | 
+  00 00974733272:682281 1 * 02 0006ab08  41 4c 4c 43 | EXCP
+  01 00974733272:682284 2 - 02 0006ab16  45 43 4b 44 | ECKD
+  01 00974733272:682287 2 - 02 0006ab28  00 00 00 04 | 
+  01 00974733272:682289 2 - 02 0006ab3e  00 00 00 20 | ...
+  01 00974733272:682297 2 - 02 0006ad7e  07 ea 4a 90 | 
+  01 00974733272:684384 2 - 00 0006ade6  46 52 45 45 | FREE
+  01 00974733272:684388 2 - 00 0006adf6  07 ea 4a 90 | 
+
+See section about predefined views for explanation of the above output!
+
+Changing the debug level
+
+
+Example::
+
+
+  > cat /sys/kernel/debug/s390dbf/dasd/level
+  3
+  > echo "5" > /sys/kernel/debug/s390dbf/dasd/level
+  > cat /sys/kernel/debug/s390dbf/dasd/level
+  5
+
+Flushing debug areas
+
+Debug areas can be flushed with piping the number of the desired
+area (0...n) to the debugfs file "flush". When using "-" all debug areas
+are flushed.
+
+Examples:
+
+1. Flush debug area 0::
+
+ > echo "0" > /sys/kernel/debug/s390dbf/dasd/flush
+
+2. Flush all debug areas::
+
+ > echo "-" > /sys/kernel/debug/s390dbf/dasd/flush
+
+Changing the size of debug areas
+
+It is possible the change the size of debug areas through piping
+the number of pages to the debugfs file "pages". The resize request will
+also flush the debug areas.
+
+Example:
+
+Define 4 pages for the debug areas of debug feature "dasd"::
+
+  > echo "4" > /sys/kernel/debug/s390dbf/dasd/pages
+
+Stooping the debug feature
+--
+Example:
+
+1. Check if stopping is allowed::
+
+ > cat /proc/sys/s390dbf/debug_stoppable
+
+2. Stop debug feature::
+
+ > echo 0 > /proc/sys/s390dbf/debug_active
+
+lcrash Interface
+
+It is planned that the dump analysis tool lcrash gets an additional command
+'s390dbf' to display all the debug logs. With this tool it will be possible
+to investigate the debug logs on a live system and with a memory dump after
+a system crash.
+
+Inv

Re: [RESEND PATCH v1 2/2] trace: events: block: Add tag in block trace events

[Steffen Maier]

On 04/16/2018 04:33 PM, Bean Huo (beanhuo) wrote:

Print the request tag along with other information in block trace events
when tracing request , and unplug type (Sync / Async).

Signed-off-by: Bean Huo 
---
  include/trace/events/block.h | 36 +---
  1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 81b43f5..f8c0b9e 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h



@@ -478,15 +486,18 @@ DECLARE_EVENT_CLASS(block_unplug,

TP_STRUCT__entry(
__field( int,   nr_rq   )
+   __field( bool,  explicit)
__array( char,  comm,   TASK_COMM_LEN   )
),

TP_fast_assign(
__entry->nr_rq = depth;
+   __entry->explicit = explicit;
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),

-   TP_printk("[%s] %d", __entry->comm, __entry->nr_rq)
+   TP_printk("[%s] %d %s", __entry->comm, __entry->nr_rq,
+  __entry->explicit ? "Sync" : "Async")
  );

  /**


This entire hunk does not seem related to this patch description.
Also, I'm not sure trace-cmd and perf et al. could format it accordingly.
See also my patch for this same functionality:
https://www.spinics.net/lists/linux-block/msg24691.html
("[PATCH v2 1/2] tracing/events: block: track and print if unplug was 
explicit or schedule")




--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Steffen Maier]

On 04/17/2018 12:00 PM, Bean Huo (beanhuo) wrote:


#Cat trace
iozone-4055  [000]    665.039276: block_unplug: [iozone] 1 Sync
iozone-4055  [000] ...1   665.039278: block_rq_insert: 8,48 WS 0 () 39604352 + 
128 tag=18 [iozone]
iozone-4055  [000] ...1   665.039280: block_rq_issue: 8,48 WS 0 () 39604352 + 
128 tag=18 [iozone]
iozone-4055  [000] ...1   665.039284: scsi_dispatch_cmd_start: host_no=0 
channel=0 id=0 lun=3 data_sgl=16 prot_sgl=0 prot_op=SCSI_PROT_NORMAL tag=18 
cmnd=(WRITE_10 lba=4950544 txlen=16 protect=0 raw=2a 00 00 4b 8a 10 00 00 10 00)
iozone-4056  [002]    665.039284: block_dirty_buffer: 8,62 sector=44375 
size=4096
-0 [000] d.h2   665.039319: scsi_dispatch_cmd_done: host_no=0 
channel=0 id=0 lun=3 data_sgl=16 prot_sgl=0 prot_op=SCSI_PROT_NORMAL tag=24 
cmnd=(WRITE_10 lba=4944016 txlen=16 protect=0 raw=2a 00 00 4b 70 90 00 00 10 00) 
result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)
-0 [000] d.h3   665.039321: block_rq_complete: 8,48 WS () 39552128 + 
128 tag=24 [0]



iozone-4058  [003]    665.039362: block_bio_remap: 8,48 WS 39568768 + 128 
<- (8,62) 337280
iozone-4058  [003]    665.039364: block_bio_queue: 8,48 WS 39568768 + 128 
[iozone]
iozone-4058  [003] ...1   665.039366: block_getrq: 8,48 WS 39568768 + 128 
[iozone]


I'm not familiar with block/scsi command tagging.

Some block events now would get a tag field.
Some block events would not get a tag field (maybe because for some the 
tag is not (yet) known).


So all block events that belong to the same request would still need to 
be correlated by something like (devt, RWBS, LBA, length) because not 
all have a tag field.



Especially, the ftrace log with tag information, I can easily figure out one 
I/O request between block layer and SCSI.


Provided this is done correctly, I would be in favor of a solution.
Since
v4.11 commit 48b77ad60844 (``block: cleanup tracing'')\newline
v4.11 commit 82ed4db499b8 (``block: split scsi\_request out of struct 
request'')
we don't have the SCSI CDB in block traces (nor in blktrace traditional 
relayfs trace format, nor in ftrace 'blk' tracer binary synthesized 
output [1]) any more (empty Packet Command payload).
Being able to correlate block events with scsi events would indeed be 
very helpful for some cases.


Is a correlation between block and scsi only necessary for these pairs?:

block_rq_issue causes scsi_dispatch_cmd_start, and
scsi_dispatch_cmd_done causes block_rq_complete.

If so, only those two block trace events would need to get a new field?


[1] v2.6.30 commit 08a06b83ff8b (``blkftrace: binary tracing, 
synthesizing old format'')
v2.6.31 commit f3948f8857ef (``blktrace: fix context-info when 
mixed-using blk tracer and trace events'')


--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 2/2] tracing/events: block: dev_t via driver core for plug and unplug events

[Steffen Maier]

obj) NULLifying q->kobj.parent anyway...
I guess I came full circle.



Side notes:
The existing code holds a reference on gendisk, but between (2) and (4).

Apparently block trace events can easily observe parts such as (1a)
which are difficult for traditional blktrace to be started via blk dev ioctl
(or for ftrace 'blk' tracer which can be enabled between (2) and (4)).



Independently, I started to wonder if there is a refcount leak in the early 
error out if kobject_add() failed:

> int blk_register_queue(struct gendisk *disk)
> struct device *dev = disk_to_dev(disk);
> struct request_queue *q = disk->queue;
> ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
> ^^^ref+1
>   if (ret < 0) {
>   blk_trace_remove_sysfs(dev);

Above kobject_get already did its side-effect to provide the second argument 
when calling kobject_add. Even if kobject_add failed, we still have the 
refcount on the gendisk but don't put it?

>   goto unlock;
>   }

>   if (q->request_fn || (q->mq_ops && q->elevator)) {
>   ret = elv_register_queue(q);
>   if (ret) {
>   mutex_unlock(&q->sysfs_lock);
>   kobject_uevent(&q->kobj, KOBJ_REMOVE);
>   kobject_del(&q->kobj);
>   blk_trace_remove_sysfs(dev);
>   kobject_put(&dev->kobj);

In contrast, this early return seems to do the pairwise put.

>   return ret;
>   }
>   }

> unlock:
>   mutex_unlock(&q->sysfs_lock);
>   return ret;
> }



-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC v2] tracing/events: block: also try to get dev_t via driver core for some events

[Steffen Maier]

Complements v2.6.31 commit 55782138e47d ("tracing/events: convert block
trace points to TRACE_EVENT()") for cases where rq->rq_disk == NULL:
  block_rq_requeue, block_rq_insert, block_rq_issue;
and for cases where bio == NULL:
  block_getrq, block_sleeprq.

NB: The NULL pointer check for q->kobj.parent is certainly racy and
I don't have enough experience if it's good enough for a trace event.

Since I don't know when above cases would occur,
I'm not sure this is worth it.

Signed-off-by: Steffen Maier 
---
 include/trace/events/block.h | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 6ea5a3899c2e..001e4f369df9 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -86,7 +86,9 @@ TRACE_EVENT(block_rq_requeue,
),
 
TP_fast_assign(
-   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) : 0;
+   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) :
+   (q->kobj.parent ?
+container_of(q->kobj.parent, struct device, kobj)->devt : 0);
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
 
@@ -162,7 +164,9 @@ DECLARE_EVENT_CLASS(block_rq,
),
 
TP_fast_assign(
-   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) : 0;
+   __entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) :
+   (q->kobj.parent ?
+container_of(q->kobj.parent, struct device, kobj)->devt : 0);
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
__entry->bytes = blk_rq_bytes(rq);
@@ -397,7 +401,9 @@ DECLARE_EVENT_CLASS(block_get_rq,
 ),
 
TP_fast_assign(
-   __entry->dev= bio ? bio_dev(bio) : 0;
+   __entry->dev= bio ? bio_dev(bio) :
+   (q->kobj.parent ?
+container_of(q->kobj.parent, struct device, kobj)->devt : 0);
__entry->sector = bio ? bio->bi_iter.bi_sector : 0;
__entry->nr_sector  = bio ? bio_sectors(bio) : 0;
blk_fill_rwbs(__entry->rwbs,
-- 
2.13.5

[PATCH v2 1/2] tracing/events: block: track and print if unplug was explicit or schedule

[Steffen Maier]

Just like blktrace distinguishes explicit and schedule by means of
BLK_TA_UNPLUG_IO and BLK_TA_UNPLUG_TIMER, actually make use of the
existing argument "explicit" to distinguish the two cases in the one
common tracepoint block_unplug.

Complements v2.6.39 commit 49cac01e1fa7 ("block: make unplug timer trace
event correspond to the schedule() unplug") and commit d9c978331790
("block: remove block_unplug_timer() trace point").

Signed-off-by: Steffen Maier 
---

Changes since v1:
Use 0 and 1 instead of false and true for __print_symbolic table.
Now "trace-cmd report" can decode it. [Steven Rostedt]

 include/trace/events/block.h | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 81b43f5bdf23..e90bb6eb8097 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -470,6 +470,11 @@ TRACE_EVENT(block_plug,
TP_printk("[%s]", __entry->comm)
 );
 
+#define show_block_unplug_explicit(val)\
+   __print_symbolic(val,   \
+{0, "schedule"},   \
+{1, "explicit"})
+
 DECLARE_EVENT_CLASS(block_unplug,
 
TP_PROTO(struct request_queue *q, unsigned int depth, bool explicit),
@@ -478,15 +483,18 @@ DECLARE_EVENT_CLASS(block_unplug,
 
TP_STRUCT__entry(
__field( int,   nr_rq   )
+   __field( bool,  explicit)
__array( char,  comm,   TASK_COMM_LEN   )
),
 
TP_fast_assign(
__entry->nr_rq = depth;
+   __entry->explicit = explicit;
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
 
-   TP_printk("[%s] %d", __entry->comm, __entry->nr_rq)
+   TP_printk("[%s] %d %s", __entry->comm, __entry->nr_rq,
+ show_block_unplug_explicit(__entry->explicit))
 );
 
 /**
-- 
2.13.5

[PATCH v2 0/2] tracing/events: block: bring more on a par with blktrace

[Steffen Maier]

I had the need to understand I/O request processing in detail.
But I also had the need to enrich block traces with other trace events
including my own dynamic kprobe events. So I preferred block trace events
over blktrace to get everything nicely sorted into one ftrace output.
However, I missed device filtering for (un)plug events and also
the difference between the two flavors of unplug.

The first two patches bring block trace events closer to their
counterpart in blktrace tooling.

The last patch is just an RFC. I still kept it in this patch set because
it is inspired by PATCH 2/2.

Changes since v1:
[PATCH v2 1/2]
Use 0 and 1 instead of false and true for __print_symbolic table.
Now "trace-cmd report" can decode it. [Steven Rostedt]

Steffen Maier (3):
  tracing/events: block: track and print if unplug was explicit or
schedule
  tracing/events: block: dev_t via driver core for plug and unplug
events
  tracing/events: block: also try to get dev_t via driver core for some
events

 include/trace/events/block.h | 33 -
 1 file changed, 28 insertions(+), 5 deletions(-)

-- 
2.13.5

[PATCH v2 2/2] tracing/events: block: dev_t via driver core for plug and unplug events

[Steffen Maier]

Complements v2.6.31 commit 55782138e47d ("tracing/events: convert block
trace points to TRACE_EVENT()") to be equivalent to traditional blktrace
output. Also this allows event filtering to not always get all (un)plug
events.

NB: The NULL pointer check for q->kobj.parent is certainly racy and
I don't have enough experience if it's good enough for a trace event.
The change did work for my cases (block device read/write I/O on
zfcp-attached SCSI disks and dm-mpath on top).

While I haven't seen any prior art using driver core (parent) relations
for trace events, there are other cases using this when no direct pointer
exists between objects, such as:
 #define to_scsi_target(d)  container_of(d, struct scsi_target, dev)
 static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 {
return to_scsi_target(sdev->sdev_gendev.parent);
 }

This is the object model we make use of here:

struct gendisk {
struct hd_struct {
struct device {  /*container_of*/
struct kobject kobj; <--+
dev_t  devt; /*deref*/  |
} __dev;|
} part0;|
struct request_queue *queue; ..+|
}  :|
   :|
struct request_queue {  <..+|
/* queue kobject */ |
struct kobject {|
struct kobject *parent; +
} kobj;
}

The parent pointer comes from:
 #define disk_to_dev(disk)  (&(disk)->part0.__dev)
int blk_register_queue(struct gendisk *disk)
struct device *dev = disk_to_dev(disk);
struct request_queue *q = disk->queue;
ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
^^^parent

$ ls -d /sys/block/sdf/queue
/sys/block/sda/queue
$ cat /sys/block/sdf/dev
80:0

A partition does not have its own request queue:

$ cat /sys/block/sdf/sdf1/dev
8:81
$ ls -d /sys/block/sdf/sdf1/queue
ls: cannot access '/sys/block/sdf/sdf1/queue': No such file or directory

The difference to blktrace parsed output is that block events don't use the
partition's minor number but the containing block device's minor number:

$ dd if=/dev/sdf1 count=1

$ cat /sys/kernel/debug/tracing/trace
block_bio_remap: 8,80 R 2048 + 32 <- (8,81) 0
block_bio_queue: 8,80 R 2048 + 32 [dd]
block_getrq: 8,80 R 2048 + 32 [dd]
block_plug: 8,80 [dd]

block_rq_insert: 8,80 R 16384 () 2048 + 32 [dd]
block_unplug: 8,80 [dd] 1 explicit
  
block_rq_issue: 8,80 R 16384 () 2048 + 32 [dd]
block_rq_complete: 8,80 R () 2048 + 32 [0]

$ btrace /dev/sdf1
  8,80   11 0.0 240240  A   R 2048 + 32 <- (8,81) 0
  8,81   12 0.000220890 240240  Q   R 2048 + 32 [dd]
  8,81   13 0.000229639 240240  G   R 2048 + 32 [dd]
  8,81   14 0.000231805 240240  P   N [dd]
^^
  8,81   15 0.000234671 240240  I   R 2048 + 32 [dd]
  8,81   16 0.000236365 240240  U   N [dd] 1
^^
  8,81   17 0.000238527 240240  D   R 2048 + 32 [dd]
  8,81   22 0.000613741 0  C   R 2048 + 32 [0]

Signed-off-by: Steffen Maier 
---
 include/trace/events/block.h | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index e90bb6eb8097..6ea5a3899c2e 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -460,14 +460,18 @@ TRACE_EVENT(block_plug,
TP_ARGS(q),
 
TP_STRUCT__entry(
+   __field( dev_t, dev )
__array( char,  comm,   TASK_COMM_LEN   )
),
 
TP_fast_assign(
+   __entry->dev = q->kobj.parent ?
+   container_of(q->kobj.parent, struct device, kobj)->devt : 0;
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
 
-   TP_printk("[%s]", __entry->comm)
+   TP_printk("%d,%d [%s]",
+ MAJOR(__entry->dev), MINOR(__entry->dev), __entry->comm)
 );
 
 #define show_block_unplug_explicit(val)\
@@ -482,18 +486,23 @@ DECLARE_EVENT_CLASS(block_unplug,
TP_ARGS(q, depth, explicit),
 
TP_STRUCT__entry(
+   __field( dev_t, dev )
__field( int,   nr_rq   )
__field( bool,  explicit)
__array( char,  comm,   TASK_COMM_LEN   )
),
 
TP_fast_assign(
+   __entry->dev   = q->kobj.parent ?
+   container_of(q->kobj.parent

Re: [PATCH 2/2] tracing/events: block: dev_t via driver core for plug and unplug events

[Steffen Maier]

Hi Greg,

On 04/15/2018 10:31 AM, Greg Kroah-Hartman wrote:

On Fri, Apr 13, 2018 at 03:07:18PM +0200, Steffen Maier wrote:

Complements v2.6.31 commit 55782138e47d ("tracing/events: convert block
trace points to TRACE_EVENT()") to be equivalent to traditional blktrace
output. Also this allows event filtering to not always get all (un)plug
events.

NB: The NULL pointer check for q->kobj.parent is certainly racy and
I don't have enough experience if it's good enough for a trace event.
The change did work for my cases (block device read/write I/O on
zfcp-attached SCSI disks and dm-mpath on top).

While I haven't seen any prior art using driver core (parent) relations
for trace events, there are other cases using this when no direct pointer
exists between objects, such as:
  #define to_scsi_target(d) container_of(d, struct scsi_target, dev)
  static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
  {
return to_scsi_target(sdev->sdev_gendev.parent);
  }


That is because you "know" the parent of a target device is a
scsi_target.


true


This is the object model we make use of here:

struct gendisk {
 struct hd_struct {
 struct device {  /*container_of*/
 struct kobject kobj; <--+
 dev_t  devt; /*deref*/  |
 } __dev;|
 } part0;|
 struct request_queue *queue; ..+|
}  :|
:|
struct request_queue {  <..+|
 /* queue kobject */ |
 struct kobject {|
 struct kobject *parent; +


Are you sure about this?


I double checked it with crash on a running system chasing pointers and 
looking at structure debug symbols.

But of course I cannot guarantee it's always been like this and will be.


 } kobj;
}




The difference to blktrace parsed output is that block events don't use the
partition's minor number but the containing block device's minor number:


Why do you want the block device's minor number here?  What is wrong
with the partition's minor number?  I would think you want that instead.


No change introduced with my patch. I just describe state of the art 
since the mentioned 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=55782138e47d.
It (or even its predecessor) used request_queue as trace function 
argument (plus mostly either request or bio). So that's the currently 
available context for these events. My change is consistent with that.
But then again, it's not much of a problem as we do have the remap event 
which shows the mapping from partition to blockdev.


blktrace, hooking with callbacks on the block trace events, has its own 
context information [struct blk_trace] and can get to e.g. the dev_t 
with its own real pointers without using driver core relations. But I 
had the impression that's only wired if one uses the blktrace IOCTL or 
the blk tracer [do_blk_trace_setup()], not for "pure" block events.

static void blk_add_trace_plug(void *ignore, struct request_queue *q)
{
struct blk_trace *bt = q->blk_trace;

   


if (bt)
__blk_add_trace(bt, 0, 0, 0, 0, BLK_TA_PLUG, 0, 0, NULL, NULL);
}

static void blk_add_trace_unplug(void *ignore, struct request_queue *q,
unsigned int depth, bool explicit)
{
struct blk_trace *bt = q->blk_trace;

   


if (bt) {
__be64 rpdu = cpu_to_be64(depth);
u32 what;

if (explicit)
what = BLK_TA_UNPLUG_IO;
else
what = BLK_TA_UNPLUG_TIMER;

__blk_add_trace(bt, 0, 0, 0, 0, what, 0, sizeof(rpdu), &rpdu, 
NULL);
}
}



struct blk_trace {
int trace_state;
struct rchan *rchan;
unsigned long __percpu *sequence;
unsigned char __percpu *msg_data;
u16 act_mask;
u64 start_lba;
u64 end_lba;
u32 pid;
u32 dev;

^^^

struct dentry *dir;
struct dentry *dropped_file;
struct dentry *msg_file;
struct list_head running_list;
atomic_t dropped;
};





$ dd if=/dev/sdf1 count=1

$ cat /sys/kernel/debug/tracing/trace
block_bio_remap: 8,80 R 2048 + 32 <- (8,81) 0
block_bio_queue: 8,80 R 2048 + 32 [dd]
block_getrq: 8,80 R 2048 + 32 [dd]
block_plug: 8,80 [dd]
 
block_rq_insert: 8,80 R 16384 () 2048 + 32 [dd]
block_unplug: 8,80 [dd] 1 explicit
   
block_rq_issue: 8,80 R 16384 () 2048 + 32 [dd]
block_rq_

Re: [PATCH 0/3] scsi: arcmsr: Add driver parameter cmd_timeout for scsi command timeout setting

[Steffen Maier]

On 05/08/2018 08:43 AM, Ching Huang wrote:

On Tue, 2018-05-08 at 14:32 +0800, Ching Huang wrote:

On Tue, 2018-05-08 at 01:41 -0400, Martin K. Petersen wrote:

Hello Ching,


1. Add driver parameter cmd_timeout, default value is ARCMSR_DEFAULT_TIMEOUT.
2. Add slave_configure callback function to set device command timeout value.
3. Update driver version to v1.40.00.06-20180504.


I am not so keen on arcmsr overriding the timeout set by the admin or
application.

Also, instead of introducing this module parameter, why not simply ask
the user to change rq_timeout?


This timeout setting only after device has been inquiry successfully.
Of course, user can set timeout value to /sys/block/sdX/device/timeout.
But user does not like to set this value once command timeout occurred.
They rather like timeout never happen.


This timeout setting apply to all devices, its better than user has to
set one bye one for each device.


Udev rules?

Something roughly like bottom of:
https://www.ibm.com/support/knowledgecenter/ST3FR7_8.1.2/com.ibm.storwize.v7000.812.doc/svc_linux_settings.html
or better doing the assignment with udev builtins (fix the syntax error 
with model):

https://www.ibm.com/support/knowledgecenter/ST3FR7_8.1.2/com.ibm.storwize.v7000.812.doc/svc_zs_statechange_3fgeri.html

--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:

On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:

I'm puzzled.

$ git bisect start fc_bsg master



3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
commit 3087864ce3d7282f59021245d8a5f83ef1caef18
Author: Johannes Thumshirn 
Date:   Wed Oct 12 15:06:28 2016 +0200

scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
helper variables bsg_request and bsg_reply. This will be helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 

:04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2 
0d9fe225615679550be91fbd9f84c09ab1e280fc M  drivers


From there (on the reverse bisect path) I get the following Oops,
except for the full patch set having another stack trace as in my previous
mail (dying in zfcp code).



[...]




@@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue *q, struct 
Scsi_Host *shost,
struct request *req;
struct fc_bsg_job *job;
enum fc_dispatch_result ret;
+   struct fc_bsg_reply *bsg_reply;

if (!get_device(dev))
return;
@@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue *q, struct 
Scsi_Host *shost,
/* check if we have the msgcode value at least */
if (job->request_len < sizeof(uint32_t)) {
BUG_ON(job->reply_len < sizeof(uint32_t));
-   job->reply->reply_payload_rcv_len = 0;
-   job->reply->result = -ENOMSG;
+   bsg_reply = job->reply;
+   bsg_reply->reply_payload_rcv_len = 0;
+   bsg_reply->result = -ENOMSG;


Compiler optimization re-ordered above two lines and the first pointer 
derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
The assignment tries to write to memory at address NULL causing the 
kernel page fault.


Does your suggested change for [PATCH v3 02/16], shuffling the 
job->request_len checks, address above kernel page fault?



job->reply_len = sizeof(uint32_t);
fc_bsg_jobdone(job);
spin_lock_irq(q->queue_lock);



Ahm and what exactly can break here? It's just assigning variables. Now
I'm puzzled too.



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

On 10/28/2016 01:31 PM, Hannes Reinecke wrote:

On 10/28/2016 11:53 AM, Steffen Maier wrote:

On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:

On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:

I'm puzzled.

$ git bisect start fc_bsg master



3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
commit 3087864ce3d7282f59021245d8a5f83ef1caef18
Author: Johannes Thumshirn 
Date:   Wed Oct 12 15:06:28 2016 +0200

scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

Don't use fc_bsg_job::request and fc_bsg_job::reply directly,
but use
helper variables bsg_request and bsg_reply. This will be
helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 

:04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2
0d9fe225615679550be91fbd9f84c09ab1e280fc Mdrivers


From there (on the reverse bisect path) I get the following Oops,
except for the full patch set having another stack trace as in my
previous
mail (dying in zfcp code).


[...]


@@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue
*q, struct Scsi_Host *shost,
 struct request *req;
 struct fc_bsg_job *job;
 enum fc_dispatch_result ret;
+struct fc_bsg_reply *bsg_reply;

 if (!get_device(dev))
 return;
@@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue
*q, struct Scsi_Host *shost,
 /* check if we have the msgcode value at least */
 if (job->request_len < sizeof(uint32_t)) {
 BUG_ON(job->reply_len < sizeof(uint32_t));
-job->reply->reply_payload_rcv_len = 0;
-job->reply->result = -ENOMSG;
+bsg_reply = job->reply;
+bsg_reply->reply_payload_rcv_len = 0;
+bsg_reply->result = -ENOMSG;


Compiler optimization re-ordered above two lines and the first pointer
derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
The assignment tries to write to memory at address NULL causing the
kernel page fault.


I spoke to our compiler people, and they strongly believed this not to
be the case. Or, put it the other way round, if such a thing would
happen it would be a compiler issue.

Have you checked the compiler output?


I just mentioned the compiler optimization to explain why the assembler 
code visible in the panic dies at bsg_reply->result = -ENOMSG and not at 
bsg_reply->reply_payload_rcv_len = 0. I don't think it makes a 
difference regarding the issue, which remains a NULL pointer dereference 
with bsg_reply either way, which I doubt is caused by compiler output. 
But then again, see further down below.



[   46.942560] Krnl PSW : 0704e0018000 007c91ec[   46.942574]  
(fc_bsg_request_handler+0x404/0x4b0)
[   46.942579]
[   46.942583]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:000:
[   46.942598]  RI:0 EA:3
[   46.942601]
[   46.942601] Krnl GPRS:  ffcb  
8001
[   46.942603]007c8fe8 64398c68 69f967e8 
6a3d8008
[   46.942605]6a5e02c8 698b5490  




   %r11 is NULL


[   46.942607]6a9ef5f8 00a36840 007c8fe8 
5d2efa00
[   46.942619] Krnl Code: 007c91de: e55dc08c0003clfhsi  
140(%r12),3[   46.942622]
[   46.942622]007c91e4: a7240004brc 2,7c91ec
 #007c91e8: a7f40001   brc 
15,7c91ea[   46.942629]
[   46.942629]   >007c91ec: 5010b000st  
%r1,0(%r11)
  007c91f0: e54cb004   mvhi
4(%r11),0[   46.942635]
[   46.942635]007c91f6: e54cc08c0004mvhi
140(%r12),4
  007c91fc: b904002c   lgr 
%r2,%r12[   46.942643]
[   46.942643]007c9200: c0e5e2c0brasl   
%r14,7c5780
[   46.942646]
[   46.942647] Call Trace:
[   46.942650] ([<007c8fe8>] fc_bsg_request_handler+0x200/0x4b0)
[   46.942656] ([<006b8e0a>] __blk_run_queue+0x52/0x68)
[   46.942661] ([<006c549a>] blk_execute_rq_nowait+0xf2/0x110)
[   46.942664] ([<006c557a>] blk_execute_rq+0xa2/0x110)
[   46.942668] ([<006de0ee>] bsg_ioctl+0x1f6/0x268)
[   46.942675] ([<0036ca20>] do_vfs_ioctl+0x680/0x6d8)
[   46.942677] ([<0036caf4>] SyS_ioctl+0x7c/0xb0)
[   46.942685] ([<009a541e>] system_call+0xd6/0x270)
[   46.942687] INFO: lockdep is turned off.
[   46.942688] Last Breaking-Event-Address:
[   46.942692]  [<007c91e4>] fc_bsg_request_handler+0x3fc/0x4b0
[   46.942696]  [   46.942698] Kernel panic - not syncing: Fatal exception: 
panic_on_oops


all the following was written from bottom to top:


cra

Re: [PATCH v2 00/16] Convert FibreChannel bsg code to use bsg-lib

[Steffen Maier]

al. All they do is change from
'struct fc_bsg_job' to 'struct bsg_job' and corresponding changes in order
to get the series bisectable.

The idea for this change arose when discussing racy sysfs handling the FC
bsg code with Christoph and is a next step in moving all bsg clients to
bsg-lib to eventually clean up the in kernel bsg API.

Changes to v1:
* Reduce the number of individual patches (44 -> 16)


nice


* Fix s390 build failure (forgotten to kill fc_bsg_job from zfcp_ext.h)


I pushed your patches on today's linux.git, i.e. post v4.8 with zfcp 
fixes of v4.9 merge window already included and it did build with our 
default_defconfig but qdio and zfcp as modules rather than built-in.



* Make bsg_job_get() call kref_get_unless_zero() and use it in 
scsi_transport_fc.c


Perfect, I had planned to suggest this based on v1 of the patch set.


Johannes Thumshirn (16):
  scsi: Get rid of struct fc_bsg_buffer
  scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly
  scsi: fc: Export fc_bsg_jobdone and use it in FC drivers
  scsi: Unify interfaces of fc_bsg_jobdone and bsg_job_done
  scsi: fc: provide fc_bsg_to_shost() helper
  scsi: fc: provide fc_bsg_to_rport() helper
  scsi: libfc: don't set FC_RQST_STATE_DONE before calling
fc_bsg_jobdone()
  scsi: fc: implement kref backed reference counting
  block: add reference counting for struct bsg_job
  scsi: change FC drivers to use 'struct bsg_job'
  scsi: fc: Use bsg_destroy_job
  scsi: fc: use bsg_softirq_done
  scsi: fc: use bsg_job_done
  block: add bsg_job_put() and bsg_job_get()
  scsi: fc: move FC transport's bsg code to bsg-lib
  block: unexport bsg_softirq_done() again

 block/bsg-lib.c  |  19 +-
 drivers/s390/scsi/zfcp_ext.h |   4 +-
 drivers/s390/scsi/zfcp_fc.c  |  33 +--
 drivers/scsi/Kconfig |   1 +
 drivers/scsi/bfa/bfad_bsg.c  |  62 +++---
 drivers/scsi/bfa/bfad_im.h   |   4 +-
 drivers/scsi/ibmvscsi/ibmvfc.c   |  40 ++--
 drivers/scsi/libfc/fc_lport.c|  47 ++--
 drivers/scsi/lpfc/lpfc_bsg.c | 375 +++-
 drivers/scsi/lpfc/lpfc_crtn.h|   4 +-
 drivers/scsi/qla2xxx/qla_bsg.c   | 449 ++-
 drivers/scsi/qla2xxx/qla_def.h   |   2 +-
 drivers/scsi/qla2xxx/qla_gbl.h   |   4 +-
 drivers/scsi/qla2xxx/qla_iocb.c  |  13 +-
 drivers/scsi/qla2xxx/qla_isr.c   |  52 +++--
 drivers/scsi/qla2xxx/qla_mr.c|  15 +-
 drivers/scsi/scsi_transport_fc.c | 409 ++-
 include/linux/bsg-lib.h  |   4 +
 include/scsi/libfc.h |   2 +-
 include/scsi/scsi_transport_fc.h |  62 ++
 20 files changed, 745 insertions(+), 856 deletions(-)



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [patch] zfcp: spin_lock_irqsave() is not nestable

[Steffen Maier]

Dan, many thanks for catching this! Sparse did not notice, is there 
other tooling that would find such things?


James, Martin, could you please queue this as fix for one of my patches 
that went into the 4.9 merge window, so for 4.9-rc I guess?

https://lkml.kernel.org/r/20161013085358.GH16198@mwanda
or
https://lkml.org/lkml/2016/10/13/94

On 10/13/2016 10:53 AM, Dan Carpenter wrote:

We accidentally overwrite the original saved value of "flags" so that
we can't re-enable IRQs at the end of the function.  Presumably this
function is mostly called with IRQs disabled or it would be obvious in
testing.

Fixes: aceeffbb59bb ("zfcp: trace full payload of all SAN records 
(req,resp,iels)")


Cc:  #2.6.38+


Signed-off-by: Dan Carpenter 


Signed-off-by: Steffen Maier 



diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 637cf89..5810019 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -384,7 +384,7 @@ void zfcp_dbf_san(char *tag, struct zfcp_dbf *dbf,
/* if (len > rec_len):
 * dump data up to cap_len ignoring small duplicate in rec->payload
 */
-   spin_lock_irqsave(&dbf->pay_lock, flags);
+   spin_lock(&dbf->pay_lock);
memset(payload, 0, sizeof(*payload));
memcpy(payload->area, paytag, ZFCP_DBF_TAG_LEN);
payload->fsf_req_id = req_id;



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

job->request->msgcode) {
+   switch (bsg_request->msgcode) {
case FC_BSG_RPT_ELS:
cmdlen += sizeof(struct fc_bsg_rport_els);
goto check_bidi;
@@ -3915,8 +3922,8 @@ check_bidi:
 fail_rport_msg:
/* return the errno failure code as the only status */
BUG_ON(job->reply_len < sizeof(uint32_t));
-   job->reply->reply_payload_rcv_len = 0;
-   job->reply->result = ret;
+   bsg_reply->reply_payload_rcv_len = 0;
+   bsg_reply->result = ret;
job->reply_len = sizeof(uint32_t);
fc_bsg_jobdone(job);
return FC_DISPATCH_UNLOCKED;




@@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue *q, struct 
Scsi_Host *shost,
struct request *req;
struct fc_bsg_job *job;
enum fc_dispatch_result ret;
+   struct fc_bsg_reply *bsg_reply;

if (!get_device(dev))
return;
@@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue *q, struct 
Scsi_Host *shost,
/* check if we have the msgcode value at least */
if (job->request_len < sizeof(uint32_t)) {
BUG_ON(job->reply_len < sizeof(uint32_t));
-   job->reply->reply_payload_rcv_len = 0;
-   job->reply->result = -ENOMSG;
+       bsg_reply = job->reply;
+   bsg_reply->reply_payload_rcv_len = 0;
+   bsg_reply->result = -ENOMSG;
job->reply_len = sizeof(uint32_t);
fc_bsg_jobdone(job);
spin_lock_irq(q->queue_lock);




--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

Hm, still behaves for me like I reported for v2:
http://marc.info/?l=linux-scsi&m=147637177902937&w=2

On 10/13/2016 05:00 PM, Johannes Thumshirn wrote:

Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
helper variables bsg_request and bsg_reply. This will be helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 
Reviewed-by: Hannes Reinecke 
---
 drivers/s390/scsi/zfcp_fc.c  |   9 +-
 drivers/scsi/bfa/bfad_bsg.c  |  40 +++---
 drivers/scsi/ibmvscsi/ibmvfc.c   |  22 ++--
 drivers/scsi/libfc/fc_lport.c|  23 ++--
 drivers/scsi/lpfc/lpfc_bsg.c | 194 +---
 drivers/scsi/qla2xxx/qla_bsg.c   | 264 ++-
 drivers/scsi/qla2xxx/qla_iocb.c  |   5 +-
 drivers/scsi/qla2xxx/qla_isr.c   |  46 ---
 drivers/scsi/qla2xxx/qla_mr.c|  10 +-
 drivers/scsi/scsi_transport_fc.c |  37 +++---
 10 files changed, 387 insertions(+), 263 deletions(-)



diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 8ff2067..eafc7555 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c



@@ -3973,8 +3981,9 @@ enum fc_dispatch_result {
/* check if we have the msgcode value at least */
if (job->request_len < sizeof(uint32_t)) {
BUG_ON(job->reply_len < sizeof(uint32_t));
-   job->reply->reply_payload_rcv_len = 0;
-   job->reply->result = -ENOMSG;
+   bsg_reply = job->reply;
+   bsg_reply->reply_payload_rcv_len = 0;
+   bsg_reply->result = -ENOMSG;
job->reply_len = sizeof(uint32_t);
fc_bsg_jobdone(job);
spin_lock_irq(q->queue_lock);



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v3 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

Hi Johannes,

On 11/15/2016 12:56 PM, Johannes Thumshirn wrote:

On Tue, Oct 25, 2016 at 09:43:14AM +0200, Johannes Thumshirn wrote:

On Fri, Oct 14, 2016 at 09:38:21AM +0200, Johannes Thumshirn wrote:

On Thu, Oct 13, 2016 at 05:55:11PM +0200, Steffen Maier wrote:

Hm, still behaves for me like I reported for v2:
http://marc.info/?l=linux-scsi&m=147637177902937&w=2


[...]



The rational behind this is, in fc_req_to_bsgjob() we're assigning
job->request as req->cmd and job->request_len = req->cmd_len. But without
checkinf job->request_len we don't know whether we're save to touch
job->request (a.k.a. bsg_request).


Hi Steffen,
Did you have any chance testing this? I hacked fcping to work with non-FCoE
and rports as well and tested with FCoE and lpfc. No problems seen from my
side. I've also pused the series (With this change folded in) to my git
tree at [1] if this helps you in any way.

[1] 
https://git.kernel.org/cgit/linux/kernel/git/jth/linux.git/log/?h=scsi-bsg-rewrite-v4



So I finally have a test system up and running. I have good and bad news. The
good news is, I can't get the system crashing with my patches, the bad news is
I can't get zfcp_ping and zfcp_show to output something but HBA_STATUS_ERROR
with my patches and without.


Assuming you run the latest package version on s390x:

Do steps 2 and 3 of the procedure in
http://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.lhdd/lhdd_t_fcp_api_runappl.html
help?

The only other thing I can think of from the top of my head is that BSG 
ioctls are sensitive regarding ABI and I once had the kernel ioctl 
return EINVAL due to unmatching kernel-headers and libzfcphbaapi maps 
this EINVAL to HBA_STATUS_ERROR because there is no more specifically 
suitable HBA constant [old SUSE bugs 834498 and 834500].



And btw, I renamed the branch to fc-bsg-rewrite-v4 in case you want to clone
from it (it has patch 2/16 changed to the v3 submission).

Can you please have a look with your setup?


I'm going to re-test hopefully within the next few days.

--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 2/2] s390: zfcp: Remove unneeded linux/miscdevice.h include

[Steffen Maier]

Thanks, Corentin, queued for next zfcp feature submission to linux-scsi as:

"zfcp: Remove unneeded linux/miscdevice.h include"

In fact, this seems a minor missing code cleanup of the following older 
"feature" which also dropped the only former use of a misc device in zfcp:

663e0890e31cb85f0cca5ac1faaee0d2d52880b5
("[SCSI] zfcp: remove access control tables interface")
b5dc3c4800cc5c2c0b3c93a97eb4c7afa0aae49a
("[SCSI] zfcp: remove access control tables interface (keep sysfs files)")
1b33ef23946adee4b7d9d6b16b7285ce61063451
("zfcp: remove access control tables interface (port leftovers)").

On 12/15/2016 03:18 PM, Corentin Labbe wrote:

drivers/s390/scsi/zfcp_aux.c does not contain any miscdevice so the
inclusion of linux/miscdevice.h is uncessary.

Signed-off-by: Corentin Labbe 
---
 drivers/s390/scsi/zfcp_aux.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
index bcc8f3d..82ac331 100644
--- a/drivers/s390/scsi/zfcp_aux.c
+++ b/drivers/s390/scsi/zfcp_aux.c
@@ -29,7 +29,6 @@
 #define KMSG_COMPONENT "zfcp"
 #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt

-#include 
 #include 
 #include 
 #include 



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] s390: scsi, use setup_timer instead of init_timer

[Steffen Maier]

Thanks, Lukáš, Jiri, queued for next zfcp feature submission to 
linux-scsi as:


"zfcp: use setup_timer instead of init_timer"

On 03/03/2017 01:46 PM, Jiri Slaby wrote:

From: Lukáš Korenčik 

Use inicialization with setup_timer function instead of using
init_timer function and data fields. It improves readability.

Signed-off-by: Lukáš Korenčik 
Signed-off-by: Jiri Slaby 
Cc: Steffen Maier 
Cc: Martin Schwidefsky 
Cc: Heiko Carstens 
Cc: 
---
 drivers/s390/scsi/zfcp_erp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index 7ccfce559034..14c0cbe335b3 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -572,9 +572,8 @@ static void zfcp_erp_memwait_handler(unsigned long data)

 static void zfcp_erp_strategy_memwait(struct zfcp_erp_action *erp_action)
 {
-   init_timer(&erp_action->timer);
-   erp_action->timer.function = zfcp_erp_memwait_handler;
-   erp_action->timer.data = (unsigned long) erp_action;
+   setup_timer(&erp_action->timer, zfcp_erp_memwait_handler,
+   (unsigned long) erp_action);
erp_action->timer.expires = jiffies + HZ;
add_timer(&erp_action->timer);
 }



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2] Revert "zram: convert remaining CLASS_ATTR() to CLASS_ATTR_RO()"

[Steffen Maier]

On 6/17/20 1:49 PM, Greg Kroah-Hartman wrote:

From: Wade Mealing 

Turns out that the permissions for 0400 really are what we want here,
otherwise any user can read from this file.

[fixed formatting, added changelog, and made attribute static - gregkh]

Reported-by: Wade Mealing 
Cc: stable 
Fixes: f40609d1591f ("zram: convert remaining CLASS_ATTR() to CLASS_ATTR_RO()")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1847832
Signed-off-by: Greg Kroah-Hartman 


Reviewed-by: Steffen Maier 


---
v2: fix read/write confusion in the changelog, thanks to Steffen for the
 review.
 was more specific as to the changes I made to the original patch.

  drivers/block/zram/zram_drv.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 6e2ad90b17a3..270dd810be54 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -2021,7 +2021,8 @@ static ssize_t hot_add_show(struct class *class,
return ret;
return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
  }
-static CLASS_ATTR_RO(hot_add);
+static struct class_attribute class_attr_hot_add =
+   __ATTR(hot_add, 0400, hot_add_show, NULL);
  
  static ssize_t hot_remove_store(struct class *class,

struct class_attribute *attr,




--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

[Steffen Maier]

On 9/9/20 10:06 PM, Joe Perches wrote:

fallthrough to a separate case/default label break; isn't very readable.

Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;

Found using:

$ grep-2.5.4 -rP --include=*.[ch] -n 
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *

Miscellanea:

o Move or coalesce a couple label blocks above a default: block.

Signed-off-by: Joe Perches 
---

Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.



  drivers/s390/scsi/zfcp_fsf.c  |  2 +-



  82 files changed, 109 insertions(+), 112 deletions(-)



diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 140186fe1d1e..2741a07df692 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2105,7 +2105,7 @@ static void zfcp_fsf_open_lun_handler(struct zfcp_fsf_req 
*req)
  
  	case FSF_PORT_HANDLE_NOT_VALID:

zfcp_erp_adapter_reopen(adapter, 0, "fsouh_1");
-   fallthrough;
+   break;
case FSF_LUN_ALREADY_OPEN:
break;
case FSF_PORT_BOXED:


Acked-by: Steffen Maier  # for zfcp


--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] Revert "zram: convert remaining CLASS_ATTR() to CLASS_ATTR_RO()"

[Steffen Maier]

On 6/17/20 12:34 PM, Greg Kroah-Hartman wrote:

From: Wade Mealing 

Turns out that the permissions for 0400 really are what we want here,
otherwise any user can write to this file.


Minor confusion on my side:
Did you mean "read from" instead of "write to"?
I.e. only owner may read but not group nor others.
Whereas using CLASS_ATTR_RO would resolve into 0444.



[fixed formatting and made static - gregkh]

Reported-by: Wade Mealing 
Cc: stable 
Fixes: f40609d1591f ("zram: convert remaining CLASS_ATTR() to CLASS_ATTR_RO()")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1847832
Signed-off-by: Greg Kroah-Hartman 
---
  drivers/block/zram/zram_drv.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 6e2ad90b17a3..270dd810be54 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -2021,7 +2021,8 @@ static ssize_t hot_add_show(struct class *class,
return ret;
return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
  }
-static CLASS_ATTR_RO(hot_add);
+static struct class_attribute class_attr_hot_add =
+   __ATTR(hot_add, 0400, hot_add_show, NULL);

  static ssize_t hot_remove_store(struct class *class,
struct class_attribute *attr,




--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] s390: scsi, use setup_timer instead of init_timer

[Steffen Maier]

Hi Jiri,

On 06/26/2017 02:06 PM, Jiri Slaby wrote:

On 03/07/2017, 02:06 PM, Steffen Maier wrote:

Thanks, Lukáš, Jiri, queued for next zfcp feature submission to
linux-scsi as:

"zfcp: use setup_timer instead of init_timer"


Hi,

was this actually submitted somewhere or did it fall through the cracks?


still queued, to be submitted soon

--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 3/3] zfcp: drop open coded assignments of timer_list.function

[Steffen Maier]

If this has not been picked/merged yet (it's not in Linus' tree yet),
could you please drop it because it's buggy?

This would buy me time to come up with a proper solution,
otherwise I would be forced to fix it within 4.15-rc and
am not sure I can make it.

On 11/08/2017 03:17 PM, Steffen Maier wrote:
> The majority of requests is regular SCSI I/O on the hot path.
> Since these use a timeout owned by the block layer, zfcp does not use
> zfcp_fsf_req.timer. Hence, the very early unconditional and even
> incomplete (handler function yet unknown) timer initialization in
> zfcp_fsf_req_create() is not necessary.
> 
> Instead defer the timer initialization to when we know zfcp needs to use
> its own request timeout in zfcp_fsf_start_timer() and
> zfcp_fsf_start_erp_timer().

This means, we no longer always initialize the timer for
any request type, but only for some request types.

However, we still do have 2 unconditional del_timer() calls
independent of the request type.

I don't understand yet why I haven't seen the following on function testing,
but I see it now while working on something else:

[  325.908536] scsi host2: scsi_eh_2: sleeping
[  325.908707] scsi host2: zfcp
[  325.912974] qdio: 0.0.1900 ZFCP on SC 11 using AI:1 QEBSM:1 PRI:1 TDD:1 
SIGA: W A 
[  331.112469] scsi 2:0:0:0: scsi scan: INQUIRY pass 1 length 36
[  331.122253] ODEBUG: assert_init not available (active state 0) object type: 
timer_list hint:   (null)
[  331.122319] [ cut here ]
[  331.122332] WARNING: CPU: 0 PID: 2195 at lib/debugobjects.c:291 
debug_print_object+0xb4/0xd8
[  331.122339] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE 
nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 
nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp 
bridge stp llc ip6table_filter ip6_tables iptable_filter sunrpc qeth_l2 
rng_core ghash_s390 prng aes_s390 des_s390 des_generic sha512_s390 zfcp 
sha256_s390 scsi_transport_fc sha1_s390 sha_common dm_multipath qeth qdio 
scsi_mod vmur ccwgroup dm_mod vhost_net tun vhost tap sch_fq_codel kvm 
ip_tables x_tables autofs4
[  331.122503] CPU: 0 PID: 2195 Comm: chccwdev Not tainted 4.14.0localversion+ 
#1
[  331.122510] Hardware name: IBM 2964 N96 702 (z/VM 6.4.0)
[  331.122518] task: 4c673200 task.stack: 5fdac000
[  331.122599] Krnl PSW : 0704d0018000 007828cc 
(debug_print_object+0xb4/0xd8)
[  331.122693]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 
RI:0 EA:3
[  331.122748] Krnl GPRS: 200088a4a0f0 8100 0061 
00c215b6
[  331.122753]007828c8  00c158da 
4cf57200
[  331.122766]5e462548 0201d608 00c65888 
00e8dc08
[  331.122830]6fbfb808 00aac910 007828c8 
6fbfb708
[  331.122843] Krnl Code: 007828bc: c02000271779larl
%r2,c657ae
  007828c2: c0e5ffd25793brasl   
%r14,1cd7e8
 #007828c8: a7f40001brc 
15,7828ca
 >007828cc: c41d003655b4lrl 
%r1,e4d434
  007828d2: e340f0e80004lg  
%r4,232(%r15)
  007828d8: a71a0001ahi %r1,1
  007828dc: eb6ff0a80004lmg 
%r6,%r15,168(%r15)
  007828e2: c41f003655a9strl
%r1,e4d434
[  331.123056] Call Trace:
[  331.123065] ([<007828c8>] debug_print_object+0xb0/0xd8)
[  331.123074]  [<00783900>] debug_object_assert_init+0x148/0x180 
[  331.123085]  [<001e8e2c>] del_timer+0x34/0x90 
[  331.123106]  [<03ff8032fad2>] zfcp_fsf_req_complete+0x2b2/0x7a8 [zfcp] 
[  331.123122]  [<03ff80331e2e>] zfcp_fsf_reqid_check+0xe6/0x150 [zfcp] 
[  331.123151]  [<03ff80332be0>] zfcp_qdio_int_resp+0x138/0x180 [zfcp] 
[  331.123167]  [<03ff801df19e>] qdio_kick_handler+0x1be/0x2c0 [qdio] 
[  331.123178]  [<03ff801e1ca6>] __tiqdio_inbound_processing+0x466/0xd00 
[qdio] 
[  331.123191]  [<0014f5e0>] tasklet_action+0x100/0x188 
[  331.123203]  [<00a56af2>] __do_softirq+0x2ca/0x5e0 
[  331.123215]  [<0014ec24>] irq_exit+0x74/0xd8 
[  331.123228]  [<0010c5c4>] do_IRQ+0xbc/0xf0 
[  331.123278]  [<00a55c2c>] io_int_handler+0x104/0x2d4 
[  331.123354]  [<00168ca6>] queue_work_on+0x8e/0xa8 
[  331.123393] ([<00168ca2>] queue_work_on+0x8a/0xa8)
[  331.123443]  [<0080a932>] pty_write+0x62/0x88 
[  331.123454]  [<00801c64>] n_tty_write+0x284/0x4b8 
[  331.123463]  [<007febb6>] tty_write+0x34e/0x378 
[  

[PATCH] checkpatch: HLIST_HEAD is also declaration

[Steffen Maier]

Fixes the following false warning:

WARNING: Missing a blank line after declarations
#71: FILE: drivers/s390/scsi/zfcp_fsf.c:422:
+   struct hlist_node *tmp;
+   HLIST_HEAD(remove_queue);

Signed-off-by: Steffen Maier 
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index baa3c7be04ad..a539fb2561bc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -714,7 +714,7 @@ our $FuncArg = 
qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)};
 
 our $declaration_macros = qr{(?x:

(?:$Storage\s+)?(?:[A-Z_][A-Z0-9]*_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,6}\s*\(|
-   (?:$Storage\s+)?LIST_HEAD\s*\(|
+   (?:$Storage\s+)?H?LIST_HEAD\s*\(|
(?:$Storage\s+)?${Type}\s+uninitialized_var\s*\(
 )};
 
-- 
2.11.2

[PATCH] docs-rst: fix broken links to dynamic-debug-howto in kernel-parameters

[Steffen Maier]

Another place in lib/Kconfig.debug was already fixed in commit f8998c226587
("lib/Kconfig.debug: correct documentation paths").

Fixes: 9d85025b0418 ("docs-rst: create an user's manual book")
Signed-off-by: Steffen Maier 
---
 Documentation/admin-guide/kernel-parameters.txt | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index facc20a3f962..774569e02959 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -721,7 +721,8 @@
See also Documentation/input/joystick-parport.txt
 
ddebug_query=   [KNL,DYNAMIC_DEBUG] Enable debug messages at early boot
-   time. See Documentation/dynamic-debug-howto.txt for
+   time. See
+   Documentation/admin-guide/dynamic-debug-howto.rst for
details.  Deprecated, see dyndbg.
 
debug   [KNL] Enable kernel debugging (events log level).
@@ -875,7 +876,8 @@
dyndbg[="val"]  [KNL,DYNAMIC_DEBUG]
module.dyndbg[="val"]
Enable debug messages at boot time.  See
-   Documentation/dynamic-debug-howto.txt for details.
+   Documentation/admin-guide/dynamic-debug-howto.rst
+   for details.
 
nompx   [X86] Disables Intel Memory Protection Extensions.
See Documentation/x86/intel_mpx.txt for more
-- 
2.11.2

[PATCH v2] checkpatch: [HLP]LIST_HEAD is also declaration

[Steffen Maier]

Fixes the following false warning
among others for LLIST_HEAD and PLIST_HEAD:

WARNING: Missing a blank line after declarations
#71: FILE: drivers/s390/scsi/zfcp_fsf.c:422:
+   struct hlist_node *tmp;
+   HLIST_HEAD(remove_queue);

Signed-off-by: Steffen Maier 
---
Changes from v1:
Extended it to also support LLIST_HEAD and PLIST_HEAD as suggested by
Joe Perches.

 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4b9569fa931b..86818b0ba8be 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -732,7 +732,7 @@ our $FuncArg = 
qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)};
 
 our $declaration_macros = qr{(?x:

(?:$Storage\s+)?(?:[A-Z_][A-Z0-9]*_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,6}\s*\(|
-   (?:$Storage\s+)?LIST_HEAD\s*\(|
+   (?:$Storage\s+)?[HLP]?LIST_HEAD\s*\(|
(?:$Storage\s+)?${Type}\s+uninitialized_var\s*\(
 )};
 
-- 
2.11.2

Re: [PATCH v2] checkpatch: [HLP]LIST_HEAD is also declaration

[Steffen Maier]

Sorry, only realized after sending v2, that you also mentioned SLIST_HEAD, 
which I haven't found on git grep'ing include/ only.

I could only find SLIST_HEAD (and even another variant) in some driver private 
header.
Am I searching in a wrong way?
If not, shall we still add this like the other official list APIs?

Linus' tree at:
$ git branch -vv
* master   63f700aab4c1 [origin/master] Merge tag 'xtensa-20170612' of 
git://github.com/jcmvbkbc/linux-xtensa

$ git grep 'define.*LIST_HEAD('
drivers/gpu/drm/nouveau/include/nvif/list.h:124:#define LIST_HEAD(name) \
drivers/gpu/drm/radeon/mkregtable.c:47:#define LIST_HEAD(name) \
drivers/scsi/aic7xxx/queue.h:107:#defineSLIST_HEAD(name, type)  
\
drivers/scsi/aic7xxx/queue.h:249:#defineBSD_LIST_HEAD(name, type)   
\
include/linux/list.h:22:#define LIST_HEAD(name) \
include/linux/list.h:625:#define HLIST_HEAD(name) struct hlist_head name = {  
.first = NULL }
include/linux/list.h:626:#define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)
include/linux/llist.h:75:#define LLIST_HEAD(name)   struct llist_head name 
= LLIST_HEAD_INIT(name)
include/linux/plist.h:104:#define PLIST_HEAD(head) \
include/linux/shm.h:57:#define shm_init_task(task) 
INIT_LIST_HEAD(&(task)->sysvshm.shm_clist)
scripts/kconfig/list.h:30:#define LIST_HEAD(name) \
tools/include/linux/list.h:21:#define LIST_HEAD(name) \
tools/include/linux/list.h:595:#define HLIST_HEAD(name) struct hlist_head name 
= {  .first = NULL }
tools/include/linux/list.h:596:#define INIT_HLIST_HEAD(ptr) ((ptr)->first = 
NULL)
tools/usb/usbip/libsrc/list.h:24:#define LIST_HEAD(name) \


On 06/14/2017 03:35 PM, Steffen Maier wrote:
> Fixes the following false warning
> among others for LLIST_HEAD and PLIST_HEAD:
> 
>  WARNING: Missing a blank line after declarations
>  #71: FILE: drivers/s390/scsi/zfcp_fsf.c:422:
>  +struct hlist_node *tmp;
>  +HLIST_HEAD(remove_queue);
> 
> Signed-off-by: Steffen Maier 
> ---
> Changes from v1:
> Extended it to also support LLIST_HEAD and PLIST_HEAD as suggested by
> Joe Perches.
> 
>   scripts/checkpatch.pl | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4b9569fa931b..86818b0ba8be 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -732,7 +732,7 @@ our $FuncArg = 
> qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)};
> 
>   our $declaration_macros = qr{(?x:
>   
> (?:$Storage\s+)?(?:[A-Z_][A-Z0-9]*_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,6}\s*\(|
> - (?:$Storage\s+)?LIST_HEAD\s*\(|
> + (?:$Storage\s+)?[HLP]?LIST_HEAD\s*\(|
>   (?:$Storage\s+)?${Type}\s+uninitialized_var\s*\(
>   )};
> 

-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] scsi: use set_host_byte instead of open-coding it

[Steffen Maier]

On 10/10/2017 05:29 PM, Johannes Thumshirn wrote:

Call set_host_byte() instead of open-coding it.

Converted using this simple Coccinelle spatch


@@
local idexpression struct scsi_cmnd *c;
expression E1;
@@

- c->result = E1 << 16;
+ set_host_byte(c, E1);


Maybe I misunderstand, but doesn't set_host_byte only set the host byte 
but leave the other 3 parts untouched in c->result?


static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
{
cmd->result = (cmd->result & 0xff00) | (status << 16);
}

In contrast, assigning something to c->result resets all parts.
If so, the semantic patch would introduce a subtle semantic change.
Unless it's guaranteed that in all the touched cases, c->result always 
has 0 for status, message, and driver byte before calling set_host_byte().


Bart's suggestion also sounds nice.

FYI: Originally, I only thought about using set_host_byte in that one 
place fix of yours; I did not expect a full framework rework.


--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2 1/1] scsi: fc: check for rport presence in fc_block_scsi_eh

[Steffen Maier]

On 09/26/2017 08:58 AM, Johannes Thumshirn wrote:

Coverity-scan recently found a possible NULL pointer dereference in
fc_block_scsi_eh() as starget_to_rport() either returns the rport for
the startget or NULL.

While it is rather unlikely to have fc_block_scsi_eh() called without
an rport associated it's a good idea to catch potential misuses of the
API gracefully.

Signed-off-by: Johannes Thumshirn 
Reviewed-by: Bart Van Assche 
---

Changes since v1:
- s/WARN_ON/WARN_ON_ONCE/ (Bart)

---
  drivers/scsi/scsi_transport_fc.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index ba9d70f8a6a1..38abff7b5dbc 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3328,6 +3328,9 @@ int fc_block_scsi_eh(struct scsi_cmnd *cmnd)
  {
struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));

+   if (WARN_ON_ONCE(!rport))
+   return 0;


Good idea.

However, return 0 or FAST_IO_FAIL?
I mean the callchains to this function (and of fc_block_rport()) react 
differently depending on the return value.
Returning 0 means that the rport left the blocked state, i.e. is usable 
for traffic again.

If there is no rport at all, I suppose one cannot use it for traffic.
If there is any I/O pending on this scope and we return 0, scsi_eh 
escalates; and if this happens for a host_reset we end up with offlined 
scsi_devices.
I wonder if returning FAST_IO_FAIL would be more appropriate here in 
this case, in order to have scsi_eh let the pending I/O bubble up for a 
timely path failover?


--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] scsi: libiscsi: fix shifting of DID_REQUEUE host byte

[Steffen Maier]

Use wrapper functions to advertize their use in an attempt to avoid 
wrong shifting in the future?


On 10/09/2017 01:33 PM, Johannes Thumshirn wrote:

The SCSI host byte should be shifted left by 16 in order to have
scsi_decide_disposition() do the right thing (.i.e. requeue the command).

Signed-off-by: Johannes Thumshirn 
Fixes: 661134ad3765 ("[SCSI] libiscsi, bnx2i: make bound ep check common")
Cc: Lee Duncan 
Cc: Hannes Reinecke 
Cc: Bart Van Assche 
Cc: Chris Leech 
---
  drivers/scsi/libiscsi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index bd4605a34f54..9cba4913b43c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1728,7 +1728,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)

if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
reason = FAILURE_SESSION_IN_RECOVERY;
-   sc->result = DID_REQUEUE;
+   sc->result = DID_REQUEUE << 16;


not sure if this really wants to reset the other parts of result, but if 
so (and they are not 0 already anyway), preceed the set_host_byte() by:

sc->result = 0;

set_host_byte(sc, DID_REQUEUE);


goto fault;
}



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[PATCH 0/3] zfcp: timer_setup() refactoring feature for v4.15-rc1

[Steffen Maier]

Hi all,

here is a small series for the timer_setup() refactoring of zfcp.
We target it for the merge window to land in v4.15-rc1.

Unfortunately, they don't seem to apply to the current state of neither
James' misc branch nor Martin's 4.15/scsi-queue branch,
because they depend on:
v4.14-rc3 686fef928bba ("timer: Prepare to change timer callback argument type")
and
v4.14-rc7 ab31fd0ce65e ("scsi: zfcp: fix erp_action use-before-initialize in 
REC action trace").

However, they do apply to Linus' tree for v4.14-rc7 or later and
thus they would also apply for the upcoming merge window.

In http://www.spinics.net/lists/linux-scsi/msg114581.html I saw a decision
to have such changes go in via the timer tree. I would be happy with that.

Kees Cook (1):
  zfcp: convert timers to use timer_setup()

Steffen Maier (2):
  zfcp: purely mechanical update using timer API, plus blank lines
  zfcp: drop open coded assignments of timer_list.function

 drivers/s390/scsi/zfcp_erp.c | 15 +--
 drivers/s390/scsi/zfcp_ext.h |  2 +-
 drivers/s390/scsi/zfcp_fsf.c | 13 ++---
 3 files changed, 16 insertions(+), 14 deletions(-)

-- 
2.13.5

[PATCH 3/3] zfcp: drop open coded assignments of timer_list.function

[Steffen Maier]

The majority of requests is regular SCSI I/O on the hot path.
Since these use a timeout owned by the block layer, zfcp does not use
zfcp_fsf_req.timer. Hence, the very early unconditional and even
incomplete (handler function yet unknown) timer initialization in
zfcp_fsf_req_create() is not necessary.

Instead defer the timer initialization to when we know zfcp needs to use
its own request timeout in zfcp_fsf_start_timer() and
zfcp_fsf_start_erp_timer(). At that point in time we also know the handler
function. So drop open coded assignments of timer_list.function and
instead use the new timer API wrapper function timer_setup().

This way, we don't have to touch zfcp again, when the cast macro
TIMER_FUNC_TYPE gets removed again after the global conversion to
timer_setup() is complete.

Depends-on: v4.14-rc3 commit 686fef928bba ("timer: Prepare to change timer 
callback argument type")
Signed-off-by: Steffen Maier 
Reviewed-by: Jens Remus 
---
 drivers/s390/scsi/zfcp_fsf.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 51b81c0a0652..c8e368f0f299 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -34,7 +34,7 @@ static void zfcp_fsf_request_timeout_handler(struct 
timer_list *t)
 static void zfcp_fsf_start_timer(struct zfcp_fsf_req *fsf_req,
 unsigned long timeout)
 {
-   fsf_req->timer.function = 
(TIMER_FUNC_TYPE)zfcp_fsf_request_timeout_handler;
+   timer_setup(&fsf_req->timer, zfcp_fsf_request_timeout_handler, 0);
fsf_req->timer.expires = jiffies + timeout;
add_timer(&fsf_req->timer);
 }
@@ -42,7 +42,7 @@ static void zfcp_fsf_start_timer(struct zfcp_fsf_req *fsf_req,
 static void zfcp_fsf_start_erp_timer(struct zfcp_fsf_req *fsf_req)
 {
BUG_ON(!fsf_req->erp_action);
-   fsf_req->timer.function = (TIMER_FUNC_TYPE)zfcp_erp_timeout_handler;
+   timer_setup(&fsf_req->timer, zfcp_erp_timeout_handler, 0);
fsf_req->timer.expires = jiffies + 30 * HZ;
add_timer(&fsf_req->timer);
 }
@@ -692,7 +692,6 @@ static struct zfcp_fsf_req *zfcp_fsf_req_create(struct 
zfcp_qdio *qdio,
adapter->req_no++;
 
INIT_LIST_HEAD(&req->list);
-   timer_setup(&req->timer, NULL, 0);
init_completion(&req->completion);
 
req->adapter = adapter;
-- 
2.13.5

[PATCH 2/3] zfcp: purely mechanical update using timer API, plus blank lines

[Steffen Maier]

erp_memwait only occurs in seldom memory pressure situations.
The typical case never uses the associated timer and thus also
does not need to initialize the timer.
Also, we don't want to re-initialize the timer each time we re-use an
erp_action in zfcp_erp_setup_act() [see also v4.14-rc7 commit ab31fd0ce65e
("scsi: zfcp: fix erp_action use-before-initialize in REC action trace")
for erp_action life cycle].
Hence, retain the lazy inintialization of zfcp_erp_action.timer
in zfcp_erp_strategy_memwait().

Add an empty line after declarations in zfcp_erp_timeout_handler()
and zfcp_fsf_request_timeout_handler() even though it was also missing
before the timer conversion.

Fix checkpatch warning:
WARNING: function definition argument 'struct timer_list *' should also have an 
identifier name
+extern void zfcp_erp_timeout_handler(struct timer_list *);

Depends-on: v4.14-rc3 commit 686fef928bba ("timer: Prepare to change timer 
callback argument type")
Signed-off-by: Steffen Maier 
Reviewed-by: Jens Remus 
---
 drivers/s390/scsi/zfcp_erp.c | 5 ++---
 drivers/s390/scsi/zfcp_ext.h | 2 +-
 drivers/s390/scsi/zfcp_fsf.c | 1 +
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index 822a852d578e..1d91a32db08e 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -56,8 +56,6 @@ enum zfcp_erp_act_result {
ZFCP_ERP_NOMEM = 5,
 };
 
-static void zfcp_erp_memwait_handler(struct timer_list *t);
-
 static void zfcp_erp_adapter_block(struct zfcp_adapter *adapter, int mask)
 {
zfcp_erp_clear_adapter_status(adapter,
@@ -239,7 +237,6 @@ static struct zfcp_erp_action *zfcp_erp_setup_act(int need, 
u32 act_status,
erp_action->fsf_req_id = 0;
erp_action->action = need;
erp_action->status = act_status;
-   timer_setup(&erp_action->timer, zfcp_erp_memwait_handler, 0);
 
return erp_action;
 }
@@ -571,6 +568,7 @@ void zfcp_erp_timeout_handler(struct timer_list *t)
 {
struct zfcp_fsf_req *fsf_req = from_timer(fsf_req, t, timer);
struct zfcp_erp_action *act = fsf_req->erp_action;
+
zfcp_erp_notify(act, ZFCP_STATUS_ERP_TIMEDOUT);
 }
 
@@ -583,6 +581,7 @@ static void zfcp_erp_memwait_handler(struct timer_list *t)
 
 static void zfcp_erp_strategy_memwait(struct zfcp_erp_action *erp_action)
 {
+   timer_setup(&erp_action->timer, zfcp_erp_memwait_handler, 0);
erp_action->timer.expires = jiffies + HZ;
add_timer(&erp_action->timer);
 }
diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
index 978a0d596f68..bf8ea4df2bb8 100644
--- a/drivers/s390/scsi/zfcp_ext.h
+++ b/drivers/s390/scsi/zfcp_ext.h
@@ -69,7 +69,7 @@ extern int  zfcp_erp_thread_setup(struct zfcp_adapter *);
 extern void zfcp_erp_thread_kill(struct zfcp_adapter *);
 extern void zfcp_erp_wait(struct zfcp_adapter *);
 extern void zfcp_erp_notify(struct zfcp_erp_action *, unsigned long);
-extern void zfcp_erp_timeout_handler(struct timer_list *);
+extern void zfcp_erp_timeout_handler(struct timer_list *t);
 
 /* zfcp_fc.c */
 extern struct kmem_cache *zfcp_fc_req_cache;
diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 6f437df1995f..51b81c0a0652 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -25,6 +25,7 @@ static void zfcp_fsf_request_timeout_handler(struct 
timer_list *t)
 {
struct zfcp_fsf_req *fsf_req = from_timer(fsf_req, t, timer);
struct zfcp_adapter *adapter = fsf_req->adapter;
+
zfcp_qdio_siosl(adapter);
zfcp_erp_adapter_reopen(adapter, ZFCP_STATUS_COMMON_ERP_FAILED,
"fsrth_1");
-- 
2.13.5

[PATCH 1/3] zfcp: convert timers to use timer_setup()

[Steffen Maier]

From: Kees Cook 

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.

Cc: Steffen Maier 
Cc: Benjamin Block 
Cc: Martin Schwidefsky 
Cc: Heiko Carstens 
Cc: linux-s...@vger.kernel.org
Signed-off-by: Kees Cook 
Signed-off-by: Martin Schwidefsky 
[ma...@linux.vnet.ibm.com:
 depends on v4.14-rc3 686fef928bba ("timer: Prepare to change timer callback 
argument type"),
 rebased onto v4.14-rc7 ab31fd0ce65e ("scsi: zfcp: fix erp_action 
use-before-initialize in REC action trace")]
Signed-off-by: Steffen Maier 
Reviewed-by: Jens Remus 
---
 drivers/s390/scsi/zfcp_erp.c | 16 ++--
 drivers/s390/scsi/zfcp_ext.h |  2 +-
 drivers/s390/scsi/zfcp_fsf.c | 13 ++---
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index cbb8156bf5e0..822a852d578e 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -56,6 +56,8 @@ enum zfcp_erp_act_result {
ZFCP_ERP_NOMEM = 5,
 };
 
+static void zfcp_erp_memwait_handler(struct timer_list *t);
+
 static void zfcp_erp_adapter_block(struct zfcp_adapter *adapter, int mask)
 {
zfcp_erp_clear_adapter_status(adapter,
@@ -237,6 +239,7 @@ static struct zfcp_erp_action *zfcp_erp_setup_act(int need, 
u32 act_status,
erp_action->fsf_req_id = 0;
erp_action->action = need;
erp_action->status = act_status;
+   timer_setup(&erp_action->timer, zfcp_erp_memwait_handler, 0);
 
return erp_action;
 }
@@ -564,21 +567,22 @@ void zfcp_erp_notify(struct zfcp_erp_action *erp_action, 
unsigned long set_mask)
  * zfcp_erp_timeout_handler - Trigger ERP action from timed out ERP request
  * @data: ERP action (from timer data)
  */
-void zfcp_erp_timeout_handler(unsigned long data)
+void zfcp_erp_timeout_handler(struct timer_list *t)
 {
-   struct zfcp_erp_action *act = (struct zfcp_erp_action *) data;
+   struct zfcp_fsf_req *fsf_req = from_timer(fsf_req, t, timer);
+   struct zfcp_erp_action *act = fsf_req->erp_action;
zfcp_erp_notify(act, ZFCP_STATUS_ERP_TIMEDOUT);
 }
 
-static void zfcp_erp_memwait_handler(unsigned long data)
+static void zfcp_erp_memwait_handler(struct timer_list *t)
 {
-   zfcp_erp_notify((struct zfcp_erp_action *)data, 0);
+   struct zfcp_erp_action *act = from_timer(act, t, timer);
+
+   zfcp_erp_notify(act, 0);
 }
 
 static void zfcp_erp_strategy_memwait(struct zfcp_erp_action *erp_action)
 {
-   setup_timer(&erp_action->timer, zfcp_erp_memwait_handler,
-   (unsigned long) erp_action);
erp_action->timer.expires = jiffies + HZ;
add_timer(&erp_action->timer);
 }
diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
index 8ca2ab7deaa9..978a0d596f68 100644
--- a/drivers/s390/scsi/zfcp_ext.h
+++ b/drivers/s390/scsi/zfcp_ext.h
@@ -69,7 +69,7 @@ extern int  zfcp_erp_thread_setup(struct zfcp_adapter *);
 extern void zfcp_erp_thread_kill(struct zfcp_adapter *);
 extern void zfcp_erp_wait(struct zfcp_adapter *);
 extern void zfcp_erp_notify(struct zfcp_erp_action *, unsigned long);
-extern void zfcp_erp_timeout_handler(unsigned long);
+extern void zfcp_erp_timeout_handler(struct timer_list *);
 
 /* zfcp_fc.c */
 extern struct kmem_cache *zfcp_fc_req_cache;
diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 00fb98f7b2cd..6f437df1995f 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -21,9 +21,10 @@
 
 struct kmem_cache *zfcp_fsf_qtcb_cache;
 
-static void zfcp_fsf_request_timeout_handler(unsigned long data)
+static void zfcp_fsf_request_timeout_handler(struct timer_list *t)
 {
-   struct zfcp_adapter *adapter = (struct zfcp_adapter *) data;
+   struct zfcp_fsf_req *fsf_req = from_timer(fsf_req, t, timer);
+   struct zfcp_adapter *adapter = fsf_req->adapter;
zfcp_qdio_siosl(adapter);
zfcp_erp_adapter_reopen(adapter, ZFCP_STATUS_COMMON_ERP_FAILED,
"fsrth_1");
@@ -32,8 +33,7 @@ static void zfcp_fsf_request_timeout_handler(unsigned long 
data)
 static void zfcp_fsf_start_timer(struct zfcp_fsf_req *fsf_req,
 unsigned long timeout)
 {
-   fsf_req->timer.function = zfcp_fsf_request_timeout_handler;
-   fsf_req->timer.data = (unsigned long) fsf_req->adapter;
+   fsf_req->timer.function = 
(TIMER_FUNC_TYPE)zfcp_fsf_request_timeout_handler;
fsf_req->timer.expires = jiffies + timeout;
add_timer(&fsf_req->timer);
 }
@@ -41,8 +41,7 @@ static void zfcp_fsf_start_timer(struct zfcp_fsf_req *fsf_req,
 static void zfcp_fsf_start_erp_timer(struct zfcp_fsf_req *fsf_req)
 {
BUG_ON(!fsf_req->erp_action);
-   fsf_req->timer.function = zfcp_erp_ti