Re: [ath5k-devel] [PATCH] ath5k: Add debug code for EEPROM

[Jiri Slaby <jirislaby@gmail.com>]

Nick Kossifidis napsal(a):
> Hello Jiri and thanks a lot for the review ;-)
> 
> 2009/1/4 Jiri Slaby <jirislaby@gmail.com>:
>>> +/* debugfs: EEPROM stuff */
>>> +
>>> +/* EEPROM Header (common) */
>>> +static ssize_t read_file_eeprom_header(struct file *file, char __user *user_buf,
>>> +                                size_t count, loff_t *ppos)
>>> +{
>>> +
>>> +     struct ath5k_softc *sc = file->private_data;
>>> +     struct ath5k_hw *ah = sc->ah;
>>> +     struct ath5k_eeprom_info ee = ah->ah_capabilities.cap_eeprom;
>>> +     char buf[2000];
>> Please don't use that much memory from stack. 2k is way too much. Note that
>> you use yet another bunch of stack (next 3k here on 64-bit) by
>> ath5k_eeprom_info and 32-bit x86 can still be configured with 4k stacks.
>>
>> Convert both of them to dynamically allocated buffers.
>>
> 
> Why dynamically allocated buffers (can you point out some docs on
> these please ?) ? The length of each file (what we print) is standard.
> I understand that we use too much stack here but we can work this out
> eg. by using a pointer to ath5k_eeprom_info or something like that.

Yes, no problem in changing it to a pointer. Anyway the buf should be
kmalloc'ed anyway.

> I
> checked out user_buffer btw and it's 4K, sometimes it's not much for
> calibration data (eg. on RF5111 that we have 10 points per pd gain
> curve or RF2413+ that we can have multiple pd gain curves per
> channel).

I don't understand here. user_buf is sized by userspace application. E.g.
cat on my system requests user_buf of size 1K. simple_read_from_buffer()
takes care about multiple invocations of fops.read function by looking at
ppos parameter and filling user_buf by correct contents (some offset in from
param). BTW as a side-effect, it is possible, that the user will get
different info in these split reads.

>> dtto
>>
> 
> What does dtto mean ?

Oops, as I checked right now, this shortcut is commonly used only in czech.
It should be detto or ditto (and it means "the same as above"), sorry for
the confusion.