Re: [PATCH 0/4] revert the commit 22a9189f (cdrom: use kmalloced buffers instead of buffers on stack)

Bartlomiej Zolnierkiewicz
2008-05-27T18:58:55Z

On Tuesday 20 May 2008, FUJITA Tomonori wrote:
> The goal of this patchset is reverting the commit
> 22a9189fd073db3d03a4cf8b8c098aa207602de1 (cdrom: use kmalloced buffers
> instead of buffers on stack).
>
> http://lkml.org/lkml/2008/4/21/634
>
> The commit is using kmalloced buffers for cdrom packet commands to
> avoid stack corruption on non coherent platforms. But allocating a
> small buffer like this is not nice (unnecessary complicity):
>
> + buffer = kmalloc(8, GFP_KERNEL);
>
> I tried to remove generic_packet() and convert cdrom users to use the
> block queue like pkt_generic_packet (as Jens suggested in the thread
> if I correctly understand), but it turned out that it needs tricky
> surgery (like handling ssleep and retries for packet commands in ide).
>
> Then I found that we can easily handle packet commands on non coherent
> platforms. The diffstat is pretty small except for the revert. All
> this patchset does is just setting the dma_pad_mask to
> ARCH_KMALLOC_MINALIGN.
>
> Only scsi and ide-cd do DMA generic_packet. In the case of scsi,
> sr_packet uses blk_rq_map_kern (the commit
> 68154e90c9d1492d570671ae181d9a8f8530da55) post 2.6.25. So if we set
> the dma padding on non coherent platforms, sr_packet uses allocated
> pages properly.
>
> In the case of IDE, ide-cd has a mechanism to handle alignment and
> padding for SG_IO. So we can easily exploit it for packet commands.
>
> If some architectures can't do DMA on stack, we also need to a new
> queue_flag like QUEUE_FLAG_NO_DMA_ON_STACK in addtion of this
> patchset.
>
> The diffstat is:
>
> block/blk-settings.c | 30 +++++-
> drivers/ata/libata-scsi.c | 3 +-
> drivers/cdrom/cdrom.c | 274 +++++++++++++++------------------------------
> drivers/ide/ide-cd.c | 17 ++-
> include/linux/blkdev.h | 1 +
> 5 files changed, 134 insertions(+), 191 deletions(-)

The patchset looks good to me after a quick look but Borislav is a more
appropriate person to contact on ide-cd specific patches.

Thanks,
Bart