| Message ID | 20241001174611.12155-1-quic_pintu@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] dma-buf: replace symbolic permission S_IRUGO with octal 0444 | expand |
Hello Pintu, On Tue, 1 Oct 2024 at 23:16, Pintu Kumar <quic_pintu@quicinc.com> wrote: > > Symbolic permissions are not preferred, instead use the octal. > Also, fix other warnings/errors as well for cleanup. > > WARNING: Block comments use * on subsequent lines > + /* only support discovering the end of the buffer, > + but also allow SEEK_SET to maintain the idiomatic > > WARNING: Block comments use a trailing */ on a separate line > + SEEK_END(0), SEEK_CUR(0) pattern */ > > WARNING: Block comments use a trailing */ on a separate line > + * before passing the sgt back to the exporter. */ > > ERROR: "foo * bar" should be "foo *bar" > +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > + d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, > > total: 1 errors, 4 warnings, 1746 lines checked > > Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com> Thanks for this patch - could you please also mention in the commit log how did you find this? It looks like you ran checkpatch, but it's not clear from the commit log. Since this patch does multiple things related to checkpatch warnings (change S_IRUGO to 0444, comments correction, function declaration correction), can I please ask you to change the commit title to also reflect that? > --- > drivers/dma-buf/dma-buf.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 8892bc701a66..2e63d50e46d3 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -176,8 +176,9 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) > dmabuf = file->private_data; > > /* only support discovering the end of the buffer, > - but also allow SEEK_SET to maintain the idiomatic > - SEEK_END(0), SEEK_CUR(0) pattern */ > + * but also allow SEEK_SET to maintain the idiomatic > + * SEEK_END(0), SEEK_CUR(0) pattern. > + */ > if (whence == SEEK_END) > base = dmabuf->size; > else if (whence == SEEK_SET) > @@ -782,13 +783,14 @@ static void mangle_sg_table(struct sg_table *sg_table) > /* To catch abuse of the underlying struct page by importers mix > * up the bits, but take care to preserve the low SG_ bits to > * not corrupt the sgt. The mixing is undone in __unmap_dma_buf > - * before passing the sgt back to the exporter. */ > + * before passing the sgt back to the exporter. > + */ > for_each_sgtable_sg(sg_table, sg, i) > sg->page_link ^= ~0xffUL; > #endif > > } > -static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, > +static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach, > enum dma_data_direction direction) > { > struct sg_table *sg_table; > @@ -1694,7 +1696,7 @@ static int dma_buf_init_debugfs(void) > > dma_buf_debugfs_dir = d; > > - d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, > + d = debugfs_create_file("bufinfo", 0444, dma_buf_debugfs_dir, > NULL, &dma_buf_debug_fops); > if (IS_ERR(d)) { > pr_debug("dma_buf: debugfs: failed to create node bufinfo\n"); > -- > 2.17.1 > Best, Sumit.
Hi Sumit, On Thu, 3 Oct 2024 at 12:27, Sumit Semwal <sumit.semwal@linaro.org> wrote: > > Hello Pintu, > > On Tue, 1 Oct 2024 at 23:16, Pintu Kumar <quic_pintu@quicinc.com> wrote: > > > > Symbolic permissions are not preferred, instead use the octal. > > Also, fix other warnings/errors as well for cleanup. > > > > WARNING: Block comments use * on subsequent lines > > + /* only support discovering the end of the buffer, > > + but also allow SEEK_SET to maintain the idiomatic > > > > WARNING: Block comments use a trailing */ on a separate line > > + SEEK_END(0), SEEK_CUR(0) pattern */ > > > > WARNING: Block comments use a trailing */ on a separate line > > + * before passing the sgt back to the exporter. */ > > > > ERROR: "foo * bar" should be "foo *bar" > > +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, > > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > > + d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, > > > > total: 1 errors, 4 warnings, 1746 lines checked > > > > Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com> > > Thanks for this patch - could you please also mention in the commit > log how did you find this? It looks like you ran checkpatch, but it's > not clear from the commit log. > Thanks for your review. Sure. Yes, it was found while using the checkpatch. I tried to put "checkpatch fixes" in the commit header but the tool did not allow me. So, I removed it. But I think I can add that in the commit log. > Since this patch does multiple things related to checkpatch warnings > (change S_IRUGO to 0444, comments correction, function declaration > correction), can I please ask you to change the commit title to also > reflect that? > ok sure. Last time I tried to mention "fix checkpatch warnings" in a general way, but the tool itself catches it and throws another warning. So, I chose the major fix as the commit header and combined others, instead of raising 3 different patches. Let me try to put the same as you mentioned above. I will correct these and send v2 in a different mail. Thanks.
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 8892bc701a66..2e63d50e46d3 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -176,8 +176,9 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) dmabuf = file->private_data; /* only support discovering the end of the buffer, - but also allow SEEK_SET to maintain the idiomatic - SEEK_END(0), SEEK_CUR(0) pattern */ + * but also allow SEEK_SET to maintain the idiomatic + * SEEK_END(0), SEEK_CUR(0) pattern. + */ if (whence == SEEK_END) base = dmabuf->size; else if (whence == SEEK_SET) @@ -782,13 +783,14 @@ static void mangle_sg_table(struct sg_table *sg_table) /* To catch abuse of the underlying struct page by importers mix * up the bits, but take care to preserve the low SG_ bits to * not corrupt the sgt. The mixing is undone in __unmap_dma_buf - * before passing the sgt back to the exporter. */ + * before passing the sgt back to the exporter. + */ for_each_sgtable_sg(sg_table, sg, i) sg->page_link ^= ~0xffUL; #endif } -static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, +static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach, enum dma_data_direction direction) { struct sg_table *sg_table; @@ -1694,7 +1696,7 @@ static int dma_buf_init_debugfs(void) dma_buf_debugfs_dir = d; - d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, + d = debugfs_create_file("bufinfo", 0444, dma_buf_debugfs_dir, NULL, &dma_buf_debug_fops); if (IS_ERR(d)) { pr_debug("dma_buf: debugfs: failed to create node bufinfo\n");
Symbolic permissions are not preferred, instead use the octal. Also, fix other warnings/errors as well for cleanup. WARNING: Block comments use * on subsequent lines + /* only support discovering the end of the buffer, + but also allow SEEK_SET to maintain the idiomatic WARNING: Block comments use a trailing */ on a separate line + SEEK_END(0), SEEK_CUR(0) pattern */ WARNING: Block comments use a trailing */ on a separate line + * before passing the sgt back to the exporter. */ ERROR: "foo * bar" should be "foo *bar" +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. + d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir, total: 1 errors, 4 warnings, 1746 lines checked Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com> --- drivers/dma-buf/dma-buf.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)