On 11/20/20 12:54 PM, Alexandru Ardelean wrote:
On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall <julia.law...@inria.fr> wrote:
On Thu, 19 Nov 2020, Joe Perches wrote:
On Thu, 2020-11-19 at 17:16 +0200, Andy Shevchenko wrote:
On Thu, Nov 19, 2020 at 4:09 PM Alexandru Ardelean
<ardeleana...@gmail.com> wrote:
Hey,
So, I stumbled on a new check that could be added to checkpatch.
Since it's in Perl, I'm reluctant to try it.
Seems many drivers got to a point where they now call (let's say)
spi_set_drvdata(), but never access that information via
spi_get_drvdata().
Reasons for this seem to be:
1. They got converted to device-managed functions and there is no
longer a remove hook to require the _get_drvdata() access
2. They look like they were copied from a driver that had a
_set_drvdata() and when the code got finalized, the _set_drvdata() was
omitted
There are a few false positives that I can notice at a quick look,
like the data being set via some xxx_set_drvdata() and retrieved via a
dev_get_drvdata().
I can say quite a few. And this makes a difference.
So, basically all drivers that are using PM callbacks would rather use
dev_get_drvdata() rather than bus specific.
I think checkpatch reporting these as well would be acceptable simply
from a reviewability perspective.
I did a shell script to quickly check these. See below.
It's pretty badly written but it is enough for me to gather a list.
And I wrote it in 5 minutes :P
I initially noticed this in some IIO drivers, and then I suspected
that this may be more widespread.
It seems more suitable for coccinelle.
To me as well.
To me as well, since it seems to involve nonlocal information.
I'm not sure to understand the original shell script. Is there
something interesting about pci_set_drvdata?
Ah, it's a stupid script I wrote in 5 minutes, so I did not bother to
make things smart.
In the text-matching I did in shell, there are some entries that come
from comments and docs.
It's only about 3-4 entries, so I just did a visual/manual ignore.
In essence:
The script searches for all strings that contain _set_drvdata.
The separators are whitespace.
It creates a list of all xxxx_set_drvdata functions.
For each xxxx_set_drvdata function:
It checks all files that have a xxxx_set_drvdata entry, but no
xxxx_get_drvdata
I piped this output into a file and started manually checking the drivers.
There is one [I forget which function] that is xxxx_set_drvdata() but
equivalent is xxxx_drvdata()
As Andy said, some precautions must be taken in places where
xxxx_set_drvdata() is called but dev_get_drvdata() is used.
Cases like PM suspend/resume calls.
And there may be some cases outside this context.
Doing something like this with coccinelle is fairly easy.
But I'd be very cautious about putting such a script into the kernel. It
will result in too many false positive drive-by patches. Such a script
will not detect cases such as:
* Driver is split over multiple files. One file does
..._set_drvdata(), another does ..._get_drvdata().
* Framework uses drvdata to exchange data with the driver. E.g driver
is expected to call set_drvdata() and then the framework uses
get_drvdata() to retrieve the data. This is not a very good pattern, but
there are some palces int he kernel where this is used. I believe for
example V4L2 uses this.
- Lars
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci