All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 11:23 ` Dan Carpenter
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-05 11:23 UTC (permalink / raw)
  To: Kalle Valo, Amitkumar Karwar
  Cc: Prameela Rani Garnepudi, Karun Eagalapati, Siva Rebbagondla,
	linux-wireless, kernel-janitors

The problem here is that we allocate "data".  Then we do
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
not the one we allocated.

I don't know if it causes an issue in real life, but it seems like a
reasonable thing to free the same pointer that we allocated.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..ca4e698ab69b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -652,11 +652,11 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw *adapter,
 static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 				    u32 *read_buf, u16 size)
 {
-	u32 addr_on_bus, *data;
+	u32 addr_on_bus, *data, *data_orig;
 	u16 ms_addr;
 	int status;
 
-	data = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+	data = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -709,7 +709,7 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 	}
 
 err:
-	kfree(data);
+	kfree(data_orig);
 	return status;
 }
 
@@ -717,10 +717,10 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 				     unsigned long addr,
 				     unsigned long data, u16 size)
 {
-	unsigned long *data_aligned;
+	unsigned long *data_aligned, *data_orig;
 	int status;
 
-	data_aligned = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+	data_aligned = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
 	if (!data_aligned)
 		return -ENOMEM;
 
@@ -743,7 +743,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 		rsi_dbg(ERR_ZONE,
 			"%s: Unable to set ms word to common reg\n",
 			__func__);
-		kfree(data_aligned);
+		kfree(data_orig);
 		return -EIO;
 	}
 	addr = addr & 0xFFFF;
@@ -757,7 +757,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 		rsi_dbg(ERR_ZONE,
 			"%s: Unable to do AHB reg write\n", __func__);
 
-	kfree(data_aligned);
+	kfree(data_orig);
 	return status;
 }
 

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 11:23 ` Dan Carpenter
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-05 11:23 UTC (permalink / raw)
  To: Kalle Valo, Amitkumar Karwar
  Cc: Prameela Rani Garnepudi, Karun Eagalapati, Siva Rebbagondla,
	linux-wireless, kernel-janitors

The problem here is that we allocate "data".  Then we do
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
not the one we allocated.

I don't know if it causes an issue in real life, but it seems like a
reasonable thing to free the same pointer that we allocated.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..ca4e698ab69b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -652,11 +652,11 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw *adapter,
 static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 				    u32 *read_buf, u16 size)
 {
-	u32 addr_on_bus, *data;
+	u32 addr_on_bus, *data, *data_orig;
 	u16 ms_addr;
 	int status;
 
-	data = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+	data = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -709,7 +709,7 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 	}
 
 err:
-	kfree(data);
+	kfree(data_orig);
 	return status;
 }
 
@@ -717,10 +717,10 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 				     unsigned long addr,
 				     unsigned long data, u16 size)
 {
-	unsigned long *data_aligned;
+	unsigned long *data_aligned, *data_orig;
 	int status;
 
-	data_aligned = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+	data_aligned = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
 	if (!data_aligned)
 		return -ENOMEM;
 
@@ -743,7 +743,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 		rsi_dbg(ERR_ZONE,
 			"%s: Unable to set ms word to common reg\n",
 			__func__);
-		kfree(data_aligned);
+		kfree(data_orig);
 		return -EIO;
 	}
 	addr = addr & 0xFFFF;
@@ -757,7 +757,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 		rsi_dbg(ERR_ZONE,
 			"%s: Unable to do AHB reg write\n", __func__);
 
-	kfree(data_aligned);
+	kfree(data_orig);
 	return status;
 }
 

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
  2018-04-05 11:23 ` Dan Carpenter
@ 2018-04-05 11:30   ` Johannes Berg
  -1 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-04-05 11:30 UTC (permalink / raw)
  To: Dan Carpenter, Kalle Valo, Amitkumar Karwar
  Cc: Prameela Rani Garnepudi, Karun Eagalapati, Siva Rebbagondla,
	linux-wireless, kernel-janitors

On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> The problem here is that we allocate "data".  Then we do
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> not the one we allocated.

That seems pretty pointless, since kmalloc guarantees such alignment for
sure. Better to just remove PTR_ALIGN()?

johannes

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 11:30   ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-04-05 11:30 UTC (permalink / raw)
  To: Dan Carpenter, Kalle Valo, Amitkumar Karwar
  Cc: Prameela Rani Garnepudi, Karun Eagalapati, Siva Rebbagondla,
	linux-wireless, kernel-janitors

