[PATCH 0/2] usb: small dma allocation fixes

[PATCH 0/2] usb: small dma allocation fixes

[Denis Orlov]

Make sure dma buffers are both properly allocated and freed (they were
not in a few places).

Denis Orlov (2):
  usb: storage: fix missing calls to free()
  usb: make sure dma buffers are properly allocated

 drivers/usb/core/usb.c          | 16 ++++---
 drivers/usb/storage/transport.c | 74 +++++++++++++++++++--------------
 drivers/usb/storage/usb.c       | 30 ++++++++-----
 3 files changed, 71 insertions(+), 49 deletions(-)

-- 
2.41.0

[PATCH 1/2] usb: storage: fix missing calls to free()

[Denis Orlov]

Memory allocated with xzalloc() was not actually being freed in a few
functions, resulting in memory leaks.

Signed-off-by: Denis Orlov <denorl2009@gmail.com>
---
 drivers/usb/storage/usb.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index dda7131960..6cdcc348e4 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -169,7 +169,7 @@ static int read_capacity_16(struct us_blk_dev *usb_blkdev)
 
 	if (ret < 0) {
 		dev_warn(dev, "Read Capacity(16) failed\n");
-		return ret;
+		goto fail;
 	}
 
 	/* Note this is logical, not physical sector size */
@@ -181,13 +181,17 @@ static int read_capacity_16(struct us_blk_dev *usb_blkdev)
 
 	if ((data[12] & 1) == 1) {
 		dev_warn(dev, "Protection unsupported\n");
-		return -ENOTSUPP;
+		ret = -ENOTSUPP;
+		goto fail;
 	}
 
 	usb_blkdev->blk.blockbits = SECTOR_SHIFT;
 	usb_blkdev->blk.num_blocks = lba + 1;
 
-	return sector_size;
+	ret = sector_size;
+fail:
+	free(data);
+	return ret;
 }
 
 static int read_capacity_10(struct us_blk_dev *usb_blkdev)
@@ -208,7 +212,7 @@ static int read_capacity_10(struct us_blk_dev *usb_blkdev)
 
 	if (ret < 0) {
 		dev_warn(dev, "Read Capacity(10) failed\n");
-		return ret;
+		goto fail;
 	}
 
 	sector_size = be32_to_cpu(data[1]);
@@ -223,7 +227,10 @@ static int read_capacity_10(struct us_blk_dev *usb_blkdev)
 	usb_blkdev->blk.num_blocks = lba + 1;
 	usb_blkdev->blk.blockbits = SECTOR_SHIFT;
 
