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

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

[Jon Hunter]

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

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

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 10:43:24AM +0100, Jon Hunter wrote:
> 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 ...

The old code upgraded the limits to the PAGE_SIZE for this case after
issunig a warning.  Your driver probably incorrectly advertised the
lower max_segment_size.  Try setting it to 64k.  I would have sent you
a patch for that, but I can't see what mmc host driver you are using.

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

[Jon Hunter]

On 27/06/2024 10:49, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 10:43:24AM +0100, Jon Hunter wrote:
>> 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 ...
> 
> The old code upgraded the limits to the PAGE_SIZE for this case after
> issunig a warning.  Your driver probably incorrectly advertised the
> lower max_segment_size.  Try setting it to 64k.  I would have sent you
> a patch for that, but I can't see what mmc host driver you are using.


We are using the sdhci-tegra.c driver. I don't see it set in there, but 
I see references to max_seg_size in the main sdhci.c driver.

Thanks,
Jon

-- 
nvpublic

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

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 10:58:58AM +0100, Jon Hunter wrote:
>
> On 27/06/2024 10:49, Christoph Hellwig wrote:
>> On Thu, Jun 27, 2024 at 10:43:24AM +0100, Jon Hunter wrote:
>>> 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 ...
>>
>> The old code upgraded the limits to the PAGE_SIZE for this case after
>> issunig a warning.  Your driver probably incorrectly advertised the
>> lower max_segment_size.  Try setting it to 64k.  I would have sent you
>> a patch for that, but I can't see what mmc host driver you are using.
>
>
> We are using the sdhci-tegra.c driver. I don't see it set in there, but I 
> see references to max_seg_size in the main sdhci.c driver.

sdhci-tegra.c sets SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC for various
devices, I guess you have one od those?  This sounds like the device
might not actually need it, but I'll need help from the SDHCI and
tegra maintainers here.

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

[Jon Hunter]

On 27/06/2024 12:19, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 10:58:58AM +0100, Jon Hunter wrote:
>>
>> On 27/06/2024 10:49, Christoph Hellwig wrote:
>>> On Thu, Jun 27, 2024 at 10:43:24AM +0100, Jon Hunter wrote:
>>>> 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 ...
>>>
>>> The old code upgraded the limits to the PAGE_SIZE for this case after
>>> issunig a warning.  Your driver probably incorrectly advertised the
>>> lower max_segment_size.  Try setting it to 64k.  I would have sent you
>>> a patch for that, but I can't see what mmc host driver you are using.
>>
>>
>> We are using the sdhci-tegra.c driver. I don't see it set in there, but I
>> see references to max_seg_size in the main sdhci.c driver.
> 
> sdhci-tegra.c sets SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC for various
> devices, I guess you have one od those?  This sounds like the device
> might not actually need it, but I'll need help from the SDHCI and
> tegra maintainers here.


I have been testing on both Tegra194 and Tegra234. Both of these set the 
above quirk. This would explain why the max_segment_size is rounded down 
to 65024 in the mmc_alloc_disk() function.

We can check if this is needed but if it is needed then it is not clear 
if/how this can be fixed?

Thanks
Jon

-- 
nvpublic

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

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 01:30:03PM +0100, Jon Hunter wrote:
> I have been testing on both Tegra194 and Tegra234. Both of these set the 
> above quirk. This would explain why the max_segment_size is rounded down to 
> 65024 in the mmc_alloc_disk() function.
>
> We can check if this is needed but if it is needed then it is not clear 
> if/how this can be fixed?

The older kernels did this:

	if (max_size < PAGE_CACHE_SIZE) {
		max_size = PAGE_CACHE_SIZE;
		printk(KERN_INFO "%s: set to minimum %d\n",
			__func__, max_size);
	}

	q->limits.max_segment_size = max_size;

so when these kernels actually worked despite the above warning it
must be ok(-ish) to just increase this value.  If that is best done
by dropping the quirk, or changing the logic in sdhci.c is something
the maintainers that understand the hardware need to decide.

The patch below gives you the pre-6.9 behavior just without the
boot time warning, but it might not be what was intended by the
quirk:

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 746f4cf7ab0338..0dc3604ac6093a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -4721,12 +4721,9 @@ int sdhci_setup_host(struct sdhci_host *host)
 	 * be larger than 64 KiB though.
 	 */
 	if (host->flags & SDHCI_USE_ADMA) {
-		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) {
+		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC)
 			host->max_adma = 65532; /* 32-bit alignment */
-			mmc->max_seg_size = 65535;
-		} else {
-			mmc->max_seg_size = 65536;
-		}
+		mmc->max_seg_size = 65536;
 	} else {
 		mmc->max_seg_size = mmc->max_req_size;
 	}

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

