[PATCH] ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K

[PATCH] ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K

[Michael Ellerman]

The pata_macio driver advertises a max_segment_size of 0xff00, because
the hardware doesn't cope with requests >= 64K.

However the SCSI core requires max_segment_size to be at least
PAGE_SIZE, which is a problem for pata_macio when the kernel is built
with 64K pages.

In older kernels the SCSI core would just increase the segment size to
be equal to PAGE_SIZE, however since the commit tagged below it causes a
warning and the device fails to probe:

  WARNING: CPU: 0 PID: 26 at block/blk-settings.c:202 .blk_validate_limits+0x2f8/0x35c
  CPU: 0 PID: 26 Comm: kworker/u4:1 Not tainted 6.10.0-rc1 #1
  Hardware name: PowerMac7,2 PPC970 0x390202 PowerMac
  ...
  NIP .blk_validate_limits+0x2f8/0x35c
  LR  .blk_alloc_queue+0xc0/0x2f8
  Call Trace:
    .blk_alloc_queue+0xc0/0x2f8
    .blk_mq_alloc_queue+0x60/0xf8
    .scsi_alloc_sdev+0x208/0x3c0
    .scsi_probe_and_add_lun+0x314/0x52c
    .__scsi_add_device+0x170/0x1a4
    .ata_scsi_scan_host+0x2bc/0x3e4
    .async_port_probe+0x6c/0xa0
    .async_run_entry_fn+0x60/0x1bc
    .process_one_work+0x228/0x510
    .worker_thread+0x360/0x530
    .kthread+0x134/0x13c
    .start_kernel_thread+0x10/0x14
  ...
  scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured

Although the hardware can't cope with a 64K segment, the driver
already deals with that internally by splitting large requests in
pata_macio_qc_prep(). That is how the driver has managed to function
until now on 64K kernels.

So fix the driver to advertise a max_segment_size of 64K, which avoids
the warning and keeps the SCSI core happy.

