linux-kernel - RE: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode

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] [day] [month] [year] [list]

Date: Fri, 19 Jan 2024 21:54:24 +0000
From: "Klymenko, Anatoliy" <Anatoliy.Klymenko@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	"aarten.lankhorst@...ux.intel.com" <aarten.lankhorst@...ux.intel.com>,
	"mripard@...nel.org" <mripard@...nel.org>, "tzimmermann@...e.de"
	<tzimmermann@...e.de>, "airlied@...il.com" <airlied@...il.com>,
	"daniel@...ll.ch" <daniel@...ll.ch>, "Simek, Michal" <michal.simek@....com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in
 live mode

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Sent: Friday, January 19, 2024 4:30 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@....com>
> Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>;
> aarten.lankhorst@...ux.intel.com; mripard@...nel.org; tzimmermann@...e.de;
> airlied@...il.com; daniel@...ll.ch; Simek, Michal <michal.simek@....com>;
> dri-devel@...ts.freedesktop.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live
> mode
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> Hi Anatoliy,
> 
> On Fri, Jan 19, 2024 at 05:53:30AM +0000, Klymenko, Anatoliy wrote:
> > On Wed, 17 Jan 2024 16:20:10 +0200, Tomi Valkeinen wrote:
> > > On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> > > > Filter out status register against interrupts' mask.
> > > > Some events are being reported via DP status register, even if
> > > > corresponding interrupts have been disabled. Avoid processing of
> > > > such events in interrupt handler context.
> > >
> > > The subject talks about vblank and live mode, the the description
> > > doesn't. Can you elaborate in the desc a bit about when this is an
> > > issue and why it wasn't before?
> >
> > Yes, I should make patch comment more consistent and elaborate. I will fix it in
> the next version. Thank you.
> >
> > >
> > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@....com>
> > > > ---
> > > >   drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +++++++++--
> > > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > > > index d60b7431603f..571c5dbc97e5 100644
> > > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > > > @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int
> irq, void *data)
> > > >       u32 status, mask;
> > > >
> > > >       status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS);
> > > > +     zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
> > > >       mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK);
> > > > -     if (!(status & ~mask))
> > > > +
> > > > +     /*
> > > > +      * Status register may report some events, which corresponding
> interrupts
> > > > +      * have been disabled. Filter out those events against interrupts' mask.
> > > > +      */
> > > > +     status &= ~mask;
> > > > +
> > > > +     if (!status)
> > > >               return IRQ_NONE;
> > > >
> > > >       /* dbg for diagnostic, but not much that the driver can do
> > > > */ @@ -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int
> irq, void *data)
> > > >       if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK)
> > > >               dev_dbg_ratelimited(dp->dev, "overflow
> > > > interrupt\n");
> > > >
> > > > -     zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status);
> > > >
> > > >       if (status & ZYNQMP_DP_INT_VBLANK_START)
> > > >               zynqmp_dpsub_drm_handle_vblank(dp->dpsub);
> > >
> > > Moving the zynqmp_dp_write() is not related to this fix, is it? I
> > > think it should be in a separate patch.
> >
> > The sole purpose of zynqmp_dp_write() is to clear status register. The
> > sooner we do it the better (after reading status in the local variable
> > of course).
> 
> No disagreement about that. Tomi's point is that it's a good change, but it should
> be done in a separate patch, by itself, not bundled with other changes. The usual
> rule in the kernel is "one change, one commit".
> 
Sure, I will move this into a separate commit in the next version. Thank you.

> > We can also reuse status variable for in-place filtering against the
> > interrupt mask, which makes this change dependent on
> > zynqmp_dp_write() reordering. I will add a comment explaining this. Is
> > it ok?
> 
> --
> Regards,
> 
> Laurent Pinchart

Thank you,
Anatoliy