Hi Christoph, Thanks for the review, please check our comments below, > -----Original Message----- > From: Christoph Hellwig [mailto:hch@xxxxxxxxxxxxx] > Sent: 2017年11月23日 16:34 > To: Xinming Hu <huxm@xxxxxxxxxxx> > Cc: Linux MMC <linux-mmc@xxxxxxxxxxxxxxx>; Ulf Hansson > <ulf.hansson@xxxxxxxxxx>; Zhiyuan Yang <yangzy@xxxxxxxxxxx>; Tim Song > <songtao@xxxxxxxxxxx>; Cathy Luo <cluo@xxxxxxxxxxx>; James Cao > <jcao@xxxxxxxxxxx>; Ganapathi Bhat <gbhat@xxxxxxxxxxx>; Xu (Shanghai) > Wang <xwang4@xxxxxxxxxxx>; Bob Tan <tanbo@xxxxxxxxxxx>; Amitkumar > Karwar <akarwar@xxxxxxxxxxx> > Subject: Re: [PATCH v5 2/2] mmc: sdio support external scatter gather list > > Please just clean the API up to allways pass a struct scatterlist Per the "grep" operation in the latest kernel, the sdio command 53 read/write operation is heavily used in many device drivers, many of these caller might not need to utilize the scatter gather memory consumption benefits. on the other hand, construction of the external scatter gather table puts heavy burden on these callers. To be able to compatible with the generic user case, and also consider the memory-limited host demands, We need a trade-off here. please correct me if I have any misunderstanding. > and just expose a maximumber number of segments value. > Sorry, I don't get the idea, do you mean the "max_segs" in "[v5,1/2] mmc: API for accessing host supported maximum segment count and size" ? > Last but not least please send this along with the actual consumers of your API. > Yes, we will send the user case used in mwifiex wireless sdio driver after this changes applied. Thanks Simon > On Wed, Nov 22, 2017 at 03:53:42PM +0800, Xinming Hu wrote: > > Currently sdio device drivers need to prepare a big continuous > > physical memory for large data transfer. mmc stack will construct > > scatter/gather buffer list to make use of ADMA2. > > > > According to the sdio host controller specification version 4.00 > > chapter 1.13.2, sdio device drivers have the ability to construct > > scatter/gather DMA buffer list. In this way, they do not need to > > allocate big memory. > > > > This patch refactors current sdio command 53 wrapper > > mmc_io_rw_extended, so that it can accept external scatter/gather > > buffer list from its caller, such as the sdio device driver. This > > patch is very useful on some embedded systems where large memory > > allocation fails sometimes. It gives 10 to 15% better throughput > > performance compared to a case in which small single data transfers are > done. > > > > Signed-off-by: Xinming Hu <huxm@xxxxxxxxxxx> > > Signed-off-by: Amitkumar Karwar <akarwar@xxxxxxxxxxx> > > --- > > v4: same as v3/v2/v1 > > v5: rebased on latest codebase > > --- > > drivers/mmc/core/sdio_io.c | 22 ++++++++++++++++-- > > drivers/mmc/core/sdio_ops.c | 53 > +++++++++++++++++++++++++++++++++++++++---- > > drivers/mmc/core/sdio_ops.h | 3 ++- > > include/linux/mmc/sdio_func.h | 6 +++++ > > 4 files changed, 76 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c > > index 4806521..6046511 100644 > > --- a/drivers/mmc/core/sdio_io.c > > +++ b/drivers/mmc/core/sdio_io.c > > @@ -327,7 +327,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func > *func, int write, > > size = blocks * func->cur_blksize; > > > > ret = mmc_io_rw_extended(func->card, write, > > - func->num, addr, incr_addr, buf, > > + func->num, addr, incr_addr, false, buf, > > blocks, func->cur_blksize); > > if (ret) > > return ret; > > @@ -345,7 +345,7 @@ static int sdio_io_rw_ext_helper(struct sdio_func > > *func, int write, > > > > /* Indicate byte mode by setting "blocks" = 0 */ > > ret = mmc_io_rw_extended(func->card, write, func->num, addr, > > - incr_addr, buf, 0, size); > > + incr_addr, false, buf, 0, size); > > if (ret) > > return ret; > > > > @@ -513,6 +513,24 @@ int sdio_writesb(struct sdio_func *func, unsigned > > int addr, void *src, } EXPORT_SYMBOL_GPL(sdio_writesb); > > > > +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr, > > + struct sg_table *dst) > > +{ > > + return mmc_io_rw_extended(func->card, 0, func->num, > > + addr, 0, true, > > + dst, 0, func->cur_blksize); > > +} > > +EXPORT_SYMBOL_GPL(sdio_readsb_sg_enh); > > + > > +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr, > > + struct sg_table *src) > > +{ > > + return mmc_io_rw_extended(func->card, 1, func->num, > > + addr, 0, true, > > + src, 0, func->cur_blksize); > > +} > > +EXPORT_SYMBOL_GPL(sdio_writesb_sg_enh); > > + > > /** > > * sdio_readw - read a 16 bit integer from a SDIO function > > * @func: SDIO function to access > > diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c > > index abaaba3..2310a2a 100644 > > --- a/drivers/mmc/core/sdio_ops.c > > +++ b/drivers/mmc/core/sdio_ops.c > > @@ -115,16 +115,39 @@ int mmc_io_rw_direct(struct mmc_card *card, int > write, unsigned fn, > > return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out); > > } > > > > +/** > > + * mmc_io_rw_extended - wrapper for sdio command 53 read/write > operation > > + * @card: destination device for read/write > > + * @write: zero indcate read, non-zero indicate write. > > + * @fn: SDIO function to access > > + * @addr: address of (single byte) FIFO > > + * @incr_addr: set to 1 indicate read or write multiple bytes > > + of data to/from an IO register address that > > + increment by 1 after each operation. > > + * @sg_enhance: if set true, the caller of this function will > > + prepare scatter gather dma buffer list. > > + * @buf: buffer that contains the data to write. if sg_enhance > > + is set, it point to SG dma buffer list. > > + * @blocks: number of blocks of data transfer. > > + if set zero, indicate byte mode > > + if set non-zero, indicate block mode > > + if sg_enhance is set, this parameter will not be used. > > + * @blksz: block size for block mode data transfer. > > + * > > + */ > > int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, > > - unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz) > > + unsigned int addr, int incr_addr, bool sg_enhance, > > + void *buf, unsigned int blocks, unsigned int blksz) > > { > > struct mmc_request mrq = {}; > > struct mmc_command cmd = {}; > > struct mmc_data data = {}; > > struct scatterlist sg, *sg_ptr; > > struct sg_table sgtable; > > + struct sg_table *sgtable_external; > > unsigned int nents, left_size, i; > > unsigned int seg_size = card->host->max_seg_size; > > + unsigned int sg_blocks = 0; > > > > WARN_ON(blksz == 0); > > > > @@ -140,16 +163,35 @@ int mmc_io_rw_extended(struct mmc_card *card, > int write, unsigned fn, > > cmd.arg |= fn << 28; > > cmd.arg |= incr_addr ? 0x04000000 : 0x00000000; > > cmd.arg |= addr << 9; > > + cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC; > > + > > + data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ; > > + data.blksz = blksz; > > + > > + if (sg_enhance) { > > + sgtable_external = buf; > > + > > + for_each_sg(sgtable_external->sgl, sg_ptr, > > + sgtable_external->nents, i) { > > + if (sg_ptr->length > card->host->max_seg_size) > > + return -EINVAL; > > + sg_blocks += DIV_ROUND_UP(sg_ptr->length, blksz); > > + } > > + > > + cmd.arg |= 0x08000000 | sg_blocks; > > + data.blocks = sg_blocks; > > + data.sg = sgtable_external->sgl; > > + data.sg_len = sgtable_external->nents; > > + goto mmc_data_req; > > + } > > + > > if (blocks == 0) > > cmd.arg |= (blksz == 512) ? 0 : blksz; /* byte mode */ > > else > > cmd.arg |= 0x08000000 | blocks; /* block mode */ > > - cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC; > > > > - data.blksz = blksz; > > /* Code in host drivers/fwk assumes that "blocks" always is >=1 */ > > data.blocks = blocks ? blocks : 1; > > - data.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ; > > > > left_size = data.blksz * data.blocks; > > nents = DIV_ROUND_UP(left_size, seg_size); @@ -172,11 +214,12 @@ > int > > mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, > > sg_init_one(&sg, buf, left_size); > > } > > > > +mmc_data_req: > > mmc_set_data_timeout(&data, card); > > > > mmc_wait_for_req(card->host, &mrq); > > > > - if (nents > 1) > > + if (!sg_enhance && nents > 1) > > sg_free_table(&sgtable); > > > > if (cmd.error) > > diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h > > index 96945ca..2d75100 100644 > > --- a/drivers/mmc/core/sdio_ops.h > > +++ b/drivers/mmc/core/sdio_ops.h > > @@ -23,7 +23,8 @@ > > int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn, > > unsigned addr, u8 in, u8* out); > > int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, > > - unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz); > > + unsigned int addr, int incr_addr, bool sg_enhance, > > + void *buf, unsigned int blocks, unsigned int blksz); > > int sdio_reset(struct mmc_host *host); unsigned int > > mmc_align_data_size(struct mmc_card *card, unsigned int sz); void > > sdio_irq_work(struct work_struct *work); diff --git > > a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h index > > 72d4de4..d36ba47 100644 > > --- a/include/linux/mmc/sdio_func.h > > +++ b/include/linux/mmc/sdio_func.h > > @@ -14,6 +14,7 @@ > > > > #include <linux/device.h> > > #include <linux/mod_devicetable.h> > > +#include <linux/scatterlist.h> > > > > #include <linux/mmc/pm.h> > > > > @@ -136,6 +137,11 @@ extern int sdio_memcpy_fromio(struct sdio_func > > *func, void *dst, extern int sdio_readsb(struct sdio_func *func, void *dst, > > unsigned int addr, int count); > > > > +int sdio_readsb_sg_enh(struct sdio_func *func, unsigned int addr, > > + struct sg_table *dst); > > +int sdio_writesb_sg_enh(struct sdio_func *func, unsigned int addr, > > + struct sg_table *src); > > + > > extern void sdio_writeb(struct sdio_func *func, u8 b, > > unsigned int addr, int *err_ret); > > extern void sdio_writew(struct sdio_func *func, u16 b, > > -- > > 1.9.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-mmc" > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo > > info at http://vger.kernel.org/majordomo-info.html > ---end quoted text--- -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