Fixes: afd53a3d8528 ("scsi: core: Initialize scsi midlayer limits before allocating the queue")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Closes: https://lore.kernel.org/all/ce2bf6af-4382-4fe1-b392-cc6829f5ceb2@roeck-us.net/
Reported-by: Doru Iorgulescu <doru.iorgulescu1@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218858
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/pata_macio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
index 817838e2f70e..3cb455a32d92 100644
--- a/drivers/ata/pata_macio.c
+++ b/drivers/ata/pata_macio.c
@@ -915,10 +915,13 @@ static const struct scsi_host_template pata_macio_sht = {
 	.sg_tablesize		= MAX_DCMDS,
 	/* We may not need that strict one */
 	.dma_boundary		= ATA_DMA_BOUNDARY,
-	/* Not sure what the real max is but we know it's less than 64K, let's
-	 * use 64K minus 256
+	/*
+	 * The SCSI core requires the segment size to cover at least a page, so
+	 * for 64K page size kernels this must be at least 64K. However the
+	 * hardware can't handle 64K, so pata_macio_qc_prep() will split large
+	 * requests.
 	 */
-	.max_segment_size	= MAX_DBDMA_SEG,
+	.max_segment_size	= SZ_64K,
 	.device_configure	= pata_macio_device_configure,
 	.sdev_groups		= ata_common_sdev_groups,
 	.can_queue		= ATA_DEF_QUEUE,
-- 
2.45.1

Re: [PATCH] ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K

[Damien Le Moal]

On 6/6/24 20:14, Michael Ellerman wrote:
> The pata_macio driver advertises a max_segment_size of 0xff00, because
> the hardware doesn't cope with requests >= 64K.
> 
> However the SCSI core requires max_segment_size to be at least
> PAGE_SIZE, which is a problem for pata_macio when the kernel is built
> with 64K pages.
> 
> In older kernels the SCSI core would just increase the segment size to
> be equal to PAGE_SIZE, however since the commit tagged below it causes a
> warning and the device fails to probe:
> 
>   WARNING: CPU: 0 PID: 26 at block/blk-settings.c:202 .blk_validate_limits+0x2f8/0x35c
>   CPU: 0 PID: 26 Comm: kworker/u4:1 Not tainted 6.10.0-rc1 #1
>   Hardware name: PowerMac7,2 PPC970 0x390202 PowerMac
>   ...
>   NIP .blk_validate_limits+0x2f8/0x35c
>   LR  .blk_alloc_queue+0xc0/0x2f8
>   Call Trace:
>     .blk_alloc_queue+0xc0/0x2f8
>     .blk_mq_alloc_queue+0x60/0xf8
>     .scsi_alloc_sdev+0x208/0x3c0
>     .scsi_probe_and_add_lun+0x314/0x52c
>     .__scsi_add_device+0x170/0x1a4
>     .ata_scsi_scan_host+0x2bc/0x3e4
>     .async_port_probe+0x6c/0xa0
>     .async_run_entry_fn+0x60/0x1bc
>     .process_one_work+0x228/0x510
>     .worker_thread+0x360/0x530
>     .kthread+0x134/0x13c
>     .start_kernel_thread+0x10/0x14
>   ...
>   scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured
> 
> Although the hardware can't cope with a 64K segment, the driver
> already deals with that internally by splitting large requests in
> pata_macio_qc_prep(). That is how the driver has managed to function
> until now on 64K kernels.
> 
> So fix the driver to advertise a max_segment_size of 64K, which avoids
> the warning and keeps the SCSI core happy.
> 
> Fixes: afd53a3d8528 ("scsi: core: Initialize scsi midlayer limits before allocating the queue")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/all/ce2bf6af-4382-4fe1-b392-cc6829f5ceb2@roeck-us.net/
> Reported-by: Doru Iorgulescu <doru.iorgulescu1@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218858
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  drivers/ata/pata_macio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
> index 817838e2f70e..3cb455a32d92 100644
> --- a/drivers/ata/pata_macio.c
> +++ b/drivers/ata/pata_macio.c
> @@ -915,10 +915,13 @@ static const struct scsi_host_template pata_macio_sht = {
>  	.sg_tablesize		= MAX_DCMDS,
>  	/* We may not need that strict one */
>  	.dma_boundary		= ATA_DMA_BOUNDARY,
> -	/* Not sure what the real max is but we know it's less than 64K, let's
> -	 * use 64K minus 256
> +	/*
> +	 * The SCSI core requires the segment size to cover at least a page, so
> +	 * for 64K page size kernels this must be at least 64K. However the
> +	 * hardware can't handle 64K, so pata_macio_qc_prep() will split large
> +	 * requests.
>  	 */
> -	.max_segment_size	= MAX_DBDMA_SEG,
> +	.max_segment_size	= SZ_64K,
>  	.device_configure	= pata_macio_device_configure,
>  	.sdev_groups		= ata_common_sdev_groups,
>  	.can_queue		= ATA_DEF_QUEUE,

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K

[Niklas Cassel]

On Thu, Jun 06, 2024 at 09:14:45PM +1000, Michael Ellerman wrote:
> The pata_macio driver advertises a max_segment_size of 0xff00, because
> the hardware doesn't cope with requests >= 64K.
> 
> However the SCSI core requires max_segment_size to be at least
> PAGE_SIZE, which is a problem for pata_macio when the kernel is built
> with 64K pages.
> 
> In older kernels the SCSI core would just increase the segment size to
> be equal to PAGE_SIZE, however since the commit tagged below it causes a
> warning and the device fails to probe:
> 
>   WARNING: CPU: 0 PID: 26 at block/blk-settings.c:202 .blk_validate_limits+0x2f8/0x35c
>   CPU: 0 PID: 26 Comm: kworker/u4:1 Not tainted 6.10.0-rc1 #1
>   Hardware name: PowerMac7,2 PPC970 0x390202 PowerMac
>   ...
>   NIP .blk_validate_limits+0x2f8/0x35c
>   LR  .blk_alloc_queue+0xc0/0x2f8
>   Call Trace:
>     .blk_alloc_queue+0xc0/0x2f8
>     .blk_mq_alloc_queue+0x60/0xf8
>     .scsi_alloc_sdev+0x208/0x3c0
>     .scsi_probe_and_add_lun+0x314/0x52c
>     .__scsi_add_device+0x170/0x1a4
>     .ata_scsi_scan_host+0x2bc/0x3e4
>     .async_port_probe+0x6c/0xa0
>     .async_run_entry_fn+0x60/0x1bc
>     .process_one_work+0x228/0x510
>     .worker_thread+0x360/0x530
>     .kthread+0x134/0x13c
>     .start_kernel_thread+0x10/0x14
>   ...
>   scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured
> 
> Although the hardware can't cope with a 64K segment, the driver
> already deals with that internally by splitting large requests in
> pata_macio_qc_prep(). That is how the driver has managed to function
> until now on 64K kernels.
> 
> So fix the driver to advertise a max_segment_size of 64K, which avoids
> the warning and keeps the SCSI core happy.
> 
> Fixes: afd53a3d8528 ("scsi: core: Initialize scsi midlayer limits before allocating the queue")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/all/ce2bf6af-4382-4fe1-b392-cc6829f5ceb2@roeck-us.net/
> Reported-by: Doru Iorgulescu <doru.iorgulescu1@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218858
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

Applied to libata/for-6.10-fixes:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.10-fixes

With John's Reviewed-by from the other thread:
https://lore.kernel.org/linux-ide/171362345502.571343.9746199181827642774.b4-ty@oracle.com/T/#t


Kind regards,
Niklas

Re: [PATCH] ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K

[Michael Ellerman]

Niklas Cassel <cassel@kernel.org> writes:
> On Thu, Jun 06, 2024 at 09:14:45PM +1000, Michael Ellerman wrote:
>> The pata_macio driver advertises a max_segment_size of 0xff00, because
>> the hardware doesn't cope with requests >= 64K.
>> 
>> However the SCSI core requires max_segment_size to be at least
>> PAGE_SIZE, which is a problem for pata_macio when the kernel is built
>> with 64K pages.
>> 
>> In older kernels the SCSI core would just increase the segment size to
>> be equal to PAGE_SIZE, however since the commit tagged below it causes a
>> warning and the device fails to probe:
>> 
>>   WARNING: CPU: 0 PID: 26 at block/blk-settings.c:202 .blk_validate_limits+0x2f8/0x35c
>>   CPU: 0 PID: 26 Comm: kworker/u4:1 Not tainted 6.10.0-rc1 #1
>>   Hardware name: PowerMac7,2 PPC970 0x390202 PowerMac
>>   ...
>>   NIP .blk_validate_limits+0x2f8/0x35c
>>   LR  .blk_alloc_queue+0xc0/0x2f8
>>   Call Trace:
>>     .blk_alloc_queue+0xc0/0x2f8
>>     .blk_mq_alloc_queue+0x60/0xf8
>>     .scsi_alloc_sdev+0x208/0x3c0
>>     .scsi_probe_and_add_lun+0x314/0x52c
>>     .__scsi_add_device+0x170/0x1a4
>>     .ata_scsi_scan_host+0x2bc/0x3e4
>>     .async_port_probe+0x6c/0xa0
>>     .async_run_entry_fn+0x60/0x1bc
>>     .process_one_work+0x228/0x510
>>     .worker_thread+0x360/0x530
>>     .kthread+0x134/0x13c
>>     .start_kernel_thread+0x10/0x14
>>   ...
>>   scsi_alloc_sdev: Allocation failure during SCSI scanning, some SCSI devices might not be configured
>> 
>> Although the hardware can't cope with a 64K segment, the driver
>> already deals with that internally by splitting large requests in
>> pata_macio_qc_prep(). That is how the driver has managed to function
>> until now on 64K kernels.
>> 
>> So fix the driver to advertise a max_segment_size of 64K, which avoids
>> the warning and keeps the SCSI core happy.
>> 
>> Fixes: afd53a3d8528 ("scsi: core: Initialize scsi midlayer limits before allocating the queue")
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Closes: https://lore.kernel.org/all/ce2bf6af-4382-4fe1-b392-cc6829f5ceb2@roeck-us.net/
>> Reported-by: Doru Iorgulescu <doru.iorgulescu1@gmail.com>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218858
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>
> Applied to libata/for-6.10-fixes:
> https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.10-fixes
>
> With John's Reviewed-by from the other thread:
> https://lore.kernel.org/linux-ide/171362345502.571343.9746199181827642774.b4-ty@oracle.com/T/#t

Thanks.

cheers