| Message ID | 20210616053934.880951-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mmc: remove an extra blk_{get,put}_queue pair | expand |
On Wed, 16 Jun 2021 at 07:40, Christoph Hellwig <hch@lst.de> wrote: > > Use the blk_mq_alloc_disk to allocate the request_queue and gendisk > together. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Nice cleanup! Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/mmc/core/block.c | 14 +++----------- > drivers/mmc/core/queue.c | 23 ++++++++++------------- > drivers/mmc/core/queue.h | 2 +- > 3 files changed, 14 insertions(+), 25 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 947624e76c33..6652e1f1d4b2 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2310,27 +2310,21 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > */ > md->read_only = mmc_blk_readonly(card); > > - md->disk = alloc_disk(perdev_minors); > - if (md->disk == NULL) { > - ret = -ENOMEM; > + md->disk = mmc_init_queue(&md->queue, card); > + if (IS_ERR(md->disk)) { > + ret = PTR_ERR(md->disk); > goto err_kfree; > } > > INIT_LIST_HEAD(&md->part); > INIT_LIST_HEAD(&md->rpmbs); > md->usage = 1; > - > - ret = mmc_init_queue(&md->queue, card); > - if (ret) > - goto err_putdisk; > - > md->queue.blkdata = md; > > md->disk->major = MMC_BLOCK_MAJOR; > md->disk->first_minor = devidx * perdev_minors; > md->disk->fops = &mmc_bdops; > md->disk->private_data = md; > - md->disk->queue = md->queue.queue; > md->parent = parent; > set_disk_ro(md->disk, md->read_only || default_ro); > md->disk->flags = GENHD_FL_EXT_DEVT; > @@ -2379,8 +2373,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > > return md; > > - err_putdisk: > - put_disk(md->disk); > err_kfree: > kfree(md); > out: > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index d600e0a4a460..cc3261777637 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -424,9 +424,10 @@ static inline bool mmc_merge_capable(struct mmc_host *host) > * > * Initialise a MMC card request queue. > */ > -int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) > +struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) > { > struct mmc_host *host = card->host; > + struct gendisk *disk; > int ret; > > mq->card = card; > @@ -464,26 +465,22 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) > > ret = blk_mq_alloc_tag_set(&mq->tag_set); > if (ret) > - return ret; > + return ERR_PTR(ret); > + > > - mq->queue = blk_mq_init_queue(&mq->tag_set); > - if (IS_ERR(mq->queue)) { > - ret = PTR_ERR(mq->queue); > - goto free_tag_set; > + disk = blk_mq_alloc_disk(&mq->tag_set, mq); > + if (IS_ERR(disk)) { > + blk_mq_free_tag_set(&mq->tag_set); > + return disk; > } > + mq->queue = disk->queue; > > if (mmc_host_is_spi(host) && host->use_spi_crc) > blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, mq->queue); > - > - mq->queue->queuedata = mq; > blk_queue_rq_timeout(mq->queue, 60 * HZ); > > mmc_setup_queue(mq, card); > - return 0; > - > -free_tag_set: > - blk_mq_free_tag_set(&mq->tag_set); > - return ret; > + return disk; > } > > void mmc_queue_suspend(struct mmc_queue *mq) > diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h > index 3319d8ab57d0..9ade3bcbb714 100644 > --- a/drivers/mmc/core/queue.h > +++ b/drivers/mmc/core/queue.h > @@ -94,7 +94,7 @@ struct mmc_queue { > struct work_struct complete_work; > }; > > -extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *); > +struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *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.30.2 >
Hi On 16.06.2021 07:39, Christoph Hellwig wrote: > Use the blk_mq_alloc_disk to allocate the request_queue and gendisk > together. > > Signed-off-by: Christoph Hellwig <hch@lst.de> This patch landed recently in linux-next as commit 281ea6a5bfdc ("mmc: switch to blk_mq_alloc_disk"). It triggers the following warning during boot on all my systems with MMC devices: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 10 at block/genhd.c:491 __device_add_disk+0x4e8/0x630 Modules linked in: CPU: 0 PID: 10 Comm: kworker/0:1 Not tainted 5.13.0-rc3-00090-g281ea6a5bfdc #3542 Hardware name: Samsung Exynos (Flattened Device Tree) Workqueue: events_freezable mmc_rescan [<c0111844>] (unwind_backtrace) from [<c010d014>] (show_stack+0x10/0x14) [<c010d014>] (show_stack) from [<c0b6f84c>] (dump_stack+0xa4/0xc4) [<c0b6f84c>] (dump_stack) from [<c0127630>] (__warn+0x118/0x11c) [<c0127630>] (__warn) from [<c01276e8>] (warn_slowpath_fmt+0xb4/0xbc) [<c01276e8>] (warn_slowpath_fmt) from [<c052a52c>] (__device_add_disk+0x4e8/0x630) [<c052a52c>] (__device_add_disk) from [<c08a8e10>] (mmc_add_disk+0x24/0x120) [<c08a8e10>] (mmc_add_disk) from [<c08a9e68>] (mmc_blk_probe+0x420/0x65c) [<c08a9e68>] (mmc_blk_probe) from [<c06bece4>] (really_probe+0x250/0x4dc) [<c06bece4>] (really_probe) from [<c06beff4>] (driver_probe_device+0x84/0x1e4) [<c06beff4>] (driver_probe_device) from [<c06bcc70>] (bus_for_each_drv+0x78/0xbc) [<c06bcc70>] (bus_for_each_drv) from [<c06be9f4>] (__device_attach+0xe8/0x180) [<c06be9f4>] (__device_attach) from [<c06bda1c>] (bus_probe_device+0x88/0x90) [<c06bda1c>] (bus_probe_device) from [<c06bb5f0>] (device_add+0x4c8/0x874) [<c06bb5f0>] (device_add) from [<c089ac6c>] (mmc_add_card+0x110/0x2d0) [<c089ac6c>] (mmc_add_card) from [<c089ebd8>] (mmc_attach_mmc+0xc8/0x180) [<c089ebd8>] (mmc_attach_mmc) from [<c089a5c4>] (mmc_rescan+0x2ec/0x334) [<c089a5c4>] (mmc_rescan) from [<c014a994>] (process_one_work+0x30c/0x884) [<c014a994>] (process_one_work) from [<c014af64>] (worker_thread+0x58/0x594) [<c014af64>] (worker_thread) from [<c0151f34>] (kthread+0x154/0x19c) [<c0151f34>] (kthread) from [<c010011c>] (ret_from_fork+0x14/0x38) Exception stack(0xc1d25fb0 to 0xc1d25ff8) ... irq event stamp: 3653 hardirqs last enabled at (3667): [<c0100b4c>] __irq_svc+0x8c/0xb0 hardirqs last disabled at (3678): [<c0b749c4>] __schedule+0x54c/0x9dc softirqs last enabled at (3648): [<c0101790>] __do_softirq+0x528/0x63c softirqs last disabled at (3641): [<c01311b8>] irq_exit+0x214/0x220 ---[ end trace e60288107691b657 ]--- It looks that something is missing in this conversion. > --- > drivers/mmc/core/block.c | 14 +++----------- > drivers/mmc/core/queue.c | 23 ++++++++++------------- > drivers/mmc/core/queue.h | 2 +- > 3 files changed, 14 insertions(+), 25 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 947624e76c33..6652e1f1d4b2 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2310,27 +2310,21 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > */ > md->read_only = mmc_blk_readonly(card); > > - md->disk = alloc_disk(perdev_minors); > - if (md->disk == NULL) { > - ret = -ENOMEM; > + md->disk = mmc_init_queue(&md->queue, card); > + if (IS_ERR(md->disk)) { > + ret = PTR_ERR(md->disk); > goto err_kfree; > } > > INIT_LIST_HEAD(&md->part); > INIT_LIST_HEAD(&md->rpmbs); > md->usage = 1; > - > - ret = mmc_init_queue(&md->queue, card); > - if (ret) > - goto err_putdisk; > - > md->queue.blkdata = md; > > md->disk->major = MMC_BLOCK_MAJOR; > md->disk->first_minor = devidx * perdev_minors; > md->disk->fops = &mmc_bdops; > md->disk->private_data = md; > - md->disk->queue = md->queue.queue; > md->parent = parent; > set_disk_ro(md->disk, md->read_only || default_ro); > md->disk->flags = GENHD_FL_EXT_DEVT; > @@ -2379,8 +2373,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > > return md; > > - err_putdisk: > - put_disk(md->disk); > err_kfree: > kfree(md); > out: > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index d600e0a4a460..cc3261777637 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -424,9 +424,10 @@ static inline bool mmc_merge_capable(struct mmc_host *host) > * > * Initialise a MMC card request queue. > */ > -int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) > +struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) > { > struct mmc_host *host = card->host; > + struct gendisk *disk; > int ret; > > mq->card = card; > @@ -464,26 +465,22 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) > > ret = blk_mq_alloc_tag_set(&mq->tag_set); > if (ret) > - return ret; > + return ERR_PTR(ret); > + > > - mq->queue = blk_mq_init_queue(&mq->tag_set); > - if (IS_ERR(mq->queue)) { > - ret = PTR_ERR(mq->queue); > - goto free_tag_set; > + disk = blk_mq_alloc_disk(&mq->tag_set, mq); > + if (IS_ERR(disk)) { > + blk_mq_free_tag_set(&mq->tag_set); > + return disk; > } > + mq->queue = disk->queue; > > if (mmc_host_is_spi(host) && host->use_spi_crc) > blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, mq->queue); > - > - mq->queue->queuedata = mq; > blk_queue_rq_timeout(mq->queue, 60 * HZ); > > mmc_setup_queue(mq, card); > - return 0; > - > -free_tag_set: > - blk_mq_free_tag_set(&mq->tag_set); > - return ret; > + return disk; > } > > void mmc_queue_suspend(struct mmc_queue *mq) > diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h > index 3319d8ab57d0..9ade3bcbb714 100644 > --- a/drivers/mmc/core/queue.h > +++ b/drivers/mmc/core/queue.h > @@ -94,7 +94,7 @@ struct mmc_queue { > struct work_struct complete_work; > }; > > -extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *); > +struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *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 *); Best regards
On Mon, Jun 21, 2021 at 08:22:07AM +0200, Marek Szyprowski wrote: > Hi > > On 16.06.2021 07:39, Christoph Hellwig wrote: > > Use the blk_mq_alloc_disk to allocate the request_queue and gendisk > > together. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > This patch landed recently in linux-next as commit 281ea6a5bfdc ("mmc: > switch to blk_mq_alloc_disk"). It triggers the following warning during > boot on all my systems with MMC devices: Please try this, I lost this hunk in the final cleanup, sorry: diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index e7f89cbf9232..9890a1532cb0 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2331,6 +2331,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, md->queue.blkdata = md; md->disk->major = MMC_BLOCK_MAJOR; + md->disk->minors = perdev_minors; md->disk->first_minor = devidx * perdev_minors; md->disk->fops = &mmc_bdops; md->disk->private_data = md;
Hi Christoph, On 21.06.2021 08:59, Christoph Hellwig wrote: > On Mon, Jun 21, 2021 at 08:22:07AM +0200, Marek Szyprowski wrote: >> On 16.06.2021 07:39, Christoph Hellwig wrote: >>> Use the blk_mq_alloc_disk to allocate the request_queue and gendisk >>> together. >>> >>> Signed-off-by: Christoph Hellwig <hch@lst.de> >> This patch landed recently in linux-next as commit 281ea6a5bfdc ("mmc: >> switch to blk_mq_alloc_disk"). It triggers the following warning during >> boot on all my systems with MMC devices: > Please try this, I lost this hunk in the final cleanup, sorry: No problem. Right, it fixes the issue. Feel free to add: Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index e7f89cbf9232..9890a1532cb0 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2331,6 +2331,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > md->queue.blkdata = md; > > md->disk->major = MMC_BLOCK_MAJOR; > + md->disk->minors = perdev_minors; > md->disk->first_minor = devidx * perdev_minors; > md->disk->fops = &mmc_bdops; > md->disk->private_data = md; > Best regards
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 947624e76c33..6652e1f1d4b2 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2310,27 +2310,21 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, */ md->read_only = mmc_blk_readonly(card); - md->disk = alloc_disk(perdev_minors); - if (md->disk == NULL) { - ret = -ENOMEM; + md->disk = mmc_init_queue(&md->queue, card); + if (IS_ERR(md->disk)) { + ret = PTR_ERR(md->disk); goto err_kfree; } INIT_LIST_HEAD(&md->part); INIT_LIST_HEAD(&md->rpmbs); md->usage = 1; - - ret = mmc_init_queue(&md->queue, card); - if (ret) - goto err_putdisk; - md->queue.blkdata = md; md->disk->major = MMC_BLOCK_MAJOR; md->disk->first_minor = devidx * perdev_minors; md->disk->fops = &mmc_bdops; md->disk->private_data = md; - md->disk->queue = md->queue.queue; md->parent = parent; set_disk_ro(md->disk, md->read_only || default_ro); md->disk->flags = GENHD_FL_EXT_DEVT; @@ -2379,8 +2373,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, return md; - err_putdisk: - put_disk(md->disk); err_kfree: kfree(md); out: diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c index d600e0a4a460..cc3261777637 100644 --- a/drivers/mmc/core/queue.c +++ b/drivers/mmc/core/queue.c @@ -424,9 +424,10 @@ static inline bool mmc_merge_capable(struct mmc_host *host) * * Initialise a MMC card request queue. */ -int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) +struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) { struct mmc_host *host = card->host; + struct gendisk *disk; int ret; mq->card = card; @@ -464,26 +465,22 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card) ret = blk_mq_alloc_tag_set(&mq->tag_set); if (ret) - return ret; + return ERR_PTR(ret); + - mq->queue = blk_mq_init_queue(&mq->tag_set); - if (IS_ERR(mq->queue)) { - ret = PTR_ERR(mq->queue); - goto free_tag_set; + disk = blk_mq_alloc_disk(&mq->tag_set, mq); + if (IS_ERR(disk)) { + blk_mq_free_tag_set(&mq->tag_set); + return disk; } + mq->queue = disk->queue; if (mmc_host_is_spi(host) && host->use_spi_crc) blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, mq->queue); - - mq->queue->queuedata = mq; blk_queue_rq_timeout(mq->queue, 60 * HZ); mmc_setup_queue(mq, card); - return 0; - -free_tag_set: - blk_mq_free_tag_set(&mq->tag_set); - return ret; + return disk; } void mmc_queue_suspend(struct mmc_queue *mq) diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h index 3319d8ab57d0..9ade3bcbb714 100644 --- a/drivers/mmc/core/queue.h +++ b/drivers/mmc/core/queue.h @@ -94,7 +94,7 @@ struct mmc_queue { struct work_struct complete_work; }; -extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *); +struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *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 *);
Use the blk_mq_alloc_disk to allocate the request_queue and gendisk together. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/mmc/core/block.c | 14 +++----------- drivers/mmc/core/queue.c | 23 ++++++++++------------- drivers/mmc/core/queue.h | 2 +- 3 files changed, 14 insertions(+), 25 deletions(-)