Re: [PATCH 17/17] mmc: pass queue_limits to blk_mq_alloc_disk

[Jon Hunter <jonathanh@nvidia.com>]

Hi Christoph,

On 15/02/2024 07:03, Christoph Hellwig wrote:
> Pass the queue limit set at initialization time directly to
> blk_mq_alloc_disk instead of updating it right after the allocation.
> 
> This requires refactoring the code a bit so that what was mmc_setup_queue
> before also allocates the gendisk now and actually sets all limits.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/mmc/core/queue.c | 97 +++++++++++++++++++++-------------------
>   1 file changed, 52 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index 67ad186d132a69..2ae60d208cdf1e 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -174,8 +174,8 @@ static struct scatterlist *mmc_alloc_sg(unsigned short sg_len, gfp_t gfp)
>   	return sg;
>   }
>   
> -static void mmc_queue_setup_discard(struct request_queue *q,
> -				    struct mmc_card *card)
> +static void mmc_queue_setup_discard(struct mmc_card *card,
> +		struct queue_limits *lim)
>   {
>   	unsigned max_discard;
>   
> @@ -183,15 +183,17 @@ static void mmc_queue_setup_discard(struct request_queue *q,
>   	if (!max_discard)
>   		return;
>   
> -	blk_queue_max_discard_sectors(q, max_discard);
> -	q->limits.discard_granularity = card->pref_erase << 9;
> -	/* granularity must not be greater than max. discard */
> -	if (card->pref_erase > max_discard)
> -		q->limits.discard_granularity = SECTOR_SIZE;
> +	lim->max_hw_discard_sectors = max_discard;
>   	if (mmc_can_secure_erase_trim(card))
> -		blk_queue_max_secure_erase_sectors(q, max_discard);
> +		lim->max_secure_erase_sectors = max_discard;
>   	if (mmc_can_trim(card) && card->erased_byte == 0)
> -		blk_queue_max_write_zeroes_sectors(q, max_discard);
> +		lim->max_write_zeroes_sectors = max_discard;
> +
> +	/* granularity must not be greater than max. discard */
> +	if (card->pref_erase > max_discard)
> +		lim->discard_granularity = SECTOR_SIZE;
> +	else
> +		lim->discard_granularity = card->pref_erase << 9;
>   }
>   
>   static unsigned short mmc_get_max_segments(struct mmc_host *host)
> @@ -341,40 +343,53 @@ static const struct blk_mq_ops mmc_mq_ops = {
>   	.timeout	= mmc_mq_timed_out,
>   };
>   
> -static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> +static struct gendisk *mmc_alloc_disk(struct mmc_queue *mq,
> +		struct mmc_card *card)
>   {
>   	struct mmc_host *host = card->host;
> -	unsigned block_size = 512;
> +	struct queue_limits lim = { };
> +	struct gendisk *disk;
>   
> -	blk_queue_flag_set(QUEUE_FLAG_NONROT, mq->queue);
> -	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, mq->queue);
>   	if (mmc_can_erase(card))
> -		mmc_queue_setup_discard(mq->queue, card);
> +		mmc_queue_setup_discard(card, &lim);
>   
>   	if (!mmc_dev(host)->dma_mask || !*mmc_dev(host)->dma_mask)
> -		blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_HIGH);
> -	blk_queue_max_hw_sectors(mq->queue,
> -		min(host->max_blk_count, host->max_req_size / 512));
> -	if (host->can_dma_map_merge)
> -		WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
> -							mmc_dev(host)),
> -		     "merging was advertised but not possible");
> -	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> -
> -	if (mmc_card_mmc(card) && card->ext_csd.data_sector_size) {
> -		block_size = card->ext_csd.data_sector_size;
> -		WARN_ON(block_size != 512 && block_size != 4096);
> -	}
> +		lim.bounce = BLK_BOUNCE_HIGH;
> +
> +	lim.max_hw_sectors = min(host->max_blk_count, host->max_req_size / 512);
> +
> +	if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
> +		lim.logical_block_size = card->ext_csd.data_sector_size;
> +	else
> +		lim.logical_block_size = 512;
> +
> +	WARN_ON_ONCE(lim.logical_block_size != 512 &&
> +		     lim.logical_block_size != 4096);
>   
> -	blk_queue_logical_block_size(mq->queue, block_size);
>   	/*
> -	 * After blk_queue_can_use_dma_map_merging() was called with succeed,
> -	 * since it calls blk_queue_virt_boundary(), the mmc should not call
> -	 * both blk_queue_max_segment_size().
> +	 * Setting a virt_boundary implicity sets a max_segment_size, so try
> +	 * to set the hardware one here.
>   	 */
> -	if (!host->can_dma_map_merge)
> -		blk_queue_max_segment_size(mq->queue,
> -			round_down(host->max_seg_size, block_size));
> +	if (host->can_dma_map_merge) {
> +		lim.virt_boundary_mask = dma_get_merge_boundary(mmc_dev(host));
> +		lim.max_segments = MMC_DMA_MAP_MERGE_SEGMENTS;
> +	} else {
> +		lim.max_segment_size =
> +			round_down(host->max_seg_size, lim.logical_block_size);
> +		lim.max_segments = host->max_segs;
> +	}
> +
> +	disk = blk_mq_alloc_disk(&mq->tag_set, &lim, mq);
> +	if (IS_ERR(disk))
> +		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);
> +	blk_queue_rq_timeout(mq->queue, 60 * HZ);
> +
> +	blk_queue_flag_set(QUEUE_FLAG_NONROT, mq->queue);
> +	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, mq->queue);
>   
>   	dma_set_max_seg_size(mmc_dev(host), queue_max_segment_size(mq->queue));
>   
> @@ -386,6 +401,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>   	init_waitqueue_head(&mq->wait);
>   
>   	mmc_crypto_setup_queue(mq->queue, host);
> +	return disk;
>   }
>   
>   static inline bool mmc_merge_capable(struct mmc_host *host)
> @@ -447,18 +463,9 @@ struct gendisk *mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card)
>   		return ERR_PTR(ret);
>   		
>   
> -	disk = blk_mq_alloc_disk(&mq->tag_set, NULL, mq);
> -	if (IS_ERR(disk)) {
> +	disk = mmc_alloc_disk(mq, card);
> +	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);
> -	blk_queue_rq_timeout(mq->queue, 60 * HZ);
> -
> -	mmc_setup_queue(mq, card);
>   	return disk;
>   }
>   


