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

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

[Linux regression tracking (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

On 20.05.24 17:15, Christoph Hellwig wrote:
> Adding ben and the linuxppc list.

Hmm, no reply and no other progress to get this resolved afaics. So lets
bring Michael into the mix, he might be able to help out.

BTW TWIMC: a PowerMac G5 user user reported similar symptoms here
recently: https://bugzilla.kernel.org/show_bug.cgi?id=218858

Ciao, Thorsten

> Context: pata_macio initialization now fails as we enforce that the
> segment size is set properly.
> 
> On Wed, May 15, 2024 at 04:52:29PM -0700, Guenter Roeck wrote:
>> pata_macio_common_init() Calling ata_host_activate() with limit 65280
>> ...
>> 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.
> 
> Yeah.  Did you actually manage to use pata macio previously?  Or is
> it just used because it's part of the pmac default config?
> 
>> Looking at the old code, I think it did what you suggested above,
> 
>> but assuming that the driver requested a lower limit on purpose that
>> may not be the best solution.
> 
>> 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).
> 
> Yes, the idea behind this change was to catch such errors.  So far
> most errors have been drivers setting lower limits than what the
> hardware can actually handle, but I'd love to track this down.
> 
> If the hardware can't actually handle the lower limit we should
> probably just fail the probe gracefully with a well comment if
> statement instead.

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

[Thorsten Leemhuis]

On 29.05.24 16:36, Linux regression tracking (Thorsten Leemhuis) wrote:
> [CCing the regression list, as it should be in the loop for regressions:
> https://docs.kernel.org/admin-guide/reporting-regressions.html]
> 
> On 20.05.24 17:15, Christoph Hellwig wrote:
>> Adding ben and the linuxppc list.
> 
> Hmm, no reply and no other progress to get this resolved afaics. So lets
> bring Michael into the mix, he might be able to help out.
> 
> BTW TWIMC: a PowerMac G5 user user reported similar symptoms here
> recently: https://bugzilla.kernel.org/show_bug.cgi?id=218858

And yet another report with similar symptoms, this time with a
"PowerMac7,2 PPC970 0x390202 PowerMac":
https://bugzilla.kernel.org/show_bug.cgi?id=218905

Ciao, Thorsten

>> Context: pata_macio initialization now fails as we enforce that the
>> segment size is set properly.
>>
>> On Wed, May 15, 2024 at 04:52:29PM -0700, Guenter Roeck wrote:
>>> pata_macio_common_init() Calling ata_host_activate() with limit 65280
>>> ...
>>> 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.
>>
>> Yeah.  Did you actually manage to use pata macio previously?  Or is
>> it just used because it's part of the pmac default config?
>>
>>> Looking at the old code, I think it did what you suggested above,
>>
>>> but assuming that the driver requested a lower limit on purpose that
>>> may not be the best solution.
>>
>>> 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).
>>
>> Yes, the idea behind this change was to catch such errors.  So far
>> most errors have been drivers setting lower limits than what the
>> hardware can actually handle, but I'd love to track this down.
>>
>> If the hardware can't actually handle the lower limit we should
>> probably just fail the probe gracefully with a well comment if
>> statement instead.

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

[Michael Ellerman]

"Linux regression tracking (Thorsten Leemhuis)" <regressions@leemhuis.info> writes:
> [CCing the regression list, as it should be in the loop for regressions:
> https://docs.kernel.org/admin-guide/reporting-regressions.html]
>
> On 20.05.24 17:15, Christoph Hellwig wrote:
>> Adding ben and the linuxppc list.
>
> Hmm, no reply and no other progress to get this resolved afaics. So lets
> bring Michael into the mix, he might be able to help out.

Sorry I didn't see the original forward for some reason.

I haven't seen this on my G5, but it's hard drive is on SATA. I think
the CDROM is pata_macio, but there isn't a disk in the drive to test
with.

> BTW TWIMC: a PowerMac G5 user user reported similar symptoms here
> recently: https://bugzilla.kernel.org/show_bug.cgi?id=218858

AFAICS that report is from a 4K page size kernel (Page orders: ...
virtual = 12), so there must be something else going on?

I've asked the reporter to confirm the page size.

cheers

>> Context: pata_macio initialization now fails as we enforce that the
>> segment size is set properly.
>> 
>> On Wed, May 15, 2024 at 04:52:29PM -0700, Guenter Roeck wrote:
>>> pata_macio_common_init() Calling ata_host_activate() with limit 65280
>>> ...
>>> 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.
>> 
>> Yeah.  Did you actually manage to use pata macio previously?  Or is
>> it just used because it's part of the pmac default config?
>> 
>>> Looking at the old code, I think it did what you suggested above,
>> 
>>> but assuming that the driver requested a lower limit on purpose that
>>> may not be the best solution.
>> 
>>> 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).
>> 
>> Yes, the idea behind this change was to catch such errors.  So far
>> most errors have been drivers setting lower limits than what the
>> hardware can actually handle, but I'd love to track this down.
>> 
>> If the hardware can't actually handle the lower limit we should
>> probably just fail the probe gracefully with a well comment if
>> statement instead.

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:
> "Linux regression tracking (Thorsten Leemhuis)" <regressions@leemhuis.info> writes:
>> [CCing the regression list, as it should be in the loop for regressions:
>> https://docs.kernel.org/admin-guide/reporting-regressions.html]
>>
>> On 20.05.24 17:15, Christoph Hellwig wrote:
>>> Adding ben and the linuxppc list.
>>
>> Hmm, no reply and no other progress to get this resolved afaics. So lets
>> bring Michael into the mix, he might be able to help out.
>
> Sorry I didn't see the original forward for some reason.
>
> I haven't seen this on my G5, but it's hard drive is on SATA. I think
> the CDROM is pata_macio, but there isn't a disk in the drive to test
> with.
>
>> BTW TWIMC: a PowerMac G5 user user reported similar symptoms here
>> recently: https://bugzilla.kernel.org/show_bug.cgi?id=218858
>
> AFAICS that report is from a 4K page size kernel (Page orders: ...
> virtual = 12), so there must be something else going on?

No that's wrong. The actual hardware page size is 4K, but
CONFIG_PAGE_SIZE and PAGE_SHIFT etc. is 64K.

So at least for this user the driver used to work with 64K pages, and
now doesn't.

cheers

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

[Christoph Hellwig]

On Fri, May 31, 2024 at 12:28:21AM +1000, Michael Ellerman wrote:
> No that's wrong. The actual hardware page size is 4K, but
> CONFIG_PAGE_SIZE and PAGE_SHIFT etc. is 64K.
> 
> So at least for this user the driver used to work with 64K pages, and
> now doesn't.

Which suggested that the communicated max_hw_sectors is wrong, and
previously we were saved by the block layer increasing it to
PAGE_SIZE after a warning.  Should we just increment it to 64k?

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

[Michael Ellerman]

Christoph Hellwig <hch@lst.de> writes:
> On Fri, May 31, 2024 at 12:28:21AM +1000, Michael Ellerman wrote:
>> No that's wrong. The actual hardware page size is 4K, but
>> CONFIG_PAGE_SIZE and PAGE_SHIFT etc. is 64K.
>> 
>> So at least for this user the driver used to work with 64K pages, and
>> now doesn't.
>
> Which suggested that the communicated max_hw_sectors is wrong, and
> previously we were saved by the block layer increasing it to
> PAGE_SIZE after a warning.  Should we just increment it to 64k?

It looks like that user actually only has the CDROM hanging off
pata_macio, so it's possible it has been broken previously and they
didn't notice. I'll see if they can confirm the CDROM has been working
up until now.

I can test the CDROM on my G5 next week.

cheers

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Christoph Hellwig <hch@lst.de> writes:
>> On Fri, May 31, 2024 at 12:28:21AM +1000, Michael Ellerman wrote:
>>> No that's wrong. The actual hardware page size is 4K, but
>>> CONFIG_PAGE_SIZE and PAGE_SHIFT etc. is 64K.
>>> 
>>> So at least for this user the driver used to work with 64K pages, and
>>> now doesn't.
>>
>> Which suggested that the communicated max_hw_sectors is wrong, and
>> previously we were saved by the block layer increasing it to
>> PAGE_SIZE after a warning.  Should we just increment it to 64k?
>
> It looks like that user actually only has the CDROM hanging off
> pata_macio, so it's possible it has been broken previously and they
> didn't notice. I'll see if they can confirm the CDROM has been working
> up until now.
>
> I can test the CDROM on my G5 next week.

I can confirm that the driver does work with 64K pages prior to the
recent changes. I'm able to boot and read CDs with no errors.

However AFAICS that's because the driver splits large requests in
pata_macio_qc_prep():

static enum ata_completion_errors pata_macio_qc_prep(struct ata_queued_cmd *qc)
{
       ...
       for_each_sg(qc->sg, sg, qc->n_elem, si) {
              u32 addr, sg_len, len;
              ...
              addr = (u32) sg_dma_address(sg);
              sg_len = sg_dma_len(sg);

              while (sg_len) {
                     ...
                     len = (sg_len < MAX_DBDMA_SEG) ? sg_len : MAX_DBDMA_SEG;
                     table->command = cpu_to_le16(write ? OUTPUT_MORE: INPUT_MORE);
                     table->req_count = cpu_to_le16(len);
                     ...
                     addr += len;
                     sg_len -= len;
                     ++table;
              }
  }

 
If I increase MAX_DBMA_SEG from 0xff00 to 64K I see IO errors at boot:

  [   24.989755] sr 4:0:0:0: [sr0] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=6s
  [   25.007310] sr 4:0:0:0: [sr0] tag#0 Sense Key : Medium Error [current]
  [   25.020502] sr 4:0:0:0: [sr0] tag#0 ASC=0x10 <<vendor>>ASCQ=0x90
  [   25.032655] sr 4:0:0:0: [sr0] tag#0 CDB: Read(10) 28 00 00 00 00 00 00 00 20 00
  [   25.047232] I/O error, dev sr0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0


On the other hand increasing max_segment_size to 64K while leaving MAX_DBDMA_SEG
at 0xff00 seems to work fine. And that's effectively what's been happening on
existing kernels until now.

The only question is whether that violates some assumption elsewhere in the
SCSI layer?

Anyway patch below that works for me on v6.10-rc2.

cheers


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,

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

[Christoph Hellwig]

On Wed, Jun 05, 2024 at 10:37:53PM +1000, Michael Ellerman wrote:
> On the other hand increasing max_segment_size to 64K while leaving MAX_DBDMA_SEG
> at 0xff00 seems to work fine. And that's effectively what's been happening on
> existing kernels until now.

Exactly.

> 
> The only question is whether that violates some assumption elsewhere in the
> SCSI layer?

It shouldn't.

> Anyway patch below that works for me on v6.10-rc2.

This looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[John Garry]

> 
> 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,


Feel free to add:
Reviewed-by: John Garry <john.g.garry@oracle.com>

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

[Michael Ellerman]

John Garry <john.g.garry@oracle.com> writes:
>> 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,
>
> Feel free to add:
> Reviewed-by: John Garry <john.g.garry@oracle.com>

Thanks.

Sorry I missed adding this when sending the proper patch, maybe whoever
applies it can add it then.

cheers