2023-02-15 12:38:16

by Po-Wen Kao

[permalink] [raw]
Subject: [PATCH 1/2] scsi: ufs: core: Fix mcq tag calcualtion

Transfer command descriptor is allocated in ufshcd_memory_alloc()
and referenced by transfer request descriptor with stride size
sizeof_utp_transfer_cmd_desc()
instead of
sizeof(struct utp_transfer_cmd_desc).

Consequently, computing tag by address offset should also refer to the
same stride.

Signed-off-by: Po-Wen Kao <[email protected]>
---
drivers/ufs/core/ufs-mcq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 31df052fbc41..3a27fa4b0024 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -265,7 +265,7 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba,
addr = (le64_to_cpu(cqe->command_desc_base_addr) & CQE_UCD_BA) -
hba->ucdl_dma_addr;

- return div_u64(addr, sizeof(struct utp_transfer_cmd_desc));
+ return div_u64(addr, sizeof_utp_transfer_cmd_desc(hba));
}

static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
--
2.18.0



2023-02-15 12:38:24

by Po-Wen Kao

[permalink] [raw]
Subject: [PATCH 2/2] scsi: ufs: core: Export symbol ufshcd_mcq_read_cqis()

Export symbol for driver module

Signed-off-by: Po-Wen Kao <[email protected]>
---
drivers/ufs/core/ufs-mcq.c | 1 +
include/ufs/ufshcd.h | 2 ++
2 files changed, 3 insertions(+)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 3a27fa4b0024..c97b45cd94ee 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -241,6 +241,7 @@ u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i)
{
return readl(mcq_opr_base(hba, OPR_CQIS, i) + REG_CQIS);
}
+EXPORT_SYMBOL_GPL(ufshcd_mcq_read_cqis);

void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i)
{
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index ed9e3d5addb3..57e3b9524b1b 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1243,6 +1243,8 @@ void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val);
void ufshcd_hba_stop(struct ufs_hba *hba);
void ufshcd_schedule_eh_work(struct ufs_hba *hba);
void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i);
+u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i);
+
unsigned long ufshcd_mcq_poll_cqe_nolock(struct ufs_hba *hba,
struct ufs_hw_queue *hwq);
void ufshcd_mcq_enable_esi(struct ufs_hba *hba);
--
2.18.0


2023-02-15 17:34:17

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH 1/2] scsi: ufs: core: Fix mcq tag calcualtion

On 2/15/23 04:37, Po-Wen Kao wrote:
> Transfer command descriptor is allocated in ufshcd_memory_alloc()
> and referenced by transfer request descriptor with stride size
> sizeof_utp_transfer_cmd_desc()
> instead of
> sizeof(struct utp_transfer_cmd_desc).
>
> Consequently, computing tag by address offset should also refer to the
> same stride.
>
> Signed-off-by: Po-Wen Kao <[email protected]>
> ---
> drivers/ufs/core/ufs-mcq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 31df052fbc41..3a27fa4b0024 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -265,7 +265,7 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba,
> addr = (le64_to_cpu(cqe->command_desc_base_addr) & CQE_UCD_BA) -
> hba->ucdl_dma_addr;
>
> - return div_u64(addr, sizeof(struct utp_transfer_cmd_desc));
> + return div_u64(addr, sizeof_utp_transfer_cmd_desc(hba));
> }
>
> static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,

Reviewed-by: Bart Van Assche <[email protected]>

2023-02-15 17:53:13

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/2] scsi: ufs: core: Export symbol ufshcd_mcq_read_cqis()

On Wed, Feb 15, 2023 at 08:37:46PM +0800, Po-Wen Kao wrote:
> Export symbol for driver module

Which one? We don't export symbols without an actual user.

2023-02-16 00:51:55

by Stanley Jhu

[permalink] [raw]
Subject: Re: [PATCH 1/2] scsi: ufs: core: Fix mcq tag calcualtion

On Wed, Feb 15, 2023 at 8:38 PM Po-Wen Kao <[email protected]> wrote:
>
> Transfer command descriptor is allocated in ufshcd_memory_alloc()
> and referenced by transfer request descriptor with stride size
> sizeof_utp_transfer_cmd_desc()
> instead of
> sizeof(struct utp_transfer_cmd_desc).
>
> Consequently, computing tag by address offset should also refer to the
> same stride.
>
> Signed-off-by: Po-Wen Kao <[email protected]>

Reviewed-by: Stanley Chu <[email protected]>

2023-02-16 07:41:04

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH 1/2] scsi: ufs: core: Fix mcq tag calcualtion

On Wed, Feb 15, 2023 at 08:37:45PM +0800, Po-Wen Kao wrote:
> Transfer command descriptor is allocated in ufshcd_memory_alloc()
> and referenced by transfer request descriptor with stride size
> sizeof_utp_transfer_cmd_desc()
> instead of
> sizeof(struct utp_transfer_cmd_desc).
>
> Consequently, computing tag by address offset should also refer to the
> same stride.
>
> Signed-off-by: Po-Wen Kao <[email protected]>

Fixes tag?

> ---
> drivers/ufs/core/ufs-mcq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 31df052fbc41..3a27fa4b0024 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -265,7 +265,7 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba,
> addr = (le64_to_cpu(cqe->command_desc_base_addr) & CQE_UCD_BA) -
> hba->ucdl_dma_addr;
>
> - return div_u64(addr, sizeof(struct utp_transfer_cmd_desc));
> + return div_u64(addr, sizeof_utp_transfer_cmd_desc(hba));

I think it is not a good practice to name variables after the standard
operators like sizeof(). It is confusing at its best.

How about renaming this function to get_ucd_size() or something relevant?

But the change itself LGTM!

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> }
>
> static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
> --
> 2.18.0
>

--
மணிவண்ணன் சதாசிவம்

2023-02-21 04:46:34

by Po-Wen Kao

[permalink] [raw]
Subject: Re: [PATCH 2/2] scsi: ufs: core: Export symbol ufshcd_mcq_read_cqis()

Then I will upstream together with our driver change later.