We have just noticed that since Linux v6.9 was released, that if we
build the kernel with 64kB MMU pages, then we see the following WARNING
and probe failure ...

[    2.828612] ------------[ cut here ]------------
[    2.829243] WARNING: CPU: 1 PID: 87 at block/blk-settings.c:206 blk_validate_limits+0x1cc/0x234
[    2.830417] Modules linked in:
[    2.830963] CPU: 1 PID: 87 Comm: kworker/1:1 Not tainted 6.10.0-rc5 #1
[    2.831773] Hardware name: NVIDIA Jetson AGX Xavier Developer Kit (DT)
[    2.832538] Workqueue: events_freezable mmc_rescan
[    2.833341] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    2.834263] pc : blk_validate_limits+0x1cc/0x234
[    2.834927] lr : blk_set_default_limits+0x18/0x24
[    2.835654] sp : ffff800084c4f730
[    2.836161] x29: ffff800084c4f730 x28: 0000000000000000 x27: 0000000000000000
[    2.837448] x26: 0000000000000000 x25: ffff000084130008 x24: 00000000ffffffff
[    2.838607] x23: ffff800082d84000 x22: ffff8000828b79c8 x21: ffff800084c4f8b8
[    2.839754] x20: 0000000000000008 x19: ffff000084170000 x18: 0000000000000000
[    2.840831] x17: 00000000900163fe x16: 000000008802ed71 x15: 00000000000003e8
[    2.842228] x14: 0000000000000002 x13: 00000000000372ef x12: 0000000000002bb0
[    2.843536] x11: 0000000000000000 x10: 0000000000004400 x9 : 0000000000000000
[    2.847832] x8 : ffff0000841703a0 x7 : 0000000000000000 x6 : 0000000000000003
[    2.855075] x5 : ffff000081279e00 x4 : 0000000000000000 x3 : 0000000000000200
[    2.862140] x2 : 000000000000ffff x1 : 000000000000fe00 x0 : ffff800084c4f8b8
[    2.869633] Call trace:
[    2.872055]  blk_validate_limits+0x1cc/0x234
[    2.876278]  blk_alloc_queue+0x7c/0x260
[    2.880038]  blk_mq_alloc_queue+0x54/0xbc
[    2.884504]  __blk_mq_alloc_disk+0x20/0x190
[    2.888266]  mmc_alloc_disk+0xbc/0x250
[    2.892062]  mmc_init_queue+0xe8/0x114
[    2.896091]  mmc_blk_alloc_req+0xb4/0x374
[    2.900237]  mmc_blk_probe+0x1d4/0x650
[    2.904194]  mmc_bus_probe+0x20/0x2c
[    2.907434]  really_probe+0xbc/0x2a4
[    2.911391]  __driver_probe_device+0x78/0x12c
[    2.915627]  driver_probe_device+0x40/0x160
[    2.919587]  __device_attach_driver+0xb8/0x134
[    2.923881]  bus_for_each_drv+0x80/0xdc
[    2.927664]  __device_attach+0xa8/0x1b0
[    2.931578]  device_initial_probe+0x14/0x20
[    2.935845]  bus_probe_device+0xa8/0xac
[    2.939606]  device_add+0x590/0x754
[    2.942864]  mmc_add_card+0x238/0x2dc
[    2.946691]  mmc_attach_mmc+0x12c/0x174
[    2.950494]  mmc_rescan+0x27c/0x318
[    2.954172]  process_one_work+0x154/0x298
[    2.957919]  worker_thread+0x304/0x408
[    2.961931]  kthread+0x118/0x11c
[    2.965187]  ret_from_fork+0x10/0x20
[    2.968835] ---[ end trace 0000000000000000 ]---
[    2.974440] mmcblk mmc0:0001: probe with driver mmcblk failed with error -22


Bisect points to this commit. This is very similar to the issue Geert
reported [0] but we are still seeing this on the latest mainline with
v6.10-rc5. When building with 4kB MMU pages we don't see this issue
when testing on the same hardware. Let me know if you have any
thoughts?

Thanks
Jon

[0] https://lore.kernel.org/linux-mmc/CAMuHMdWV4nWQHUpBKM2gHWeH9j9c0Di4bhg+F4-SAPEAmZuNSA@mail.gmail.com/
-- 
nvpublic

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/