| Message ID | 3187035019207d96dd8b290bbcce203a38e7d1d3.1474856262.git.joe@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: > Use a bit more consistent style with kernel loglevels I'm not convinced this is worth doing if we're going to keep the WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN to DRM_WARNING since it's so widely used. Sean > without > using macro argument concatenation. > > Miscellanea: > > o Single statement macros don't need do {} while (0) > > Signed-off-by: Joe Perches <joe@perches.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++---------- > include/drm/drmP.h | 26 +++++++++++++------------- > 2 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 6fd39efb7894..bc4f9895f356 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -566,7 +566,7 @@ int intel_guc_setup(struct drm_device *dev) > else if (err == 0) > DRM_INFO("GuC firmware load skipped\n"); > else if (ret != -EIO) > - DRM_NOTE("GuC firmware load failed: %d\n", err); > + DRM_NOTICE("GuC firmware load failed: %d\n", err); > else > DRM_WARN("GuC firmware load failed: %d\n", err); > > @@ -574,7 +574,7 @@ int intel_guc_setup(struct drm_device *dev) > if (fw_path == NULL) > DRM_INFO("GuC submission without firmware not supported\n"); > if (ret == 0) > - DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > + DRM_NOTICE("Falling back from GuC submission to execlist mode\n"); > else > DRM_ERROR("GuC init failed: %d\n", ret); > } > @@ -606,7 +606,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > /* Check the size of the blob before examining buffer contents */ > if (fw->size < sizeof(struct guc_css_header)) { > - DRM_NOTE("Firmware header is missing\n"); > + DRM_NOTICE("Firmware header is missing\n"); > goto fail; > } > > @@ -618,7 +618,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > css->key_size_dw - css->exponent_size_dw) * sizeof(u32); > > if (guc_fw->header_size != sizeof(struct guc_css_header)) { > - DRM_NOTE("CSS header definition mismatch\n"); > + DRM_NOTICE("CSS header definition mismatch\n"); > goto fail; > } > > @@ -628,7 +628,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > /* now RSA */ > if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) { > - DRM_NOTE("RSA key size is bad\n"); > + DRM_NOTICE("RSA key size is bad\n"); > goto fail; > } > guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size; > @@ -637,14 +637,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > /* At least, it should have header, uCode and RSA. Size of all three. */ > size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size; > if (fw->size < size) { > - DRM_NOTE("Missing firmware components\n"); > + DRM_NOTICE("Missing firmware components\n"); > goto fail; > } > > /* Header and uCode will be loaded to WOPCM. Size of the two. */ > size = guc_fw->header_size + guc_fw->ucode_size; > if (size > guc_wopcm_size(to_i915(dev))) { > - DRM_NOTE("Firmware is too large to fit in WOPCM\n"); > + DRM_NOTICE("Firmware is too large to fit in WOPCM\n"); > goto fail; > } > > @@ -659,9 +659,11 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { > - DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n", > - guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found, > - guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted); > + DRM_NOTICE("GuC firmware version %d.%d, required %d.%d\n", > + guc_fw->guc_fw_major_found, > + guc_fw->guc_fw_minor_found, > + guc_fw->guc_fw_major_wanted, > + guc_fw->guc_fw_minor_wanted); > err = -ENOEXEC; > goto fail; > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index c53dc90942e0..95cd04aa9bf7 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -168,25 +168,25 @@ void drm_printk(const char *level, unsigned int category, > /** \name Macros to make printk easier */ > /*@{*/ > > -#define _DRM_PRINTK(once, level, fmt, ...) \ > - do { \ > - printk##once(KERN_##level "[" DRM_NAME "] " fmt, \ > - ##__VA_ARGS__); \ > - } while (0) > +#define _drm_printk(level, fmt, ...) \ > + printk(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__) > > #define DRM_INFO(fmt, ...) \ > - _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__) > -#define DRM_NOTE(fmt, ...) \ > - _DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__) > + _drm_printk(KERN_INFO, fmt, ##__VA_ARGS__) > +#define DRM_NOTICE(fmt, ...) \ > + _drm_printk(KERN_NOTICE, fmt, ##__VA_ARGS__) > #define DRM_WARN(fmt, ...) \ > - _DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__) > + _drm_printk(KERN_WARNING, fmt, ##__VA_ARGS__) > + > +#define _drm_printk_once(level, fmt, ...) \ > + printk_once(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__) > > #define DRM_INFO_ONCE(fmt, ...) \ > - _DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__) > -#define DRM_NOTE_ONCE(fmt, ...) \ > - _DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__) > + _drm_printk_once(KERN_INFO, fmt, ##__VA_ARGS__) > +#define DRM_NOTICE_ONCE(fmt, ...) \ > + _drm_printk_once(KERN_NOTICE, fmt, ##__VA_ARGS__) > #define DRM_WARN_ONCE(fmt, ...) \ > - _DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__) > + _drm_printk_once(KERN_WARNING, fmt, ##__VA_ARGS__) > > /** > * Error output. > -- > 2.10.0.rc2.1.g053435c > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote: > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: > > Use a bit more consistent style with kernel loglevels > > I'm not convinced this is worth doing if we're going to keep the > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN > to DRM_WARNING since it's so widely used. There is no DRM_WARN inconsistency. What is used is pr_warn and dev_warn, not pr_warning and dev_warning Well, there are still a few pr_warning uses, but those will eventually be removed/converted.
On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote: > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote: >> > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: >> > Use a bit more consistent style with kernel loglevels >> > I'm not convinced this is worth doing if we're going to keep the >> WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN >> to DRM_WARNING since it's so widely used. > > There is no DRM_WARN inconsistency. > DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and DRM_NOTE is to DRM_NOTICE... is what I'm thinking and seemingly so does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the patch, so it might be worth keeping separate ? -Emil
On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote: > On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote: > > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote: > > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: > > > > Use a bit more consistent style with kernel loglevels > > > I'm not convinced this is worth doing if we're going to keep the > > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN > > > to DRM_WARNING since it's so widely used. > > There is no DRM_WARN inconsistency. > DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and > DRM_NOTE is to DRM_NOTICE... DRM_INFORMATION doesn't exist in the kernel tree. > is what I'm thinking and seemingly so > does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the > patch, so it might be worth keeping separate ? To me, simplifying the macro means using the common kernel macro forms.
On 27 September 2016 at 17:43, Joe Perches <joe@perches.com> wrote: > On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote: >> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote: >> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote: >> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: >> > > > Use a bit more consistent style with kernel loglevels >> > > I'm not convinced this is worth doing if we're going to keep the >> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN >> > > to DRM_WARNING since it's so widely used. >> > There is no DRM_WARN inconsistency. >> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and >> DRM_NOTE is to DRM_NOTICE... > > DRM_INFORMATION doesn't exist in the kernel tree. > >> is what I'm thinking and seemingly so >> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the >> patch, so it might be worth keeping separate ? > > To me, simplifying the macro means using the common kernel > macro forms. > "unify" might be better, but I agree. Either way there's no point in elaborating on the point me(Sean?) meant since it's just going to get shoot down like a dog ;-) Regards, Emil
On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote: > On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote: > > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote: > > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: > > > > Use a bit more consistent style with kernel loglevels > > > I'm not convinced this is worth doing if we're going to keep the > > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN > > > to DRM_WARNING since it's so widely used. > > There is no DRM_WARN inconsistency. > DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and > DRM_NOTE is to DRM_NOTICE... DRM_INFORMATION doesn't exist in the kernel tree. > is what I'm thinking and seemingly so > does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the > patch, so it might be worth keeping separate ? To me, simplifying the macro means using the common kernel macro forms.
On Tue, Sep 27, 2016 at 12:54 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 27 September 2016 at 17:43, Joe Perches <joe@perches.com> wrote: >> On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote: >>> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote: >>> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote: >>> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote: >>> > > > Use a bit more consistent style with kernel loglevels >>> > > I'm not convinced this is worth doing if we're going to keep the >>> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN >>> > > to DRM_WARNING since it's so widely used. >>> > There is no DRM_WARN inconsistency. >>> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and >>> DRM_NOTE is to DRM_NOTICE... >> >> DRM_INFORMATION doesn't exist in the kernel tree. >> >>> is what I'm thinking and seemingly so >>> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the >>> patch, so it might be worth keeping separate ? >> >> To me, simplifying the macro means using the common kernel >> macro forms. >> > "unify" might be better, but I agree. > > Either way there's no point in elaborating on the point me(Sean?) > meant since it's just going to get shoot down like a dog ;-) Yeah, I can see both sides, and I suppose I don't really care either way. Given that DRM_NOTE/NOTICE is only used 7 places (in one file), I doubt there are going to be any strong feelings. Sean > > Regards, > Emil
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 6fd39efb7894..bc4f9895f356 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -566,7 +566,7 @@ int intel_guc_setup(struct drm_device *dev) else if (err == 0) DRM_INFO("GuC firmware load skipped\n"); else if (ret != -EIO) - DRM_NOTE("GuC firmware load failed: %d\n", err); + DRM_NOTICE("GuC firmware load failed: %d\n", err); else DRM_WARN("GuC firmware load failed: %d\n", err); @@ -574,7 +574,7 @@ int intel_guc_setup(struct drm_device *dev) if (fw_path == NULL) DRM_INFO("GuC submission without firmware not supported\n"); if (ret == 0) - DRM_NOTE("Falling back from GuC submission to execlist mode\n"); + DRM_NOTICE("Falling back from GuC submission to execlist mode\n"); else DRM_ERROR("GuC init failed: %d\n", ret); } @@ -606,7 +606,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) /* Check the size of the blob before examining buffer contents */ if (fw->size < sizeof(struct guc_css_header)) { - DRM_NOTE("Firmware header is missing\n"); + DRM_NOTICE("Firmware header is missing\n"); goto fail; } @@ -618,7 +618,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) css->key_size_dw - css->exponent_size_dw) * sizeof(u32); if (guc_fw->header_size != sizeof(struct guc_css_header)) { - DRM_NOTE("CSS header definition mismatch\n"); + DRM_NOTICE("CSS header definition mismatch\n"); goto fail; } @@ -628,7 +628,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) /* now RSA */ if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) { - DRM_NOTE("RSA key size is bad\n"); + DRM_NOTICE("RSA key size is bad\n"); goto fail; } guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size; @@ -637,14 +637,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) /* At least, it should have header, uCode and RSA. Size of all three. */ size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size; if (fw->size < size) { - DRM_NOTE("Missing firmware components\n"); + DRM_NOTICE("Missing firmware components\n"); goto fail; } /* Header and uCode will be loaded to WOPCM. Size of the two. */ size = guc_fw->header_size + guc_fw->ucode_size; if (size > guc_wopcm_size(to_i915(dev))) { - DRM_NOTE("Firmware is too large to fit in WOPCM\n"); + DRM_NOTICE("Firmware is too large to fit in WOPCM\n"); goto fail; } @@ -659,9 +659,11 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted || guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) { - DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n", - guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found, - guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted); + DRM_NOTICE("GuC firmware version %d.%d, required %d.%d\n", + guc_fw->guc_fw_major_found, + guc_fw->guc_fw_minor_found, + guc_fw->guc_fw_major_wanted, + guc_fw->guc_fw_minor_wanted); err = -ENOEXEC; goto fail; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c53dc90942e0..95cd04aa9bf7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -168,25 +168,25 @@ void drm_printk(const char *level, unsigned int category, /** \name Macros to make printk easier */ /*@{*/ -#define _DRM_PRINTK(once, level, fmt, ...) \ - do { \ - printk##once(KERN_##level "[" DRM_NAME "] " fmt, \ - ##__VA_ARGS__); \ - } while (0) +#define _drm_printk(level, fmt, ...) \ + printk(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__) #define DRM_INFO(fmt, ...) \ - _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__) -#define DRM_NOTE(fmt, ...) \ - _DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__) + _drm_printk(KERN_INFO, fmt, ##__VA_ARGS__) +#define DRM_NOTICE(fmt, ...) \ + _drm_printk(KERN_NOTICE, fmt, ##__VA_ARGS__) #define DRM_WARN(fmt, ...) \ - _DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__) + _drm_printk(KERN_WARNING, fmt, ##__VA_ARGS__) + +#define _drm_printk_once(level, fmt, ...) \ + printk_once(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__) #define DRM_INFO_ONCE(fmt, ...) \ - _DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__) -#define DRM_NOTE_ONCE(fmt, ...) \ - _DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__) + _drm_printk_once(KERN_INFO, fmt, ##__VA_ARGS__) +#define DRM_NOTICE_ONCE(fmt, ...) \ + _drm_printk_once(KERN_NOTICE, fmt, ##__VA_ARGS__) #define DRM_WARN_ONCE(fmt, ...) \ - _DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__) + _drm_printk_once(KERN_WARNING, fmt, ##__VA_ARGS__) /** * Error output.
Use a bit more consistent style with kernel loglevels without using macro argument concatenation. Miscellanea: o Single statement macros don't need do {} while (0) Signed-off-by: Joe Perches <joe@perches.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++---------- include/drm/drmP.h | 26 +++++++++++++------------- 2 files changed, 25 insertions(+), 23 deletions(-)