On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> The problem here is that we allocate "data".  Then we do
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> not the one we allocated.

That seems pretty pointless, since kmalloc guarantees such alignment for
sure. Better to just remove PTR_ALIGN()?

johannes

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
  2018-04-05 11:30   ` Johannes Berg
@ 2018-04-05 11:39     ` Dan Carpenter
  -1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-05 11:39 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > The problem here is that we allocate "data".  Then we do
> > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > not the one we allocated.
> 
> That seems pretty pointless, since kmalloc guarantees such alignment for
> sure. Better to just remove PTR_ALIGN()?

Yeah.  You're probably right.  I was thinking that maybe
ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
think it's always 8 or more.

Let me resend with the ALIGN() removed.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 11:39     ` Dan Carpenter
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-05 11:39 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > The problem here is that we allocate "data".  Then we do
> > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > not the one we allocated.
> 
> That seems pretty pointless, since kmalloc guarantees such alignment for
> sure. Better to just remove PTR_ALIGN()?

Yeah.  You're probably right.  I was thinking that maybe
ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
think it's always 8 or more.

Let me resend with the ALIGN() removed.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
  2018-04-05 11:39     ` Dan Carpenter
@ 2018-04-05 11:41       ` Dan Carpenter
  -1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-05 11:41 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
> On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > > The problem here is that we allocate "data".  Then we do
> > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > > not the one we allocated.
> > 
> > That seems pretty pointless, since kmalloc guarantees such alignment for
> > sure. Better to just remove PTR_ALIGN()?
> 
> Yeah.  You're probably right.  I was thinking that maybe
> ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
> think it's always 8 or more.
> 

Perhaps on certain xtensa variants?

arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH                4       /* data width in bytes */

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 11:41       ` Dan Carpenter
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-05 11:41 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
> On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > > The problem here is that we allocate "data".  Then we do
> > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > > not the one we allocated.
> > 
> > That seems pretty pointless, since kmalloc guarantees such alignment for
> > sure. Better to just remove PTR_ALIGN()?
> 
> Yeah.  You're probably right.  I was thinking that maybe
> ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
> think it's always 8 or more.
> 

Perhaps on certain xtensa variants?

arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH                4       /* data width in bytes */

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
  2018-04-05 11:41       ` Dan Carpenter
@ 2018-04-05 11:46         ` Johannes Berg
  -1 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-04-05 11:46 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

On Thu, 2018-04-05 at 14:41 +0300, Dan Carpenter wrote:
> On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
> > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > > > The problem here is that we allocate "data".  Then we do
> > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > > > not the one we allocated.
> > > 
> > > That seems pretty pointless, since kmalloc guarantees such alignment for
> > > sure. Better to just remove PTR_ALIGN()?
> > 
> > Yeah.  You're probably right.  I was thinking that maybe
> > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
> > think it's always 8 or more.
> > 
> 
> Perhaps on certain xtensa variants?
> 
> arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
> arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH                4       /* data width in bytes */

That's ... interesting. The comment on the original of this says it's
supposed to be used for "better alignment" (more zero bits), and I'd
think that there's lots of code making such assumptions...

I'd argue it's an xtensa bug, if we need to deal with this everywhere
then it might get messy. Mostly we don't have to care, since pointer
alignment is sufficient in many cases, but still...

Hmm. Dunno what to do here then.

johannes

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 11:46         ` Johannes Berg
  0 siblings, 0 replies; 20+ messages in thread
From: Johannes Berg @ 2018-04-05 11:46 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Kalle Valo, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

On Thu, 2018-04-05 at 14:41 +0300, Dan Carpenter wrote:
> On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
> > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > > > The problem here is that we allocate "data".  Then we do
> > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > > > not the one we allocated.
> > > 
> > > That seems pretty pointless, since kmalloc guarantees such alignment for
> > > sure. Better to just remove PTR_ALIGN()?
> > 
> > Yeah.  You're probably right.  I was thinking that maybe
> > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
> > think it's always 8 or more.
> > 
> 
> Perhaps on certain xtensa variants?
> 
> arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
> arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH                4       /* data width in bytes */

That's ... interesting. The comment on the original of this says it's
supposed to be used for "better alignment" (more zero bits), and I'd
think that there's lots of code making such assumptions...

I'd argue it's an xtensa bug, if we need to deal with this everywhere
then it might get messy. Mostly we don't have to care, since pointer
alignment is sufficient in many cases, but still...

Hmm. Dunno what to do here then.

johannes

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
  2018-04-05 11:46         ` Johannes Berg
