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
