warn_unused_result for fsp_queue_msg()

I recently added a bunch of __warn_unused_result around the place.

I tried adding it here to see what would happen:


and funnily enough, I ended up getting a *lot* of warnings.

It seems we're quite sporadic in checking the result of fsp_queue_msg.

there's a bunch of places in fsp.c which are all early boot and if we
ever fail we should probably just abort - as this likely means we've
somehow really screwed up by not detecting an FSP (thus active_fsp is
NULL... I don't *think* there's anywhere but early boot where this could
be the case) or we fail at allocating the fsp message (and ENOMEM during
early boot is... well.. abort worthy).

The bits that are interesting are runtime conditions... where we could
run out of memory (eep!) and the typical call pattern is
fsp_queue_msg(fsp_mkmsg()).

So... what are peoples thoughts on this?

I'm rather tempted to enable the warning and then force us to go and fix
everywhere... and in the meantime spew warnings.

On 11/27/2014 11:30 AM, Stewart Smith wrote:
> I recently added a bunch of __warn_unused_result around the place.
>
> I tried adding it here to see what would happen:
>
> --- a/include/fsp.h
> +++ b/include/fsp.h
> @@ -661,7 +661,7 @@ extern void fsp_cancelmsg(struct fsp_msg *msg);
>    * commands to the FSP.
>    */
>   extern int fsp_queue_msg(struct fsp_msg *msg,
> -                        void (*comp)(struct fsp_msg *msg));
> +                        void (*comp)(struct fsp_msg *msg)) __warn_unused_result;
>
>   /* Synchronously send a command. If there's a response, the status is
>    * returned as a positive number. A negative result means an error
>
> and funnily enough, I ended up getting a *lot* of warnings.
>
> It seems we're quite sporadic in checking the result of fsp_queue_msg.
>
> there's a bunch of places in fsp.c which are all early boot and if we
> ever fail we should probably just abort - as this likely means we've
> somehow really screwed up by not detecting an FSP (thus active_fsp is
> NULL... I don't *think* there's anywhere but early boot where this could
> be the case) or we fail at allocating the fsp message (and ENOMEM during
> early boot is... well.. abort worthy).
>
> The bits that are interesting are runtime conditions... where we could
> run out of memory (eep!) and the typical call pattern is
> fsp_queue_msg(fsp_mkmsg()).
>
> So... what are peoples thoughts on this?
>
> I'm rather tempted to enable the warning and then force us to go and fix
> everywhere... and in the meantime spew warnings.

Make sense..  There are quite a few warning coming from drivers like LED, 
EPOW/DPO, fsp etc..

I'm making changes to LED driver.. Will include these changes as well.

-Vasant

On Thu, Nov 27, 2014 at 05:00:02PM +1100, Stewart Smith wrote:
> I recently added a bunch of __warn_unused_result around the place.
> 
> I tried adding it here to see what would happen:
> 
> --- a/include/fsp.h
> +++ b/include/fsp.h
> @@ -661,7 +661,7 @@ extern void fsp_cancelmsg(struct fsp_msg *msg);
>   * commands to the FSP.
>   */
>  extern int fsp_queue_msg(struct fsp_msg *msg,
> -                        void (*comp)(struct fsp_msg *msg));
> +                        void (*comp)(struct fsp_msg *msg)) __warn_unused_result;
>  
>  /* Synchronously send a command. If there's a response, the status is
>   * returned as a positive number. A negative result means an error
> 
> and funnily enough, I ended up getting a *lot* of warnings.
> 
> It seems we're quite sporadic in checking the result of fsp_queue_msg.
> 
> there's a bunch of places in fsp.c which are all early boot and if we
> ever fail we should probably just abort - as this likely means we've
> somehow really screwed up by not detecting an FSP (thus active_fsp is
> NULL... I don't *think* there's anywhere but early boot where this could
> be the case) or we fail at allocating the fsp message (and ENOMEM during
> early boot is... well.. abort worthy).
> 
> The bits that are interesting are runtime conditions... where we could
> run out of memory (eep!) and the typical call pattern is
> fsp_queue_msg(fsp_mkmsg()).
> 
> So... what are peoples thoughts on this?
> 
> I'm rather tempted to enable the warning and then force us to go and fix
> everywhere... and in the meantime spew warnings.

I second the plan. Please enable the warning.

Ananth