phy: tegra: xusb: Complete exception handling in five probe functions

phy: tegra: xusb: Complete exception handling in five probe functions

[Markus Elfring]

Hello,

I have taken another look also at the implementations of five probe functions.
I have got the impression then that the exception handling is incomplete so far
at these source code places.
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/phy/tegra/xusb-tegra124.c#L608
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/tegra/xusb-tegra124.c?id€05803a2ca0af49f36f6e9329b5ecda3df27347#n608

How do you think about to move kfree() calls to other jump targets
according to the Linux coding style?

Regards,
Markus

phy: tegra: xusb: Complete exception handling in five probe functions

[Markus Elfring]

Hello,

I have taken another look also at the implementations of five probe functions.
I have got the impression then that the exception handling is incomplete so far
at these source code places.
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/phy/tegra/xusb-tegra124.c#L608
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/tegra/xusb-tegra124.c?id=8005803a2ca0af49f36f6e9329b5ecda3df27347#n608

How do you think about to move kfree() calls to other jump targets
according to the Linux coding style?

Regards,
Markus

Re: [Cocci] phy: tegra: xusb: Complete exception handling in probe functions

[Markus Elfring]

> I have got the impression then that the exception handling is incomplete so far
> at these source code places.

How do you think about to try the following small script for the semantic patch
language out on the directory “drivers/phy/tegra”?

@display@
expression object;
identifier exit;
@@
 ... when any
 object = kzalloc(...)
 ...
 if (...)
 {
*kfree(object);
 goto exit;
 }
 ...
 if (...)
*   goto unregister;


Will any search pattern variations become more interesting for corresponding
automatic software transformations?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: phy: tegra: xusb: Complete exception handling in probe functions

[Markus Elfring]

> I have got the impression then that the exception handling is incomplete so far
> at these source code places.

How do you think about to try the following small script for the semantic patch
language out on the directory “drivers/phy/tegra”?

@display@
expression object;
identifier exit;
@@
 ... when any
 object = kzalloc(...)
 ...
 if (...)
 {
*kfree(object);
 goto exit;
 }
 ...
 if (...)
*   goto unregister;


Will any search pattern variations become more interesting for corresponding
automatic software transformations?

Regards,
Markus

Re: phy: tegra: xusb: Complete exception handling in probe functions

[Markus Elfring]

> I have got the impression then that the exception handling is incomplete so far
> at these source code places.

How do you think about to try the following small script for the semantic patch
language out on the directory “drivers/phy/tegra”?

@display@
expression object;
identifier exit;
@@
 ... when any
 object = kzalloc(...)
 ...
 if (...)
 {
*kfree(object);
 goto exit;
 }
 ...
 if (...)
*   goto unregister;


Will any search pattern variations become more interesting for corresponding
automatic software transformations?

Regards,
Markus

