[PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[Wey-Yi Guy]

From: Johannes Berg <johannes.berg@intel.com>

The ICT code erroneously uses PAGE_SIZE. The bug
is that PAGE_SIZE isn't necessarily 4096, so on
such platforms this code will not work correctly
as we'll try to attempt to read an index in the
table that the device never wrote, it always has
4096-byte pages.

Additionally, the manual alignment code here is
unnecessary -- Documentation/DMA-API-HOWTO.txt
states:
  The cpu return address and the DMA bus master address are both
  guaranteed to be aligned to the smallest PAGE_SIZE order which
  is greater than or equal to the requested size.  This invariant
  exists (for example) to guarantee that if you allocate a chunk
  which is smaller than or equal to 64 kilobytes, the extent of the
  buffer you receive will not cross a 64K boundary.

Just use appropriate new constants and get rid of
the alignment code.

Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
---
this patch will be also available from wireless branch on
 git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi.git

 drivers/net/wireless/iwlwifi/iwl-trans-pcie-int.h |    2 -
 drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c  |   73 +++++++++------------
 2 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-int.h b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-int.h
index 2b6756e..5c29281 100644
--- a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-int.h
+++ b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-int.h
@@ -219,9 +219,7 @@ struct iwl_trans_pcie {
 
 	/* INT ICT Table */
 	__le32 *ict_tbl;
-	void *ict_tbl_vir;
 	dma_addr_t ict_tbl_dma;
-	dma_addr_t aligned_ict_tbl_dma;
 	int ict_index;
 	u32 inta;
 	bool use_ict;
diff --git a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
index 374c68c..1920237 100644
--- a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
@@ -1136,7 +1136,11 @@ void iwl_irq_tasklet(struct iwl_trans *trans)
  * ICT functions
  *
  ******************************************************************************/
-#define ICT_COUNT (PAGE_SIZE/sizeof(u32))
+
+/* a device (PCI-E) page is 4096 bytes long */
+#define ICT_SHIFT	12
+#define ICT_SIZE	(1 << ICT_SHIFT)
+#define ICT_COUNT	(ICT_SIZE / sizeof(u32))
 
 /* Free dram table */
 void iwl_free_isr_ict(struct iwl_trans *trans)
@@ -1144,21 +1148,19 @@ void iwl_free_isr_ict(struct iwl_trans *trans)
 	struct iwl_trans_pcie *trans_pcie =
 		IWL_TRANS_GET_PCIE_TRANS(trans);
 
-	if (trans_pcie->ict_tbl_vir) {
-		dma_free_coherent(bus(trans)->dev,
-				  (sizeof(u32) * ICT_COUNT) + PAGE_SIZE,
-				  trans_pcie->ict_tbl_vir,
+	if (trans_pcie->ict_tbl) {
+		dma_free_coherent(bus(trans)->dev, ICT_SIZE,
+				  trans_pcie->ict_tbl,
 				  trans_pcie->ict_tbl_dma);
-		trans_pcie->ict_tbl_vir = NULL;
-		memset(&trans_pcie->ict_tbl_dma, 0,
-			sizeof(trans_pcie->ict_tbl_dma));
-		memset(&trans_pcie->aligned_ict_tbl_dma, 0,
-			sizeof(trans_pcie->aligned_ict_tbl_dma));
+		trans_pcie->ict_tbl = NULL;
+		trans_pcie->ict_tbl_dma = 0;
 	}
 }
 
 
-/* allocate dram shared table it is a PAGE_SIZE aligned
+/*
+ * allocate dram shared table, it is an aligned memory
+ * block of ICT_SIZE.
  * also reset all data related to ICT table interrupt.
  */
 int iwl_alloc_isr_ict(struct iwl_trans *trans)
@@ -1166,36 +1168,26 @@ int iwl_alloc_isr_ict(struct iwl_trans *trans)
 	struct iwl_trans_pcie *trans_pcie =
 		IWL_TRANS_GET_PCIE_TRANS(trans);
 
-	/* allocate shrared data table */
-	trans_pcie->ict_tbl_vir =
-		dma_alloc_coherent(bus(trans)->dev,
-				   (sizeof(u32) * ICT_COUNT) + PAGE_SIZE,
-				   &trans_pcie->ict_tbl_dma, GFP_KERNEL);
-	if (!trans_pcie->ict_tbl_vir)
+	trans_pcie->ict_tbl =
+		dma_alloc_coherent(bus(trans)->dev, ICT_SIZE,
+				   &trans_pcie->ict_tbl_dma,
+				   GFP_KERNEL);
+	if (!trans_pcie->ict_tbl)
 		return -ENOMEM;
 
-	/* align table to PAGE_SIZE boundary */
-	trans_pcie->aligned_ict_tbl_dma =
-		ALIGN(trans_pcie->ict_tbl_dma, PAGE_SIZE);
-
-	IWL_DEBUG_ISR(trans, "ict dma addr %Lx dma aligned %Lx diff %d\n",
-			   (unsigned long long)trans_pcie->ict_tbl_dma,
-			   (unsigned long long)trans_pcie->aligned_ict_tbl_dma,
-			   (int)(trans_pcie->aligned_ict_tbl_dma -
-			   trans_pcie->ict_tbl_dma));
+	/* just an API sanity check ... it is guaranteed to be aligned */
+	if (WARN_ON(trans_pcie->ict_tbl_dma & (ICT_SIZE - 1))) {
+		iwl_free_isr_ict(trans);
+		return -EINVAL;
+	}
 
-	trans_pcie->ict_tbl =  trans_pcie->ict_tbl_vir +
-			  (trans_pcie->aligned_ict_tbl_dma -
-			  trans_pcie->ict_tbl_dma);
+	IWL_DEBUG_ISR(trans, "ict dma addr %Lx\n",
+		      (unsigned long long)trans_pcie->ict_tbl_dma);
 
-	IWL_DEBUG_ISR(trans, "ict vir addr %p vir aligned %p diff %d\n",
-			     trans_pcie->ict_tbl, trans_pcie->ict_tbl_vir,
-			(int)(trans_pcie->aligned_ict_tbl_dma -
-			    trans_pcie->ict_tbl_dma));
+	IWL_DEBUG_ISR(trans, "ict vir addr %p\n", trans_pcie->ict_tbl);
 
 	/* reset table and index to all 0 */
-	memset(trans_pcie->ict_tbl_vir, 0,
-		(sizeof(u32) * ICT_COUNT) + PAGE_SIZE);
+	memset(trans_pcie->ict_tbl, 0, ICT_SIZE);
 	trans_pcie->ict_index = 0;
 
 	/* add periodic RX interrupt */
@@ -1213,23 +1205,20 @@ int iwl_reset_ict(struct iwl_trans *trans)
 	struct iwl_trans_pcie *trans_pcie =
 		IWL_TRANS_GET_PCIE_TRANS(trans);
 
-	if (!trans_pcie->ict_tbl_vir)
+	if (!trans_pcie->ict_tbl)
 		return 0;
 
 	spin_lock_irqsave(&trans->shrd->lock, flags);
 	iwl_disable_interrupts(trans);
 
-	memset(&trans_pcie->ict_tbl[0], 0, sizeof(u32) * ICT_COUNT);
+	memset(trans_pcie->ict_tbl, 0, ICT_SIZE);
 
-	val = trans_pcie->aligned_ict_tbl_dma >> PAGE_SHIFT;
+	val = trans_pcie->ict_tbl_dma >> ICT_SHIFT;
 
 	val |= CSR_DRAM_INT_TBL_ENABLE;
 	val |= CSR_DRAM_INIT_TBL_WRAP_CHECK;
 
-	IWL_DEBUG_ISR(trans, "CSR_DRAM_INT_TBL_REG =0x%X "
-			"aligned dma address %Lx\n",
-			val,
-			(unsigned long long)trans_pcie->aligned_ict_tbl_dma);
+	IWL_DEBUG_ISR(trans, "CSR_DRAM_INT_TBL_REG =0x%x\n", val);
 
 	iwl_write32(bus(trans), CSR_DRAM_INT_TBL_REG, val);
 	trans_pcie->use_ict = true;
-- 
1.7.0.4

Re: [PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[John W. Linville]

On Mon, Dec 19, 2011 at 02:00:59PM -0800, Wey-Yi Guy wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> The ICT code erroneously uses PAGE_SIZE. The bug
> is that PAGE_SIZE isn't necessarily 4096, so on
> such platforms this code will not work correctly
> as we'll try to attempt to read an index in the
> table that the device never wrote, it always has
> 4096-byte pages.
> 
> Additionally, the manual alignment code here is
> unnecessary -- Documentation/DMA-API-HOWTO.txt
> states:
>   The cpu return address and the DMA bus master address are both
>   guaranteed to be aligned to the smallest PAGE_SIZE order which
>   is greater than or equal to the requested size.  This invariant
>   exists (for example) to guarantee that if you allocate a chunk
>   which is smaller than or equal to 64 kilobytes, the extent of the
>   buffer you receive will not cross a 64K boundary.
> 
> Just use appropriate new constants and get rid of
> the alignment code.
> 
> Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>

This seems like a big patch.  It is a bit late in the release cycle
to rewrite all the DMA allocation code. :-)

What platforms does this affect?  Are there actual users involved?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[Guy, Wey-Yi]

Hi John,

On Wed, 2011-12-21 at 08:19 -0800, John W. Linville wrote:
> On Mon, Dec 19, 2011 at 02:00:59PM -0800, Wey-Yi Guy wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > The ICT code erroneously uses PAGE_SIZE. The bug
> > is that PAGE_SIZE isn't necessarily 4096, so on
> > such platforms this code will not work correctly
> > as we'll try to attempt to read an index in the
> > table that the device never wrote, it always has
> > 4096-byte pages.
> > 
> > Additionally, the manual alignment code here is
> > unnecessary -- Documentation/DMA-API-HOWTO.txt
> > states:
> >   The cpu return address and the DMA bus master address are both
> >   guaranteed to be aligned to the smallest PAGE_SIZE order which
> >   is greater than or equal to the requested size.  This invariant
> >   exists (for example) to guarantee that if you allocate a chunk
> >   which is smaller than or equal to 64 kilobytes, the extent of the
> >   buffer you receive will not cross a 64K boundary.
> > 
> > Just use appropriate new constants and get rid of
> > the alignment code.
> > 
> > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> 
> This seems like a big patch.  It is a bit late in the release cycle
> to rewrite all the DMA allocation code. :-)
> 
> What platforms does this affect?  Are there actual users involved?

I understand the patch is bigger than the regular patch should be
generated for stable kernel. Johannes can explain better, but he is on
vacation. as what I understood, he work with community user who
encounter issue and root cause it is PAGE SIZE cause the problem.

Thanks
Wey

Re: [PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[John W. Linville]

On Wed, Dec 21, 2011 at 07:45:00AM -0800, Guy, Wey-Yi wrote:
> Hi John,
> 
> On Wed, 2011-12-21 at 08:19 -0800, John W. Linville wrote:
> > On Mon, Dec 19, 2011 at 02:00:59PM -0800, Wey-Yi Guy wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > The ICT code erroneously uses PAGE_SIZE. The bug
> > > is that PAGE_SIZE isn't necessarily 4096, so on
> > > such platforms this code will not work correctly
> > > as we'll try to attempt to read an index in the
> > > table that the device never wrote, it always has
> > > 4096-byte pages.
> > > 
> > > Additionally, the manual alignment code here is
> > > unnecessary -- Documentation/DMA-API-HOWTO.txt
> > > states:
> > >   The cpu return address and the DMA bus master address are both
> > >   guaranteed to be aligned to the smallest PAGE_SIZE order which
> > >   is greater than or equal to the requested size.  This invariant
> > >   exists (for example) to guarantee that if you allocate a chunk
> > >   which is smaller than or equal to 64 kilobytes, the extent of the
> > >   buffer you receive will not cross a 64K boundary.
> > > 
> > > Just use appropriate new constants and get rid of
> > > the alignment code.
> > > 
> > > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> > 
> > This seems like a big patch.  It is a bit late in the release cycle
> > to rewrite all the DMA allocation code. :-)
> > 
> > What platforms does this affect?  Are there actual users involved?
> 
> I understand the patch is bigger than the regular patch should be
> generated for stable kernel. Johannes can explain better, but he is on
> vacation. as what I understood, he work with community user who
> encounter issue and root cause it is PAGE SIZE cause the problem.
> 
> Thanks
> Wey

Unless someone screams, I'd be a lot more comfortable saving this
for 3.3.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[wwguy]

Hi John,


On Wed, 2011-12-21 at 11:58 -0800, John W. Linville wrote:
> On Wed, Dec 21, 2011 at 07:45:00AM -0800, Guy, Wey-Yi wrote:
> > Hi John,
> > 
> > On Wed, 2011-12-21 at 08:19 -0800, John W. Linville wrote:
> > > On Mon, Dec 19, 2011 at 02:00:59PM -0800, Wey-Yi Guy wrote:
> > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > 
> > > > The ICT code erroneously uses PAGE_SIZE. The bug
> > > > is that PAGE_SIZE isn't necessarily 4096, so on
> > > > such platforms this code will not work correctly
> > > > as we'll try to attempt to read an index in the
> > > > table that the device never wrote, it always has
> > > > 4096-byte pages.
> > > > 
> > > > Additionally, the manual alignment code here is
> > > > unnecessary -- Documentation/DMA-API-HOWTO.txt
> > > > states:
> > > >   The cpu return address and the DMA bus master address are both
> > > >   guaranteed to be aligned to the smallest PAGE_SIZE order which
> > > >   is greater than or equal to the requested size.  This invariant
> > > >   exists (for example) to guarantee that if you allocate a chunk
> > > >   which is smaller than or equal to 64 kilobytes, the extent of the
> > > >   buffer you receive will not cross a 64K boundary.
> > > > 
> > > > Just use appropriate new constants and get rid of
> > > > the alignment code.
> > > > 
> > > > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > > Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> > > 
> > > This seems like a big patch.  It is a bit late in the release cycle
> > > to rewrite all the DMA allocation code. :-)
> > > 
> > > What platforms does this affect?  Are there actual users involved?
> > 
> > I understand the patch is bigger than the regular patch should be
> > generated for stable kernel. Johannes can explain better, but he is on
> > vacation. as what I understood, he work with community user who
> > encounter issue and root cause it is PAGE SIZE cause the problem.
> > 
> > Thanks
> > Wey
> 
> Unless someone screams, I'd be a lot more comfortable saving this
> for 3.3.
unless Johannes has objection, I am ok for 3.3, it is not something
people will encounter everyday on common platform..

Thanks
Wey

Re: [PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[Emmanuel Grumbach]

>> > > > From: Johannes Berg <johannes.berg@intel.com>
>> > > >
>> > > > The ICT code erroneously uses PAGE_SIZE. The bug
>> > > > is that PAGE_SIZE isn't necessarily 4096, so on
>> > > > such platforms this code will not work correctly
>> > > > as we'll try to attempt to read an index in the
>> > > > table that the device never wrote, it always has
>> > > > 4096-byte pages.
>> > > >
>> > > > Additionally, the manual alignment code here is
>> > > > unnecessary -- Documentation/DMA-API-HOWTO.txt
>> > > > states:
>> > > >   The cpu return address and the DMA bus master address are both
>> > > >   guaranteed to be aligned to the smallest PAGE_SIZE order which
>> > > >   is greater than or equal to the requested size.  This invariant
>> > > >   exists (for example) to guarantee that if you allocate a chunk
>> > > >   which is smaller than or equal to 64 kilobytes, the extent of the
>> > > >   buffer you receive will not cross a 64K boundary.
>> > > >
>> > > > Just use appropriate new constants and get rid of
>> > > > the alignment code.
>> > > >
>> > > > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
>> > > > Cc: stable@vger.kernel.org
>> > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>> > > > Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
>> > >
>> > > This seems like a big patch.  It is a bit late in the release cycle
>> > > to rewrite all the DMA allocation code. :-)
>> > >
>> > > What platforms does this affect?  Are there actual users involved?
>> >
>> > I understand the patch is bigger than the regular patch should be
>> > generated for stable kernel. Johannes can explain better, but he is on
>> > vacation. as what I understood, he work with community user who
>> > encounter issue and root cause it is PAGE SIZE cause the problem.
>> >
>> > Thanks
>> > Wey
>>
>> Unless someone screams, I'd be a lot more comfortable saving this
>> for 3.3.
> unless Johannes has objection, I am ok for 3.3, it is not something
> people will encounter everyday on common platform..
>

Please note that this patch has two parts: one does the real stuff
(very small), and the other removes unneeded DAM alignment code (big
part).
I guess it can wait to 3.3 and the small part will then be posted to stable.

RE: [PATCH 3.2] iwlagn: fix (remove) use of PAGE_SIZE

[Berg, Johannes]

> >> Unless someone screams, I'd be a lot more comfortable saving this for
> >> 3.3.
> > unless Johannes has objection, I am ok for 3.3, it is not something
> > people will encounter everyday on common platform..
> >
> 
> Please note that this patch has two parts: one does the real stuff (very small),
> and the other removes unneeded DAM alignment code (big part).
> I guess it can wait to 3.3 and the small part will then be posted to stable.

Sounds like a good plan, I might need a reminder next year though :-)

FWIW, I'm not aware of anyone having run into this issue since most platforms do indeed use 4k pages.

johannes
--------------------------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland 
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052