[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b3939ccc-0da2-9156-551b-dce6ee3e8b5a@oracle.com>
Date: Mon, 22 May 2023 17:48:50 -0500
From: michael.christie@...cle.com
To: John Garry <john.g.garry@...cle.com>,
Bart Van Assche <bvanassche@....org>,
Juergen Gross <jgross@...e.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>
Cc: linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
"James E.J. Bottomley" <jejb@...ux.ibm.com>, stable@...r.kernel.org
Subject: Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid
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 */
}
}
}
Powered by blists - more mailing lists