lkml.org 
[lkml]   [2016]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver
Date
> >
> >> > +#define ndev_pdev(ndev) ((ndev)->ntb.pdev) #define
> ndev_name(ndev)
> >> > +pci_name(ndev_pdev(ndev)) #define ndev_dev(ndev)
> >> > +(&ndev_pdev(ndev)->dev) #define ntb_ndev(ntb) container_of(ntb,
> >> > +struct amd_ntb_dev, ntb) #define hb_ndev(work) container_of(work,
> >> > +struct amd_ntb_dev,
> >> hb_timer.work)
> >> > +#define ntb_hotplug_ndev(context) (container_of((context), \
> >> > + struct ntb_acpi_hotplug_context, hp)->ndev)
> >>
> >> Seems like these are hiding things too. Please use them directly (or
> >> at least put them in the C file and not the header file).
> >
> > I like these macros for up/down casting. Putting them close to the
> structure definition seems appropriate to me, too. I would rather see them
> moved to right below the definition of struct amd_ntb_dev, instead of to the
> c file. That is my opinion, but Jon can make the final decision on it.
>
> My opinion wasn't super strong on these. If Allen is fine with them, then
> good enough for me :)

Agree with Allen's opinion.

>
> > However, these in particular are buggy:
> >
> >> > +#define ntb_ndev(ntb) container_of(ntb, struct amd_ntb_dev, ntb)
> >> > +#define hb_ndev(work) container_of(work, struct amd_ntb_dev,
> >> hb_timer.work)
> >
> > Note: "ntb" will be replaced in all occurrences to the right. This only works
> if the name "ntb" is passed as the argument. If the argument is named "foo",
> it will either fail at compile time to find the member "foo" in struct
> amd_ntb_dev, or worse, it will hide a bug accessing the wrong member of
> the struct.
> >
> > Rename the macro parameter __ntb.
>
> Good call. Please make the necessary mods Xiangliang.

Ok
\
 
 \ /
  Last update: 2016-01-07 04:21    [W:0.334 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site