| Message ID | 20180911230533.31873-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: xilinx_dma: Fix __aligned attribute on zynqmp_dma_desc_ll | expand |
On Tue, Sep 11, 2018 at 4:06 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns: > > drivers/dma/xilinx/zynqmp_dma.c:166:4: warning: attribute 'aligned' is > ignored, place it after "struct" to apply attribute to type declaration > [-Wignored-attributes] > }; __aligned(64) > ^ > ./include/linux/compiler_types.h:200:38: note: expanded from macro > '__aligned' > #define __aligned(x) __attribute__((aligned(x))) > ^ > 1 warning generated. > > Place __aligned before the semicolon. > > Fixes: b0cc417c1637 ("dmaengine: Add Xilinx zynqmp dma engine driver support") > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/dma/xilinx/zynqmp_dma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c > index c74a88b65039..dc19d67cb8c1 100644 > --- a/drivers/dma/xilinx/zynqmp_dma.c > +++ b/drivers/dma/xilinx/zynqmp_dma.c > @@ -163,7 +163,7 @@ struct zynqmp_dma_desc_ll { > u32 ctrl; > u64 nxtdscraddr; > u64 rsvd; > -}; __aligned(64) > +} __aligned(64); Thanks for this patch Nathan. Thinking more about this...the integer passed to __attribute__((aligned(x))) should be in terms of bytes. 64 bytes seems kind of high. Maybe they meant 64 *bits* thus 8 *bytes* which already the default alignment of the struct: https://godbolt.org/z/7vW6E3 In which case, the correct fix is to remove the `__aligned(64);` outright. Since that doesn't change anything (thanks to clang's helpful -Wignored-attributes), such a patch would be "No Functional Change" (does not change the status quo). Still, it might be good for the maintainer to remark if 64 *byte* alignment was intentional (I would think not, but I don't have the datasheet for this piece of hardware in front of me; never say never) before sending such a patch. If the 64 *byte* (512 bit) alignment was intentional (again, which I doubt), then this patch is good to go, but that would then be a functional change and should be tested by someone with hardware. > > /** > * struct zynqmp_dma_desc_sw - Per Transaction structure > -- > 2.18.0 >
On Tue, Sep 11, 2018 at 04:37:48PM -0700, Nick Desaulniers wrote: > On Tue, Sep 11, 2018 at 4:06 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns: > > > > drivers/dma/xilinx/zynqmp_dma.c:166:4: warning: attribute 'aligned' is > > ignored, place it after "struct" to apply attribute to type declaration > > [-Wignored-attributes] > > }; __aligned(64) > > ^ > > ./include/linux/compiler_types.h:200:38: note: expanded from macro > > '__aligned' > > #define __aligned(x) __attribute__((aligned(x))) > > ^ > > 1 warning generated. > > > > Place __aligned before the semicolon. > > > > Fixes: b0cc417c1637 ("dmaengine: Add Xilinx zynqmp dma engine driver support") > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > drivers/dma/xilinx/zynqmp_dma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c > > index c74a88b65039..dc19d67cb8c1 100644 > > --- a/drivers/dma/xilinx/zynqmp_dma.c > > +++ b/drivers/dma/xilinx/zynqmp_dma.c > > @@ -163,7 +163,7 @@ struct zynqmp_dma_desc_ll { > > u32 ctrl; > > u64 nxtdscraddr; > > u64 rsvd; > > -}; __aligned(64) > > +} __aligned(64); > > Thanks for this patch Nathan. Thinking more about this...the integer > passed to __attribute__((aligned(x))) should be in terms of bytes. 64 > bytes seems kind of high. Maybe they meant 64 *bits* thus 8 *bytes* > which already the default alignment of the struct: > https://godbolt.org/z/7vW6E3 > > In which case, the correct fix is to remove the `__aligned(64);` > outright. Since that doesn't change anything (thanks to clang's > helpful -Wignored-attributes), such a patch would be "No Functional > Change" (does not change the status quo). Still, it might be good for > the maintainer to remark if 64 *byte* alignment was intentional (I > would think not, but I don't have the datasheet for this piece of > hardware in front of me; never say never) before sending such a patch. > > If the 64 *byte* (512 bit) alignment was intentional (again, which I > doubt), then this patch is good to go, but that would then be a > functional change and should be tested by someone with hardware. > Thanks for the review Nick. If that is indeed the case, I will spin up a v2. > > > > > /** > > * struct zynqmp_dma_desc_sw - Per Transaction structure > > -- > > 2.18.0 > > > > > -- > Thanks, > ~Nick Desaulniers
diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c index c74a88b65039..dc19d67cb8c1 100644 --- a/drivers/dma/xilinx/zynqmp_dma.c +++ b/drivers/dma/xilinx/zynqmp_dma.c @@ -163,7 +163,7 @@ struct zynqmp_dma_desc_ll { u32 ctrl; u64 nxtdscraddr; u64 rsvd; -}; __aligned(64) +} __aligned(64); /** * struct zynqmp_dma_desc_sw - Per Transaction structure
Clang warns: drivers/dma/xilinx/zynqmp_dma.c:166:4: warning: attribute 'aligned' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes] }; __aligned(64) ^ ./include/linux/compiler_types.h:200:38: note: expanded from macro '__aligned' #define __aligned(x) __attribute__((aligned(x))) ^ 1 warning generated. Place __aligned before the semicolon. Fixes: b0cc417c1637 ("dmaengine: Add Xilinx zynqmp dma engine driver support") Reported-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/dma/xilinx/zynqmp_dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)