[PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Avri Altman]

Replace manual offset calculations for response_upiu and prd_table in
ufshcd_init_lrb() with pre-calculated offsets already stored in the
utp_transfer_req_desc structure. The pre-calculated offsets are set
differently in ufshcd_host_memory_configure() based on the
UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
access.

Fixes: 26f968d7de82 ("scsi: ufs: Introduce UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk")
Cc: stable@vger.kernel.org
Signed-off-by: Avri Altman <avri.altman@wdc.com>

---
Changes in v2:
 - add Fixes: and Cc: stable tags
 - fix kernel test robot warning about type mismatch by using le16_to_cpu
---
 drivers/ufs/core/ufshcd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8ea5a82503a9..85251c176ef7 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2919,9 +2919,8 @@ static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
 	struct utp_transfer_req_desc *utrdlp = hba->utrdl_base_addr;
 	dma_addr_t cmd_desc_element_addr = hba->ucdl_dma_addr +
 		i * ufshcd_get_ucd_size(hba);
-	u16 response_offset = offsetof(struct utp_transfer_cmd_desc,
-				       response_upiu);
-	u16 prdt_offset = offsetof(struct utp_transfer_cmd_desc, prd_table);
+	u16 response_offset = le16_to_cpu(utrdlp[i].response_upiu_offset);
+	u16 prdt_offset = le16_to_cpu(utrdlp[i].prd_table_offset);
 
 	lrb->utr_descriptor_ptr = utrdlp + i;
 	lrb->utrd_dma_addr = hba->utrdl_dma_addr +
-- 
2.25.1

Re: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Bart Van Assche]

On 9/9/24 9:45 PM, Avri Altman wrote:
> Replace manual offset calculations for response_upiu and prd_table in
> ufshcd_init_lrb() with pre-calculated offsets already stored in the
> utp_transfer_req_desc structure. The pre-calculated offsets are set
> differently in ufshcd_host_memory_configure() based on the
> UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> access.

With which host controllers has this patch been tested?

Thanks,

Bart.

RE: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Avri Altman]

> On 9/9/24 9:45 PM, Avri Altman wrote:
> > Replace manual offset calculations for response_upiu and prd_table in
> > ufshcd_init_lrb() with pre-calculated offsets already stored in the
> > utp_transfer_req_desc structure. The pre-calculated offsets are set
> > differently in ufshcd_host_memory_configure() based on the
> > UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> > access.
> 
> With which host controllers has this patch been tested?
Qualcomm RB5 platform.
I guess I'll be needing help with testing it on an exynos platforms?

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

RE: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Avri Altman]

+Alim

> > On 9/9/24 9:45 PM, Avri Altman wrote:
> > > Replace manual offset calculations for response_upiu and prd_table
> > > in
> > > ufshcd_init_lrb() with pre-calculated offsets already stored in the
> > > utp_transfer_req_desc structure. The pre-calculated offsets are set
> > > differently in ufshcd_host_memory_configure() based on the
> > > UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> > > access.
> >
> > With which host controllers has this patch been tested?
> Qualcomm RB5 platform.
> I guess I'll be needing help with testing it on an exynos platforms?
Would appreciate any help testing it on Exynos.

Thanks,
Avri

> 
> Thanks,
> Avri
> 
> >
> > Thanks,
> >
> > Bart.

Re: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Bart Van Assche]

On 9/9/24 9:45 PM, Avri Altman wrote:
> Replace manual offset calculations for response_upiu and prd_table in
> ufshcd_init_lrb() with pre-calculated offsets already stored in the
> utp_transfer_req_desc structure. The pre-calculated offsets are set
> differently in ufshcd_host_memory_configure() based on the
> UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> access.
> 
> Fixes: 26f968d7de82 ("scsi: ufs: Introduce UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk")
> Cc: stable@vger.kernel.org
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> 
> ---
> Changes in v2:
>   - add Fixes: and Cc: stable tags
>   - fix kernel test robot warning about type mismatch by using le16_to_cpu
> ---
>   drivers/ufs/core/ufshcd.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8ea5a82503a9..85251c176ef7 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2919,9 +2919,8 @@ static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
>   	struct utp_transfer_req_desc *utrdlp = hba->utrdl_base_addr;
>   	dma_addr_t cmd_desc_element_addr = hba->ucdl_dma_addr +
>   		i * ufshcd_get_ucd_size(hba);
> -	u16 response_offset = offsetof(struct utp_transfer_cmd_desc,
> -				       response_upiu);
> -	u16 prdt_offset = offsetof(struct utp_transfer_cmd_desc, prd_table);
> +	u16 response_offset = le16_to_cpu(utrdlp[i].response_upiu_offset);
> +	u16 prdt_offset = le16_to_cpu(utrdlp[i].prd_table_offset);
>   
>   	lrb->utr_descriptor_ptr = utrdlp + i;
>   	lrb->utrd_dma_addr = hba->utrdl_dma_addr +

