[PATCH] alauda: do not use stack for URB transfer_buffers

[PATCH] alauda: do not use stack for URB transfer_buffers

[Jussi Kivilinna]

Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
be DMA-able, which stack is not.

Patch is only compile tested.

Cc: stable@vger.kernel.org
Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
---
 drivers/mtd/nand/alauda.c |   74 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/alauda.c b/drivers/mtd/nand/alauda.c
index 60a0dfd..e99e302 100644
--- a/drivers/mtd/nand/alauda.c
+++ b/drivers/mtd/nand/alauda.c
@@ -69,6 +69,9 @@ struct alauda {
 	struct mtd_info		*mtd;
 	struct alauda_card	*card;
 	struct mutex		card_mutex;
+	void			*card_cmdbuf;
+	void			*card_oobbuf;
+	void			*card_ignore_buf;
 	u32			pagemask;
 	u32			bytemask;
 	u32			blockmask;
@@ -119,6 +122,13 @@ static void alauda_delete(struct kref *kref)
 {
 	struct alauda *al = container_of(kref, struct alauda, kref);
 
+	kfree(al[0].card_cmdbuf);
+	kfree(al[1].card_cmdbuf);
+	kfree(al[0].card_oobbuf);
+	kfree(al[1].card_oobbuf);
+	kfree(al[0].card_ignore_buf);
+	kfree(al[1].card_ignore_buf);
+
 	if (al->mtd) {
 		mtd_device_unregister(al->mtd);
 		kfree(al->mtd);
@@ -133,7 +143,9 @@ static int alauda_get_media_status(struct alauda *al, void *buf)
 
 	mutex_lock(&al->card_mutex);
 	ret = usb_control_msg(al->dev, usb_rcvctrlpipe(al->dev, 0),
-			ALAUDA_GET_XD_MEDIA_STATUS, 0xc0, 0, 1, buf, 2, HZ);
+			ALAUDA_GET_XD_MEDIA_STATUS, 0xc0, 0, 1, al->card_cmdbuf,
+			2, HZ);
+	memcpy(buf, al->card_cmdbuf, 2);
 	mutex_unlock(&al->card_mutex);
 	return ret;
 }
@@ -155,7 +167,9 @@ static int alauda_get_media_signatures(struct alauda *al, void *buf)
 
 	mutex_lock(&al->card_mutex);
 	ret = usb_control_msg(al->dev, usb_rcvctrlpipe(al->dev, 0),
-			ALAUDA_GET_XD_MEDIA_SIG, 0xc0, 0, 0, buf, 4, HZ);
+			ALAUDA_GET_XD_MEDIA_SIG, 0xc0, 0, 0, al->card_cmdbuf,
+			4, HZ);
+	memcpy(buf, al->card_cmdbuf, 4);
 	mutex_unlock(&al->card_mutex);
 	return ret;
 }
@@ -167,7 +181,8 @@ static void alauda_reset(struct alauda *al)
 		0, 0, 0, 0, al->port
 	};
 	mutex_lock(&al->card_mutex);
-	usb_bulk_msg(al->dev, al->bulk_out, command, 9, NULL, HZ);
+	memcpy(al->card_cmdbuf, command, 9);
+	usb_bulk_msg(al->dev, al->bulk_out, al->card_cmdbuf, 9, NULL, HZ);
 	mutex_unlock(&al->card_mutex);
 }
 
@@ -223,14 +238,17 @@ static int __alauda_read_page(struct mtd_info *mtd, loff_t from, void *buf,
 			goto out;
 	}
 	init_completion(&sg.comp);
-	usb_fill_bulk_urb(sg.urb[0], al->dev, al->bulk_out, command, 9,
+
+	mutex_lock(&al->card_mutex);
+
+	memcpy(al->card_cmdbuf, command, 9);
+	usb_fill_bulk_urb(sg.urb[0], al->dev, al->bulk_out, al->card_cmdbuf, 9,
 			alauda_complete, NULL);
 	usb_fill_bulk_urb(sg.urb[1], al->dev, al->bulk_in, buf, mtd->writesize,
 			alauda_complete, NULL);
-	usb_fill_bulk_urb(sg.urb[2], al->dev, al->bulk_in, oob, 16,
+	usb_fill_bulk_urb(sg.urb[2], al->dev, al->bulk_in, al->card_oobbuf, 16,
 			alauda_complete, &sg.comp);
 
-	mutex_lock(&al->card_mutex);
 	for (i=0; i<3; i++) {
 		err = usb_submit_urb(sg.urb[i], GFP_NOIO);
 		if (err)
@@ -243,6 +261,7 @@ cancel:
 			usb_kill_urb(sg.urb[i]);
 		}
 	}
+	memcpy(oob, al->card_oobbuf, 16);
 	mutex_unlock(&al->card_mutex);
 
 out:
@@ -288,14 +307,18 @@ static int alauda_write_page(struct mtd_info *mtd, loff_t to, void *buf,
 			goto out;
 	}
 	init_completion(&sg.comp);
-	usb_fill_bulk_urb(sg.urb[0], al->dev, al->bulk_out, command, 9,
-			alauda_complete, NULL);
-	usb_fill_bulk_urb(sg.urb[1], al->dev, al->write_out, buf,mtd->writesize,
-			alauda_complete, NULL);
-	usb_fill_bulk_urb(sg.urb[2], al->dev, al->write_out, oob, 16,
-			alauda_complete, &sg.comp);
 
 	mutex_lock(&al->card_mutex);
+
+	memcpy(al->card_cmdbuf, command, 9);
+	memcpy(al->card_oobbuf, oob, 16);
+	usb_fill_bulk_urb(sg.urb[0], al->dev, al->bulk_out, al->card_cmdbuf, 9,
+			alauda_complete, NULL);
+	usb_fill_bulk_urb(sg.urb[1], al->dev, al->write_out, buf,
+			  mtd->writesize, alauda_complete, NULL);
+	usb_fill_bulk_urb(sg.urb[2], al->dev, al->write_out, al->card_oobbuf,
+			  16, alauda_complete, &sg.comp);
+
 	for (i=0; i<3; i++) {
 		err = usb_submit_urb(sg.urb[i], GFP_NOIO);
 		if (err)
@@ -326,7 +349,6 @@ static int alauda_erase_block(struct mtd_info *mtd, loff_t ofs)
 		ALAUDA_BULK_CMD, ALAUDA_BULK_ERASE_BLOCK, PBA_HI(pba),
 		PBA_ZONE(pba), 0, PBA_LO(pba), 0x02, 0, al->port
 	};
-	u8 buf[2];
 	int i, err;
 
 	for (i=0; i<2; i++)
@@ -339,12 +361,15 @@ static int alauda_erase_block(struct mtd_info *mtd, loff_t ofs)
 			goto out;
 	}
 	init_completion(&sg.comp);
