| Message ID | 20190605170639.8368-2-dingchen.zhang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: not to read outside the boundary for CRC source name. | expand |
Hi Dingchen I do not know this code, so please await feedback from others on the patch itself. On Wed, Jun 05, 2019 at 01:06:39PM -0400, Dingchen Zhang wrote: > to terminate the while-loop in drm_dp_aux_crc_work when drm_dp_start/stop_crc > are called in the hook to set crc source. > > Cc:Leo Li <sunpeng.li@amd.com>, Harry <Harry.Wentland@amd.com>, Nick <Nicholas.Kazlauskas@amd.com> Only one pair of name/email per line. And try to use the full name as reported by scripts/get_maintainer.pl Sam
On 2019-06-05 1:06 p.m., Dingchen Zhang wrote: > to terminate the while-loop in drm_dp_aux_crc_work when drm_dp_start/stop_crc > are called in the hook to set crc source. > > Cc:Leo Li <sunpeng.li@amd.com>, Harry <Harry.Wentland@amd.com>, Nick <Nicholas.Kazlauskas@amd.com> > Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com> I wonder how this isn't creating problems for Rockchip with the Analogix bridge. Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > drivers/gpu/drm/drm_debugfs_crc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c > index e20adef9d623..0e8bcc130383 100644 > --- a/drivers/gpu/drm/drm_debugfs_crc.c > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > @@ -249,6 +249,13 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) > struct drm_crtc *crtc = filep->f_inode->i_private; > struct drm_crtc_crc *crc = &crtc->crc; > > + /* terminate the infinite while loop if 'drm_dp_aux_crc_work' running */ > + if (crc->opened) { > + spin_lock_irq(&crc->lock); > + crc->opened = false; > + spin_unlock_irq(&crc->lock); > + } > + > crtc->funcs->set_crc_source(crtc, NULL); > > spin_lock_irq(&crc->lock); >
On Fri, Jun 21, 2019 at 02:21:43PM +0000, Harry Wentland wrote: > On 2019-06-05 1:06 p.m., Dingchen Zhang wrote: > > to terminate the while-loop in drm_dp_aux_crc_work when drm_dp_start/stop_crc > > are called in the hook to set crc source. > > > > Cc:Leo Li <sunpeng.li@amd.com>, Harry <Harry.Wentland@amd.com>, Nick <Nicholas.Kazlauskas@amd.com> > > Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com> > > I wonder how this isn't creating problems for Rockchip with the Analogix > bridge. > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > Harry > > > --- > > drivers/gpu/drm/drm_debugfs_crc.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c > > index e20adef9d623..0e8bcc130383 100644 > > --- a/drivers/gpu/drm/drm_debugfs_crc.c > > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > > @@ -249,6 +249,13 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) > > struct drm_crtc *crtc = filep->f_inode->i_private; > > struct drm_crtc_crc *crc = &crtc->crc; > > > > + /* terminate the infinite while loop if 'drm_dp_aux_crc_work' running */ > > + if (crc->opened) { > > + spin_lock_irq(&crc->lock); > > + crc->opened = false; > > + spin_unlock_irq(&crc->lock); > > + } Either you don't need a lock to look at ->opened, or you need it. Not both. Too lazy check which way this is, it's practically w/e here :-) -Daniel > > + > > crtc->funcs->set_crc_source(crtc, NULL); > > > > spin_lock_irq(&crc->lock); > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 6/21/19 11:46 AM, Daniel Vetter wrote: > On Fri, Jun 21, 2019 at 02:21:43PM +0000, Harry Wentland wrote: >> On 2019-06-05 1:06 p.m., Dingchen Zhang wrote: >>> to terminate the while-loop in drm_dp_aux_crc_work when drm_dp_start/stop_crc >>> are called in the hook to set crc source. >>> >>> Cc:Leo Li <sunpeng.li@amd.com>, Harry <Harry.Wentland@amd.com>, Nick <Nicholas.Kazlauskas@amd.com> The Cc: need to be cleaned up before merge like you did with your other patch. >>> Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com> >> >> I wonder how this isn't creating problems for Rockchip with the Analogix >> bridge. >> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> >> >> Harry >> >>> --- >>> drivers/gpu/drm/drm_debugfs_crc.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c >>> index e20adef9d623..0e8bcc130383 100644 >>> --- a/drivers/gpu/drm/drm_debugfs_crc.c >>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c >>> @@ -249,6 +249,13 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) >>> struct drm_crtc *crtc = filep->f_inode->i_private; >>> struct drm_crtc_crc *crc = &crtc->crc; >>> >>> + /* terminate the infinite while loop if 'drm_dp_aux_crc_work' running */ >>> + if (crc->opened) { >>> + spin_lock_irq(&crc->lock); >>> + crc->opened = false; >>> + spin_unlock_irq(&crc->lock); >>> + } > > Either you don't need a lock to look at ->opened, or you need it. Not > both. Too lazy check which way this is, it's practically w/e here :-) > -Daniel The check on crc->opened should probably just be dropped since it's not actually doing anything here. With those fixed, this patch is: Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> Nicholas Kazlauskas > >>> + >>> crtc->funcs->set_crc_source(crtc, NULL); >>> >>> spin_lock_irq(&crc->lock); >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 2019-06-21 11:46 a.m., Daniel Vetter wrote: > On Fri, Jun 21, 2019 at 02:21:43PM +0000, Harry Wentland wrote: >> On 2019-06-05 1:06 p.m., Dingchen Zhang wrote: >>> to terminate the while-loop in drm_dp_aux_crc_work when drm_dp_start/stop_crc >>> are called in the hook to set crc source. >>> >>> Cc:Leo Li <sunpeng.li@amd.com>, Harry <Harry.Wentland@amd.com>, Nick <Nicholas.Kazlauskas@amd.com> >>> Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com> >> >> I wonder how this isn't creating problems for Rockchip with the Analogix >> bridge. >> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> >> >> Harry >> >>> --- >>> drivers/gpu/drm/drm_debugfs_crc.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c >>> index e20adef9d623..0e8bcc130383 100644 >>> --- a/drivers/gpu/drm/drm_debugfs_crc.c >>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c >>> @@ -249,6 +249,13 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) >>> struct drm_crtc *crtc = filep->f_inode->i_private; >>> struct drm_crtc_crc *crc = &crtc->crc; >>> >>> + /* terminate the infinite while loop if 'drm_dp_aux_crc_work' running */ >>> + if (crc->opened) { >>> + spin_lock_irq(&crc->lock); >>> + crc->opened = false; >>> + spin_unlock_irq(&crc->lock); >>> + } > > Either you don't need a lock to look at ->opened, or you need it. Not > both. Too lazy check which way this is, it's practically w/e here :-) > -Daniel > Hi Daniel, Thanks for pointing out and sorry for late reply. Will create the new patch for that. -Dingchen >>> + >>> crtc->funcs->set_crc_source(crtc, NULL); >>> >>> spin_lock_irq(&crc->lock); >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index e20adef9d623..0e8bcc130383 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -249,6 +249,13 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) struct drm_crtc *crtc = filep->f_inode->i_private; struct drm_crtc_crc *crc = &crtc->crc; + /* terminate the infinite while loop if 'drm_dp_aux_crc_work' running */ + if (crc->opened) { + spin_lock_irq(&crc->lock); + crc->opened = false; + spin_unlock_irq(&crc->lock); + } + crtc->funcs->set_crc_source(crtc, NULL); spin_lock_irq(&crc->lock);
to terminate the while-loop in drm_dp_aux_crc_work when drm_dp_start/stop_crc are called in the hook to set crc source. Cc:Leo Li <sunpeng.li@amd.com>, Harry <Harry.Wentland@amd.com>, Nick <Nicholas.Kazlauskas@amd.com> Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com> --- drivers/gpu/drm/drm_debugfs_crc.c | 7 +++++++ 1 file changed, 7 insertions(+)