Re: [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?

[Markus Elfring]

> Will any search pattern variations become more interesting for corresponding
> automatic software transformations?

I hoped to achieve something together with the semantic patch language
by the following transformation approach.

@adjustment@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


Do you find such a change suggestion reasonable (in principle)?

But I stumble on another unexpected test result.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions1.cocci
init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
HANDLING: drivers/phy/tegra/xusb-tegra186.c


How would you like to clarify why diff hunks were not generated
for functions like “tegra186_usb2_pad_probe” and “tegra186_usb3_pad_probe”
in such an use case?
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/phy/tegra/xusb-tegra186.c#L445
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/tegra/xusb-tegra186.c?id=bbf711682cd570697086e88388a2c718da918894#n445

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?

[Markus Elfring]

> Will any search pattern variations become more interesting for corresponding
> automatic software transformations?

I hoped to achieve something together with the semantic patch language
by the following transformation approach.

@adjustment@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


Do you find such a change suggestion reasonable (in principle)?

But I stumble on another unexpected test result.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions1.cocci
init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
HANDLING: drivers/phy/tegra/xusb-tegra186.c


How would you like to clarify why diff hunks were not generated
for functions like “tegra186_usb2_pad_probe” and “tegra186_usb3_pad_probe”
in such an use case?
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/phy/tegra/xusb-tegra186.c#L445
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/tegra/xusb-tegra186.c?id=bbf711682cd570697086e88388a2c718da918894#n445

Regards,
Markus

Re: drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?

[Markus Elfring]

> Will any search pattern variations become more interesting for corresponding
> automatic software transformations?

I hoped to achieve something together with the semantic patch language
by the following transformation approach.

@adjustment@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


Do you find such a change suggestion reasonable (in principle)?

But I stumble on another unexpected test result.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions1.cocci
init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
HANDLING: drivers/phy/tegra/xusb-tegra186.c


How would you like to clarify why diff hunks were not generated
for functions like “tegra186_usb2_pad_probe” and “tegra186_usb3_pad_probe”
in such an use case?
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/phy/tegra/xusb-tegra186.c#L445
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/tegra/xusb-tegra186.c?id=bbf711682cd570697086e88388a2c718da918894#n445

Regards,
Markus

Re: [Cocci] xusb-tegra186: Renaming known jump labels with SmPL?

[Markus Elfring]

> But I stumble on another unexpected test result.

I have tried a related tiny transformation approach out.

@renaming@
@@
-out
+exit
 :
 return ERR_PTR(...);


I got a diff hunk like the following then.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/rename_out_label1.cocci
…
warning: line 3: should out be a metavariable?
…
@@ -483,7 +483,7 @@ tegra186_usb2_pad_probe(struct tegra_xus

 unregister:
 	device_unregister(&pad->dev);
-out:
+exit :
 	return ERR_PTR(err);
 }
…


Should the addition of an extra space character usually be avoided
for such a jump label?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?

[Markus Elfring]

> But I stumble on another unexpected test result.

I would like point out that the following simple (?) transformation approach
does not generate the expected diff hunk at the moment.

@addition@
expression object;
@@
 object = kzalloc(...)
 ... when any
 device_unregister(...);
 out:
+kfree(object);
 return ERR_PTR(...);


elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/add_kfree_call1.cocci


Will further collateral evolution happen then also for the Coccinelle software?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?

[Markus Elfring]

> There is no reason why a patch should be generated in this case.
> As you should know well, A ... B only matches in a transformation case
> if every path from A leads to code matching B.  That is not the case in your example.

The exception handling code should usually be executed at the end of
function implementations after an error situation was detected.

Do you try to refer to specific information from the software documentation
like the following?

https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L179
“…
A depends on clause can further indicate whether the clause should be satisfied
for all the branches (forall) or only for one (exists). exists is the default.
…”

The following simple transformation approach seems to work in the way
which I expected somehow initially.

@addition exists@
expression object;
@@
 object = kzalloc(...)
 ... when any
 device_unregister(...);
 out:
+kfree(object);
 return ERR_PTR(...);


Does this change specification indicate then a disagreement about
a default SmPL rule property?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]



On Wed, 30 Oct 2019, Markus Elfring wrote:

> > There is no reason why a patch should be generated in this case.
> > As you should know well, A ... B only matches in a transformation case
> > if every path from A leads to code matching B.  That is not the case in your example.
>
> The exception handling code should usually be executed at the end of
> function implementations after an error situation was detected.
>
> Do you try to refer to specific information from the software documentation
> like the following?
>
> https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L179
> “…
> A depends on clause can further indicate whether the clause should be satisfied
> for all the branches (forall) or only for one (exists). exists is the default.
> …”

There is no depends on in your code.  The cited text is not relevant.

julia

>
> The following simple transformation approach seems to work in the way
> which I expected somehow initially.
>
> @addition exists@
> expression object;
> @@
>  object = kzalloc(...)
>  ... when any
>  device_unregister(...);
>  out:
> +kfree(object);
>  return ERR_PTR(...);
>
>
> Does this change specification indicate then a disagreement about
> a default SmPL rule property?
>
> Regards,
> Markus
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?

[Markus Elfring]

>> @addition@
>> expression object;
>> @@
>>  object = kzalloc(...)
>>  ... when any
>>  device_unregister(...);
>>  out:
>> +kfree(object);
>>  return ERR_PTR(...);
>
> There is no reason why a patch should be generated in this case.

We come along different software development views also for this use case.
I tend still to interpret some of my source code search patterns in other directions.


> As you should know well, A ... B only matches in a transformation case
> if every path from A leads to code matching B.