-	usb_fill_bulk_urb(sg.urb[0], al->dev, al->bulk_out, command, 9,
+
+	mutex_lock(&al->card_mutex);
+
+	memcpy(al->card_cmdbuf, command, 9);
+	usb_fill_bulk_urb(sg.urb[0], al->dev, al->bulk_out, al->card_cmdbuf, 9,
 			alauda_complete, NULL);
-	usb_fill_bulk_urb(sg.urb[1], al->dev, al->bulk_in, buf, 2,
+	usb_fill_bulk_urb(sg.urb[1], al->dev, al->bulk_in, al->card_oobbuf, 2,
 			alauda_complete, &sg.comp);
 
-	mutex_lock(&al->card_mutex);
 	for (i=0; i<2; i++) {
 		err = usb_submit_urb(sg.urb[i], GFP_NOIO);
 		if (err)
@@ -367,9 +392,9 @@ out:
 
 static int alauda_read_oob(struct mtd_info *mtd, loff_t from, void *oob)
 {
-	static u8 ignore_buf[512]; /* write only */
+	struct alauda *al = mtd->priv;
 
-	return __alauda_read_page(mtd, from, ignore_buf, oob);
+	return __alauda_read_page(mtd, from, al->card_ignore_buf, oob);
 }
 
 static int alauda_isbad(struct mtd_info *mtd, loff_t ofs)
@@ -677,6 +702,19 @@ static int alauda_probe(struct usb_interface *interface,
 	/* second device is identical up to now */
 	memcpy(al+1, al, sizeof(*al));
 
+	al[0].card_cmdbuf = kmalloc(9, GFP_KERNEL);
+	al[1].card_cmdbuf = kmalloc(9, GFP_KERNEL);
+	al[0].card_oobbuf = kmalloc(16, GFP_KERNEL);
+	al[1].card_oobbuf = kmalloc(16, GFP_KERNEL);
+	al[0].card_ignore_buf = kmalloc(512, GFP_KERNEL);
+	al[1].card_ignore_buf = kmalloc(512, GFP_KERNEL);
+	if (!al[0].card_cmdbuf || !al[1].card_cmdbuf || !al[0].card_oobbuf ||
+	    !al[1].card_oobbuf || !al[0].card_ignore_buf ||
+	    !al[1].card_ignore_buf) {
+		err = -ENOMEM;
+		goto error;
+	}
+
 	mutex_init(&al[0].card_mutex);
 	mutex_init(&al[1].card_mutex);
 

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Jörn Engel]

On Tue, 6 August 2013 15:03:29 +0300, Jussi Kivilinna wrote:
> 
> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
> be DMA-able, which stack is not.
> 
> Patch is only compile tested.

I have tested the driver back when I wrote it.  Not sure why it worked
then, maybe the chip in my notebook back then didn't care about
alignment or I just got lucky with the memory allocations.

My old test hardware was likely thrown out in the last move, about two
years back.  Performance sucked anyway, 400kB/s was the limit.  Maybe
we should just remove the driver and not spend any more time on it?

Jörn

--
Victory in war is not repetitious.
-- Sun Tzu

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Jussi Kivilinna]

On 06.08.2013 19:49, Jörn Engel wrote:
> On Tue, 6 August 2013 15:03:29 +0300, Jussi Kivilinna wrote:
>>
>> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
>> be DMA-able, which stack is not.
>>
>> Patch is only compile tested.
> 
> I have tested the driver back when I wrote it.  Not sure why it worked
> then, maybe the chip in my notebook back then didn't care about
> alignment or I just got lucky with the memory allocations.

You might not see problems with your hardware/architecture.

"Documentation/DMA-API-HOWTO.txt" discusses about "What memory is DMA'able?".

> 
> My old test hardware was likely thrown out in the last move, about two
> years back.  Performance sucked anyway, 400kB/s was the limit.  Maybe
> we should just remove the driver and not spend any more time on it?
> 
> Jörn
> 
> --
> Victory in war is not repetitious.
> -- Sun Tzu
> 

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Jörn Engel]

On Wed, 7 August 2013 08:50:53 +0300, Jussi Kivilinna wrote:
> On 06.08.2013 19:49, Jörn Engel wrote:
> > On Tue, 6 August 2013 15:03:29 +0300, Jussi Kivilinna wrote:
> >>
> >> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
> >> be DMA-able, which stack is not.
> >>
> >> Patch is only compile tested.
> > 
> > I have tested the driver back when I wrote it.  Not sure why it worked
> > then, maybe the chip in my notebook back then didn't care about
> > alignment or I just got lucky with the memory allocations.
> 
> You might not see problems with your hardware/architecture.
> 
> "Documentation/DMA-API-HOWTO.txt" discusses about "What memory is DMA'able?".

Makes sense.  I've read your patch over and it also makes sense.
Acked-by: Joern Engel <joern@logfs.org>

Jörn

--
It's just what we asked for, but not what we want!
-- anonymous

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Brian Norris]

