[PATCH 1/2] staging: iio replaced kmalloc with local variables.

[PATCH 1/2] staging: iio replaced kmalloc with local variables.

[anish]

From: anish kumar <anish198519851985@gmail.com>

Replace kmalloc with local variables as it was un-necessary and
also removed the redudant code after this change.

Signed-off-by: anish kumar <anish198519851985@gmail.com>
---
 drivers/staging/iio/accel/kxsd9.c      |   19 +++----------------
 drivers/staging/iio/adc/max1363_core.c |    3 +--
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/iio/accel/kxsd9.c b/drivers/staging/iio/accel/kxsd9.c
index 431aa0f..7f6e6e5 100644
--- a/drivers/staging/iio/accel/kxsd9.c
+++ b/drivers/staging/iio/accel/kxsd9.c
@@ -255,7 +255,10 @@ static const struct attribute_group kxsd9_attribute_group = {
 
 static int __devinit kxsd9_power_up(struct spi_device *spi)
 {
+	struct spi_message msg;
 	int ret;
+	u8 tx[2], tx2[2];
+
 	struct spi_transfer xfers[2] = {
 		{
 			.bits_per_word = 8,
@@ -267,19 +270,7 @@ static int __devinit kxsd9_power_up(struct spi_device *spi)
 			.cs_change = 1,
 		},
 	};
-	struct spi_message msg;
-	u8 *tx2;
-	u8 *tx = kmalloc(2, GFP_KERNEL);
 
-	if (tx == NULL) {
-		ret = -ENOMEM;
-		goto error_ret;
-	}
-	tx2 = kmalloc(2, GFP_KERNEL);
-	if (tx2 == NULL) {
-		ret = -ENOMEM;
-		goto error_free_tx;
-	}
 	tx[0] = 0x0d;
 	tx[1] = 0x40;
 
@@ -293,10 +284,6 @@ static int __devinit kxsd9_power_up(struct spi_device *spi)
 	spi_message_add_tail(&xfers[1], &msg);
 	ret = spi_sync(spi, &msg);
 
-	kfree(tx2);
-error_free_tx:
-	kfree(tx);
-error_ret:
 	return ret;
 
 };
diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
index 1037087..0026242 100644
--- a/drivers/staging/iio/adc/max1363_core.c
+++ b/drivers/staging/iio/adc/max1363_core.c
@@ -207,7 +207,7 @@ 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;
@@ -215,7 +215,6 @@ static int max1363_write_basic_config(struct i2c_client *client,
 	tx_buf[1] = d2;
 
 	ret = i2c_master_send(client, tx_buf, 2);
-	kfree(tx_buf);
 
 	return (ret > 0) ? 0 : ret;
 }
-- 
1.7.0.4

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Geert Uytterhoeven]

On Mon, Jun 6, 2011 at 21:07, anish <anish198519851985@gmail.com> wrote:
> From: anish kumar <anish198519851985@gmail.com>
>
> Replace kmalloc with local variables as it was un-necessary and

Is it really unnecessary?
Or is this hardware that cannot transfer buffers on the stack?
IIRC there have been similar problems with SCSI command buffers on the stack.

