diff mbox series

mmc: sdhci: Fix max_seg_size for 64KiB PAGE_SIZE

Message ID 20240710180737.142504-1-adrian.hunter@intel.com (mailing list archive)
State New
Headers show
Series mmc: sdhci: Fix max_seg_size for 64KiB PAGE_SIZE | expand

Commit Message

Adrian Hunter July 10, 2024, 6:07 p.m. UTC
blk_queue_max_segment_size() ensured:

	if (max_size < PAGE_SIZE)
		max_size = PAGE_SIZE;

whereas:

blk_validate_limits() makes it an error:

	if (WARN_ON_ONCE(lim->max_segment_size < PAGE_SIZE))
		return -EINVAL;

The change from one to the other, exposed sdhci which was setting maximum
segment size too low in some circumstances.

Fix the maximum segment size when it is too low.

Fixes: 616f87661792 ("mmc: pass queue_limits to blk_mq_alloc_disk")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Christoph Hellwig July 11, 2024, 4:01 a.m. UTC | #1
On Wed, Jul 10, 2024 at 09:07:37PM +0300, Adrian Hunter wrote:
> blk_queue_max_segment_size() ensured:
> 
> 	if (max_size < PAGE_SIZE)
> 		max_size = PAGE_SIZE;

This is a bit misleading, as it also warned about it and papered over it
with the above as it had no way to return errors.  Any everyone seeing
these problems now ignored the warnings before, probably for years..

Except for that:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Adrian Hunter July 11, 2024, 5:17 a.m. UTC | #2
On 11/07/24 07:01, Christoph Hellwig wrote:
> On Wed, Jul 10, 2024 at 09:07:37PM +0300, Adrian Hunter wrote:
>> blk_queue_max_segment_size() ensured:
>>
>> 	if (max_size < PAGE_SIZE)
>> 		max_size = PAGE_SIZE;
> 
> This is a bit misleading, as it also warned about it and papered over it
> with the above as it had no way to return errors.  Any everyone seeing
> these problems now ignored the warnings before, probably for years..

Was there a warning, since the message in blk_queue_max_segment_size()
was:

		pr_info("%s: set to minimum %u\n", __func__, max_size);

> 
> Except for that:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
Christoph Hellwig July 11, 2024, 6:48 a.m. UTC | #3
On Thu, Jul 11, 2024 at 08:17:47AM +0300, Adrian Hunter wrote:
> On 11/07/24 07:01, Christoph Hellwig wrote:
> > On Wed, Jul 10, 2024 at 09:07:37PM +0300, Adrian Hunter wrote:
> >> blk_queue_max_segment_size() ensured:
> >>
> >> 	if (max_size < PAGE_SIZE)
> >> 		max_size = PAGE_SIZE;
> > 
> > This is a bit misleading, as it also warned about it and papered over it
> > with the above as it had no way to return errors.  Any everyone seeing
> > these problems now ignored the warnings before, probably for years..
> 
> Was there a warning, since the message in blk_queue_max_segment_size()
> was:
> 
> 		pr_info("%s: set to minimum %u\n", __func__, max_size);

