| Message ID | 20190322132108.25501-1-sakari.ailus@linux.intel.com |
|---|---|
| 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.
On Fri, Mar 22, 2019 at 2:21 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > %pS and %ps are now the preferred conversion specifiers to print function > %names. The functionality is equivalent; remove the old, deprecated %pF > %and %pf support. Are %pF and %pf really not used any more in the kernel? If that is not the case, you need to convert the remaining users of them to using %ps or %pS before making support for them go away completely. That said, checkpatch can be made treat %pf/F as invalid format right away IMO.
On Mon, Mar 25, 2019 at 10:30 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Mar 22, 2019 at 2:21 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > %pS and %ps are now the preferred conversion specifiers to print function > > %names. The functionality is equivalent; remove the old, deprecated %pF > > %and %pf support. > > Are %pF and %pf really not used any more in the kernel? > > If that is not the case, you need to convert the remaining users of > them to using %ps or %pS before making support for them go away > completely. Well, this is a [2/2] in a series, sorry for the noise (/me blames gmail for the confusion).
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.
Hi all, 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. Sakari Ailus (2): treewide: Switch printk users from %pf and %pF to %ps and %pS, respectively vsprintf: Remove support for %pF and %pf in favour of %pS and %ps Documentation/core-api/printk-formats.rst | 10 ---------- arch/alpha/kernel/pci_iommu.c | 20 ++++++++++---------- arch/arm/mach-imx/pm-imx6.c | 2 +- arch/arm/mm/alignment.c | 2 +- arch/arm/nwfpe/fpmodule.c | 2 +- arch/microblaze/mm/pgtable.c | 2 +- arch/sparc/kernel/ds.c | 2 +- arch/um/kernel/sysrq.c | 2 +- arch/x86/include/asm/trace/exceptions.h | 2 +- arch/x86/kernel/irq_64.c | 2 +- arch/x86/mm/extable.c | 4 ++-- arch/x86/xen/multicalls.c | 2 +- drivers/acpi/device_pm.c | 2 +- drivers/base/power/main.c | 6 +++--- drivers/base/syscore.c | 12 ++++++------ drivers/block/drbd/drbd_receiver.c | 2 +- drivers/block/floppy.c | 10 +++++----- drivers/cpufreq/cpufreq.c | 2 +- drivers/mmc/core/quirks.h | 2 +- drivers/nvdimm/bus.c | 2 +- drivers/nvdimm/dimm_devs.c | 2 +- drivers/pci/pci-driver.c | 14 +++++++------- drivers/pci/quirks.c | 4 ++-- drivers/pnp/quirks.c | 2 +- drivers/scsi/esp_scsi.c | 2 +- fs/btrfs/tests/free-space-tree-tests.c | 4 ++-- fs/f2fs/f2fs.h | 2 +- fs/pstore/inode.c | 2 +- include/trace/events/btrfs.h | 2 +- include/trace/events/cpuhp.h | 4 ++-- include/trace/events/preemptirq.h | 2 +- include/trace/events/rcu.h | 4 ++-- include/trace/events/sunrpc.h | 2 +- include/trace/events/timer.h | 8 ++++---- include/trace/events/vmscan.h | 4 ++-- include/trace/events/workqueue.h | 4 ++-- include/trace/events/xen.h | 2 +- init/main.c | 6 +++--- kernel/async.c | 4 ++-- kernel/events/uprobes.c | 2 +- kernel/fail_function.c | 2 +- kernel/irq/debugfs.c | 2 +- kernel/irq/handle.c | 2 +- kernel/irq/manage.c | 2 +- kernel/irq/spurious.c | 4 ++-- kernel/rcu/tree.c | 2 +- kernel/stop_machine.c | 2 +- kernel/time/sched_clock.c | 2 +- kernel/time/timer.c | 2 +- kernel/workqueue.c | 12 ++++++------ lib/error-inject.c | 2 +- lib/percpu-refcount.c | 4 ++-- lib/vsprintf.c | 8 ++------ mm/memblock.c | 12 ++++++------ mm/memory.c | 2 +- mm/vmscan.c | 2 +- net/ceph/osd_client.c | 2 +- net/core/net-procfs.c | 2 +- net/core/netpoll.c | 4 ++-- scripts/checkpatch.pl | 5 ----- 60 files changed, 111 insertions(+), 130 deletions(-)