> also removed the redudant code after this change.
>
> Signed-off-by: anish kumar <anish198519851985@gmail.com>
> ---
>  drivers/staging/iio/accel/kxsd9.c      |   19 +++----------------
>  drivers/staging/iio/adc/max1363_core.c |    3 +--
>  2 files changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/kxsd9.c b/drivers/staging/iio/accel/kxsd9.c
> index 431aa0f..7f6e6e5 100644
> --- a/drivers/staging/iio/accel/kxsd9.c
> +++ b/drivers/staging/iio/accel/kxsd9.c
> @@ -255,7 +255,10 @@ static const struct attribute_group kxsd9_attribute_group = {
>
>  static int __devinit kxsd9_power_up(struct spi_device *spi)
>  {
> +       struct spi_message msg;
>        int ret;
> +       u8 tx[2], tx2[2];
> +
>        struct spi_transfer xfers[2] = {
>                {
>                        .bits_per_word = 8,
> @@ -267,19 +270,7 @@ static int __devinit kxsd9_power_up(struct spi_device *spi)
>                        .cs_change = 1,
>                },
>        };
> -       struct spi_message msg;
> -       u8 *tx2;
> -       u8 *tx = kmalloc(2, GFP_KERNEL);
>
> -       if (tx == NULL) {
> -               ret = -ENOMEM;
> -               goto error_ret;
> -       }
> -       tx2 = kmalloc(2, GFP_KERNEL);
> -       if (tx2 == NULL) {
> -               ret = -ENOMEM;
> -               goto error_free_tx;
> -       }
>        tx[0] = 0x0d;
>        tx[1] = 0x40;
>
> @@ -293,10 +284,6 @@ static int __devinit kxsd9_power_up(struct spi_device *spi)
>        spi_message_add_tail(&xfers[1], &msg);
>        ret = spi_sync(spi, &msg);
>
> -       kfree(tx2);
> -error_free_tx:
> -       kfree(tx);
> -error_ret:
>        return ret;
>
>  };
> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
> index 1037087..0026242 100644
> --- a/drivers/staging/iio/adc/max1363_core.c
> +++ b/drivers/staging/iio/adc/max1363_core.c
> @@ -207,7 +207,7 @@ 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;
> @@ -215,7 +215,6 @@ static int max1363_write_basic_config(struct i2c_client *client,
>        tx_buf[1] = d2;
>
>        ret = i2c_master_send(client, tx_buf, 2);
> -       kfree(tx_buf);
>
>        return (ret > 0) ? 0 : ret;
>  }
> --
> 1.7.0.4

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Peter Hüwe]

Am Montag 06 Juni 2011, 21:07:37 schrieb anish:
> +++ b/drivers/staging/iio/adc/max1363_core.c
> @@ -207,7 +207,7 @@ 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;

These last two lines can also be removed ;)

Thanks,
Peter

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Dan Carpenter]

On Mon, Jun 06, 2011 at 09:13:29PM +0200, Geert Uytterhoeven wrote:
> On Mon, Jun 6, 2011 at 21:07, anish <anish198519851985@gmail.com> wrote:
> > From: anish kumar <anish198519851985@gmail.com>
> >
> > Replace kmalloc with local variables as it was un-necessary and
> 
> Is it really unnecessary?
> Or is this hardware that cannot transfer buffers on the stack?
> IIRC there have been similar problems with SCSI command buffers on the stack.

Yes.  You're right.  These buffers are not allowed to be stack
memory.  The documentation is the section "What memory is DMA'able?"
in Documentation/DMA-API-HOWTO.txt.

regards,
dan carpenter

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Greg KH]

On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
> From: anish kumar <anish198519851985@gmail.com>
> 
> Replace kmalloc with local variables as it was un-necessary and
> also removed the redudant code after this change.

No, it was necessary, you just broke this driver on ARM-based systems,
which isn't nice at all :(

SPI data, like USB data, has to come from kmalloced data, not from the
stack, or bad things can, and will, happen.

So I can't accept this patch, sorry.

greg k-h

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Joe Perches]

On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
> On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
> > From: anish kumar <anish198519851985@gmail.com>
> > Replace kmalloc with local variables as it was un-necessary and
> > also removed the redudant code after this change.
> SPI data, like USB data, has to come from kmalloced data, not from the
> stack, or bad things can, and will, happen.

Perhaps just add a comment like:

+       u8 *tx = kmalloc(2, GFP_KERNEL);	/* can't be on stack */

It might be better to do a single kmalloc(4)
than 2 separate kmalloc(2)'s.

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Greg KH]

On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
> On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
> > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
> > > From: anish kumar <anish198519851985@gmail.com>
> > > Replace kmalloc with local variables as it was un-necessary and
> > > also removed the redudant code after this change.
> > SPI data, like USB data, has to come from kmalloced data, not from the
> > stack, or bad things can, and will, happen.
> 
> Perhaps just add a comment like:
> 
> +       u8 *tx = kmalloc(2, GFP_KERNEL);	/* can't be on stack */

You really want to do to that for _EVERY_ SPI and USB driver?  I don't
think so.  It's a known thing that this is a requirement for SPI and USB
drivers.

> It might be better to do a single kmalloc(4)
> than 2 separate kmalloc(2)'s.

No, don't do that, that might cause problems as well with some
controllers.  We see some ARM controllers having problems with alignment
issues.  Now ideally we are fixing them in those controllers, and not
having to fix them in the individual drivers, but if at all possible, a
new allocation is the way to go.

thanks,

greg k-h

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Joe Perches]

On Mon, 2011-06-06 at 15:21 -0700, Greg KH wrote:
> On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
> > On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
> > > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
> > > > From: anish kumar <anish198519851985@gmail.com>
> > > > Replace kmalloc with local variables as it was un-necessary and
> > > > also removed the redudant code after this change.
> > > SPI data, like USB data, has to come from kmalloced data, not from the
> > > stack, or bad things can, and will, happen.
> > Perhaps just add a comment like:
> > +       u8 *tx = kmalloc(2, GFP_KERNEL);	/* can't be on stack */
> You really want to do to that for _EVERY_ SPI and USB driver?  I don't
> think so.

Nope, only the ones that look especially odd because 
	kmalloc(sizeof(struct foo), ...)
or
	kmalloc(sizeof("type), ...)
is not used.

It might be better to just declare a 2 byte struct.

cheers, Joe

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Greg KH]

On Mon, Jun 06, 2011 at 03:28:29PM -0700, Joe Perches wrote:
> On Mon, 2011-06-06 at 15:21 -0700, Greg KH wrote:
> > On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
> > > On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
> > > > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
> > > > > From: anish kumar <anish198519851985@gmail.com>
> > > > > Replace kmalloc with local variables as it was un-necessary and
> > > > > also removed the redudant code after this change.
> > > > SPI data, like USB data, has to come from kmalloced data, not from the
> > > > stack, or bad things can, and will, happen.
> > > Perhaps just add a comment like:
> > > +       u8 *tx = kmalloc(2, GFP_KERNEL);	/* can't be on stack */
> > You really want to do to that for _EVERY_ SPI and USB driver?  I don't
> > think so.
> 
> Nope, only the ones that look especially odd because 
> 	kmalloc(sizeof(struct foo), ...)
> or
> 	kmalloc(sizeof("type), ...)
> is not used.
> 
> It might be better to just declare a 2 byte struct.

No, this is a very common thing for all USB and SPI drivers.  It's so
obvious that once I saw the Subject: line, I knew this patch was going
to be wrong.

This is something that the USB and SPI developers know all about, it's
the way things work, and this driver works, so why are people trying to
"clean" it up in ways that will break it, or cause extra work with
structures where they are not needed at all?

odd.

greg k-h

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Joe Perches]

On Mon, 2011-06-06 at 15:41 -0700, Greg KH wrote:
> On Mon, Jun 06, 2011 at 03:28:29PM -0700, Joe Perches wrote:
> > On Mon, 2011-06-06 at 15:21 -0700, Greg KH wrote:
> > > On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
> > > > On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
> > > > > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
> > > > > > From: anish kumar <anish198519851985@gmail.com>
> > > > > > Replace kmalloc with local variables as it was un-necessary and
> > > > > > also removed the redudant code after this change.
> > > > > SPI data, like USB data, has to come from kmalloced data, not from the
> > > > > stack, or bad things can, and will, happen.
> > > > Perhaps just add a comment like:
> > > > +       u8 *tx = kmalloc(2, GFP_KERNEL);	/* can't be on stack */
> > > You really want to do to that for _EVERY_ SPI and USB driver?  I don't
> > > think so.
> > Nope, only the ones that look especially odd because 
> > 	kmalloc(sizeof(struct foo), ...)
> > or
> > 	kmalloc(sizeof("type), ...)
> > is not used.
> > It might be better to just declare a 2 byte struct.
> No, this is a very common thing for all USB and SPI drivers.  It's so
> obvious that once I saw the Subject: line, I knew this patch was going
> to be wrong.

As did I.
I seek to find a way to avoid seeing them in the future too.

> This is something that the USB and SPI developers know all about, it's
> the way things work, and this driver works, so why are people trying to
> "clean" it up in ways that will break it, or cause extra work with
> structures where they are not needed at all?
> odd.

Because people perform pattern recognition as a means to avoid
the work required for complete understanding.

Comments akin to the one in drivers/usb/serial/io_ti.c:

	lsr = kmalloc(1, GFP_KERNEL);	/* Sigh, that's right, just one byte,
					   as not all platforms can do DMA
					   from stack */

help people avoid applying patterns to inappropriate uses.

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Jonathan Cameron]

On 06/06/11 23:10, Joe Perches wrote:
> On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
>> On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
>>> From: anish kumar <anish198519851985@gmail.com>
>>> Replace kmalloc with local variables as it was un-necessary and
>>> also removed the redudant code after this change.
>> SPI data, like USB data, has to come from kmalloced data, not from the
>> stack, or bad things can, and will, happen.
> 
> Perhaps just add a comment like:
> 
> +       u8 *tx = kmalloc(2, GFP_KERNEL);	/* can't be on stack */
> 
> It might be better to do a single kmalloc(4)
> than 2 separate kmalloc(2)'s.
Actually, this little corner of the driver is the only place it isn't
using the buffers allocated with the chip state.  After I send our
latest clean up series in these are all explicitly marked
____cacheline_aligned anyway which should make it clear something
a little unusual is going on.

I'll clean up this function and credit it to Anish (if Anish doesn't
mind of course!)

Jonathan

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Jonathan Cameron]

On 06/07/11 05:56, anish singh wrote:
> 
> 
> On Tue, Jun 7, 2011 at 4:11 AM, Greg KH <gregkh@suse.de <mailto:gregkh@suse.de>> wrote:
> 
>     On Mon, Jun 06, 2011 at 03:28:29PM -0700, Joe Perches wrote:
>     > On Mon, 2011-06-06 at 15:21 -0700, Greg KH wrote:
>     > > On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
>     > > > On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
>     > > > > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
>     > > > > > From: anish kumar <anish198519851985@gmail.com <mailto:anish198519851985@gmail.com>>
>     > > > > > Replace kmalloc with local variables as it was un-necessary and
>     > > > > > also removed the redudant code after this change.
>     > > > > SPI data, like USB data, has to come from kmalloced data, not from the
>     > > > > stack, or bad things can, and will, happen.
>     > > > Perhaps just add a comment like:
>     > > > +       u8 *tx = kmalloc(2, GFP_KERNEL);  /* can't be on stack */
>     > > You really want to do to that for _EVERY_ SPI and USB driver?  I don't
>     > > think so.
>     >
>     > Nope, only the ones that look especially odd because
>     >       kmalloc(sizeof(struct foo), ...)
>     > or
>     >       kmalloc(sizeof("type), ...)
>     > is not used.
>     >
>     > It might be better to just declare a 2 byte struct.
> 
>     No, this is a very common thing for all USB and SPI drivers.  It's so
>     obvious that once I saw the Subject: line, I knew this patch was going
>     to be wrong.
> 
>     This is something that the USB and SPI developers know all about, it's
>     the way things work, and this driver works, so why are people trying to
>     "clean" it up in ways that will break it, or cause extra work with
>     structures where they are not needed at all?
> 
> Sorry for noise as i didn't the SPI requirements,thought it is similar to I2C and
> in cleaning up below part i wrongly cleaned SPI part also.Below was also part
> of patch.
Not to worry, you are far from the first person to fall into this issue!
Also, you have highlighted a weird corner in that driver, that could do with
tidying up (just not quite the fix you had in mind!).
> 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;
> @@ -215,7 +215,6 @@ static int max1363_write_basic_config(struct i2c_client *client,
>        tx_buf[1] = d2;
>        ret = i2c_master_send(client, tx_buf, 2);
> -       kfree(tx_buf);
>        return (ret > 0) ? 0 : ret;
>  }
> I think above patch is ok as it is I2C and I2C doesn't have that requirement.
Yes.  I2C bus drivers that do dma do the copy into dma safe memory internally.
Makes for more bouncing around of data, but i2c is slow anyway so it doesn't matter.
Also, based on a quick look this morning, the dma buffers tend to require various
headers to be in place etc which isn't typically the case for spi (a much more 'raw'
bus).

Can you cc linux-iio@vger.kernel.org on that patch when you send it out please.

Jonathan

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[anish kumar]

Jonathan Cameron wrote:
> On 06/07/11 05:56, anish singh wrote:
>>
>>
>> On Tue, Jun 7, 2011 at 4:11 AM, Greg KH <gregkh@suse.de
>> <mailto:gregkh@suse.de>> wrote:
>>
>>     On Mon, Jun 06, 2011 at 03:28:29PM -0700, Joe Perches wrote:
>>     > On Mon, 2011-06-06 at 15:21 -0700, Greg KH wrote:
>>     > > On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
>>     > > > On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
>>     > > > > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
>>     > > > > > From: anish kumar <anish198519851985@gmail.com
>>     <mailto:anish198519851985@gmail.com>> > > > > > Replace kmalloc with
>>     local variables as it was un-necessary and > > > > > also removed the
>>     redudant code after this change. > > > > SPI data, like USB data, has to
>>     come from kmalloced data, not from the > > > > stack, or bad things can,
>>     and will, happen. > > > Perhaps just add a comment like:
>>     > > > +       u8 *tx = kmalloc(2, GFP_KERNEL);  /* can't be on stack */
>>     > > You really want to do to that for _EVERY_ SPI and USB driver?  I
>>     don't > > think so.
>>     >
>>     > Nope, only the ones that look especially odd because
>>     >       kmalloc(sizeof(struct foo), ...)
>>     > or
>>     >       kmalloc(sizeof("type), ...)
>>     > is not used.
>>     >
>>     > It might be better to just declare a 2 byte struct.
>>
>>     No, this is a very common thing for all USB and SPI drivers.  It's so
>>     obvious that once I saw the Subject: line, I knew this patch was going
>>     to be wrong.
>>
>>     This is something that the USB and SPI developers know all about, it's
>>     the way things work, and this driver works, so why are people trying to
>>     "clean" it up in ways that will break it, or cause extra work with
>>     structures where they are not needed at all?
>>
>> Sorry for noise as i didn't the SPI requirements,thought it is similar to
>> I2C and
>> in cleaning up below part i wrongly cleaned SPI part also.Below was also part
>> of patch.
> Not to worry, you are far from the first person to fall into this issue!
> Also, you have highlighted a weird corner in that driver, that could do with
> tidying up (just not quite the fix you had in mind!).
>> 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;
>> @@ -215,7 +215,6 @@ static int max1363_write_basic_config(struct i2c_client
>>        *client, tx_buf[1] = d2;
>>        ret = i2c_master_send(client, tx_buf, 2);
>> -       kfree(tx_buf);
>>        return (ret > 0) ? 0 : ret;
>>  }
>> I think above patch is ok as it is I2C and I2C doesn't have that requirement.
> Yes.  I2C bus drivers that do dma do the copy into dma safe memory internally.
> Makes for more bouncing around of data, but i2c is slow anyway so it doesn't
> matter. Also, based on a quick look this morning, the dma buffers tend to
> require various headers to be in place etc which isn't typically the case for
> spi (a much more 'raw' bus).
I couldn't understand this comment.Specifically "various headers"?
Will appreciate it if you kindly explain.
>
> Can you cc linux-iio@vger.kernel.org on that patch when you send it out
> please.
Sure.Sorry for not sending it there.
>
> Jonathan 

Re: [PATCH 1/2] staging: iio replaced kmalloc with local variables.

[Jonathan Cameron]

On 06/07/11 11:32, anish kumar wrote:
> Jonathan Cameron wrote:
>> On 06/07/11 05:56, anish singh wrote:
>>>
>>>
>>> On Tue, Jun 7, 2011 at 4:11 AM, Greg KH <gregkh@suse.de
>>> <mailto:gregkh@suse.de>> wrote:
>>>
>>>     On Mon, Jun 06, 2011 at 03:28:29PM -0700, Joe Perches wrote:
>>>     > On Mon, 2011-06-06 at 15:21 -0700, Greg KH wrote:
>>>     > > On Mon, Jun 06, 2011 at 03:10:57PM -0700, Joe Perches wrote:
>>>     > > > On Mon, 2011-06-06 at 14:55 -0700, Greg KH wrote:
>>>     > > > > On Tue, Jun 07, 2011 at 12:37:37AM +0530, anish wrote:
>>>     > > > > > From: anish kumar <anish198519851985@gmail.com
>>>     <mailto:anish198519851985@gmail.com>> > > > > > Replace kmalloc with
>>>     local variables as it was un-necessary and > > > > > also removed the
>>>     redudant code after this change. > > > > SPI data, like USB data, has to
>>>     come from kmalloced data, not from the > > > > stack, or bad things can,
>>>     and will, happen. > > > Perhaps just add a comment like:
>>>     > > > +       u8 *tx = kmalloc(2, GFP_KERNEL);  /* can't be on stack */
>>>     > > You really want to do to that for _EVERY_ SPI and USB driver?  I
>>>     don't > > think so.
>>>     >
>>>     > Nope, only the ones that look especially odd because
>>>     >       kmalloc(sizeof(struct foo), ...)
>>>     > or
>>>     >       kmalloc(sizeof("type), ...)
>>>     > is not used.
>>>     >
>>>     > It might be better to just declare a 2 byte struct.
>>>
>>>     No, this is a very common thing for all USB and SPI drivers.  It's so
>>>     obvious that once I saw the Subject: line, I knew this patch was going
>>>     to be wrong.
>>>
>>>     This is something that the USB and SPI developers know all about, it's
>>>     the way things work, and this driver works, so why are people trying to
>>>     "clean" it up in ways that will break it, or cause extra work with
>>>     structures where they are not needed at all?
>>>
>>> Sorry for noise as i didn't the SPI requirements,thought it is similar to
>>> I2C and
>>> in cleaning up below part i wrongly cleaned SPI part also.Below was also part
>>> of patch.
>> Not to worry, you are far from the first person to fall into this issue!
>> Also, you have highlighted a weird corner in that driver, that could do with
>> tidying up (just not quite the fix you had in mind!).
>>> 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;
>>> @@ -215,7 +215,6 @@ static int max1363_write_basic_config(struct i2c_client
>>>        *client, tx_buf[1] = d2;
>>>        ret = i2c_master_send(client, tx_buf, 2);
>>> -       kfree(tx_buf);
>>>        return (ret > 0) ? 0 : ret;
>>>  }
>>> I think above patch is ok as it is I2C and I2C doesn't have that requirement.
>> Yes.  I2C bus drivers that do dma do the copy into dma safe memory internally.
>> Makes for more bouncing around of data, but i2c is slow anyway so it doesn't
>> matter. Also, based on a quick look this morning, the dma buffers tend to
>> require various headers to be in place etc which isn't typically the case for
>> spi (a much more 'raw' bus).
> I couldn't understand this comment.Specifically "various headers"?
principally looking at some drivers, i2c has an address.  This is sometimes at the
start of the dma buffer sent to the controller.
> Will appreciate it if you kindly explain.
Take a look at say, i2c-cpm.c (first example grep gave me ;)
cpm_i2c_parse_message
is the function that builds up the dma buffer contents.
The memcpy gives it away, as that is copying to +1 in the buffer that is dma'd off
to the controller.  The tb[0] contains the address.

Anyhow, this is probably needed for some spi controllers as well, but certainly not
all of them. Digging down in pxa2xx_spi for example, we have a the original tx and rx
pointers passed all the way through to the eventual dma transfer ( I think, that driver
isn't all that easy to follow!).

Jonathan