Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()
On Fri, Nov 20, 2020 at 3:16 PM Lars-Peter Clausen wrote: > > On 11/20/20 12:54 PM, Alexandru Ardelean wrote: > > On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall 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 > 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 _set_drvdata functions. > > For each _set_drvdata function: > > It checks all files that have a _set_drvdata entry, but no > > _get_drvdata > > > > I piped this output into a file and started manually checking the drivers. > > There is one [I forget which function] that is _set_drvdata() but > > equivalent is _drvdata() > > > > As Andy said, some precautions must be taken in places where > > _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: Yeah, it would probably be a good idea to start with a few simple checks, then scale it. If we go for the existing _set_drvdata() / _get_drvdata() pair checks, there is a risk of breaking things. > > * 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 >
Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()
On 11/20/20 12:54 PM, Alexandru Ardelean wrote: On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall 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 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 _set_drvdata functions. For each _set_drvdata function: It checks all files that have a _set_drvdata entry, but no _get_drvdata I piped this output into a file and started manually checking the drivers. There is one [I forget which function] that is _set_drvdata() but equivalent is _drvdata() As Andy said, some precautions must be taken in places where _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
Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()
On Fri, Nov 20, 2020 at 1:57 PM Julia Lawall wrote: > > > > On Fri, 20 Nov 2020, Alexandru Ardelean wrote: > > > On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall 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 > > > > > 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 _set_drvdata functions. > > For each _set_drvdata function: > > It checks all files that have a _set_drvdata entry, but no > > _get_drvdata > > OK, but I have the impression that you want to ignore pci_set_drvdata for > some reason? Or did I misunderstand? Yes. See difficultly visible double quote :P ' "pci_set_drvdata ' Apologies for the confusion if [ "$fn" == '"pci_set_drvdata' ] ; then continue fi > > julia > > > > > I piped this output into a file and started manually checking the drivers. > > There is one [I forget which function] that is _set_drvdata() but > > equivalent is _drvdata() > > > > As Andy said, some precautions must be taken in places where > > _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. > > > > > > > > > > julia > >
Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()
On Fri, 20 Nov 2020, Alexandru Ardelean wrote: > On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall 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 > > > > 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 _set_drvdata functions. > For each _set_drvdata function: > It checks all files that have a _set_drvdata entry, but no > _get_drvdata OK, but I have the impression that you want to ignore pci_set_drvdata for some reason? Or did I misunderstand? julia > > I piped this output into a file and started manually checking the drivers. > There is one [I forget which function] that is _set_drvdata() but > equivalent is _drvdata() > > As Andy said, some precautions must be taken in places where > _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. > > > > > > julia >
Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()
On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall 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 > > > 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 _set_drvdata functions. For each _set_drvdata function: It checks all files that have a _set_drvdata entry, but no _get_drvdata I piped this output into a file and started manually checking the drivers. There is one [I forget which function] that is _set_drvdata() but equivalent is _drvdata() As Andy said, some precautions must be taken in places where _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. > > julia
Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()
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 > > 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? julia