Please always Cc the author of the original patch when posting a
candidate fix.

Alim, since the upstream kernel code seems to work fine with Exynos UFS
host controllers, is the description of UFSHCD_QUIRK_PRDT_BYTE_GRAN
perhaps wrong? I'm referring to the following description:

	/*
	 * This quirk needs to be enabled if the host controller regards
	 * resolution of the values of PRDTO and PRDTL in UTRD as byte.
	 */
	UFSHCD_QUIRK_PRDT_BYTE_GRAN			= 1 << 9,

Thanks,

Bart.

RE: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Avri Altman]

+Daejun

Thanks,
Avri 
> On 9/9/24 9:45 PM, Avri Altman wrote:
> > Replace manual offset calculations for response_upiu and prd_table in
> > ufshcd_init_lrb() with pre-calculated offsets already stored in the
> > utp_transfer_req_desc structure. The pre-calculated offsets are set
> > differently in ufshcd_host_memory_configure() based on the
> > UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> > access.
> >
> > Fixes: 26f968d7de82 ("scsi: ufs: Introduce
> UFSHCD_QUIRK_PRDT_BYTE_GRAN
> > quirk")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> >
> > ---
> > Changes in v2:
> >   - add Fixes: and Cc: stable tags
> >   - fix kernel test robot warning about type mismatch by using
> > le16_to_cpu
> > ---
> >   drivers/ufs/core/ufshcd.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 8ea5a82503a9..85251c176ef7 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -2919,9 +2919,8 @@ static void ufshcd_init_lrb(struct ufs_hba *hba,
> struct ufshcd_lrb *lrb, int i)
> >       struct utp_transfer_req_desc *utrdlp = hba->utrdl_base_addr;
> >       dma_addr_t cmd_desc_element_addr = hba->ucdl_dma_addr +
> >               i * ufshcd_get_ucd_size(hba);
> > -     u16 response_offset = offsetof(struct utp_transfer_cmd_desc,
> > -                                    response_upiu);
> > -     u16 prdt_offset = offsetof(struct utp_transfer_cmd_desc, prd_table);
> > +     u16 response_offset = le16_to_cpu(utrdlp[i].response_upiu_offset);
> > +     u16 prdt_offset = le16_to_cpu(utrdlp[i].prd_table_offset);
> >
> >       lrb->utr_descriptor_ptr = utrdlp + i;
> >       lrb->utrd_dma_addr = hba->utrdl_dma_addr +
> 
> Please always Cc the author of the original patch when posting a candidate fix.
> 
> Alim, since the upstream kernel code seems to work fine with Exynos UFS host
> controllers, is the description of UFSHCD_QUIRK_PRDT_BYTE_GRAN perhaps
> wrong? I'm referring to the following description:
> 
>         /*
>          * This quirk needs to be enabled if the host controller regards
>          * resolution of the values of PRDTO and PRDTL in UTRD as byte.
>          */
>         UFSHCD_QUIRK_PRDT_BYTE_GRAN                     = 1 << 9,
> 
> Thanks,
> 
> Bart.

RE: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Avri Altman]

> +Daejun
> 
> Thanks,
> Avri
Bart - How do you want to proceed with this fix?

Thanks,
Avri

