[Cbe-oss-dev] [PATCH] ps3_storage scheduling while atomic
Florin Malita
fmalita at gmail.com
Mon Jan 29 11:33:43 EST 2007
Florin Malita wrote:
> While testing the latest PS3 patchset, I tried enabling CONFIG_PREEMPT
> and the storage driver triggered this:
>
> BUG: scheduling while atomic: ps3stor-1-0/0x00000001/183
> Call Trace:
> [C000000001B17AB0] [C00000000000F7D4] .show_stack+0x54/0x1f0 (unreliable)
> [C000000001B17B60] [C000000000288C94] .schedule+0xbb4/0xc70
> [C000000001B17CC0] [C000000000288EC8] .wait_for_completion+0xb8/0x140
> [C000000001B17D80] [C0000000001B5704] .ps3_stor_common_handle_write+0x104/0x260
> [C000000001B17E40] [C0000000001B5AC8] .ps3_stor_main_thread+0x108/0x160
> [C000000001B17EE0] [C000000000067548] .kthread+0x158/0x170
> [C000000001B17F90] [C000000000021D18] .kernel_thread+0x4c/0x68
> ...
>
>
> Both ps3_stor_common_handle_read() and ps3_stor_common_handle_write()
> call wait_for_completion() while holding a rw spinlock
> (dev_info->bounce_lock). Looks like the only race is with
> ps3_stor_set_max_sectors() so maybe there's a way to avoid locking
> altogether but in the meantime here's a patch converting it to a
> rw_semaphore.
>
Any plans to address this? Any input on the patch?
---
fm
> Signed-off-by: Florin Malita <fmalita at gmail.com>
> ---
>
> ps3_storage.c | 31 ++++++++++++++++---------------
> ps3_storage.h | 10 +++++-----
> 2 files changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/ps3_storage.c b/drivers/block/ps3_storage.c
> index 7a9697c..a146151 100644
> --- a/drivers/block/ps3_storage.c
> +++ b/drivers/block/ps3_storage.c
> @@ -30,6 +30,7 @@
> #include <linux/blkdev.h>
> #include <linux/kthread.h>
> #include <linux/interrupt.h>
> +#include <linux/rwsem.h>
> #include <scsi/scsi.h>
> #include <scsi/scsi_host.h>
> #include <scsi/scsi_cmnd.h>
> @@ -853,7 +854,7 @@ static int ps3_stor_common_handle_read(struct ps3_stor_dev_info * dev_info,
> }
>
> /* issue read */
> - read_lock(&dev_info->bounce_lock);
> + down_read(&dev_info->bounce_sem);
> lpar_addr = ps3_stor_virtual_to_lpar(dev_info, dev_info->bounce_buf);
> region_id = lv1_dev_info->region_info_array[(cmnd[1] >> 5)].region_id;
> init_completion(&(dev_info->irq_done));
> @@ -895,7 +896,7 @@ static int ps3_stor_common_handle_read(struct ps3_stor_dev_info * dev_info,
> }
>
> ps3_stor_srb_done(dev_info);
> - read_unlock(&dev_info->bounce_lock);
> + up_read(&dev_info->bounce_sem);
> return ret;
> }
>
> @@ -929,7 +930,7 @@ static int ps3_stor_common_handle_write(struct ps3_stor_dev_info * dev_info,
> break;
> }
>
> - read_lock(&dev_info->bounce_lock);
> + down_read(&dev_info->bounce_sem);
> ret = fetch_to_dev_buffer(dev_info->srb,
> dev_info->bounce_buf,
> sectors * dev_info->sector_size);
> @@ -973,7 +974,7 @@ static int ps3_stor_common_handle_write(struct ps3_stor_dev_info * dev_info,
>
> }
> ps3_stor_srb_done(dev_info);
> - read_unlock(&dev_info->bounce_lock);
> + up_read(&dev_info->bounce_sem);
> return ret;
> }
>
> @@ -1072,7 +1073,7 @@ static int ps3_stor_handle_write_flash(struct ps3_stor_dev_info * dev_info,
> return ps3_stor_common_handle_write(dev_info, srb);
> };
>
> - read_lock(&dev_info->bounce_lock);
> + down_read(&dev_info->bounce_sem);
> region_id = region_info->region_id;
>
>
> @@ -1257,7 +1258,7 @@ static int ps3_stor_handle_write_flash(struct ps3_stor_dev_info * dev_info,
> } /* for */
> done:
> ps3_stor_srb_done(dev_info);
> - read_unlock(&dev_info->bounce_lock);
> + up_read(&dev_info->bounce_sem);
> DPRINTK(KERN_ERR "%s: end\n", __FUNCTION__);
> return ret;
> }
> @@ -1761,7 +1762,7 @@ static int ps3_stor_add_adapter(struct ps3_stor_lv1_bus_info * lv1_bus_info)
> dev_info->host_info = host_info;
> INIT_LIST_HEAD(&dev_info->dev_list);
> spin_lock_init(&dev_info->srb_lock);
> - rwlock_init(&dev_info->bounce_lock);
> + init_rwsem(&dev_info->bounce_sem);
> list_add_tail(&dev_info->dev_list, &host_info->dev_info_list);
> }
>
> @@ -2165,7 +2166,7 @@ static int ps3_stor_slave_alloc(struct scsi_device * scsi_dev)
> FUNC_STEP_C("3");
>
> /* prepare dma regions for the device */
> - write_lock(&dev_info->bounce_lock);
> + down_write(&dev_info->bounce_sem);
> switch (get_dedicated_buffer_type(lv1_dev_info->device_type)) {
> case DEDICATED_KMALLOC:
> /*
> @@ -2176,7 +2177,7 @@ static int ps3_stor_slave_alloc(struct scsi_device * scsi_dev)
> /* create its own static bouce buffer */
> dev_info->dedicated_bounce_size = get_default_max_sector(lv1_dev_info) * lv1_dev_info->sector_size;
> dev_info->bounce_buf = kmalloc(dev_info->dedicated_bounce_size, GFP_KERNEL | __GFP_DMA);
> - write_unlock(&dev_info->bounce_lock);
> + up_write(&dev_info->bounce_sem);
> if (!dev_info->bounce_buf) {
> printk(KERN_ERR "%s:kmalloc for static bounce buffer failed %#x\n", __FUNCTION__,
> dev_info->dedicated_bounce_size);
> @@ -2195,7 +2196,7 @@ static int ps3_stor_slave_alloc(struct scsi_device * scsi_dev)
> error = -ENOMEM;
> goto fail_free_irq;
> }
> - write_unlock(&dev_info->bounce_lock);
> + up_write(&dev_info->bounce_sem);
> dev_info->bounce_type = DEDICATED_SPECIAL;
> break;
> }
> @@ -2422,9 +2423,9 @@ static ssize_t ps3_stor_get_max_sectors(struct device *dev,
> (struct ps3_stor_dev_info *)scsi_dev->hostdata;
> ssize_t ret;
>
> - read_lock(&dev_info->bounce_lock);
> + down_read(&dev_info->bounce_sem);
> ret = sprintf(buf, "%u\n", scsi_dev->request_queue->max_sectors);
> - read_unlock(&dev_info->bounce_lock);
> + up_read(&dev_info->bounce_sem);
> return ret;
> }
>
> @@ -2446,12 +2447,12 @@ static ssize_t ps3_stor_set_max_sectors(struct device *dev,
> /* FIXME: need remap dma region !!! */
> return -EINVAL;
> }
> - write_lock(&dev_info->bounce_lock);
> + down_write(&dev_info->bounce_sem);
> if (dev_info->bounce_type == DEDICATED_KMALLOC) {
> /* try to allocate new bounce buffer */
> bounce_buf = kmalloc(max_sectors * lv1_dev_info->sector_size, GFP_NOIO | __GFP_DMA | __GFP_NOWARN);
> if (!bounce_buf) {
> - write_unlock(&dev_info->bounce_lock);
> + up_write(&dev_info->bounce_sem);
> return -ENOMEM;
> }
> kfree(dev_info->bounce_buf);
> @@ -2459,7 +2460,7 @@ static ssize_t ps3_stor_set_max_sectors(struct device *dev,
> dev_info->dedicated_bounce_size = max_sectors * lv1_dev_info->sector_size;
> }
> blk_queue_max_sectors(scsi_dev->request_queue, max_sectors);
> - write_unlock(&dev_info->bounce_lock);
> + up_write(&dev_info->bounce_sem);
> return strlen(buf);
> }
> return -EINVAL;
> diff --git a/drivers/block/ps3_storage.h b/drivers/block/ps3_storage.h
> index 97011f4..ea83549 100644
> --- a/drivers/block/ps3_storage.h
> +++ b/drivers/block/ps3_storage.h
> @@ -163,11 +163,11 @@ struct ps3_stor_dev_info {
>
> u64 sector_size; /* copied from lv1 repository at initialize */
> /* devices may change these value */
> - rwlock_t bounce_lock; /* protect the following members:
> - * bounce_buf (pointer itself, not buffer),
> - * dedicated_bounce_size
> - * max_sectors in scsi_dev->request_queue
> - */
> + struct rw_semaphore bounce_sem; /* protect the following members:
> + * bounce_buf (pointer itself, not buffer),
> + * dedicated_bounce_size
> + * max_sectors in scsi_dev->request_queue
> + */
> int dedicated_bounce; /* set nonzero if the bounce buffer is dedicated */
> int dedicated_bounce_size;
> int dedicated_dma_region; /* set if partial dma region allocated */
>
>
>
>
More information about the cbe-oss-dev
mailing list