linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
@ 2021-10-14  3:53 Yang Yingliang
  2021-10-14  8:20 ` Hans de Goede
  2021-10-14 14:54 ` Andy Shevchenko
  0 siblings, 2 replies; 7+ messages in thread
From: Yang Yingliang @ 2021-10-14  3:53 UTC (permalink / raw)
  To: linux-kernel, linux-iio; +Cc: lars, jic23, hdegoede, andriy.shevchenko, ddvlad

When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
memory leak as follows:

unreferenced object 0xffff888009551400 (size 512):
  comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
  hex dump (first 32 bytes):
    02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
  backtrace:
    [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
    [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
    [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer]
    [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]
    [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0
    [<00000000d9f581a6>] really_probe+0x299/0xc30
    [<00000000c6c16cde>] __driver_probe_device+0x357/0x500
    [<00000000909852a1>] driver_probe_device+0x4e/0x140
    [<000000008419ba53>] __device_attach_driver+0x257/0x340
    [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0
    [<000000005bf45d75>] __device_attach+0x272/0x420
    [<0000000075220311>] bus_probe_device+0x1eb/0x2a0
    [<0000000015587e85>] device_add+0xbf0/0x1f90
    [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20
    [<000000000865ca18>] new_device_store+0x1fa/0x420
    [<0000000059a3d183>] dev_attr_store+0x58/0x80

Fix it by remove data->dready_trig condition in probe and remove.

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/iio/accel/kxcjk-1013.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index a51fdd3c9b5b..24c9387c2968 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
 	return 0;
 
 err_buffer_cleanup:
-	if (data->dready_trig)
-		iio_triggered_buffer_cleanup(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
 err_trigger_unregister:
 	if (data->dready_trig)
 		iio_trigger_unregister(data->dready_trig);
@@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
+	iio_triggered_buffer_cleanup(indio_dev);
 	if (data->dready_trig) {
-		iio_triggered_buffer_cleanup(indio_dev);
 		iio_trigger_unregister(data->dready_trig);
 		iio_trigger_unregister(data->motion_trig);
 	}
-- 
2.25.1


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

* Re: [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
  2021-10-14  3:53 [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove Yang Yingliang
@ 2021-10-14  8:20 ` Hans de Goede
  2021-10-14 17:38   ` Jonathan Cameron
  2021-10-14 14:54 ` Andy Shevchenko
  1 sibling, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2021-10-14  8:20 UTC (permalink / raw)
  To: Yang Yingliang, linux-kernel, linux-iio
  Cc: lars, jic23, andriy.shevchenko, ddvlad

Hi,

On 10/14/21 5:53 AM, Yang Yingliang wrote:
> When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
> memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
> memory leak as follows:
> 
> unreferenced object 0xffff888009551400 (size 512):
>   comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
>   hex dump (first 32 bytes):
>     02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
>   backtrace:
>     [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
>     [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
>     [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer]
>     [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]
>     [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0
>     [<00000000d9f581a6>] really_probe+0x299/0xc30
>     [<00000000c6c16cde>] __driver_probe_device+0x357/0x500
>     [<00000000909852a1>] driver_probe_device+0x4e/0x140
>     [<000000008419ba53>] __device_attach_driver+0x257/0x340
>     [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0
>     [<000000005bf45d75>] __device_attach+0x272/0x420
>     [<0000000075220311>] bus_probe_device+0x1eb/0x2a0
>     [<0000000015587e85>] device_add+0xbf0/0x1f90
>     [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20
>     [<000000000865ca18>] new_device_store+0x1fa/0x420
>     [<0000000059a3d183>] dev_attr_store+0x58/0x80
> 
> Fix it by remove data->dready_trig condition in probe and remove.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>

Hmm, wouldn't the right fix be to also move the
iio_triggered_buffer_setup() call to inside the:

	if (client->irq > 0 && data->acpi_type != ACPI_SMO8500) {
	}

block ?

Jonathan (jic23) can you take a look at this, to me it seems that having
a triggered buffer allocated without any triggers is not useful ?

Regards,

Hans



> ---
>  drivers/iio/accel/kxcjk-1013.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index a51fdd3c9b5b..24c9387c2968 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
>  	return 0;
>  
>  err_buffer_cleanup:
> -	if (data->dready_trig)
> -		iio_triggered_buffer_cleanup(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
>  err_trigger_unregister:
>  	if (data->dready_trig)
>  		iio_trigger_unregister(data->dready_trig);
> @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
>  
> +	iio_triggered_buffer_cleanup(indio_dev);
>  	if (data->dready_trig) {
> -		iio_triggered_buffer_cleanup(indio_dev);
>  		iio_trigger_unregister(data->dready_trig);
>  		iio_trigger_unregister(data->motion_trig);
>  	}
> 


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

* Re: [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
  2021-10-14 14:54 ` Andy Shevchenko
@ 2021-10-14 13:38   ` Yang Yingliang
  2021-10-14 17:01     ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Yang Yingliang @ 2021-10-14 13:38 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel, linux-iio, lars, jic23, hdegoede, ddvlad

Hi,

On 2021/10/14 22:54, Andy Shevchenko wrote:
> On Thu, Oct 14, 2021 at 11:53:38AM +0800, Yang Yingliang wrote:
>> When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
>> memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
>> memory leak as follows:
> It seems it's not first time I'm telling you to shrink the noise in the commit
> message.  Can you please LEARN this once and forever?
Thanks for you advice, I searched the whole kernel source tree commit 
message to
learn how to make the memory leak report, I found most of them using the 
whole
backtrace, so I make the report like they did in this patch. I will 
shrink the noise in v2.
How about shrink it like this:

unreferenced object 0xffff888009551400 (size 512):
   comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
   hex dump (first 32 bytes):
     02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
     00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
   backtrace:
     [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
     [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
     [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 
[industrialio_triggered_buffer]
     [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]

Thanks,
Yang
>
>> unreferenced object 0xffff888009551400 (size 512):
>>    comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
>>    hex dump (first 32 bytes):
>>      02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>      00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
>>    backtrace:
>>      [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
>>      [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
>>      [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer]
>>      [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]
>>      [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0
>>      [<00000000d9f581a6>] really_probe+0x299/0xc30
>>      [<00000000c6c16cde>] __driver_probe_device+0x357/0x500
>>      [<00000000909852a1>] driver_probe_device+0x4e/0x140
>>      [<000000008419ba53>] __device_attach_driver+0x257/0x340
>>      [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0
>>      [<000000005bf45d75>] __device_attach+0x272/0x420
>>      [<0000000075220311>] bus_probe_device+0x1eb/0x2a0
>>      [<0000000015587e85>] device_add+0xbf0/0x1f90
>>      [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20
>>      [<000000000865ca18>] new_device_store+0x1fa/0x420
>>      [<0000000059a3d183>] dev_attr_store+0x58/0x80

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

* Re: [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
  2021-10-14  3:53 [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove Yang Yingliang
  2021-10-14  8:20 ` Hans de Goede
@ 2021-10-14 14:54 ` Andy Shevchenko
  2021-10-14 13:38   ` Yang Yingliang
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-14 14:54 UTC (permalink / raw)
  To: Yang Yingliang; +Cc: linux-kernel, linux-iio, lars, jic23, hdegoede, ddvlad

On Thu, Oct 14, 2021 at 11:53:38AM +0800, Yang Yingliang wrote:
> When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
> memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
> memory leak as follows:

It seems it's not first time I'm telling you to shrink the noise in the commit
message.  Can you please LEARN this once and forever?

> unreferenced object 0xffff888009551400 (size 512):
>   comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
>   hex dump (first 32 bytes):
>     02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
>   backtrace:
>     [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
>     [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
>     [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer]
>     [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]
>     [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0
>     [<00000000d9f581a6>] really_probe+0x299/0xc30
>     [<00000000c6c16cde>] __driver_probe_device+0x357/0x500
>     [<00000000909852a1>] driver_probe_device+0x4e/0x140
>     [<000000008419ba53>] __device_attach_driver+0x257/0x340
>     [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0
>     [<000000005bf45d75>] __device_attach+0x272/0x420
>     [<0000000075220311>] bus_probe_device+0x1eb/0x2a0
>     [<0000000015587e85>] device_add+0xbf0/0x1f90
>     [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20
>     [<000000000865ca18>] new_device_store+0x1fa/0x420
>     [<0000000059a3d183>] dev_attr_store+0x58/0x80

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
  2021-10-14 13:38   ` Yang Yingliang
@ 2021-10-14 17:01     ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-14 17:01 UTC (permalink / raw)
  To: Yang Yingliang; +Cc: linux-kernel, linux-iio, lars, jic23, hdegoede, ddvlad

On Thu, Oct 14, 2021 at 09:38:52PM +0800, Yang Yingliang wrote:
> On 2021/10/14 22:54, Andy Shevchenko wrote:
> > On Thu, Oct 14, 2021 at 11:53:38AM +0800, Yang Yingliang wrote:
> > > When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
> > > memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
> > > memory leak as follows:
> > It seems it's not first time I'm telling you to shrink the noise in the commit
> > message.  Can you please LEARN this once and forever?
> Thanks for you advice, I searched the whole kernel source tree commit
> message to
> learn how to make the memory leak report, I found most of them using the
> whole
> backtrace, so I make the report like they did in this patch.

Some maintainers do not care about bloated sizes of the commit messages,
however there are several advantages to have them shorter:

1/ saving resources (followed by disk storages and energy around the world,
   means being environment friendly for real);

2/ when reading log, noise make it much harder to understand, besides the
   fact that reading itself will be time consuming;

3/ nowadays some people are tending to read or even discuss the changes on
   the mobile devices, where screen size is not so big;

4/ the copied'n'pasted backtrace means that the contributor probably
   haven't thought through it and the quality of the change is in doubt.

> I will shrink
> the noise in v2.
> How about shrink it like this:

Much better!

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
  2021-10-14  8:20 ` Hans de Goede
@ 2021-10-14 17:38   ` Jonathan Cameron
  2021-10-14 18:41     ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2021-10-14 17:38 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Yang Yingliang, linux-kernel, linux-iio, lars, andriy.shevchenko, ddvlad

On Thu, 14 Oct 2021 10:20:34 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 10/14/21 5:53 AM, Yang Yingliang wrote:
> > When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
> > memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
> > memory leak as follows:
> > 
> > unreferenced object 0xffff888009551400 (size 512):
> >   comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
> >   hex dump (first 32 bytes):
> >     02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >     00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
> >   backtrace:
> >     [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
> >     [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
> >     [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer]
> >     [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]
> >     [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0
> >     [<00000000d9f581a6>] really_probe+0x299/0xc30
> >     [<00000000c6c16cde>] __driver_probe_device+0x357/0x500
> >     [<00000000909852a1>] driver_probe_device+0x4e/0x140
> >     [<000000008419ba53>] __device_attach_driver+0x257/0x340
> >     [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0
> >     [<000000005bf45d75>] __device_attach+0x272/0x420
> >     [<0000000075220311>] bus_probe_device+0x1eb/0x2a0
> >     [<0000000015587e85>] device_add+0xbf0/0x1f90
> >     [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20
> >     [<000000000865ca18>] new_device_store+0x1fa/0x420
> >     [<0000000059a3d183>] dev_attr_store+0x58/0x80
> > 
> > Fix it by remove data->dready_trig condition in probe and remove.
> > 
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger")
> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>  
> 
> Hmm, wouldn't the right fix be to also move the
> iio_triggered_buffer_setup() call to inside the:
> 
> 	if (client->irq > 0 && data->acpi_type != ACPI_SMO8500) {
> 	}
> 
> block ?
> 
> Jonathan (jic23) can you take a look at this, to me it seems that having
> a triggered buffer allocated without any triggers is not useful ?

It can use another trigger not supplied by this particular device.
e.g. sysfs or hrtimer trigger.  This is common for cases where
we may or may not have an irq wired and the validate_* callbacks are
not provided (which would indicate we had to use the device's own trigger).

Jonathan

> 
> Regards,
> 
> Hans
> 
> 
> 
> > ---
> >  drivers/iio/accel/kxcjk-1013.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> > index a51fdd3c9b5b..24c9387c2968 100644
> > --- a/drivers/iio/accel/kxcjk-1013.c
> > +++ b/drivers/iio/accel/kxcjk-1013.c
> > @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
> >  	return 0;
> >  
> >  err_buffer_cleanup:
> > -	if (data->dready_trig)
> > -		iio_triggered_buffer_cleanup(indio_dev);
> > +	iio_triggered_buffer_cleanup(indio_dev);
> >  err_trigger_unregister:
> >  	if (data->dready_trig)
> >  		iio_trigger_unregister(data->dready_trig);
> > @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client)
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> >  
> > +	iio_triggered_buffer_cleanup(indio_dev);
> >  	if (data->dready_trig) {
> > -		iio_triggered_buffer_cleanup(indio_dev);
> >  		iio_trigger_unregister(data->dready_trig);
> >  		iio_trigger_unregister(data->motion_trig);
> >  	}
> >   
> 


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

* Re: [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove
  2021-10-14 17:38   ` Jonathan Cameron
@ 2021-10-14 18:41     ` Hans de Goede
  0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2021-10-14 18:41 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Yang Yingliang, linux-kernel, linux-iio, lars, andriy.shevchenko, ddvlad

Hi,

On 10/14/21 7:38 PM, Jonathan Cameron wrote:
> On Thu, 14 Oct 2021 10:20:34 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 10/14/21 5:53 AM, Yang Yingliang wrote:
>>> When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the
>>> memory allocated by iio_triggered_buffer_setup() will not be freed, and cause
>>> memory leak as follows:
>>>
>>> unreferenced object 0xffff888009551400 (size 512):
>>>   comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s)
>>>   hex dump (first 32 bytes):
>>>     02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>>     00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff  ........ .......
>>>   backtrace:
>>>     [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360
>>>     [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf]
>>>     [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer]
>>>     [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013]
>>>     [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0
>>>     [<00000000d9f581a6>] really_probe+0x299/0xc30
>>>     [<00000000c6c16cde>] __driver_probe_device+0x357/0x500
>>>     [<00000000909852a1>] driver_probe_device+0x4e/0x140
>>>     [<000000008419ba53>] __device_attach_driver+0x257/0x340
>>>     [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0
>>>     [<000000005bf45d75>] __device_attach+0x272/0x420
>>>     [<0000000075220311>] bus_probe_device+0x1eb/0x2a0
>>>     [<0000000015587e85>] device_add+0xbf0/0x1f90
>>>     [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20
>>>     [<000000000865ca18>] new_device_store+0x1fa/0x420
>>>     [<0000000059a3d183>] dev_attr_store+0x58/0x80
>>>
>>> Fix it by remove data->dready_trig condition in probe and remove.
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger")
>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>  
>>
>> Hmm, wouldn't the right fix be to also move the
>> iio_triggered_buffer_setup() call to inside the:
>>
>> 	if (client->irq > 0 && data->acpi_type != ACPI_SMO8500) {
>> 	}
>>
>> block ?
>>
>> Jonathan (jic23) can you take a look at this, to me it seems that having
>> a triggered buffer allocated without any triggers is not useful ?
> 
> It can use another trigger not supplied by this particular device.
> e.g. sysfs or hrtimer trigger.  This is common for cases where
> we may or may not have an irq wired and the validate_* callbacks are
> not provided (which would indicate we had to use the device's own trigger).

Ok, thank you for clarifying this.

With that resolved, this patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


>>> ---
>>>  drivers/iio/accel/kxcjk-1013.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>>> index a51fdd3c9b5b..24c9387c2968 100644
>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>> @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>>  	return 0;
>>>  
>>>  err_buffer_cleanup:
>>> -	if (data->dready_trig)
>>> -		iio_triggered_buffer_cleanup(indio_dev);
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>>  err_trigger_unregister:
>>>  	if (data->dready_trig)
>>>  		iio_trigger_unregister(data->dready_trig);
>>> @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client)
>>>  	pm_runtime_disable(&client->dev);
>>>  	pm_runtime_set_suspended(&client->dev);
>>>  
>>> +	iio_triggered_buffer_cleanup(indio_dev);
>>>  	if (data->dready_trig) {
>>> -		iio_triggered_buffer_cleanup(indio_dev);
>>>  		iio_trigger_unregister(data->dready_trig);
>>>  		iio_trigger_unregister(data->motion_trig);
>>>  	}
>>>   
>>
> 


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

end of thread, other threads:[~2021-10-14 18:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14  3:53 [PATCH] iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove Yang Yingliang
2021-10-14  8:20 ` Hans de Goede
2021-10-14 17:38   ` Jonathan Cameron
2021-10-14 18:41     ` Hans de Goede
2021-10-14 14:54 ` Andy Shevchenko
2021-10-14 13:38   ` Yang Yingliang
2021-10-14 17:01     ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).