[Jon Hunter]

On 27/06/2024 13:44, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 01:30:03PM +0100, Jon Hunter wrote:
>> I have been testing on both Tegra194 and Tegra234. Both of these set the
>> above quirk. This would explain why the max_segment_size is rounded down to
>> 65024 in the mmc_alloc_disk() function.
>>
>> We can check if this is needed but if it is needed then it is not clear
>> if/how this can be fixed?
> 
> The older kernels did this:
> 
> 	if (max_size < PAGE_CACHE_SIZE) {
> 		max_size = PAGE_CACHE_SIZE;
> 		printk(KERN_INFO "%s: set to minimum %d\n",
> 			__func__, max_size);
> 	}
> 
> 	q->limits.max_segment_size = max_size;
> 
> so when these kernels actually worked despite the above warning it
> must be ok(-ish) to just increase this value.  If that is best done
> by dropping the quirk, or changing the logic in sdhci.c is something
> the maintainers that understand the hardware need to decide.
> 
> The patch below gives you the pre-6.9 behavior just without the
> boot time warning, but it might not be what was intended by the
> quirk:
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 746f4cf7ab0338..0dc3604ac6093a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -4721,12 +4721,9 @@ int sdhci_setup_host(struct sdhci_host *host)
>   	 * be larger than 64 KiB though.
>   	 */
>   	if (host->flags & SDHCI_USE_ADMA) {
> -		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) {
> +		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC)
>   			host->max_adma = 65532; /* 32-bit alignment */
> -			mmc->max_seg_size = 65535;
> -		} else {
> -			mmc->max_seg_size = 65536;
> -		}
> +		mmc->max_seg_size = 65536;
>   	} else {
>   		mmc->max_seg_size = mmc->max_req_size;
>   	}


As a quick test I removed the quirk for Tegra194 and that did work, 
which achieves the same thing as the above.

I guess there are two open questions here ...

1. Do we need the quirk for all the current Tegra devices? Thierry and I
    can confirm this.
2. For devices that need that quirk and use 64kB pages, what is the
    appropriate way to handle this for sdhci controllers?

Jon

-- 
nvpublic

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

[Adrian Hunter]

On 27/06/24 16:09, Jon Hunter wrote:
> 
> On 27/06/2024 13:44, Christoph Hellwig wrote:
>> On Thu, Jun 27, 2024 at 01:30:03PM +0100, Jon Hunter wrote:
>>> I have been testing on both Tegra194 and Tegra234. Both of these set the
>>> above quirk. This would explain why the max_segment_size is rounded down to
>>> 65024 in the mmc_alloc_disk() function.
>>>
>>> We can check if this is needed but if it is needed then it is not clear
>>> if/how this can be fixed?
>>
>> The older kernels did this:
>>
>>     if (max_size < PAGE_CACHE_SIZE) {
>>         max_size = PAGE_CACHE_SIZE;
>>         printk(KERN_INFO "%s: set to minimum %d\n",
>>             __func__, max_size);
>>     }
>>
>>     q->limits.max_segment_size = max_size;
>>
>> so when these kernels actually worked despite the above warning it
>> must be ok(-ish) to just increase this value.  If that is best done
>> by dropping the quirk, or changing the logic in sdhci.c is something
>> the maintainers that understand the hardware need to decide.
>>
>> The patch below gives you the pre-6.9 behavior just without the
>> boot time warning, but it might not be what was intended by the
>> quirk:
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 746f4cf7ab0338..0dc3604ac6093a 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -4721,12 +4721,9 @@ int sdhci_setup_host(struct sdhci_host *host)
>>        * be larger than 64 KiB though.
>>        */
>>       if (host->flags & SDHCI_USE_ADMA) {
>> -        if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) {
>> +        if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC)
>>               host->max_adma = 65532; /* 32-bit alignment */
>> -            mmc->max_seg_size = 65535;
>> -        } else {
>> -            mmc->max_seg_size = 65536;
>> -        }
>> +        mmc->max_seg_size = 65536;
>>       } else {
>>           mmc->max_seg_size = mmc->max_req_size;
>>       }
> 
> 
> As a quick test I removed the quirk for Tegra194 and that did work, which achieves the same thing as the above.
> 
> I guess there are two open questions here ...
> 
> 1. Do we need the quirk for all the current Tegra devices? Thierry and I
>    can confirm this.
> 2. For devices that need that quirk and use 64kB pages, what is the
>    appropriate way to handle this for sdhci controllers?

