linux-kernel - Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei auxiliary device

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: <7ed77377-1e6e-4329-1fda-87854f9bb938@linux.intel.com>
Date:   Wed, 16 Feb 2022 12:03:59 +0000
From:   Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To:     "Usyskin, Alexander" <alexander.usyskin@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Winkler, Tomas" <tomas.winkler@...el.com>,
        "Lubart, Vitaly" <vitaly.lubart@...el.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
 auxiliary device



On 15/02/2022 15:22, Usyskin, Alexander wrote:

>>> +{
>>> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
>>> +				      handle_simple_irq, "gsc_irq_handler");
>>> +
>>> +	return irq_set_chip_data(irq, dev_priv);
>>
>> I am not familiar with this interrupt scheme - does dev_priv get used at
>> all by handle_simple_irq, or anyone, after being set here?

What about this? Is dev_priv required or you could pass in NULL just as 
well?

>>
>>> +}
>>> +
>>> +struct intel_gsc_def {
>>> +	const char *name;
>>> +	const unsigned long bar;
>>
>> Unusual, why const out of curiosity? And is it "bar" or "base" would be
>> more accurate?
>>
> Some leftover, thanks for spotting this!
> It is a base of bar. I prefer bar name here. But not really matter.

Is it?

+	adev->bar.start = def->bar + pdev->resource[0].start;

Looks like offset on top of BAR, no?

>>> +{
>>> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>>> +	struct mei_aux_device *adev;
>>> +	struct auxiliary_device *aux_dev;
>>> +	const struct intel_gsc_def *def;
>>> +	int ret;
>>> +
>>> +	intf->irq = -1;
>>> +	intf->id = intf_id;
>>> +
>>> +	if (intf_id == 0 && !HAS_HECI_PXP(dev_priv))
>>> +		return;
>>
>> Isn't inf_id == 0 always a bug with this patch, regardless of
>> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
>> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
>>
> There will be patches for other cards that have pxp as soon as this is reviewed.
> It is better to have infra prepared for two heads.

My point is things are half-prepared since you don't have the id 0 in 
the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but 
if you add a patch which enables it to be true, you have to modify the 
array at the same time or risk a broken patch in the middle.

I don't see the point of the condition making it sound like there are 
two criteria to enter below, while in fact there is only one in current 
code, and that it that it must not be entered because array is incomplete!

>>> +
>>> +	if (!HAS_HECI_GSC(gt->i915))
>>> +		return;
>>
>> Likewise?
>>
>>> +
>>> +	if (gt->gsc.intf[intf_id].irq <= 0) {
>>> +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
>> set");
>>
>> Like this, but use logging functions which say which device please.
>>
> drm_err_ratelimited fits here?

AFAICT it would be a programming bug and not something that can happen 
at runtime hence drm_warn_on_once sounds correct for both.

>>>    }
>>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>    	/* Disable RCS, BCS, VCS and VECS class engines. */
>>>    	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
>> 0);
>>>    	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE,	  0);
>>> +	if (HAS_HECI_GSC(gt->i915))
>>> +		intel_uncore_write(uncore,
>> GEN11_GUNIT_CSME_INTR_ENABLE, 0);
>>>
>>>    	/* Restore masks irqs on RCS, BCS, VCS and VECS engines. */
>>>    	intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK,	~0);
>>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>    	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
>> 	~0);
>>>    	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
>>>    		intel_uncore_write(uncore,
>> GEN12_VECS2_VECS3_INTR_MASK, ~0);
>>> +	if (HAS_HECI_GSC(gt->i915))
>>> +		intel_uncore_write(uncore,
>> GEN11_GUNIT_CSME_INTR_MASK, ~0);
>>>
>>>    	intel_uncore_write(uncore,
>> GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0);
>>>    	intel_uncore_write(uncore,
>> GEN11_GPM_WGBOXPERF_INTR_MASK,  ~0);
>>> @@ -209,6 +216,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
>>>    {
>>>    	struct intel_uncore *uncore = gt->uncore;
>>>    	u32 irqs = GT_RENDER_USER_INTERRUPT;
>>> +	const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1);
>>
>> Why enable the one which is not supported by the patch? No harm doing it?
>>
> No harm and the next patch will be soon, this patch unfortunately is long overdue.

Just feels a bit lazy. You are adding two feature test macros to 
prepare, so why not use them.

Regards,

Tvrtko