linux-kernel - Re: [PATCH 0/4] drm: core: Convert logging to drm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200713135808.GA6783@blackclown>
Date:   Mon, 13 Jul 2020 19:28:08 +0530
From:   Suraj Upadhyay <usuraj35@...il.com>
To:     Joe Perches <joe@...ches.com>, maarten.lankhorst@...ux.intel.com,
        mripard@...nel.org, tzimmermann@...e.de, airlied@...ux.ie,
        daniel@...ll.ch
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 0/4] drm: core: Convert logging to drm_* functions.

On Sun, Jul 12, 2020 at 12:07:45PM -0700, Joe Perches wrote:
> On Mon, 2020-07-13 at 00:24 +0530, Suraj Upadhyay wrote:
> > On Sat, Jul 11, 2020 at 11:16:33AM -0700, Joe Perches wrote:
> []
> > > Perhaps change the __drm_printk macro to not
> > > dereference the drm argument when NULL.
> > > 
> > > A trivial but perhaps inefficient way might be
> > > used like:
> > > 
> > > 	drm_<level>(NULL, fmt, ...)
> []
> > > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> []
> > > @@ -395,8 +395,8 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  
> > >  /* Helper for struct drm_device based logging. */
> > >  #define __drm_printk(drm, level, type, fmt, ...)			\
> > > -	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
> > > -
> > > +	dev_##level##type((drm) ? (drm)->dev : NULL, "[drm] " fmt,	\
> > > +			  ##__VA_ARGS__)
> > >  
> > >  #define drm_info(drm, fmt, ...)					\
> > >  	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)
> > > 
> > 
> > Hi Joe,
> > 	Thanks for your input.
> > But I don't think that this change would be a good idea as we are
> > supposed to find or make a substitute of WARN_* macros which
> > take a `condition` as an argument and check for its truth.
> > And I guess passing a NULL to dev_<level> would cause a format warning.
> > 
> > Also, the WARN_* macros are doing their job fine, and passing a NULL
> > value everytime you want to warn about a certain condition at a
> > particular line, doesn't seem good to me.
> > 
> > Thus, I think that WARN_* macros should be untouched.
> 
> So do I but the suggestion was not about WARN macros
> only about drm_<level> macros and possibly unnecessary
> conversions to dev_<level> when a drm_device context
> is unavailable.
> 
> Also, you don't have to guess, the code is there for
> you to inspect.
> 
> dev_<level> when a NULL is used as the first argument
> emits "(NULL device *)" instead of dev_driver_string(dev)
> and dev_name(dev).
> 
> See: drivers/base/core.c::__dev_printk()

Yes, Thanks my bad.
But the dev_<level> usages in drm/* always have a context and doesn't need
NULL to be passed, i.e. some of them have only a `struct device` context which
cannot be simply converted into drm_<level> since they take a struct
pointer with a `dev` member and not a `dev` itself.
What we can convert is calls to dev_<level> with a drm_device context or
with a struct pointer which has a dev member.

And, I really want the MAINTAINERS to look into it.

Thanks and Cheers,

Suraj Upadhyay.


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)