[RFC]: Standardizing the SAL calls in arch/ia64

[RFC]: Standardizing the SAL calls in arch/ia64

[Prarit Bhargava]

[RFC]: Standardizing the SAL calls in arch/ia64

Hello all,

I recently did a small amount of work on a SAL call in the patch

http://www.gelato.unsw.edu.au/archives/linux-ia64/0512/16309.html

While coding for this patch I noticed several minor-medium issues with
the SAL calls.

1) Some SAL calls do not properly return a SAL status.
2) Many of the SAL calls do not properly set ia64_sal_retval values to 0.
3) Callers of the SAL calls are not doing error checking.
4) The SAL status returns are not #define'd anywhere.
5) Several "dead" calls remaining in the code base.
6) Lots of Documentation/CodingStyle fix ups needed...

After looking at the SAL calls in arch/ia64, I suggest that we standardize
the SAL calls by using:

#define SAL_CALL_RETURN(isrv) do {      \
         return isrv.status;             \
} while (0)

b) return status and isrv.v0,

#define SAL_CALL_RETURN_V0(isrv, value) do {    \
         *value = isrv.v0;                       \
         return isrv.status;                     \
} while (0)

#define SAL_RETVAL_DECLARE(isrv) do {                   \
         struct ia64_sal_retval isrv = { 0,0,0,0 };      \
}while (0)

Admittedly, there are two or three cases which do not fit this model, but
this goes a long way to cleaning up the code.

For example,

static inline u64
ia64_sal_get_state_info_size (u64 sal_info_type)
{
         struct ia64_sal_retval isrv;
         SAL_CALL_REENTRANT(isrv, SAL_GET_STATE_INFO_SIZE, sal_info_type,
			   0, 0, 0, 0, 0, 0);
         if (isrv.status)
                 return 0;
         return isrv.v0;
}

would become

static s64
ia64_sal_get_state_info_size (u64 sal_info_type, u64 *result)
{
         SAL_RETVAL_DECLARE(isrv);
         SAL_CALL_REENTRANT(isrv, SAL_GET_STATE_INFO_SIZE, sal_info_type,
			   0, 0, 0, 0, 0, 0);
         SAL_CALL_RETURN_V0(isrv,result);
}

Thoughts, concerns?

P.

Re: [RFC]: Standardizing the SAL calls in arch/ia64

[Jes Sorensen]

>>>>> "Prarit" = Prarit Bhargava <prarit@sgi.com> writes:

Prarit> After looking at the SAL calls in arch/ia64, I suggest that we
Prarit> standardize the SAL calls by using:

Prarit> #define SAL_CALL_RETURN(isrv) do { \ return isrv.status; \ }
Prarit> while (0)

Prarit> b) return status and isrv.v0,

Prarit> #define SAL_CALL_RETURN_V0(isrv, value) do { \ *value Prarit> isrv.v0; \ return isrv.status; \ } while (0)

Prarit> #define SAL_RETVAL_DECLARE(isrv) do { \ struct ia64_sal_retval
Prarit> isrv = { 0,0,0,0 }; \ }while (0)

Prarit> Thoughts, concerns?

Hi Prarit,

I agree with the principle, the current SAL code is an utter
mess. However, rather than obfuscating the code with a bunch of
macros, I'd prefer to simply stick a guideline at the top of the
relevant files and then change the code to match that. In particular
declaring variables within macros and macros that call 'return' aren't
good for making the code easy to read.

Cheers,
Jes

Re: [RFC]: Standardizing the SAL calls in arch/ia64

[Luck, Tony]

> I agree with the principle, the current SAL code is an utter
> mess. However, rather than obfuscating the code with a bunch of
> macros, I'd prefer to simply stick a guideline at the top of the
> relevant files and then change the code to match that. In particular
> declaring variables within macros and macros that call 'return' aren't
> good for making the code easy to read.

If the SAL stubs are sufficently regular, then you *might* make
a macro (or small set of macros) to generate them:

#define MK_VOID_SAL_STUB(fname, salfn)			\
static inline s64					\
fname(void)						\
{							\
	struct ia64_sal_retval isrv;			\
	SAL_CALL(isrv, salfn, 0, 0, 0, 0, 0, 0, 0);	\
	return isrv.status;				\
}

MK_VOID_SAL_STUB(ia64_sal_cache_init, SAL_CACHE_INIT)


But I think that you'd end up with lots of generator macros that are
only used once each unless you are astonishingly clever[1] with cpp macros,
so you'd have been better off to just write them in C.

Macros to generate functions can also be irritating as tools like
cscope can't show you the definition ... but they are used elsewhere
in the Linux kernel.

Summary: take a quick look at whether macros might help, if not then
just clean up the C code.

-Tony

[1] which can have its own issues of code legibility.