On Tue, Aug 06, 2013 at 03:03:29PM +0300, Jussi Kivilinna wrote:
> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
> be DMA-able, which stack is not.
> 
> Patch is only compile tested.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
> ---
>  drivers/mtd/nand/alauda.c |   74 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 18 deletions(-)

Just FYI, this driver is being removed, so I'm obviously not taking this
patch :)

Brian

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Bjørn Mork]

Brian Norris <computersforpeace@gmail.com> writes:
> On Tue, Aug 06, 2013 at 03:03:29PM +0300, Jussi Kivilinna wrote:
>> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
>> be DMA-able, which stack is not.
>> 
>> Patch is only compile tested.
>> 
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
>> ---
>>  drivers/mtd/nand/alauda.c |   74 ++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 56 insertions(+), 18 deletions(-)
>
> Just FYI, this driver is being removed, so I'm obviously not taking this
> patch :)

I think you should apply it anyway. The driver is still in v3.11 AFAICS,
and the patch should also go to the maintained stable kernels. You
cannot remove the driver from them, and I don't see a later driver
removal as a valid reason not to fix a known bug with a patch.


Bjørn

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Brian Norris]

On Wed, Aug 21, 2013 at 09:59:27AM +0200, Bjørn Mork wrote:
> Brian Norris <computersforpeace@gmail.com> writes:
> > On Tue, Aug 06, 2013 at 03:03:29PM +0300, Jussi Kivilinna wrote:
> >> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
> >> be DMA-able, which stack is not.
> >>
> >> Patch is only compile tested.
> >>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
> >> ---
> >>  drivers/mtd/nand/alauda.c |   74 ++++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 56 insertions(+), 18 deletions(-)
> >
> > Just FYI, this driver is being removed, so I'm obviously not taking this
> > patch :)
>
> I think you should apply it anyway. The driver is still in v3.11 AFAICS,
> and the patch should also go to the maintained stable kernels. You
> cannot remove the driver from them, and I don't see a later driver
> removal as a valid reason not to fix a known bug with a patch.

Seriously?

The reasons given for removal:

  "The driver has very low utility.  Devices in question are limited to
  about 400kB/s and the only known user (me) discarded the hardware
  several years back."

And:

  "Maybe we should just remove the driver and not spend any more time on
  it?"

So you're suggesting applying an untested (compile-only) fix for an
unobserved bug for the theoretical user of old, slow hardware who wants
to use a recent stable kernel, when the last known user has given up on
the driver entirely?

Anyway, just because you complained, I rebased and applied this to
l2-mtd.git before the driver removal. I'll defer to dwmw2 whether this
patch gets squashed out of existence.

Brian

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Jussi Kivilinna]

On 21.08.2013 11:41, Brian Norris wrote:
> On Wed, Aug 21, 2013 at 09:59:27AM +0200, Bjørn Mork wrote:
>> Brian Norris <computersforpeace@gmail.com> writes:
>>> On Tue, Aug 06, 2013 at 03:03:29PM +0300, Jussi Kivilinna wrote:
>>>> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
>>>> be DMA-able, which stack is not.
>>>>
>>>> Patch is only compile tested.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
>>>> ---
>>>>  drivers/mtd/nand/alauda.c |   74 ++++++++++++++++++++++++++++++++++-----------
>>>>  1 file changed, 56 insertions(+), 18 deletions(-)
>>>
>>> Just FYI, this driver is being removed, so I'm obviously not taking this
>>> patch :)
>>
>> I think you should apply it anyway. The driver is still in v3.11 AFAICS,
>> and the patch should also go to the maintained stable kernels. You
>> cannot remove the driver from them, and I don't see a later driver
>> removal as a valid reason not to fix a known bug with a patch.
> 
> Seriously?
> 
> The reasons given for removal:
> 
>   "The driver has very low utility.  Devices in question are limited to
>   about 400kB/s and the only known user (me) discarded the hardware
>   several years back."
> 
> And:
> 
>   "Maybe we should just remove the driver and not spend any more time on
>   it?"
> 
> So you're suggesting applying an untested (compile-only) fix for an
> unobserved bug for the theoretical user of old, slow hardware who wants
> to use a recent stable kernel, when the last known user has given up on
> the driver entirely?
> 
> Anyway, just because you complained, I rebased and applied this to
> l2-mtd.git before the driver removal. I'll defer to dwmw2 whether this
> patch gets squashed out of existence.

