Re: [PATCH 04/23] scsi: initialize scsi midlayer limits before allocating the queue

Guenter Roeck
2024-05-16T14:50:18Z

On 5/16/24 06:08, John Garry wrote:
> On 15/05/2024 17:52, Guenter Roeck wrote:
>> max_segment_size is 65280; PAGE_SIZE is 65536; BLK_MAX_SEGMENT_SIZE is 65536
>> WARNING: CPU: 0 PID: 12 at block/blk-settings.c:202 blk_validate_limits+0x2d4/0x364
>> ...
>>
>> This is with PPC_BOOK3S_64 which selects a default page size of 64k.
>> Looking at the old code, I think it did what you suggested above,
>>
>> void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
>> {
>>          if (max_size < PAGE_SIZE) {
>>                  max_size = PAGE_SIZE;
>>                  printk(KERN_INFO "%s: set to minimum %d\n",
>>                         __func__, max_size);
>>          }
>> ...
>>
>> but assuming that the driver requested a lower limit on purpose that
>> may not be the best solution.
>
> Right, it is relied on that PAGE_SIZE can fit into a segment.
>
>>
>> Never mind, though - I updated my test configuration to explicitly
>> configure the page size to 4k to work around the problem. With that,
>> please consider this report a note in case someone hits the problem
>> on a real system (and sorry for the noise).
>
> Other controllers do have a 4K segment limit and are broken on systems with 16/64K PAGE_SIZE, like:
>
> https://lore.kernel.org/linux-block/20230612203314.17820-1-bvanassche@acm.org/
>

Understood, only it isn't just 4k segment limit with 16/64k page size, but more
generally any system with segment limit < PAGE_SIZE.

It is a bit sad that support for affected configurations is [now ?] broken
because above patch series was rejected (and, no, that has nothing to do
with me working for the same company as the submitter of that patch series -
that is me testing the upstream kernel with qemu).

Given that various controllers are affected by that problem, would it be
acceptable to submit patches such as the following to avoid runtime failures ?

diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
index 817838e2f70e..6adf9105b5fb 100644
--- a/drivers/ata/pata_macio.c
+++ b/drivers/ata/pata_macio.c
@@ -1380,6 +1380,8 @@ static int __init pata_macio_init(void)
{
int rc;

+ BUILD_BUG_ON(MAX_DBDMA_SEG < PAGE_SIZE);
+
if (!machine_is(powermac))
return -ENODEV;

or, alternatively,

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b595494ab9b4..d7bd64702109 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -789,6 +789,7 @@ config PATA_JMICRON
config PATA_MACIO
tristate "Apple PowerMac/PowerBook internal 'MacIO' IDE"
depends on PPC_PMAC
+ depends on PAGE_SIZE_LESS_THAN_64KB
help
Most IDE capable PowerMacs have IDE busses driven by a variant
of this controller which is part of the Apple chipset used on

Thanks,
Guenter