| Message ID | 20180405112311.GD4218@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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
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
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
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.
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; }
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>