| Message ID | 20180702110729.8171-2-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mahesh, Thank you for the patch. On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote: > This patch adds a new callback function "verify_crc_source" which will > be used during setting the crc source in control node and while opening > data node for crc reading. This will help in avoiding setting of wrong > string for source. Why do you need to verify the source in the open() operation ? Isn't it enough to verify it in the write() handler ? The answer to this question might lie in patch 08/10, in which case I'd add the .verify_crc_source() call in open() in that patch, not here. > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: dri-devel@lists.freedesktop.org > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++ > include/drm/drm_crtc.h | 15 +++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c > b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644 > --- a/drivers/gpu/drm/drm_debugfs_crc.c > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const > char __user *ubuf, struct drm_crtc *crtc = m->private; > struct drm_crtc_crc *crc = &crtc->crc; > char *source; > + size_t values_cnt; > + int ret = 0; > > if (len == 0) > return 0; > @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, > const char __user *ubuf, if (source[len] == '\n') > source[len] = '\0'; > > + if (crtc->funcs->verify_crc_source) { > + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); > + if (ret) > + return ret; > + } > + > spin_lock_irq(&crc->lock); > > if (crc->opened) { > @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct > file *filep) return ret; > } > > + if (crtc->funcs->verify_crc_source) { > + ret = crtc->funcs->verify_crc_source(crtc, crc->source, > + &values_cnt); > + if (ret) > + return ret; > + } > + > spin_lock_irq(&crc->lock); > if (!crc->opened) > crc->opened = true; > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 23eddbccab10..1a6dcbf91744 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -661,6 +661,21 @@ struct drm_crtc_funcs { > */ > int (*set_crc_source)(struct drm_crtc *crtc, const char *source, > size_t *values_cnt); > + /** > + * @verify_crc_source: > + * > + * verifies the source of CRC checksums of frames before setting the > + * source for CRC and during crc open. > + * > + * This callback is optional if the driver does not support any CRC > + * generation functionality. > + * > + * RETURNS: > + * > + * 0 on success or a negative error code on failure. > + */ > + int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, > + size_t *values_cnt); > > /** > * @atomic_print_state:
Hi, Thanks for the review, On 7/10/2018 4:56 PM, Laurent Pinchart wrote: > Hi Mahesh, > > Thank you for the patch. > > On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote: >> This patch adds a new callback function "verify_crc_source" which will >> be used during setting the crc source in control node and while opening >> data node for crc reading. This will help in avoiding setting of wrong >> string for source. > Why do you need to verify the source in the open() operation ? Isn't it enough > to verify it in the write() handler ? The answer to this question might lie in > patch 08/10, in which case I'd add the .verify_crc_source() call in open() in > that patch, not here. Yes, final goal is achieved by patch 08/10. But if crc_read is called before setting the source, it may have wrong source set in that case I wanted to return early at least for the drivers implemented verify_crc_source. If you still think otherwise I will modify accordingly in next series. -Mahesh > >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> Cc: dri-devel@lists.freedesktop.org >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++ >> include/drm/drm_crtc.h | 15 +++++++++++++++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c >> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644 >> --- a/drivers/gpu/drm/drm_debugfs_crc.c >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c >> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const >> char __user *ubuf, struct drm_crtc *crtc = m->private; >> struct drm_crtc_crc *crc = &crtc->crc; >> char *source; >> + size_t values_cnt; >> + int ret = 0; >> >> if (len == 0) >> return 0; >> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, >> const char __user *ubuf, if (source[len] == '\n') >> source[len] = '\0'; >> >> + if (crtc->funcs->verify_crc_source) { >> + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); >> + if (ret) >> + return ret; >> + } >> + >> spin_lock_irq(&crc->lock); >> >> if (crc->opened) { >> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct >> file *filep) return ret; >> } >> >> + if (crtc->funcs->verify_crc_source) { >> + ret = crtc->funcs->verify_crc_source(crtc, crc->source, >> + &values_cnt); >> + if (ret) >> + return ret; >> + } >> + >> spin_lock_irq(&crc->lock); >> if (!crc->opened) >> crc->opened = true; >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 23eddbccab10..1a6dcbf91744 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -661,6 +661,21 @@ struct drm_crtc_funcs { >> */ >> int (*set_crc_source)(struct drm_crtc *crtc, const char *source, >> size_t *values_cnt); >> + /** >> + * @verify_crc_source: >> + * >> + * verifies the source of CRC checksums of frames before setting the >> + * source for CRC and during crc open. >> + * >> + * This callback is optional if the driver does not support any CRC >> + * generation functionality. >> + * >> + * RETURNS: >> + * >> + * 0 on success or a negative error code on failure. >> + */ >> + int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, >> + size_t *values_cnt); >> >> /** >> * @atomic_print_state: >
Hi Mahesh, On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote: > On 7/10/2018 4:56 PM, Laurent Pinchart wrote: > > On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote: > >> This patch adds a new callback function "verify_crc_source" which will > >> be used during setting the crc source in control node and while opening > >> data node for crc reading. This will help in avoiding setting of wrong > >> string for source. > > > > Why do you need to verify the source in the open() operation ? Isn't it > > enough to verify it in the write() handler ? The answer to this question > > might lie in patch 08/10, in which case I'd add the .verify_crc_source() > > call in open() in that patch, not here. > > Yes, final goal is achieved by patch 08/10. But if crc_read is called > before setting the source, it may have wrong source set in that case I > wanted to return early at least for the drivers implemented > verify_crc_source. If you still think otherwise I will modify > accordingly in next series. I don't disagree with you, but I don't think this issue is new. As I got a bit confused as to why the call is needed in open() (there's no explanation in this patch), I think fixing the problem in 08/10 would be better. > >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > >> Cc: dri-devel@lists.freedesktop.org > >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> > >> drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++ > >> include/drm/drm_crtc.h | 15 +++++++++++++++ > >> 2 files changed, 30 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c > >> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 > >> 100644 > >> --- a/drivers/gpu/drm/drm_debugfs_crc.c > >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c > >> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, > >> const > >> char __user *ubuf, struct drm_crtc *crtc = m->private; > >> > >> struct drm_crtc_crc *crc = &crtc->crc; > >> char *source; > >> > >> + size_t values_cnt; > >> + int ret = 0; > >> > >> if (len == 0) > >> > >> return 0; > >> > >> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, > >> const char __user *ubuf, if (source[len] == '\n') > >> > >> source[len] = '\0'; > >> > >> + if (crtc->funcs->verify_crc_source) { > >> + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> > >> spin_lock_irq(&crc->lock); > >> > >> if (crc->opened) { > >> > >> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct > >> file *filep) return ret; > >> > >> } > >> > >> + if (crtc->funcs->verify_crc_source) { > >> + ret = crtc->funcs->verify_crc_source(crtc, crc->source, > >> + &values_cnt); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> > >> spin_lock_irq(&crc->lock); > >> if (!crc->opened) > >> > >> crc->opened = true; > >> > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> index 23eddbccab10..1a6dcbf91744 100644 > >> --- a/include/drm/drm_crtc.h > >> +++ b/include/drm/drm_crtc.h > >> @@ -661,6 +661,21 @@ struct drm_crtc_funcs { > >> > >> */ > >> > >> int (*set_crc_source)(struct drm_crtc *crtc, const char *source, > >> > >> size_t *values_cnt); > >> > >> + /** > >> + * @verify_crc_source: > >> + * > >> + * verifies the source of CRC checksums of frames before setting the > >> + * source for CRC and during crc open. > >> + * > >> + * This callback is optional if the driver does not support any CRC > >> + * generation functionality. > >> + * > >> + * RETURNS: > >> + * > >> + * 0 on success or a negative error code on failure. > >> + */ > >> + int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, > >> + size_t *values_cnt); > >> > >> /** > >> > >> * @atomic_print_state:
Hi, On 7/10/2018 5:40 PM, Laurent Pinchart wrote: > Hi Mahesh, > > On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote: >> On 7/10/2018 4:56 PM, Laurent Pinchart wrote: >>> On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote: >>>> This patch adds a new callback function "verify_crc_source" which will >>>> be used during setting the crc source in control node and while opening >>>> data node for crc reading. This will help in avoiding setting of wrong >>>> string for source. >>> Why do you need to verify the source in the open() operation ? Isn't it >>> enough to verify it in the write() handler ? The answer to this question >>> might lie in patch 08/10, in which case I'd add the .verify_crc_source() >>> call in open() in that patch, not here. >> Yes, final goal is achieved by patch 08/10. But if crc_read is called >> before setting the source, it may have wrong source set in that case I >> wanted to return early at least for the drivers implemented >> verify_crc_source. If you still think otherwise I will modify >> accordingly in next series. > I don't disagree with you, but I don't think this issue is new. As I got a bit > confused as to why the call is needed in open() (there's no explanation in > this patch), I think fixing the problem in 08/10 would be better. ok sure :) -Mahesh > >>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >>>> Cc: dri-devel@lists.freedesktop.org >>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> >>>> drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++ >>>> include/drm/drm_crtc.h | 15 +++++++++++++++ >>>> 2 files changed, 30 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c >>>> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 >>>> 100644 >>>> --- a/drivers/gpu/drm/drm_debugfs_crc.c >>>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c >>>> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, >>>> const >>>> char __user *ubuf, struct drm_crtc *crtc = m->private; >>>> >>>> struct drm_crtc_crc *crc = &crtc->crc; >>>> char *source; >>>> >>>> + size_t values_cnt; >>>> + int ret = 0; >>>> >>>> if (len == 0) >>>> >>>> return 0; >>>> >>>> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, >>>> const char __user *ubuf, if (source[len] == '\n') >>>> >>>> source[len] = '\0'; >>>> >>>> + if (crtc->funcs->verify_crc_source) { >>>> + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> >>>> spin_lock_irq(&crc->lock); >>>> >>>> if (crc->opened) { >>>> >>>> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct >>>> file *filep) return ret; >>>> >>>> } >>>> >>>> + if (crtc->funcs->verify_crc_source) { >>>> + ret = crtc->funcs->verify_crc_source(crtc, crc->source, >>>> + &values_cnt); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> >>>> spin_lock_irq(&crc->lock); >>>> if (!crc->opened) >>>> >>>> crc->opened = true; >>>> >>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >>>> index 23eddbccab10..1a6dcbf91744 100644 >>>> --- a/include/drm/drm_crtc.h >>>> +++ b/include/drm/drm_crtc.h >>>> @@ -661,6 +661,21 @@ struct drm_crtc_funcs { >>>> >>>> */ >>>> >>>> int (*set_crc_source)(struct drm_crtc *crtc, const char *source, >>>> >>>> size_t *values_cnt); >>>> >>>> + /** >>>> + * @verify_crc_source: >>>> + * >>>> + * verifies the source of CRC checksums of frames before setting the >>>> + * source for CRC and during crc open. >>>> + * >>>> + * This callback is optional if the driver does not support any CRC >>>> + * generation functionality. >>>> + * >>>> + * RETURNS: >>>> + * >>>> + * 0 on success or a negative error code on failure. >>>> + */ >>>> + int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, >>>> + size_t *values_cnt); >>>> >>>> /** >>>> >>>> * @atomic_print_state: >
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf, struct drm_crtc *crtc = m->private; struct drm_crtc_crc *crc = &crtc->crc; char *source; + size_t values_cnt; + int ret = 0; if (len == 0) return 0; @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf, if (source[len] == '\n') source[len] = '\0'; + if (crtc->funcs->verify_crc_source) { + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt); + if (ret) + return ret; + } + spin_lock_irq(&crc->lock); if (crc->opened) { @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) return ret; } + if (crtc->funcs->verify_crc_source) { + ret = crtc->funcs->verify_crc_source(crtc, crc->source, + &values_cnt); + if (ret) + return ret; + } + spin_lock_irq(&crc->lock); if (!crc->opened) crc->opened = true; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..1a6dcbf91744 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -661,6 +661,21 @@ struct drm_crtc_funcs { */ int (*set_crc_source)(struct drm_crtc *crtc, const char *source, size_t *values_cnt); + /** + * @verify_crc_source: + * + * verifies the source of CRC checksums of frames before setting the + * source for CRC and during crc open. + * + * This callback is optional if the driver does not support any CRC + * generation functionality. + * + * RETURNS: + * + * 0 on success or a negative error code on failure. + */ + int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, + size_t *values_cnt); /** * @atomic_print_state: