Re: [PATCH] staging iio: Replace kmalloc with local variable

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jonathan Cameron]

On 06/07/11 13:39, anish wrote:
> From: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> replaced kmalloc with local variable as I2C(in this case) doesn't require
> kmalloc memory it can do with stack memory.
I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
dma safe buffers bit...
> 
> Signed-off-by: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> ---
>  drivers/staging/iio/adc/max1363_core.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
> index 1037087..9462230 100644
> --- a/drivers/staging/iio/adc/max1363_core.c
> +++ b/drivers/staging/iio/adc/max1363_core.c
> @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
>  				      unsigned char d2)
>  {
>  	int ret;
> -	u8 *tx_buf = kmalloc(2, GFP_KERNEL);
> +	u8 tx_buf[2];
>  
> -	if (!tx_buf)
> -		return -ENOMEM;
>  	tx_buf[0] = d1;
>  	tx_buf[1] = d2;
>  
>  	ret = i2c_master_send(client, tx_buf, 2);
> -	kfree(tx_buf);
>  
>  	return (ret > 0) ? 0 : ret;
>  }

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Ben Dooks]

On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
> On 06/07/11 13:39, anish wrote:
> > From: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > replaced kmalloc with local variable as I2C(in this case) doesn't require
> > kmalloc memory it can do with stack memory.
> I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
> dma safe buffers bit...

No, it is down to the i2c driver, and from recollection dma from stack is
not recommended, due to things like cache line alignment. Please do not
do this.
 
> > Signed-off-by: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> > ---
> >  drivers/staging/iio/adc/max1363_core.c |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
> > index 1037087..9462230 100644
> > --- a/drivers/staging/iio/adc/max1363_core.c
> > +++ b/drivers/staging/iio/adc/max1363_core.c
> > @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
> >  				      unsigned char d2)
> >  {
> >  	int ret;
> > -	u8 *tx_buf = kmalloc(2, GFP_KERNEL);
> > +	u8 tx_buf[2];
> >  
> > -	if (!tx_buf)
> > -		return -ENOMEM;
> >  	tx_buf[0] = d1;
> >  	tx_buf[1] = d2;
> >  
> >  	ret = i2c_master_send(client, tx_buf, 2);
> > -	kfree(tx_buf);
> >  
> >  	return (ret > 0) ? 0 : ret;
> >  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jonathan Cameron]

On 06/07/11 14:41, Ben Dooks wrote:
> On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
>> On 06/07/11 13:39, anish wrote:
>>> From: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>
>>> replaced kmalloc with local variable as I2C(in this case) doesn't require
>>> kmalloc memory it can do with stack memory.
>> I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
>> dma safe buffers bit...
> 
> No, it is down to the i2c driver, and from recollection dma from stack is
> not recommended, due to things like cache line alignment. Please do not
> do this.
Then lets drop this.  Sorry Anish, seems I led you down the garden path.
I'll check all my i2c drivers don't do this...
>  
>>> Signed-off-by: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>>> ---
>>>  drivers/staging/iio/adc/max1363_core.c |    5 +----
>>>  1 files changed, 1 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
>>> index 1037087..9462230 100644
>>> --- a/drivers/staging/iio/adc/max1363_core.c
>>> +++ b/drivers/staging/iio/adc/max1363_core.c
>>> @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct i2c_client *client,
>>>  				      unsigned char d2)
>>>  {
>>>  	int ret;
>>> -	u8 *tx_buf = kmalloc(2, GFP_KERNEL);
>>> +	u8 tx_buf[2];
>>>  
>>> -	if (!tx_buf)
>>> -		return -ENOMEM;
>>>  	tx_buf[0] = d1;
>>>  	tx_buf[1] = d2;
>>>  
>>>  	ret = i2c_master_send(client, tx_buf, 2);
>>> -	kfree(tx_buf);
>>>  
>>>  	return (ret > 0) ? 0 : ret;
>>>  }
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH] staging iio: Replace kmalloc with local variable

[Hennerich, Michael]

