2009-09-17 23:48:16

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH 1/5] Blackfin SPI: fix resources leakage

From: Daniel Mack <[email protected]>

Re-order setup() a bit so we don't leak memory/dma/gpio resources upon
errors.

Signed-off-by: Daniel Mack <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/spi/spi_bfin5xx.c | 99 +++++++++++++++++++++++++--------------------
1 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 73e24ef..8ec4969 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -1005,18 +1005,19 @@ static u16 ssel[][MAX_SPI_SSEL] = {
/* first setup for new devices */
static int bfin_spi_setup(struct spi_device *spi)
{
- struct bfin5xx_spi_chip *chip_info = NULL;
- struct chip_data *chip;
+ struct bfin5xx_spi_chip *chip_info;
+ struct chip_data *chip = NULL;
struct driver_data *drv_data = spi_master_get_devdata(spi->master);
- int ret;
+ int ret = -EINVAL;

if (spi->bits_per_word != 8 && spi->bits_per_word != 16)
- return -EINVAL;
+ goto error;

/* Only alloc (or use chip_info) on first setup */
+ chip_info = NULL;
chip = spi_get_ctldata(spi);
if (chip == NULL) {
- chip = kzalloc(sizeof(struct chip_data), GFP_KERNEL);
+ chip = kzalloc(sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;

@@ -1035,7 +1036,7 @@ static int bfin_spi_setup(struct spi_device *spi)
if (chip_info->ctl_reg & (SPE|MSTR|CPOL|CPHA|LSBF|SIZE)) {
dev_err(&spi->dev, "do not set bits in ctl_reg "
"that the SPI framework manages\n");
- return -EINVAL;
+ goto error;
}

chip->enable_dma = chip_info->enable_dma != 0
@@ -1059,26 +1060,6 @@ static int bfin_spi_setup(struct spi_device *spi)
chip->ctl_reg |= MSTR;

/*
- * if any one SPI chip is registered and wants DMA, request the
- * DMA channel for it
- */
- if (chip->enable_dma && !drv_data->dma_requested) {
- /* register dma irq handler */
- if (request_dma(drv_data->dma_channel, "BFIN_SPI_DMA") < 0) {
- dev_dbg(&spi->dev,
- "Unable to request BlackFin SPI DMA channel\n");
- return -ENODEV;
- }
- if (set_dma_callback(drv_data->dma_channel,
- bfin_spi_dma_irq_handler, drv_data) < 0) {
- dev_dbg(&spi->dev, "Unable to set dma callback\n");
- return -EPERM;
- }
- dma_disable_irq(drv_data->dma_channel);
- drv_data->dma_requested = 1;
- }
-
- /*
* Notice: for blackfin, the speed_hz is the value of register
* SPI_BAUD, not the real baudrate
*/
@@ -1086,16 +1067,6 @@ static int bfin_spi_setup(struct spi_device *spi)
chip->flag = 1 << (spi->chip_select);
chip->chip_select_num = spi->chip_select;

- if (chip->chip_select_num == 0) {
- ret = gpio_request(chip->cs_gpio, spi->modalias);
- if (ret) {
- if (drv_data->dma_requested)
- free_dma(drv_data->dma_channel);
- return ret;
- }
- gpio_direction_output(chip->cs_gpio, 1);
- }
-
switch (chip->bits_per_word) {
case 8:
chip->n_bytes = 1;
@@ -1122,9 +1093,37 @@ static int bfin_spi_setup(struct spi_device *spi)
default:
dev_err(&spi->dev, "%d bits_per_word is not supported\n",
chip->bits_per_word);
- if (chip_info)
- kfree(chip);
- return -ENODEV;
+ goto error;
+ }
+
+ /*
+ * if any one SPI chip is registered and wants DMA, request the
+ * DMA channel for it
+ */
+ if (chip->enable_dma && !drv_data->dma_requested) {
+ /* register dma irq handler */
+ ret = request_dma(drv_data->dma_channel, "BFIN_SPI_DMA");
+ if (ret) {
+ dev_dbg(&spi->dev,
+ "Unable to request BlackFin SPI DMA channel\n");
+ goto error;
+ }
+ drv_data->dma_requested = 1;
+
+ ret = set_dma_callback(drv_data->dma_channel,
+ bfin_spi_dma_irq_handler, drv_data);
+ if (ret) {
+ dev_dbg(&spi->dev, "Unable to set dma callback\n");
+ goto error;
+ }
+ dma_disable_irq(drv_data->dma_channel);
+ }
+
+ if (chip->chip_select_num == 0) {
+ ret = gpio_request(chip->cs_gpio, spi->modalias);
+ if (ret)
+ goto error;
+ gpio_direction_output(chip->cs_gpio, 1);
}

dev_dbg(&spi->dev, "setup spi chip %s, width is %d, dma is %d\n",
@@ -1135,14 +1134,26 @@ static int bfin_spi_setup(struct spi_device *spi)
spi_set_ctldata(spi, chip);

dev_dbg(&spi->dev, "chip select number is %d\n", chip->chip_select_num);
- if ((chip->chip_select_num > 0)
- && (chip->chip_select_num <= spi->master->num_chipselect))
- peripheral_request(ssel[spi->master->bus_num]
- [chip->chip_select_num-1], spi->modalias);
+ if (chip->chip_select_num > 0 &&
+ chip->chip_select_num <= spi->master->num_chipselect) {
+ ret = peripheral_request(ssel[spi->master->bus_num]
+ [chip->chip_select_num-1], spi->modalias);
+ if (ret)
+ goto error;
+ }

bfin_spi_cs_deactive(drv_data, chip);
+ ret = 0;

- return 0;
+ error:
+ if (ret) {
+ kfree(chip);
+ if (drv_data->dma_requested)
+ free_dma(drv_data->dma_channel);
+ drv_data->dma_requested = 0;
+ }
+
+ return ret;
}

/*
--
1.6.5.rc1


2009-09-17 23:47:54

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH 2/5] Blackfin SPI: work around anomaly 05000119

From: Sonic Zhang <[email protected]>

Anomaly 05000119 states that the DMA_RUN bit with peripherals isn't
reliable. However, the way the driver is currently written (DMA IRQ
callback), we don't need the polling in the first place, so drop it.

Signed-off-by: Sonic Zhang <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/spi/spi_bfin5xx.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 8ec4969..5591bce 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -539,10 +539,6 @@ static irqreturn_t bfin_spi_dma_irq_handler(int irq, void *dev_id)

clear_dma_irqstat(drv_data->dma_channel);

- /* Wait for DMA to complete */
- while (get_dma_curr_irqstat(drv_data->dma_channel) & DMA_RUN)
- cpu_relax();
-
/*
* wait for the last transaction shifted out. HRM states:
* at this point there may still be data in the SPI DMA FIFO waiting
--
1.6.5.rc1

2009-09-17 23:48:07

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH 3/5] Blackfin SPI: force sane state at boot

From: Wolfgang Muees <[email protected]>

We should make sure the SPI controller is in a sane state in case the
boot loader left it in a crappy state. Such as DMA pending which causes
interrupts to fire on us.

Signed-off-by: Wolfgang Muees <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/spi/spi_bfin5xx.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 5591bce..3249c5d 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -1334,6 +1334,12 @@ static int __init bfin_spi_probe(struct platform_device *pdev)
goto out_error_queue_alloc;
}

+ /* Reset SPI registers. If these registers were used by the boot loader,
+ * the sky may fall on your head if you enable the dma controller.
+ */
+ write_CTRL(drv_data, 0x0400);
+ write_FLAG(drv_data, 0xFF00);
+
/* Register with the SPI framework */
platform_set_drvdata(pdev, drv_data);
status = spi_register_master(master);
--
1.6.5.rc1

2009-09-17 23:48:32

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH 4/5] Blackfin SPI: utilize the SPI interrupt in PIO mode

From: Yi Li <[email protected]>

The current behavior in PIO mode is to poll the SPI status registers which
can obviously lead to higher latencies when doing a lot of SPI traffic.
There is a SPI interrupt which can be used instead to signal individual
completion of transactions.

Signed-off-by: Yi Li <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/spi/spi_bfin5xx.c | 235 +++++++++++++++++++++++++++++++++++----------
1 files changed, 185 insertions(+), 50 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 3249c5d..a8fc922 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -91,6 +91,9 @@ struct driver_data {
dma_addr_t rx_dma;
dma_addr_t tx_dma;

+ int irq_requested;
+ int spi_irq;
+
size_t rx_map_len;
size_t tx_map_len;
u8 n_bytes;
@@ -114,6 +117,7 @@ struct chip_data {
u16 cs_chg_udelay; /* Some devices require > 255usec delay */
u32 cs_gpio;
u16 idle_tx_val;
+ u8 pio_interrupt; /* use spi data irq */
void (*write) (struct driver_data *);
void (*read) (struct driver_data *);
void (*duplex) (struct driver_data *);
@@ -524,6 +528,79 @@ static void bfin_spi_giveback(struct driver_data *drv_data)
msg->complete(msg->context);
}

+/* spi data irq handler */
+static irqreturn_t bfin_spi_pio_irq_handler(int irq, void *dev_id)
+{
+ struct driver_data *drv_data = dev_id;
+ struct chip_data *chip = drv_data->cur_chip;
+ struct spi_message *msg = drv_data->cur_msg;
+ int n_bytes = drv_data->n_bytes;
+
+ /* wait until transfer finished. */
+ while (!(read_STAT(drv_data) & BIT_STAT_RXS))
+ cpu_relax();
+
+ if ((drv_data->tx && drv_data->tx >= drv_data->tx_end) ||
+ (drv_data->rx && drv_data->rx >= (drv_data->rx_end - n_bytes))) {
+ /* last read */
+ if (drv_data->rx) {
+ dev_dbg(&drv_data->pdev->dev, "last read\n");
+ if (n_bytes == 2)
+ *(u16 *) (drv_data->rx) = read_RDBR(drv_data);
+ else if (n_bytes == 1)
+ *(u8 *) (drv_data->rx) = read_RDBR(drv_data);
+ drv_data->rx += n_bytes;
+ }
+
+ msg->actual_length += drv_data->len_in_bytes;
+ if (drv_data->cs_change)
+ bfin_spi_cs_deactive(drv_data, chip);
+ /* Move to next transfer */
+ msg->state = bfin_spi_next_transfer(drv_data);
+
+ disable_irq(drv_data->spi_irq);
+
+ /* Schedule transfer tasklet */
+ tasklet_schedule(&drv_data->pump_transfers);
+ return IRQ_HANDLED;
+ }
+
+ if (drv_data->rx && drv_data->tx) {
+ /* duplex */
+ dev_dbg(&drv_data->pdev->dev, "duplex: write_TDBR\n");
+ if (drv_data->n_bytes == 2) {
+ *(u16 *) (drv_data->rx) = read_RDBR(drv_data);
+ write_TDBR(drv_data, (*(u16 *) (drv_data->tx)));
+ } else if (drv_data->n_bytes == 1) {
+ *(u8 *) (drv_data->rx) = read_RDBR(drv_data);
+ write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
+ }
+ } else if (drv_data->rx) {
+ /* read */
+ dev_dbg(&drv_data->pdev->dev, "read: write_TDBR\n");
+ if (drv_data->n_bytes == 2)
+ *(u16 *) (drv_data->rx) = read_RDBR(drv_data);
+ else if (drv_data->n_bytes == 1)
+ *(u8 *) (drv_data->rx) = read_RDBR(drv_data);
+ write_TDBR(drv_data, chip->idle_tx_val);
+ } else if (drv_data->tx) {
+ /* write */
+ dev_dbg(&drv_data->pdev->dev, "write: write_TDBR\n");
+ bfin_spi_dummy_read(drv_data);
+ if (drv_data->n_bytes == 2)
+ write_TDBR(drv_data, (*(u16 *) (drv_data->tx)));
+ else if (drv_data->n_bytes == 1)
+ write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
+ }
+
+ if (drv_data->tx)
+ drv_data->tx += n_bytes;
+ if (drv_data->rx)
+ drv_data->rx += n_bytes;
+
+ return IRQ_HANDLED;
+}
+
static irqreturn_t bfin_spi_dma_irq_handler(int irq, void *dev_id)
{
struct driver_data *drv_data = dev_id;
@@ -699,6 +776,7 @@ static void bfin_spi_pump_transfers(unsigned long data)

default:
/* No change, the same as default setting */
+ transfer->bits_per_word = chip->bits_per_word;
drv_data->n_bytes = chip->n_bytes;
width = chip->width;
drv_data->write = drv_data->tx ? chip->write : bfin_spi_null_writer;
@@ -841,60 +919,86 @@ static void bfin_spi_pump_transfers(unsigned long data)
dma_enable_irq(drv_data->dma_channel);
local_irq_restore(flags);

- } else {
- /* IO mode write then read */
- dev_dbg(&drv_data->pdev->dev, "doing IO transfer\n");
+ return;
+ }

- /* we always use SPI_WRITE mode. SPI_READ mode
- seems to have problems with setting up the
- output value in TDBR prior to the transfer. */
+ if (chip->pio_interrupt) {
+ /* use write mode. spi irq should have been disabled */
+ cr = (read_CTRL(drv_data) & (~BIT_CTL_TIMOD));
write_CTRL(drv_data, (cr | CFG_SPI_WRITE));

- if (full_duplex) {
- /* full duplex mode */
- BUG_ON((drv_data->tx_end - drv_data->tx) !=
- (drv_data->rx_end - drv_data->rx));
- dev_dbg(&drv_data->pdev->dev,
- "IO duplex: cr is 0x%x\n", cr);
-
- drv_data->duplex(drv_data);
-
- if (drv_data->tx != drv_data->tx_end)
- tranf_success = 0;
- } else if (drv_data->tx != NULL) {
- /* write only half duplex */
- dev_dbg(&drv_data->pdev->dev,
- "IO write: cr is 0x%x\n", cr);
+ /* discard old RX data and clear RXS */
+ bfin_spi_dummy_read(drv_data);

- drv_data->write(drv_data);
+ /* start transfer */
+ if (drv_data->tx == NULL)
+ write_TDBR(drv_data, chip->idle_tx_val);
+ else {
+ if (transfer->bits_per_word == 8)
+ write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
+ else if (transfer->bits_per_word == 16)
+ write_TDBR(drv_data, (*(u16 *) (drv_data->tx)));
+ drv_data->tx += drv_data->n_bytes;
+ }

- if (drv_data->tx != drv_data->tx_end)
- tranf_success = 0;
- } else if (drv_data->rx != NULL) {
- /* read only half duplex */
- dev_dbg(&drv_data->pdev->dev,
- "IO read: cr is 0x%x\n", cr);
+ /* once TDBR is empty, interrupt is triggered */
+ enable_irq(drv_data->spi_irq);
+ return;
+ }

- drv_data->read(drv_data);
- if (drv_data->rx != drv_data->rx_end)
- tranf_success = 0;
- }
+ /* IO mode */
+ dev_dbg(&drv_data->pdev->dev, "doing IO transfer\n");
+
+ /* we always use SPI_WRITE mode. SPI_READ mode
+ seems to have problems with setting up the
+ output value in TDBR prior to the transfer. */
+ write_CTRL(drv_data, (cr | CFG_SPI_WRITE));
+
+ if (full_duplex) {
+ /* full duplex mode */
+ BUG_ON((drv_data->tx_end - drv_data->tx) !=
+ (drv_data->rx_end - drv_data->rx));
+ dev_dbg(&drv_data->pdev->dev,
+ "IO duplex: cr is 0x%x\n", cr);
+
+ drv_data->duplex(drv_data);
+
+ if (drv_data->tx != drv_data->tx_end)
+ tranf_success = 0;
+ } else if (drv_data->tx != NULL) {
+ /* write only half duplex */
+ dev_dbg(&drv_data->pdev->dev,
+ "IO write: cr is 0x%x\n", cr);
+
+ drv_data->write(drv_data);
+
+ if (drv_data->tx != drv_data->tx_end)
+ tranf_success = 0;
+ } else if (drv_data->rx != NULL) {
+ /* read only half duplex */
+ dev_dbg(&drv_data->pdev->dev,
+ "IO read: cr is 0x%x\n", cr);
+
+ drv_data->read(drv_data);
+ if (drv_data->rx != drv_data->rx_end)
+ tranf_success = 0;
+ }

- if (!tranf_success) {
- dev_dbg(&drv_data->pdev->dev,
- "IO write error!\n");
- message->state = ERROR_STATE;
- } else {
- /* Update total byte transfered */
- message->actual_length += drv_data->len_in_bytes;
- /* Move to next transfer of this msg */
- message->state = bfin_spi_next_transfer(drv_data);
- if (drv_data->cs_change)
- bfin_spi_cs_deactive(drv_data, chip);
- }
- /* Schedule next transfer tasklet */
- tasklet_schedule(&drv_data->pump_transfers);
+ if (!tranf_success) {
+ dev_dbg(&drv_data->pdev->dev,
+ "IO write error!\n");
+ message->state = ERROR_STATE;
+ } else {
+ /* Update total byte transfered */
+ message->actual_length += drv_data->len_in_bytes;
+ /* Move to next transfer of this msg */
+ message->state = bfin_spi_next_transfer(drv_data);
+ if (drv_data->cs_change)
+ bfin_spi_cs_deactive(drv_data, chip);
}
+
+ /* Schedule next transfer tasklet */
+ tasklet_schedule(&drv_data->pump_transfers);
}

/* pop a msg from queue and kick off real transfer */
@@ -1043,6 +1147,7 @@ static int bfin_spi_setup(struct spi_device *spi)
chip->cs_chg_udelay = chip_info->cs_chg_udelay;
chip->cs_gpio = chip_info->cs_gpio;
chip->idle_tx_val = chip_info->idle_tx_val;
+ chip->pio_interrupt = chip_info->pio_interrupt;
}

/* translate common spi framework into our register */
@@ -1092,6 +1197,11 @@ static int bfin_spi_setup(struct spi_device *spi)
goto error;
}

+ if (chip->enable_dma && chip->pio_interrupt) {
+ dev_err(&spi->dev, "enable_dma is set, "
+ "do not set pio_interrupt\n");
+ goto error;
+ }
/*
* if any one SPI chip is registered and wants DMA, request the
* DMA channel for it
@@ -1115,6 +1225,18 @@ static int bfin_spi_setup(struct spi_device *spi)
dma_disable_irq(drv_data->dma_channel);
}

+ if (chip->pio_interrupt && !drv_data->irq_requested) {
+ ret = request_irq(drv_data->spi_irq, bfin_spi_pio_irq_handler,
+ IRQF_DISABLED, "BFIN_SPI", drv_data);
+ if (ret) {
+ printk(KERN_NOTICE "Unable to register spi IRQ\n");
+ goto error;
+ }
+ drv_data->irq_requested = 1;
+ /* we use write mode, spi irq has to be disabled here */
+ disable_irq(drv_data->spi_irq);
+ }
+
if (chip->chip_select_num == 0) {
ret = gpio_request(chip->cs_gpio, spi->modalias);
if (ret)
@@ -1308,11 +1430,19 @@ static int __init bfin_spi_probe(struct platform_device *pdev)
goto out_error_ioremap;
}

