RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

[Hubbe, Allen]

From: Jon Mason <jdmason@xxxxxxxx>:
> On Wed, Dec 23, 2015 at 8:42 AM, Xiangliang Yu <Xiangliang.Yu@xxxxxxx>
> wrote:

> > +#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.

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.

Allen