Jonathan Cameron wrote on 2011-06-07:
> On 06/07/11 14:41, Ben Dooks wrote:
>> On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
>>> On 06/07/11 13:39, anish wrote:
>>>> From: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>
>>>> replaced kmalloc with local variable as I2C(in this case) doesn't
>>>> require kmalloc memory it can do with stack memory.
>>> I've cc'd linux-i2c just to check I'm right about the whole i2c
>>> doesn't need dma safe buffers bit...
>>
>> No, it is down to the i2c driver, and from recollection dma from stack
>> is not recommended, due to things like cache line alignment. Please do
>> not do this.
> Then lets drop this.  Sorry Anish, seems I led you down the garden path.
> I'll check all my i2c drivers don't do this...

Can you point to a i2c bus driver that does dma and uses the buffer from the
client directly?

>>
>>>> Signed-off-by: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Acked-by:
>>>> Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org> ---
>>>>  drivers/staging/iio/adc/max1363_core.c |    5 +----
>>>>  1 files changed, 1 insertions(+), 4 deletions(-)
>>>> diff --git a/drivers/staging/iio/adc/max1363_core.c
>>>> b/drivers/staging/iio/adc/max1363_core.c
>>>> index 1037087..9462230 100644
>>>> --- a/drivers/staging/iio/adc/max1363_core.c
>>>> +++ b/drivers/staging/iio/adc/max1363_core.c
>>>> @@ -207,15 +207,12 @@ static int max1363_write_basic_config(struct
> i2c_client *client,
>>>>                                  unsigned char d2)
>>>>  {
>>>>    int ret;
>>>> -  u8 *tx_buf = kmalloc(2, GFP_KERNEL);
>>>> +  u8 tx_buf[2];
>>>>
>>>> -  if (!tx_buf)
>>>> -          return -ENOMEM;
>>>>    tx_buf[0] = d1;
>>>>    tx_buf[1] = d2;
>>>>
>>>>    ret = i2c_master_send(client, tx_buf, 2);
>>>> -  kfree(tx_buf);
>>>>
>>>>    return (ret > 0) ? 0 : ret;
>>>>  }
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-i2c"
>>> in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More
>>> majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jean Delvare]

On Tue, 7 Jun 2011 14:41:35 +0100, Ben Dooks wrote:
> On Tue, Jun 07, 2011 at 02:00:23PM +0100, Jonathan Cameron wrote:
> > On 06/07/11 13:39, anish wrote:
> > > From: anish kumar <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > replaced kmalloc with local variable as I2C(in this case) doesn't require
> > > kmalloc memory it can do with stack memory.
> > I've cc'd linux-i2c just to check I'm right about the whole i2c doesn't need
> > dma safe buffers bit...
> 
> No, it is down to the i2c driver, and from recollection dma from stack is
> not recommended, due to things like cache line alignment. Please do not
> do this.

To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
device driver. So the patch is correct.

Keep in mind that not all I2C/SMBus controllers care about DMA. In fact,
most don't (at least in the set I am maintaining - might be different
an embedded.) So calling kmalloc for every transfer in every I2C device
driver "just in case" is very much counterproductive.

And, if a controller does DMA and needs well-aligned, dynamically
allocated buffer, its driver would hopefully allocate the buffer ONCE
and keep it around, rather than reallocating it for every transfer.

-- 
Jean Delvare

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jean Delvare]

On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
> On Wed, Jun 8, 2011 at 10:41 AM, anish singh <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>wrote:
> >  On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
> >> device driver. So the patch is correct.
> >>
> > I think i can take Jean ack on this?If yes then Joanthan kindly apply
> > this patch and i think you didn't lead me in wrong way as whatever
> > said by you is corroborated by jean also i.e. it is I2C bus driver
> > responsiblity
> > to care about DMA.
> >
> Sorry to ping you again.Can i take your ack on this?

Yes of course.

Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jonathan Cameron]