- drv_data->dma_channel = platform_get_irq(pdev, 0);
- if (drv_data->dma_channel < 0) {
+ res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+ if (res == NULL) {
dev_err(dev, "No DMA channel specified\n");
status = -ENOENT;
- goto out_error_no_dma_ch;
+ goto out_error_free_io;
+ }
+ drv_data->dma_channel = res->start;
+
+ drv_data->spi_irq = platform_get_irq(pdev, 0);
+ if (drv_data->spi_irq < 0) {
+ dev_err(dev, "No spi pio irq specified\n");
+ status = -ENOENT;
+ goto out_error_free_io;
}

/* Initial and start queue */
@@ -1355,7 +1485,7 @@ static int __init bfin_spi_probe(struct platform_device *pdev)

out_error_queue_alloc:
bfin_spi_destroy_queue(drv_data);
-out_error_no_dma_ch:
+out_error_free_io:
iounmap((void *) drv_data->regs_base);
out_error_ioremap:
out_error_get_res:
@@ -1387,6 +1517,11 @@ static int __devexit bfin_spi_remove(struct platform_device *pdev)
free_dma(drv_data->dma_channel);
}

+ if (drv_data->irq_requested) {
+ free_irq(drv_data->spi_irq, drv_data);
+ drv_data->irq_requested = 0;
+ }
+
/* Disconnect from the SPI framework */
spi_unregister_master(drv_data->master);

