From: Alexandru Ardelean <ardeleanalex@gmail.com> To: Julia Lawall <julia.lawall@inria.fr> Cc: Joe Perches <joe@perches.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Robo Bot <apw@canonical.com>, Alexandru Ardelean <alexandru.ardelean@analog.com>, LKML <linux-kernel@vger.kernel.org>, cocci <cocci@systeme.lip6.fr> Subject: Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata() Date: Fri, 20 Nov 2020 13:54:20 +0200 [thread overview] Message-ID: <CA+U=Dsp8Aws7_GARfgNE4w_1pK-hDVW9WVsWHF1TfZUEUo0Hbw@mail.gmail.com> (raw) In-Reply-To: <alpine.DEB.2.22.394.2011201140480.2750@hadrien> On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall <julia.lawall@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 > > > <ardeleanalex@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. > > julia
WARNING: multiple messages have this Message-ID (diff)
From: Alexandru Ardelean <ardeleanalex@gmail.com> To: Julia Lawall <julia.lawall@inria.fr> Cc: LKML <linux-kernel@vger.kernel.org>, Robo Bot <apw@canonical.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Joe Perches <joe@perches.com>, Alexandru Ardelean <alexandru.ardelean@analog.com>, cocci <cocci@systeme.lip6.fr> Subject: Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata() Date: Fri, 20 Nov 2020 13:54:20 +0200 [thread overview] Message-ID: <CA+U=Dsp8Aws7_GARfgNE4w_1pK-hDVW9WVsWHF1TfZUEUo0Hbw@mail.gmail.com> (raw) In-Reply-To: <alpine.DEB.2.22.394.2011201140480.2750@hadrien> On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall <julia.lawall@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 > > > <ardeleanalex@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. > > julia _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
next prev parent reply other threads:[~2020-11-20 11:54 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-19 14:09 Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata() Alexandru Ardelean 2020-11-19 15:16 ` Andy Shevchenko 2020-11-20 0:11 ` Joe Perches 2020-11-20 0:11 ` [Cocci] " Joe Perches 2020-11-20 10:47 ` Julia Lawall 2020-11-20 10:47 ` Julia Lawall 2020-11-20 11:54 ` Alexandru Ardelean [this message] 2020-11-20 11:54 ` Alexandru Ardelean 2020-11-20 11:57 ` Julia Lawall 2020-11-20 11:57 ` Julia Lawall 2020-11-20 11:58 ` Alexandru Ardelean 2020-11-20 11:58 ` Alexandru Ardelean 2020-11-20 13:16 ` Lars-Peter Clausen 2020-11-20 13:16 ` Lars-Peter Clausen 2020-11-20 13:27 ` Alexandru Ardelean 2020-11-20 13:27 ` Alexandru Ardelean
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CA+U=Dsp8Aws7_GARfgNE4w_1pK-hDVW9WVsWHF1TfZUEUo0Hbw@mail.gmail.com' \ --to=ardeleanalex@gmail.com \ --cc=alexandru.ardelean@analog.com \ --cc=andy.shevchenko@gmail.com \ --cc=apw@canonical.com \ --cc=cocci@systeme.lip6.fr \ --cc=joe@perches.com \ --cc=julia.lawall@inria.fr \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.