-	return SECTOR_SIZE;
+	ret = SECTOR_SIZE;
+fail:
+	free(data);
+	return ret;
 }
 
 static int usb_stor_io_16(struct us_blk_dev *usb_blkdev, u8 opcode,
-- 
2.41.0

[PATCH 2/2] usb: make sure dma buffers are properly allocated

[Denis Orlov]

When we are doing dma_map/unmap we will end up clearing caches. As we
can only do that with the granularity of a cache line, we must ensure
that the corresponding buffers are properly aligned, as otherwise we may
accidentally overwrite some data that happens to reside in the same
cache line. This is exactly what dma_alloc() is for, so use that for
buffers which we are going to map for dma.

Signed-off-by: Denis Orlov <denorl2009@gmail.com>
---
 drivers/usb/core/usb.c          | 16 ++++---
 drivers/usb/storage/transport.c | 74 +++++++++++++++++++--------------
 drivers/usb/storage/usb.c       | 17 ++++----
 3 files changed, 61 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 178ddbbca5..62d9f11146 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -964,15 +964,15 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid,
  */
 int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 {
-	unsigned char mybuf[USB_BUFSIZ];
 	unsigned char *tbuf;
-	int err;
+	int err = 0;
 	unsigned int u, idx;
 
 	if (size <= 0 || !buf || !index)
 		return -1;
+
+	tbuf = dma_alloc(USB_BUFSIZ);
 	buf[0] = 0;
-	tbuf = &mybuf[0];
 
 	/* get langid for strings if it's not yet known */
 	if (!dev->have_langid) {
@@ -980,10 +980,12 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 		if (err < 0) {
 			dev_dbg(&dev->dev, "error getting string descriptor 0 " \
 				   "(error=%lx)\n", dev->status);
-			return -1;
+			err = -1;
+			goto fail;
 		} else if (tbuf[0] < 4) {
 			pr_debug("string descriptor 0 too short\n");
-			return -1;
+			err = -1;
+			goto fail;
 		} else {
 			dev->have_langid = -1;
 			dev->string_langid = tbuf[2] | (tbuf[3] << 8);
@@ -996,7 +998,7 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 
 	err = usb_string_sub(dev, dev->string_langid, index, tbuf);
 	if (err < 0)
-		return err;
+		goto fail;
 
 	size--;		/* leave room for trailing NULL char in output buffer */
 	for (idx = 0, u = 2; u < err; u += 2) {
@@ -1009,6 +1011,8 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 	}
 	buf[idx] = 0;
 	err = idx;
+fail:
+	dma_free(tbuf);
 	return err;
 }
 
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index ad347dfce5..be3b18dc66 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -86,39 +86,44 @@ int usb_stor_Bulk_transport(struct us_blk_dev *usb_blkdev,
 {
 	struct us_data *us = usb_blkdev->us;
 	struct device *dev = &us->pusb_dev->dev;
-	struct bulk_cb_wrap cbw;
-	struct bulk_cs_wrap csw;
+	struct bulk_cb_wrap *cbw;
+	struct bulk_cs_wrap *csw;
 	int actlen, data_actlen;
 	int result;
 	unsigned int residue;
 	unsigned int pipein = usb_rcvbulkpipe(us->pusb_dev, us->recv_bulk_ep);
 	unsigned int pipeout = usb_sndbulkpipe(us->pusb_dev, us->send_bulk_ep);
 	int dir_in = US_DIRECTION(cmd[0]);
+	int ret = 0;
+
+	cbw = dma_alloc(sizeof(*cbw));
+	csw = dma_alloc(sizeof(*csw));
 
 	/* set up the command wrapper */
-	cbw.Signature = cpu_to_le32(US_BULK_CB_SIGN);
-	cbw.DataTransferLength = cpu_to_le32(datalen);
-	cbw.Flags = (dir_in ? US_BULK_FLAG_IN : US_BULK_FLAG_OUT);
-	cbw.Tag = ++cbw_tag;
-	cbw.Lun = usb_blkdev->lun;
-	cbw.Length = cmdlen;
+	cbw->Signature = cpu_to_le32(US_BULK_CB_SIGN);
+	cbw->DataTransferLength = cpu_to_le32(datalen);
+	cbw->Flags = (dir_in ? US_BULK_FLAG_IN : US_BULK_FLAG_OUT);
+	cbw->Tag = ++cbw_tag;
+	cbw->Lun = usb_blkdev->lun;
+	cbw->Length = cmdlen;
 
 	/* copy the command payload */
-	memset(cbw.CDB, 0, sizeof(cbw.CDB));
-	memcpy(cbw.CDB, cmd, cbw.Length);
+	memset(cbw->CDB, 0, sizeof(cbw->CDB));
+	memcpy(cbw->CDB, cmd, cbw->Length);
 
 	/* send it to out endpoint */
 	dev_dbg(dev, "Bulk Command S 0x%x T 0x%x L %d F %d Trg %d LUN %d CL %d\n",
-		le32_to_cpu(cbw.Signature), cbw.Tag,
-		le32_to_cpu(cbw.DataTransferLength), cbw.Flags,
-		(cbw.Lun >> 4), (cbw.Lun & 0x0F),
-		cbw.Length);
-	result = usb_bulk_msg(us->pusb_dev, pipeout, &cbw, US_BULK_CB_WRAP_LEN,
+		le32_to_cpu(cbw->Signature), cbw->Tag,
+		le32_to_cpu(cbw->DataTransferLength), cbw->Flags,
+		(cbw->Lun >> 4), (cbw->Lun & 0x0F),
+		cbw->Length);
+	result = usb_bulk_msg(us->pusb_dev, pipeout, cbw, US_BULK_CB_WRAP_LEN,
 			      &actlen, USB_BULK_TO);
 	dev_dbg(dev, "Bulk command transfer result=%d\n", result);
 	if (result < 0) {
 		usb_stor_Bulk_reset(us);
-		return USB_STOR_TRANSPORT_FAILED;
+		ret = USB_STOR_TRANSPORT_FAILED;
+		goto fail;
 	}
 
 	/* DATA STAGE */
@@ -141,13 +146,14 @@ int usb_stor_Bulk_transport(struct us_blk_dev *usb_blkdev,
 		if (result < 0) {
 			dev_dbg(dev, "Device status: %lx\n", us->pusb_dev->status);
 			usb_stor_Bulk_reset(us);
-			return USB_STOR_TRANSPORT_FAILED;
+			ret = USB_STOR_TRANSPORT_FAILED;
+			goto fail;
 		}
 	}
 
 	/* STATUS phase + error handling */
 	dev_dbg(dev, "Attempting to get CSW...\n");
-	result = usb_bulk_msg(us->pusb_dev, pipein, &csw, US_BULK_CS_WRAP_LEN,
+	result = usb_bulk_msg(us->pusb_dev, pipein, csw, US_BULK_CS_WRAP_LEN,
 	                      &actlen, USB_BULK_TO);
 
 	/* did the endpoint stall? */
@@ -158,7 +164,7 @@ int usb_stor_Bulk_transport(struct us_blk_dev *usb_blkdev,
 		if (result >= 0) {
 			dev_dbg(dev, "Attempting to get CSW...\n");
 			result = usb_bulk_msg(us->pusb_dev, pipein,
-			                      &csw, US_BULK_CS_WRAP_LEN,
+			                      csw, US_BULK_CS_WRAP_LEN,
 			                      &actlen, USB_BULK_TO);
 		}
 	}
@@ -166,35 +172,39 @@ int usb_stor_Bulk_transport(struct us_blk_dev *usb_blkdev,
 	if (result < 0) {
 		dev_dbg(dev, "Device status: %lx\n", us->pusb_dev->status);
 		usb_stor_Bulk_reset(us);
-		return USB_STOR_TRANSPORT_FAILED;
+		ret = USB_STOR_TRANSPORT_FAILED;
+		goto fail;
 	}
 
 	/* check bulk status */
-	residue = le32_to_cpu(csw.Residue);
+	residue = le32_to_cpu(csw->Residue);
 	dev_dbg(dev, "Bulk Status S 0x%x T 0x%x R %u Stat 0x%x\n",
-		le32_to_cpu(csw.Signature), csw.Tag, residue, csw.Status);
-	if (csw.Signature != cpu_to_le32(US_BULK_CS_SIGN)) {
+		le32_to_cpu(csw->Signature), csw->Tag, residue, csw->Status);
+	if (csw->Signature != cpu_to_le32(US_BULK_CS_SIGN)) {
 		dev_dbg(dev, "Bad CSW signature\n");
 		usb_stor_Bulk_reset(us);
-		return USB_STOR_TRANSPORT_FAILED;
-	} else if (csw.Tag != cbw_tag) {
+		ret = USB_STOR_TRANSPORT_FAILED;
+	} else if (csw->Tag != cbw_tag) {
 		dev_dbg(dev, "Mismatching tag\n");
 		usb_stor_Bulk_reset(us);
-		return USB_STOR_TRANSPORT_FAILED;
-	} else if (csw.Status >= US_BULK_STAT_PHASE) {
+		ret = USB_STOR_TRANSPORT_FAILED;
+	} else if (csw->Status >= US_BULK_STAT_PHASE) {
 		dev_dbg(dev, "Status >= phase\n");
 		usb_stor_Bulk_reset(us);
-		return USB_STOR_TRANSPORT_ERROR;
+		ret = USB_STOR_TRANSPORT_ERROR;
 	} else if (residue > datalen) {
 		dev_dbg(dev, "residue (%uB) > req data (%uB)\n",
 		          residue, datalen);
-		return USB_STOR_TRANSPORT_FAILED;
-	} else if (csw.Status == US_BULK_STAT_FAIL) {
+		ret = USB_STOR_TRANSPORT_FAILED;
+	} else if (csw->Status == US_BULK_STAT_FAIL) {
 		dev_dbg(dev, "FAILED\n");
-		return USB_STOR_TRANSPORT_FAILED;
+		ret = USB_STOR_TRANSPORT_FAILED;
 	}
 
-	return 0;
+fail:
+	dma_free(cbw);
+	dma_free(csw);
+	return ret;
 }
 
 
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 6cdcc348e4..f281e0186d 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -10,6 +10,7 @@
 #include <common.h>
 #include <init.h>
 #include <malloc.h>
+#include <dma.h>
 #include <errno.h>
 #include <scsi.h>
 #include <linux/usb/usb.h>
@@ -33,7 +34,7 @@ static int usb_stor_request_sense(struct us_blk_dev *usb_blkdev)
 	struct device *dev = &us->pusb_dev->dev;
 	u8 cmd[6];
 	const u8 datalen = 18;
-	u8 *data = xzalloc(datalen);
+	u8 *data = dma_alloc(datalen);
 
 	dev_dbg(dev, "SCSI_REQ_SENSE\n");
 
@@ -44,7 +45,7 @@ static int usb_stor_request_sense(struct us_blk_dev *usb_blkdev)
 	dev_dbg(dev, "Request Sense returned %02X %02X %02X\n",
 		data[2], data[12], data[13]);
 
-	free(data);
+	dma_free(data);
 
 	return 0;
 }
@@ -104,7 +105,7 @@ static int usb_stor_inquiry(struct us_blk_dev *usb_blkdev)
 	int ret;
 	u8 cmd[6];
 	const u16 datalen = 36;
-	u8 *data = xzalloc(datalen);
+	u8 *data = dma_alloc(datalen);
 
 	memset(cmd, 0, sizeof(cmd));
 	cmd[0] = SCSI_INQUIRY;
@@ -126,7 +127,7 @@ static int usb_stor_inquiry(struct us_blk_dev *usb_blkdev)
 	// TODO:  process and store device info
 
 exit:
-	free(data);
+	dma_free(data);
 	return ret;
 }
 
@@ -154,7 +155,7 @@ static int read_capacity_16(struct us_blk_dev *usb_blkdev)
 	struct device *dev = &usb_blkdev->us->pusb_dev->dev;
 	unsigned char cmd[16];
 	const u8 datalen = 32;
-	u8 *data = xzalloc(datalen);
+	u8 *data = dma_alloc(datalen);
 	int ret;
 	sector_t lba;
 	unsigned sector_size;
@@ -190,7 +191,7 @@ static int read_capacity_16(struct us_blk_dev *usb_blkdev)
 
 	ret = sector_size;
 fail:
-	free(data);
+	dma_free(data);
 	return ret;
 }
 
@@ -199,7 +200,7 @@ static int read_capacity_10(struct us_blk_dev *usb_blkdev)
 	struct device *dev = &usb_blkdev->us->pusb_dev->dev;
 	unsigned char cmd[16];
 	const u32 datalen = 8;
-	__be32 *data = xzalloc(datalen);
+	__be32 *data = dma_alloc(datalen);
 	int ret;
 	sector_t lba;
 	unsigned sector_size;
@@ -229,7 +230,7 @@ static int read_capacity_10(struct us_blk_dev *usb_blkdev)
 
 	ret = SECTOR_SIZE;
 fail:
-	free(data);
+	dma_free(data);
 	return ret;
 }
 
-- 
2.41.0

Re: [PATCH 1/2] usb: storage: fix missing calls to free()

[Ahmad Fatoum]

On 29.06.23 21:52, Denis Orlov wrote:
> Memory allocated with xzalloc() was not actually being freed in a few
> functions, resulting in memory leaks.
> 
> Signed-off-by: Denis Orlov <denorl2009@gmail.com>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/usb/storage/usb.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index dda7131960..6cdcc348e4 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -169,7 +169,7 @@ static int read_capacity_16(struct us_blk_dev *usb_blkdev)
>  
>  	if (ret < 0) {
>  		dev_warn(dev, "Read Capacity(16) failed\n");
> -		return ret;
> +		goto fail;
>  	}
>  
>  	/* Note this is logical, not physical sector size */
> @@ -181,13 +181,17 @@ static int read_capacity_16(struct us_blk_dev *usb_blkdev)
>  
>  	if ((data[12] & 1) == 1) {
>  		dev_warn(dev, "Protection unsupported\n");
> -		return -ENOTSUPP;
> +		ret = -ENOTSUPP;
> +		goto fail;
>  	}
>  
>  	usb_blkdev->blk.blockbits = SECTOR_SHIFT;
>  	usb_blkdev->blk.num_blocks = lba + 1;
>  
> -	return sector_size;
> +	ret = sector_size;
> +fail:
> +	free(data);
> +	return ret;
>  }
>  
>  static int read_capacity_10(struct us_blk_dev *usb_blkdev)
> @@ -208,7 +212,7 @@ static int read_capacity_10(struct us_blk_dev *usb_blkdev)
>  
>  	if (ret < 0) {
>  		dev_warn(dev, "Read Capacity(10) failed\n");
> -		return ret;
> +		goto fail;
>  	}
>  
>  	sector_size = be32_to_cpu(data[1]);
> @@ -223,7 +227,10 @@ static int read_capacity_10(struct us_blk_dev *usb_blkdev)
>  	usb_blkdev->blk.num_blocks = lba + 1;
>  	usb_blkdev->blk.blockbits = SECTOR_SHIFT;
>  
> -	return SECTOR_SIZE;
> +	ret = SECTOR_SIZE;
> +fail:
> +	free(data);
> +	return ret;
>  }
>  
>  static int usb_stor_io_16(struct us_blk_dev *usb_blkdev, u8 opcode,

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/2] usb: small dma allocation fixes

[Sascha Hauer]

On Thu, Jun 29, 2023 at 10:52:56PM +0300, Denis Orlov wrote:
> Make sure dma buffers are both properly allocated and freed (they were
> not in a few places).
> 
> Denis Orlov (2):
>   usb: storage: fix missing calls to free()
>   usb: make sure dma buffers are properly allocated
> 
>  drivers/usb/core/usb.c          | 16 ++++---
>  drivers/usb/storage/transport.c | 74 +++++++++++++++++++--------------
>  drivers/usb/storage/usb.c       | 30 ++++++++-----
>  3 files changed, 71 insertions(+), 49 deletions(-)

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |