diff mbox series

[10/23] zfcp: drop duplicate fsf_command from zfcp_fsf_req which is also in QTCB header

Message ID 20181108144458.29012-11-maier@linux.ibm.com (mailing list archive)
State Accepted
Headers show
Series zfcp updates for v4.21 | expand

Commit Message

Steffen Maier Nov. 8, 2018, 2:44 p.m. UTC 
Status read buffers (SRBs, unsolicited notifications) never use a QTCB
[zfcp_fsf_req_create()]. zfcp_fsf_req_send() already uses this to
distinguish SRBs from other FSF request types. We can re-use this method
in zfcp_fsf_req_complete(). Introduce a helper function to make the check
for req->qtcb less magic.

SRBs always are FSF_QTCB_UNSOLICITED_STATUS, so we can hard-code this for
the two trace functions dealing with SRBs.

All other FSF request types have a QTCB and we can get the fsf_command
from there.

zfcp_dbf_hba_fsf_response() and thus zfcp_dbf_hba_fsf_res() are only called
for non-SRB requests so it's safe to dereference the QTCB
[zfcp_fsf_req_complete() returns early on SRB,  else calls
 zfcp_fsf_protstatus_eval() which calls zfcp_dbf_hba_fsf_response()].
In zfcp_scsi_forget_cmnd() we guard the QTCB dereference with a preceding
NULL check and rely on boolean shortcut evaluation.

As a side effect, this causes an alignment hole which we can close in
a later patch after having cleaned up all fields of struct zfcp_fsf_req.
Before:
$ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
...
	u32                        status;               /*   136     4 */
	u32                        fsf_command;          /*   140     4 */
	struct fsf_qtcb *          qtcb;                 /*   144     8 */
...
After:
$ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
...
	u32                        status;               /*   136     4 */
	/* XXX 4 bytes hole, try to pack */
	struct fsf_qtcb *          qtcb;                 /*   144     8 */
...

Signed-off-by: Steffen Maier <maier@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_dbf.c  |  8 ++++----
 drivers/s390/scsi/zfcp_dbf.h  |  4 ++--
 drivers/s390/scsi/zfcp_def.h  |  7 +++++--
 drivers/s390/scsi/zfcp_fsf.c  | 14 ++++++--------
 drivers/s390/scsi/zfcp_scsi.c |  4 +++-
 5 files changed, 20 insertions(+), 17 deletions(-)

Comments

Hannes Reinecke Nov. 16, 2018, 11:11 a.m. UTC | #1 
On 11/8/18 3:44 PM, Steffen Maier wrote:
> Status read buffers (SRBs, unsolicited notifications) never use a QTCB
> [zfcp_fsf_req_create()]. zfcp_fsf_req_send() already uses this to
> distinguish SRBs from other FSF request types. We can re-use this method
> in zfcp_fsf_req_complete(). Introduce a helper function to make the check
> for req->qtcb less magic.
> 
> SRBs always are FSF_QTCB_UNSOLICITED_STATUS, so we can hard-code this for
> the two trace functions dealing with SRBs.
> 
> All other FSF request types have a QTCB and we can get the fsf_command
> from there.
> 
> zfcp_dbf_hba_fsf_response() and thus zfcp_dbf_hba_fsf_res() are only called
> for non-SRB requests so it's safe to dereference the QTCB
> [zfcp_fsf_req_complete() returns early on SRB,  else calls
>   zfcp_fsf_protstatus_eval() which calls zfcp_dbf_hba_fsf_response()].
> In zfcp_scsi_forget_cmnd() we guard the QTCB dereference with a preceding
> NULL check and rely on boolean shortcut evaluation.
> 
> As a side effect, this causes an alignment hole which we can close in
> a later patch after having cleaned up all fields of struct zfcp_fsf_req.
> Before:
> $ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
> ...
> 	u32                        status;               /*   136     4 */
> 	u32                        fsf_command;          /*   140     4 */
> 	struct fsf_qtcb *          qtcb;                 /*   144     8 */
> ...
> After:
> $ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
> ...
> 	u32                        status;               /*   136     4 */
> 	/* XXX 4 bytes hole, try to pack */
> 	struct fsf_qtcb *          qtcb;                 /*   144     8 */
> ...
> 
> Signed-off-by: Steffen Maier <maier@linux.ibm.com>
> Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
> ---
>   drivers/s390/scsi/zfcp_dbf.c  |  8 ++++----
>   drivers/s390/scsi/zfcp_dbf.h  |  4 ++--
>   drivers/s390/scsi/zfcp_def.h  |  7 +++++--
>   drivers/s390/scsi/zfcp_fsf.c  | 14 ++++++--------
>   drivers/s390/scsi/zfcp_scsi.c |  4 +++-
>   5 files changed, 20 insertions(+), 17 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 3b368fcf13f4..d20977bb27a4 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -81,7 +81,7 @@  void zfcp_dbf_hba_fsf_res(char *tag, int level, struct zfcp_fsf_req *req)
 	rec->id = ZFCP_DBF_HBA_RES;
 	rec->fsf_req_id = req->req_id;
 	rec->fsf_req_status = req->status;