@ 2018-04-05 12:23           ` Kalle Valo
  -1 siblings, 0 replies; 20+ messages in thread
From: Kalle Valo @ 2018-04-05 12:23 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Dan Carpenter, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2018-04-05 at 14:41 +0300, Dan Carpenter wrote:
>> On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
>> > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
>> > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
>> > > > The problem here is that we allocate "data".  Then we do
>> > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
>> > > > not the one we allocated.
>> > > 
>> > > That seems pretty pointless, since kmalloc guarantees such alignment for
>> > > sure. Better to just remove PTR_ALIGN()?
>> > 
>> > Yeah.  You're probably right.  I was thinking that maybe
>> > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
>> > think it's always 8 or more.
>> > 
>> 
>> Perhaps on certain xtensa variants?
>> 
>> arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
>> arch/xtensa/variants/fsf/include/variant/core.h:#define
>> XCHAL_DATA_WIDTH 4 /* data width in bytes */
>
> That's ... interesting. The comment on the original of this says it's
> supposed to be used for "better alignment" (more zero bits), and I'd
> think that there's lots of code making such assumptions...
>
> I'd argue it's an xtensa bug, if we need to deal with this everywhere
> then it might get messy. Mostly we don't have to care, since pointer
> alignment is sufficient in many cases, but still...
>
> Hmm. Dunno what to do here then.

IMHO let's just get rid of the ugly PTR_ALIGN(), I strongly doubt it was
added because of this xtensa "feature" :) If we ever get a bug report
about this we can then talk with the xtensa folks.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] rsi: Free the unaligned pointer
@ 2018-04-05 12:23           ` Kalle Valo
  0 siblings, 0 replies; 20+ messages in thread
From: Kalle Valo @ 2018-04-05 12:23 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Dan Carpenter, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2018-04-05 at 14:41 +0300, Dan Carpenter wrote:
>> On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
>> > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
>> > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
>> > > > The problem here is that we allocate "data".  Then we do
>> > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
>> > > > not the one we allocated.
>> > > 
>> > > That seems pretty pointless, since kmalloc guarantees such alignment for
>> > > sure. Better to just remove PTR_ALIGN()?
>> > 
>> > Yeah.  You're probably right.  I was thinking that maybe
>> > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
>> > think it's always 8 or more.
>> > 
>> 
>> Perhaps on certain xtensa variants?
>> 
>> arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
>> arch/xtensa/variants/fsf/include/variant/core.h:#define
>> XCHAL_DATA_WIDTH 4 /* data width in bytes */
>
> That's ... interesting. The comment on the original of this says it's
> supposed to be used for "better alignment" (more zero bits), and I'd
> think that there's lots of code making such assumptions...
>
> I'd argue it's an xtensa bug, if we need to deal with this everywhere
> then it might get messy. Mostly we don't have to care, since pointer
> alignment is sufficient in many cases, but still...
>
> Hmm. Dunno what to do here then.

IMHO let's just get rid of the ugly PTR_ALIGN(), I strongly doubt it was
added because of this xtensa "feature" :) If we ever get a bug report
about this we can then talk with the xtensa folks.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2] rsi: remove unecessary PTR_ALIGN()s
  2018-04-05 12:23           ` Kalle Valo
@ 2018-04-06  8:37             ` Dan Carpenter
  -1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-06  8:37 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, Prameela Rani Garnepudi, Karun Eagalapati,
	Siva Rebbagondla, linux-wireless, kernel-janitors

The issue here is that we allocate "data" and then set
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
instead of the original pointer.

kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
more on everything except certain Xtensa variants.  We decided that if
the Xtensa people ever notice a bug here then we'll tell them the bug is
on their side.  ;)

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Instead of saving the original pointer, just remove the ALIGN()s

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..8ef00582c6ea 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -660,8 +660,6 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 	if (!data)
 		return -ENOMEM;
 
