| Message ID | 20170204181727.15426-1-sunny@sunbase.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Oyvind, Thanks for looking at this. At this moment, I don't really see a benefit one way or anther, so I am going to stick with the macro version for now, changing the one remaining module_param_named() you mentioned to using the macros to keep things consistent. If one day we have a paramter with a long ORing of the macros, then we can revisit this decision. Sinclair On Sat, Feb 04, 2017 at 07:17:27PM +0100, Øyvind A. Holm wrote: > This reverts commit 2d8e60e8b0742b7a5cddc806fe38bb81ee876c33. > > The commit belongs to the series of 1285 patches sent to LKML on > 2016-08-02, it changes the representation of file permissions from the > octal value "0600" to "S_IRUSR | S_IWUSR". > > The general consensus was that the changes does not increase > readability, quite the opposite; 0600 is easier to parse mentally than > S_IRUSR | S_IWUSR. > > It also causes argument inconsistency, due to commit 04319d89fbec > ("drm/vmwgfx: Add an option to change assumed FB bpp") that added > another call to module_param_named() where the permissions are written > as 0600. > > Signed-off-by: Øyvind A. Holm <sunny@sunbase.org> > --- > This is a resend of the patch originally sent on 2017-01-16. The only > difference from v1 is an improved commit message with some more details. > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > index 18061a4bc2f2..e8ae3dc476d1 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > @@ -241,15 +241,15 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, > void *ptr); > > MODULE_PARM_DESC(enable_fbdev, "Enable vmwgfx fbdev"); > -module_param_named(enable_fbdev, enable_fbdev, int, S_IRUSR | S_IWUSR); > +module_param_named(enable_fbdev, enable_fbdev, int, 0600); > MODULE_PARM_DESC(force_dma_api, "Force using the DMA API for TTM pages"); > -module_param_named(force_dma_api, vmw_force_iommu, int, S_IRUSR | S_IWUSR); > +module_param_named(force_dma_api, vmw_force_iommu, int, 0600); > MODULE_PARM_DESC(restrict_iommu, "Try to limit IOMMU usage for TTM pages"); > -module_param_named(restrict_iommu, vmw_restrict_iommu, int, S_IRUSR | S_IWUSR); > +module_param_named(restrict_iommu, vmw_restrict_iommu, int, 0600); > MODULE_PARM_DESC(force_coherent, "Force coherent TTM pages"); > -module_param_named(force_coherent, vmw_force_coherent, int, S_IRUSR | S_IWUSR); > +module_param_named(force_coherent, vmw_force_coherent, int, 0600); > MODULE_PARM_DESC(restrict_dma_mask, "Restrict DMA mask to 44 bits with IOMMU"); > -module_param_named(restrict_dma_mask, vmw_restrict_dma_mask, int, S_IRUSR | S_IWUSR); > +module_param_named(restrict_dma_mask, vmw_restrict_dma_mask, int, 0600); > MODULE_PARM_DESC(assume_16bpp, "Assume 16-bpp when filtering modes"); > module_param_named(assume_16bpp, vmw_assume_16bpp, int, 0600); > > -- > 2.12.0.rc0 >
On Fri, Feb 24, 2017 at 11:06:03AM -0800, Sinclair Yeh wrote: > Hi Oyvind, > > Thanks for looking at this. > > At this moment, I don't really see a benefit one way or anther, so > I am going to stick with the macro version for now, changing the one > remaining module_param_named() you mentioned to using the macros to > keep things consistent. > > If one day we have a paramter with a long ORing of the macros, then > we can revisit this decision. Actually, now that I see checkpatch.pl has been modified to prefer octal permissions, I think your suggestion is better. So I'll just take your patch instead. thanks! Sinclair > > Sinclair > > On Sat, Feb 04, 2017 at 07:17:27PM +0100, Øyvind A. Holm wrote: > > This reverts commit 2d8e60e8b0742b7a5cddc806fe38bb81ee876c33. > > > > The commit belongs to the series of 1285 patches sent to LKML on > > 2016-08-02, it changes the representation of file permissions from the > > octal value "0600" to "S_IRUSR | S_IWUSR". > > > > The general consensus was that the changes does not increase > > readability, quite the opposite; 0600 is easier to parse mentally than > > S_IRUSR | S_IWUSR. > > > > It also causes argument inconsistency, due to commit 04319d89fbec > > ("drm/vmwgfx: Add an option to change assumed FB bpp") that added > > another call to module_param_named() where the permissions are written > > as 0600. > > > > Signed-off-by: Øyvind A. Holm <sunny@sunbase.org> > > --- > > This is a resend of the patch originally sent on 2017-01-16. The only > > difference from v1 is an improved commit message with some more details. > > > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > > index 18061a4bc2f2..e8ae3dc476d1 100644 > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > > @@ -241,15 +241,15 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, > > void *ptr); > > > > MODULE_PARM_DESC(enable_fbdev, "Enable vmwgfx fbdev"); > > -module_param_named(enable_fbdev, enable_fbdev, int, S_IRUSR | S_IWUSR); > > +module_param_named(enable_fbdev, enable_fbdev, int, 0600); > > MODULE_PARM_DESC(force_dma_api, "Force using the DMA API for TTM pages"); > > -module_param_named(force_dma_api, vmw_force_iommu, int, S_IRUSR | S_IWUSR); > > +module_param_named(force_dma_api, vmw_force_iommu, int, 0600); > > MODULE_PARM_DESC(restrict_iommu, "Try to limit IOMMU usage for TTM pages"); > > -module_param_named(restrict_iommu, vmw_restrict_iommu, int, S_IRUSR | S_IWUSR); > > +module_param_named(restrict_iommu, vmw_restrict_iommu, int, 0600); > > MODULE_PARM_DESC(force_coherent, "Force coherent TTM pages"); > > -module_param_named(force_coherent, vmw_force_coherent, int, S_IRUSR | S_IWUSR); > > +module_param_named(force_coherent, vmw_force_coherent, int, 0600); > > MODULE_PARM_DESC(restrict_dma_mask, "Restrict DMA mask to 44 bits with IOMMU"); > > -module_param_named(restrict_dma_mask, vmw_restrict_dma_mask, int, S_IRUSR | S_IWUSR); > > +module_param_named(restrict_dma_mask, vmw_restrict_dma_mask, int, 0600); > > MODULE_PARM_DESC(assume_16bpp, "Assume 16-bpp when filtering modes"); > > module_param_named(assume_16bpp, vmw_assume_16bpp, int, 0600); > > > > -- > > 2.12.0.rc0 > >
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 18061a4bc2f2..e8ae3dc476d1 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -241,15 +241,15 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, void *ptr); MODULE_PARM_DESC(enable_fbdev, "Enable vmwgfx fbdev"); -module_param_named(enable_fbdev, enable_fbdev, int, S_IRUSR | S_IWUSR); +module_param_named(enable_fbdev, enable_fbdev, int, 0600); MODULE_PARM_DESC(force_dma_api, "Force using the DMA API for TTM pages"); -module_param_named(force_dma_api, vmw_force_iommu, int, S_IRUSR | S_IWUSR); +module_param_named(force_dma_api, vmw_force_iommu, int, 0600); MODULE_PARM_DESC(restrict_iommu, "Try to limit IOMMU usage for TTM pages"); -module_param_named(restrict_iommu, vmw_restrict_iommu, int, S_IRUSR | S_IWUSR); +module_param_named(restrict_iommu, vmw_restrict_iommu, int, 0600); MODULE_PARM_DESC(force_coherent, "Force coherent TTM pages"); -module_param_named(force_coherent, vmw_force_coherent, int, S_IRUSR | S_IWUSR); +module_param_named(force_coherent, vmw_force_coherent, int, 0600); MODULE_PARM_DESC(restrict_dma_mask, "Restrict DMA mask to 44 bits with IOMMU"); -module_param_named(restrict_dma_mask, vmw_restrict_dma_mask, int, S_IRUSR | S_IWUSR); +module_param_named(restrict_dma_mask, vmw_restrict_dma_mask, int, 0600); MODULE_PARM_DESC(assume_16bpp, "Assume 16-bpp when filtering modes"); module_param_named(assume_16bpp, vmw_assume_16bpp, int, 0600);
This reverts commit 2d8e60e8b0742b7a5cddc806fe38bb81ee876c33. The commit belongs to the series of 1285 patches sent to LKML on 2016-08-02, it changes the representation of file permissions from the octal value "0600" to "S_IRUSR | S_IWUSR". The general consensus was that the changes does not increase readability, quite the opposite; 0600 is easier to parse mentally than S_IRUSR | S_IWUSR. It also causes argument inconsistency, due to commit 04319d89fbec ("drm/vmwgfx: Add an option to change assumed FB bpp") that added another call to module_param_named() where the permissions are written as 0600. Signed-off-by: Øyvind A. Holm <sunny@sunbase.org> --- This is a resend of the patch originally sent on 2017-01-16. The only difference from v1 is an improved commit message with some more details. drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)