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

[Arnd Bergmann <arnd@arndb.de>]

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.

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. 

> - 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.

	Arnd