Re: [PATCH 6/6] mmc: stop abusing the request queue_lock pointer
On 16 November 2018 at 09:10, Christoph Hellwig wrote: > Replace 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 > Signed-off-by: Christoph Hellwig Looks good, thanks! Reviewed-by: Ulf Hansson Kind regards Uffe > --- > 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(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 70ec465beb69..2c329a3e3fdb 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -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(>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(>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(>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(>lock, flags); > > if (put_card) > mmc_put_card(mq->card, >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(>lock, flags); > mq->complete_req = req; > mq->rw_wait = false; > waiting = mq->waiting; > - spin_unlock_irqrestore(mq->lock, flags); > + spin_unlock_irqrestore(>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(>brq) || > mmc_blk_urgent_bkops_needed(mq, mqrq)) { > - spin_lock_irqsave(mq->lock, flags); > + spin_lock_irqsave(>lock, flags); > mq->recovery_needed = true; > mq->recovery_req = req; > - spin_unlock_irqrestore(mq->lock, flags); > + spin_unlock_irqrestore(>lock, flags); > wake_up(>wait); > schedule_work(>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(>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(>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(>lock); > INIT_LIST_HEAD(>part); > INIT_LIST_HEAD(>rpmbs); > md->usage = 1; > > - ret = mmc_init_queue(>queue, card, >lock); > + ret = mmc_init_queue(>queue, card); > if (ret) > goto err_putdisk; > > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index 4485cf12218c..35cc138b096d 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -89,9 +89,9 @@ void
Re: [PATCH 6/6] mmc: stop abusing the request queue_lock pointer
On Fri, Nov 16, 2018 at 09:10:06AM +0100, Christoph Hellwig wrote: > Replace 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. Looks sane to me, but I'll let the mmc people ack. > Suggested-by: Ulf Hansson > Signed-off-by: Christoph Hellwig > --- > 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(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 70ec465beb69..2c329a3e3fdb 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -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(>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(>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(>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(>lock, flags); > > if (put_card) > mmc_put_card(mq->card, >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(>lock, flags); > mq->complete_req = req; > mq->rw_wait = false; > waiting = mq->waiting; > - spin_unlock_irqrestore(mq->lock, flags); > + spin_unlock_irqrestore(>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(>brq) || > mmc_blk_urgent_bkops_needed(mq, mqrq)) { > - spin_lock_irqsave(mq->lock, flags); > + spin_lock_irqsave(>lock, flags); > mq->recovery_needed = true; > mq->recovery_req = req; > - spin_unlock_irqrestore(mq->lock, flags); > + spin_unlock_irqrestore(>lock, flags); > wake_up(>wait); > schedule_work(>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(>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(>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(>lock); > INIT_LIST_HEAD(>part); > INIT_LIST_HEAD(>rpmbs); > md->usage = 1; > > - ret = mmc_init_queue(>queue, card, >lock); > + ret = mmc_init_queue(>queue, card); > if (ret) > goto err_putdisk; > > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index 4485cf12218c..35cc138b096d 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -89,9 +89,9 @@ void mmc_cqe_recovery_notifier(struct mmc_request *mrq) > struct mmc_queue *mq = q->queuedata; > unsigned long flags; > > -