Message ID | 20190322132108.25501-1-sakari.ailus@linux.intel.com (mailing list archive) |
---|---|
Headers | show |
Series | Remove support for deprecated %pf and %pF in vsprintf | expand |
Hi Sakari, On Fri, Mar 22, 2019 at 2:25 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > The printk family of functions supports %ps and %pS conversion specifiers > to print function names. Yet the deprecated %pf and %pF conversion > specifiers with equivalent functionality remain supported. A number of > users of %pf and %pF remain. > > This patchsets converts the existing users of %pf and %pF to %ps and %pS, > respectively, and removes support for the deprecated %pf and %pF. > > The patches apply cleanly both on 5.1-rc1 as well as on Linux-next. No new > %pf or %pF users have been added in the meantime so the patch is > sufficient as itself on linux-next, too. Do you know in which commit they became deprecated, so the backporters know how far this can be backported safely? Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On Fri, Mar 22, 2019 at 02:37:18PM +0100, Geert Uytterhoeven wrote: > Hi Sakari, > > On Fri, Mar 22, 2019 at 2:25 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > The printk family of functions supports %ps and %pS conversion specifiers > > to print function names. Yet the deprecated %pf and %pF conversion > > specifiers with equivalent functionality remain supported. A number of > > users of %pf and %pF remain. > > > > This patchsets converts the existing users of %pf and %pF to %ps and %pS, > > respectively, and removes support for the deprecated %pf and %pF. > > > > The patches apply cleanly both on 5.1-rc1 as well as on Linux-next. No new > > %pf or %pF users have been added in the meantime so the patch is > > sufficient as itself on linux-next, too. > > Do you know in which commit they became deprecated, so the backporters > know how far this can be backported safely? That appears to be 04b8eb7a4ccd ("symbol lookup: introduce dereference_symbol_descriptor()"), the same patch that made %p[fF] and %p[sS] functionally equivalent. But my personal opinion would be not to backport the patch for two reasons: the sheer number of files it touches (those format strings change for various reasons) and the meager benefits it has on older kernels as any backported patch using %s or %S still works as such. Porting a patch forward should have no issues either as checkpatch.pl has been complaining of the use of %pf and %pF for a while now.
On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote: > Porting a patch > forward should have no issues either as checkpatch.pl has been complaining > of the use of %pf and %pF for a while now. And that's exactly the reason why I think instead of removing warning on checkpatch, it makes sense to convert to an error for a while. People are tending read documentation on internet and thus might have outdated one. And yes, the compiler doesn't tell a thing about it. P.S. Though, if majority of people will tell that I'm wrong, then it's okay to remove.
Hi Andy, On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote: > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote: > > > Porting a patch > > forward should have no issues either as checkpatch.pl has been complaining > > of the use of %pf and %pF for a while now. > > And that's exactly the reason why I think instead of removing warning on > checkpatch, it makes sense to convert to an error for a while. People are > tending read documentation on internet and thus might have outdated one. And > yes, the compiler doesn't tell a thing about it. > > P.S. Though, if majority of people will tell that I'm wrong, then it's okay to > remove. I wonder if you wrote this before seeing my other patchset. For others as the background, it adds %pfw to print fwnode node names. Assuming this would be merged, %pfw could be in use relatively soon. With the current patchset, %pf prints nothing just as %pO ("F" missing). What I think could be done is to warn of plain %pf (without following "w") in checkpatch.pl, and %pf that is not followed by "w" in the kernel. Although we didn't have such checks to begin with. The case is still a little bit different as %pf used to be a valid conversion specifier whereas %pO likely has never existed. So, how about adding such checks in the other set? I can retain %p[fF] check here, too, if you like.
On Sun, Mar 24, 2019 at 11:10:08PM +0200, Sakari Ailus wrote: > On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote: > > > > > Porting a patch > > > forward should have no issues either as checkpatch.pl has been complaining > > > of the use of %pf and %pF for a while now. > > > > And that's exactly the reason why I think instead of removing warning on > > checkpatch, it makes sense to convert to an error for a while. People are > > tending read documentation on internet and thus might have outdated one. And > > yes, the compiler doesn't tell a thing about it. > > > > P.S. Though, if majority of people will tell that I'm wrong, then it's okay to > > remove. > > I wonder if you wrote this before seeing my other patchset. Yes, I wrote it before seeing another series. > What I think could be done is to warn of plain %pf (without following "w") > in checkpatch.pl, and %pf that is not followed by "w" in the kernel. > Although we didn't have such checks to begin with. The case is still a > little bit different as %pf used to be a valid conversion specifier whereas > %pO likely has never existed. > > So, how about adding such checks in the other set? I can retain %p[fF] check > here, too, if you like. Consistency tells me that the warning->error transformation in checkpatch.pl belongs this series.
Hi Andy, On Sun, Mar 24, 2019 at 11:19:32PM +0200, Andy Shevchenko wrote: > On Sun, Mar 24, 2019 at 11:10:08PM +0200, Sakari Ailus wrote: > > On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote: > > > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote: > > > > > > > Porting a patch > > > > forward should have no issues either as checkpatch.pl has been complaining > > > > of the use of %pf and %pF for a while now. > > > > > > And that's exactly the reason why I think instead of removing warning on > > > checkpatch, it makes sense to convert to an error for a while. People are > > > tending read documentation on internet and thus might have outdated one. And > > > yes, the compiler doesn't tell a thing about it. > > > > > > P.S. Though, if majority of people will tell that I'm wrong, then it's okay to > > > remove. > > > > I wonder if you wrote this before seeing my other patchset. > > Yes, I wrote it before seeing another series. > > > What I think could be done is to warn of plain %pf (without following "w") > > in checkpatch.pl, and %pf that is not followed by "w" in the kernel. > > Although we didn't have such checks to begin with. The case is still a > > little bit different as %pf used to be a valid conversion specifier whereas > > %pO likely has never existed. > > > > So, how about adding such checks in the other set? I can retain %p[fF] check > > here, too, if you like. > > Consistency tells me that the warning->error transformation in checkpatch.pl > belongs this series. All other invalid pointer conversion specifiers currently result into a warning only. I see that as an orthogonal change to this set. I found another issue in checkpatch.pl that may require some discussion; would you be ok with addressing this in another set?
On Mon, Mar 25, 2019 at 05:13:00PM +0200, Sakari Ailus wrote: > All other invalid pointer conversion specifiers currently result into a > warning only. I see that as an orthogonal change to this set. I found > another issue in checkpatch.pl that may require some discussion; would you > be ok with addressing this in another set? If it looks better that way, I have no objection.