It stated out as a plain printk and became an info later, but yes,
not the big warning sign it was supposed to be.
Jon Hunter July 11, 2024, 9:47 a.m. UTC | #4
On 10/07/2024 19:07, Adrian Hunter wrote:
> blk_queue_max_segment_size() ensured:
> 
> 	if (max_size < PAGE_SIZE)
> 		max_size = PAGE_SIZE;
> 
> whereas:
> 
> blk_validate_limits() makes it an error:
> 
> 	if (WARN_ON_ONCE(lim->max_segment_size < PAGE_SIZE))
> 		return -EINVAL;
> 
> The change from one to the other, exposed sdhci which was setting maximum
> segment size too low in some circumstances.
> 
> Fix the maximum segment size when it is too low.
> 
> Fixes: 616f87661792 ("mmc: pass queue_limits to blk_mq_alloc_disk")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>   drivers/mmc/host/sdhci.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a20df9383b20..4b91c9e96635 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -4708,6 +4708,21 @@ 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;
> +			/*
> +			 * sdhci_adma_table_pre() expects to define 1 DMA
> +			 * descriptor per segment, so the maximum segment size
> +			 * is set accordingly. SDHCI allows up to 64KiB per DMA
> +			 * descriptor (16-bit field), but some controllers do
> +			 * not support "zero means 65536" reducing the maximum
> +			 * for them to 65535. That is a problem if PAGE_SIZE is
> +			 * 64KiB because the block layer does not support
> +			 * max_seg_size < PAGE_SIZE, however
> +			 * sdhci_adma_table_pre() has a workaround to handle
> +			 * that case, and split the descriptor. Refer also
> +			 * comment in sdhci_adma_table_pre().
> +			 */
> +			if (mmc->max_seg_size < PAGE_SIZE)
> +				mmc->max_seg_size = PAGE_SIZE;
>   		} else {
>   			mmc->max_seg_size = 65536;
>   		}

Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon
Ulf Hansson July 11, 2024, 4:03 p.m. UTC | #5
On Wed, 10 Jul 2024 at 20:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> blk_queue_max_segment_size() ensured:
>
>         if (max_size < PAGE_SIZE)
>                 max_size = PAGE_SIZE;
>
> whereas:
>
> blk_validate_limits() makes it an error:
>
>         if (WARN_ON_ONCE(lim->max_segment_size < PAGE_SIZE))
>                 return -EINVAL;
>
> The change from one to the other, exposed sdhci which was setting maximum
> segment size too low in some circumstances.
>
> Fix the maximum segment size when it is too low.
>
> Fixes: 616f87661792 ("mmc: pass queue_limits to blk_mq_alloc_disk")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Applied for fixes, thanks!

If we want to log a message due to the wrong max_seg_size, let's do
that from a patch on top.

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a20df9383b20..4b91c9e96635 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -4708,6 +4708,21 @@ 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;
> +                       /*
> +                        * sdhci_adma_table_pre() expects to define 1 DMA
> +                        * descriptor per segment, so the maximum segment size
> +                        * is set accordingly. SDHCI allows up to 64KiB per DMA
> +                        * descriptor (16-bit field), but some controllers do
> +                        * not support "zero means 65536" reducing the maximum
> +                        * for them to 65535. That is a problem if PAGE_SIZE is
> +                        * 64KiB because the block layer does not support
> +                        * max_seg_size < PAGE_SIZE, however
> +                        * sdhci_adma_table_pre() has a workaround to handle
> +                        * that case, and split the descriptor. Refer also
> +                        * comment in sdhci_adma_table_pre().
> +                        */
> +                       if (mmc->max_seg_size < PAGE_SIZE)
> +                               mmc->max_seg_size = PAGE_SIZE;
>                 } else {
>                         mmc->max_seg_size = 65536;
>                 }
> --
> 2.34.1
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a20df9383b20..4b91c9e96635 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -4708,6 +4708,21 @@  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;
+			/*
+			 * sdhci_adma_table_pre() expects to define 1 DMA
+			 * descriptor per segment, so the maximum segment size
+			 * is set accordingly. SDHCI allows up to 64KiB per DMA
+			 * descriptor (16-bit field), but some controllers do
+			 * not support "zero means 65536" reducing the maximum
+			 * for them to 65535. That is a problem if PAGE_SIZE is
+			 * 64KiB because the block layer does not support
+			 * max_seg_size < PAGE_SIZE, however
+			 * sdhci_adma_table_pre() has a workaround to handle
+			 * that case, and split the descriptor. Refer also
+			 * comment in sdhci_adma_table_pre().
+			 */
+			if (mmc->max_seg_size < PAGE_SIZE)
+				mmc->max_seg_size = PAGE_SIZE;
 		} else {
 			mmc->max_seg_size = 65536;
 		}