sata_nv: fix ADMA ATAPI issues with memory over 4GB

drivers/ata/sata_nv.c +9 -1

@@ -747,11 +748,29 @@
 		   on the port. */
 		adma_enable = 0;
 		nv_adma_register_mode(ap);
+		if (!(pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)) {
+			/* Transitioning to legacy mode. Free the pad buffer. */
+			ata_pad_free(ap, ap->host->dev);
+			ap->pad = NULL;
+			ap->pad_dma = 0;
+		}
 	} else {
-		bounce_limit = *ap->dev->dma_mask;
+		bounce_limit = pp->adma_dma_mask;
 		segment_boundary = NV_ADMA_DMA_BOUNDARY;
 		sg_tablesize = NV_ADMA_SGTBL_TOTAL_LEN;
 		adma_enable = 1;
+
+		if (pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) {

How about always initialize DMA mask to ATA_DMA_MASK regardless of ADMA mode such that PRD and PAD buffers are always accessible by register mode and just raising PCI dma mask and queue bounce limit if ADMA mode is active?

drivers/ata/sata_nv.c line 787

Quoted 4 lines

drivers/ata/sata_nv.c +3 -0

+	/* Set appropriate DMA mask. */
+	pci_set_dma_mask(pdev, bounce_limit);
+	pci_set_consistent_dma_mask(pdev, bounce_limit);

These can fail.

Also, please separate out the result TF handling to a separate patch. I know it's a small change but as both introduces important behavior changes, I think it would be nice to have a bisection point inbetween.

Thanks.

Quoted 4 lines · Tejun Heo

How about always initialize DMA mask to ATA_DMA_MASK regardless of ADMA mode such that PRD and PAD buffers are always accessible by register mode and just raising PCI dma mask and queue bounce limit if ADMA mode is active?

Could be done.. but, I don't want to constrain the ADMA APRD/CPB area in that way (there are some dual-socket Opteron boxes with this controller, forcing an allocation below 4GB for this could force a non-optimal node allocation I think..) To do this I'd have to raise the mask for the APRD allocation, drop it again, then raise it again in ADMA mode, which is kind of ugly.

Also, I'd rather not allocate the legacy PRD at all if we're in ADMA mode. That way, if some bug causes us to try and do legacy DMA in ADMA mode, we'll crash from null pointer dereference instead of potentially transferring incorrect data (as we had in this case) and corrupting things.

Quoted 5 lines

drivers/ata/sata_nv.c line 787

Quoted 3 lines

drivers/ata/sata_nv.c +3 -0

+    /* Set appropriate DMA mask. */
+    pci_set_dma_mask(pdev, bounce_limit);
+    pci_set_consistent_dma_mask(pdev, bounce_limit);

These can fail.

Yes, it should likely do something with these return values. Though theoretically it shouldn't fail, since the DMA mask is either 32-bit, which shouldn't fail, or one that was successfully set before. Also I don't think the SCSI layer actually checks the slave_config return value.. sigh.

Quoted 3 lines

Also, please separate out the result TF handling to a separate patch. I know it's a small change but as both introduces important behavior changes, I think it would be nice to have a bisection point inbetween.

Could do. That change would have to come first though, as the change to not allocate the PRD except when necessary would cause some cases there to blow up when before they might have worked in some cases.

Quoted 1 line · Robert Hancock

Tejun Heo wrote:

Quoted 5 lines

Yes, it should likely do something with these return values. Though theoretically it shouldn't fail, since the DMA mask is either 32-bit, which shouldn't fail, or one that was successfully set before. Also I don't think the SCSI layer actually checks the slave_config return value.. sigh.

Then please at least add WARN_ON() && another reason why allocating / deallocating resources from ->slave_config isn't such a good idea.

       if (sdev->host->hostt->slave_configure) {
                int ret = sdev->host->hostt->slave_configure(sdev);
                if (ret) {
                        /*
                         * if LLDD reports slave not present, don't clutter
                         * console with alloc failure messages

                         */
                        if (ret != -ENXIO) {
                                sdev_printk(KERN_ERR, sdev,
                                        "failed to configure device\n");
                        }
                        return SCSI_SCAN_NO_RESPONSE;
                }
        }

Quoted 2 lines · Tejun Heo

Quoted 1 line · Robert Hancock

Tejun Heo wrote:

..

Quoted 8 lines

Quoted 5 lines

Yes, it should likely do something with these return values. Though theoretically it shouldn't fail, since the DMA mask is either 32-bit, which shouldn't fail, or one that was successfully set before. Also I don't think the SCSI layer actually checks the slave_config return value.. sigh.

Then please at least add WARN_ON() && another reason why allocating / deallocating resources from ->slave_config isn't such a good idea.

..

The entire point of "slave_configure" is to provide a point for the LLD to do per-device data structure allocation/init.

And yes, SCSI does check the return code. Whether the code around that check
is buggy or not is another question, but it's always worked for me.

Quoted 6 lines

Could be done.. but, I don't want to constrain the ADMA APRD/CPB area in that way (there are some dual-socket Opteron boxes with this controller, forcing an allocation below 4GB for this could force a non-optimal node allocation I think..) To do this I'd have to raise the mask for the APRD allocation, drop it again, then raise it again in ADMA mode, which is kind of ugly.

I don't think it really matters. The table isn't too big and it's not like access to the table has any processor locality. Maybe it's better to allocate to the same node as the irq but raising DMA mask doesn't help at all.

I think performance impact is nil either way but even in highly unlikely case it has any impact, allocating PRDs under 4G should be better as it avoids DAC cycles on the bus. But again, this is just irrelevant.

I'd say just allocate everything under 4G.

Quoted 5 lines

Also, I'd rather not allocate the legacy PRD at all if we're in ADMA mode. That way, if some bug causes us to try and do legacy DMA in ADMA mode, we'll crash from null pointer dereference instead of potentially transferring incorrect data (as we had in this case) and corrupting things.

Yeap, I can agree with this. But can you add BUG_ON()/WARN_ON() at places instead? I know blanking pointers feel safer but I think it's best to keep resource allocation / release in ->port_start/stop().