On 09/12/2022 06:13, Mike Christie wrote:
Subject:
[PATCH v2 01/15] scsi: Add struct for args to execution functions
From:
Mike Christie <michael.christie@xxxxxxxxxx>
Date:
09/12/2022, 06:13
To:
john.g.garry@xxxxxxxxxx, bvanassche@xxxxxxx, mwilck@xxxxxxxx,
hch@xxxxxx, martin.petersen@xxxxxxxxxx, linux-scsi@xxxxxxxxxxxxxxx,
james.bottomley@xxxxxxxxxxxxxxxxxxxxx
CC:
Mike Christie <michael.christie@xxxxxxxxxx>
Generally I think that this looks ok, but I have a bit of a niggle about
the sense_len argument, below.
This begins to move the SCSI execution functions to use a struct for
passing in optional args. This patch adds the new struct, temporarily
converts scsi_execute and scsi_execute_req and add two helpers:
1. scsi_execute_args which takes the scsi_exec_args struct.
2. scsi_execute_cmd does not support the scsi_exec_args struct.
The next patches will convert scsi_execute and scsi_execute_req users to
the new helpers then remove scsi_execute and scsi_execute_req.
Signed-off-by: Mike Christie<michael.christie@xxxxxxxxxx>
---
...
/*
* head injection*required* here otherwise quiesce won't work
@@ -249,13 +238,14 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen))
memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len);
- if (resid)
- *resid = scmd->resid_len;
- if (sense && scmd->sense_len)
- memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
- if (sshdr)
- scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len,
- sshdr);
+ if (args.resid)
+ *args.resid = scmd->resid_len;
+ if (args.sense && scmd->sense_len)
I am not sure that you require the sense_len check as you effectively
have that same check in scsi_execute_args(), which is the only caller
which would have args.sense set. But I suppose __scsi_execute() is still
a public API (so should still check); but, by that same token, we have
no sanity check for args.sense_len value here then. Is it possible to
make __scsi_execute() non-public or move/add the check for proper
sense_len here? I'm being extra cautious about this, I suppose.
+ memcpy(args.sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
+ if (args.sshdr)
+ scsi_normalize_sense(scmd->sense_buffer,
+ scmd->sense_len, args.sshdr);
+
ret = scmd->result;
out:
blk_mq_free_request(req);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3642b8e3928b..eb960aa73b3b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
...
+
+#define scsi_execute_cmd(sdev, cmd, opf, buffer, bufflen, timeout, \
+ retries) \
+({ \
+ struct scsi_exec_args exec_args = {}; \
nit: I think that this can be static const, but no biggie
+ \
+ __scsi_execute(sdev, cmd, opf, buffer, bufflen, timeout, \
+ retries, exec_args); \
+})
+
