Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[michael.christie@xxxxxxxxxx]

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 */

                }
        }

}