linux-kernel - Re: [PATCH v3 2/4] drm/vc4: Report underrun errors

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: <87munovd3u.fsf@anholt.net>
Date:   Fri, 25 Jan 2019 09:52:37 -0800
From:   Eric Anholt <eric@...olt.net>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Cc:     David Airlie <airlied@...ux.ie>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Eben Upton <eben@...pberrypi.org>,
        Daniel Vetter <daniel@...ll.ch>,
        Boris Brezillon <boris.brezillon@...tlin.com>
Subject: Re: [PATCH v3 2/4] drm/vc4: Report underrun errors

Paul Kocialkowski <paul.kocialkowski@...tlin.com> writes:

> Hi Eric,
>
> On Wed, 2019-01-23 at 10:47 -0800, Eric Anholt wrote:
>> Paul Kocialkowski <paul.kocialkowski@...tlin.com> writes:
>> > +void vc4_hvs_mask_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
>> > +
>> > +	dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
>> > +		      SCALER_DISPCTRL_DSPEISLUR(1) |
>> > +		      SCALER_DISPCTRL_DSPEISLUR(2));
>> > +
>> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>> > +}
>> > +
>> > +void vc4_hvs_unmask_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
>> > +
>> > +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
>> > +		    SCALER_DISPCTRL_DSPEISLUR(1) |
>> > +		    SCALER_DISPCTRL_DSPEISLUR(2);
>> > +
>> > +	HVS_WRITE(SCALER_DISPSTAT,
>> > +		  SCALER_DISPSTAT_EUFLOW(0) |
>> > +		  SCALER_DISPSTAT_EUFLOW(1) |
>> > +		  SCALER_DISPSTAT_EUFLOW(2));
>> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>> > +}
>> > +
>> > +static void vc4_hvs_report_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +
>> > +	atomic_inc(&vc4->underrun);
>> > +	DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
>> > +}
>> > +
>> > +static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
>> > +{
>> > +	struct drm_device *dev = data;
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 status;
>> > +
>> > +	status = HVS_READ(SCALER_DISPSTAT);
>> > +
>> > +	if (status &
>> > +	    (SCALER_DISPSTAT_EUFLOW(0) | SCALER_DISPSTAT_EUFLOW(1) |
>> > +	     SCALER_DISPSTAT_EUFLOW(2))) {
>> > +		vc4_hvs_mask_underrun(dev);
>> > +		vc4_hvs_report_underrun(dev);
>> > +	}
>> > +
>> > +	HVS_WRITE(SCALER_DISPSTAT, status);
>> > +
>> > +	return status ? IRQ_HANDLED : IRQ_NONE;
>> > +}
>> 
>> So, if UFLOW is set then we incremented the counter and disabled the
>> interrupt, otherwise we acked this specific interrupt and return?  Given
>> that a short-line error (the other potential cause of EISLUR) would be
>> likely to persist, we should probably either vc4_hvs_mask_underrun() in
>> that case too, or only return IRQ_HANDLED for the case we actually
>> handled.
>
> I see, there is definitely an inconsistency here. I don't think we
> should be disabling the interrupt if we get a short line indication,
> just in case the interrupt gets triggered later for a legitimate
> underrun (before the next commit).
>
> So I think we should just totally ignore the short line status bit for
> the IRQ return (although it certainly doesn't hurt to clear it as
> well). What do you think?

You just have to make sure that you return UNHANDLED for short line, so
an IRQ storm doesn't take down the machine.

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