Re: [PATCH] mmc: block: delete packed command support

[Jaehoon Chung <jh80.chung@samsung.com>]

On 11/21/2016 11:23 PM, Alex Lemberg wrote:
> Hi,
> 
> 
> On 11/21/16, 1:11 PM, "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
>> On Monday, November 21, 2016 11:08:57 AM CET Linus Walleij wrote:
>>> I've had it with this code now.
>>>
>>> The packed command support is a complex hurdle in the MMC/SD block
>>> layer, around 500+ lines of code which was introduced in 2013 in
>>> commits
>>>
>>> ce39f9d17c14 ("mmc: support packed write command for eMMC4.5 devices")
>>> abd9ac144947 ("mmc: add packed command feature of eMMC4.5")
>>>
>>> ...and since then it has been rotting. The original author of the
>>> code has disappeared from the community and the mail address is
>>> bouncing.
>>>
>>> For the code to be exercised the host must flag that it supports
>>> packed commands, so in mmc_blk_prep_packed_list() which is called for
>>> every single request, the following construction appears:
>>>
>>> u8 max_packed_rw = 0;
>>>
>>> if ((rq_data_dir(cur) == WRITE) &&
>>>     mmc_host_packed_wr(card->host))
>>>         max_packed_rw = card->ext_csd.max_packed_writes;
>>>
>>> if (max_packed_rw == 0)
>>>     goto no_packed;
>>>
>>> This has the following logical deductions:
>>>
>>> - Only WRITE commands can really be packed, so the solution is
>>>   only half-done: we support packed WRITE but not packed READ.
>>>   The packed command support has not been finalized by supporting
>>>   reads in three years!
>>
>> Packed reads don't make a lot of sense, there is very little
>> for an MMC to optimize in reads that it can't already do without
>> the packing. For writes, packing makes could be an important
>> performance optimization, if the eMMC supports it.
>>
>> I've added Luca Porzio  and Alex Lemberg to Cc. I think they
>> are subscribed to the list already, but it would be good to
>> get some estimate from them too about how common the packed
>> write support is on existing hardware from their respective
>> employers before we kill it off.
> 
> Correct, in general there is no value in using packed for Read.
> But I can’t say this for all existing flash management solution.
> The eMMC spec allows to use it for Read as well.

As i know, when packed command had implemented, early eMMC had the firmware problem
for Packed Read operation. but so I can't say Packed Read doesn't have the benefit for performance.
But Packed Write command can see the benefit for performance.

> 
>>
>> If none of Samsung/Micron/Sandisk are currently shipping
>> devices that support eMMC-4.5 packed commands but don't
>> support the eMMC-5.1 command queuing (which IIRC is a more
>> general way to achieve the same), we probably don't need to
>> worry about it too much. 
> 
> Please note that even by having eMMC-5.1 device,
> not all chipset vendors are having HW/SW CMDQ support.
> So they might be using packed commands instead.
> 
>>
>>> - mmc_host_packed_wr() is just a static inline that checks
>>>   host->caps2 & MMC_CAP2_PACKED_WR. The problem with this is
>>>   that NO upstream host sets this capability flag! No driver
>>>   in the kernel is using it, and we can't test it. Packed
>>>   command may be supported in out-of-tree code, but I doubt
>>>   it. I doubt that the code is even working anymore due to
>>>   other refactorings in the MMC block layer, who would
>>>   notice if patches affecting it broke packed commands?
>>>   No one.
>>
>> This is a very good indication that it's not really used.
>> It would be useful to also check out the Android AOSP
>> kernel tree to see if it's in there, if anything important
>> supports packed commands, it's probably in there.
> 
> As far as I can say from reviewing the mobile (Android)
> platforms kernel source (from different vendors), 
> many of them are enabling Packed Commands.

Actually, some shipping Samsung devices with eMMC4.5 might be used packed command.
(For Android/Tizen OS and ARTIK boards)

Best Regards,
Jaehoon Chung

> 
>>
>> 	Arnd
>