linux-kernel - Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 03 Sep 2006 12:03:56 +0200
From:	Pierre Ossman <drzeus-list@...eus.cx>
To:	Alex Dubov <oakad@...oo.com>
CC:	linux-kernel@...r.kernel.org
Subject: Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash
 card readers

Alex Dubov wrote:
>> tifm_sd_fetch_resp() could be redone as a for loop
>> to make it more
>> obvious what's going on.
>>     
> I'm not sure it's a good idea. The response value is
> returned in 8 16-bit registers, which are mapped over
> 8 32-bit registers (so that only LS part of each
> register is valid). Additionally, the fetch order is
> reversed, so cmd->resp[0] is fetched from offsets 24
> and 28, while cmd->resp[3] is fetched from offsets 0
> and 4. To write this as a loop requires moderately
> complex address calculation that may look even less
> obvious.
>
>   

I suppose it's a matter of taste, but personally I think the mere
mentioning of 'for' allows you to directly see that there is some kind
of looping involved. And it shouldn't be terribly complex:

for (i = 0;i < 8;i++) {
    resp[i] = readw(addr + RESPONSE + (7 - i)*4) << 16;
    resp[i] |= readw(addr + RESPONSE + (6 - i)*4);
}

>> You should probably rename tifm_sd_set_data_to(). It
>> isn't obvious that
>> 'to' stands for 'timeout'. Same thing with other
>> instances of 'to'.
>>     
> I agree, yet I wanted to retain the names of the
> registers as defined in datasheet (so it's easier to
> search for them; for some reason it always abbreviates
> timeout as TO). Apparently TI does the same in their
> drivers.
>
>   

The problem is that it's a big difference between seeing "data TO" and
seeing "data to" in the code. How about using the three letter
abbreviations in those places? I.e. "cto" and "dto"?

>> What I'd like to see from you is to double check
>> that bytes_xfered is
>> set to the number of bytes successfully sent to the
>> _card_, not the
>> controller. This is critical for correct handling of
>> bus errors.
>>     
> The OMAP datasheet is somewhat unclear, but I think
> that block and byte counters truly represent the
> amount of data shifted out to the mmc bus. Whether
> this data really reaches the flash memory I don't know
> to tell.
>
>   

Hmm.... I'm planning on putting together a test module to determine this
(as my specs aren't crystal clear on the subject either). I'll try to
remember to send it to you. :)

Rgds
Pierre


-- 
VGER BF report: U 0.5
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/