Re: [PATCH 17/20] misc: genwqe: card_base: Do not pass unused argument 'fatal_err'

[haver <haver@linux.vnet.ibm.com>]

On 2020-06-29 16:04, Lee Jones wrote:
> 'fatal_err' is taken as an argument to a static function which is only
> invoked once.  During this invocation 'fatal_err' is not set.  There
> doesn't appear to be a good reason to keep it around.
> 
> Also fixes the following W=1 kernel build warning:
> 
>  drivers/misc/genwqe/card_base.c:588: warning: Function parameter or
> member 'fatal_err' not described in 'genwqe_recover_card'
> 
> Cc: Michael Jung <mijung@gmx.net>
> Cc: Michael Ruettger <michael@ibmra.de>
> Cc: Frank Haverkamp <haver@linux.ibm.com>
> Cc: Joerg-Stephan Vogt <jsvogt@de.ibm.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/misc/genwqe/card_base.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/misc/genwqe/card_base.c 
> b/drivers/misc/genwqe/card_base.c
> index bceebf49de2d5..809a6f46f6de3 100644
> --- a/drivers/misc/genwqe/card_base.c
> +++ b/drivers/misc/genwqe/card_base.c
> @@ -569,30 +569,18 @@ static int genwqe_stop(struct genwqe_dev *cd)
> 
>  /**
>   * genwqe_recover_card() - Try to recover the card if it is possible
> - *
> - * If fatal_err is set no register access is possible anymore. It is
> - * likely that genwqe_start fails in that situation. Proper error
> - * handling is required in this case.
> + * @cd: GenWQE device information
>   *
>   * genwqe_bus_reset() will cause the pci code to call genwqe_remove()
>   * and later genwqe_probe() for all virtual functions.
>   */
> -static int genwqe_recover_card(struct genwqe_dev *cd, int fatal_err)
> +static int genwqe_recover_card(struct genwqe_dev *cd)
>  {
>  	int rc;
>  	struct pci_dev *pci_dev = cd->pci_dev;
> 
>  	genwqe_stop(cd);
> 
> -	/*
> -	 * Make sure chip is not reloaded to maintain FFDC. Write SLU
> -	 * Reset Register, CPLDReset field to 0.
> -	 */
> -	if (!fatal_err) {
> -		cd->softreset = 0x70ull;
> -		__genwqe_writeq(cd, IO_SLC_CFGREG_SOFTRESET, cd->softreset);
> -	}
> -
>  	rc = genwqe_bus_reset(cd);
>  	if (rc != 0) {
>  		dev_err(&pci_dev->dev,
> @@ -958,7 +946,7 @@ static int genwqe_health_thread(void *data)
> 
>  			cd->card_state = GENWQE_CARD_FATAL_ERROR;
> 
> -			rc = genwqe_recover_card(cd, 0);
> +			rc = genwqe_recover_card(cd);
>  			if (rc < 0) {
>  				/* FIXME Card is unusable and needs unbind! */
>  				goto fatal_error;

I think this one I want to keep. Since fatal_err is 0, !fatal_error is 1 
and the register write is actually executed.
I also want to keep the parameter even though it is only used with 0. 
The register bit causes a less drastic recovery, but if we would 
discover that that is not working ok, we rather discard the debug data 
we get in this case instead of letting the recovery fail. I think it 
does not hurt to keep it. Maybe you can add a comment?

Thanks

Frank