Hello Igor, TJ, On Fri, Jun 07, 2024 at 05:57:42PM +0000, TJ Adams wrote: > From: Igor Pylypiv <ipylypiv@xxxxxxxxxx> > > v6.2 commit 811be570a9a8 ("scsi: pm8001: Use sas_ata_device_link_abort() Do not specify kernel version (it is irrelevant), SHA1 is enough. > to handle NCQ errors") removed duplicate NCQ EH from the pm80xx driver > and started relying on libata to handle the NCQ errors. The PM8006 > controller has a special EH sequence that was added in v4.15 commit > 869ddbdcae3b ("scsi: pm80xx: corrected SATA abort handling sequence."). Do not specify kernel version (it is irrelevant), SHA1 is enough. Since the code added in 869ddbdcae3b still exists in the pm80xx driver, I think that you should mention the commits in chronological order. (Right now you mention the oldest still existing code last, which seems a bit backwards.) > The special EH sequence issues a hard reset to a drive before libata EH > has a chance to read the NCQ log page. Libata EH gets confused by empty > NCQ log page which results in HSM violation. The failed command gets > retried a few times and each time fails with the same HSM violation. > Finally, libata decides to disable NCQ due to subsequent HSM vioaltions. s/vioaltions/violations/ I'm not an expert in libsas EH, but I think that your commit message fails to explain why this change actually fixes anything. You do not mention the relationship between the code that you add pm8001_work_fn() and the existing code in pm8001_abort_task(), and the order in which the functions get executed. Does calling sas_execute_internal_abort_dev() from pm8001_work_fn() ensure that the libsas EH is never invoked? Or does it cancel the hard reset that is part of the "special EH sequence" in pm8001_abort_task() ? Wouldn't it be better if this was fixed in pm8001_abort_task() or similar instead? It appears that the code you add to pm8001_work_fn() (that has a very ugly if (pm8006)) is only there to undo or avoid the hard reset that is done in pm8001_abort_task() (which also has a very ugly if (pm8006)). Now we have this ugly if (pm8006) in two different functions... which makes my "this could be solved in a nicer way" detector go off. If this patch (as is) is really the way to go, then I think there should be a more detailed reasoning why this change is the most sensible one. Kind regards, Niklas