diff mbox

[v2,04/18] spi: fsl-espi: pre-allocate message buffer

Message ID 0e60b4b5-5016-e1c3-d29d-b81a41f64f48@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Heiner Kallweit Sept. 4, 2016, 7:58 a.m. UTC
Currently the driver allocates a 64kb buffer with each single message.
On systems with little and fragmented memory this can result in
memory allocation errors. Solve this by pre-allocating a buffer.

This patch was developed in OpenWRT long ago, however it never
made it upstream. Author: Gabor Juhos <juhosg@openwrt.org>

I slightly modified the original patch to re-initialize the buffer
at the beginning of each transfer.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- fixed coding style issue
- fixed commit message
---
 drivers/spi/spi-fsl-espi.c | 41 ++++++++++++++++++-----------------------
 drivers/spi/spi-fsl-lib.h  |  1 +
 2 files changed, 19 insertions(+), 23 deletions(-)

Comments

Mark Brown Sept. 6, 2016, 10:58 a.m. UTC | #1
On Sun, Sep 04, 2016 at 09:58:02AM +0200, Heiner Kallweit wrote:
> Currently the driver allocates a 64kb buffer with each single message.
> On systems with little and fragmented memory this can result in
> memory allocation errors. Solve this by pre-allocating a buffer.
> 
> This patch was developed in OpenWRT long ago, however it never
> made it upstream. Author: Gabor Juhos <juhosg@openwrt.org>
> 
> I slightly modified the original patch to re-initialize the buffer
> at the beginning of each transfer.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

When you sent this previously I asked what happened with the signoff
from Gabor.  A little Google tells me that he did in fact sign off the
copy that's in OpenWRT.  Please don't drop signoffs, they're important
for tracking licensing - see SubmittingPatches for details.  If the
modifications you made really are slight as you say then I'd also expect
Gabor's authorship to be preserved.
Heiner Kallweit Sept. 6, 2016, 8:57 p.m. UTC | #2
Am 06.09.2016 um 12:58 schrieb Mark Brown:
> On Sun, Sep 04, 2016 at 09:58:02AM +0200, Heiner Kallweit wrote:
>> Currently the driver allocates a 64kb buffer with each single message.
>> On systems with little and fragmented memory this can result in
>> memory allocation errors. Solve this by pre-allocating a buffer.
>>
>> This patch was developed in OpenWRT long ago, however it never
>> made it upstream. Author: Gabor Juhos <juhosg@openwrt.org>
>>
>> I slightly modified the original patch to re-initialize the buffer
>> at the beginning of each transfer.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> When you sent this previously I asked what happened with the signoff
> from Gabor.  A little Google tells me that he did in fact sign off the
> copy that's in OpenWRT.  Please don't drop signoffs, they're important
> for tracking licensing - see SubmittingPatches for details.  If the
> modifications you made really are slight as you say then I'd also expect
> Gabor's authorship to be preserved.
> 
Sorry, then I misunderstood your comment.
I'll resend the patch with Gabor's Signed-off-by. Do you want my
Signed-off-by in addition?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 6, 2016, 10:40 p.m. UTC | #3
On Tue, Sep 06, 2016 at 10:57:49PM +0200, Heiner Kallweit wrote:

> Sorry, then I misunderstood your comment.
> I'll resend the patch with Gabor's Signed-off-by. Do you want my
> Signed-off-by in addition?

You need your signoff on anything you send including this - like I say
see SubmittingPatches for what this is doing.
diff mbox

