[PATCH v2] gpu: host1x: Fix compiler errors by converting to dma_addr_t
The compiler is complaining with the following errors:
drivers/gpu/host1x/cdma.c:94:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
drivers/gpu/host1x/cdma.c:113:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
The expected pointer type of the third argument to dma_alloc_wc() is
dma_addr_t but phys_addr_t is passed.
Change the phys member of struct push_buffer to be dma_addr_t so that we
pass the correct type to dma_alloc_wc().
Also check pb->mapped for non-NULL in the destroy function as that is the
right way of checking if dma_alloc_wc() was successful.
Signed-off-by: Emil Goode <emil@goode.io>
---
v2: - Change the phys member type instead of adding casts.
- Check pb->mapped in the destroy function as 0 is a valid value
for dma_addr_t.
drivers/gpu/host1x/cdma.c | 2 +-
drivers/gpu/host1x/cdma.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
index 28541b280739..02a0b61be18f 100644
--- a/drivers/gpu/host1x/cdma.c
+++ b/drivers/gpu/host1x/cdma.c
@@ -51,7 +51,7 @@ static void host1x_pushbuffer_destroy(struct push_buffer *pb)
struct host1x_cdma *cdma = pb_to_cdma(pb);
struct host1x *host1x = cdma_to_host1x(cdma);
- if (!pb->phys)
+ if (!pb->mapped)
return;
if (host1x->domain) {
diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h
index 286d49386be9..446ee1a84969 100644
--- a/drivers/gpu/host1x/cdma.h
+++ b/drivers/gpu/host1x/cdma.h
@@ -44,7 +44,7 @@ struct host1x_job;
struct push_buffer {
void *mapped; /* mapped pushbuffer memory */
dma_addr_t dma; /* device address of pushbuffer */
- phys_addr_t phys; /* physical address of pushbuffer */
+ dma_addr_t phys;/* physical address of pushbuffer */
u32 fence; /* index we've written */
u32 pos;/* index to write to */
u32 size;
--
2.11.0
[PATCH v2] gpu: host1x: Fix compiler errors by converting to dma_addr_t
The compiler is complaining with the following errors:
drivers/gpu/host1x/cdma.c:94:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
drivers/gpu/host1x/cdma.c:113:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
The expected pointer type of the third argument to dma_alloc_wc() is
dma_addr_t but phys_addr_t is passed.
Change the phys member of struct push_buffer to be dma_addr_t so that we
pass the correct type to dma_alloc_wc().
Also check pb->mapped for non-NULL in the destroy function as that is the
right way of checking if dma_alloc_wc() was successful.
Signed-off-by: Emil Goode
---
v2: - Change the phys member type instead of adding casts.
- Check pb->mapped in the destroy function as 0 is a valid value
for dma_addr_t.
drivers/gpu/host1x/cdma.c | 2 +-
drivers/gpu/host1x/cdma.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
index 28541b280739..02a0b61be18f 100644
--- a/drivers/gpu/host1x/cdma.c
+++ b/drivers/gpu/host1x/cdma.c
@@ -51,7 +51,7 @@ static void host1x_pushbuffer_destroy(struct push_buffer *pb)
struct host1x_cdma *cdma = pb_to_cdma(pb);
struct host1x *host1x = cdma_to_host1x(cdma);
- if (!pb->phys)
+ if (!pb->mapped)
return;
if (host1x->domain) {
diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h
index 286d49386be9..446ee1a84969 100644
--- a/drivers/gpu/host1x/cdma.h
+++ b/drivers/gpu/host1x/cdma.h
@@ -44,7 +44,7 @@ struct host1x_job;
struct push_buffer {
void *mapped; /* mapped pushbuffer memory */
dma_addr_t dma; /* device address of pushbuffer */
- phys_addr_t phys; /* physical address of pushbuffer */
+ dma_addr_t phys;/* physical address of pushbuffer */
u32 fence; /* index we've written */
u32 pos;/* index to write to */
u32 size;
--
2.11.0
Re: [PATCH] gpu: host1x: Fix compiler errors
Hello,
On Mon, May 14, 2018 at 10:43:08AM +0200, Thierry Reding wrote:
> On Mon, Mar 26, 2018 at 04:44:14PM +0200, Emil Goode wrote:
> > The compiler is complaining with the following errors:
> >
> > drivers/gpu/host1x/cdma.c:94:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > drivers/gpu/host1x/cdma.c:113:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > The expected pointer type of the third argument to dma_alloc_wc() is
> > dma_addr_t but phys_addr_t is passed. Fix this by adding casts to the
> > expected pointer type.
> >
> > Signed-off-by: Emil Goode <emil@goode.io>
> > ---
> > drivers/gpu/host1x/cdma.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
> > index 28541b280739..5e8b321a751e 100644
> > --- a/drivers/gpu/host1x/cdma.c
> > +++ b/drivers/gpu/host1x/cdma.c
> > @@ -91,8 +91,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
> >
> > size = iova_align(>iova, size);
> >
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
> > @@ -110,8 +110,8 @@ static int host1x_pushbuffer_init(struct push_buffer
> > *pb)
> > if (err)
> > goto iommu_free_iova;
> > } else {
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
>
> This doesn't seem right. There's no guarantee that phys_addr_t and
> dma_addr_t will be compatible, so the above isn't always correct. Also,
> I don't see a need for pb->phys to ever be phys_addr_t. It's allocated
> through dma_alloc_wc() exclusively, so it should just be dma_addr_t.
I agree that this is a better solution, however when changing pb->phys to
dma_addr_t
the type will be wrong when it's passed to iommu_map() as phys_addr_t is
expected
but that doesn't cause a compilation error.
> Note that the !pb->phys check in host1x_pushbuffer_destroy() becomes
> technically wrong if pb->phys is dma_addr_t (0 is a perfectly valid
> value for dma_addr_t), so make sure to flip that to !pb->mapped instead.
> pb->mapped and pb->phys are always set in tandem, and checking mapped
> for non-NULL is the right check to test whether the pair is valid or
> not.
Ok I will change this as well.
Best regards,
Emil
Re: [PATCH] gpu: host1x: Fix compiler errors
Hello,
On Mon, May 14, 2018 at 10:43:08AM +0200, Thierry Reding wrote:
> On Mon, Mar 26, 2018 at 04:44:14PM +0200, Emil Goode wrote:
> > The compiler is complaining with the following errors:
> >
> > drivers/gpu/host1x/cdma.c:94:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > drivers/gpu/host1x/cdma.c:113:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > The expected pointer type of the third argument to dma_alloc_wc() is
> > dma_addr_t but phys_addr_t is passed. Fix this by adding casts to the
> > expected pointer type.
> >
> > Signed-off-by: Emil Goode
> > ---
> > drivers/gpu/host1x/cdma.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
> > index 28541b280739..5e8b321a751e 100644
> > --- a/drivers/gpu/host1x/cdma.c
> > +++ b/drivers/gpu/host1x/cdma.c
> > @@ -91,8 +91,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
> >
> > size = iova_align(>iova, size);
> >
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
> > @@ -110,8 +110,8 @@ static int host1x_pushbuffer_init(struct push_buffer
> > *pb)
> > if (err)
> > goto iommu_free_iova;
> > } else {
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
>
> This doesn't seem right. There's no guarantee that phys_addr_t and
> dma_addr_t will be compatible, so the above isn't always correct. Also,
> I don't see a need for pb->phys to ever be phys_addr_t. It's allocated
> through dma_alloc_wc() exclusively, so it should just be dma_addr_t.
I agree that this is a better solution, however when changing pb->phys to
dma_addr_t
the type will be wrong when it's passed to iommu_map() as phys_addr_t is
expected
but that doesn't cause a compilation error.
> Note that the !pb->phys check in host1x_pushbuffer_destroy() becomes
> technically wrong if pb->phys is dma_addr_t (0 is a perfectly valid
> value for dma_addr_t), so make sure to flip that to !pb->mapped instead.
> pb->mapped and pb->phys are always set in tandem, and checking mapped
> for non-NULL is the right check to test whether the pair is valid or
> not.
Ok I will change this as well.
Best regards,
Emil
Re: [PATCH] gpu: host1x: Fix compiler errors
Hello,
On Mon, Mar 26, 2018 at 04:57:34PM +0200, Thierry Reding wrote:
> On Mon, Mar 26, 2018 at 04:44:14PM +0200, Emil Goode wrote:
> > The compiler is complaining with the following errors:
> >
> > drivers/gpu/host1x/cdma.c:94:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > drivers/gpu/host1x/cdma.c:113:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > The expected pointer type of the third argument to dma_alloc_wc() is
> > dma_addr_t but phys_addr_t is passed. Fix this by adding casts to the
> > expected pointer type.
> >
> > Signed-off-by: Emil Goode <emil@goode.io>
> > ---
> > drivers/gpu/host1x/cdma.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> What compiler do you use? I do regular builds and check for warnings and
> errors, and this one is new to me.
>
> Thierry
>
I use gcc version 6.3.0 cross compiler for armhf supplied with Debian Stretch.
Best regards,
Emil
> >
> > diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
> > index 28541b280739..5e8b321a751e 100644
> > --- a/drivers/gpu/host1x/cdma.c
> > +++ b/drivers/gpu/host1x/cdma.c
> > @@ -91,8 +91,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
> >
> > size = iova_align(>iova, size);
> >
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
> > @@ -110,8 +110,8 @@ static int host1x_pushbuffer_init(struct push_buffer
> > *pb)
> > if (err)
> > goto iommu_free_iova;
> > } else {
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
> > --
> > 2.11.0
> >
Re: [PATCH] gpu: host1x: Fix compiler errors
Hello,
On Mon, Mar 26, 2018 at 04:57:34PM +0200, Thierry Reding wrote:
> On Mon, Mar 26, 2018 at 04:44:14PM +0200, Emil Goode wrote:
> > The compiler is complaining with the following errors:
> >
> > drivers/gpu/host1x/cdma.c:94:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > drivers/gpu/host1x/cdma.c:113:48: error:
> > passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >
> > The expected pointer type of the third argument to dma_alloc_wc() is
> > dma_addr_t but phys_addr_t is passed. Fix this by adding casts to the
> > expected pointer type.
> >
> > Signed-off-by: Emil Goode
> > ---
> > drivers/gpu/host1x/cdma.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> What compiler do you use? I do regular builds and check for warnings and
> errors, and this one is new to me.
>
> Thierry
>
I use gcc version 6.3.0 cross compiler for armhf supplied with Debian Stretch.
Best regards,
Emil
> >
> > diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
> > index 28541b280739..5e8b321a751e 100644
> > --- a/drivers/gpu/host1x/cdma.c
> > +++ b/drivers/gpu/host1x/cdma.c
> > @@ -91,8 +91,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
> >
> > size = iova_align(>iova, size);
> >
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
> > @@ -110,8 +110,8 @@ static int host1x_pushbuffer_init(struct push_buffer
> > *pb)
> > if (err)
> > goto iommu_free_iova;
> > } else {
> > - pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
> > - GFP_KERNEL);
> > + pb->mapped = dma_alloc_wc(host1x->dev, size,
> > + (dma_addr_t *)>phys, GFP_KERNEL);
> > if (!pb->mapped)
> > return -ENOMEM;
> >
> > --
> > 2.11.0
> >
[PATCH] gpu: host1x: Fix compiler errors
The compiler is complaining with the following errors:
drivers/gpu/host1x/cdma.c:94:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
drivers/gpu/host1x/cdma.c:113:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
The expected pointer type of the third argument to dma_alloc_wc() is
dma_addr_t but phys_addr_t is passed. Fix this by adding casts to the
expected pointer type.
Signed-off-by: Emil Goode <emil@goode.io>
---
drivers/gpu/host1x/cdma.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
index 28541b280739..5e8b321a751e 100644
--- a/drivers/gpu/host1x/cdma.c
+++ b/drivers/gpu/host1x/cdma.c
@@ -91,8 +91,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
size = iova_align(>iova, size);
- pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
- GFP_KERNEL);
+ pb->mapped = dma_alloc_wc(host1x->dev, size,
+ (dma_addr_t *)>phys, GFP_KERNEL);
if (!pb->mapped)
return -ENOMEM;
@@ -110,8 +110,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
if (err)
goto iommu_free_iova;
} else {
- pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
- GFP_KERNEL);
+ pb->mapped = dma_alloc_wc(host1x->dev, size,
+ (dma_addr_t *)>phys, GFP_KERNEL);
if (!pb->mapped)
return -ENOMEM;
--
2.11.0
[PATCH] gpu: host1x: Fix compiler errors
The compiler is complaining with the following errors:
drivers/gpu/host1x/cdma.c:94:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
drivers/gpu/host1x/cdma.c:113:48: error:
passing argument 3 of ‘dma_alloc_wc’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
The expected pointer type of the third argument to dma_alloc_wc() is
dma_addr_t but phys_addr_t is passed. Fix this by adding casts to the
expected pointer type.
Signed-off-by: Emil Goode
---
drivers/gpu/host1x/cdma.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c
index 28541b280739..5e8b321a751e 100644
--- a/drivers/gpu/host1x/cdma.c
+++ b/drivers/gpu/host1x/cdma.c
@@ -91,8 +91,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
size = iova_align(>iova, size);
- pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
- GFP_KERNEL);
+ pb->mapped = dma_alloc_wc(host1x->dev, size,
+ (dma_addr_t *)>phys, GFP_KERNEL);
if (!pb->mapped)
return -ENOMEM;
@@ -110,8 +110,8 @@ static int host1x_pushbuffer_init(struct push_buffer *pb)
if (err)
goto iommu_free_iova;
} else {
- pb->mapped = dma_alloc_wc(host1x->dev, size, >phys,
- GFP_KERNEL);
+ pb->mapped = dma_alloc_wc(host1x->dev, size,
+ (dma_addr_t *)>phys, GFP_KERNEL);
if (!pb->mapped)
return -ENOMEM;
--
2.11.0
Re: wlcore: Fix regression in wlcore_set_partition()
Hello Ross
On Wed, Feb 24, 2016 at 04:40:50PM +1100, Ross Green wrote:
> On Wed, Feb 17, 2016 at 4:34 PM, Ross Green <rgker...@gmail.com> wrote:
> > Appreciate your efforts!
> >
> > Just trying to make sure it does not get lost.
> > Introduced in rc1, not fixed by ... rc4.
> >
> > Anyway, I will continue to test, lots of other things still to chase
> > even in rc4!
> >
> > Regards,
> >
> > Ross Green
> >
> > On Wed, Feb 17, 2016 at 2:24 AM, Kalle Valo <kv...@codeaurora.org> wrote:
> >> Ross Green <rgker...@gmail.com> writes:
> >>
> >>> On Fri, Feb 12, 2016 at 8:45 PM, Kalle Valo <kv...@codeaurora.org> wrote:
> >>>>
> >>>>> The commit 3719c17e1816 ("wlcore/wl18xx: fw logger over sdio")
> >>>>> introduced a
> >>>>> regression causing the wlcore to time out and go into recovery.
> >>>>> Reverting the
> >>>>> changes regarding write of the last partition size brings the module
> >>>>> back to
> >>>>> it's functional state.
> >>>>>
> >>>>> Fixes: 3719c17e1816 ("wlcore/wl18xx: fw logger over sdio")
> >>>>> Reported-by: Ross Green <rgker...@gmail.com>
> >>>>> Signed-off-by: Emil Goode <emil@goode.io>
> >>>>> [kv...@codeaurora.org: improved commit log]
> >>>>
> >>>> Thanks, applied to wireless-drivers.git.
> >>>>
> >>>> Kalle Valo
> >>>
> >>> I just tested linux-4.5-rc4 it appears the above fix missed the release
> >>> for rc4!
> >>> So the behaviour of firmware reset being called after the access of
> >>> the last partition timesout.
> >>>
> >>> Again tested patch with the new release - 4.5-rc4 and found everything
> >>> to work as expected again.
> >>>
> >>> So Hopefully for rc5 - Please!
> >>
> >> It takes some time to get patches into Linus' tree. And being in a
> >> conference and then getting sick is not really helping. I'm not sure if
> >> this patch makes to rc5 on time, but I'll try.
> >>
> >> --
> >> Kalle Valo
>
> G'day all,
>
> I have tested Emil's patch with each 4.5-rc release.
>
> Seems to work fine with rc2, rc3, rc4.
> I tried it with rc5 and get the following output from dmesg see attachment.
>
> So it looks like there is a reset that it recovers from and then proceeds OK.
>
> I see the patch has been queued by David Miller so it might make it into rc6.
> That will be great. It still does not look quite as clean as it should
> be however, given the noise in the dmesg output from rc5
>
> Regards,
>
>
> Ross Green
I'm unable to reproduce that ELP wakeup timeout with v4.5-rc5 on my pandaboard
es.
Can you easily reproduce it and are you able to reproduce it with commit
3719c17e1816 reverted?
However, I'm seeing another bug that occurs when the wlan is not configured to
connect to
an AP directly after boot (dmesg attached). It is not related to any recent
changes and goes back
before the 3.14 kernel. There seem to be an issue with the looped IRQ handling
implementation.
A scan on 2GHZ is performed and the wlcore_fw_status() call in
wlcore_irq_locked() returns
WL1271_ACX_INTR_HW_AVAILABLE, then nothing seem to happen until the delayed
work queue
scan_complete_work starts executing after the 30 sec timeout runs out and the
wlcore goes into
recovery. I guess a new scan should be initiated after wlcore_fw_status()
return hw available,
does anyone have input on that?
Best regards,
Emil Goode
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.5.0-rc5-armv7-x1-1-gffdb57e (emil@lianli)
(gcc version 4.9.3 20141031 (prerelease) (Linaro GCC 2014.11) ) #23 SMP Wed Feb
24 20:36:15 CET 2016
[0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: TI OMAP4 PandaBoard-ES
[0.00] cma: Reserved 16 MiB at 0xbe80
[0.00] Memory policy: Data cache writealloc
[0.00] OMAP4: Map 0xbfe0 to fe60 for dram barrier
[0.00] On node 0 totalpages: 261632
[0.00] free_area_init_node: node 0, pgdat c0cbab40, node_mem_map
ef6f9000
[0.00] Normal zone: 1728 pages used for memmap
[0.00] Normal zone: 0 pages reserved
[0.00] Normal zone: 196608 pages, LIFO batch:31
[0.00] HighMem zone: 65024 pages, LIFO batch:15
[0.00] OMAP4460 ES1.1
[0.00] PERCPU: E
Re: wlcore: Fix regression in wlcore_set_partition()
Hello Ross
On Wed, Feb 24, 2016 at 04:40:50PM +1100, Ross Green wrote:
> On Wed, Feb 17, 2016 at 4:34 PM, Ross Green wrote:
> > Appreciate your efforts!
> >
> > Just trying to make sure it does not get lost.
> > Introduced in rc1, not fixed by ... rc4.
> >
> > Anyway, I will continue to test, lots of other things still to chase
> > even in rc4!
> >
> > Regards,
> >
> > Ross Green
> >
> > On Wed, Feb 17, 2016 at 2:24 AM, Kalle Valo wrote:
> >> Ross Green writes:
> >>
> >>> On Fri, Feb 12, 2016 at 8:45 PM, Kalle Valo wrote:
> >>>>
> >>>>> The commit 3719c17e1816 ("wlcore/wl18xx: fw logger over sdio")
> >>>>> introduced a
> >>>>> regression causing the wlcore to time out and go into recovery.
> >>>>> Reverting the
> >>>>> changes regarding write of the last partition size brings the module
> >>>>> back to
> >>>>> it's functional state.
> >>>>>
> >>>>> Fixes: 3719c17e1816 ("wlcore/wl18xx: fw logger over sdio")
> >>>>> Reported-by: Ross Green
> >>>>> Signed-off-by: Emil Goode
> >>>>> [kv...@codeaurora.org: improved commit log]
> >>>>
> >>>> Thanks, applied to wireless-drivers.git.
> >>>>
> >>>> Kalle Valo
> >>>
> >>> I just tested linux-4.5-rc4 it appears the above fix missed the release
> >>> for rc4!
> >>> So the behaviour of firmware reset being called after the access of
> >>> the last partition timesout.
> >>>
> >>> Again tested patch with the new release - 4.5-rc4 and found everything
> >>> to work as expected again.
> >>>
> >>> So Hopefully for rc5 - Please!
> >>
> >> It takes some time to get patches into Linus' tree. And being in a
> >> conference and then getting sick is not really helping. I'm not sure if
> >> this patch makes to rc5 on time, but I'll try.
> >>
> >> --
> >> Kalle Valo
>
> G'day all,
>
> I have tested Emil's patch with each 4.5-rc release.
>
> Seems to work fine with rc2, rc3, rc4.
> I tried it with rc5 and get the following output from dmesg see attachment.
>
> So it looks like there is a reset that it recovers from and then proceeds OK.
>
> I see the patch has been queued by David Miller so it might make it into rc6.
> That will be great. It still does not look quite as clean as it should
> be however, given the noise in the dmesg output from rc5
>
> Regards,
>
>
> Ross Green
I'm unable to reproduce that ELP wakeup timeout with v4.5-rc5 on my pandaboard
es.
Can you easily reproduce it and are you able to reproduce it with commit
3719c17e1816 reverted?
However, I'm seeing another bug that occurs when the wlan is not configured to
connect to
an AP directly after boot (dmesg attached). It is not related to any recent
changes and goes back
before the 3.14 kernel. There seem to be an issue with the looped IRQ handling
implementation.
A scan on 2GHZ is performed and the wlcore_fw_status() call in
wlcore_irq_locked() returns
WL1271_ACX_INTR_HW_AVAILABLE, then nothing seem to happen until the delayed
work queue
scan_complete_work starts executing after the 30 sec timeout runs out and the
wlcore goes into
recovery. I guess a new scan should be initiated after wlcore_fw_status()
return hw available,
does anyone have input on that?
Best regards,
Emil Goode
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.5.0-rc5-armv7-x1-1-gffdb57e (emil@lianli)
(gcc version 4.9.3 20141031 (prerelease) (Linaro GCC 2014.11) ) #23 SMP Wed Feb
24 20:36:15 CET 2016
[0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: TI OMAP4 PandaBoard-ES
[0.00] cma: Reserved 16 MiB at 0xbe80
[0.00] Memory policy: Data cache writealloc
[0.00] OMAP4: Map 0xbfe0 to fe60 for dram barrier
[0.00] On node 0 totalpages: 261632
[0.00] free_area_init_node: node 0, pgdat c0cbab40, node_mem_map
ef6f9000
[0.00] Normal zone: 1728 pages used for memmap
[0.00] Normal zone: 0 pages reserved
[0.00] Normal zone: 196608 pages, LIFO batch:31
[0.00] HighMem zone: 65024 pages, LIFO batch:15
[0.00] OMAP4460 ES1.1
[0.00] PERCPU: Embedded 14 pages/cpu @ef696000 s24832 r8192 d24320
u57344
[0.00] pcpu-alloc: s24832 r8192 d24320 u57344 alloc=14*4096
[0.00] pcpu-alloc: [0] 0
[PATCH] wlcore: Fix regression in wlcore_set_partition()
The below commit introduced a regression causing the wlcore
to time out and go into recovery.
commit 3719c17e1816695f415dd3b4ddcb679f7dc617c8
("wlcore/wl18xx: fw logger over sdio")
Reverting the changes regarding write of the last partition size
brings the module back to it's functional state.
Reported-by: Ross Green
Signed-off-by: Emil Goode
---
drivers/net/wireless/ti/wlcore/io.c | 8
drivers/net/wireless/ti/wlcore/io.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/io.c
b/drivers/net/wireless/ti/wlcore/io.c
index 9ac118e..564ca75 100644
--- a/drivers/net/wireless/ti/wlcore/io.c
+++ b/drivers/net/wireless/ti/wlcore/io.c
@@ -175,14 +175,14 @@ int wlcore_set_partition(struct wl1271 *wl,
if (ret < 0)
goto out;
+ /* We don't need the size of the last partition, as it is
+* automatically calculated based on the total memory size and
+* the sizes of the previous partitions.
+*/
ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
if (ret < 0)
goto out;
- ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
- if (ret < 0)
- goto out;
-
out:
return ret;
}
diff --git a/drivers/net/wireless/ti/wlcore/io.h
b/drivers/net/wireless/ti/wlcore/io.h
index 6c257b5..10cf374 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -36,8 +36,8 @@
#define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
#define HW_PART2_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 16)
#define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
-#define HW_PART3_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
-#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
+#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
+
#define HW_ACCESS_REGISTER_SIZE 4
#define HW_ACCESS_PRAM_MAX_RANGE 0x3c000
--
2.1.4
[PATCH] wlcore: Fix regression in wlcore_set_partition()
The below commit introduced a regression causing the wlcore
to time out and go into recovery.
commit 3719c17e1816695f415dd3b4ddcb679f7dc617c8
("wlcore/wl18xx: fw logger over sdio")
Reverting the changes regarding write of the last partition size
brings the module back to it's functional state.
Reported-by: Ross Green <rgker...@gmail.com>
Signed-off-by: Emil Goode <emil@goode.io>
---
drivers/net/wireless/ti/wlcore/io.c | 8
drivers/net/wireless/ti/wlcore/io.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/io.c
b/drivers/net/wireless/ti/wlcore/io.c
index 9ac118e..564ca75 100644
--- a/drivers/net/wireless/ti/wlcore/io.c
+++ b/drivers/net/wireless/ti/wlcore/io.c
@@ -175,14 +175,14 @@ int wlcore_set_partition(struct wl1271 *wl,
if (ret < 0)
goto out;
+ /* We don't need the size of the last partition, as it is
+* automatically calculated based on the total memory size and
+* the sizes of the previous partitions.
+*/
ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
if (ret < 0)
goto out;
- ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
- if (ret < 0)
- goto out;
-
out:
return ret;
}
diff --git a/drivers/net/wireless/ti/wlcore/io.h
b/drivers/net/wireless/ti/wlcore/io.h
index 6c257b5..10cf374 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -36,8 +36,8 @@
#define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
#define HW_PART2_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 16)
#define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
-#define HW_PART3_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
-#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
+#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
+
#define HW_ACCESS_REGISTER_SIZE 4
#define HW_ACCESS_PRAM_MAX_RANGE 0x3c000
--
2.1.4
Re: wl12xx regression on 4.5 (was: Re: linux-4.5-rc1 TI pandboard-es wifi wlcore locks and reset)
Hello Ross,
On Mon, Feb 08, 2016 at 07:26:34PM +1100, Ross Green wrote:
> On Mon, Feb 8, 2016 at 9:05 AM, Emil Goode wrote:
> > Hello,
> >
> > On Tue, Feb 02, 2016 at 05:05:38PM +0100, Sebastian Reichel wrote:
> >> On Tue, Feb 02, 2016 at 04:41:13PM +1100, Ross Green wrote:
> >> > On Tue, Feb 2, 2016 at 3:34 PM, Sebastian Reichel
> >> > wrote:
> >> > > On Mon, Feb 01, 2016 at 11:38:38PM +1100, Ross Green wrote:
> >> > >> On Mon, Jan 25, 2016 at 11:47 PM, Ross Green
> >> > >> wrote:
> >> > >> > Just tried the new kernel release on faithful pandaboard es with the
> >> > >> > new 4.5-rc1 release.
> >> > >> >
> >> > >> > There is a problem with the wifi modules once the modules are
> >> > >> > loaded.
> >> > >> > Looks like the wifi firmware gets loaded put no response after that
> >> > >> > causing recovery action.
> >> > >> >
> >> > >> > the kernel 4.4 works quite happily with this board.
> >> > >> >
> >> > >> > Here is a dmesg dump in the attachment.
> >> > >> >
> >> > >> > Anyone have any ideas here?
> >> >
> >> > [...]
> >> >
> >> > If I get time, this evening, I'll see if I can give a bisect a try.
> >>
> >> On N950 [wl1271 via SPI, using extra patches to init from DT] I get wifi
> >> working again on 4.5-rc1 with 3719c17e1816 ("wlcore/wl18xx: fw logger
> >> over sdio") reverted.
> >
> > Reverting the changes in wlcore_set_partition() as below seem to help.
> >
> > Best regards,
> >
> > Emil Goode
> >
> > 8<
> >
> > --- a/drivers/net/wireless/ti/wlcore/io.c
> > +++ b/drivers/net/wireless/ti/wlcore/io.c
> > @@ -175,14 +175,15 @@ int wlcore_set_partition(struct wl1271 *wl,
> > if (ret < 0)
> > goto out;
> >
> > + /*
> > +* We don't need the size of the last partition, as it is
> > +* automatically calculated based on the total memory size and
> > +* the sizes of the previous partitions.
> > +*/
> > ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
> > if (ret < 0)
> > goto out;
> >
> > - ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
> > - if (ret < 0)
> > - goto out;
> > -
> > out:
> > return ret;
> > }
> > diff --git a/drivers/net/wireless/ti/wlcore/io.h
> > b/drivers/net/wireless/ti/wlcore/io.h
> > index 6c257b5..10cf374 100644
> > --- a/drivers/net/wireless/ti/wlcore/io.h
> > +++ b/drivers/net/wireless/ti/wlcore/io.h
> > @@ -36,8 +36,8 @@
> > #define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
> > #define HW_PART2_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 16)
> > #define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
> > -#define HW_PART3_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
> > -#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
> > +#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
> > +
> > #define HW_ACCESS_REGISTER_SIZE 4
> >
> > #define HW_ACCESS_PRAM_MAX_RANGE 0x3c000
> >
>
> Thanks Emil,
>
> Just tested linux-4.5-rc3 with the above patch and confirm everything
> works as expected. So it would be good if this patch can find its way
> into linux-4.5-rc4.
>
> So all is happy with the wlcore code with this patch.
> Now I can continue with the rest of the testing ;-)
Thank you for testing!
I'm not familiar with this code so it would be good to get a comment from ti
developers,
if not I will send this fix.
Best regards,
Emil Goode
Re: wl12xx regression on 4.5 (was: Re: linux-4.5-rc1 TI pandboard-es wifi wlcore locks and reset)
Hello Ross,
On Mon, Feb 08, 2016 at 07:26:34PM +1100, Ross Green wrote:
> On Mon, Feb 8, 2016 at 9:05 AM, Emil Goode <emil@goode.io> wrote:
> > Hello,
> >
> > On Tue, Feb 02, 2016 at 05:05:38PM +0100, Sebastian Reichel wrote:
> >> On Tue, Feb 02, 2016 at 04:41:13PM +1100, Ross Green wrote:
> >> > On Tue, Feb 2, 2016 at 3:34 PM, Sebastian Reichel <s...@kernel.org>
> >> > wrote:
> >> > > On Mon, Feb 01, 2016 at 11:38:38PM +1100, Ross Green wrote:
> >> > >> On Mon, Jan 25, 2016 at 11:47 PM, Ross Green <rgker...@gmail.com>
> >> > >> wrote:
> >> > >> > Just tried the new kernel release on faithful pandaboard es with the
> >> > >> > new 4.5-rc1 release.
> >> > >> >
> >> > >> > There is a problem with the wifi modules once the modules are
> >> > >> > loaded.
> >> > >> > Looks like the wifi firmware gets loaded put no response after that
> >> > >> > causing recovery action.
> >> > >> >
> >> > >> > the kernel 4.4 works quite happily with this board.
> >> > >> >
> >> > >> > Here is a dmesg dump in the attachment.
> >> > >> >
> >> > >> > Anyone have any ideas here?
> >> >
> >> > [...]
> >> >
> >> > If I get time, this evening, I'll see if I can give a bisect a try.
> >>
> >> On N950 [wl1271 via SPI, using extra patches to init from DT] I get wifi
> >> working again on 4.5-rc1 with 3719c17e1816 ("wlcore/wl18xx: fw logger
> >> over sdio") reverted.
> >
> > Reverting the changes in wlcore_set_partition() as below seem to help.
> >
> > Best regards,
> >
> > Emil Goode
> >
> > 8<
> >
> > --- a/drivers/net/wireless/ti/wlcore/io.c
> > +++ b/drivers/net/wireless/ti/wlcore/io.c
> > @@ -175,14 +175,15 @@ int wlcore_set_partition(struct wl1271 *wl,
> > if (ret < 0)
> > goto out;
> >
> > + /*
> > +* We don't need the size of the last partition, as it is
> > +* automatically calculated based on the total memory size and
> > +* the sizes of the previous partitions.
> > +*/
> > ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
> > if (ret < 0)
> > goto out;
> >
> > - ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
> > - if (ret < 0)
> > - goto out;
> > -
> > out:
> > return ret;
> > }
> > diff --git a/drivers/net/wireless/ti/wlcore/io.h
> > b/drivers/net/wireless/ti/wlcore/io.h
> > index 6c257b5..10cf374 100644
> > --- a/drivers/net/wireless/ti/wlcore/io.h
> > +++ b/drivers/net/wireless/ti/wlcore/io.h
> > @@ -36,8 +36,8 @@
> > #define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
> > #define HW_PART2_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 16)
> > #define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
> > -#define HW_PART3_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
> > -#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
> > +#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
> > +
> > #define HW_ACCESS_REGISTER_SIZE 4
> >
> > #define HW_ACCESS_PRAM_MAX_RANGE 0x3c000
> >
>
> Thanks Emil,
>
> Just tested linux-4.5-rc3 with the above patch and confirm everything
> works as expected. So it would be good if this patch can find its way
> into linux-4.5-rc4.
>
> So all is happy with the wlcore code with this patch.
> Now I can continue with the rest of the testing ;-)
Thank you for testing!
I'm not familiar with this code so it would be good to get a comment from ti
developers,
if not I will send this fix.
Best regards,
Emil Goode
Re: wl12xx regression on 4.5 (was: Re: linux-4.5-rc1 TI pandboard-es wifi wlcore locks and reset)
Hello,
On Tue, Feb 02, 2016 at 05:05:38PM +0100, Sebastian Reichel wrote:
> On Tue, Feb 02, 2016 at 04:41:13PM +1100, Ross Green wrote:
> > On Tue, Feb 2, 2016 at 3:34 PM, Sebastian Reichel wrote:
> > > On Mon, Feb 01, 2016 at 11:38:38PM +1100, Ross Green wrote:
> > >> On Mon, Jan 25, 2016 at 11:47 PM, Ross Green wrote:
> > >> > Just tried the new kernel release on faithful pandaboard es with the
> > >> > new 4.5-rc1 release.
> > >> >
> > >> > There is a problem with the wifi modules once the modules are loaded.
> > >> > Looks like the wifi firmware gets loaded put no response after that
> > >> > causing recovery action.
> > >> >
> > >> > the kernel 4.4 works quite happily with this board.
> > >> >
> > >> > Here is a dmesg dump in the attachment.
> > >> >
> > >> > Anyone have any ideas here?
> >
> > [...]
> >
> > If I get time, this evening, I'll see if I can give a bisect a try.
>
> On N950 [wl1271 via SPI, using extra patches to init from DT] I get wifi
> working again on 4.5-rc1 with 3719c17e1816 ("wlcore/wl18xx: fw logger
> over sdio") reverted.
Reverting the changes in wlcore_set_partition() as below seem to help.
Best regards,
Emil Goode
8<
--- a/drivers/net/wireless/ti/wlcore/io.c
+++ b/drivers/net/wireless/ti/wlcore/io.c
@@ -175,14 +175,15 @@ int wlcore_set_partition(struct wl1271 *wl,
if (ret < 0)
goto out;
+ /*
+* We don't need the size of the last partition, as it is
+* automatically calculated based on the total memory size and
+* the sizes of the previous partitions.
+*/
ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
if (ret < 0)
goto out;
- ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
- if (ret < 0)
- goto out;
-
out:
return ret;
}
diff --git a/drivers/net/wireless/ti/wlcore/io.h
b/drivers/net/wireless/ti/wlcore/io.h
index 6c257b5..10cf374 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -36,8 +36,8 @@
#define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
#define HW_PART2_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 16)
#define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
-#define HW_PART3_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
-#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
+#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
+
#define HW_ACCESS_REGISTER_SIZE 4
#define HW_ACCESS_PRAM_MAX_RANGE 0x3c000
Re: wl12xx regression on 4.5 (was: Re: linux-4.5-rc1 TI pandboard-es wifi wlcore locks and reset)
Hello,
On Tue, Feb 02, 2016 at 05:05:38PM +0100, Sebastian Reichel wrote:
> On Tue, Feb 02, 2016 at 04:41:13PM +1100, Ross Green wrote:
> > On Tue, Feb 2, 2016 at 3:34 PM, Sebastian Reichel <s...@kernel.org> wrote:
> > > On Mon, Feb 01, 2016 at 11:38:38PM +1100, Ross Green wrote:
> > >> On Mon, Jan 25, 2016 at 11:47 PM, Ross Green <rgker...@gmail.com> wrote:
> > >> > Just tried the new kernel release on faithful pandaboard es with the
> > >> > new 4.5-rc1 release.
> > >> >
> > >> > There is a problem with the wifi modules once the modules are loaded.
> > >> > Looks like the wifi firmware gets loaded put no response after that
> > >> > causing recovery action.
> > >> >
> > >> > the kernel 4.4 works quite happily with this board.
> > >> >
> > >> > Here is a dmesg dump in the attachment.
> > >> >
> > >> > Anyone have any ideas here?
> >
> > [...]
> >
> > If I get time, this evening, I'll see if I can give a bisect a try.
>
> On N950 [wl1271 via SPI, using extra patches to init from DT] I get wifi
> working again on 4.5-rc1 with 3719c17e1816 ("wlcore/wl18xx: fw logger
> over sdio") reverted.
Reverting the changes in wlcore_set_partition() as below seem to help.
Best regards,
Emil Goode
8<
--- a/drivers/net/wireless/ti/wlcore/io.c
+++ b/drivers/net/wireless/ti/wlcore/io.c
@@ -175,14 +175,15 @@ int wlcore_set_partition(struct wl1271 *wl,
if (ret < 0)
goto out;
+ /*
+* We don't need the size of the last partition, as it is
+* automatically calculated based on the total memory size and
+* the sizes of the previous partitions.
+*/
ret = wlcore_raw_write32(wl, HW_PART3_START_ADDR, p->mem3.start);
if (ret < 0)
goto out;
- ret = wlcore_raw_write32(wl, HW_PART3_SIZE_ADDR, p->mem3.size);
- if (ret < 0)
- goto out;
-
out:
return ret;
}
diff --git a/drivers/net/wireless/ti/wlcore/io.h
b/drivers/net/wireless/ti/wlcore/io.h
index 6c257b5..10cf374 100644
--- a/drivers/net/wireless/ti/wlcore/io.h
+++ b/drivers/net/wireless/ti/wlcore/io.h
@@ -36,8 +36,8 @@
#define HW_PART1_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 12)
#define HW_PART2_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 16)
#define HW_PART2_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 20)
-#define HW_PART3_SIZE_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
-#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 28)
+#define HW_PART3_START_ADDR (HW_PARTITION_REGISTERS_ADDR + 24)
+
#define HW_ACCESS_REGISTER_SIZE 4
#define HW_ACCESS_PRAM_MAX_RANGE 0x3c000
Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
Hello Arend,
Sorry for the late reply. I have attached a kernel log with brcmfmac
debugging enabled (without my patch applied).
Let me know if I can provide any other useful information.
Best regards,
Emil
On Mon, Sep 22, 2014 at 11:56:43AM +0200, Arend van Spriel wrote:
> On 09/21/14 00:58, Emil Goode wrote:
> >In the brcmf_count_20mhz_channels function we are looping through a list
> >of channels received from firmware. Since the index of the first channel
> >is 0 the condition leads to an off by one bug. This is causing us to hit
> >the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
> >how I discovered the bug.
>
> The fix is fine. Would like to know what exactly is going wrong. Can you
> provide a kernel log with brcmfmac debugging enabled, ie. insmod brcmfmac.ko
> debug=0x1416
>
> Regards,
> Arend
>
> >Introduced by:
> >commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
> >("brcmfmac: rework wiphy structure setup")
> >
> >Signed-off-by: Emil Goode
> >---
> > drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >index 02fe706..93b5dd9 100644
> >--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct
> >brcmf_cfg80211_info *cfg,
> > struct brcmu_chan ch;
> > int i;
> >
> >-for (i = 0; i<= total; i++) {
> >+for (i = 0; i< total; i++) {
> > ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
> > cfg->d11inf.decchspec();
> >
>
[0.00] Booting Linux on physical CPU 0x0
[0.00] Initializing cgroup subsys cpuset
[0.00] Initializing cgroup subsys cpu
[0.00] Initializing cgroup subsys cpuacct
[0.00] Linux version 3.17.0-rc6-armv7-x2 (emil@lianli) (gcc version
4.8.3 20140401 (prerelease) (crosstool-NG linaro-1.13.1-4.8-2014.04 - Linaro
GCC 4.8-2014.04) ) #3 SMP Mon Sep 22 21:25:25 CEST 2014
[0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: Wandboard i.MX6 Quad Board
[0.00] cma: Reserved 16 MiB at 3e80
[0.00] Memory policy: Data cache writealloc
[0.00] On node 0 totalpages: 524288
[0.00] free_area_init_node: node 0, pgdat c0f66b40, node_mem_map
ed7f
[0.00] Normal zone: 1520 pages used for memmap
[0.00] Normal zone: 0 pages reserved
[0.00] Normal zone: 194560 pages, LIFO batch:31
[0.00] HighMem zone: 2576 pages used for memmap
[0.00] HighMem zone: 329728 pages, LIFO batch:31
[0.00] PERCPU: Embedded 9 pages/cpu @ed7ab000 s14336 r8192 d14336 u36864
[0.00] pcpu-alloc: s14336 r8192 d14336 u36864 alloc=9*4096
[0.00] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3
[0.00] Built 1 zonelists in Zone order, mobility grouping on. Total
pages: 522768
[0.00] Kernel command line: console=ttymxc0,115200 console=tty0
brcmfmac.debug=0x1416 root=/dev/mmcblk0p5 ro rootfstype=ext4 rootwait
video=HDMI-A-1:1024x768@60e
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] allocated 4194304 bytes of page_cgroup
[0.00] please try 'cgroup_disable=memory' option if you don't want
memory cgroups
[0.00] Memory: 2042336K/2097152K available (10309K kernel code, 926K
rwdata, 3788K rodata, 830K init, 922K bss, 54816K reserved, 1318912K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector : 0x - 0x1000 ( 4 kB)
[0.00] fixmap : 0xffc0 - 0xffe0 (2048 kB)
[0.00] vmalloc : 0xf000 - 0xff00 ( 240 MB)
[0.00] lowmem : 0xc000 - 0xef80 ( 760 MB)
[0.00] pkmap : 0xbfe0 - 0xc000 ( 2 MB)
[0.00] modules : 0xbf00 - 0xbfe0 ( 14 MB)
[0.00] .text : 0xc0008000 - 0xc0dcca40 (14099 kB)
[0.00] .init : 0xc0dcd000 - 0xc0e9c800 ( 830 kB)
[0.00] .data : 0xc0e9e000 - 0xc0f859b8 ( 927 kB)
[0.00].bss : 0xc0f859b8 - 0xc106c51c ( 923 kB)
[0.00] Hierarchical RCU implementation.
[0.00] RCU dyntick-idle grace-period acceleration is enabled.
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] L2C-310 erratum 769419
[PATCH v2] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.
Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
("brcmfmac: rework wiphy structure setup")
Acked-by: Arend van Spriel
Signed-off-by: Emil Goode
---
v2: Added Arends "Acked-by" tag.
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 12a60ca..0517687 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4924,7 +4924,7 @@ static void brcmf_count_20mhz_channels(struct
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;
- for (i = 0; i <= total; i++) {
+ for (i = 0; i < total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
cfg->d11inf.decchspec();
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
Hello Arend,
Ok I will resend with your ack.
Best regards,
Emil
On Mon, Sep 22, 2014 at 11:49:56AM +0200, Arend van Spriel wrote:
> On 09/21/14 00:58, Emil Goode wrote:
> >In the brcmf_count_20mhz_channels function we are looping through a list
> >of channels received from firmware. Since the index of the first channel
> >is 0 the condition leads to an off by one bug. This is causing us to hit
> >the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
> >how I discovered the bug.
> >
> >Introduced by:
> >commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
> >("brcmfmac: rework wiphy structure setup")
>
> My bad :-(. You can add:
>
> Acked-by: Arend van Spriel
> >Signed-off-by: Emil Goode
> >---
> > drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >index 02fe706..93b5dd9 100644
> >--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> >@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct
> >brcmf_cfg80211_info *cfg,
> > struct brcmu_chan ch;
> > int i;
> >
> >-for (i = 0; i<= total; i++) {
> >+for (i = 0; i< total; i++) {
> > ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
> > cfg->d11inf.decchspec();
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
Hello Arend,
Ok I will resend with your ack.
Best regards,
Emil
On Mon, Sep 22, 2014 at 11:49:56AM +0200, Arend van Spriel wrote:
On 09/21/14 00:58, Emil Goode wrote:
In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.
Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
(brcmfmac: rework wiphy structure setup)
My bad :-(. You can add:
Acked-by: Arend van Spriel ar...@broadcom.com
Signed-off-by: Emil Goodeemilgo...@gmail.com
---
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;
-for (i = 0; i= total; i++) {
+for (i = 0; i total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist-element[i]);
cfg-d11inf.decchspec(ch);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.
Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
(brcmfmac: rework wiphy structure setup)
Acked-by: Arend van Spriel ar...@broadcom.com
Signed-off-by: Emil Goode emilgo...@gmail.com
---
v2: Added Arends Acked-by tag.
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 12a60ca..0517687 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4924,7 +4924,7 @@ static void brcmf_count_20mhz_channels(struct
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;
- for (i = 0; i = total; i++) {
+ for (i = 0; i total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist-element[i]);
cfg-d11inf.decchspec(ch);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
Hello Arend,
Sorry for the late reply. I have attached a kernel log with brcmfmac
debugging enabled (without my patch applied).
Let me know if I can provide any other useful information.
Best regards,
Emil
On Mon, Sep 22, 2014 at 11:56:43AM +0200, Arend van Spriel wrote:
On 09/21/14 00:58, Emil Goode wrote:
In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.
The fix is fine. Would like to know what exactly is going wrong. Can you
provide a kernel log with brcmfmac debugging enabled, ie. insmod brcmfmac.ko
debug=0x1416
Regards,
Arend
Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
(brcmfmac: rework wiphy structure setup)
Signed-off-by: Emil Goodeemilgo...@gmail.com
---
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;
-for (i = 0; i= total; i++) {
+for (i = 0; i total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist-element[i]);
cfg-d11inf.decchspec(ch);
[0.00] Booting Linux on physical CPU 0x0
[0.00] Initializing cgroup subsys cpuset
[0.00] Initializing cgroup subsys cpu
[0.00] Initializing cgroup subsys cpuacct
[0.00] Linux version 3.17.0-rc6-armv7-x2 (emil@lianli) (gcc version
4.8.3 20140401 (prerelease) (crosstool-NG linaro-1.13.1-4.8-2014.04 - Linaro
GCC 4.8-2014.04) ) #3 SMP Mon Sep 22 21:25:25 CEST 2014
[0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[0.00] Machine model: Wandboard i.MX6 Quad Board
[0.00] cma: Reserved 16 MiB at 3e80
[0.00] Memory policy: Data cache writealloc
[0.00] On node 0 totalpages: 524288
[0.00] free_area_init_node: node 0, pgdat c0f66b40, node_mem_map
ed7f
[0.00] Normal zone: 1520 pages used for memmap
[0.00] Normal zone: 0 pages reserved
[0.00] Normal zone: 194560 pages, LIFO batch:31
[0.00] HighMem zone: 2576 pages used for memmap
[0.00] HighMem zone: 329728 pages, LIFO batch:31
[0.00] PERCPU: Embedded 9 pages/cpu @ed7ab000 s14336 r8192 d14336 u36864
[0.00] pcpu-alloc: s14336 r8192 d14336 u36864 alloc=9*4096
[0.00] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3
[0.00] Built 1 zonelists in Zone order, mobility grouping on. Total
pages: 522768
[0.00] Kernel command line: console=ttymxc0,115200 console=tty0
brcmfmac.debug=0x1416 root=/dev/mmcblk0p5 ro rootfstype=ext4 rootwait
video=HDMI-A-1:1024x768@60e
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] allocated 4194304 bytes of page_cgroup
[0.00] please try 'cgroup_disable=memory' option if you don't want
memory cgroups
[0.00] Memory: 2042336K/2097152K available (10309K kernel code, 926K
rwdata, 3788K rodata, 830K init, 922K bss, 54816K reserved, 1318912K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector : 0x - 0x1000 ( 4 kB)
[0.00] fixmap : 0xffc0 - 0xffe0 (2048 kB)
[0.00] vmalloc : 0xf000 - 0xff00 ( 240 MB)
[0.00] lowmem : 0xc000 - 0xef80 ( 760 MB)
[0.00] pkmap : 0xbfe0 - 0xc000 ( 2 MB)
[0.00] modules : 0xbf00 - 0xbfe0 ( 14 MB)
[0.00] .text : 0xc0008000 - 0xc0dcca40 (14099 kB)
[0.00] .init : 0xc0dcd000 - 0xc0e9c800 ( 830 kB)
[0.00] .data : 0xc0e9e000 - 0xc0f859b8 ( 927 kB)
[0.00].bss : 0xc0f859b8 - 0xc106c51c ( 923 kB)
[0.00] Hierarchical RCU implementation.
[0.00] RCU dyntick-idle grace-period acceleration is enabled.
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] L2C-310 erratum 769419 enabled
[0.00] L2C-310 enabling early BRESP for Cortex-A9
[0.00] L2C-310 full line of zeros enabled for Cortex-A9
[0.00] L2C-310 ID prefetch enabled, offset 1 lines
[0.00] L2C-310 dynamic clock gating enabled, standby mode enabled
[0.00
[PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.
Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
("brcmfmac: rework wiphy structure setup")
Signed-off-by: Emil Goode
---
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;
- for (i = 0; i <= total; i++) {
+ for (i = 0; i < total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist->element[i]);
cfg->d11inf.decchspec();
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] brcmfmac: Fix off by one bug in brcmf_count_20mhz_channels()
In the brcmf_count_20mhz_channels function we are looping through a list
of channels received from firmware. Since the index of the first channel
is 0 the condition leads to an off by one bug. This is causing us to hit
the WARN_ON_ONCE(1) calls in the brcmu_d11n_decchspec function, which is
how I discovered the bug.
Introduced by:
commit b48d891676f756d48b4d0ee131e4a7a5d43ca417
(brcmfmac: rework wiphy structure setup)
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 02fe706..93b5dd9 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
@@ -4918,7 +4918,7 @@ static void brcmf_count_20mhz_channels(struct
brcmf_cfg80211_info *cfg,
struct brcmu_chan ch;
int i;
- for (i = 0; i = total; i++) {
+ for (i = 0; i total; i++) {
ch.chspec = (u16)le32_to_cpu(chlist-element[i]);
cfg-d11inf.decchspec(ch);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: usb5303: make use of uninitialized err variable
Hello Greg and Geert, On Thu, Jul 10, 2014 at 09:38:00AM +0200, Geert Uytterhoeven wrote: > Hi Greg, > > On Thu, Jul 10, 2014 at 12:46 AM, Greg Kroah-Hartman > wrote: > > On Mon, Jun 02, 2014 at 07:45:25PM +0200, Emil Goode wrote: > >> The variable err is not initialized here, this patch uses it > >> to store an eventual error value from devm_clk_get(). > >> > >> Signed-off-by: Emil Goode > >> Acked-by: Geert Uytterhoeven > >> --- > >> drivers/usb/misc/usb3503.c | 10 +++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > > > This patch fails to apply to my tree :( > > You've already applied > commit ec5734c41bee2ee7c938a8f34853d31cada7e67a > Author: Tushar Behera > Date: Tue Jun 17 16:38:50 2014 +0530 > > usb: misc: usb3503: Update error code in print message > > 'err' is uninitialized, rather print the error code directly. > > which fixes it slightly different. Yes, my patch was sent about two weeks earlier, but of course please pick the one you think is best. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: usb5303: make use of uninitialized err variable
Hello Greg and Geert, On Thu, Jul 10, 2014 at 09:38:00AM +0200, Geert Uytterhoeven wrote: Hi Greg, On Thu, Jul 10, 2014 at 12:46 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Mon, Jun 02, 2014 at 07:45:25PM +0200, Emil Goode wrote: The variable err is not initialized here, this patch uses it to store an eventual error value from devm_clk_get(). Signed-off-by: Emil Goode emilgo...@gmail.com Acked-by: Geert Uytterhoeven ge...@linux-m68k.org --- drivers/usb/misc/usb3503.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) This patch fails to apply to my tree :( You've already applied commit ec5734c41bee2ee7c938a8f34853d31cada7e67a Author: Tushar Behera tusha...@samsung.com Date: Tue Jun 17 16:38:50 2014 +0530 usb: misc: usb3503: Update error code in print message 'err' is uninitialized, rather print the error code directly. which fixes it slightly different. Yes, my patch was sent about two weeks earlier, but of course please pick the one you think is best. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
Hello Maciej,
I didn't know it was possible to go beyond commit 1da177e,
thank you for the info.
Best regards,
Emil
On Sun, Jul 06, 2014 at 12:16:38PM +0100, Maciej W. Rozycki wrote:
> On Sun, 6 Jul 2014, Emil Goode wrote:
>
> > > > diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> > > > index d657493..6546758 100644
> > > > --- a/arch/mips/mm/tlb-r3k.c
> > > > +++ b/arch/mips/mm/tlb-r3k.c
> > > > @@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct
> > > > *vma, unsigned long page)
> > > > {
> > > > int cpu = smp_processor_id();
> > > >
> > > > - if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> > > > + if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
> > >
> > > Sorry for replying "too late", but grepping through the kernel code I
> > > fail to find any caller that does not dereference vma before calling
> > > (local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
> > > be NULL, so I would say it is safe to assume vma is never NULL, and
> > > the NULL check can be removed completely.
> > >
> > > Also it looks like this "bug" was there since at least 2.6.12, and
> > > never seem to have bitten anyone.
> >
> > Yes, the bug pre-dates GIT history and I agree that it is most unlikely
> > that it ever caused a problem. I will send a new patch that removes the
> > NULL check of vma.
>
> Well, as at 2.4.26 (picked randomly) code in arch/mips/mm/tlb-r4k.c had
> the same check:
>
> void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> {
> int cpu = smp_processor_id();
>
> if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> unsigned long flags;
> [...]
>
> but code in arch/mips64/mm/tlb-r4k.c did not:
>
> void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> {
> int cpu = smp_processor_id();
>
> if (cpu_context(cpu, vma->vm_mm) != 0) {
> unsigned long flags;
> [...]
>
> and the current situation is an artefact from the 32-bit and 64-bit port
> unification:
>
> commit efc2f9a8a100511399309a50a33b46ff099f3454
> Author: Ralf Baechle
> Date: Mon Jul 21 03:03:15 2003 +
>
> Unify tlb-r4k.c.
>
> -- at which point tlb-r3k.c wasn't updated accordingly. The condition was
> eventually removed from arch/mips/mm/tlb-r4k.c in 2.4 too with:
>
> commit fd8917812546ef2278e006237a7cc38497bfbf52
> Author: Ralf Baechle
> Date: Thu Nov 25 22:18:38 2004 +
>
> Try to glue the hazard avoidance stuff the same way it was done for
> 2.6 before the TLB synthesizer. Previously the code for some CPUs
> such as the RM9000 was completly bogus nonsense ... I guess we may
> eventually want to backport the tlb synthesizer to 2.4 once the dust
> has settled.
>
> And arch/mips64/mm/tlb-r4k.c was introduced with:
>
> commit 49be6d9f37bbac0a0c413a2a63dcf7cc497d144a
> Author: Ralf Baechle
> Date: Wed Jul 24 16:12:03 2002 +
>
> Random 64-bit updates and fixes.
>
> and never had the condition in the first place.
>
> We do have full 2.6 GIT history, beyond 2.6.12. It seems cut off at
> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 for some operations, but not
> plain `git log' for example, and I was able to peek beyond that by
> checking out the immediately preceding commit, that is
> 66f0a432564b5f0ebf632263ceac84a10a99de09:
>
> commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
> Author: Linus Torvalds
> Date: Sat Apr 16 15:20:36 2005 -0700
>
> Linux-2.6.12-rc2
>
> Initial git repository build. I'm not bothering with the full history,
> even though we have it. We can create a separate "historical" git
> archive of that later if we want to, and in the meantime it's about
> 3.2GB when imported into git - space that would just make the early
> git days unnecessarily complicated, when we don't have a lot of good
> infrastructure for it.
>
> Let it rip!
>
> commit 66f0a432564b5f0ebf632263ceac84a10a99de09
> Author: Ralf Baechle
> Date: Fri Apr 15 14:40:46 2005 +
>
> Add resource managment.
>
> There may be a better way, I don't claim GIT expertise.
>
> Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
Hello Maciej,
I didn't know it was possible to go beyond commit 1da177e,
thank you for the info.
Best regards,
Emil
On Sun, Jul 06, 2014 at 12:16:38PM +0100, Maciej W. Rozycki wrote:
On Sun, 6 Jul 2014, Emil Goode wrote:
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct
*vma, unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
+ if (vma cpu_context(cpu, vma-vm_mm) != 0) {
Sorry for replying too late, but grepping through the kernel code I
fail to find any caller that does not dereference vma before calling
(local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
be NULL, so I would say it is safe to assume vma is never NULL, and
the NULL check can be removed completely.
Also it looks like this bug was there since at least 2.6.12, and
never seem to have bitten anyone.
Yes, the bug pre-dates GIT history and I agree that it is most unlikely
that it ever caused a problem. I will send a new patch that removes the
NULL check of vma.
Well, as at 2.4.26 (picked randomly) code in arch/mips/mm/tlb-r4k.c had
the same check:
void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
{
int cpu = smp_processor_id();
if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
unsigned long flags;
[...]
but code in arch/mips64/mm/tlb-r4k.c did not:
void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
{
int cpu = smp_processor_id();
if (cpu_context(cpu, vma-vm_mm) != 0) {
unsigned long flags;
[...]
and the current situation is an artefact from the 32-bit and 64-bit port
unification:
commit efc2f9a8a100511399309a50a33b46ff099f3454
Author: Ralf Baechle r...@linux-mips.org
Date: Mon Jul 21 03:03:15 2003 +
Unify tlb-r4k.c.
-- at which point tlb-r3k.c wasn't updated accordingly. The condition was
eventually removed from arch/mips/mm/tlb-r4k.c in 2.4 too with:
commit fd8917812546ef2278e006237a7cc38497bfbf52
Author: Ralf Baechle r...@linux-mips.org
Date: Thu Nov 25 22:18:38 2004 +
Try to glue the hazard avoidance stuff the same way it was done for
2.6 before the TLB synthesizer. Previously the code for some CPUs
such as the RM9000 was completly bogus nonsense ... I guess we may
eventually want to backport the tlb synthesizer to 2.4 once the dust
has settled.
And arch/mips64/mm/tlb-r4k.c was introduced with:
commit 49be6d9f37bbac0a0c413a2a63dcf7cc497d144a
Author: Ralf Baechle r...@linux-mips.org
Date: Wed Jul 24 16:12:03 2002 +
Random 64-bit updates and fixes.
and never had the condition in the first place.
We do have full 2.6 GIT history, beyond 2.6.12. It seems cut off at
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 for some operations, but not
plain `git log' for example, and I was able to peek beyond that by
checking out the immediately preceding commit, that is
66f0a432564b5f0ebf632263ceac84a10a99de09:
commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
Author: Linus Torvalds torva...@ppc970.osdl.org
Date: Sat Apr 16 15:20:36 2005 -0700
Linux-2.6.12-rc2
Initial git repository build. I'm not bothering with the full history,
even though we have it. We can create a separate historical git
archive of that later if we want to, and in the meantime it's about
3.2GB when imported into git - space that would just make the early
git days unnecessarily complicated, when we don't have a lot of good
infrastructure for it.
Let it rip!
commit 66f0a432564b5f0ebf632263ceac84a10a99de09
Author: Ralf Baechle r...@linux-mips.org
Date: Fri Apr 15 14:40:46 2005 +
Add resource managment.
There may be a better way, I don't claim GIT expertise.
Maciej
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MIPS: Remove incorrect NULL check in local_flush_tlb_page()
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent was to make sure vma is
not NULL but this is not necessary since the bug pre-dates GIT history
and seem to never have caused a problem. The tlb-4k and tlb-8k versions
of local_flush_tlb_page() don't bother checking if vma is NULL, also
vma is dereferenced before being passed to local_flush_tlb_page(),
thus it is safe to remove this NULL check.
Signed-off-by: Emil Goode
---
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..4094bbd 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
+ if (cpu_context(cpu, vma->vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
Hello,
On Sat, Jul 05, 2014 at 09:10:44PM +0200, Jonas Gorski wrote:
> On Sat, Jul 5, 2014 at 8:26 PM, Emil Goode wrote:
> > We check that the struct vm_area_struct pointer vma is NULL and then
> > dereference it a few lines below. The intent must have been to make sure
> > that vma is not NULL and then to check the value from cpu_context() for
> > the condition to be true.
> >
> > Signed-off-by: Emil Goode
> > ---
> >
> > v2: Updated the commit message with a better explanation.
> >
> > arch/mips/mm/tlb-r3k.c |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> > index d657493..6546758 100644
> > --- a/arch/mips/mm/tlb-r3k.c
> > +++ b/arch/mips/mm/tlb-r3k.c
> > @@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
> > unsigned long page)
> > {
> > int cpu = smp_processor_id();
> >
> > - if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> > + if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
>
> Sorry for replying "too late", but grepping through the kernel code I
> fail to find any caller that does not dereference vma before calling
> (local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
> be NULL, so I would say it is safe to assume vma is never NULL, and
> the NULL check can be removed completely.
>
> Also it looks like this "bug" was there since at least 2.6.12, and
> never seem to have bitten anyone.
Yes, the bug pre-dates GIT history and I agree that it is most unlikely
that it ever caused a problem. I will send a new patch that removes the
NULL check of vma.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent must have been to make sure
that vma is not NULL and then to check the value from cpu_context() for
the condition to be true.
Signed-off-by: Emil Goode
---
v2: Updated the commit message with a better explanation.
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
+ if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent must have been to make sure
that vma is not NULL and then to check the value from cpu_context() for
the condition to be true.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
v2: Updated the commit message with a better explanation.
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
+ if (vma cpu_context(cpu, vma-vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
Hello,
On Sat, Jul 05, 2014 at 09:10:44PM +0200, Jonas Gorski wrote:
On Sat, Jul 5, 2014 at 8:26 PM, Emil Goode emilgo...@gmail.com wrote:
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent must have been to make sure
that vma is not NULL and then to check the value from cpu_context() for
the condition to be true.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
v2: Updated the commit message with a better explanation.
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
+ if (vma cpu_context(cpu, vma-vm_mm) != 0) {
Sorry for replying too late, but grepping through the kernel code I
fail to find any caller that does not dereference vma before calling
(local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
be NULL, so I would say it is safe to assume vma is never NULL, and
the NULL check can be removed completely.
Also it looks like this bug was there since at least 2.6.12, and
never seem to have bitten anyone.
Yes, the bug pre-dates GIT history and I agree that it is most unlikely
that it ever caused a problem. I will send a new patch that removes the
NULL check of vma.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MIPS: Remove incorrect NULL check in local_flush_tlb_page()
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent was to make sure vma is
not NULL but this is not necessary since the bug pre-dates GIT history
and seem to never have caused a problem. The tlb-4k and tlb-8k versions
of local_flush_tlb_page() don't bother checking if vma is NULL, also
vma is dereferenced before being passed to local_flush_tlb_page(),
thus it is safe to remove this NULL check.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..4094bbd 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
+ if (cpu_context(cpu, vma-vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
Hello Jonas,
On Fri, Jul 04, 2014 at 11:52:51PM +0200, Jonas Gorski wrote:
> On Fri, Jul 4, 2014 at 7:07 PM, Emil Goode wrote:
> > We check that the struct vm_area_struct pointer vma is NULL and
> > then dereference it. The intent must have been to check that
> > vma is not NULL before we dereference it in the next condition.
>
> Actually if it is NULL, then it will short-cut and won't dereference
> it (because !vma is true it can never become false again), so the
> condition would be fine previously.
>
> But, looking at the code a few lines into branch:
>
> if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> unsigned long flags;
> int oldpid, newpid, idx;
>
> #ifdef DEBUG_TLB
> printk("[tlbpage<%lu,0x%08lx>]", cpu_context(cpu,
> vma->vm_mm), page);
> #endif
> newpid = cpu_context(cpu, vma->vm_mm) & ASID_MASK;
>
> it will be then dereferenced here, so the change is actually sensible,
> even if the description isn't quite spot-on where it breaks.
Sorry, this is what I meant but failed to explain clearly.
Perhaps the following is a bit better?
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent must have been to make sure
that vma is not NULL and then to check the value from cpu_context() for the
condition to be true.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc
Hello,
I noticed one more thing.
On Fri, Jul 04, 2014 at 07:07:48PM +0200, Rickard Strandqvist wrote:
> 2014-07-04 11:10 GMT+02:00 Emil Goode :
> > Hello Rickard,
> >
> > Since this is a probe function there is also no need to release the devm_*
> > resources in the i2c_pxa_remove function, this leads to double free.
> >
> > Also I have a few nit-pick comments below.
> >
> > On Thu, Jul 03, 2014 at 10:19:16PM +0200, Rickard Strandqvist wrote:
> >> Fix for possible null pointer dereferenc, and there is a risk for memory
> >> leak if something unexpected happens and the function returns.
> >> It now use Managed Device Resource instead.
> >>
> >> Signed-off-by: Rickard Strandqvist
> >> ---
> >> drivers/i2c/busses/i2c-pxa.c | 37 -
> >> 1 file changed, 16 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> >> index be671f7..2edb633 100644
> >> --- a/drivers/i2c/busses/i2c-pxa.c
> >> +++ b/drivers/i2c/busses/i2c-pxa.c
> >> @@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device
> >> *dev)
> >> struct resource *res = NULL;
> >> int ret, irq;
> >>
> >> - i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> >> + i2c = devm_kzalloc(>dev, sizeof(struct pxa_i2c), GFP_KERNEL);
> >> if (!i2c) {
> >> ret = -ENOMEM;
> >> - goto emalloc;
> >> + goto err_nothing_to_release;
> >
> > Perhaps its a good idea to return directly here, then the label
> > err_nothing_to_release can be removed.
> >
> >> }
> >>
> >> /* Default adapter num to device id; i2c_pxa_probe_dt can override.
> >> */
> >> @@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device
> >> *dev)
> >> if (ret > 0)
> >> ret = i2c_pxa_probe_pdata(dev, i2c, _type);
> >> if (ret < 0)
> >> - goto eclk;
> >> + goto err_nothing_to_release;
> >
> > Same here.
> >
> >>
> >> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> >> irq = platform_get_irq(dev, 0);
> >> if (res == NULL || irq < 0) {
> >> ret = -ENODEV;
> >> - goto eclk;
> >> + goto err_nothing_to_release;
> >
> > Same here.
> >
> >> }
> >>
> >> - if (!request_mem_region(res->start, resource_size(res), res->name)) {
> >> + if (!devm_request_mem_region(>dev, res->start,
> >> + resource_size(res), res->name)) {
> >> ret = -ENOMEM;
> >> - goto eclk;
> >> + goto emalloc;
> >
> > We could also return directly here since the release_mem_region call should
> > be removed, as mentioned in Jingoos reply.
> >
> >> }
> >>
> >> i2c->adap.owner = THIS_MODULE;
> >> @@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device
> >> *dev)
> >>
> >> strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
> >>
> >> - i2c->clk = clk_get(>dev, NULL);
> >> + i2c->clk = devm_clk_get(>dev, NULL);
> >> if (IS_ERR(i2c->clk)) {
> >> ret = PTR_ERR(i2c->clk);
> >> - goto eclk;
> >> + goto emalloc;
> >
> > Same here.
> >
> >> }
> >>
> >> - i2c->reg_base = ioremap(res->start, resource_size(res));
> >> - if (!i2c->reg_base) {
> >> + i2c->reg_base = devm_ioremap_resource(>dev, res));
> >> + if (IS_ERR(i2c->reg_base)) {
> >> ret = -EIO;
> >> - goto eremap;
> >> + goto emalloc;
> >
> > Same here.
> >
> >> }
> >>
> >> i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
> >> @@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device
> >> *dev)
> >> i2c->adap.algo = _pxa_pio_algorithm;
> >> } else {
> >> i2c->adap.algo = _pxa_algorithm;
> >> - ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
> >> -
[PATCH] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
We check that the struct vm_area_struct pointer vma is NULL and
then dereference it. The intent must have been to check that
vma is not NULL before we dereference it in the next condition.
Signed-off-by: Emil Goode
---
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
+ if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc
Hello Rickard,
Since this is a probe function there is also no need to release the devm_*
resources in the i2c_pxa_remove function, this leads to double free.
Also I have a few nit-pick comments below.
On Thu, Jul 03, 2014 at 10:19:16PM +0200, Rickard Strandqvist wrote:
> Fix for possible null pointer dereferenc, and there is a risk for memory leak
> if something unexpected happens and the function returns.
> It now use Managed Device Resource instead.
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/i2c/busses/i2c-pxa.c | 37 -
> 1 file changed, 16 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index be671f7..2edb633 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
> struct resource *res = NULL;
> int ret, irq;
>
> - i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> + i2c = devm_kzalloc(>dev, sizeof(struct pxa_i2c), GFP_KERNEL);
> if (!i2c) {
> ret = -ENOMEM;
> - goto emalloc;
> + goto err_nothing_to_release;
Perhaps its a good idea to return directly here, then the label
err_nothing_to_release can be removed.
> }
>
> /* Default adapter num to device id; i2c_pxa_probe_dt can override. */
> @@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device *dev)
> if (ret > 0)
> ret = i2c_pxa_probe_pdata(dev, i2c, _type);
> if (ret < 0)
> - goto eclk;
> + goto err_nothing_to_release;
Same here.
>
> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(dev, 0);
> if (res == NULL || irq < 0) {
> ret = -ENODEV;
> - goto eclk;
> + goto err_nothing_to_release;
Same here.
> }
>
> - if (!request_mem_region(res->start, resource_size(res), res->name)) {
> + if (!devm_request_mem_region(>dev, res->start,
> + resource_size(res), res->name)) {
> ret = -ENOMEM;
> - goto eclk;
> + goto emalloc;
We could also return directly here since the release_mem_region call should
be removed, as mentioned in Jingoos reply.
> }
>
> i2c->adap.owner = THIS_MODULE;
> @@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
>
> strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
>
> - i2c->clk = clk_get(>dev, NULL);
> + i2c->clk = devm_clk_get(>dev, NULL);
> if (IS_ERR(i2c->clk)) {
> ret = PTR_ERR(i2c->clk);
> - goto eclk;
> + goto emalloc;
Same here.
> }
>
> - i2c->reg_base = ioremap(res->start, resource_size(res));
> - if (!i2c->reg_base) {
> + i2c->reg_base = devm_ioremap_resource(>dev, res));
> + if (IS_ERR(i2c->reg_base)) {
> ret = -EIO;
> - goto eremap;
> + goto emalloc;
Same here.
> }
>
> i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
> @@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
> i2c->adap.algo = _pxa_pio_algorithm;
> } else {
> i2c->adap.algo = _pxa_algorithm;
> - ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
> - dev_name(>dev), i2c);
> + ret = devm_request_irq(>dev, irq, i2c_pxa_handler,
> + IRQF_SHARED, dev_name(>dev), i2c);
> if (ret)
> - goto ereqirq;
> + goto emalloc;
Same here.
> }
>
> i2c_pxa_reset(i2c);
> @@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
> eadapt:
> if (!i2c->use_pio)
> free_irq(irq, i2c);
The free_irq() call should also be removed since devm_request_irq() was used.
> -ereqirq:
> - clk_disable_unprepare(i2c->clk);
> - iounmap(i2c->reg_base);
> -eremap:
> - clk_put(i2c->clk);
> -eclk:
> - kfree(i2c);
> emalloc:
> release_mem_region(res->start, resource_size(res));
> +err_nothing_to_release:
> return ret;
> }
>
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
Hello Jonas,
On Fri, Jul 04, 2014 at 11:52:51PM +0200, Jonas Gorski wrote:
On Fri, Jul 4, 2014 at 7:07 PM, Emil Goode emilgo...@gmail.com wrote:
We check that the struct vm_area_struct pointer vma is NULL and
then dereference it. The intent must have been to check that
vma is not NULL before we dereference it in the next condition.
Actually if it is NULL, then it will short-cut and won't dereference
it (because !vma is true it can never become false again), so the
condition would be fine previously.
But, looking at the code a few lines into branch:
if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
#ifdef DEBUG_TLB
printk([tlbpage%lu,0x%08lx], cpu_context(cpu,
vma-vm_mm), page);
#endif
newpid = cpu_context(cpu, vma-vm_mm) ASID_MASK;
it will be then dereferenced here, so the change is actually sensible,
even if the description isn't quite spot-on where it breaks.
Sorry, this is what I meant but failed to explain clearly.
Perhaps the following is a bit better?
We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent must have been to make sure
that vma is not NULL and then to check the value from cpu_context() for the
condition to be true.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc
Hello Rickard,
Since this is a probe function there is also no need to release the devm_*
resources in the i2c_pxa_remove function, this leads to double free.
Also I have a few nit-pick comments below.
On Thu, Jul 03, 2014 at 10:19:16PM +0200, Rickard Strandqvist wrote:
Fix for possible null pointer dereferenc, and there is a risk for memory leak
if something unexpected happens and the function returns.
It now use Managed Device Resource instead.
Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
---
drivers/i2c/busses/i2c-pxa.c | 37 -
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..2edb633 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
struct resource *res = NULL;
int ret, irq;
- i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
+ i2c = devm_kzalloc(dev-dev, sizeof(struct pxa_i2c), GFP_KERNEL);
if (!i2c) {
ret = -ENOMEM;
- goto emalloc;
+ goto err_nothing_to_release;
Perhaps its a good idea to return directly here, then the label
err_nothing_to_release can be removed.
}
/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
@@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device *dev)
if (ret 0)
ret = i2c_pxa_probe_pdata(dev, i2c, i2c_type);
if (ret 0)
- goto eclk;
+ goto err_nothing_to_release;
Same here.
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
irq = platform_get_irq(dev, 0);
if (res == NULL || irq 0) {
ret = -ENODEV;
- goto eclk;
+ goto err_nothing_to_release;
Same here.
}
- if (!request_mem_region(res-start, resource_size(res), res-name)) {
+ if (!devm_request_mem_region(dev-dev, res-start,
+ resource_size(res), res-name)) {
ret = -ENOMEM;
- goto eclk;
+ goto emalloc;
We could also return directly here since the release_mem_region call should
be removed, as mentioned in Jingoos reply.
}
i2c-adap.owner = THIS_MODULE;
@@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
strlcpy(i2c-adap.name, pxa_i2c-i2c, sizeof(i2c-adap.name));
- i2c-clk = clk_get(dev-dev, NULL);
+ i2c-clk = devm_clk_get(dev-dev, NULL);
if (IS_ERR(i2c-clk)) {
ret = PTR_ERR(i2c-clk);
- goto eclk;
+ goto emalloc;
Same here.
}
- i2c-reg_base = ioremap(res-start, resource_size(res));
- if (!i2c-reg_base) {
+ i2c-reg_base = devm_ioremap_resource(adev-dev, res));
+ if (IS_ERR(i2c-reg_base)) {
ret = -EIO;
- goto eremap;
+ goto emalloc;
Same here.
}
i2c-reg_ibmr = i2c-reg_base + pxa_reg_layout[i2c_type].ibmr;
@@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
i2c-adap.algo = i2c_pxa_pio_algorithm;
} else {
i2c-adap.algo = i2c_pxa_algorithm;
- ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
- dev_name(dev-dev), i2c);
+ ret = devm_request_irq(dev-dev, irq, i2c_pxa_handler,
+ IRQF_SHARED, dev_name(dev-dev), i2c);
if (ret)
- goto ereqirq;
+ goto emalloc;
Same here.
}
i2c_pxa_reset(i2c);
@@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
eadapt:
if (!i2c-use_pio)
free_irq(irq, i2c);
The free_irq() call should also be removed since devm_request_irq() was used.
-ereqirq:
- clk_disable_unprepare(i2c-clk);
- iounmap(i2c-reg_base);
-eremap:
- clk_put(i2c-clk);
-eclk:
- kfree(i2c);
emalloc:
release_mem_region(res-start, resource_size(res));
+err_nothing_to_release:
return ret;
}
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MIPS: Fix incorrect NULL check in local_flush_tlb_page()
We check that the struct vm_area_struct pointer vma is NULL and
then dereference it. The intent must have been to check that
vma is not NULL before we dereference it in the next condition.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
arch/mips/mm/tlb-r3k.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma,
unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma-vm_mm) != 0) {
+ if (vma cpu_context(cpu, vma-vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc
Hello,
I noticed one more thing.
On Fri, Jul 04, 2014 at 07:07:48PM +0200, Rickard Strandqvist wrote:
2014-07-04 11:10 GMT+02:00 Emil Goode emilgo...@gmail.com:
Hello Rickard,
Since this is a probe function there is also no need to release the devm_*
resources in the i2c_pxa_remove function, this leads to double free.
Also I have a few nit-pick comments below.
On Thu, Jul 03, 2014 at 10:19:16PM +0200, Rickard Strandqvist wrote:
Fix for possible null pointer dereferenc, and there is a risk for memory
leak if something unexpected happens and the function returns.
It now use Managed Device Resource instead.
Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
---
drivers/i2c/busses/i2c-pxa.c | 37 -
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..2edb633 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device
*dev)
struct resource *res = NULL;
int ret, irq;
- i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
+ i2c = devm_kzalloc(dev-dev, sizeof(struct pxa_i2c), GFP_KERNEL);
if (!i2c) {
ret = -ENOMEM;
- goto emalloc;
+ goto err_nothing_to_release;
Perhaps its a good idea to return directly here, then the label
err_nothing_to_release can be removed.
}
/* Default adapter num to device id; i2c_pxa_probe_dt can override.
*/
@@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device
*dev)
if (ret 0)
ret = i2c_pxa_probe_pdata(dev, i2c, i2c_type);
if (ret 0)
- goto eclk;
+ goto err_nothing_to_release;
Same here.
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
irq = platform_get_irq(dev, 0);
if (res == NULL || irq 0) {
ret = -ENODEV;
- goto eclk;
+ goto err_nothing_to_release;
Same here.
}
- if (!request_mem_region(res-start, resource_size(res), res-name)) {
+ if (!devm_request_mem_region(dev-dev, res-start,
+ resource_size(res), res-name)) {
ret = -ENOMEM;
- goto eclk;
+ goto emalloc;
We could also return directly here since the release_mem_region call should
be removed, as mentioned in Jingoos reply.
}
i2c-adap.owner = THIS_MODULE;
@@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device
*dev)
strlcpy(i2c-adap.name, pxa_i2c-i2c, sizeof(i2c-adap.name));
- i2c-clk = clk_get(dev-dev, NULL);
+ i2c-clk = devm_clk_get(dev-dev, NULL);
if (IS_ERR(i2c-clk)) {
ret = PTR_ERR(i2c-clk);
- goto eclk;
+ goto emalloc;
Same here.
}
- i2c-reg_base = ioremap(res-start, resource_size(res));
- if (!i2c-reg_base) {
+ i2c-reg_base = devm_ioremap_resource(adev-dev, res));
+ if (IS_ERR(i2c-reg_base)) {
ret = -EIO;
- goto eremap;
+ goto emalloc;
Same here.
}
i2c-reg_ibmr = i2c-reg_base + pxa_reg_layout[i2c_type].ibmr;
@@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device
*dev)
i2c-adap.algo = i2c_pxa_pio_algorithm;
} else {
i2c-adap.algo = i2c_pxa_algorithm;
- ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
- dev_name(dev-dev), i2c);
+ ret = devm_request_irq(dev-dev, irq, i2c_pxa_handler,
+ IRQF_SHARED, dev_name(dev-dev), i2c);
if (ret)
- goto ereqirq;
+ goto emalloc;
Same here.
}
i2c_pxa_reset(i2c);
@@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device
*dev)
eadapt:
if (!i2c-use_pio)
free_irq(irq, i2c);
The free_irq() call should also be removed since devm_request_irq() was
used.
-ereqirq:
- clk_disable_unprepare(i2c-clk);
I think we should keep the clk_disable_unprepare() call.
- iounmap(i2c-reg_base);
-eremap:
- clk_put(i2c-clk);
-eclk:
- kfree(i2c);
emalloc:
release_mem_region(res-start, resource_size(res));
+err_nothing_to_release:
return ret;
}
Best regards,
Emil Goode
Hi All!
Ok, so there is literally nothing to do release anymore.
A good system Devres, I understand you want everyone to use it.
I also think it is also better to return directly then.
But who decides, Wolfram?
Yes, Wolfram is the maintainer.
Personally I would do the devm_* convertion in a second patch.
Best
Re: [PATCH] Staging: rtl8192e: Fix potential NULL pointer dereference
Hello Greg, On Wed, Jul 02, 2014 at 09:33:34AM -0700, Greg Kroah-Hartman wrote: > On Wed, Jul 02, 2014 at 11:25:51AM +0200, Emil Goode wrote: > > We need to make sure the struct rtllib_device pointer ieee is not NULL > > after the goto rx_dropped label since it is dereferenced there. > > > > Signed-off-by: Emil Goode > > --- > > drivers/staging/rtl8192e/rtllib_rx.c |3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/rtl8192e/rtllib_rx.c > > b/drivers/staging/rtl8192e/rtllib_rx.c > > index 60de54c..7db3e74 100644 > > --- a/drivers/staging/rtl8192e/rtllib_rx.c > > +++ b/drivers/staging/rtl8192e/rtllib_rx.c > > @@ -1496,7 +1496,8 @@ int rtllib_rx(struct rtllib_device *ieee, struct > > sk_buff *skb, > > return ret; > > > > rx_dropped: > > - ieee->stats.rx_dropped++; > > + if (ieee) > > + ieee->stats.rx_dropped++; > > return 0; > > } > > EXPORT_SYMBOL(rtllib_rx); > > Is this something that is hitting users today in the tree, or is this > just a bug you found looking at the code? It's a static checker fix and I'm not aware of any impact on users. If you want I will resend with that information added? Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: rtl8192e: Fix potential NULL pointer dereference
We need to make sure the struct rtllib_device pointer ieee is not NULL after the goto rx_dropped label since it is dereferenced there. Signed-off-by: Emil Goode --- drivers/staging/rtl8192e/rtllib_rx.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c index 60de54c..7db3e74 100644 --- a/drivers/staging/rtl8192e/rtllib_rx.c +++ b/drivers/staging/rtl8192e/rtllib_rx.c @@ -1496,7 +1496,8 @@ int rtllib_rx(struct rtllib_device *ieee, struct sk_buff *skb, return ret; rx_dropped: - ieee->stats.rx_dropped++; + if (ieee) + ieee->stats.rx_dropped++; return 0; } EXPORT_SYMBOL(rtllib_rx); -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: rtl8192e: Fix potential NULL pointer dereference
We need to make sure the struct rtllib_device pointer ieee is not NULL after the goto rx_dropped label since it is dereferenced there. Signed-off-by: Emil Goode emilgo...@gmail.com --- drivers/staging/rtl8192e/rtllib_rx.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c index 60de54c..7db3e74 100644 --- a/drivers/staging/rtl8192e/rtllib_rx.c +++ b/drivers/staging/rtl8192e/rtllib_rx.c @@ -1496,7 +1496,8 @@ int rtllib_rx(struct rtllib_device *ieee, struct sk_buff *skb, return ret; rx_dropped: - ieee-stats.rx_dropped++; + if (ieee) + ieee-stats.rx_dropped++; return 0; } EXPORT_SYMBOL(rtllib_rx); -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: rtl8192e: Fix potential NULL pointer dereference
Hello Greg, On Wed, Jul 02, 2014 at 09:33:34AM -0700, Greg Kroah-Hartman wrote: On Wed, Jul 02, 2014 at 11:25:51AM +0200, Emil Goode wrote: We need to make sure the struct rtllib_device pointer ieee is not NULL after the goto rx_dropped label since it is dereferenced there. Signed-off-by: Emil Goode emilgo...@gmail.com --- drivers/staging/rtl8192e/rtllib_rx.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c index 60de54c..7db3e74 100644 --- a/drivers/staging/rtl8192e/rtllib_rx.c +++ b/drivers/staging/rtl8192e/rtllib_rx.c @@ -1496,7 +1496,8 @@ int rtllib_rx(struct rtllib_device *ieee, struct sk_buff *skb, return ret; rx_dropped: - ieee-stats.rx_dropped++; + if (ieee) + ieee-stats.rx_dropped++; return 0; } EXPORT_SYMBOL(rtllib_rx); Is this something that is hitting users today in the tree, or is this just a bug you found looking at the code? It's a static checker fix and I'm not aware of any impact on users. If you want I will resend with that information added? Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [SCSI] qla2xxx: Remove duplicate __iomem annotation
The device_reg_t type now has __iomem annotation.
Sparse is warning about this:
drivers/scsi/qla2xxx/qla_init.c:2040:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_iocb.c:477:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_iocb.c:1866:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_dbg.c:632:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_target.c:1435:22: warning: duplicate [noderef]
Introduced by commit:
f73cb695d3eccd171f03ed194e72d67732b17487
("[SCSI] qla2xxx: Add support for ISP2071.")
Signed-off-by: Emil Goode
---
drivers/scsi/qla2xxx/qla_dbg.c|2 +-
drivers/scsi/qla2xxx/qla_init.c |2 +-
drivers/scsi/qla2xxx/qla_iocb.c |4 ++--
drivers/scsi/qla2xxx/qla_target.c |2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index c72ee97b..47998b9 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -629,7 +629,7 @@ qla25xx_copy_mq(struct qla_hw_data *ha, void *ptr, uint32_t
**last_chain)
uint32_t cnt, que_idx;
uint8_t que_cnt;
struct qla2xxx_mq_chain *mq = ptr;
- device_reg_t __iomem *reg;
+ device_reg_t *reg;
if (!ha->mqenable || IS_QLA83XX(ha) || IS_QLA27XX(ha))
return ptr;
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index e218441..8d4e368 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2037,7 +2037,7 @@ void
qla24xx_config_rings(struct scsi_qla_host *vha)
{
struct qla_hw_data *ha = vha->hw;
- device_reg_t __iomem *reg = ISP_QUE_REG(ha, 0);
+ device_reg_t *reg = ISP_QUE_REG(ha, 0);
struct device_reg_2xxx __iomem *ioreg = >iobase->isp;
struct qla_msix_entry *msix;
struct init_cb_24xx *icb;
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 150529d..39ebf34 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -474,7 +474,7 @@ void
qla2x00_start_iocbs(struct scsi_qla_host *vha, struct req_que *req)
{
struct qla_hw_data *ha = vha->hw;
- device_reg_t __iomem *reg = ISP_QUE_REG(ha, req->id);
+ device_reg_t *reg = ISP_QUE_REG(ha, req->id);
if (IS_P3P_TYPE(ha)) {
qla82xx_start_iocbs(vha);
@@ -1863,7 +1863,7 @@ qla2x00_alloc_iocbs(scsi_qla_host_t *vha, srb_t *sp)
{
struct qla_hw_data *ha = vha->hw;
struct req_que *req = ha->req_q_map[0];
- device_reg_t __iomem *reg = ISP_QUE_REG(ha, req->id);
+ device_reg_t *reg = ISP_QUE_REG(ha, req->id);
uint32_t index, handle;
request_t *pkt;
uint16_t cnt, req_cnt;
diff --git a/drivers/scsi/qla2xxx/qla_target.c
b/drivers/scsi/qla2xxx/qla_target.c
index e632e14..6fc0702 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1432,7 +1432,7 @@ static int qlt_check_reserve_free_req(struct
scsi_qla_host *vha,
uint32_t req_cnt)
{
struct qla_hw_data *ha = vha->hw;
- device_reg_t __iomem *reg = ha->iobase;
+ device_reg_t *reg = ha->iobase;
uint32_t cnt;
if (vha->req->cnt < (req_cnt + 2)) {
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [SCSI] qla2xxx: Remove duplicate __iomem annotation
The device_reg_t type now has __iomem annotation.
Sparse is warning about this:
drivers/scsi/qla2xxx/qla_init.c:2040:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_iocb.c:477:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_iocb.c:1866:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_dbg.c:632:22: warning: duplicate [noderef]
drivers/scsi/qla2xxx/qla_target.c:1435:22: warning: duplicate [noderef]
Introduced by commit:
f73cb695d3eccd171f03ed194e72d67732b17487
([SCSI] qla2xxx: Add support for ISP2071.)
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/scsi/qla2xxx/qla_dbg.c|2 +-
drivers/scsi/qla2xxx/qla_init.c |2 +-
drivers/scsi/qla2xxx/qla_iocb.c |4 ++--
drivers/scsi/qla2xxx/qla_target.c |2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index c72ee97b..47998b9 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -629,7 +629,7 @@ qla25xx_copy_mq(struct qla_hw_data *ha, void *ptr, uint32_t
**last_chain)
uint32_t cnt, que_idx;
uint8_t que_cnt;
struct qla2xxx_mq_chain *mq = ptr;
- device_reg_t __iomem *reg;
+ device_reg_t *reg;
if (!ha-mqenable || IS_QLA83XX(ha) || IS_QLA27XX(ha))
return ptr;
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index e218441..8d4e368 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2037,7 +2037,7 @@ void
qla24xx_config_rings(struct scsi_qla_host *vha)
{
struct qla_hw_data *ha = vha-hw;
- device_reg_t __iomem *reg = ISP_QUE_REG(ha, 0);
+ device_reg_t *reg = ISP_QUE_REG(ha, 0);
struct device_reg_2xxx __iomem *ioreg = ha-iobase-isp;
struct qla_msix_entry *msix;
struct init_cb_24xx *icb;
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 150529d..39ebf34 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -474,7 +474,7 @@ void
qla2x00_start_iocbs(struct scsi_qla_host *vha, struct req_que *req)
{
struct qla_hw_data *ha = vha-hw;
- device_reg_t __iomem *reg = ISP_QUE_REG(ha, req-id);
+ device_reg_t *reg = ISP_QUE_REG(ha, req-id);
if (IS_P3P_TYPE(ha)) {
qla82xx_start_iocbs(vha);
@@ -1863,7 +1863,7 @@ qla2x00_alloc_iocbs(scsi_qla_host_t *vha, srb_t *sp)
{
struct qla_hw_data *ha = vha-hw;
struct req_que *req = ha-req_q_map[0];
- device_reg_t __iomem *reg = ISP_QUE_REG(ha, req-id);
+ device_reg_t *reg = ISP_QUE_REG(ha, req-id);
uint32_t index, handle;
request_t *pkt;
uint16_t cnt, req_cnt;
diff --git a/drivers/scsi/qla2xxx/qla_target.c
b/drivers/scsi/qla2xxx/qla_target.c
index e632e14..6fc0702 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1432,7 +1432,7 @@ static int qlt_check_reserve_free_req(struct
scsi_qla_host *vha,
uint32_t req_cnt)
{
struct qla_hw_data *ha = vha-hw;
- device_reg_t __iomem *reg = ha-iobase;
+ device_reg_t *reg = ha-iobase;
uint32_t cnt;
if (vha-req-cnt (req_cnt + 2)) {
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] [media] Cleanup line > 80 character violations
This cleans up some line over 80 character violations.
Signed-off-by: Emil Goode
---
drivers/media/dvb-frontends/stb6100_cfg.h | 12
drivers/media/dvb-frontends/tda8261_cfg.h |9 ++---
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/media/dvb-frontends/stb6100_cfg.h
b/drivers/media/dvb-frontends/stb6100_cfg.h
index 0e10ad89..6edc153 100644
--- a/drivers/media/dvb-frontends/stb6100_cfg.h
+++ b/drivers/media/dvb-frontends/stb6100_cfg.h
@@ -27,7 +27,8 @@ static int stb6100_get_frequency(struct dvb_frontend *fe, u32
*frequency)
int err = 0;
if (tuner_ops->get_state) {
- if ((err = tuner_ops->get_state(fe, DVBFE_TUNER_FREQUENCY,
_state)) < 0) {
+ err = tuner_ops->get_state(fe, DVBFE_TUNER_FREQUENCY, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
@@ -46,7 +47,8 @@ static int stb6100_set_frequency(struct dvb_frontend *fe, u32
frequency)
t_state.frequency = frequency;
if (tuner_ops->set_state) {
- if ((err = tuner_ops->set_state(fe, DVBFE_TUNER_FREQUENCY,
_state)) < 0) {
+ err = tuner_ops->set_state(fe, DVBFE_TUNER_FREQUENCY, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
@@ -62,7 +64,8 @@ static int stb6100_get_bandwidth(struct dvb_frontend *fe, u32
*bandwidth)
int err = 0;
if (tuner_ops->get_state) {
- if ((err = tuner_ops->get_state(fe, DVBFE_TUNER_BANDWIDTH,
_state)) < 0) {
+ err = tuner_ops->get_state(fe, DVBFE_TUNER_BANDWIDTH, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
@@ -81,7 +84,8 @@ static int stb6100_set_bandwidth(struct dvb_frontend *fe, u32
bandwidth)
t_state.bandwidth = bandwidth;
if (tuner_ops->set_state) {
- if ((err = tuner_ops->set_state(fe, DVBFE_TUNER_BANDWIDTH,
_state)) < 0) {
+ err = tuner_ops->set_state(fe, DVBFE_TUNER_BANDWIDTH, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
diff --git a/drivers/media/dvb-frontends/tda8261_cfg.h
b/drivers/media/dvb-frontends/tda8261_cfg.h
index 7de65c3..04a19e1 100644
--- a/drivers/media/dvb-frontends/tda8261_cfg.h
+++ b/drivers/media/dvb-frontends/tda8261_cfg.h
@@ -25,7 +25,8 @@ static int tda8261_get_frequency(struct dvb_frontend *fe, u32
*frequency)
int err = 0;
if (tuner_ops->get_state) {
- if ((err = tuner_ops->get_state(fe, DVBFE_TUNER_FREQUENCY,
_state)) < 0) {
+ err = tuner_ops->get_state(fe, DVBFE_TUNER_FREQUENCY, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
@@ -45,7 +46,8 @@ static int tda8261_set_frequency(struct dvb_frontend *fe, u32
frequency)
t_state.frequency = frequency;
if (tuner_ops->set_state) {
- if ((err = tuner_ops->set_state(fe, DVBFE_TUNER_FREQUENCY,
_state)) < 0) {
+ err = tuner_ops->set_state(fe, DVBFE_TUNER_FREQUENCY, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
@@ -62,7 +64,8 @@ static int tda8261_get_bandwidth(struct dvb_frontend *fe, u32
*bandwidth)
int err = 0;
if (tuner_ops->get_state) {
- if ((err = tuner_ops->get_state(fe, DVBFE_TUNER_BANDWIDTH,
_state)) < 0) {
+ err = tuner_ops->get_state(fe, DVBFE_TUNER_BANDWIDTH, _state);
+ if (err < 0) {
printk("%s: Invalid parameter\n", __func__);
return err;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] [media] Remove checks of struct member addresses
This removes checks of struct member addresses since they likely result
in the condition always being true. Also in the stb6100_get_bandwidth
and tda8261_get_bandwidth functions the pointers frontend_ops and
tuner_ops are assigned the same addresses twice.
Signed-off-by: Emil Goode
---
drivers/media/dvb-frontends/stb6100_cfg.h | 30 +++-
drivers/media/dvb-frontends/stb6100_proc.h | 34
drivers/media/dvb-frontends/stv0367.c |9 ++--
drivers/media/dvb-frontends/tda8261_cfg.h | 21 -
4 files changed, 25 insertions(+), 69 deletions(-)
diff --git a/drivers/media/dvb-frontends/stb6100_cfg.h
b/drivers/media/dvb-frontends/stb6100_cfg.h
index 6314d18..0e10ad89 100644
--- a/drivers/media/dvb-frontends/stb6100_cfg.h
+++ b/drivers/media/dvb-frontends/stb6100_cfg.h
@@ -21,15 +21,11 @@
static int stb6100_get_frequency(struct dvb_frontend *fe, u32 *frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = >ops;
+ struct dvb_tuner_ops*tuner_ops = _ops->tuner_ops;
struct tuner_state t_state;
int err = 0;
- if (>ops)
- frontend_ops = >ops;
- if (_ops->tuner_ops)
- tuner_ops = _ops->tuner_ops;
if (tuner_ops->get_state) {
if ((err = tuner_ops->get_state(fe, DVBFE_TUNER_FREQUENCY,
_state)) < 0) {
printk("%s: Invalid parameter\n", __func__);
@@ -42,16 +38,13 @@ static int stb6100_get_frequency(struct dvb_frontend *fe,
u32 *frequency)
static int stb6100_set_frequency(struct dvb_frontend *fe, u32 frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = >ops;
+ struct dvb_tuner_ops*tuner_ops = _ops->tuner_ops;
struct tuner_state t_state;
int err = 0;
t_state.frequency = frequency;
- if (>ops)
- frontend_ops = >ops;
- if (_ops->tuner_ops)
- tuner_ops = _ops->tuner_ops;
+
if (tuner_ops->set_state) {
if ((err = tuner_ops->set_state(fe, DVBFE_TUNER_FREQUENCY,
_state)) < 0) {
printk("%s: Invalid parameter\n", __func__);
@@ -68,10 +61,6 @@ static int stb6100_get_bandwidth(struct dvb_frontend *fe,
u32 *bandwidth)
struct tuner_state t_state;
int err = 0;
- if (>ops)
- frontend_ops = >ops;
- if (_ops->tuner_ops)
- tuner_ops = _ops->tuner_ops;
if (tuner_ops->get_state) {
if ((err = tuner_ops->get_state(fe, DVBFE_TUNER_BANDWIDTH,
_state)) < 0) {
printk("%s: Invalid parameter\n", __func__);
@@ -84,16 +73,13 @@ static int stb6100_get_bandwidth(struct dvb_frontend *fe,
u32 *bandwidth)
static int stb6100_set_bandwidth(struct dvb_frontend *fe, u32 bandwidth)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = >ops;
+ struct dvb_tuner_ops*tuner_ops = _ops->tuner_ops;
struct tuner_state t_state;
int err = 0;
t_state.bandwidth = bandwidth;
- if (>ops)
- frontend_ops = >ops;
- if (_ops->tuner_ops)
- tuner_ops = _ops->tuner_ops;
+
if (tuner_ops->set_state) {
if ((err = tuner_ops->set_state(fe, DVBFE_TUNER_BANDWIDTH,
_state)) < 0) {
printk("%s: Invalid parameter\n", __func__);
diff --git a/drivers/media/dvb-frontends/stb6100_proc.h
b/drivers/media/dvb-frontends/stb6100_proc.h
index 112163a..bd8a0ec 100644
--- a/drivers/media/dvb-frontends/stb6100_proc.h
+++ b/drivers/media/dvb-frontends/stb6100_proc.h
@@ -19,15 +19,11 @@
static int stb6100_get_freq(struct dvb_frontend *fe, u32 *frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = >ops;
+ struct dvb_tuner_ops*tuner_ops = _ops->tuner_ops;
struct tuner_state state;
int err = 0;
- if (>ops)
- frontend_ops = >ops;
- if (_ops->tuner_ops)
- tuner_ops = _ops->tuner_ops;
if (tuner_ops->get_state) {
if (frontend_ops->i2c_gate_ctrl)
frontend_ops->i2c_gate_ctrl(fe, 1);
@@ -49,16 +45,13 @@ static int stb6100_get_freq(struct dvb_frontend *fe, u32
*frequency)
static int stb6100_set_freq(struct dvb_frontend *fe, u32 frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner
[PATCH 2/2] [media] Cleanup line 80 character violations
This cleans up some line over 80 character violations.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/media/dvb-frontends/stb6100_cfg.h | 12
drivers/media/dvb-frontends/tda8261_cfg.h |9 ++---
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/media/dvb-frontends/stb6100_cfg.h
b/drivers/media/dvb-frontends/stb6100_cfg.h
index 0e10ad89..6edc153 100644
--- a/drivers/media/dvb-frontends/stb6100_cfg.h
+++ b/drivers/media/dvb-frontends/stb6100_cfg.h
@@ -27,7 +27,8 @@ static int stb6100_get_frequency(struct dvb_frontend *fe, u32
*frequency)
int err = 0;
if (tuner_ops-get_state) {
- if ((err = tuner_ops-get_state(fe, DVBFE_TUNER_FREQUENCY,
t_state)) 0) {
+ err = tuner_ops-get_state(fe, DVBFE_TUNER_FREQUENCY, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
@@ -46,7 +47,8 @@ static int stb6100_set_frequency(struct dvb_frontend *fe, u32
frequency)
t_state.frequency = frequency;
if (tuner_ops-set_state) {
- if ((err = tuner_ops-set_state(fe, DVBFE_TUNER_FREQUENCY,
t_state)) 0) {
+ err = tuner_ops-set_state(fe, DVBFE_TUNER_FREQUENCY, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
@@ -62,7 +64,8 @@ static int stb6100_get_bandwidth(struct dvb_frontend *fe, u32
*bandwidth)
int err = 0;
if (tuner_ops-get_state) {
- if ((err = tuner_ops-get_state(fe, DVBFE_TUNER_BANDWIDTH,
t_state)) 0) {
+ err = tuner_ops-get_state(fe, DVBFE_TUNER_BANDWIDTH, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
@@ -81,7 +84,8 @@ static int stb6100_set_bandwidth(struct dvb_frontend *fe, u32
bandwidth)
t_state.bandwidth = bandwidth;
if (tuner_ops-set_state) {
- if ((err = tuner_ops-set_state(fe, DVBFE_TUNER_BANDWIDTH,
t_state)) 0) {
+ err = tuner_ops-set_state(fe, DVBFE_TUNER_BANDWIDTH, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
diff --git a/drivers/media/dvb-frontends/tda8261_cfg.h
b/drivers/media/dvb-frontends/tda8261_cfg.h
index 7de65c3..04a19e1 100644
--- a/drivers/media/dvb-frontends/tda8261_cfg.h
+++ b/drivers/media/dvb-frontends/tda8261_cfg.h
@@ -25,7 +25,8 @@ static int tda8261_get_frequency(struct dvb_frontend *fe, u32
*frequency)
int err = 0;
if (tuner_ops-get_state) {
- if ((err = tuner_ops-get_state(fe, DVBFE_TUNER_FREQUENCY,
t_state)) 0) {
+ err = tuner_ops-get_state(fe, DVBFE_TUNER_FREQUENCY, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
@@ -45,7 +46,8 @@ static int tda8261_set_frequency(struct dvb_frontend *fe, u32
frequency)
t_state.frequency = frequency;
if (tuner_ops-set_state) {
- if ((err = tuner_ops-set_state(fe, DVBFE_TUNER_FREQUENCY,
t_state)) 0) {
+ err = tuner_ops-set_state(fe, DVBFE_TUNER_FREQUENCY, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
@@ -62,7 +64,8 @@ static int tda8261_get_bandwidth(struct dvb_frontend *fe, u32
*bandwidth)
int err = 0;
if (tuner_ops-get_state) {
- if ((err = tuner_ops-get_state(fe, DVBFE_TUNER_BANDWIDTH,
t_state)) 0) {
+ err = tuner_ops-get_state(fe, DVBFE_TUNER_BANDWIDTH, t_state);
+ if (err 0) {
printk(%s: Invalid parameter\n, __func__);
return err;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] [media] Remove checks of struct member addresses
This removes checks of struct member addresses since they likely result
in the condition always being true. Also in the stb6100_get_bandwidth
and tda8261_get_bandwidth functions the pointers frontend_ops and
tuner_ops are assigned the same addresses twice.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/media/dvb-frontends/stb6100_cfg.h | 30 +++-
drivers/media/dvb-frontends/stb6100_proc.h | 34
drivers/media/dvb-frontends/stv0367.c |9 ++--
drivers/media/dvb-frontends/tda8261_cfg.h | 21 -
4 files changed, 25 insertions(+), 69 deletions(-)
diff --git a/drivers/media/dvb-frontends/stb6100_cfg.h
b/drivers/media/dvb-frontends/stb6100_cfg.h
index 6314d18..0e10ad89 100644
--- a/drivers/media/dvb-frontends/stb6100_cfg.h
+++ b/drivers/media/dvb-frontends/stb6100_cfg.h
@@ -21,15 +21,11 @@
static int stb6100_get_frequency(struct dvb_frontend *fe, u32 *frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = fe-ops;
+ struct dvb_tuner_ops*tuner_ops = frontend_ops-tuner_ops;
struct tuner_state t_state;
int err = 0;
- if (fe-ops)
- frontend_ops = fe-ops;
- if (frontend_ops-tuner_ops)
- tuner_ops = frontend_ops-tuner_ops;
if (tuner_ops-get_state) {
if ((err = tuner_ops-get_state(fe, DVBFE_TUNER_FREQUENCY,
t_state)) 0) {
printk(%s: Invalid parameter\n, __func__);
@@ -42,16 +38,13 @@ static int stb6100_get_frequency(struct dvb_frontend *fe,
u32 *frequency)
static int stb6100_set_frequency(struct dvb_frontend *fe, u32 frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = fe-ops;
+ struct dvb_tuner_ops*tuner_ops = frontend_ops-tuner_ops;
struct tuner_state t_state;
int err = 0;
t_state.frequency = frequency;
- if (fe-ops)
- frontend_ops = fe-ops;
- if (frontend_ops-tuner_ops)
- tuner_ops = frontend_ops-tuner_ops;
+
if (tuner_ops-set_state) {
if ((err = tuner_ops-set_state(fe, DVBFE_TUNER_FREQUENCY,
t_state)) 0) {
printk(%s: Invalid parameter\n, __func__);
@@ -68,10 +61,6 @@ static int stb6100_get_bandwidth(struct dvb_frontend *fe,
u32 *bandwidth)
struct tuner_state t_state;
int err = 0;
- if (fe-ops)
- frontend_ops = fe-ops;
- if (frontend_ops-tuner_ops)
- tuner_ops = frontend_ops-tuner_ops;
if (tuner_ops-get_state) {
if ((err = tuner_ops-get_state(fe, DVBFE_TUNER_BANDWIDTH,
t_state)) 0) {
printk(%s: Invalid parameter\n, __func__);
@@ -84,16 +73,13 @@ static int stb6100_get_bandwidth(struct dvb_frontend *fe,
u32 *bandwidth)
static int stb6100_set_bandwidth(struct dvb_frontend *fe, u32 bandwidth)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = fe-ops;
+ struct dvb_tuner_ops*tuner_ops = frontend_ops-tuner_ops;
struct tuner_state t_state;
int err = 0;
t_state.bandwidth = bandwidth;
- if (fe-ops)
- frontend_ops = fe-ops;
- if (frontend_ops-tuner_ops)
- tuner_ops = frontend_ops-tuner_ops;
+
if (tuner_ops-set_state) {
if ((err = tuner_ops-set_state(fe, DVBFE_TUNER_BANDWIDTH,
t_state)) 0) {
printk(%s: Invalid parameter\n, __func__);
diff --git a/drivers/media/dvb-frontends/stb6100_proc.h
b/drivers/media/dvb-frontends/stb6100_proc.h
index 112163a..bd8a0ec 100644
--- a/drivers/media/dvb-frontends/stb6100_proc.h
+++ b/drivers/media/dvb-frontends/stb6100_proc.h
@@ -19,15 +19,11 @@
static int stb6100_get_freq(struct dvb_frontend *fe, u32 *frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct dvb_frontend_ops *frontend_ops = fe-ops;
+ struct dvb_tuner_ops*tuner_ops = frontend_ops-tuner_ops;
struct tuner_state state;
int err = 0;
- if (fe-ops)
- frontend_ops = fe-ops;
- if (frontend_ops-tuner_ops)
- tuner_ops = frontend_ops-tuner_ops;
if (tuner_ops-get_state) {
if (frontend_ops-i2c_gate_ctrl)
frontend_ops-i2c_gate_ctrl(fe, 1);
@@ -49,16 +45,13 @@ static int stb6100_get_freq(struct dvb_frontend *fe, u32
*frequency)
static int stb6100_set_freq(struct dvb_frontend *fe, u32 frequency)
{
- struct dvb_frontend_ops *frontend_ops = NULL;
- struct dvb_tuner_ops*tuner_ops = NULL;
+ struct
[PATCH] video: Remove kfree call since devm_kzalloc() is used
We use devm_kzalloc() to allocate memory for the struct vt8500lcd_info pointer fbi, so there is no need to free it in vt8500lcd_remove(). Signed-off-by: Emil Goode --- drivers/video/fbdev/vt8500lcdfb.c |2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/video/fbdev/vt8500lcdfb.c b/drivers/video/fbdev/vt8500lcdfb.c index a8f2b28..a1134c3 100644 --- a/drivers/video/fbdev/vt8500lcdfb.c +++ b/drivers/video/fbdev/vt8500lcdfb.c @@ -474,8 +474,6 @@ static int vt8500lcd_remove(struct platform_device *pdev) res = platform_get_resource(pdev, IORESOURCE_MEM, 0); release_mem_region(res->start, resource_size(res)); - kfree(fbi); - return 0; } -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] video: Remove kfree call since devm_kzalloc() is used
We use devm_kzalloc() to allocate memory for the struct vt8500lcd_info pointer fbi, so there is no need to free it in vt8500lcd_remove(). Signed-off-by: Emil Goode emilgo...@gmail.com --- drivers/video/fbdev/vt8500lcdfb.c |2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/video/fbdev/vt8500lcdfb.c b/drivers/video/fbdev/vt8500lcdfb.c index a8f2b28..a1134c3 100644 --- a/drivers/video/fbdev/vt8500lcdfb.c +++ b/drivers/video/fbdev/vt8500lcdfb.c @@ -474,8 +474,6 @@ static int vt8500lcd_remove(struct platform_device *pdev) res = platform_get_resource(pdev, IORESOURCE_MEM, 0); release_mem_region(res-start, resource_size(res)); - kfree(fbi); - return 0; } -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb: usb5303: make use of uninitialized err variable
The variable err is not initialized here, this patch uses it
to store an eventual error value from devm_clk_get().
Signed-off-by: Emil Goode
---
drivers/usb/misc/usb3503.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
index f43c619..c86c3fa 100644
--- a/drivers/usb/misc/usb3503.c
+++ b/drivers/usb/misc/usb3503.c
@@ -191,9 +191,13 @@ static int usb3503_probe(struct usb3503 *hub)
hub->port_off_mask = 0;
clk = devm_clk_get(dev, "refclk");
- if (IS_ERR(clk) && PTR_ERR(clk) != -ENOENT) {
- dev_err(dev, "unable to request refclk (%d)\n", err);
- return PTR_ERR(clk);
+ if (IS_ERR(clk)) {
+ err = PTR_ERR(clk);
+ if (err != -ENOENT) {
+ dev_err(dev, "unable to request refclk (%d)\n",
+ err);
+ return err;
+ }
}
if (!IS_ERR(clk)) {
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] NFC: st21nfca: Remove double assignment of .owner in struct device_driver
The .owner member of struct device_driver is assigned THIS_MODULE twice.
Introduced by:
commit c44cb2edd01ca31471d9385f0895891b006ab904
("NFC: dts: st21nfca: Add device-tree (Open Firmware) support to st21nfca")
Signed-off-by: Emil Goode
---
drivers/nfc/st21nfca/i2c.c |1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c
index 3f954ed..95942ca 100644
--- a/drivers/nfc/st21nfca/i2c.c
+++ b/drivers/nfc/st21nfca/i2c.c
@@ -710,7 +710,6 @@ static struct i2c_driver st21nfca_hci_i2c_driver = {
.driver = {
.owner = THIS_MODULE,
.name = ST21NFCA_HCI_I2C_DRIVER_NAME,
- .owner = THIS_MODULE,
.of_match_table = of_match_ptr(of_st21nfca_i2c_match),
},
.probe = st21nfca_hci_i2c_probe,
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] stmmac: Remove spin_lock call in stmmac_get_pauseparam()
The following patch removed unnecessary spin_lock/unlock calls in ethtool_ops callback functions. In the second and final version of the patch one spin_lock call was left behind. commit cab6715c3e8029e98b0b5d4056ceda007c0f6380 Author: Yang Wei Date: Sun May 25 09:53:44 2014 +0800 net: driver: stmicro: Remove some useless the lock protection This introduced the following sparse warning: drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:424:1: warning: context imbalance in 'stmmac_get_pauseparam' - different lock contexts for basic block Signed-off-by: Emil Goode --- Hello, Some spin_lock/unlock calls where also left behind in stmmac_ethtool_setsettings(), I'm not sure if it was intentional? Best regards, Emil Goode drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c |2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 7892666..c62e67f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -429,8 +429,6 @@ stmmac_get_pauseparam(struct net_device *netdev, if (priv->pcs) /* FIXME */ return; - spin_lock(>lock); - pause->rx_pause = 0; pause->tx_pause = 0; pause->autoneg = priv->phydev->autoneg; -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] stmmac: Remove spin_lock call in stmmac_get_pauseparam()
The following patch removed unnecessary spin_lock/unlock calls in ethtool_ops callback functions. In the second and final version of the patch one spin_lock call was left behind. commit cab6715c3e8029e98b0b5d4056ceda007c0f6380 Author: Yang Wei wei.y...@windriver.com Date: Sun May 25 09:53:44 2014 +0800 net: driver: stmicro: Remove some useless the lock protection This introduced the following sparse warning: drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:424:1: warning: context imbalance in 'stmmac_get_pauseparam' - different lock contexts for basic block Signed-off-by: Emil Goode emilgo...@gmail.com --- Hello, Some spin_lock/unlock calls where also left behind in stmmac_ethtool_setsettings(), I'm not sure if it was intentional? Best regards, Emil Goode drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c |2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 7892666..c62e67f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -429,8 +429,6 @@ stmmac_get_pauseparam(struct net_device *netdev, if (priv-pcs) /* FIXME */ return; - spin_lock(priv-lock); - pause-rx_pause = 0; pause-tx_pause = 0; pause-autoneg = priv-phydev-autoneg; -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] NFC: st21nfca: Remove double assignment of .owner in struct device_driver
The .owner member of struct device_driver is assigned THIS_MODULE twice.
Introduced by:
commit c44cb2edd01ca31471d9385f0895891b006ab904
(NFC: dts: st21nfca: Add device-tree (Open Firmware) support to st21nfca)
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/nfc/st21nfca/i2c.c |1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/nfc/st21nfca/i2c.c b/drivers/nfc/st21nfca/i2c.c
index 3f954ed..95942ca 100644
--- a/drivers/nfc/st21nfca/i2c.c
+++ b/drivers/nfc/st21nfca/i2c.c
@@ -710,7 +710,6 @@ static struct i2c_driver st21nfca_hci_i2c_driver = {
.driver = {
.owner = THIS_MODULE,
.name = ST21NFCA_HCI_I2C_DRIVER_NAME,
- .owner = THIS_MODULE,
.of_match_table = of_match_ptr(of_st21nfca_i2c_match),
},
.probe = st21nfca_hci_i2c_probe,
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb: usb5303: make use of uninitialized err variable
The variable err is not initialized here, this patch uses it
to store an eventual error value from devm_clk_get().
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/usb/misc/usb3503.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
index f43c619..c86c3fa 100644
--- a/drivers/usb/misc/usb3503.c
+++ b/drivers/usb/misc/usb3503.c
@@ -191,9 +191,13 @@ static int usb3503_probe(struct usb3503 *hub)
hub-port_off_mask = 0;
clk = devm_clk_get(dev, refclk);
- if (IS_ERR(clk) PTR_ERR(clk) != -ENOENT) {
- dev_err(dev, unable to request refclk (%d)\n, err);
- return PTR_ERR(clk);
+ if (IS_ERR(clk)) {
+ err = PTR_ERR(clk);
+ if (err != -ENOENT) {
+ dev_err(dev, unable to request refclk (%d)\n,
+ err);
+ return err;
+ }
}
if (!IS_ERR(clk)) {
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] mfd: fix unintentional NULL check in menelaus_set_voltage()
The struct menelaus_vtg pointer vtg cannot be NULL here
so the condition is never true and if it ever was true
it would lead to a NULL pointer dereference when we goto
label set_voltage.
Before the below patch was applied the code was:
if (vtg == 0)
The intention was to check if vtg_val is 0.
commit 59a9f7a32adf6537b4e4db8ca204eeb77d7a634e
("mfd: menelaus: Use NULL instead of 0")
Signed-off-by: Emil Goode
---
v2: Changed (vtg_val == 0) to (!vtg_val)
drivers/mfd/menelaus.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c
index ad25bfa..40df76a 100644
--- a/drivers/mfd/menelaus.c
+++ b/drivers/mfd/menelaus.c
@@ -466,7 +466,7 @@ static int menelaus_set_voltage(const struct menelaus_vtg
*vtg, int mV,
struct i2c_client *c = the_menelaus->client;
mutex_lock(_menelaus->lock);
- if (!vtg)
+ if (!vtg_val)
goto set_voltage;
ret = menelaus_read_reg(vtg->vtg_reg);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: fix unintentional NULL check in menelaus_set_voltage()
Hello,
On Wed, May 28, 2014 at 04:20:11PM +0100, Lee Jones wrote:
> On Wed, 28 May 2014, Emil Goode wrote:
>
> > The struct menelaus_vtg pointer vtg cannot be NULL here
> > so the condition is never true and if it ever was true
> > it would lead to a NULL pointer dereference when we goto
> > label set_voltage.
> >
> > Before the below patch was applied the code was:
> >
> > if (vtg == 0)
> >
> > The intention was to check if vtg_val is 0.
> >
> > commit 59a9f7a32adf6537b4e4db8ca204eeb77d7a634e
> > ("mfd: menelaus: Use NULL instead of 0")
> >
> > Signed-off-by: Emil Goode
> > ---
> > Hello,
> >
> > This is only build tested, it would be good to get a comment
> > from someone who is familiar with this code.
> >
> > Found using coccinelle.
> >
> > Best regards,
> >
> > Emil Goode
> >
> > drivers/mfd/menelaus.c |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c
> > index ad25bfa..4859597 100644
> > --- a/drivers/mfd/menelaus.c
> > +++ b/drivers/mfd/menelaus.c
> > @@ -466,7 +466,7 @@ static int menelaus_set_voltage(const struct
> > menelaus_vtg *vtg, int mV,
> > struct i2c_client *c = the_menelaus->client;
> >
> > mutex_lock(_menelaus->lock);
> > - if (!vtg)
> > + if (vtg_val == 0)
>
> I would prefer:
>
> if (!vtg_val)
Ok, I will resend.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mfd: fix unintentional NULL check in menelaus_set_voltage()
The struct menelaus_vtg pointer vtg cannot be NULL here
so the condition is never true and if it ever was true
it would lead to a NULL pointer dereference when we goto
label set_voltage.
Before the below patch was applied the code was:
if (vtg == 0)
The intention was to check if vtg_val is 0.
commit 59a9f7a32adf6537b4e4db8ca204eeb77d7a634e
("mfd: menelaus: Use NULL instead of 0")
Signed-off-by: Emil Goode
---
Hello,
This is only build tested, it would be good to get a comment
from someone who is familiar with this code.
Found using coccinelle.
Best regards,
Emil Goode
drivers/mfd/menelaus.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c
index ad25bfa..4859597 100644
--- a/drivers/mfd/menelaus.c
+++ b/drivers/mfd/menelaus.c
@@ -466,7 +466,7 @@ static int menelaus_set_voltage(const struct menelaus_vtg
*vtg, int mV,
struct i2c_client *c = the_menelaus->client;
mutex_lock(_menelaus->lock);
- if (!vtg)
+ if (vtg_val == 0)
goto set_voltage;
ret = menelaus_read_reg(vtg->vtg_reg);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mfd: fix unintentional NULL check in menelaus_set_voltage()
The struct menelaus_vtg pointer vtg cannot be NULL here so the condition is never true and if it ever was true it would lead to a NULL pointer dereference when we goto label set_voltage. Before the below patch was applied the code was: if (vtg == 0) The intention was to check if vtg_val is 0. commit 59a9f7a32adf6537b4e4db8ca204eeb77d7a634e (mfd: menelaus: Use NULL instead of 0) Signed-off-by: Emil Goode emilgo...@gmail.com --- Hello, This is only build tested, it would be good to get a comment from someone who is familiar with this code. Found using coccinelle. Best regards, Emil Goode drivers/mfd/menelaus.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c index ad25bfa..4859597 100644 --- a/drivers/mfd/menelaus.c +++ b/drivers/mfd/menelaus.c @@ -466,7 +466,7 @@ static int menelaus_set_voltage(const struct menelaus_vtg *vtg, int mV, struct i2c_client *c = the_menelaus-client; mutex_lock(the_menelaus-lock); - if (!vtg) + if (vtg_val == 0) goto set_voltage; ret = menelaus_read_reg(vtg-vtg_reg); -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: fix unintentional NULL check in menelaus_set_voltage()
Hello, On Wed, May 28, 2014 at 04:20:11PM +0100, Lee Jones wrote: On Wed, 28 May 2014, Emil Goode wrote: The struct menelaus_vtg pointer vtg cannot be NULL here so the condition is never true and if it ever was true it would lead to a NULL pointer dereference when we goto label set_voltage. Before the below patch was applied the code was: if (vtg == 0) The intention was to check if vtg_val is 0. commit 59a9f7a32adf6537b4e4db8ca204eeb77d7a634e (mfd: menelaus: Use NULL instead of 0) Signed-off-by: Emil Goode emilgo...@gmail.com --- Hello, This is only build tested, it would be good to get a comment from someone who is familiar with this code. Found using coccinelle. Best regards, Emil Goode drivers/mfd/menelaus.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c index ad25bfa..4859597 100644 --- a/drivers/mfd/menelaus.c +++ b/drivers/mfd/menelaus.c @@ -466,7 +466,7 @@ static int menelaus_set_voltage(const struct menelaus_vtg *vtg, int mV, struct i2c_client *c = the_menelaus-client; mutex_lock(the_menelaus-lock); - if (!vtg) + if (vtg_val == 0) I would prefer: if (!vtg_val) Ok, I will resend. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] mfd: fix unintentional NULL check in menelaus_set_voltage()
The struct menelaus_vtg pointer vtg cannot be NULL here so the condition is never true and if it ever was true it would lead to a NULL pointer dereference when we goto label set_voltage. Before the below patch was applied the code was: if (vtg == 0) The intention was to check if vtg_val is 0. commit 59a9f7a32adf6537b4e4db8ca204eeb77d7a634e (mfd: menelaus: Use NULL instead of 0) Signed-off-by: Emil Goode emilgo...@gmail.com --- v2: Changed (vtg_val == 0) to (!vtg_val) drivers/mfd/menelaus.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c index ad25bfa..40df76a 100644 --- a/drivers/mfd/menelaus.c +++ b/drivers/mfd/menelaus.c @@ -466,7 +466,7 @@ static int menelaus_set_voltage(const struct menelaus_vtg *vtg, int mV, struct i2c_client *c = the_menelaus-client; mutex_lock(the_menelaus-lock); - if (!vtg) + if (!vtg_val) goto set_voltage; ret = menelaus_read_reg(vtg-vtg_reg); -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] driver core/platform: don't leak memory allocated for dma_mask
Hello,
On Mon, May 26, 2014 at 10:30:46PM +0300, Dan Carpenter wrote:
> On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote:
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 9e9227e..dd1fa07 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -166,6 +166,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
> > struct platform_object {
> > struct platform_device pdev;
> > char name[1];
> > + u64 dma_mask;
> > };
>
> Heh. No this doesn't work as patch #1. You have to have name at the
> end of the struct.
Yes I missed that one, obviously the order is important here.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] driver core/platform: don't leak memory allocated for dma_mask
Hello Russell, On Mon, May 26, 2014 at 05:51:05PM +0100, Russell King - ARM Linux wrote: > On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: > > @@ -211,6 +215,7 @@ struct platform_device *platform_device_alloc(const > > char *name, int id) > > strcpy(pa->name, name); > > pa->pdev.name = pa->name; > > pa->pdev.id = id; > > + pa->pdev.dev.dma_mask = >dma_mask; > > There is code in the kernel which, rightly or wrongly, checks whether > dev->dma_mask is NULL to determine whether the device can do any kind > of DMA. The above results in devices allocated via this interface > always having this member set, which is a change of core kernel behaviour. > > How sure are you that this will not break anything? Thank you for pointing this out, considering the number of calls made to platform_device_alloc it would be easy to miss an occurrance of this problem. I would say that the risk heavily outweighs the gain and it's better to not apply this series. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] driver core/platform: make .name of struct platform_object a C99 flexible array
From: Yann Droneaud
The changes made by this patch was originally part of the patch available
in the below link. It was requested that the change to .name was made in
a separate patch.
http://lkml.kernel.org/r/1390817152-30898-1-git-send-email-ydrone...@opteya.com
It's a pity that 7 bytes are wasted at the end of struct platform_object
in the form of padding after name field: unfortunately this padding is not
used when allocating the memory to hold the name.
By making .name of struct platform_object a C99 flexible array member
(eg. a zero sized array), padding at the end of the structure is removed
making the storage for .dma_mask almost for free.
To handle this change, memory allocation is updated to take care of
allocating an additional byte for the NUL terminating character.
Built on Debian jessie, using GCC 4.8, for ARM and x86_64 architectures,
the size of the object file and the data structure layout are updated
as follows, produced with commands:
$ size drivers/base/platform.o
$ pahole drivers/base/platform.o \
--recursive \
--class_name device,pdev_archdata,platform_device,platform_object
-- size+pahole_3.15-rc6_ARM
++ size+pahole_3.15-rc6_ARM+
@@ -2,7 +2,7 @@
text data bss dec hex filename
- 6482 1008 874981d4a drivers/base/platform.o
+ 6486 1008 875021d4e drivers/base/platform.o
@@ -91,15 +91,10 @@ struct platform_object {
/* XXX last struct has 4 bytes of padding */
/* --- cacheline 12 boundary (768 bytes) was 40 bytes ago --- */
- char name[1]; /* 808 1 */
+ u64dma_mask;/* 808 8 */
+ char name[0]; /* 816 0 */
- /* XXX 7 bytes hole, try to pack */
- u64dma_mask;/* 816 8 */
- /* size: 824, cachelines: 13, members: 3 */
- /* sum members: 817, holes: 1, sum holes: 7 */
+ /* size: 816, cachelines: 13, members: 3 */
/* paddings: 1, sum paddings: 4 */
- /* last cacheline: 56 bytes */
+ /* last cacheline: 48 bytes */
};
(Note that on x86 the size command didn't show any difference)
-- size+pahole_3.15-rc6_x86_64
++ size+pahole_3.15-rc6_x86_64+
@@ -93,13 +93,10 @@ struct platform_device {
struct platform_object {
struct platform_device pdev;/* 0 1440 */
/* --- cacheline 22 boundary (1408 bytes) was 32 bytes ago --- */
- char name[1]; /* 1440 1 */
+ u64dma_mask;/* 1440 8 */
+ char name[0]; /* 1448 0 */
- /* XXX 7 bytes hole, try to pack */
- u64dma_mask;/* 1448 8 */
- /* size: 1456, cachelines: 23, members: 3 */
- /* sum members: 1449, holes: 1, sum holes: 7 */
- /* last cacheline: 48 bytes */
+ /* size: 1448, cachelines: 23, members: 3 */
+ /* last cacheline: 40 bytes */
};
Cc: Yann Droneaud
Cc: Uwe Kleine-König
Cc: Dmitry Torokhov
Cc: Greg Kroah-Hartman
Signed-off-by: Yann Droneaud
[Emil: split the original patch, updated changelog and
generated new data structure layout info]
Signed-off-by: Emil Goode
---
drivers/base/platform.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dd1fa07..ba98219 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -165,8 +165,8 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
- char name[1];
u64 dma_mask;
+ char name[];
};
/**
@@ -210,7 +210,7 @@ struct platform_device *platform_device_alloc(const char
*name, int id)
{
struct platform_object *pa;
- pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+ pa = kzalloc(sizeof(*pa) + strlen(name) + 1, GFP_KERNEL);
if (pa) {
strcpy(pa->name, name);
pa->pdev.name = pa->name;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] ARM: imx: don't allocate memory for .dma_mask of struct device
The first patch in this series changed the platform_device_alloc
function to assign the u64 .dma_mask pointer of struct device to
the address of a u64 .dma_mask member of struct platform_object.
This means that we no longer should allocate memory for the .dma_mask
pointer of struct device when using platform_device_alloc() and the
u64 .dma_mask of struct platform_object that it points to will be freed
when the release callback function platform_device_release is called.
Signed-off-by: Emil Goode
---
arch/arm/mach-imx/devices/platform-ipu-core.c |5 -
1 file changed, 5 deletions(-)
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index 6bd7c3f..4499a7d 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -79,10 +79,6 @@ struct platform_device *__init imx_alloc_mx3_camera(
if (!pdev)
return ERR_PTR(-ENOMEM);
- pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
*pdev->dev.dma_mask = DMA_BIT_MASK(32);
pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
@@ -96,7 +92,6 @@ struct platform_device *__init imx_alloc_mx3_camera(
ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
if (ret) {
err:
- kfree(pdev->dev.dma_mask);
platform_device_put(pdev);
return ERR_PTR(-ENODEV);
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] driver core/platform: don't leak memory allocated for dma_mask
From: Yann Droneaud
Since commit 01dcc60a7cb8, platform_device_register_full() is
available to allocate and register a platform device.
If a dma_mask is provided as part of platform_device_info,
platform_device_register_full() allocate memory for a u64 using
kmalloc().
A comment in the code state that "[t]his memory isn't freed when the
device is put".
It's never a good thing to leak memory, but there's only very few
users of platform_device_info's dma_mask, and those are mostly
"static" devices that are not going to be plugged/unplugged often.
So memory leak is not really an issue, but allocating 8 bytes through
kmalloc() seems overkill.
To address this issue, this patch adds dma_mask to struct platform_object
and when using platform_device_alloc() or platform_device_register_full()
the .dma_mask pointer of struct device is assigned the address of this new
dma_mask member of struct platform_object. And since it's within struct
platform_object, dma_mask won't be leaked anymore when struct platform_device
get released.
No more memory leak, no small allocation and a slight reduction in code
size.
Built on Debian jessie, using GCC 4.8, for ARM and x86_64 architectures,
the size of the object file and the data structure layout are updated
as follows, produced with commands:
$ size drivers/base/platform.o
$ pahole drivers/base/platform.o \
--recursive \
--class_name device,pdev_archdata,platform_device,platform_object
-- size+pahole_3.15-rc6_ARM
++ size+pahole_3.15-rc6_ARM+
@@ -2,7 +2,7 @@
text data bss dec hex filename
- 6530 1008 875461d7a drivers/base/platform.o
+ 6482 1008 874981d4a drivers/base/platform.o
@@ -93,8 +93,13 @@ struct platform_object {
/* --- cacheline 12 boundary (768 bytes) was 40 bytes ago --- */
char name[1];/* 808 1 */
- /* size: 816, cachelines: 13, members: 2 */
- /* padding: 7 */
+ /* XXX 7 bytes hole, try to pack */
+ u64dma_mask; /* 816 8 */
+ /* size: 824, cachelines: 13, members: 3 */
+ /* sum members: 817, holes: 1, sum holes: 7 */
/* paddings: 1, sum paddings: 4 */
- /* last cacheline: 48 bytes */
+ /* last cacheline: 56 bytes */
};
-- size+pahole_3.15-rc6_x86_64
++ size+pahole_3.15-rc6_x86_64+
@@ -2,7 +2,7 @@
text data bss dec hex filename
- 8570 50323424 170264282 drivers/base/platform.o
+ 8509 50323408 169494235 drivers/base/platform.o
@@ -95,7 +95,11 @@ struct platform_object {
/* --- cacheline 22 boundary (1408 bytes) was 32 bytes ago --- */
char name[1]; /* 1440 1 */
- /* size: 1448, cachelines: 23, members: 2 */
- /* padding: 7 */
- /* last cacheline: 40 bytes */
+ /* XXX 7 bytes hole, try to pack */
+ u64dma_mask;/* 1448 8 */
+ /* size: 1456, cachelines: 23, members: 3 */
+ /* sum members: 1449, holes: 1, sum holes: 7 */
+ /* last cacheline: 48 bytes */
};
Changes from v4 [1]:
- Split v4 of the patch into two separate patches.
- Generated new object file size and data structure layout info.
- Updated the changelog message.
Changes from v3 [2]:
- fixed commit message so that git am doesn't fail.
Changes from v2 [3]:
- move 'dma_mask' to platform_object so that it's always
allocated and won't leak on release; remove all previously
added support functions.
- use C99 flexible array member for 'name' to remove padding
at the end of platform_object.
Changes from v1 [4]:
- remove unneeded kfree() from error path
- add reference to author/commit adding allocation of dmamask
Changes from v0 [5]:
- small rewrite to squeeze the patch to a bare minimal
[1]
http://lkml.kernel.org/r/1390817152-30898-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3541871/
[2]
http://lkml.kernel.org/r/1390771138-28348-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3540081/
[3]
http://lkml.kernel.org/r/1389683909-17495-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3484411/
[4]
http://lkml.kernel.org/r/1389649085-7365-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3480961/
[5]
http://lkml.kernel.org/r/1386886207-2735-1-git-send-email-ydrone...@opteya.com
Cc: Yann Droneaud
Cc: Uwe Kleine-König
Cc: Dmitry Torokhov
Cc: Greg Kroah-Hartman
Signed-off-by: Yann Droneaud
[Emil: split v4 of the patch in two and updated changelog]
Signed-off-by: Emil Goode
---
Hello Greg,
The first two patches in the series are created from v4 of the
original patch, since I have not changed how the code works I think
it is correct to keep the original author and Signed-off-by
[PATCH 1/3] driver core/platform: don't leak memory allocated for dma_mask
From: Yann Droneaud ydrone...@opteya.com
Since commit 01dcc60a7cb8, platform_device_register_full() is
available to allocate and register a platform device.
If a dma_mask is provided as part of platform_device_info,
platform_device_register_full() allocate memory for a u64 using
kmalloc().
A comment in the code state that [t]his memory isn't freed when the
device is put.
It's never a good thing to leak memory, but there's only very few
users of platform_device_info's dma_mask, and those are mostly
static devices that are not going to be plugged/unplugged often.
So memory leak is not really an issue, but allocating 8 bytes through
kmalloc() seems overkill.
To address this issue, this patch adds dma_mask to struct platform_object
and when using platform_device_alloc() or platform_device_register_full()
the .dma_mask pointer of struct device is assigned the address of this new
dma_mask member of struct platform_object. And since it's within struct
platform_object, dma_mask won't be leaked anymore when struct platform_device
get released.
No more memory leak, no small allocation and a slight reduction in code
size.
Built on Debian jessie, using GCC 4.8, for ARM and x86_64 architectures,
the size of the object file and the data structure layout are updated
as follows, produced with commands:
$ size drivers/base/platform.o
$ pahole drivers/base/platform.o \
--recursive \
--class_name device,pdev_archdata,platform_device,platform_object
-- size+pahole_3.15-rc6_ARM
++ size+pahole_3.15-rc6_ARM+
@@ -2,7 +2,7 @@
text data bss dec hex filename
- 6530 1008 875461d7a drivers/base/platform.o
+ 6482 1008 874981d4a drivers/base/platform.o
@@ -93,8 +93,13 @@ struct platform_object {
/* --- cacheline 12 boundary (768 bytes) was 40 bytes ago --- */
char name[1];/* 808 1 */
- /* size: 816, cachelines: 13, members: 2 */
- /* padding: 7 */
+ /* XXX 7 bytes hole, try to pack */
+ u64dma_mask; /* 816 8 */
+ /* size: 824, cachelines: 13, members: 3 */
+ /* sum members: 817, holes: 1, sum holes: 7 */
/* paddings: 1, sum paddings: 4 */
- /* last cacheline: 48 bytes */
+ /* last cacheline: 56 bytes */
};
-- size+pahole_3.15-rc6_x86_64
++ size+pahole_3.15-rc6_x86_64+
@@ -2,7 +2,7 @@
text data bss dec hex filename
- 8570 50323424 170264282 drivers/base/platform.o
+ 8509 50323408 169494235 drivers/base/platform.o
@@ -95,7 +95,11 @@ struct platform_object {
/* --- cacheline 22 boundary (1408 bytes) was 32 bytes ago --- */
char name[1]; /* 1440 1 */
- /* size: 1448, cachelines: 23, members: 2 */
- /* padding: 7 */
- /* last cacheline: 40 bytes */
+ /* XXX 7 bytes hole, try to pack */
+ u64dma_mask;/* 1448 8 */
+ /* size: 1456, cachelines: 23, members: 3 */
+ /* sum members: 1449, holes: 1, sum holes: 7 */
+ /* last cacheline: 48 bytes */
};
Changes from v4 [1]:
- Split v4 of the patch into two separate patches.
- Generated new object file size and data structure layout info.
- Updated the changelog message.
Changes from v3 [2]:
- fixed commit message so that git am doesn't fail.
Changes from v2 [3]:
- move 'dma_mask' to platform_object so that it's always
allocated and won't leak on release; remove all previously
added support functions.
- use C99 flexible array member for 'name' to remove padding
at the end of platform_object.
Changes from v1 [4]:
- remove unneeded kfree() from error path
- add reference to author/commit adding allocation of dmamask
Changes from v0 [5]:
- small rewrite to squeeze the patch to a bare minimal
[1]
http://lkml.kernel.org/r/1390817152-30898-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3541871/
[2]
http://lkml.kernel.org/r/1390771138-28348-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3540081/
[3]
http://lkml.kernel.org/r/1389683909-17495-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3484411/
[4]
http://lkml.kernel.org/r/1389649085-7365-1-git-send-email-ydrone...@opteya.com
https://patchwork.kernel.org/patch/3480961/
[5]
http://lkml.kernel.org/r/1386886207-2735-1-git-send-email-ydrone...@opteya.com
Cc: Yann Droneaud ydrone...@opteya.com
Cc: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Yann Droneaud ydrone...@opteya.com
[Emil: split v4 of the patch in two and updated changelog]
Signed-off-by: Emil Goode emilgo...@gmail.com
---
Hello Greg,
The first two patches in the series are created from v4
[PATCH 3/3] ARM: imx: don't allocate memory for .dma_mask of struct device
The first patch in this series changed the platform_device_alloc
function to assign the u64 .dma_mask pointer of struct device to
the address of a u64 .dma_mask member of struct platform_object.
This means that we no longer should allocate memory for the .dma_mask
pointer of struct device when using platform_device_alloc() and the
u64 .dma_mask of struct platform_object that it points to will be freed
when the release callback function platform_device_release is called.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
arch/arm/mach-imx/devices/platform-ipu-core.c |5 -
1 file changed, 5 deletions(-)
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index 6bd7c3f..4499a7d 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -79,10 +79,6 @@ struct platform_device *__init imx_alloc_mx3_camera(
if (!pdev)
return ERR_PTR(-ENOMEM);
- pdev-dev.dma_mask = kmalloc(sizeof(*pdev-dev.dma_mask), GFP_KERNEL);
- if (!pdev-dev.dma_mask)
- goto err;
-
*pdev-dev.dma_mask = DMA_BIT_MASK(32);
pdev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
@@ -96,7 +92,6 @@ struct platform_device *__init imx_alloc_mx3_camera(
ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
if (ret) {
err:
- kfree(pdev-dev.dma_mask);
platform_device_put(pdev);
return ERR_PTR(-ENODEV);
}
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] driver core/platform: make .name of struct platform_object a C99 flexible array
From: Yann Droneaud ydrone...@opteya.com
The changes made by this patch was originally part of the patch available
in the below link. It was requested that the change to .name was made in
a separate patch.
http://lkml.kernel.org/r/1390817152-30898-1-git-send-email-ydrone...@opteya.com
It's a pity that 7 bytes are wasted at the end of struct platform_object
in the form of padding after name field: unfortunately this padding is not
used when allocating the memory to hold the name.
By making .name of struct platform_object a C99 flexible array member
(eg. a zero sized array), padding at the end of the structure is removed
making the storage for .dma_mask almost for free.
To handle this change, memory allocation is updated to take care of
allocating an additional byte for the NUL terminating character.
Built on Debian jessie, using GCC 4.8, for ARM and x86_64 architectures,
the size of the object file and the data structure layout are updated
as follows, produced with commands:
$ size drivers/base/platform.o
$ pahole drivers/base/platform.o \
--recursive \
--class_name device,pdev_archdata,platform_device,platform_object
-- size+pahole_3.15-rc6_ARM
++ size+pahole_3.15-rc6_ARM+
@@ -2,7 +2,7 @@
text data bss dec hex filename
- 6482 1008 874981d4a drivers/base/platform.o
+ 6486 1008 875021d4e drivers/base/platform.o
@@ -91,15 +91,10 @@ struct platform_object {
/* XXX last struct has 4 bytes of padding */
/* --- cacheline 12 boundary (768 bytes) was 40 bytes ago --- */
- char name[1]; /* 808 1 */
+ u64dma_mask;/* 808 8 */
+ char name[0]; /* 816 0 */
- /* XXX 7 bytes hole, try to pack */
- u64dma_mask;/* 816 8 */
- /* size: 824, cachelines: 13, members: 3 */
- /* sum members: 817, holes: 1, sum holes: 7 */
+ /* size: 816, cachelines: 13, members: 3 */
/* paddings: 1, sum paddings: 4 */
- /* last cacheline: 56 bytes */
+ /* last cacheline: 48 bytes */
};
(Note that on x86 the size command didn't show any difference)
-- size+pahole_3.15-rc6_x86_64
++ size+pahole_3.15-rc6_x86_64+
@@ -93,13 +93,10 @@ struct platform_device {
struct platform_object {
struct platform_device pdev;/* 0 1440 */
/* --- cacheline 22 boundary (1408 bytes) was 32 bytes ago --- */
- char name[1]; /* 1440 1 */
+ u64dma_mask;/* 1440 8 */
+ char name[0]; /* 1448 0 */
- /* XXX 7 bytes hole, try to pack */
- u64dma_mask;/* 1448 8 */
- /* size: 1456, cachelines: 23, members: 3 */
- /* sum members: 1449, holes: 1, sum holes: 7 */
- /* last cacheline: 48 bytes */
+ /* size: 1448, cachelines: 23, members: 3 */
+ /* last cacheline: 40 bytes */
};
Cc: Yann Droneaud ydrone...@opteya.com
Cc: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Cc: Dmitry Torokhov dmitry.torok...@gmail.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Yann Droneaud ydrone...@opteya.com
[Emil: split the original patch, updated changelog and
generated new data structure layout info]
Signed-off-by: Emil Goode emilgo...@gmail.com
---
drivers/base/platform.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dd1fa07..ba98219 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -165,8 +165,8 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
- char name[1];
u64 dma_mask;
+ char name[];
};
/**
@@ -210,7 +210,7 @@ struct platform_device *platform_device_alloc(const char
*name, int id)
{
struct platform_object *pa;
- pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+ pa = kzalloc(sizeof(*pa) + strlen(name) + 1, GFP_KERNEL);
if (pa) {
strcpy(pa-name, name);
pa-pdev.name = pa-name;
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] driver core/platform: don't leak memory allocated for dma_mask
Hello Russell, On Mon, May 26, 2014 at 05:51:05PM +0100, Russell King - ARM Linux wrote: On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote: @@ -211,6 +215,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) strcpy(pa-name, name); pa-pdev.name = pa-name; pa-pdev.id = id; + pa-pdev.dev.dma_mask = pa-dma_mask; There is code in the kernel which, rightly or wrongly, checks whether dev-dma_mask is NULL to determine whether the device can do any kind of DMA. The above results in devices allocated via this interface always having this member set, which is a change of core kernel behaviour. How sure are you that this will not break anything? Thank you for pointing this out, considering the number of calls made to platform_device_alloc it would be easy to miss an occurrance of this problem. I would say that the risk heavily outweighs the gain and it's better to not apply this series. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] driver core/platform: don't leak memory allocated for dma_mask
Hello,
On Mon, May 26, 2014 at 10:30:46PM +0300, Dan Carpenter wrote:
On Mon, May 26, 2014 at 06:41:21PM +0200, Emil Goode wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9e9227e..dd1fa07 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -166,6 +166,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
char name[1];
+ u64 dma_mask;
};
Heh. No this doesn't work as patch #1. You have to have name at the
end of the struct.
Yes I missed that one, obviously the order is important here.
Best regards,
Emil Goode
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
Hello Uwe and Greg,
> You'd do a better deed if you picked up
> http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995
Since there is nothing wrong with the last version of the patch in
the above thread, I feel strange about picking it up and just splitting
it into two patches. However it would be good to have it applied.
I think the reason why the author didn't resend is that the object file
and data structure layout information in the changelog depend on the
changes to both .name and .dma_mask and by splitting the patch this
information would not apply to any one of the resulting two patches.
Perhaps keeping this information in the changelog is a good reason for
applying the patch as it is?
(I have attached the patch in question)
Best regards,
Emil Goode
>From 66b72cb8eb71974903dd40ed67a34412faf818f0 Mon Sep 17 00:00:00 2001
From: Yann Droneaud
Date: Mon, 27 Jan 2014 11:05:52 +0100
Subject: [PATCH] driver core/platform: don't leak memory allocated for
dma_mask
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Since commit 01dcc60a7cb8, platform_device_register_full() is
available to allocate and register a platform device.
If a dma_mask is provided as part of platform_device_info,
platform_device_register_full() allocate memory for a u64 using
kmalloc().
A comment in the code state that "[t]his memory isn't freed when the
device is put".
It's never a good thing to leak memory, but there's only very few
users of platform_device_info's dma_mask, and those are mostly
"static" devices that are not going to be plugged/unplugged often.
So memory leak is not really an issue, but allocating 8 bytes through
kmalloc() seems overkill.
And it's a pity to have to allocate 8 bytes for the dma_mask while up
to 7 bytes are wasted at the end of struct platform_object in the form
of padding after name field: unfortunately this padding is not used
when allocating the memory to hold the name.
To address theses issues, this patch adds dma_mask to platform_object
struct so that it is always allocated for all struct platform_device
allocated through platform_device_alloc() or platform_device_register_full().
And since it's within struct platform_object, dma_mask won't be leaked
anymore when struct platform_device got released. Storage for dma_mask
is added almost for free by removing the padding at the end of struct
platform_object.
Padding at the end of the structure is removed by making name a C99
flexible array member (eg. a zero sized array). To handle this change,
memory allocation is updated to take care of allocating an additional
byte for the NUL terminating character.
No more memory leak, no small allocation, no byte wasted and
a slight reduction in code size.
Built on Fedora 20, using GCC 4.8, for ARM, i386, x86_64 and PPC64
architectures, the size of the object file and the data structure layout
are updated as follow, produced with commands:
$ size drivers/base/platform.o
$ pahole drivers/base/platform.o \
--recursive \
--class_name device,pdev_archdata,platform_device,platform_object
--- size+pahole.next-20140124
+++ size+pahole.next-20140124+
@@ -1,5 +1,5 @@
textdata bss dec hex filename
- 5616 472 32612017e8 obj-arm/drivers/base/platform.o
+ 5572 472 32607617bc obj-arm/drivers/base/platform.o
struct device {
struct device *parent; /* 0 4 */
struct device_private *p;/* 4 4 */
@@ -77,15 +77,15 @@ struct platform_object {
/* XXX last struct has 4 bytes of padding */
/* --- cacheline 6 boundary (384 bytes) --- */
-char name[1]; /* 384 1 */
+u64dma_mask; /* 384 8 */
+char name[0]; /* 392 0 */
-/* size: 392, cachelines: 7, members: 2 */
-/* padding: 7 */
+/* size: 392, cachelines: 7, members: 3 */
/* paddings: 1, sum paddings: 4 */
/* last cacheline: 8 bytes */
};
textdata bss dec hex filename
- 6007 532 32657119ab obj-i386/drivers/base/platform.o
+ 5943 532 326507196b obj-i386/drivers/base/platform.o
struct device {
struct device *parent; /* 0 4 */
struct device_private *p;/* 4 4 */
@@ -161,14 +161,14 @@ struct platform_device {
struct platform_object {
struct platform_device pdev; /* 0 392 */
/* --- cacheline 6 boundary (384 bytes) was 8 bytes ago --- */
-char name[1]; /* 392 1 */
+u64dma_mas
Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
Hello Uwe and Greg,
You'd do a better deed if you picked up
http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995
Since there is nothing wrong with the last version of the patch in
the above thread, I feel strange about picking it up and just splitting
it into two patches. However it would be good to have it applied.
I think the reason why the author didn't resend is that the object file
and data structure layout information in the changelog depend on the
changes to both .name and .dma_mask and by splitting the patch this
information would not apply to any one of the resulting two patches.
Perhaps keeping this information in the changelog is a good reason for
applying the patch as it is?
(I have attached the patch in question)
Best regards,
Emil Goode
From 66b72cb8eb71974903dd40ed67a34412faf818f0 Mon Sep 17 00:00:00 2001
From: Yann Droneaud ydrone...@opteya.com
Date: Mon, 27 Jan 2014 11:05:52 +0100
Subject: [PATCH] driver core/platform: don't leak memory allocated for
dma_mask
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Since commit 01dcc60a7cb8, platform_device_register_full() is
available to allocate and register a platform device.
If a dma_mask is provided as part of platform_device_info,
platform_device_register_full() allocate memory for a u64 using
kmalloc().
A comment in the code state that [t]his memory isn't freed when the
device is put.
It's never a good thing to leak memory, but there's only very few
users of platform_device_info's dma_mask, and those are mostly
static devices that are not going to be plugged/unplugged often.
So memory leak is not really an issue, but allocating 8 bytes through
kmalloc() seems overkill.
And it's a pity to have to allocate 8 bytes for the dma_mask while up
to 7 bytes are wasted at the end of struct platform_object in the form
of padding after name field: unfortunately this padding is not used
when allocating the memory to hold the name.
To address theses issues, this patch adds dma_mask to platform_object
struct so that it is always allocated for all struct platform_device
allocated through platform_device_alloc() or platform_device_register_full().
And since it's within struct platform_object, dma_mask won't be leaked
anymore when struct platform_device got released. Storage for dma_mask
is added almost for free by removing the padding at the end of struct
platform_object.
Padding at the end of the structure is removed by making name a C99
flexible array member (eg. a zero sized array). To handle this change,
memory allocation is updated to take care of allocating an additional
byte for the NUL terminating character.
No more memory leak, no small allocation, no byte wasted and
a slight reduction in code size.
Built on Fedora 20, using GCC 4.8, for ARM, i386, x86_64 and PPC64
architectures, the size of the object file and the data structure layout
are updated as follow, produced with commands:
$ size drivers/base/platform.o
$ pahole drivers/base/platform.o \
--recursive \
--class_name device,pdev_archdata,platform_device,platform_object
--- size+pahole.next-20140124
+++ size+pahole.next-20140124+
@@ -1,5 +1,5 @@
textdata bss dec hex filename
- 5616 472 32612017e8 obj-arm/drivers/base/platform.o
+ 5572 472 32607617bc obj-arm/drivers/base/platform.o
struct device {
struct device *parent; /* 0 4 */
struct device_private *p;/* 4 4 */
@@ -77,15 +77,15 @@ struct platform_object {
/* XXX last struct has 4 bytes of padding */
/* --- cacheline 6 boundary (384 bytes) --- */
-char name[1]; /* 384 1 */
+u64dma_mask; /* 384 8 */
+char name[0]; /* 392 0 */
-/* size: 392, cachelines: 7, members: 2 */
-/* padding: 7 */
+/* size: 392, cachelines: 7, members: 3 */
/* paddings: 1, sum paddings: 4 */
/* last cacheline: 8 bytes */
};
textdata bss dec hex filename
- 6007 532 32657119ab obj-i386/drivers/base/platform.o
+ 5943 532 326507196b obj-i386/drivers/base/platform.o
struct device {
struct device *parent; /* 0 4 */
struct device_private *p;/* 4 4 */
@@ -161,14 +161,14 @@ struct platform_device {
struct platform_object {
struct platform_device pdev; /* 0 392 */
/* --- cacheline 6 boundary (384 bytes) was 8 bytes ago --- */
-char name[1]; /* 392 1 */
+u64dma_mask; /* 392
Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
Hello Uwe, On Thu, May 22, 2014 at 08:10:24PM +0200, Uwe Kleine-König wrote: > Hello Emil, > > On Thu, May 22, 2014 at 07:51:19PM +0200, Emil Goode wrote: > > We forgot to free pdev->dev.dma_mask on error after > > having called the imx_alloc_mx3_camera function. > > This patch introduces the imx_free_mx3_camera function > > that adds the missing kfree call and is practical for > > future usage with imx_alloc_mx3_camera(). > > > > Signed-off-by: Emil Goode > > --- > > arch/arm/mach-imx/devices-imx31.h |2 ++ > > arch/arm/mach-imx/devices-imx35.h |2 ++ > > arch/arm/mach-imx/devices/devices-common.h|1 + > > arch/arm/mach-imx/devices/platform-ipu-core.c | 11 +-- > > arch/arm/mach-imx/mach-mx31_3ds.c |2 +- > > arch/arm/mach-imx/mach-mx31moboard.c |3 +-- > > arch/arm/mach-imx/mach-mx35_3ds.c |2 +- > > arch/arm/mach-imx/mach-pcm037.c |2 +- > > 8 files changed, 18 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm/mach-imx/devices-imx31.h > > b/arch/arm/mach-imx/devices-imx31.h > > index e8d1611..900d3b0 100644 > > --- a/arch/arm/mach-imx/devices-imx31.h > > +++ b/arch/arm/mach-imx/devices-imx31.h > > @@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data; > > imx_add_ipu_core(_ipu_core_data) > > #define imx31_alloc_mx3_camera(pdata) \ > > imx_alloc_mx3_camera(_ipu_core_data, pdata) > > +#define imx31_free_mx3_camera(pdev)\ > > + imx_free_mx3_camera(pdev) > I wouldn't make this a globally visible function. Today all imx machines > should get their devices from an oftree, so the various functions to add > devices started to bitrot. Moreover there is no reason to remove a > device once it was successfully added. Ok I see. In mx31_3ds_init_camera() there are two checks that could fail before the device is added though. > Note that platform_device_register_full has the same problem (i.e. > pdev->dev.dma_mask isn't freed when the last reference to a device is > dropped.) You'd do a better deed if you picked up > http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995 > instead of fixing dead code. But feel free to choose yourself where you > want to patch. Thank you for the hint, I was about to leave this code alone and move on but couldn't resist one more patch :) Yes I realized that leaking dma_mask is a general problem, I will look into that thread, thanks. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: imx: introduce function imx_free_mx3_camera
We forgot to free pdev->dev.dma_mask on error after
having called the imx_alloc_mx3_camera function.
This patch introduces the imx_free_mx3_camera function
that adds the missing kfree call and is practical for
future usage with imx_alloc_mx3_camera().
Signed-off-by: Emil Goode
---
arch/arm/mach-imx/devices-imx31.h |2 ++
arch/arm/mach-imx/devices-imx35.h |2 ++
arch/arm/mach-imx/devices/devices-common.h|1 +
arch/arm/mach-imx/devices/platform-ipu-core.c | 11 +--
arch/arm/mach-imx/mach-mx31_3ds.c |2 +-
arch/arm/mach-imx/mach-mx31moboard.c |3 +--
arch/arm/mach-imx/mach-mx35_3ds.c |2 +-
arch/arm/mach-imx/mach-pcm037.c |2 +-
8 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-imx/devices-imx31.h
b/arch/arm/mach-imx/devices-imx31.h
index e8d1611..900d3b0 100644
--- a/arch/arm/mach-imx/devices-imx31.h
+++ b/arch/arm/mach-imx/devices-imx31.h
@@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data;
imx_add_ipu_core(_ipu_core_data)
#define imx31_alloc_mx3_camera(pdata) \
imx_alloc_mx3_camera(_ipu_core_data, pdata)
+#define imx31_free_mx3_camera(pdev)\
+ imx_free_mx3_camera(pdev)
#define imx31_add_mx3_sdc_fb(pdata)\
imx_add_mx3_sdc_fb(_ipu_core_data, pdata)
diff --git a/arch/arm/mach-imx/devices-imx35.h
b/arch/arm/mach-imx/devices-imx35.h
index 780d824..02d22e8 100644
--- a/arch/arm/mach-imx/devices-imx35.h
+++ b/arch/arm/mach-imx/devices-imx35.h
@@ -53,6 +53,8 @@ extern const struct imx_ipu_core_data imx35_ipu_core_data;
imx_add_ipu_core(_ipu_core_data)
#define imx35_alloc_mx3_camera(pdata) \
imx_alloc_mx3_camera(_ipu_core_data, pdata)
+#define imx35_free_mx3_camera(pdev)\
+ imx_free_mx3_camera(pdev)
#define imx35_add_mx3_sdc_fb(pdata)\
imx_add_mx3_sdc_fb(_ipu_core_data, pdata)
diff --git a/arch/arm/mach-imx/devices/devices-common.h
b/arch/arm/mach-imx/devices/devices-common.h
index 69bafc8..0b3ca11 100644
--- a/arch/arm/mach-imx/devices/devices-common.h
+++ b/arch/arm/mach-imx/devices/devices-common.h
@@ -185,6 +185,7 @@ struct imx_ipu_core_data {
};
struct platform_device *__init imx_add_ipu_core(
const struct imx_ipu_core_data *data);
+void imx_free_mx3_camera(struct platform_device *pdev);
struct platform_device *__init imx_alloc_mx3_camera(
const struct imx_ipu_core_data *data,
const struct mx3_camera_pdata *pdata);
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7c..77424c4 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -58,6 +58,14 @@ struct platform_device *__init imx_add_ipu_core(
res, ARRAY_SIZE(res), NULL, 0);
}
+void imx_free_mx3_camera(struct platform_device *pdev)
+{
+ if (pdev)
+ kfree(pdev->dev.dma_mask);
+
+ platform_device_put(pdev);
+}
+
struct platform_device *__init imx_alloc_mx3_camera(
const struct imx_ipu_core_data *data,
const struct mx3_camera_pdata *pdata)
@@ -96,8 +104,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
if (ret) {
err:
- kfree(pdev->dev.dma_mask);
- platform_device_put(pdev);
+ imx_free_mx3_camera(pdev);
return ERR_PTR(-ENODEV);
}
copied_pdata = dev_get_platdata(>dev);
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c
b/arch/arm/mach-imx/mach-mx31_3ds.c
index 453f41a..f8e6158 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -203,7 +203,7 @@ static int __init mx31_3ds_init_camera(void)
ret = platform_device_add(pdev);
if (ret)
err:
- platform_device_put(pdev);
+ imx31_free_mx3_camera(pdev);
return ret;
}
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c
b/arch/arm/mach-imx/mach-mx31moboard.c
index 6bed570..25228e3 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -496,10 +496,9 @@ static int __init mx31moboard_init_cam(void)
ret = platform_device_add(pdev);
if (ret)
err:
- platform_device_put(pdev);
+ imx31_free_mx3_camera(pdev);
return ret;
-
}
static void mx31moboard_poweroff(void)
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c
b/arch/arm/mach-imx/mach-mx35_3ds.c
index 72cd77d..77b230e 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -286,7 +286,7 @@ static int __init imx35_3ds_init_camera(void)
ret = platform_device_add(pdev);
i
Re: [PATCH] ARM: imx: add missing kfree call in error paths
Hello Dan, On Thu, May 22, 2014 at 04:50:16PM +0300, Dan Carpenter wrote: > On Thu, May 22, 2014 at 03:14:42PM +0200, Emil Goode wrote: > > We forgot to free pdev->dev.dma_mask as it is not freed > > by platform_device_put(). > > > > Every function which calls imx31_alloc_mx3_camera() is buggy. That > means that, at best, it is a 2 on Rusty's API rating. > http://sweng.the-davies.net/Home/rustys-api-design-manifesto > > The correct fix is to introduce an imx31_free_mx3_camera() otherwise we > will just keep introducing these bugs. I guess I could add this to arch/arm/mach-imx/devices/platform-ipu-core.c it would be a very small function though. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: imx: add missing kfree call in error paths
We forgot to free pdev->dev.dma_mask as it is not freed
by platform_device_put().
Signed-off-by: Emil Goode
---
arch/arm/mach-imx/mach-mx31_3ds.c|4 +++-
arch/arm/mach-imx/mach-mx31moboard.c |4 +++-
arch/arm/mach-imx/mach-mx35_3ds.c|4 +++-
arch/arm/mach-imx/mach-pcm037.c |4 +++-
4 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c
b/arch/arm/mach-imx/mach-mx31_3ds.c
index 453f41a..8de1019 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -201,9 +201,11 @@ static int __init mx31_3ds_init_camera(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev->dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
}
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c
b/arch/arm/mach-imx/mach-mx31moboard.c
index 6bed570..f24a93c 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -494,9 +494,11 @@ static int __init mx31moboard_init_cam(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev->dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c
b/arch/arm/mach-imx/mach-mx35_3ds.c
index 72cd77d..ffdddb3 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -284,9 +284,11 @@ static int __init imx35_3ds_init_camera(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev->dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
}
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index 8eb1570..764a111 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -427,9 +427,11 @@ static int __init pcm037_init_camera(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev->dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ARM: imx: convert camera init to use platform_device_register_full()
Hello Uwe, On Mon, May 19, 2014 at 08:27:22AM +0200, Uwe Kleine-König wrote: > Hello Emil, > > thanks for your effort. > > On Sun, May 18, 2014 at 10:51:00PM +0200, Emil Goode wrote: > > This converts the imx camera allocation and initialization functions > > to use platform_device_register_full() thus simplifying the code. > > > > Signed-off-by: Emil Goode > > --- > > Only build tested, unfortunately I currently don't have the hardware. > > > > arch/arm/mach-imx/devices/platform-ipu-core.c | 43 > > + > > arch/arm/mach-imx/mach-mx31_3ds.c |5 ++- > > arch/arm/mach-imx/mach-mx31moboard.c |6 ++-- > > arch/arm/mach-imx/mach-mx35_3ds.c |5 ++- > > arch/arm/mach-imx/mach-pcm037.c |5 ++- > > 5 files changed, 23 insertions(+), 41 deletions(-) > > > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c > > b/arch/arm/mach-imx/devices/platform-ipu-core.c > > index 6bd7c3f..13ea542 100644 > > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > @@ -69,42 +69,29 @@ struct platform_device *__init imx_alloc_mx3_camera( > IMHO you should better rename the function because now it doesn't only > allocate the device, but also registers it. > > Also I doubt that it's OK to call dma_declare_coherent_memory after the > device is added. In this case maybe extend > platform_device_register_full? And also add a warning to > dma_declare_coherent_memory if it is called for an already added device? > (Added a few people to Cc: that might be able to comment this. I don't > even know if there is a reliable way to check if a device is already > added.) According to the documentation dma_declare_coherent_memory() is only used in unlikely corner cases, so I concluded that it is probably better not to include it in platform_device_register_full(). Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ARM: imx: convert camera init to use platform_device_register_full()
Hello Uwe, On Mon, May 19, 2014 at 08:27:22AM +0200, Uwe Kleine-König wrote: Hello Emil, thanks for your effort. On Sun, May 18, 2014 at 10:51:00PM +0200, Emil Goode wrote: This converts the imx camera allocation and initialization functions to use platform_device_register_full() thus simplifying the code. Signed-off-by: Emil Goode emilgo...@gmail.com --- Only build tested, unfortunately I currently don't have the hardware. arch/arm/mach-imx/devices/platform-ipu-core.c | 43 + arch/arm/mach-imx/mach-mx31_3ds.c |5 ++- arch/arm/mach-imx/mach-mx31moboard.c |6 ++-- arch/arm/mach-imx/mach-mx35_3ds.c |5 ++- arch/arm/mach-imx/mach-pcm037.c |5 ++- 5 files changed, 23 insertions(+), 41 deletions(-) diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c index 6bd7c3f..13ea542 100644 --- a/arch/arm/mach-imx/devices/platform-ipu-core.c +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c @@ -69,42 +69,29 @@ struct platform_device *__init imx_alloc_mx3_camera( IMHO you should better rename the function because now it doesn't only allocate the device, but also registers it. Also I doubt that it's OK to call dma_declare_coherent_memory after the device is added. In this case maybe extend platform_device_register_full? And also add a warning to dma_declare_coherent_memory if it is called for an already added device? (Added a few people to Cc: that might be able to comment this. I don't even know if there is a reliable way to check if a device is already added.) According to the documentation dma_declare_coherent_memory() is only used in unlikely corner cases, so I concluded that it is probably better not to include it in platform_device_register_full(). Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: imx: add missing kfree call in error paths
We forgot to free pdev-dev.dma_mask as it is not freed
by platform_device_put().
Signed-off-by: Emil Goode emilgo...@gmail.com
---
arch/arm/mach-imx/mach-mx31_3ds.c|4 +++-
arch/arm/mach-imx/mach-mx31moboard.c |4 +++-
arch/arm/mach-imx/mach-mx35_3ds.c|4 +++-
arch/arm/mach-imx/mach-pcm037.c |4 +++-
4 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c
b/arch/arm/mach-imx/mach-mx31_3ds.c
index 453f41a..8de1019 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -201,9 +201,11 @@ static int __init mx31_3ds_init_camera(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev-dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
}
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c
b/arch/arm/mach-imx/mach-mx31moboard.c
index 6bed570..f24a93c 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -494,9 +494,11 @@ static int __init mx31moboard_init_cam(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev-dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c
b/arch/arm/mach-imx/mach-mx35_3ds.c
index 72cd77d..ffdddb3 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -284,9 +284,11 @@ static int __init imx35_3ds_init_camera(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev-dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
}
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index 8eb1570..764a111 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -427,9 +427,11 @@ static int __init pcm037_init_camera(void)
goto err;
ret = platform_device_add(pdev);
- if (ret)
+ if (ret) {
err:
+ kfree(pdev-dev.dma_mask);
platform_device_put(pdev);
+ }
return ret;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: imx: add missing kfree call in error paths
Hello Dan, On Thu, May 22, 2014 at 04:50:16PM +0300, Dan Carpenter wrote: On Thu, May 22, 2014 at 03:14:42PM +0200, Emil Goode wrote: We forgot to free pdev-dev.dma_mask as it is not freed by platform_device_put(). Every function which calls imx31_alloc_mx3_camera() is buggy. That means that, at best, it is a 2 on Rusty's API rating. http://sweng.the-davies.net/Home/rustys-api-design-manifesto The correct fix is to introduce an imx31_free_mx3_camera() otherwise we will just keep introducing these bugs. I guess I could add this to arch/arm/mach-imx/devices/platform-ipu-core.c it would be a very small function though. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: imx: introduce function imx_free_mx3_camera
We forgot to free pdev-dev.dma_mask on error after
having called the imx_alloc_mx3_camera function.
This patch introduces the imx_free_mx3_camera function
that adds the missing kfree call and is practical for
future usage with imx_alloc_mx3_camera().
Signed-off-by: Emil Goode emilgo...@gmail.com
---
arch/arm/mach-imx/devices-imx31.h |2 ++
arch/arm/mach-imx/devices-imx35.h |2 ++
arch/arm/mach-imx/devices/devices-common.h|1 +
arch/arm/mach-imx/devices/platform-ipu-core.c | 11 +--
arch/arm/mach-imx/mach-mx31_3ds.c |2 +-
arch/arm/mach-imx/mach-mx31moboard.c |3 +--
arch/arm/mach-imx/mach-mx35_3ds.c |2 +-
arch/arm/mach-imx/mach-pcm037.c |2 +-
8 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-imx/devices-imx31.h
b/arch/arm/mach-imx/devices-imx31.h
index e8d1611..900d3b0 100644
--- a/arch/arm/mach-imx/devices-imx31.h
+++ b/arch/arm/mach-imx/devices-imx31.h
@@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data;
imx_add_ipu_core(imx31_ipu_core_data)
#define imx31_alloc_mx3_camera(pdata) \
imx_alloc_mx3_camera(imx31_ipu_core_data, pdata)
+#define imx31_free_mx3_camera(pdev)\
+ imx_free_mx3_camera(pdev)
#define imx31_add_mx3_sdc_fb(pdata)\
imx_add_mx3_sdc_fb(imx31_ipu_core_data, pdata)
diff --git a/arch/arm/mach-imx/devices-imx35.h
b/arch/arm/mach-imx/devices-imx35.h
index 780d824..02d22e8 100644
--- a/arch/arm/mach-imx/devices-imx35.h
+++ b/arch/arm/mach-imx/devices-imx35.h
@@ -53,6 +53,8 @@ extern const struct imx_ipu_core_data imx35_ipu_core_data;
imx_add_ipu_core(imx35_ipu_core_data)
#define imx35_alloc_mx3_camera(pdata) \
imx_alloc_mx3_camera(imx35_ipu_core_data, pdata)
+#define imx35_free_mx3_camera(pdev)\
+ imx_free_mx3_camera(pdev)
#define imx35_add_mx3_sdc_fb(pdata)\
imx_add_mx3_sdc_fb(imx35_ipu_core_data, pdata)
diff --git a/arch/arm/mach-imx/devices/devices-common.h
b/arch/arm/mach-imx/devices/devices-common.h
index 69bafc8..0b3ca11 100644
--- a/arch/arm/mach-imx/devices/devices-common.h
+++ b/arch/arm/mach-imx/devices/devices-common.h
@@ -185,6 +185,7 @@ struct imx_ipu_core_data {
};
struct platform_device *__init imx_add_ipu_core(
const struct imx_ipu_core_data *data);
+void imx_free_mx3_camera(struct platform_device *pdev);
struct platform_device *__init imx_alloc_mx3_camera(
const struct imx_ipu_core_data *data,
const struct mx3_camera_pdata *pdata);
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7c..77424c4 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -58,6 +58,14 @@ struct platform_device *__init imx_add_ipu_core(
res, ARRAY_SIZE(res), NULL, 0);
}
+void imx_free_mx3_camera(struct platform_device *pdev)
+{
+ if (pdev)
+ kfree(pdev-dev.dma_mask);
+
+ platform_device_put(pdev);
+}
+
struct platform_device *__init imx_alloc_mx3_camera(
const struct imx_ipu_core_data *data,
const struct mx3_camera_pdata *pdata)
@@ -96,8 +104,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
if (ret) {
err:
- kfree(pdev-dev.dma_mask);
- platform_device_put(pdev);
+ imx_free_mx3_camera(pdev);
return ERR_PTR(-ENODEV);
}
copied_pdata = dev_get_platdata(pdev-dev);
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c
b/arch/arm/mach-imx/mach-mx31_3ds.c
index 453f41a..f8e6158 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -203,7 +203,7 @@ static int __init mx31_3ds_init_camera(void)
ret = platform_device_add(pdev);
if (ret)
err:
- platform_device_put(pdev);
+ imx31_free_mx3_camera(pdev);
return ret;
}
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c
b/arch/arm/mach-imx/mach-mx31moboard.c
index 6bed570..25228e3 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -496,10 +496,9 @@ static int __init mx31moboard_init_cam(void)
ret = platform_device_add(pdev);
if (ret)
err:
- platform_device_put(pdev);
+ imx31_free_mx3_camera(pdev);
return ret;
-
}
static void mx31moboard_poweroff(void)
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c
b/arch/arm/mach-imx/mach-mx35_3ds.c
index 72cd77d..77b230e 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -286,7 +286,7 @@ static int __init imx35_3ds_init_camera(void)
ret = platform_device_add(pdev
Re: [PATCH] ARM: imx: introduce function imx_free_mx3_camera
Hello Uwe, On Thu, May 22, 2014 at 08:10:24PM +0200, Uwe Kleine-König wrote: Hello Emil, On Thu, May 22, 2014 at 07:51:19PM +0200, Emil Goode wrote: We forgot to free pdev-dev.dma_mask on error after having called the imx_alloc_mx3_camera function. This patch introduces the imx_free_mx3_camera function that adds the missing kfree call and is practical for future usage with imx_alloc_mx3_camera(). Signed-off-by: Emil Goode emilgo...@gmail.com --- arch/arm/mach-imx/devices-imx31.h |2 ++ arch/arm/mach-imx/devices-imx35.h |2 ++ arch/arm/mach-imx/devices/devices-common.h|1 + arch/arm/mach-imx/devices/platform-ipu-core.c | 11 +-- arch/arm/mach-imx/mach-mx31_3ds.c |2 +- arch/arm/mach-imx/mach-mx31moboard.c |3 +-- arch/arm/mach-imx/mach-mx35_3ds.c |2 +- arch/arm/mach-imx/mach-pcm037.c |2 +- 8 files changed, 18 insertions(+), 7 deletions(-) diff --git a/arch/arm/mach-imx/devices-imx31.h b/arch/arm/mach-imx/devices-imx31.h index e8d1611..900d3b0 100644 --- a/arch/arm/mach-imx/devices-imx31.h +++ b/arch/arm/mach-imx/devices-imx31.h @@ -45,6 +45,8 @@ extern const struct imx_ipu_core_data imx31_ipu_core_data; imx_add_ipu_core(imx31_ipu_core_data) #define imx31_alloc_mx3_camera(pdata) \ imx_alloc_mx3_camera(imx31_ipu_core_data, pdata) +#define imx31_free_mx3_camera(pdev)\ + imx_free_mx3_camera(pdev) I wouldn't make this a globally visible function. Today all imx machines should get their devices from an oftree, so the various functions to add devices started to bitrot. Moreover there is no reason to remove a device once it was successfully added. Ok I see. In mx31_3ds_init_camera() there are two checks that could fail before the device is added though. Note that platform_device_register_full has the same problem (i.e. pdev-dev.dma_mask isn't freed when the last reference to a device is dropped.) You'd do a better deed if you picked up http://thread.gmane.org/gmane.linux.kernel/1613364/focus=1635995 instead of fixing dead code. But feel free to choose yourself where you want to patch. Thank you for the hint, I was about to leave this code alone and move on but couldn't resist one more patch :) Yes I realized that leaking dma_mask is a general problem, I will look into that thread, thanks. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ARM: imx: convert camera init to use platform_device_register_full()
Hello Uwe, Thank you for the review and sorry for the late response. On Mon, May 19, 2014 at 08:27:22AM +0200, Uwe Kleine-König wrote: > Hello Emil, > > thanks for your effort. > > On Sun, May 18, 2014 at 10:51:00PM +0200, Emil Goode wrote: > > This converts the imx camera allocation and initialization functions > > to use platform_device_register_full() thus simplifying the code. > > > > Signed-off-by: Emil Goode > > --- > > Only build tested, unfortunately I currently don't have the hardware. > > > > arch/arm/mach-imx/devices/platform-ipu-core.c | 43 > > + > > arch/arm/mach-imx/mach-mx31_3ds.c |5 ++- > > arch/arm/mach-imx/mach-mx31moboard.c |6 ++-- > > arch/arm/mach-imx/mach-mx35_3ds.c |5 ++- > > arch/arm/mach-imx/mach-pcm037.c |5 ++- > > 5 files changed, 23 insertions(+), 41 deletions(-) > > > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c > > b/arch/arm/mach-imx/devices/platform-ipu-core.c > > index 6bd7c3f..13ea542 100644 > > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > @@ -69,42 +69,29 @@ struct platform_device *__init imx_alloc_mx3_camera( > IMHO you should better rename the function because now it doesn't only > allocate the device, but also registers it. Agreed. > > Also I doubt that it's OK to call dma_declare_coherent_memory after the > device is added. In this case maybe extend > platform_device_register_full? And also add a warning to > dma_declare_coherent_memory if it is called for an already added device? > (Added a few people to Cc: that might be able to comment this. I don't > even know if there is a reliable way to check if a device is already > added.) Yes I also had doubts about that. However, apart from this patch there are three other places in arch/arm/mach-imx/ where dma_declare_coherent_memory() is called after calling platform_device_register_full(). Perhaps this is enough reason to extend platform_device_register_full(). It would be great to get some more feedback on this and also if there is a reliable way to check if a device is already added. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ARM: imx: convert camera init to use platform_device_register_full()
Hello Uwe, Thank you for the review and sorry for the late response. On Mon, May 19, 2014 at 08:27:22AM +0200, Uwe Kleine-König wrote: Hello Emil, thanks for your effort. On Sun, May 18, 2014 at 10:51:00PM +0200, Emil Goode wrote: This converts the imx camera allocation and initialization functions to use platform_device_register_full() thus simplifying the code. Signed-off-by: Emil Goode emilgo...@gmail.com --- Only build tested, unfortunately I currently don't have the hardware. arch/arm/mach-imx/devices/platform-ipu-core.c | 43 + arch/arm/mach-imx/mach-mx31_3ds.c |5 ++- arch/arm/mach-imx/mach-mx31moboard.c |6 ++-- arch/arm/mach-imx/mach-mx35_3ds.c |5 ++- arch/arm/mach-imx/mach-pcm037.c |5 ++- 5 files changed, 23 insertions(+), 41 deletions(-) diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c index 6bd7c3f..13ea542 100644 --- a/arch/arm/mach-imx/devices/platform-ipu-core.c +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c @@ -69,42 +69,29 @@ struct platform_device *__init imx_alloc_mx3_camera( IMHO you should better rename the function because now it doesn't only allocate the device, but also registers it. Agreed. Also I doubt that it's OK to call dma_declare_coherent_memory after the device is added. In this case maybe extend platform_device_register_full? And also add a warning to dma_declare_coherent_memory if it is called for an already added device? (Added a few people to Cc: that might be able to comment this. I don't even know if there is a reliable way to check if a device is already added.) Yes I also had doubts about that. However, apart from this patch there are three other places in arch/arm/mach-imx/ where dma_declare_coherent_memory() is called after calling platform_device_register_full(). Perhaps this is enough reason to extend platform_device_register_full(). It would be great to get some more feedback on this and also if there is a reliable way to check if a device is already added. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] ARM: imx: bug fix and convertion to platform_device_register_full()
This small series fixes a potential NULL pointer dereference bug and also converts the imx camera allocation and initialization functions to use platform_device_register_full(). The change the first patch makes is removed by the second patch, so the purpose of the first one is purely for easy backporting and git history. Emil Goode (2): ARM: imx: fix error handling in ipu device registration ARM: imx: convert camera init to use platform_device_register_full() arch/arm/mach-imx/devices/platform-ipu-core.c | 43 + arch/arm/mach-imx/mach-mx31_3ds.c |5 ++- arch/arm/mach-imx/mach-mx31moboard.c |6 ++-- arch/arm/mach-imx/mach-mx35_3ds.c |5 ++- arch/arm/mach-imx/mach-pcm037.c |5 ++- 5 files changed, 23 insertions(+), 41 deletions(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] ARM: imx: convert camera init to use platform_device_register_full()
This converts the imx camera allocation and initialization functions
to use platform_device_register_full() thus simplifying the code.
Signed-off-by: Emil Goode
---
Only build tested, unfortunately I currently don't have the hardware.
arch/arm/mach-imx/devices/platform-ipu-core.c | 43 +
arch/arm/mach-imx/mach-mx31_3ds.c |5 ++-
arch/arm/mach-imx/mach-mx31moboard.c |6 ++--
arch/arm/mach-imx/mach-mx35_3ds.c |5 ++-
arch/arm/mach-imx/mach-pcm037.c |5 ++-
5 files changed, 23 insertions(+), 41 deletions(-)
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index 6bd7c3f..13ea542 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -69,42 +69,29 @@ struct platform_device *__init imx_alloc_mx3_camera(
.flags = IORESOURCE_MEM,
},
};
- int ret = -ENOMEM;
- struct platform_device *pdev;
+ struct platform_device_info pdevinfo;
+ struct mx3_camera_pdata tmppdata;
if (IS_ERR_OR_NULL(imx_ipu_coredev))
return ERR_PTR(-ENODEV);
- pdev = platform_device_alloc("mx3-camera", 0);
- if (!pdev)
- return ERR_PTR(-ENOMEM);
-
- pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
- *pdev->dev.dma_mask = DMA_BIT_MASK(32);
- pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-
- ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
- if (ret)
- goto err;
+ memset(, 0, sizeof(pdevinfo));
+ pdevinfo.name = "mx3-camera";
+ pdevinfo.id = 0;
+ pdevinfo.res = res;
+ pdevinfo.num_res = ARRAY_SIZE(res);
+ pdevinfo.dma_mask = DMA_BIT_MASK(32);
if (pdata) {
- struct mx3_camera_pdata *copied_pdata;
-
- ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
- if (ret) {
-err:
- kfree(pdev->dev.dma_mask);
- platform_device_put(pdev);
- return ERR_PTR(-ENODEV);
- }
- copied_pdata = dev_get_platdata(>dev);
- copied_pdata->dma_dev = _ipu_coredev->dev;
+ tmppdata = *pdata;
+ tmppdata.dma_dev = _ipu_coredev->dev;
+
+ pdata =
+ pdevinfo.data = pdata;
+ pdevinfo.size_data = sizeof(*pdata);
}
- return pdev;
+ return platform_device_register_full();
}
struct platform_device *__init imx_add_mx3_sdc_fb(
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c
b/arch/arm/mach-imx/mach-mx31_3ds.c
index 4217871..a73a933 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -199,10 +199,9 @@ static int __init mx31_3ds_init_camera(void)
if (!(dma & DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
}
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c
b/arch/arm/mach-imx/mach-mx31moboard.c
index 08730f2..14ca3b8 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -492,13 +492,11 @@ static int __init mx31moboard_init_cam(void)
if (!(dma & DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
-
}
static void mx31moboard_poweroff(void)
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c
b/arch/arm/mach-imx/mach-mx35_3ds.c
index 4e8b184..c5a1b44 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -282,10 +282,9 @@ static int __init imx35_3ds_init_camera(void)
if (!(dma & DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
}
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index 81b8aff..fa00e6d 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -425,10 +425,9 @@ static int __init pcm037_init_camera(void)
if (!(dma & DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe l
[PATCH 1/2 v4] ARM: imx: fix error handling in ipu device registration
If we fail to allocate struct platform_device pdev we
dereference it after the goto label err.
This bug was found using coccinelle.
Signed-off-by: Emil Goode
---
v4: Simplified version that just fixes the bug.
Also updated the changelog.
v3: Made subject line more specific.
v2: Changed to return -ENOMEM instead of ret where possible and
updated the subject line.
arch/arm/mach-imx/devices/platform-ipu-core.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7c..6bd7c3f 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
pdev = platform_device_alloc("mx3-camera", 0);
if (!pdev)
- goto err;
+ return ERR_PTR(-ENOMEM);
pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
if (!pdev->dev.dma_mask)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ARM: imx: fix error handling
I appologise for providing incomplete information in my previous message. The involved call sites are the following: arch/arm/mach-imx/mach-mx35_3ds.c +265 imx35_3ds_init_camera() imx35_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-mx31moboard.c +477 mx31moboard_init_cam() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-mx31_3ds.c +182 mx31_3ds_init_camera() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-pcm037.c +413 pcm037_init_camera() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() Sorry about the mistake! Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ARM: imx: fix error handling
Hello Uwe,
On Sat, May 17, 2014 at 09:05:46PM +0200, Uwe Kleine-König wrote:
> Hello Emil,
>
> On Sat, May 17, 2014 at 05:35:40PM +0200, Emil Goode wrote:
> > On Fri, May 16, 2014 at 09:31:39PM +0200, Uwe Kleine-König wrote:
> > > On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote:
> > > > Am 16.05.2014 13:16, schrieb Emil Goode:
> > > > > Hello Walter,
> > > > >
> > > > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote:
> > > > >>
> > > > >>
> > > > >> Am 16.05.2014 11:54, schrieb Emil Goode:
> > > > >>> If we fail to allocate struct platform_device pdev we
> > > > >>> dereference it after the goto label err.
> > > > >>>
> > > > >>> I have rearranged the error handling a bit to fix the issue
> > > > >>> and also make it more clear.
> > > > >>>
> > > > >>> Signed-off-by: Emil Goode
> > > > >>> ---
> > > > >>> v2: Changed to return -ENOMEM instead of ret where possible and
> > > > >>> updated the subject line.
> > > > >>>
> > > > >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22
> > > > >>> +-
> > > > >>> 1 file changed, 13 insertions(+), 9 deletions(-)
> > > > >>>
> > > > >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
> > > > >>> b/arch/arm/mach-imx/devices/platform-ipu-core.c
> > > > >>> index fc4dd7c..68f2a4a 100644
> > > > >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c
> > > > >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
> > > > >>> @@ -77,34 +77,38 @@ struct platform_device *__init
> > > > >>> imx_alloc_mx3_camera(
> > > > >>>
> > > > >>> pdev = platform_device_alloc("mx3-camera", 0);
> > > > >>> if (!pdev)
> > > > >>> - goto err;
> > > > >>> + return ERR_PTR(-ENOMEM);
> > > > >>>
> > > > >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask),
> > > > >>> GFP_KERNEL);
> > > > >>> if (!pdev->dev.dma_mask)
> > > > >>> - goto err;
> > > > >>> + goto put_pdev;
> > > > >>>
> > > > >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32);
> > > > >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> > > > >>>
> > > > >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
> > > > >>> if (ret)
> > > > >>> - goto err;
> > > > >>> + goto free_dma_mask;
> > > > >>>
> > > > >>> if (pdata) {
> > > > >>> struct mx3_camera_pdata *copied_pdata;
> > > > >>>
> > > > >>> ret = platform_device_add_data(pdev, pdata,
> > > > >>> sizeof(*pdata));
> > > > >>> - if (ret) {
> > > > >>> -err:
> > > > >>> - kfree(pdev->dev.dma_mask);
> > > > >>> - platform_device_put(pdev);
> > > > >>> - return ERR_PTR(-ENODEV);
> > > > >>> - }
> > > > >>> + if (ret)
> > > > >>> + goto free_dma_mask;
> > > > >>> +
> > > > >>> copied_pdata = dev_get_platdata(>dev);
> > > > >>> copied_pdata->dma_dev = _ipu_coredev->dev;
> > > > >>
> > > > >>
> > > > >> the patch is fine, but what use is this copied_pdata ?
> > > > >> It scope ends next line ?
> > > > >>
> > > > >> re,
> > > > >> wh
> > > > >
> > > > > I also thought that looked a bit odd, but copied_pdata is a temporary
> > > > > pointer to platform_data of the dev struct.
> > > > >
> > > > > dev_get_platdata looks like this:
> > > > >
> > > > >
Re: [PATCH v2] ARM: imx: fix error handling
Hello Uwe,
On Sat, May 17, 2014 at 09:05:46PM +0200, Uwe Kleine-König wrote:
Hello Emil,
On Sat, May 17, 2014 at 05:35:40PM +0200, Emil Goode wrote:
On Fri, May 16, 2014 at 09:31:39PM +0200, Uwe Kleine-König wrote:
On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote:
Am 16.05.2014 13:16, schrieb Emil Goode:
Hello Walter,
On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote:
Am 16.05.2014 11:54, schrieb Emil Goode:
If we fail to allocate struct platform_device pdev we
dereference it after the goto label err.
I have rearranged the error handling a bit to fix the issue
and also make it more clear.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
v2: Changed to return -ENOMEM instead of ret where possible and
updated the subject line.
arch/arm/mach-imx/devices/platform-ipu-core.c | 22
+-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7c..68f2a4a 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -77,34 +77,38 @@ struct platform_device *__init
imx_alloc_mx3_camera(
pdev = platform_device_alloc(mx3-camera, 0);
if (!pdev)
- goto err;
+ return ERR_PTR(-ENOMEM);
pdev-dev.dma_mask = kmalloc(sizeof(*pdev-dev.dma_mask),
GFP_KERNEL);
if (!pdev-dev.dma_mask)
- goto err;
+ goto put_pdev;
*pdev-dev.dma_mask = DMA_BIT_MASK(32);
pdev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
if (ret)
- goto err;
+ goto free_dma_mask;
if (pdata) {
struct mx3_camera_pdata *copied_pdata;
ret = platform_device_add_data(pdev, pdata,
sizeof(*pdata));
- if (ret) {
-err:
- kfree(pdev-dev.dma_mask);
- platform_device_put(pdev);
- return ERR_PTR(-ENODEV);
- }
+ if (ret)
+ goto free_dma_mask;
+
copied_pdata = dev_get_platdata(pdev-dev);
copied_pdata-dma_dev = imx_ipu_coredev-dev;
the patch is fine, but what use is this copied_pdata ?
It scope ends next line ?
re,
wh
I also thought that looked a bit odd, but copied_pdata is a temporary
pointer to platform_data of the dev struct.
dev_get_platdata looks like this:
static inline void *dev_get_platdata(const struct device *dev)
{
return dev-platform_data;
}
So I believe it's a more compact way of writing:
pdev-dev-platform_data-dma_dev = imx_ipu_coredev-dev;
It's not about compactness. The dev_get_platdata accessor exists to be
used instead of directly accessing dev-platform_data. I admit a comment
would be nice ...
Anyhow this is all ugly, actually you'd want to have the dma_dev member
already fixed when calling platform_device_add_data. But you cannot
simply do
pdata-dma_dev = imx_ipu_coredev-dev;
ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
because *pdata is const.
Thank you for the explanation. Regarding the possibility of using
platform_device_register_full() to simplify this function. It seem to
be possible, the following inline function is available to help with this.
imx_add_platform_device_dmamask()
I'd prefer to use platform_device_register_full directly (and let the
wrapper die).
But as you mentioned above we need to allocate a new platform_device
struct before we can assign imx_ipu_coredev-dev to dma_dev, since
pdata is const. I guess this assignment could be done after calling
imx_add_platform_device_dmamask() but I don't think that makes the
No, that won't work, because after platform_device_register_full returns
you must assume that the device is already bound by a driver. And then
you must not change platform_data anymore.
The only thing that would work is:
struct mx3_camera_pdata tmppdata;
if (pdata) {
tmppdata = *pdata;
tmppdata.dma_dev = imx_ipu_coredev-dev;
pdata = tmppdata;
}
platform_device_register_full(... pdata ...)
Looking at converting to platform_device_register_full() again
it is a little bit more complicated than I first thought. The call
to platform_device_add() is acctually done in a separate function.
The involed functions are these:
mx31_3ds_init_camera()
imx31_alloc_mx3_camera
Re: [PATCH v2] ARM: imx: fix error handling
I appologise for providing incomplete information in my previous message. The involved call sites are the following: arch/arm/mach-imx/mach-mx35_3ds.c +265 imx35_3ds_init_camera() imx35_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-mx31moboard.c +477 mx31moboard_init_cam() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-mx31_3ds.c +182 mx31_3ds_init_camera() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-pcm037.c +413 pcm037_init_camera() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() Sorry about the mistake! Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] ARM: imx: convert camera init to use platform_device_register_full()
This converts the imx camera allocation and initialization functions
to use platform_device_register_full() thus simplifying the code.
Signed-off-by: Emil Goode emilgo...@gmail.com
---
Only build tested, unfortunately I currently don't have the hardware.
arch/arm/mach-imx/devices/platform-ipu-core.c | 43 +
arch/arm/mach-imx/mach-mx31_3ds.c |5 ++-
arch/arm/mach-imx/mach-mx31moboard.c |6 ++--
arch/arm/mach-imx/mach-mx35_3ds.c |5 ++-
arch/arm/mach-imx/mach-pcm037.c |5 ++-
5 files changed, 23 insertions(+), 41 deletions(-)
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c
b/arch/arm/mach-imx/devices/platform-ipu-core.c
index 6bd7c3f..13ea542 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -69,42 +69,29 @@ struct platform_device *__init imx_alloc_mx3_camera(
.flags = IORESOURCE_MEM,
},
};
- int ret = -ENOMEM;
- struct platform_device *pdev;
+ struct platform_device_info pdevinfo;
+ struct mx3_camera_pdata tmppdata;
if (IS_ERR_OR_NULL(imx_ipu_coredev))
return ERR_PTR(-ENODEV);
- pdev = platform_device_alloc(mx3-camera, 0);
- if (!pdev)
- return ERR_PTR(-ENOMEM);
-
- pdev-dev.dma_mask = kmalloc(sizeof(*pdev-dev.dma_mask), GFP_KERNEL);
- if (!pdev-dev.dma_mask)
- goto err;
-
- *pdev-dev.dma_mask = DMA_BIT_MASK(32);
- pdev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
-
- ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
- if (ret)
- goto err;
+ memset(pdevinfo, 0, sizeof(pdevinfo));
+ pdevinfo.name = mx3-camera;
+ pdevinfo.id = 0;
+ pdevinfo.res = res;
+ pdevinfo.num_res = ARRAY_SIZE(res);
+ pdevinfo.dma_mask = DMA_BIT_MASK(32);
if (pdata) {
- struct mx3_camera_pdata *copied_pdata;
-
- ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
- if (ret) {
-err:
- kfree(pdev-dev.dma_mask);
- platform_device_put(pdev);
- return ERR_PTR(-ENODEV);
- }
- copied_pdata = dev_get_platdata(pdev-dev);
- copied_pdata-dma_dev = imx_ipu_coredev-dev;
+ tmppdata = *pdata;
+ tmppdata.dma_dev = imx_ipu_coredev-dev;
+
+ pdata = tmppdata;
+ pdevinfo.data = pdata;
+ pdevinfo.size_data = sizeof(*pdata);
}
- return pdev;
+ return platform_device_register_full(pdevinfo);
}
struct platform_device *__init imx_add_mx3_sdc_fb(
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c
b/arch/arm/mach-imx/mach-mx31_3ds.c
index 4217871..a73a933 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -199,10 +199,9 @@ static int __init mx31_3ds_init_camera(void)
if (!(dma DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
}
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c
b/arch/arm/mach-imx/mach-mx31moboard.c
index 08730f2..14ca3b8 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -492,13 +492,11 @@ static int __init mx31moboard_init_cam(void)
if (!(dma DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
-
}
static void mx31moboard_poweroff(void)
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c
b/arch/arm/mach-imx/mach-mx35_3ds.c
index 4e8b184..c5a1b44 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -282,10 +282,9 @@ static int __init imx35_3ds_init_camera(void)
if (!(dma DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
}
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index 81b8aff..fa00e6d 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -425,10 +425,9 @@ static int __init pcm037_init_camera(void)
if (!(dma DMA_MEMORY_MAP))
goto err;
- ret = platform_device_add(pdev);
- if (ret)
+ return 0;
err:
- platform_device_put(pdev);
+ platform_device_put(pdev);
return ret;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message
[PATCH 1/2 v4] ARM: imx: fix error handling in ipu device registration
If we fail to allocate struct platform_device pdev we dereference it after the goto label err. This bug was found using coccinelle. Signed-off-by: Emil Goode emilgo...@gmail.com --- v4: Simplified version that just fixes the bug. Also updated the changelog. v3: Made subject line more specific. v2: Changed to return -ENOMEM instead of ret where possible and updated the subject line. arch/arm/mach-imx/devices/platform-ipu-core.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c index fc4dd7c..6bd7c3f 100644 --- a/arch/arm/mach-imx/devices/platform-ipu-core.c +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera( pdev = platform_device_alloc(mx3-camera, 0); if (!pdev) - goto err; + return ERR_PTR(-ENOMEM); pdev-dev.dma_mask = kmalloc(sizeof(*pdev-dev.dma_mask), GFP_KERNEL); if (!pdev-dev.dma_mask) -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] ARM: imx: bug fix and convertion to platform_device_register_full()
This small series fixes a potential NULL pointer dereference bug and also converts the imx camera allocation and initialization functions to use platform_device_register_full(). The change the first patch makes is removed by the second patch, so the purpose of the first one is purely for easy backporting and git history. Emil Goode (2): ARM: imx: fix error handling in ipu device registration ARM: imx: convert camera init to use platform_device_register_full() arch/arm/mach-imx/devices/platform-ipu-core.c | 43 + arch/arm/mach-imx/mach-mx31_3ds.c |5 ++- arch/arm/mach-imx/mach-mx31moboard.c |6 ++-- arch/arm/mach-imx/mach-mx35_3ds.c |5 ++- arch/arm/mach-imx/mach-pcm037.c |5 ++- 5 files changed, 23 insertions(+), 41 deletions(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