-	rec->fsf_cmd = req->fsf_command;
+	rec->fsf_cmd = q_head->fsf_command;
 	rec->fsf_seq_no = req->seq_no;
 	rec->u.res.req_issued = req->issued;
 	rec->u.res.prot_status = q_pref->prot_status;
@@ -94,7 +94,7 @@  void zfcp_dbf_hba_fsf_res(char *tag, int level, struct zfcp_fsf_req *req)
 	memcpy(rec->u.res.fsf_status_qual, &q_head->fsf_status_qual,
 	       FSF_STATUS_QUALIFIER_SIZE);
 
-	if (req->fsf_command != FSF_QTCB_FCP_CMND) {
+	if (q_head->fsf_command != FSF_QTCB_FCP_CMND) {
 		rec->pl_len = q_head->log_length;
 		zfcp_dbf_pl_write(dbf, (char *)q_pref + q_head->log_start,
 				  rec->pl_len, "fsf_res", req->req_id);
@@ -127,7 +127,7 @@  void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req *req)
 	rec->id = ZFCP_DBF_HBA_USS;
 	rec->fsf_req_id = req->req_id;
 	rec->fsf_req_status = req->status;
-	rec->fsf_cmd = req->fsf_command;
+	rec->fsf_cmd = FSF_QTCB_UNSOLICITED_STATUS;
 
 	if (!srb)
 		goto log;
@@ -174,7 +174,7 @@  void zfcp_dbf_hba_bit_err(char *tag, struct zfcp_fsf_req *req)
 	rec->id = ZFCP_DBF_HBA_BIT;
 	rec->fsf_req_id = req->req_id;
 	rec->fsf_req_status = req->status;
-	rec->fsf_cmd = req->fsf_command;
+	rec->fsf_cmd = FSF_QTCB_UNSOLICITED_STATUS;
 	memcpy(&rec->u.be, &sr_buf->payload.bit_error,
 	       sizeof(struct fsf_bit_error_payload));
 
diff --git a/drivers/s390/scsi/zfcp_dbf.h b/drivers/s390/scsi/zfcp_dbf.h
index d116c07ed77a..b4438713d1cc 100644
--- a/drivers/s390/scsi/zfcp_dbf.h
+++ b/drivers/s390/scsi/zfcp_dbf.h
@@ -339,8 +339,8 @@  void zfcp_dbf_hba_fsf_response(struct zfcp_fsf_req *req)
 				      zfcp_dbf_hba_fsf_resp_suppress(req)
 				      ? 5 : 1, req);
 