> > On 9/9/24 9:45 PM, Avri Altman wrote:
> > > Replace manual offset calculations for response_upiu and prd_table
> > > in
> > > ufshcd_init_lrb() with pre-calculated offsets already stored in the
> > > utp_transfer_req_desc structure. The pre-calculated offsets are set
> > > differently in ufshcd_host_memory_configure() based on the
> > > UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> > > access.
> > >
> > > Fixes: 26f968d7de82 ("scsi: ufs: Introduce
> > UFSHCD_QUIRK_PRDT_BYTE_GRAN
> > > quirk")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> > >
> > > ---
> > > Changes in v2:
> > >   - add Fixes: and Cc: stable tags
> > >   - fix kernel test robot warning about type mismatch by using
> > > le16_to_cpu
> > > ---
> > >   drivers/ufs/core/ufshcd.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > > index 8ea5a82503a9..85251c176ef7 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -2919,9 +2919,8 @@ static void ufshcd_init_lrb(struct ufs_hba
> > > *hba,
> > struct ufshcd_lrb *lrb, int i)
> > >       struct utp_transfer_req_desc *utrdlp = hba->utrdl_base_addr;
> > >       dma_addr_t cmd_desc_element_addr = hba->ucdl_dma_addr +
> > >               i * ufshcd_get_ucd_size(hba);
> > > -     u16 response_offset = offsetof(struct utp_transfer_cmd_desc,
> > > -                                    response_upiu);
> > > -     u16 prdt_offset = offsetof(struct utp_transfer_cmd_desc, prd_table);
> > > +     u16 response_offset = le16_to_cpu(utrdlp[i].response_upiu_offset);
> > > +     u16 prdt_offset = le16_to_cpu(utrdlp[i].prd_table_offset);
> > >
> > >       lrb->utr_descriptor_ptr = utrdlp + i;
> > >       lrb->utrd_dma_addr = hba->utrdl_dma_addr +
> >
> > Please always Cc the author of the original patch when posting a candidate
> fix.
> >
> > Alim, since the upstream kernel code seems to work fine with Exynos
> > UFS host controllers, is the description of
> > UFSHCD_QUIRK_PRDT_BYTE_GRAN perhaps wrong? I'm referring to the
> following description:
> >
> >         /*
> >          * This quirk needs to be enabled if the host controller regards
> >          * resolution of the values of PRDTO and PRDTL in UTRD as byte.
> >          */
> >         UFSHCD_QUIRK_PRDT_BYTE_GRAN                     = 1 << 9,
> >
> > Thanks,
> >
> > Bart.

Re: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Bart Van Assche]

On 10/1/24 12:19 AM, Avri Altman wrote:
> Bart - How do you want to proceed with this fix?

Since this patch affects the Exynos UFSHCI, I think we need feedback
from the maintainers of the Exynos UFSHCI code.

Personally I'm hesitant to proceed with this patch. As you may know the
android-mainline kernel tracks the upstream kernel very closely.
Currently that kernel branch is only one week behind Linus' master
branch. The android-mainline kernel boots fine on Pixel 6 devices. These
devices have an Exynos UFS host controller. Since this patch modifies
the behavior for the Exynos UFS host controller there is a potential
that it will break the support for that controller.

Thanks,

Bart.

Re: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Bart Van Assche]

On 10/1/24 12:19 AM, Avri Altman wrote:
> Bart - How do you want to proceed with this fix?

As one can see here [1], if this patch is applied on top of the
android-mainline kernel branch (close to v6.12-rc1) then all presubmit
tests pass. This includes booting the kernel on a Pixel 6 device that
has an Exynos UFS host controller. So I'm fine with this patch.

[1] https://android-review.googlesource.com/c/kernel/common/+/3291741

Thanks,

Bart.

Re: [PATCH v2] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb

[Martin K. Petersen]

On Tue, 10 Sep 2024 07:45:43 +0300, Avri Altman wrote:

> Replace manual offset calculations for response_upiu and prd_table in
> ufshcd_init_lrb() with pre-calculated offsets already stored in the
> utp_transfer_req_desc structure. The pre-calculated offsets are set
> differently in ufshcd_host_memory_configure() based on the
> UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk, ensuring correct alignment and
> access.
> 
> [...]

Applied to 6.12/scsi-fixes, thanks!

[1/1] scsi: ufs: Use pre-calculated offsets in ufshcd_init_lrb
      https://git.kernel.org/mkp/scsi/c/d5130c5a0932

-- 
Martin K. Petersen	Oracle Linux Engineering