Re: [RFC][PATCH] libata ATAPI alignment

Tejun Heo
2005-08-07T05:50:23Z

On Fri, Jul 29, 2005 at 01:06:54AM -0400, Jeff Garzik wrote:
>
> So, one thing that's terribly ugly about SATA ATAPI is that we need to
> pad DMA transfers to the next 32-bit boundary, if the length is not
> evenly divisible by 4.
>
> Messing with the scatterlist to accomplish this is terribly ugly
> no matter how you slice it. One way would be to create my own
> scatterlist, via memcpy and then manual labor. Another way would be
> to special case a pad buffer, appending it onto the end of various
> scatterlist code.
>
> Complicating matters, we currently must support two methods of data
> buffer submission: a single kernel virtual address, or a struct
> scatterlist.
>
> Review is requested by any and all parties, as well as suggestions for
> a prettier approach.
>
> This is one of the last steps needed to get ATAPI going.
>

Hello, Jeff, Jens & Alan.

I've rewritten the patch to fix some bugs and make it a bit (IMHO)
simpler to use.

Bug fixes...

* When copying a sg, original implementation just kmap'ed sg->page
which can cause trouble as a sg can span more than a page.

* In ata_sg_clean(), the original implementation accesses last sg by
indexing w/ (qc->n_elem - 1). This is incorrect as qc->n_elem
could have been shrunk by dma_map_sg() in ata_sg_setup().

* In the original code (before Jeff's patch), sata_sx4 used
sg[i].legnth to calculate total_len. This is wrong for the same
reason as above and Jeff's patch fixes it. I separated out this
fix just to clarify.

Changes...

* Instead of adding pad sg handling to each fill_sg function,
ata_for_each_sg() macro is added. Normal sg entries and
qc->pad_sgent are setup by sg_setup routines and fill_sg functions
can just iterate w/ ata_for_each_sg() without caring about padding.

* hw_max_segments is automatically decremented in slave_config if
attached device is ATAPI.

Questions/suggestions...

* I didn't include AHCI_MAX_SG change as it looked a bit more out of
place w/ other changes to ahci gone. Also, I'm curious about how
meaningful increasing AHCI_MAX_SG is. We're currently setting
max_phys_segments to LIBATA_MAX_PRD, which is 128, and AFAIK max hw
segments higher than phys segments is meaningless (phys segs are
merged into hw segs and one phys segment cannot be splitted into
two hw segs). Am I missing something here?

* About core routines/callbacks. Currently, libata-core file
contains actual libata core routines and all helper functions for
taskfile controllers. ata_ prefix is also shared by both function
categories. This is a bit confusing, IMO.

I think ata_port_start should allocate stuff necessary for libata
core and call ->port_start callback and similary for ata_port_stop,
and current ata_port_start/stop need to be renamed to something
like ata_tf_port_start/stop, such that we don't have to allocate
data structures needed by libata core in specific drivers (ahci in
this case).

I've tested both sg and non-sg paths with sg_test_rwbuf. When
testing sg path, I've commented out sg.c:L1951 (sg_build_indirect:L10)
to prevent it padding transfer size to 512byte boundary. Read/write
padding in both paths work.

Two patches will follow this mail. The first one fixes sata_sx4 as
mentioned above and the second implements atapi align.

Thanks.

--
tejun