-	} else if ((req->fsf_command == FSF_QTCB_OPEN_PORT_WITH_DID) ||
-		   (req->fsf_command == FSF_QTCB_OPEN_LUN)) {
+	} else if ((qtcb->header.fsf_command == FSF_QTCB_OPEN_PORT_WITH_DID) ||
+		   (qtcb->header.fsf_command == FSF_QTCB_OPEN_LUN)) {
 		zfcp_dbf_hba_fsf_resp("fs_open", 4, req);
 
 	} else if (qtcb->header.log_length) {
diff --git a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
index 572debf2f528..d65adb0ae9f1 100644
--- a/drivers/s390/scsi/zfcp_def.h
+++ b/drivers/s390/scsi/zfcp_def.h
@@ -277,7 +277,6 @@  static inline u64 zfcp_scsi_dev_lun(struct scsi_device *sdev)
  * @qdio_req: qdio queue related values
  * @completion: used to signal the completion of the request
  * @status: status of the request
- * @fsf_command: FSF command issued
  * @qtcb: associated QTCB
  * @seq_no: sequence number of this request
  * @data: private data
@@ -294,7 +293,6 @@  struct zfcp_fsf_req {
 	struct zfcp_qdio_req	qdio_req;
 	struct completion	completion;
 	u32			status;
-	u32			fsf_command;
 	struct fsf_qtcb		*qtcb;
 	u32			seq_no;
 	void			*data;
@@ -311,4 +309,9 @@  int zfcp_adapter_multi_buffer_active(struct zfcp_adapter *adapter)
 	return atomic_read(&adapter->status) & ZFCP_STATUS_ADAPTER_MB_ACT;
 }
 
+static inline bool zfcp_fsf_req_is_status_read_buffer(struct zfcp_fsf_req *req)
+{
+	return req->qtcb == NULL;
+}
+
 #endif /* ZFCP_DEF_H */
diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 62311bd2df03..07b86375b461 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -84,13 +84,13 @@  static void zfcp_fsf_class_not_supp(struct zfcp_fsf_req *req)
 void zfcp_fsf_req_free(struct zfcp_fsf_req *req)
 {
 	if (likely(req->pool)) {
-		if (likely(req->qtcb))
+		if (likely(!zfcp_fsf_req_is_status_read_buffer(req)))
 			mempool_free(req->qtcb, req->adapter->pool.qtcb_pool);
 		mempool_free(req, req->pool);
 		return;
 	}
 
-	if (likely(req->qtcb))
+	if (likely(!zfcp_fsf_req_is_status_read_buffer(req)))
 		kmem_cache_free(zfcp_fsf_qtcb_cache, req->qtcb);
 	kfree(req);
 }
@@ -393,7 +393,7 @@  static void zfcp_fsf_protstatus_eval(struct zfcp_fsf_req *req)
  */
 static void zfcp_fsf_req_complete(struct zfcp_fsf_req *req)
 {
-	if (unlikely(req->fsf_command == FSF_QTCB_UNSOLICITED_STATUS)) {
+	if (unlikely(zfcp_fsf_req_is_status_read_buffer(req))) {
 		zfcp_fsf_status_read_handler(req);
 		return;
 	}
@@ -710,7 +710,6 @@  static struct zfcp_fsf_req *zfcp_fsf_req_create(struct zfcp_qdio *qdio,
 	init_completion(&req->completion);
 
 	req->adapter = adapter;
-	req->fsf_command = fsf_cmd;
 	req->req_id = adapter->req_no;
 
 	if (likely(fsf_cmd != FSF_QTCB_UNSOLICITED_STATUS)) {
@@ -729,10 +728,10 @@  static struct zfcp_fsf_req *zfcp_fsf_req_create(struct zfcp_qdio *qdio,
 		req->qtcb->prefix.req_seq_no = adapter->fsf_req_seq_no;
 		req->qtcb->prefix.req_id = req->req_id;
 		req->qtcb->prefix.ulp_info = 26;
-		req->qtcb->prefix.qtcb_type = fsf_qtcb_type[req->fsf_command];
+		req->qtcb->prefix.qtcb_type = fsf_qtcb_type[fsf_cmd];
 		req->qtcb->prefix.qtcb_version = FSF_QTCB_CURRENT_VERSION;
 		req->qtcb->header.req_handle = req->req_id;
-		req->qtcb->header.fsf_command = req->fsf_command;
+		req->qtcb->header.fsf_command = fsf_cmd;
 	}
 
 	zfcp_qdio_req_init(adapter->qdio, &req->qdio_req, req->req_id, sbtype,
@@ -745,7 +744,6 @@  static int zfcp_fsf_req_send(struct zfcp_fsf_req *req)
 {
 	struct zfcp_adapter *adapter = req->adapter;
 	struct zfcp_qdio *qdio = adapter->qdio;
-	int with_qtcb = (req->qtcb != NULL);
 	int req_id = req->req_id;
 
 	zfcp_reqlist_add(adapter->req_list, req);
@@ -761,7 +759,7 @@  static int zfcp_fsf_req_send(struct zfcp_fsf_req *req)
 	}
 
 	/* Don't increase for unsolicited status */
-	if (with_qtcb)
+	if (!zfcp_fsf_req_is_status_read_buffer(req))
 		adapter->fsf_req_seq_no++;
 	adapter->req_no++;
 
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index 2b8c33627460..a8b53ed61c1e 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -230,7 +230,9 @@  static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
 		(struct zfcp_scsi_req_filter *)data;
 
 	/* already aborted - prevent side-effects - or not a SCSI command */
-	if (old_req->data == NULL || old_req->fsf_command != FSF_QTCB_FCP_CMND)
+	if (old_req->data == NULL ||
+	    zfcp_fsf_req_is_status_read_buffer(old_req) ||
+	    old_req->qtcb->header.fsf_command != FSF_QTCB_FCP_CMND)
 		return;
 
 	/* (tmf_scope == FCP_TMF_TGT_RESET || tmf_scope == FCP_TMF_LUN_RESET) */