Bug in vpfe_capture.c vpfe_config_image_format

Matthew Schuckmann

I'm working through the vpfe_capture.c driver code and the mt90p031 driver code in the hopes of understanding it so I can implement my own sub device driver for an imager. I'm using the dvsk-4_02_00_06 with the RidgeRun SDK.

I noticed that there is some faulty logic in vpfe_config_image_format() specifically when the sub device doesn't support the g_fmt ioctl (the case for the mt9p031 driver).

The function sets up some defaults for in the vpfe_dev.fmt struct then attempts to get the actual format from the sub device in a temporary structure sd_fmt.
If the g_fmt call fails with ENOICTLCMD (i.e. the sub device doesn't implement it) the function goes ahead and overwrites the defaults with the crap values in the temporary sd_fmt and things are likely to fail further on.

I think the code following code:

	if (ret && ret != -ENOIOCTLCMD) {
		v4l2_err(&vpfe_dev->v4l2_dev,
			"error in getting g_fmt from sub device\n");
		return ret;
	}
	vpfe_dev->fmt = sd_fmt;

Should be changed to

	if (ret && ret != -ENOIOCTLCMD) {
		v4l2_err(&vpfe_dev->v4l2_dev,
			"error in getting g_fmt from sub device\n");
		return ret;
	}

else if( !ret )

	vpfe_dev->fmt = sd_fmt;

So we only fail on a true error and only overwrite the defaults g_fmt really succeeds.

I'm brand new to working with Linux driver code so don't know the proper way to go about submitting this as a bug and a corresponding fix so please advise me on what to do.

Thanks,

Matt Schuckmann

over 13 years ago

0 Matthew Schuckmann over 13 years ago

Intellectual 270 points

After digging even further I'm even more confused the comment on line 530 of vpfe_capture.c states "if sub device supports g_fmt, override the defaults"

However v4l2_device_call_until_err() doesn't tell you if the sub_dev doesn't implement a ioctl.

The macro __v4l2_device_call_subdevs_until_err() (which is what v4l2_device_call_until_err() is based on) returns 0 if the sub device for the group id is not found or the callback for the operation is not set in the sub device operations structure is not set or ENOIOCTLCMD is returned by the callback, or if the callback returns 0, so how are we to know if the sub device supports the operation?

This is completely baffling to me, I must be missing something here, could somebody please help me out. If there is another forum or list I should be posting to please tell me.

Thanks

Matt Schuckmann

0 Vaibhav Hiremath over 13 years ago in reply to Matthew Schuckmann

TI__Expert 8180 points

I don't understand, what is an issue or confusion here. The code snippet clearly means,

Any error except -ENOIOCTLCMD will exit the loop with that error. If no errors (except -ENOIOCTLCMD) occured, then 0 is returned.

Thanks,

Vaibhav

0 Matthew Schuckmann over 13 years ago in reply to Vaibhav Hiremath

Intellectual 270 points

Thanks for responding, I'm new to this kernel driver code stuff so bear with me.

It seems that if a sub device doesn't implement a function you should be able to tell the difference between that and success, it simply baffles the mind considering if success is returned then something should have been done and it should be safe for one to do something with the results (in this case the sd_fmt structure should be filled in) and it's not.

The the logic and comments in vpfe_capture.c vpfe_config_image_format() suggest that the code will be able to tell if the device supports the g_fmt function and it simply can't, in fact the v4l2_device_call_until_err() macro can never return -ENOIOCTLCMD so this suggests that whomever wrote vpfe_capture_image_format() didn't understand what they were doing.

Further more the way the vpfe_config_image_format() is written after the attempt to call g_fmt silently fails (with a sub device like mt9p031) the function assumes it's OK to use the sd_fmt structure and that not the case, it's uninitialized and full of crap.

static int vpfe_config_image_format(struct vpfe_device *vpfe_dev,
				    const v4l2_std_id *std_id)

....

/* if sub device supports g_fmt, override the defaults */
	ret = v4l2_device_call_until_err(&vpfe_dev->v4l2_dev,
	    sdinfo->grp_id, video, g_fmt, &sd_fmt);


	if (ret && ret != -ENOIOCTLCMD) {
		v4l2_err(&vpfe_dev->v4l2_dev,
			"error in getting g_fmt from sub device\n");
		return ret;
	}
	vpfe_dev->fmt = sd_fmt;

....

Processors

Processors forum

Bug in vpfe_capture.c vpfe_config_image_format