| Message ID | 20190617030349.26415-10-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | use sg helper to operate scatterlist | expand |
On Mon, Jun 17, 2019 at 11:03:42AM +0800, Ming Lei wrote: > Use the scatterlist iterators and remove direct indexing of the > scatterlist array. > > This way allows us to pre-allocate one small scatterlist, which can be > chained with one runtime allocated scatterlist if the pre-allocated one > isn't enough for the whole request. The scatterlist handling here looks completely bogus, it really needs to stop using the index and switch to proper sg_next-based iteration.
On Mon, Jun 17, 2019 at 10:27:06AM +0200, Christoph Hellwig wrote: > On Mon, Jun 17, 2019 at 11:03:42AM +0800, Ming Lei wrote: > > Use the scatterlist iterators and remove direct indexing of the > > scatterlist array. > > > > This way allows us to pre-allocate one small scatterlist, which can be > > chained with one runtime allocated scatterlist if the pre-allocated one > > isn't enough for the whole request. > > The scatterlist handling here looks completely bogus, it really > needs to stop using the index and switch to proper sg_next-based > iteration. Yeah, I agree, looks we may convert in the following way by storing current 'sg', and the 'offset' already stores the actual offset on the current 'sg'. diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d7895608..861b4cc4562e 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -32,7 +32,7 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer, unsigned int buflen, struct scsi_cmnd *srb, - unsigned int *index, + struct scatterlist **cur_sg, unsigned int *offset, enum xfer_buf_dir dir) { @@ -60,18 +60,18 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer, * each page has to be kmap()'ed separately. */ } else { - struct scatterlist *sg = - (struct scatterlist *)scsi_sglist(srb) - + *index; + struct scatterlist *sg = *cur_sg ?: + (struct scatterlist *)scsi_sglist(srb); + /* * This loop handles a single s-g list entry, which may * include multiple pages. Find the initial page structure * and the starting offset within the page, and update - * the *offset and *index values for the next loop. + * the *offset values for the next loop. */ cnt = 0; - while (cnt < buflen && *index < scsi_sg_count(srb)) { + while (cnt < buflen && sg) { struct page *page = sg_page(sg) + ((sg->offset + *offset) >> PAGE_SHIFT); unsigned int poff = (sg->offset + *offset) & @@ -85,8 +85,7 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer, } else { /* Transfer continues to next s-g entry */ *offset = 0; - ++*index; - ++sg; + sg = sg_next(sg); } while (sglen > 0) { @@ -120,9 +119,10 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer, void rtsx_stor_set_xfer_buf(unsigned char *buffer, unsigned int buflen, struct scsi_cmnd *srb) { - unsigned int index = 0, offset = 0; + unsigned int offset = 0; + struct scatterlist *sg = NULL; - rtsx_stor_access_xfer_buf(buffer, buflen, srb, &index, &offset, + rtsx_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset, TO_XFER_BUF); if (buflen < scsi_bufflen(srb)) scsi_set_resid(srb, scsi_bufflen(srb) - buflen); @@ -131,9 +131,10 @@ void rtsx_stor_set_xfer_buf(unsigned char *buffer, void rtsx_stor_get_xfer_buf(unsigned char *buffer, unsigned int buflen, struct scsi_cmnd *srb) { - unsigned int index = 0, offset = 0; + unsigned int offset = 0; + struct scatterlist *sg = NULL; - rtsx_stor_access_xfer_buf(buffer, buflen, srb, &index, &offset, + rtsx_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset, FROM_XFER_BUF); if (buflen < scsi_bufflen(srb)) scsi_set_resid(srb, scsi_bufflen(srb) - buflen); diff --git a/drivers/staging/rts5208/spi.c b/drivers/staging/rts5208/spi.c index f1e9e80044ed..6ad4278de531 100644 --- a/drivers/staging/rts5208/spi.c +++ b/drivers/staging/rts5208/spi.c @@ -554,7 +554,8 @@ int spi_read_flash_id(struct scsi_cmnd *srb, struct rtsx_chip *chip) int spi_read_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip) { int retval; - unsigned int index = 0, offset = 0; + unsigned int offset = 0; + struct scatterlist *sg = NULL; u8 ins, slow_read; u32 addr; u16 len; @@ -631,7 +632,7 @@ int spi_read_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip) return STATUS_FAIL; } - rtsx_stor_access_xfer_buf(buf, pagelen, srb, &index, &offset, + rtsx_stor_access_xfer_buf(buf, pagelen, srb, &sg, &offset, TO_XFER_BUF); addr += pagelen; @@ -651,7 +652,8 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip) u32 addr; u16 len; u8 *buf; - unsigned int index = 0, offset = 0; + unsigned int offset = 0; + struct scatterlist *sg = NULL; spi_set_err_code(chip, SPI_NO_ERR); @@ -679,7 +681,7 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip) return STATUS_FAIL; } - rtsx_stor_access_xfer_buf(buf, 1, srb, &index, &offset, + rtsx_stor_access_xfer_buf(buf, 1, srb, &sg, &offset, FROM_XFER_BUF); rtsx_init_cmd(chip); @@ -722,7 +724,7 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip) return STATUS_ERROR; while (len) { - rtsx_stor_access_xfer_buf(buf, 1, srb, &index, &offset, + rtsx_stor_access_xfer_buf(buf, 1, srb, &sg, &offset, FROM_XFER_BUF); rtsx_init_cmd(chip); @@ -788,7 +790,7 @@ int spi_write_flash(struct scsi_cmnd *srb, struct rtsx_chip *chip) rtsx_send_cmd_no_wait(chip); - rtsx_stor_access_xfer_buf(buf, pagelen, srb, &index, + rtsx_stor_access_xfer_buf(buf, pagelen, srb, &sg, &offset, FROM_XFER_BUF); retval = rtsx_transfer_data(chip, 0, buf, pagelen, 0, Thanks, Ming
> + struct scatterlist *sg = *cur_sg ?: > + (struct scatterlist *)scsi_sglist(srb); > + No need for the cast here. And I have to say I hate that GNU C non-standard shortshut in ? :. Why not simply: struct scatterlist *sg = *cur_sg; if (!sg) sg = scsi_sglist(srb); Which is a little more verbose, but much more readabe. The rest of the patch looks fine to me.
diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d7895608..407c9079b052 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -63,6 +63,8 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer, struct scatterlist *sg = (struct scatterlist *)scsi_sglist(srb) + *index; + if (sg_is_chain(sg)) + sg = sg_chain_ptr(sg); /* * This loop handles a single s-g list entry, which may @@ -86,7 +88,7 @@ unsigned int rtsx_stor_access_xfer_buf(unsigned char *buffer, /* Transfer continues to next s-g entry */ *offset = 0; ++*index; - ++sg; + sg = sg_next(sg); } while (sglen > 0) {
Use the scatterlist iterators and remove direct indexing of the scatterlist array. This way allows us to pre-allocate one small scatterlist, which can be chained with one runtime allocated scatterlist if the pre-allocated one isn't enough for the whole request. Cc: Kim Bradley <kim.jamie.bradley@gmail.com> Cc: devel@driverdev.osuosl.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/staging/rts5208/rtsx_transport.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)