drm/tve200: Checking for a failed platform_get_irq() call in tve200_probe()

drm/tve200: Checking for a failed platform_get_irq() call in tve200_probe()

[Markus Elfring]

Hello,

I have taken another look at the implementation of the function “tve200_probe”.
A software analysis approach points the following source code out for
further development considerations.
https://elixir.bootlin.com/linux/v5.6.3/source/drivers/gpu/drm/tve200/tve200_drv.c#L212
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/tve200/tve200_drv.c?id=5d30bcacd91af6874481129797af364a53cd9b46#n212

	irq = platform_get_irq(pdev, 0);
	if (!irq) {
		ret = -EINVAL;
		goto clk_disable;
	}


The software documentation is providing the following information
for the used programming interface.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/platform.c?id=5d30bcacd91af6874481129797af364a53cd9b46#n221
https://elixir.bootlin.com/linux/v5.6.3/source/drivers/base/platform.c#L202

“…
 * Return: IRQ number on success, negative error number on failure.
…”

Would you like to reconsider the shown condition check?

Regards,
Markus

Re: drm/tve200: Checking for a failed platform_get_irq() call in tve200_probe()

[Sam Ravnborg]

Hi Markus.

On Thu, Apr 09, 2020 at 03:05:17PM +0200, Markus Elfring wrote:
> Hello,
> 
> I have taken another look at the implementation of the function “tve200_probe”.
> A software analysis approach points the following source code out for
> further development considerations.
> https://elixir.bootlin.com/linux/v5.6.3/source/drivers/gpu/drm/tve200/tve200_drv.c#L212
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/tve200/tve200_drv.c?id=5d30bcacd91af6874481129797af364a53cd9b46#n212
> 
> 	irq = platform_get_irq(pdev, 0);
> 	if (!irq) {
> 		ret = -EINVAL;
> 		goto clk_disable;
> 	}
> 
> 
> The software documentation is providing the following information
> for the used programming interface.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/platform.c?id=5d30bcacd91af6874481129797af364a53cd9b46#n221
> https://elixir.bootlin.com/linux/v5.6.3/source/drivers/base/platform.c#L202
> 
> “…
>  * Return: IRQ number on success, negative error number on failure.
> …”
> 
> Would you like to reconsider the shown condition check?
Thansk for spotting this.

The right way to check for errors is to check if the return value is
less than 0.
Could you please audit all uses of platform_get_irq() in drivers/gpu/
and send a series of patches, one for each driver.

A quick "git grep -C 5 platform_get_irq" identified a few extra drivers
that does not implement the recommend way to check for errors.

Try to be a bit more terse in the changelog - but refer to
the documentation of platform_get_irq():

 * Example:
 *		int irq = platform_get_irq(pdev, 0);
 *		if (irq < 0)
 *			return irq;

Easier to embed it - rather than to link it.
Fine with links in the intro mail.

 	Sam

Re: drm/tve200: Checking for a failed platform_get_irq() call in tve200_probe()

[Markus Elfring]

> The right way to check for errors is to check if the return value is
> less than 0.

Thanks for your constructive feedback.

I was unsure if I noticed another programming mistake.


> Could you please audit all uses of platform_get_irq() in drivers/gpu/

I found 20 source files from the software “Linux next-20200408”
which seem to contain similar update candidates.
Would you like to clarify any extensions for improved applications
of scripts with the semantic patch language (Coccinelle software)
for corresponding analysis and transformation purposes?


> and send a series of patches, one for each driver.

Do other contributors know the affected software module better than me?

Regards,
Markus

Re: drm/tve200: Checking for a failed platform_get_irq() call in tve200_probe()

[Sam Ravnborg]

Hi Markus.

On Fri, Apr 10, 2020 at 12:56:25PM +0200, Markus Elfring wrote:
> > The right way to check for errors is to check if the return value is
> > less than 0.
> 
> Thanks for your constructive feedback.
> 
> I was unsure if I noticed another programming mistake.
> 
> 
> > Could you please audit all uses of platform_get_irq() in drivers/gpu/
> 
> I found 20 source files from the software “Linux next-20200408”
> which seem to contain similar update candidates.
> Would you like to clarify any extensions for improved applications
> of scripts with the semantic patch language (Coccinelle software)
> for corresponding analysis and transformation purposes?
Please just send a series of patches, one for each driver.
Each changelog needs to explain the rationale behind the change.
Look at how this is often done.

As for coccinelle - I cannot help you.

	Sam

Re: drm/tve200: Checking for a failed platform_get_irq() call in tve200_probe()

[Markus Elfring]

>> I found 20 source files from the software “Linux next-20200408”
>> which seem to contain similar update candidates.
>> Would you like to clarify any extensions for improved applications
>> of scripts with the semantic patch language (Coccinelle software)
>> for corresponding analysis and transformation purposes?
> Please just send a series of patches, one for each driver.

I am used to this possibility for years.


> Each changelog needs to explain the rationale behind the change.

I hope to achieve higher confidence also around specific checks
for return values of Linux functions so that unwanted misunderstandings
can be avoided for mentioned implementation details.


> As for coccinelle - I cannot help you.

I might help you more also by the means of this development tool
if related system factors can be improved somehow.

Regards,
Markus