From: Can Guo <cang@codeaurora.org>
To: Bean Huo <huobean@gmail.com>
Cc: Avri Altman <Avri.Altman@wdc.com>,
daejun7.park@samsung.com, Greg KH <gregkh@linuxfoundation.org>,
jejb@linux.ibm.com, martin.petersen@oracle.com,
asutoshd@codeaurora.org, stanley.chu@mediatek.com,
bvanassche@acm.org, ALIM AKHTAR <alim.akhtar@samsung.com>,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
Sung-Jun Park <sungjun07.park@samsung.com>,
yongmyung lee <ymhungry.lee@samsung.com>,
Jinyoung CHOI <j-young.choi@samsung.com>,
BoRam Shin <boram.shin@samsung.com>,
SEUNGUK SHIN <seunguk.shin@samsung.com>
Subject: Re: [PATCH v19 3/3] scsi: ufs: Prepare HPB read for cached sub-region
Date: Wed, 10 Feb 2021 13:32:17 +0800 [thread overview]
Message-ID: <bbfc6fa427b7b1972bf55e77458bf809@codeaurora.org> (raw)
In-Reply-To: <4355a1ebffea1da290389c57eb2b42df75990c6e.camel@gmail.com>
On 2021-02-09 22:21, Bean Huo wrote:
> On Tue, 2021-02-09 at 13:25 +0000, Avri Altman wrote:
>> >
>> >
>> > > > > + put_unaligned_be64(ppn, &cdb[6]);
>> > > >
>> > > > You are assuming the HPB entries read out by "HPB Read Buffer"
>> > > > cmd
>> > > > are
>> > > > in Little
>> > > > Endian, which is why you are using put_unaligned_be64 here.
>> > > > However,
>> > > > this assumption
>> > > > is not right for all the other flash vendors - HPB entries read
>> > > > out
>> > > > by
>> > > > "HPB Read Buffer"
>> > > > cmd may come in Big Endian, if so, their random read
>> > > > performance are
>> > > > screwed.
>> > >
>> > > For this question, it is very hard to make a correct format since
>> > > the
>> > > Spec doesn't give a clear definition. Should we have a default
>> > > format,
>> > > if there is conflict, and then add quirk or add a vendor-specific
>> > > table?
>> > >
>> > > Hi Avri
>> > > Do you have a good idea?
>> >
>> > I don't know. Better let Daejun answer this.
>> > This was working for me for both Galaxy S20 (Exynos) as well as
>> > Xiaomi Mi10
>> > (8250).
>>
>> As for the endianity issue -
>> I don't think that any fix is needed in the hpb driver.
>> It is readily seen that the ppn from get_ppn, and the one in the upiu
>> cdb (upiu trace) are identical.
>> Therefore, if an issue exist, it is IMHO a device issue.
>>
>> kworker/u16:10-315 [001] d..2 62.283264: ufshpb_get_ppn: Avri
>> ppn 480d2f8244c21abd
>> kworker/u16:10-315 [001] d..2 62.283336: ufshcd_upiu: v:1.10
>> send: T:62283314922, HDR:014000000000000000000000,
>> CDB:8800002ddaac480d2f8244c21abd0100, D:
>>
>> Again, verified on both gs20 (exynos) and mi10 (8250).
>> Thanks,
>> Avri
>
>
> Hi Avri,
> Your testing method is no problem, the current problem is in function
> ufshpb_get_ppn().
>
>
> +static u64 ufshpb_get_ppn(struct ufshpb_lu *hpb,
> + struct ufshpb_map_ctx *mctx, int pos, int
> *error)
> +{
> + u64 *ppn_table;
> + struct page *page;
> + int index, offset;
> +
> + index = pos / (PAGE_SIZE / HPB_ENTRY_SIZE);
> + offset = pos % (PAGE_SIZE / HPB_ENTRY_SIZE);
> +
> + page = mctx->m_page[index];
> + if (unlikely(!page)) {
> + *error = -ENOMEM;
> + dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> + "error. cannot find page in mctx\n");
> + return 0;
> + }
> +
> + ppn_table = page_address(page);
> + if (unlikely(!ppn_table)) {
> + *error = -ENOMEM;
> + dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> + "error. cannot get ppn_table\n");
> + return 0;
> + }
> +
> + return ppn_table[offset];
> +}
>
>
> Say, the UFS device outputs the L2P entry in big-endian, which means
> the most significant byte of an L2P entry will be output firstly, then
> the less significant byte..., let's take an example of one L2P entry:
>
> 0x 12 34 56 78 90 12 34 56
>
> 0x12 is the most significant byte, will be store in the lowest address
> in the L2P cache.
>
> eg,
>
> F0000008: 1234 5678 9012 3456
>
> In the ARM based system, If we use "return ppn_table[offset]",
> the original L2P entry 0x1234 5678 9012 3456, will be converted to
> 0x5634 1290 7856 3412. then use put_unaligned_be64(), UFS receive
> unexpected L2P entry(L2P entry miss).
>
> If the UFS output L2P entry in the big-endian, this is a problem.
>
>
> For the UFS outputs L2P entry in little-endian, no problem,
>
> Because of the L2P entry in the memory:
>
> F0000008: 5634 1290 7856 3412
>
> After return ppn_table[offset], L2P entry will be correct L2P entry:
>
> 0x1234567890123456. then use put_unaligned_be64(), UFS can receive
> expected L2P etnry(L2P entry hit).
>
>
> we need to figure out which way is the JEDEC recommended L2P entry
> output endianness. otherwise, two methods co-exist in HPB driver, there
> will confuse customer.
> If you have a look at the JEDEC HPB 2.0, seems the big-endian is
> correct way. This need you and Daejun to double check inside your
> company.
>
Bean is right, finally you know what I was saying...
We need to fix it before move on - all the UFS3.1 HPB parts which I
tested
over the last few weeks are screwed due to this... I don't care
where/how
you want to get it fixed in next version.
In my case, which may not be a valid fix, I simply hack the code as
below
and it works for me.
- put_unaligned_be64(ppn, &cdb[6]);
+ memcpy(&cdb[6], &ppn, sizeof(u64));
Thanks,
Can Guo.
> thanks,
> Bean
next prev parent reply other threads:[~2021-02-10 5:33 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p6>
2021-01-29 5:28 ` [PATCH v19 0/3] scsi: ufs: Add Host Performance Booster Support Daejun Park
[not found] ` <CGME20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p3>
2021-01-29 5:30 ` [PATCH v19 2/3] scsi: ufs: L2P map management for HPB read Daejun Park
2021-02-05 11:15 ` Bean Huo
2021-02-05 11:42 ` Avri Altman
2021-02-05 11:54 ` Bean Huo
[not found] ` <CGME20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p2>
2021-02-08 8:00 ` Daejun Park
2021-02-06 7:23 ` Can Guo
2021-02-06 9:14 ` Bean Huo
2021-02-06 19:17 ` Avri Altman
2021-02-08 7:21 ` Can Guo
[not found] ` <CGME20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p5>
2021-01-29 5:30 ` [PATCH v19 3/3] scsi: ufs: Prepare HPB read for cached sub-region Daejun Park
2021-02-05 3:29 ` Can Guo
2021-02-05 12:35 ` Bean Huo
2021-02-05 14:06 ` Avri Altman
2021-02-05 15:08 ` Bean Huo
2021-02-07 7:36 ` Can Guo
2021-02-07 10:44 ` Bean Huo
2021-02-09 13:25 ` Avri Altman
2021-02-09 14:21 ` Bean Huo
2021-02-10 5:32 ` Can Guo [this message]
2021-02-08 8:16 ` Bean Huo
2021-02-08 9:58 ` Can Guo
2021-02-10 9:36 ` Avri Altman
2021-02-08 8:03 ` Re: [PATCH v19 2/3] scsi: ufs: L2P map management for HPB read Daejun Park
2021-02-08 8:34 ` Can Guo
[not found] ` <CGME20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p1>
2021-01-29 5:29 ` [PATCH v19 1/3] scsi: ufs: Introduce HPB feature Daejun Park
2021-02-02 7:46 ` Avri Altman
2021-02-02 11:12 ` Greg KH
2021-02-08 8:53 ` Re: [PATCH v19 2/3] scsi: ufs: L2P map management for HPB read Daejun Park
2021-02-08 9:23 ` Can Guo
[not found] ` <CGME20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p8>
2021-02-09 1:27 ` Daejun Park
2021-02-09 2:28 ` Can Guo
2021-02-08 8:01 ` Daejun Park
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bbfc6fa427b7b1972bf55e77458bf809@codeaurora.org \
--to=cang@codeaurora.org \
--cc=Avri.Altman@wdc.com \
--cc=alim.akhtar@samsung.com \
--cc=asutoshd@codeaurora.org \
--cc=boram.shin@samsung.com \
--cc=bvanassche@acm.org \
--cc=daejun7.park@samsung.com \
--cc=gregkh@linuxfoundation.org \
--cc=huobean@gmail.com \
--cc=j-young.choi@samsung.com \
--cc=jejb@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=seunguk.shin@samsung.com \
--cc=stanley.chu@mediatek.com \
--cc=sungjun07.park@samsung.com \
--cc=ymhungry.lee@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).