| Message ID | 20240619212743.3193985-1-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: protect ctl ops calls with validity checks | expand |
On Thu, 20 Jun 2024 at 00:27, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > dpu_encoder_helper_phys_cleanup() calls the ctl ops without checking if > the ops are assigned causing discrepancy between its callers where the > checks are performed and the API itself which does not. > > Two approaches can be taken: either drop the checks even in the caller > OR add the checks even in dpu_encoder_helper_phys_cleanup(). > > Adopt the latter approach as ctl ops are assigned revision based so may not > be always assigned. NAK, these calls are always assigned. Please make sure that they are documented as required and drop offending checks. > > Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/all/464fbd84-0d1c-43c3-a40b-31656ac06456@moroto.mountain/T/ > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 708657598cce..7f7e6d4e974b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -2180,9 +2180,12 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) > if (ctl->ops.reset_intf_cfg) > ctl->ops.reset_intf_cfg(ctl, &intf_cfg); > > - ctl->ops.trigger_flush(ctl); > - ctl->ops.trigger_start(ctl); > - ctl->ops.clear_pending_flush(ctl); > + if (ctl->ops.trigger_flush) > + ctl->ops.trigger_flush(ctl); > + if (ctl->ops.trigger_start) > + ctl->ops.trigger_start(ctl); > + if (ctl->ops.clear_pending_flush) > + ctl->ops.clear_pending_flush(ctl); > } > > void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc, > -- > 2.44.0 >
On Thu, Jun 20, 2024 at 6:08 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 20 Jun 2024 at 00:27, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > dpu_encoder_helper_phys_cleanup() calls the ctl ops without checking if > > the ops are assigned causing discrepancy between its callers where the > > checks are performed and the API itself which does not. > > > > Two approaches can be taken: either drop the checks even in the caller > > OR add the checks even in dpu_encoder_helper_phys_cleanup(). > > > > Adopt the latter approach as ctl ops are assigned revision based so may not > > be always assigned. > > NAK, these calls are always assigned. Please make sure that they are > documented as required and drop offending checks. agreed, I'd rather see the obvious crash if somehow a required callback didn't get set up, than a subtle/silent problem. It is easier to debug that way. BR, -R > > > > Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/all/464fbd84-0d1c-43c3-a40b-31656ac06456@moroto.mountain/T/ > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 708657598cce..7f7e6d4e974b 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -2180,9 +2180,12 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) > > if (ctl->ops.reset_intf_cfg) > > ctl->ops.reset_intf_cfg(ctl, &intf_cfg); > > > > - ctl->ops.trigger_flush(ctl); > > - ctl->ops.trigger_start(ctl); > > - ctl->ops.clear_pending_flush(ctl); > > + if (ctl->ops.trigger_flush) > > + ctl->ops.trigger_flush(ctl); > > + if (ctl->ops.trigger_start) > > + ctl->ops.trigger_start(ctl); > > + if (ctl->ops.clear_pending_flush) > > + ctl->ops.clear_pending_flush(ctl); > > } > > > > void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc, > > -- > > 2.44.0 > > > > > -- > With best wishes > Dmitry
On 6/20/2024 9:41 AM, Rob Clark wrote: > On Thu, Jun 20, 2024 at 6:08 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On Thu, 20 Jun 2024 at 00:27, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>> >>> dpu_encoder_helper_phys_cleanup() calls the ctl ops without checking if >>> the ops are assigned causing discrepancy between its callers where the >>> checks are performed and the API itself which does not. >>> >>> Two approaches can be taken: either drop the checks even in the caller >>> OR add the checks even in dpu_encoder_helper_phys_cleanup(). >>> >>> Adopt the latter approach as ctl ops are assigned revision based so may not >>> be always assigned. >> >> NAK, these calls are always assigned. Please make sure that they are >> documented as required and drop offending checks. > > agreed, I'd rather see the obvious crash if somehow a required > callback didn't get set up, than a subtle/silent problem. It is > easier to debug that way. > > BR, > -R Thank you both for the review. Yes, as I wrote in the commit text, there were two ways to go about it. And looks like the consensus is to go with the other way (drop the checks). I will update the v2 that way and I also update the documentation of the ctl op of interest to this patch that it is always expected to be assigned. > >>> >>> Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback") >>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >>> Closes: https://lore.kernel.org/all/464fbd84-0d1c-43c3-a40b-31656ac06456@moroto.mountain/T/ >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 708657598cce..7f7e6d4e974b 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -2180,9 +2180,12 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) >>> if (ctl->ops.reset_intf_cfg) >>> ctl->ops.reset_intf_cfg(ctl, &intf_cfg); >>> >>> - ctl->ops.trigger_flush(ctl); >>> - ctl->ops.trigger_start(ctl); >>> - ctl->ops.clear_pending_flush(ctl); >>> + if (ctl->ops.trigger_flush) >>> + ctl->ops.trigger_flush(ctl); >>> + if (ctl->ops.trigger_start) >>> + ctl->ops.trigger_start(ctl); >>> + if (ctl->ops.clear_pending_flush) >>> + ctl->ops.clear_pending_flush(ctl); >>> } >>> >>> void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc, >>> -- >>> 2.44.0 >>> >> >> >> -- >> With best wishes >> Dmitry
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 708657598cce..7f7e6d4e974b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -2180,9 +2180,12 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) if (ctl->ops.reset_intf_cfg) ctl->ops.reset_intf_cfg(ctl, &intf_cfg); - ctl->ops.trigger_flush(ctl); - ctl->ops.trigger_start(ctl); - ctl->ops.clear_pending_flush(ctl); + if (ctl->ops.trigger_flush) + ctl->ops.trigger_flush(ctl); + if (ctl->ops.trigger_start) + ctl->ops.trigger_start(ctl); + if (ctl->ops.clear_pending_flush) + ctl->ops.clear_pending_flush(ctl); } void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc,
dpu_encoder_helper_phys_cleanup() calls the ctl ops without checking if the ops are assigned causing discrepancy between its callers where the checks are performed and the API itself which does not. Two approaches can be taken: either drop the checks even in the caller OR add the checks even in dpu_encoder_helper_phys_cleanup(). Adopt the latter approach as ctl ops are assigned revision based so may not be always assigned. Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/all/464fbd84-0d1c-43c3-a40b-31656ac06456@moroto.mountain/T/ Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)