linux-kernel - Re: [PATCH RFC v3 2/7] ata: libata-scsi: Add ata_internal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07028dac-d6cc-d707-db08-b92c365a6220@opensource.wdc.com>
Date:   Fri, 28 Oct 2022 07:35:17 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     John Garry <john.garry@...wei.com>, Hannes Reinecke <hare@...e.de>,
        jejb@...ux.ibm.com, martin.petersen@...cle.com, bvanassche@....org,
        hch@....de, ming.lei@...hat.com, niklas.cassel@....com
Cc:     axboe@...nel.dk, jinpu.wang@...ud.ionos.com,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-ide@...r.kernel.org, linux-scsi@...r.kernel.org,
        linuxarm@...wei.com, john.garry2@...l.dcu.ie
Subject: Re: [PATCH RFC v3 2/7] ata: libata-scsi: Add
 ata_internal_queuecommand()

On 10/28/22 02:23, John Garry wrote:
> On 27/10/2022 14:02, Hannes Reinecke wrote:
>>>>>   /**
>>>>>    *    ata_scsi_slave_config - Set SCSI device attributes
>>>>>    *    @sdev: SCSI device to examine
>>>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>>>> index 8938b584520f..f09c5dca16ce 100644
>>>>> --- a/include/linux/libata.h
>>>>> +++ b/include/linux/libata.h
>>>>> @@ -1141,6 +1141,8 @@ extern int ata_std_bios_param(struct 
>>>>> scsi_device *sdev,
>>>>>                     sector_t capacity, int geom[]);
>>>>>   extern void ata_scsi_unlock_native_capacity(struct scsi_device 
>>>>> *sdev);
>>>>>   extern int ata_scsi_slave_config(struct scsi_device *sdev);
>>>>> +extern int ata_internal_queuecommand(struct Scsi_Host *shost,
>>>>> +                struct scsi_cmnd *scmd);
>>>>>   extern void ata_scsi_slave_destroy(struct scsi_device *sdev);
>>>>>   extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
>>>>>                          int queue_depth);
>>>>> @@ -1391,7 +1393,8 @@ extern const struct attribute_group 
>>>>> *ata_common_sdev_groups[];
>>>>>       .slave_destroy        = ata_scsi_slave_destroy,    \
>>>>>       .bios_param        = ata_std_bios_param,        \
>>>>>       .unlock_native_capacity    = ata_scsi_unlock_native_capacity,\
>>>>> -    .max_sectors        = ATA_MAX_SECTORS_LBA48
>>>>> +    .max_sectors        = ATA_MAX_SECTORS_LBA48,\
>>>>> +    .reserved_queuecommand = ata_internal_queuecommand
>>>>>   #define ATA_SUBBASE_SHT(drv_name)                \
>>>>>       __ATA_BASE_SHT(drv_name),                \
>>>>
>>>
>>
>> But that means we can't use it before the SCSI host is initialized; some 
>> HBAs require to send commands before the host can be initialized properly.
> 
> At what stage do you want to send these commands? The tags for the shost 
> are not setup until scsi_add_host() -> scsi_mq_setup_tags() is called, 
> so can't expect blk-mq to manage reserved tags before then.
> 
> If you are required to send commands prior to scsi_add_host(), then I 
> suppose the low-level driver still needs to manage tags until the shost 
> is ready. I guess that some very simple scheme can be used, like always 
> use tag 0, since most probe is done serially per-host. But that's not a 
> case which I have had to deal with yet.

In libata case, ata_dev_configure() will cause a lot of
ata_exec_internal_sg() calls for IDENTIFY and various READ LOG commands.
That is all done with non-ncq commands, which means that we do not require
a hw tag. But given that you are changing ata_exec_internal_sg() to call
alloc_request + blk_execute_rq_nowait(), how would these work without a
tag, at least a soft one ? Or we would need to keep the current code to
use ata_qc_issue() directly for probe time ? That will look very ugly...

> 
> Thanks,
> John

-- 
Damien Le Moal
Western Digital Research