On 06/09/11 09:34, anish singh wrote:
> 
> 
> On Thu, Jun 9, 2011 at 1:59 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>> wrote:
> 
>     On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
>     > On Wed, Jun 8, 2011 at 10:41 AM, anish singh <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <mailto:anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>>wrote:
>     > >  On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>> wrote:
>     > >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
>     > >> device driver. So the patch is correct.
>     > >>
>     > > I think i can take Jean ack on this?If yes then Joanthan kindly apply
>     > > this patch and i think you didn't lead me in wrong way as whatever
>     > > said by you is corroborated by jean also i.e. it is I2C bus driver
>     > > responsiblity
>     > > to care about DMA.
>     > >
>     > Sorry to ping you again.Can i take your ack on this?
> 
>     Yes of course.
> 
>     Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>>
> 
> Thanks a ton.Jonathan kindly apply it now :)
Greg, I'll send this one on to you with the set I currently have out for review.

Jonathan

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jonathan Cameron]

On 06/09/11 09:48, Jonathan Cameron wrote:
> On 06/09/11 09:34, anish singh wrote:
>>
>>
>> On Thu, Jun 9, 2011 at 1:59 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>> wrote:
>>
>>     On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
>>     > On Wed, Jun 8, 2011 at 10:41 AM, anish singh <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <mailto:anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>>wrote:
>>     > >  On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>> wrote:
>>     > >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
>>     > >> device driver. So the patch is correct.
>>     > >>
>>     > > I think i can take Jean ack on this?If yes then Joanthan kindly apply
>>     > > this patch and i think you didn't lead me in wrong way as whatever
>>     > > said by you is corroborated by jean also i.e. it is I2C bus driver
>>     > > responsiblity
>>     > > to care about DMA.
>>     > >
>>     > Sorry to ping you again.Can i take your ack on this?
>>
>>     Yes of course.
>>
>>     Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>>
>>
>> Thanks a ton.Jonathan kindly apply it now :)
> Greg, I'll send this one on to you with the set I currently have out for review.
> 
Doh, after all this, I just tried to apply this to find the code in question has already
gone.  Anish, what tree are you working against? Looks like I did an equivalent clean up
(with a load of others) back in May then forgot about it.

Sorry all. I should have actually have checked it was still relevant rather than reviewing
purely on basis of content of patch...

Jonathan

Re: [PATCH] staging iio: Replace kmalloc with local variable

[Jonathan Cameron]

On 06/09/11 15:29, anish singh wrote:
> 
> 
> On Thu, Jun 9, 2011 at 7:46 PM, Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org <mailto:jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>> wrote:
> 
>     On 06/09/11 09:48, Jonathan Cameron wrote:
>     > On 06/09/11 09:34, anish singh wrote:
>     >>
>     >>
>     >> On Thu, Jun 9, 2011 at 1:59 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>>> wrote:
>     >>
>     >>     On Thu, 9 Jun 2011 13:53:09 +0530, anish singh wrote:
>     >>     > On Wed, Jun 8, 2011 at 10:41 AM, anish singh <anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <mailto:anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> <mailto:anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <mailto:anish198519851985-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>>>wrote:
>     >>     > >  On Tue, Jun 7, 2011 at 7:32 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>>> wrote:
>     >>     > >> To be clearer, it is down to the i2c BUS (adapter) driver, NOT the i2c
>     >>     > >> device driver. So the patch is correct.
>     >>     > >>
>     >>     > > I think i can take Jean ack on this?If yes then Joanthan kindly apply
>     >>     > > this patch and i think you didn't lead me in wrong way as whatever
>     >>     > > said by you is corroborated by jean also i.e. it is I2C bus driver
>     >>     > > responsiblity
>     >>     > > to care about DMA.
>     >>     > >
>     >>     > Sorry to ping you again.Can i take your ack on this?
>     >>
>     >>     Yes of course.
>     >>
>     >>     Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org <mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>>>
>     >>
>     >> Thanks a ton.Jonathan kindly apply it now :)
>     > Greg, I'll send this one on to you with the set I currently have out for review.
>     >
>     Doh, after all this, I just tried to apply this to find the code in question has already
>     gone.  Anish, what tree are you working against?
> 
> I am using linux-next.Is it not the right tree for staging?
Something curious is going on then because this code is no longer in linux-next either.
> 
>     Looks like I did an equivalent clean up
>     (with a load of others) back in May then forgot about it.
> 
>     Sorry all. I should have actually have checked it was still relevant rather than reviewing
>     purely on basis of content of patch...
> 
>     Jonathan
> 
>