> On 21.11.2018 16:50, 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 ?
> >
> > Hi Eugen,
> >
> > Thanks for your comments. The primary goal of my patch is to the solve the
> immediate issue of NULL de-reference of the that struct member. My current
> sensors actually do not offer a RAW format, which is why this bug happens in
> my case (see more details below).
>
> I am not sure if I am correct, but your sensor surely provides data in some
> format. This format might be the 'raw' one that ISC receives. So, adjustments to
> the format list might solve this.
I think this flag is referring specifically to RAW bayer formats, so that the ISC can set registers properly for other modules in the pipeline in those cases - see comments in code for this flag:
/* Indicate a Raw Bayer format */
#define FMT_FLAG_RAW_FORMAT BIT(2)
> >> 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.
> >
> > I agree that the current method of setting a struct member based on a RAW
> flag is flawed and ideally there needs to be a more fundamental change to the
> architecture of the driver so that this situation would never possibly occur,
> however I will present one below that can very likely happen as it does for me:
> >
> >> 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.
> >
> > Right, so in the initial iteration in isc_formats_init() the driver calls the sub-
> device/sensor enum_mbus_code function to step through all its supported
> formats and try and find them in the list of supported ISC formats. If none of
> the formats in the sub-device/sensor are of RAW type, then isc-raw_fmt will
> not be set. This is the fundamental flaw in using this member.
> >
> > Following this, the driver will attempt to set a default format for the ISC in
> isc_set_default_fmt(). This appears to be based on the first format in the list of
> ISC formats. The driver then does a check to see if the sensor is preferred to
> the ISC. If the default format is not supported by the sub-device/sensor, it will
> not be preferred and we will get a resulting crash because it will assume that
> we must use the raw_fmt member that never got set.
>
> I saw this part of the code as well. I was thinking to rewrite it to have it iterate
> through all formats until a suitable one is found (instead of just taking the first
> and win or fail directly).
>
> >
> >>>
> >>> 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 ?
> >
> > It's more of a safe case for where this occurs in my example above. As you
> mentioned yourself, raw_fmt could possibly set to any of the RAW flag formats
> supported by the sub-device. Assuming the sub-device did indeed support a
> RAW format of some sort, but did not necessarily support the current format,
> the driver as of today would be referencing this alternative mbus code
> anyways. In the example above, this occurred while setting the default format,
> and then subsequently will always occur when setting the pipeline in
> isc_set_pipeline() as this function always de-references this member to set the
> pointer even if a RAW format isn't necessarily being used (and so do others as
> seen in my patch).
>
> I tend to disagree, the driver of today will fail if the sensor does not provide a
> RAW format of some sort, so it will not use alternative mbus code.
>
> >
> >>> + 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.
> > I should probably make config point to the current_fmt in the else case here
> so that it uses its bay_cfg, however I believe the WB module would be disabled
> anyways in this case. Regarding if this would be proper or useful, similar
> comments to above.
>
> I am not sure that the current_fmt is the actual format that the sensor is set to
> use, and if this format is OK, however you may be right that the bay_cfg is not
> needed
>
> >
> >> 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
> >
> > My sensor inputs a ITU-R 656 interface to the ISC, so this would be the
> format:
> >
> > {
> > .fourcc = V4L2_PIX_FMT_YUYV,
> > .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > .flags = FMT_FLAG_FROM_CONTROLLER |
> > FMT_FLAG_FROM_SENSOR,
> > .bpp = 16,
> > },
>
> If this format is added with a RAW flag, how does this affect your output ?
Doing this will indeed prevent a crash and allow the sensor to function in my use case.
> > Note that the driver as of today does not support ITU-R 656 without
> modifications but rather ITU-R 601. However, this is as simple as setting some
> additional bits and I plan to submit a separate patch soon that allows this to
> occur from device tree in a standard way.
> > I am using a custom board that is based on the SAMA5D27-SOM1-EK1 board
> so I tested my sensor with this board using gstreamer to direct the image to the
> display. Happy to help out as I am able, let me know what you think.
> Which sensor is it?
> You managed to get it working OK with just this patch?
>
> My general feeling is that this workaround patch will hide the problem, and not
> solve the issue we are having here, we add more code to cope around with this
> raw_fmt NULL issue.
>
> Anyway I am thinking to get more opinions on this issue, about which is the best
> way we can go further with it.
>
> Thanks,
> Eugen
Sounds good Eugen, thanks for your help. Let me know how I can be of assistance going forward.
Thanks,
Ken
