[5/6] ide: don't acquire queue_lock in ide_complete_pm_rq
diff mbox series

Message ID 20181116081006.5083-6-hch@lst.de
State New
Headers show
Series
  • [1/6] block: remove the rq_alloc_data request_queue field
Related show

Commit Message

Christoph Hellwig Nov. 16, 2018, 8:10 a.m. UTC
blk_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 <hch@lst.de>
---
 drivers/ide/ide-pm.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Omar Sandoval Nov. 16, 2018, 8:37 a.m. UTC | #1
On Fri, Nov 16, 2018 at 09:10:05AM +0100, Christoph Hellwig wrote:
> blk_mq_stop_hw_queues doesn't need any locking, and the ide
> dev_flags field isn't protected by it either.

Is it a bug that dev_flags is no longer protected by queue_lock after
the mq conversion?

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ide/ide-pm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
> index 56690f523100..192e6c65d34e 100644
> --- a/drivers/ide/ide-pm.c
> +++ b/drivers/ide/ide-pm.c
> @@ -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;
>  
> -- 
> 2.19.1
>
Christoph Hellwig Nov. 16, 2018, 8:40 a.m. UTC | #2
On Fri, Nov 16, 2018 at 12:37:32AM -0800, Omar Sandoval wrote:
> On Fri, Nov 16, 2018 at 09:10:05AM +0100, Christoph Hellwig wrote:
> > blk_mq_stop_hw_queues doesn't need any locking, and the ide
> > dev_flags field isn't protected by it either.
> 
> Is it a bug that dev_flags is no longer protected by queue_lock after
> 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 Nov. 16, 2018, 8:42 a.m. UTC | #3
On Fri, Nov 16, 2018 at 09:40:04AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 16, 2018 at 12:37:32AM -0800, Omar Sandoval wrote:
> > On Fri, Nov 16, 2018 at 09:10:05AM +0100, Christoph Hellwig wrote:
> > > blk_mq_stop_hw_queues doesn't need any locking, and the ide
> > > dev_flags field isn't protected by it either.
> > 
> > Is it a bug that dev_flags is no longer protected by queue_lock after
> > 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.

Good enough for me, you can add

Reviewed-by: Omar Sandoval <osandov@fb.com>

Patch
diff mbox series

diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index 56690f523100..192e6c65d34e 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -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;