From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrian Hunter Subject: Re: [PATCH] mmc: block: delete packed command support Date: Mon, 21 Nov 2016 14:44:38 +0200 Message-ID: References: <1479722937-19551-1-git-send-email-linus.walleij@linaro.org> <2697103.lML87IM8sj@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mga06.intel.com ([134.134.136.31]:23551 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754482AbcKUMtk (ORCPT ); Mon, 21 Nov 2016 07:49:40 -0500 In-Reply-To: <2697103.lML87IM8sj@wuerfel> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Linus Walleij Cc: Arnd Bergmann , linux-mmc@vger.kernel.org, Ulf Hansson , Chunyan Zhang , Baolin Wang , Jaehoon Chung , Namjae Jeon , Maya Erez , Luca Porzio , Alex Lemberg On 21/11/16 13:11, Arnd Bergmann 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. > > 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. > >> - There is no Device Tree binding or code to mark a host as >> supporting packed read or write commands, just this flag >> in caps2, so for sure there are not any DT systems using >> it either. >> >> It has other problems as well: mmc_blk_prep_packed_list() is >> speculatively picking requests out of the request queue with >> blk_fetch_request() making the MMC/SD stack harder to convert >> to the multiqueue block layer. By this we get rid of an >> obstacle. SDHCIv4 has a feature (ADMA3) which is slightly similar to packed commands but it does not require card support. Why is it a problem for blk-mq to allow some extra requests to pick from for packing?