linux-kernel - Re: [PATCH v1 0/19] drm/panel: drmP.h removal and 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190131215417.GA13156@mithrandir>
Date:   Thu, 31 Jan 2019 22:54:18 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Sam Ravnborg <sam@...nborg.org>
Cc:     Sean Paul <sean@...rly.run>, dri-devel@...ts.freedesktop.org,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Linus Walleij <linus.walleij@...aro.org>,
        Stefan Mavrodiev <stefan@...mex.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 0/19] drm/panel: drmP.h removal and DRM_DEV*

On Thu, Jan 31, 2019 at 10:03:12PM +0100, Sam Ravnborg wrote:
> Hi Sean.
> 
> > Hey Sam,
> > Thanks for the patchset, this will make dmesg grepping easier! One comment, and
> > you're going to hate me for it: Why use DRM_DEV* instead of DRM_*?
> > 
> > When I introduced DRM_DEV, it was to cover the case where there are multiple
> > instances of the same driver (ie: dual-channel mipi, multiple crtcs, etc). I
> > suppose that _could_ happen in the panel space, but it seems more unlikely than
> > not.
> 
> The rationale for using DRM_DEV* are solely that if a struct device * is avalible,
> then we can use this to provide more information about the origin of the logging.
> 
> I have not testet it - but from browsing the code I could not see that
> DRM_ERROR and friends picked up the module name.
> If DRM_ERROR is the right choice I will redo the patches - no problem.
> 
> But if we loose the module name then the DRM_DEV* variants are preferable IMO.

I personally like the DRM_DEV_* variants better because of the
additional information that they provide. That can be useful when
grepping logs etc.

I'm slightly on the fence about this patch. The unwritten, and
admittedly fuzzy, rules that I've been using so far are that dev_*() are
used or messages that have to do with the panel device itself, whereas
DRM_* variants are used for things that are actually related to DRM. So
typically this would mean that roughly everything in ->probe() or
->remove() would be dev_*(), while the rest would be DRM_DEV_*().

The reason for this is that during most of ->probe() there's not really
any connection to DRM. In many cases the DRM device doesn't even exist
yet. Consider component/master setups where some display component will
wait for the panel to be registered before binding the master. So I find
it confusing to have the DRM style messages when there's actually no DRM
going on yet.

That said, I understand that this might not be an immediately obvious
rule and it might not be followed rigorously, which both quite possibly
contribute to the perception that the messages are all inconsistent. It
always made sense to me, but if everyone else thinks that it's totally
nuts, I'm sure I can find a way to get over it.

Thierry

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