Patch

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index a9004fe..a3a75ae 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c
@@ -300,57 +300,44 @@  static void fsl_espi_do_trans(struct spi_message *m,
 static void fsl_espi_cmd_trans(struct spi_message *m,
 				struct fsl_espi_transfer *trans, u8 *rx_buff)
 {
+	struct mpc8xxx_spi *mspi = spi_master_get_devdata(m->spi->master);
 	struct spi_transfer *t;
-	u8 *local_buf;
 	int i = 0;
 	struct fsl_espi_transfer *espi_trans = trans;
 
-	local_buf = kzalloc(SPCOM_TRANLEN_MAX, GFP_KERNEL);
-	if (!local_buf) {
-		espi_trans->status = -ENOMEM;
-		return;
-	}
-
 	list_for_each_entry(t, &m->transfers, transfer_list) {
 		if (t->tx_buf) {
-			memcpy(local_buf + i, t->tx_buf, t->len);
+			memcpy(mspi->local_buf + i, t->tx_buf, t->len);
 			i += t->len;
 		}
 	}
 
-	espi_trans->tx_buf = local_buf;
-	espi_trans->rx_buf = local_buf;
+	espi_trans->tx_buf = mspi->local_buf;
+	espi_trans->rx_buf = mspi->local_buf;
 	fsl_espi_do_trans(m, espi_trans);
 
 	espi_trans->actual_length = espi_trans->len;
-	kfree(local_buf);
 }
 
 static void fsl_espi_rw_trans(struct spi_message *m,
 				struct fsl_espi_transfer *trans, u8 *rx_buff)
 {
+	struct mpc8xxx_spi *mspi = spi_master_get_devdata(m->spi->master);
 	struct spi_transfer *t;
-	u8 *local_buf;
 	unsigned int tx_only = 0;
 	int i = 0;
 
-	local_buf = kzalloc(SPCOM_TRANLEN_MAX, GFP_KERNEL);
-	if (!local_buf) {
-		trans->status = -ENOMEM;
-		return;
-	}
-
 	list_for_each_entry(t, &m->transfers, transfer_list) {
 		if (t->tx_buf) {
-			memcpy(local_buf + i, t->tx_buf, t->len);
+			memcpy(mspi->local_buf + i, t->tx_buf, t->len);
 			i += t->len;
 			if (!t->rx_buf)
 				tx_only += t->len;
 		}
 	}
 
-	trans->tx_buf = local_buf;
-	trans->rx_buf = local_buf;
+	trans->tx_buf = mspi->local_buf;
+	trans->rx_buf = mspi->local_buf;
 	fsl_espi_do_trans(m, trans);
 
 	if (!trans->status) {
@@ -360,18 +347,19 @@  static void fsl_espi_rw_trans(struct spi_message *m,
 			       trans->len - tx_only);
 		trans->actual_length += trans->len;
 	}
-
-	kfree(local_buf);
 }
 
 static int fsl_espi_do_one_msg(struct spi_master *master,
 			       struct spi_message *m)
 {
+	struct mpc8xxx_spi *mspi = spi_master_get_devdata(master);
 	struct spi_transfer *t;
 	u8 *rx_buf = NULL;
 	unsigned int xfer_len = 0;
 	struct fsl_espi_transfer espi_trans;
 
+	memset(mspi->local_buf, 0, SPCOM_TRANLEN_MAX);
+
 	list_for_each_entry(t, &m->transfers, transfer_list) {
 		if (t->rx_buf)
 			rx_buf = t->rx_buf;
@@ -617,6 +605,13 @@  static struct spi_master * fsl_espi_probe(struct device *dev,
 
 	mpc8xxx_spi = spi_master_get_devdata(master);
 
+	mpc8xxx_spi->local_buf =
+		devm_kmalloc(dev, SPCOM_TRANLEN_MAX, GFP_KERNEL);
+	if (!mpc8xxx_spi->local_buf) {
+		ret = -ENOMEM;
+		goto err_probe;
+	}
+
 	mpc8xxx_spi->reg_base = devm_ioremap_resource(dev, mem);
 	if (IS_ERR(mpc8xxx_spi->reg_base)) {
 		ret = PTR_ERR(mpc8xxx_spi->reg_base);
diff --git a/drivers/spi/spi-fsl-lib.h b/drivers/spi/spi-fsl-lib.h
index 84f5dcb..065b9db 100644
--- a/drivers/spi/spi-fsl-lib.h
+++ b/drivers/spi/spi-fsl-lib.h
@@ -30,6 +30,7 @@  struct mpc8xxx_spi {
 	void *rx;
 #if IS_ENABLED(CONFIG_SPI_FSL_ESPI)
 	int len;
+	u8 *local_buf;
 #endif
 
 	int subblock;