On 10/19/22 18:42, John Garry wrote: > On 19/10/2022 06:04, Damien Le Moal wrote: >> On 10/19/22 14:03, Damien Le Moal wrote: >>> On 10/18/22 22:24, John Garry wrote: >>>> Hi guys, >>>> >>>> In the hisi_sas driver there are times in which we need to issue an ATA >>>> software reset. For this we use hisi_sas_softreset_ata_disk() -> >>>> sas_execute_ata_cmd() -> sas_execute_tmf(), which uses libsas "slow >>>> task" mechanism to issue the command. >>> Something is wrong here... The reset command sent by that function is >>> for ATAPI (DEVICE RESET command). There is no device reset command for >>> SATA disks following the ACS standard. > > Yeah, that looks wrong. > >>> >>> So hisi_sas_softreset_ata_disk() seems totally bogus to me, unless you >>> have a CD/DVD drive connected to the HBA:) > > Sure > >>> >>> This is why the softreset function is a port operation defined by LLDs. >>> How you reset the device depends on the adapter. E.g., for AHCI, you >>> need to send a host2device FIS with the software reset bit set. > > This would be quite a standard method, right? The TF part should be standard. Would need to dig in SATA-IO specs to check. How the TF/FIS should be issued/polled for is definitely dependent on the adapter itself I think, exactly like issuing a qc is. > >> See: ahci_do_softreset() for AHCI. > > For ahci_do_softreset(), do you just implicitly use ATA_CMD_NOP as the > command? To be frank, it is the first time I really look at the reset code :) I need to read SATA-IO specs to understand why it is doing things like that. > > For hisi_sas, maybe ATA_CMD_DEV_RESET is silently ignored when issued > for a SATA disk, but having SRST set/unset still takes effect (and that > is how it still works). I need to check on that. Yes, it may be that having the ATA_SRST bit set causes the device to ignore the command field, hence you never saw any problem. Still feels terribly wrong to use an ATAPI command for an ATA disk. ATAPI device reset command code is 0x08 which is not defined as a command code in ACS. > > Thanks, > John -- Damien Le Moal Western Digital Research
