On 12/9/22 10:37, Mike Christie wrote:
On 12/9/22 11:15 AM, Mike Christie wrote:
On 12/9/22 4:40 AM, John Garry wrote:
* 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.
Do people want the BUILD_BUG_ON we have now or a WARN/BUG?
If we move to a single check in __scsi_execute or some non-macro wrapper
around it then we have to do a WARN/BUG. We do the macro approach now
so we can do the BUILD_BUG_ON.
Maybe we have to switch to a WARN/BUG.
It looks like some compilers don't like:
const struct scsi_exec_args exec_args = {
.sshdr = &sshdr,
};
scsi_execute_args(.... exec_args);
and will hit the:
#define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout, \
retries, args) \
({ \
BUILD_BUG_ON(args.sense && \
args.sense_len != SCSI_SENSE_BUFFERSIZE); \
because the args's sense and sense_len are not cleared yet.
My understanding is that the __scsi_execute() macro was introduced to
prevent that every single scsi_execute() caller would have to be
modified. I'm fine with removing the BUILD_BUG_ON(sense_len !=
SCSI_SENSE_BUFFER_SIZE) check and replacing it with a WARN_ON_ONCE()
statement, e.g. inside __scsi_execute().
Thanks,
Bart.
