| Message ID | 1338904504-2750-1-git-send-email-bharat.bhushan@freescale.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | a5cb82da786f610f7e84a0a8bcf9e0218c363040 |
| Delegated to: | Michael Ellerman |
| Headers | show |
On Tue, 2012-06-05 at 19:25 +0530, Bharat Bhushan wrote: > memblock_end_of_DRAM() returns end_address + 1, not end address. > While some code assumes that it returns end address. Shouldn't we instead fix it the other way around ? IE, make memblock_end_of_DRAM() does what the name implies, which is to return the last byte of DRAM, and fix the -other- callers not to make bad assumptions ? Cheers, Ben. > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > --- > This patch is based on next branch of https://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git > > arch/powerpc/platforms/44x/currituck.c | 2 +- > arch/powerpc/platforms/85xx/corenet_ds.c | 2 +- > arch/powerpc/platforms/85xx/ge_imp3a.c | 2 +- > arch/powerpc/platforms/85xx/mpc8536_ds.c | 2 +- > arch/powerpc/platforms/85xx/mpc85xx_ds.c | 2 +- > arch/powerpc/platforms/85xx/mpc85xx_mds.c | 2 +- > arch/powerpc/platforms/85xx/p1022_ds.c | 2 +- > arch/powerpc/platforms/86xx/mpc86xx_hpcn.c | 2 +- > 8 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/platforms/44x/currituck.c b/arch/powerpc/platforms/44x/currituck.c > index 583e67f..9f6c33d 100644 > --- a/arch/powerpc/platforms/44x/currituck.c > +++ b/arch/powerpc/platforms/44x/currituck.c > @@ -160,7 +160,7 @@ static void __init ppc47x_setup_arch(void) > /* No need to check the DMA config as we /know/ our windows are all of > * RAM. Lets hope that doesn't change */ > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > 0xffffffff) { > + if ((memblock_end_of_DRAM() - 1) > 0xffffffff) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c b/arch/powerpc/platforms/85xx/corenet_ds.c > index dd3617c..925b028 100644 > --- a/arch/powerpc/platforms/85xx/corenet_ds.c > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c > @@ -77,7 +77,7 @@ void __init corenet_ds_setup_arch(void) > #endif > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/85xx/ge_imp3a.c b/arch/powerpc/platforms/85xx/ge_imp3a.c > index 1801462..b6a728b 100644 > --- a/arch/powerpc/platforms/85xx/ge_imp3a.c > +++ b/arch/powerpc/platforms/85xx/ge_imp3a.c > @@ -125,7 +125,7 @@ static void __init ge_imp3a_setup_arch(void) > mpc85xx_smp_init(); > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c b/arch/powerpc/platforms/85xx/mpc8536_ds.c > index 585bd22..767c7cf 100644 > --- a/arch/powerpc/platforms/85xx/mpc8536_ds.c > +++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c > @@ -75,7 +75,7 @@ static void __init mpc8536_ds_setup_arch(void) > #endif > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c > index 1fd91e9..d30f6c4 100644 > --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c > +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c > @@ -173,7 +173,7 @@ static void __init mpc85xx_ds_setup_arch(void) > mpc85xx_smp_init(); > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > index d208ebc..8e4b094 100644 > --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c > +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > @@ -359,7 +359,7 @@ static void __init mpc85xx_mds_setup_arch(void) > mpc85xx_mds_qe_init(); > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c > index f700c81..74e310b 100644 > --- a/arch/powerpc/platforms/85xx/p1022_ds.c > +++ b/arch/powerpc/platforms/85xx/p1022_ds.c > @@ -450,7 +450,7 @@ static void __init p1022_ds_setup_arch(void) > mpc85xx_smp_init(); > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; > diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c > index 3755e61..817245b 100644 > --- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c > +++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c > @@ -102,7 +102,7 @@ mpc86xx_hpcn_setup_arch(void) > #endif > > #ifdef CONFIG_SWIOTLB > - if (memblock_end_of_DRAM() > max) { > + if ((memblock_end_of_DRAM() - 1) > max) { > ppc_swiotlb_enable = 1; > set_pci_dma_ops(&swiotlb_dma_ops); > ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Wed, 06 Jun 2012 08:17:39 +1000 > On Tue, 2012-06-05 at 19:25 +0530, Bharat Bhushan wrote: >> memblock_end_of_DRAM() returns end_address + 1, not end address. >> While some code assumes that it returns end address. > > Shouldn't we instead fix it the other way around ? IE, make > memblock_end_of_DRAM() does what the name implies, which is to return > the last byte of DRAM, and fix the -other- callers not to make bad > assumptions ? That was my impression too when I saw this patch.
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Wednesday, June 06, 2012 3:51 AM > To: benh@kernel.crashing.org > Cc: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org; linux- > kernel@vger.kernel.org; galak@kernel.crashing.org; Bhushan Bharat-R65777 > Subject: Re: [PATCH] powerpc: Fix assmption of end_of_DRAM() returns end address > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Date: Wed, 06 Jun 2012 08:17:39 +1000 > > > On Tue, 2012-06-05 at 19:25 +0530, Bharat Bhushan wrote: > >> memblock_end_of_DRAM() returns end_address + 1, not end address. > >> While some code assumes that it returns end address. > > > > Shouldn't we instead fix it the other way around ? IE, make > > memblock_end_of_DRAM() does what the name implies, which is to return > > the last byte of DRAM, and fix the -other- callers not to make bad > > assumptions ? > > That was my impression too when I saw this patch. Initially I also intended to do so. I initiated a email on linux-mm@ subject "memblock_end_of_DRAM() return end address + 1" and the only response I received from Andrea was: " It's normal that "end" means "first byte offset out of the range". End = not ok. end = start+size. This is true for vm_end too. So it's better to keep it that way. My suggestion is to just fix point 1 below and audit the rest :) " Thanks -Bharat
On Wed, 2012-06-06 at 00:46 +0000, Bhushan Bharat-R65777 wrote: > > >> memblock_end_of_DRAM() returns end_address + 1, not end address. > > >> While some code assumes that it returns end address. > > > > > > Shouldn't we instead fix it the other way around ? IE, make > > > memblock_end_of_DRAM() does what the name implies, which is to > return > > > the last byte of DRAM, and fix the -other- callers not to make bad > > > assumptions ? > > > > That was my impression too when I saw this patch. > > Initially I also intended to do so. I initiated a email on linux-mm@ > subject "memblock_end_of_DRAM() return end address + 1" and the only > response I received from Andrea was: > > " > It's normal that "end" means "first byte offset out of the range". End > = not ok. > end = start+size. > This is true for vm_end too. So it's better to keep it that way. > My suggestion is to just fix point 1 below and audit the rest :) > " Oh well, I don't care enough to fight this battle in my current state so unless Dave has more stamina than I have today, I'm ok with the patch. Cheers, Ben.
Hi, On Wed, Jun 06, 2012 at 03:30:17PM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-06 at 00:46 +0000, Bhushan Bharat-R65777 wrote: > > > > >> memblock_end_of_DRAM() returns end_address + 1, not end address. > > > >> While some code assumes that it returns end address. > > > > > > > > Shouldn't we instead fix it the other way around ? IE, make > > > > memblock_end_of_DRAM() does what the name implies, which is to > > return > > > > the last byte of DRAM, and fix the -other- callers not to make bad > > > > assumptions ? > > > > > > That was my impression too when I saw this patch. > > > > Initially I also intended to do so. I initiated a email on linux-mm@ > > subject "memblock_end_of_DRAM() return end address + 1" and the only > > response I received from Andrea was: > > > > " > > It's normal that "end" means "first byte offset out of the range". End > > = not ok. > > end = start+size. > > This is true for vm_end too. So it's better to keep it that way. > > My suggestion is to just fix point 1 below and audit the rest :) > > " > > Oh well, I don't care enough to fight this battle in my current state so I wish you to get well soon Ben! > unless Dave has more stamina than I have today, I'm ok with the patch. Well it doesn't really matter in the end what is decided as long as something is decided :). I was asked through a forward so I only expressed my preference...
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Wed, 06 Jun 2012 15:30:17 +1000 > On Wed, 2012-06-06 at 00:46 +0000, Bhushan Bharat-R65777 wrote: > >> > >> memblock_end_of_DRAM() returns end_address + 1, not end address. >> > >> While some code assumes that it returns end address. >> > > >> > > Shouldn't we instead fix it the other way around ? IE, make >> > > memblock_end_of_DRAM() does what the name implies, which is to >> return >> > > the last byte of DRAM, and fix the -other- callers not to make bad >> > > assumptions ? >> > >> > That was my impression too when I saw this patch. >> >> Initially I also intended to do so. I initiated a email on linux-mm@ >> subject "memblock_end_of_DRAM() return end address + 1" and the only >> response I received from Andrea was: >> >> " >> It's normal that "end" means "first byte offset out of the range". End >> = not ok. >> end = start+size. >> This is true for vm_end too. So it's better to keep it that way. >> My suggestion is to just fix point 1 below and audit the rest :) >> " > > Oh well, I don't care enough to fight this battle in my current state so > unless Dave has more stamina than I have today, I'm ok with the patch. I'm definitely without the samina to fight something like this right now :)
diff --git a/arch/powerpc/platforms/44x/currituck.c b/arch/powerpc/platforms/44x/currituck.c index 583e67f..9f6c33d 100644 --- a/arch/powerpc/platforms/44x/currituck.c +++ b/arch/powerpc/platforms/44x/currituck.c @@ -160,7 +160,7 @@ static void __init ppc47x_setup_arch(void) /* No need to check the DMA config as we /know/ our windows are all of * RAM. Lets hope that doesn't change */ #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > 0xffffffff) { + if ((memblock_end_of_DRAM() - 1) > 0xffffffff) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c b/arch/powerpc/platforms/85xx/corenet_ds.c index dd3617c..925b028 100644 --- a/arch/powerpc/platforms/85xx/corenet_ds.c +++ b/arch/powerpc/platforms/85xx/corenet_ds.c @@ -77,7 +77,7 @@ void __init corenet_ds_setup_arch(void) #endif #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/85xx/ge_imp3a.c b/arch/powerpc/platforms/85xx/ge_imp3a.c index 1801462..b6a728b 100644 --- a/arch/powerpc/platforms/85xx/ge_imp3a.c +++ b/arch/powerpc/platforms/85xx/ge_imp3a.c @@ -125,7 +125,7 @@ static void __init ge_imp3a_setup_arch(void) mpc85xx_smp_init(); #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c b/arch/powerpc/platforms/85xx/mpc8536_ds.c index 585bd22..767c7cf 100644 --- a/arch/powerpc/platforms/85xx/mpc8536_ds.c +++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c @@ -75,7 +75,7 @@ static void __init mpc8536_ds_setup_arch(void) #endif #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c index 1fd91e9..d30f6c4 100644 --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c @@ -173,7 +173,7 @@ static void __init mpc85xx_ds_setup_arch(void) mpc85xx_smp_init(); #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c index d208ebc..8e4b094 100644 --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c @@ -359,7 +359,7 @@ static void __init mpc85xx_mds_setup_arch(void) mpc85xx_mds_qe_init(); #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c index f700c81..74e310b 100644 --- a/arch/powerpc/platforms/85xx/p1022_ds.c +++ b/arch/powerpc/platforms/85xx/p1022_ds.c @@ -450,7 +450,7 @@ static void __init p1022_ds_setup_arch(void) mpc85xx_smp_init(); #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb; diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c index 3755e61..817245b 100644 --- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c +++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c @@ -102,7 +102,7 @@ mpc86xx_hpcn_setup_arch(void) #endif #ifdef CONFIG_SWIOTLB - if (memblock_end_of_DRAM() > max) { + if ((memblock_end_of_DRAM() - 1) > max) { ppc_swiotlb_enable = 1; set_pci_dma_ops(&swiotlb_dma_ops); ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
memblock_end_of_DRAM() returns end_address + 1, not end address. While some code assumes that it returns end address. Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> --- This patch is based on next branch of https://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git arch/powerpc/platforms/44x/currituck.c | 2 +- arch/powerpc/platforms/85xx/corenet_ds.c | 2 +- arch/powerpc/platforms/85xx/ge_imp3a.c | 2 +- arch/powerpc/platforms/85xx/mpc8536_ds.c | 2 +- arch/powerpc/platforms/85xx/mpc85xx_ds.c | 2 +- arch/powerpc/platforms/85xx/mpc85xx_mds.c | 2 +- arch/powerpc/platforms/85xx/p1022_ds.c | 2 +- arch/powerpc/platforms/86xx/mpc86xx_hpcn.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-)