linux-kernel - Re: [PATCH v1 0/19] drm/panel: drmP.h removal and DRM_DEV*

Open Source and information security mailing list archives

Message-ID: <87tvhnyf65.fsf@intel.com>
Date:   Fri, 01 Feb 2019 12:30:10 +0200
From:   Jani Nikula <jani.nikula@...ux.intel.com>
To:     Sam Ravnborg <sam@...nborg.org>,
        Thierry Reding <thierry.reding@...il.com>
Cc:     Stefan Mavrodiev <stefan@...mex.com>,
        David Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, Sean Paul <sean@...rly.run>
Subject: Re: [PATCH v1 0/19] drm/panel: drmP.h removal and DRM_DEV*

On Fri, 01 Feb 2019, Sam Ravnborg <sam@...nborg.org> wrote:
> Hi Thierry.
>
>> 
>> 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_*().
>
> For a rookie like me it is much simpler if one can use the same
> logging primitives all over or at least the rules when to use what is simple.
> It is simple to say that everything that exists below drivers/gpu/drm/
> relates to drm.
>
> Suggested set of rules to follow:
> - If in drm core, use DRM_XXX where XXX represent the core functionality
> - If in a driver use DRM_DEV* if a struct device is available
> - If in a driver and no struct device, use plain DRM_ERROR/INFO

Core and drivers are already pretty conflated:

http://patchwork.freedesktop.org/patch/msgid/20181227162310.13023-1-jani.nikula@intel.com

---

Side note, I'd like to switch i915 to dev based debugs, but I absolutely
hate the idea of changing:

	DRM_DEBUG_KMS("...")

to:

	DRM_DEV_DEBUG_KMS(dev_priv->drm.dev, "...")

I think the dev based macros are way too long, and would serve *most*
(though not all) drivers better by having struct drm_device * rather
than struct device * as the first param. In the above, just the
boilerplate consumes half the line.

Basically I'd like to see drm_ prefixed analogues to all the dev_ based
logging functions, e.g. drm_dbg that takes drm_device. But it's so much
churn that I'm contemplating just making i915 specific wrappers
instead. :(

BR,
Jani.

>
> If there is a need to distingush before/after one has a drm_device,
> the best way would be to have a set of logging primitives that
> take a drm_device. So we could extend the rule set:
> - If in a driver use DRM_DRM* if a struct drm_device is available
>   (This rule would take precedence over a struct device)
>
> DRM_DRM*, or DRM_DDEV* or ... But you get the idea.
>
> But this is not where we are today.
>
> Shall I redo the patch-set so we go back to dev_*() in probe() / remove()?
>
> 	Sam
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Graphics Center

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