On 20.11.2018 22:43, Ken Sloat wrote:
> From: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
>
> In some usages isc->raw_fmt will not be initialized. If this
> is the case, it is very possible that a NULL struct de-reference
> will occur, as this member is referenced many times.
Hello Ken,
Do you have any confidence that just by avoiding the NULL situation,
this fix makes things right for adding new sensors that for example, do
not offer a raw format ?
The check that actually sets the raw_fmt comes from an iteration through
the formats, and the one having the RAW flag gets put into this
variable. One could just alter the formats table and get the raw_fmt
that is needed.
My feeling is that the method of adding this variable (raw_fmt) is very
unfortunate, and I did not completely understand the situations where
it's needed.
Look inline about what I mean...
>
> To prevent this, add safety checks for this member and handle
> situations accordingly.
>
> Signed-off-by: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> ---
> drivers/media/platform/atmel/atmel-isc.c | 64 ++++++++++++++++--------
> 1 file changed, 44 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index 50178968b8a6..4cccaa4f2ce9 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -902,6 +902,15 @@ static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
> !isc_fmt->isc_support;
> }
>
> +static inline u32 get_preferred_mbus_code(const struct isc_device *isc,
> + const struct isc_format *isc_fmt)
> +{
> + if (sensor_is_preferred(isc_fmt) || !isc->raw_fmt)
> + return isc_fmt->mbus_code;
For example here, if we do _not_ have a raw format, what makes us
believe that the right format is the one from the mbus_code from the
isc_fmt ? Is there anything useful there at all ?
> + else
> + return isc->raw_fmt->mbus_code;
> +}
> +
> static struct fmt_config *get_fmt_config(u32 fourcc)
> {
> struct fmt_config *config;
> @@ -955,7 +964,7 @@ static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
> {
> struct regmap *regmap = isc->regmap;
> struct isc_ctrls *ctrls = &isc->ctrls;
> - struct fmt_config *config = get_fmt_config(isc->raw_fmt->fourcc);
> + struct fmt_config *config;
> u32 val, bay_cfg;
> const u32 *gamma;
> unsigned int i;
> @@ -969,7 +978,12 @@ static void isc_set_pipeline(struct isc_device *isc, u32 pipeline)
> if (!pipeline)
> return;
>
> - bay_cfg = config->cfa_baycfg;
> + if (isc->raw_fmt) {
> + config = get_fmt_config(isc->raw_fmt->fourcc);
> + bay_cfg = config->cfa_baycfg;
> + } else {
> + bay_cfg = 0;
> + }
Having bay_cfg zero, in the case when we do not have a raw format, is
the real proper way to do this ? it is possible that this bay cfg is
required at a different value, or corresponding to different formats in
the pipeline of the ISC.
>
> regmap_write(regmap, ISC_WB_CFG, bay_cfg);
> regmap_write(regmap, ISC_WB_O_RGR, 0x0);
> @@ -1022,12 +1036,20 @@ static void isc_set_histogram(struct isc_device *isc)
> {
> struct regmap *regmap = isc->regmap;
> struct isc_ctrls *ctrls = &isc->ctrls;
> - struct fmt_config *config = get_fmt_config(isc->raw_fmt->fourcc);
> + struct fmt_config *config;
> + u32 cfa_baycfg;
> +
> + if (isc->raw_fmt) {
> + config = get_fmt_config(isc->raw_fmt->fourcc);
> + cfa_baycfg = config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
> + } else {
> + cfa_baycfg = 0;
> + }
Ditto
>
> if (ctrls->awb && (ctrls->hist_stat != HIST_ENABLED)) {
> regmap_write(regmap, ISC_HIS_CFG,
> ISC_HIS_CFG_MODE_R |
> - (config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT) |
> + cfa_baycfg |
> ISC_HIS_CFG_RAR);
> regmap_write(regmap, ISC_HIS_CTRL, ISC_HIS_CTRL_EN);
> regmap_write(regmap, ISC_INTEN, ISC_INT_HISDONE);
> @@ -1075,7 +1097,7 @@ static int isc_configure(struct isc_device *isc)
> struct regmap *regmap = isc->regmap;
> const struct isc_format *current_fmt = isc->current_fmt;
> struct fmt_config *curfmt_config = get_fmt_config(current_fmt->fourcc);
> - struct fmt_config *rawfmt_config = get_fmt_config(isc->raw_fmt->fourcc);
> + struct fmt_config *rawfmt_config;
> struct isc_subdev_entity *subdev = isc->current_subdev;
> u32 pfe_cfg0, rlp_mode, dcfg, mask, pipeline;
>
> @@ -1085,7 +1107,12 @@ static int isc_configure(struct isc_device *isc)
> isc_get_param(current_fmt, &rlp_mode, &dcfg);
> isc->ctrls.hist_stat = HIST_INIT;
> } else {
> - pfe_cfg0 = rawfmt_config->pfe_cfg0_bps;
> + if (isc->raw_fmt) {
> + rawfmt_config = get_fmt_config(isc->raw_fmt->fourcc);
> + pfe_cfg0 = rawfmt_config->pfe_cfg0_bps;
> + } else {
> + pfe_cfg0 = curfmt_config->pfe_cfg0_bps;
> + }
> pipeline = curfmt_config->bits_pipeline;
> rlp_mode = curfmt_config->rlp_cfg_mode;
> dcfg = curfmt_config->dcfg_imode |
> @@ -1315,10 +1342,7 @@ static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
> if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
>
> - if (sensor_is_preferred(isc_fmt))
> - mbus_code = isc_fmt->mbus_code;
> - else
> - mbus_code = isc->raw_fmt->mbus_code;
> + mbus_code = get_preferred_mbus_code(isc, isc_fmt);
>
> v4l2_fill_mbus_format(&format.format, pixfmt, mbus_code);
> ret = v4l2_subdev_call(isc->current_subdev->sd, pad, set_fmt,
> @@ -1442,10 +1466,7 @@ static int isc_enum_framesizes(struct file *file, void *fh,
> if (!isc_fmt)
> return -EINVAL;
>
> - if (sensor_is_preferred(isc_fmt))
> - fse.code = isc_fmt->mbus_code;
> - else
> - fse.code = isc->raw_fmt->mbus_code;
> + fse.code = get_preferred_mbus_code(isc, isc_fmt);
>
> ret = v4l2_subdev_call(isc->current_subdev->sd, pad, enum_frame_size,
> NULL, &fse);
> @@ -1476,10 +1497,7 @@ static int isc_enum_frameintervals(struct file *file, void *fh,
> if (!isc_fmt)
> return -EINVAL;
>
> - if (sensor_is_preferred(isc_fmt))
> - fie.code = isc_fmt->mbus_code;
> - else
> - fie.code = isc->raw_fmt->mbus_code;
> + fie.code = get_preferred_mbus_code(isc, isc_fmt);
>
> ret = v4l2_subdev_call(isc->current_subdev->sd, pad,
> enum_frame_interval, NULL, &fie);
> @@ -1668,7 +1686,7 @@ static void isc_awb_work(struct work_struct *w)
> struct isc_device *isc =
> container_of(w, struct isc_device, awb_work);
> struct regmap *regmap = isc->regmap;
> - struct fmt_config *config = get_fmt_config(isc->raw_fmt->fourcc);
> + struct fmt_config *config;
> struct isc_ctrls *ctrls = &isc->ctrls;
> u32 hist_id = ctrls->hist_id;
> u32 baysel;
> @@ -1686,7 +1704,13 @@ static void isc_awb_work(struct work_struct *w)
> }
>
> ctrls->hist_id = hist_id;
> - baysel = config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
> +
> + if (isc->raw_fmt) {
> + config = get_fmt_config(isc->raw_fmt->fourcc);
> + baysel = config->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
> + } else {
> + baysel = 0;
> + }
>
> pm_runtime_get_sync(isc->dev);
>
>
So , in short, I am not convinced that this is a proper way to solve it,
so we have to dig in further to see if this is OK or not.
Which sensors do you have and how did you test this, which board and setup?
Thanks for your help,
Eugen
