On 28 November 2012 18:12, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> wrote: > On Wed, Nov 28, 2012 at 05:55:13PM +0100, Per Forlin wrote: >> I have tried to work out an if-statement to check this properly. Here >> is my conclusion, >> This only works if sg len is 1 (in the SDIO case) >> >> if (WRITE) >> align = sg->offset & 3 >> else if (READ) >> align = 0 >> >> if (sg->offset & 3 && (PAGE_SIZE - (sg->offset + align) < host->size)) >> error; /* cannot be handled by mmci driver */ > > This looks wrong. For starters, why in the write case do you _add_ the > byte offset in the word to sg->offset ? So, if we're offset by one byte, > it becomes two bytes. Makes no sense. > > Secondly, (PAGE_SIZE - whatever) < host->size will be mostly always true. > What that means is the check is effectively just "sg->offset & 3". > > Ah, maybe that's what you're trying to do - obfuscate the code in the hope > that I won't analyse it so you can get your check past me and then remove > all the obfuscation later... but maybe I'm just paranoid. :) > >> Is this if-statement align with your view of the alignment issue ? > > I've no idea why you're making it soo complicated. > > Let's review. It copes fine with aligned buffers. It copes fine with > misaligned buffers that don't cross a page boundary. It also copes > fine with transfers that end with a non-word sized number of bytes > (which we don't need a check for). > > So: > > /* We can't cope with a misaligned transfer which crosses a page */ > if (sg->offset & 3 && (sg->offset + sg->len > PAGE_SIZE || > data->sg_len > 1)) > bad; This is ok for read but not for write. If your data also resides in any of the last 3 bytes of the PAGE, you might get page faults since at the final writesl you will access data outside the PAGE. That is what pseudo code was trying to accomplish as well. You might want to solve this similarly as we did for pio_read with a local cache buffer in pio_write. That should not be that hard to fix. Do you think this is what we want? > > See what I've done there? I've taken the English and converted it directly > to code. And the comment also matches the code. > > And this way we continue to allow buffers that the string-IO operations > can cope with just fine, but avoid the issue of a word crossing a page > boundary. Some overall ideas we might want to consider, maybe as a the second step. >From mmci perspective it is good that we can cope with any constraints at all from alignment perspective. Although, that will require a quite extensive hacks in both pio_read and pio_write. By looking into how other host drives has solved this, for example "dw_mci_push*" functions gives you an idea. In the end this will mean complicated code which will be affecting "hot path" and maybe performance. Do we think this is ok? Is it worth a try? Per and myself was kind of going in the opposite direction, thus keep code simple and make sure it is optimized. On the other hand it means putting constraints on buffer alignment. Right now there is no SDIO API to tell SDIO clients about alignments constraints. Additionally drivers is normally interested in using buffers that can be handled in an optimized manner. Kind regards Ulf Hansson > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
