Re: 2.4.21-pre3 kernel crash

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2003-01-28 at 23:06, Jens Axboe wrote:
> On Tue, Jan 28 2003, Larry Sendlosky wrote:
> > I was glad to see the physical page support in 2.4.20.
> > (and also noticed that the current BK tree clobbered it
> > on a patch set from Alan).
> > 
> > One question, 
> > 
> > +		lastdataend = bh_phys(bh) + bh->b_size;
> > 
> > bh_phys(x) uses bh->b_page. Does it make a difference
> > if bh->b_page is zero? What if someone combines virt and phys
> > buffer addresses in bh list?
> 
> Yes good catch! New version attached.

That's interesting. I wasn't awaye you could have a request
containing such a "mixed" set of bh without valid pages.
Actually, I though b_page was always valid. Looking at
other drivers (typically the the csiss.c driver), it also
unconditionally use b_page & bh_phys(). So either we are
looking at a false problem, or that driver need fixing as
well.

Now assuming that mix can happen, I don't like the fact that
your new version will use lastdataend to compare against both
physical and virtual addresses.

While I agree they should usually not collide (as those virtual
addresses are supposed to be grok-able by pci_map_* they are
in the linear mapping while the physical ones will typically
be smaller than c0000000), there is still some risk of
collision especially with HIGHMEM, or did I miss something ?

Maybe the solution is to have an additional variable indicating
if the last bh was virtual or physical, and reset lastdataend
to ~0 when the current one is different... 

Ben.

> ===== drivers/ide/ide-dma.c 1.7 vs edited =====
> --- 1.7/drivers/ide/ide-dma.c	Wed Nov 20 18:46:24 2002
> +++ edited/drivers/ide/ide-dma.c	Tue Jan 28 23:04:32 2003
> @@ -249,6 +249,7 @@
>  {
>  	struct buffer_head *bh;
>  	struct scatterlist *sg = hwif->sg_table;
> +	unsigned long lastdataend = ~0UL;
>  	int nents = 0;
>  
>  	if (hwif->sg_dma_active)
> @@ -256,24 +257,42 @@
>  
>  	bh = rq->bh;
>  	do {
> -		unsigned char *virt_addr = bh->b_data;
> -		unsigned int size = bh->b_size;
> +		int contig = 0;
> +
> +		if (bh->b_page) {
> +			if (bh_phys(bh) == lastdataend)
> +				contig = 1;
> +		} else {
> +			if ((unsigned long) bh->b_data == lastdataend)
> +				contig = 1;
> +		}
> +
> +		if (contig) {
> +			sg[nents - 1].length += bh->b_size;
> +			lastdataend += bh->b_size;
> +			continue;
> +		}
>  
>  		if (nents >= PRD_ENTRIES)
>  			return 0;
>  
> -		while ((bh = bh->b_reqnext) != NULL) {
> -			if ((virt_addr + size) != (unsigned char *) bh->b_data)
> -				break;
> -			size += bh->b_size;
> -		}
>  		memset(&sg[nents], 0, sizeof(*sg));
> -		sg[nents].address = virt_addr;
> -		sg[nents].length = size;
> -		if(size == 0)
> -			BUG();
> +
> +		if (bh->b_page) {
> +			sg[nents].page = bh->b_page;
> +			sg[nents].offset = bh_offset(bh);
> +			lastdataend = bh_phys(bh) + bh->b_size;
> +		} else {
> +			if ((unsigned long) bh->b_data < PAGE_SIZE)
> +				BUG();
> +
> +			sg[nents].address = bh->b_data;
> +			lastdataend = (unsigned long) bh->b_data + bh->b_size;
> +		}
> +
> +		sg[nents].length = bh->b_size;
>  		nents++;
> -	} while (bh != NULL);
> +	} while ((bh = bh->b_reqnext) != NULL);
>  
>  	if(nents == 0)
>  		BUG();
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>