Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Thu, 10 Dec 2009 09:43:38 -0600 James Bottomley wrote: > On Wed, 2009-12-02 at 17:51 -0800, Pravin Bathija wrote: > > Powerpc 44x uses 36 bit real address while the real address defined > > in MPT Fusion driver is of type 32 bit. This causes ioremap to fail and > > driver > > fails to initialize. This fix changes the data types representing the > > real > > address from unsigned long 32-bit types to resource_size_t which is > > 64-bit. The > > driver has been tested, the disks get discovered correctly and can do > > IO. > > > > Signed-off-by: Pravin Bathija > > Acked-by: Feng Kan > > Acked-by: Fushen Chen > > Acked-by: Loc Ho > > Acked-by: Tirumala Reddy Marri > > Acked-by: Victor Gallardo > > --- > > drivers/message/fusion/mptbase.c |2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/message/fusion/mptbase.c > > b/drivers/message/fusion/mptbase.c > > index 5d496a9..9f14a60 100644 > > --- a/drivers/message/fusion/mptbase.c > > +++ b/drivers/message/fusion/mptbase.c > > @@ -1511,7 +1511,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > > { > > u8 __iomem *mem; > > int ii; > > - unsigned longmem_phys; > > + resource_size_t mem_phys; > > You never actually compiled this, did you? > > drivers/message/fusion/mptbase.c: In function 'mpt_mapresources': > drivers/message/fusion/mptbase.c:1680: warning: format '%lx' expects type > 'long unsigned int', but argument 4 has type 'resource_size_t' > > I'll just fold the fix in another patch (inlined below) should probably also go in as 'mem_phys' is assigned to ioc->mem_phys which is 'u32'. ioc->mem_phys is never used in the driver, however. Some time ago I posted a patch which enables using second LSI SAS HBA on PPC440SPe based katmai board again: http://thread.gmane.org/gmane.linux.scsi/54839 Could someone comment on this patch, please. Thanks! Anatolij --- diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h index 8dd4d21..8dc58e3 100644 --- a/drivers/message/fusion/mptbase.h +++ b/drivers/message/fusion/mptbase.h @@ -605,7 +605,7 @@ typedef struct _MPT_ADAPTER SYSIF_REGS __iomem *chip; /* == c8817000 (mmap) */ SYSIF_REGS __iomem *pio_chip; /* Programmed IO (downloadboot) */ u8 bus_type; - u32 mem_phys; /* == f402 (mmap) */ + resource_size_t mem_phys; /* == f402 (mmap) */ u32 pio_mem_phys; /* Programmed IO (downloadboot) */ int mem_size; /* mmap memory size */ int number_of_buses; ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Wed, 2009-12-02 at 17:51 -0800, Pravin Bathija wrote: > Powerpc 44x uses 36 bit real address while the real address defined > in MPT Fusion driver is of type 32 bit. This causes ioremap to fail and > driver > fails to initialize. This fix changes the data types representing the real > address from unsigned long 32-bit types to resource_size_t which is > 64-bit. The > driver has been tested, the disks get discovered correctly and can do IO. > > Signed-off-by: Pravin Bathija > Acked-by: Feng Kan > Acked-by: Fushen Chen > Acked-by: Loc Ho > Acked-by: Tirumala Reddy Marri > Acked-by: Victor Gallardo > --- > drivers/message/fusion/mptbase.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/message/fusion/mptbase.c > b/drivers/message/fusion/mptbase.c > index 5d496a9..9f14a60 100644 > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1511,7 +1511,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > { > u8 __iomem *mem; > int ii; > - unsigned longmem_phys; > + resource_size_t mem_phys; You never actually compiled this, did you? drivers/message/fusion/mptbase.c: In function 'mpt_mapresources': drivers/message/fusion/mptbase.c:1680: warning: format '%lx' expects type 'long unsigned int', but argument 4 has type 'resource_size_t' I'll just fold the fix in James --- diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c index 162923f..85bc6a6 100644 --- a/drivers/message/fusion/mptbase.c +++ b/drivers/message/fusion/mptbase.c @@ -1677,8 +1677,8 @@ mpt_mapresources(MPT_ADAPTER *ioc) return -EINVAL; } ioc->memmap = mem; - dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", - ioc->name, mem, mem_phys)); + dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %llx\n", + ioc->name, mem, (unsigned long long)mem_phys)); ioc->mem_phys = mem_phys; ioc->chip = (SYSIF_REGS __iomem *)mem; ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Thu, 2009-12-03 at 15:21 -0800, Pravin Bathija wrote: > Hi Wolfgang, .../... > > I'm not sure if this one-liner really covers all the related issues. > > We submitted a similar (but apparently more complete) patch more than > > a year ago. Dunno why it has never been picked up. See > > http://thread.gmane.org/gmane.linux.scsi/46082 for reference. > > > > I submitted a patch on similar lines several weeks ago and it wasn't > accepted on grounds that it was too tied to the powerpc platform. Below > is a link > > http://article.gmane.org/gmane.linux.scsi/55794 I believe the simple patch is fine. But only testing can tell, so it's up to you guys to test it :-) None of the churn related to PIO that was in the previous patches is necessary. PIO on powerpc "appears" to work just like x86, the illusion is maintained by the arch code. PIO resources always fit inside 32-bit, never need to be ioremapped etc... so as long as you use the result of pci_resource_start() and pass that (or an offset from that) to inb/outb/intw/outw... PIO should just work, no change is required to the driver. IE. PIO resources don't contain physical addresses. The powerpc PCI code puts in there an offset from _IO_BASE to an already ioremapped area mapping the PCI host bridge IO space. It can use funky pointer arithmetic so don't be surprised if the values look like negative 32-bit ints, but it should just work. The only problem I can see with the driver, which is fixed by the simple patch, is that for -MMIO-, the resources (which in the case of MMIO do contain physical addresses) can be >32 bit, and thus must be stored into a type of the right size before being passed to ioremap(). This is what the one-liner patch does and according to the patch author, it was tested and appears to work. So I'm happy with the one liner patch. If you have more concerns, please explain precisely what you believe will not work :-) Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
Hi Wolfgang, > -Original Message- > From: Wolfgang Denk [mailto:w...@denx.de] > Sent: Thursday, December 03, 2009 12:56 AM > To: Pravin Bathija; Benjamin Herrenschmidt; Desai, Kashyap > Cc: linux-s...@vger.kernel.org; linuxppc-...@ozlabs.org; > eric.mo...@lsi.com > Subject: Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 > bit resources. > > Dear Pravin Bathija, > > In message <1259805106-23636-1-git-send-email-pbath...@amcc.com> you > wrote: > > Powerpc 44x uses 36 bit real address while the real address > defined > > in MPT Fusion driver is of type 32 bit. This causes ioremap to > fail and driver > > fails to initialize. This fix changes the data types representing > the real > > address from unsigned long 32-bit types to resource_size_t which > is 64-bit. The > > driver has been tested, the disks get discovered correctly and > can do IO. > ... > > --- a/drivers/message/fusion/mptbase.c > > +++ b/drivers/message/fusion/mptbase.c > > @@ -1511,7 +1511,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > > { > > u8 __iomem *mem; > > int ii; > > - unsigned longmem_phys; > > + resource_size_t mem_phys; > > unsigned longport; > > u32 msize; > > u32 psize; > > I'm not sure if this one-liner really covers all the related issues. > We submitted a similar (but apparently more complete) patch more than > a year ago. Dunno why it has never been picked up. See > http://thread.gmane.org/gmane.linux.scsi/46082 for reference. > I submitted a patch on similar lines several weeks ago and it wasn't accepted on grounds that it was too tied to the powerpc platform. Below is a link http://article.gmane.org/gmane.linux.scsi/55794 > > > From: Yuri Tikhonov > To: linux-s...@vger.kernel.org > Subject: [PATCH] mptbase: use resource_size_t instead of unsigned long > and u32 > Date: Thu, 13 Nov 2008 11:33:16 +0300 > > Hello, > > The following patch adds using resource_size_t for the > pci_resource_start()/pci_resource_len() return values. This > makes mptbase driver work correctly on 32 bit systems with > 64 bit resources (e.g. PPC440SPe). > > Do some minor cleanups in mpt_mapresources() as well. > > Signed-off-by: Yuri Tikhonov > Signed-off-by: Ilya Yanok > --- > drivers/message/fusion/mptbase.c | 38 ++ > > drivers/message/fusion/mptbase.h |5 +++-- > 2 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/message/fusion/mptbase.c > b/drivers/message/fusion/mptbase.c > index d6a0074..9daf844 100644 > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1488,11 +1488,12 @@ static int > mpt_mapresources(MPT_ADAPTER *ioc) > { > u8 __iomem *mem; > + u8 __iomem *port; > int ii; > - unsigned longmem_phys; > - unsigned longport; > - u32 msize; > - u32 psize; > + resource_size_t mem_phys; > + resource_size_t port_phys; > + resource_size_t msize; > + resource_size_t psize; > u8 revision; > int r = -ENODEV; > struct pci_dev *pdev; > @@ -1530,13 +1531,13 @@ mpt_mapresources(MPT_ADAPTER *ioc) > } > > mem_phys = msize = 0; > - port = psize = 0; > + port_phys = psize = 0; > for (ii = 0; ii < DEVICE_COUNT_RESOURCE; ii++) { > if (pci_resource_flags(pdev, ii) & > PCI_BASE_ADDRESS_SPACE_IO) { > if (psize) > continue; > /* Get I/O space! */ > - port = pci_resource_start(pdev, ii); > + port_phys = pci_resource_start(pdev, ii); > psize = pci_resource_len(pdev, ii); > } else { > if (msize) > @@ -1546,11 +1547,8 @@ mpt_mapresources(MPT_ADAPTER *ioc) > msize = pci_resource_len(pdev, ii); > } > } > - ioc->mem_size = msize; > > - mem = NULL; > /* Get logical ptr for PciMem0 space */ > - /*mem = ioremap(mem_phys, msize);*/ > mem = ioremap(mem_phys, msize); > if (mem == NULL) { > printk(MYIOC_s_ERR_FMT ": ERROR - Unable to map adapter" > @@ -1558,14 +1556,24 @@ mpt_mapresources(MPT_ADAPTER *ioc) > return -EINVAL; > } > ioc->memmap = mem; > - dinitprintk(ioc, printk(MYIOC_s_INFO_FM
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
Dear Pravin Bathija, In message <1259805106-23636-1-git-send-email-pbath...@amcc.com> you wrote: > Powerpc 44x uses 36 bit real address while the real address defined > in MPT Fusion driver is of type 32 bit. This causes ioremap to fail and > driver > fails to initialize. This fix changes the data types representing the real > address from unsigned long 32-bit types to resource_size_t which is > 64-bit. The > driver has been tested, the disks get discovered correctly and can do IO. ... > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1511,7 +1511,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > { > u8 __iomem *mem; > int ii; > - unsigned longmem_phys; > + resource_size_t mem_phys; > unsigned longport; > u32 msize; > u32 psize; I'm not sure if this one-liner really covers all the related issues. We submitted a similar (but apparently more complete) patch more than a year ago. Dunno why it has never been picked up. See http://thread.gmane.org/gmane.linux.scsi/46082 for reference. > From: Yuri Tikhonov To: linux-s...@vger.kernel.org Subject: [PATCH] mptbase: use resource_size_t instead of unsigned long and u32 Date: Thu, 13 Nov 2008 11:33:16 +0300 Hello, The following patch adds using resource_size_t for the pci_resource_start()/pci_resource_len() return values. This makes mptbase driver work correctly on 32 bit systems with 64 bit resources (e.g. PPC440SPe). Do some minor cleanups in mpt_mapresources() as well. Signed-off-by: Yuri Tikhonov Signed-off-by: Ilya Yanok --- drivers/message/fusion/mptbase.c | 38 ++ drivers/message/fusion/mptbase.h |5 +++-- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c index d6a0074..9daf844 100644 --- a/drivers/message/fusion/mptbase.c +++ b/drivers/message/fusion/mptbase.c @@ -1488,11 +1488,12 @@ static int mpt_mapresources(MPT_ADAPTER *ioc) { u8 __iomem *mem; + u8 __iomem *port; int ii; - unsigned longmem_phys; - unsigned longport; - u32 msize; - u32 psize; + resource_size_t mem_phys; + resource_size_t port_phys; + resource_size_t msize; + resource_size_t psize; u8 revision; int r = -ENODEV; struct pci_dev *pdev; @@ -1530,13 +1531,13 @@ mpt_mapresources(MPT_ADAPTER *ioc) } mem_phys = msize = 0; - port = psize = 0; + port_phys = psize = 0; for (ii = 0; ii < DEVICE_COUNT_RESOURCE; ii++) { if (pci_resource_flags(pdev, ii) & PCI_BASE_ADDRESS_SPACE_IO) { if (psize) continue; /* Get I/O space! */ - port = pci_resource_start(pdev, ii); + port_phys = pci_resource_start(pdev, ii); psize = pci_resource_len(pdev, ii); } else { if (msize) @@ -1546,11 +1547,8 @@ mpt_mapresources(MPT_ADAPTER *ioc) msize = pci_resource_len(pdev, ii); } } - ioc->mem_size = msize; - mem = NULL; /* Get logical ptr for PciMem0 space */ - /*mem = ioremap(mem_phys, msize);*/ mem = ioremap(mem_phys, msize); if (mem == NULL) { printk(MYIOC_s_ERR_FMT ": ERROR - Unable to map adapter" @@ -1558,14 +1556,24 @@ mpt_mapresources(MPT_ADAPTER *ioc) return -EINVAL; } ioc->memmap = mem; - dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", - ioc->name, mem, mem_phys)); + dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem=%p, mem_phys=%llx\n", + ioc->name, mem, (u64)mem_phys)); ioc->mem_phys = mem_phys; ioc->chip = (SYSIF_REGS __iomem *)mem; /* Save Port IO values in case we need to do downloadboot */ - ioc->pio_mem_phys = port; + port = ioremap(port_phys, psize); + if (port == NULL) { + printk(MYIOC_s_ERR_FMT ": ERROR - Unable to map adapter" + " port!\n", ioc->name); + return -EINVAL; + } + ioc->portmap = port; + dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "port=%p, port_phys=%llx\n", + ioc->name, port, (u64)port_phys)); + + ioc->pio_mem_phys = port_phys; ioc->pio_chip = (SYSIF_REGS __iomem *)port; return 0; @@ -1790,6 +1798,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) list_del(&ioc->list); if (ioc->alt_ioc) ioc->alt_ioc->alt_ioc = NULL; + iounmap(ioc->portmap); iou
RE: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
This patch looks OK. Please consider it as ACKed by me on behalf of LSI. - Kashyap > -Original Message- > From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- > ow...@vger.kernel.org] On Behalf Of Benjamin Herrenschmidt > Sent: Thursday, December 03, 2009 8:30 AM > To: Pravin Bathija > Cc: linux-s...@vger.kernel.org; linuxppc-...@ozlabs.org; > jwbo...@linux.vnet.ibm.com; Moore, Eric > Subject: Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit > resources. > > On Wed, 2009-12-02 at 17:51 -0800, Pravin Bathija wrote: > > Powerpc 44x uses 36 bit real address while the real address defined > > in MPT Fusion driver is of type 32 bit. This causes ioremap to fail > and driver > > fails to initialize. This fix changes the data types representing > the real > > address from unsigned long 32-bit types to resource_size_t which is > 64-bit. The > > driver has been tested, the disks get discovered correctly and can > do IO. > > > > Signed-off-by: Pravin Bathija > > Acked-by: Benjamin Herrenschmidt > --- > > James, this one should be good ;-) > > > Acked-by: Feng Kan > > Acked-by: Fushen Chen > > Acked-by: Loc Ho > > Acked-by: Tirumala Reddy Marri > > Acked-by: Victor Gallardo > > --- > > drivers/message/fusion/mptbase.c |2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/message/fusion/mptbase.c > b/drivers/message/fusion/mptbase.c > > index 5d496a9..9f14a60 100644 > > --- a/drivers/message/fusion/mptbase.c > > +++ b/drivers/message/fusion/mptbase.c > > @@ -1511,7 +1511,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > > { > > u8 __iomem *mem; > > int ii; > > - unsigned longmem_phys; > > + resource_size_t mem_phys; > > unsigned longport; > > u32 msize; > > u32 psize; > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Wed, 2009-12-02 at 17:51 -0800, Pravin Bathija wrote: > Powerpc 44x uses 36 bit real address while the real address defined > in MPT Fusion driver is of type 32 bit. This causes ioremap to fail and > driver > fails to initialize. This fix changes the data types representing the real > address from unsigned long 32-bit types to resource_size_t which is > 64-bit. The > driver has been tested, the disks get discovered correctly and can do IO. > > Signed-off-by: Pravin Bathija Acked-by: Benjamin Herrenschmidt --- James, this one should be good ;-) > Acked-by: Feng Kan > Acked-by: Fushen Chen > Acked-by: Loc Ho > Acked-by: Tirumala Reddy Marri > Acked-by: Victor Gallardo > --- > drivers/message/fusion/mptbase.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/message/fusion/mptbase.c > b/drivers/message/fusion/mptbase.c > index 5d496a9..9f14a60 100644 > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1511,7 +1511,7 @@ mpt_mapresources(MPT_ADAPTER *ioc) > { > u8 __iomem *mem; > int ii; > - unsigned longmem_phys; > + resource_size_t mem_phys; > unsigned longport; > u32 msize; > u32 psize; ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Tue, 2009-11-17 at 16:16 -0800, pbath...@amcc.com wrote: > From: Pravin Bathija > > Powerpc 44x uses 36 bit real address while the real address defined > in MPT Fusion driver is of type 32 bit. This causes ioremap to fail and > driver > fails to initialize. This fix changes the data types representing the real > address from unsigned long 32-bit types to "phys_addr_t" which is 64-bit. > The > driver has been tested, the disks get discovered correctly and can do IO. > Removed > ioremap and used hose->io_base_virt for IO space to make it platform > independent. > Content-Type: text/plain; charset=utf-8 Except that this is all wrong :-) You basically made it powerpc specific since none of that pci controller stuff is generic. I don't understand what you are trying to do though. The -only- change you need to do is to change the longs into resource_size_t. IO Port numbers are "special" and handled as such already (and besides are never bigger than 32-bit neither, at least on x86 and powerpc). Just leave the PIO code alone, hopefully, if the driver isn't full of crack, the code should be fine already. If the driver does something wrong then you can attempt to fix it separately. The only problem that you need to address afaik, is purely that pci_resource_start() can return a resource_size_t which can be 64-bit, and as such it is broken for the driver to manipulate and store that value as an unsigned long or a u32. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
-Original Message- From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] Sent: Thu 11/5/2009 12:00 PM To: James Bottomley Cc: Josh Boyer; eric.mo...@lsi.com; Pravin Bathija; linux-s...@vger.kernel.org; linuxppc-...@ozlabs.org Subject: Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources. On Thu, 2009-11-05 at 10:07 -0600, James Bottomley wrote: > > > ioc->memmap = mem; > > >- dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", > > >- ioc->name, mem, mem_phys)); > > >+ dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %llx\n", > > >+ ioc->name, mem, (u64)mem_phys)); > > > > > > ioc->mem_phys = mem_phys; > > > ioc->chip = (SYSIF_REGS __iomem *)mem; > > > > > > /* Save Port IO values in case we need to do downloadboot */ > > >- ioc->pio_mem_phys = port; > > >+ port = ioremap(port_phys, psize); > > >+ if (port == NULL) { > > >+ printk(MYIOC_s_ERR_FMT " : ERROR - Unable to map adapter" > > >+ " port !\n", ioc->name); > > >+ return -EINVAL; > > So this looks problematic on a few platforms ... what happens to > platforms that have no IO space? They automatically fail here and it > looks like the adapter never attaches. > Yup, that part of the patch looks wrong. > However, a mechanical replacement of unsigned long's with > resource_size_t to hold physical addresses should be fine despite the > lack of feedback from LSI. > Pravin, that ioremap definitely seems like it has nothing to do there, > port IO is already remapped for you by the core PCI code and should work > "as is". Please respin without that change. > Cheers, >Ben. Thanks for the input. Will make the suggested changes and re-submit the patch. Regards, Pravin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
-Original Message- From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Benjamin Herrenschmidt Sent: Friday, November 06, 2009 1:30 AM To: James Bottomley Cc: Josh Boyer; Moore, Eric; pbath...@amcc.com; linux-s...@vger.kernel.org; linuxppc-...@ozlabs.org Subject: Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources. On Thu, 2009-11-05 at 10:07 -0600, James Bottomley wrote: > > > ioc->memmap = mem; > > >- dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", > > >- ioc->name, mem, mem_phys)); > > >+ dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %llx\n", > > >+ ioc->name, mem, (u64)mem_phys)); > > > > > > ioc->mem_phys = mem_phys; > > > ioc->chip = (SYSIF_REGS __iomem *)mem; > > > > > > /* Save Port IO values in case we need to do downloadboot */ > > >- ioc->pio_mem_phys = port; > > >+ port = ioremap(port_phys, psize); > > >+ if (port == NULL) { > > >+ printk(MYIOC_s_ERR_FMT " : ERROR - Unable to map adapter" > > >+ " port !\n", ioc->name); > > >+ return -EINVAL; > > So this looks problematic on a few platforms ... what happens to > platforms that have no IO space? They automatically fail here and it > looks like the adapter never attaches. Yup, that part of the patch looks wrong. However, a mechanical replacement of unsigned long's with resource_size_t to hold physical addresses should be fine despite the lack of feedback from LSI. --> I was thinking this was actual fix for the issue. Use of resource_size_t is understood. Why submitter has added extra ioremap code in this patch? Because of ioremap code only this patch was on hold before going for ACK. Pravin, Any specific reason to add above code? Pravin, that ioremap definitely seems like it has nothing to do there, port IO is already remapped for you by the core PCI code and should work "as is". Please respin without that change. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
> -Original Message- > From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] > Sent: Thursday, November 05, 2009 12:00 PM > To: James Bottomley > Cc: Josh Boyer; eric.mo...@lsi.com; Pravin Bathija; linux- > s...@vger.kernel.org; linuxppc-...@ozlabs.org > Subject: Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 > bit resources. > > On Thu, 2009-11-05 at 10:07 -0600, James Bottomley wrote: > > > > > ioc->memmap = mem; > > > >-dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, > mem_phys = %lx\n", > > > >-ioc->name, mem, mem_phys)); > > > >+dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, > mem_phys = %llx\n", > > > >+ioc->name, mem, (u64)mem_phys)); > > > > > > > > ioc->mem_phys = mem_phys; > > > > ioc->chip = (SYSIF_REGS __iomem *)mem; > > > > > > > > /* Save Port IO values in case we need to do downloadboot > */ > > > >-ioc->pio_mem_phys = port; > > > >+port = ioremap(port_phys, psize); > > > >+if (port == NULL) { > > > >+printk(MYIOC_s_ERR_FMT " : ERROR - Unable to map > adapter" > > > >+" port !\n", ioc->name); > > > >+return -EINVAL; > > > > So this looks problematic on a few platforms ... what happens to > > platforms that have no IO space? They automatically fail here and it > > looks like the adapter never attaches. > > Yup, that part of the patch looks wrong. > > However, a mechanical replacement of unsigned long's with > resource_size_t to hold physical addresses should be fine despite the > lack of feedback from LSI. > > Pravin, that ioremap definitely seems like it has nothing to do there, > port IO is already remapped for you by the core PCI code and should > work > "as is". Please respin without that change. > > Cheers, > Ben. > Thanks for the input. Will make the suggested changes and re-submit patch. Regards, Pravin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Thu, 2009-11-05 at 08:43 -0500, Josh Boyer wrote: > On Tue, Sep 15, 2009 at 03:25:55PM -0700, pbath...@amcc.com wrote: > >From: Pravin Bathija > > > >Powerpc 44x uses 36 bit real address while the real address defined > >in MPT Fusion driver is of type 32 bit. This causes ioremap to fail and > >driver > >fails to initialize. This fix changes the data types representing the real > >address from unsigned long 32-bit types to "phys_addr_t" which is 64-bit. The > >driver has been tested, the disks get discovered correctly and can do IO. > >Also, > >replaced phys_addr_t with resource_size_t as suggested by Ben. > > > >Signed-off-by: Pravin Bathija > >Acked-by: Feng Kan > >Acked-by: Prodyut Hazarika > >Acked-by: Loc Ho > >Acked-by: Tirumala Reddy Marri > >Acked-by: Victor Gallardo > > Is this patch included in the scsi tree at all? I can't seem to find it in > linux-next and I know it's not in the powerpc tree. Are there further changes > needed, or has it simply been missed? What was the feedback from LSI ... I haven't seen any here? > josh > > > > >--- > > drivers/message/fusion/mptbase.c | 34 +- > > drivers/message/fusion/mptbase.h |5 +++-- > > 2 files changed, 28 insertions(+), 11 deletions(-) > > > >diff --git a/drivers/message/fusion/mptbase.c > >b/drivers/message/fusion/mptbase.c > >index 5d496a9..e296f2e 100644 > >--- a/drivers/message/fusion/mptbase.c > >+++ b/drivers/message/fusion/mptbase.c > >@@ -1510,11 +1510,12 @@ static int > > mpt_mapresources(MPT_ADAPTER *ioc) > > { > > u8 __iomem *mem; > >+u8 __iomem *port; > > int ii; > >-unsigned longmem_phys; > >-unsigned longport; > >-u32 msize; > >-u32 psize; > >+resource_size_t mem_phys; > >+resource_size_t port_phys; > >+resource_size_t msize; > >+resource_size_t psize; > > u8 revision; > > int r = -ENODEV; > > struct pci_dev *pdev; > >@@ -1552,13 +1553,13 @@ mpt_mapresources(MPT_ADAPTER *ioc) > > } > > > > mem_phys = msize = 0; > >-port = psize = 0; > >+port_phys = psize = 0; > > for (ii = 0; ii < DEVICE_COUNT_RESOURCE; ii++) { > > if (pci_resource_flags(pdev, ii) & PCI_BASE_ADDRESS_SPACE_IO) { > > if (psize) > > continue; > > /* Get I/O space! */ > >-port = pci_resource_start(pdev, ii); > >+port_phys = pci_resource_start(pdev, ii); > > psize = pci_resource_len(pdev, ii); > > } else { > > if (msize) > >@@ -1580,14 +1581,23 @@ mpt_mapresources(MPT_ADAPTER *ioc) > > return -EINVAL; > > } > > ioc->memmap = mem; > >-dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", > >-ioc->name, mem, mem_phys)); > >+dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %llx\n", > >+ioc->name, mem, (u64)mem_phys)); > > > > ioc->mem_phys = mem_phys; > > ioc->chip = (SYSIF_REGS __iomem *)mem; > > > > /* Save Port IO values in case we need to do downloadboot */ > >-ioc->pio_mem_phys = port; > >+port = ioremap(port_phys, psize); > >+if (port == NULL) { > >+printk(MYIOC_s_ERR_FMT " : ERROR - Unable to map adapter" > >+" port !\n", ioc->name); > >+return -EINVAL; So this looks problematic on a few platforms ... what happens to platforms that have no IO space? They automatically fail here and it looks like the adapter never attaches. James ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources.
On Thu, 2009-11-05 at 10:07 -0600, James Bottomley wrote: > > > ioc->memmap = mem; > > >- dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %lx\n", > > >- ioc->name, mem, mem_phys)); > > >+ dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "mem = %p, mem_phys = %llx\n", > > >+ ioc->name, mem, (u64)mem_phys)); > > > > > > ioc->mem_phys = mem_phys; > > > ioc->chip = (SYSIF_REGS __iomem *)mem; > > > > > > /* Save Port IO values in case we need to do downloadboot */ > > >- ioc->pio_mem_phys = port; > > >+ port = ioremap(port_phys, psize); > > >+ if (port == NULL) { > > >+ printk(MYIOC_s_ERR_FMT " : ERROR - Unable to map adapter" > > >+ " port !\n", ioc->name); > > >+ return -EINVAL; > > So this looks problematic on a few platforms ... what happens to > platforms that have no IO space? They automatically fail here and it > looks like the adapter never attaches. Yup, that part of the patch looks wrong. However, a mechanical replacement of unsigned long's with resource_size_t to hold physical addresses should be fine despite the lack of feedback from LSI. Pravin, that ioremap definitely seems like it has nothing to do there, port IO is already remapped for you by the core PCI code and should work "as is". Please respin without that change. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] [SCSI] mpt fusion: Fix 32 bit platforms with 64 bit resources
> diff --git a/drivers/message/fusion/mptbase.c > b/drivers/message/fusion/mptbase.c > index 5d496a9..d5b0f15 100644 > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1510,11 +1510,12 @@ static int > mpt_mapresources(MPT_ADAPTER *ioc) > { > u8 __iomem *mem; > + u8 __iomem *port; > int ii; > - unsigned longmem_phys; > - unsigned longport; > - u32 msize; > - u32 psize; > + phys_addr_t mem_phys; > + phys_addr_t port_phys; > + resource_size_t msize; > + resource_size_t psize; Is phys_addr_t defined for all archs nowadays ? Why not use resource_size_t for everything ? resource_size_t is a bit of a misnomer, it's not a type supposed to reference a "size" but really a physical address (or a size)... it's been called resource_size_t I believe because it's "sized" appropriately for holding a physical address. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev