On 5/22/23 10:54 AM, John Garry wrote:
On 22/05/2023 14:31, Bart Van Assche wrote:
On 5/22/23 02:55, John Garry wrote:
On 19/05/2023 18:39, Bart Van Assche wrote:
*args->resid = scmd->resid_len; - if (args->sense) - memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); + if (args->sense) { + *args->sense = scmd->sense_buffer; + scmd->sense_buffer = NULL;
I think that you will agree that this is not a good pattern to follow. We cannot have SCSI core allocating the sense buffer but a driver freeing it.
Why not? Something similar can happen anywhere in the kernel anywhere reference counting is used.
Sure, but you are not using ref counting. If you could use ref counting then it would be better.
What about killing the sense buffer arg and doing a callback?
For the retries patchset, one issue we had was scsi_execute_cmd callers for the most part just wanted to check different sense/asc/ascq codes. However, there are several places that want to do something more advanced and that's specific to their use. For them, Martin W and I had talked about a callback.
For this sense case, the callback can look at the sense buffer scsi-ml creates for all cmds in scsi_mq_init_request, and just copy the values it wants to copy like in ata_task_ioctl. Something like
scsi_check_passthrough()
...
if (scsi_cmnd->failures->check_failure) scsi_cmnd->failures->check_failure(scmd, &sshdr)
ata_task_ioctl() struct scsi_failure *failures = { .check_failure = ata_task_check_failure, .check_args = args, ....
bool ata_task_check_failure(struct scsi_cmnd *cmd) { u8 *args = scsi_cmnd->failures->check_args; u8 *sense = cmd->sensebuf;
.....
if (scsi_sense_valid(&sshdr)) {/* sense data available */ u8 *desc = sensebuf + 8;
/* If we set cc then ATA pass-through will cause a * check condition even if no error. Filter that. */ if (cmd_result & SAM_STAT_CHECK_CONDITION) { if (sshdr.sense_key == RECOVERED_ERROR && sshdr.asc == 0 && sshdr.ascq == 0x1d) cmd_result &= ~SAM_STAT_CHECK_CONDITION; }
/* Send userspace ATA registers */ if (sensebuf[0] == 0x72 && /* format is "descriptor" */ desc[0] == 0x09) {/* code is "ATA Descriptor" */ args[0] = desc[13]; /* status */ args[1] = desc[3]; /* error */ args[2] = desc[5]; /* sector count (0:7) */ args[3] = desc[7]; /* lbal */ args[4] = desc[9]; /* lbam */ args[5] = desc[11]; /* lbah */ args[6] = desc[12]; /* select */
} }
}