On Mon, Oct 17, 2016 at 11:46:36PM +0200, Fernando Apesteguía wrote:
> On Mon, Oct 17, 2016 at 10:29 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Fri, Oct 14, 2016 at 07:22:57PM +0200, Fernando Apesteguia wrote:
> >> The patch replaces the macro with a function (dgnc_get_board) and
> >> substitutes the macro statement with a call to that function and a
> >> comparison on the returned value.
> >>
> >> This removes a checkpatch warning.
> >>
> >> Signed-off-by: Fernando Apesteguia <fernando.apesteguia@xxxxxxxxx>
> >> ---
> >> 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..3ea23a9 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_get_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;
> >> +}
> >
> > No, this macro should be removed entirely as what it does is pointless
> > in some parts, wrong in others, and not needed at all in the rest :(
> >
> > I've asked others to fix this up properly in the past, but it doesn't
> > seem like anyone wants to do the work...
> >
>
> I tried to find the discussion the relevant mails in lkml.org but
> couldn't find them. Could you point me to them so I can have a look?
Last time I mentioned this, it was on the outreachy-kernel mailing list.
Just walk through the code for yourself and see which, if any, of these
things could ever actually cause the function to "fail". I think you
will find that none of them can ever happen...
The first test is a huge proof that the original author didn't
understand how sysfs or "container_of()" works, given that it is
impossible to ever have happen.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
