Re: [PATCH RFC v2 2/2] rt2x00: Add debugfs access for rfcsr register

[Ivo Van Doorn <ivdoorn@gmail.com>]

Hi,

On Thu, Apr 19, 2012 at 11:20 AM, Anisse Astier <anisse@astier.eu> wrote:
> RFCSR is only used in rt2800. For other chipsets, the debug struct
> for rfcsr should be zeroed, so it shouldn't be an issue.
>
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> ---
> Changes since v1:
>  - removed initialization in chipsets other than rt2800, we now rely on
>   default initialization.

Thanks, it looks much cleaner now. :)

> diff --git a/drivers/net/wireless/rt2x00/rt2800.h b/drivers/net/wireless/rt2x00/rt2800.h
> index d91f4f6..9348521 100644
> --- a/drivers/net/wireless/rt2x00/rt2800.h
> +++ b/drivers/net/wireless/rt2x00/rt2800.h
> @@ -102,6 +102,8 @@
>  #define BBP_SIZE                       0x00ff
>  #define RF_BASE                                0x0004
>  #define RF_SIZE                                0x0010
> +#define RFCSR_BASE                     0x0000
> +#define RFCSR_SIZE                     0x0040
>
>  /*
>  * Number of TX queues.
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index 6c95101..0cdbf2b 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -836,6 +836,13 @@ const struct rt2x00debug rt2800_rt2x00debug = {
>                .word_size      = sizeof(u32),
>                .word_count     = RF_SIZE / sizeof(u32),
>        },
> +       .rfcsr  = {
> +               .read           = rt2800_rfcsr_read,
> +               .write          = rt2800_rfcsr_write,
> +               .word_base      = RFCSR_BASE,
> +               .word_size      = sizeof(u8),
> +               .word_count     = RFCSR_SIZE / sizeof(u8),
> +       },
>  };
>  EXPORT_SYMBOL_GPL(rt2800_rt2x00debug);
>  #endif /* CONFIG_RT2X00_LIB_DEBUGFS */
> diff --git a/drivers/net/wireless/rt2x00/rt2x00debug.c b/drivers/net/wireless/rt2x00/rt2x00debug.c
> index 78787fc..128dac7 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00debug.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00debug.c
> @@ -70,6 +70,7 @@ struct rt2x00debug_intf {
>         *     - eeprom offset/value files
>         *     - bbp offset/value files
>         *     - rf offset/value files
> +        *     - rfcsr offset/value files
>         *   - queue folder
>         *     - frame dump file
>         *     - queue stats file
> @@ -89,6 +90,8 @@ struct rt2x00debug_intf {
>        struct dentry *bbp_val_entry;
>        struct dentry *rf_off_entry;
>        struct dentry *rf_val_entry;
> +       struct dentry *rfcsr_off_entry;
> +       struct dentry *rfcsr_val_entry;
>        struct dentry *queue_folder;
>        struct dentry *queue_frame_dump_entry;
>        struct dentry *queue_stats_entry;
> @@ -131,6 +134,7 @@ struct rt2x00debug_intf {
>        unsigned int offset_eeprom;
>        unsigned int offset_bbp;
>        unsigned int offset_rf;
> +       unsigned int offset_rfcsr;
>  };
>
>  void rt2x00debug_update_crypto(struct rt2x00_dev *rt2x00dev,
> @@ -448,6 +452,8 @@ static ssize_t rt2x00debug_read_##__name(struct file *file, \
>        if (*offset)                                            \
>                return 0;                                       \
>                                                                \
> +       /* This is should always be checked first because       \
> +        * debug->__name could be zeroed (when unspecified) */  \
>        if (index >= debug->__name.word_count)                  \
>                return -EINVAL;                                 \

Actually it would be nicer to not create these files at all when the
register was not provided.
This means checking during initialization if the files should be created or not.


> @@ -640,6 +649,10 @@ static struct dentry *rt2x00debug_create_file_chipset(const char *name,
>                        debug->rf.word_base,
>                        debug->rf.word_count,
>                        debug->rf.word_size);
> +       data += sprintf(data, "rfcsr\t%d\t%d\t%d\n",
> +                       debug->rfcsr.word_base,
> +                       debug->rfcsr.word_count,
> +                       debug->rfcsr.word_size);

This should only be done if the rfcsr is actually available.
So I think whole block should be changed to check the availability of
each register.
Also you are adding a line here, have you ensures that data contains
sufficient data?
I know by default we are allocating a couple of bytes extra, but we
reserve X bytes per line,
so adding a line should require some extra data to be allocated.

>        blob->size = strlen(blob->data);
>
>        return debugfs_create_blob(name, S_IRUSR, intf->driver_folder, blob);
> @@ -719,6 +732,7 @@ void rt2x00debug_register(struct rt2x00_dev *rt2x00dev)
>        RT2X00DEBUGFS_CREATE_REGISTER_ENTRY(intf, eeprom);
>        RT2X00DEBUGFS_CREATE_REGISTER_ENTRY(intf, bbp);
>        RT2X00DEBUGFS_CREATE_REGISTER_ENTRY(intf, rf);
> +       RT2X00DEBUGFS_CREATE_REGISTER_ENTRY(intf, rfcsr);

So if we update the RT2X00DEBUGFS_CREATE_REGISTER_ENTRY() define,
we can ensure the debugfs files are only created when the register is present
on the device.

Ivo