queue_lock fixups
Subsystems (3)
Hi Jens,
a few fixups for the queue_lock conversion, drop a few more bogus queue_lock uses in drivers, and clean up the mmc use of the queue_lock as suggested by Ulf.
PATCH 1/6 block: remove the rq_alloc_data request_queue field
Signed-off-by Christoph Hellwig <[email protected]>
--- include/linux/blkdev.h | 1 - 1 file changed, 1 deletion(-)
include/linux/blkdev.h +0 -1
@@ -567,7 +567,6 @@ struct request_queue {bool mq_sysfs_init_done;size_t cmd_size; -void *rq_alloc_data;struct work_struct release_work;
Signature · 1 line
2.19.1
Re: [PATCH 1/6] block: remove the rq_alloc_data request_queue field
On Fri, Nov 16, 2018 at 09:10:01AM +0100, Christoph Hellwig wrote:
Reviewed-by Omar Sandoval <[email protected]>
Quoted 20 lines
Signed-off-by Christoph Hellwig <[email protected]>--- include/linux/blkdev.h | 1 - 1 file changed, 1 deletion(-)include/linux/blkdev.h +0
-1@@ -567,7 +567,6 @@ struct request_queue {bool mq_sysfs_init_done;size_t cmd_size; -void *rq_alloc_data;struct work_struct release_work;Signature · 1 line
2.19.1
PATCH 2/6 floppy: remove queue_lock around floppy_end_request
In reply to Christoph HellwigThere is nothing the queue_lock could protect inside floppy_end_request, so remove it.
Signed-off-by Christoph Hellwig <[email protected]>
--- drivers/block/floppy.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/block/floppy.c +0 -5
@@ -2254,10 +2254,7 @@ static void request_done(int uptodate)if (block > _floppy->sect)DRS ->maxtrack =1 ; -/* unlock chained buffers */ -spin_lock_irqsave (&q->queue_lock, flags);floppy_end_request (req,0 ); -spin_unlock_irqrestore (&q->queue_lock, flags); }else {if (rq_data_dir (req) ==WRITE ) {/* record write error information */
@@ -2269,9 +2266,7 @@ static void request_done(int uptodate)DRWE ->last_error_sector =blk_rq_pos (req);DRWE ->last_error_generation =DRS ->generation; } -spin_lock_irqsave (&q->queue_lock, flags);floppy_end_request (req,BLK_STS_IOERR ); -spin_unlock_irqrestore (&q->queue_lock, flags); } }
Signature · 1 line
2.19.1
Re: [PATCH 2/6] floppy: remove queue_lock around floppy_end_request
Quoted 2 lines · Christoph Hellwig
There is nothing the
queue_lockcould protect insidefloppy_end_request, so remove it.
Reviewed-by Omar Sandoval <[email protected]>
Quoted 33 lines
Signed-off-by Christoph Hellwig <[email protected]>--- drivers/block/floppy.c | 5 ----- 1 file changed, 5 deletions(-)drivers/block/floppy.c +0
-5@@ -2254,10 +2254,7 @@ static void request_done(int uptodate)if (block > _floppy->sect)DRS ->maxtrack =1 ; -/* unlock chained buffers */ -spin_lock_irqsave (&q->queue_lock, flags);floppy_end_request (req,0 ); -spin_unlock_irqrestore (&q->queue_lock, flags); }else {if (rq_data_dir (req) ==WRITE ) {/* record write error information */ @@ -2269,9 +2266,7 @@ static void request_done(int uptodate)DRWE ->last_error_sector =blk_rq_pos (req);DRWE ->last_error_generation =DRS ->generation; } -spin_lock_irqsave (&q->queue_lock, flags);floppy_end_request (req,BLK_STS_IOERR ); -spin_unlock_irqrestore (&q->queue_lock, flags); } }Signature · 1 line
2.19.1
PATCH 3/6 pktcdvd: remove queue_lock around blk_queue_max_hw_sectors
In reply to Christoph Hellwigblk_queue_max_hw_sectors can't do anything with queue_lock protection so don't hold it.
Signed-off-by Christoph Hellwig <[email protected]>
--- drivers/block/pktcdvd.c | 2 -- 1 file changed, 2 deletions(-)
drivers/block/pktcdvd.c +0 -2
@@ -2203,9 +2203,7 @@ static int pkt_open_dev(struct pktcdvd_device *pd, fmode_t write)* Some CDRW drives can not handle writes larger than one packet, * even if the size is a multiple of the packet size. */ -spin_lock_irq (&q->queue_lock);blk_queue_max_hw_sectors (q, pd->settings.size); -spin_unlock_irq (&q->queue_lock);set_bit (PACKET_WRITABLE , &pd->flags); }else {pkt_set_speed (pd,MAX_SPEED ,MAX_SPEED );
Signature · 1 line
2.19.1
Re: [PATCH 3/6] pktcdvd: remove queue_lock around blk_queue_max_hw_sectors
Quoted 2 lines · Christoph Hellwig
blk_queue_max_hw_sectorscan't do anything withqueue_lockprotection so don't hold it.
Reviewed-by Omar Sandoval <[email protected]>
Quoted 22 lines
Signed-off-by Christoph Hellwig <[email protected]>--- drivers/block/pktcdvd.c | 2 -- 1 file changed, 2 deletions(-)drivers/block/pktcdvd.c +0
-2@@ -2203,9 +2203,7 @@ static int pkt_open_dev(struct pktcdvd_device *pd, fmode_t write)* Some CDRW drives can not handle writes larger than one packet, * even if the size is a multiple of the packet size. */ -spin_lock_irq (&q->queue_lock);blk_queue_max_hw_sectors (q, pd->settings.size); -spin_unlock_irq (&q->queue_lock);set_bit (PACKET_WRITABLE , &pd->flags); }else {pkt_set_speed (pd,MAX_SPEED ,MAX_SPEED );Signature · 1 line
2.19.1
PATCH 4/6 ide: don't acquire queue lock in ide_pm_execute_rq
In reply to Christoph HellwigThere is nothing we can synchronize against over a call to blk_queue_dying.
Signed-off-by Christoph Hellwig <[email protected]>
--- drivers/ide/ide-pm.c | 3 --- 1 file changed, 3 deletions(-)
drivers/ide/ide-pm.c +0 -3
@@ -44,15 +44,12 @@ static int ide_pm_execute_rq(struct request *rq) {struct request_queue *q = rq->q; -spin_lock_irq (&q->queue_lock);if (unlikely (blk_queue_dying (q))) { rq->rq_flags |=RQF_QUIET ;scsi_req (rq)->result = -ENXIO ; -spin_unlock_irq (&q->queue_lock);blk_mq_end_request (rq,BLK_STS_OK );return -ENXIO ; } -spin_unlock_irq (&q->queue_lock);blk_execute_rq (q,NULL , rq, true);return scsi_req (rq)->result ? -EIO :0 ;
Signature · 1 line
2.19.1
Re: [PATCH 4/6] ide: don't acquire queue lock in ide_pm_execute_rq
Quoted 2 lines · Christoph Hellwig
There is nothing we can synchronize against over a call to
blk_queue_dying.
Reviewed-by Omar Sandoval <[email protected]>
Quoted 28 lines
Signed-off-by Christoph Hellwig <[email protected]>--- drivers/ide/ide-pm.c | 3 --- 1 file changed, 3 deletions(-)drivers/ide/ide-pm.c +0
-3@@ -44,15 +44,12 @@ static int ide_pm_execute_rq(struct request *rq) {struct request_queue *q = rq->q; -spin_lock_irq (&q->queue_lock);if (unlikely (blk_queue_dying (q))) { rq->rq_flags |=RQF_QUIET ;scsi_req (rq)->result = -ENXIO ; -spin_unlock_irq (&q->queue_lock);blk_mq_end_request (rq,BLK_STS_OK );return -ENXIO ; } -spin_unlock_irq (&q->queue_lock);blk_execute_rq (q,NULL , rq, true);return scsi_req (rq)->result ? -EIO :0 ;Signature · 1 line
2.19.1
PATCH 5/6 ide: don't acquire queue_lock in ide_complete_pm_rq
In reply to Christoph Hellwigblk_mq_stop_hw_queues doesn't need any locking, and the ide dev_flags field isn't protected by it either.
Signed-off-by Christoph Hellwig <[email protected]>
--- drivers/ide/ide-pm.c | 3 --- 1 file changed, 3 deletions(-)
drivers/ide/ide-pm.c +0 -3
@@ -201,7 +201,6 @@ void ide_complete_pm_rq(ide_drive_t *drive, struct request *rq) {struct request_queue *q = drive->queue;struct ide_pm_state *pm =ide_req (rq)->special; -unsigned long flags;ide_complete_power_step (drive, rq);if (pm->pm_step !=IDE_PM_COMPLETED )
@@ -211,12 +210,10 @@ void ide_complete_pm_rq(ide_drive_t *drive, struct request *rq)printk ("%s: completing PM request, %s\n" , drive->name, (ide_req (rq)->type ==ATA_PRIV_PM_SUSPEND ) ?"suspend" :"resume" );#endif -spin_lock_irqsave (&q->queue_lock, flags);if (ide_req (rq)->type ==ATA_PRIV_PM_SUSPEND )blk_mq_stop_hw_queues (q);else drive->dev_flags &= ~IDE_DFLAG_BLOCKED ; -spin_unlock_irqrestore (&q->queue_lock, flags); drive->hwif->rq =NULL ;
Signature · 1 line
2.19.1
Re: [PATCH 5/6] ide: don't acquire queue_lock in ide_complete_pm_rq
Quoted 2 lines · Christoph Hellwig
blk_mq_stop_hw_queuesdoesn't need any locking, and the idedev_flagsfield isn't protected by it either.
Is it a bug that dev_flags is no longer protected by queue_lock after the mq conversion?
Quoted 33 lines
Signed-off-by Christoph Hellwig <[email protected]>--- drivers/ide/ide-pm.c | 3 --- 1 file changed, 3 deletions(-)drivers/ide/ide-pm.c +0
-3@@ -201,7 +201,6 @@ void ide_complete_pm_rq(ide_drive_t *drive, struct request *rq) {struct request_queue *q = drive->queue;struct ide_pm_state *pm =ide_req (rq)->special; -unsigned long flags;ide_complete_power_step (drive, rq);if (pm->pm_step !=IDE_PM_COMPLETED )@@ -211,12 +210,10 @@ void ide_complete_pm_rq(ide_drive_t *drive, struct request *rq)printk ("%s: completing PM request, %s\n" , drive->name, (ide_req (rq)->type ==ATA_PRIV_PM_SUSPEND ) ?"suspend" :"resume" );#endif -spin_lock_irqsave (&q->queue_lock, flags);if (ide_req (rq)->type ==ATA_PRIV_PM_SUSPEND )blk_mq_stop_hw_queues (q);else drive->dev_flags &= ~IDE_DFLAG_BLOCKED ; -spin_unlock_irqrestore (&q->queue_lock, flags); drive->hwif->rq =NULL ;Signature · 1 line
2.19.1
Re: [PATCH 5/6] ide: don't acquire queue_lock in ide_complete_pm_rq
Quoted 6 lines · Omar Sandoval
Quoted 2 lines · Christoph Hellwig
blk_mq_stop_hw_queuesdoesn't need any locking, and the idedev_flagsfield isn't protected by it either.Is it a bug that
dev_flagsis no longer protected byqueue_lockafter the mq conversion?
Most if not all users weren't under queue_lock before. As far as I can tell it generally is set in the probe path, and without any obvious locking.
Omar Sandoval
Reviewed-byRe: [PATCH 5/6] ide: don't acquire queue_lock in ide_complete_pm_rq
Quoted 11 lines · Christoph Hellwig
Quoted 6 lines · Omar Sandoval
Quoted 2 lines · Christoph Hellwig
blk_mq_stop_hw_queuesdoesn't need any locking, and the idedev_flagsfield isn't protected by it either.Is it a bug that
dev_flagsis no longer protected byqueue_lockafter the mq conversion?Most if not all users weren't under
queue_lockbefore. As far as I can tell it generally is set in the probe path, and without any obvious locking.
Good enough for me, you can add
Reviewed-by Omar Sandoval <[email protected]>
PATCH 6/6 mmc: stop abusing the request queue_lock pointer
In reply to Christoph HellwigReplace the lock in mmc_blk_data that is only used through a pointer in struct mmc_queue and to protect fields in that structure with an actual lock in struct mmc_queue.
Suggested-by Ulf Hansson <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>
--- drivers/mmc/core/block.c | 24 +++++++++++------------- drivers/mmc/core/queue.c | 31 +++++++++++++++---------------- drivers/mmc/core/queue.h | 4 ++-- 3 files changed, 28 insertions(+), 31 deletions(-)
drivers/mmc/core/block.c +11 -13
@@ -100,7 +100,6 @@ static DEFINE_IDA(mmc_rpmb_ida);* There is one mmc_blk_data per slot. */ struct mmc_blk_data { -spinlock_t lock;struct device *parent;struct gendisk *disk;struct mmc_queue queue;
@@ -1483,7 +1482,7 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)blk_mq_end_request (req,BLK_STS_OK ); } -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->in_flight[mmc_issue_type (mq, req)] -=1 ;
@@ -1491,7 +1490,7 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)mmc_cqe_check_busy (mq); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);if (!mq->cqe_busy)blk_mq_run_hw_queues (q, true);
@@ -1991,13 +1990,13 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)unsigned long flags;bool put_card; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->in_flight[mmc_issue_type (mq, req)] -=1 ; put_card = (mmc_tot_in_flight (mq) ==0 ); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);if (put_card)mmc_put_card (mq->card, &mq->ctx);
@@ -2093,11 +2092,11 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)* request does not need to wait (although it does need to * complete complete_req first). */ -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->complete_req = req; mq->rw_wait = false; waiting = mq->waiting; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);/* * If 'waiting' then the waiting task will complete this
@@ -2116,10 +2115,10 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)/* Take the recovery path for errors or urgent background operations */ if (mmc_blk_rq_error (&mqrq->brq) ||mmc_blk_urgent_bkops_needed (mq, mqrq)) { -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->recovery_needed = true; mq->recovery_req = req; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);wake_up (&mq->wait);schedule_work (&mq->recovery_work);return ;
@@ -2142,7 +2141,7 @@ static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)* Wait while there is another request in progress, but not if recovery * is needed. Also indicate whether there is a request waiting to start. */ -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);if (mq->recovery_needed) { *err = -EBUSY ; done = true;
@@ -2150,7 +2149,7 @@ static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err) done = !mq->rw_wait; } mq->waiting = !done; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);return done; }
@@ -2327,12 +2326,11 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, goto err_kfree; } -spin_lock_init (&md->lock);INIT_LIST_HEAD (&md->part);INIT_LIST_HEAD (&md->rpmbs); md->usage =1 ; - ret =mmc_init_queue (&md->queue, card, &md->lock); + ret =mmc_init_queue (&md->queue, card);if (ret) goto err_putdisk;
drivers/mmc/core/queue.c +15 -16
@@ -89,9 +89,9 @@ void mmc_cqe_recovery_notifier(struct mmc_request *mrq)struct mmc_queue *mq = q->queuedata;unsigned long flags; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);__mmc_cqe_recovery_notifier (mq); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags); }static enum blk_eh_timer_return mmc_cqe_timed_out (struct request *req)
@@ -128,14 +128,14 @@ static enum blk_eh_timer_return mmc_mq_timed_out(struct request *req,unsigned long flags;int ret; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);if (mq->recovery_needed || !mq->use_cqe) ret =BLK_EH_RESET_TIMER ;else ret =mmc_cqe_timed_out (req); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);return ret; }
@@ -157,9 +157,9 @@ static void mmc_mq_recovery_handler(struct work_struct *work) mq->in_recovery = false; -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock); mq->recovery_needed = false; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);mmc_put_card (mq->card, &mq->ctx);
@@ -258,10 +258,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx, issue_type =mmc_issue_type (mq, req); -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock);if (mq->recovery_needed || mq->busy) { -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);return BLK_STS_RESOURCE ; }
@@ -269,7 +269,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,case MMC_ISSUE_DCMD :if (mmc_cqe_dcmd_busy (mq)) { mq->cqe_busy |=MMC_CQE_DCMD_BUSY ; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);return BLK_STS_RESOURCE ; }break ;
@@ -294,7 +294,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx, get_card = (mmc_tot_in_flight (mq) ==1 ); cqe_retune_ok = (mmc_cqe_qcnt (mq) ==1 ); -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);if (!(req->rq_flags &RQF_DONTPREP )) {req_to_mmc_queue_req (req)->retries =0 ;
@@ -328,12 +328,12 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,if (issued !=MMC_REQ_STARTED ) {bool put_card = false; -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock); mq->in_flight[issue_type] -=1 ;if (mmc_tot_in_flight (mq) ==0 ) put_card = true; mq->busy = false; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);if (put_card)mmc_put_card (card, &mq->ctx); }else {
@@ -385,19 +385,18 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)* mmc_init_queue - initialise a queue structure. * @mq: mmc queue * @card: mmc card to attach this queue -* @lock: queue lock * * Initialise a MMC card request queue. */ -int mmc_init_queue (struct mmc_queue *mq,struct mmc_card *card, -spinlock_t *lock) +int mmc_init_queue (struct mmc_queue *mq,struct mmc_card *card) {struct mmc_host *host = card->host;int ret; mq->card = card; - mq->lock = lock; mq->use_cqe = host->cqe_enabled; + +spin_lock_init (&mq->lock);memset (&mq->tag_set,0 ,sizeof (mq->tag_set)); mq->tag_set.ops = &mmc_mq_ops;
drivers/mmc/core/queue.h +2 -2
@@ -73,11 +73,11 @@ struct mmc_queue_req {struct mmc_queue {struct mmc_card *card; -spinlock_t *lock;struct mmc_ctx ctx;struct blk_mq_tag_set tag_set;struct mmc_blk_data *blkdata;struct request_queue *queue; +spinlock_t lock;int in_flight[MMC_ISSUE_MAX ];unsigned int cqe_busy;#define MMC_CQE_DCMD_BUSY BIT(0)
@@ -96,7 +96,7 @@ struct mmc_queue {struct work_struct complete_work; }; -extern int mmc_init_queue (struct mmc_queue *,struct mmc_card *,spinlock_t *); +extern int mmc_init_queue (struct mmc_queue *,struct mmc_card *);extern void mmc_cleanup_queue (struct mmc_queue *);extern void mmc_queue_suspend (struct mmc_queue *);extern void mmc_queue_resume (struct mmc_queue *);
Signature · 1 line
2.19.1
Re: [PATCH 6/6] mmc: stop abusing the request queue_lock pointer
Quoted 3 lines · Christoph Hellwig
Replace the lock in
mmc_blk_datathat is only used through a pointer in structmmc_queueand to protect fields in that structure with an actual lock in structmmc_queue.
Looks sane to me, but I'll let the mmc people ack.
Quoted 256 lines
Suggested-by Ulf Hansson <[email protected]>Signed-off-by Christoph Hellwig <[email protected]>--- drivers/mmc/core/block.c | 24 +++++++++++------------- drivers/mmc/core/queue.c | 31 +++++++++++++++---------------- drivers/mmc/core/queue.h | 4 ++-- 3 files changed, 28 insertions(+), 31 deletions(-)drivers/mmc/core/block.c +11
-13@@ -100,7 +100,6 @@ static DEFINE_IDA(mmc_rpmb_ida);* There is one mmc_blk_data per slot. */ struct mmc_blk_data { -spinlock_t lock;struct device *parent;struct gendisk *disk;struct mmc_queue queue;@@ -1483,7 +1482,7 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)blk_mq_end_request (req,BLK_STS_OK ); } -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->in_flight[mmc_issue_type (mq, req)] -=1 ;@@ -1491,7 +1490,7 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)mmc_cqe_check_busy (mq); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);if (!mq->cqe_busy)blk_mq_run_hw_queues (q, true);@@ -1991,13 +1990,13 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)unsigned long flags;bool put_card; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->in_flight[mmc_issue_type (mq, req)] -=1 ; put_card = (mmc_tot_in_flight (mq) ==0 ); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);if (put_card)mmc_put_card (mq->card, &mq->ctx);@@ -2093,11 +2092,11 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)* request does not need to wait (although it does need to * complete complete_req first). */ -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->complete_req = req; mq->rw_wait = false; waiting = mq->waiting; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);/* * If 'waiting' then the waiting task will complete this @@ -2116,10 +2115,10 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)/* Take the recovery path for errors or urgent background operations */ if (mmc_blk_rq_error (&mqrq->brq) ||mmc_blk_urgent_bkops_needed (mq, mqrq)) { -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->recovery_needed = true; mq->recovery_req = req; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);wake_up (&mq->wait);schedule_work (&mq->recovery_work);return ;@@ -2142,7 +2141,7 @@ static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)* Wait while there is another request in progress, but not if recovery * is needed. Also indicate whether there is a request waiting to start. */ -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);if (mq->recovery_needed) { *err = -EBUSY ; done = true;@@ -2150,7 +2149,7 @@ static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err) done = !mq->rw_wait; } mq->waiting = !done; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);return done; }@@ -2327,12 +2326,11 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, goto err_kfree; } -spin_lock_init (&md->lock);INIT_LIST_HEAD (&md->part);INIT_LIST_HEAD (&md->rpmbs); md->usage =1 ; - ret =mmc_init_queue (&md->queue, card, &md->lock); + ret =mmc_init_queue (&md->queue, card);if (ret) goto err_putdisk;drivers/mmc/core/queue.c +15
-16@@ -89,9 +89,9 @@ void mmc_cqe_recovery_notifier(struct mmc_request *mrq)struct mmc_queue *mq = q->queuedata;unsigned long flags; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);__mmc_cqe_recovery_notifier (mq); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags); }static enum blk_eh_timer_return mmc_cqe_timed_out (struct request *req)@@ -128,14 +128,14 @@ static enum blk_eh_timer_return mmc_mq_timed_out(struct request *req,unsigned long flags;int ret; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);if (mq->recovery_needed || !mq->use_cqe) ret =BLK_EH_RESET_TIMER ;else ret =mmc_cqe_timed_out (req); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);return ret; }@@ -157,9 +157,9 @@ static void mmc_mq_recovery_handler(struct work_struct *work) mq->in_recovery = false; -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock); mq->recovery_needed = false; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);mmc_put_card (mq->card, &mq->ctx);@@ -258,10 +258,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx, issue_type =mmc_issue_type (mq, req); -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock);if (mq->recovery_needed || mq->busy) { -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);return BLK_STS_RESOURCE ; }@@ -269,7 +269,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,case MMC_ISSUE_DCMD :if (mmc_cqe_dcmd_busy (mq)) { mq->cqe_busy |=MMC_CQE_DCMD_BUSY ; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);return BLK_STS_RESOURCE ; }break ;@@ -294,7 +294,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx, get_card = (mmc_tot_in_flight (mq) ==1 ); cqe_retune_ok = (mmc_cqe_qcnt (mq) ==1 ); -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);if (!(req->rq_flags &RQF_DONTPREP )) {req_to_mmc_queue_req (req)->retries =0 ;@@ -328,12 +328,12 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,if (issued !=MMC_REQ_STARTED ) {bool put_card = false; -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock); mq->in_flight[issue_type] -=1 ;if (mmc_tot_in_flight (mq) ==0 ) put_card = true; mq->busy = false; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);if (put_card)mmc_put_card (card, &mq->ctx); }else {@@ -385,19 +385,18 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)* mmc_init_queue - initialise a queue structure. * @mq: mmc queue * @card: mmc card to attach this queue -* @lock: queue lock * * Initialise a MMC card request queue. */ -int mmc_init_queue (struct mmc_queue *mq,struct mmc_card *card, -spinlock_t *lock) +int mmc_init_queue (struct mmc_queue *mq,struct mmc_card *card) {struct mmc_host *host = card->host;int ret; mq->card = card; - mq->lock = lock; mq->use_cqe = host->cqe_enabled; + +spin_lock_init (&mq->lock);memset (&mq->tag_set,0 ,sizeof (mq->tag_set)); mq->tag_set.ops = &mmc_mq_ops;drivers/mmc/core/queue.h +2
-2@@ -73,11 +73,11 @@ struct mmc_queue_req {struct mmc_queue {struct mmc_card *card; -spinlock_t *lock;struct mmc_ctx ctx;struct blk_mq_tag_set tag_set;struct mmc_blk_data *blkdata;struct request_queue *queue; +spinlock_t lock;int in_flight[MMC_ISSUE_MAX ];unsigned int cqe_busy;#define MMC_CQE_DCMD_BUSY BIT(0)@@ -96,7 +96,7 @@ struct mmc_queue {struct work_struct complete_work; }; -extern int mmc_init_queue (struct mmc_queue *,struct mmc_card *,spinlock_t *); +extern int mmc_init_queue (struct mmc_queue *,struct mmc_card *);extern void mmc_cleanup_queue (struct mmc_queue *);extern void mmc_queue_suspend (struct mmc_queue *);extern void mmc_queue_resume (struct mmc_queue *);Signature · 1 line
2.19.1
Re: [PATCH 6/6] mmc: stop abusing the request queue_lock pointer
In reply to Christoph HellwigQuoted 6 lines · Christoph Hellwig
Replace the lock in
mmc_blk_datathat is only used through a pointer in structmmc_queueand to protect fields in that structure with an actual lock in structmmc_queue.Suggested-by Ulf Hansson <[email protected]>Signed-off-by Christoph Hellwig <[email protected]>
Looks good, thanks!
Reviewed-by Ulf Hansson <[email protected]>
Kind regards
Uffe
Quoted 254 lines
--- drivers/mmc/core/block.c | 24 +++++++++++------------- drivers/mmc/core/queue.c | 31 +++++++++++++++---------------- drivers/mmc/core/queue.h | 4 ++-- 3 files changed, 28 insertions(+), 31 deletions(-)drivers/mmc/core/block.c +11
-13@@ -100,7 +100,6 @@ static DEFINE_IDA(mmc_rpmb_ida);* There is one mmc_blk_data per slot. */ struct mmc_blk_data { -spinlock_t lock;struct device *parent;struct gendisk *disk;struct mmc_queue queue;@@ -1483,7 +1482,7 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)blk_mq_end_request (req,BLK_STS_OK ); } -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->in_flight[mmc_issue_type (mq, req)] -=1 ;@@ -1491,7 +1490,7 @@ static void mmc_blk_cqe_complete_rq(struct mmc_queue *mq, struct request *req)mmc_cqe_check_busy (mq); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);if (!mq->cqe_busy)blk_mq_run_hw_queues (q, true);@@ -1991,13 +1990,13 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)unsigned long flags;bool put_card; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->in_flight[mmc_issue_type (mq, req)] -=1 ; put_card = (mmc_tot_in_flight (mq) ==0 ); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);if (put_card)mmc_put_card (mq->card, &mq->ctx);@@ -2093,11 +2092,11 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)* request does not need to wait (although it does need to * complete complete_req first). */ -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->complete_req = req; mq->rw_wait = false; waiting = mq->waiting; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);/* * If 'waiting' then the waiting task will complete this @@ -2116,10 +2115,10 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)/* Take the recovery path for errors or urgent background operations */ if (mmc_blk_rq_error (&mqrq->brq) ||mmc_blk_urgent_bkops_needed (mq, mqrq)) { -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags); mq->recovery_needed = true; mq->recovery_req = req; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);wake_up (&mq->wait);schedule_work (&mq->recovery_work);return ;@@ -2142,7 +2141,7 @@ static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)* Wait while there is another request in progress, but not if recovery * is needed. Also indicate whether there is a request waiting to start. */ -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);if (mq->recovery_needed) { *err = -EBUSY ; done = true;@@ -2150,7 +2149,7 @@ static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err) done = !mq->rw_wait; } mq->waiting = !done; -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);return done; }@@ -2327,12 +2326,11 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, goto err_kfree; } -spin_lock_init (&md->lock);INIT_LIST_HEAD (&md->part);INIT_LIST_HEAD (&md->rpmbs); md->usage =1 ; - ret =mmc_init_queue (&md->queue, card, &md->lock); + ret =mmc_init_queue (&md->queue, card);if (ret) goto err_putdisk;drivers/mmc/core/queue.c +15
-16@@ -89,9 +89,9 @@ void mmc_cqe_recovery_notifier(struct mmc_request *mrq)struct mmc_queue *mq = q->queuedata;unsigned long flags; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);__mmc_cqe_recovery_notifier (mq); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags); }static enum blk_eh_timer_return mmc_cqe_timed_out (struct request *req)@@ -128,14 +128,14 @@ static enum blk_eh_timer_return mmc_mq_timed_out(struct request *req,unsigned long flags;int ret; -spin_lock_irqsave (mq->lock, flags); +spin_lock_irqsave (&mq->lock, flags);if (mq->recovery_needed || !mq->use_cqe) ret =BLK_EH_RESET_TIMER ;else ret =mmc_cqe_timed_out (req); -spin_unlock_irqrestore (mq->lock, flags); +spin_unlock_irqrestore (&mq->lock, flags);return ret; }@@ -157,9 +157,9 @@ static void mmc_mq_recovery_handler(struct work_struct *work) mq->in_recovery = false; -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock); mq->recovery_needed = false; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);mmc_put_card (mq->card, &mq->ctx);@@ -258,10 +258,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx, issue_type =mmc_issue_type (mq, req); -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock);if (mq->recovery_needed || mq->busy) { -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);return BLK_STS_RESOURCE ; }@@ -269,7 +269,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,case MMC_ISSUE_DCMD :if (mmc_cqe_dcmd_busy (mq)) { mq->cqe_busy |=MMC_CQE_DCMD_BUSY ; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);return BLK_STS_RESOURCE ; }break ;@@ -294,7 +294,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx, get_card = (mmc_tot_in_flight (mq) ==1 ); cqe_retune_ok = (mmc_cqe_qcnt (mq) ==1 ); -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);if (!(req->rq_flags &RQF_DONTPREP )) {req_to_mmc_queue_req (req)->retries =0 ;@@ -328,12 +328,12 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,if (issued !=MMC_REQ_STARTED ) {bool put_card = false; -spin_lock_irq (mq->lock); +spin_lock_irq (&mq->lock); mq->in_flight[issue_type] -=1 ;if (mmc_tot_in_flight (mq) ==0 ) put_card = true; mq->busy = false; -spin_unlock_irq (mq->lock); +spin_unlock_irq (&mq->lock);if (put_card)mmc_put_card (card, &mq->ctx); }else {@@ -385,19 +385,18 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)* mmc_init_queue - initialise a queue structure. * @mq: mmc queue * @card: mmc card to attach this queue -* @lock: queue lock * * Initialise a MMC card request queue. */ -int mmc_init_queue (struct mmc_queue *mq,struct mmc_card *card, -spinlock_t *lock) +int mmc_init_queue (struct mmc_queue *mq,struct mmc_card *card) {struct mmc_host *host = card->host;int ret; mq->card = card; - mq->lock = lock; mq->use_cqe = host->cqe_enabled; + +spin_lock_init (&mq->lock);memset (&mq->tag_set,0 ,sizeof (mq->tag_set)); mq->tag_set.ops = &mmc_mq_ops;drivers/mmc/core/queue.h +2
-3@@ -73,11 +73,11 @@ struct mmc_queue_req {struct mmc_queue {struct mmc_card *card; -spinlock_t *lock;struct mmc_ctx ctx;struct blk_mq_tag_set tag_set;struct mmc_blk_data *blkdata;struct request_queue *queue; +spinlock_t lock;int in_flight[MMC_ISSUE_MAX ];unsigned int cqe_busy;#define MMC_CQE_DCMD_BUSY BIT(0)@@ -96,7 +96,7 @@ struct mmc_queue {struct work_struct complete_work; }; -extern int mmc_init_queue (struct mmc_queue *,struct mmc_card *,spinlock_t *); +extern int mmc_init_queue (struct mmc_queue *,struct mmc_card *);extern void mmc_cleanup_queue (struct mmc_queue *);extern void mmc_queue_suspend (struct mmc_queue *);extern void mmc_queue_resume (struct mmc_queue *); --2.19.1
Jens Axboe
maintainerQuoted 5 lines · Christoph Hellwig
Hi Jens,
a few fixups for the
queue_lockconversion, drop a few more bogusqueue_lockuses in drivers, and clean up the mmc use of thequeue_lockas suggested by Ulf.
Applied 1-5 for now, giving Ulf a chance to checkout #6.
Signature · 1 line
Jens Axboe