PATCH: Config option to pad SDIO sizes

[Johan Adolfsson]

> -----Original Message-----
> From: Pierre Ossman [mailto:drzeus-mmc at drzeus.cx] 
> Sent: den 28 juni 2008 12:59
> To: Dan Williams; Johan Adolfsson
> Cc: libertas-dev at lists.infradead.org
> Subject: Re: PATCH: Config option to pad SDIO sizes
> 
> 
> On Fri, 27 Jun 2008 14:26:50 -0400
> Dan Williams <dcbw at redhat.com> wrote:
> 
> > 
> > Sounds like a good start to me; only the HD knows what it's padding
> > requirements are, and in the unlikely case where there are 
> two different
> > HDs in the same system with libertas devices attached, 
> there might be
> > two different padding requirements which each libertas 
> instance would
> > need to keep track of.
> > 
> 
> Suggested patch included. It also serves as a nice example of why I
> consider this problem too complex to force onto the 
> individual drivers.
> 
> I've tested it here on my Ricoh controller, but please test with
> whatever you have access to over there.
> 
> -- 
>      -- Pierre Ossman
> 
>   Linux kernel, MMC maintainer        http://www.kernel.org
>   rdesktop, core developer          http://www.rdesktop.org
> 
>   WARNING: This correspondence is being monitored by the
>   Swedish government. Make sure your server uses encryption
>   for SMTP traffic and consider using PGP for end-to-end
>   encryption.

Havent tested, but I guess it makes sence to move it up from the driver,
but here are some comments/thoughts:

+unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz)
..
+	 */
+	if (sz & 3)
+		sz = ((sz + 3) / 4) * 4;
+
+	return sz;

I suggest skip the if(), since it doesn't save any instructions
compared to always doing the padding (not for x86, cris or arm at least).
Isn't: sz = (sz + 3) &~3; clearer or can we trust all compilers to do 
the right thing here?

Possibly make the function inline ?


+	/*
+	 * If we can still do this with just a byte transfer, then
+	 * we're done.
+	 */
+	if ((sz < func->cur_blksize) && (sz < 512))
+		return sz;
vs

-	chunk = size;
-	if ((chunk > card->func->cur_blksize) || (chunk > 512)) {
-		chunk = (chunk + card->func->cur_blksize - 1) /
-			card->func->cur_blksize * card->func->cur_blksize;
-	}

indicates that it should really be: (note the added =)

+	if ((sz <= func->cur_blksize) && (sz <= 512))
+		return sz;

Shouldn't it?


Is the added multi_block stuff really related to the pad size
or just needed anyway ?
Isn't it a valid assumption that the cur_blksize is 32 bit aligned so that
aligning size would not increase the size over the cur_blksize if 
it was not already (and that is handled elsewhere)?
(Sorry - I have no indepth knowledge of sdio - so I may be way off here,
 but it just looks like a lot of extra overhead)

Best regards
/Johan