-	data = PTR_ALIGN(data, 8);
-
 	ms_addr = (addr >> 16);
 	status = rsi_sdio_master_access_msword(adapter, ms_addr);
 	if (status < 0) {
@@ -724,8 +722,6 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 	if (!data_aligned)
 		return -ENOMEM;
 
-	data_aligned = PTR_ALIGN(data_aligned, 8);
-
 	if (size == 2) {
 		*data_aligned = ((data << 16) | (data & 0xFFFF));
 	} else if (size == 1) {

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2] rsi: remove unecessary PTR_ALIGN()s
@ 2018-04-06  8:37             ` Dan Carpenter
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-04-06  8:37 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, Prameela Rani Garnepudi, Karun Eagalapati,
	Siva Rebbagondla, linux-wireless, kernel-janitors

The issue here is that we allocate "data" and then set
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
instead of the original pointer.

kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
more on everything except certain Xtensa variants.  We decided that if
the Xtensa people ever notice a bug here then we'll tell them the bug is
on their side.  ;)

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Instead of saving the original pointer, just remove the ALIGN()s

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..8ef00582c6ea 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -660,8 +660,6 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 	if (!data)
 		return -ENOMEM;
 
-	data = PTR_ALIGN(data, 8);
-
 	ms_addr = (addr >> 16);
 	status = rsi_sdio_master_access_msword(adapter, ms_addr);
 	if (status < 0) {
@@ -724,8 +722,6 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 	if (!data_aligned)
 		return -ENOMEM;
 
-	data_aligned = PTR_ALIGN(data_aligned, 8);
-
 	if (size = 2) {
 		*data_aligned = ((data << 16) | (data & 0xFFFF));
 	} else if (size = 1) {

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] rsi: remove unecessary PTR_ALIGN()s
  2018-04-06  8:37             ` Dan Carpenter
@ 2018-04-06  8:45               ` Arend van Spriel
  -1 siblings, 0 replies; 20+ messages in thread
From: Arend van Spriel @ 2018-04-06  8:45 UTC (permalink / raw)
  To: Dan Carpenter, Kalle Valo
  Cc: Amitkumar Karwar, Prameela Rani Garnepudi, Karun Eagalapati,
	Siva Rebbagondla, linux-wireless, kernel-janitors

On 4/6/2018 10:37 AM, Dan Carpenter wrote:
> The issue here is that we allocate "data" and then set
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
> instead of the original pointer.
>
> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
> more on everything except certain Xtensa variants.  We decided that if
> the Xtensa people ever notice a bug here then we'll tell them the bug is
> on their side.  ;)

I am not sure if it was decided to be a xtensa bug, but just to ignore 
the issue until it would arise. Anyway, not sure if the last sentence is 
useful in the commit message.

Regards,
Arend

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] rsi: remove unecessary PTR_ALIGN()s
@ 2018-04-06  8:45               ` Arend van Spriel
  0 siblings, 0 replies; 20+ messages in thread
From: Arend van Spriel @ 2018-04-06  8:45 UTC (permalink / raw)
  To: Dan Carpenter, Kalle Valo
  Cc: Amitkumar Karwar, Prameela Rani Garnepudi, Karun Eagalapati,
	Siva Rebbagondla, linux-wireless, kernel-janitors

On 4/6/2018 10:37 AM, Dan Carpenter wrote:
> The issue here is that we allocate "data" and then set
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
> instead of the original pointer.
>
> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
> more on everything except certain Xtensa variants.  We decided that if
> the Xtensa people ever notice a bug here then we'll tell them the bug is
> on their side.  ;)

I am not sure if it was decided to be a xtensa bug, but just to ignore 
the issue until it would arise. Anyway, not sure if the last sentence is 
useful in the commit message.

Regards,
Arend


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] rsi: remove unecessary PTR_ALIGN()s
  2018-04-06  8:45               ` Arend van Spriel
@ 2018-04-06  9:01                 ` Kalle Valo
  -1 siblings, 0 replies; 20+ messages in thread
From: Kalle Valo @ 2018-04-06  9:01 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: Dan Carpenter, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 4/6/2018 10:37 AM, Dan Carpenter wrote:
>> The issue here is that we allocate "data" and then set
>> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
>> instead of the original pointer.
>>
>> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
>> more on everything except certain Xtensa variants.  We decided that if
>> the Xtensa people ever notice a bug here then we'll tell them the bug is
>> on their side.  ;)
>
> I am not sure if it was decided to be a xtensa bug, but just to ignore
> the issue until it would arise.

IIRC on other architectures the allocation is already aligned properly
so there shouldn't be any functional changes with this patch, expect on
xtensa of course.