If this is the case, please ignore this patch. I should not have marked it for stable in first place, since it is just compile-tested.

-Jussi

> 
> Brian
> 

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Brian Norris]

On Wed, Aug 21, 2013 at 2:59 AM, Jussi Kivilinna <jussi.kivilinna@iki.fi> wrote:
> On 21.08.2013 11:41, Brian Norris wrote:
>> On Wed, Aug 21, 2013 at 09:59:27AM +0200, Bjørn Mork wrote:
>>> Brian Norris <computersforpeace@gmail.com> writes:
>>>> On Tue, Aug 06, 2013 at 03:03:29PM +0300, Jussi Kivilinna wrote:
>>>>> Patch fixes alauda not to use stack as URB transfer_buffer. URB buffers need to
>>>>> be DMA-able, which stack is not.
>>>>>
>>>>> Patch is only compile tested.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
>>>>> ---
>>>>>  drivers/mtd/nand/alauda.c |   74 ++++++++++++++++++++++++++++++++++-----------
>>>>>  1 file changed, 56 insertions(+), 18 deletions(-)
>>>>
>>>> Just FYI, this driver is being removed, so I'm obviously not taking this
>>>> patch :)
>>>
>>> I think you should apply it anyway. The driver is still in v3.11 AFAICS,
>>> and the patch should also go to the maintained stable kernels. You
>>> cannot remove the driver from them, and I don't see a later driver
>>> removal as a valid reason not to fix a known bug with a patch.
>>
>> Seriously?
>>
>> The reasons given for removal:
>>
>>   "The driver has very low utility.  Devices in question are limited to
>>   about 400kB/s and the only known user (me) discarded the hardware
>>   several years back."
>>
>> And:
>>
>>   "Maybe we should just remove the driver and not spend any more time on
>>   it?"
>>
>> So you're suggesting applying an untested (compile-only) fix for an
>> unobserved bug for the theoretical user of old, slow hardware who wants
>> to use a recent stable kernel, when the last known user has given up on
>> the driver entirely?
>>
>> Anyway, just because you complained, I rebased and applied this to
>> l2-mtd.git before the driver removal. I'll defer to dwmw2 whether this
>> patch gets squashed out of existence.
>
> If this is the case, please ignore this patch. I should not have marked it for stable in first place, since it is just compile-tested.

Yes, that's a good point. Quoting Documentation/stable_kernel_rules.txt:

"It must be obviously correct and tested."

Seeing as it was not tested, I am dropping the patch entirely (it is
not stable material, and there is no point including it along with the
driver removal).

Thanks,
Brian

Re: [PATCH] alauda: do not use stack for URB transfer_buffers

[Jörn Engel]

On Wed, 21 August 2013 13:00:15 -0700, Brian Norris wrote:
> 
> Yes, that's a good point. Quoting Documentation/stable_kernel_rules.txt:
> 
> "It must be obviously correct and tested."
> 
> Seeing as it was not tested, I am dropping the patch entirely (it is
> not stable material, and there is no point including it along with the
> driver removal).

Thank you!  Without quoting rules, this also fails the common sense
test.  For a stable patch, one might assume a non-empty group of
people that would benefit from said patch and care enough to install
the patch.

Jörn

--
I have always found the word "Europe" in the mouth of those politicians
that were demanding something from other powers they did not dare demand
in their own name.
-- Otto von Bismarck