--
1.6.5.rc1

2009-09-17 23:48:03

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH 5/5] Blackfin SPI: clean up error handling in client setup

From: Yi Li <[email protected]>

Make sure we don't leak peripheral/gpio resources when an error occurs
during setup, and make sure we don't call kfree() twice on the same
object. Also add/fix some error messages in the process.

Signed-off-by: Yi Li <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/spi/spi_bfin5xx.c | 33 ++++++++++++++++++++++++++-------
1 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index a8fc922..0dcc2ca 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -1118,8 +1118,10 @@ static int bfin_spi_setup(struct spi_device *spi)
chip = spi_get_ctldata(spi);
if (chip == NULL) {
chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (!chip)
+ if (!chip) {
+ dev_err(&spi->dev, "cannot allocate chip data\n");
return -ENOMEM;
+ }

chip->enable_dma = 0;
chip_info = spi->controller_data;
@@ -1210,7 +1212,7 @@ static int bfin_spi_setup(struct spi_device *spi)
/* register dma irq handler */
ret = request_dma(drv_data->dma_channel, "BFIN_SPI_DMA");
if (ret) {
- dev_dbg(&spi->dev,
+ dev_err(&spi->dev,
"Unable to request BlackFin SPI DMA channel\n");
goto error;
}
@@ -1219,7 +1221,7 @@ static int bfin_spi_setup(struct spi_device *spi)
ret = set_dma_callback(drv_data->dma_channel,
bfin_spi_dma_irq_handler, drv_data);
if (ret) {
- dev_dbg(&spi->dev, "Unable to set dma callback\n");
+ dev_err(&spi->dev, "Unable to set dma callback\n");
goto error;
}
dma_disable_irq(drv_data->dma_channel);
@@ -1229,7 +1231,7 @@ static int bfin_spi_setup(struct spi_device *spi)
ret = request_irq(drv_data->spi_irq, bfin_spi_pio_irq_handler,
IRQF_DISABLED, "BFIN_SPI", drv_data);
if (ret) {
- printk(KERN_NOTICE "Unable to register spi IRQ\n");
+ dev_err(&spi->dev, "Unable to register spi IRQ\n");
goto error;
}
drv_data->irq_requested = 1;
@@ -1239,8 +1241,10 @@ static int bfin_spi_setup(struct spi_device *spi)

if (chip->chip_select_num == 0) {
ret = gpio_request(chip->cs_gpio, spi->modalias);
- if (ret)
+ if (ret) {
+ dev_err(&spi->dev, "gpio_request() error\n");
goto error;
+ }
gpio_direction_output(chip->cs_gpio, 1);
}

@@ -1256,8 +1260,10 @@ static int bfin_spi_setup(struct spi_device *spi)
chip->chip_select_num <= spi->master->num_chipselect) {
ret = peripheral_request(ssel[spi->master->bus_num]
[chip->chip_select_num-1], spi->modalias);
- if (ret)
+ if (ret) {
+ dev_err(&spi->dev, "peripheral_request() error\n");
goto error;
+ }
}

bfin_spi_cs_deactive(drv_data, chip);
@@ -1265,10 +1271,21 @@ static int bfin_spi_setup(struct spi_device *spi)

error:
if (ret) {
- kfree(chip);
if (drv_data->dma_requested)
free_dma(drv_data->dma_channel);
drv_data->dma_requested = 0;
+
+ if ((chip->chip_select_num > 0)
+ && (chip->chip_select_num <= spi->master->num_chipselect))
+ peripheral_free(ssel[spi->master->bus_num]
+ [chip->chip_select_num-1]);
+
+ if (chip->chip_select_num == 0)
+ gpio_free(chip->cs_gpio);
+
+ kfree(chip);
+ /* prevent free 'chip' twice */
+ spi_set_ctldata(spi, NULL);
}

return ret;
@@ -1294,6 +1311,8 @@ static void bfin_spi_cleanup(struct spi_device *spi)
gpio_free(chip->cs_gpio);

kfree(chip);
+ /* prevent free 'chip' twice */
+ spi_set_ctldata(spi, NULL);
}

static inline int bfin_spi_init_queue(struct driver_data *drv_data)
--
1.6.5.rc1

2009-10-01 04:27:49

by Mike Frysinger

[permalink] [raw]
Subject: Re: [Uclinux-dist-devel] [PATCH 5/5] Blackfin SPI: clean up error handling in client setup

On Thu, Sep 17, 2009 at 19:47, Mike Frysinger <[email protected]> wrote:
> From: Yi Li <[email protected]>
>
> Make sure we don't leak peripheral/gpio resources when an error occurs
> during setup, and make sure we don't call kfree() twice on the same
> object.  Also add/fix some error messages in the process.

not sure if you've picked any of these changes up David, but if you
havent, please skip this last patch. it needs some updating as it
introduces at least one NULL ptr deref in the error path.
-mike