Now I imagine that an explanation of such information can become more interesting
also based on the applied technologies as a knowledge background.
https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L860

https://en.wikipedia.org/wiki/Computation_tree_logic#Relations_with_other_logics


> That is not the case in your example.

The shown SmPL script can probably indicate that a return statement should occasionally
be executed if a jump to it (according to the label “out” for example) was performed before
(in one if branch).

The following simple transformation approach seems to work also in the way
which I expected somehow initially.

@addition@
expression object;
@@
 object = kzalloc(...)
 ... when exists
 device_unregister(...);
 out:
+kfree(object);
 return ERR_PTR(...);


Can any extensions for the software documentation help to make the application
of the semantic patch language easier and safer finally?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?

[Markus Elfring]

> But I stumble on another unexpected test result.

I got more promising results by the following transformation approach.

@adjustment exists@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


The scope property of such a SmPL rule occasionally needs also more
software development attention.
https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L860

I observe that the pretty-printing for the generated source code will need
further improvements (according to the Linux coding style).

Example:

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions6.cocci
…
@@ -461,10 +461,8 @@ tegra186_usb2_pad_probe(struct tegra_xus
 	pad->soc = soc;

 	err = tegra_xusb_pad_init(pad, padctl, np);
-	if (err < 0) {
-		kfree(usb2);
-		goto out;
-	}
+	if (err < 0)
+		goto release_memory;

 	priv->usb2_trk_clk = devm_clk_get(&pad->dev, "trk");
 	if (IS_ERR(priv->usb2_trk_clk)) {
@@ -483,7 +481,8 @@ tegra186_usb2_pad_probe(struct tegra_xus

 unregister:
 	device_unregister(&pad->dev);
-out:
+release_memory :
+kfree(usb2);
 	return ERR_PTR(err);
 }
…


How reasonable do you find the presented update suggestion?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?

[Markus Elfring]

> But I stumble on another unexpected test result.

I got more promising results by the following transformation approach.

@adjustment exists@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


The scope property of such a SmPL rule occasionally needs also more
software development attention.
https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L860

I observe that the pretty-printing for the generated source code will need
further improvements (according to the Linux coding style).

Example:

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions6.cocci
…
@@ -461,10 +461,8 @@ tegra186_usb2_pad_probe(struct tegra_xus
 	pad->soc = soc;

 	err = tegra_xusb_pad_init(pad, padctl, np);
-	if (err < 0) {
-		kfree(usb2);
-		goto out;
-	}
+	if (err < 0)
+		goto release_memory;

 	priv->usb2_trk_clk = devm_clk_get(&pad->dev, "trk");
 	if (IS_ERR(priv->usb2_trk_clk)) {
@@ -483,7 +481,8 @@ tegra186_usb2_pad_probe(struct tegra_xus

 unregister:
 	device_unregister(&pad->dev);
-out:
+release_memory :
+kfree(usb2);
 	return ERR_PTR(err);
 }
…


How reasonable do you find the presented update suggestion?

Regards,
Markus

Re: drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?

[Markus Elfring]

> But I stumble on another unexpected test result.

I got more promising results by the following transformation approach.

@adjustment exists@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


The scope property of such a SmPL rule occasionally needs also more
software development attention.
https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L860

I observe that the pretty-printing for the generated source code will need
further improvements (according to the Linux coding style).

Example:

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions6.cocci
…
@@ -461,10 +461,8 @@ tegra186_usb2_pad_probe(struct tegra_xus
 	pad->soc = soc;

 	err = tegra_xusb_pad_init(pad, padctl, np);
-	if (err < 0) {
-		kfree(usb2);
-		goto out;
-	}
+	if (err < 0)
+		goto release_memory;

 	priv->usb2_trk_clk = devm_clk_get(&pad->dev, "trk");
 	if (IS_ERR(priv->usb2_trk_clk)) {
@@ -483,7 +481,8 @@ tegra186_usb2_pad_probe(struct tegra_xus

 unregister:
 	device_unregister(&pad->dev);
-out:
+release_memory :
+kfree(usb2);
 	return ERR_PTR(err);
 }
…


How reasonable do you find the presented update suggestion?

Regards,
Markus