From: Konrad Rzeszutek <konrad@darnok.org>
To: James Bottomley <James.Bottomley@hansenpartnership.com>
Subject: Re: [PATCH] Add iSCSI iBFT support (v0.4.6)
Date: Fri, 8 Feb 2008 15:37:17 -0500
User-Agent: KMail/1.9.6
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org,
       Greg KH <greg@kroah.com>, michaelc@cs.wisc.edu, dwm@enoyolf.org,
       darnok@68k.org, pjones@redhat.com, konradr@redhat.com,
       konradr@linux.vnet.ibm.com, randy.dunlap@oracle.com, hpa@zytor.com,
       lenb@kernel.org, mike.anderson@us.ibm.com, dwm@austin.ibm.com,
       arjan@infradead.org, Andy Whitcroft <apw@shadowen.org>
References: <20080130213700.GA28529@andromeda.dapyr.net> <1201911489.3134.79.camel@localhost.localdomain>
In-Reply-To: <1201911489.3134.79.camel@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain;
  charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <200802081537.33767.konrad@darnok.org>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1364
Lines: 35

On Friday 01 February 2008 19:18:09 James Bottomley wrote:
> On Wed, 2008-01-30 at 17:37 -0400, Konrad Rzeszutek wrote:
> > This patch (v0.4.6) adds

> Some pieces of the patch are obviously wrong:  find_ibft() shouldn't be
> in ibft_init ... if ibft_phys was zero, it means the bootmem reservation
> wasn't done and you shouldn't be poking about in memory which has likely
> now been overwritten.
Fixed.
>
> Also, why is ibft_phys the global variable you pass?  You never actually
> want to use the physical address, what you always end up using is the
> kernel virtual address.
>
> I'd simply use the ibft variable to point to the virtual address of the
> ibft or null if not found, then you can throw out all the phys_to_virt()
> calls.
Fixed.
>
> Also, move the reserve_bootmem into the ibft_find routines and ensure
> they're only called once on boot.  Refuse to attach the ibft driver if
> the virtual pointer ibft is null.

James,

Thanks for your review. If you wouldn't mind, can you take a look
at the version 0.4.7 (http://lkml.org/lkml/2008/2/8/350) which has your 
suggestions incorporated.

Thanks again.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/