[PATCH v1] usb: storage: sddr09: move write buffers into info struct

[PATCH v1] usb: storage: sddr09: move write buffers into info struct

[Alex Tran]

Changelog:
- Moved allocation of buffers ('blockbuffer', 'buffer') in 
  'sddr09_write_data' into info struct and freeing into 
  'sddr09_card_info_destructor' so that the operations are only 
  performed once.
- 'buffer' length is now size of a full block instead of being 
  dependent on sectors.

Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
---
 drivers/usb/storage/sddr09.c | 100 ++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 49 deletions(-)

diff --git a/drivers/usb/storage/sddr09.c b/drivers/usb/storage/sddr09.c
index e66b920e9..27c318266 100644
--- a/drivers/usb/storage/sddr09.c
+++ b/drivers/usb/storage/sddr09.c
@@ -255,6 +255,8 @@ struct sddr09_card_info {
 	int		*pba_to_lba;	/* physical to logical map */
 	int		lbact;		/* number of available pages */
 	int		flags;
+	unsigned char	*buffer; /* staging buffer */
+	unsigned char	*blockbuffer; /* bounce buffer */
 #define	SDDR09_WP	1		/* write protected */
 };
 
@@ -847,11 +849,9 @@ sddr09_find_unused_pba(struct sddr09_card_info *info, unsigned int lba) {
 	return 0;
 }
 
-static int
-sddr09_write_lba(struct us_data *us, unsigned int lba,
-		 unsigned int page, unsigned int pages,
-		 unsigned char *ptr, unsigned char *blockbuffer) {
-
+static int sddr09_write_lba(struct us_data *us, unsigned int lba,
+			    unsigned int page, unsigned int pages)
+{
 	struct sddr09_card_info *info = (struct sddr09_card_info *) us->extra;
 	unsigned long address;
 	unsigned int pba, lbap;
@@ -890,13 +890,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)) {
@@ -915,9 +915,9 @@ sddr09_write_lba(struct us_data *us, unsigned int lba,
 	}
 
 	/* copy in new stuff and compute ECC */
-	xptr = ptr;
+	xptr = info->buffer;
 	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 +930,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,9 +961,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;
 	int result;
@@ -975,35 +972,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,
-	 * we have to create a bounce buffer and move the data a piece
-	 * at a time between the bounce buffer and the actual transfer buffer.
-	 */
-
-	len = min_t(unsigned int, sectors, info->blocksize) * info->pagesize;
-	buffer = kmalloc(len, GFP_NOIO);
-	if (!buffer) {
-		kfree(blockbuffer);
-		return -ENOMEM;
-	}
-
 	result = 0;
 	offset = 0;
 	sg = NULL;
@@ -1024,11 +992,10 @@ sddr09_write_data(struct us_data *us,
 		}
 
 		/* Get the data from the transfer buffer */
-		usb_stor_access_xfer_buf(buffer, len, us->srb,
-				&sg, &offset, FROM_XFER_BUF);
+		usb_stor_access_xfer_buf(info->buffer, len, us->srb, &sg,
+					 &offset, FROM_XFER_BUF);
 
-		result = sddr09_write_lba(us, lba, page, pages,
-				buffer, blockbuffer);
+		result = sddr09_write_lba(us, lba, page, pages);
 		if (result)
 			break;
 
@@ -1037,9 +1004,6 @@ sddr09_write_data(struct us_data *us,
 		sectors -= pages;
 	}
 
-	kfree(buffer);
-	kfree(blockbuffer);
-
 	return result;
 }
 
@@ -1193,6 +1157,36 @@ sddr09_get_cardinfo(struct us_data *us, unsigned char flags) {
 	return cardinfo;
 }
 
+static int sddr09_init_card_buffers(struct us_data *us)
+{
+	struct sddr09_card_info *info = (struct sddr09_card_info *)us->extra;
+	unsigned int pagelen, blocklen, len;
+
+	/*
+	 * blockbuffer is used for reading in the old data, overwriting
+	 * with the new data, and performing ECC calculations
+	 */
+	pagelen = (1 << info->pageshift) + (1 << CONTROL_SHIFT);
+	blocklen = (pagelen << info->blockshift);
+	info->blockbuffer = kmalloc(blocklen, GFP_NOIO);
+	if (!info->blockbuffer)
+		return -ENOMEM;
+
+	/*
+	 * Since we don't write the user data directly to the device,
+	 * we have to create a bounce buffer and move the data a piece
+	 * at a time between the bounce buffer and the actual transfer buffer.
+	 */
+	len = info->blocksize * info->pagesize;
+	info->buffer = kmalloc(len, GFP_NOIO);
+	if (!info->buffer) {
+		kfree(info->blockbuffer);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 static int
 sddr09_read_map(struct us_data *us) {
 
@@ -1403,6 +1397,8 @@ sddr09_card_info_destructor(void *extra) {
 	if (!info)
 		return;
 
+	kfree(info->buffer);
+	kfree(info->blockbuffer);
 	kfree(info->lba_to_pba);
 	kfree(info->pba_to_lba);
 }
@@ -1592,6 +1588,8 @@ static int sddr09_transport(struct scsi_cmnd *srb, struct us_data *us)
 		if (!cardinfo) {
 			/* probably no media */
 		init_error:
+			kfree(info->buffer);
+			kfree(info->blockbuffer);
 			sensekey = 0x02;	/* not ready */
 			sensecode = 0x3a;	/* medium not present */
 			return USB_STOR_TRANSPORT_FAILED;
@@ -1604,6 +1602,10 @@ static int sddr09_transport(struct scsi_cmnd *srb, struct us_data *us)
 		info->blocksize = (1 << info->blockshift);
 		info->blockmask = info->blocksize - 1;
 
+		if (sddr09_init_card_buffers(us)) {
+			goto init_error;
+		}
+
 		// map initialization, must follow get_cardinfo()
 		if (sddr09_read_map(us)) {
 			/* probably out of memory */
-- 
2.51.0

Re: [PATCH v1] usb: storage: sddr09: move write buffers into info struct

[Greg KH]

On Sun, Aug 31, 2025 at 12:22:47PM -0700, Alex Tran wrote:
> Changelog:

No need for that line here, right?

> - Moved allocation of buffers ('blockbuffer', 'buffer') in 
>   'sddr09_write_data' into info struct and freeing into 
>   'sddr09_card_info_destructor' so that the operations are only 
>   performed once.

Trailing spaces :(

> - 'buffer' length is now size of a full block instead of being 
>   dependent on sectors.

But why are you doing any of this?

Please take a look at the kernel documentation for how to write a good
changelog text.

And do you have this device to test that your changes here work
properly?

thanks,

greg k-h

Re: [PATCH v1] usb: storage: sddr09: move write buffers into info struct

[Oliver Neukum]

Hi,

On 8/31/25 21:22, Alex Tran wrote:
> +static int sddr09_init_card_buffers(struct us_data *us)
> +{
> +	struct sddr09_card_info *info = (struct sddr09_card_info *)us->extra;
> +	unsigned int pagelen, blocklen, len;
> +
> +	/*
> +	 * blockbuffer is used for reading in the old data, overwriting
> +	 * with the new data, and performing ECC calculations
> +	 */
> +	pagelen = (1 << info->pageshift) + (1 << CONTROL_SHIFT);
> +	blocklen = (pagelen << info->blockshift);
> +	info->blockbuffer = kmalloc(blocklen, GFP_NOIO);

there is no reason for GFP_NOIO under these circumstances. Please
use GFP_KERNEL.

	Regards
		Oliver