> Anyway, not sure if the last sentence is useful in the commit message.

IMHO it is useful as it gives a summary of our discussion, just with a
humorous tone :)

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] rsi: remove unecessary PTR_ALIGN()s
@ 2018-04-06  9:01                 ` Kalle Valo
  0 siblings, 0 replies; 20+ messages in thread
From: Kalle Valo @ 2018-04-06  9:01 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: Dan Carpenter, Amitkumar Karwar, Prameela Rani Garnepudi,
	Karun Eagalapati, Siva Rebbagondla, linux-wireless,
	kernel-janitors

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 4/6/2018 10:37 AM, Dan Carpenter wrote:
>> The issue here is that we allocate "data" and then set
>> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
>> instead of the original pointer.
>>
>> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
>> more on everything except certain Xtensa variants.  We decided that if
>> the Xtensa people ever notice a bug here then we'll tell them the bug is
>> on their side.  ;)
>
> I am not sure if it was decided to be a xtensa bug, but just to ignore
> the issue until it would arise.

IIRC on other architectures the allocation is already aligned properly
so there shouldn't be any functional changes with this patch, expect on
xtensa of course.

> Anyway, not sure if the last sentence is useful in the commit message.

IMHO it is useful as it gives a summary of our discussion, just with a
humorous tone :)

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v2] rsi: remove unecessary PTR_ALIGN()s
  2018-04-06  8:37             ` Dan Carpenter
@ 2018-04-24 17:24               ` Kalle Valo
  -1 siblings, 0 replies; 20+ messages in thread
From: Kalle Valo @ 2018-04-24 17:24 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Amitkumar Karwar, Prameela Rani Garnepudi, Karun Eagalapati,
	Siva Rebbagondla, linux-wireless, kernel-janitors

Dan Carpenter <dan.carpenter@oracle.com> wrote:

> The issue here is that we allocate "data" and then set
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
> instead of the original pointer.
> 
> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
> more on everything except certain Xtensa variants.  We decided that if
> the Xtensa people ever notice a bug here then we'll tell them the bug is
> on their side.  ;)
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch applied to wireless-drivers-next.git, thanks.

350fcdb83457 rsi: remove unecessary PTR_ALIGN()s

-- 
https://patchwork.kernel.org/patch/10325781/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v2] rsi: remove unecessary PTR_ALIGN()s
@ 2018-04-24 17:24               ` Kalle Valo
  0 siblings, 0 replies; 20+ messages in thread
From: Kalle Valo @ 2018-04-24 17:24 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Amitkumar Karwar, Prameela Rani Garnepudi, Karun Eagalapati,
	Siva Rebbagondla, linux-wireless, kernel-janitors

Dan Carpenter <dan.carpenter@oracle.com> wrote:

> The issue here is that we allocate "data" and then set
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
> instead of the original pointer.
> 
> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
> more on everything except certain Xtensa variants.  We decided that if
> the Xtensa people ever notice a bug here then we'll tell them the bug is
> on their side.  ;)
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch applied to wireless-drivers-next.git, thanks.

350fcdb83457 rsi: remove unecessary PTR_ALIGN()s

-- 
https://patchwork.kernel.org/patch/10325781/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-04-24 17:24 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-05 11:23 [PATCH] rsi: Free the unaligned pointer Dan Carpenter
2018-04-05 11:23 ` Dan Carpenter
2018-04-05 11:30 ` Johannes Berg
2018-04-05 11:30   ` Johannes Berg
2018-04-05 11:39   ` Dan Carpenter
2018-04-05 11:39     ` Dan Carpenter
2018-04-05 11:41     ` Dan Carpenter
2018-04-05 11:41       ` Dan Carpenter
2018-04-05 11:46       ` Johannes Berg
2018-04-05 11:46         ` Johannes Berg
2018-04-05 12:23         ` Kalle Valo
2018-04-05 12:23           ` Kalle Valo
2018-04-06  8:37           ` [PATCH v2] rsi: remove unecessary PTR_ALIGN()s Dan Carpenter
2018-04-06  8:37             ` Dan Carpenter
2018-04-06  8:45             ` Arend van Spriel
2018-04-06  8:45               ` Arend van Spriel
2018-04-06  9:01               ` Kalle Valo
2018-04-06  9:01                 ` Kalle Valo
2018-04-24 17:24             ` [v2] " Kalle Valo
2018-04-24 17:24               ` Kalle Valo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.