st.c page_to_pfn() usage

Hi,

I'm auditing kernel uses of page_to_pfn() and I came across st.c. st.c uses page_to_pfn() twice, the first usage is:

if (page_to_pfn(STbp->sg[i].page) == page_to_pfn(STbp->sg[i-1].page) + 1)

	STp->nbr_combinable++;

which looks quite incorrect, I *think* the code wants to check "physically contiguous" however pfn is the wrong thing to check there; with discontigmem (which is basically true now for all 64 bit architectures), the assumption "pfn and pfn+1 are physically next to each other" isn't per se true, example:

|----- zone 1 ----------| gap |-------- zone 2 ----------|

the last page of zone 1 and the first page of zone 2 may be 1 pfn appart in the discontigmem case, but physically far appart.

the other page_to_pfn() user is st_map_user_pages():

        for (i=0; i < nr_pages; i++) {
                if (page_to_pfn(sgl[i].page) > max_pfn)
                        goto out_unmap;
        }

and again I get the suspicion this is not the right thing to do... it *looks* like you want to detect userspace mmaped vmalloc() pages or mmaped pci mmio ranges and the like, however the test is entirely the wrong thing for that, take an average x86 server PC with say 6Gb of memory:

0 3.5Gb 4Gb 6.5Gb
|-------------------||-- PCI GAP --||---------------|

and something similar happens for vmalloc; however I fully admit that I don't quite entirely understand the purpose of this code yet, so maybe I'm misreading the intention of the code

I'm rather hard pressed to see how contiguous pfn's would result in discontiguous memory. They're meant to be truly physical; the only abuses of this I'm aware of are for hypervisor-related use, whose IO invariants are such that all these things about bouncing and most all handling of physical limitations of devices are irrelevant anyway.

I'm not sure what the numbers described in that diagram have relations to. The 2.4.x invariant was that within a zone pfn's were monotonically increasing, the 2.6.x invariant is that zones are literally intervals of pfn's, which as one might expect makes various machines unhappy.

The name max_pfn is almost certainly an accident, as the name of the formal parameter clashes with the global variable. This seems to be a speculative attempt to do IO without bouncing for the direct IO case.

The max_pfn passed is STp->max_pfn, which I suppose is Hungarian notation for some pfn stored in a structure meant to represent the bounce_pfn for the tape device. I suppose one could hunt for where it's initialized.

-- wli

The code is inside DEB(), i.e., it is debugging code. The purpose of the code is to count the occasions where physically contiguous pages could be combined to a single scatter/gather segment. No decisions are made based on the result. My experiments suggested that combining is not usually possible and it is not worth trying. I did not rip off the code in case it would prove useful later. If it is incorrect, the easiest solution is to rip off everything related to STp->nbr_combinable.

The purpose of the code is to check if the pages are directly reachable by the SCSI HBA. If not, the driver reverts to bouncing all pages of this request. As William points out, a better name for the field would have been bounce_pfn. The value is computed by the code fragment:

        bounce_limit = scsi_calculate_bounce_limit(SDp->host) >> PAGE_SHIFT;
        if (bounce_limit > ULONG_MAX)
                bounce_limit = ULONG_MAX;
        tpnt->max_pfn = bounce_limit;

I have never been quite sure that this is correct in all architectures. However, I have copied the usage from ll_rw_blk.c. If you suggest a better method, please see that ll_rw_blk.c is also fixed (unless I have read ll_rw_blk.c incorrectly).

Kai

just ignore my email; as william points out I'm flat out wrong....