Re: [PATCH v2] staging: dgnc: replace DGNC_VERIFY_BOARD macro with dgnc_verify_board function.

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Oct 10, 2016 at 11:00:16AM -0300, Elise Lennion wrote:
> Fix checkpatch warning:
> 
> WARNING: Macros with flow control statements should be avoided
> 
> Macros with flow control statements should be avoided. They break
> the flow of the calling function and make it harder to test the code.
> 
> Signed-off-by: Elise Lennion <elise.lennion@gmail.com>
> ---
> 
> v2: Fixed the return values of dgnc_verify_board. 
>     Reworked to return NULL instead of 0, as suggested 
>     by Julia Lawall <julia.lawall@lip6.fr>.
> 
>  drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> index 290bf6e..cdec40a 100644
> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> @@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver)
>  	driver_remove_file(driverfs, &driver_attr_pollrate);
>  }
>  
> -#define DGNC_VERIFY_BOARD(p, bd)				\
> -	do {							\
> -		if (!p)						\
> -			return 0;				\
> -								\
> -		bd = dev_get_drvdata(p);			\
> -		if (!bd || bd->magic != DGNC_BOARD_MAGIC)	\
> -			return 0;				\
> -		if (bd->state != BOARD_READY)			\
> -			return 0;				\
> -	} while (0)
> +static struct dgnc_board *dgnc_verify_board(struct device *p)
> +{
> +	struct dgnc_board *bd;
> +
> +	if (!p)
> +		return NULL;
> +
> +	bd = dev_get_drvdata(p);
> +	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
> +		return NULL;
> +	if (bd->state != BOARD_READY)
> +		return NULL;
> +
> +	return bd;
> +}

While this is a nice cleanup, take a step back and see what is being
done with this code.

First off, (!p) can never be true, so checking for it shows that the
original author didn't understand how sysfs worked.  After that, bd will
also always be not NULL, as the driver already set that up.  Same for
the ->magic field, that's pointless to check as it can't be wrong.

So maybe, BOARD_READY matters, but that's impossible to be not correct
as it is set before the sysfs files are created, and never changes from
what I can tell.

So all of these checks are pointless and can just be removed.

But then, step back even further, what code is checking for these
things?  It's odd sysfs files that I have said need to be removed from
the driver as they are pointless and not something that any tty driver
should have.

So how about just dropping the sysfs files instead?  That deletes a
bunch of code (some of it wrong and pointless as described above), gets
the driver closer to being mergable out of the staging directory, and it
keeps anyone from ever copying bad coding patterns like this ever again.

thanks,

greg k-h