On Thu, Feb 17, 2022 at 2:30 PM Damien Le Moal
<damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
>
> In the pm8001_chip_sata_req() and pm80xx_chip_sata_req() functions, all
> tasks with a DMA direction of DMA_NONE (no data transfer) are
> initialized using the ATAP value 0x04. However, NCQ NON DATA commands,
> while being DMA_NONE commands are NCQ commands and need to be
> initialized using the value 0x07 for ATAP, similarly to other NCQ
> commands.
>
> Make sure that NCQ NON DATA command tasks are initialized similarly to
> other NCQ commands by also testing the task "use_ncq" field in addition
> to the DMA direction. While at it, reorganize the code into a chain of
> if - else if - else to avoid useless affectations and debug messages.
>
> Fixes: dbf9bfe61571 ("[SCSI] pm8001: add SAS/SATA HBA driver")
> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Jack Wang <jinpu.wang@xxxxxxxxx>
thx!
> ---
> drivers/scsi/pm8001/pm8001_hwi.c | 14 +++++++-------
> drivers/scsi/pm8001/pm80xx_hwi.c | 13 ++++++-------
> 2 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index e20a1d4db026..c0215a35a7b5 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -4265,22 +4265,22 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> u32 opc = OPC_INB_SATA_HOST_OPSTART;
> memset(&sata_cmd, 0, sizeof(sata_cmd));
> circularQ = &pm8001_ha->inbnd_q_tbl[0];
> - if (task->data_dir == DMA_NONE) {
> +
> + if (task->data_dir == DMA_NONE && !task->ata_task.use_ncq) {
> ATAP = 0x04; /* no data*/
> pm8001_dbg(pm8001_ha, IO, "no data\n");
> } else if (likely(!task->ata_task.device_control_reg_update)) {
> - if (task->ata_task.dma_xfer) {
> + if (task->ata_task.use_ncq &&
> + dev->sata_dev.class != ATA_DEV_ATAPI) {
> + ATAP = 0x07; /* FPDMA */
> + pm8001_dbg(pm8001_ha, IO, "FPDMA\n");
> + } else if (task->ata_task.dma_xfer) {
> ATAP = 0x06; /* DMA */
> pm8001_dbg(pm8001_ha, IO, "DMA\n");
> } else {
> ATAP = 0x05; /* PIO*/
> pm8001_dbg(pm8001_ha, IO, "PIO\n");
> }
> - if (task->ata_task.use_ncq &&
> - dev->sata_dev.class != ATA_DEV_ATAPI) {
> - ATAP = 0x07; /* FPDMA */
> - pm8001_dbg(pm8001_ha, IO, "FPDMA\n");
> - }
> }
> if (task->ata_task.use_ncq && pm8001_get_ncq_tag(task, &hdr_tag)) {
> task->ata_task.fis.sector_count |= (u8) (hdr_tag << 3);
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 60c305f987b5..3deb89b11d2f 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -4546,22 +4546,21 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> q_index = (u32) (cpu_id) % (pm8001_ha->max_q_num);
> circularQ = &pm8001_ha->inbnd_q_tbl[q_index];
>
> - if (task->data_dir == DMA_NONE) {
> + if (task->data_dir == DMA_NONE && !task->ata_task.use_ncq) {
> ATAP = 0x04; /* no data*/
> pm8001_dbg(pm8001_ha, IO, "no data\n");
> } else if (likely(!task->ata_task.device_control_reg_update)) {
> - if (task->ata_task.dma_xfer) {
> + if (task->ata_task.use_ncq &&
> + dev->sata_dev.class != ATA_DEV_ATAPI) {
> + ATAP = 0x07; /* FPDMA */
> + pm8001_dbg(pm8001_ha, IO, "FPDMA\n");
> + } else if (task->ata_task.dma_xfer) {
> ATAP = 0x06; /* DMA */
> pm8001_dbg(pm8001_ha, IO, "DMA\n");
> } else {
> ATAP = 0x05; /* PIO*/
> pm8001_dbg(pm8001_ha, IO, "PIO\n");
> }
> - if (task->ata_task.use_ncq &&
> - dev->sata_dev.class != ATA_DEV_ATAPI) {
> - ATAP = 0x07; /* FPDMA */
> - pm8001_dbg(pm8001_ha, IO, "FPDMA\n");
> - }
> }
> if (task->ata_task.use_ncq && pm8001_get_ncq_tag(task, &hdr_tag)) {
> task->ata_task.fis.sector_count |= (u8) (hdr_tag << 3);
> --
> 2.34.1
>
