Re: [PATCH][kgdb] Make mem access function weak in kgdb.c.

[Jason Wessel <jason.wessel@windriver.com>]

sonic zhang wrote:
> L1 instruction memory and MMR memory on blackfin can not be accessed by
> common functions probe_kernel_read() and probe_kernel_write().
> Blackfin asks for 2/4 byte align access to MMR memory and DMA access to L1
> instruction memory. These functions need to be reimplemented in
> architecture specific kgdb.c.
>

This patch is a reasonable change if you are looking to use the common
kgdb code, however your patch needs some further work in that the
documentation and prototypes in include/linux/kgdb.h file need to be
updated as well.

--- functions from your patch ---
>
> -int kgdb_mem2hex(char *mem, char *buf, int count)
> +int __weak kgdb_mem2hex(char *mem, char *buf, int count)


> -static int kgdb_ebin2mem(char *buf, char *mem, int count)
> +int __weak kgdb_ebin2mem(char *buf, char *mem, int count)


> -int kgdb_hex2mem(char *buf, char *mem, int count)
> +int __weak kgdb_hex2mem(char *buf, char *mem, int count)
--------------------

In include/linux/kgdb.h you have:
    270 extern int kgdb_mem2hex(char *mem, char *buf, int count);
    271 extern int kgdb_hex2mem(char *buf, char *mem, int count);

The above two lines need to be changed by your patch, as well as
adding a definition for the ebin2mem, because we do not want to make
new sparse warnings run have something link to the wrong function with
the various revs of gcc out there.

To fix the documentation, you add some extra information to the
comments preceding the functions prototype in include/linux/kgdb.h,
there are other examples in the same file. IE:

/**
 *      kgdb_mem2hex - (optional arch overide) traslate bin to hex chars
 *      @mem: source buffer
 *	@buf: target buffer
 *      @count: number of bytes in mem
 *
 *      Archictures which do not support probe_kernel_(read|write), 
 *	can make an alternate implmentation of this function.
 *	This function safely reads memory to into hex
 *	characters for use with the kgdb protocol.
 */
int __weak kgdb_mem2hex(char *mem, char *buf, int count);



The other approach here would be to consider changing the
probe_kernel_write() and probe_kernel_read() to work safely on the
black fin architecture, then you would not need to have an
architecture specific override for these kgdb internals.  In the long
run, I suspect other kernel functionality may use the probe_kernel_*
functions so you might consider this.  Regardless of the direction you
choose, I have no strong objection to accepting your patch if you make
the requested changes.

Thanks,
Jason.