| Message ID | 20130806120329.8864.36301.stgit@localhost6.localdomain6 |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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
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
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 >
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
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
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);
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(-)