Probably just do what the block layer was doing e.g.

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 746f4cf7ab03..6a1dea0d8d64 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -4724,6 +4724,13 @@ int sdhci_setup_host(struct sdhci_host *host)
 		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) {
 			host->max_adma = 65532; /* 32-bit alignment */
 			mmc->max_seg_size = 65535;
+			/*
+			 * Block layer cannot accept max_seg_size < PAGE_SIZE
+			 * whereas sdhci_adma_table_pre() will anyway split the
+			 * descriptor to handle size > max_adma.
+			 */
+			if (mmc->max_seg_size < PAGE_SIZE)
+				mmc->max_seg_size = PAGE_SIZE;
 		} else {
 			mmc->max_seg_size = 65536;
 		}

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

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 04:44:30PM +0300, Adrian Hunter wrote:
> Probably just do what the block layer was doing e.g.

That was really just a hack due to the lack of error handling in
the old blk_queue_* interfaces.  If ADMA really doesn't actually have
a limitation, please just don't set any max_segment_size at all.

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

[Adrian Hunter]

On 27/06/24 17:47, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 04:44:30PM +0300, Adrian Hunter wrote:
>> Probably just do what the block layer was doing e.g.
> 
> That was really just a hack due to the lack of error handling in
> the old blk_queue_* interfaces.  If ADMA really doesn't actually have
> a limitation, please just don't set any max_segment_size at all.
> 

There is a limitation of 64KiB max. (or less with 
SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) per descriptor.

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

[Jon Hunter]

On 27/06/2024 14:44, Adrian Hunter wrote:
> On 27/06/24 16:09, Jon Hunter wrote:
>>
>> On 27/06/2024 13:44, Christoph Hellwig wrote:
>>> On Thu, Jun 27, 2024 at 01:30:03PM +0100, Jon Hunter wrote:
>>>> I have been testing on both Tegra194 and Tegra234. Both of these set the
>>>> above quirk. This would explain why the max_segment_size is rounded down to
>>>> 65024 in the mmc_alloc_disk() function.
>>>>
>>>> We can check if this is needed but if it is needed then it is not clear
>>>> if/how this can be fixed?
>>>
>>> The older kernels did this:
>>>
>>>      if (max_size < PAGE_CACHE_SIZE) {
>>>          max_size = PAGE_CACHE_SIZE;
>>>          printk(KERN_INFO "%s: set to minimum %d\n",
>>>              __func__, max_size);
>>>      }
>>>
>>>      q->limits.max_segment_size = max_size;
>>>
>>> so when these kernels actually worked despite the above warning it
>>> must be ok(-ish) to just increase this value.  If that is best done
>>> by dropping the quirk, or changing the logic in sdhci.c is something
>>> the maintainers that understand the hardware need to decide.
>>>
>>> The patch below gives you the pre-6.9 behavior just without the
>>> boot time warning, but it might not be what was intended by the
>>> quirk:
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 746f4cf7ab0338..0dc3604ac6093a 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -4721,12 +4721,9 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>         * be larger than 64 KiB though.
>>>         */
>>>        if (host->flags & SDHCI_USE_ADMA) {
>>> -        if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) {
>>> +        if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC)
>>>                host->max_adma = 65532; /* 32-bit alignment */
>>> -            mmc->max_seg_size = 65535;
>>> -        } else {
>>> -            mmc->max_seg_size = 65536;
>>> -        }
>>> +        mmc->max_seg_size = 65536;
>>>        } else {
>>>            mmc->max_seg_size = mmc->max_req_size;
>>>        }
>>
>>
>> As a quick test I removed the quirk for Tegra194 and that did work, which achieves the same thing as the above.
>>
>> I guess there are two open questions here ...
>>
>> 1. Do we need the quirk for all the current Tegra devices? Thierry and I
>>     can confirm this.
>> 2. For devices that need that quirk and use 64kB pages, what is the
>>     appropriate way to handle this for sdhci controllers?
> 
> Probably just do what the block layer was doing e.g.
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 746f4cf7ab03..6a1dea0d8d64 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -4724,6 +4724,13 @@ int sdhci_setup_host(struct sdhci_host *host)
>   		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) {
>   			host->max_adma = 65532; /* 32-bit alignment */
>   			mmc->max_seg_size = 65535;
> +			/*
> +			 * Block layer cannot accept max_seg_size < PAGE_SIZE
> +			 * whereas sdhci_adma_table_pre() will anyway split the
> +			 * descriptor to handle size > max_adma.
> +			 */
> +			if (mmc->max_seg_size < PAGE_SIZE)
> +				mmc->max_seg_size = PAGE_SIZE;
>   		} else {
>   			mmc->max_seg_size = 65536;
>   		}
> 

I know that this is expected to work, but just wanted to confirm that 
this does work.

