[RFC PATCH] usb: storage: Add blockbuffer ptr to info struct of sddr09 driver

[Jake Rice]

Hi all,

This patch updates the sddr09 driver to allocate a reusable block
buffer. Unfortunately, I don't have access to the SDDR-00 hardware
(which I know is pretty ancient), so I'm requesting testing from anyone who does.
Please let me now if the patch causes any issues or improves performance.

Best,
Jake

---
Currently, upon every write the block buffer is allocated and freed which is
computationally expensive. With this implementation, a buffer pointer
is added as a member to the info struct and allocated when the card
information is read. The buffer is freed during desconstruction if
necessary.

Signed-off-by: Jake Rice <ja...@jakerice.dev>
---
drivers/usb/storage/sddr09.c | 60 +++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/storage/sddr09.c b/drivers/usb/storage/sddr09.c
index e66b920e99e2..1d75b1a88c17 100644
--- a/drivers/usb/storage/sddr09.c
+++ b/drivers/usb/storage/sddr09.c
@@ -255,6 +255,7 @@ struct sddr09_card_info {
int *pba_to_lba; /* physical to logical map */
int lbact; /* number of available pages */
int flags;
+ unsigned char *blockbuffer;
#define SDDR09_WP 1 /* write protected */
};

@@ -850,7 +851,7 @@ sddr09_find_unused_pba(struct sddr09_card_info *info, unsigned int lba) {
static int
sddr09_write_lba(struct us_data *us, unsigned int lba,
unsigned int page, unsigned int pages,
- unsigned char *ptr, unsigned char *blockbuffer) {
+ unsigned char *ptr) {

struct sddr09_card_info *info = (struct sddr09_card_info *) us->extra;
unsigned long address;
@@ -890,13 +891,13 @@ sddr09_write_lba(struct us_data *us, unsigned int lba,
/* read old contents */
address = (pba << (info->pageshift + info->blockshift));
result = sddr09_read22(us, address>>1, info->blocksize,
- info->pageshift, blockbuffer, 0);
+ info->pageshift, info->blockbuffer, 0);
if (result)
return result;

/* check old contents and fill lba */
for (i = 0; i < info->blocksize; i++) {
- bptr = blockbuffer + i*pagelen;
+ bptr = info->blockbuffer + i*pagelen;
cptr = bptr + info->pagesize;
nand_compute_ecc(bptr, ecc);
if (!nand_compare_ecc(cptr+13, ecc)) {
@@ -917,7 +918,7 @@ sddr09_write_lba(struct us_data *us, unsigned int lba,
/* copy in new stuff and compute ECC */
xptr = ptr;
for (i = page; i < page+pages; i++) {
- bptr = blockbuffer + i*pagelen;
+ bptr = info->blockbuffer + i*pagelen;
cptr = bptr + info->pagesize;
memcpy(bptr, xptr, info->pagesize);
xptr += info->pagesize;
@@ -930,7 +931,7 @@ sddr09_write_lba(struct us_data *us, unsigned int lba,
usb_stor_dbg(us, "Rewrite PBA %d (LBA %d)\n", pba, lba);

result = sddr09_write_inplace(us, address>>1, info->blocksize,
- info->pageshift, blockbuffer, 0);
+ info->pageshift, info->blockbuffer, 0);

usb_stor_dbg(us, "sddr09_write_inplace returns %d\n", result);

@@ -961,8 +962,6 @@ sddr09_write_data(struct us_data *us,

struct sddr09_card_info *info = (struct sddr09_card_info *) us->extra;
unsigned int lba, maxlba, page, pages;
- unsigned int pagelen, blocklen;
- unsigned char *blockbuffer;
unsigned char *buffer;
unsigned int len, offset;
struct scatterlist *sg;
@@ -975,21 +974,6 @@ sddr09_write_data(struct us_data *us,
if (lba >= maxlba)
return -EIO;

- /*
- * blockbuffer is used for reading in the old data, overwriting
- * with the new data, and performing ECC calculations
- */
-
- /*
- * TODO: instead of doing kmalloc/kfree for each write,
- * add a bufferpointer to the info structure
- */
-
- pagelen = (1 << info->pageshift) + (1 << CONTROL_SHIFT);
- blocklen = (pagelen << info->blockshift);
- blockbuffer = kmalloc(blocklen, GFP_NOIO);
- if (!blockbuffer)
- return -ENOMEM;

/*
* Since we don't write the user data directly to the device,
@@ -999,10 +983,8 @@ sddr09_write_data(struct us_data *us,

len = min_t(unsigned int, sectors, info->blocksize) * info->pagesize;
buffer = kmalloc(len, GFP_NOIO);
- if (!buffer) {
- kfree(blockbuffer);
+ if (!buffer)
return -ENOMEM;
- }

result = 0;
offset = 0;
@@ -1028,7 +1010,7 @@ sddr09_write_data(struct us_data *us,
&sg, &offset, FROM_XFER_BUF);

result = sddr09_write_lba(us, lba, page, pages,
- buffer, blockbuffer);
+ buffer);
if (result)
break;

@@ -1037,9 +1019,6 @@ sddr09_write_data(struct us_data *us,
sectors -= pages;
}

- kfree(buffer);
- kfree(blockbuffer);
-
return result;
}

@@ -1405,6 +1384,7 @@ sddr09_card_info_destructor(void *extra) {

kfree(info->lba_to_pba);
kfree(info->pba_to_lba);
+ kfree(info->blockbuffer);
}

static int
@@ -1585,6 +1565,8 @@ static int sddr09_transport(struct scsi_cmnd *srb, struct us_data *us)

if (srb->cmnd[0] == READ_CAPACITY) {
const struct nand_flash_dev *cardinfo;
+ unsigned int pagelen;
+ unsigned int blocklen;

sddr09_get_wp(us, info); /* read WP bit */

@@ -1603,6 +1585,26 @@ static int sddr09_transport(struct scsi_cmnd *srb, struct us_data *us)
info->blockshift = cardinfo->blockshift;
info->blocksize = (1 << info->blockshift);
info->blockmask = info->blocksize - 1;
+
+ pagelen = (1 << info->pageshift) + (1 << CONTROL_SHIFT);
+ blocklen = (pagelen << info->blockshift);
+
+ /*
+ * If it has already been allocated and is changed
+ * (i.e. a new card is inserted), we want to free
+ * it and reallocate it with the updates size
+ */
+
+ kfree(info->blockbuffer);
+
+ /*
+ * blockbuffer is used for reading in the old data, overwriting
+ * with the new data, and performing ECC calculations
+ */
+
+ info->blockbuffer = kmalloc(blocklen, GFP_NOIO);
+ if (!info->blockbuffer)
+ return -ENOMEM;

// map initialization, must follow get_cardinfo()
if (sddr09_read_map(us)) {
--
2.34.1

[Alan Stern]

On Tue, May 06, 2025 at 03:15:31PM -0400, Jake Rice wrote:
> Hi all,
>
> This patch updates the sddr09 driver to allocate a reusable block
> buffer. Unfortunately, I don't have access to the SDDR-00 hardware
> (which I know is pretty ancient), so I'm requesting testing from anyone who does.
> Please let me now if the patch causes any issues or improves performance.
>
> Best,
> Jake
>
> ---
> Currently, upon every write the block buffer is allocated and freed which is
> computationally expensive. With this implementation, a buffer pointer
> is added as a member to the info struct and allocated when the card
> information is read. The buffer is freed during desconstruction if
> necessary.

This isn't a high-priority sort of thing. SDDR-09 is indeed very old
and likely quite slow (by comparison with current hardware); the time
required for the memory allocations and deallocations probably doesn't
have much effect on the overall I/O rate.

Still, I guess it doesn't hurt to do this. However, I don't have time
to review the patch properly right now, and I don't have the hardware to
test it with.

Alan Stern

[Greg KH]

On Tue, May 06, 2025 at 03:15:31PM -0400, Jake Rice wrote:

> Hi all,
>
> This patch updates the sddr09 driver to allocate a reusable block
> buffer. Unfortunately, I don't have access to the SDDR-00 hardware
> (which I know is pretty ancient), so I'm requesting testing from anyone who does.
> Please let me now if the patch causes any issues or improves performance.
>
> Best,
> Jake
>
> ---
> Currently, upon every write the block buffer is allocated and freed which is
> computationally expensive. With this implementation, a buffer pointer
> is added as a member to the info struct and allocated when the card
> information is read. The buffer is freed during desconstruction if
> necessary.

As Alan said, this is really slow hardware so I doubt allocating/free
the buffer will even be noticeable. Why make this change at all if you
don't have access to the hardware to test it?

thanks,

greg k-h

[Jake Rice]

Alan and Greg,

I apologize, I was learning about the usb subsystem/usb drivers and
saw the TODO for this and figured it was a quick implementation. I
should have waited until I got the hardware before submitting the
change. It won't happen again.

In any case, thank you, I appreciate the feedback. I ordered the
device to test this. If I find a significant performance improvement,
I will re-submit.

Best,
Jake