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

[Jon Mason]

On Wed, Jan 6, 2016 at 11:52 AM, Hubbe, Allen <Allen.Hubbe@xxxxxxx> wrote:
> 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.

My opinion wasn't super strong on these.  If Allen is fine with them,
then good enough for me :)


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

Thanks,
Jon


> Allen
>
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@xxxxxxxxxxxxxxxxx
> To post to this group, send email to linux-ntb@xxxxxxxxxxxxxxxxx
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/40F65EF2B5E2254199711F58E3ACB84D99EF72E1%40MX104CL02.corp.emc.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/