Jon

-- 
nvpublic

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

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 11:06:54AM +0300, Adrian Hunter wrote:
> On 27/06/24 17:47, Christoph Hellwig wrote:
> > On Thu, Jun 27, 2024 at 04:44:30PM +0300, Adrian Hunter wrote:
> >> Probably just do what the block layer was doing e.g.
> > 
> > That was really just a hack due to the lack of error handling in
> > the old blk_queue_* interfaces.  If ADMA really doesn't actually have
> > a limitation, please just don't set any max_segment_size at all.
> > 
> 
> There is a limitation of 64KiB max. (or less with 
> SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) per descriptor.

Hmm, I thought the conflusion was that it can be more, which is why
you proposed to increase it if the PAGE_SIZE is >= 64k.  And based on
Jon's report at least for his tegra setups it works with 64k.

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

[Adrian Hunter]

On 28/06/24 15:32, Christoph Hellwig wrote:
> On Fri, Jun 28, 2024 at 11:06:54AM +0300, Adrian Hunter wrote:
>> On 27/06/24 17:47, Christoph Hellwig wrote:
>>> On Thu, Jun 27, 2024 at 04:44:30PM +0300, Adrian Hunter wrote:
>>>> Probably just do what the block layer was doing e.g.
>>>
>>> That was really just a hack due to the lack of error handling in
>>> the old blk_queue_* interfaces.  If ADMA really doesn't actually have
>>> a limitation, please just don't set any max_segment_size at all.
>>>
>>
>> There is a limitation of 64KiB max. (or less with 
>> SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC) per descriptor.
> 
> Hmm, I thought the conflusion was that it can be more, which is why
> you proposed to increase it if the PAGE_SIZE is >= 64k.  And based on
> Jon's report at least for his tegra setups it works with 64k.

There is a workaround in that case to split to 32KiB chunks

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

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 03:37:35PM +0300, Adrian Hunter wrote:
> > Hmm, I thought the conflusion was that it can be more, which is why
> > you proposed to increase it if the PAGE_SIZE is >= 64k.  And based on
> > Jon's report at least for his tegra setups it works with 64k.
> 
> There is a workaround in that case to split to 32KiB chunks

Which I guess is less optimal than just using the block layer
splitting?  Maybe add a big fat comment explaining this?

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

[Adrian Hunter]

On 28/06/24 15:51, Christoph Hellwig wrote:
> On Fri, Jun 28, 2024 at 03:37:35PM +0300, Adrian Hunter wrote:
>>> Hmm, I thought the conflusion was that it can be more, which is why
>>> you proposed to increase it if the PAGE_SIZE is >= 64k.  And based on
>>> Jon's report at least for his tegra setups it works with 64k.
>>
>> There is a workaround in that case to split to 32KiB chunks
> 
> Which I guess is less optimal than just using the block layer
> splitting?  Maybe add a big fat comment explaining this?

Improving it looks straight forward, but then there is testing and
checking the code for anything that might be assuming an sg element
is not more than 64KiB.

However it doesn't seem to offer any benefit, so it is difficult to
justify doing.

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

[Jon Hunter]

Hi Adrian,

On 28/06/2024 14:45, Adrian Hunter wrote:
> On 28/06/24 15:51, Christoph Hellwig wrote:
>> On Fri, Jun 28, 2024 at 03:37:35PM +0300, Adrian Hunter wrote:
>>>> Hmm, I thought the conflusion was that it can be more, which is why
>>>> you proposed to increase it if the PAGE_SIZE is >= 64k.  And based on
>>>> Jon's report at least for his tegra setups it works with 64k.
>>>
>>> There is a workaround in that case to split to 32KiB chunks
>>
>> Which I guess is less optimal than just using the block layer
>> splitting?  Maybe add a big fat comment explaining this?
> 
> Improving it looks straight forward, but then there is testing and
> checking the code for anything that might be assuming an sg element
> is not more than 64KiB.
> 
> However it doesn't seem to offer any benefit, so it is difficult to
> justify doing.


Did we come to any agreement on how to fix this?

Thanks
Jon

-- 
nvpublic

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

[Christoph Hellwig]

On Wed, Jul 03, 2024 at 09:22:14AM +0100, Jon Hunter wrote:
>> Improving it looks straight forward, but then there is testing and
>> checking the code for anything that might be assuming an sg element
>> is not more than 64KiB.
>>
>> However it doesn't seem to offer any benefit, so it is difficult to
>> justify doing.
>
>
> Did we come to any agreement on how to fix this?

FYI, all I'm asking is to add a clear comment explaining setting
a different value depending on the page size.  Otherwise I'm ok with
the patch from Adrian.